[PATCH 0/2] Improve VCHIQ cache line size handling

[PATCH 0/2] Improve VCHIQ cache line size handling

[Phil Elwell]

Both sides of the VCHIQ communications mechanism need to agree on the cache
line size. Using an incorrect value can lead to data corruption, but having the
two sides using different values is usually worse.

In the absence of an obvious convenient run-time method to determine the
correct value in the ARCH=arm world, the downstream Raspberry Pi trees used a
Device Tree property, written by the firmware, to configure the kernel driver.
This method was vetoed during the upstreaming process, so a fixed value of 32
was used instead, and some corruptions ensued. This is take 2 at arriving at
the correct value.

Part one of the fix is deriving the correct value from the ARM's cpuid register.
Part two is a (seemingly cosmetic) correction of the Device Tree reg declaration
used by the driver, but it doubles as an indication to the Raspberry Pi firmware
that the kernel driver is running a recent kernel driver that chooses the
correct value. As such I would like very much for the DT patch not to be merged
before the driver patch - just tell me what hoops I need to jump through.

Phil Elwell (2):
  staging/vc04_services: Derive g_cache_line_size
  ARM: dts: bcm283x: Correct mailbox register sizes

 arch/arm/boot/dts/bcm2835-rpi.dtsi                 |  2 +-
 .../interface/vchiq_arm/vchiq_2835_arm.c           | 24 +++++++++++++++++-----
 2 files changed, 20 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH 1/2] staging/vc04_services: Derive g_cache_line_size

[Phil Elwell]

The ARM coprocessor registers include dcache line size, but there is no
function to expose this value. Rather than create a new one, use the
read_cpuid_id function to derive the correct value, which is 32 for
BCM2835 and 64 for BCM2836/7.

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
 .../interface/vchiq_arm/vchiq_2835_arm.c           | 24 +++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index e767209..3537f60 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -42,6 +42,7 @@
 #include <linux/uaccess.h>
 #include <linux/mm.h>
 #include <linux/of.h>
+#include <asm/cputype.h>
 #include <soc/bcm2835/raspberrypi-firmware.h>
 
 #define TOTAL_SLOTS (VCHIQ_SLOT_ZERO_SLOTS + 2 * 32)
@@ -81,13 +82,15 @@ static void __iomem *g_regs;
  * VPU firmware, which determines the required alignment of the
  * offsets/sizes in pagelists.
  *
- * Modern VPU firmware looks for a DT "cache-line-size" property in
- * the VCHIQ node and will overwrite it with the actual L2 cache size,
+ * Previous VPU firmware looked for a DT "cache-line-size" property in
+ * the VCHIQ node and would overwrite it with the actual L2 cache size,
  * which the kernel must then respect.  That property was rejected
- * upstream, so we have to use the VPU firmware's compatibility value
- * of 32.
+ * upstream, so we now rely on both sides to "do the right thing" independently
+ * of the other. To improve backwards compatibility, this new behaviour is
+ * signalled to the firmware by the use of a corrected "reg" property on the
+ * relevant Device Tree node.
  */
-static unsigned int g_cache_line_size = 32;
+static unsigned int g_cache_line_size;
 static unsigned int g_fragments_size;
 static char *g_fragments_base;
 static char *g_free_fragments;
@@ -127,6 +130,17 @@ int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
 	if (err < 0)
 		return err;
 
+	/*
+	 * The tempting L1_CACHE_BYTES macro doesn't work in the case of
+	 * a kernel built with bcm2835_defconfig running on a BCM2836/7
+	 * processor, hence the need for a runtime check. The dcache line size
+	 * is encoded in one of the coprocessor registers, but there is no
+	 * convenient way to access it short of embedded assembler, hence
+	 * the use of read_cpuid_id(). The following test evaluates to true
+	 * on a BCM2835 showing that it is ARMv6-ish, whereas
+	 * cpu_architecture() will indicate that it is an ARMv7.
+	 */
+	g_cache_line_size = ((read_cpuid_id() & 0x7f000) == 0x7b000) ? 32 : 64;
 	g_fragments_size = 2 * g_cache_line_size;
 
 	/* Allocate space for the channels in coherent memory */
-- 
2.7.4

Re: [PATCH 1/2] staging/vc04_services: Derive g_cache_line_size

[Stefan Wahren]

Hi Phil,

