[PATCH 0/5] VCHI DT bindings

[PATCH 0/5] VCHI DT bindings

[Eric Anholt]

This series removes a gross DT property and adds a DT binding for the
VCHI driver so that we can start probing it automatically on RPi.

This is useful immediately as it lets you use vcgencmd for measuring
clocks (we have code for this in Linux but no userland interface).
It'll also be useful as a base for getting the consumer drivers to
probe automatically, which I'm expecting we'll do using a bus provided
by VCHI, since we'll be able to do some service discovery on VCHI.

VCHI itself can't just use a bus from the firmware driver, because of
the regs and interrupts -- the firmware link it has is just to allow a
request to the VPU to bring up VCHI on this hardware doorbell.

Greg: I'm good with merging patch 1 whenever, but I'd like to make
sure Stefan has a chance to take a look at the cache-line-size
property change, at least.

Eric Anholt (5):
  staging: vc04_services: Remove dead FRAGMENTS_T
  staging: vc04_services: Remove cache-line-size property.
  dt-bindings: soc: Add a binding for the Broadcom VCHI services.
  ARM: dts: bcm2835: Add VCHI node to the Raspberry Pi boards.
  staging: vc04_services: Mark the "DT bindings" job done.

 .../bindings/soc/bcm/brcm,bcm2835-vchiq.txt        | 18 ++++++++++++++++
 arch/arm/boot/dts/bcm2835-rpi.dtsi                 |  7 ++++++
 drivers/staging/vc04_services/interface/vchi/TODO  |  9 ++------
 .../interface/vchiq_arm/vchiq_2835_arm.c           | 25 +++++++---------------
 .../interface/vchiq_arm/vchiq_pagelist.h           |  6 ------
 5 files changed, 35 insertions(+), 30 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt

-- 
2.16.2

[PATCH 1/5] staging: vc04_services: Remove dead FRAGMENTS_T

[Eric Anholt]

It's not used in-tree, or in the downstream tree.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_pagelist.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_pagelist.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_pagelist.h
index 926c247fd9d3..a6c5f7cc78f0 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_pagelist.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_pagelist.h
@@ -49,9 +49,4 @@ typedef struct pagelist_struct {
 			 */
 } PAGELIST_T;
 
-typedef struct fragments_struct {
-	char headbuf[CACHE_LINE_SIZE];
-	char tailbuf[CACHE_LINE_SIZE];
-} FRAGMENTS_T;
-
 #endif /* VCHIQ_PAGELIST_H */
-- 
2.16.2

[PATCH 2/5] staging: vc04_services: Remove cache-line-size property.

[Eric Anholt]

This was just a way for the DT-passed value to get out of sync with
what Linux has configured the ARM for.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 .../interface/vchiq_arm/vchiq_2835_arm.c           | 25 +++++++---------------
 .../interface/vchiq_arm/vchiq_pagelist.h           |  1 -
 2 files changed, 8 insertions(+), 18 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 b59ef14890aa..e0e01c812036 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
@@ -77,7 +77,6 @@ struct vchiq_pagelist_info {
 };
 
 static void __iomem *g_regs;
-static unsigned int g_cache_line_size = sizeof(CACHE_LINE_SIZE);
 static unsigned int g_fragments_size;
 static char *g_fragments_base;
 static char *g_free_fragments;
@@ -117,15 +116,7 @@ int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
 	if (err < 0)
 		return err;
 
-	err = of_property_read_u32(dev->of_node, "cache-line-size",
-				   &g_cache_line_size);
-
-	if (err) {
-		dev_err(dev, "Missing cache-line-size property\n");
-		return -ENODEV;
-	}
-
-	g_fragments_size = 2 * g_cache_line_size;
+	g_fragments_size = 2 * cache_line_size();
 
 	/* Allocate space for the channels in coherent memory */
 	slot_mem_size = PAGE_ALIGN(TOTAL_SLOTS * VCHIQ_SLOT_SIZE);
