| Message ID | 1479099731-28108-2-git-send-email-pankaj.dubey@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Pankaj, On Mon, 14 Nov 2016 10:31:56 +0530 Pankaj Dubey wrote: > Many platforms are duplicating code for enabling SCU, lets add > common code to enable SCU by parsing SCU device node so the duplication > in each platform can be avoided. > > CC: Krzysztof Kozlowski <krzk@kernel.org> > CC: Jisheng Zhang <jszhang@marvell.com> > CC: Russell King <linux@armlinux.org.uk> > CC: Dinh Nguyen <dinguyen@opensource.altera.com> > CC: Patrice Chotard <patrice.chotard@st.com> > CC: Linus Walleij <linus.walleij@linaro.org> > CC: Liviu Dudau <liviu.dudau@arm.com> > CC: Ray Jui <rjui@broadcom.com> > CC: Stephen Warren <swarren@wwwdotorg.org> > CC: Heiko Stuebner <heiko@sntech.de> > CC: Shawn Guo <shawnguo@kernel.org> > CC: Michal Simek <michal.simek@xilinx.com> > CC: Wei Xu <xuwei5@hisilicon.com> > CC: Andrew Lunn <andrew@lunn.ch> > CC: Jun Nie <jun.nie@linaro.org> > Suggested-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > --- > arch/arm/include/asm/smp_scu.h | 4 +++ > arch/arm/kernel/smp_scu.c | 56 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 60 insertions(+) > > diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h > index bfe163c..fdeec07 100644 > --- a/arch/arm/include/asm/smp_scu.h > +++ b/arch/arm/include/asm/smp_scu.h > @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, unsigned int mode) > > #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU) > void scu_enable(void __iomem *scu_base); > +void __iomem *of_scu_get_base(void); > +int of_scu_enable(void); > #else > static inline void scu_enable(void __iomem *scu_base) {} > +static inline void __iomem *of_scu_get_base(void) {return NULL; } > +static inline int of_scu_enable(void) {return 0; } > #endif > > #endif > diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c > index 72f9241..d0ac3ed 100644 > --- a/arch/arm/kernel/smp_scu.c > +++ b/arch/arm/kernel/smp_scu.c > @@ -10,6 +10,7 @@ > */ > #include <linux/init.h> > #include <linux/io.h> > +#include <linux/of_address.h> > > #include <asm/smp_plat.h> > #include <asm/smp_scu.h> > @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base) > */ > flush_cache_all(); > } > + > +static const struct of_device_id scu_match[] = { > + { .compatible = "arm,cortex-a9-scu", }, > + { .compatible = "arm,cortex-a5-scu", }, > + { } > +}; > + > +/* > + * Helper API to get SCU base address > + * In case platform DT do not have SCU node, or iomap fails > + * this call will fallback and will try to map via call to > + * scu_a9_get_base. > + * This will return ownership of scu_base to the caller > + */ > +void __iomem *of_scu_get_base(void) > +{ > + unsigned long base = 0; > + struct device_node *np; > + void __iomem *scu_base; > + > + np = of_find_matching_node(NULL, scu_match); could we check np before calling of_iomap()? > + scu_base = of_iomap(np, 0); > + of_node_put(np); > + if (!scu_base) { > + pr_err("%s failed to map scu_base via DT\n", __func__); For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand what does it mean, but it may confuse normal users. In current version, berlin doesn't complain like this for non-ca9 SoCs > + if (scu_a9_has_base()) { > + base = scu_a9_get_base(); > + scu_base = ioremap(base, SZ_4K); > + } > + if (!scu_base) { > + pr_err("%s failed to map scu_base\n", __func__); ditto > + return IOMEM_ERR_PTR(-ENOMEM); > + } > + } > + return scu_base; > +} > + > +/* > + * Enable SCU via mapping scu_base DT > + * If scu_base mapped successfully scu will be enabled and in case of > + * failure if will return non-zero error code > + */ > +int of_scu_enable(void) > +{ > + void __iomem *scu_base; > + > + scu_base = of_scu_get_base(); > + if (!IS_ERR(scu_base)) { > + scu_enable(scu_base); > + iounmap(scu_base); > + return 0; > + } > + return PTR_ERR(scu_base); > +} > + > #endif > > /*
Hi Jisheng, On Monday 14 November 2016 11:42 AM, Jisheng Zhang wrote: > Hi Pankaj, > > On Mon, 14 Nov 2016 10:31:56 +0530 Pankaj Dubey wrote: <snip> >> + >> + np = of_find_matching_node(NULL, scu_match); > > could we check np before calling of_iomap()? > of_iomap takes care of that, and will return NULL if np is NULL. So additional check of np is not required here. >> + scu_base = of_iomap(np, 0); >> + of_node_put(np); >> + if (!scu_base) { >> + pr_err("%s failed to map scu_base via DT\n", __func__); > > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand > what does it mean, but it may confuse normal users. In current version, > berlin doesn't complain like this for non-ca9 SoCs > OK, let me see other reviewer's comment on this. Then we will decide if this error message is required or can be omitted. Thanks, Pankaj Dubey
On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote: > >> + scu_base = of_iomap(np, 0); > >> + of_node_put(np); > >> + if (!scu_base) { > >> + pr_err("%s failed to map scu_base via DT\n", __func__); > > > > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand > > what does it mean, but it may confuse normal users. In current version, > > berlin doesn't complain like this for non-ca9 SoCs > > > > OK, let me see other reviewer's comment on this. Then we will decide if > this error message is required or can be omitted. We need to look at all callers here, to see if the function ever gets called for a CPU that doesn't have an SCU. I'd say we should warn if we know there is an SCU but we cannot map it, but never warn on any of the CPU cores that don't support an SCU. Arnd
This should be sent _to_ me because it's touching generic ARM code. Thanks. On Mon, Nov 14, 2016 at 10:31:56AM +0530, Pankaj Dubey wrote: > Many platforms are duplicating code for enabling SCU, lets add > common code to enable SCU by parsing SCU device node so the duplication > in each platform can be avoided. > > CC: Krzysztof Kozlowski <krzk@kernel.org> > CC: Jisheng Zhang <jszhang@marvell.com> > CC: Russell King <linux@armlinux.org.uk> > CC: Dinh Nguyen <dinguyen@opensource.altera.com> > CC: Patrice Chotard <patrice.chotard@st.com> > CC: Linus Walleij <linus.walleij@linaro.org> > CC: Liviu Dudau <liviu.dudau@arm.com> > CC: Ray Jui <rjui@broadcom.com> > CC: Stephen Warren <swarren@wwwdotorg.org> > CC: Heiko Stuebner <heiko@sntech.de> > CC: Shawn Guo <shawnguo@kernel.org> > CC: Michal Simek <michal.simek@xilinx.com> > CC: Wei Xu <xuwei5@hisilicon.com> > CC: Andrew Lunn <andrew@lunn.ch> > CC: Jun Nie <jun.nie@linaro.org> > Suggested-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > --- > arch/arm/include/asm/smp_scu.h | 4 +++ > arch/arm/kernel/smp_scu.c | 56 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 60 insertions(+) > > diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h > index bfe163c..fdeec07 100644 > --- a/arch/arm/include/asm/smp_scu.h > +++ b/arch/arm/include/asm/smp_scu.h > @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, unsigned int mode) > > #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU) > void scu_enable(void __iomem *scu_base); > +void __iomem *of_scu_get_base(void); > +int of_scu_enable(void); > #else > static inline void scu_enable(void __iomem *scu_base) {} > +static inline void __iomem *of_scu_get_base(void) {return NULL; } > +static inline int of_scu_enable(void) {return 0; } > #endif > > #endif > diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c > index 72f9241..d0ac3ed 100644 > --- a/arch/arm/kernel/smp_scu.c > +++ b/arch/arm/kernel/smp_scu.c > @@ -10,6 +10,7 @@ > */ > #include <linux/init.h> > #include <linux/io.h> > +#include <linux/of_address.h> > > #include <asm/smp_plat.h> > #include <asm/smp_scu.h> > @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base) > */ > flush_cache_all(); > } > + > +static const struct of_device_id scu_match[] = { > + { .compatible = "arm,cortex-a9-scu", }, > + { .compatible = "arm,cortex-a5-scu", }, > + { } > +}; > + > +/* > + * Helper API to get SCU base address > + * In case platform DT do not have SCU node, or iomap fails > + * this call will fallback and will try to map via call to > + * scu_a9_get_base. > + * This will return ownership of scu_base to the caller > + */ > +void __iomem *of_scu_get_base(void) > +{ > + unsigned long base = 0; > + struct device_node *np; > + void __iomem *scu_base; > + > + np = of_find_matching_node(NULL, scu_match); > + scu_base = of_iomap(np, 0); > + of_node_put(np); > + if (!scu_base) { > + pr_err("%s failed to map scu_base via DT\n", __func__); > + if (scu_a9_has_base()) { > + base = scu_a9_get_base(); > + scu_base = ioremap(base, SZ_4K); > + } > + if (!scu_base) { > + pr_err("%s failed to map scu_base\n", __func__); > + return IOMEM_ERR_PTR(-ENOMEM); I can't see the point of using error-pointers here - it's not like we really know why we're failing, so just return NULL. > + } > + } > + return scu_base; > +} > + > +/* > + * Enable SCU via mapping scu_base DT > + * If scu_base mapped successfully scu will be enabled and in case of > + * failure if will return non-zero error code > + */ > +int of_scu_enable(void) > +{ > + void __iomem *scu_base; > + > + scu_base = of_scu_get_base(); > + if (!IS_ERR(scu_base)) { > + scu_enable(scu_base); > + iounmap(scu_base); > + return 0; > + } > + return PTR_ERR(scu_base); and just return your -ENOMEM here.
On Mon, Nov 14, 2016 at 01:03:09PM +0100, Arnd Bergmann wrote: > On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote: > > >> + scu_base = of_iomap(np, 0); > > >> + of_node_put(np); > > >> + if (!scu_base) { > > >> + pr_err("%s failed to map scu_base via DT\n", __func__); > > > > > > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand > > > what does it mean, but it may confuse normal users. In current version, > > > berlin doesn't complain like this for non-ca9 SoCs > > > > > > > OK, let me see other reviewer's comment on this. Then we will decide if > > this error message is required or can be omitted. > > We need to look at all callers here, to see if the function ever gets > called for a CPU that doesn't have an SCU. I'd say we should warn if > we know there is an SCU but we cannot map it, but never warn on > any of the CPU cores that don't support an SCU. Maybe there should be two helpers: of_scu_enable() which _only_ looks up the SCU address in DT and enables it if it finds it, otherwise returning failure. a9_scu_enable() which tries to use the A9 provided SCU address and enables it if it finds it, otherwise returning failure. Then callers can decide which of these to call, and what error messages to print on their failures.
On Monday, November 14, 2016 1:50:18 PM CET Russell King - ARM Linux wrote: > On Mon, Nov 14, 2016 at 01:03:09PM +0100, Arnd Bergmann wrote: > > On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote: > > > >> + scu_base = of_iomap(np, 0); > > > >> + of_node_put(np); > > > >> + if (!scu_base) { > > > >> + pr_err("%s failed to map scu_base via DT\n", __func__); > > > > > > > > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand > > > > what does it mean, but it may confuse normal users. In current version, > > > > berlin doesn't complain like this for non-ca9 SoCs > > > > > > > > > > OK, let me see other reviewer's comment on this. Then we will decide if > > > this error message is required or can be omitted. > > > > We need to look at all callers here, to see if the function ever gets > > called for a CPU that doesn't have an SCU. I'd say we should warn if > > we know there is an SCU but we cannot map it, but never warn on > > any of the CPU cores that don't support an SCU. > > Maybe there should be two helpers: > > of_scu_enable() which _only_ looks up the SCU address in DT and enables > it if it finds it, otherwise returning failure. > > a9_scu_enable() which tries to use the A9 provided SCU address and > enables it if it finds it, otherwise returning failure. > > Then callers can decide which of these to call, and what error messages > to print on their failures. Splitting the function in two is probably simpler overall, but we may still have to look at all the callers: Any platform that currently tries to map it on any CPU and doesn't warn about the absence of the device node (or about scu_a9_has_base() == false) should really continue not to warn about that. If all platforms only call these on SMP machines with an ARM11MPcore, Cortex-A5 or Cortex-A9, everything should be fine here, otherwise we can leave the warning in the caller after checking the return code of the new APIs. Arnd
On Mon, Nov 14, 2016 at 03:37:44PM +0100, Arnd Bergmann wrote: > On Monday, November 14, 2016 1:50:18 PM CET Russell King - ARM Linux wrote: > > On Mon, Nov 14, 2016 at 01:03:09PM +0100, Arnd Bergmann wrote: > > > On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote: > > > > >> + scu_base = of_iomap(np, 0); > > > > >> + of_node_put(np); > > > > >> + if (!scu_base) { > > > > >> + pr_err("%s failed to map scu_base via DT\n", __func__); > > > > > > > > > > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand > > > > > what does it mean, but it may confuse normal users. In current version, > > > > > berlin doesn't complain like this for non-ca9 SoCs > > > > > > > > > > > > > OK, let me see other reviewer's comment on this. Then we will decide if > > > > this error message is required or can be omitted. > > > > > > We need to look at all callers here, to see if the function ever gets > > > called for a CPU that doesn't have an SCU. I'd say we should warn if > > > we know there is an SCU but we cannot map it, but never warn on > > > any of the CPU cores that don't support an SCU. > > > > Maybe there should be two helpers: > > > > of_scu_enable() which _only_ looks up the SCU address in DT and enables > > it if it finds it, otherwise returning failure. > > > > a9_scu_enable() which tries to use the A9 provided SCU address and > > enables it if it finds it, otherwise returning failure. > > > > Then callers can decide which of these to call, and what error messages > > to print on their failures. > > Splitting the function in two is probably simpler overall, but > we may still have to look at all the callers: Any platform that > currently tries to map it on any CPU and doesn't warn about the > absence of the device node (or about scu_a9_has_base() == false) > should really continue not to warn about that. Did you miss the bit where none of of_scu_enable() or a9_scu_enable() should produce any warnings or errors to be printed. It's up to the caller to report the failure, otherwise doing this doesn't make sense: if (of_scu_enable() < 0 && a9_scu_enable() < 0) pr_err("Failed to map and enable the SCU\n"); because if of_scu_enable() prints a warning/error, then it's patently misleading.
Hi Russell, On Monday 14 November 2016 07:18 PM, Russell King - ARM Linux wrote: > This should be sent _to_ me because it's touching generic ARM code. > Thanks. > Sorry for this. I had included your email in CC for this patch, but looks like my email client had some issue and this patch could not reach to your mailbox. I will take care in future. > On Mon, Nov 14, 2016 at 10:31:56AM +0530, Pankaj Dubey wrote: >> Many platforms are duplicating code for enabling SCU, lets add >> common code to enable SCU by parsing SCU device node so the duplication >> in each platform can be avoided. >> >> CC: Krzysztof Kozlowski <krzk@kernel.org> >> CC: Jisheng Zhang <jszhang@marvell.com> >> CC: Russell King <linux@armlinux.org.uk> >> CC: Dinh Nguyen <dinguyen@opensource.altera.com> >> CC: Patrice Chotard <patrice.chotard@st.com> >> CC: Linus Walleij <linus.walleij@linaro.org> >> CC: Liviu Dudau <liviu.dudau@arm.com> >> CC: Ray Jui <rjui@broadcom.com> >> CC: Stephen Warren <swarren@wwwdotorg.org> >> CC: Heiko Stuebner <heiko@sntech.de> >> CC: Shawn Guo <shawnguo@kernel.org> >> CC: Michal Simek <michal.simek@xilinx.com> >> CC: Wei Xu <xuwei5@hisilicon.com> >> CC: Andrew Lunn <andrew@lunn.ch> >> CC: Jun Nie <jun.nie@linaro.org> >> Suggested-by: Arnd Bergmann <arnd@arndb.de> >> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> >> --- >> arch/arm/include/asm/smp_scu.h | 4 +++ >> arch/arm/kernel/smp_scu.c | 56 ++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 60 insertions(+) >> >> diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h >> index bfe163c..fdeec07 100644 >> --- a/arch/arm/include/asm/smp_scu.h >> +++ b/arch/arm/include/asm/smp_scu.h >> @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, unsigned int mode) >> >> #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU) >> void scu_enable(void __iomem *scu_base); >> +void __iomem *of_scu_get_base(void); >> +int of_scu_enable(void); >> #else >> static inline void scu_enable(void __iomem *scu_base) {} >> +static inline void __iomem *of_scu_get_base(void) {return NULL; } >> +static inline int of_scu_enable(void) {return 0; } >> #endif >> >> #endif >> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c >> index 72f9241..d0ac3ed 100644 >> --- a/arch/arm/kernel/smp_scu.c >> +++ b/arch/arm/kernel/smp_scu.c >> @@ -10,6 +10,7 @@ >> */ >> #include <linux/init.h> >> #include <linux/io.h> >> +#include <linux/of_address.h> >> >> #include <asm/smp_plat.h> >> #include <asm/smp_scu.h> >> @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base) >> */ >> flush_cache_all(); >> } >> + >> +static const struct of_device_id scu_match[] = { >> + { .compatible = "arm,cortex-a9-scu", }, >> + { .compatible = "arm,cortex-a5-scu", }, >> + { } >> +}; >> + >> +/* >> + * Helper API to get SCU base address >> + * In case platform DT do not have SCU node, or iomap fails >> + * this call will fallback and will try to map via call to >> + * scu_a9_get_base. >> + * This will return ownership of scu_base to the caller >> + */ >> +void __iomem *of_scu_get_base(void) >> +{ >> + unsigned long base = 0; >> + struct device_node *np; >> + void __iomem *scu_base; >> + >> + np = of_find_matching_node(NULL, scu_match); >> + scu_base = of_iomap(np, 0); >> + of_node_put(np); >> + if (!scu_base) { >> + pr_err("%s failed to map scu_base via DT\n", __func__); >> + if (scu_a9_has_base()) { >> + base = scu_a9_get_base(); >> + scu_base = ioremap(base, SZ_4K); >> + } >> + if (!scu_base) { >> + pr_err("%s failed to map scu_base\n", __func__); >> + return IOMEM_ERR_PTR(-ENOMEM); > > I can't see the point of using error-pointers here - it's not like we > really know why we're failing, so just return NULL. > >> + } >> + } >> + return scu_base; >> +} >> + >> +/* >> + * Enable SCU via mapping scu_base DT >> + * If scu_base mapped successfully scu will be enabled and in case of >> + * failure if will return non-zero error code >> + */ >> +int of_scu_enable(void) >> +{ >> + void __iomem *scu_base; >> + >> + scu_base = of_scu_get_base(); >> + if (!IS_ERR(scu_base)) { >> + scu_enable(scu_base); >> + iounmap(scu_base); >> + return 0; >> + } >> + return PTR_ERR(scu_base); > > and just return your -ENOMEM here. >
Hi Russell, On Monday 14 November 2016 08:21 PM, Russell King - ARM Linux wrote: > On Mon, Nov 14, 2016 at 03:37:44PM +0100, Arnd Bergmann wrote: >> On Monday, November 14, 2016 1:50:18 PM CET Russell King - ARM Linux wrote: >>> On Mon, Nov 14, 2016 at 01:03:09PM +0100, Arnd Bergmann wrote: >>>> On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote: >>>>>>> + scu_base = of_iomap(np, 0); >>>>>>> + of_node_put(np); >>>>>>> + if (!scu_base) { >>>>>>> + pr_err("%s failed to map scu_base via DT\n", __func__); >>>>>> >>>>>> For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand >>>>>> what does it mean, but it may confuse normal users. In current version, >>>>>> berlin doesn't complain like this for non-ca9 SoCs >>>>>> >>>>> >>>>> OK, let me see other reviewer's comment on this. Then we will decide if >>>>> this error message is required or can be omitted. >>>> >>>> We need to look at all callers here, to see if the function ever gets >>>> called for a CPU that doesn't have an SCU. I'd say we should warn if >>>> we know there is an SCU but we cannot map it, but never warn on >>>> any of the CPU cores that don't support an SCU. >>> >>> Maybe there should be two helpers: >>> >>> of_scu_enable() which _only_ looks up the SCU address in DT and enables >>> it if it finds it, otherwise returning failure. >>> >>> a9_scu_enable() which tries to use the A9 provided SCU address and >>> enables it if it finds it, otherwise returning failure. >>> OK, In that case I can see need for following four helpers as: 1: of_scu_enable() which will __only__ lookup the SCU address in DT and enables it if it finds, otherwise return -ENOMEM failure. This helper APIs is required and sufficient for most of platforms such as exynos, berlin, realview, socfpga, STi, ux500, vexpress, rockchip and mvebu 2: a9_scu_enable(), which will __only__ use A9 provided SCU address and enables it, if address mapped successfully, otherwise returning failure. This helper APIs is required and sufficient for two ARM platforms as of now tegra and hisi. 3: of_scu_get_base() which will lookup the SCU address in DT and if node found maps address and returns ioremapped address to caller. This helper APIs is required for three ARM plaforms rockchip, mvebu and ux500, along with scu_enable() API to enable and find number_of_cores. 4: s9_scu_iomap_base() which will internally use s9_scu_get_base() and do ioremap of scu address and returns ioremapped address to the caller along with ownership (caller has responsibility to unmap it). This helper APIs is required to simplify SCU enable and related code in two ARM plaforms BCM ans ZX. For remaining two ARM platforms (IMX and ZYNQ), none of these helpers are useful for the time-being, as they need SCU mapping very early of boot, where we can't use iomap APIs. So I will drop patches related to these platforms in v2 version. Please let me know if any concern in this approach. >>> Then callers can decide which of these to call, and what error messages >>> to print on their failures. >> >> Splitting the function in two is probably simpler overall, but >> we may still have to look at all the callers: Any platform that >> currently tries to map it on any CPU and doesn't warn about the >> absence of the device node (or about scu_a9_has_base() == false) >> should really continue not to warn about that. > > Did you miss the bit where none of of_scu_enable() or a9_scu_enable() > should produce any warnings or errors to be printed. It's up to the > caller to report the failure, otherwise doing this doesn't make sense: > > if (of_scu_enable() < 0 && a9_scu_enable() < 0) > pr_err("Failed to map and enable the SCU\n"); > > because if of_scu_enable() prints a warning/error, then it's patently > misleading. > I will move out error message out of these helpers and let caller (platform specific code) handle and print error if required. Thanks, Pankaj Dubey
On Thursday, November 17, 2016 9:50:27 AM CET pankaj.dubey wrote: > > >>> of_scu_enable() which _only_ looks up the SCU address in DT and enables > >>> it if it finds it, otherwise returning failure. > >>> > >>> a9_scu_enable() which tries to use the A9 provided SCU address and > >>> enables it if it finds it, otherwise returning failure. > >>> > > OK, In that case I can see need for following four helpers as: > > 1: of_scu_enable() which will __only__ lookup the SCU address in DT and > enables it if it finds, otherwise return -ENOMEM failure. > This helper APIs is required and sufficient for most of platforms such > as exynos, berlin, realview, socfpga, STi, ux500, vexpress, rockchip and > mvebu > > 2: a9_scu_enable(), which will __only__ use A9 provided SCU address and > enables it, if address mapped successfully, otherwise returning failure. > This helper APIs is required and sufficient for two ARM platforms as of > now tegra and hisi. > > 3: of_scu_get_base() which will lookup the SCU address in DT and if node > found maps address and returns ioremapped address to caller. > This helper APIs is required for three ARM plaforms rockchip, mvebu and > ux500, along with scu_enable() API to enable and find number_of_cores. > > 4: s9_scu_iomap_base() which will internally use s9_scu_get_base() and > do ioremap of scu address and returns ioremapped address to the caller > along with ownership (caller has responsibility to unmap it). > This helper APIs is required to simplify SCU enable and related code in > two ARM plaforms BCM ans ZX. > > For remaining two ARM platforms (IMX and ZYNQ), none of these helpers > are useful for the time-being, as they need SCU mapping very early of > boot, where we can't use iomap APIs. So I will drop patches related to > these platforms in v2 version. > > Please let me know if any concern in this approach. I think ideally we wouldn't even need to know the virtual address outside of smp_scu.c. If we can move all users of the address into that file directly, it could become a local variable and we change scu_power_mode() and scu_get_core_count() instead to not require the address argument. The only user I could find outside of that file is static int shmobile_smp_scu_psr_core_disabled(int cpu) { unsigned long mask = SCU_PM_POWEROFF << (cpu * 8); if ((__raw_readl(shmobile_scu_base + 8) & mask) == mask) return 1; return 0; } which can be done in the same file as well. > >>> Then callers can decide which of these to call, and what error messages > >>> to print on their failures. > >> > >> Splitting the function in two is probably simpler overall, but > >> we may still have to look at all the callers: Any platform that > >> currently tries to map it on any CPU and doesn't warn about the > >> absence of the device node (or about scu_a9_has_base() == false) > >> should really continue not to warn about that. > > > > Did you miss the bit where none of of_scu_enable() or a9_scu_enable() > > should produce any warnings or errors to be printed. It's up to the > > caller to report the failure, otherwise doing this doesn't make sense: > > > > if (of_scu_enable() < 0 && a9_scu_enable() < 0) > > pr_err("Failed to map and enable the SCU\n"); > > > > because if of_scu_enable() prints a warning/error, then it's patently > > misleading. > > That's why I said "otherwise we can leave the warning in the caller after checking the return code of the new APIs." for the case where we actually need it. > I will move out error message out of these helpers and let caller > (platform specific code) handle and print error if required. Ok. Arnd
On Thursday 17 November 2016 10:33 PM, Arnd Bergmann wrote: > On Thursday, November 17, 2016 9:50:27 AM CET pankaj.dubey wrote: >> >>>>> of_scu_enable() which _only_ looks up the SCU address in DT and enables >>>>> it if it finds it, otherwise returning failure. >>>>> >>>>> a9_scu_enable() which tries to use the A9 provided SCU address and >>>>> enables it if it finds it, otherwise returning failure. >>>>> >> >> OK, In that case I can see need for following four helpers as: >> >> 1: of_scu_enable() which will __only__ lookup the SCU address in DT and >> enables it if it finds, otherwise return -ENOMEM failure. >> This helper APIs is required and sufficient for most of platforms such >> as exynos, berlin, realview, socfpga, STi, ux500, vexpress, rockchip and >> mvebu >> >> 2: a9_scu_enable(), which will __only__ use A9 provided SCU address and >> enables it, if address mapped successfully, otherwise returning failure. >> This helper APIs is required and sufficient for two ARM platforms as of >> now tegra and hisi. >> >> 3: of_scu_get_base() which will lookup the SCU address in DT and if node >> found maps address and returns ioremapped address to caller. >> This helper APIs is required for three ARM plaforms rockchip, mvebu and >> ux500, along with scu_enable() API to enable and find number_of_cores. >> >> 4: s9_scu_iomap_base() which will internally use s9_scu_get_base() and >> do ioremap of scu address and returns ioremapped address to the caller >> along with ownership (caller has responsibility to unmap it). >> This helper APIs is required to simplify SCU enable and related code in >> two ARM plaforms BCM ans ZX. >> >> For remaining two ARM platforms (IMX and ZYNQ), none of these helpers >> are useful for the time-being, as they need SCU mapping very early of >> boot, where we can't use iomap APIs. So I will drop patches related to >> these platforms in v2 version. >> >> Please let me know if any concern in this approach. > > I think ideally we wouldn't even need to know the virtual address > outside of smp_scu.c. If we can move all users of the address > into that file directly, it could become a local variable and > we change scu_power_mode() and scu_get_core_count() instead to > not require the address argument. > If we change scu_get_core_count() without address argument, what about those platforms which are calling this API very early boot (mostly in smp_init_cpus), during this stage we can't use iomap. These platforms are doing static mapping of SCU base, and passing virtual address. Currently following are platforms which needs SCU virtual address at very early stage mostly for calling scu_get_core_count(scu_address): IMX, ZYNQ, SPEAr, and OMAP2. I can think of handling of these platform's early need of SCU in the following way: Probably we need something like early_a9_scu_get_core_count() which can be handled in this way in smp_scu.c file itself: +static struct map_desc scu_map __initdata = { + .length = SZ_256, + .type = MT_DEVICE, +}; + +static void __iomem *early_scu_map_io(void) +{ + unsigned long base; + void __iomem *scu_base; + + base = scu_a9_get_base(); + scu_map.pfn = __phys_to_pfn(base); + scu_map.virtual = base; + iotable_init(&scu_map, 1); + scu_base = (void __iomem *)base; + return scu_base; +} + +/* + * early_a9_scu_get_core_count - Get number of CPU cores at very early boot + * Only platforms which needs number_of_cores during early boot should call this + */ +unsigned int __init early_a9_scu_get_core_count(void) +{ + void __iomem *scu_base; + + scu_base = early_scu_map_io(); + return scu_get_core_count(scu_base); +} + Please let me know how about using above approach to simplify platform specific code of early users of SCU address. Also for rest platforms, which are not using scu base at very early stage, but are using virtual address which is mapped either from SCU device node or using s9_scu_get_base() to map to SCU virtual address. To separate these two we discussed to separate scu_enable in two helper APIs as of_scu_enable and s9_scu_enable. So if we change scu_get_core_count which do not requires address argument, this also needs to be separated in two as of_scu_get_core_count and s9_scu_get_core_count. Thanks, Pankaj Dubey > The only user I could find outside of that file is > > static int shmobile_smp_scu_psr_core_disabled(int cpu) > { > unsigned long mask = SCU_PM_POWEROFF << (cpu * 8); > > if ((__raw_readl(shmobile_scu_base + 8) & mask) == mask) > return 1; > > return 0; > } > > which can be done in the same file as well. > >>>>> Then callers can decide which of these to call, and what error messages >>>>> to print on their failures. >>>> >>>> Splitting the function in two is probably simpler overall, but >>>> we may still have to look at all the callers: Any platform that >>>> currently tries to map it on any CPU and doesn't warn about the >>>> absence of the device node (or about scu_a9_has_base() == false) >>>> should really continue not to warn about that. >>> >>> Did you miss the bit where none of of_scu_enable() or a9_scu_enable() >>> should produce any warnings or errors to be printed. It's up to the >>> caller to report the failure, otherwise doing this doesn't make sense: >>> >>> if (of_scu_enable() < 0 && a9_scu_enable() < 0) >>> pr_err("Failed to map and enable the SCU\n"); >>> >>> because if of_scu_enable() prints a warning/error, then it's patently >>> misleading. >>> > > That's why I said "otherwise we can leave the warning in the caller > after checking the return code of the new APIs." for the case where > we actually need it. > >> I will move out error message out of these helpers and let caller >> (platform specific code) handle and print error if required. > > Ok. > > Arnd > > >
diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h index bfe163c..fdeec07 100644 --- a/arch/arm/include/asm/smp_scu.h +++ b/arch/arm/include/asm/smp_scu.h @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, unsigned int mode) #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU) void scu_enable(void __iomem *scu_base); +void __iomem *of_scu_get_base(void); +int of_scu_enable(void); #else static inline void scu_enable(void __iomem *scu_base) {} +static inline void __iomem *of_scu_get_base(void) {return NULL; } +static inline int of_scu_enable(void) {return 0; } #endif #endif diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c index 72f9241..d0ac3ed 100644 --- a/arch/arm/kernel/smp_scu.c +++ b/arch/arm/kernel/smp_scu.c @@ -10,6 +10,7 @@ */ #include <linux/init.h> #include <linux/io.h> +#include <linux/of_address.h> #include <asm/smp_plat.h> #include <asm/smp_scu.h> @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base) */ flush_cache_all(); } + +static const struct of_device_id scu_match[] = { + { .compatible = "arm,cortex-a9-scu", }, + { .compatible = "arm,cortex-a5-scu", }, + { } +}; + +/* + * Helper API to get SCU base address + * In case platform DT do not have SCU node, or iomap fails + * this call will fallback and will try to map via call to + * scu_a9_get_base. + * This will return ownership of scu_base to the caller + */ +void __iomem *of_scu_get_base(void) +{ + unsigned long base = 0; + struct device_node *np; + void __iomem *scu_base; + + np = of_find_matching_node(NULL, scu_match); + scu_base = of_iomap(np, 0); + of_node_put(np); + if (!scu_base) { + pr_err("%s failed to map scu_base via DT\n", __func__); + if (scu_a9_has_base()) { + base = scu_a9_get_base(); + scu_base = ioremap(base, SZ_4K); + } + if (!scu_base) { + pr_err("%s failed to map scu_base\n", __func__); + return IOMEM_ERR_PTR(-ENOMEM); + } + } + return scu_base; +} + +/* + * Enable SCU via mapping scu_base DT + * If scu_base mapped successfully scu will be enabled and in case of + * failure if will return non-zero error code + */ +int of_scu_enable(void) +{ + void __iomem *scu_base; + + scu_base = of_scu_get_base(); + if (!IS_ERR(scu_base)) { + scu_enable(scu_base); + iounmap(scu_base); + return 0; + } + return PTR_ERR(scu_base); +} + #endif /*
Many platforms are duplicating code for enabling SCU, lets add common code to enable SCU by parsing SCU device node so the duplication in each platform can be avoided. CC: Krzysztof Kozlowski <krzk@kernel.org> CC: Jisheng Zhang <jszhang@marvell.com> CC: Russell King <linux@armlinux.org.uk> CC: Dinh Nguyen <dinguyen@opensource.altera.com> CC: Patrice Chotard <patrice.chotard@st.com> CC: Linus Walleij <linus.walleij@linaro.org> CC: Liviu Dudau <liviu.dudau@arm.com> CC: Ray Jui <rjui@broadcom.com> CC: Stephen Warren <swarren@wwwdotorg.org> CC: Heiko Stuebner <heiko@sntech.de> CC: Shawn Guo <shawnguo@kernel.org> CC: Michal Simek <michal.simek@xilinx.com> CC: Wei Xu <xuwei5@hisilicon.com> CC: Andrew Lunn <andrew@lunn.ch> CC: Jun Nie <jun.nie@linaro.org> Suggested-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> --- arch/arm/include/asm/smp_scu.h | 4 +++ arch/arm/kernel/smp_scu.c | 56 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+)