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@xxxxxxxxxxxxxxxx>
> >>>
> >>> 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.
--
Regards,
Laurent Pinchart
