[RFC,0/2] MIIM regmap and RTL8231 GPIO expander support

Message ID cover.1617914861.git.sander@svanheule.net
Headers show
Series
  • MIIM regmap and RTL8231 GPIO expander support
Related show

Message

Sander Vanheule April 8, 2021, 8:52 p.m.
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

Comments

Andrew Lunn April 8, 2021, 10:18 p.m. | #1
> - 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
Sander Vanheule April 9, 2021, 5:42 a.m. | #2
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
Mark Brown April 9, 2021, 4:07 p.m. | #3
On Thu, Apr 08, 2021 at 10:52:34PM +0200, Sander Vanheule wrote:
> Basic support for MIIM bus access. Support only includes clause-22
> register access, with 5-bit addresses, and 16-bit wide registers.

What is "MIIM"?  A quick search isn't showing up useful hits for that.
Why not just call this MDIO like the rest of the kernel is doing, it
seems like using something else is at best going to make it harder to
discover this code?  If MIIM is some subset or something it's not
obvious how we're limited to that.
Sander Vanheule April 9, 2021, 6:14 p.m. | #4
Hi Mark,

On Fri, 2021-04-09 at 17:07 +0100, Mark Brown wrote:
> On Thu, Apr 08, 2021 at 10:52:34PM +0200, Sander Vanheule wrote:
> > Basic support for MIIM bus access. Support only includes clause-22
> > register access, with 5-bit addresses, and 16-bit wide registers.
> 
> What is "MIIM"?  A quick search isn't showing up useful hits for that.
> Why not just call this MDIO like the rest of the kernel is doing, it
> seems like using something else is at best going to make it harder to
> discover this code?  If MIIM is some subset or something it's not
> obvious how we're limited to that.

MIIM stands for "MII management", i.e. the management bus for devices
with some form of MII interface. MDIO is also frequently used to refer
to the data pin of the bus (there's also MDC: the clock pin), so I
wanted to make the distinction.

The kernel has the mii_bus struct to describe the bus master, but like
you noted the bus is generaly refered to as an MDIO interface. I'm fine
with naming it MDIO to make it easier to spot.

Best,
Sander
Andrew Lunn April 9, 2021, 8:10 p.m. | #5
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