| Message ID | 20240326004902.17054-1-laurent.pinchart@ideasonboard.com |
|---|---|
| Headers | show |
| Series | dt-bindings: arm: bcm: raspberrypi,bcm2835-firmware: Drive-by fixes | expand |
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
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/
Hello, This small series includes two drive-by fixes to the raspberrypi,bcm2835-firmware DT bindings that fix validation errors with the Raspberry Pi 4 device tree sources. I noticed those issues when working on the Raspberry Pi Unicam driver, but two patches are independent of that work, they can thus be applied separately. Laurent Pinchart (2): dt-bindings: arm: bcm: raspberrypi,bcm2835-firmware: Add missing properties dt-bindings: arm: bcm: raspberrypi,bcm2835-firmware: Add gpio child node .../arm/bcm/raspberrypi,bcm2835-firmware.yaml | 45 +++++++++++++++++++ .../gpio/raspberrypi,firmware-gpio.txt | 30 ------------- 2 files changed, 45 insertions(+), 30 deletions(-) delete mode 100644 Documentation/devicetree/bindings/gpio/raspberrypi,firmware-gpio.txt