| Message ID | cover.1617914861.git.sander@svanheule.net |
|---|---|
| Headers | show |
| Series | MIIM regmap and RTL8231 GPIO expander support | expand |
> - Providing no compatible for an MDIO child node is considered to be equivalent > to a c22 ethernet phy, so one must be provided. However, this node is then > not automatically probed. It cannot be automatically probed, since register 2 and 3 do not contain an ID, which PHYs do. So you need to explicitly list in on the MDIO bus, and when the of_mdiobus_register() is called, the device will be instantiated. Is it okay to provide a binding without a driver? > If some code is required, where should this be put? > Current devicetree structure: > mdio-bus { > compatible = "vendor,mdio"; > ... > > expander0: expander@0 { > /* > * Provide compatible for working registration of mdio device. > * Device probing happens in gpio1 node. > */ > compatible = "realtek,rtl8231-expander"; > reg = <0>; > }; > > }; > gpio1 : ext_gpio { > compatible = "realtek,rtl8231-mdio"; > gpio-controller; > ... > }; I don't understand this split. Why not mdio-bus { compatible = "vendor,mdio"; ... expander0: expander@0 { /* * Provide compatible for working registration of mdio device. * Device probing happens in gpio1 node. */ compatible = "realtek,rtl8231-expander"; reg = <0>; gpio-controller; }; }; You can list whatever properties you need in the node. Ethernet switches have interrupt-controller, embedded MDIO busses with PHYs on them etc. > - MFD driver: > The RTL8231 is not just a GPIO expander, but also a pin controller and LED > matrix controller. Regmap initialisation could probably be moved to a parent > MFD, with gpio, led, and pinctrl cells. Is this a hard requirement if only a > GPIO controller is provided? You need to think about forward/backwards compatibility. You are defining a binding now, which you need to keep. Do you see how an MFD could be added without breaking backwards compatibility? Andrew
Hi Andrew, Thank you for the feedback. You can find a (leaked) datasheet at: https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_1.2.pdf On Fri, 2021-04-09 at 00:18 +0200, Andrew Lunn wrote: > > - Providing no compatible for an MDIO child node is considered to > > be equivalent > > to a c22 ethernet phy, so one must be provided. However, this > > node is then > > not automatically probed. > > It cannot be automatically probed, since register 2 and 3 do not > contain an ID, which PHYs do. So you need to explicitly list in on > the > MDIO bus, and when the of_mdiobus_register() is called, the device > will be instantiated. > > Is it okay to provide a binding without a driver? > > If some code is required, where should this be put? > > Current devicetree structure: > > mdio-bus { > > compatible = "vendor,mdio"; > > ... > > > > expander0: expander@0 { > > /* > > * Provide compatible for working registration of mdio > > device. > > * Device probing happens in gpio1 node. > > */ > > compatible = "realtek,rtl8231-expander"; > > reg = <0>; > > }; > > > > }; > > gpio1 : ext_gpio { > > compatible = "realtek,rtl8231-mdio"; > > gpio-controller; > > ... > > }; > > I don't understand this split. Why not > > mdio-bus { > compatible = "vendor,mdio"; > ... > > expander0: expander@0 { > /* > * Provide compatible for working registration of mdio > device. > * Device probing happens in gpio1 node. > */ > compatible = "realtek,rtl8231-expander"; > reg = <0>; > gpio-controller; > }; > }; > > You can list whatever properties you need in the node. Ethernet > switches have interrupt-controller, embedded MDIO busses with PHYs on > them etc. This is what I tried initially, but it doesn't seem to work. The node is probably still added as an MDIO device, but rtl8231_gpio_probe() doesn't appear to get called at all. I do agree it would be preferable over the split specification. Having another look, I see mdio_device_id is used for ethernet phys, but like you said this requires and ID in registers 2 & 3. These registers contain pin configuration on the RTL8231, so this can't be used. Registering as a phy_driver appears to have the same issue, although it looks like I could use a custom match_phy_device(). I do feel like this would be stretching the meaning of what a PHY is. > > - MFD driver: > > The RTL8231 is not just a GPIO expander, but also a pin > > controller and LED > > matrix controller. Regmap initialisation could probably be moved > > to a parent > > MFD, with gpio, led, and pinctrl cells. Is this a hard > > requirement if only a > > GPIO controller is provided? > > You need to think about forward/backwards compatibility. You are > defining a binding now, which you need to keep. Do you see how an MFD > could be added without breaking backwards compatibility? There are pin-/gpio-controllers that have the gpio and pinctrl nodes in the device's root node. So I think adding pinctrl later shouldn't be an issue. The LED matrix description would probably need a dedicated sub- node. I'll see if I can write some preliminary bindings later today or this weekend. Best, Sander
On Fri, Apr 09, 2021 at 07:42:32AM +0200, Sander Vanheule wrote: > Hi Andrew, > > Thank you for the feedback. You can find a (leaked) datasheet at: > https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_1.2.pdf So this is not really an MFD. It has different ways of making use of pins, which could be used for GPIO, but can also be used for LEDs. You could look if it better fits in drivers/leds. But you can also use GPIO drivers for LEDs via led-gpio. > > I don't understand this split. Why not > > > > mdio-bus { > > compatible = "vendor,mdio"; > > ... > > > > expander0: expander@0 { > > /* > > * Provide compatible for working registration of mdio > > device. > > * Device probing happens in gpio1 node. > > */ > > compatible = "realtek,rtl8231-expander"; > > reg = <0>; > > gpio-controller; > > }; > > }; > > > > You can list whatever properties you need in the node. Ethernet > > switches have interrupt-controller, embedded MDIO busses with PHYs on > > them etc. > > This is what I tried initially, but it doesn't seem to work. The node > is probably still added as an MDIO device, but rtl8231_gpio_probe() > doesn't appear to get called at all. I do agree it would be preferable > over the split specification. Look at drivers/net/dsa/mv88e6xxx/chip.c for how to register an mdio driver. If you still cannot get it to work, post your code and i will take a look. Andrew
Hi Andrew, On Fri, 2021-04-09 at 22:10 +0200, Andrew Lunn wrote: > On Fri, Apr 09, 2021 at 07:42:32AM +0200, Sander Vanheule wrote: > > Hi Andrew, > > > > Thank you for the feedback. You can find a (leaked) datasheet at: > > https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_1.2.pdf > > So this is not really an MFD. It has different ways of making use of > pins, which could be used for GPIO, but can also be used for LEDs. You > could look if it better fits in drivers/leds. But you can also use > GPIO drivers for LEDs via led-gpio. The chip provides LED scanning matrix functionality, for which one needs to set up row and column pins. The chip supports 3×12 + 3×12 + 2×8 (88) LEDs; a lot more than it has pins available. There is also (limited) support for hardware-accelerated blinking. For example, single LED color scan matrix ("group A" for ports 0-11) would be wired up as follows: Row and column pins of scan matrix for LED0, LED1, LED2 Columns control LED0/1/2 for ports [n] and [n+6] L0[n] L1[n] L2[n] L0[n+6] L1[n+6] L2[n+6] | | | | | | P0/P6 --X--------X--------X--------X--------X--------X (3) | | | | | | P1/P7 --X--------X--------X--------X--------X--------X (4) | | | | | | P2/P8 --X--------X--------X--------X--------X--------X (5) | | | | | | P3/P9 --X--------X--------X--------X--------X--------X (6) | | | | | | P4/P10 --X--------X--------X--------X--------X--------X (7) | | | | | | P5/P11 --X--------X--------X--------X--------X--------X (8) (0) (1) (2) (9) (10) (11) So far, I haven't seen any actual hardware implementation that uses the scanning matrix functionality (or the buzzer control feature with frequency selection). 1:1 use of GPIO pins for LEDs is indeed trivial with led-gpio, and I am currently using this on one of my devices. > > > I don't understand this split. Why not > > > > > > mdio-bus { > > > compatible = "vendor,mdio"; > > > ... > > > > > > expander0: expander@0 { > > > /* > > > * Provide compatible for working registration of mdio > > > device. > > > * Device probing happens in gpio1 node. > > > */ > > > compatible = "realtek,rtl8231-expander"; > > > reg = <0>; > > > gpio-controller; > > > }; > > > }; > > > > > > You can list whatever properties you need in the node. Ethernet > > > switches have interrupt-controller, embedded MDIO busses with PHYs on > > > them etc. > > > > This is what I tried initially, but it doesn't seem to work. The node > > is probably still added as an MDIO device, but rtl8231_gpio_probe() > > doesn't appear to get called at all. I do agree it would be preferable > > over the split specification. > > Look at drivers/net/dsa/mv88e6xxx/chip.c for how to register an mdio > driver. If you still cannot get it to work, post your code and i will > take a look. Thanks for the suggestion. I've managed to create a cleaner mdio_device driver with a single corresponding DT node. Would the following make sense for a more complete DT description? Or would I need sub-nodes to group e.g. the pin control or LED nodes/properties? expander@31 { /* Either "realtek,rtl8231-mdio" or "realtek,rtl8231-smi" */ compatible = "realtek,rtl8231-mdio"; reg = <31>; /* Must be <1> (8 bit) or <2> (16 bit); only for "realtek,rtl8231-smi" */ realtek,smi-regnum-width = <1>; /** GPIO controller properties **/ gpio-controller; #gpio-cells = <2>; ngpios = <37>; poe_enable { gpio-hog; gpios = <10 0>; output-high; }; /** Pin controller properties **/ /* Can only set a global drive strength, 4mA or 8mA */ realtek,gpio-drive-strength = <4>; /* Global LED scan matrix setting, 0 (single-color) or 1 (bi-color) */ realtek,led-color-scan-mode = <0>; pinctrl-names = "default"; pinctrl-0 = <&user_button>, <&port_leds>; user_button : user_button_cfg { pins = "gpio31"; function = "gpio"; /* Only GPIOs 31-35 can do hardware debouncing */ /* Debouncing is either disabled or 100ms */ input-debounce = <100000>; }; port_leds : port_leds_cfg { /* Select two columns (LED colors) for switch ports 0-7 */ pins = "gpio0", "gpio1", "gpio9", "gpio10", "gpio3", "gpio4", "gpio5", "gpio6"; function = "led"; }; /** LED config **/ #address-cells = <2>; #size-cells = <0>; led@0.0 { /* LED0 for port 0, corresponds to bits [2:0] in regnum 0x09 */ reg = <0 0>; ... }; led@0.1 { /* LED1 for port 0 */ reg = <0 1>; ... }; /* LEDs 1.x, 2.x, 3.x, 6.x, 7.x, 8.x, 9.x omitted */ }; As a final remark, I have found out that this chip doesn't actually talk I2C, but rather Realtek's proprietary SMI. These two protocols are very similar w.r.t. to byte framing, but SMI requires the bus master to write the register number byte(s) on both READ and WRITE frames. I2C/SMBUS does a WRITE for the register number first, then a separate READ for the register value. This means I can't get regmap_i2c to work. There is an existing, bit-banged implementation of this SMI protocol (see "realtek,smi-mdio", realtek- smi.c). If I could re-use this in some way, there would only need to be an MDIO implementation. However, we have noticed that the larger phy address space (7-bit in SMI vs. 5-bit in MDIO) did require a patch to make it work on ethernet switches with more than 32 ports (and corresponding phys) on a single SMI bus. Best, Sander
The RTL8231 GPIO and LED expander can be configured for use as an MIIM or I2C bus device. To provide uniform data access between these two modes, the regmap interface is used. Since no regmap interface exists for MIIM busses, a basic implementation is provided. Currently outstanding questions: - The REGMAP_MIIM symbol should depend on MDIO_DEVICE (or a better suited, related symbol), but this results in circular Kconfig dependencies: drivers/of/Kconfig:69:error: recursive dependency detected! drivers/of/Kconfig:69: symbol OF_IRQ depends on IRQ_DOMAIN kernel/irq/Kconfig:59: symbol IRQ_DOMAIN is selected by REGMAP drivers/base/regmap/Kconfig:7: symbol REGMAP default is visible depending on REGMAP_MIIM drivers/base/regmap/Kconfig:39: symbol REGMAP_MIIM depends on MDIO_DEVICE drivers/net/mdio/Kconfig:6: symbol MDIO_DEVICE is selected by PHYLIB drivers/net/phy/Kconfig:16: symbol PHYLIB is selected by ARC_EMAC_CORE drivers/net/ethernet/arc/Kconfig:19: symbol ARC_EMAC_CORE is selected by ARC_EMAC drivers/net/ethernet/arc/Kconfig:25: symbol ARC_EMAC depends on OF_IRQ Suggestions on how to resolve this are welcome. - Providing no compatible for an MDIO child node is considered to be equivalent to a c22 ethernet phy, so one must be provided. However, this node is then not automatically probed. Is it okay to provide a binding without a driver? If some code is required, where should this be put? Current devicetree structure: mdio-bus { compatible = "vendor,mdio"; ... expander0: expander@0 { /* * Provide compatible for working registration of mdio device. * Device probing happens in gpio1 node. */ compatible = "realtek,rtl8231-expander"; reg = <0>; }; }; gpio1 : ext_gpio { compatible = "realtek,rtl8231-mdio"; gpio-controller; ... }; - MFD driver: The RTL8231 is not just a GPIO expander, but also a pin controller and LED matrix controller. Regmap initialisation could probably be moved to a parent MFD, with gpio, led, and pinctrl cells. Is this a hard requirement if only a GPIO controller is provided? Sander Vanheule (2): regmap: add miim bus support gpio: Add Realtek RTL8231 support drivers/base/regmap/Kconfig | 6 +- drivers/base/regmap/Makefile | 1 + drivers/base/regmap/regmap-miim.c | 58 +++++ drivers/gpio/Kconfig | 9 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-rtl8231.c | 404 ++++++++++++++++++++++++++++++ include/linux/regmap.h | 36 +++ 7 files changed, 514 insertions(+), 1 deletion(-) create mode 100644 drivers/base/regmap/regmap-miim.c create mode 100644 drivers/gpio/gpio-rtl8231.c