Re: [PATCH v6] platform/x86/intel/pmc: core: Add support to show LTR-ignored components

[David E. Box]

If you can do this now please go ahead. It would be a few weeks before we could
get to it.

David

On Tue, 2023-03-07 at 18:58 +0530, Rajat Khandelwal wrote:
> Hi David,
> 
> On 3/6/2023 11:29 PM, Box, David E wrote:
> > Hi Rajat,
> > 
> > We have some changes coming for this driver in order to support multiple PMC
> > devices on the same SoC. As part of this we are modifying the output of
> > ltr_show
> > to prefix the IP with number of the managing PMC. For example:
> > 
> > PMC[0]:SOUTHPORT_A      LTR: RAW: 0x0   Non-Snoop(ns): 0        Snoop(ns): 0
> > PMC[0]:SOUTHPORT_B      LTR: RAW: 0x0   Non-Snoop(ns): 0        Snoop(ns): 0
> > ...
> > PMC[1]:SOUTHPORT_A      LTR: RAW: 0x0   Non-Snoop(ns): 0        Snoop(ns): 0
> > PMC[1]:SOUTHPORT_B      LTR: RAW: 0x0   Non-Snoop(ns): 0        Snoop(ns): 0
> > 
> > We're thinking that it would be better to have a column in ltr_show for the
> > ignore status. This would make everything viewable in one display. A list
> > would
> > not be needed. The status can be read back from the ltr_ignore register.
> > Would
> > that work for you? Thanks.
> 
> Aah, ok. That is doable.
> Is the plan to incorporate the ignore column certain? Further, is there an ETA
> for the
> said changes in the upstream?
> 
> Thanks
> Rajat
> 
> > 
> > David
> > 
> > On Wed, 2023-03-01 at 21:27 +0530, Rajat Khandelwal wrote:
> > > Currently, 'ltr_ignore' sysfs attribute, when read, returns nothing, even
> > > if there are components whose LTR values have been ignored.
> > > 
> > > Make the sysfs attribute print out such components, if they exist, and
> > > return EEXIST, if tried to set an already ignored component.
> > > 
> > > Signed-off-by: Rajat Khandelwal <rajat.khandelwal@xxxxxxxxxxxxxxx>
> > > ---
> > > 
> > > v6: Description written in imperative sense
> > > 
> > > v5:
> > > 1. Ignore the LTR of the respective component after unlocking the mutex
> > > lock
> > > 2. Adding error code details to the commit message
> > > 
> > > v4: Mutex unlock during error conditions
> > > 
> > > v3: Incorporated a mutex lock for accessing 'ltr_ignore_list'
> > > 
> > > v2: kmalloc -> devm_kmalloc
> > > 
> > >   drivers/platform/x86/intel/pmc/core.c | 64 ++++++++++++++++++++++-----
> > >   drivers/platform/x86/intel/pmc/core.h |  2 +-
> > >   2 files changed, 53 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/intel/pmc/core.c
> > > b/drivers/platform/x86/intel/pmc/core.c
> > > index 3a15d32d7644..16cf6c634db8 100644
> > > --- a/drivers/platform/x86/intel/pmc/core.c
> > > +++ b/drivers/platform/x86/intel/pmc/core.c
> > > @@ -53,6 +53,17 @@ const struct pmc_bit_map msr_map[] = {
> > >          {}
> > >   };
> > >   
> > > +/* Mutual exclusion to access the list of LTR-ignored components */
> > > +static DEFINE_MUTEX(ltr_entry_mutex);
> > > +
> > > +struct ltr_entry {
> > > +       u32 comp_index;
> > > +       const char *comp_name;
> > > +       struct list_head node;
> > > +};
> > > +
> > > +static LIST_HEAD(ltr_ignore_list);
> > > +
> > >   static inline u32 pmc_core_reg_read(struct pmc_dev *pmcdev, int
> > > reg_offset)
> > >   {
> > >          return readl(pmcdev->regbase + reg_offset);
> > > @@ -435,27 +446,18 @@ static int pmc_core_pll_show(struct seq_file *s,
> > > void
> > > *unused)
> > >   }
> > >   DEFINE_SHOW_ATTRIBUTE(pmc_core_pll);
> > >   
> > > -int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value)
> > > +void pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value)
> > >   {
> > >          const struct pmc_reg_map *map = pmcdev->map;
> > >          u32 reg;
> > > -       int err = 0;
> > >   
> > >          mutex_lock(&pmcdev->lock);
> > >   
> > > -       if (value > map->ltr_ignore_max) {
> > > -               err = -EINVAL;
> > > -               goto out_unlock;
> > > -       }
> > > -
> > >          reg = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset);
> > >          reg |= BIT(value);
> > >          pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, reg);
> > >   
> > > -out_unlock:
> > >          mutex_unlock(&pmcdev->lock);
> > > -
> > > -       return err;
> > >   }
> > >   
> > >   static ssize_t pmc_core_ltr_ignore_write(struct file *file,
> > > @@ -464,6 +466,8 @@ static ssize_t pmc_core_ltr_ignore_write(struct file
> > > *file,
> > >   {
> > >          struct seq_file *s = file->private_data;
> > >          struct pmc_dev *pmcdev = s->private;
> > > +       const struct pmc_reg_map *map = pmcdev->map;
> > > +       struct ltr_entry *entry;
> > >          u32 buf_size, value;
> > >          int err;
> > >   
> > > @@ -473,13 +477,49 @@ static ssize_t pmc_core_ltr_ignore_write(struct file
> > > *file,
> > >          if (err)
> > >                  return err;
> > >   
> > > -       err = pmc_core_send_ltr_ignore(pmcdev, value);
> > > +       if (value > map->ltr_ignore_max)
> > > +               return -EINVAL;
> > > +
> > > +       mutex_lock(&ltr_entry_mutex);
> > > +
> > > +       list_for_each_entry(entry, &ltr_ignore_list, node) {
> > > +               if (entry->comp_index == value) {
> > > +                       err = -EEXIST;
> > > +                       goto out_unlock;
> > > +               }
> > > +       }
> > > +
> > > +       entry = devm_kmalloc(&pmcdev->pdev->dev, sizeof(*entry),
> > > GFP_KERNEL);
> > > +       if (!entry) {
> > > +               err = -ENOMEM;
> > > +               goto out_unlock;
> > > +       }
> > > +
> > > +       entry->comp_name = map->ltr_show_sts[value].name;
> > > +       entry->comp_index = value;
> > > +       list_add_tail(&entry->node, &ltr_ignore_list);
> > > +
> > > +out_unlock:
> > > +       mutex_unlock(&ltr_entry_mutex);
> > > +
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       pmc_core_send_ltr_ignore(pmcdev, value);
> > >   
> > > -       return err == 0 ? count : err;
> > > +       return count;
> > >   }
> > >   
> > >   static int pmc_core_ltr_ignore_show(struct seq_file *s, void *unused)
> > >   {
> > > +       struct ltr_entry *entry;
> > > +
> > > +       mutex_lock(&ltr_entry_mutex);
> > > +       list_for_each_entry(entry, &ltr_ignore_list, node) {
> > > +               seq_printf(s, "%s\n", entry->comp_name);
> > > +       }
> > > +       mutex_unlock(&ltr_entry_mutex);
> > > +
> > >          return 0;
> > >   }
> > >   
> > > diff --git a/drivers/platform/x86/intel/pmc/core.h
> > > b/drivers/platform/x86/intel/pmc/core.h
> > > index 810204d758ab..da35b0fcbe6e 100644
> > > --- a/drivers/platform/x86/intel/pmc/core.h
> > > +++ b/drivers/platform/x86/intel/pmc/core.h
> > > @@ -396,7 +396,7 @@ extern const struct pmc_reg_map adl_reg_map;
> > >   extern const struct pmc_reg_map mtl_reg_map;
> > >   
> > >   extern void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev);
> > > -extern int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value);
> > > +extern void pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value);
> > >   
> > >   void spt_core_init(struct pmc_dev *pmcdev);
> > >   void cnp_core_init(struct pmc_dev *pmcdev);