From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932621AbcILGzr (ORCPT ); Mon, 12 Sep 2016 02:55:47 -0400 Received: from mail-db5eur01on0043.outbound.protection.outlook.com ([104.47.2.43]:13570 "EHLO EUR01-DB5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932501AbcILGzn (ORCPT ); Mon, 12 Sep 2016 02:55:43 -0400 From: "Y.B. Lu" To: Scott Wood , "linux-mmc@vger.kernel.org" , "ulf.hansson@linaro.org" , Arnd Bergmann CC: "linuxppc-dev@lists.ozlabs.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "linux-clk@vger.kernel.org" , "linux-i2c@vger.kernel.org" , "iommu@lists.linux-foundation.org" , "netdev@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 Thread-Topic: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms Thread-Index: AQHSCBpjBHLZUqXlBE6VUHktl62PQ6BwiYYAgAS2mcA= Date: Mon, 12 Sep 2016 06:39:45 +0000 Message-ID: References: <1473150503-9550-1-git-send-email-yangbo.lu@nxp.com> <1473150503-9550-6-git-send-email-yangbo.lu@nxp.com> <1473392840.30217.170.camel@buserror.net> In-Reply-To: <1473392840.30217.170.camel@buserror.net> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: buserror.net; dkim=none (message not signed) header.d=none;buserror.net; dmarc=none action=none header.from=nxp.com; x-originating-ip: [199.59.230.102] x-ms-office365-filtering-correlation-id: a1b0b515-8403-4af7-4b99-08d3dad795ae x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:HE1PR04MB1194; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(72170088055959)(9452136761055)(65623756079841)(185117386973197)(258649278758335); x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(6040176)(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046)(6055026);SRVR:HE1PR04MB1194;BCL:0;PCL:0;RULEID:;SRVR:HE1PR04MB1194; x-forefront-prvs: 006339698F x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(6009001)(7916002)(377454003)(13464003)(24454002)(377424004)(7696004)(11100500001)(122556002)(5002640100001)(189998001)(77096005)(81166006)(33656002)(66066001)(8676002)(15975445007)(7416002)(5660300001)(87386001)(76576001)(87936001)(9686002)(7846002)(8936002)(305945005)(5001770100001)(4001520100001)(74316002)(586003)(7736002)(3660700001)(2906002)(76176999)(2501003)(102836003)(50986999)(6116002)(3280700002)(3846002)(54356999)(4326007)(86362001)(2900100001)(2950100001)(19580405001)(92566002)(106116001)(2201001)(19580395003)(10400500002)(5001760100003);DIR:OUT;SFP:1101;SCL:1;SRVR:HE1PR04MB1194;H:HE1PR04MB0889.eurprd04.prod.outlook.com;FPR:;SPF:None;LANG:en; Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Sep 2016 06:39:45.0401 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR04MB1194 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u8C6tsS2010521 Hi Scott, Thanks for your review :) See my comment inline. > -----Original Message----- > From: Scott Wood [mailto:oss@buserror.net] > Sent: Friday, September 09, 2016 11:47 AM > To: Y.B. Lu; linux-mmc@vger.kernel.org; ulf.hansson@linaro.org; Arnd > Bergmann > Cc: linuxppc-dev@lists.ozlabs.org; devicetree@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux- > clk@vger.kernel.org; linux-i2c@vger.kernel.org; iommu@lists.linux- > foundation.org; netdev@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 > > 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. > [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 Signed-off-by: Yangbo Lu > > +/* 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