[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-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

[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-FnsA7b+Nu9XbIbC87yuRow@public.gmane.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,

> Phil Elwell <phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org> hat am 12. September 2018 um 17:06 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.
> 

thanks for sending this series. But please resend with Russell King, Arnd Bergmann, linux-arm-kernel and bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w@public.gmane.org mailing list in CC.

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

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