Am 12.09.2018 um 18:42 schrieb Phil Elwell:
> The ARM coprocessor registers include dcache line size, but there is no
> function to expose this value. Rather than create a new one, use the
> read_cpuid_id function to derive the correct value, which is 32 for
> BCM2835 and 64 for BCM2836/7.
>
> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
> ---
>  .../interface/vchiq_arm/vchiq_2835_arm.c           | 24 +++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
> index e767209..3537f60 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
> @@ -42,6 +42,7 @@
>  #include <linux/uaccess.h>
>  #include <linux/mm.h>
>  #include <linux/of.h>
> +#include <asm/cputype.h>
>  #include <soc/bcm2835/raspberrypi-firmware.h>
>  
>  #define TOTAL_SLOTS (VCHIQ_SLOT_ZERO_SLOTS + 2 * 32)
> @@ -81,13 +82,15 @@ static void __iomem *g_regs;
>   * VPU firmware, which determines the required alignment of the
>   * offsets/sizes in pagelists.
>   *
> - * Modern VPU firmware looks for a DT "cache-line-size" property in
> - * the VCHIQ node and will overwrite it with the actual L2 cache size,
> + * Previous VPU firmware looked for a DT "cache-line-size" property in
> + * the VCHIQ node and would overwrite it with the actual L2 cache size,
>   * which the kernel must then respect.  That property was rejected
> - * upstream, so we have to use the VPU firmware's compatibility value
> - * of 32.
> + * upstream, so we now rely on both sides to "do the right thing" independently
> + * of the other. To improve backwards compatibility, this new behaviour is
> + * signalled to the firmware by the use of a corrected "reg" property on the
> + * relevant Device Tree node.
>   */
> -static unsigned int g_cache_line_size = 32;
> +static unsigned int g_cache_line_size;
>  static unsigned int g_fragments_size;
>  static char *g_fragments_base;
>  static char *g_free_fragments;
> @@ -127,6 +130,17 @@ int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
>  	if (err < 0)
>  		return err;
>  
> +	/*
> +	 * The tempting L1_CACHE_BYTES macro doesn't work in the case of
> +	 * a kernel built with bcm2835_defconfig running on a BCM2836/7
> +	 * processor, hence the need for a runtime check. The dcache line size
> +	 * is encoded in one of the coprocessor registers, but there is no
> +	 * convenient way to access it short of embedded assembler, hence
> +	 * the use of read_cpuid_id(). The following test evaluates to true
> +	 * on a BCM2835 showing that it is ARMv6-ish, whereas
> +	 * cpu_architecture() will indicate that it is an ARMv7.
> +	 */
> +	g_cache_line_size = ((read_cpuid_id() & 0x7f000) == 0x7b000) ? 32 : 64;

is there a chance to use ARM_CPU_PART_ARM1176 or something else instead
of this magic numbers?

Sorry, i cannot remember the whole discussion but why we can use
different compatible here instead of using ARM specific stuff.

compatible = "brcm,bcm2836-vchiq", "brcm,bcm2835-vchiq"; // for BCM2836/7

compatible = "brcm,bcm2835"; // only for BCM2835


>  	g_fragments_size = 2 * g_cache_line_size;
>  
>  	/* Allocate space for the channels in coherent memory */

Re: [PATCH 1/2] staging/vc04_services: Derive g_cache_line_size

[Phil Elwell]

Hi Stefan,

