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

[Y.B. Lu]

> -----Original Message-----
> From: linux-mmc-owner@xxxxxxxxxxxxxxx [mailto:linux-mmc-
> owner@xxxxxxxxxxxxxxx] On Behalf Of Scott Wood
> Sent: Tuesday, September 13, 2016 7:25 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 Mon, 2016-09-12 at 06:39 +0000, Y.B. Lu wrote:
> > 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>
> 
> No. ÂThis isn't my patch so my signoff shouldn't be on it.

[Lu Yangbo-B47093] Ok, will remove 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 = 0x85400000,
> 		.mask = 0xfff00000,
> 	},
> 	{
> 		.die = "T1040",
> 		.svr = 0x85200000,
> 		.mask = 0xfff00000,
> 	},
> 	{
> 		.die = "LS1088A",
> 		.svr = 0x87030000,
> 		.mask = 0xffff0000,
> 	},
> 	...
> }
> 
> 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.
> 

[Lu Yangbo-B47093] You mean we will not define soc device attribute for each soc and we will define attribute for each die instead, right?
If so, when we want to match a specific soc we need to use its svr value in code. If it's acceptable, I can try in next version.

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

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

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

> > > > +
> > > > +	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 ? :)
> 
> ret = PTR_ERR(soc_dev);

[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 :)
> 
> If you allocate these with devm_kzalloc(), devm_kasprintf(),
> devm_kstrdup(), etc. then they will be freed automatically when the
> device is unbound.
> 
> >
> > >
> > >
> > > >
> > > > +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.
> 
> That comment is wrong. ÂFreeing the memory first creates a race condition
> that could result in accessing freed memory, if something accesses the
> soc device in parallel with unbinding.
> 

[Lu Yangbo-B47093] Ok, will unregister the device first. Thanks.

> -Scott
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at
> http://vger.kernel.org/majordomo-info.html