@@ -548,9 +539,9 @@ create_pagelist(char __user *buf, size_t count, unsigned short type)
 
 	/* Partial cache lines (fragments) require special measures */
 	if ((type == PAGELIST_READ) &&
-		((pagelist->offset & (g_cache_line_size - 1)) ||
+		((pagelist->offset & (cache_line_size() - 1)) ||
 		((pagelist->offset + pagelist->length) &
-		(g_cache_line_size - 1)))) {
+		 (cache_line_size() - 1)))) {
 		char *fragments;
 
 		if (down_interruptible(&g_free_fragments_sema) != 0) {
@@ -598,10 +589,10 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
 			g_fragments_size;
 		int head_bytes, tail_bytes;
 
-		head_bytes = (g_cache_line_size - pagelist->offset) &
-			(g_cache_line_size - 1);
+		head_bytes = (cache_line_size() - pagelist->offset) &
+			(cache_line_size() - 1);
 		tail_bytes = (pagelist->offset + actual) &
-			(g_cache_line_size - 1);
+			(cache_line_size() - 1);
 
 		if ((actual >= 0) && (head_bytes != 0)) {
 			if (head_bytes > actual)
@@ -617,8 +608,8 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
 			(tail_bytes != 0)) {
 			memcpy((char *)kmap(pages[num_pages - 1]) +
 				((pagelist->offset + actual) &
-				(PAGE_SIZE - 1) & ~(g_cache_line_size - 1)),
-				fragments + g_cache_line_size,
+				(PAGE_SIZE - 1) & ~(cache_line_size() - 1)),
+				fragments + cache_line_size(),
 				tail_bytes);
 			kunmap(pages[num_pages - 1]);
 		}
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_pagelist.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_pagelist.h
index a6c5f7cc78f0..bec411061554 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_pagelist.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_pagelist.h
@@ -34,7 +34,6 @@
 #ifndef VCHIQ_PAGELIST_H
 #define VCHIQ_PAGELIST_H
 
-#define CACHE_LINE_SIZE 32
 #define PAGELIST_WRITE 0
 #define PAGELIST_READ 1
 #define PAGELIST_READ_WITH_FRAGMENTS 2
-- 
2.16.2

[PATCH 3/5] dt-bindings: soc: Add a binding for the Broadcom VCHI services.

[Eric Anholt]

The VCHI communication channel can be provided by BCM283x and Capri
SoCs, to communicate with the VPU-side OS services.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 .../devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt

diff --git a/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt
new file mode 100644
index 000000000000..4055a8977f3e
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt
@@ -0,0 +1,18 @@
+Broadcom VCHIQ firmware services
+
+Required properties:
+
+- compatible:	Should be "brcm,bcm2835-vchiq"
+- reg:		Physical base address and length of the doorbell register pair
+- interrupts:	The interrupt number
+		  See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
+- firmware:	Reference to the RPi firmware device node
+
+Example:
+
+vchi@7e00b840 {
+	compatible = "brcm,bcm2835-vchiq";
+	reg = <0x7e00b840 0xf>;
+	interrupts = <0 2>;
+	firmware = <&firmware>;
+};
-- 
2.16.2

[PATCH 4/5] ARM: dts: bcm2835: Add VCHI node to the Raspberry Pi boards.

[Eric Anholt]

The VCHI firmware communication channel operates in parallel with our
other mailbox-based channel.  This is the communication channel that
exposes the firmware's media decode/encode and ISP interfaces.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 arch/arm/boot/dts/bcm2835-rpi.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi
index e36c392a2b8f..c128bc6a1435 100644
--- a/arch/arm/boot/dts/bcm2835-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi
@@ -27,6 +27,13 @@
 			firmware = <&firmware>;
 			#power-domain-cells = <1>;
 		};
+
+		vchi@7e00b840 {
+			compatible = "brcm,bcm2835-vchiq";
+			reg = <0x7e00b840 0xf>;
+			interrupts = <0 2>;
+			firmware = <&firmware>;
+		};
 	};
 };
 
-- 
2.16.2

[PATCH 5/5] staging: vc04_services: Mark the "DT bindings" job done.

[Eric Anholt]