On 14/09/2018 11:11, Stefan Wahren wrote:
> Hi Phil,
> 
> Am 12.09.2018 um 18:42 schrieb Phil Elwell:
>> The ARM coprocessor registers include dcache line size, but there is no
>> function to expose this value. Rather than create a new one, use the
>> read_cpuid_id function to derive the correct value, which is 32 for
>> BCM2835 and 64 for BCM2836/7.
>>
>> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
>> ---
>>  .../interface/vchiq_arm/vchiq_2835_arm.c           | 24 +++++++++++++++++-----
>>  1 file changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
>> index e767209..3537f60 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
>> @@ -42,6 +42,7 @@
>>  #include <linux/uaccess.h>
>>  #include <linux/mm.h>
>>  #include <linux/of.h>
>> +#include <asm/cputype.h>
>>  #include <soc/bcm2835/raspberrypi-firmware.h>
>>  
>>  #define TOTAL_SLOTS (VCHIQ_SLOT_ZERO_SLOTS + 2 * 32)
>> @@ -81,13 +82,15 @@ static void __iomem *g_regs;
>>   * VPU firmware, which determines the required alignment of the
>>   * offsets/sizes in pagelists.
>>   *
>> - * Modern VPU firmware looks for a DT "cache-line-size" property in
>> - * the VCHIQ node and will overwrite it with the actual L2 cache size,
>> + * Previous VPU firmware looked for a DT "cache-line-size" property in
>> + * the VCHIQ node and would overwrite it with the actual L2 cache size,
>>   * which the kernel must then respect.  That property was rejected
>> - * upstream, so we have to use the VPU firmware's compatibility value
>> - * of 32.
>> + * upstream, so we now rely on both sides to "do the right thing" independently
>> + * of the other. To improve backwards compatibility, this new behaviour is
>> + * signalled to the firmware by the use of a corrected "reg" property on the
>> + * relevant Device Tree node.
>>   */
>> -static unsigned int g_cache_line_size = 32;
>> +static unsigned int g_cache_line_size;
>>  static unsigned int g_fragments_size;
>>  static char *g_fragments_base;
>>  static char *g_free_fragments;
>> @@ -127,6 +130,17 @@ int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
>>  	if (err < 0)
>>  		return err;
>>  
>> +	/*
>> +	 * The tempting L1_CACHE_BYTES macro doesn't work in the case of
>> +	 * a kernel built with bcm2835_defconfig running on a BCM2836/7
>> +	 * processor, hence the need for a runtime check. The dcache line size
>> +	 * is encoded in one of the coprocessor registers, but there is no
>> +	 * convenient way to access it short of embedded assembler, hence
>> +	 * the use of read_cpuid_id(). The following test evaluates to true
>> +	 * on a BCM2835 showing that it is ARMv6-ish, whereas
>> +	 * cpu_architecture() will indicate that it is an ARMv7.
>> +	 */
>> +	g_cache_line_size = ((read_cpuid_id() & 0x7f000) == 0x7b000) ? 32 : 64;
> 
> is there a chance to use ARM_CPU_PART_ARM1176 or something else instead
> of this magic numbers?

One could. It's a trade-off between specific and human-readable vs. generic and less-so,
and possibly moot given the next question.

> Sorry, i cannot remember the whole discussion but why we can use
> different compatible here instead of using ARM specific stuff.
> 
> compatible = "brcm,bcm2836-vchiq", "brcm,bcm2835-vchiq"; // for BCM2836/7
> 
> compatible = "brcm,bcm2835"; // only for BCM2835

It wasn't an option you raised in the offline discussion. Assuming you meant
"brcm,bcm2835-vchiq", I suppose you could think of it as being a different type
of device if that is more acceptable.

>>  	g_fragments_size = 2 * g_cache_line_size;
>>  
>>  	/* Allocate space for the channels in coherent memory */

Re: [PATCH 1/2] staging/vc04_services: Derive g_cache_line_size

[Stefan Wahren]

Hi,

