| Message ID | 20220905071717.1500568-4-benedikt.niedermayr@siemens.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | omap-gpmc wait pin additions | expand |
Hi Benedikt, On 05/09/2022 10:17, B. Niedermayr wrote: > From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> > > Add a new dt-binding for the wait-pin-polarity property > > Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> > --- > .../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? > + with "gpmc,wait-pin". > + $ref: /schemas/types.yaml#/definitions/uint32 Why can't type be boolean? > + default: 0 > + > gpmc,wait-on-read: > description: Enables wait monitoring on reads. > type: boolean cheers, -roger
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@siemens.com> > > > > Add a new dt-binding for the wait-pin-polarity property > > > > Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com > > > > > --- > > .../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. > > > + default: 0 > > + > > gpmc,wait-on-read: > > description: Enables wait monitoring on reads. > > type: boolean > > cheers, > -roger cheers, benedikt
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@siemens.com> >>> >>> Add a new dt-binding for the wait-pin-polarity property >>> >>> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com >>>> >>> --- >>> .../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. >> >>> + default: 0 >>> + >>> gpmc,wait-on-read: >>> description: Enables wait monitoring on reads. >>> type: boolean >> >> cheers, >> -roger > > cheers, > benedikt > cheers, -roger
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@siemens.com> >>>> >>>> Add a new dt-binding for the wait-pin-polarity property >>>> >>>> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com >>>>> >>>> --- >>>> .../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
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@siemens.com> > > > > > > > > > > Add a new dt-binding for the wait-pin-polarity property > > > > > > > > > > Signed-off-by: Benedikt Niedermayr < > > > > > benedikt.niedermayr@siemens.com > > > > > --- > > > > > .../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> 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" cheers, benedikt
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@siemens.com> >>>>>> >>>>>> Add a new dt-binding for the wait-pin-polarity property >>>>>> >>>>>> Signed-off-by: Benedikt Niedermayr < >>>>>> benedikt.niedermayr@siemens.com >>>>>> --- >>>>>> .../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? > > 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? Best regards, Krzysztof
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@siemens.com> > > > > > > > > > > > > > > Add a new dt-binding for the wait-pin-polarity property > > > > > > > > > > > > > > Signed-off-by: Benedikt Niedermayr < > > > > > > > benedikt.niedermayr@siemens.com > > > > > > > --- > > > > > > > .../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
Hi, On 05/09/2022 16:33, Niedermayr, BENEDIKT wrote: > 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@siemens.com> >>>>>>>> >>>>>>>> Add a new dt-binding for the wait-pin-polarity property >>>>>>>> >>>>>>>> Signed-off-by: Benedikt Niedermayr < >>>>>>>> benedikt.niedermayr@siemens.com >>>>>>>> --- >>>>>>>> .../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: Let me clarify a bit. The GPMC subsystem has one wait_pin per Chip select region. Some SoCs may have 2 Chip Selects some may have more. Each wait_pin can be used in 2 ways currently. 1) As a General purpose GPIO (GPI actually as output not supported) via the Linux GPIO subsystem 2) As a wait state signalling for memory interface independently to Linux GPIO subsystem. Via GPMC configuration. > > interrupts = <0 IRQ_TYPE_NONE>, /* fifoevent */ > <1 > IRQ_TYPE_NONE>; /* termcount */ > rb-gpios = <&gpmc 0 GPIO_ACTIVE_HIGH>; The rb-gpios is an example of (1) I listed above. Here you can use the standard GPIO polarity specifier and it should work currently. An example of (2) you can see in omap3-devkit8000-common.dtsi: ethernet@6,0 { compatible = "davicom,dm9000"; ... gpmc,mux-add-data = <0>; gpmc,device-width = <1>; --> gpmc,wait-pin = <0>; .. }; Here we set the GPMC hardware configuration to use wait_pin of Chip Select 0 to add wait state to each bus access cycle to the external Ethernet device. Linux GPIO subsystem has no role to play here and everything is dealt with in GPMC hardware. Now what this current patch series is trying to do is to add a polarity specifier to the use case (2). There is a GPMC hardware setting to decide the polarity of the wait_pin. The code just needs to get the polarity setting from DT and set this setting correctly. I don't think this has got to do anything with GPIO as it is very specific to GPMC configuration. So the vendor specific property, "gpmc,wait-pin-active-low" is appropriate I think. Hope this clarifies everything ;) > /* 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 > cheers, -roger
On Tue, 2022-09-06 at 14:38 +0300, Roger Quadros wrote: > Hi, > > On 05/09/2022 16:33, Niedermayr, BENEDIKT wrote: > > 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@siemens.com> > > > > > > > > > > > > > > > > > > Add a new dt-binding for the wait-pin-polarity > > > > > > > > > property > > > > > > > > > > > > > > > > > > Signed-off-by: Benedikt Niedermayr < > > > > > > > > > benedikt.niedermayr@siemens.com > > > > > > > > > --- > > > > > > > > > .../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: > > Let me clarify a bit. > > The GPMC subsystem has one wait_pin per Chip select region. Some SoCs > may have 2 Chip Selects some may have more. > > Each wait_pin can be used in 2 ways currently. > 1) As a General purpose GPIO (GPI actually as output not supported) > via the Linux GPIO subsystem > 2) As a wait state signalling for memory interface independently to > Linux GPIO subsystem. Via GPMC configuration. > > > interrupts = <0 IRQ_TYPE_NONE>, /* fifoevent */ > > <1 > > IRQ_TYPE_NONE>; /* termcount */ > > rb-gpios = <&gpmc 0 GPIO_ACTIVE_HIGH>; > > The rb-gpios is an example of (1) I listed above. Here you can use > the standard GPIO polarity specifier and it should work currently. > > An example of (2) you can see in omap3-devkit8000-common.dtsi: > ethernet@6,0 { > compatible = "davicom,dm9000"; > ... > gpmc,mux-add-data = <0>; > gpmc,device-width = <1>; > --> gpmc,wait-pin = <0>; > .. > }; > > Here we set the GPMC hardware configuration to use wait_pin of > Chip Select 0 to add wait state to each bus access cycle to the > external Ethernet device. Linux GPIO subsystem has no role to play > here and everything is dealt with in GPMC hardware. > > Now what this current patch series is trying to do is to add a > polarity specifier to the use case (2). > There is a GPMC hardware setting to decide the polarity of the > wait_pin. > The code just needs to get the polarity setting from DT and set > this setting correctly. > I don't think this has got to do anything with GPIO as it is very > specific > to GPMC configuration. So the vendor specific property, "gpmc,wait- > pin-active-low" > is appropriate I think. > > Hope this clarifies everything ;) > Thanks roger for clarification! It's a bit confusing that both ways work (GPI & direct GPMC) configuration. And yes, my patches are based on the direct configuration of GPMC, which has nothing to do with GPIO. The more I dig into the code myself the better I understand things and the better I now how to explain those things. I think it can be seen somthing similar to the SPI subsystem, where you can on the one hand use SPI hw-chipselects which are directly controlled by the SPI controller and not by the GPIO subsystem. and on the other hand use GPIO chipselects which where handled by the GPIO subsystem. So would propose to refactor the patches and send a v3. > > > /* 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 > > cheers, > -roger cheers, benedikt
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 + with "gpmc,wait-pin". + $ref: /schemas/types.yaml#/definitions/uint32 + default: 0 + gpmc,wait-on-read: description: Enables wait monitoring on reads. type: boolean