Now we just need to get the other drivers merged and finish the style
cleanups/garbage collecting so we can get out of staging.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/staging/vc04_services/interface/vchi/TODO | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchi/TODO b/drivers/staging/vc04_services/interface/vchi/TODO
index df93154b1aa6..46b20a1961a2 100644
--- a/drivers/staging/vc04_services/interface/vchi/TODO
+++ b/drivers/staging/vc04_services/interface/vchi/TODO
@@ -1,9 +1,4 @@
-1) Write a DT binding doc and get the corresponding DT node merged to
-   bcm2835.
-
-This will let the driver probe when enabled.
-
-2) Import drivers using VCHI.
+1) Import drivers using VCHI.
 
 VCHI is just a tool to let drivers talk to the firmware.  Here are
 some of the ones we want:
@@ -26,7 +21,7 @@ some of the ones we want:
   to manage these buffers as dmabufs so that we can zero-copy import
   camera images into vc4 for rendering/display.
 
-3) Garbage-collect unused code
+2) Garbage-collect unused code
 
 One of the reasons this driver wasn't upstreamed previously was that
 there's a lot code that got built that's probably unnecessary these
-- 
2.16.2

Re: [PATCH 3/5] dt-bindings: soc: Add a binding for the Broadcom VCHI services.

[Rob Herring]

On Mon, Mar 5, 2018 at 2:28 PM, Eric Anholt <eric@anholt.net> wrote:
> The VCHI communication channel can be provided by BCM283x and Capri

VCHI or VCHIQ?

> SoCs, to communicate with the VPU-side OS services.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  .../devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt
>
> diff --git a/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt
> new file mode 100644
> index 000000000000..4055a8977f3e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt
> @@ -0,0 +1,18 @@
> +Broadcom VCHIQ firmware services
> +
> +Required properties:
> +
> +- compatible:  Should be "brcm,bcm2835-vchiq"
> +- reg:         Physical base address and length of the doorbell register pair
> +- interrupts:  The interrupt number
> +                 See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
> +- firmware:    Reference to the RPi firmware device node

There's only one, just get it by compatible or path.

> +
> +Example:
> +
> +vchi@7e00b840 {
> +       compatible = "brcm,bcm2835-vchiq";
> +       reg = <0x7e00b840 0xf>;
> +       interrupts = <0 2>;
> +       firmware = <&firmware>;
> +};
> --
> 2.16.2
>

Re: [PATCH 2/5] staging: vc04_services: Remove cache-line-size property.

[Stefan Wahren]

Hi Eric,