Am 14.09.2018 um 12:26 schrieb Phil Elwell:
> Hi Stefan,
>
> On 14/09/2018 11:11, Stefan Wahren wrote:
>> Hi Phil,
>>
>> Am 12.09.2018 um 18:42 schrieb Phil Elwell:
>>> The ARM coprocessor registers include dcache line size, but there is no
>>> function to expose this value. Rather than create a new one, use the
>>> read_cpuid_id function to derive the correct value, which is 32 for
>>> BCM2835 and 64 for BCM2836/7.
>>>
>>> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
>>> ---
>>>  .../interface/vchiq_arm/vchiq_2835_arm.c           | 24 +++++++++++++++++-----
>>>  1 file changed, 19 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
>>> index e767209..3537f60 100644
>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
>>> @@ -42,6 +42,7 @@
>>>  #include <linux/uaccess.h>
>>>  #include <linux/mm.h>
>>>  #include <linux/of.h>
>>> +#include <asm/cputype.h>
>>>  #include <soc/bcm2835/raspberrypi-firmware.h>
>>>  
>>>  #define TOTAL_SLOTS (VCHIQ_SLOT_ZERO_SLOTS + 2 * 32)
>>> @@ -81,13 +82,15 @@ static void __iomem *g_regs;
>>>   * VPU firmware, which determines the required alignment of the
>>>   * offsets/sizes in pagelists.
>>>   *
>>> - * Modern VPU firmware looks for a DT "cache-line-size" property in
>>> - * the VCHIQ node and will overwrite it with the actual L2 cache size,
>>> + * Previous VPU firmware looked for a DT "cache-line-size" property in
>>> + * the VCHIQ node and would overwrite it with the actual L2 cache size,
>>>   * which the kernel must then respect.  That property was rejected
>>> - * upstream, so we have to use the VPU firmware's compatibility value
>>> - * of 32.
>>> + * upstream, so we now rely on both sides to "do the right thing" independently
>>> + * of the other. To improve backwards compatibility, this new behaviour is
>>> + * signalled to the firmware by the use of a corrected "reg" property on the
>>> + * relevant Device Tree node.
>>>   */
>>> -static unsigned int g_cache_line_size = 32;
>>> +static unsigned int g_cache_line_size;
>>>  static unsigned int g_fragments_size;
>>>  static char *g_fragments_base;
>>>  static char *g_free_fragments;
>>> @@ -127,6 +130,17 @@ int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
>>>  	if (err < 0)
>>>  		return err;
>>>  
>>> +	/*
>>> +	 * The tempting L1_CACHE_BYTES macro doesn't work in the case of
>>> +	 * a kernel built with bcm2835_defconfig running on a BCM2836/7
>>> +	 * processor, hence the need for a runtime check. The dcache line size
>>> +	 * is encoded in one of the coprocessor registers, but there is no
>>> +	 * convenient way to access it short of embedded assembler, hence
>>> +	 * the use of read_cpuid_id(). The following test evaluates to true
>>> +	 * on a BCM2835 showing that it is ARMv6-ish, whereas
>>> +	 * cpu_architecture() will indicate that it is an ARMv7.
>>> +	 */
>>> +	g_cache_line_size = ((read_cpuid_id() & 0x7f000) == 0x7b000) ? 32 : 64;
>> is there a chance to use ARM_CPU_PART_ARM1176 or something else instead
>> of this magic numbers?
> One could. It's a trade-off between specific and human-readable vs. generic and less-so,
> and possibly moot given the next question.
>
>> Sorry, i cannot remember the whole discussion but why we can use
>> different compatible here instead of using ARM specific stuff.
>>
>> compatible = "brcm,bcm2836-vchiq", "brcm,bcm2835-vchiq"; // for BCM2836/7
>>
>> compatible = "brcm,bcm2835"; // only for BCM2835
> It wasn't an option you raised in the offline discussion. 

maybe this was too obvious :-(

> Assuming you meant
> "brcm,bcm2835-vchiq", I suppose you could think of it as being a different type
> of device if that is more acceptable.

Yes, it's a different SoC. Do you see any problems with the VC4 firmware
according to this approach?

>
>>>  	g_fragments_size = 2 * g_cache_line_size;
>>>  
>>>  	/* Allocate space for the channels in coherent memory */
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] staging/vc04_services: Derive g_cache_line_size

[Phil Elwell]

