Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1457963 > unrolled thread

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

Started byMarc Zyngier <marc.zyngier@arm.com>
First post2016-08-08 20:10 +0200
Last post2016-08-09 17:30 +0200
Articles 4 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [Patch v3 10/11] driver/edac/layerscape_edac: Add Layerscape  EDAC support Marc Zyngier <marc.zyngier@arm.com> - 2016-08-08 20:10 +0200
    Re: [Patch v3 10/11] driver/edac/layerscape_edac: Add Layerscape EDAC  support york sun <york.sun@nxp.com> - 2016-08-08 22:00 +0200
      Re: [Patch v3 10/11] driver/edac/layerscape_edac: Add Layerscape  EDAC support Will Deacon <will.deacon@arm.com> - 2016-08-09 13:20 +0200
        Re: [Patch v3 10/11] driver/edac/layerscape_edac: Add Layerscape EDAC  support york sun <york.sun@nxp.com> - 2016-08-09 17:30 +0200

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

FromMarc Zyngier <marc.zyngier@arm.com>
Date2016-08-08 20:10 +0200
SubjectRe: [Patch v3 10/11] driver/edac/layerscape_edac: Add Layerscape EDAC support
Message-ID<s3X9T-4P8-11@gated-at.bofh.it>
On Thu, 4 Aug 2016 15:58:35 -0700
York Sun <york.sun@nxp.com> wrote:

> Add DDR EDAC for ARM-based compatible controllers. Both big-endian
> and little-endian are supported.
> 
> Signed-off-by: York Sun <york.sun@nxp.com>
> 
> ---
> 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
> 
> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index 7ef1d05..185a215 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -41,6 +41,7 @@ config ARCH_EXYNOS
>  
>  config ARCH_LAYERSCAPE
>  	bool "ARMv8 based Freescale Layerscape SoC family"
> +	select EDAC_SUPPORT
>  	help
>  	  This enables support for the Freescale Layerscape SoC family.
>  
> diff --git a/arch/arm/include/asm/edac.h b/arch/arm64/include/asm/edac.h
> similarity index 79%
> copy from arch/arm/include/asm/edac.h
> copy to arch/arm64/include/asm/edac.h
> index 5189fa8..36a226c 100644
> --- a/arch/arm/include/asm/edac.h
> +++ b/arch/arm64/include/asm/edac.h
> @@ -18,16 +18,15 @@
>  #define ASM_EDAC_H
>  /*
>   * ECC atomic, DMA, SMP and interrupt safe scrub function.
> - * Implements the per arch edac_atomic_scrub() that EDAC use for software
> + * Implements the per arch atomic_scrub() that EDAC use for software
>   * ECC scrubbing.  It reads memory and then writes back the original
>   * value, allowing the hardware to detect and correct memory errors.
>   */
> -
> -static inline void edac_atomic_scrub(void *va, u32 size)
> +static inline void atomic_scrub(void *va, u32 size)
>  {
> -#if __LINUX_ARM_ARCH__ >= 6
> -	unsigned int *virt_addr = va;
> -	unsigned int temp, temp2;
> +	unsigned long *virt_addr = va;
> +	unsigned long temp;
> +	unsigned int temp2;
>  	unsigned int i;
>  
>  	for (i = 0; i < size / sizeof(*virt_addr); i++, virt_addr++) {
> @@ -35,15 +34,13 @@ static inline void edac_atomic_scrub(void *va, u32 size)
>  		 * so we are interrupt, DMA and SMP safe.
>  		 */
>  		__asm__ __volatile__("\n"
> -			"1:	ldrex	%0, [%2]\n"
> -			"	strex	%1, %0, [%2]\n"
> -			"	teq	%1, #0\n"
> -			"	bne	1b\n"
> +			"1:	ldxr	%0, [%2]\n"
> +			"	stxr	%w1, %0, [%2]\n"
> +			"	cbnz	%w1, 1b\n"

Hiding architecture code in a driver, are we? Hmmm.

Also, how can this be safe if you have non cache-coherent DMA going on?

>  			: "=&r"(temp), "=&r"(temp2)
>  			: "r"(virt_addr)
> -			: "cc");
> +			: "memory");
>  	}
> -#endif
>  }
>  
>  #endif
> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
> index b77197d..d09c008 100644
> --- a/arch/arm64/include/asm/irq.h
> +++ b/arch/arm64/include/asm/irq.h
> @@ -11,6 +11,10 @@
>  #include <asm-generic/irq.h>
>  #include <asm/thread_info.h>
>  
> +#ifndef NO_IRQ
> +#define NO_IRQ	((unsigned int)(-1))
> +#endif

NAK.

