| Message ID | 1526615528-9707-2-git-send-email-djw@t-chip.com.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote: > From: Levin Du <djw@t-chip.com.cn> > > Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs, > which do not belong to the general pinctrl. > > Adding gpio-syscon support makes controlling regulator or > LED using these special pins very easy by reusing existing > drivers, such as gpio-regulator and led-gpio. > > Signed-off-by: Levin Du <djw@t-chip.com.cn> > > --- > > Changes in v2: > - Rename gpio_syscon10 to gpio_mute in doc > > Changes in v1: > - Refactured for general gpio-syscon usage for Rockchip SoCs. > - Add doc rockchip,gpio-syscon.txt > > .../bindings/gpio/rockchip,gpio-syscon.txt | 41 ++++++++++++++++++++++ > drivers/gpio/gpio-syscon.c | 30 ++++++++++++++++ > 2 files changed, 71 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt > > diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt > new file mode 100644 > index 0000000..b1b2a67 > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt > @@ -0,0 +1,41 @@ > +* Rockchip GPIO support for GRF_SOC_CON registers > + > +Required properties: > +- compatible: Should contain "rockchip,gpio-syscon". > +- gpio-controller: Marks the device node as a gpio controller. > +- #gpio-cells: Should be two. The first cell is the pin number and > + the second cell is used to specify the gpio polarity: > + 0 = Active high, > + 1 = Active low. There's no need for this child node. Just make the parent node a gpio controller. Rob
On 2018-05-23 2:02 AM, Rob Herring wrote: > On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote: >> From: Levin Du <djw@t-chip.com.cn> >> >> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs, >> which do not belong to the general pinctrl. >> >> Adding gpio-syscon support makes controlling regulator or >> LED using these special pins very easy by reusing existing >> drivers, such as gpio-regulator and led-gpio. >> >> Signed-off-by: Levin Du <djw@t-chip.com.cn> >> >> --- >> >> Changes in v2: >> - Rename gpio_syscon10 to gpio_mute in doc >> >> Changes in v1: >> - Refactured for general gpio-syscon usage for Rockchip SoCs. >> - Add doc rockchip,gpio-syscon.txt >> >> .../bindings/gpio/rockchip,gpio-syscon.txt | 41 ++++++++++++++++++++++ >> drivers/gpio/gpio-syscon.c | 30 ++++++++++++++++ >> 2 files changed, 71 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >> >> diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >> new file mode 100644 >> index 0000000..b1b2a67 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >> @@ -0,0 +1,41 @@ >> +* Rockchip GPIO support for GRF_SOC_CON registers >> + >> +Required properties: >> +- compatible: Should contain "rockchip,gpio-syscon". >> +- gpio-controller: Marks the device node as a gpio controller. >> +- #gpio-cells: Should be two. The first cell is the pin number and >> + the second cell is used to specify the gpio polarity: >> + 0 = Active high, >> + 1 = Active low. > There's no need for this child node. Just make the parent node a gpio > controller. > > Rob Hi Rob, it is not clear to me. Do you suggest that the grf node should be a gpio controller, like below? + grf: syscon at ff100000 { + compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf", "syscon", "simple-mfd"; + //... + gpio-controller; + #gpio-cells = <2>; + gpio,syscon-dev = <&grf 0x0428 0>; + }; or just reserve the following case in the doc? + grf: syscon at ff100000 { + compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd"; + //... + }; + + gpio_mute: gpio-mute { + compatible = "rockchip,gpio-syscon"; + gpio-controller; + #gpio-cells = <2>; + gpio,syscon-dev = <&grf 0x0428 0>; + }; Thanks Levin
On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote: > On 2018-05-23 2:02 AM, Rob Herring wrote: >> >> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote: >>> >>> From: Levin Du <djw@t-chip.com.cn> >>> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs, >>> which do not belong to the general pinctrl. >>> >>> Adding gpio-syscon support makes controlling regulator or >>> LED using these special pins very easy by reusing existing >>> drivers, such as gpio-regulator and led-gpio. >>> >>> Signed-off-by: Levin Du <djw@t-chip.com.cn> >>> >>> --- >>> >>> Changes in v2: >>> - Rename gpio_syscon10 to gpio_mute in doc >>> >>> Changes in v1: >>> - Refactured for general gpio-syscon usage for Rockchip SoCs. >>> - Add doc rockchip,gpio-syscon.txt >>> >>> .../bindings/gpio/rockchip,gpio-syscon.txt | 41 >>> ++++++++++++++++++++++ >>> drivers/gpio/gpio-syscon.c | 30 >>> ++++++++++++++++ >>> 2 files changed, 71 insertions(+) >>> create mode 100644 >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >>> >>> diff --git >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >>> new file mode 100644 >>> index 0000000..b1b2a67 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >>> @@ -0,0 +1,41 @@ >>> +* Rockchip GPIO support for GRF_SOC_CON registers >>> + >>> +Required properties: >>> +- compatible: Should contain "rockchip,gpio-syscon". >>> +- gpio-controller: Marks the device node as a gpio controller. >>> +- #gpio-cells: Should be two. The first cell is the pin number and >>> + the second cell is used to specify the gpio polarity: >>> + 0 = Active high, >>> + 1 = Active low. >> >> There's no need for this child node. Just make the parent node a gpio >> controller. >> >> Rob > > Hi Rob, it is not clear to me. Do you suggest that the grf node should be a > gpio controller, > like below? > > + grf: syscon at ff100000 { > + compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf", > "syscon", "simple-mfd"; Yes, but drop "rockchip,gpio-syscon" and "simple-mfd". > + //... > + gpio-controller; > + #gpio-cells = <2>; > + gpio,syscon-dev = <&grf 0x0428 0>; And drop this. It may be used in the kernel, but it is not a documented property. > + }; > > or just reserve the following case in the doc? > > + grf: syscon at ff100000 { > + compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd"; > + //... > + }; > + > + gpio_mute: gpio-mute { > + compatible = "rockchip,gpio-syscon"; > + gpio-controller; > + #gpio-cells = <2>; > + gpio,syscon-dev = <&grf 0x0428 0>; > + }; > > > Thanks > Levin > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rob, Levin, sorry for being late to the party. Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring: > On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote: > > On 2018-05-23 2:02 AM, Rob Herring wrote: > >> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote: > >>> From: Levin Du <djw@t-chip.com.cn> > >>> > >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs, > >>> which do not belong to the general pinctrl. > >>> > >>> Adding gpio-syscon support makes controlling regulator or > >>> LED using these special pins very easy by reusing existing > >>> drivers, such as gpio-regulator and led-gpio. > >>> > >>> Signed-off-by: Levin Du <djw@t-chip.com.cn> > >>> > >>> --- > >>> > >>> Changes in v2: > >>> - Rename gpio_syscon10 to gpio_mute in doc > >>> > >>> Changes in v1: > >>> - Refactured for general gpio-syscon usage for Rockchip SoCs. > >>> - Add doc rockchip,gpio-syscon.txt > >>> > >>> .../bindings/gpio/rockchip,gpio-syscon.txt | 41 > >>> > >>> ++++++++++++++++++++++ > >>> > >>> drivers/gpio/gpio-syscon.c | 30 > >>> > >>> ++++++++++++++++ > >>> > >>> 2 files changed, 71 insertions(+) > >>> create mode 100644 > >>> > >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt > >>> > >>> diff --git > >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt > >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt > >>> new file mode 100644 > >>> index 0000000..b1b2a67 > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt > >>> @@ -0,0 +1,41 @@ > >>> +* Rockchip GPIO support for GRF_SOC_CON registers > >>> + > >>> +Required properties: > >>> +- compatible: Should contain "rockchip,gpio-syscon". > >>> +- gpio-controller: Marks the device node as a gpio controller. > >>> +- #gpio-cells: Should be two. The first cell is the pin number and > >>> + the second cell is used to specify the gpio polarity: > >>> + 0 = Active high, > >>> + 1 = Active low. > >> > >> There's no need for this child node. Just make the parent node a gpio > >> controller. > >> > >> Rob > > > > Hi Rob, it is not clear to me. Do you suggest that the grf node should be > > a > > gpio controller, > > like below? > > > > + grf: syscon at ff100000 { > > + compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf", > > "syscon", "simple-mfd"; > > Yes, but drop "rockchip,gpio-syscon" and "simple-mfd". I would disagree quite a bit here. The grf are the "general register files", a bunch of registers used for quite a lot of things, and so it seems among other users, also a gpio-controller for some more random pins not controlled through the regular gpio controllers. For a more fully stocked grf, please see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338 So the gpio controller should definitly also be a subnode. The gpio in question is called "mute", so I'd think the gpio-syscon driver should just define a "rockchip,rk3328-gpio-mute" compatible and contain all the register voodoo in the driver itself and not define it in the dt. So it should probably look like grf: syscon at ff100000 { compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd"; [all the other syscon sub-devices] gpio_mute: gpio-mute { compatible = "rockchip,rk3328-gpio-mute"; gpio-controller; #gpio-cells = <2>; }; [more other syscon sub-devices] }; Heiko > > + //... > > + gpio-controller; > > + #gpio-cells = <2>; > > + gpio,syscon-dev = <&grf 0x0428 0>; > > And drop this. It may be used in the kernel, but it is not a > documented property. > > > + }; > > > > or just reserve the following case in the doc? > > > > + grf: syscon at ff100000 { > > + compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd"; > > + //... > > + }; > > + > > + gpio_mute: gpio-mute { > > + compatible = "rockchip,gpio-syscon"; > > + gpio-controller; > > + #gpio-cells = <2>; > > + gpio,syscon-dev = <&grf 0x0428 0>; > > + }; > > > > > > Thanks > > Levin > > > > -- > > To unsubscribe from this list: send the line "unsubscribe devicetree" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner <heiko@sntech.de> wrote: > Hi Rob, Levin, > > sorry for being late to the party. > > Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring: >> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote: >> > On 2018-05-23 2:02 AM, Rob Herring wrote: >> >> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote: >> >>> From: Levin Du <djw@t-chip.com.cn> >> >>> >> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs, >> >>> which do not belong to the general pinctrl. >> >>> >> >>> Adding gpio-syscon support makes controlling regulator or >> >>> LED using these special pins very easy by reusing existing >> >>> drivers, such as gpio-regulator and led-gpio. >> >>> >> >>> Signed-off-by: Levin Du <djw@t-chip.com.cn> >> >>> >> >>> --- >> >>> >> >>> Changes in v2: >> >>> - Rename gpio_syscon10 to gpio_mute in doc >> >>> >> >>> Changes in v1: >> >>> - Refactured for general gpio-syscon usage for Rockchip SoCs. >> >>> - Add doc rockchip,gpio-syscon.txt >> >>> >> >>> .../bindings/gpio/rockchip,gpio-syscon.txt | 41 >> >>> >> >>> ++++++++++++++++++++++ >> >>> >> >>> drivers/gpio/gpio-syscon.c | 30 >> >>> >> >>> ++++++++++++++++ >> >>> >> >>> 2 files changed, 71 insertions(+) >> >>> create mode 100644 >> >>> >> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >> >>> >> >>> diff --git >> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >> >>> new file mode 100644 >> >>> index 0000000..b1b2a67 >> >>> --- /dev/null >> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >> >>> @@ -0,0 +1,41 @@ >> >>> +* Rockchip GPIO support for GRF_SOC_CON registers >> >>> + >> >>> +Required properties: >> >>> +- compatible: Should contain "rockchip,gpio-syscon". >> >>> +- gpio-controller: Marks the device node as a gpio controller. >> >>> +- #gpio-cells: Should be two. The first cell is the pin number and >> >>> + the second cell is used to specify the gpio polarity: >> >>> + 0 = Active high, >> >>> + 1 = Active low. >> >> >> >> There's no need for this child node. Just make the parent node a gpio >> >> controller. >> >> >> >> Rob >> > >> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be >> > a >> > gpio controller, >> > like below? >> > >> > + grf: syscon at ff100000 { >> > + compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf", >> > "syscon", "simple-mfd"; >> >> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd". > > I would disagree quite a bit here. The grf are the "general register files", > a bunch of registers used for quite a lot of things, and so it seems > among other users, also a gpio-controller for some more random pins > not controlled through the regular gpio controllers. > > For a more fully stocked grf, please see > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855 > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338 > > So the gpio controller should definitly also be a subnode. Sigh, yes, if there are a bunch of functions needing subnodes like the above, then yes that makes sense. But that's not what has been presented. Please make some attempt at defining *all* the functions. An actual binding would be nice, but I'll settle for just a list of things. The list should have functions that have DT dependencies (like clocks for phys in the above) because until you do, you don't need child nodes. > The gpio in question is called "mute", so I'd think the gpio-syscon driver > should just define a "rockchip,rk3328-gpio-mute" compatible and contain > all the register voodoo in the driver itself and not define it in the dt. Is there really just one GPIO? If it has a defined function, then is it really GP? Can you control direction? I know Linus W doesn't like that kind of abuse of GPIO. > So it should probably look like > > grf: syscon at ff100000 { > compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd"; > > [all the other syscon sub-devices] > > gpio_mute: gpio-mute { > compatible = "rockchip,rk3328-gpio-mute"; > gpio-controller; > #gpio-cells = <2>; > }; > > [more other syscon sub-devices] > };
Hi all, I'd like to quote reply of Robin Murphy at http://lists.infradead.org/pipermail/linux-rockchip/2018-May/020619.html > > I would suggest s/pin number/bit number in the associated GRF register/ > here. At least in this RK3328 case there's only one pin, which isn't > numbered, and if you naively considered it pin 0 of this 'bank' you'd > already have the wrong number. Since we're dealing with the "random > SoC-specific controls" region of the GRF as opposed to the > relatively-consistent and organised pinmux parts, I don't think we > should rely on any assumptions about how things are laid out. > > I was initially going to suggest a more specific compatible string as > well, but on reflection I think the generic "rockchip,gpio-syscon" for > basic "flip this single GRF bit" functionality actually is the right way > to go. In the specific RK3328 GPIO_MUTE case, there look to be 4 bits in > total related to this pin - the enable, value, and some pull controls > (which I assume apply when the output is disabled) - if at some point in > future we *did* want to start explicitly controlling the rest of them > too, then would be a good time to define a separate > "rockchip,rk3328-gpio-mute" binding (and probably a dedicated driver) > for that specialised functionality, independently of this basic one. Shall we go the generic "rockchip,gpio-syscon" way, or the specific "rockchip,rk3328-gpio-mute" way? I prefer the former one. The property of "gpio,syscon-dev" in gpio-syscon driver should be documented. Since the gpio controller is defined in the dtsi file, which inevitably contains voodoo register addresses. But at the board level dts file, there won't be more register addresses. On 2018-05-24 3:53 AM, Rob Herring wrote: > On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner <heiko@sntech.de> wrote: >> Hi Rob, Levin, >> >> sorry for being late to the party. >> >> Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring: >>> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote: >>>> On 2018-05-23 2:02 AM, Rob Herring wrote: >>>>> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote: >>>>>> From: Levin Du <djw@t-chip.com.cn> >>>>>> >>>>>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs, >>>>>> which do not belong to the general pinctrl. >>>>>> >>>>>> Adding gpio-syscon support makes controlling regulator or >>>>>> LED using these special pins very easy by reusing existing >>>>>> drivers, such as gpio-regulator and led-gpio. >>>>>> >>>>>> Signed-off-by: Levin Du <djw@t-chip.com.cn> >>>>>> >>>>>> --- >>>>>> >>>>>> Changes in v2: >>>>>> - Rename gpio_syscon10 to gpio_mute in doc >>>>>> >>>>>> Changes in v1: >>>>>> - Refactured for general gpio-syscon usage for Rockchip SoCs. >>>>>> - Add doc rockchip,gpio-syscon.txt >>>>>> >>>>>> .../bindings/gpio/rockchip,gpio-syscon.txt | 41 >>>>>> >>>>>> ++++++++++++++++++++++ >>>>>> >>>>>> drivers/gpio/gpio-syscon.c | 30 >>>>>> >>>>>> ++++++++++++++++ >>>>>> >>>>>> 2 files changed, 71 insertions(+) >>>>>> create mode 100644 >>>>>> >>>>>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >>>>>> >>>>>> diff --git >>>>>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >>>>>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >>>>>> new file mode 100644 >>>>>> index 0000000..b1b2a67 >>>>>> --- /dev/null >>>>>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >>>>>> @@ -0,0 +1,41 @@ >>>>>> +* Rockchip GPIO support for GRF_SOC_CON registers >>>>>> + >>>>>> +Required properties: >>>>>> +- compatible: Should contain "rockchip,gpio-syscon". >>>>>> +- gpio-controller: Marks the device node as a gpio controller. >>>>>> +- #gpio-cells: Should be two. The first cell is the pin number and >>>>>> + the second cell is used to specify the gpio polarity: >>>>>> + 0 = Active high, >>>>>> + 1 = Active low. >>>>> There's no need for this child node. Just make the parent node a gpio >>>>> controller. >>>>> >>>>> Rob >>>> Hi Rob, it is not clear to me. Do you suggest that the grf node should be >>>> a >>>> gpio controller, >>>> like below? >>>> >>>> + grf: syscon at ff100000 { >>>> + compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf", >>>> "syscon", "simple-mfd"; >>> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd". >> I would disagree quite a bit here. The grf are the "general register files", >> a bunch of registers used for quite a lot of things, and so it seems >> among other users, also a gpio-controller for some more random pins >> not controlled through the regular gpio controllers. >> >> For a more fully stocked grf, please see >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855 >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338 >> >> So the gpio controller should definitly also be a subnode. > Sigh, yes, if there are a bunch of functions needing subnodes like the > above, then yes that makes sense. But that's not what has been > presented. Please make some attempt at defining *all* the functions. > An actual binding would be nice, but I'll settle for just a list of > things. The list should have functions that have DT dependencies (like > clocks for phys in the above) because until you do, you don't need > child nodes. In rk3328.dtsi file, there are lots of line "rockchip,grf = <&grf>;" in various nodes, such as tsadc, cru, gmac2io, gmac2phy, and also pinctrl, which are not sub nodes of `grf`, but for reference only. The gpio-syscon node should also have similar behavior. They are not strongly coupled. The gpio-syscon node should be defined outside of the `grf` node. > >> The gpio in question is called "mute", so I'd think the gpio-syscon driver >> should just define a "rockchip,rk3328-gpio-mute" compatible and contain >> all the register voodoo in the driver itself and not define it in the dt. > Is there really just one GPIO? If it has a defined function, then is > it really GP? Can you control direction? I know Linus W doesn't like > that kind of abuse of GPIO. The "mute" pin is a output only GPIO, which is already supported by setting flags in the gpio-syscon driver. And yes, this pin has a defined function, but can also be used for general purpose operation. Thanks Levin
On Wed, May 23, 2018 at 5:12 PM, Heiko Stübner <heiko@sntech.de> wrote: > So the gpio controller should definitly also be a subnode. > > The gpio in question is called "mute", so I'd think the gpio-syscon driver > should just define a "rockchip,rk3328-gpio-mute" compatible and contain > all the register voodoo in the driver itself and not define it in the dt. > > So it should probably look like > > grf: syscon at ff100000 { > compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd"; > > [all the other syscon sub-devices] > > gpio_mute: gpio-mute { > compatible = "rockchip,rk3328-gpio-mute"; > gpio-controller; > #gpio-cells = <2>; > }; I'm sceptic. That doesn't sound like "general purpose input output" at all. It sounds like special purpose, for a mute button. Does it use IRQ? I would recommend implementing drivers/input/keyboard/syscon-keys.c in the same vein as drivers/leds/leds-syscon.c so you can avoid indirection through GPIO for no good reason at all. I already have other good uses for such a generic input driver. Yours, Linus Walleij
Hi Linus, Am Donnerstag, 24. Mai 2018, 10:28:44 CEST schrieb Linus Walleij: > On Wed, May 23, 2018 at 5:12 PM, Heiko Stübner <heiko@sntech.de> wrote: > > So the gpio controller should definitly also be a subnode. > > > > The gpio in question is called "mute", so I'd think the gpio-syscon driver > > should just define a "rockchip,rk3328-gpio-mute" compatible and contain > > all the register voodoo in the driver itself and not define it in the dt. > > > > So it should probably look like > > > > grf: syscon at ff100000 { > > > > compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd"; > > > > [all the other syscon sub-devices] > > > > gpio_mute: gpio-mute { > > > > compatible = "rockchip,rk3328-gpio-mute"; > > gpio-controller; > > #gpio-cells = <2>; > > > > }; > > I'm sceptic. > > That doesn't sound like "general purpose input output" at all. > > It sounds like special purpose, for a mute button. > > Does it use IRQ? I would recommend implementing > drivers/input/keyboard/syscon-keys.c in the same vein > as drivers/leds/leds-syscon.c so you can avoid indirection > through GPIO for no good reason at all. To quote Levin from the other mail: -------- The "mute" pin is a output only GPIO, which is already supported by setting flags in the gpio-syscon driver. And yes, this pin has a defined function, but can also be used for general purpose operation. -------- So to summarize, the documentation calls it "mute", but it is usable as a general pin, which is the reason Levin is working on it - because on his board this pin is used to switch between two voltages (aka a gpio-regulator) for the sdmmc controller [3.3V + 1.8V]. Available pin settings are output-enable + of course the high/low setting and I think I remember there is even a pull setting for it in the GRF somewhere - but my memory might be fuzzy here. Heiko
On Thu, May 24, 2018 at 10:35 AM, Heiko Stübner <heiko@sntech.de> wrote: > Am Donnerstag, 24. Mai 2018, 10:28:44 CEST schrieb Linus Walleij: >> On Wed, May 23, 2018 at 5:12 PM, Heiko Stübner <heiko@sntech.de> wrote: >> > So the gpio controller should definitly also be a subnode. >> > >> > The gpio in question is called "mute", so I'd think the gpio-syscon driver >> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain >> > all the register voodoo in the driver itself and not define it in the dt. >> > >> > So it should probably look like >> > >> > grf: syscon at ff100000 { >> > >> > compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd"; >> > >> > [all the other syscon sub-devices] >> > >> > gpio_mute: gpio-mute { >> > >> > compatible = "rockchip,rk3328-gpio-mute"; >> > gpio-controller; >> > #gpio-cells = <2>; >> > >> > }; >> >> I'm sceptic. >> >> That doesn't sound like "general purpose input output" at all. >> >> It sounds like special purpose, for a mute button. >> >> Does it use IRQ? I would recommend implementing >> drivers/input/keyboard/syscon-keys.c in the same vein >> as drivers/leds/leds-syscon.c so you can avoid indirection >> through GPIO for no good reason at all. > > To quote Levin from the other mail: > -------- > The "mute" pin is a output only GPIO, which is already supported by > setting flags in the gpio-syscon > driver. And yes, this pin has a defined function, but can also be used > for general purpose operation. > -------- > > So to summarize, the documentation calls it "mute", but it is usable as > a general pin, which is the reason Levin is working on it - because on his > board this pin is used to switch between two voltages (aka a gpio-regulator) > for the sdmmc controller [3.3V + 1.8V]. OK then, I was wrong! :) Go ahead with this, sorry for the fuzz. Yours, Linus Walleij
Hi Rob, Am Mittwoch, 23. Mai 2018, 21:53:53 CEST schrieb Rob Herring: > On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner <heiko@sntech.de> wrote: > > Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring: > >> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote: > >> > On 2018-05-23 2:02 AM, Rob Herring wrote: > >> >> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote: > >> >>> From: Levin Du <djw@t-chip.com.cn> > >> >>> > >> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs, > >> >>> which do not belong to the general pinctrl. > >> >>> > >> >>> Adding gpio-syscon support makes controlling regulator or > >> >>> LED using these special pins very easy by reusing existing > >> >>> drivers, such as gpio-regulator and led-gpio. > >> >>> > >> >>> Signed-off-by: Levin Du <djw@t-chip.com.cn> > >> >>> > >> >>> --- > >> >>> > >> >>> Changes in v2: > >> >>> - Rename gpio_syscon10 to gpio_mute in doc > >> >>> > >> >>> Changes in v1: > >> >>> - Refactured for general gpio-syscon usage for Rockchip SoCs. > >> >>> - Add doc rockchip,gpio-syscon.txt > >> >>> > >> >>> .../bindings/gpio/rockchip,gpio-syscon.txt | 41 > >> >>> > >> >>> ++++++++++++++++++++++ > >> >>> > >> >>> drivers/gpio/gpio-syscon.c | 30 > >> >>> > >> >>> ++++++++++++++++ > >> >>> > >> >>> 2 files changed, 71 insertions(+) > >> >>> create mode 100644 > >> >>> > >> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt > >> >>> > >> >>> diff --git > >> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt > >> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt > >> >>> new file mode 100644 > >> >>> index 0000000..b1b2a67 > >> >>> --- /dev/null > >> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt > >> >>> @@ -0,0 +1,41 @@ > >> >>> +* Rockchip GPIO support for GRF_SOC_CON registers > >> >>> + > >> >>> +Required properties: > >> >>> +- compatible: Should contain "rockchip,gpio-syscon". > >> >>> +- gpio-controller: Marks the device node as a gpio controller. > >> >>> +- #gpio-cells: Should be two. The first cell is the pin number and > >> >>> + the second cell is used to specify the gpio polarity: > >> >>> + 0 = Active high, > >> >>> + 1 = Active low. > >> >> > >> >> There's no need for this child node. Just make the parent node a gpio > >> >> controller. > >> >> > >> >> Rob > >> > > >> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be > >> > a > >> > gpio controller, > >> > like below? > >> > > >> > + grf: syscon at ff100000 { > >> > + compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf", > >> > "syscon", "simple-mfd"; > >> > >> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd". > > > > I would disagree quite a bit here. The grf are the "general register files", > > a bunch of registers used for quite a lot of things, and so it seems > > among other users, also a gpio-controller for some more random pins > > not controlled through the regular gpio controllers. > > > > For a more fully stocked grf, please see > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855 > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338 > > > > So the gpio controller should definitly also be a subnode. > > Sigh, yes, if there are a bunch of functions needing subnodes like the > above, then yes that makes sense. But that's not what has been > presented. Please make some attempt at defining *all* the functions. > An actual binding would be nice, but I'll settle for just a list of > things. The list should have functions that have DT dependencies (like > clocks for phys in the above) because until you do, you don't need > child nodes. That's the problem with the Rockchip-GRF, you only realize its content when implementing specific features. Like on the rk3399 the table of the register-list of the GRF alone is 11 pages long with the register details tables taking up another 230 pages. And functional description is often somewhat thin. So I'm not sure I fully understand what you're asking, but in general we define the bindings for sub-devices when tackling these individual components, see for example https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=72580a49a837c2c7da83f698c00592eac41537d8 which also has a real phy-driver behind it and binding against that subnode of the GRF simple-mfd. These are real IP blocks somewhere on the socs, with regular supplies like resets, clocks etc in most cases. Only their controlling registers got dumped into the GRF for some reason. And in retrospect it really looks like we're doing something right, because it seems these bindings seem quite stable over time. > > The gpio in question is called "mute", so I'd think the gpio-syscon driver > > should just define a "rockchip,rk3328-gpio-mute" compatible and contain > > all the register voodoo in the driver itself and not define it in the dt. > > Is there really just one GPIO? If it has a defined function, then is > it really GP? Can you control direction? I know Linus W doesn't like > that kind of abuse of GPIO. looks like I convinced Linus that we're not abusing anything with this :-) . Heiko
Hi Levin, Am Donnerstag, 24. Mai 2018, 03:59:36 CEST schrieb Levin Du: > Hi all, I'd like to quote reply of Robin Murphy at > http://lists.infradead.org/pipermail/linux-rockchip/2018-May/020619.html > > > > > I would suggest s/pin number/bit number in the associated GRF register/ > > here. At least in this RK3328 case there's only one pin, which isn't > > numbered, and if you naively considered it pin 0 of this 'bank' you'd > > already have the wrong number. Since we're dealing with the "random > > SoC-specific controls" region of the GRF as opposed to the > > relatively-consistent and organised pinmux parts, I don't think we > > should rely on any assumptions about how things are laid out. > > > > I was initially going to suggest a more specific compatible string as > > well, but on reflection I think the generic "rockchip,gpio-syscon" for > > basic "flip this single GRF bit" functionality actually is the right way > > to go. In the specific RK3328 GPIO_MUTE case, there look to be 4 bits in > > total related to this pin - the enable, value, and some pull controls > > (which I assume apply when the output is disabled) - if at some point in > > future we *did* want to start explicitly controlling the rest of them > > too, then would be a good time to define a separate > > "rockchip,rk3328-gpio-mute" binding (and probably a dedicated driver) > > for that specialised functionality, independently of this basic one. > > > Shall we go the generic "rockchip,gpio-syscon" way, or the specific > "rockchip,rk3328-gpio-mute" way? I prefer the former one. > > The property of "gpio,syscon-dev" in gpio-syscon driver should be > documented. > Since the gpio controller is defined in the dtsi file, which inevitably > contains voodoo > register addresses. But at the board level dts file, there won't be more > register > addresses. Past experience shows that the GRF is not really suitable for generalization, as it's more of a dumping ground where chip designers can put everything that's left over. This is especially true for GRF_SOC_CONx registers, that really only contain pretty random bits. So personally, I'd really prefer soc-specific compatibles as everywhere else, instead of trying to push stuff into the devicetree that won't hold up on future socs. > On 2018-05-24 3:53 AM, Rob Herring wrote: > > On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner <heiko@sntech.de> wrote: > >> Hi Rob, Levin, > >> > >> sorry for being late to the party. > >> > >> Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring: > >>> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote: > >>>> On 2018-05-23 2:02 AM, Rob Herring wrote: > >>>>> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote: > >>>>>> From: Levin Du <djw@t-chip.com.cn> > >>>>>> > >>>>>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs, > >>>>>> which do not belong to the general pinctrl. > >>>>>> > >>>>>> Adding gpio-syscon support makes controlling regulator or > >>>>>> LED using these special pins very easy by reusing existing > >>>>>> drivers, such as gpio-regulator and led-gpio. > >>>>>> > >>>>>> Signed-off-by: Levin Du <djw@t-chip.com.cn> > >>>>>> > >>>>>> --- > >>>>>> > >>>>>> Changes in v2: > >>>>>> - Rename gpio_syscon10 to gpio_mute in doc > >>>>>> > >>>>>> Changes in v1: > >>>>>> - Refactured for general gpio-syscon usage for Rockchip SoCs. > >>>>>> - Add doc rockchip,gpio-syscon.txt > >>>>>> > >>>>>> .../bindings/gpio/rockchip,gpio-syscon.txt | 41 > >>>>>> > >>>>>> ++++++++++++++++++++++ > >>>>>> > >>>>>> drivers/gpio/gpio-syscon.c | 30 > >>>>>> > >>>>>> ++++++++++++++++ > >>>>>> > >>>>>> 2 files changed, 71 insertions(+) > >>>>>> create mode 100644 > >>>>>> > >>>>>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt > >>>>>> > >>>>>> diff --git > >>>>>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt > >>>>>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt > >>>>>> new file mode 100644 > >>>>>> index 0000000..b1b2a67 > >>>>>> --- /dev/null > >>>>>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt > >>>>>> @@ -0,0 +1,41 @@ > >>>>>> +* Rockchip GPIO support for GRF_SOC_CON registers > >>>>>> + > >>>>>> +Required properties: > >>>>>> +- compatible: Should contain "rockchip,gpio-syscon". > >>>>>> +- gpio-controller: Marks the device node as a gpio controller. > >>>>>> +- #gpio-cells: Should be two. The first cell is the pin number and > >>>>>> + the second cell is used to specify the gpio polarity: > >>>>>> + 0 = Active high, > >>>>>> + 1 = Active low. > >>>>> There's no need for this child node. Just make the parent node a gpio > >>>>> controller. > >>>>> > >>>>> Rob > >>>> Hi Rob, it is not clear to me. Do you suggest that the grf node should be > >>>> a > >>>> gpio controller, > >>>> like below? > >>>> > >>>> + grf: syscon at ff100000 { > >>>> + compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf", > >>>> "syscon", "simple-mfd"; > >>> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd". > >> I would disagree quite a bit here. The grf are the "general register files", > >> a bunch of registers used for quite a lot of things, and so it seems > >> among other users, also a gpio-controller for some more random pins > >> not controlled through the regular gpio controllers. > >> > >> For a more fully stocked grf, please see > >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855 > >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338 > >> > >> So the gpio controller should definitly also be a subnode. > > Sigh, yes, if there are a bunch of functions needing subnodes like the > > above, then yes that makes sense. But that's not what has been > > presented. Please make some attempt at defining *all* the functions. > > An actual binding would be nice, but I'll settle for just a list of > > things. The list should have functions that have DT dependencies (like > > clocks for phys in the above) because until you do, you don't need > > child nodes. > > In rk3328.dtsi file, there are lots of line "rockchip,grf = <&grf>;" in > various nodes, > such as tsadc, cru, gmac2io, gmac2phy, and also pinctrl, which are not > sub nodes of > `grf`, but for reference only. The gpio-syscon node should also have > similar behavior. > They are not strongly coupled. The gpio-syscon node should be defined > outside of the > `grf` node. Not necessarily. I.e. things like the tsadc, cru etc have their own register space and only some minor additional bits inside the GRF. Other things like some phys and your mute-gpio are _fully embedded_ inside the GRF and thus become child devices. This describes the hardware layout way better, helps unclutter the devicetree and also shows this distinction between "additional bits" and "embedded" clearly. Heiko
On Thu, May 24, 2018 at 7:07 AM, Heiko Stuebner <heiko@sntech.de> wrote: > Hi Rob, > > Am Mittwoch, 23. Mai 2018, 21:53:53 CEST schrieb Rob Herring: >> On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner <heiko@sntech.de> wrote: >> > Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring: >> >> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote: >> >> > On 2018-05-23 2:02 AM, Rob Herring wrote: >> >> >> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote: >> >> >>> From: Levin Du <djw@t-chip.com.cn> >> >> >>> >> >> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs, >> >> >>> which do not belong to the general pinctrl. >> >> >>> >> >> >>> Adding gpio-syscon support makes controlling regulator or >> >> >>> LED using these special pins very easy by reusing existing >> >> >>> drivers, such as gpio-regulator and led-gpio. >> >> >>> >> >> >>> Signed-off-by: Levin Du <djw@t-chip.com.cn> >> >> >>> >> >> >>> --- >> >> >>> >> >> >>> Changes in v2: >> >> >>> - Rename gpio_syscon10 to gpio_mute in doc >> >> >>> >> >> >>> Changes in v1: >> >> >>> - Refactured for general gpio-syscon usage for Rockchip SoCs. >> >> >>> - Add doc rockchip,gpio-syscon.txt >> >> >>> >> >> >>> .../bindings/gpio/rockchip,gpio-syscon.txt | 41 >> >> >>> >> >> >>> ++++++++++++++++++++++ >> >> >>> >> >> >>> drivers/gpio/gpio-syscon.c | 30 >> >> >>> >> >> >>> ++++++++++++++++ >> >> >>> >> >> >>> 2 files changed, 71 insertions(+) >> >> >>> create mode 100644 >> >> >>> >> >> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >> >> >>> >> >> >>> diff --git >> >> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >> >> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >> >> >>> new file mode 100644 >> >> >>> index 0000000..b1b2a67 >> >> >>> --- /dev/null >> >> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >> >> >>> @@ -0,0 +1,41 @@ >> >> >>> +* Rockchip GPIO support for GRF_SOC_CON registers >> >> >>> + >> >> >>> +Required properties: >> >> >>> +- compatible: Should contain "rockchip,gpio-syscon". >> >> >>> +- gpio-controller: Marks the device node as a gpio controller. >> >> >>> +- #gpio-cells: Should be two. The first cell is the pin number and >> >> >>> + the second cell is used to specify the gpio polarity: >> >> >>> + 0 = Active high, >> >> >>> + 1 = Active low. >> >> >> >> >> >> There's no need for this child node. Just make the parent node a gpio >> >> >> controller. >> >> >> >> >> >> Rob >> >> > >> >> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be >> >> > a >> >> > gpio controller, >> >> > like below? >> >> > >> >> > + grf: syscon at ff100000 { >> >> > + compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf", >> >> > "syscon", "simple-mfd"; >> >> >> >> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd". >> > >> > I would disagree quite a bit here. The grf are the "general register files", >> > a bunch of registers used for quite a lot of things, and so it seems >> > among other users, also a gpio-controller for some more random pins >> > not controlled through the regular gpio controllers. >> > >> > For a more fully stocked grf, please see >> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855 >> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338 >> > >> > So the gpio controller should definitly also be a subnode. >> >> Sigh, yes, if there are a bunch of functions needing subnodes like the >> above, then yes that makes sense. But that's not what has been >> presented. Please make some attempt at defining *all* the functions. >> An actual binding would be nice, but I'll settle for just a list of >> things. The list should have functions that have DT dependencies (like >> clocks for phys in the above) because until you do, you don't need >> child nodes. > > That's the problem with the Rockchip-GRF, you only realize its content > when implementing specific features. > > Like on the rk3399 the table of the register-list of the GRF alone is 11 > pages long with the register details tables taking up another 230 pages. > And functional description is often somewhat thin. But surely one can scan thru it and have some clue what functions there are. For example, does this chip have phy registers in GRF? > So I'm not sure I fully understand what you're asking, but in general > we define the bindings for sub-devices when tackling these individual > components, see for example > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=72580a49a837c2c7da83f698c00592eac41537d8 Yes, and in that case it makes sense. The individual functions themselves have resources defined in DT like clocks. What I don't want to see are child nodes defining *only* a compatible and any provider properties (e.g. #gpio-cells). The only reason to do that is to make Linux bind a driver, but DT is not a list of drivers to bind. This is what I don't want to see: syscon { compatible = "foo,soc-sysctrl", "syscon", "simple-mfd"; reg = <...>; clock-controller { compatible = "foo,soc-sysctrl-clocks"; #clock-cells = <1>; }; reset-controller { compatible = "foo,soc-sysctrl-resets"; #reset-cells = <1>; }; gpio { compatible = "foo,soc-sysctrl-gpios"; #gpio-cells = <2>; gpio-controller; }; }; But rather: syscon { compatible = "foo,soc-sysctrl"; reg = <...>; #clock-cells = <1>; #reset-cells = <1>; #gpio-cells = <2>; gpio-controller; }; > which also has a real phy-driver behind it and binding against that > subnode of the GRF simple-mfd. > > These are real IP blocks somewhere on the socs, with regular supplies > like resets, clocks etc in most cases. Only their controlling registers > got dumped into the GRF for some reason. I can tell that from your examples, but I can't tell that with this binding. For this binding, it looks like you are adding a sub-node for 1 register bit. That wouldn't scale if you have 11 page register list. > And in retrospect it really looks like we're doing something right, > because it seems these bindings seem quite stable over time. > > >> > The gpio in question is called "mute", so I'd think the gpio-syscon driver >> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain >> > all the register voodoo in the driver itself and not define it in the dt. >> >> Is there really just one GPIO? If it has a defined function, then is >> it really GP? Can you control direction? I know Linus W doesn't like >> that kind of abuse of GPIO. > > looks like I convinced Linus that we're not abusing anything with this :-) . Okay, but still my question remains: is it really only 1 GPIO? Dropping "-mute" would be more future proof if not. Rob
On 2018-05-24 8:18 PM, Heiko Stuebner wrote: > Hi Levin, > > Am Donnerstag, 24. Mai 2018, 03:59:36 CEST schrieb Levin Du: >> Hi all, I'd like to quote reply of Robin Murphy at >> http://lists.infradead.org/pipermail/linux-rockchip/2018-May/020619.html >> >>> I would suggest s/pin number/bit number in the associated GRF register/ >>> here. At least in this RK3328 case there's only one pin, which isn't >>> numbered, and if you naively considered it pin 0 of this 'bank' you'd >>> already have the wrong number. Since we're dealing with the "random >>> SoC-specific controls" region of the GRF as opposed to the >>> relatively-consistent and organised pinmux parts, I don't think we >>> should rely on any assumptions about how things are laid out. >>> >>> I was initially going to suggest a more specific compatible string as >>> well, but on reflection I think the generic "rockchip,gpio-syscon" for >>> basic "flip this single GRF bit" functionality actually is the right way >>> to go. In the specific RK3328 GPIO_MUTE case, there look to be 4 bits in >>> total related to this pin - the enable, value, and some pull controls >>> (which I assume apply when the output is disabled) - if at some point in >>> future we *did* want to start explicitly controlling the rest of them >>> too, then would be a good time to define a separate >>> "rockchip,rk3328-gpio-mute" binding (and probably a dedicated driver) >>> for that specialised functionality, independently of this basic one. >> >> Shall we go the generic "rockchip,gpio-syscon" way, or the specific >> "rockchip,rk3328-gpio-mute" way? I prefer the former one. >> >> The property of "gpio,syscon-dev" in gpio-syscon driver should be >> documented. >> Since the gpio controller is defined in the dtsi file, which inevitably >> contains voodoo >> register addresses. But at the board level dts file, there won't be more >> register >> addresses. > Past experience shows that the GRF is not really suitable for > generalization, as it's more of a dumping ground where chip designers > can put everything that's left over. This is especially true for > GRF_SOC_CONx registers, that really only contain pretty random bits. > > So personally, I'd really prefer soc-specific compatibles as everywhere > else, instead of trying to push stuff into the devicetree that won't hold > up on future socs. > > >> On 2018-05-24 3:53 AM, Rob Herring wrote: >>> On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner <heiko@sntech.de> wrote: >>>> Hi Rob, Levin, >>>> >>>> sorry for being late to the party. >>>> >>>> Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring: >>>>> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote: >>>>>> On 2018-05-23 2:02 AM, Rob Herring wrote: >>>>>>> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote: >>>>>>>> From: Levin Du <djw@t-chip.com.cn> >>>>>>>> >>>>>>>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs, >>>>>>>> which do not belong to the general pinctrl. >>>>>>>> >>>>>>>> Adding gpio-syscon support makes controlling regulator or >>>>>>>> LED using these special pins very easy by reusing existing >>>>>>>> drivers, such as gpio-regulator and led-gpio. >>>>>>>> >>>>>>>> Signed-off-by: Levin Du <djw@t-chip.com.cn> >>>>>>>> >>>>>>>> --- >>>>>>>> >>>>>>>> Changes in v2: >>>>>>>> - Rename gpio_syscon10 to gpio_mute in doc >>>>>>>> >>>>>>>> Changes in v1: >>>>>>>> - Refactured for general gpio-syscon usage for Rockchip SoCs. >>>>>>>> - Add doc rockchip,gpio-syscon.txt >>>>>>>> >>>>>>>> .../bindings/gpio/rockchip,gpio-syscon.txt | 41 >>>>>>>> >>>>>>>> ++++++++++++++++++++++ >>>>>>>> >>>>>>>> drivers/gpio/gpio-syscon.c | 30 >>>>>>>> >>>>>>>> ++++++++++++++++ >>>>>>>> >>>>>>>> 2 files changed, 71 insertions(+) >>>>>>>> create mode 100644 >>>>>>>> >>>>>>>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >>>>>>>> >>>>>>>> diff --git >>>>>>>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >>>>>>>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >>>>>>>> new file mode 100644 >>>>>>>> index 0000000..b1b2a67 >>>>>>>> --- /dev/null >>>>>>>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt >>>>>>>> @@ -0,0 +1,41 @@ >>>>>>>> +* Rockchip GPIO support for GRF_SOC_CON registers >>>>>>>> + >>>>>>>> +Required properties: >>>>>>>> +- compatible: Should contain "rockchip,gpio-syscon". >>>>>>>> +- gpio-controller: Marks the device node as a gpio controller. >>>>>>>> +- #gpio-cells: Should be two. The first cell is the pin number and >>>>>>>> + the second cell is used to specify the gpio polarity: >>>>>>>> + 0 = Active high, >>>>>>>> + 1 = Active low. >>>>>>> There's no need for this child node. Just make the parent node a gpio >>>>>>> controller. >>>>>>> >>>>>>> Rob >>>>>> Hi Rob, it is not clear to me. Do you suggest that the grf node should be >>>>>> a >>>>>> gpio controller, >>>>>> like below? >>>>>> >>>>>> + grf: syscon at ff100000 { >>>>>> + compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf", >>>>>> "syscon", "simple-mfd"; >>>>> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd". >>>> I would disagree quite a bit here. The grf are the "general register files", >>>> a bunch of registers used for quite a lot of things, and so it seems >>>> among other users, also a gpio-controller for some more random pins >>>> not controlled through the regular gpio controllers. >>>> >>>> For a more fully stocked grf, please see >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855 >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338 >>>> >>>> So the gpio controller should definitly also be a subnode. >>> Sigh, yes, if there are a bunch of functions needing subnodes like the >>> above, then yes that makes sense. But that's not what has been >>> presented. Please make some attempt at defining *all* the functions. >>> An actual binding would be nice, but I'll settle for just a list of >>> things. The list should have functions that have DT dependencies (like >>> clocks for phys in the above) because until you do, you don't need >>> child nodes. >> In rk3328.dtsi file, there are lots of line "rockchip,grf = <&grf>;" in >> various nodes, >> such as tsadc, cru, gmac2io, gmac2phy, and also pinctrl, which are not >> sub nodes of >> `grf`, but for reference only. The gpio-syscon node should also have >> similar behavior. >> They are not strongly coupled. The gpio-syscon node should be defined >> outside of the >> `grf` node. > Not necessarily. > > I.e. things like the tsadc, cru etc have their own register space and only > some minor additional bits inside the GRF. > > Other things like some phys and your mute-gpio are _fully embedded_ inside > the GRF and thus become child devices. This describes the hardware layout > way better, helps unclutter the devicetree and also shows this distinction > between "additional bits" and "embedded" clearly. > > > Heiko Your good point convinced me. I'd like to discuss the V3 patch here. Since there's only one GPIO pin (the GPIO_MUTE pin) in GRF_SOC_CON10 register, the GPIO controller is named `gpio-mute` and has only one GPIO pin which is referred to as `<&gpio-mute 0>`: In rk3328.dtsi: grf: syscon@ff100000 { //... /* The GPIO_MUTE pin is referred to as <&gpio-mute 0>.*/ gpio_mute: gpio-mute { compatible = "rockchip,rk3328-mute-gpio"; gpio-controller; #gpio-cells = <2>; }; }; In gpio-syscon.c: static const struct syscon_gpio_data rockchip_rk3328_mute_gpio = { /* rk3328 mute gpio is an output only pin at GRF_SOC_CON10[1] */ .flags = GPIO_SYSCON_FEAT_OUT, .bit_count = 1, .dat_bit_offset = 0x0428 * 8 + 1, .set = rockchip_gpio_set, }; static const struct of_device_id syscon_gpio_ids[] = { //... { .compatible = "rockchip,rk3328-mute-gpio", .data = &rockchip_rk3328_mute_gpio, }, {} }; Compared to V0 patch, the bit_count changes from 2 to 1 and the dat_bit_offset increases 1. Therefore the GPIO_MUTE pin is now referred to as `<&gpio-mute 0>`. IMHO it is better than `<&gpio-mute 1>` in the V0 patch. In V0, `1` is the physical offset of the output pin in register and <&gpio-mute 0> is an invalid GPIO. Thanks Levin
diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt new file mode 100644 index 0000000..b1b2a67 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt @@ -0,0 +1,41 @@ +* Rockchip GPIO support for GRF_SOC_CON registers + +Required properties: +- compatible: Should contain "rockchip,gpio-syscon". +- gpio-controller: Marks the device node as a gpio controller. +- #gpio-cells: Should be two. The first cell is the pin number and + the second cell is used to specify the gpio polarity: + 0 = Active high, + 1 = Active low. +- gpio,syscon-dev: Should contain <grf_phandle syscon_offset 0>. + If declared as child of the grf node, the grf_phandle can be 0. + +Example: + +1. As child of grf node: + + grf: syscon@ff100000 { + compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd"; + + gpio_mute: gpio-mute { + compatible = "rockchip,gpio-syscon"; + gpio-controller; + #gpio-cells = <2>; + gpio,syscon-dev = <0 0x0428 0>; + }; + }; + + +2. Not child of grf node: + + grf: syscon@ff100000 { + compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd"; + //... + }; + + gpio_mute: gpio-mute { + compatible = "rockchip,gpio-syscon"; + gpio-controller; + #gpio-cells = <2>; + gpio,syscon-dev = <&grf 0x0428 0>; + }; diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c index 7325b86..e24b408 100644 --- a/drivers/gpio/gpio-syscon.c +++ b/drivers/gpio/gpio-syscon.c @@ -135,6 +135,32 @@ static const struct syscon_gpio_data clps711x_mctrl_gpio = { .dat_bit_offset = 0x40 * 8 + 8, }; +static void rockchip_gpio_set(struct gpio_chip *chip, unsigned int offset, + int val) +{ + struct syscon_gpio_priv *priv = gpiochip_get_data(chip); + unsigned int offs; + u8 bit; + u32 data; + int ret; + + offs = priv->dreg_offset + priv->data->dat_bit_offset + offset; + bit = offs % SYSCON_REG_BITS; + data = (val ? BIT(bit) : 0) | BIT(bit + 16); + ret = regmap_write(priv->syscon, + (offs / SYSCON_REG_BITS) * SYSCON_REG_SIZE, + data); + if (ret < 0) + dev_err(chip->parent, "gpio write failed ret(%d)\n", ret); +} + +static const struct syscon_gpio_data rockchip_gpio_syscon = { + /* Rockchip GRF_SOC_CON Bits 0-15 */ + .flags = GPIO_SYSCON_FEAT_OUT, + .bit_count = 16, + .set = rockchip_gpio_set, +}; + #define KEYSTONE_LOCK_BIT BIT(0) static void keystone_gpio_set(struct gpio_chip *chip, unsigned offset, int val) @@ -175,6 +201,10 @@ static const struct of_device_id syscon_gpio_ids[] = { .compatible = "ti,keystone-dsp-gpio", .data = &keystone_dsp_gpio, }, + { + .compatible = "rockchip,gpio-syscon", + .data = &rockchip_gpio_syscon, + }, { } }; MODULE_DEVICE_TABLE(of, syscon_gpio_ids);