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 at 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 at 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 at t-chip.com.cn wrote: >>>>>>>> From: Levin Du <djw at 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 at 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 at 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
