[PATCH v2 1/6] staging: vc04_services: Replace "firmware" node with a compatible lookup.

[PATCH v2 1/6] staging: vc04_services: Replace "firmware" node with a compatible lookup.

[Eric Anholt]

This was requested by Rob Herring in DT bindings review.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
v2: new patch

 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index f5cefda49b22..8068c0308b34 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -3594,7 +3594,8 @@ static int vchiq_probe(struct platform_device *pdev)
 	struct rpi_firmware *fw;
 	int err;
 
-	fw_node = of_parse_phandle(pdev->dev.of_node, "firmware", 0);
+	fw_node = of_find_compatible_node(NULL, NULL,
+					  "raspberrypi,bcm2835-firmware");
 	if (!fw_node) {
 		dev_err(&pdev->dev, "Missing firmware node\n");
 		return -ENOENT;
-- 
2.16.2

[PATCH v2 2/6] staging: vc04_services: Add comments describing g_cache_line_size.

[Eric Anholt]

It's been tempting to replace this with (L1) cache_line_size(), but
that's really not what the value is about.  It's about coordinating
the condition for the pagelist fragment behavior between the two
sides.

Signed-off-by: Eric Anholt <eric@anholt.net>
---

v2: new patch to replace the cache_line_size() patch.

 .../staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 10 +++++++++-
 .../staging/vc04_services/interface/vchiq_arm/vchiq_pagelist.h |  1 -
 2 files changed, 9 insertions(+), 2 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..3aeffa189f64 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,11 @@ struct vchiq_pagelist_info {
 };
 
 static void __iomem *g_regs;
-static unsigned int g_cache_line_size = sizeof(CACHE_LINE_SIZE);
+/* This value is the size of the L2 cache lines as understood by the
+ * VPU firmware, which determines the required alignment of the
+ * offsets/sizes in pagelists.
+ */
+static unsigned int g_cache_line_size = 32;
 static unsigned int g_fragments_size;
 static char *g_fragments_base;
 static char *g_free_fragments;
@@ -117,6 +121,10 @@ int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
 	if (err < 0)
 		return err;
 
+	/* Get the L2 cache-line-size as set by the VPU.  If the
+	 * property is missing, then the firmware assumes an older
+	 * kernel using a 32-byte cache line size for compatibility.
+	 */
 	err = of_property_read_u32(dev->of_node, "cache-line-size",
 				   &g_cache_line_size);
 
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 v2 3/6] dt-bindings: soc: Add a binding for the Broadcom VCHIQ services.

[Eric Anholt]

The VCHIQ 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>
---

v2: dropped firmware property, added cache-line-size.

 .../bindings/soc/bcm/brcm,bcm2835-vchiq.txt        | 28 ++++++++++++++++++++++
 1 file changed, 28 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..cdef4abc5e47
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt
@@ -0,0 +1,28 @@
+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
+
+Optional properties:
+
+- cache-line-size:
+		Size of L2 cache lines.  The VPU firmware detects
+		  this property and overrides it with the actual L2
+		  cache line size it's using when loading the
+		  device-tree.  Determines the required alignment of
+		  offsets/sizes of VCHIQ pagelists.  If missing, the
+		  firmware assumes an older kernel using 32-byte
+		  alignment.
+
+Example:
+
+vchiq@7e00b840 {
+	compatible = "brcm,bcm2835-vchiq";
+	reg = <0x7e00b840 0xf>;
+	interrupts = <0 2>;
+	cache-line-size: <32>;
+};
-- 
2.16.2

[PATCH v2 4/6] ARM: dts: bcm2835: Add VCHIQ node to the Raspberry Pi boards.

[Eric Anholt]

The VCHIQ 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>
---

v2: dropped firmware property, added cache-line-size.

 arch/arm/boot/dts/bcm2835-rpi.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi
index e36c392a2b8f..ba6dd25ac93f 100644
--- a/arch/arm/boot/dts/bcm2835-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi
@@ -27,6 +27,17 @@
 			firmware = <&firmware>;
 			#power-domain-cells = <1>;
 		};
+
+		vchiq@7e00b840 {
+			compatible = "brcm,bcm2835-vchiq";
+			reg = <0x7e00b840 0xf>;
+			interrupts = <0 2>;
+			/* The VPU firmware will override this DT property
+			 * (if present) with the L2 cache line size value it
+			 * wants to use.
+			 */
+			cache-line-size = <32>;
+		};
 	};
 };
 
-- 
2.16.2

[PATCH v2 5/6] 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>
---

v2: no changes

 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

[PATCH v2 6/6] staging: vc04_services: Remove vchiq_queue_bulk_{transmit,receive}.

[Eric Anholt]

These are dead code, including in the downstream Raspberry Pi tree.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
v2: new patch while I was looking around the code.

 .../vc04_services/interface/vchiq_arm/vchiq_arm.c    | 20 --------------------
 .../vc04_services/interface/vchiq_arm/vchiq_if.h     | 10 ----------
 2 files changed, 30 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 8068c0308b34..24d456b0a6f0 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -406,26 +406,6 @@ VCHIQ_STATUS_T vchiq_open_service(
 }
 EXPORT_SYMBOL(vchiq_open_service);
 
