On Mon, 2022-09-05 at 14:08 +0200, Krzysztof Kozlowski wrote:
> On 05/09/2022 13:48, Niedermayr, BENEDIKT wrote:
> > On Mon, 2022-09-05 at 11:54 +0200, Krzysztof Kozlowski wrote:
> > > On 05/09/2022 11:21, Roger Quadros wrote:
> > > > On 05/09/2022 12:14, Niedermayr, BENEDIKT wrote:
> > > > > On Mon, 2022-09-05 at 11:56 +0300, Roger Quadros wrote:
> > > > > > Hi Benedikt,
> > > > > >
> > > > > > On 05/09/2022 10:17, B. Niedermayr wrote:
> > > > > > > From: Benedikt Niedermayr <
> > > > > > > benedikt.niedermayr@xxxxxxxxxxx>
> > > > > > >
> > > > > > > Add a new dt-binding for the wait-pin-polarity property
> > > > > > >
> > > > > > > Signed-off-by: Benedikt Niedermayr <
> > > > > > > benedikt.niedermayr@xxxxxxxxxxx
> > > > > > > ---
> > > > > > > .../bindings/memory-controllers/ti,gpmc-
> > > > > > > child.yaml |
> > > > > > > 7
> > > > > > > +++++++
> > > > > > > 1 file changed, 7 insertions(+)
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/memory-
> > > > > > > controllers/ti,gpmc-child.yaml
> > > > > > > b/Documentation/devicetree/bindings/memory-
> > > > > > > controllers/ti,gpmc-
> > > > > > > child.yaml
> > > > > > > index 6e3995bb1630..7c721206f10b 100644
> > > > > > > --- a/Documentation/devicetree/bindings/memory-
> > > > > > > controllers/ti,gpmc-
> > > > > > > child.yaml
> > > > > > > +++ b/Documentation/devicetree/bindings/memory-
> > > > > > > controllers/ti,gpmc-
> > > > > > > child.yaml
> > > > > > > @@ -230,6 +230,13 @@ properties:
> > > > > > > Wait-pin used by client. Must be less than
> > > > > > > "gpmc,num-
> > > > > > > waitpins".
> > > > > > > $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > >
> > > > > > > + gpmc,wait-pin-polarity:
> > > > > > > + description: |
> > > > > > > + Wait-pin polarity used by the clien. It relates to
> > > > > > > the
> > > > > > > pin
> > > > > > > defined
> > > > > >
> > > > > > did you mean "client?"
> > > > > > Can you please specify what value is for Active Low vs
> > > > > > Active
> > > > > > High?
> > > > >
> > > > > Yes, that makes sense. And yes I meant "client". My typo.....
> > > > > > > + with "gpmc,wait-pin".
> > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > > >
> > > > > > Why can't type be boolean?
> > > > >
> > > > > Of course we can use the boolean there. In that case I should
> > > > > give the
> > > > > property a more meaningful name e.g. wait-pin-active-high or
> > > > > wait-pin-
> > > > > active-low.
> > > > > Since the default behavour of this pin is Active High,
> > > > > a bool property "gpmc,wait-pin-active-low" would make more
> > > > > sense
> > > > > for
> > > > > backwards compatibility.
> > > > > If the property is missing, than the polarity stays on Active
> > > > > High like
> > > > > before.
> > > > >
> > > >
> > > > OK, in that case you don't have to clarify the polarity in
> > > > description.
> > >
> > > I don't understand (and it is not explained in commit msg), why
> > > do
> > > you
> > > need such property instead of using standard GPIO flags.
> > >
> > > The driver should use standard GPIO descriptor and standard
> > > bindings.
> > > If
> > > it cannot, this has to be explained.
> > >
> > > Best regards,
> > > Krzysztof
> >
> > I think this is beacause the GPMC controller itself is not
> > respecting
> > the GPIO flags. Instead the GPMC is reading the Line Level directly
> > (high,low) and then evaluates the logic depending how
> > the WAIT<x>PINPOLARITY bit is set in the GPMPC_CONFIG register.
> >
> > Until now gpiochip_request_own_desc() was hardcorded
> > to GPIO_ACTIVE_HIGH. An the GPMC_CONFIG register configuration has
> > no
> > relation to the GPIO setting (in the current implementation).
> > My first approach was to make this part configurable via a new
> > device
> > tree property (wait-pin-polarity).
> >
> > IMHO (correct me if I'm wrong) the current implementation also does
> > not
> > make ues of standart GPIO bindings and defines the wait pin via a
> > separate "gpmc,waitpin" binding.
> >
> > E.g. gpmc,watipin = <0> or gpmc,waitpin=<1>
> >
> > The best solution would should be when setting the binding this way
> > for
> > example: gpmc,wait-pin = <&gpiox y ACTIVE_X>
>
> Yes and I am afraid this will grow instead of adding proper GPIO
> usage.
> Any reason why it cannot be a standard GPIO pin desc?
This change (gpmc,wait-pin = <&gpiox y ACTIVE_X>) may break current
implementations when the GPMC is used with NAND devices. It comes to
some configuration in the GPMC_CONFIG register when IRQs are setup
in Nand Mode.
But when using the Nand mode the register configuration in question is
properly configured, but in a complete different context:
E.g. in am335x-baltos.dtsi:
interrupts = <0 IRQ_TYPE_NONE>, /* fifoevent */
<1
IRQ_TYPE_NONE>; /* termcount */
rb-gpios = <&gpmc 0 GPIO_ACTIVE_HIGH>;
/* gpmc_wait0 */
The "interrupts" property will configure the GPMC_CONFIG register bits
for the waitpins.
But in a non-NAND case, the "interrupt" configuration wouldn't make any
sense, since interrupts are not used at all.
The "rb-gpios" is *not* handled by the omap-gpmc(maybe somewhere in the
NAND subsystem?).
Changing the wait-pin property to "gpmc,wait-pin = <&gpmc X ACTIVE_X>"
will currently break at least 3 device trees:
arch/arm/boot/dts/omap3-devkit8000-common.dtsi
arch/arm/boot/dts/omap-zoom-common.dtsi
arch/arm/boot/dts/omap3-lilly-a83x.dtsi
I think it makes sense to implement a v3 as POC?
> > But I think the current omap-gpmc.c implementation does not offer
> > such
> > a usecase and as roger already mentioned:
> > "GPMC wait_pin polarity logic is hard-wired and doesn't depend on
> > GPIO
> > subsystem for its polarity"
>
> This part I don't get. You mean hard-wired in the driver or hard-
> wired
> in the hardware? If the first, please un-wire it. If the latter, your
> property makes no sense, right?
>
IMHO roger meant that configuring the GPMC registers via gpiolib
callbacks would be the wrong place to implementent. I implemented
the gpmc register configuration in the gpio_direction_input function.
> Best regards,
> Krzysztof