NO_IRQ shouldn't exist at all, and if it had to exist, it would have
the value 0. This really is a leftover of a distant past that has no
real relevance on recent architectures.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

[toc] | [next] | [standalone]

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

Fromyork sun <york.sun@nxp.com>
Date2016-08-08 22:00 +0200
SubjectRe: [Patch v3 10/11] driver/edac/layerscape_edac: Add Layerscape EDAC support
Message-ID<s3YSl-5KE-1@gated-at.bofh.it>
In reply to#1457963
On 08/08/2016 11:07 AM, Marc Zyngier wrote:
> On Thu, 4 Aug 2016 15:58:35 -0700
> York Sun <york.sun@nxp.com> wrote:
>
>> Add DDR EDAC for ARM-based compatible controllers. Both big-endian
>> and little-endian are supported.
>>
>> Signed-off-by: York Sun <york.sun@nxp.com>
>>
>> ---
>> 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
>>
>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>> index 7ef1d05..185a215 100644
>> --- a/arch/arm64/Kconfig.platforms
>> +++ b/arch/arm64/Kconfig.platforms
>> @@ -41,6 +41,7 @@ config ARCH_EXYNOS
>>
>>  config ARCH_LAYERSCAPE
>>  	bool "ARMv8 based Freescale Layerscape SoC family"
>> +	select EDAC_SUPPORT
>>  	help
>>  	  This enables support for the Freescale Layerscape SoC family.
>>
>> diff --git a/arch/arm/include/asm/edac.h b/arch/arm64/include/asm/edac.h
>> similarity index 79%
>> copy from arch/arm/include/asm/edac.h
>> copy to arch/arm64/include/asm/edac.h
>> index 5189fa8..36a226c 100644
>> --- a/arch/arm/include/asm/edac.h
>> +++ b/arch/arm64/include/asm/edac.h
>> @@ -18,16 +18,15 @@
>>  #define ASM_EDAC_H
>>  /*
>>   * ECC atomic, DMA, SMP and interrupt safe scrub function.
>> - * Implements the per arch edac_atomic_scrub() that EDAC use for software
>> + * Implements the per arch atomic_scrub() that EDAC use for software
>>   * ECC scrubbing.  It reads memory and then writes back the original
>>   * value, allowing the hardware to detect and correct memory errors.
>>   */
>> -
>> -static inline void edac_atomic_scrub(void *va, u32 size)
>> +static inline void atomic_scrub(void *va, u32 size)
>>  {
>> -#if __LINUX_ARM_ARCH__ >= 6
>> -	unsigned int *virt_addr = va;
>> -	unsigned int temp, temp2;
>> +	unsigned long *virt_addr = va;
>> +	unsigned long temp;
>> +	unsigned int temp2;
>>  	unsigned int i;
>>
>>  	for (i = 0; i < size / sizeof(*virt_addr); i++, virt_addr++) {
>> @@ -35,15 +34,13 @@ static inline void edac_atomic_scrub(void *va, u32 size)
>>  		 * so we are interrupt, DMA and SMP safe.
>>  		 */
>>  		__asm__ __volatile__("\n"
>> -			"1:	ldrex	%0, [%2]\n"
>> -			"	strex	%1, %0, [%2]\n"
>> -			"	teq	%1, #0\n"
>> -			"	bne	1b\n"
>> +			"1:	ldxr	%0, [%2]\n"
>> +			"	stxr	%w1, %0, [%2]\n"
>> +			"	cbnz	%w1, 1b\n"
>
> Hiding architecture code in a driver, are we? Hmmm.

This adds atomic_scrub() for arm64. Arm already has this function. Are 
you suggesting to separate this part as an individual patch?
>
> Also, how can this be safe if you have non cache-coherent DMA going on?

I am clueless on this topic. What's your suggestion.

>
>>  			: "=&r"(temp), "=&r"(temp2)
>>  			: "r"(virt_addr)
>> -			: "cc");
>> +			: "memory");
>>  	}
>> -#endif
>>  }
>>
>>  #endif
>> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
>> index b77197d..d09c008 100644
>> --- a/arch/arm64/include/asm/irq.h
>> +++ b/arch/arm64/include/asm/irq.h
>> @@ -11,6 +11,10 @@
>>  #include <asm-generic/irq.h>
>>  #include <asm/thread_info.h>
>>
>> +#ifndef NO_IRQ
>> +#define NO_IRQ	((unsigned int)(-1))
>> +#endif
>
> NAK.
>
> NO_IRQ shouldn't exist at all, and if it had to exist, it would have
> the value 0. This really is a leftover of a distant past that has no
> real relevance on recent architectures.
>

OK. I will drop the NO_IRQ in the driver.

York

[toc] | [prev] | [next] | [standalone]