-VCHIQ_STATUS_T
-vchiq_queue_bulk_transmit(VCHIQ_SERVICE_HANDLE_T handle,
-	const void *data, unsigned int size, void *userdata)
-{
-	return vchiq_bulk_transfer(handle,
-		VCHI_MEM_HANDLE_INVALID, (void *)data, size, userdata,
-		VCHIQ_BULK_MODE_CALLBACK, VCHIQ_BULK_TRANSMIT);
-}
-EXPORT_SYMBOL(vchiq_queue_bulk_transmit);
-
-VCHIQ_STATUS_T
-vchiq_queue_bulk_receive(VCHIQ_SERVICE_HANDLE_T handle, void *data,
-	unsigned int size, void *userdata)
-{
-	return vchiq_bulk_transfer(handle,
-		VCHI_MEM_HANDLE_INVALID, data, size, userdata,
-		VCHIQ_BULK_MODE_CALLBACK, VCHIQ_BULK_RECEIVE);
-}
-EXPORT_SYMBOL(vchiq_queue_bulk_receive);
-
 VCHIQ_STATUS_T
 vchiq_bulk_transmit(VCHIQ_SERVICE_HANDLE_T handle, const void *data,
 	unsigned int size, void *userdata, VCHIQ_BULK_MODE_T mode)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_if.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_if.h
index 0e270852900d..e4109a83e628 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_if.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_if.h
@@ -149,16 +149,6 @@ vchiq_queue_message(VCHIQ_SERVICE_HANDLE_T handle,
 		    size_t size);
 extern void           vchiq_release_message(VCHIQ_SERVICE_HANDLE_T service,
 	VCHIQ_HEADER_T *header);
-extern VCHIQ_STATUS_T vchiq_queue_bulk_transmit(VCHIQ_SERVICE_HANDLE_T service,
-	const void *data, unsigned int size, void *userdata);
-extern VCHIQ_STATUS_T vchiq_queue_bulk_receive(VCHIQ_SERVICE_HANDLE_T service,
-	void *data, unsigned int size, void *userdata);
-extern VCHIQ_STATUS_T vchiq_queue_bulk_transmit_handle(
-	VCHIQ_SERVICE_HANDLE_T service, VCHI_MEM_HANDLE_T handle,
-	const void *offset, unsigned int size, void *userdata);
-extern VCHIQ_STATUS_T vchiq_queue_bulk_receive_handle(
-	VCHIQ_SERVICE_HANDLE_T service, VCHI_MEM_HANDLE_T handle,
-	void *offset, unsigned int size, void *userdata);
 extern VCHIQ_STATUS_T vchiq_bulk_transmit(VCHIQ_SERVICE_HANDLE_T service,
 	const void *data, unsigned int size, void *userdata,
 	VCHIQ_BULK_MODE_T mode);
-- 
2.16.2

Re: [PATCH v2 3/6] dt-bindings: soc: Add a binding for the Broadcom VCHIQ services.

[Rob Herring]

On Wed, Mar 7, 2018 at 12:57 PM, Eric Anholt <eric@anholt.net> wrote:
> The VCHIQ 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>
> ---
>
> v2: dropped firmware property, added cache-line-size.
>
>  .../bindings/soc/bcm/brcm,bcm2835-vchiq.txt        | 28 ++++++++++++++++++++++
>  1 file changed, 28 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..cdef4abc5e47
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt
> @@ -0,0 +1,28 @@
> +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
> +
> +Optional properties:
> +
> +- cache-line-size:
> +               Size of L2 cache lines.  The VPU firmware detects
> +                 this property and overrides it with the actual L2
> +                 cache line size it's using when loading the
> +                 device-tree.  Determines the required alignment of
> +                 offsets/sizes of VCHIQ pagelists.  If missing, the
> +                 firmware assumes an older kernel using 32-byte
> +                 alignment.

How is this a VCHIQ property? This is a standard property for cache
nodes, but this is not a cache node.

Is it really a problem to just use a fixed maximum alignment? That
seems to be good enough for all the rest of the kernel.

> +
> +Example:
> +
> +vchiq@7e00b840 {

mailbox@...

> +       compatible = "brcm,bcm2835-vchiq";
> +       reg = <0x7e00b840 0xf>;
> +       interrupts = <0 2>;
> +       cache-line-size: <32>;
> +};
> --
> 2.16.2
>

Re: [PATCH v2 3/6] dt-bindings: soc: Add a binding for the Broadcom VCHIQ services.

[Sudeep Holla]

On 07/03/18 18:57, Eric Anholt wrote:
> The VCHIQ 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>
> ---
> 
> v2: dropped firmware property, added cache-line-size.
> 
>  .../bindings/soc/bcm/brcm,bcm2835-vchiq.txt        | 28 ++++++++++++++++++++++
>  1 file changed, 28 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..cdef4abc5e47
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt
> @@ -0,0 +1,28 @@
> +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
> +
> +Optional properties:
> +
> +- cache-line-size:
> +		Size of L2 cache lines.  The VPU firmware detects