On 14/09/2018 12:03, Stefan Wahren wrote:
> Hi,
> 
> Am 14.09.2018 um 12:26 schrieb Phil Elwell:
>> Hi Stefan,
>>
>> On 14/09/2018 11:11, Stefan Wahren wrote:
>>> Hi Phil,
>>>
>>> Am 12.09.2018 um 18:42 schrieb Phil Elwell:
>>>> The ARM coprocessor registers include dcache line size, but there is no
>>>> function to expose this value. Rather than create a new one, use the
>>>> read_cpuid_id function to derive the correct value, which is 32 for
>>>> BCM2835 and 64 for BCM2836/7.
>>>>
>>>> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
>>>> ---
>>>>  .../interface/vchiq_arm/vchiq_2835_arm.c           | 24 +++++++++++++++++-----
>>>>  1 file changed, 19 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
>>>> index e767209..3537f60 100644
>>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
>>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
>>>> @@ -42,6 +42,7 @@
>>>>  #include <linux/uaccess.h>
>>>>  #include <linux/mm.h>
>>>>  #include <linux/of.h>
>>>> +#include <asm/cputype.h>
>>>>  #include <soc/bcm2835/raspberrypi-firmware.h>
>>>>  
>>>>  #define TOTAL_SLOTS (VCHIQ_SLOT_ZERO_SLOTS + 2 * 32)
>>>> @@ -81,13 +82,15 @@ static void __iomem *g_regs;
>>>>   * VPU firmware, which determines the required alignment of the
>>>>   * offsets/sizes in pagelists.
>>>>   *
>>>> - * Modern VPU firmware looks for a DT "cache-line-size" property in
>>>> - * the VCHIQ node and will overwrite it with the actual L2 cache size,
>>>> + * Previous VPU firmware looked for a DT "cache-line-size" property in
>>>> + * the VCHIQ node and would overwrite it with the actual L2 cache size,
>>>>   * which the kernel must then respect.  That property was rejected
>>>> - * upstream, so we have to use the VPU firmware's compatibility value
>>>> - * of 32.
>>>> + * upstream, so we now rely on both sides to "do the right thing" independently
>>>> + * of the other. To improve backwards compatibility, this new behaviour is
>>>> + * signalled to the firmware by the use of a corrected "reg" property on the
>>>> + * relevant Device Tree node.
>>>>   */
>>>> -static unsigned int g_cache_line_size = 32;
>>>> +static unsigned int g_cache_line_size;
>>>>  static unsigned int g_fragments_size;
>>>>  static char *g_fragments_base;
>>>>  static char *g_free_fragments;
>>>> @@ -127,6 +130,17 @@ int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
>>>>  	if (err < 0)
>>>>  		return err;
>>>>  
>>>> +	/*
>>>> +	 * The tempting L1_CACHE_BYTES macro doesn't work in the case of
>>>> +	 * a kernel built with bcm2835_defconfig running on a BCM2836/7
>>>> +	 * processor, hence the need for a runtime check. The dcache line size
>>>> +	 * is encoded in one of the coprocessor registers, but there is no
>>>> +	 * convenient way to access it short of embedded assembler, hence
>>>> +	 * the use of read_cpuid_id(). The following test evaluates to true
>>>> +	 * on a BCM2835 showing that it is ARMv6-ish, whereas
>>>> +	 * cpu_architecture() will indicate that it is an ARMv7.
>>>> +	 */
>>>> +	g_cache_line_size = ((read_cpuid_id() & 0x7f000) == 0x7b000) ? 32 : 64;
>>> is there a chance to use ARM_CPU_PART_ARM1176 or something else instead
>>> of this magic numbers?
>> One could. It's a trade-off between specific and human-readable vs. generic and less-so,
>> and possibly moot given the next question.
>>
>>> Sorry, i cannot remember the whole discussion but why we can use
>>> different compatible here instead of using ARM specific stuff.
>>>
>>> compatible = "brcm,bcm2836-vchiq", "brcm,bcm2835-vchiq"; // for BCM2836/7
>>>
>>> compatible = "brcm,bcm2835"; // only for BCM2835
>> It wasn't an option you raised in the offline discussion. 
> 
> maybe this was too obvious :-(
> 
>> Assuming you meant
>> "brcm,bcm2835-vchiq", I suppose you could think of it as being a different type
>> of device if that is more acceptable.
> 
> Yes, it's a different SoC. Do you see any problems with the VC4 firmware
> according to this approach?

No, it can be made to work. Expect V2 shortly.

> 
>>
>>>>  	g_fragments_size = 2 * g_cache_line_size;
>>>>  
>>>>  	/* Allocate space for the channels in coherent memory */
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 

Re: [PATCH 1/2] staging/vc04_services: Derive g_cache_line_size

[Stefan Wahren]

Am 14.09.2018 um 13:09 schrieb Phil Elwell:
> On 14/09/2018 12:03, Stefan Wahren wrote:
>> Hi,
>>
>> Am 14.09.2018 um 12:26 schrieb Phil Elwell:
>>> Hi Stefan,
>>>
>>> On 14/09/2018 11:11, Stefan Wahren wrote:
>>>> Hi Phil,
>>>>
>>>> Am 12.09.2018 um 18:42 schrieb Phil Elwell:
>>>>> The ARM coprocessor registers include dcache line size, but there is no
>>>>> function to expose this value. Rather than create a new one, use the
>>>>> read_cpuid_id function to derive the correct value, which is 32 for
>>>>> BCM2835 and 64 for BCM2836/7.
>>>>>
>>>>> Signed-off-by: Phil Elwell <phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
>>>>> ---
>>>>>  .../interface/vchiq_arm/vchiq_2835_arm.c           | 24 +++++++++++++++++-----
>>>>>  1 file changed, 19 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
>>>>> index e767209..3537f60 100644
>>>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
>>>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
>>>>> @@ -42,6 +42,7 @@
>>>>>  #include <linux/uaccess.h>
>>>>>  #include <linux/mm.h>
>>>>>  #include <linux/of.h>
>>>>> +#include <asm/cputype.h>
>>>>>  #include <soc/bcm2835/raspberrypi-firmware.h>
>>>>>  
>>>>>  #define TOTAL_SLOTS (VCHIQ_SLOT_ZERO_SLOTS + 2 * 32)
>>>>> @@ -81,13 +82,15 @@ static void __iomem *g_regs;
>>>>>   * VPU firmware, which determines the required alignment of the
>>>>>   * offsets/sizes in pagelists.
>>>>>   *
>>>>> - * Modern VPU firmware looks for a DT "cache-line-size" property in
>>>>> - * the VCHIQ node and will overwrite it with the actual L2 cache size,
>>>>> + * Previous VPU firmware looked for a DT "cache-line-size" property in
>>>>> + * the VCHIQ node and would overwrite it with the actual L2 cache size,
>>>>>   * which the kernel must then respect.  That property was rejected
>>>>> - * upstream, so we have to use the VPU firmware's compatibility value
>>>>> - * of 32.
>>>>> + * upstream, so we now rely on both sides to "do the right thing" independently
>>>>> + * of the other. To improve backwards compatibility, this new behaviour is
>>>>> + * signalled to the firmware by the use of a corrected "reg" property on the
>>>>> + * relevant Device Tree node.
>>>>>   */
>>>>> -static unsigned int g_cache_line_size = 32;
>>>>> +static unsigned int g_cache_line_size;
>>>>>  static unsigned int g_fragments_size;
>>>>>  static char *g_fragments_base;
>>>>>  static char *g_free_fragments;
>>>>> @@ -127,6 +130,17 @@ int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
>>>>>  	if (err < 0)
>>>>>  		return err;
>>>>>  
>>>>> +	/*
>>>>> +	 * The tempting L1_CACHE_BYTES macro doesn't work in the case of
>>>>> +	 * a kernel built with bcm2835_defconfig running on a BCM2836/7
>>>>> +	 * processor, hence the need for a runtime check. The dcache line size
>>>>> +	 * is encoded in one of the coprocessor registers, but there is no
>>>>> +	 * convenient way to access it short of embedded assembler, hence
>>>>> +	 * the use of read_cpuid_id(). The following test evaluates to true
>>>>> +	 * on a BCM2835 showing that it is ARMv6-ish, whereas
>>>>> +	 * cpu_architecture() will indicate that it is an ARMv7.
>>>>> +	 */
>>>>> +	g_cache_line_size = ((read_cpuid_id() & 0x7f000) == 0x7b000) ? 32 : 64;
>>>> is there a chance to use ARM_CPU_PART_ARM1176 or something else instead
>>>> of this magic numbers?
>>> One could. It's a trade-off between specific and human-readable vs. generic and less-so,
>>> and possibly moot given the next question.
>>>
>>>> Sorry, i cannot remember the whole discussion but why we can use
>>>> different compatible here instead of using ARM specific stuff.
>>>>
>>>> compatible = "brcm,bcm2836-vchiq", "brcm,bcm2835-vchiq"; // for BCM2836/7
>>>>
>>>> compatible = "brcm,bcm2835"; // only for BCM2835
>>> It wasn't an option you raised in the offline discussion. 
>> maybe this was too obvious :-(
>>
>>> Assuming you meant
>>> "brcm,bcm2835-vchiq", I suppose you could think of it as being a different type
>>> of device if that is more acceptable.
>> Yes, it's a different SoC. Do you see any problems with the VC4 firmware
>> according to this approach?
> No, it can be made to work. Expect V2 shortly.

Great. I forgot a mailing list in my first comment. Please also add
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org in CC.

>
>>>>>  	g_fragments_size = 2 * g_cache_line_size;
>>>>>  
>>>>>  	/* Allocate space for the channels in coherent memory */
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>

Re: [PATCH 1/2] staging/vc04_services: Derive g_cache_line_size

[Russell King - ARM Linux]

On Fri, Sep 14, 2018 at 12:11:54PM +0200, Stefan Wahren wrote:
> Hi Phil,
> 
> Am 12.09.2018 um 18:42 schrieb Phil Elwell:
> > +	/*
> > +	 * The tempting L1_CACHE_BYTES macro doesn't work in the case of
> > +	 * a kernel built with bcm2835_defconfig running on a BCM2836/7
> > +	 * processor, hence the need for a runtime check. The dcache line size
> > +	 * is encoded in one of the coprocessor registers, but there is no
> > +	 * convenient way to access it short of embedded assembler, hence
> > +	 * the use of read_cpuid_id(). The following test evaluates to true
> > +	 * on a BCM2835 showing that it is ARMv6-ish, whereas
> > +	 * cpu_architecture() will indicate that it is an ARMv7.
> > +	 */
> > +	g_cache_line_size = ((read_cpuid_id() & 0x7f000) == 0x7b000) ? 32 : 64;
> 
> is there a chance to use ARM_CPU_PART_ARM1176 or something else instead
> of this magic numbers?

In any case, reading the CPU ID and masking it in this way is really
not on.

The above checks that the ID _may_ be a "new ID" format, and that the
top four bits of the CPU part number are 11.  However, there are three
formats of the CPU ID register which can conflict if you do this kind
of trivial mask-and-test.

The relevant cases for are:

bits 19:16 inclusive being all ones define whether the ID is "new ID"
format with the additional CPU ID registers.  This is used for all
modern CPUs.

Otherwise, if bit 19 is clear, it is an older ID format, and bits
18:16 may contain '7'.

The value of bits 15:4 depend on the implementer - "1011" in 15:12 are
used by Intel for StrongARM1110 (though they won't have '7' in 18:16),
ARM Ltd for ARM1136, ARM1156, ARM1176 and ARM11MPCore (which will have
'7' in 18:16) - and hence will match this test.

So, the above code does not match just BCM2835, but also matches other
CPUs.

There is a reason why the old macros that allow the part number to be
fetched in asm/cputype.h are marked as deprecated, and that is to
encourage correct behaviour - to use both the part number and
implementer fields.

The comment is also wrong.  It claims that in order to read the cache
line size, one needs to use assembler.  asm/cputype.h has macros and
inline functions (of which read_cpuid_id() is one of them) to allow
access without having assembler encoded in drivers.  However, there is
a gotcha - the format of the cache type register also depends on the
CPU architecture, so it's not a simple case of grabbing the value of a
bitfield.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

[PATCH 2/2] ARM: dts: bcm283x: Correct mailbox register sizes

[Phil Elwell]

The size field in a Device Tree "reg" property is encoded in bytes, not
words.

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
 arch/arm/boot/dts/bcm2835-rpi.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi
index cb2d6d7..c481eab 100644
--- a/arch/arm/boot/dts/bcm2835-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi
@@ -32,7 +32,7 @@
 
 		mailbox@7e00b840 {
 			compatible = "brcm,bcm2835-vchiq";
-			reg = <0x7e00b840 0xf>;
+			reg = <0x7e00b840 0x3c>;
 			interrupts = <0 2>;
 		};
 	};
-- 
2.7.4

Re: [PATCH 0/2] Improve VCHIQ cache line size handling

[Stefan Wahren]

Hi,

> Phil Elwell <phil@raspberrypi.org> hat am 12. September 2018 um 18:42 geschrieben:
> 
> 
> Both sides of the VCHIQ communications mechanism need to agree on the cache
> line size. Using an incorrect value can lead to data corruption, but having the
> two sides using different values is usually worse.
> 
> In the absence of an obvious convenient run-time method to determine the
> correct value in the ARCH=arm world, the downstream Raspberry Pi trees used a
> Device Tree property, written by the firmware, to configure the kernel driver.
> This method was vetoed during the upstreaming process, so a fixed value of 32
> was used instead, and some corruptions ensued. This is take 2 at arriving at
> the correct value.
> 
> Part one of the fix is deriving the correct value from the ARM's cpuid register.
> Part two is a (seemingly cosmetic) correction of the Device Tree reg declaration
> used by the driver, but it doubles as an indication to the Raspberry Pi firmware
> that the kernel driver is running a recent kernel driver that chooses the
> correct value. As such I would like very much for the DT patch not to be merged
> before the driver patch - just tell me what hoops I need to jump through.
> 

even if this version isn't the best, the test results for vchiq_test -f 1 (using recent firmware 2018-09-10) are very good:

Raspberry 1 B, bcm2835_defconfig -> success
Raspberry 2 B, multi_v7_defconfig -> success
Raspberry 3 B, arm64/defconfig -> success

Thanks Stefan

[PATCH 0/2] Improve VCHIQ cache line size handling

[Phil Elwell]

Both sides of the VCHIQ communications mechanism need to agree on the cache
line size. Using an incorrect value can lead to data corruption, but having the
two sides using different values is usually worse.

In the absence of an obvious convenient run-time method to determine the
correct value in the ARCH=arm world, the downstream Raspberry Pi trees used a
Device Tree property, written by the firmware, to configure the kernel driver.
This method was vetoed during the upstreaming process, so a fixed value of 32
was used instead, and some corruptions ensued. This is take 2 at arriving at
the correct value.

Part one of the fix is deriving the correct value from the ARM's cpuid register.
Part two is a (seemingly cosmetic) correction of the Device Tree reg declaration
used by the driver, but it doubles as an indication to the Raspberry Pi firmware
that the kernel driver is running a recent kernel driver that chooses the
correct value. As such I would like very much for the DT patch not to be merged
before the driver patch - just tell me what hoops I need to jump through.

Phil Elwell (2):
  staging/vc04_services: Derive g_cache_line_size
  ARM: dts: bcm283x: Correct mailbox register sizes

 arch/arm/boot/dts/bcm2835-rpi.dtsi                 |  2 +-
 .../interface/vchiq_arm/vchiq_2835_arm.c           | 24 +++++++++++++++++-----
 2 files changed, 20 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH 1/2] staging/vc04_services: Derive g_cache_line_size

[Phil Elwell]

The ARM coprocessor registers include dcache line size, but there is no
function to expose this value. Rather than create a new one, use the
read_cpuid_id function to derive the correct value, which is 32 for
BCM2835 and 64 for BCM2836/7.

Signed-off-by: Phil Elwell <phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
---
 .../interface/vchiq_arm/vchiq_2835_arm.c           | 24 +++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index e767209..3537f60 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -42,6 +42,7 @@
 #include <linux/uaccess.h>
 #include <linux/mm.h>
 #include <linux/of.h>
+#include <asm/cputype.h>
 #include <soc/bcm2835/raspberrypi-firmware.h>
 
 #define TOTAL_SLOTS (VCHIQ_SLOT_ZERO_SLOTS + 2 * 32)
@@ -81,13 +82,15 @@ static void __iomem *g_regs;
  * VPU firmware, which determines the required alignment of the
  * offsets/sizes in pagelists.
  *
- * Modern VPU firmware looks for a DT "cache-line-size" property in
- * the VCHIQ node and will overwrite it with the actual L2 cache size,
+ * Previous VPU firmware looked for a DT "cache-line-size" property in
+ * the VCHIQ node and would overwrite it with the actual L2 cache size,
  * which the kernel must then respect.  That property was rejected
- * upstream, so we have to use the VPU firmware's compatibility value
- * of 32.
+ * upstream, so we now rely on both sides to "do the right thing" independently
+ * of the other. To improve backwards compatibility, this new behaviour is
+ * signalled to the firmware by the use of a corrected "reg" property on the
+ * relevant Device Tree node.
  */
-static unsigned int g_cache_line_size = 32;
+static unsigned int g_cache_line_size;
 static unsigned int g_fragments_size;
 static char *g_fragments_base;
 static char *g_free_fragments;
@@ -127,6 +130,17 @@ int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
 	if (err < 0)
 		return err;
 
+	/*
+	 * The tempting L1_CACHE_BYTES macro doesn't work in the case of
+	 * a kernel built with bcm2835_defconfig running on a BCM2836/7
+	 * processor, hence the need for a runtime check. The dcache line size
+	 * is encoded in one of the coprocessor registers, but there is no
+	 * convenient way to access it short of embedded assembler, hence
+	 * the use of read_cpuid_id(). The following test evaluates to true
+	 * on a BCM2835 showing that it is ARMv6-ish, whereas
+	 * cpu_architecture() will indicate that it is an ARMv7.
+	 */
+	g_cache_line_size = ((read_cpuid_id() & 0x7f000) == 0x7b000) ? 32 : 64;
 	g_fragments_size = 2 * g_cache_line_size;
 
 	/* Allocate space for the channels in coherent memory */
-- 
2.7.4