Am 05.03.2018 um 21:28 schrieb Eric Anholt:
> This was just a way for the DT-passed value to get out of sync with
> what Linux has configured the ARM for.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>   .../interface/vchiq_arm/vchiq_2835_arm.c           | 25 +++++++---------------
>   .../interface/vchiq_arm/vchiq_pagelist.h           |  1 -
>   2 files changed, 8 insertions(+), 18 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 b59ef14890aa..e0e01c812036 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
> @@ -77,7 +77,6 @@ struct vchiq_pagelist_info {
>   };
>   
>   static void __iomem *g_regs;
> -static unsigned int g_cache_line_size = sizeof(CACHE_LINE_SIZE);
>   static unsigned int g_fragments_size;
>   static char *g_fragments_base;
>   static char *g_free_fragments;
> @@ -117,15 +116,7 @@ int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
>   	if (err < 0)
>   		return err;
>   
> -	err = of_property_read_u32(dev->of_node, "cache-line-size",
> -				   &g_cache_line_size);
> -
> -	if (err) {
> -		dev_err(dev, "Missing cache-line-size property\n");
> -		return -ENODEV;
> -	}
> -
> -	g_fragments_size = 2 * g_cache_line_size;
> +	g_fragments_size = 2 * cache_line_size();
>   
>   	/* Allocate space for the channels in coherent memory */
>   	slot_mem_size = PAGE_ALIGN(TOTAL_SLOTS * VCHIQ_SLOT_SIZE);
> @@ -548,9 +539,9 @@ create_pagelist(char __user *buf, size_t count, unsigned short type)
>   
>   	/* Partial cache lines (fragments) require special measures */
>   	if ((type == PAGELIST_READ) &&
> -		((pagelist->offset & (g_cache_line_size - 1)) ||
> +		((pagelist->offset & (cache_line_size() - 1)) ||
>   		((pagelist->offset + pagelist->length) &
> -		(g_cache_line_size - 1)))) {
> +		 (cache_line_size() - 1)))) {
>   		char *fragments;
>   
>   		if (down_interruptible(&g_free_fragments_sema) != 0) {
> @@ -598,10 +589,10 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
>   			g_fragments_size;
>   		int head_bytes, tail_bytes;
>   
> -		head_bytes = (g_cache_line_size - pagelist->offset) &
> -			(g_cache_line_size - 1);
> +		head_bytes = (cache_line_size() - pagelist->offset) &
> +			(cache_line_size() - 1);
>   		tail_bytes = (pagelist->offset + actual) &
> -			(g_cache_line_size - 1);
> +			(cache_line_size() - 1);

should it be so easy? Back in 2016 we said that cache_line_size() won't 
work. I always thought that we need the cache line size of L2 not of the 
L1 one.

Did you already test the behavior for these combinations?
BCM2835 ARM32, expected cache line size = 32
BCM2836 ARM32, expected cache line size = 64
BCM2837 ARM32, expected cache line size = 64
BCM2837 ARM64, expected cache line size = 64

Regards
Stefan

>   
>   		if ((actual >= 0) && (head_bytes != 0)) {
>   			if (head_bytes > actual)
> @@ -617,8 +608,8 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
>   			(tail_bytes != 0)) {
>   			memcpy((char *)kmap(pages[num_pages - 1]) +
>   				((pagelist->offset + actual) &
> -				(PAGE_SIZE - 1) & ~(g_cache_line_size - 1)),
> -				fragments + g_cache_line_size,
> +				(PAGE_SIZE - 1) & ~(cache_line_size() - 1)),
> +				fragments + cache_line_size(),
>   				tail_bytes);
>   			kunmap(pages[num_pages - 1]);
>   		}
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_pagelist.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_pagelist.h
> index a6c5f7cc78f0..bec411061554 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_pagelist.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_pagelist.h
> @@ -34,7 +34,6 @@
>   #ifndef VCHIQ_PAGELIST_H
>   #define VCHIQ_PAGELIST_H
>   
> -#define CACHE_LINE_SIZE 32
>   #define PAGELIST_WRITE 0
>   #define PAGELIST_READ 1
>   #define PAGELIST_READ_WITH_FRAGMENTS 2

Re: [PATCH 2/5] staging: vc04_services: Remove cache-line-size property.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 3997 bytes --]

Stefan Wahren <stefan.wahren@i2se.com> writes:

