| Message ID | 20230222183932.33267-1-arinc.unal@arinc9.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | pinctrl: ralink: fix ABI, improve driver, move to mediatek, improve dt-bindings | expand |
Hi Arınç, All of this looks pretty good to me. You did a really big effort with this series. Thanks for doing this! On Wed, Feb 22, 2023 at 7:39 PM <arinc9.unal@gmail.com> wrote: > > This is an ambitious effort I've been wanting to do for months. > > Straight off the bat, I'm fixing the ABI that I broke a while back, by > reintroducing the ralink,rt2880-pinmux compatible string. > > If you take a look at the schema for mt7620 and rt305x, some functions got > multiple lists for groups. Like refclk on mt7620. Because mt7620 and > mt7628/mt7688 SoCs use the same compatible string, it's impossible to > differentiate on the binding which SoC a devicetree is actually for. > Therefore, the binding will allow all groups listed for that function. For > example, if the SoC is mt7620, only the refclk function for the mdio group > can be used. If one were to put "spi cs1" as the function there, there > wouldn't be a warning. > > I address this by introducing new compatible strings for these SoCs, then > split the schemas. I also separate mt7628/mt7688 from mt7620 pinctrl > subdriver in the process. > > I wanted to split the rt305x driver too but too much code would be reused > so I backed down from that. > > Ralink was acquired by MediaTek in 2011. These SoCs have been rebranded as > MediaTek. We're moving the Ralink pinctrl driver to MediaTek, and rename > the schemas to mediatek. > > I've renamed the ralink core driver to mtmips. I decided to call the core > mtmips as I've seen folks from MediaTek use the same name when they added > support for MT7621 pinctrl on U-Boot. Feel free to comment on this. > > The MTMIPS pinctrl driver requires rt_sysc_membase from > arch/mips/ralink/of.c, so, for COMPILE_TEST to be useful, RALINK must be > selected. These headers, asm/mach-ralink/ralink_regs.h and > asm/mach-ralink/mt7620.h, from arch/mips/include are also required but > they can easily be included: > > ifeq ($(CONFIG_COMPILE_TEST),y) > CFLAGS_pinctrl-mtmips.o += -I$(srctree)/arch/mips/include > endif > > Sergio, do you see a way to make the pinctrl driver independent of > architecture code? At least avoid using rt_sysc_membase. The only really dependent architecture code in these drivers now is because of the use of 'rt_sysc_r32()' and 'rt_sysc_w32()' in 'ralink_pmx_group_enable()' function [0]. This is just to set the gpio mode. The read and write registers here SYSC_REG_GPIO_MODE and SYSC_REG_GPIO_MODE2 are in the system controller area. In all single ralink platform 'sysc' nodes should be a syscon that can be accessed from the driver side. That way you can just get those syscon areas via regmap APIs and properly read and write desired registers. For the mt7621.dtsi file, the node is already a syscon [1]. Other ralink device tree files should also be modified to include this in its 'sysc' node (I think in openWRT dts files at least for mt7620 is already included). You have to add that in all of them since 'ralink_pmx_group_enable()' is common code for all. I think this can be done in a different patch series. I can help you to do this after this series is reviewed and accepted. > > dtbs_check will print warnings for DTs with the old strings as it will > match multiple bindings. I assume that's acceptable in order to have the > things properly documented without breaking the ABI. The bindings will work > fine with the new compatible strings. > > I could define the checks under $defs:, then refer to it if the compatible > string is the one which would work fine. Or I could put only the new > compatible strings on the documentation. What are your thoughts Krzysztof, > Rob? > > Arınç > > Best regards, Sergio Paracuellos [0]: https://elixir.bootlin.com/linux/v6.2/source/drivers/pinctrl/ralink/pinctrl-ralink.c#L117 [1]: https://elixir.bootlin.com/linux/v6.2/source/arch/mips/boot/dts/ralink/mt7621.dtsi#L62
On 23.02.2023 07:58, Sergio Paracuellos wrote: > Hi Arınç, > > All of this looks pretty good to me. You did a really big effort with > this series. Thanks for doing this! > > On Wed, Feb 22, 2023 at 7:39 PM <arinc9.unal@gmail.com> wrote: >> >> This is an ambitious effort I've been wanting to do for months. >> >> Straight off the bat, I'm fixing the ABI that I broke a while back, by >> reintroducing the ralink,rt2880-pinmux compatible string. >> >> If you take a look at the schema for mt7620 and rt305x, some functions got >> multiple lists for groups. Like refclk on mt7620. Because mt7620 and >> mt7628/mt7688 SoCs use the same compatible string, it's impossible to >> differentiate on the binding which SoC a devicetree is actually for. >> Therefore, the binding will allow all groups listed for that function. For >> example, if the SoC is mt7620, only the refclk function for the mdio group >> can be used. If one were to put "spi cs1" as the function there, there >> wouldn't be a warning. >> >> I address this by introducing new compatible strings for these SoCs, then >> split the schemas. I also separate mt7628/mt7688 from mt7620 pinctrl >> subdriver in the process. >> >> I wanted to split the rt305x driver too but too much code would be reused >> so I backed down from that. >> >> Ralink was acquired by MediaTek in 2011. These SoCs have been rebranded as >> MediaTek. We're moving the Ralink pinctrl driver to MediaTek, and rename >> the schemas to mediatek. >> >> I've renamed the ralink core driver to mtmips. I decided to call the core >> mtmips as I've seen folks from MediaTek use the same name when they added >> support for MT7621 pinctrl on U-Boot. Feel free to comment on this. >> >> The MTMIPS pinctrl driver requires rt_sysc_membase from >> arch/mips/ralink/of.c, so, for COMPILE_TEST to be useful, RALINK must be >> selected. These headers, asm/mach-ralink/ralink_regs.h and >> asm/mach-ralink/mt7620.h, from arch/mips/include are also required but >> they can easily be included: >> >> ifeq ($(CONFIG_COMPILE_TEST),y) >> CFLAGS_pinctrl-mtmips.o += -I$(srctree)/arch/mips/include >> endif >> >> Sergio, do you see a way to make the pinctrl driver independent of >> architecture code? At least avoid using rt_sysc_membase. > > The only really dependent architecture code in these drivers now is > because of the use of > 'rt_sysc_r32()' and 'rt_sysc_w32()' in 'ralink_pmx_group_enable()' > function [0]. This is just to set the gpio mode. The read and write > registers here SYSC_REG_GPIO_MODE and SYSC_REG_GPIO_MODE2 are in the > system controller area. In all single ralink platform 'sysc' nodes > should be a syscon that can be accessed from the driver side. That way > you can just get those syscon areas via regmap APIs and properly read > and write desired registers. For the mt7621.dtsi file, the node is > already a syscon [1]. Other ralink device tree files should also be > modified to include this in its 'sysc' node (I think in openWRT dts > files at least for mt7620 is already included). You have to add that > in all of them since 'ralink_pmx_group_enable()' is common code for > all. I think this can be done in a different patch series. I can help > you to do this after this series is reviewed and accepted. Awesome, thanks a lot! Note to self, "depends on RALINK" menu entries should be changed to "depends on OF" when this happens. Arınç