Re: [PATCH 1/2] dt-bindings: arm: bcm: raspberrypi,bcm2835-firmware: Add missing properties

From: Stefan Wahren <wahrenst@gmx.net>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: devicetree@vger.kernel.org, linux-rpi-kernel@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
	Dave Stevenson <dave.stevenson@raspberrypi.com>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Florian Fainelli <florian.fainelli@broadcom.com>,
	Broadcom internal kernel review list
	<bcm-kernel-feedback-list@broadcom.com>,
	Ray Jui <rjui@broadcom.com>,
	Scott Branden <sbranden@broadcom.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <brgl@bgdev.pl>
Subject: Re: [PATCH 1/2] dt-bindings: arm: bcm: raspberrypi,bcm2835-firmware: Add missing properties
Date: Tue, 26 Mar 2024 18:40:52 +0100	[thread overview]
Message-ID: <e6334490-501c-4967-a48b-168868acfd39@gmx.net> (raw)
In-Reply-To: <20240326171837.GB17067@pendragon.ideasonboard.com>

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