#1458610

FromWill Deacon <will.deacon@arm.com>
Date2016-08-09 13:20 +0200
Message-ID<s4deF-6O8-1@gated-at.bofh.it>
In reply to#1458190
On Mon, Aug 08, 2016 at 07:56:04PM +0000, york sun wrote:
> On 08/08/2016 11:07 AM, Marc Zyngier wrote:
> > On Thu, 4 Aug 2016 15:58:35 -0700
> > York Sun <york.sun@nxp.com> wrote:
> >
> >> Add DDR EDAC for ARM-based compatible controllers. Both big-endian
> >> and little-endian are supported.
> >>
> >> Signed-off-by: York Sun <york.sun@nxp.com>
> >>
> >> ---
> >> 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
> >>
> >> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> >> index 7ef1d05..185a215 100644
> >> --- a/arch/arm64/Kconfig.platforms
> >> +++ b/arch/arm64/Kconfig.platforms
> >> @@ -41,6 +41,7 @@ config ARCH_EXYNOS
> >>
> >>  config ARCH_LAYERSCAPE
> >>  	bool "ARMv8 based Freescale Layerscape SoC family"
> >> +	select EDAC_SUPPORT
> >>  	help
> >>  	  This enables support for the Freescale Layerscape SoC family.
> >>
> >> diff --git a/arch/arm/include/asm/edac.h b/arch/arm64/include/asm/edac.h
> >> similarity index 79%
> >> copy from arch/arm/include/asm/edac.h
> >> copy to arch/arm64/include/asm/edac.h
> >> index 5189fa8..36a226c 100644
> >> --- a/arch/arm/include/asm/edac.h
> >> +++ b/arch/arm64/include/asm/edac.h
> >> @@ -18,16 +18,15 @@
> >>  #define ASM_EDAC_H
> >>  /*
> >>   * ECC atomic, DMA, SMP and interrupt safe scrub function.
> >> - * Implements the per arch edac_atomic_scrub() that EDAC use for software
> >> + * Implements the per arch atomic_scrub() that EDAC use for software
> >>   * ECC scrubbing.  It reads memory and then writes back the original
> >>   * value, allowing the hardware to detect and correct memory errors.
> >>   */
> >> -
> >> -static inline void edac_atomic_scrub(void *va, u32 size)
> >> +static inline void atomic_scrub(void *va, u32 size)
> >>  {
> >> -#if __LINUX_ARM_ARCH__ >= 6
> >> -	unsigned int *virt_addr = va;
> >> -	unsigned int temp, temp2;
> >> +	unsigned long *virt_addr = va;
> >> +	unsigned long temp;
> >> +	unsigned int temp2;
> >>  	unsigned int i;
> >>
> >>  	for (i = 0; i < size / sizeof(*virt_addr); i++, virt_addr++) {
> >> @@ -35,15 +34,13 @@ static inline void edac_atomic_scrub(void *va, u32 size)
> >>  		 * so we are interrupt, DMA and SMP safe.
> >>  		 */
> >>  		__asm__ __volatile__("\n"
> >> -			"1:	ldrex	%0, [%2]\n"
> >> -			"	strex	%1, %0, [%2]\n"
> >> -			"	teq	%1, #0\n"
> >> -			"	bne	1b\n"
> >> +			"1:	ldxr	%0, [%2]\n"
> >> +			"	stxr	%w1, %0, [%2]\n"
> >> +			"	cbnz	%w1, 1b\n"
> >
> > Hiding architecture code in a driver, are we? Hmmm.
> 
> This adds atomic_scrub() for arm64. Arm already has this function. Are 
> you suggesting to separate this part as an individual patch?
> >
> > Also, how can this be safe if you have non cache-coherent DMA going on?
> 
> I am clueless on this topic. What's your suggestion.

We've discussed this in the past for ARMv7 and the bottom line is that
it's not safe to scrub memory backing non-coherent DMA buffers. You will
likely end up with data corruption if you do that.

The two options are:

  (1) Restrict scrubbing to non-DMA memory, or
  (2) Only enable scrubbing if the system is completely cache-coherent

The problem with (1) is that we can't detect that, particularly when
the streaming DMA API is in use.

The problem with (2) is that you can have a coherent system that decides
dynamically to drop in and out of coherency as a result of, e.g. PCI
NoSnoop transactions.

Will

[toc] | [prev] | [next] | [standalone]

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

Fromyork sun <york.sun@nxp.com>
Date2016-08-09 17:30 +0200
SubjectRe: [Patch v3 10/11] driver/edac/layerscape_edac: Add Layerscape EDAC support
Message-ID<s4h8B-UE-7@gated-at.bofh.it>
In reply to#1458610
On 08/09/2016 04:12 AM, Will Deacon wrote:
> On Mon, Aug 08, 2016 at 07:56:04PM +0000, york sun wrote:
>> On 08/08/2016 11:07 AM, Marc Zyngier wrote:
>>> On Thu, 4 Aug 2016 15:58:35 -0700
>>> York Sun <york.sun@nxp.com> wrote:
>>>
>>>> Add DDR EDAC for ARM-based compatible controllers. Both big-endian
>>>> and little-endian are supported.
>>>>
>>>> Signed-off-by: York Sun <york.sun@nxp.com>
>>>>
>>>> ---
>>>> 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
>>>>
>>>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>>>> index 7ef1d05..185a215 100644
>>>> --- a/arch/arm64/Kconfig.platforms
>>>> +++ b/arch/arm64/Kconfig.platforms
>>>> @@ -41,6 +41,7 @@ config ARCH_EXYNOS
>>>>
>>>>  config ARCH_LAYERSCAPE
>>>>  	bool "ARMv8 based Freescale Layerscape SoC family"
>>>> +	select EDAC_SUPPORT
>>>>  	help
>>>>  	  This enables support for the Freescale Layerscape SoC family.
>>>>
>>>> diff --git a/arch/arm/include/asm/edac.h b/arch/arm64/include/asm/edac.h
>>>> similarity index 79%
>>>> copy from arch/arm/include/asm/edac.h
>>>> copy to arch/arm64/include/asm/edac.h
>>>> index 5189fa8..36a226c 100644
>>>> --- a/arch/arm/include/asm/edac.h
>>>> +++ b/arch/arm64/include/asm/edac.h
>>>> @@ -18,16 +18,15 @@
>>>>  #define ASM_EDAC_H
>>>>  /*
>>>>   * ECC atomic, DMA, SMP and interrupt safe scrub function.
>>>> - * Implements the per arch edac_atomic_scrub() that EDAC use for software
>>>> + * Implements the per arch atomic_scrub() that EDAC use for software
>>>>   * ECC scrubbing.  It reads memory and then writes back the original
>>>>   * value, allowing the hardware to detect and correct memory errors.
>>>>   */
>>>> -
>>>> -static inline void edac_atomic_scrub(void *va, u32 size)
>>>> +static inline void atomic_scrub(void *va, u32 size)
>>>>  {
>>>> -#if __LINUX_ARM_ARCH__ >= 6
>>>> -	unsigned int *virt_addr = va;
>>>> -	unsigned int temp, temp2;
>>>> +	unsigned long *virt_addr = va;
>>>> +	unsigned long temp;
>>>> +	unsigned int temp2;
>>>>  	unsigned int i;
>>>>
>>>>  	for (i = 0; i < size / sizeof(*virt_addr); i++, virt_addr++) {
>>>> @@ -35,15 +34,13 @@ static inline void edac_atomic_scrub(void *va, u32 size)
>>>>  		 * so we are interrupt, DMA and SMP safe.
>>>>  		 */
>>>>  		__asm__ __volatile__("\n"
>>>> -			"1:	ldrex	%0, [%2]\n"
>>>> -			"	strex	%1, %0, [%2]\n"
>>>> -			"	teq	%1, #0\n"
>>>> -			"	bne	1b\n"
>>>> +			"1:	ldxr	%0, [%2]\n"
>>>> +			"	stxr	%w1, %0, [%2]\n"
>>>> +			"	cbnz	%w1, 1b\n"
>>>
>>> Hiding architecture code in a driver, are we? Hmmm.
>>
>> This adds atomic_scrub() for arm64. Arm already has this function. Are
>> you suggesting to separate this part as an individual patch?
>>>
>>> Also, how can this be safe if you have non cache-coherent DMA going on?
>>
>> I am clueless on this topic. What's your suggestion.
>
> We've discussed this in the past for ARMv7 and the bottom line is that
> it's not safe to scrub memory backing non-coherent DMA buffers. You will
> likely end up with data corruption if you do that.
>
> The two options are:
>
>   (1) Restrict scrubbing to non-DMA memory, or
>   (2) Only enable scrubbing if the system is completely cache-coherent
>
> The problem with (1) is that we can't detect that, particularly when
> the streaming DMA API is in use.
>
> The problem with (2) is that you can have a coherent system that decides
> dynamically to drop in and out of coherency as a result of, e.g. PCI
> NoSnoop transactions.
>

Good information! I realize I added this code on an older kernel and 
rebased to the latest one. This code is actually not needed, as long as 
CONFIG_EDAC_ATOMIC_SCRUB.

You said this has been discussed for ARMv7, what was the conclusion? I 
see edac_atomic_scrub() is still used for ARM.

York

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web