Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms

[Scott Wood]

On Tue, 2016-09-13 at 07:23 +, Y.B. Lu wrote:
> > 


> > 
> > -Original Message-
> > From: linux-mmc-ow...@vger.kernel.org [mailto:linux-mmc-
> > ow...@vger.kernel.org] On Behalf Of Scott Wood
> > Sent: Tuesday, September 13, 2016 7:25 AM
> > To: Y.B. Lu; linux-...@vger.kernel.org; ulf.hans...@linaro.org; Arnd
> > Bergmann
> > Cc: linuxppc-...@lists.ozlabs.org; devicet...@vger.kernel.org; linux-arm-
> > ker...@lists.infradead.org; linux-ker...@vger.kernel.org; linux-
> > c...@vger.kernel.org; linux-...@vger.kernel.org; iommu@lists.linux-
> > foundation.org; net...@vger.kernel.org; Mark Rutland; Rob Herring;
> > Russell King; Jochen Friedrich; Joerg Roedel; Claudiu Manoil; Bhupesh
> > Sharma; Qiang Zhao; Kumar Gala; Santosh Shilimkar; Leo Li; X.B. Xie
> > Subject: Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms
> > 
> > BTW, aren't ls2080a and ls2085a the same die?  And is there no non-E
> > version of LS2080A/LS2040A?
> [Lu Yangbo-B47093] I checked all the svr values in chip errata doc "Revision
> level to part marking cross-reference" table.
> I found ls2080a and ls2085a were in two separate doc. And I didn’t find non-
> E version of LS2080A/LS2040A in chip errata doc.
> Do you know is there any other doc we can confirm this?

No.  Traditionally we've always had E and non-E versions of each chip, but I
have no knowledge of whether that has changed (I do note that the way that E-
status is indicated in SVR has changed).

But please label LS2080A and LS2085A as the same die (or provide strong
evidence that they are not).

> 
> > 
> > 
> > > 
> > > > > 
> > > > > + do {
> > > > > + if (!matches->soc_id)
> > > > > + return NULL;
> > > > > + if (glob_match(svr_match, matches->soc_id))
> > > > > + break;
> > > > > + } while (matches++);
> > > > Are you expecting "matches++" to ever evaluate as false?
> > > [Lu Yangbo-B47093] Yes, this is used to match the soc we use in
> > > qoriq_soc array until getting true.
> > > We need to get the name and die information defined in array.
> > I'm not asking whether the glob_match will ever return true.  I'm saying
> > that "matches++" will never become NULL.
> [Lu Yangbo-B47093] The matches++ will never become NULL while it will return
> NULL after matching for all the members in array.

"matches++" will never "return NULL".  It's just an incrementing address.  It
won't be null until you wrap around the address space, and even if the other
loop terminators never kicked in you'd crash long before that happens.

Please rewrite the loop as something like:

while (matches->soc_id) {
if (glob_match(...))
return matches;

matches++;
}

return NULL;


> > > > > + /* Register soc device */
> > > > > + soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> > > > > + if (!soc_dev_attr) {
> > > > > + ret = -ENOMEM;
> > > > > + goto out_unmap;
> > > > > + }
> > > > Couldn't this be statically allocated?
> > > [Lu Yangbo-B47093] Do you mean we define this struct statically ?
> > > 
> > > static struct soc_device_attribute soc_dev_attr;
> > Yes.
> > 
> [Lu Yangbo-B47093] It's ok to define it statically. Is there any need to do
> that?

It's simpler.

-Scott

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms

[Scott Wood]