> Hi Eric,
>
>
> Am 05.03.2018 um 21:28 schrieb Eric Anholt:
>> This was just a way for the DT-passed value to get out of sync with
>> what Linux has configured the ARM for.
>>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> ---
>>   .../interface/vchiq_arm/vchiq_2835_arm.c           | 25 +++++++---------------
>>   .../interface/vchiq_arm/vchiq_pagelist.h           |  1 -
>>   2 files changed, 8 insertions(+), 18 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 b59ef14890aa..e0e01c812036 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
>> @@ -77,7 +77,6 @@ struct vchiq_pagelist_info {
>>   };
>>   
>>   static void __iomem *g_regs;
>> -static unsigned int g_cache_line_size = sizeof(CACHE_LINE_SIZE);
>>   static unsigned int g_fragments_size;
>>   static char *g_fragments_base;
>>   static char *g_free_fragments;
>> @@ -117,15 +116,7 @@ int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
>>   	if (err < 0)
>>   		return err;
>>   
>> -	err = of_property_read_u32(dev->of_node, "cache-line-size",
>> -				   &g_cache_line_size);
>> -
>> -	if (err) {
>> -		dev_err(dev, "Missing cache-line-size property\n");
>> -		return -ENODEV;
>> -	}
>> -
>> -	g_fragments_size = 2 * g_cache_line_size;
>> +	g_fragments_size = 2 * cache_line_size();
>>   
>>   	/* Allocate space for the channels in coherent memory */
>>   	slot_mem_size = PAGE_ALIGN(TOTAL_SLOTS * VCHIQ_SLOT_SIZE);
>> @@ -548,9 +539,9 @@ create_pagelist(char __user *buf, size_t count, unsigned short type)
>>   
>>   	/* Partial cache lines (fragments) require special measures */
>>   	if ((type == PAGELIST_READ) &&
>> -		((pagelist->offset & (g_cache_line_size - 1)) ||
>> +		((pagelist->offset & (cache_line_size() - 1)) ||
>>   		((pagelist->offset + pagelist->length) &
>> -		(g_cache_line_size - 1)))) {
>> +		 (cache_line_size() - 1)))) {
>>   		char *fragments;
>>   
>>   		if (down_interruptible(&g_free_fragments_sema) != 0) {
>> @@ -598,10 +589,10 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
>>   			g_fragments_size;
>>   		int head_bytes, tail_bytes;
>>   
>> -		head_bytes = (g_cache_line_size - pagelist->offset) &
>> -			(g_cache_line_size - 1);
>> +		head_bytes = (cache_line_size() - pagelist->offset) &
>> +			(cache_line_size() - 1);
>>   		tail_bytes = (pagelist->offset + actual) &
>> -			(g_cache_line_size - 1);
>> +			(cache_line_size() - 1);
>
> should it be so easy? Back in 2016 we said that cache_line_size() won't 
> work. I always thought that we need the cache line size of L2 not of the 
> L1 one.
>
> Did you already test the behavior for these combinations?
> BCM2835 ARM32, expected cache line size = 32
> BCM2836 ARM32, expected cache line size = 64
> BCM2837 ARM32, expected cache line size = 64
> BCM2837 ARM64, expected cache line size = 64

I didn't explicitly test, but was going by:

config ARM_L1_CACHE_SHIFT_6
	bool
	default y if CPU_V7
	help
	  Setting ARM L1 cache line size to 64 Bytes.

config ARM_L1_CACHE_SHIFT_7
	bool
	help
	  Setting ARM L1 cache line size to 128 Bytes.

config ARM_L1_CACHE_SHIFT
	int
	default 7 if ARM_L1_CACHE_SHIFT_7
	default 6 if ARM_L1_CACHE_SHIFT_6
	default 5

and only L1_CACHE_SHIFT_7 gets selected by UNIPHIER and neither one is
accessible by menus.

I think you're technically correct that it's the size of L2 that matters
(or, specifically, the hardcoded value that the firmware is using on its
side for the fragments list handling.  It overrides a cache-line-size DT
property with that number if present).  However, I think L1==L2 cache
line size this should be a safe assumption for us.

Phil, any opinion?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 2/5] staging: vc04_services: Remove cache-line-size property.

[Phil Elwell]

Hi Eric,

On 06/03/2018 19:02, Eric Anholt wrote:
> Stefan Wahren <stefan.wahren@i2se.com> writes:
> 
>> Hi Eric,
>>
>>
>> Am 05.03.2018 um 21:28 schrieb Eric Anholt:
>>> This was just a way for the DT-passed value to get out of sync with
>>> what Linux has configured the ARM for.
>>>
>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>> ---
>>>    .../interface/vchiq_arm/vchiq_2835_arm.c           | 25 +++++++---------------
>>>    .../interface/vchiq_arm/vchiq_pagelist.h           |  1 -
>>>    2 files changed, 8 insertions(+), 18 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 b59ef14890aa..e0e01c812036 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
>>> @@ -77,7 +77,6 @@ struct vchiq_pagelist_info {
>>>    };
>>>    
>>>    static void __iomem *g_regs;
>>> -static unsigned int g_cache_line_size = sizeof(CACHE_LINE_SIZE);
>>>    static unsigned int g_fragments_size;
>>>    static char *g_fragments_base;
>>>    static char *g_free_fragments;
>>> @@ -117,15 +116,7 @@ int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
>>>    	if (err < 0)
>>>    		return err;
>>>    
>>> -	err = of_property_read_u32(dev->of_node, "cache-line-size",
>>> -				   &g_cache_line_size);
>>> -
>>> -	if (err) {
>>> -		dev_err(dev, "Missing cache-line-size property\n");
>>> -		return -ENODEV;
>>> -	}
>>> -
>>> -	g_fragments_size = 2 * g_cache_line_size;
>>> +	g_fragments_size = 2 * cache_line_size();
>>>    
>>>    	/* Allocate space for the channels in coherent memory */
>>>    	slot_mem_size = PAGE_ALIGN(TOTAL_SLOTS * VCHIQ_SLOT_SIZE);
>>> @@ -548,9 +539,9 @@ create_pagelist(char __user *buf, size_t count, unsigned short type)
>>>    
>>>    	/* Partial cache lines (fragments) require special measures */
>>>    	if ((type == PAGELIST_READ) &&
>>> -		((pagelist->offset & (g_cache_line_size - 1)) ||
>>> +		((pagelist->offset & (cache_line_size() - 1)) ||
>>>    		((pagelist->offset + pagelist->length) &
>>> -		(g_cache_line_size - 1)))) {
>>> +		 (cache_line_size() - 1)))) {
>>>    		char *fragments;
>>>    
>>>    		if (down_interruptible(&g_free_fragments_sema) != 0) {
>>> @@ -598,10 +589,10 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
>>>    			g_fragments_size;
>>>    		int head_bytes, tail_bytes;
>>>    
>>> -		head_bytes = (g_cache_line_size - pagelist->offset) &
>>> -			(g_cache_line_size - 1);
>>> +		head_bytes = (cache_line_size() - pagelist->offset) &
>>> +			(cache_line_size() - 1);
>>>    		tail_bytes = (pagelist->offset + actual) &
>>> -			(g_cache_line_size - 1);
>>> +			(cache_line_size() - 1);
>>
>> should it be so easy? Back in 2016 we said that cache_line_size() won't
>> work. I always thought that we need the cache line size of L2 not of the
>> L1 one.
>>
>> Did you already test the behavior for these combinations?
>> BCM2835 ARM32, expected cache line size = 32
>> BCM2836 ARM32, expected cache line size = 64
>> BCM2837 ARM32, expected cache line size = 64
>> BCM2837 ARM64, expected cache line size = 64
> 
> I didn't explicitly test, but was going by:
> 
> config ARM_L1_CACHE_SHIFT_6
> 	bool
> 	default y if CPU_V7
> 	help
> 	  Setting ARM L1 cache line size to 64 Bytes.
> 
> config ARM_L1_CACHE_SHIFT_7
> 	bool
> 	help
> 	  Setting ARM L1 cache line size to 128 Bytes.
> 
> config ARM_L1_CACHE_SHIFT
> 	int
> 	default 7 if ARM_L1_CACHE_SHIFT_7
> 	default 6 if ARM_L1_CACHE_SHIFT_6
> 	default 5
> 
> and only L1_CACHE_SHIFT_7 gets selected by UNIPHIER and neither one is
> accessible by menus.
> 
> I think you're technically correct that it's the size of L2 that matters
> (or, specifically, the hardcoded value that the firmware is using on its
> side for the fragments list handling.  It overrides a cache-line-size DT
> property with that number if present).  However, I think L1==L2 cache
> line size this should be a safe assumption for us.
> 
> Phil, any opinion?

It is the L2 cache line size that matters, but as long as you end up with
the numbers Stefan mentioned - 32 on BCM2835, 64 on BCM2836 and BCM2837 -
I'm not too bothered how you get there.

Phil

Re: [PATCH 2/5] staging: vc04_services: Remove cache-line-size property.

[Stefan Wahren]

Hi Phil,

> Phil Elwell <phil@raspberrypi.org> hat am 7. März 2018 um 09:02 geschrieben:
> 
> 
> Hi Eric,
> 
> On 06/03/2018 19:02, Eric Anholt wrote:
> > Stefan Wahren <stefan.wahren@i2se.com> writes:
> > 
> >> Hi Eric,
> >>
> >>
> >> Am 05.03.2018 um 21:28 schrieb Eric Anholt:
> >>> This was just a way for the DT-passed value to get out of sync with
> >>> what Linux has configured the ARM for.
> >>>
> >>> Signed-off-by: Eric Anholt <eric@anholt.net>
> >>> ---
> >>>    .../interface/vchiq_arm/vchiq_2835_arm.c           | 25 +++++++---------------
> >>>    .../interface/vchiq_arm/vchiq_pagelist.h           |  1 -
> >>>    2 files changed, 8 insertions(+), 18 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 b59ef14890aa..e0e01c812036 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
> >>> @@ -77,7 +77,6 @@ struct vchiq_pagelist_info {
> >>>    };
> >>>    
> >>>    static void __iomem *g_regs;
> >>> -static unsigned int g_cache_line_size = sizeof(CACHE_LINE_SIZE);
> >>>    static unsigned int g_fragments_size;
> >>>    static char *g_fragments_base;
> >>>    static char *g_free_fragments;
> >>> @@ -117,15 +116,7 @@ int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
> >>>    	if (err < 0)
> >>>    		return err;
> >>>    
> >>> -	err = of_property_read_u32(dev->of_node, "cache-line-size",
> >>> -				   &g_cache_line_size);
> >>> -
> >>> -	if (err) {
> >>> -		dev_err(dev, "Missing cache-line-size property\n");
> >>> -		return -ENODEV;
> >>> -	}
> >>> -
> >>> -	g_fragments_size = 2 * g_cache_line_size;
> >>> +	g_fragments_size = 2 * cache_line_size();
> >>>    
> >>>    	/* Allocate space for the channels in coherent memory */
> >>>    	slot_mem_size = PAGE_ALIGN(TOTAL_SLOTS * VCHIQ_SLOT_SIZE);
> >>> @@ -548,9 +539,9 @@ create_pagelist(char __user *buf, size_t count, unsigned short type)
> >>>    
> >>>    	/* Partial cache lines (fragments) require special measures */
> >>>    	if ((type == PAGELIST_READ) &&
> >>> -		((pagelist->offset & (g_cache_line_size - 1)) ||
> >>> +		((pagelist->offset & (cache_line_size() - 1)) ||
> >>>    		((pagelist->offset + pagelist->length) &
> >>> -		(g_cache_line_size - 1)))) {
> >>> +		 (cache_line_size() - 1)))) {
> >>>    		char *fragments;
> >>>    
> >>>    		if (down_interruptible(&g_free_fragments_sema) != 0) {
> >>> @@ -598,10 +589,10 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
> >>>    			g_fragments_size;
> >>>    		int head_bytes, tail_bytes;
> >>>    
> >>> -		head_bytes = (g_cache_line_size - pagelist->offset) &
> >>> -			(g_cache_line_size - 1);
> >>> +		head_bytes = (cache_line_size() - pagelist->offset) &
> >>> +			(cache_line_size() - 1);
> >>>    		tail_bytes = (pagelist->offset + actual) &
> >>> -			(g_cache_line_size - 1);
> >>> +			(cache_line_size() - 1);
> >>
> >> should it be so easy? Back in 2016 we said that cache_line_size() won't
> >> work. I always thought that we need the cache line size of L2 not of the
> >> L1 one.
> >>
> >> Did you already test the behavior for these combinations?
> >> BCM2835 ARM32, expected cache line size = 32
> >> BCM2836 ARM32, expected cache line size = 64
> >> BCM2837 ARM32, expected cache line size = 64
> >> BCM2837 ARM64, expected cache line size = 64
> > 
> > I didn't explicitly test, but was going by:
> > 
> > config ARM_L1_CACHE_SHIFT_6
> > 	bool
> > 	default y if CPU_V7
> > 	help
> > 	  Setting ARM L1 cache line size to 64 Bytes.
> > 
> > config ARM_L1_CACHE_SHIFT_7
> > 	bool
> > 	help
> > 	  Setting ARM L1 cache line size to 128 Bytes.
> > 
> > config ARM_L1_CACHE_SHIFT
> > 	int
> > 	default 7 if ARM_L1_CACHE_SHIFT_7
> > 	default 6 if ARM_L1_CACHE_SHIFT_6
> > 	default 5
> > 
> > and only L1_CACHE_SHIFT_7 gets selected by UNIPHIER and neither one is
> > accessible by menus.
> > 
> > I think you're technically correct that it's the size of L2 that matters
> > (or, specifically, the hardcoded value that the firmware is using on its
> > side for the fragments list handling.  It overrides a cache-line-size DT
> > property with that number if present).  However, I think L1==L2 cache
> > line size this should be a safe assumption for us.
> > 
> > Phil, any opinion?
> 
> It is the L2 cache line size that matters, but as long as you end up with
> the numbers Stefan mentioned - 32 on BCM2835, 64 on BCM2836 and BCM2837 -
> I'm not too bothered how you get there.

i think a kernel with bcm2835_defconfig on RPi 2 could be such a corncase.

Am i right that the firmware doesn't rely on the existence of "cache-line-size"?

Btw it would be nice to get fixed the corruption on ARM64 [1].

Stefan

[1] - https://github.com/lategoodbye/rpi-zero/issues/23

> 
> Phil
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] staging: vc04_services: Remove cache-line-size property.

[Phil Elwell]

On 07/03/2018 12:10, Stefan Wahren wrote:
> Hi Phil,

<snip>

>> It is the L2 cache line size that matters, but as long as you end up with
>> the numbers Stefan mentioned - 32 on BCM2835, 64 on BCM2836 and BCM2837 -
>> I'm not too bothered how you get there.
> 
> i think a kernel with bcm2835_defconfig on RPi 2 could be such a corncase.
> 
> Am i right that the firmware doesn't rely on the existence of "cache-line-size"?

Because of the way partial cache lines are handled it is more important that the
two sides agree than that the value is correct. As a result, the firmware treats
the absence of a "cache_line_size" DT parameter (that sets the "cache-line-size"
property) in the DTB as an indication that the kernel driver pre-dates the ability
to switch, and uses the old fixed value of 32 as a fallback. Otherwise it sets the
parameter and the internal value used by the VPU-side VCHIQ to the correct value.

There are a number of ways to fix this, the easiest of which is to assume that the kernel
driver will either read the property or be able to work out the correct value, so
the VPU should always use the correct value regardless of the success of applying
the parameter/changing the property.

> Btw it would be nice to get fixed the corruption on ARM64 [1].

This is almost certainly due to the logic described above.

> 
> Stefan
> 
> [1] - https://github.com/lategoodbye/rpi-zero/issues/23
> 
>>
>> Phil
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] staging: vc04_services: Remove cache-line-size property.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1629 bytes --]

