From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <1464832042.22191.23.camel@buserror.net> From: Scott Wood To: Arnd Bergmann , linuxppc-dev@lists.ozlabs.org Cc: Ulf Hansson , Yangbo Lu , Mark Rutland , Xiaobo Xie , "linux-i2c@vger.kernel.org" , "linux-clk@vger.kernel.org" , Qiang Zhao , Russell King , Bhupesh Sharma , Joerg Roedel , Claudiu Manoil , "devicetree@vger.kernel.org" , Rob Herring , Santosh Shilimkar , "linux-arm-kernel@lists.infradead.org" , "netdev@vger.kernel.org" , "linux-mmc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Yang-Leo Li , "iommu@lists.linux-foundation.org" , Kumar Gala , linuxppc-dev@lists.ozlabs.org Date: Wed, 01 Jun 2016 20:47:22 -0500 In-Reply-To: <5339448.D8rpq4ofAC@wuerfel> References: <1462417950-46796-1-git-send-email-yangbo.lu@nxp.com> <5339448.D8rpq4ofAC@wuerfel> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Subject: Re: [PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms Sender: linux-kernel-owner@vger.kernel.org List-ID: 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? > + bool little_endian; > + struct soc_device_attribute soc; > +}; > + > +static u32 fsl_guts_get_svr(struct guts *guts) > +{ > + if (guts->little_endian) > + return ioread32(guts->regs + GUTS_SVR); > + else > + return ioread32be(guts->regs + GUTS_SVR); > +} > + > +static u32 fsl_guts_get_pvr(struct guts *guts) > +{ > + if (guts->little_endian) > + return ioread32(guts->regs + GUTS_PVR); > + else > + return ioread32be(guts->regs + GUTS_PVR); > +} You've removed the fallback to mfspr() on PPC, which would be helpful in some virtualized environments where we don't have the guts node (but do have other directly assigned devices). Of course, this is a consequence of the conversion into a platform device. > + > +/* > + * 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). > + /* > + * 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/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. > + > + 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. -Scott