On Mon, 2016-09-12 at 06:39 +, Y.B. Lu wrote:
> Hi Scott,
> 
> Thanks for your review :)
> See my comment inline.
> 
> > 
> > -Original Message-
> > From: Scott Wood [mailto:o...@buserror.net]
> > Sent: Friday, September 09, 2016 11:47 AM
> > To: Y.B. Lu; linux-...@vger.kernel.org; ulf.hans...@linaro.org; Arnd
> > Bergmann
> > Cc: linuxppc-...@lists.ozlabs.org; devicet...@vger.kernel.org; linux-arm-
> > ker...@lists.infradead.org; linux-ker...@vger.kernel.org; linux-
> > c...@vger.kernel.org; linux-...@vger.kernel.org; iommu@lists.linux-
> > foundation.org; net...@vger.kernel.org; Mark Rutland; Rob Herring;
> > Russell King; Jochen Friedrich; Joerg Roedel; Claudiu Manoil; Bhupesh
> > Sharma; Qiang Zhao; Kumar Gala; Santosh Shilimkar; Leo Li; X.B. Xie
> > Subject: Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms
> > 
> > On Tue, 2016-09-06 at 16:28 +0800, Yangbo Lu wrote:
> > > 
> > > The global utilities block controls power management, I/O device
> > > enabling, power-onreset(POR) configuration monitoring, alternate
> > > function selection for multiplexed signals,and clock control.
> > > 
> > > This patch adds a driver to manage and access global utilities block.
> > > Initially only reading SVR and registering soc device are supported.
> > > Other guts accesses, such as reading RCW, should eventually be moved
> > > into this driver as well.
> > > 
> > > Signed-off-by: Yangbo Lu <yangbo...@nxp.com>
> > > Signed-off-by: Scott Wood <o...@buserror.net>
> > Don't put my signoff on patches that I didn't put it on
> > myself.  Definitely don't put mine *after* yours on patches that were
> > last modified by you.
> > 
> > If you want to mention that the soc_id encoding was my suggestion, then
> > do so explicitly.
> > 
> [Lu Yangbo-B47093] I found your 'signoff' on this patch at below link.
> http://patchwork.ozlabs.org/patch/649211/
> 
> So, let me just change the order in next version ?
> Signed-off-by: Scott Wood <o...@buserror.net>
> Signed-off-by: Yangbo Lu <yangbo...@nxp.com>

No.  This isn't my patch so my signoff shouldn't be on it.

> [Lu Yangbo-B47093] It's a good idea to move die into .family I think.
> In my opinion, it's better to keep svr and name in soc_id just like your
> suggestion above.
> > 
> > {
> > .soc_id = "svr:0x85490010,name:T1023E,",
> > .family = "QorIQ T1024",
> > }
> The user probably don’t like to learn the svr value. What they want is just
> to match the soc they use.
> It's convenient to use name+rev for them to match a soc.

What the user should want 99% of the time is to match the die (plus revision),
not the soc.

> Regarding shrinking the table, I think it's hard to use svr+mask. Because I
> find many platforms use different masks.
> We couldn’t know the mask according svr value.

The mask would be part of the table:

{
{
.die = "T1024",
.svr = 0x8540,
.mask = 0xfff0,
},
{
.die = "T1040",
.svr = 0x8520,
.mask = 0xfff0,
},
{
.die = "LS1088A",
.svr = 0x8703,
.mask = 0x,
},
...
}

There's a small risk that we get the mask wrong and a different die is created
that matches an existing table, but it doesn't seem too likely, and can easily
be fixed with a kernel update if it happens.

BTW, aren't ls2080a and ls2085a the same die?  And is there no non-E version
of LS2080A/LS2040A?

> > > + do {
> > > + if (!matches->soc_id)
> > > + return NULL;
> > > + if (glob_match(svr_match, matches->soc_id))
> > > + break;
> > > + } while (matches++);
> > Are you expecting "matches++" to ever evaluate as false?
> [Lu Yangbo-B47093] Yes, this is used to match the soc we use in qoriq_soc
> array until getting true. 
> We need to get the name and die information defined in array.

I'm not asking whether the glob_match will ever return true.  I'm saying that
"matches++" will never become NULL.

> > > + /* Register soc device */
> > > + soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> > > + if (!soc_dev_attr) {
> > > + ret = -ENOMEM;
> > > + goto out_unmap;
> > > + }
> > Couldn't this be statically allocated?
> [Lu Yangbo-B47093] Do you mean we define this struct statically ?
> 
>

RE: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms

[Y.B. Lu]

Hi Scott,

Thanks for your review :)
See my comment inline.

