Re: [PATCH 08/11] misc: rp1: RaspberryPi RP1 misc driver

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On 22/08/2024 16:33, Andrea della Porta wrote:
> Hi Krzysztof,
> 
> On 16:20 Wed 21 Aug     , Krzysztof Kozlowski wrote:
>> On 21/08/2024 10:38, Krzysztof Kozlowski wrote:
>>> On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote:
>>
>> ...
>>
>>>>  drivers/misc/Kconfig                  |   1 +
>>>>  drivers/misc/Makefile                 |   1 +
>>>>  drivers/misc/rp1/Kconfig              |  20 ++
>>>>  drivers/misc/rp1/Makefile             |   3 +
>>>>  drivers/misc/rp1/rp1-pci.c            | 333 ++++++++++++++++++++++++++
>>>>  drivers/misc/rp1/rp1-pci.dtso         |   8 +
>>>>  drivers/pci/quirks.c                  |   1 +
>>>>  include/linux/pci_ids.h               |   3 +
>>>>  10 files changed, 524 insertions(+)
>>>>  create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso
>>>>  create mode 100644 drivers/misc/rp1/Kconfig
>>>>  create mode 100644 drivers/misc/rp1/Makefile
>>>>  create mode 100644 drivers/misc/rp1/rp1-pci.c
>>>>  create mode 100644 drivers/misc/rp1/rp1-pci.dtso
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 67f460c36ea1..1359538b76e8 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -19119,9 +19119,11 @@ F:	include/uapi/linux/media/raspberrypi/
>>>>  RASPBERRY PI RP1 PCI DRIVER
>>>>  M:	Andrea della Porta <andrea.porta@xxxxxxxx>
>>>>  S:	Maintained
>>>> +F:	arch/arm64/boot/dts/broadcom/rp1.dtso
>>>>  F:	Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
>>>>  F:	Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml
>>>>  F:	drivers/clk/clk-rp1.c
>>>> +F:	drivers/misc/rp1/
>>>>  F:	drivers/pinctrl/pinctrl-rp1.c
>>>>  F:	include/dt-bindings/clock/rp1.h
>>>>  F:	include/dt-bindings/misc/rp1.h
>>>> diff --git a/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso
>>>> new file mode 100644
>>>> index 000000000000..d80178a278ee
>>>> --- /dev/null
>>>> +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso
>>>> @@ -0,0 +1,152 @@
>>>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>>>> +
>>>> +#include <dt-bindings/gpio/gpio.h>
>>>> +#include <dt-bindings/interrupt-controller/irq.h>
>>>> +#include <dt-bindings/clock/rp1.h>
>>>> +#include <dt-bindings/misc/rp1.h>
>>>> +
>>>> +/dts-v1/;
>>>> +/plugin/;
>>>> +
>>>> +/ {
>>>> +	fragment@0 {
>>>> +		target-path="";
>>>> +		__overlay__ {
>>>> +			#address-cells = <3>;
>>>> +			#size-cells = <2>;
>>>> +
>>>> +			rp1: rp1@0 {
>>>> +				compatible = "simple-bus";
>>>> +				#address-cells = <2>;
>>>> +				#size-cells = <2>;
>>>> +				interrupt-controller;
>>>> +				interrupt-parent = <&rp1>;
>>>> +				#interrupt-cells = <2>;
>>>> +
>>>> +				// ranges and dma-ranges must be provided by the includer
>>>> +				ranges = <0xc0 0x40000000
>>>> +					  0x01/*0x02000000*/ 0x00 0x00000000
>>>> +					  0x00 0x00400000>;
>>>
>>> Are you 100% sure you do not have here dtc W=1 warnings?
>>
>> One more thing, I do not see this overlay applied to any target, which
>> means it cannot be tested. You miss entry in Makefile.
>>
> 
> The dtso is intended to be built from driver/misc/rp1/Makefile as it will
> be included in the driver obj:
> 
> --- /dev/null
> +++ b/drivers/misc/rp1/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +rp1-pci-objs                   := rp1-pci.o rp1-pci.dtbo.o
> +obj-$(CONFIG_MISC_RP1)         += rp1-pci.o
> 
> and not as part of the dtb system, hence it's m issing in
> arch/arm64/boot/dts/broadcom/Makefile.
> 
> On the other hand:
> 
> #> make W=1 CHECK_DTBS=y broadcom/rp1.dtbo
>   DTC     arch/arm64/boot/dts/broadcom/rp1.dtbo
> arch/arm64/boot/dts/broadcom/rp1.dtso:37.24-42.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/clk_xosc: missing or empty reg/ranges property
> arch/arm64/boot/dts/broadcom/rp1.dtso:44.26-49.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_pclk: missing or empty reg/ranges property
> arch/arm64/boot/dts/broadcom/rp1.dtso:51.26-56.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_hclk: missing or empty reg/ranges property
> arch/arm64/boot/dts/broadcom/rp1.dtso:14.15-173.5: Warning (avoid_unnecessary_addr_size): /fragment@0/__overlay__: unnecessary #address-cells/#size-cells without "ranges", "dma-ranges" or child "reg" property
> 
> seems to do the checks, unless I'm missing something.

Yeah, there is still no target which applies the overlay, so no one can
tell whether it applies cleanly or not. You can only test single
overlay, but it is expected to test each overlay being applied to chosen
DTB.

Best regards,
Krzysztof