Re: [PATCH v4 2/2] arm64: dts: imx8mp: add reserve-memory nodes for DSP

[Alexander Stein]

Hi Mathieu,

Am Montag, 23. Oktober 2023, 19:24:28 CEST schrieb Mathieu Poirier:
> Hey guys,
> 
> On Fri, Oct 13, 2023 at 06:27:31PM +0300, Iuliana Prodan (OSS) wrote:
> > From: Iuliana Prodan 
> > 
> > Add the reserve-memory nodes used by DSP when the rpmsg
> > feature is enabled.
> > 
> > Signed-off-by: Iuliana Prodan 
> > ---
> > 
> >  arch/arm64/boot/dts/freescale/imx8mp-evk.dts | 22 
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
> > b/arch/arm64/boot/dts/freescale/imx8mp-evk.dts index
> > fa37ce89f8d3..b677ad8ef042 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
> > @@ -125,6 +125,28 @@
> > 
> > };
> > 
> > };
> > 
> > +
> > +   reserved-memory {
> > +   #address-cells = <2>;
> > +   #size-cells = <2>;
> > +   ranges;
> > +
> > +   dsp_vdev0vring0: vdev0vring0@942f {
> > +   reg = <0 0x942f 0 0x8000>;
> > +   no-map;
> > +   };
> > +
> > +   dsp_vdev0vring1: vdev0vring1@942f8000 {
> > +   reg = <0 0x942f8000 0 0x8000>;
> > +   no-map;
> > +   };
> > +
> > +   dsp_vdev0buffer: vdev0buffer@9430 {
> > +   compatible = "shared-dma-pool";
> > +   reg = <0 0x9430 0 0x10>;
> > +   no-map;
> > +   };
> > +   };
> 
> Alexander: Are you good with the refactoring?

Yes, adding this to EVK is good for me.
Acked-by: Alexander Stein 

> Rob and Krzysztof: I'm not sure if you want to ack this patch but giving you
> the benefit of the doubt.
> 
> Shawn and Sascha: Did you plan on picking up this patch or shoud I?
> 
> Thanks,
> Mathieu
> 
> >  };
> >  
> >   {


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v3 2/2] arm64: dts: imx8mp: add reserve-memory nodes for DSP

[Alexander Stein]

Hi Iuliana,

Am Freitag, 13. Oktober 2023, 10:35:17 CEST schrieb Iuliana Prodan:
> Hi Alexander,
> 
> On 10/11/2023 8:37 AM, Alexander Stein wrote:
> > Hi Iuliana,
> > 
> > Am Dienstag, 10. Oktober 2023, 11:09:29 CEST schrieb Iuliana Prodan (OSS):
> >> From: Iuliana Prodan 
> >> 
> >> Add the reserve-memory nodes used by DSP when the rpmsg
> >> feature is enabled.
> >> 
> >> Signed-off-by: Iuliana Prodan 
> >> ---
> >> 
> >>   arch/arm64/boot/dts/freescale/imx8mp.dtsi | 16 
> >>   1 file changed, 16 insertions(+)
> >> 
> >> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >> b/arch/arm64/boot/dts/freescale/imx8mp.dtsi index
> >> cc406bb338fe..22815b3ea890 100644
> >> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >> @@ -211,6 +211,22 @@
> >> 
> >>reg = <0 0x9240 0 0x200>;
> >>no-map;
> >>
> >>};
> >> 
> >> +
> >> +  dsp_vdev0vring0: vdev0vring0@942f {
> >> +  reg = <0 0x942f 0 0x8000>;
> >> +  no-map;
> >> +  };
> >> +
> >> +  dsp_vdev0vring1: vdev0vring1@942f8000 {
> >> +  reg = <0 0x942f8000 0 0x8000>;
> >> +  no-map;
> >> +  };
> >> +
> >> +  dsp_vdev0buffer: vdev0buffer@9430 {
> >> +  compatible = "shared-dma-pool";
> >> +  reg = <0 0x9430 0 0x10>;
> >> +  no-map;
> >> +  };
> > 
> > Please configure these reserved memories on board level. Not every i.MX8MP
> > based board uses this DSP or has these memory addresses available.
> 
> Will it be ok in imx8mp-evk.dts?

If that is the board using the DSP and the reserved memory, then yes.

Best regards,
Alexander

> Thanks,
> Iulia
> 
> > Best regards,
> > Alexander
> > 
> >>};
> >>
> >>pmu {


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v3 2/2] arm64: dts: imx8mp: add reserve-memory nodes for DSP

[Alexander Stein]

Hi Iuliana,

Am Dienstag, 10. Oktober 2023, 11:09:29 CEST schrieb Iuliana Prodan (OSS):
> From: Iuliana Prodan 
> 
> Add the reserve-memory nodes used by DSP when the rpmsg
> feature is enabled.
> 
> Signed-off-by: Iuliana Prodan 
> ---
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mp.dtsi index
> cc406bb338fe..22815b3ea890 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -211,6 +211,22 @@
>   reg = <0 0x9240 0 0x200>;
>   no-map;
>   };
> +
> + dsp_vdev0vring0: vdev0vring0@942f {
> + reg = <0 0x942f 0 0x8000>;
> + no-map;
> + };
> +
> + dsp_vdev0vring1: vdev0vring1@942f8000 {
> + reg = <0 0x942f8000 0 0x8000>;
> + no-map;
> + };
> +
> + dsp_vdev0buffer: vdev0buffer@9430 {
> + compatible = "shared-dma-pool";
> + reg = <0 0x9430 0 0x10>;
> + no-map;
> + };

Please configure these reserved memories on board level. Not every i.MX8MP 
based board uses this DSP or has these memory addresses available.

Best regards,
Alexander

>   };
> 
>   pmu {


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH] clk: bcm2835: Fix memory leak in bcm2835_register_pll

[Alexander Stein]

Hello,