> -Original Message-
> From: Scott Wood [mailto:o...@buserror.net]
> Sent: Friday, September 09, 2016 11:47 AM
> To: Y.B. Lu; linux-...@vger.kernel.org; ulf.hans...@linaro.org; Arnd
> Bergmann
> Cc: linuxppc-...@lists.ozlabs.org; devicet...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-ker...@vger.kernel.org; linux-
> c...@vger.kernel.org; linux-...@vger.kernel.org; iommu@lists.linux-
> foundation.org; net...@vger.kernel.org; Mark Rutland; Rob Herring;
> Russell King; Jochen Friedrich; Joerg Roedel; Claudiu Manoil; Bhupesh
> Sharma; Qiang Zhao; Kumar Gala; Santosh Shilimkar; Leo Li; X.B. Xie
> Subject: Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms
> 
> On Tue, 2016-09-06 at 16:28 +0800, Yangbo Lu wrote:
> > The global utilities block controls power management, I/O device
> > enabling, power-onreset(POR) configuration monitoring, alternate
> > function selection for multiplexed signals,and clock control.
> >
> > This patch adds a driver to manage and access global utilities block.
> > Initially only reading SVR and registering soc device are supported.
> > Other guts accesses, such as reading RCW, should eventually be moved
> > into this driver as well.
> >
> > Signed-off-by: Yangbo Lu <yangbo...@nxp.com>
> > Signed-off-by: Scott Wood <o...@buserror.net>
> 
> Don't put my signoff on patches that I didn't put it on
> myself.  Definitely don't put mine *after* yours on patches that were
> last modified by you.
> 
> If you want to mention that the soc_id encoding was my suggestion, then
> do so explicitly.
> 

[Lu Yangbo-B47093] I found your 'signoff' on this patch at below link.
http://patchwork.ozlabs.org/patch/649211/

So, let me just change the order in next version ?
Signed-off-by: Scott Wood <o...@buserror.net>
Signed-off-by: Yangbo Lu <yangbo...@nxp.com>

