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

From: Y.B. Lu
Date: Mon Sep 12 2016 - 02:55:53 EST


Hi Scott,

Thanks for your review :)
See my comment inline.

> -----Original Message-----
> From: Scott Wood [mailto:oss@xxxxxxxxxxxx]
> Sent: Friday, September 09, 2016 11:47 AM
> To: Y.B. Lu; linux-mmc@xxxxxxxxxxxxxxx; ulf.hansson@xxxxxxxxxx; Arnd
> Bergmann
> Cc: linuxppc-dev@xxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> clk@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; iommu@xxxxxxxxxxxx
> foundation.org; netdev@xxxxxxxxxxxxxxx; 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.lu@xxxxxxx>
> > Signed-off-by: Scott Wood <oss@xxxxxxxxxxxx>
>
> 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 <oss@xxxxxxxxxxxx>
Signed-off-by: Yangbo Lu <yangbo.lu@xxxxxxx>

> > +/* 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:0xnnnnnnnn" 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 0xnnnnnnnn" 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)
> > + 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.

>
> > + /* 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;

>
> > +
> > + 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.

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

>
> > + 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);

[Lu Yangbo-B47093] Sorry, will add that.

>
>
> > +
> > + soc_dev = soc_device_register(soc_dev_attr);
> > + if (IS_ERR(soc_dev)) {
> > + ret = -ENODEV;
>
> Why are you changing the error code?

[Lu Yangbo-B47093] What error code should we use ? :)

>
> > + goto out;
> > + } else {
>
> Unnecessary "else".

[Lu Yangbo-B47093] Oh.. Correct!

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

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

>
> > + 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

[Lu Yangbo-B47093] Ok, will do that.

>
>
> > + }
> > + 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

[Lu Yangbo-B47093] What's the devm meaning here :)

>
> > +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);
> > + soc_device_unregister(soc_dev);
> > + iounmap(guts->regs);
> > + kfree(guts);
> > + return 0;
> > +}
>
> Don't free the memory before you unregister the device that uses it (moot
> if you use devm).

[Lu Yangbo-B47093] The soc.c driver mentions that.
Ensure soc_dev->attr is freed prior to calling soc_device_unregister.

>
> >
> > +#ifdef CONFIG_FSL_GUTS
> > +unsigned int fsl_guts_get_svr(void);
> > +#endif
>
> Don't ifdef prototypes (unless you're going to provide a stub
> alternative).

[Lu Yangbo-B47093] Ok, will remove ifdef.

>
> -Scott