From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751272AbcFWCqS (ORCPT ); Wed, 22 Jun 2016 22:46:18 -0400 Received: from mail-am1on0086.outbound.protection.outlook.com ([157.56.112.86]:52088 "EHLO emea01-am1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750721AbcFWCqO (ORCPT ); Wed, 22 Jun 2016 22:46:14 -0400 From: Yangbo Lu To: Scott Wood , Arnd Bergmann , "linuxppc-dev@lists.ozlabs.org" CC: Mark Rutland , Ulf Hansson , "linux-kernel@vger.kernel.org" , "linux-i2c@vger.kernel.org" , "linux-clk@vger.kernel.org" , Qiang Zhao , Russell King , Bhupesh Sharma , Joerg Roedel , "Claudiu Manoil" , "devicetree@vger.kernel.org" , Kumar Gala , Rob Herring , Santosh Shilimkar , "linux-arm-kernel@lists.infradead.org" , "netdev@vger.kernel.org" , "linux-mmc@vger.kernel.org" , Xiaobo Xie , Yang-Leo Li , "iommu@lists.linux-foundation.org" Subject: RE: [PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms Thread-Topic: [PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms Thread-Index: AQHRunVLXff/i1tiD02C+hOioe5D5J/VbH4AgAB0K4CADbHMgIAS6tuw Date: Thu, 23 Jun 2016 02:46:10 +0000 Message-ID: References: <1462417950-46796-1-git-send-email-yangbo.lu@nxp.com> <5339448.D8rpq4ofAC@wuerfel> <1464832042.22191.23.camel@buserror.net> <3435609.s5m1DCsoNb@wuerfel> <1465609853.22191.151.camel@buserror.net> In-Reply-To: <1465609853.22191.151.camel@buserror.net> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=yangbo.lu@nxp.com; x-originating-ip: [199.59.230.102] x-ms-office365-filtering-correlation-id: 65cd7760-b4bb-419c-5590-08d39b1088bf x-microsoft-exchange-diagnostics: 1;VI1PR04MB1200;6:MqUgfxHFKihleo6eszMtCb5CDWvb0mh1vnVJn9rB2ltV8TaHf+pePfXnnw4JvP2Dw24e8x2hE4/D5HwB3feJYrlgiSSCBhzT5F0mshtzsEXjDSkyRgKV70TYoVwgrrzqfPqkXuZWknh/W8O1I/CxTYpoJqzkFAWSbubE9TOZK53dVc/V6d0I3eSurbi1/gE0K3w9O1jiVonB0w0KfMw2zNx9V8Xvu2JRGlquKKFqTovY6lpct+gU3Leyxf06GCNdcVD2TbxB8mvXhGpi0MjerHx9nbA8N2fhUMFtQCGRRfsqsaFIHa73Mz+SfW9YxLSH6P3WfCV90fDkzMJp83dg4uGy5XUr0TCN1gDxJYgLn8Q=;5:HAnzetFt9j365Vvn36Iwd7rTKNhqcw8uJM1wBKu1BW+2DdRoDHyuXVfB9uRnqW4RqYKkQpF0UPO8BRLru8U7YdCN9LCSn/jmd6xSRJj+Nk61S7Hzdrpnq/zQ6EBqS404ZzJcOckggk2HKt6yuZ4aqQ==;24:6h+J1eo5xWfu+2myNG72Sif/y85iNIrivkHuikbGcZVlRfp6hABE/1Z2b+GnY80amQ8d+0rfx5iKrmYbeUQwrnALgIwEtF5+gTyl12z3q/w=;7:J2I6gSdvA8+T2Mhst8rdJ54kThWyGAyufoecccNgfb0vT4lqcBoOUAIPkO7qtkv5EH73j7hhEvdAHrpekTu1M4bbywLJJK2FpAdK8CYagDivagVASiLJNSAgKc4VfDucec7xvQJWtGReF9rpG2bM0Un7fBCCrBjlDliD5W9h99RFJW/pPKwv8N3rZJLOW7F1pkEJwxEJaiBvsrYugIluJQw+7SFAJdB6DqAr2mGb4sfxy8CxS6imx7RhHTtbzB0EkFV5D9zC4ceN5rYwfo0vMw== x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:VI1PR04MB1200; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(9452136761055)(65623756079841)(258649278758335); x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001)(6055026);SRVR:VI1PR04MB1200;BCL:0;PCL:0;RULEID:;SRVR:VI1PR04MB1200; x-forefront-prvs: 098291215C x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(6009001)(7916002)(189002)(13464003)(24454002)(377454003)(199003)(377424004)(19580395003)(6116002)(19580405001)(102836003)(3846002)(8676002)(586003)(101416001)(66066001)(97736004)(68736007)(8666005)(7846002)(3660700001)(54356999)(9686002)(2906002)(76176999)(50986999)(5001770100001)(3280700002)(10400500002)(105586002)(2900100001)(2950100001)(575784001)(86362001)(74316001)(92566002)(87936001)(77096005)(305945005)(106356001)(33656002)(106116001)(7696003)(7736002)(8936002)(5003600100003)(5002640100001)(93886004)(81156014)(81166006)(189998001)(2501003)(122556002)(76576001)(4326007)(7059030)(2004002);DIR:OUT;SFP:1101;SCL:1;SRVR:VI1PR04MB1200;H:AM2PR04MB0883.eurprd04.prod.outlook.com;FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Jun 2016 02:46:10.4183 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB1200 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 u5N2kcR9015765 Hi Arnd, Could you comment on these? Thanks. Best regards, Yangbo Lu > -----Original Message----- > From: Scott Wood [mailto:oss@buserror.net] > Sent: Saturday, June 11, 2016 9:51 AM > To: Arnd Bergmann; linuxppc-dev@lists.ozlabs.org > Cc: Mark Rutland; Ulf Hansson; linux-kernel@vger.kernel.org; linux- > i2c@vger.kernel.org; linux-clk@vger.kernel.org; Qiang Zhao; Russell King; > Bhupesh Sharma; Joerg Roedel; Claudiu Manoil; devicetree@vger.kernel.org; > Kumar Gala; Rob Herring; Santosh Shilimkar; linux-arm- > kernel@lists.infradead.org; netdev@vger.kernel.org; linux- > mmc@vger.kernel.org; Xiaobo Xie; Yang-Leo Li; iommu@lists.linux- > foundation.org; Yangbo Lu > Subject: Re: [PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms > > On Thu, 2016-06-02 at 10:43 +0200, Arnd Bergmann wrote: > > On Wednesday, June 1, 2016 8:47:22 PM CEST Scott Wood wrote: > > > On Mon, 2016-05-30 at 15:15 +0200, Arnd Bergmann wrote: > > > > diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c new > > > > file mode 100644 index 000000000000..2f30698f5bcf > > > > --- /dev/null > > > > +++ b/drivers/soc/fsl/guts.c > > > > @@ -0,0 +1,130 @@ > > > > +/* > > > > + * Freescale QorIQ Platforms GUTS Driver > > > > + * > > > > + * Copyright (C) 2016 Freescale Semiconductor, Inc. > > > > + * > > > > + * This program is free software; you can redistribute it and/or > > > > +modify > > > > + * it under the terms of the GNU General Public License as > > > > +published by > > > > + * the Free Software Foundation; either version 2 of the License, > > > > +or > > > > + * (at your option) any later version. > > > > + */ > > > > + > > > > +#include > > > > +#include #include > > > > +#include #include #include > > > > + #include > > > > + > > > > +#define GUTS_PVR 0x0a0 > > > > +#define GUTS_SVR 0x0a4 > > > > + > > > > +struct guts { > > > > + void __iomem *regs; > > > > > > We already have a struct to define guts. Why are you not using it? > > > Why do you consider using it to be "abuse"? What if we want to move > > > more guts functionality into this driver? > > > > This structure was in the original patch, I left it in there, only > > removed the inclusion of the powerpc header file, which seemed to be > > misplaced. > > I'm not refering "struct guts". I'm referring to changing "struct > ccsr_guts __iomem *regs" into "void __iomem *regs". > > And it's not a powerpc header file. > > > > > +/* > > > > + * Table for matching compatible strings, for device tree > > > > + * guts node, for Freescale QorIQ SOCs. > > > > + */ > > > > +static const struct of_device_id fsl_guts_of_match[] = { > > > > + /* For T4 & B4 Series SOCs */ > > > > + { .compatible = "fsl,qoriq-device-config-1.0", .data = "T4/B4 > > > > series" }, > > > [snip] > > > > + { .compatible = "fsl,qoriq-device-config-2.0", .data = "P > > > > series" > > > > > > As noted in my comment on patch 3/4, these descriptions are reversed. > > > > > > They're also incomplete. t2080 has device config 2.0. t1040 is > > > described as > > > 2.0 though it should probably be 2.1 (or better, drop the generic > > > compatible altogether). > > > > Ok. Ideally I think we'd even look up the specific SoC names from the > > SVC rather than the compatible string. I just didn't have a good list > > for those to put in the driver. > > The list is in arch/powerpc/include/asm/mpc85xx.h but I don't know why we > need to convert it to a string in the first place. > > > > > > > + /* > > > > + * syscon devices default to little-endian, but on powerpc we > > > > have > > > > + * existing device trees with big-endian maps and an absent > > > > endianess > > > > + * "big-property" > > > > + */ > > > > + if (!IS_ENABLED(CONFIG_POWERPC) && > > > > + !of_property_read_bool(dev->of_node, "big-endian")) > > > > + guts->little_endian = true; > > > > > > This is not a syscon device (Yangbo's patch to add a guts node on > > > ls2080 is the only guts node that says "syscon", and that was a > > > leftover from earlier revisions and should probably be removed). > > > Even if it were, where is it documented that syscon defaults to > > > little-endian? > > > > Documentation/devicetree/bindings/regmap/regmap.txt > > > > We had a little screwup here, basically regmap (and by consequence, > > syscon) always defaulted to little-endian way before that was > > documented, so it's too late to change it, > > What causes a device node to fall under the jurisdiction of regmap.txt? > Again, these nodes do not claim "syscon" compatibility. > > > although I agree it would have made sense to document regmap to > > default to big-endian on powerpc. > > Please don't. It's enough of a mess as is; no need to start throwing in > architecture ifdefs. > > > > Documentation/devicetree/bindings/common-properties.txt says that > > > the individual binding specifies the default. The default for this > > > node should be big-endian because that's what existed before there > > > was a need to describe the endianness. And we need an update to the > > > guts binding to specify that. > > > > Good point. This proably means that specifying both the "guts" and > "syscon" > > compatible strings implies having to also specify the endianess > > explicitly both ways, because otherwise we break one of the two > bindings. > > Yes, but the node should only specify "guts". > > > > > > > + > > > > + guts->regs = devm_ioremap_resource(dev, 0); > > > > + if (!guts->regs) { > > > > + ret = -ENOMEM; > > > > + kfree(guts); > > > > + goto out; > > > > + } > > > > + > > > > + fsl_guts_init(dev, guts); > > > > + ret = 0; > > > > +out: > > > > + return ret; > > > > +} > > > > + > > > > +static struct platform_driver fsl_soc_guts = { > > > > + .probe = fsl_guts_probe, > > > > + .driver.of_match_table = fsl_guts_of_match, }; > > > > + > > > > +module_platform_driver(fsl_soc_guts); > > > > > > Again, this means that the information is not available during early > > > boot, such as in the clock driver. Thus we would not be able to > > > convert clk -qoriq's direct mfspr(SPRN_SVR) into an > > > soc_device_match() (or anything else that makes use of this file), > > > nor would we be able to move its access of the guts RCW registers > > > into this driver. > > > > Correct. Do we have a reason to convert the mfspr() though? I don't > > really see an improvement over the current state if we do that, > > Then should we drop this patchset and put a similar PPC ifdef in > drivers/mmc/host/sdhci-of-esdhc.c? > > There's also the RCW access. You said in the patch 4/4 discussion that > you di dn't like any random driver ioremapping the registers... > > > and for new devices > > that might need the erratum workaround, we could add a DT property > > that would be preferred to both. > > It's unlikely that we would know the erratum exists at the time the > device tree is created. We also generally don't have separate device > trees for each revision of a chip (and if we did, we'd have users that > use the wrong one). > > -Scott