On Thursday, October 10, 2019, 3:30:58 AM CEST Navid Emamdoost wrote:
> In the implementation of bcm2835_register_pll(), the allocated memory
> for pll should be released if devm_clk_hw_register() fails.
> 
> Fixes: b19f009d4510 ("clk: bcm2835: Migrate to clk_hw based registration and 
> OF APIs")
> Signed-off-by: Navid Emamdoost 
> ---
>  drivers/clk/bcm/clk-bcm2835.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> index 802e488fd3c3..99549642110a 100644
> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> @@ -1320,8 +1320,10 @@ static struct clk_hw *bcm2835_register_pll(struct 
> bcm2835_cprman *cprman,
>   pll->hw.init = 
>  
>   ret = devm_clk_hw_register(cprman->dev, >hw);
> - if (ret)
> + if (ret) {
> + kfree(pll);
>   return NULL;
> + }
>   return >hw;
>  }

Eh, is pll freed at all, even in successful case? I failed to find a 
corresponding kfree().
The pointer itself is lost once the function returns.
The solution would rather be to use devm_kzalloc instead of kzalloc, like the 
other clocks
in e.g. bcm2835_register_pll()

Best regards,
Alexander

[PATCH v2 1/1] iio: core: Fix fractional format generation

[Alexander Stein]

In case the result is -0.3252 tmp0 is 0 after the div_s64_rem, so tmp0 is
non-negative which results in an output of 0.3252.
Fix this by explicitly handling the negative sign ourselves.

Signed-off-by: Alexander Stein 
---
Changes in v2:
* Support vals[0] >= and vals[1] < 0 in IIO_VAL_FRACTIONAL
* Note: IIO_VAL_FRACTIONAL is untested, as I lack hardware
* Note2: Currently IIO_VAL_FRACTIONAL is only called with vals[1] from
 in-kernel drivers AFAICS

 drivers/iio/industrialio-core.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 245b5844028d..247338142c87 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -568,6 +568,7 @@ static ssize_t __iio_format_value(char *buf, size_t len, 
unsigned int type,
 {
unsigned long long tmp;
int tmp0, tmp1;
+   char *sign;
bool scale_db = false;
 
switch (type) {
@@ -593,11 +594,17 @@ static ssize_t __iio_format_value(char *buf, size_t len, 
unsigned int type,
tmp = div_s64((s64)vals[0] * 10LL, vals[1]);
tmp1 = vals[1];
tmp0 = (int)div_s64_rem(tmp, 10, );
-   return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1));
+   if (vals[1] < 0) {
+   sign = vals[0] >= 0 ? "-" : "";
+   } else {
+   sign = vals[0] < 0 ? "-" : "";
+   }
+   return snprintf(buf, len, "%s%u.%09u", sign, abs(tmp0), 
abs(tmp1));
case IIO_VAL_FRACTIONAL_LOG2:
+   sign = vals[0] < 0 ? "-" : "";
tmp = shift_right((s64)vals[0] * 10LL, vals[1]);
tmp0 = (int)div_s64_rem(tmp, 10LL, );
-   return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1));
+   return snprintf(buf, len, "%s%u.%09u", sign, abs(tmp0), 
abs(tmp1));
case IIO_VAL_INT_MULTIPLE:
{
int i;
-- 
2.23.0

[PATCH 1/1] iio: core: Fix fractional format generation

[Alexander Stein]

In case the result is -0.3252 tmp0 is 0 after the div_s64_rem, so tmp0 is
non-negative which results in an output of 0.3252.
Fix this by explicitly handling the negative sign ourselves.

Signed-off-by: Alexander Stein 
---
 drivers/iio/industrialio-core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 245b5844028d..18350c1959ae 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -568,6 +568,7 @@ static ssize_t __iio_format_value(char *buf, size_t len, 
unsigned int type,
 {
unsigned long long tmp;
int tmp0, tmp1;
+   const char *sign = vals[0] < 0 ? "-" : "";
bool scale_db = false;
 
switch (type) {
@@ -593,11 +594,11 @@ static ssize_t __iio_format_value(char *buf, size_t len, 
unsigned int type,
tmp = div_s64((s64)vals[0] * 10LL, vals[1]);
tmp1 = vals[1];
tmp0 = (int)div_s64_rem(tmp, 10, );
-   return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1));
+   return snprintf(buf, len, "%s%u.%09u", sign, abs(tmp0), 
abs(tmp1));
case IIO_VAL_FRACTIONAL_LOG2:
tmp = shift_right((s64)vals[0] * 10LL, vals[1]);
tmp0 = (int)div_s64_rem(tmp, 10LL, );
-   return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1));
+   return snprintf(buf, len, "%s%u.%09u", sign, abs(tmp0), 
abs(tmp1));
case IIO_VAL_INT_MULTIPLE:
{
int i;
-- 
2.23.0

Re: [PATCH] i2c: imx: Allow the driver to be built for ARM64 SoCs such as i.MX8M

[Alexander Stein]

On Wednesday, August 7, 2019, 1:44:06 PM CEST Schrempf Frieder wrote:
> From: Frieder Schrempf 
> 
> The imx I2C controller is used in some ARM64 SoCs such as i.MX8M.
> To make use of it, append ARM64 to the list of dependencies.
> 
> Signed-off-by: Frieder Schrempf 
> ---
>  drivers/i2c/busses/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 09367fc014c3..46b653621513 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -664,7 +664,7 @@ config I2C_IMG
>  
>  config I2C_IMX
>   tristate "IMX I2C interface"
> - depends on ARCH_MXC || ARCH_LAYERSCAPE || COLDFIRE
> + depends on ARCH_MXC || ARCH_LAYERSCAPE || COLDFIRE || ARM64

I don't think this should be necessary at all as ARCH_MXC is also available for 
arm64, see 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/Kconfig.platforms#n167
I rather wonder why ARCH_MXC is not set. Same for fec.

>   help
> Say Y here if you want to use the IIC bus controller on
> the Freescale i.MX/MXC, Layerscape or ColdFire processors.
> 

Re: [PATCH v1] gpio: mpc8xxx: Add new platforms GPIO DT node description

[Alexander Stein]

On Monday, August 5, 2019, 11:14:32 AM CEST Hui Song wrote:
> From: Song Hui 
> 
> Update the NXP GPIO node dt-binding file for QorIQ and
> Layerscape platforms, and add one more example with
> ls1028a GPIO node.
> 
> Signed-off-by: Song Hui 
> ---
>  Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt 
> b/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
> index 69d4616..fbe6d75 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
> @@ -28,7 +28,7 @@ gpio0: gpio@1100 {
>  Example of gpio-controller node for a ls2080a SoC:

   ^^^   ^^^
This is an example for ls2080a...

>  gpio0: gpio@230 {
> - compatible = "fsl,ls2080a-gpio", "fsl,qoriq-gpio";
> + compatible = "fsl,ls1028a-gpio","fsl,ls2080a-gpio", "fsl,qoriq-gpio";

so I doubt there should be a ls1028a compatible here though.

>   reg = <0x0 0x230 0x0 0x1>;
>   interrupts = <0 36 0x4>; /* Level high type */
>   gpio-controller;

Best regards,
Alexander

Re: [PATCH] power: reset: at91-reset: enable I-cache for at91sam9260_reset

[Alexander Stein]

On Tuesday, October 16, 2018, 3:30:24 PM CEST claudiu.bez...@microchip.com 
wrote:
> Hi Jonas,
> 
> On 07.10.2018 15:57, Jonas Danielsson wrote:
> > From: Jonas Danielsson 
> > 
> > This fixes a bug where our embedded system (AT91SAM9260 based) would
> > hang at reboot. At the most we managed 16 boot loops without a hang.
> > 
> > With this patch applied the problem has not been observed and the board
> > has managed above 250 boot loops.
> > 
> > The AT91SAM9260 datasheet tells us that with the instruction cache
> > disabled all instructions are fetched from SDRAM. And we have an errata
> > telling us we must power down the SDRAM before issuing cpu reset.
> > 
> > This means we need the instruction cache enabled in at91sam9260_reset()
> > At the moment it is being disabled in cpu_proc_fin() which is called from
> > arch/arm/kernel/reboot.c.
> 
> Are you using kexec reboot or implemented hibernate mode on this machine?
> I'm seeing cpu_proc_fin() is called only in case of kexec reboot or
> switching to hibernate mode.
> 
> In case of normal reboot (e.g. reboot command) machine_restart() from
> arch/arm/kernel/reboot.c is called. Please correct me if I'm wrong.

Another location is cpu_reset() aka cpu_arm926_reset() in proc-arm926.S
which also disables I-cache. But I can't track down a callstack
ending there.

Best regards,
Alexander

Re: [PATCH] power: reset: at91-reset: enable I-cache for at91sam9260_reset

[Alexander Stein]

On Tuesday, October 16, 2018, 3:30:24 PM CEST claudiu.bez...@microchip.com 
wrote:
> Hi Jonas,
> 
> On 07.10.2018 15:57, Jonas Danielsson wrote:
> > From: Jonas Danielsson 
> > 
> > This fixes a bug where our embedded system (AT91SAM9260 based) would
> > hang at reboot. At the most we managed 16 boot loops without a hang.
> > 
> > With this patch applied the problem has not been observed and the board
> > has managed above 250 boot loops.
> > 
> > The AT91SAM9260 datasheet tells us that with the instruction cache
> > disabled all instructions are fetched from SDRAM. And we have an errata
> > telling us we must power down the SDRAM before issuing cpu reset.
> > 
> > This means we need the instruction cache enabled in at91sam9260_reset()
> > At the moment it is being disabled in cpu_proc_fin() which is called from
> > arch/arm/kernel/reboot.c.
> 
> Are you using kexec reboot or implemented hibernate mode on this machine?
> I'm seeing cpu_proc_fin() is called only in case of kexec reboot or
> switching to hibernate mode.
> 
> In case of normal reboot (e.g. reboot command) machine_restart() from
> arch/arm/kernel/reboot.c is called. Please correct me if I'm wrong.

Another location is cpu_reset() aka cpu_arm926_reset() in proc-arm926.S
which also disables I-cache. But I can't track down a callstack
ending there.

Best regards,
Alexander

Re: [PATCH] power: reset: at91-reset: enable I-cache for at91sam9260_reset

[Alexander Stein]

Do you have CONFIG_CPU_ICACHE_DISABLE enabled?
I wonder why I-cache is disabled. I know about this errata, AT91SAM9G20 is 
affected as well.

Best regards,
Alexander

On Sunday, October 7, 2018, 2:57:45 PM CEST Jonas Danielsson wrote:
> From: Jonas Danielsson 
> 
> This fixes a bug where our embedded system (AT91SAM9260 based) would
> hang at reboot. At the most we managed 16 boot loops without a hang.
> 
> With this patch applied the problem has not been observed and the board
> has managed above 250 boot loops.
> 
> The AT91SAM9260 datasheet tells us that with the instruction cache
> disabled all instructions are fetched from SDRAM. And we have an errata
> telling us we must power down the SDRAM before issuing cpu reset.
> 
> This means we need the instruction cache enabled in at91sam9260_reset()
> At the moment it is being disabled in cpu_proc_fin() which is called from
> arch/arm/kernel/reboot.c.
> 
> Signed-off-by: Jonas Danielsson 
> ---
>  drivers/power/reset/at91-reset.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/reset/at91-reset.c 
> b/drivers/power/reset/at91-reset.c
> index f44a9ffcc2ab..78972bba64df 100644
> --- a/drivers/power/reset/at91-reset.c
> +++ b/drivers/power/reset/at91-reset.c
> @@ -50,14 +50,24 @@ static void __iomem *at91_ramc_base[2], *at91_rstc_base;
>  static struct clk *sclk;
>  
>  /*
> -* unless the SDRAM is cleanly shutdown before we hit the
> +* Errata 43.1.7.1 RSTC: Reset during SDRAM Accesses
> +*
> +* Unless the SDRAM is cleanly shutdown before we hit the
>  * reset register it can be left driving the data bus and
>  * killing the chance of a subsequent boot from NAND
> +*
> +* Since we are disabling SDRAM need to make sure that the
> +* instruction cache is enabled.
>  */
>  static int at91sam9260_restart(struct notifier_block *this, unsigned long 
> mode,
>  void *cmd)
>  {
>   asm volatile(
> + /* Enable I-cache */
> + "mrcp15, 0, r0, c1, c0, 0\n\t"
> + "orrr0, r0, #4096\n\t" /* CR_I (bit 12) */
> + "mcrp15, 0, r0, c1, c0, 0\n\t"
> +
>   /* Align to cache lines */
>   ".balign 32\n\t"
>  
> 


-- 
SYS TEC electronic GmbH
Am Windrad 2
08468 Heinsdorfergrund
Germany
Telefon : +49 (0) 3765 38600-0
Fax : +49 (0) 3765 38600-4100
Email   : alexander.st...@systec-electronic.com
Website : http://www.systec-electronic.com

Managing Director   : Dipl.-Phys. Siegmar Schmidt
Commercial registry : Amtsgericht Chemnitz, HRB 28082
USt.-Id Nr. : DE150534010

Diese E-Mail enthält vertrauliche und/oder rechtlich geschützte Informationen.
Wenn Sie nicht der richtige Adressat sind oder diese E-Mail irrtümlich erhalten 
haben, informieren Sie bitte sofort den Absender und vernichten Sie diese Mail.
Das unerlaubte Kopieren sowie die unbefugte Weitergabe dieser Mail sind nicht 
gestattet.
 
This e-mail may contain confidential and/or privileged information.
If you are not the intended recipient (or have received this e-mail in error) 
please notify the sender immediately and destroy this e-mail.
Any unauthorized copying, disclosure or distribution of the material in this 
e-mail is strictly forbidden.

Re: [PATCH] power: reset: at91-reset: enable I-cache for at91sam9260_reset

[Alexander Stein]

Do you have CONFIG_CPU_ICACHE_DISABLE enabled?
I wonder why I-cache is disabled. I know about this errata, AT91SAM9G20 is 
affected as well.

Best regards,
Alexander

On Sunday, October 7, 2018, 2:57:45 PM CEST Jonas Danielsson wrote:
> From: Jonas Danielsson 
> 
> This fixes a bug where our embedded system (AT91SAM9260 based) would
> hang at reboot. At the most we managed 16 boot loops without a hang.
> 
> With this patch applied the problem has not been observed and the board
> has managed above 250 boot loops.
> 
> The AT91SAM9260 datasheet tells us that with the instruction cache
> disabled all instructions are fetched from SDRAM. And we have an errata
> telling us we must power down the SDRAM before issuing cpu reset.
> 
> This means we need the instruction cache enabled in at91sam9260_reset()
> At the moment it is being disabled in cpu_proc_fin() which is called from
> arch/arm/kernel/reboot.c.
> 
> Signed-off-by: Jonas Danielsson 
> ---
>  drivers/power/reset/at91-reset.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/reset/at91-reset.c 
> b/drivers/power/reset/at91-reset.c
> index f44a9ffcc2ab..78972bba64df 100644
> --- a/drivers/power/reset/at91-reset.c
> +++ b/drivers/power/reset/at91-reset.c
> @@ -50,14 +50,24 @@ static void __iomem *at91_ramc_base[2], *at91_rstc_base;
>  static struct clk *sclk;
>  
>  /*
> -* unless the SDRAM is cleanly shutdown before we hit the
> +* Errata 43.1.7.1 RSTC: Reset during SDRAM Accesses
> +*
> +* Unless the SDRAM is cleanly shutdown before we hit the
>  * reset register it can be left driving the data bus and
>  * killing the chance of a subsequent boot from NAND
> +*
> +* Since we are disabling SDRAM need to make sure that the
> +* instruction cache is enabled.
>  */
>  static int at91sam9260_restart(struct notifier_block *this, unsigned long 
> mode,
>  void *cmd)
>  {
>   asm volatile(
> + /* Enable I-cache */
> + "mrcp15, 0, r0, c1, c0, 0\n\t"
> + "orrr0, r0, #4096\n\t" /* CR_I (bit 12) */
> + "mcrp15, 0, r0, c1, c0, 0\n\t"
> +
>   /* Align to cache lines */
>   ".balign 32\n\t"
>  
> 


-- 
SYS TEC electronic GmbH
Am Windrad 2
08468 Heinsdorfergrund
Germany
Telefon : +49 (0) 3765 38600-0
Fax : +49 (0) 3765 38600-4100
Email   : alexander.st...@systec-electronic.com
Website : http://www.systec-electronic.com

Managing Director   : Dipl.-Phys. Siegmar Schmidt
Commercial registry : Amtsgericht Chemnitz, HRB 28082
USt.-Id Nr. : DE150534010

Diese E-Mail enthält vertrauliche und/oder rechtlich geschützte Informationen.
Wenn Sie nicht der richtige Adressat sind oder diese E-Mail irrtümlich erhalten 
haben, informieren Sie bitte sofort den Absender und vernichten Sie diese Mail.
Das unerlaubte Kopieren sowie die unbefugte Weitergabe dieser Mail sind nicht 
gestattet.
 
This e-mail may contain confidential and/or privileged information.
If you are not the intended recipient (or have received this e-mail in error) 
please notify the sender immediately and destroy this e-mail.
Any unauthorized copying, disclosure or distribution of the material in this 
e-mail is strictly forbidden.

Re: [x86] BUG()/BUG_ON() macros cannot be disabled

[Alexander Stein]

Hello Arnd,

On Tuesday, September 25, 2018, 2:06:38 PM CEST Arnd Bergmann wrote:
> We don't really want an empty macro any more, this was used in
> the past, but causes compile-time warnings and undefined behavior
> for no good reason.

Can you elaborate or point me to some other discussion about that undefined 
behavior?

Best regards,
Alexander

Re: [x86] BUG()/BUG_ON() macros cannot be disabled

[Alexander Stein]

Hello Arnd,

On Tuesday, September 25, 2018, 2:06:38 PM CEST Arnd Bergmann wrote:
> We don't really want an empty macro any more, this was used in
> the past, but causes compile-time warnings and undefined behavior
> for no good reason.

Can you elaborate or point me to some other discussion about that undefined 
behavior?

Best regards,
Alexander

Re: [PATCH] staging: most: video: fix registration of an empty comp core_component

[Alexander Stein]

On Wednesday, September 5, 2018, 11:46:05 AM CEST Colin King wrote:
> From: Colin Ian King 
> 
> Currently we have structrues comp (which is empty) and comp_info being
> used to register and deregister the component.  This mismatch in naming
> occurred from a previous commit that renamed aim_info to comp. Fix this
> to use consistent component naming in line with most/net, most/sound etc.
> 
> This fixes the message two issues, one with a null empty name when
> loading the module:
> 
> [ 1485.269515] most_core: registered new core component (null)
> 
> and an Oops when removing the module:
> 
> [ 1485.277971] BUG: unable to handle kernel NULL pointer dereference at 
> 0008
> [ 1485.278648] PGD 0 P4D 0
> [ 1485.279253] Oops: 0002 [#2] SMP PTI
> [ 1485.279847] CPU: 1 PID: 32629 Comm: modprobe Tainted: P  D WC OE 
> 4.18.0-8-generic #9
> [ 1485.280442] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
> 0.0.0 02/06/2015
> [ 1485.281040] RIP: 0010:most_deregister_component+0x3c/0x70 [most_core]
> .. etc
> 
> Fixes: 1b10a0316e2d ("staging: most: video: remove aim designators")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/staging/most/video/video.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/most/video/video.c 
> b/drivers/staging/most/video/video.c
> index cf342eb58e10..ad7e28ab9a4f 100644
> --- a/drivers/staging/most/video/video.c
> +++ b/drivers/staging/most/video/video.c
> @@ -530,7 +530,7 @@ static int comp_disconnect_channel(struct most_interface 
> *iface,
>   return 0;
>  }
>  
> -static struct core_component comp_info = {
> +static struct core_component comp = {
>   .name = "video",
>   .probe_channel = comp_probe_channel,
>   .disconnect_channel = comp_disconnect_channel,

Doesn't it make more sense to move that variable defintion where currently the 
forward declaration is?
This way you can't have 2 variables accidentally. You will need forward 
declarations for those two functions, but a mismatch here results in a linker 
error rather than a runtime NULL pointer access

Best regards,
Alexander

Re: [PATCH] staging: most: video: fix registration of an empty comp core_component

[Alexander Stein]

On Wednesday, September 5, 2018, 11:46:05 AM CEST Colin King wrote:
> From: Colin Ian King 
> 
> Currently we have structrues comp (which is empty) and comp_info being
> used to register and deregister the component.  This mismatch in naming
> occurred from a previous commit that renamed aim_info to comp. Fix this
> to use consistent component naming in line with most/net, most/sound etc.
> 
> This fixes the message two issues, one with a null empty name when
> loading the module:
> 
> [ 1485.269515] most_core: registered new core component (null)
> 
> and an Oops when removing the module:
> 
> [ 1485.277971] BUG: unable to handle kernel NULL pointer dereference at 
> 0008
> [ 1485.278648] PGD 0 P4D 0
> [ 1485.279253] Oops: 0002 [#2] SMP PTI
> [ 1485.279847] CPU: 1 PID: 32629 Comm: modprobe Tainted: P  D WC OE 
> 4.18.0-8-generic #9
> [ 1485.280442] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
> 0.0.0 02/06/2015
> [ 1485.281040] RIP: 0010:most_deregister_component+0x3c/0x70 [most_core]
> .. etc
> 
> Fixes: 1b10a0316e2d ("staging: most: video: remove aim designators")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/staging/most/video/video.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/most/video/video.c 
> b/drivers/staging/most/video/video.c
> index cf342eb58e10..ad7e28ab9a4f 100644
> --- a/drivers/staging/most/video/video.c
> +++ b/drivers/staging/most/video/video.c
> @@ -530,7 +530,7 @@ static int comp_disconnect_channel(struct most_interface 
> *iface,
>   return 0;
>  }
>  
> -static struct core_component comp_info = {
> +static struct core_component comp = {
>   .name = "video",
>   .probe_channel = comp_probe_channel,
>   .disconnect_channel = comp_disconnect_channel,

Doesn't it make more sense to move that variable defintion where currently the 
forward declaration is?
This way you can't have 2 variables accidentally. You will need forward 
declarations for those two functions, but a mismatch here results in a linker 
error rather than a runtime NULL pointer access

Best regards,
Alexander

Re: [PATCH] gpio: aspeed: fix compile testing warning

[Alexander Stein]

On Monday, July 9, 2018, 4:56:03 PM CEST Arnd Bergmann wrote:
> Gcc cannot always see that BUG_ON(1) is guaranteed to not
> return, so we get a warning message in some configurations:
> 
> drivers/gpio/gpio-aspeed.c: In function 'bank_reg':
> drivers/gpio/gpio-aspeed.c:244:1: error: control reaches end of non-void
> function [-Werror=return-type]
> 
> Using a plain BUG() is easier here and avoids the problem.
> 
> Fixes: 44ddf559d579 ("gpio: aspeed: Rework register type accessors")
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/gpio/gpio-aspeed.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
> index 1e00f4045f9d..2342e154029b 100644
> --- a/drivers/gpio/gpio-aspeed.c
> +++ b/drivers/gpio/gpio-aspeed.c
> @@ -240,7 +240,7 @@ static inline void __iomem *bank_reg(struct aspeed_gpio
> *gpio, case reg_cmdsrc1:
>   return gpio->base + bank->cmdsrc_regs + GPIO_CMDSRC_1;
>   }
> - BUG_ON(1);
> + BUG();
>  }
> 
>  #define GPIO_BANK(x) ((x) >> 5)

Is the semantic of BUG() (and BUG_ON as well) to never return? If so, then 
just an idea: Is it possible to add some macro magic in BUG_ON(x) to fail 
compiling if x is compile-constant? Giving a hint the passed condition always 
fails, which indicates a problem, at least to me.
>From a short search I found this in drivers/gpu/vga/vgaarb.c L630-633:
>   if (vgadev_find(pdev) != NULL) {
>   BUG_ON(1);
>   goto fail;
>   }
You can't fail with a BUG_ON(1) and try to do some error handling after that.

Best regards,
Alexander

Re: [PATCH] gpio: aspeed: fix compile testing warning

[Alexander Stein]

On Monday, July 9, 2018, 4:56:03 PM CEST Arnd Bergmann wrote:
> Gcc cannot always see that BUG_ON(1) is guaranteed to not
> return, so we get a warning message in some configurations:
> 
> drivers/gpio/gpio-aspeed.c: In function 'bank_reg':
> drivers/gpio/gpio-aspeed.c:244:1: error: control reaches end of non-void
> function [-Werror=return-type]
> 
> Using a plain BUG() is easier here and avoids the problem.
> 
> Fixes: 44ddf559d579 ("gpio: aspeed: Rework register type accessors")
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/gpio/gpio-aspeed.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
> index 1e00f4045f9d..2342e154029b 100644
> --- a/drivers/gpio/gpio-aspeed.c
> +++ b/drivers/gpio/gpio-aspeed.c
> @@ -240,7 +240,7 @@ static inline void __iomem *bank_reg(struct aspeed_gpio
> *gpio, case reg_cmdsrc1:
>   return gpio->base + bank->cmdsrc_regs + GPIO_CMDSRC_1;
>   }
> - BUG_ON(1);
> + BUG();
>  }
> 
>  #define GPIO_BANK(x) ((x) >> 5)

Is the semantic of BUG() (and BUG_ON as well) to never return? If so, then 
just an idea: Is it possible to add some macro magic in BUG_ON(x) to fail 
compiling if x is compile-constant? Giving a hint the passed condition always 
fails, which indicates a problem, at least to me.
>From a short search I found this in drivers/gpu/vga/vgaarb.c L630-633:
>   if (vgadev_find(pdev) != NULL) {
>   BUG_ON(1);
>   goto fail;
>   }
You can't fail with a BUG_ON(1) and try to do some error handling after that.

Best regards,
Alexander

Re: [PATCH v2 1/2] gpio: davinci: Shuffle IRQ resource fetching from DT to beginning of probe

[Alexander Stein]

On Tuesday, June 12, 2018, 7:27:52 AM CEST Keerthy wrote:
> This is needed in case of PROBE_DEFER if IRQ resource is not yet ready.
> 
> Signed-off-by: Keerthy 
> ---
> [...]
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> [...]
> @@ -168,7 +168,7 @@ static int davinci_gpio_probe(struct platform_device 
> *pdev)
>  {
>   static int ctrl_num, bank_base;
>   int gpio, bank, ret = 0;
> - unsigned ngpio, nbank;
> + unsigned ngpio, nbank, bank_irq;

bank_irq should be an int, not unsigned, no?

>   struct davinci_gpio_controller *chips;
>   struct davinci_gpio_platform_data *pdata;
>   struct device *dev = >dev;
> @@ -209,6 +209,12 @@ static int davinci_gpio_probe(struct platform_device 
> *pdev)
>   if (IS_ERR(gpio_base))
>   return PTR_ERR(gpio_base);
>  
> + bank_irq = platform_get_irq(pdev, 0);
> + if (bank_irq < 0) {

This won't work using an unsigned.

Best regards,
Alexander

Re: [PATCH v2 1/2] gpio: davinci: Shuffle IRQ resource fetching from DT to beginning of probe

[Alexander Stein]

On Tuesday, June 12, 2018, 7:27:52 AM CEST Keerthy wrote:
> This is needed in case of PROBE_DEFER if IRQ resource is not yet ready.
> 
> Signed-off-by: Keerthy 
> ---
> [...]
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> [...]
> @@ -168,7 +168,7 @@ static int davinci_gpio_probe(struct platform_device 
> *pdev)
>  {
>   static int ctrl_num, bank_base;
>   int gpio, bank, ret = 0;
> - unsigned ngpio, nbank;
> + unsigned ngpio, nbank, bank_irq;

bank_irq should be an int, not unsigned, no?

>   struct davinci_gpio_controller *chips;
>   struct davinci_gpio_platform_data *pdata;
>   struct device *dev = >dev;
> @@ -209,6 +209,12 @@ static int davinci_gpio_probe(struct platform_device 
> *pdev)
>   if (IS_ERR(gpio_base))
>   return PTR_ERR(gpio_base);
>  
> + bank_irq = platform_get_irq(pdev, 0);
> + if (bank_irq < 0) {

This won't work using an unsigned.

Best regards,
Alexander

Re: [bug, bisected] pfifo_fast causes packet reordering

[Alexander Stein]

On Friday, March 16, 2018, 11:26:47 AM CET Jakob Unterwurzacher wrote:
> On 15.03.18 23:30, John Fastabend wrote:
> >> I have reproduced it using two USB network cards connected to each other. 
> >> The test tool sends UDP packets containing a counter and listens on the 
> >> other interface, it is available at
> >> https://github.com/jakob-tsd/pfifo_stress/blob/master/pfifo_stress.py
> >>
> > 
> > Great thanks, can you also run this with taskset to bind to
> > a single CPU,
> > 
> >   # taskset 0x1 ./pifof_stress.py
> > 
> > And let me know if you still see the OOO.
> 
> Interesting. Looks like it depends on which core it runs on. CPU0 is 
> clean, CPU1 is not.
> 
> Clean:taskset --cpu-list 0 ./pfifo_stress.py
> 
> Broken:   taskset --cpu-list 1 ./pfifo_stress.py
> 
> Maybe related: CPU0 is where USB interrupts are handled:
> 
> > root@rk3399-q7:~# cat /proc/interrupts   
> >CPU0   CPU1   CPU2   CPU3   CPU4   CPU5
> > 217:2175353  0  0  0  0  0 
> > GICv3 142 Level xhci-hcd:usb5

This reminds me somewhat of this thread: 
https://marc.info/?l=linux-can=148007442317274=2

Best regards,
Alexander

Re: [bug, bisected] pfifo_fast causes packet reordering

[Alexander Stein]

On Friday, March 16, 2018, 11:26:47 AM CET Jakob Unterwurzacher wrote:
> On 15.03.18 23:30, John Fastabend wrote:
> >> I have reproduced it using two USB network cards connected to each other. 
> >> The test tool sends UDP packets containing a counter and listens on the 
> >> other interface, it is available at
> >> https://github.com/jakob-tsd/pfifo_stress/blob/master/pfifo_stress.py
> >>
> > 
> > Great thanks, can you also run this with taskset to bind to
> > a single CPU,
> > 
> >   # taskset 0x1 ./pifof_stress.py
> > 
> > And let me know if you still see the OOO.
> 
> Interesting. Looks like it depends on which core it runs on. CPU0 is 
> clean, CPU1 is not.
> 
> Clean:taskset --cpu-list 0 ./pfifo_stress.py
> 
> Broken:   taskset --cpu-list 1 ./pfifo_stress.py
> 
> Maybe related: CPU0 is where USB interrupts are handled:
> 
> > root@rk3399-q7:~# cat /proc/interrupts   
> >CPU0   CPU1   CPU2   CPU3   CPU4   CPU5
> > 217:2175353  0  0  0  0  0 
> > GICv3 142 Level xhci-hcd:usb5

This reminds me somewhat of this thread: 
https://marc.info/?l=linux-can=148007442317274=2

Best regards,
Alexander

Re: [RFC] irqchip: add support for LS1021A external interrupt lines

[Alexander Stein]

On Monday, December 11, 2017, 3:06:52 PM CET Rasmus Villemoes wrote:
> On 2017-12-11 14:45, Rasmus Villemoes wrote:
> > On 2017-12-11 11:02, Alexander Stein wrote:
> > 
> >> Oh, and what is the content of register SCFG_SCFGREVCR?
> > 
> > Good point. On my board it's 0x, set even before U-boot starts,
> > and lots board support code in U-boot expects this. I can't immediately
> > find examples in the linux source code that actually writes to the scfg,
> 
> Not a write, but we do already implicitly assume SCFG_SCFGREVCR is set
> to all-ones: In drivers/pci/dwc/pci-layerscape.c, bits which are
> numbered 6-11 in the reference manual are extracted with a regmap_read()
> followed by a left-shift by 20 and mask with 0x3f. That's consistent
> with me setting bit 0 (reference manual enumeration) using 1U<<31.

We set SCFG_SCFGREVCR to all-ones too, even before u-boot in rcw. Problem
is, this bit-reversal is only valid for SCFG. It's a shame, but at least add
a comment in the code you expect SCFG_SCFGREVCR as 0x.

Best regards,
Alexander

Re: [RFC] irqchip: add support for LS1021A external interrupt lines

[Alexander Stein]

On Monday, December 11, 2017, 3:06:52 PM CET Rasmus Villemoes wrote:
> On 2017-12-11 14:45, Rasmus Villemoes wrote:
> > On 2017-12-11 11:02, Alexander Stein wrote:
> > 
> >> Oh, and what is the content of register SCFG_SCFGREVCR?
> > 
> > Good point. On my board it's 0x, set even before U-boot starts,
> > and lots board support code in U-boot expects this. I can't immediately
> > find examples in the linux source code that actually writes to the scfg,
> 
> Not a write, but we do already implicitly assume SCFG_SCFGREVCR is set
> to all-ones: In drivers/pci/dwc/pci-layerscape.c, bits which are
> numbered 6-11 in the reference manual are extracted with a regmap_read()
> followed by a left-shift by 20 and mask with 0x3f. That's consistent
> with me setting bit 0 (reference manual enumeration) using 1U<<31.

We set SCFG_SCFGREVCR to all-ones too, even before u-boot in rcw. Problem
is, this bit-reversal is only valid for SCFG. It's a shame, but at least add
a comment in the code you expect SCFG_SCFGREVCR as 0x.

Best regards,
Alexander

Re: [RFC] irqchip: add support for LS1021A external interrupt lines

[Alexander Stein]

On Monday, December 11, 2017, 10:45:09 AM CET Alexander Stein wrote:
> On Monday, December 11, 2017, 10:08:20 AM CET Rasmus Villemoes wrote:
> > >>> +static int
> > >>> +ls1021a_extirq_set_type(struct irq_data *data, unsigned int type)
> > >>> +{
> > >>> +   irq_hw_number_t hwirq = data->hwirq;
> > >>> +   struct extirq_chip_data *chip_data = data->chip_data;
> > >>> +   u32 value, mask;
> > >>> +   int ret;
> > >>> +
> > >>> +   mask = 1U << (31 - hwirq);
> > >>
> > >> Is this really correct? IRQ0 is still at bit position 0. Don't be mislead
> > >> by the left most position in the register layout. This is just strange 
> > >> way
> > >> to express bit-endian access.
> > 
> > Yes, I'm sure. The 26 unused bits in the INTPCR register are marked as
> > reserved with a POR value of 0. Fortunately, they can still be set and
> > read back, and when I did 1U << hwirq it was some of those bits that got
> > set (the POR value of the six used bits are all 1, so the hardware still
> > worked on my board because all the lines happen to be of negative polarity).
> 
> Which functions do reg_read and reg_write in chip_data->syscon->bus_context
> actually point to?
> bus_context is actually a struct regmap_mmio_context *.

Oh, and what is the content of register SCFG_SCFGREVCR?

Alexander

Re: [RFC] irqchip: add support for LS1021A external interrupt lines

[Alexander Stein]

On Monday, December 11, 2017, 10:45:09 AM CET Alexander Stein wrote:
> On Monday, December 11, 2017, 10:08:20 AM CET Rasmus Villemoes wrote:
> > >>> +static int
> > >>> +ls1021a_extirq_set_type(struct irq_data *data, unsigned int type)
> > >>> +{
> > >>> +   irq_hw_number_t hwirq = data->hwirq;
> > >>> +   struct extirq_chip_data *chip_data = data->chip_data;
> > >>> +   u32 value, mask;
> > >>> +   int ret;
> > >>> +
> > >>> +   mask = 1U << (31 - hwirq);
> > >>
> > >> Is this really correct? IRQ0 is still at bit position 0. Don't be mislead
> > >> by the left most position in the register layout. This is just strange 
> > >> way
> > >> to express bit-endian access.
> > 
> > Yes, I'm sure. The 26 unused bits in the INTPCR register are marked as
> > reserved with a POR value of 0. Fortunately, they can still be set and
> > read back, and when I did 1U << hwirq it was some of those bits that got
> > set (the POR value of the six used bits are all 1, so the hardware still
> > worked on my board because all the lines happen to be of negative polarity).
> 
> Which functions do reg_read and reg_write in chip_data->syscon->bus_context
> actually point to?
> bus_context is actually a struct regmap_mmio_context *.

Oh, and what is the content of register SCFG_SCFGREVCR?

Alexander

Re: [RFC] irqchip: add support for LS1021A external interrupt lines

[Alexander Stein]

On Monday, December 11, 2017, 10:08:20 AM CET Rasmus Villemoes wrote:
> >>> +static int
> >>> +ls1021a_extirq_set_type(struct irq_data *data, unsigned int type)
> >>> +{
> >>> + irq_hw_number_t hwirq = data->hwirq;
> >>> + struct extirq_chip_data *chip_data = data->chip_data;
> >>> + u32 value, mask;
> >>> + int ret;
> >>> +
> >>> + mask = 1U << (31 - hwirq);
> >>
> >> Is this really correct? IRQ0 is still at bit position 0. Don't be mislead
> >> by the left most position in the register layout. This is just strange way
> >> to express bit-endian access.
> 
> Yes, I'm sure. The 26 unused bits in the INTPCR register are marked as
> reserved with a POR value of 0. Fortunately, they can still be set and
> read back, and when I did 1U << hwirq it was some of those bits that got
> set (the POR value of the six used bits are all 1, so the hardware still
> worked on my board because all the lines happen to be of negative polarity).

Which functions do reg_read and reg_write in chip_data->syscon->bus_context
actually point to?
bus_context is actually a struct regmap_mmio_context *.

> >> Anyway, please use BIT(x) instead.
> 
> I really prefer not to, that macro obfuscates the type, and unsigned
> long is the wrong thing to use for something that must be a 32 bit
> quantity. Sure, BITS_PER_LONG==32 in this case, but I don't think
> BIT(foo) is any easier to read than 1U << (foo).

Well, there a lots of other places where BIT(x) is used for u32 data types,
or even 16 Bit types. IMHO BIT(x) is more obvious as it already says set Bit x

> >>> + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int
> >>> +ls1021a_extirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> >>> + unsigned int nr_irqs, void *arg)
> >>> +{
> >>> + static const unsigned xlate[NIRQ] = {163,164,165,167,168,169};
> >> ^^
> >> No need for static here.
> > 
> > Why would you store this on the stack each time you enter the function?
> 
> Exactly, it takes a lot less .rodata to make this static than having gcc
> generate .text to build this array on the stack.
> 
> > That's the wrong construct (these values should come from DT), but
> > static is perfectly fine.
> 
> OK.

Intresting. Thanks for the info.

Regards,
Alexander

Re: [RFC] irqchip: add support for LS1021A external interrupt lines

[Alexander Stein]

On Monday, December 11, 2017, 10:08:20 AM CET Rasmus Villemoes wrote:
> >>> +static int
> >>> +ls1021a_extirq_set_type(struct irq_data *data, unsigned int type)
> >>> +{
> >>> + irq_hw_number_t hwirq = data->hwirq;
> >>> + struct extirq_chip_data *chip_data = data->chip_data;
> >>> + u32 value, mask;
> >>> + int ret;
> >>> +
> >>> + mask = 1U << (31 - hwirq);
> >>
> >> Is this really correct? IRQ0 is still at bit position 0. Don't be mislead
> >> by the left most position in the register layout. This is just strange way
> >> to express bit-endian access.
> 
> Yes, I'm sure. The 26 unused bits in the INTPCR register are marked as
> reserved with a POR value of 0. Fortunately, they can still be set and
> read back, and when I did 1U << hwirq it was some of those bits that got
> set (the POR value of the six used bits are all 1, so the hardware still
> worked on my board because all the lines happen to be of negative polarity).

Which functions do reg_read and reg_write in chip_data->syscon->bus_context
actually point to?
bus_context is actually a struct regmap_mmio_context *.

> >> Anyway, please use BIT(x) instead.
> 
> I really prefer not to, that macro obfuscates the type, and unsigned
> long is the wrong thing to use for something that must be a 32 bit
> quantity. Sure, BITS_PER_LONG==32 in this case, but I don't think
> BIT(foo) is any easier to read than 1U << (foo).

Well, there a lots of other places where BIT(x) is used for u32 data types,
or even 16 Bit types. IMHO BIT(x) is more obvious as it already says set Bit x

> >>> + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int
> >>> +ls1021a_extirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> >>> + unsigned int nr_irqs, void *arg)
> >>> +{
> >>> + static const unsigned xlate[NIRQ] = {163,164,165,167,168,169};
> >> ^^
> >> No need for static here.
> > 
> > Why would you store this on the stack each time you enter the function?
> 
> Exactly, it takes a lot less .rodata to make this static than having gcc
> generate .text to build this array on the stack.
> 
> > That's the wrong construct (these values should come from DT), but
> > static is perfectly fine.
> 
> OK.

Intresting. Thanks for the info.

Regards,
Alexander

Re: [RFC] irqchip: add support for LS1021A external interrupt lines

[Alexander Stein]

Hi Rasmus,

thanks for your effort. unfortunatly I won't be able to test it currently :(
But some comments below.

On Friday, December 8, 2017, 3:33:00 PM CET Rasmus Villemoes wrote:
> The LS1021A allows inverting the polarity of six interrupt lines
> IRQ[0:5] via the scfg_intpcr register, effectively allowing
> IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_EDGE_FALLING for those. We just need to
> check the type, set the relevant bit in INTPCR accordingly, and fixup
> the type argument before calling the GIC's irq_set_type.
> 
> In fact, the power-on-reset value of the INTPCR register is so that all
> six lines have their polarity inverted. Hence any hardware connected to
> those lines is unusable without this: If the line is indeed active low,
> the generic GIC code will reject an irq spec with IRQ_TYPE_LEVEL_LOW,
> while if the line is active high, we must obviously disable the polarity
> inversion before unmasking the interrupt.
> 
> I suspect other layerscape SOCs may have something similar, but I have
> neither hardware nor documentation.
> 
> Since we only need to keep a single pointer in the chip_data (the syscon
> regmap), the code could be a little simpler by dropping the struct
> extirq_chip_data and just store the regmap directly - but I don't know
> if I do need to add a lock or something else to the chip_data, so for
> this RFC I've kept the struct.
> 
> Signed-off-by: Rasmus Villemoes 
> ---
> Marc, Alexander, thanks a lot for your hints. This is what I came up
> with, mostly just copy-pasted from the mtk-sysirq case. I've tested
> that it works as expected on my board.
> 
>  .../interrupt-controller/fsl,ls1021a-extirq.txt|  19 +++
>  drivers/irqchip/Makefile   |   1 +
>  drivers/irqchip/irq-ls1021a.c  | 157 
> +
>  3 files changed, 177 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
>  create mode 100644 drivers/irqchip/irq-ls1021a.c
> 
> diff --git 
> a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
>  
> b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
> new file mode 100644
> index ..53b04b6e1a80
> --- /dev/null
> +++ 
> b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
> @@ -0,0 +1,19 @@
> +* Freescale LS1021A external IRQs
> +
> +The LS1021A supports inverting the polarity of six external interrupt lines.
> +
> +Required properties:
> +- compatible: should be "fsl,ls1021a-extirq"
> +- interrupt-controller: Identifies the node as an interrupt controller
> +- #interrupt-cells: Use the same format as specified by GIC in arm,gic.txt.

Do you really need 3 interrupt-cells here? As you've written below you don't
support PPI anyway the 1st flag might be dropped then. So support just 2 cells:
* IRQ number (IRQ0 - IRQ5)
* IRQ flags

> +- interrupt-parent: phandle of GIC.
> +- syscon: phandle of Supplemental Configuration Unit (scfg).
> +
> +Example:
> + extirq: interrupt-controller@15701ac {
> + compatible = "fsl,ls1021a-extirq";
> + #interrupt-cells = <3>;
> + interrupt-controller;
> + interrupt-parent = <>;
> + syscon = <>;
> + };
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b842dfdc903f..d4576dce24b2 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -80,3 +80,4 @@ obj-$(CONFIG_ARCH_ASPEED)   += irq-aspeed-vic.o 
> irq-aspeed-i2c-ic.o
>  obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
>  obj-$(CONFIG_QCOM_IRQ_COMBINER)  += qcom-irq-combiner.o
>  obj-$(CONFIG_IRQ_UNIPHIER_AIDET) += irq-uniphier-aidet.o
> +obj-$(CONFIG_SOC_LS1021A)+= irq-ls1021a.o

I guess this should be kept sorted alphabetically.

> diff --git a/drivers/irqchip/irq-ls1021a.c b/drivers/irqchip/irq-ls1021a.c
> new file mode 100644
> index ..2ec4fc023549
> --- /dev/null
> +++ b/drivers/irqchip/irq-ls1021a.c
> @@ -0,0 +1,157 @@
> +#define pr_fmt(fmt) "irq-ls1021a: " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define INTPCR_REG 0x01ac
> +#define NIRQ 6
> +
> +struct extirq_chip_data {
> + struct regmap *syscon;
> +};
> +
> +static int
> +ls1021a_extirq_set_type(struct irq_data *data, unsigned int type)
> +{
> + irq_hw_number_t hwirq = data->hwirq;
> + struct extirq_chip_data *chip_data = data->chip_data;
> + u32 value, mask;
> + int ret;
> +
> + mask = 1U << (31 - hwirq);

Is this really correct? IRQ0 is still at bit position 0. Don't be mislead
by the left most position in the register layout. This is just strange way
to express bit-endian access.
Anyway, please use BIT(x) instead.

> + if (type == IRQ_TYPE_LEVEL_LOW || type == 

Re: [RFC] irqchip: add support for LS1021A external interrupt lines

[Alexander Stein]

Hi Rasmus,

thanks for your effort. unfortunatly I won't be able to test it currently :(
But some comments below.

On Friday, December 8, 2017, 3:33:00 PM CET Rasmus Villemoes wrote:
> The LS1021A allows inverting the polarity of six interrupt lines
> IRQ[0:5] via the scfg_intpcr register, effectively allowing
> IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_EDGE_FALLING for those. We just need to
> check the type, set the relevant bit in INTPCR accordingly, and fixup
> the type argument before calling the GIC's irq_set_type.
> 
> In fact, the power-on-reset value of the INTPCR register is so that all
> six lines have their polarity inverted. Hence any hardware connected to
> those lines is unusable without this: If the line is indeed active low,
> the generic GIC code will reject an irq spec with IRQ_TYPE_LEVEL_LOW,
> while if the line is active high, we must obviously disable the polarity
> inversion before unmasking the interrupt.
> 
> I suspect other layerscape SOCs may have something similar, but I have
> neither hardware nor documentation.
> 
> Since we only need to keep a single pointer in the chip_data (the syscon
> regmap), the code could be a little simpler by dropping the struct
> extirq_chip_data and just store the regmap directly - but I don't know
> if I do need to add a lock or something else to the chip_data, so for
> this RFC I've kept the struct.
> 
> Signed-off-by: Rasmus Villemoes 
> ---
> Marc, Alexander, thanks a lot for your hints. This is what I came up
> with, mostly just copy-pasted from the mtk-sysirq case. I've tested
> that it works as expected on my board.
> 
>  .../interrupt-controller/fsl,ls1021a-extirq.txt|  19 +++
>  drivers/irqchip/Makefile   |   1 +
>  drivers/irqchip/irq-ls1021a.c  | 157 
> +
>  3 files changed, 177 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
>  create mode 100644 drivers/irqchip/irq-ls1021a.c
> 
> diff --git 
> a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
>  
> b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
> new file mode 100644
> index ..53b04b6e1a80
> --- /dev/null
> +++ 
> b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
> @@ -0,0 +1,19 @@
> +* Freescale LS1021A external IRQs
> +
> +The LS1021A supports inverting the polarity of six external interrupt lines.
> +
> +Required properties:
> +- compatible: should be "fsl,ls1021a-extirq"
> +- interrupt-controller: Identifies the node as an interrupt controller
> +- #interrupt-cells: Use the same format as specified by GIC in arm,gic.txt.

Do you really need 3 interrupt-cells here? As you've written below you don't
support PPI anyway the 1st flag might be dropped then. So support just 2 cells:
* IRQ number (IRQ0 - IRQ5)
* IRQ flags

> +- interrupt-parent: phandle of GIC.
> +- syscon: phandle of Supplemental Configuration Unit (scfg).
> +
> +Example:
> + extirq: interrupt-controller@15701ac {
> + compatible = "fsl,ls1021a-extirq";
> + #interrupt-cells = <3>;
> + interrupt-controller;
> + interrupt-parent = <>;
> + syscon = <>;
> + };
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b842dfdc903f..d4576dce24b2 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -80,3 +80,4 @@ obj-$(CONFIG_ARCH_ASPEED)   += irq-aspeed-vic.o 
> irq-aspeed-i2c-ic.o
>  obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
>  obj-$(CONFIG_QCOM_IRQ_COMBINER)  += qcom-irq-combiner.o
>  obj-$(CONFIG_IRQ_UNIPHIER_AIDET) += irq-uniphier-aidet.o
> +obj-$(CONFIG_SOC_LS1021A)+= irq-ls1021a.o

I guess this should be kept sorted alphabetically.

> diff --git a/drivers/irqchip/irq-ls1021a.c b/drivers/irqchip/irq-ls1021a.c
> new file mode 100644
> index ..2ec4fc023549
> --- /dev/null
> +++ b/drivers/irqchip/irq-ls1021a.c
> @@ -0,0 +1,157 @@
> +#define pr_fmt(fmt) "irq-ls1021a: " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define INTPCR_REG 0x01ac
> +#define NIRQ 6
> +
> +struct extirq_chip_data {
> + struct regmap *syscon;
> +};
> +
> +static int
> +ls1021a_extirq_set_type(struct irq_data *data, unsigned int type)
> +{
> + irq_hw_number_t hwirq = data->hwirq;
> + struct extirq_chip_data *chip_data = data->chip_data;
> + u32 value, mask;
> + int ret;
> +
> + mask = 1U << (31 - hwirq);

Is this really correct? IRQ0 is still at bit position 0. Don't be mislead
by the left most position in the register layout. This is just strange way
to express bit-endian access.
Anyway, please use BIT(x) instead.

> + if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING) {
> +  

Re: polarity inversion on LS1021a

[Alexander Stein]

On Monday, December 4, 2017, 4:37:20 PM CET Marc Zyngier wrote:
> On 04/12/17 15:31, Alexander Stein wrote:
> > On Monday, December 4, 2017, 4:11:06 PM CET Rasmus Villemoes wrote:
> >> The LS1021A has a standard GIC-400, but allows inverting the polarity of
> >> six external interrupt lines via a certain register, effectively
> >> supporting IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_EDGE_FALLING for those.
> >>
> >> I'm trying to figure out how one would add support for this. The patch
> >> below works but is obviously just meant to help show what I mean, so
> >> please don't comment on all the things that are wrong with it.
> >>
> >> It feels wrong to create a whole new irqchip driver copy-pasting the
> >> entire irg-gic.c, but I can't figure out how and where one could hook
> >> into the existing one. Any pointers on how to do this properly will be
> >> greatly appreciated.
> > 
> > In my opinion a new irqchip is still required, but solely for modifying
> > SCFG_INTPCR depending on IRQ_TYPE_*
> > You would need to insert it as a cascading interrupt chip in device tree.
> > You also need to protect accesses to this register using a spinlock.
> > This is at least my idea how I would have done it, though never got time
> > for it.
> Almost. See my earlier reply. You just need a very minimal driver that
> only takes care of the polarity thing. Nobody needs to see yet another
> GIC driver...

Ah, maybe I should have made that more clear. Of course only a rather simple
driver for that single register is needed. The driver needs to handle the
infamous big-little-endian mismatch in ls1021a.

Best regards,
Alexander

Re: polarity inversion on LS1021a

[Alexander Stein]

On Monday, December 4, 2017, 4:37:20 PM CET Marc Zyngier wrote:
> On 04/12/17 15:31, Alexander Stein wrote:
> > On Monday, December 4, 2017, 4:11:06 PM CET Rasmus Villemoes wrote:
> >> The LS1021A has a standard GIC-400, but allows inverting the polarity of
> >> six external interrupt lines via a certain register, effectively
> >> supporting IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_EDGE_FALLING for those.
> >>
> >> I'm trying to figure out how one would add support for this. The patch
> >> below works but is obviously just meant to help show what I mean, so
> >> please don't comment on all the things that are wrong with it.
> >>
> >> It feels wrong to create a whole new irqchip driver copy-pasting the
> >> entire irg-gic.c, but I can't figure out how and where one could hook
> >> into the existing one. Any pointers on how to do this properly will be
> >> greatly appreciated.
> > 
> > In my opinion a new irqchip is still required, but solely for modifying
> > SCFG_INTPCR depending on IRQ_TYPE_*
> > You would need to insert it as a cascading interrupt chip in device tree.
> > You also need to protect accesses to this register using a spinlock.
> > This is at least my idea how I would have done it, though never got time
> > for it.
> Almost. See my earlier reply. You just need a very minimal driver that
> only takes care of the polarity thing. Nobody needs to see yet another
> GIC driver...

Ah, maybe I should have made that more clear. Of course only a rather simple
driver for that single register is needed. The driver needs to handle the
infamous big-little-endian mismatch in ls1021a.

Best regards,
Alexander

Re: polarity inversion on LS1021a

[Alexander Stein]

Hi Rasmus,

On Monday, December 4, 2017, 4:11:06 PM CET Rasmus Villemoes wrote:
> The LS1021A has a standard GIC-400, but allows inverting the polarity of
> six external interrupt lines via a certain register, effectively
> supporting IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_EDGE_FALLING for those.
> 
> I'm trying to figure out how one would add support for this. The patch
> below works but is obviously just meant to help show what I mean, so
> please don't comment on all the things that are wrong with it.
> 
> It feels wrong to create a whole new irqchip driver copy-pasting the
> entire irg-gic.c, but I can't figure out how and where one could hook
> into the existing one. Any pointers on how to do this properly will be
> greatly appreciated.

In my opinion a new irqchip is still required, but solely for modifying
SCFG_INTPCR depending on IRQ_TYPE_*
You would need to insert it as a cascading interrupt chip in device tree.
You also need to protect accesses to this register using a spinlock.
This is at least my idea how I would have done it, though never got time
for it.

Best regards,
Alexander

Re: polarity inversion on LS1021a

[Alexander Stein]

Hi Rasmus,

On Monday, December 4, 2017, 4:11:06 PM CET Rasmus Villemoes wrote:
> The LS1021A has a standard GIC-400, but allows inverting the polarity of
> six external interrupt lines via a certain register, effectively
> supporting IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_EDGE_FALLING for those.
> 
> I'm trying to figure out how one would add support for this. The patch
> below works but is obviously just meant to help show what I mean, so
> please don't comment on all the things that are wrong with it.
> 
> It feels wrong to create a whole new irqchip driver copy-pasting the
> entire irg-gic.c, but I can't figure out how and where one could hook
> into the existing one. Any pointers on how to do this properly will be
> greatly appreciated.

In my opinion a new irqchip is still required, but solely for modifying
SCFG_INTPCR depending on IRQ_TYPE_*
You would need to insert it as a cascading interrupt chip in device tree.
You also need to protect accesses to this register using a spinlock.
This is at least my idea how I would have done it, though never got time
for it.

Best regards,
Alexander

tools/iio: build race condition

[Alexander Stein]

Hi,

I tried to compile the new v4.14 kernel and hit a race condition in the build 
system of tools/iio.
Here is my output (sorry for German strings, I wasn't able to reproduce with 
LANG=C):
> {master linux} % make O=build_x86/ -j9 tools/iio
> make[1]: Verzeichnis „/home/alex/Dokumente/repo/linux/build_x86“ wird betreten
>   DESCEND  iio
> mkdir -p 
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/include/linux/iio 2>&1 || 
> true
> ln -sf 
> /home/alex/Dokumente/repo/linux/tools/iio/../../include/uapi/linux/iio/events.h
>  /home/alex/Dokumente/repo/linux/build_x86/tools/iio/include/linux/iio
> ln -sf 
> /home/alex/Dokumente/repo/linux/tools/iio/../../include/uapi/linux/iio/types.h
>  /home/alex/Dokumente/repo/linux/build_x86/tools/iio/include/linux/iio
>   CC   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/lsiio.o
>   CC   
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_event_monitor.o
>   CC   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_utils.o
>   CC   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_utils.o
>   CC   
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_generic_buffer.o
>   CC   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_utils.o
>   LD   
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_event_monitor-in.o
>   LD   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/lsiio-in.o
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_utils.o: file not 
> recognized: File truncated
> make[4]: *** [/home/alex/Dokumente/repo/linux/tools/build/Makefile.build:145: 
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_event_monitor-in.o] 
> Fehler 1
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_utils.o: file not 
> recognized: File truncated
> make[3]: *** [Makefile:45: 
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_event_monitor-in.o] 
> Fehler 2
> make[3]: *** Es wird auf noch nicht beendete Prozesse gewartet
> make[4]: *** [/home/alex/Dokumente/repo/linux/tools/build/Makefile.build:145: 
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/lsiio-in.o] Fehler 1
> make[3]: *** [Makefile:39: 
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/lsiio-in.o] Fehler 2
>   LD   
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_generic_buffer-in.o
> make[2]: *** [Makefile:62: iio] Fehler 2
> make[1]: *** [/home/alex/Dokumente/repo/linux/Makefile:1628: tools/iio] 
> Fehler 2
> make[1]: Verzeichnis „/home/alex/Dokumente/repo/linux/build_x86“ wird 
> verlassen
> make: *** [Makefile:146: sub-make] Fehler 2

This is my log using LANG=C:
> {master linux} % git describe
> v4.14-3-gd893dbcff8e3
> {master linux} % rm -fr build_x86/tools/
> {master linux} % LANG=C make O=build_x86/ -j9 tools/iio
> make[1]: Entering directory '/home/alex/Dokumente/repo/linux/build_x86'
>   DESCEND  iio
> mkdir -p 
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/include/linux/iio 2>&1 || 
> true
> ln -sf 
> /home/alex/Dokumente/repo/linux/tools/iio/../../include/uapi/linux/iio/events.h
>  /home/alex/Dokumente/repo/linux/build_x86/tools/iio/include/linux/iio
> ln -sf 
> /home/alex/Dokumente/repo/linux/tools/iio/../../include/uapi/linux/iio/types.h
>  /home/alex/Dokumente/repo/linux/build_x86/tools/iio/include/linux/iio
>   CC   
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_event_monitor.o
>   CC   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_utils.o
>   CC   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/lsiio.o
>   CC   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_utils.o
>   CC   
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_generic_buffer.o
>   CC   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_utils.o
>   LD   
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_event_monitor-in.o
>   LINK 
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_event_monitor
>   LD   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/lsiio-in.o
>   LD   
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_generic_buffer-in.o
>   LINK /home/alex/Dokumente/repo/linux/build_x86/tools/iio/lsiio
>   LINK 
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_generic_buffer
> make[1]: Leaving directory '/home/alex/Dokumente/repo/linux/build_x86'

In both cases tools/iio/iio_utils.o is compiled thrice. Depending on the exact 
timing you might get a truncated file.
I suspect the following commit: 18956cf2d78a ("iio: tools: move to tools 
buildsystem", 2017-07-29)
as iio_utils.o is referenced in all 3 tools.

Best regards,
Alexander

tools/iio: build race condition

[Alexander Stein]

Hi,

I tried to compile the new v4.14 kernel and hit a race condition in the build 
system of tools/iio.
Here is my output (sorry for German strings, I wasn't able to reproduce with 
LANG=C):
> {master linux} % make O=build_x86/ -j9 tools/iio
> make[1]: Verzeichnis „/home/alex/Dokumente/repo/linux/build_x86“ wird betreten
>   DESCEND  iio
> mkdir -p 
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/include/linux/iio 2>&1 || 
> true
> ln -sf 
> /home/alex/Dokumente/repo/linux/tools/iio/../../include/uapi/linux/iio/events.h
>  /home/alex/Dokumente/repo/linux/build_x86/tools/iio/include/linux/iio
> ln -sf 
> /home/alex/Dokumente/repo/linux/tools/iio/../../include/uapi/linux/iio/types.h
>  /home/alex/Dokumente/repo/linux/build_x86/tools/iio/include/linux/iio
>   CC   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/lsiio.o
>   CC   
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_event_monitor.o
>   CC   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_utils.o
>   CC   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_utils.o
>   CC   
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_generic_buffer.o
>   CC   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_utils.o
>   LD   
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_event_monitor-in.o
>   LD   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/lsiio-in.o
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_utils.o: file not 
> recognized: File truncated
> make[4]: *** [/home/alex/Dokumente/repo/linux/tools/build/Makefile.build:145: 
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_event_monitor-in.o] 
> Fehler 1
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_utils.o: file not 
> recognized: File truncated
> make[3]: *** [Makefile:45: 
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_event_monitor-in.o] 
> Fehler 2
> make[3]: *** Es wird auf noch nicht beendete Prozesse gewartet
> make[4]: *** [/home/alex/Dokumente/repo/linux/tools/build/Makefile.build:145: 
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/lsiio-in.o] Fehler 1
> make[3]: *** [Makefile:39: 
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/lsiio-in.o] Fehler 2
>   LD   
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_generic_buffer-in.o
> make[2]: *** [Makefile:62: iio] Fehler 2
> make[1]: *** [/home/alex/Dokumente/repo/linux/Makefile:1628: tools/iio] 
> Fehler 2
> make[1]: Verzeichnis „/home/alex/Dokumente/repo/linux/build_x86“ wird 
> verlassen
> make: *** [Makefile:146: sub-make] Fehler 2

This is my log using LANG=C:
> {master linux} % git describe
> v4.14-3-gd893dbcff8e3
> {master linux} % rm -fr build_x86/tools/
> {master linux} % LANG=C make O=build_x86/ -j9 tools/iio
> make[1]: Entering directory '/home/alex/Dokumente/repo/linux/build_x86'
>   DESCEND  iio
> mkdir -p 
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/include/linux/iio 2>&1 || 
> true
> ln -sf 
> /home/alex/Dokumente/repo/linux/tools/iio/../../include/uapi/linux/iio/events.h
>  /home/alex/Dokumente/repo/linux/build_x86/tools/iio/include/linux/iio
> ln -sf 
> /home/alex/Dokumente/repo/linux/tools/iio/../../include/uapi/linux/iio/types.h
>  /home/alex/Dokumente/repo/linux/build_x86/tools/iio/include/linux/iio
>   CC   
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_event_monitor.o
>   CC   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_utils.o
>   CC   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/lsiio.o
>   CC   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_utils.o
>   CC   
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_generic_buffer.o
>   CC   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_utils.o
>   LD   
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_event_monitor-in.o
>   LINK 
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_event_monitor
>   LD   /home/alex/Dokumente/repo/linux/build_x86/tools/iio/lsiio-in.o
>   LD   
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_generic_buffer-in.o
>   LINK /home/alex/Dokumente/repo/linux/build_x86/tools/iio/lsiio
>   LINK 
> /home/alex/Dokumente/repo/linux/build_x86/tools/iio/iio_generic_buffer
> make[1]: Leaving directory '/home/alex/Dokumente/repo/linux/build_x86'

In both cases tools/iio/iio_utils.o is compiled thrice. Depending on the exact 
timing you might get a truncated file.
I suspect the following commit: 18956cf2d78a ("iio: tools: move to tools 
buildsystem", 2017-07-29)
as iio_utils.o is referenced in all 3 tools.

Best regards,
Alexander

Re: [tpmdd-devel] [PATCH v2] tpm_tis: fix stall after iowrite*()s

[Alexander Stein]

On Wednesday 16 August 2017 17:15:55, Ken Goldman wrote:
> On 8/15/2017 4:13 PM, Haris Okanovic wrote:
> > ioread8() operations to TPM MMIO addresses can stall the cpu when
> > immediately following a sequence of iowrite*()'s to the same region.
> > 
> > For example, cyclitest measures ~400us latency spikes when a non-RT
> > usermode application communicates with an SPI-based TPM chip (Intel Atom
> > E3940 system, PREEMPT_RT_FULL kernel). The spikes are caused by a
> > stalling ioread8() operation following a sequence of 30+ iowrite8()s to
> > the same address. I believe this happens because the write sequence is
> > buffered (in cpu or somewhere along the bus), and gets flushed on the
> > first LOAD instruction (ioread*()) that follows.
> > 
> > The enclosed change appears to fix this issue: read the TPM chip's
> > access register (status code) after every iowrite*() operation to
> > amortize the cost of flushing data to chip across multiple instructions.
> 
> I worry a bit about "appears to fix".  It seems odd that the TPM device
> driver would be the first code to uncover this.  Can anyone confirm that
> the chipset does indeed have this bug?

No, there was already a similar problem in e1000e where a PCIe read stalled 
the CPU, hence no interrupts are serviced. See 
https://www.spinics.net/lists/linux-rt-users/msg14077.html
AFAIK there was no outcome though.

> I'd also like an indication of the performance penalty.  We're doing a
> lot of work to improve the performance and I worry that "do a read after
> every write" will have a performance impact.

Realtime will always affect performance, but IMHO the latter is much more 
important.

Best regards,
Alexander

Re: [tpmdd-devel] [PATCH v2] tpm_tis: fix stall after iowrite*()s

[Alexander Stein]

On Wednesday 16 August 2017 17:15:55, Ken Goldman wrote:
> On 8/15/2017 4:13 PM, Haris Okanovic wrote:
> > ioread8() operations to TPM MMIO addresses can stall the cpu when
> > immediately following a sequence of iowrite*()'s to the same region.
> > 
> > For example, cyclitest measures ~400us latency spikes when a non-RT
> > usermode application communicates with an SPI-based TPM chip (Intel Atom
> > E3940 system, PREEMPT_RT_FULL kernel). The spikes are caused by a
> > stalling ioread8() operation following a sequence of 30+ iowrite8()s to
> > the same address. I believe this happens because the write sequence is
> > buffered (in cpu or somewhere along the bus), and gets flushed on the
> > first LOAD instruction (ioread*()) that follows.
> > 
> > The enclosed change appears to fix this issue: read the TPM chip's
> > access register (status code) after every iowrite*() operation to
> > amortize the cost of flushing data to chip across multiple instructions.
> 
> I worry a bit about "appears to fix".  It seems odd that the TPM device
> driver would be the first code to uncover this.  Can anyone confirm that
> the chipset does indeed have this bug?

No, there was already a similar problem in e1000e where a PCIe read stalled 
the CPU, hence no interrupts are serviced. See 
https://www.spinics.net/lists/linux-rt-users/msg14077.html
AFAIK there was no outcome though.

> I'd also like an indication of the performance penalty.  We're doing a
> lot of work to improve the performance and I worry that "do a read after
> every write" will have a performance impact.

Realtime will always affect performance, but IMHO the latter is much more 
important.

Best regards,
Alexander

Re: [PATCH] tpm_tis: fix stall after iowrite*()s

[Alexander Stein]

On Monday 14 August 2017 17:53:47, Haris Okanovic wrote:
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -52,6 +52,22 @@ static inline struct tpm_tis_tcg_phy
> *to_tpm_tis_tcg_phy(struct tpm_tis_data *da return container_of(data,
> struct tpm_tis_tcg_phy, priv);
>  }
> 
> +static inline void tpm_tis_iowrite8(u8 b, void __iomem *iobase, u32 addr)
> +{
> + iowrite8(b, iobase + addr);
> +#ifdef CONFIG_PREEMPT_RT_FULL
> + ioread8(iobase + TPM_ACCESS(0));
> +#endif
> +}

Maybe add some comment why an iorad8 is actually requried after each write on 
RT. Currently it is rather obvious why this additional read is necessary. But 
is this still the case in a year?

> +static inline void tpm_tis_iowrite32(u32 b, void __iomem *iobase, u32 addr)
> +{
> + iowrite32(b, iobase + addr);
> +#ifdef CONFIG_PREEMPT_RT_FULL
> + ioread8(iobase + TPM_ACCESS(0));
> +#endif
> +}

Same applies here. Or add a comment above both functions describing their 
purpose.

Just my 2 cents

Best regards,
Alexander

Re: [PATCH] tpm_tis: fix stall after iowrite*()s

[Alexander Stein]

On Monday 14 August 2017 17:53:47, Haris Okanovic wrote:
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -52,6 +52,22 @@ static inline struct tpm_tis_tcg_phy
> *to_tpm_tis_tcg_phy(struct tpm_tis_data *da return container_of(data,
> struct tpm_tis_tcg_phy, priv);
>  }
> 
> +static inline void tpm_tis_iowrite8(u8 b, void __iomem *iobase, u32 addr)
> +{
> + iowrite8(b, iobase + addr);
> +#ifdef CONFIG_PREEMPT_RT_FULL
> + ioread8(iobase + TPM_ACCESS(0));
> +#endif
> +}

Maybe add some comment why an iorad8 is actually requried after each write on 
RT. Currently it is rather obvious why this additional read is necessary. But 
is this still the case in a year?

> +static inline void tpm_tis_iowrite32(u32 b, void __iomem *iobase, u32 addr)
> +{
> + iowrite32(b, iobase + addr);
> +#ifdef CONFIG_PREEMPT_RT_FULL
> + ioread8(iobase + TPM_ACCESS(0));
> +#endif
> +}

Same applies here. Or add a comment above both functions describing their 
purpose.

Just my 2 cents

Best regards,
Alexander

Re: [PATCH 4/6] gpio: mxc: use devres for irq generic chip

[Alexander Stein]

Hi,

On Wednesday 02 August 2017 09:51:24, Bartosz Golaszewski wrote:
> Use resource managed variants of irq_alloc_generic_chip() and
> irq_setup_generic_chip().

Is this really useful for drivers which can only be built-in? This is probably 
valid for the other drives as well.

Best regards,
Alexander

Re: [PATCH 4/6] gpio: mxc: use devres for irq generic chip

[Alexander Stein]

Hi,

On Wednesday 02 August 2017 09:51:24, Bartosz Golaszewski wrote:
> Use resource managed variants of irq_alloc_generic_chip() and
> irq_setup_generic_chip().

Is this really useful for drivers which can only be built-in? This is probably 
valid for the other drives as well.

Best regards,
Alexander

Re: [PATCH V2 2/2] timer: imx-tpm: add imx tpm timer support

[Alexander Stein]

On Tuesday 13 June 2017 15:58:45, Dong Aisheng wrote:
> diff --git a/drivers/clocksource/timer-imx-tpm.c
> b/drivers/clocksource/timer-imx-tpm.c new file mode 100644
> index 000..940a4f75
> --- /dev/null
> +++ b/drivers/clocksource/timer-imx-tpm.c
> @@ -0,0 +1,227 @@
> [...]
> +static int tpm_set_next_event(unsigned long delta,
> + struct clock_event_device *evt)
> +{
> + unsigned long next, now;
> +
> + next = readl(timer_base + TPM_CNT) + delta;
> + writel(next, timer_base + TPM_C0V);
> + now = readl(timer_base + TPM_CNT);

What about:
> now = readl(timer_base + TPM_CNT);
> next = now + delta;
> writel(next, timer_base + TPM_C0V);
> return 0;

> + return (int)((next - now) <= 0) ? -ETIME : 0;

Can this error actually happen, even with your implementation?

Best regards,
Alexander

Re: [PATCH V2 2/2] timer: imx-tpm: add imx tpm timer support

[Alexander Stein]

On Tuesday 13 June 2017 15:58:45, Dong Aisheng wrote:
> diff --git a/drivers/clocksource/timer-imx-tpm.c
> b/drivers/clocksource/timer-imx-tpm.c new file mode 100644
> index 000..940a4f75
> --- /dev/null
> +++ b/drivers/clocksource/timer-imx-tpm.c
> @@ -0,0 +1,227 @@
> [...]
> +static int tpm_set_next_event(unsigned long delta,
> + struct clock_event_device *evt)
> +{
> + unsigned long next, now;
> +
> + next = readl(timer_base + TPM_CNT) + delta;
> + writel(next, timer_base + TPM_C0V);
> + now = readl(timer_base + TPM_CNT);

What about:
> now = readl(timer_base + TPM_CNT);
> next = now + delta;
> writel(next, timer_base + TPM_C0V);
> return 0;

> + return (int)((next - now) <= 0) ? -ETIME : 0;

Can this error actually happen, even with your implementation?

Best regards,
Alexander

Re: [PATCH 1/2] clk: bcm2835: Add AUX interrupt controller

[Alexander Stein]

 On Wednesday, June 7, 2017, 2:37:41 PM CEST Phil Elwell wrote:
> On 07/06/2017 13:07, Alexander Stein wrote:
> > On Wednesday 07 June 2017 12:11:45, Phil Elwell wrote:
> >> Devices in the AUX block share a common interrupt line, with a register
> >> indicating which devices have active IRQs. Expose this as a nested
> >> interrupt controller to avoid IRQ sharing problems (easily observed if
> >> UART1 and SPI1/2 are enabled simultaneously).
> >> 
> >> Signed-off-by: Phil Elwell <p...@raspberrypi.org>
> >> ---
> >> 
> >>  drivers/clk/bcm/clk-bcm2835-aux.c | 120
> >> 
> >> ++ 1 file changed, 120 insertions(+)
> >> 
> >> diff --git a/drivers/clk/bcm/clk-bcm2835-aux.c
> >> b/drivers/clk/bcm/clk-bcm2835-aux.c index bd750cf..41e0702 100644
> >> --- a/drivers/clk/bcm/clk-bcm2835-aux.c
> >> +++ b/drivers/clk/bcm/clk-bcm2835-aux.c
> >> [...]
> >> +struct auxirq_state {
> >> +  void __iomem  *status;
> >> +  u32enables;
> >> +  struct irq_domain *domain;
> >> +  struct regmap *local_regmap;
> >> +};
> >> +
> >> +static struct auxirq_state auxirq __read_mostly;
> >> +
> >> +static irqreturn_t bcm2835_auxirq_handler(int irq, void *dev_id)
> >> +{
> >> +  u32 stat = readl_relaxed(auxirq.status);
> >> +  u32 masked = stat & auxirq.enables;
> > 
> > Doesn't this hide any spurious interrupts? Is this acceptable? I mean
> > getting informed about spurious interrupts seems nice to me, as it
> > indicates a hardware/configuration problem.
> 
> Thanks for the reply. This interrupt handler is capable of dispatching
> multiple interrupts but must return a single value - IRQ_HANDLED or
> IRQ_NONE. I've assumed that returning IRQ_NONE repeatedly will trigger the
> spurious interrupt detection.
> 
> This implementation returns IRQ_HANDLED if any unmasked interrupts are
> raised, otherwise it returns IRQ_NONE. Therefore provided any spurious
> interrupt isn't always coincident with a real interrupt then it ought
> eventually to be identified as spurious. The alternative - returning
> IRQ_NONE if there are any spurious interrupts - seems prone to causing
> collateral damage.
> 
> What did you have in mind?

I was wondering about "stat & auxirq.enables". With that you wouldn't forward 
any spurious interrupts to e.g. SPI1. I don't know which way is better, 
returning IRQ_NONE is a masked interrupt happens, or pass it further down. I 
guess this also raises a warning if the SPI driver also returns IRQ_NONE.
BTW: Is it even allowed to call generic_handle_irq on a masked irq?

> >> +  if (masked & BCM2835_AUXIRQ_UART_MASK)
> >> +  generic_handle_irq(irq_linear_revmap(auxirq.domain,
> >> +   BCM2835_AUXIRQ_UART_IRQ));
> >> +
> >> +  if (masked & BCM2835_AUXIRQ_SPI1_MASK)
> >> +  generic_handle_irq(irq_linear_revmap(auxirq.domain,
> >> +   BCM2835_AUXIRQ_SPI1_IRQ));
> >> +
> >> +  if (masked & BCM2835_AUXIRQ_SPI2_MASK)
> >> +  generic_handle_irq(irq_linear_revmap(auxirq.domain,
> >> +   BCM2835_AUXIRQ_SPI2_IRQ));
> >> +
> >> +  return (masked & BCM2835_AUXIRQ_ALL_MASK) ? IRQ_HANDLED : IRQ_NONE;
> >> +}
> > 
> > How does interrupt acknowledgement work in these 3 interrupts work?
> 
> The interrupt "controller" is just combinatorial logic on the three
> level-sensitive interrupt lines from the devices. Interrupts must be
> acknowledged and cleared at source.

Thanks for the info.

Best regards,
Alexander

Re: [PATCH 1/2] clk: bcm2835: Add AUX interrupt controller

[Alexander Stein]

 On Wednesday, June 7, 2017, 2:37:41 PM CEST Phil Elwell wrote:
> On 07/06/2017 13:07, Alexander Stein wrote:
> > On Wednesday 07 June 2017 12:11:45, Phil Elwell wrote:
> >> Devices in the AUX block share a common interrupt line, with a register
> >> indicating which devices have active IRQs. Expose this as a nested
> >> interrupt controller to avoid IRQ sharing problems (easily observed if
> >> UART1 and SPI1/2 are enabled simultaneously).
> >> 
> >> Signed-off-by: Phil Elwell 
> >> ---
> >> 
> >>  drivers/clk/bcm/clk-bcm2835-aux.c | 120
> >> 
> >> ++ 1 file changed, 120 insertions(+)
> >> 
> >> diff --git a/drivers/clk/bcm/clk-bcm2835-aux.c
> >> b/drivers/clk/bcm/clk-bcm2835-aux.c index bd750cf..41e0702 100644
> >> --- a/drivers/clk/bcm/clk-bcm2835-aux.c
> >> +++ b/drivers/clk/bcm/clk-bcm2835-aux.c
> >> [...]
> >> +struct auxirq_state {
> >> +  void __iomem  *status;
> >> +  u32enables;
> >> +  struct irq_domain *domain;
> >> +  struct regmap *local_regmap;
> >> +};
> >> +
> >> +static struct auxirq_state auxirq __read_mostly;
> >> +
> >> +static irqreturn_t bcm2835_auxirq_handler(int irq, void *dev_id)
> >> +{
> >> +  u32 stat = readl_relaxed(auxirq.status);
> >> +  u32 masked = stat & auxirq.enables;
> > 
> > Doesn't this hide any spurious interrupts? Is this acceptable? I mean
> > getting informed about spurious interrupts seems nice to me, as it
> > indicates a hardware/configuration problem.
> 
> Thanks for the reply. This interrupt handler is capable of dispatching
> multiple interrupts but must return a single value - IRQ_HANDLED or
> IRQ_NONE. I've assumed that returning IRQ_NONE repeatedly will trigger the
> spurious interrupt detection.
> 
> This implementation returns IRQ_HANDLED if any unmasked interrupts are
> raised, otherwise it returns IRQ_NONE. Therefore provided any spurious
> interrupt isn't always coincident with a real interrupt then it ought
> eventually to be identified as spurious. The alternative - returning
> IRQ_NONE if there are any spurious interrupts - seems prone to causing
> collateral damage.
> 
> What did you have in mind?

I was wondering about "stat & auxirq.enables". With that you wouldn't forward 
any spurious interrupts to e.g. SPI1. I don't know which way is better, 
returning IRQ_NONE is a masked interrupt happens, or pass it further down. I 
guess this also raises a warning if the SPI driver also returns IRQ_NONE.
BTW: Is it even allowed to call generic_handle_irq on a masked irq?

> >> +  if (masked & BCM2835_AUXIRQ_UART_MASK)
> >> +  generic_handle_irq(irq_linear_revmap(auxirq.domain,
> >> +   BCM2835_AUXIRQ_UART_IRQ));
> >> +
> >> +  if (masked & BCM2835_AUXIRQ_SPI1_MASK)
> >> +  generic_handle_irq(irq_linear_revmap(auxirq.domain,
> >> +   BCM2835_AUXIRQ_SPI1_IRQ));
> >> +
> >> +  if (masked & BCM2835_AUXIRQ_SPI2_MASK)
> >> +  generic_handle_irq(irq_linear_revmap(auxirq.domain,
> >> +   BCM2835_AUXIRQ_SPI2_IRQ));
> >> +
> >> +  return (masked & BCM2835_AUXIRQ_ALL_MASK) ? IRQ_HANDLED : IRQ_NONE;
> >> +}
> > 
> > How does interrupt acknowledgement work in these 3 interrupts work?
> 
> The interrupt "controller" is just combinatorial logic on the three
> level-sensitive interrupt lines from the devices. Interrupts must be
> acknowledged and cleared at source.

Thanks for the info.

Best regards,
Alexander

Re: [PATCH 1/2] clk: bcm2835: Add AUX interrupt controller

[Alexander Stein]

On Wednesday 07 June 2017 12:11:45, Phil Elwell wrote:
> Devices in the AUX block share a common interrupt line, with a register
> indicating which devices have active IRQs. Expose this as a nested
> interrupt controller to avoid IRQ sharing problems (easily observed if
> UART1 and SPI1/2 are enabled simultaneously).
> 
> Signed-off-by: Phil Elwell 
> ---
>  drivers/clk/bcm/clk-bcm2835-aux.c | 120
> ++ 1 file changed, 120 insertions(+)
> 
> diff --git a/drivers/clk/bcm/clk-bcm2835-aux.c
> b/drivers/clk/bcm/clk-bcm2835-aux.c index bd750cf..41e0702 100644
> --- a/drivers/clk/bcm/clk-bcm2835-aux.c
> +++ b/drivers/clk/bcm/clk-bcm2835-aux.c
> [...]
> +struct auxirq_state {
> + void __iomem  *status;
> + u32enables;
> + struct irq_domain *domain;
> + struct regmap *local_regmap;
> +};
> +
> +static struct auxirq_state auxirq __read_mostly;
> +
> +static irqreturn_t bcm2835_auxirq_handler(int irq, void *dev_id)
> +{
> + u32 stat = readl_relaxed(auxirq.status);
> + u32 masked = stat & auxirq.enables;

Doesn't this hide any spurious interrupts? Is this acceptable? I mean getting 
informed about spurious interrupts seems nice to me, as it indicates a 
hardware/configuration problem.

> + if (masked & BCM2835_AUXIRQ_UART_MASK)
> + generic_handle_irq(irq_linear_revmap(auxirq.domain,
> +  BCM2835_AUXIRQ_UART_IRQ));
> +
> + if (masked & BCM2835_AUXIRQ_SPI1_MASK)
> + generic_handle_irq(irq_linear_revmap(auxirq.domain,
> +  BCM2835_AUXIRQ_SPI1_IRQ));
> +
> + if (masked & BCM2835_AUXIRQ_SPI2_MASK)
> + generic_handle_irq(irq_linear_revmap(auxirq.domain,
> +  BCM2835_AUXIRQ_SPI2_IRQ));
> +
> + return (masked & BCM2835_AUXIRQ_ALL_MASK) ? IRQ_HANDLED : IRQ_NONE;
> +}

How does interrupt acknowledgement work in these 3 interrupts work?

Best regards,
Alexander

Re: [PATCH 1/2] clk: bcm2835: Add AUX interrupt controller

[Alexander Stein]

On Wednesday 07 June 2017 12:11:45, Phil Elwell wrote:
> Devices in the AUX block share a common interrupt line, with a register
> indicating which devices have active IRQs. Expose this as a nested
> interrupt controller to avoid IRQ sharing problems (easily observed if
> UART1 and SPI1/2 are enabled simultaneously).
> 
> Signed-off-by: Phil Elwell 
> ---
>  drivers/clk/bcm/clk-bcm2835-aux.c | 120
> ++ 1 file changed, 120 insertions(+)
> 
> diff --git a/drivers/clk/bcm/clk-bcm2835-aux.c
> b/drivers/clk/bcm/clk-bcm2835-aux.c index bd750cf..41e0702 100644
> --- a/drivers/clk/bcm/clk-bcm2835-aux.c
> +++ b/drivers/clk/bcm/clk-bcm2835-aux.c
> [...]
> +struct auxirq_state {
> + void __iomem  *status;
> + u32enables;
> + struct irq_domain *domain;
> + struct regmap *local_regmap;
> +};
> +
> +static struct auxirq_state auxirq __read_mostly;
> +
> +static irqreturn_t bcm2835_auxirq_handler(int irq, void *dev_id)
> +{
> + u32 stat = readl_relaxed(auxirq.status);
> + u32 masked = stat & auxirq.enables;

Doesn't this hide any spurious interrupts? Is this acceptable? I mean getting 
informed about spurious interrupts seems nice to me, as it indicates a 
hardware/configuration problem.

> + if (masked & BCM2835_AUXIRQ_UART_MASK)
> + generic_handle_irq(irq_linear_revmap(auxirq.domain,
> +  BCM2835_AUXIRQ_UART_IRQ));
> +
> + if (masked & BCM2835_AUXIRQ_SPI1_MASK)
> + generic_handle_irq(irq_linear_revmap(auxirq.domain,
> +  BCM2835_AUXIRQ_SPI1_IRQ));
> +
> + if (masked & BCM2835_AUXIRQ_SPI2_MASK)
> + generic_handle_irq(irq_linear_revmap(auxirq.domain,
> +  BCM2835_AUXIRQ_SPI2_IRQ));
> +
> + return (masked & BCM2835_AUXIRQ_ALL_MASK) ? IRQ_HANDLED : IRQ_NONE;
> +}

How does interrupt acknowledgement work in these 3 interrupts work?

Best regards,
Alexander

[PATCH 1/1] ARM: imx: Enable REGMAP_MMIO per default

[Alexander Stein]

If unset, the gpc drivers fails to link with this error:
drivers/built-in.o: In function `imx_gpc_probe':
core.c:(.text+0x1e16c): undefined reference to
 `__devm_regmap_init_mmio_clk'

As the gpc driver is built when ARCH_MXC is set, enable REGMAP_MMIO in
this case too.

Signed-off-by: Alexander Stein <alexander.st...@systec-electronic.com>
Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
---
 arch/arm/mach-imx/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index 936c59d0e18b..1b8eead0d113 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -7,6 +7,7 @@ menuconfig ARCH_MXC
select GPIOLIB
select PINCTRL
select PM_OPP if PM
+   select REGMAP_MMIO
select SOC_BUS
select SRAM
help
-- 
2.13.0

[PATCH 1/1] ARM: imx: Enable REGMAP_MMIO per default

[Alexander Stein]

If unset, the gpc drivers fails to link with this error:
drivers/built-in.o: In function `imx_gpc_probe':
core.c:(.text+0x1e16c): undefined reference to
 `__devm_regmap_init_mmio_clk'

As the gpc driver is built when ARCH_MXC is set, enable REGMAP_MMIO in
this case too.

Signed-off-by: Alexander Stein 
Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
---
 arch/arm/mach-imx/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index 936c59d0e18b..1b8eead0d113 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -7,6 +7,7 @@ menuconfig ARCH_MXC
select GPIOLIB
select PINCTRL
select PM_OPP if PM
+   select REGMAP_MMIO
select SOC_BUS
select SRAM
help
-- 
2.13.0

Re: [PATCH v2 1/1] xhci: add USB2 test mode support

[Alexander Stein]

On Wednesday 08 February 2017 16:17:45, Mathias Nyman wrote:
> On 08.02.2017 15:44, Alexander Stein wrote:
> > This patch adds support for USB2 test mode (Test_J, Test_K,
> > Test_SE0_NAK and Test_Packet) per XHCI spec 4.19.6.
> > 
> > USB2 test mode is a required hardware feature for system integrators
> > validating their hardware according to USB spec, regarding signal
> > strength and stuff. It is purely a hardware test feature.
> > 
> > Usually you need an oscilloscope and have to enable those test modes on
> > the hardware. This will send some specific test patterns on D+/D-. There
> > is no report available (in Linux itself) as it is purely externally
> > visible. Regular USB usage is not possible at that time.
> > Anyone (well access to e.g. /dev/bus/usb/001/001 provided) can use it by
> > sending appropriate USB_PORT_FEAT_TEST requests to the hub.
> > The same feature for ehci based hosts is already available at
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drive
> > rs/usb/host/ehci-hub.c#n1267
> > 
> > Signed-off-by: "Wang, Yu" <yu.y.w...@intel.com>
> > Signed-off-by: "Li, Guanglei" <guangleix...@intel.com>
> > Signed-off-by: "Wu, Hao" <hao...@intel.com>
> > Signed-off-by: Alexander Stein <alexander.st...@systec-electronic.com>
> > ---
> 
> I think there's a reworked series of this feature already posted at:
> 
> http://marc.info/?l=linux-usb=148050258916147=2
> 
> But this change log is now better
> I was thinking about adding that to 4.12

Interesting, I was not aware of that. This patchset seems better and cleaner 
than my patch. It even powers off all USB3 _and_ USB2 ports. In my patch only 
USB3 ports are powered off. Also TEST_FORCE_EN seems to be supported.
Feel free to take that series and use my commit message as appropriate.

Best regards,
Alexader
-- 
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.st...@systec-electronic.com

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-0
Fax:+49 (0) 3765 38600-4100
 
Managing Directors:
Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010

Re: [PATCH v2 1/1] xhci: add USB2 test mode support

[Alexander Stein]

On Wednesday 08 February 2017 16:17:45, Mathias Nyman wrote:
> On 08.02.2017 15:44, Alexander Stein wrote:
> > This patch adds support for USB2 test mode (Test_J, Test_K,
> > Test_SE0_NAK and Test_Packet) per XHCI spec 4.19.6.
> > 
> > USB2 test mode is a required hardware feature for system integrators
> > validating their hardware according to USB spec, regarding signal
> > strength and stuff. It is purely a hardware test feature.
> > 
> > Usually you need an oscilloscope and have to enable those test modes on
> > the hardware. This will send some specific test patterns on D+/D-. There
> > is no report available (in Linux itself) as it is purely externally
> > visible. Regular USB usage is not possible at that time.
> > Anyone (well access to e.g. /dev/bus/usb/001/001 provided) can use it by
> > sending appropriate USB_PORT_FEAT_TEST requests to the hub.
> > The same feature for ehci based hosts is already available at
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drive
> > rs/usb/host/ehci-hub.c#n1267
> > 
> > Signed-off-by: "Wang, Yu" 
> > Signed-off-by: "Li, Guanglei" 
> > Signed-off-by: "Wu, Hao" 
> > Signed-off-by: Alexander Stein 
> > ---
> 
> I think there's a reworked series of this feature already posted at:
> 
> http://marc.info/?l=linux-usb=148050258916147=2
> 
> But this change log is now better
> I was thinking about adding that to 4.12

Interesting, I was not aware of that. This patchset seems better and cleaner 
than my patch. It even powers off all USB3 _and_ USB2 ports. In my patch only 
USB3 ports are powered off. Also TEST_FORCE_EN seems to be supported.
Feel free to take that series and use my commit message as appropriate.

Best regards,
Alexader
-- 
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.st...@systec-electronic.com

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-0
Fax:+49 (0) 3765 38600-4100
 
Managing Directors:
Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010

[PATCH v2 1/1] xhci: add USB2 test mode support

[Alexander Stein]

This patch adds support for USB2 test mode (Test_J, Test_K,
Test_SE0_NAK and Test_Packet) per XHCI spec 4.19.6.

USB2 test mode is a required hardware feature for system integrators
validating their hardware according to USB spec, regarding signal
strength and stuff. It is purely a hardware test feature.

Usually you need an oscilloscope and have to enable those test modes on
the hardware. This will send some specific test patterns on D+/D-. There
is no report available (in Linux itself) as it is purely externally
visible. Regular USB usage is not possible at that time.
Anyone (well access to e.g. /dev/bus/usb/001/001 provided) can use it by
sending appropriate USB_PORT_FEAT_TEST requests to the hub.
The same feature for ehci based hosts is already available at
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/host/ehci-hub.c#n1267

Signed-off-by: "Wang, Yu" <yu.y.w...@intel.com>
Signed-off-by: "Li, Guanglei" <guangleix...@intel.com>
Signed-off-by: "Wu, Hao" <hao...@intel.com>
Signed-off-by: Alexander Stein <alexander.st...@systec-electronic.com>
---
Changes in v2:
* Added additional info about USB2 test modes itself into commit message
* Add URL for same test mode feature on ehci hosts.

 drivers/usb/host/xhci-hub.c | 63 -
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 0ef1690..cd561f1 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -879,13 +879,14 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, 
u16 wValue,
int max_ports;
unsigned long flags;
u32 temp, status;
-   int retval = 0;
+   int i, retval = 0;
__le32 __iomem **port_array;
int slot_id;
struct xhci_bus_state *bus_state;
u16 link_state = 0;
u16 wake_mask = 0;
u16 timeout = 0;
+   u16 selector = 0;
 
max_ports = xhci_get_ports(hcd, _array);
bus_state = >bus_state[hcd_index(hcd)];
@@ -959,6 +960,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 
wValue,
link_state = (wIndex & 0xff00) >> 3;
if (wValue == USB_PORT_FEAT_REMOTE_WAKE_MASK)
wake_mask = wIndex & 0xff00;
+   if (wValue == USB_PORT_FEAT_TEST)
+   selector = (wIndex & 0xff00) >> 8;
/* The MSB of wIndex is the U1/U2 timeout */
timeout = (wIndex & 0xff00) >> 8;
wIndex &= 0xff;
@@ -1134,6 +1137,64 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, 
u16 wValue,
temp |= PORT_U2_TIMEOUT(timeout);
writel(temp, port_array[wIndex] + PORTPMSC);
break;
+   case USB_PORT_FEAT_TEST:
+   /* 4.19.6 Port Test Modes (USB2 Test Mode) */
+   if (hcd->speed != HCD_USB2)
+   goto error;
+
+   /* FIXME: Test_Force_Enable case to be implemented */
+   if (selector < TEST_J || selector > TEST_PACKET)
+   goto error;
+
+   /* Disable all Device Slots */
+   for (i = 0; i < MAX_HC_SLOTS; i++) {
+   struct xhci_command *command;
+
+   if (!xhci->dcbaa->dev_context_ptrs[i])
+   continue;
+   command = xhci_alloc_command(xhci, false,
+   false, GFP_ATOMIC);
+   if (!command)
+   return -ENOMEM;
+   if (xhci_queue_slot_control(xhci, command,
+   TRB_DISABLE_SLOT, i)) {
+   xhci_err(xhci,
+   "Disable slot[%d] fail!\n", i);
+   goto error;
+   }
+   xhci_dbg(xhci, "Disable Slot[%d].\n", i);
+   }
+
+   /* Put all ports to the Disable state by clear PP */
+   xhci_dbg(xhci, "Disable all port (PP = 0)\n");
+   for (i = 0; i < max_ports; i++) {
+   temp = readl(port_array[i]);
+   temp &= ~PORT_POWER;
+   writel(temp, port_array[i]);
+   }
+
+   /* Stop the controller */
+   xhci_dbg(xhci, "Stop controller\n");
+   temp = readl(>op_regs-

[PATCH v2 1/1] xhci: add USB2 test mode support

[Alexander Stein]

This patch adds support for USB2 test mode (Test_J, Test_K,
Test_SE0_NAK and Test_Packet) per XHCI spec 4.19.6.

USB2 test mode is a required hardware feature for system integrators
validating their hardware according to USB spec, regarding signal
strength and stuff. It is purely a hardware test feature.

Usually you need an oscilloscope and have to enable those test modes on
the hardware. This will send some specific test patterns on D+/D-. There
is no report available (in Linux itself) as it is purely externally
visible. Regular USB usage is not possible at that time.
Anyone (well access to e.g. /dev/bus/usb/001/001 provided) can use it by
sending appropriate USB_PORT_FEAT_TEST requests to the hub.
The same feature for ehci based hosts is already available at
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/host/ehci-hub.c#n1267

Signed-off-by: "Wang, Yu" 
Signed-off-by: "Li, Guanglei" 
Signed-off-by: "Wu, Hao" 
Signed-off-by: Alexander Stein 
---
Changes in v2:
* Added additional info about USB2 test modes itself into commit message
* Add URL for same test mode feature on ehci hosts.

 drivers/usb/host/xhci-hub.c | 63 -
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 0ef1690..cd561f1 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -879,13 +879,14 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, 
u16 wValue,
int max_ports;
unsigned long flags;
u32 temp, status;
-   int retval = 0;
+   int i, retval = 0;
__le32 __iomem **port_array;
int slot_id;
struct xhci_bus_state *bus_state;
u16 link_state = 0;
u16 wake_mask = 0;
u16 timeout = 0;
+   u16 selector = 0;
 
max_ports = xhci_get_ports(hcd, _array);
bus_state = >bus_state[hcd_index(hcd)];
@@ -959,6 +960,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 
wValue,
link_state = (wIndex & 0xff00) >> 3;
if (wValue == USB_PORT_FEAT_REMOTE_WAKE_MASK)
wake_mask = wIndex & 0xff00;
+   if (wValue == USB_PORT_FEAT_TEST)
+   selector = (wIndex & 0xff00) >> 8;
/* The MSB of wIndex is the U1/U2 timeout */
timeout = (wIndex & 0xff00) >> 8;
wIndex &= 0xff;
@@ -1134,6 +1137,64 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, 
u16 wValue,
temp |= PORT_U2_TIMEOUT(timeout);
writel(temp, port_array[wIndex] + PORTPMSC);
break;
+   case USB_PORT_FEAT_TEST:
+   /* 4.19.6 Port Test Modes (USB2 Test Mode) */
+   if (hcd->speed != HCD_USB2)
+   goto error;
+
+   /* FIXME: Test_Force_Enable case to be implemented */
+   if (selector < TEST_J || selector > TEST_PACKET)
+   goto error;
+
+   /* Disable all Device Slots */
+   for (i = 0; i < MAX_HC_SLOTS; i++) {
+   struct xhci_command *command;
+
+   if (!xhci->dcbaa->dev_context_ptrs[i])
+   continue;
+   command = xhci_alloc_command(xhci, false,
+   false, GFP_ATOMIC);
+   if (!command)
+   return -ENOMEM;
+   if (xhci_queue_slot_control(xhci, command,
+   TRB_DISABLE_SLOT, i)) {
+   xhci_err(xhci,
+   "Disable slot[%d] fail!\n", i);
+   goto error;
+   }
+   xhci_dbg(xhci, "Disable Slot[%d].\n", i);
+   }
+
+   /* Put all ports to the Disable state by clear PP */
+   xhci_dbg(xhci, "Disable all port (PP = 0)\n");
+   for (i = 0; i < max_ports; i++) {
+   temp = readl(port_array[i]);
+   temp &= ~PORT_POWER;
+   writel(temp, port_array[i]);
+   }
+
+   /* Stop the controller */
+   xhci_dbg(xhci, "Stop controller\n");
+   temp = readl(>op_regs->command);
+   temp &= ~CMD_RUN;
+   writel(temp, >op_

Re: [PATCH 1/1] xhci: add USB2 test mode support

[Alexander Stein]

On Wednesday 08 February 2017 13:53:30, Greg Kroah-Hartman wrote:
> On Wed, Feb 08, 2017 at 11:26:35AM +0100, Alexander Stein wrote:
> > This patch adds support for USB2 test mode (Test_J, Test_K,
> > Test_SE0_NAK and Test_Packet) per XHCI spec 4.19.6.
> 
> What does that mean "in English"?  In other words, why do we want this?
> What does it provide for a user?  Why do we care?

USB2 test mode is a required hardware feature for system integrators 
validating their hardware according to USB spec, regarding signal strength and 
stuff. It is purely a hardware test feature.

> > @@ -1134,6 +1137,64 @@ int xhci_hub_control(struct usb_hcd *hcd, u16
> > typeReq, u16 wValue,> 
> > temp |= PORT_U2_TIMEOUT(timeout);
> > writel(temp, port_array[wIndex] + PORTPMSC);
> > break;
> > 
> > +   case USB_PORT_FEAT_TEST:
> > +   /* 4.19.6 Port Test Modes (USB2 Test Mode) */
> > +   if (hcd->speed != HCD_USB2)
> > +   goto error;
> > +
> > +   /* FIXME: Test_Force_Enable case to be implemented */
> > +   if (selector < TEST_J || selector > TEST_PACKET)
> > +   goto error;
> > +
> > +   /* Disable all Device Slots */
> > +   for (i = 0; i < MAX_HC_SLOTS; i++) {
> > +   struct xhci_command *command;
> > +
> > +   if (!xhci->dcbaa->dev_context_ptrs[i])
> > +   continue;
> > +   command = xhci_alloc_command(xhci, false,
> > +   false, GFP_ATOMIC);
> > +   if (!command)
> > +   return -ENOMEM;
> > +   if (xhci_queue_slot_control(xhci, command,
> > +   TRB_DISABLE_SLOT, i)) {
> > +   xhci_err(xhci,
> > +   "Disable slot[%d] fail!\n", i);
> > +   goto error;
> > +   }
> > +   xhci_dbg(xhci, "Disable Slot[%d].\n", i);
> > +   }
> > +
> > +   /* Put all ports to the Disable state by clear PP */
> > +   xhci_dbg(xhci, "Disable all port (PP = 0)\n");
> > +   for (i = 0; i < max_ports; i++) {
> > +   temp = readl(port_array[i]);
> > +   temp &= ~PORT_POWER;
> > +   writel(temp, port_array[i]);
> > +   }
> > +
> > +   /* Stop the controller */
> > +   xhci_dbg(xhci, "Stop controller\n");
> > +   temp = readl(>op_regs->command);
> > +   temp &= ~CMD_RUN;
> > +   writel(temp, >op_regs->command);
> > +
> > +   if (xhci_handshake(>op_regs->status,
> > +   STS_HALT, STS_HALT, XHCI_MAX_HALT_USEC)) {
> > +   xhci_warn(xhci, "Stop controller timeout\n");
> > +   return -ETIMEDOUT;
> > +   }
> > +
> > +   /* Disable runtime PM for test mode */
> > +   pm_runtime_forbid(hcd->self.controller);
> > +
> > +   /* Set PORTPMSC.PTC field for selected test mode */
> > +   xhci_dbg(xhci, "Enter Test Mode: %d\n", selector);
> > +   temp = readl(port_array[wIndex] + PORTPMSC);
> > +   temp |= selector << 28;
> > +   writel(temp, port_array[wIndex] + PORTPMSC);
> > +
> > +   break;
> 
> What does this "test mode" do?  Where does it report the information?
> Who can use it?

Useually you need an oscilloscope and have to enable those test modes on the 
hardware. This will send some specific test patterns on D+/D-. There is no 
report available (in linux itself) as it is purely externaly visible.
Regular USB usage is not possible at that time.
Anyone (well access to e.g. /dev/bus/usb/001/001 provided) can use it by 
sending appropriate USB_PORT_FEAT_TEST requests to the hub.
The same feature for ehci based hosts is already available at 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/host/ehci-hub.c#n1267

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.st...@systec-electronic.com

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-0
Fax:+49 (0) 3765 38600-4100
 
Managing Directors:
Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010

Re: [PATCH 1/1] xhci: add USB2 test mode support

[Alexander Stein]

On Wednesday 08 February 2017 13:53:30, Greg Kroah-Hartman wrote:
> On Wed, Feb 08, 2017 at 11:26:35AM +0100, Alexander Stein wrote:
> > This patch adds support for USB2 test mode (Test_J, Test_K,
> > Test_SE0_NAK and Test_Packet) per XHCI spec 4.19.6.
> 
> What does that mean "in English"?  In other words, why do we want this?
> What does it provide for a user?  Why do we care?

USB2 test mode is a required hardware feature for system integrators 
validating their hardware according to USB spec, regarding signal strength and 
stuff. It is purely a hardware test feature.

> > @@ -1134,6 +1137,64 @@ int xhci_hub_control(struct usb_hcd *hcd, u16
> > typeReq, u16 wValue,> 
> > temp |= PORT_U2_TIMEOUT(timeout);
> > writel(temp, port_array[wIndex] + PORTPMSC);
> > break;
> > 
> > +   case USB_PORT_FEAT_TEST:
> > +   /* 4.19.6 Port Test Modes (USB2 Test Mode) */
> > +   if (hcd->speed != HCD_USB2)
> > +   goto error;
> > +
> > +   /* FIXME: Test_Force_Enable case to be implemented */
> > +   if (selector < TEST_J || selector > TEST_PACKET)
> > +   goto error;
> > +
> > +   /* Disable all Device Slots */
> > +   for (i = 0; i < MAX_HC_SLOTS; i++) {
> > +   struct xhci_command *command;
> > +
> > +   if (!xhci->dcbaa->dev_context_ptrs[i])
> > +   continue;
> > +   command = xhci_alloc_command(xhci, false,
> > +   false, GFP_ATOMIC);
> > +   if (!command)
> > +   return -ENOMEM;
> > +   if (xhci_queue_slot_control(xhci, command,
> > +   TRB_DISABLE_SLOT, i)) {
> > +   xhci_err(xhci,
> > +   "Disable slot[%d] fail!\n", i);
> > +   goto error;
> > +   }
> > +   xhci_dbg(xhci, "Disable Slot[%d].\n", i);
> > +   }
> > +
> > +   /* Put all ports to the Disable state by clear PP */
> > +   xhci_dbg(xhci, "Disable all port (PP = 0)\n");
> > +   for (i = 0; i < max_ports; i++) {
> > +   temp = readl(port_array[i]);
> > +   temp &= ~PORT_POWER;
> > +   writel(temp, port_array[i]);
> > +   }
> > +
> > +   /* Stop the controller */
> > +   xhci_dbg(xhci, "Stop controller\n");
> > +   temp = readl(>op_regs->command);
> > +   temp &= ~CMD_RUN;
> > +   writel(temp, >op_regs->command);
> > +
> > +   if (xhci_handshake(>op_regs->status,
> > +   STS_HALT, STS_HALT, XHCI_MAX_HALT_USEC)) {
> > +   xhci_warn(xhci, "Stop controller timeout\n");
> > +   return -ETIMEDOUT;
> > +   }
> > +
> > +   /* Disable runtime PM for test mode */
> > +   pm_runtime_forbid(hcd->self.controller);
> > +
> > +   /* Set PORTPMSC.PTC field for selected test mode */
> > +   xhci_dbg(xhci, "Enter Test Mode: %d\n", selector);
> > +   temp = readl(port_array[wIndex] + PORTPMSC);
> > +   temp |= selector << 28;
> > +   writel(temp, port_array[wIndex] + PORTPMSC);
> > +
> > +   break;
> 
> What does this "test mode" do?  Where does it report the information?
> Who can use it?

Useually you need an oscilloscope and have to enable those test modes on the 
hardware. This will send some specific test patterns on D+/D-. There is no 
report available (in linux itself) as it is purely externaly visible.
Regular USB usage is not possible at that time.
Anyone (well access to e.g. /dev/bus/usb/001/001 provided) can use it by 
sending appropriate USB_PORT_FEAT_TEST requests to the hub.
The same feature for ehci based hosts is already available at 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/host/ehci-hub.c#n1267

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.st...@systec-electronic.com

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-0
Fax:+49 (0) 3765 38600-4100
 
Managing Directors:
Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010

[PATCH 1/1] xhci: add USB2 test mode support

[Alexander Stein]

This patch adds support for USB2 test mode (Test_J, Test_K,
Test_SE0_NAK and Test_Packet) per XHCI spec 4.19.6.

Signed-off-by: "Wang, Yu" <yu.y.w...@intel.com>
Signed-off-by: "Li, Guanglei" <guangleix...@intel.com>
Signed-off-by: "Wu, Hao" <hao...@intel.com>
Signed-off-by: Alexander Stein <alexander.st...@systec-electronic.com>
---
This patch was taken from 
https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/0001-xhci-add-USB2-test-mode-support.patch
A major difference is that now a xhci_command has to be allocated manually.

 drivers/usb/host/xhci-hub.c | 63 -
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 0ef1690..cd561f1 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -879,13 +879,14 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, 
u16 wValue,
int max_ports;
unsigned long flags;
u32 temp, status;
-   int retval = 0;
+   int i, retval = 0;
__le32 __iomem **port_array;
int slot_id;
struct xhci_bus_state *bus_state;
u16 link_state = 0;
u16 wake_mask = 0;
u16 timeout = 0;
+   u16 selector = 0;
 
max_ports = xhci_get_ports(hcd, _array);
bus_state = >bus_state[hcd_index(hcd)];
@@ -959,6 +960,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 
wValue,
link_state = (wIndex & 0xff00) >> 3;
if (wValue == USB_PORT_FEAT_REMOTE_WAKE_MASK)
wake_mask = wIndex & 0xff00;
+   if (wValue == USB_PORT_FEAT_TEST)
+   selector = (wIndex & 0xff00) >> 8;
/* The MSB of wIndex is the U1/U2 timeout */
timeout = (wIndex & 0xff00) >> 8;
wIndex &= 0xff;
@@ -1134,6 +1137,64 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, 
u16 wValue,
temp |= PORT_U2_TIMEOUT(timeout);
writel(temp, port_array[wIndex] + PORTPMSC);
break;
+   case USB_PORT_FEAT_TEST:
+   /* 4.19.6 Port Test Modes (USB2 Test Mode) */
+   if (hcd->speed != HCD_USB2)
+   goto error;
+
+   /* FIXME: Test_Force_Enable case to be implemented */
+   if (selector < TEST_J || selector > TEST_PACKET)
+   goto error;
+
+   /* Disable all Device Slots */
+   for (i = 0; i < MAX_HC_SLOTS; i++) {
+   struct xhci_command *command;
+
+   if (!xhci->dcbaa->dev_context_ptrs[i])
+   continue;
+   command = xhci_alloc_command(xhci, false,
+   false, GFP_ATOMIC);
+   if (!command)
+   return -ENOMEM;
+   if (xhci_queue_slot_control(xhci, command,
+   TRB_DISABLE_SLOT, i)) {
+   xhci_err(xhci,
+   "Disable slot[%d] fail!\n", i);
+   goto error;
+   }
+   xhci_dbg(xhci, "Disable Slot[%d].\n", i);
+   }
+
+   /* Put all ports to the Disable state by clear PP */
+   xhci_dbg(xhci, "Disable all port (PP = 0)\n");
+   for (i = 0; i < max_ports; i++) {
+   temp = readl(port_array[i]);
+   temp &= ~PORT_POWER;
+   writel(temp, port_array[i]);
+   }
+
+   /* Stop the controller */
+   xhci_dbg(xhci, "Stop controller\n");
+   temp = readl(>op_regs->command);
+   temp &= ~CMD_RUN;
+   writel(temp, >op_regs->command);
+
+   if (xhci_handshake(>op_regs->status,
+   STS_HALT, STS_HALT, XHCI_MAX_HALT_USEC)) {
+   xhci_warn(xhci, "Stop controller timeout\n");
+   return -ETIMEDOUT;
+   }
+
+   /* Disable runtime PM for test mode */
+   pm_runtime_forbid(hcd->self.controller);
+
+   /* Set PORTPMSC.PTC field for selected test mode */
+   xhci_dbg

[PATCH 1/1] xhci: add USB2 test mode support

[Alexander Stein]

This patch adds support for USB2 test mode (Test_J, Test_K,
Test_SE0_NAK and Test_Packet) per XHCI spec 4.19.6.

Signed-off-by: "Wang, Yu" 
Signed-off-by: "Li, Guanglei" 
Signed-off-by: "Wu, Hao" 
Signed-off-by: Alexander Stein 
---
This patch was taken from 
https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/0001-xhci-add-USB2-test-mode-support.patch
A major difference is that now a xhci_command has to be allocated manually.

 drivers/usb/host/xhci-hub.c | 63 -
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 0ef1690..cd561f1 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -879,13 +879,14 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, 
u16 wValue,
int max_ports;
unsigned long flags;
u32 temp, status;
-   int retval = 0;
+   int i, retval = 0;
__le32 __iomem **port_array;
int slot_id;
struct xhci_bus_state *bus_state;
u16 link_state = 0;
u16 wake_mask = 0;
u16 timeout = 0;
+   u16 selector = 0;
 
max_ports = xhci_get_ports(hcd, _array);
bus_state = >bus_state[hcd_index(hcd)];
@@ -959,6 +960,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 
wValue,
link_state = (wIndex & 0xff00) >> 3;
if (wValue == USB_PORT_FEAT_REMOTE_WAKE_MASK)
wake_mask = wIndex & 0xff00;
+   if (wValue == USB_PORT_FEAT_TEST)
+   selector = (wIndex & 0xff00) >> 8;
/* The MSB of wIndex is the U1/U2 timeout */
timeout = (wIndex & 0xff00) >> 8;
wIndex &= 0xff;
@@ -1134,6 +1137,64 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, 
u16 wValue,
temp |= PORT_U2_TIMEOUT(timeout);
writel(temp, port_array[wIndex] + PORTPMSC);
break;
+   case USB_PORT_FEAT_TEST:
+   /* 4.19.6 Port Test Modes (USB2 Test Mode) */
+   if (hcd->speed != HCD_USB2)
+   goto error;
+
+   /* FIXME: Test_Force_Enable case to be implemented */
+   if (selector < TEST_J || selector > TEST_PACKET)
+   goto error;
+
+   /* Disable all Device Slots */
+   for (i = 0; i < MAX_HC_SLOTS; i++) {
+   struct xhci_command *command;
+
+   if (!xhci->dcbaa->dev_context_ptrs[i])
+   continue;
+   command = xhci_alloc_command(xhci, false,
+   false, GFP_ATOMIC);
+   if (!command)
+   return -ENOMEM;
+   if (xhci_queue_slot_control(xhci, command,
+   TRB_DISABLE_SLOT, i)) {
+   xhci_err(xhci,
+   "Disable slot[%d] fail!\n", i);
+   goto error;
+   }
+   xhci_dbg(xhci, "Disable Slot[%d].\n", i);
+   }
+
+   /* Put all ports to the Disable state by clear PP */
+   xhci_dbg(xhci, "Disable all port (PP = 0)\n");
+   for (i = 0; i < max_ports; i++) {
+   temp = readl(port_array[i]);
+   temp &= ~PORT_POWER;
+   writel(temp, port_array[i]);
+   }
+
+   /* Stop the controller */
+   xhci_dbg(xhci, "Stop controller\n");
+   temp = readl(>op_regs->command);
+   temp &= ~CMD_RUN;
+   writel(temp, >op_regs->command);
+
+   if (xhci_handshake(>op_regs->status,
+   STS_HALT, STS_HALT, XHCI_MAX_HALT_USEC)) {
+   xhci_warn(xhci, "Stop controller timeout\n");
+   return -ETIMEDOUT;
+   }
+
+   /* Disable runtime PM for test mode */
+   pm_runtime_forbid(hcd->self.controller);
+
+   /* Set PORTPMSC.PTC field for selected test mode */
+   xhci_dbg(xhci, "Enter Test Mode: %d\n", selector);
+   temp = readl(port_array[wIndex] + POR

Re: [PATCH v2 2/2] arm: perf: Mark as non-removable

[Alexander Stein]

Hi,

On Thursday 22 December 2016 22:48:32, Mark Rutland wrote:
> On Wed, Dec 21, 2016 at 04:03:40PM +0100, Alexander Stein wrote:
> > This driver can only built into the kernel. So disallow driver bind/unbind
> > and also prevent a kernel error in case DEBUG_TEST_DRIVER_REMOVE is
> > enabled.
> > 
> > Signed-off-by: Alexander Stein <alexander.st...@systec-electronic.com>
> > ---
> > 
> >  arch/arm/kernel/perf_event_v7.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm/kernel/perf_event_v7.c
> > b/arch/arm/kernel/perf_event_v7.c index b942349..795e373 100644
> > --- a/arch/arm/kernel/perf_event_v7.c
> > +++ b/arch/arm/kernel/perf_event_v7.c
> > @@ -2029,6 +2029,7 @@ static int armv7_pmu_device_probe(struct
> > platform_device *pdev)> 
> >  static struct platform_driver armv7_pmu_driver = {
> >  
> > .driver = {
> > 
> > .name   = "armv7-pmu",
> > 
> > +   .suppress_bind_attrs = true,
> > 
> > .of_match_table = armv7_pmu_of_device_ids,
> > 
> > },
> 
> While this patch looks correct, the other perf_event_* drivers (e.g. those
> under arch/arm/) will need similar treatment.

Yep, but this is the only one I can actually test.

> More generally, updating each and every driver in this manner seems like a
> scattergun approach that is tiresome and error prone.
> 
> IMO, it would be vastly better for a higher layer to enforce that we don't
> attempt to unbind drivers where the driver does not have a remove callback,
> as is the case here (and I suspect most over cases where
> DEBUG_TEST_DRIVER_REMOVE is blowing up).

You mean something like this?
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index 4eabfe2..3b6c1a2d 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -158,6 +158,9 @@ int driver_register(struct device_driver *drv)
> 
> printk(KERN_WARNING "Driver '%s' needs updating - please use
> "
> 
> "bus_type methods\n", drv->name);
> 
> +   if (!drv->remove)
> +   drv->suppress_bind_attrs = true;
> +
> 
> other = driver_find(drv->name, drv->bus);
> if (other) {
> 
> printk(KERN_ERR "Error: Driver '%s' is already registered, "

> Is there any reason that can't be enforced at the bus layer, say?

I'm not sure if the change above works with remove functions set in struct 
bus_type too.
But on the other hand this would hide errors in drivers which are actually 
removable but do not cleanup properly which DEBUG_TEST_DRIVER_REMOVE tries to 
detect. By setting .suppress_bind_attrs = true explicitely you state "This 
driver cannot be removed!", so the remove callback is not missing by accident.

Best regards,
Alexander

Re: [PATCH v2 2/2] arm: perf: Mark as non-removable

[Alexander Stein]

Hi,

On Thursday 22 December 2016 22:48:32, Mark Rutland wrote:
> On Wed, Dec 21, 2016 at 04:03:40PM +0100, Alexander Stein wrote:
> > This driver can only built into the kernel. So disallow driver bind/unbind
> > and also prevent a kernel error in case DEBUG_TEST_DRIVER_REMOVE is
> > enabled.
> > 
> > Signed-off-by: Alexander Stein 
> > ---
> > 
> >  arch/arm/kernel/perf_event_v7.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm/kernel/perf_event_v7.c
> > b/arch/arm/kernel/perf_event_v7.c index b942349..795e373 100644
> > --- a/arch/arm/kernel/perf_event_v7.c
> > +++ b/arch/arm/kernel/perf_event_v7.c
> > @@ -2029,6 +2029,7 @@ static int armv7_pmu_device_probe(struct
> > platform_device *pdev)> 
> >  static struct platform_driver armv7_pmu_driver = {
> >  
> > .driver = {
> > 
> > .name   = "armv7-pmu",
> > 
> > +   .suppress_bind_attrs = true,
> > 
> > .of_match_table = armv7_pmu_of_device_ids,
> > 
> > },
> 
> While this patch looks correct, the other perf_event_* drivers (e.g. those
> under arch/arm/) will need similar treatment.

Yep, but this is the only one I can actually test.

> More generally, updating each and every driver in this manner seems like a
> scattergun approach that is tiresome and error prone.
> 
> IMO, it would be vastly better for a higher layer to enforce that we don't
> attempt to unbind drivers where the driver does not have a remove callback,
> as is the case here (and I suspect most over cases where
> DEBUG_TEST_DRIVER_REMOVE is blowing up).

You mean something like this?
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index 4eabfe2..3b6c1a2d 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -158,6 +158,9 @@ int driver_register(struct device_driver *drv)
> 
> printk(KERN_WARNING "Driver '%s' needs updating - please use
> "
> 
> "bus_type methods\n", drv->name);
> 
> +   if (!drv->remove)
> +   drv->suppress_bind_attrs = true;
> +
> 
> other = driver_find(drv->name, drv->bus);
> if (other) {
> 
> printk(KERN_ERR "Error: Driver '%s' is already registered, "

> Is there any reason that can't be enforced at the bus layer, say?

I'm not sure if the change above works with remove functions set in struct 
bus_type too.
But on the other hand this would hide errors in drivers which are actually 
removable but do not cleanup properly which DEBUG_TEST_DRIVER_REMOVE tries to 
detect. By setting .suppress_bind_attrs = true explicitely you state "This 
driver cannot be removed!", so the remove callback is not missing by accident.

Best regards,
Alexander

[PATCH v2 2/2] arm: perf: Mark as non-removable

[Alexander Stein]

This driver can only built into the kernel. So disallow driver bind/unbind
and also prevent a kernel error in case DEBUG_TEST_DRIVER_REMOVE is
enabled.

Signed-off-by: Alexander Stein <alexander.st...@systec-electronic.com>
---
 arch/arm/kernel/perf_event_v7.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
index b942349..795e373 100644
--- a/arch/arm/kernel/perf_event_v7.c
+++ b/arch/arm/kernel/perf_event_v7.c
@@ -2029,6 +2029,7 @@ static int armv7_pmu_device_probe(struct platform_device 
*pdev)
 static struct platform_driver armv7_pmu_driver = {
.driver = {
.name   = "armv7-pmu",
+   .suppress_bind_attrs = true,
.of_match_table = armv7_pmu_of_device_ids,
},
.probe  = armv7_pmu_device_probe,
-- 
2.10.2

[PATCH v2 1/2] drivers/perf: arm_pmu: Use devm_ allocators

[Alexander Stein]

This eliminates several calls to kfree.

Signed-off-by: Alexander Stein <alexander.st...@systec-electronic.com>
---
 drivers/perf/arm_pmu.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index b37b572..a9bbdbf 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -917,7 +917,8 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
bool using_spi = false;
struct platform_device *pdev = pmu->plat_device;
 
-   irqs = kcalloc(pdev->num_resources, sizeof(*irqs), GFP_KERNEL);
+   irqs = devm_kcalloc(>dev, pdev->num_resources, sizeof(*irqs),
+   GFP_KERNEL);
if (!irqs)
return -ENOMEM;
 
@@ -939,7 +940,6 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
pr_err("PPI/SPI IRQ type mismatch for %s!\n",
dn->name);
of_node_put(dn);
-   kfree(irqs);
return -EINVAL;
}
 
@@ -988,10 +988,8 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
int ret;
 
ret = irq_get_percpu_devid_partition(irq, 
>supported_cpus);
-   if (ret) {
-   kfree(irqs);
+   if (ret)
return ret;
-   }
} else {
/* Otherwise default to all CPUs */
cpumask_setall(>supported_cpus);
@@ -1001,8 +999,6 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
/* If we matched up the IRQ affinities, use them to route the SPIs */
if (using_spi && i == pdev->num_resources)
pmu->irq_affinity = irqs;
-   else
-   kfree(irqs);
 
return 0;
 }
@@ -1017,7 +1013,7 @@ int arm_pmu_device_probe(struct platform_device *pdev,
struct arm_pmu *pmu;
int ret = -ENODEV;
 
-   pmu = kzalloc(sizeof(struct arm_pmu), GFP_KERNEL);
+   pmu = devm_kzalloc(>dev, sizeof(struct arm_pmu), GFP_KERNEL);
if (!pmu) {
pr_info("failed to allocate PMU device!\n");
return -ENOMEM;
@@ -1074,8 +1070,6 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 out_free:
pr_info("%s: failed to register PMU devices!\n",
of_node_full_name(node));
-   kfree(pmu->irq_affinity);
-   kfree(pmu);
return ret;
 }
 
-- 
2.10.2

[PATCH v2 0/2] mark driver as non-removable

[Alexander Stein]

Changes in v2;
* Instead of adding a remove function which is unused otherwise, mark the driver
  as non-remoable

Using DEBUG_TEST_DRIVER_REMOVE every driver gets probed, removed and probed
again. This breaks drivers without removal function, e.g. arm perf
resulting in the following dump:
[ cut here ]
WARNING: CPU: 1 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x6c/0x7c
sysfs: cannot create duplicate filename '/devices/armv7_cortex_a7'
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.9.0+ #212
Hardware name: Freescale LS1021A
Backtrace:
[<8020c044>] (dump_backtrace) from [<8020c2f0>] (show_stack+0x18/0x1c)
 r7:0009 r6:6013 r5:80c1776c r4:
[<8020c2d8>] (show_stack) from [<8046ead8>] (dump_stack+0x94/0xa8)
[<8046ea44>] (dump_stack) from [<8021ecd0>] (__warn+0xec/0x104)
 r7:0009 r6:8092efc8 r5: r4:bf04bd80
[<8021ebe4>] (__warn) from [<8021ed28>] (warn_slowpath_fmt+0x40/0x48)
 r9:80a32848 r8: r7: r6:bf0ab780 r5:8091d590 r4:bf0df000
[<8021ecec>] (warn_slowpath_fmt) from [<8036d53c>] (sysfs_warn_dup+0x6c/0x7c)
 r3:bf0df000 r2:8092ef94
[<8036d4d0>] (sysfs_warn_dup) from [<8036d628>] (sysfs_create_dir_ns+0x8c/0x9c)
 r6:bf0ab780 r5:bf20ee08 r4:ffef
[<8036d59c>] (sysfs_create_dir_ns) from [<80471860>] 
(kobject_add_internal+0xa8/0x34c)
 r6:bf0aa198 r5: r4:bf20ee08
[<804717b8>] (kobject_add_internal) from [<80471b54>] (kobject_add+0x50/0x94)
 r7: r6: r5: r4:bf20ee08
[<80471b08>] (kobject_add) from [<80524590>] (device_add+0xe4/0x590)
 r3: r2:
 r6:bf20ee00 r5: r4:bf20ee08
[<805244ac>] (device_add) from [<802a38a8>] (pmu_dev_alloc+0x88/0xd8)
 r10:0091 r9:80a32848 r8: r7:80a32840 r6:80c0ed3c r5:
 r4:bf1fe800
[<802a3820>] (pmu_dev_alloc) from [<80a0cd20>] (perf_event_sysfs_init+0x5c/0xb4)
 r7:80a32840 r6: r5:bf1fe800 r4:80c0ede0
[<80a0ccc4>] (perf_event_sysfs_init) from [<80201778>] 
(do_one_initcall+0x48/0x178)
 r6:80c45000 r5:80a0ccc4 r4:0006
[<80201730>] (do_one_initcall) from [<80a00ecc>] 
(kernel_init_freeable+0x168/0x20c)
 r8:80a00610 r7:80a32840 r6:80c45000 r5:80c45000 r4:0006
[<80a00d64>] (kernel_init_freeable) from [<806febcc>] (kernel_init+0x10/0x11c)
 r10: r9: r8: r7: r6: r5:806febbc
 r4:
[<806febbc>] (kernel_init) from [<80208388>] (ret_from_fork+0x14/0x2c)
 r5:806febbc r4:
---[ end trace 9d251d389382804f ]---

This patchset marks the driver as explicitly non-remoavle preventing this error.
This disables bin/unbind for this driver as well.

Alexander Stein (2):
  drivers/perf: arm_pmu: Use devm_ allocators
  arm: perf: Mark as non-removable

 arch/arm/kernel/perf_event_v7.c |  1 +
 drivers/perf/arm_pmu.c  | 14 --
 2 files changed, 5 insertions(+), 10 deletions(-)

-- 
2.10.2

[PATCH v2 2/2] arm: perf: Mark as non-removable

[Alexander Stein]

This driver can only built into the kernel. So disallow driver bind/unbind
and also prevent a kernel error in case DEBUG_TEST_DRIVER_REMOVE is
enabled.

Signed-off-by: Alexander Stein 
---
 arch/arm/kernel/perf_event_v7.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
index b942349..795e373 100644
--- a/arch/arm/kernel/perf_event_v7.c
+++ b/arch/arm/kernel/perf_event_v7.c
@@ -2029,6 +2029,7 @@ static int armv7_pmu_device_probe(struct platform_device 
*pdev)
 static struct platform_driver armv7_pmu_driver = {
.driver = {
.name   = "armv7-pmu",
+   .suppress_bind_attrs = true,
.of_match_table = armv7_pmu_of_device_ids,
},
.probe  = armv7_pmu_device_probe,
-- 
2.10.2

[PATCH v2 1/2] drivers/perf: arm_pmu: Use devm_ allocators

[Alexander Stein]

This eliminates several calls to kfree.

Signed-off-by: Alexander Stein 
---
 drivers/perf/arm_pmu.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index b37b572..a9bbdbf 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -917,7 +917,8 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
bool using_spi = false;
struct platform_device *pdev = pmu->plat_device;
 
-   irqs = kcalloc(pdev->num_resources, sizeof(*irqs), GFP_KERNEL);
+   irqs = devm_kcalloc(>dev, pdev->num_resources, sizeof(*irqs),
+   GFP_KERNEL);
if (!irqs)
return -ENOMEM;
 
@@ -939,7 +940,6 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
pr_err("PPI/SPI IRQ type mismatch for %s!\n",
dn->name);
of_node_put(dn);
-   kfree(irqs);
return -EINVAL;
}
 
@@ -988,10 +988,8 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
int ret;
 
ret = irq_get_percpu_devid_partition(irq, 
>supported_cpus);
-   if (ret) {
-   kfree(irqs);
+   if (ret)
return ret;
-   }
} else {
/* Otherwise default to all CPUs */
cpumask_setall(>supported_cpus);
@@ -1001,8 +999,6 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
/* If we matched up the IRQ affinities, use them to route the SPIs */
if (using_spi && i == pdev->num_resources)
pmu->irq_affinity = irqs;
-   else
-   kfree(irqs);
 
return 0;
 }
@@ -1017,7 +1013,7 @@ int arm_pmu_device_probe(struct platform_device *pdev,
struct arm_pmu *pmu;
int ret = -ENODEV;
 
-   pmu = kzalloc(sizeof(struct arm_pmu), GFP_KERNEL);
+   pmu = devm_kzalloc(>dev, sizeof(struct arm_pmu), GFP_KERNEL);
if (!pmu) {
pr_info("failed to allocate PMU device!\n");
return -ENOMEM;
@@ -1074,8 +1070,6 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 out_free:
pr_info("%s: failed to register PMU devices!\n",
of_node_full_name(node));
-   kfree(pmu->irq_affinity);
-   kfree(pmu);
return ret;
 }
 
-- 
2.10.2

[PATCH v2 0/2] mark driver as non-removable

[Alexander Stein]

Changes in v2;
* Instead of adding a remove function which is unused otherwise, mark the driver
  as non-remoable

Using DEBUG_TEST_DRIVER_REMOVE every driver gets probed, removed and probed
again. This breaks drivers without removal function, e.g. arm perf
resulting in the following dump:
[ cut here ]
WARNING: CPU: 1 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x6c/0x7c
sysfs: cannot create duplicate filename '/devices/armv7_cortex_a7'
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.9.0+ #212
Hardware name: Freescale LS1021A
Backtrace:
[<8020c044>] (dump_backtrace) from [<8020c2f0>] (show_stack+0x18/0x1c)
 r7:0009 r6:6013 r5:80c1776c r4:
[<8020c2d8>] (show_stack) from [<8046ead8>] (dump_stack+0x94/0xa8)
[<8046ea44>] (dump_stack) from [<8021ecd0>] (__warn+0xec/0x104)
 r7:0009 r6:8092efc8 r5: r4:bf04bd80
[<8021ebe4>] (__warn) from [<8021ed28>] (warn_slowpath_fmt+0x40/0x48)
 r9:80a32848 r8: r7: r6:bf0ab780 r5:8091d590 r4:bf0df000
[<8021ecec>] (warn_slowpath_fmt) from [<8036d53c>] (sysfs_warn_dup+0x6c/0x7c)
 r3:bf0df000 r2:8092ef94
[<8036d4d0>] (sysfs_warn_dup) from [<8036d628>] (sysfs_create_dir_ns+0x8c/0x9c)
 r6:bf0ab780 r5:bf20ee08 r4:ffef
[<8036d59c>] (sysfs_create_dir_ns) from [<80471860>] 
(kobject_add_internal+0xa8/0x34c)
 r6:bf0aa198 r5: r4:bf20ee08
[<804717b8>] (kobject_add_internal) from [<80471b54>] (kobject_add+0x50/0x94)
 r7: r6: r5: r4:bf20ee08
[<80471b08>] (kobject_add) from [<80524590>] (device_add+0xe4/0x590)
 r3: r2:
 r6:bf20ee00 r5: r4:bf20ee08
[<805244ac>] (device_add) from [<802a38a8>] (pmu_dev_alloc+0x88/0xd8)
 r10:0091 r9:80a32848 r8: r7:80a32840 r6:80c0ed3c r5:
 r4:bf1fe800
[<802a3820>] (pmu_dev_alloc) from [<80a0cd20>] (perf_event_sysfs_init+0x5c/0xb4)
 r7:80a32840 r6: r5:bf1fe800 r4:80c0ede0
[<80a0ccc4>] (perf_event_sysfs_init) from [<80201778>] 
(do_one_initcall+0x48/0x178)
 r6:80c45000 r5:80a0ccc4 r4:0006
[<80201730>] (do_one_initcall) from [<80a00ecc>] 
(kernel_init_freeable+0x168/0x20c)
 r8:80a00610 r7:80a32840 r6:80c45000 r5:80c45000 r4:0006
[<80a00d64>] (kernel_init_freeable) from [<806febcc>] (kernel_init+0x10/0x11c)
 r10: r9: r8: r7: r6: r5:806febbc
 r4:
[<806febbc>] (kernel_init) from [<80208388>] (ret_from_fork+0x14/0x2c)
 r5:806febbc r4:
---[ end trace 9d251d389382804f ]---

This patchset marks the driver as explicitly non-remoavle preventing this error.
This disables bin/unbind for this driver as well.

Alexander Stein (2):
  drivers/perf: arm_pmu: Use devm_ allocators
  arm: perf: Mark as non-removable

 arch/arm/kernel/perf_event_v7.c |  1 +
 drivers/perf/arm_pmu.c  | 14 --
 2 files changed, 5 insertions(+), 10 deletions(-)

-- 
2.10.2

Re: [PATCH 2/3] drivers/perf: arm_pmu: add arm_pmu_device_remove

[Alexander Stein]

On Wednesday 21 December 2016 14:38:54, Peter Zijlstra wrote:
> On Wed, Dec 21, 2016 at 11:19:34AM +0100, Alexander Stein wrote:
> > Add ARM PMU removal function. This will be required by perf event drivers
> > when option DEBUG_TEST_DRIVER_REMOVE is enabled.
> > 
> > Signed-off-by: Alexander Stein <alexander.st...@systec-electronic.com>
> > ---
> > 
> >  drivers/perf/arm_pmu.c   | 14 ++
> >  include/linux/perf/arm_pmu.h |  2 ++
> >  2 files changed, 16 insertions(+)
> > 
> > diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> > index a9bbdbf..b7ddc4c 100644
> > --- a/drivers/perf/arm_pmu.c
> > +++ b/drivers/perf/arm_pmu.c
> > @@ -1022,6 +1022,7 @@ int arm_pmu_device_probe(struct platform_device
> > *pdev,> 
> > armpmu_init(pmu);
> > 
> > pmu->plat_device = pdev;
> > 
> > +   platform_set_drvdata(pdev, pmu);
> > 
> > if (node && (of_id = of_match_node(of_table, pdev->dev.of_node))) {
> > 
> > init_fn = of_id->data;
> > 
> > @@ -1073,6 +1074,19 @@ int arm_pmu_device_probe(struct platform_device
> > *pdev,> 
> > return ret;
> >  
> >  }
> > 
> > +int arm_pmu_device_remove(struct platform_device *pdev)
> > +{
> > +   struct arm_pmu *pmu = platform_get_drvdata(pdev);
> > +
> > +   __oprofile_cpu_pmu = NULL;
> > +
> > +   perf_pmu_unregister(>pmu);
> > +
> > +   cpu_pmu_destroy(pmu);
> > +
> > +   return 0;
> > +}
> 
> So normally, if there are events that use this pmu, we hold a reference
> on its module, which avoids removal from happening.
> 
> How is that guarantee made by DEBUG_TEST_DRIVER_REMOVE ? Or will it
> simply kill everything even though there's active events for the PMU?

AFAICS you won't be able to hold any reference until this test remove is done. 
This feature is implemented in really_probe(). If the driver is successfully 
probed it will be removed and probed again.
But reading that part of the code I stumbled over suppress_bind_attrs which 
would prevent this procedure. After some grepping I found commit 80c6397c3 
("clk: oxnas: make it explicitly non-modular").
Essentially setting
> .suppress_bind_attrs = true
in the platform_driver.
IMHO this seems far better than to add some remove functions only for testing 
a non-removable driver. I'll come up with a 2nd series, patch 1/3 is still 
valid.

Best regards,
Alexander

Re: [PATCH 2/3] drivers/perf: arm_pmu: add arm_pmu_device_remove

[Alexander Stein]

On Wednesday 21 December 2016 14:38:54, Peter Zijlstra wrote:
> On Wed, Dec 21, 2016 at 11:19:34AM +0100, Alexander Stein wrote:
> > Add ARM PMU removal function. This will be required by perf event drivers
> > when option DEBUG_TEST_DRIVER_REMOVE is enabled.
> > 
> > Signed-off-by: Alexander Stein 
> > ---
> > 
> >  drivers/perf/arm_pmu.c   | 14 ++
> >  include/linux/perf/arm_pmu.h |  2 ++
> >  2 files changed, 16 insertions(+)
> > 
> > diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> > index a9bbdbf..b7ddc4c 100644
> > --- a/drivers/perf/arm_pmu.c
> > +++ b/drivers/perf/arm_pmu.c
> > @@ -1022,6 +1022,7 @@ int arm_pmu_device_probe(struct platform_device
> > *pdev,> 
> > armpmu_init(pmu);
> > 
> > pmu->plat_device = pdev;
> > 
> > +   platform_set_drvdata(pdev, pmu);
> > 
> > if (node && (of_id = of_match_node(of_table, pdev->dev.of_node))) {
> > 
> > init_fn = of_id->data;
> > 
> > @@ -1073,6 +1074,19 @@ int arm_pmu_device_probe(struct platform_device
> > *pdev,> 
> > return ret;
> >  
> >  }
> > 
> > +int arm_pmu_device_remove(struct platform_device *pdev)
> > +{
> > +   struct arm_pmu *pmu = platform_get_drvdata(pdev);
> > +
> > +   __oprofile_cpu_pmu = NULL;
> > +
> > +   perf_pmu_unregister(>pmu);
> > +
> > +   cpu_pmu_destroy(pmu);
> > +
> > +   return 0;
> > +}
> 
> So normally, if there are events that use this pmu, we hold a reference
> on its module, which avoids removal from happening.
> 
> How is that guarantee made by DEBUG_TEST_DRIVER_REMOVE ? Or will it
> simply kill everything even though there's active events for the PMU?

AFAICS you won't be able to hold any reference until this test remove is done. 
This feature is implemented in really_probe(). If the driver is successfully 
probed it will be removed and probed again.
But reading that part of the code I stumbled over suppress_bind_attrs which 
would prevent this procedure. After some grepping I found commit 80c6397c3 
("clk: oxnas: make it explicitly non-modular").
Essentially setting
> .suppress_bind_attrs = true
in the platform_driver.
IMHO this seems far better than to add some remove functions only for testing 
a non-removable driver. I'll come up with a 2nd series, patch 1/3 is still 
valid.

Best regards,
Alexander

[PATCH 2/3] drivers/perf: arm_pmu: add arm_pmu_device_remove

[Alexander Stein]

Add ARM PMU removal function. This will be required by perf event drivers
when option DEBUG_TEST_DRIVER_REMOVE is enabled.

Signed-off-by: Alexander Stein <alexander.st...@systec-electronic.com>
---
 drivers/perf/arm_pmu.c   | 14 ++
 include/linux/perf/arm_pmu.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index a9bbdbf..b7ddc4c 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -1022,6 +1022,7 @@ int arm_pmu_device_probe(struct platform_device *pdev,
armpmu_init(pmu);
 
pmu->plat_device = pdev;
+   platform_set_drvdata(pdev, pmu);
 
if (node && (of_id = of_match_node(of_table, pdev->dev.of_node))) {
init_fn = of_id->data;
@@ -1073,6 +1074,19 @@ int arm_pmu_device_probe(struct platform_device *pdev,
return ret;
 }
 
+int arm_pmu_device_remove(struct platform_device *pdev)
+{
+   struct arm_pmu *pmu = platform_get_drvdata(pdev);
+
+   __oprofile_cpu_pmu = NULL;
+
+   perf_pmu_unregister(>pmu);
+
+   cpu_pmu_destroy(pmu);
+
+   return 0;
+}
+
 static int arm_pmu_hp_init(void)
 {
int ret;
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index 8462da2..8106f27 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -159,6 +159,8 @@ struct pmu_probe_info {
 int arm_pmu_device_probe(struct platform_device *pdev,
 const struct of_device_id *of_table,
 const struct pmu_probe_info *probe_table);
+int arm_pmu_device_remove(struct platform_device *pdev);
+
 
 #define ARMV8_PMU_PDEV_NAME "armv8-pmu"
 
-- 
2.10.2

[PATCH 2/3] drivers/perf: arm_pmu: add arm_pmu_device_remove

[Alexander Stein]

Add ARM PMU removal function. This will be required by perf event drivers
when option DEBUG_TEST_DRIVER_REMOVE is enabled.

Signed-off-by: Alexander Stein 
---
 drivers/perf/arm_pmu.c   | 14 ++
 include/linux/perf/arm_pmu.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index a9bbdbf..b7ddc4c 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -1022,6 +1022,7 @@ int arm_pmu_device_probe(struct platform_device *pdev,
armpmu_init(pmu);
 
pmu->plat_device = pdev;
+   platform_set_drvdata(pdev, pmu);
 
if (node && (of_id = of_match_node(of_table, pdev->dev.of_node))) {
init_fn = of_id->data;
@@ -1073,6 +1074,19 @@ int arm_pmu_device_probe(struct platform_device *pdev,
return ret;
 }
 
+int arm_pmu_device_remove(struct platform_device *pdev)
+{
+   struct arm_pmu *pmu = platform_get_drvdata(pdev);
+
+   __oprofile_cpu_pmu = NULL;
+
+   perf_pmu_unregister(>pmu);
+
+   cpu_pmu_destroy(pmu);
+
+   return 0;
+}
+
 static int arm_pmu_hp_init(void)
 {
int ret;
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index 8462da2..8106f27 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -159,6 +159,8 @@ struct pmu_probe_info {
 int arm_pmu_device_probe(struct platform_device *pdev,
 const struct of_device_id *of_table,
 const struct pmu_probe_info *probe_table);
+int arm_pmu_device_remove(struct platform_device *pdev);
+
 
 #define ARMV8_PMU_PDEV_NAME "armv8-pmu"
 
-- 
2.10.2

[PATCH 0/3] arm perf: Support DEBUG_TEST_DRIVER_REMOVE

[Alexander Stein]

Using DEBUG_TEST_DRIVER_REMOVE every driver gets probed, removed and probed
again. This breaks drivers without removal function, e.g. arm perf
resulting in the following dump:
[ cut here ]
WARNING: CPU: 1 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x6c/0x7c
sysfs: cannot create duplicate filename '/devices/armv7_cortex_a7'
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.9.0+ #212
Hardware name: Freescale LS1021A
Backtrace:
[<8020c044>] (dump_backtrace) from [<8020c2f0>] (show_stack+0x18/0x1c)
 r7:0009 r6:6013 r5:80c1776c r4:
[<8020c2d8>] (show_stack) from [<8046ead8>] (dump_stack+0x94/0xa8)
[<8046ea44>] (dump_stack) from [<8021ecd0>] (__warn+0xec/0x104)
 r7:0009 r6:8092efc8 r5: r4:bf04bd80
[<8021ebe4>] (__warn) from [<8021ed28>] (warn_slowpath_fmt+0x40/0x48)
 r9:80a32848 r8: r7: r6:bf0ab780 r5:8091d590 r4:bf0df000
[<8021ecec>] (warn_slowpath_fmt) from [<8036d53c>] (sysfs_warn_dup+0x6c/0x7c)
 r3:bf0df000 r2:8092ef94
[<8036d4d0>] (sysfs_warn_dup) from [<8036d628>] (sysfs_create_dir_ns+0x8c/0x9c)
 r6:bf0ab780 r5:bf20ee08 r4:ffef
[<8036d59c>] (sysfs_create_dir_ns) from [<80471860>] 
(kobject_add_internal+0xa8/0x34c)
 r6:bf0aa198 r5: r4:bf20ee08
[<804717b8>] (kobject_add_internal) from [<80471b54>] (kobject_add+0x50/0x94)
 r7: r6: r5: r4:bf20ee08
[<80471b08>] (kobject_add) from [<80524590>] (device_add+0xe4/0x590)
 r3: r2:
 r6:bf20ee00 r5: r4:bf20ee08
[<805244ac>] (device_add) from [<802a38a8>] (pmu_dev_alloc+0x88/0xd8)
 r10:0091 r9:80a32848 r8: r7:80a32840 r6:80c0ed3c r5:
 r4:bf1fe800
[<802a3820>] (pmu_dev_alloc) from [<80a0cd20>] (perf_event_sysfs_init+0x5c/0xb4)
 r7:80a32840 r6: r5:bf1fe800 r4:80c0ede0
[<80a0ccc4>] (perf_event_sysfs_init) from [<80201778>] 
(do_one_initcall+0x48/0x178)
 r6:80c45000 r5:80a0ccc4 r4:0006
[<80201730>] (do_one_initcall) from [<80a00ecc>] 
(kernel_init_freeable+0x168/0x20c)
 r8:80a00610 r7:80a32840 r6:80c45000 r5:80c45000 r4:0006
[<80a00d64>] (kernel_init_freeable) from [<806febcc>] (kernel_init+0x10/0x11c)
 r10: r9: r8: r7: r6: r5:806febbc
 r4:
[<806febbc>] (kernel_init) from [<80208388>] (ret_from_fork+0x14/0x2c)
 r5:806febbc r4:
---[ end trace 9d251d389382804f ]---

This patchset add a removal support for arm_pmu and perf_events_v7 and while
at it, use devm_ allocators in arm_pmu.

Alexander Stein (3):
  drivers/perf: arm_pmu: Use devm_ allocators
  drivers/perf: arm_pmu: add arm_pmu_device_remove
  arm: perf: Add platform driver removal function

 arch/arm/kernel/perf_event_v7.c |  6 ++
 drivers/perf/arm_pmu.c  | 28 ++--
 include/linux/perf/arm_pmu.h|  2 ++
 3 files changed, 26 insertions(+), 10 deletions(-)

-- 
2.10.2

[PATCH 0/3] arm perf: Support DEBUG_TEST_DRIVER_REMOVE

[Alexander Stein]

Using DEBUG_TEST_DRIVER_REMOVE every driver gets probed, removed and probed
again. This breaks drivers without removal function, e.g. arm perf
resulting in the following dump:
[ cut here ]
WARNING: CPU: 1 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x6c/0x7c
sysfs: cannot create duplicate filename '/devices/armv7_cortex_a7'
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.9.0+ #212
Hardware name: Freescale LS1021A
Backtrace:
[<8020c044>] (dump_backtrace) from [<8020c2f0>] (show_stack+0x18/0x1c)
 r7:0009 r6:6013 r5:80c1776c r4:
[<8020c2d8>] (show_stack) from [<8046ead8>] (dump_stack+0x94/0xa8)
[<8046ea44>] (dump_stack) from [<8021ecd0>] (__warn+0xec/0x104)
 r7:0009 r6:8092efc8 r5: r4:bf04bd80
[<8021ebe4>] (__warn) from [<8021ed28>] (warn_slowpath_fmt+0x40/0x48)
 r9:80a32848 r8: r7: r6:bf0ab780 r5:8091d590 r4:bf0df000
[<8021ecec>] (warn_slowpath_fmt) from [<8036d53c>] (sysfs_warn_dup+0x6c/0x7c)
 r3:bf0df000 r2:8092ef94
[<8036d4d0>] (sysfs_warn_dup) from [<8036d628>] (sysfs_create_dir_ns+0x8c/0x9c)
 r6:bf0ab780 r5:bf20ee08 r4:ffef
[<8036d59c>] (sysfs_create_dir_ns) from [<80471860>] 
(kobject_add_internal+0xa8/0x34c)
 r6:bf0aa198 r5: r4:bf20ee08
[<804717b8>] (kobject_add_internal) from [<80471b54>] (kobject_add+0x50/0x94)
 r7: r6: r5: r4:bf20ee08
[<80471b08>] (kobject_add) from [<80524590>] (device_add+0xe4/0x590)
 r3: r2:
 r6:bf20ee00 r5: r4:bf20ee08
[<805244ac>] (device_add) from [<802a38a8>] (pmu_dev_alloc+0x88/0xd8)
 r10:0091 r9:80a32848 r8: r7:80a32840 r6:80c0ed3c r5:
 r4:bf1fe800
[<802a3820>] (pmu_dev_alloc) from [<80a0cd20>] (perf_event_sysfs_init+0x5c/0xb4)
 r7:80a32840 r6: r5:bf1fe800 r4:80c0ede0
[<80a0ccc4>] (perf_event_sysfs_init) from [<80201778>] 
(do_one_initcall+0x48/0x178)
 r6:80c45000 r5:80a0ccc4 r4:0006
[<80201730>] (do_one_initcall) from [<80a00ecc>] 
(kernel_init_freeable+0x168/0x20c)
 r8:80a00610 r7:80a32840 r6:80c45000 r5:80c45000 r4:0006
[<80a00d64>] (kernel_init_freeable) from [<806febcc>] (kernel_init+0x10/0x11c)
 r10: r9: r8: r7: r6: r5:806febbc
 r4:
[<806febbc>] (kernel_init) from [<80208388>] (ret_from_fork+0x14/0x2c)
 r5:806febbc r4:
---[ end trace 9d251d389382804f ]---

This patchset add a removal support for arm_pmu and perf_events_v7 and while
at it, use devm_ allocators in arm_pmu.

Alexander Stein (3):
  drivers/perf: arm_pmu: Use devm_ allocators
  drivers/perf: arm_pmu: add arm_pmu_device_remove
  arm: perf: Add platform driver removal function

 arch/arm/kernel/perf_event_v7.c |  6 ++
 drivers/perf/arm_pmu.c  | 28 ++--
 include/linux/perf/arm_pmu.h|  2 ++
 3 files changed, 26 insertions(+), 10 deletions(-)

-- 
2.10.2

[PATCH 1/3] drivers/perf: arm_pmu: Use devm_ allocators

[Alexander Stein]

This eliminates several calls to kfree.

Signed-off-by: Alexander Stein <alexander.st...@systec-electronic.com>
---
 drivers/perf/arm_pmu.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index b37b572..a9bbdbf 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -917,7 +917,8 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
bool using_spi = false;
struct platform_device *pdev = pmu->plat_device;
 
-   irqs = kcalloc(pdev->num_resources, sizeof(*irqs), GFP_KERNEL);
+   irqs = devm_kcalloc(>dev, pdev->num_resources, sizeof(*irqs),
+   GFP_KERNEL);
if (!irqs)
return -ENOMEM;
 
@@ -939,7 +940,6 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
pr_err("PPI/SPI IRQ type mismatch for %s!\n",
dn->name);
of_node_put(dn);
-   kfree(irqs);
return -EINVAL;
}
 
@@ -988,10 +988,8 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
int ret;
 
ret = irq_get_percpu_devid_partition(irq, 
>supported_cpus);
-   if (ret) {
-   kfree(irqs);
+   if (ret)
return ret;
-   }
} else {
/* Otherwise default to all CPUs */
cpumask_setall(>supported_cpus);
@@ -1001,8 +999,6 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
/* If we matched up the IRQ affinities, use them to route the SPIs */
if (using_spi && i == pdev->num_resources)
pmu->irq_affinity = irqs;
-   else
-   kfree(irqs);
 
return 0;
 }
@@ -1017,7 +1013,7 @@ int arm_pmu_device_probe(struct platform_device *pdev,
struct arm_pmu *pmu;
int ret = -ENODEV;
 
-   pmu = kzalloc(sizeof(struct arm_pmu), GFP_KERNEL);
+   pmu = devm_kzalloc(>dev, sizeof(struct arm_pmu), GFP_KERNEL);
if (!pmu) {
pr_info("failed to allocate PMU device!\n");
return -ENOMEM;
@@ -1074,8 +1070,6 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 out_free:
pr_info("%s: failed to register PMU devices!\n",
of_node_full_name(node));
-   kfree(pmu->irq_affinity);
-   kfree(pmu);
return ret;
 }
 
-- 
2.10.2

[PATCH 1/3] drivers/perf: arm_pmu: Use devm_ allocators

[Alexander Stein]

This eliminates several calls to kfree.

Signed-off-by: Alexander Stein 
---
 drivers/perf/arm_pmu.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index b37b572..a9bbdbf 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -917,7 +917,8 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
bool using_spi = false;
struct platform_device *pdev = pmu->plat_device;
 
-   irqs = kcalloc(pdev->num_resources, sizeof(*irqs), GFP_KERNEL);
+   irqs = devm_kcalloc(>dev, pdev->num_resources, sizeof(*irqs),
+   GFP_KERNEL);
if (!irqs)
return -ENOMEM;
 
@@ -939,7 +940,6 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
pr_err("PPI/SPI IRQ type mismatch for %s!\n",
dn->name);
of_node_put(dn);
-   kfree(irqs);
return -EINVAL;
}
 
@@ -988,10 +988,8 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
int ret;
 
ret = irq_get_percpu_devid_partition(irq, 
>supported_cpus);
-   if (ret) {
-   kfree(irqs);
+   if (ret)
return ret;
-   }
} else {
/* Otherwise default to all CPUs */
cpumask_setall(>supported_cpus);
@@ -1001,8 +999,6 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
/* If we matched up the IRQ affinities, use them to route the SPIs */
if (using_spi && i == pdev->num_resources)
pmu->irq_affinity = irqs;
-   else
-   kfree(irqs);
 
return 0;
 }
@@ -1017,7 +1013,7 @@ int arm_pmu_device_probe(struct platform_device *pdev,
struct arm_pmu *pmu;
int ret = -ENODEV;
 
-   pmu = kzalloc(sizeof(struct arm_pmu), GFP_KERNEL);
+   pmu = devm_kzalloc(>dev, sizeof(struct arm_pmu), GFP_KERNEL);
if (!pmu) {
pr_info("failed to allocate PMU device!\n");
return -ENOMEM;
@@ -1074,8 +1070,6 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 out_free:
pr_info("%s: failed to register PMU devices!\n",
of_node_full_name(node));
-   kfree(pmu->irq_affinity);
-   kfree(pmu);
return ret;
 }
 
-- 
2.10.2

[PATCH 3/3] arm: perf: Add platform driver removal function

[Alexander Stein]

Despite the driver cannot be built as a module using the option
DEBUG_TEST_DRIVER_REMOVE requires the drivers to actually remove the
device.

Signed-off-by: Alexander Stein <alexander.st...@systec-electronic.com>
---
 arch/arm/kernel/perf_event_v7.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
index b942349..3ad02a2 100644
--- a/arch/arm/kernel/perf_event_v7.c
+++ b/arch/arm/kernel/perf_event_v7.c
@@ -2026,12 +2026,18 @@ static int armv7_pmu_device_probe(struct 
platform_device *pdev)
armv7_pmu_probe_table);
 }
 
+static int armv7_pmu_device_remove(struct platform_device *pdev)
+{
+   return arm_pmu_device_remove(pdev);
+}
+
 static struct platform_driver armv7_pmu_driver = {
.driver = {
.name   = "armv7-pmu",
.of_match_table = armv7_pmu_of_device_ids,
},
.probe  = armv7_pmu_device_probe,
+   .remove = armv7_pmu_device_remove,
 };
 
 static int __init register_armv7_pmu_driver(void)
-- 
2.10.2

[PATCH 3/3] arm: perf: Add platform driver removal function

[Alexander Stein]

Despite the driver cannot be built as a module using the option
DEBUG_TEST_DRIVER_REMOVE requires the drivers to actually remove the
device.

Signed-off-by: Alexander Stein 
---
 arch/arm/kernel/perf_event_v7.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
index b942349..3ad02a2 100644
--- a/arch/arm/kernel/perf_event_v7.c
+++ b/arch/arm/kernel/perf_event_v7.c
@@ -2026,12 +2026,18 @@ static int armv7_pmu_device_probe(struct 
platform_device *pdev)
armv7_pmu_probe_table);
 }
 
+static int armv7_pmu_device_remove(struct platform_device *pdev)
+{
+   return arm_pmu_device_remove(pdev);
+}
+
 static struct platform_driver armv7_pmu_driver = {
.driver = {
.name   = "armv7-pmu",
.of_match_table = armv7_pmu_of_device_ids,
},
.probe  = armv7_pmu_device_probe,
+   .remove = armv7_pmu_device_remove,
 };
 
 static int __init register_armv7_pmu_driver(void)
-- 
2.10.2

Re: [PATCH v3] mmc: sdhci-of-esdhc: fixup PRESENT_STATE read

[Alexander Stein]

On Monday 14 November 2016 16:12:27, Michael Walle wrote:
> Since commit 87a18a6a5652 ("mmc: mmc: Use ->card_busy() to detect busy
> cards in __mmc_switch()") the ESDHC driver is broken:
>   mmc0: Card stuck in programming state! __mmc_switch
>   mmc0: error -110 whilst initialising MMC card
> 
> Since this commit __mmc_switch() uses ->card_busy(), which is
> sdhci_card_busy() for the esdhc driver. sdhci_card_busy() uses the
> PRESENT_STATE register, specifically the DAT0 signal level bit. But the
> ESDHC uses a non-conformant PRESENT_STATE register, thus a read fixup is
> required to make the driver work again.
> 
> Signed-off-by: Michael Walle 
> Fixes: 87a18a6a5652 ("mmc: mmc: Use ->card_busy() to detect busy cards in
> __mmc_switch()") ---
> v3:
>  - explain the bits in the comments
>  - use bits[19:0] from the original value, all other will be taken from the
>fixup value.
> 
> v2:
>  - use lower bits of the original value (that was actually a typo)
>  - add fixes tag
>  - fix typo
> 
>  drivers/mmc/host/sdhci-of-esdhc.c | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c
> b/drivers/mmc/host/sdhci-of-esdhc.c index fb71c86..74cf3b1 100644
> --- a/drivers/mmc/host/sdhci-of-esdhc.c
> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> @@ -66,6 +66,19 @@ static u32 esdhc_readl_fixup(struct sdhci_host *host,
>   return ret;
>   }
>   }
> + /*
> +  * The DAT[3:0] line signal levels and the CMD line signal level are
> +  * not compatible with standard SDHC register. The line signal levels
> +  * DAT[7:0] are at bits 31:24 and the line signal level is at bit 23.
  ^
I guess there is a "command" missing, no?

Best regards,
Alexander

Re: [PATCH v3] mmc: sdhci-of-esdhc: fixup PRESENT_STATE read

[Alexander Stein]

On Monday 14 November 2016 16:12:27, Michael Walle wrote:
> Since commit 87a18a6a5652 ("mmc: mmc: Use ->card_busy() to detect busy
> cards in __mmc_switch()") the ESDHC driver is broken:
>   mmc0: Card stuck in programming state! __mmc_switch
>   mmc0: error -110 whilst initialising MMC card
> 
> Since this commit __mmc_switch() uses ->card_busy(), which is
> sdhci_card_busy() for the esdhc driver. sdhci_card_busy() uses the
> PRESENT_STATE register, specifically the DAT0 signal level bit. But the
> ESDHC uses a non-conformant PRESENT_STATE register, thus a read fixup is
> required to make the driver work again.
> 
> Signed-off-by: Michael Walle 
> Fixes: 87a18a6a5652 ("mmc: mmc: Use ->card_busy() to detect busy cards in
> __mmc_switch()") ---
> v3:
>  - explain the bits in the comments
>  - use bits[19:0] from the original value, all other will be taken from the
>fixup value.
> 
> v2:
>  - use lower bits of the original value (that was actually a typo)
>  - add fixes tag
>  - fix typo
> 
>  drivers/mmc/host/sdhci-of-esdhc.c | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c
> b/drivers/mmc/host/sdhci-of-esdhc.c index fb71c86..74cf3b1 100644
> --- a/drivers/mmc/host/sdhci-of-esdhc.c
> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> @@ -66,6 +66,19 @@ static u32 esdhc_readl_fixup(struct sdhci_host *host,
>   return ret;
>   }
>   }
> + /*
> +  * The DAT[3:0] line signal levels and the CMD line signal level are
> +  * not compatible with standard SDHC register. The line signal levels
> +  * DAT[7:0] are at bits 31:24 and the line signal level is at bit 23.
  ^
I guess there is a "command" missing, no?

Best regards,
Alexander

Re: [PATCH v4] i2c: designware: Implement support for SMBus block read and write

[Alexander Stein]

On Thursday 10 November 2016 09:56:33, tnhu...@apm.com wrote:
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c
> b/drivers/i2c/busses/i2c-designware-pcidrv.c index 96f8230..8ffe2da 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -75,6 +75,7 @@ struct dw_pci_controller {
>   I2C_FUNC_SMBUS_BYTE |   \
>   I2C_FUNC_SMBUS_BYTE_DATA |  \
>   I2C_FUNC_SMBUS_WORD_DATA |  \
> + I2C_FUNC_SMBUS_BLOCK_DATA | \
>   I2C_FUNC_SMBUS_I2C_BLOCK)
> 
>  /* Merrifield HCNT/LCNT/SDA hold time */
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c index 0b42a12..886fb62 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -220,6 +220,7 @@ static int dw_i2c_plat_probe(struct platform_device
> *pdev) I2C_FUNC_SMBUS_BYTE |
>   I2C_FUNC_SMBUS_BYTE_DATA |
>   I2C_FUNC_SMBUS_WORD_DATA |
> + I2C_FUNC_SMBUS_BLOCK_DATA |
>   I2C_FUNC_SMBUS_I2C_BLOCK;
> 
>   dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |

Shouldn't those functionality bits moved to a common place, like i2c-
designware-core.h?

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.st...@systec-electronic.com

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-0
Fax:+49 (0) 3765 38600-4100
 
Managing Directors:
Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010

Re: [PATCH v4] i2c: designware: Implement support for SMBus block read and write

[Alexander Stein]

On Thursday 10 November 2016 09:56:33, tnhu...@apm.com wrote:
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c
> b/drivers/i2c/busses/i2c-designware-pcidrv.c index 96f8230..8ffe2da 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -75,6 +75,7 @@ struct dw_pci_controller {
>   I2C_FUNC_SMBUS_BYTE |   \
>   I2C_FUNC_SMBUS_BYTE_DATA |  \
>   I2C_FUNC_SMBUS_WORD_DATA |  \
> + I2C_FUNC_SMBUS_BLOCK_DATA | \
>   I2C_FUNC_SMBUS_I2C_BLOCK)
> 
>  /* Merrifield HCNT/LCNT/SDA hold time */
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c index 0b42a12..886fb62 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -220,6 +220,7 @@ static int dw_i2c_plat_probe(struct platform_device
> *pdev) I2C_FUNC_SMBUS_BYTE |
>   I2C_FUNC_SMBUS_BYTE_DATA |
>   I2C_FUNC_SMBUS_WORD_DATA |
> + I2C_FUNC_SMBUS_BLOCK_DATA |
>   I2C_FUNC_SMBUS_I2C_BLOCK;
> 
>   dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |

Shouldn't those functionality bits moved to a common place, like i2c-
designware-core.h?

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.st...@systec-electronic.com

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-0
Fax:+49 (0) 3765 38600-4100
 
Managing Directors:
Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010

Re: [PATCH] gpio: mcp23s08: Add option to configure pullups.

[Alexander Stein]

On Monday 24 October 2016 02:53:31, Linus Walleij wrote:
> On Fri, Oct 21, 2016 at 5:00 PM, Enric Balletbo i Serra
> 
>  wrote:
> > Default is without pullups, but if property is specified in DT and the bit
> > is set, set a pullup on GPIO-n.
> > 
> > Signed-off-by: Enric Balletbo i Serra 
> 
> I don't generally like this.
> 
> In device tree it is the consumer that specifies how the line is used,
> not the producer (gpiochip).
> 
> We currently only specify polarity, open drain and open source
> for consumers though.
> 
> And the entire pin control system came into being *exactly* because
> Grant didn't like me adding these things to the GPIO drivers.
> 
> So how many other things does the MCP support? Drive strength?
> Schmitt trigger? Is there a datasheet?

Some unsupported features (AFAIK):
* Input polarity (IPOL register)
* open-drain interrupt pin
* pullup on pins (proposed patch)

Datasheet is here: http://ww1.microchip.com/downloads/en/DeviceDoc/21919e.pdf

Best regards,
Alexander

Re: [PATCH] gpio: mcp23s08: Add option to configure pullups.

[Alexander Stein]

On Monday 24 October 2016 02:53:31, Linus Walleij wrote:
> On Fri, Oct 21, 2016 at 5:00 PM, Enric Balletbo i Serra
> 
>  wrote:
> > Default is without pullups, but if property is specified in DT and the bit
> > is set, set a pullup on GPIO-n.
> > 
> > Signed-off-by: Enric Balletbo i Serra 
> 
> I don't generally like this.
> 
> In device tree it is the consumer that specifies how the line is used,
> not the producer (gpiochip).
> 
> We currently only specify polarity, open drain and open source
> for consumers though.
> 
> And the entire pin control system came into being *exactly* because
> Grant didn't like me adding these things to the GPIO drivers.
> 
> So how many other things does the MCP support? Drive strength?
> Schmitt trigger? Is there a datasheet?

Some unsupported features (AFAIK):
* Input polarity (IPOL register)
* open-drain interrupt pin
* pullup on pins (proposed patch)

Datasheet is here: http://ww1.microchip.com/downloads/en/DeviceDoc/21919e.pdf

Best regards,
Alexander

Re: [PATCH 0/2] ARM: at91: properly handle LPDDR poweroff

[Alexander Stein]

Hi Alexandre,

On Friday 07 October 2016 18:34:25, Alexandre Belloni wrote:
> Hi,
> 
> This patch set improves LPDDR support on SoCs using the Atmel MPDDR
> controller.
> 
> LPDDR memoris can only handle up to 400 uncontrolled power offs in their
> life. The proper power off sequence has to be applied before shutting down
> the SoC.
> 
> I'm not too happy with the code duplication but this is a design choice
> that has been made before because both shitdown controler are really
 

I guess you mean shutdown? :) Same for the suject of 2nd patch.

Best regards,
Alexander

Re: [PATCH 0/2] ARM: at91: properly handle LPDDR poweroff

[Alexander Stein]

Hi Alexandre,

On Friday 07 October 2016 18:34:25, Alexandre Belloni wrote:
> Hi,
> 
> This patch set improves LPDDR support on SoCs using the Atmel MPDDR
> controller.
> 
> LPDDR memoris can only handle up to 400 uncontrolled power offs in their
> life. The proper power off sequence has to be applied before shutting down
> the SoC.
> 
> I'm not too happy with the code duplication but this is a design choice
> that has been made before because both shitdown controler are really
 

I guess you mean shutdown? :) Same for the suject of 2nd patch.

Best regards,
Alexander

Re: [PATCH] clocksource/fsl: Fix errata A-007728 for flextimer

[Alexander Stein]

On Thursday 08 September 2016 10:04:55, Meng Yi wrote:
> If the FTM counter reaches the FTM_MOD value between the reading of the
> TOF bit and the writing of 0 to the TOF bit, the process of clearing the
> TOF bit does not work as expected when FTMx_CONF[NUMTOF] != 0 and the
> current TOF count is less than FTMx_CONF[NUMTOF]. If the above condition
> is met, the TOF bit remains set. If the TOF interrupt is enabled
> (FTMx_SC[TOIE] = 1), the TOF interrupt also remains asserted.
> 
> Above is the errata discription
> 
> The workaround is clearing TOF bit until it is cleaned(FTM counter doesn't
> always reache the FTM_MOD anyway),which may cost some cycles.
> 
> Signed-off-by: Meng Yi 
> ---
>  drivers/clocksource/fsl_ftm_timer.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clocksource/fsl_ftm_timer.c
> b/drivers/clocksource/fsl_ftm_timer.c index 738515b..ade26e5 100644
> --- a/drivers/clocksource/fsl_ftm_timer.c
> +++ b/drivers/clocksource/fsl_ftm_timer.c
> @@ -83,11 +83,10 @@ static inline void ftm_counter_disable(void __iomem
> *base)
> 
>  static inline void ftm_irq_acknowledge(void __iomem *base)
>  {
> - u32 val;
> -
> - val = ftm_readl(base + FTM_SC);
> - val &= ~FTM_SC_TOF;
> - ftm_writel(val, base + FTM_SC);
> + /*read and clean the FTM_SC_TOF bit until its cleared*/
> + while (FTM_SC_TOF & ftm_readl(base + FTM_SC))
> + ftm_writel(ftm_readl(base + FTM_SC) & (~FTM_SC_TOF),
> +base + FTM_SC);
>  }

So you are essentially polling hardware in interrupt context. Please add a 
sensible timeout to abort this loop in case of defective hardware, and maybe 
disable the interrupt in that case.

Best regards,
Alexander

Re: [PATCH] clocksource/fsl: Fix errata A-007728 for flextimer

[Alexander Stein]

On Thursday 08 September 2016 10:04:55, Meng Yi wrote:
> If the FTM counter reaches the FTM_MOD value between the reading of the
> TOF bit and the writing of 0 to the TOF bit, the process of clearing the
> TOF bit does not work as expected when FTMx_CONF[NUMTOF] != 0 and the
> current TOF count is less than FTMx_CONF[NUMTOF]. If the above condition
> is met, the TOF bit remains set. If the TOF interrupt is enabled
> (FTMx_SC[TOIE] = 1), the TOF interrupt also remains asserted.
> 
> Above is the errata discription
> 
> The workaround is clearing TOF bit until it is cleaned(FTM counter doesn't
> always reache the FTM_MOD anyway),which may cost some cycles.
> 
> Signed-off-by: Meng Yi 
> ---
>  drivers/clocksource/fsl_ftm_timer.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clocksource/fsl_ftm_timer.c
> b/drivers/clocksource/fsl_ftm_timer.c index 738515b..ade26e5 100644
> --- a/drivers/clocksource/fsl_ftm_timer.c
> +++ b/drivers/clocksource/fsl_ftm_timer.c
> @@ -83,11 +83,10 @@ static inline void ftm_counter_disable(void __iomem
> *base)
> 
>  static inline void ftm_irq_acknowledge(void __iomem *base)
>  {
> - u32 val;
> -
> - val = ftm_readl(base + FTM_SC);
> - val &= ~FTM_SC_TOF;
> - ftm_writel(val, base + FTM_SC);
> + /*read and clean the FTM_SC_TOF bit until its cleared*/
> + while (FTM_SC_TOF & ftm_readl(base + FTM_SC))
> + ftm_writel(ftm_readl(base + FTM_SC) & (~FTM_SC_TOF),
> +base + FTM_SC);
>  }

So you are essentially polling hardware in interrupt context. Please add a 
sensible timeout to abort this loop in case of defective hardware, and maybe 
disable the interrupt in that case.

Best regards,
Alexander

Re: [PATCH v1 4/5] ARM: dts: ls1043a: add qDMA node

[Alexander Stein]

On Thursday 18 August 2016 14:38:47, Yuan Yao wrote:
> From: Yuan Yao 
> 
> Add the QDMA node for ls1043a platform to
> support QDMA driver.
> 
> Signed-off-by: Yuan Yao 
> ---
>  arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi index e8e4c3e..e463074
> 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> @@ -467,6 +467,16 @@
>< 4 0>;
>   };
> 
> + qdma: qdma@839 {
> + compatible = "fsl,ls1021a-qdma", "fsl,ls1043a-qdma";

This doesn't match the order of your binding in Patch 2/5:
> +- compatible :
> +   - "fsl,ls1021a-qdma",
> +   Or "fsl,ls1043a-qdma" followed by "fsl,ls1021a-qdma",

It should be the other way around.

Best regards,
Alexander

Re: [PATCH v1 4/5] ARM: dts: ls1043a: add qDMA node

[Alexander Stein]

On Thursday 18 August 2016 14:38:47, Yuan Yao wrote:
> From: Yuan Yao 
> 
> Add the QDMA node for ls1043a platform to
> support QDMA driver.
> 
> Signed-off-by: Yuan Yao 
> ---
>  arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi index e8e4c3e..e463074
> 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> @@ -467,6 +467,16 @@
>< 4 0>;
>   };
> 
> + qdma: qdma@839 {
> + compatible = "fsl,ls1021a-qdma", "fsl,ls1043a-qdma";

This doesn't match the order of your binding in Patch 2/5:
> +- compatible :
> +   - "fsl,ls1021a-qdma",
> +   Or "fsl,ls1043a-qdma" followed by "fsl,ls1021a-qdma",

It should be the other way around.

Best regards,
Alexander

Re: [PATCH v1 4/5] ARM: dts: ls1043a: add qDMA node

[Alexander Stein]

On Thursday 18 August 2016 14:38:47, Yuan Yao wrote:
> From: Yuan Yao 
> 
> Add the QDMA node for ls1043a platform to
> support QDMA driver.
> 
> Signed-off-by: Yuan Yao 
> ---
>  arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi index e8e4c3e..e463074
> 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> @@ -467,6 +467,16 @@
>< 4 0>;
>   };
> 
> + qdma: qdma@839 {
> + compatible = "fsl,ls1021a-qdma", "fsl,ls1043a-qdma";

This doesn't match the order of your binding in Patch 2/5:
> +- compatible :
> +   - "fsl,ls1021a-qdma",
> +   Or "fsl,ls1043a-qdma" followed by "fsl,ls1021a-qdma",

It should be the other way around.

Best regards,
Alexander

Re: [PATCH v1 4/5] ARM: dts: ls1043a: add qDMA node

[Alexander Stein]

On Thursday 18 August 2016 14:38:47, Yuan Yao wrote:
> From: Yuan Yao 
> 
> Add the QDMA node for ls1043a platform to
> support QDMA driver.
> 
> Signed-off-by: Yuan Yao 
> ---
>  arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi index e8e4c3e..e463074
> 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> @@ -467,6 +467,16 @@
>< 4 0>;
>   };
> 
> + qdma: qdma@839 {
> + compatible = "fsl,ls1021a-qdma", "fsl,ls1043a-qdma";

This doesn't match the order of your binding in Patch 2/5:
> +- compatible :
> +   - "fsl,ls1021a-qdma",
> +   Or "fsl,ls1043a-qdma" followed by "fsl,ls1021a-qdma",

It should be the other way around.

Best regards,
Alexander

Re: [Patch v3 10/11] driver/edac/layerscape_edac: Add Layerscape EDAC support

[Alexander Stein]

On Thursday 04 August 2016 15:58:35, York Sun wrote:
> Add DDR EDAC for ARM-based compatible controllers. Both big-endian
> and little-endian are supported.
> 
> Signed-off-by: York Sun 
> 
> ---
> Change log
>   v3: no change
>   v2: Create new driver using shared DDR object
> 
>  arch/arm64/Kconfig.platforms   |  1 +
>  arch/{arm => arm64}/include/asm/edac.h | 21 +--
>  arch/arm64/include/asm/irq.h   |  4 ++
>  drivers/edac/Kconfig   |  7 
>  drivers/edac/Makefile  |  3 ++
>  drivers/edac/fsl_ddr_edac.c|  1 +
>  drivers/edac/layerscape_edac.c | 67
> ++ 7 files changed, 92 insertions(+), 12
> deletions(-)
>  copy arch/{arm => arm64}/include/asm/edac.h (79%)
>  create mode 100644 drivers/edac/layerscape_edac.c

This only adds EDAC support for ARM64. What would be necessary for ARM 
support, e.g. LS1021A?

Best regards,
Alexander

Re: [Patch v3 10/11] driver/edac/layerscape_edac: Add Layerscape EDAC support

[Alexander Stein]

On Thursday 04 August 2016 15:58:35, York Sun wrote:
> Add DDR EDAC for ARM-based compatible controllers. Both big-endian
> and little-endian are supported.
> 
> Signed-off-by: York Sun 
> 
> ---
> Change log
>   v3: no change
>   v2: Create new driver using shared DDR object
> 
>  arch/arm64/Kconfig.platforms   |  1 +
>  arch/{arm => arm64}/include/asm/edac.h | 21 +--
>  arch/arm64/include/asm/irq.h   |  4 ++
>  drivers/edac/Kconfig   |  7 
>  drivers/edac/Makefile  |  3 ++
>  drivers/edac/fsl_ddr_edac.c|  1 +
>  drivers/edac/layerscape_edac.c | 67
> ++ 7 files changed, 92 insertions(+), 12
> deletions(-)
>  copy arch/{arm => arm64}/include/asm/edac.h (79%)
>  create mode 100644 drivers/edac/layerscape_edac.c

This only adds EDAC support for ARM64. What would be necessary for ARM 
support, e.g. LS1021A?

Best regards,
Alexander

Re: [PATCH v4 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD

[Alexander Stein]

On Thursday 04 August 2016 15:28:18, Ksenija Stanojevic wrote:
> Add core files for mxs-lradc MFD driver.
> 
> Note:  this patch won't compile in iio/testing without this patch:
> a8f447be8056 ("mfd: Add resource managed APIs for mfd_add_devices")
> 
> Signed-off-by: Ksenija Stanojevic 
> ---
> [...]
> +static int mxs_lradc_probe(struct platform_device *pdev)
> +{
> + const struct of_device_id *of_id;
> + struct device *dev = >dev;
> + struct device_node *node = dev->of_node;
> + struct mxs_lradc *lradc;
> + struct resource *iores;
> + struct mfd_cell *cells = NULL;
> + int ret = 0;
> + u32 ts_wires = 0;
> +
> + lradc = devm_kzalloc(>dev, sizeof(*lradc), GFP_KERNEL);
> + if (!lradc)
> + return -ENOMEM;
> +
> + of_id = of_match_device(mxs_lradc_dt_ids, >dev);
> + lradc->soc = (enum mxs_lradc_id)of_id->data;
> +
> + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + lradc->base = devm_ioremap_resource(dev, iores);

Out of couriosity: Is it common to pass already ioremapped addresses to (MFD 
cell) platform drivers? I would have expected, and done myself, that ioremap 
is done in the driver itself, same as the IRQ.

Best regards,
Alexander

Re: [PATCH v4 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD

[Alexander Stein]

On Thursday 04 August 2016 15:28:18, Ksenija Stanojevic wrote:
> Add core files for mxs-lradc MFD driver.
> 
> Note:  this patch won't compile in iio/testing without this patch:
> a8f447be8056 ("mfd: Add resource managed APIs for mfd_add_devices")
> 
> Signed-off-by: Ksenija Stanojevic 
> ---
> [...]
> +static int mxs_lradc_probe(struct platform_device *pdev)
> +{
> + const struct of_device_id *of_id;
> + struct device *dev = >dev;
> + struct device_node *node = dev->of_node;
> + struct mxs_lradc *lradc;
> + struct resource *iores;
> + struct mfd_cell *cells = NULL;
> + int ret = 0;
> + u32 ts_wires = 0;
> +
> + lradc = devm_kzalloc(>dev, sizeof(*lradc), GFP_KERNEL);
> + if (!lradc)
> + return -ENOMEM;
> +
> + of_id = of_match_device(mxs_lradc_dt_ids, >dev);
> + lradc->soc = (enum mxs_lradc_id)of_id->data;
> +
> + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + lradc->base = devm_ioremap_resource(dev, iores);

Out of couriosity: Is it common to pass already ioremapped addresses to (MFD 
cell) platform drivers? I would have expected, and done myself, that ioremap 
is done in the driver itself, same as the IRQ.

Best regards,
Alexander

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Alexander Stein]

On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
> On 26/07/2016 11:05, Alexander Stein wrote:
> > On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
> >> On 26/07/2016 10:21, Alexander Stein wrote:
> >>> On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
> >>>> iio_channel_get_all returns -ENODEV when it cannot find either phandles
> >>>> and
> >>>> properties in the Device Tree or channels whose consumer_dev_name
> >>>> matches
> >>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
> >>>> which might be probed after iio_hwmon.
> >>> 
> >>> Would it work if iio_channel_get_all returning ENODEV is used for
> >>> returning
> >>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
> >>> driver/device
> >>> dependencies seems not right for me at this place.
> >> 
> >> Then what if the iio_channel_get_all is called outside of the probe of a
> >> driver? We'll have to change the error code, things we are apparently
> >> trying to avoid (see v2 patches' discussions).
> > 
> > Maybe I didn't express my idea enough. I don't want to change the behavior
> > of iio_channel_get_all at all. Just the result evaluation of
> > iio_channel_get_all in iio_hwmon_probe. I have something link the patch
> > below in mind.
> > 
> > Best regards,
> > Alexander
> > ---
> > diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> > index b550ba5..e32d150 100644
> > --- a/drivers/hwmon/iio_hwmon.c
> > +++ b/drivers/hwmon/iio_hwmon.c
> > @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device
> > *pdev)
> > 
> > name = dev->of_node->name;
> > 
> > channels = iio_channel_get_all(dev);
> > 
> > -   if (IS_ERR(channels))
> > -   return PTR_ERR(channels);
> > +   if (IS_ERR(channels)) {
> > +   if (PTR_ERR(channels) == -ENODEV)
> > +   return -EPROBE_DEFER;
> > +   else
> > +   return PTR_ERR(channels);
> > +   }
> > 
> > st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> > if (st == NULL) {
> 
> Indeed, I misunderstood what you told me.
> 
> Actually, the patch you proposed is part of my v1
> (https://lkml.org/lkml/2016/6/28/203) and v2
> (https://lkml.org/lkml/2016/7/15/215).
> Jonathan and Guenter didn't really like the idea of changing the -ENODEV
> in -EPROBE_DEFER.

Thanks for the links.

> What I thought you were proposing was to change the -ENODEV return code
> inside iio_channel_get_all. This cannot be an option since the function
> might be called outside of a probe (it is not yet, but might be in the
> future?).

AFAICS this is a helper function not knowing about device probing itself. And 
it should stay at that.

> Of what I understood, two possibilities are then possible (proposed
> either by Guenter or Jonathan): either rework the iio framework to
> register iio map array earlier or to use late_initcall instead of init
> for the driver consuming the iio channels.

Interestingly using this problem would not arise due to module dependencies. 
But using late_initcall would mean this needs to be done on any driver using 
iio channels? I would rather keep those consumers simple.

Best regards,
Alexander

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Alexander Stein]

On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
> On 26/07/2016 11:05, Alexander Stein wrote:
> > On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
> >> On 26/07/2016 10:21, Alexander Stein wrote:
> >>> On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
> >>>> iio_channel_get_all returns -ENODEV when it cannot find either phandles
> >>>> and
> >>>> properties in the Device Tree or channels whose consumer_dev_name
> >>>> matches
> >>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
> >>>> which might be probed after iio_hwmon.
> >>> 
> >>> Would it work if iio_channel_get_all returning ENODEV is used for
> >>> returning
> >>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
> >>> driver/device
> >>> dependencies seems not right for me at this place.
> >> 
> >> Then what if the iio_channel_get_all is called outside of the probe of a
> >> driver? We'll have to change the error code, things we are apparently
> >> trying to avoid (see v2 patches' discussions).
> > 
> > Maybe I didn't express my idea enough. I don't want to change the behavior
> > of iio_channel_get_all at all. Just the result evaluation of
> > iio_channel_get_all in iio_hwmon_probe. I have something link the patch
> > below in mind.
> > 
> > Best regards,
> > Alexander
> > ---
> > diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> > index b550ba5..e32d150 100644
> > --- a/drivers/hwmon/iio_hwmon.c
> > +++ b/drivers/hwmon/iio_hwmon.c
> > @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device
> > *pdev)
> > 
> > name = dev->of_node->name;
> > 
> > channels = iio_channel_get_all(dev);
> > 
> > -   if (IS_ERR(channels))
> > -   return PTR_ERR(channels);
> > +   if (IS_ERR(channels)) {
> > +   if (PTR_ERR(channels) == -ENODEV)
> > +   return -EPROBE_DEFER;
> > +   else
> > +   return PTR_ERR(channels);
> > +   }
> > 
> > st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> > if (st == NULL) {
> 
> Indeed, I misunderstood what you told me.
> 
> Actually, the patch you proposed is part of my v1
> (https://lkml.org/lkml/2016/6/28/203) and v2
> (https://lkml.org/lkml/2016/7/15/215).
> Jonathan and Guenter didn't really like the idea of changing the -ENODEV
> in -EPROBE_DEFER.

Thanks for the links.

> What I thought you were proposing was to change the -ENODEV return code
> inside iio_channel_get_all. This cannot be an option since the function
> might be called outside of a probe (it is not yet, but might be in the
> future?).

AFAICS this is a helper function not knowing about device probing itself. And 
it should stay at that.

> Of what I understood, two possibilities are then possible (proposed
> either by Guenter or Jonathan): either rework the iio framework to
> register iio map array earlier or to use late_initcall instead of init
> for the driver consuming the iio channels.

Interestingly using this problem would not arise due to module dependencies. 
But using late_initcall would mean this needs to be done on any driver using 
iio channels? I would rather keep those consumers simple.

Best regards,
Alexander

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Alexander Stein]

On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
> On 26/07/2016 10:21, Alexander Stein wrote:
> > On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
> >> iio_channel_get_all returns -ENODEV when it cannot find either phandles
> >> and
> >> properties in the Device Tree or channels whose consumer_dev_name matches
> >> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
> >> which might be probed after iio_hwmon.
> > 
> > Would it work if iio_channel_get_all returning ENODEV is used for
> > returning
> > EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
> > driver/device
> > dependencies seems not right for me at this place.
> 
> Then what if the iio_channel_get_all is called outside of the probe of a
> driver? We'll have to change the error code, things we are apparently
> trying to avoid (see v2 patches' discussions).

Maybe I didn't express my idea enough. I don't want to change the behavior of  
iio_channel_get_all at all. Just the result evaluation of iio_channel_get_all 
in iio_hwmon_probe. I have something link the patch below in mind.

Best regards,
Alexander
---
diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
index b550ba5..e32d150 100644
--- a/drivers/hwmon/iio_hwmon.c
+++ b/drivers/hwmon/iio_hwmon.c
@@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device *pdev)
name = dev->of_node->name;
 
channels = iio_channel_get_all(dev);
-   if (IS_ERR(channels))
-   return PTR_ERR(channels);
+   if (IS_ERR(channels)) {
+   if (PTR_ERR(channels) == -ENODEV)
+   return -EPROBE_DEFER;
+   else
+   return PTR_ERR(channels);
+   }
 
st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
if (st == NULL) {

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Alexander Stein]

On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
> On 26/07/2016 10:21, Alexander Stein wrote:
> > On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
> >> iio_channel_get_all returns -ENODEV when it cannot find either phandles
> >> and
> >> properties in the Device Tree or channels whose consumer_dev_name matches
> >> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
> >> which might be probed after iio_hwmon.
> > 
> > Would it work if iio_channel_get_all returning ENODEV is used for
> > returning
> > EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
> > driver/device
> > dependencies seems not right for me at this place.
> 
> Then what if the iio_channel_get_all is called outside of the probe of a
> driver? We'll have to change the error code, things we are apparently
> trying to avoid (see v2 patches' discussions).

Maybe I didn't express my idea enough. I don't want to change the behavior of  
iio_channel_get_all at all. Just the result evaluation of iio_channel_get_all 
in iio_hwmon_probe. I have something link the patch below in mind.

Best regards,
Alexander
---
diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
index b550ba5..e32d150 100644
--- a/drivers/hwmon/iio_hwmon.c
+++ b/drivers/hwmon/iio_hwmon.c
@@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device *pdev)
name = dev->of_node->name;
 
channels = iio_channel_get_all(dev);
-   if (IS_ERR(channels))
-   return PTR_ERR(channels);
+   if (IS_ERR(channels)) {
+   if (PTR_ERR(channels) == -ENODEV)
+   return -EPROBE_DEFER;
+   else
+   return PTR_ERR(channels);
+   }
 
st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
if (st == NULL) {

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Alexander Stein]

On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
> iio_channel_get_all returns -ENODEV when it cannot find either phandles and
> properties in the Device Tree or channels whose consumer_dev_name matches
> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
> which might be probed after iio_hwmon.

Would it work if iio_channel_get_all returning ENODEV is used for returning 
EPROBE_DEFER in iio_channel_get_all? Using late initcalls for driver/device 
dependencies seems not right for me at this place.

Best regards,
Alexander

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Alexander Stein]

On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
> iio_channel_get_all returns -ENODEV when it cannot find either phandles and
> properties in the Device Tree or channels whose consumer_dev_name matches
> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
> which might be probed after iio_hwmon.

Would it work if iio_channel_get_all returning ENODEV is used for returning 
EPROBE_DEFER in iio_channel_get_all? Using late initcalls for driver/device 
dependencies seems not right for me at this place.

Best regards,
Alexander