| Message ID | 20240326004902.17054-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dt-bindings: arm: bcm: raspberrypi,bcm2835-firmware: Drive-by fixes | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 34 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 26/03/2024 01:49, Laurent Pinchart wrote: > The raspberrypi,bcm2835-firmware devices requires a dma-ranges property, > and, as a result, also needs to specify #address-cells and #size-cells. > Those properties have been added to thebcm2835-rpi.dtsi in commits > be08d278eb09 ("ARM: dts: bcm283x: Add cells encoding format to firmware > bus") and 55c7c0621078 ("ARM: dts: bcm283x: Fix vc4's firmware bus DMA > limitations"), but the DT bindings haven't been updated, resulting in > validation errors: > > arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b.dtb: firmware: '#address-cells', '#size-cells', 'dma-ranges', 'gpio' do not match any of the regexes: 'pinctrl-[0-9]+' > from schema $id: http://devicetree.org/schemas/arm/bcm/raspberrypi,bcm2835-firmware.yaml# > > Fix this by adding the properties to the bindings. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Children do not perform any IO on their own, because everything is handled by parent. It is really odd to see dma-ranges without ranges. Referenced commits might be also wrong. Best regards, Krzysztof
[add Dave since he's working on DMA for Raspberry Pi 4 and maybe have a opinion about this] [drop Emma Anholt old address since she is not involved anymore] Am 26.03.24 um 08:06 schrieb Krzysztof Kozlowski: > On 26/03/2024 01:49, Laurent Pinchart wrote: >> The raspberrypi,bcm2835-firmware devices requires a dma-ranges property, >> and, as a result, also needs to specify #address-cells and #size-cells. >> Those properties have been added to thebcm2835-rpi.dtsi in commits >> be08d278eb09 ("ARM: dts: bcm283x: Add cells encoding format to firmware >> bus") and 55c7c0621078 ("ARM: dts: bcm283x: Fix vc4's firmware bus DMA >> limitations"), but the DT bindings haven't been updated, resulting in >> validation errors: >> >> arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b.dtb: firmware: '#address-cells', '#size-cells', 'dma-ranges', 'gpio' do not match any of the regexes: 'pinctrl-[0-9]+' >> from schema $id: http://devicetree.org/schemas/arm/bcm/raspberrypi,bcm2835-firmware.yaml# >> >> Fix this by adding the properties to the bindings. >> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Children do not perform any IO on their own, because everything is > handled by parent. It is really odd to see dma-ranges without ranges. > Referenced commits might be also wrong. > > Best regards, > Krzysztof > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Mar 26, 2024 at 12:47:34PM +0100, Stefan Wahren wrote: > [add Dave since he's working on DMA for Raspberry Pi 4 and maybe have a > opinion about this] > > [drop Emma Anholt old address since she is not involved anymore] > > Am 26.03.24 um 08:06 schrieb Krzysztof Kozlowski: > > On 26/03/2024 01:49, Laurent Pinchart wrote: > >> The raspberrypi,bcm2835-firmware devices requires a dma-ranges property, > >> and, as a result, also needs to specify #address-cells and #size-cells. > >> Those properties have been added to thebcm2835-rpi.dtsi in commits > >> be08d278eb09 ("ARM: dts: bcm283x: Add cells encoding format to firmware > >> bus") and 55c7c0621078 ("ARM: dts: bcm283x: Fix vc4's firmware bus DMA > >> limitations"), but the DT bindings haven't been updated, resulting in > >> validation errors: > >> > >> arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b.dtb: firmware: '#address-cells', '#size-cells', 'dma-ranges', 'gpio' do not match any of the regexes: 'pinctrl-[0-9]+' > >> from schema $id: http://devicetree.org/schemas/arm/bcm/raspberrypi,bcm2835-firmware.yaml# > >> > >> Fix this by adding the properties to the bindings. > >> > >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Children do not perform any IO on their own, because everything is > > handled by parent. It is really odd to see dma-ranges without ranges. > > Referenced commits might be also wrong. Comunication with the firmware goes through a mailbox interface, which uses DMA transfers. See for instance rpi_firmware_transaction(struct rpi_firmware *fw, u32 chan, u32 data) { u32 message = MBOX_MSG(chan, data); int ret; WARN_ON(data & 0xf); mutex_lock(&transaction_lock); reinit_completion(&fw->c); ret = mbox_send_message(fw->chan, &message); if (ret >= 0) { if (wait_for_completion_timeout(&fw->c, HZ)) { ret = 0; } else { ret = -ETIMEDOUT; WARN_ONCE(1, "Firmware transaction timeout"); } } else { dev_err(fw->cl.dev, "mbox_send_message returned %d\n", ret); } mutex_unlock(&transaction_lock); return ret; } int rpi_firmware_property_list(struct rpi_firmware *fw, void *data, size_t tag_size) { size_t size = tag_size + 12; u32 *buf; dma_addr_t bus_addr; int ret; /* Packets are processed a dword at a time. */ if (size & 3) return -EINVAL; buf = dma_alloc_coherent(fw->cl.dev, PAGE_ALIGN(size), &bus_addr, GFP_ATOMIC); if (!buf) return -ENOMEM; /* The firmware will error out without parsing in this case. */ WARN_ON(size >= 1024 * 1024); buf[0] = size; buf[1] = RPI_FIRMWARE_STATUS_REQUEST; memcpy(&buf[2], data, tag_size); buf[size / 4 - 1] = RPI_FIRMWARE_PROPERTY_END; wmb(); ret = rpi_firmware_transaction(fw, MBOX_CHAN_PROPERTY, bus_addr); rmb(); memcpy(data, &buf[2], tag_size); if (ret == 0 && buf[1] != RPI_FIRMWARE_STATUS_SUCCESS) { /* * The tag name here might not be the one causing the * error, if there were multiple tags in the request. * But single-tag is the most common, so go with it. */ dev_err(fw->cl.dev, "Request 0x%08x returned status 0x%08x\n", buf[2], buf[1]); ret = -EINVAL; } dma_free_coherent(fw->cl.dev, PAGE_ALIGN(size), buf, bus_addr); return ret; } fw->cl.dev is the device for the firmware child node. That may be where the problem comes from, shouldn't we use the mailbox device for DMA mapping ?
Am 26.03.24 um 18:18 schrieb Laurent Pinchart: > On Tue, Mar 26, 2024 at 12:47:34PM +0100, Stefan Wahren wrote: >> [add Dave since he's working on DMA for Raspberry Pi 4 and maybe have a >> opinion about this] >> >> [drop Emma Anholt old address since she is not involved anymore] >> >> Am 26.03.24 um 08:06 schrieb Krzysztof Kozlowski: >>> On 26/03/2024 01:49, Laurent Pinchart wrote: >>>> The raspberrypi,bcm2835-firmware devices requires a dma-ranges property, >>>> and, as a result, also needs to specify #address-cells and #size-cells. >>>> Those properties have been added to thebcm2835-rpi.dtsi in commits >>>> be08d278eb09 ("ARM: dts: bcm283x: Add cells encoding format to firmware >>>> bus") and 55c7c0621078 ("ARM: dts: bcm283x: Fix vc4's firmware bus DMA >>>> limitations"), but the DT bindings haven't been updated, resulting in >>>> validation errors: >>>> >>>> arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b.dtb: firmware: '#address-cells', '#size-cells', 'dma-ranges', 'gpio' do not match any of the regexes: 'pinctrl-[0-9]+' >>>> from schema $id: http://devicetree.org/schemas/arm/bcm/raspberrypi,bcm2835-firmware.yaml# >>>> >>>> Fix this by adding the properties to the bindings. >>>> >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> Children do not perform any IO on their own, because everything is >>> handled by parent. It is really odd to see dma-ranges without ranges. >>> Referenced commits might be also wrong. > Comunication with the firmware goes through a mailbox interface, which > uses DMA transfers. See for instance > > rpi_firmware_transaction(struct rpi_firmware *fw, u32 chan, u32 data) > { > u32 message = MBOX_MSG(chan, data); > int ret; > > WARN_ON(data & 0xf); > > mutex_lock(&transaction_lock); > reinit_completion(&fw->c); > ret = mbox_send_message(fw->chan, &message); > if (ret >= 0) { > if (wait_for_completion_timeout(&fw->c, HZ)) { > ret = 0; > } else { > ret = -ETIMEDOUT; > WARN_ONCE(1, "Firmware transaction timeout"); > } > } else { > dev_err(fw->cl.dev, "mbox_send_message returned %d\n", ret); > } > mutex_unlock(&transaction_lock); > > return ret; > } > > int rpi_firmware_property_list(struct rpi_firmware *fw, > void *data, size_t tag_size) > { > size_t size = tag_size + 12; > u32 *buf; > dma_addr_t bus_addr; > int ret; > > /* Packets are processed a dword at a time. */ > if (size & 3) > return -EINVAL; > > buf = dma_alloc_coherent(fw->cl.dev, PAGE_ALIGN(size), &bus_addr, > GFP_ATOMIC); > if (!buf) > return -ENOMEM; > > /* The firmware will error out without parsing in this case. */ > WARN_ON(size >= 1024 * 1024); > > buf[0] = size; > buf[1] = RPI_FIRMWARE_STATUS_REQUEST; > memcpy(&buf[2], data, tag_size); > buf[size / 4 - 1] = RPI_FIRMWARE_PROPERTY_END; > wmb(); > > ret = rpi_firmware_transaction(fw, MBOX_CHAN_PROPERTY, bus_addr); > > rmb(); > memcpy(data, &buf[2], tag_size); > if (ret == 0 && buf[1] != RPI_FIRMWARE_STATUS_SUCCESS) { > /* > * The tag name here might not be the one causing the > * error, if there were multiple tags in the request. > * But single-tag is the most common, so go with it. > */ > dev_err(fw->cl.dev, "Request 0x%08x returned status 0x%08x\n", > buf[2], buf[1]); > ret = -EINVAL; > } > > dma_free_coherent(fw->cl.dev, PAGE_ALIGN(size), buf, bus_addr); > > return ret; > } > > fw->cl.dev is the device for the firmware child node. That may be where > the problem comes from, shouldn't we use the mailbox device for DMA > mapping ? > From devicetree perspective this is the mailbox DT part [1] and this the matching dt-binding [2]. [1] - https://elixir.bootlin.com/linux/v6.9-rc1/source/arch/arm/boot/dts/broadcom/bcm283x.dtsi#L100 [2] - https://elixir.bootlin.com/linux/v6.9-rc1/source/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.yaml
On Tue, Mar 26, 2024 at 06:40:52PM +0100, Stefan Wahren wrote: > Am 26.03.24 um 18:18 schrieb Laurent Pinchart: > > On Tue, Mar 26, 2024 at 12:47:34PM +0100, Stefan Wahren wrote: > >> [add Dave since he's working on DMA for Raspberry Pi 4 and maybe have a > >> opinion about this] > >> > >> [drop Emma Anholt old address since she is not involved anymore] > >> > >> Am 26.03.24 um 08:06 schrieb Krzysztof Kozlowski: > >>> On 26/03/2024 01:49, Laurent Pinchart wrote: > >>>> The raspberrypi,bcm2835-firmware devices requires a dma-ranges property, > >>>> and, as a result, also needs to specify #address-cells and #size-cells. > >>>> Those properties have been added to thebcm2835-rpi.dtsi in commits > >>>> be08d278eb09 ("ARM: dts: bcm283x: Add cells encoding format to firmware > >>>> bus") and 55c7c0621078 ("ARM: dts: bcm283x: Fix vc4's firmware bus DMA > >>>> limitations"), but the DT bindings haven't been updated, resulting in > >>>> validation errors: > >>>> > >>>> arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b.dtb: firmware: '#address-cells', '#size-cells', 'dma-ranges', 'gpio' do not match any of the regexes: 'pinctrl-[0-9]+' > >>>> from schema $id: http://devicetree.org/schemas/arm/bcm/raspberrypi,bcm2835-firmware.yaml# > >>>> > >>>> Fix this by adding the properties to the bindings. > >>>> > >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> > >>> Children do not perform any IO on their own, because everything is > >>> handled by parent. It is really odd to see dma-ranges without ranges. > >>> Referenced commits might be also wrong. > > > > Comunication with the firmware goes through a mailbox interface, which > > uses DMA transfers. See for instance > > > > rpi_firmware_transaction(struct rpi_firmware *fw, u32 chan, u32 data) > > { > > u32 message = MBOX_MSG(chan, data); > > int ret; > > > > WARN_ON(data & 0xf); > > > > mutex_lock(&transaction_lock); > > reinit_completion(&fw->c); > > ret = mbox_send_message(fw->chan, &message); > > if (ret >= 0) { > > if (wait_for_completion_timeout(&fw->c, HZ)) { > > ret = 0; > > } else { > > ret = -ETIMEDOUT; > > WARN_ONCE(1, "Firmware transaction timeout"); > > } > > } else { > > dev_err(fw->cl.dev, "mbox_send_message returned %d\n", ret); > > } > > mutex_unlock(&transaction_lock); > > > > return ret; > > } > > > > int rpi_firmware_property_list(struct rpi_firmware *fw, > > void *data, size_t tag_size) > > { > > size_t size = tag_size + 12; > > u32 *buf; > > dma_addr_t bus_addr; > > int ret; > > > > /* Packets are processed a dword at a time. */ > > if (size & 3) > > return -EINVAL; > > > > buf = dma_alloc_coherent(fw->cl.dev, PAGE_ALIGN(size), &bus_addr, > > GFP_ATOMIC); > > if (!buf) > > return -ENOMEM; > > > > /* The firmware will error out without parsing in this case. */ > > WARN_ON(size >= 1024 * 1024); > > > > buf[0] = size; > > buf[1] = RPI_FIRMWARE_STATUS_REQUEST; > > memcpy(&buf[2], data, tag_size); > > buf[size / 4 - 1] = RPI_FIRMWARE_PROPERTY_END; > > wmb(); > > > > ret = rpi_firmware_transaction(fw, MBOX_CHAN_PROPERTY, bus_addr); > > > > rmb(); > > memcpy(data, &buf[2], tag_size); > > if (ret == 0 && buf[1] != RPI_FIRMWARE_STATUS_SUCCESS) { > > /* > > * The tag name here might not be the one causing the > > * error, if there were multiple tags in the request. > > * But single-tag is the most common, so go with it. > > */ > > dev_err(fw->cl.dev, "Request 0x%08x returned status 0x%08x\n", > > buf[2], buf[1]); > > ret = -EINVAL; > > } > > > > dma_free_coherent(fw->cl.dev, PAGE_ALIGN(size), buf, bus_addr); > > > > return ret; > > } > > > > fw->cl.dev is the device for the firmware child node. That may be where > > the problem comes from, shouldn't we use the mailbox device for DMA > > mapping ? > > From devicetree perspective this is the mailbox DT part [1] and this > the matching dt-binding [2]. > > [1] - https://elixir.bootlin.com/linux/v6.9-rc1/source/arch/arm/boot/dts/broadcom/bcm283x.dtsi#L100 > [2] - https://elixir.bootlin.com/linux/v6.9-rc1/source/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.yaml That's the device performing DMA, so I think it should be used for DMA mapping.
On Tue, 26 Mar 2024 at 11:47, Stefan Wahren <wahrenst@gmx.net> wrote: > > [add Dave since he's working on DMA for Raspberry Pi 4 and maybe have a > opinion about this] No real opinion from me, but I am far from a DT expert, and AFAIK this bit isn't impacted by the stuff I'm looking at. Laurent is correct that it's missing from the binding doc when it looks to be needed. Adding it would therefore be the correct thing to do. Dave > [drop Emma Anholt old address since she is not involved anymore] > > Am 26.03.24 um 08:06 schrieb Krzysztof Kozlowski: > > On 26/03/2024 01:49, Laurent Pinchart wrote: > >> The raspberrypi,bcm2835-firmware devices requires a dma-ranges property, > >> and, as a result, also needs to specify #address-cells and #size-cells. > >> Those properties have been added to thebcm2835-rpi.dtsi in commits > >> be08d278eb09 ("ARM: dts: bcm283x: Add cells encoding format to firmware > >> bus") and 55c7c0621078 ("ARM: dts: bcm283x: Fix vc4's firmware bus DMA > >> limitations"), but the DT bindings haven't been updated, resulting in > >> validation errors: > >> > >> arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b.dtb: firmware: '#address-cells', '#size-cells', 'dma-ranges', 'gpio' do not match any of the regexes: 'pinctrl-[0-9]+' > >> from schema $id: http://devicetree.org/schemas/arm/bcm/raspberrypi,bcm2835-firmware.yaml# > >> > >> Fix this by adding the properties to the bindings. > >> > >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Children do not perform any IO on their own, because everything is > > handled by parent. It is really odd to see dma-ranges without ranges. > > Referenced commits might be also wrong. > > > > Best regards, > > Krzysztof > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Tue, Mar 26, 2024 at 02:49:01AM +0200, Laurent Pinchart wrote: > The raspberrypi,bcm2835-firmware devices requires a dma-ranges property, > and, as a result, also needs to specify #address-cells and #size-cells. > Those properties have been added to thebcm2835-rpi.dtsi in commits > be08d278eb09 ("ARM: dts: bcm283x: Add cells encoding format to firmware > bus") and 55c7c0621078 ("ARM: dts: bcm283x: Fix vc4's firmware bus DMA > limitations"), but the DT bindings haven't been updated, resulting in > validation errors: I don't understand. We treat no dma-ranges the same as empty dma-ranges (dma-ranges;). If we didn't, *every* DT would be broken. We should never have dma-ranges without ranges either. Rob
Hi Rob, On Tue, Mar 26, 2024 at 04:30:53PM -0500, Rob Herring wrote: > On Tue, Mar 26, 2024 at 02:49:01AM +0200, Laurent Pinchart wrote: > > The raspberrypi,bcm2835-firmware devices requires a dma-ranges property, > > and, as a result, also needs to specify #address-cells and #size-cells. > > Those properties have been added to thebcm2835-rpi.dtsi in commits > > be08d278eb09 ("ARM: dts: bcm283x: Add cells encoding format to firmware > > bus") and 55c7c0621078 ("ARM: dts: bcm283x: Fix vc4's firmware bus DMA > > limitations"), but the DT bindings haven't been updated, resulting in > > validation errors: > > I don't understand. We treat no dma-ranges the same as empty dma-ranges > (dma-ranges;). If we didn't, *every* DT would be broken. > > We should never have dma-ranges without ranges either. Please see v2 :-) https://lore.kernel.org/linux-arm-kernel/20240326195807.15163-3-laurent.pinchart@ideasonboard.com/ https://lore.kernel.org/linux-arm-kernel/20240326195807.15163-4-laurent.pinchart@ideasonboard.com/
diff --git a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml index 39e3c248f5b7..dc38f2be7ad6 100644 --- a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml +++ b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml @@ -25,6 +25,14 @@ properties: - const: raspberrypi,bcm2835-firmware - const: simple-mfd + "#address-cells": + const: 1 + + "#size-cells": + const: 1 + + dma-ranges: true + mboxes: maxItems: 1 @@ -81,6 +89,9 @@ properties: required: - compatible + - "#address-cells" + - "#size-cells" + - dma-ranges - mboxes additionalProperties: false @@ -89,6 +100,11 @@ examples: - | firmware { compatible = "raspberrypi,bcm2835-firmware", "simple-mfd"; + + #address-cells = <1>; + #size-cells = <1>; + dma-ranges; + mboxes = <&mailbox>; firmware_clocks: clocks {
The raspberrypi,bcm2835-firmware devices requires a dma-ranges property, and, as a result, also needs to specify #address-cells and #size-cells. Those properties have been added to thebcm2835-rpi.dtsi in commits be08d278eb09 ("ARM: dts: bcm283x: Add cells encoding format to firmware bus") and 55c7c0621078 ("ARM: dts: bcm283x: Fix vc4's firmware bus DMA limitations"), but the DT bindings haven't been updated, resulting in validation errors: arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b.dtb: firmware: '#address-cells', '#size-cells', 'dma-ranges', 'gpio' do not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/arm/bcm/raspberrypi,bcm2835-firmware.yaml# Fix this by adding the properties to the bindings. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- .../arm/bcm/raspberrypi,bcm2835-firmware.yaml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)