Re: [PATCH 1/3] dt-bindings: pinctrl: Add bindings for mscc,ocelot-sgpio

[Lars Povlsen]

Linus Walleij writes:

> On Mon, May 18, 2020 at 10:50 PM Lars Povlsen
> <lars.povlsen@xxxxxxxxxxxxx> wrote:
>> Linus Walleij writes:
>>
>> > On Wed, May 13, 2020 at 4:11 PM Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx> wrote:
>> >
>> >> This adds DT bindings for the Microsemi SGPIO controller, bindings
>> >> mscc,ocelot-sgpio and mscc,luton-sgpio.
>> >>
>> >> Reviewed-by: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
>> >> Signed-off-by: Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx>
>> >
>> >> +  microchip,sgpio-ports:
>> >> +    description: This is a 32-bit bitmask, configuring whether a
>> >> +      particular port in the controller is enabled or not. This allows
>> >> +      unused ports to be removed from the bitstream and reduce latency.
>> >> +    $ref: "/schemas/types.yaml#/definitions/uint32"
>> >
>> > I don't know about this.
>> >
>> > You are saying this pin controller can have up to 32 GPIO "ports"
>> > (also known as banks).
>> >
>> > Why can't you just represent each such port as a separate GPIO
>> > node:
>> >
>> > pinctrl@nnn {
>> >     gpio@0 {
>> >         ....
>> >     };
>> >     gpio@1 {
>> >         ....
>> >     };
>> >     ....
>> >     gpio@31 {
>> >         ....
>> >     };
>> > };
>> >
>> > Then if some of them are unused just set it to status = "disabled";
>> >
>> > This also makes your Linux driver simpler because each GPIO port
>> > just becomes a set of 32bit registers and you can use
>> > select GPIO_GENERIC and bgpio_init() and save a whole
>> > slew of standard stock code.
>> >
>>
>> Linus, thank you for your input.
>>
>> The controller handles an array of 32*n signals, where n >= 1 && n <=
>> 4.
>>
>> The problem with the above approach is that the ports are disabled
>> *port*-wise - so they remove all (upto) 4 bits. That would be across the
>> banks.
>>
>> You could of course have the "implied" semantics that a disabled port at
>> any bit position disabled all (bit positions for the same port).
>
> I don't understand this, you would have to elaborate...
>
> In any case microchip,sgpio-ports is probably not the right thing,
> use ngpios which is documented and just divide by 32 to get the
> number of ports I think? But that is just in case they get
> enabled strictly in sequence, otherwise you'd need a custom
> property.
>

Hi Linus,

Yes, the problem is they're not in sequence. F.ex. you could have ports
0,1 enabled, skip 2,3,4 and have 5,6,7 enabled.

In the data stream you would then have:

p0.0 p0.1 p0.2 p0.3
p1.0 p1.1 p1.2 p1.3
p5.0 p5.1 p5.2 p5.3
p6.0 p6.1 p6.2 p6.3
p7.0 p7.1 p7.2 p7.3

I will mull about this and try to come up with something better and more
understandable.

Luckily, this is not gating for integrating sparx5, so its possible
we'll just skip the SGPIO driver for now.

I'll provide an update as soon as possible.

---Lars

> Yours,
> Linus Walleij

-- 
Lars Povlsen,
Microchip