Re: [PATCH 2/2] soc: ti: k3-socinfo: Add support for AM62P variants via NVMEM

[Judith Mendez]

Hi Andrew,

On 2/4/26 3:54 PM, Andrew Davis wrote:
> On 2/4/26 3:37 PM, Judith Mendez wrote:
> > Add support for detecting AM62P silicon revisions.
> >
> > On AM62P, silicon revision is discovered with GP_SW1 register instead
> > of JTAGID register. Use the NVMEM framework to read GP_SW1 from the
> > gpsw-efuse nvmem provider to determine SoC revision.
> >
> > Signed-off-by: Judith Mendez <jm@xxxxxx>
> > ---
> >   drivers/soc/ti/k3-socinfo.c | 48 ++++++++++++++++++++++++++++++++++---
> >   1 file changed, 45 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
> > index 42275cb5ba1c8..4b6947a9ceb4d 100644
> > --- a/drivers/soc/ti/k3-socinfo.c
> > +++ b/drivers/soc/ti/k3-socinfo.c
> > @@ -6,6 +6,7 @@
> >    */
> >   #include <linux/mfd/syscon.h>
> > +#include <linux/nvmem-consumer.h>
> >   #include <linux/of.h>
> >   #include <linux/of_address.h>
> >   #include <linux/regmap.h>
> > @@ -25,6 +26,9 @@
> >   #define CTRLMMR_WKUP_JTAGID_VARIANT_SHIFT    (28)
> >   #define CTRLMMR_WKUP_JTAGID_VARIANT_MASK    GENMASK(31, 28)
> > +#define GP_SW1_VALID_BIT            BIT(4)
> > +#define GP_SW1_ADR_MASK            GENMASK(3, 0)
> > +
> >   #define CTRLMMR_WKUP_JTAGID_PARTNO_SHIFT    (12)
> >   #define CTRLMMR_WKUP_JTAGID_PARTNO_MASK        GENMASK(27, 12)
> > @@ -70,6 +74,29 @@ static const char * const am62lx_rev_string_map[] = {
> >       "1.0", "1.1",
> >   };
> > +static const char * const am62p_gpsw_rev_string_map[] = {
> > +    "1.0", "1.1", "1.2",
> > +};
> > +
> > +static int
> > +k3_chipinfo_get_gpsw_variant(struct platform_device *pdev)
> > +{
> > +    struct device *dev = &pdev->dev;
> > +    u32 gpsw_val, adr_val = 0;
> > +    int ret;
> > +
> > +    ret = nvmem_cell_read_u32(dev, "gpsw1", &gpsw_val);
> > +    if (ret)
> > +        return ret;
> > +
> > +    if (!(gpsw_val & GP_SW1_VALID_BIT))
> > +        return 0;
>
> Return -1 here so you will get the warning message about setting default 
> SR1.0.
>
> > +
> > +    adr_val = gpsw_val & GP_SW1_ADR_MASK;
> > +
> > +    return adr_val;
>
> Merge the above two lines,
>
> return gpsw_val & GP_SW1_ADR_MASK;
>
> Or maybe try using FIELD_GET() or similar if you are feeling fancy.
>
> > +}
> > +
> >   static int
> >   k3_chipinfo_partno_to_names(unsigned int partno,
> >                   struct soc_device_attribute *soc_dev_attr)
> > @@ -86,9 +113,11 @@ k3_chipinfo_partno_to_names(unsigned int partno,
> >   }
> >   static int
> > -k3_chipinfo_variant_to_sr(unsigned int partno, unsigned int variant,
> > -              struct soc_device_attribute *soc_dev_attr)
> > +k3_chipinfo_variant_to_sr(struct platform_device *pdev, unsigned int 
> > partno,
>
> You pass in the platform_device struct pointer, but only ever use the
> ->dev member, just pass in the "dev" device pointer.
>
> Otherwise LGTM
>
> Andrew

Thanks for reviewing!
Will fix according to your review and respin the series.

~ Judith