> -----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@lists.linux- > 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@lists.linux- 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 ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