Which L2 cache is this ? VPU or CPUs ?
If CPUs, just drop it and get it from CPU nodes if you need it.

If VPUs, it's better add that to the name as CPUs use "cache-line-size"
At-least for me, it looked like you are specifying CPU L2 cache line
size and hence thought to comment.

-- 
Regards,
Sudeep

Re: [PATCH v2 1/6] staging: vc04_services: Replace "firmware" node with a compatible lookup.

[Stefan Wahren]

Hi Eric,

> Eric Anholt <eric@anholt.net> hat am 7. März 2018 um 19:57 geschrieben:
> 
> 
> This was requested by Rob Herring in DT bindings review.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>

Acked-by: Stefan Wahren <stefan.wahren@i2se.com>

for Patches V2: #1, #4, #5

I'm okay with #6 as long as it's unused by any new driver.

Stefan

Re: [PATCH v2 3/6] dt-bindings: soc: Add a binding for the Broadcom VCHIQ services.

[Eric Anholt]

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

Rob Herring <robh+dt@kernel.org> writes:

> On Wed, Mar 7, 2018 at 12:57 PM, Eric Anholt <eric@anholt.net> wrote:
>> The VCHIQ 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>
>> ---
>>
>> v2: dropped firmware property, added cache-line-size.
>>
>>  .../bindings/soc/bcm/brcm,bcm2835-vchiq.txt        | 28 ++++++++++++++++++++++
>>  1 file changed, 28 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..cdef4abc5e47
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt
>> @@ -0,0 +1,28 @@
>> +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
>> +
>> +Optional properties:
>> +
>> +- cache-line-size:
>> +               Size of L2 cache lines.  The VPU firmware detects
>> +                 this property and overrides it with the actual L2
>> +                 cache line size it's using when loading the
>> +                 device-tree.  Determines the required alignment of
>> +                 offsets/sizes of VCHIQ pagelists.  If missing, the
>> +                 firmware assumes an older kernel using 32-byte
>> +                 alignment.
>
> How is this a VCHIQ property? This is a standard property for cache
> nodes, but this is not a cache node.

Because the existing firmware code is choosing a value based on the
property's presence in this node.  This is the DT ABI for the firmware
that's been shipping for a long time (at least since the 4.9 era).

> Is it really a problem to just use a fixed maximum alignment? That
> seems to be good enough for all the rest of the kernel.

If we can't have this DT property, then it looks like we need the
upstream kernel to just use 32, since that's what the firmware will
assume in its absence.  Maybe the firmware maintainers can give us a new
arg to the mailbox call for setup where we could pass in the value to
use (or flag for them to pass their preferred value back to us) so we
can avoid DT.

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

Re: [PATCH v2 3/6] dt-bindings: soc: Add a binding for the Broadcom VCHIQ services.

[Rob Herring]

On Thu, Mar 8, 2018 at 2:15 PM, Eric Anholt <eric@anholt.net> wrote:
> Rob Herring <robh+dt@kernel.org> writes:
>
>> On Wed, Mar 7, 2018 at 12:57 PM, Eric Anholt <eric@anholt.net> wrote:
>>> The VCHIQ 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>
>>> ---
>>>
>>> v2: dropped firmware property, added cache-line-size.
>>>
>>>  .../bindings/soc/bcm/brcm,bcm2835-vchiq.txt        | 28 ++++++++++++++++++++++
>>>  1 file changed, 28 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..cdef4abc5e47
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-vchiq.txt
>>> @@ -0,0 +1,28 @@
>>> +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
>>> +
>>> +Optional properties:
>>> +
>>> +- cache-line-size:
>>> +               Size of L2 cache lines.  The VPU firmware detects
>>> +                 this property and overrides it with the actual L2
>>> +                 cache line size it's using when loading the
>>> +                 device-tree.  Determines the required alignment of
>>> +                 offsets/sizes of VCHIQ pagelists.  If missing, the
>>> +                 firmware assumes an older kernel using 32-byte
>>> +                 alignment.
>>
>> How is this a VCHIQ property? This is a standard property for cache
>> nodes, but this is not a cache node.
>
> Because the existing firmware code is choosing a value based on the
> property's presence in this node.  This is the DT ABI for the firmware
> that's been shipping for a long time (at least since the 4.9 era).

It's not an ABI if it is not upstream and documented.

>> Is it really a problem to just use a fixed maximum alignment? That
>> seems to be good enough for all the rest of the kernel.
>
> If we can't have this DT property, then it looks like we need the
> upstream kernel to just use 32, since that's what the firmware will
> assume in its absence.  Maybe the firmware maintainers can give us a new
> arg to the mailbox call for setup where we could pass in the value to
> use (or flag for them to pass their preferred value back to us) so we
> can avoid DT.

I read thru the discussion on the previous version and skimmed thru
the code, but still don't really have a grasp on things. If the actual
cache line size is 64 bytes, but you use 32 things would be pretty
broken I'd think.

BTW, I'm pretty sure this line is not doing what you want:

static unsigned int g_cache_line_size = sizeof(CACHE_LINE_SIZE);

Rob