Phil Elwell <phil@raspberrypi.org> writes:

> On 07/03/2018 12:10, Stefan Wahren wrote:
>> Hi Phil,
>
> <snip>
>
>>> It is the L2 cache line size that matters, but as long as you end up with
>>> the numbers Stefan mentioned - 32 on BCM2835, 64 on BCM2836 and BCM2837 -
>>> I'm not too bothered how you get there.
>> 
>> i think a kernel with bcm2835_defconfig on RPi 2 could be such a corncase.
>> 
>> Am i right that the firmware doesn't rely on the existence of "cache-line-size"?
>
> Because of the way partial cache lines are handled it is more important that the
> two sides agree than that the value is correct. As a result, the firmware treats
> the absence of a "cache_line_size" DT parameter (that sets the "cache-line-size"
> property) in the DTB as an indication that the kernel driver pre-dates the ability
> to switch, and uses the old fixed value of 32 as a fallback. Otherwise it sets the
> parameter and the internal value used by the VPU-side VCHIQ to the correct value.
>
> There are a number of ways to fix this, the easiest of which is to assume that the kernel
> driver will either read the property or be able to work out the correct value, so
> the VPU should always use the correct value regardless of the success of applying
> the parameter/changing the property.

Oh, interesting.  So with my patch, we end up with a mismatch where VPU
is treating things as 32, and the kernel is using 64.  I wasn't seeing
errors in vchiq_test in this state, which is a bit concerning.

I'll go ahead and drop this patch and replace it with a comment in the
code about this discussion.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]