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

[Tomer Maimon]

On Tue, 20 Sept 2022 at 11:47, Krzysztof Kozlowski
<krzysztof.kozlowski at linaro.org> wrote:
>
> On 20/09/2022 10:32, Tomer Maimon wrote:
> > On Tue, 20 Sept 2022 at 11:21, Krzysztof Kozlowski
> > <krzysztof.kozlowski at linaro.org> wrote:
> >>
> >> On 20/09/2022 09:59, Tomer Maimon wrote:
> >>>>>>>>> +      pinctrl: pinctrl at f0800000 {
> >>>>>>>>> +        compatible = "nuvoton,npcm845-pinctrl";
> >>>>>>>>> +        ranges = <0x0 0x0 0xf0010000 0x8000>;
> >>>>>>>>> +        #address-cells = <1>;
> >>>>>>>>> +        #size-cells = <1>;
> >>>>>>>>> +        nuvoton,sysgcr = <&gcr>;
> >>>>>>>>> +
> >>>>>>>>> +        gpio0: gpio at f0010000 {
> >>>>>>>>
> >>>>>>>> gpio at 0
> >>>>>>>>
> >>>>>>>> Is this really a child block of the pinctrl? Doesn't really look like it
> >>>>>>>> based on addressess. Where are the pinctrl registers? In the sysgcr? If
> >>>>>>>> so, then pinctrl should be a child of it. But that doesn't really work
> >>>>>>>> too well with gpio child nodes...
> >>>>>>> the pin controller mux is handled by sysgcr this is why the sysgcr in
> >>>>>>> the mother node,
> >>>>>>> and the pin configuration are handled by the GPIO registers.  each
> >>>>>>> GPIO bank (child) contains 32 GPIO.
> >>>>>>> this is why the GPIO is the child node.
> >>>>>>
> >>>>>> Then maybe pinctrl should be the sysgcr and expose regmap for other devices?
> >>>>> The pin controller using the sysgcr to handle the pinmux, this is why
> >>>>> the sysgcr is in the mother node, is it problematic?
> >>>>
> >>>> You said pin-controller mux registers are in sysgcr, so it should not be
> >>>> used via syscon.
> >>> Sorry but maybe I missed something.
> >>> the sysgcr is used for miscellaneous features and not only for the pin
> >>> controller mux, this is why it used syscon and defined in the dtsi:
> >>>                 gcr: system-controller at f0800000 {
> >>>                         compatible = "nuvoton,npcm845-gcr", "syscon";
> >>>                         reg = <0x0 0xf0800000 0x0 0x1000>;
> >>>                 };
> >>>>
> >>>> Please provide address map description to convince us that this is
> >>>> correct HW representation.
> >>> GCR (sysgcr) registers 0xf0800000-0xf0801000 - used for miscellaneous
> >>> features, not only pin mux.
> >>> GPIO0 0xf0010000-0xf0011000
> >>> GPIO1 0xf0011000-0xf0012000
> >>> ...
> >>> GPIO7 0xf0017000-0xf0018000
> >>>>
> >>
> >> Then why your pinctrl is in sysgcr IO range? (pinctrl at f0800000)
> > you suggest using pinctrl at 0 or pinctrl at f0010000 and not
> > pinctrl at f0800000 because 0xf0800000 is the GCR address that serve
> > miscellaneous features and not only pinmux controller ?
>
> If you have a map like you pasted, then DTS like this:
>
> syscon at f0800000 {}
> pinctrl at f0800000 {
>   gpio at f0010000 {}
> }
>
> Is quite weird, don't you think? You have two devices on the same unit
> address which is not allowed. You have child of pinctrl with entirely
O.K.
> different unit address, so how is it its child?
The pinctrl node name will modify the pinctrl at f0010000 the same as the
range property and the start of the child registers,is it fine?
> Best regards,
> Krzysztof

Best regards,

Tomer