> > +/* SoC attribute definition for QorIQ platform */ static const struct
> > +soc_device_attribute qoriq_soc[] = { #ifdef CONFIG_PPC
> > +   /*
> > +    * Power Architecture-based SoCs T Series
> > +    */
> > +
> > +   /* SoC: T1024/T1014/T1023/T1013 Rev: 1.0 */
> > +   { .soc_id   = "svr:0x85400010,name:T1024,die:T1024",
> > +     .revision = "1.0",
> > +   },
> > +   { .soc_id   = "svr:0x85480010,name:T1024E,die:T1024",
> > +     .revision = "1.0",
> > +   },
> 
> Revision could be computed from the low 8 bits of SVR (just as you do for
> unknown SVRs).
>
 
[Lu Yangbo-B47093] Yes, you're right. Will remove it here.

> We could move the die name into .family:
> 
>   {
>   .soc_id = "svr:0x85490010,name:T1023E,",
>   .family = "QorIQ T1024",
>   }
> 
> I see you dropped svre (and the trailing comma), though I guess the vast
> majority of potential users will be looking at .family.  In which case do
> we even need name?  If we just make the soc_id be "svr:0x" then
> we could shrink the table to an svr+mask that identifies each die.  I'd
> still want to keep the "svr:" even if we're giving up on the general
> tagging system, to make it clear what the number refers to, and to
> provide some defense against users who match only against soc_id rather
> than soc_id+family.  Or we could go further and format soc_id as "QorIQ
> SVR 0x" so that soc_id-only matches are fully acceptable rather
> than just less dangerous.

[Lu Yangbo-B47093] It's a good idea to move die into .family I think.
In my opinion, it's better to keep svr and name in soc_id just like your 
suggestion above.
>   {
>   .soc_id = "svr:0x85490010,name:T1023E,",
>   .family = "QorIQ T1024",
>   }
The user probably don’t like to learn the svr value. What they want is just to 
match the soc they use.
It's convenient to use name+rev for them to match a soc.

Regarding shrinking the table, I think it's hard to use svr+mask. Because I 
find many platforms use different masks.
We couldn’t know the mask according svr value.

> 
> > +static const struct soc_device_attribute *fsl_soc_device_match(
> > +   unsigned int svr, const struct soc_device_attribute *matches) {
> > +   char svr_match[50];
> > +   int n;
> > +
> > +   n = sprintf(svr_match, "*%08x*", svr);
> 
> n = sprintf(svr_match, "svr:0x%08x,*", svr);
> 
> (according to the current encoding)
> 

[Lu Yangbo-B47093] Ok. Will do that.

> > +
> > +   do {
> > +   if (!matches->soc_id)
> > +   ret

Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms

[Scott Wood]

On Tue, 2016-09-06 at 16:28 +0800, Yangbo Lu wrote:
> The global utilities block controls power management, I/O device
> enabling, power-onreset(POR) configuration monitoring, alternate
> function selection for multiplexed signals,and clock control.
> 
> This patch adds a driver to manage and access global utilities block.
> Initially only reading SVR and registering soc device are supported.
> Other guts accesses, such as reading RCW, should eventually be moved
> into this driver as well.
> 
> Signed-off-by: Yangbo Lu 
> Signed-off-by: Scott Wood 

Don't put my signoff on patches that I didn't put it on myself.  Definitely
don't put mine *after* yours on patches that were last modified by you.

If you want to mention that the soc_id encoding was my suggestion, then do so
explicitly.

> +/* SoC attribute definition for QorIQ platform */
> +static const struct soc_device_attribute qoriq_soc[] = {
> +#ifdef CONFIG_PPC
> + /*
> +  * Power Architecture-based SoCs T Series
> +  */
> +
> + /* SoC: T1024/T1014/T1023/T1013 Rev: 1.0 */
> + { .soc_id   = "svr:0x85400010,name:T1024,die:T1024",
> +   .revision = "1.0",
> + },
> + { .soc_id   = "svr:0x85480010,name:T1024E,die:T1024",
> +   .revision = "1.0",
> + },

Revision could be computed from the low 8 bits of SVR (just as you do for 
unknown SVRs).

We could move the die name into .family:

{
.soc_id = "svr:0x85490010,name:T1023E,",
.family = "QorIQ T1024",
}

I see you dropped svre (and the trailing comma), though I guess the vast
majority of potential users will be looking at .family.  In which case do we
even need name?  If we just make the soc_id be "svr:0x" then we could
shrink the table to an svr+mask that identifies each die.  I'd still want to
keep the "svr:" even if we're giving up on the general tagging system, to make
it clear what the number refers to, and to provide some defense against users
who match only against soc_id rather than soc_id+family.  Or we could go
further and format soc_id as "QorIQ SVR 0x" so that soc_id-only
matches are fully acceptable rather than just less dangerous.

> +static const struct soc_device_attribute *fsl_soc_device_match(
> + unsigned int svr, const struct soc_device_attribute *matches)
> +{
> + char svr_match[50];
> + int n;
> +
> + n = sprintf(svr_match, "*%08x*", svr);

n = sprintf(svr_match, "svr:0x%08x,*", svr);

(according to the current encoding)

> +
> + do {
> + if (!matches->soc_id)
> + return NULL;
> + if (glob_match(svr_match, matches->soc_id))
> + break;
> + } while (matches++);

Are you expecting "matches++" to ever evaluate as false?

> + /* Register soc device */
> + soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> + if (!soc_dev_attr) {
> + ret = -ENOMEM;
> + goto out_unmap;
> + }

Couldn't this be statically allocated?

> +
> + machine = of_flat_dt_get_machine_name();
> + if (machine)
> + soc_dev_attr->machine = kasprintf(GFP_KERNEL, "%s",
> machine);
> +
> + soc_dev_attr->family = kasprintf(GFP_KERNEL, "QorIQ");
> +
> + svr = fsl_guts_get_svr();
> + fsl_soc = fsl_soc_device_match(svr, qoriq_soc);
> + if (fsl_soc) {
> + soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s",
> +  fsl_soc->soc_id);

You can use kstrdup() if you're just copying the string as is.

> + soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%s",
> +    fsl_soc->revision);
> + } else {
> + soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "0x%08x",
> svr);

kasprintf(GFP_KERNEL, "svr:0x%08x,", svr);


> +
> + soc_dev = soc_device_register(soc_dev_attr);
> + if (IS_ERR(soc_dev)) {
> + ret = -ENODEV;

Why are you changing the error code?

> + goto out;
> + } else {

Unnecessary "else".

> + pr_info("Detected: %s\n", soc_dev_attr->machine);

Machine: %s

> + pr_info("Detected SoC family: %s\n", soc_dev_attr->family);
> + pr_info("Detected SoC ID: %s, revision: %s\n",
> + soc_dev_attr->soc_id, soc_dev_attr->revision);

s/Detected //g


> + }
> + return 0;
> +out:
> + kfree(soc_dev_attr->machine);
> + kfree(soc_dev_attr->family);
> + kfree(soc_dev_attr->soc_id);
> + kfree(soc_dev_attr->revision);
> + kfree(soc_dev_attr);
> +out_unmap:
> + iounmap(guts->regs);
> +out_free:
> + kfree(guts);

devm

> +static int fsl_guts_remove(struct platform_device *dev)
> +{
> + kfree(soc_dev_attr->machine);
> + kfree(soc_dev_attr->family);
> + kfree(soc_dev_attr->soc_id);
> + kfree(soc_dev_attr->revision);
> + kfree(soc_dev_attr);
> +