Re: [PATCH v1 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation

[Tomer Maimon <tmaimon77@gmail.com>]

Hi Krzysztof,

Thanks for your clarifications.

On Tue, 12 Jul 2022 at 23:44, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 12/07/2022 20:44, Tomer Maimon wrote:
> > Hi Krzysztof,
> >
> > Thanks for your clarifications.
> >
> > On Tue, 12 Jul 2022 at 16:45, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 12/07/2022 15:29, Tomer Maimon wrote:
> >>> Hi Krzysztof,
> >>>
> >>> Thanks for your comments.
> >>>
> >>> On Tue, 12 Jul 2022 at 12:48, Krzysztof Kozlowski
> >>> <krzysztof.kozlowski@linaro.org> wrote:
> >>>>
> >>>> On 10/07/2022 12:21, Tomer Maimon wrote:
> >>>>> Added device tree binding documentation for Nuvoton Arbel BMC NPCM8XX
> >>>>> pinmux and GPIO controller.
> >>>>>
> >>>>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> >>>>> ---
> >>>>>  .../pinctrl/nuvoton,npcm845-pinctrl.yaml      | 205 ++++++++++++++++++
> >>>>>  1 file changed, 205 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
> >>>>> new file mode 100644
> >>>>> index 000000000000..6395ef2bf5b3
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
> >>>>> @@ -0,0 +1,205 @@
> >>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> >>>>> +%YAML 1.2
> >>>>> +---
> >>>>> +$id: http://devicetree.org/schemas/pinctrl/nuvoton,npcm845-pinctrl.yaml#
> >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>> +
> >>>>> +title: Nuvoton NPCM845 Pin Controller and GPIO
> >>>>> +
> >>>>> +maintainers:
> >>>>> +  - Tomer Maimon <tmaimon77@gmail.com>
> >>>>> +
> >>>>> +description:
> >>>>> +  The Nuvoton BMC NPCM8XX Pin Controller multi-function routed through
> >>>>> +  the multiplexing block, Each pin supports GPIO functionality (GPIOx)
> >>>>> +  and multiple functions that directly connect the pin to different
> >>>>> +  hardware blocks.
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    const: nuvoton,npcm845-pinctrl
> >>>>> +
> >>>>> +  ranges:
> >>>>> +    maxItems: 1
> >>>>
> >>>> ranges without reg? Does it even work? Did you test the bindings?
> >>> The ranges related to GPIO node reg
> >>
> >> But you do not allow here a 'reg', do you? So how can you have an unit
> >> address in pinctrl node?
> > I allow the reg unit address in the GPIO node.
> > This is why reg is in the GPIO node as follow:
> >
> >                 compatible = "nuvoton,npcm845-pinctrl";
> >                 ranges = <0x0 0x0 0xf0010000 0x8000>;
> >                 #address-cells = <1>;
> >                 #size-cells = <1>;
> >                 status = "okay";
> >                 gpio0: gpio@f0010000 {
> >                         gpio-controller;
> >                         #gpio-cells = <2>;
> >                         reg = <0x0 0xB0>;
> >                         interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>;
> >                         gpio-ranges = <&pinctrl 0 0 32>;
> >                 };
> >                 gpio1: gpio@f0011000 {
> >                         gpio-controller;
> >                         #gpio-cells = <2>;
> >                         reg = <0x1000 0xB0>;
> >                         interrupts = <GIC_SPI 117 IRQ_TYPE_LEVEL_HIGH>;
> >                         gpio-ranges = <&pinctrl 0 32 32>;
> >                 };
> >                 gpio2: gpio@f0012000 {
> >                         gpio-controller;
> >                         #gpio-cells = <2>;
> >                         reg = <0x2000 0xB0>;
> >                         interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>;
> >                         gpio-ranges = <&pinctrl 0 64 32>;
> >                 };
> > ...
> > Is it problematic?
>
>
> It seems not, looks ok because of ranges, although it is a bit confusing
> that your pinctrl unit address is 0xf0800000 but ranges is 0xf0010000.
The reason the pinctrl address 0xf0800000 because the pin mux is
handled by the GCR registers and the ranges related to the GPIO.
>
> >>
> >>>
> >>> I did test the pin controller document and it passed.
> >>> bash-4.2$ make ARCH=arm64 dt_binding_check
> >>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
> >>>   LINT    Documentation/devicetree/bindings
> >>>   CHKDT   Documentation/devicetree/bindings/processed-schema.json
> >>>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
> >>>   DTEX    Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dts
> >>>   DTC     Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dtb
> >>>   CHECK   Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dtb
> >>> Did I need to run anything else than dt_binding_check for testing the document?
> >>
> >> Indeed it will pass, because you do not have reg in pinctrl node. But
> >> your dts won't pass make dtbs W=1
> > After running make ARCH=arm64 dtbs W=1 I don't see warning related to pinctrl
> > bash-4.2$ make ARCH=arm64 dtbs W=1
> >   DTC     arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dtb
> > arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi:69.7-183.5:
> > Warning (unit_address_vs_reg): /ahb/apb: node has a reg or ranges
> > property, but no unit name
> > arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts:20.9-22.4: Warning
> > (unit_address_vs_reg): /memory: node has a reg or ranges property, but
> > no unit name
> > arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi:69.7-183.5:
> > Warning (simple_bus_reg): /ahb/apb: simple-bus unit address format
> > error, expected "f0000000"
> > arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi:56.35-61.5:
> > Warning (unique_unit_address): /ahb/reset-controller@f0801000:
> > duplicate unit-address (also used in node
> > /ahb/clock-controller@f0801000)
> > I did got warning but it dont related to the pinctrl, Maybe I didn't
> > run the test correct?
>
> Looks correct, indeed.
>
> Best regards,
> Krzysztof

Best regards,

Tomer