[RFC PATCH 0/7] realtek: MFD for switch core

[Sander Vanheule]

Hi Birger,

In the future, could you please CC me when replying to my patches?

On Sat, 2022-07-16 at 23:09 +0200, Birger Koblitz wrote:
> Hi,
> 
> On 7/16/22 21:31, Sander Vanheule wrote:
> > On Sat, 2022-07-16 at 21:09 +0200, Sander Vanheule wrote:
> > > This RFC series introduces a new MFD device for the switch core found
> > > in the Realtek SoCs. Currently only an implementation is provided for
> > > RTL8380, but it written with the register structure of other generations
> > > in mind.
> this looks very promising as it offers the pin control we always needed.
> 
> I have not looked through the code in detail, yet. The biggest question 
> I have at this point is that at this point the code does not include the 
> other SoC generations. 

It doesn't because most users are currently on RTL8380, so that's where I can
get some feedback from real use cases. I usually run an initramfs image loaded
via u-boot, so it's quite possible some setup is still missing.

> Especially when it comes to the LEDs, they are 
> very different. In fact the RTL838x is the odd-man-out. I would really 
> like to see how they will be included. In the past several design 
> decision turned out to be not so optimal after we learned about the 
> newer SoCs. Today we have all of them very well understood, so it should 
> not be an issue to add at least some code for the RTL93xx generation, 
> which has the LEDs modernized quite a bit.

Although I only wrote the RTL8380 implementation, the code is already structured
to follow the RTL8390 and later. Aside from correctly defining most pin muxes, I
was able to add port LED and pinctrl support for RTL8390 this morning in about
4h. I wouldn't say my code was that badly suited to those chips then.

This is still an RFC, and if it goes upstream it needs to go through staging.
Staging should allow for things to evolve more freely. You're right that is
isn't a finished product, but if we don't push this out early to get feedback,
it's never going to be.

> 
> The other question I have is whether this allows us to solve the other 
> big issue we have with these SoCs: Providing information between 
> drivers.

Thats why this is an MFD. IIUC these can provide extra data to child devices, so
that could solve that issue (if needed). Generally speaking, this is something
you want to avoid however IMHO. In most cases, a SoC-generation specific DT
compatible already provides sufficient info.

One potential issue that comes to mind, is that the regmap is currently globally
locked on any access. I can imagine this will casue performace issues for the
ethernet driver. Those registers are mostly independent from the rest of the
register space though, so custom regmap locking will probably need to be used.

> For example you have written the 2nd copy of the model name 
> reading function in rtl8380_probe_model_name(), and it does not even 
> probe the other 3 SoC generations.

See above, this isn't intended to be a complete implementation. And it's really
there just to print the SoC name so the user knows which platform they are on,
nothing else.

> But this information is needed 
> already at the very start of the boot process and several other drivers.
> So why not provide a global structure like on other MIPS architectures 
> and populate it with information such as machine name and especially 
> switch structure for other drivers to use plus all the other 
> configuration details of a particular SoC (there are nearly 2 dozen 
> different RTL9300 SoCs all having a different structure for the MII 
> ports). In a very simple example, NOR access needs information about the 
> 3/4 byte strapping pin from the switch core, but its registers live at a 
> completely different place in the SoC. The lack of such global 
> information is evident from e.g. your Netgear .dts.

I could be wrong, but providing global structures seems to be a thing of the
past. I think device tree is the way to go wherever possible. If needed, the
SPI-peripheral could get a reference to the switchcore (syscon) node, to be able
to also access the regmap and read any required info by itself.

> Although you know 
> that the first used port is 8, from the fact its an RTL8382M SoC or 
> alternatively the ports in the .dts, you need to add another time the 
> information about port to LED number.

Is there another way to know which internal switch port an LED happens to be
connected to? We don't set up an LED-to-PHY interconnect matrix, it's all
static. Yes, the device tree spec is verbose, but that's just the way it is.
Every element needs to be specified and all that's really required is the 'reg'
property. That's _one_ line per LED node. Only referencing a specific phy
instead wouldn't work, because then the LED index info is still missing.

The way the port LEDs are currently defined in the DT in my RFC, allows one to
reference one of the port LEDs just like any other DT LED; for example as the
"boot_led" in OpenWrt. This is useful for the Cisco SG220-series, which uses one
of the port LEDs as system status LED.

> 
> Another point I have is more general: The pinctrl driver is a lot of 
> code for controlling the LEDs, which are not even pins on the SoC, but 
> controlled through a serial bus. Is this really the right type of 
> driver?

The pinctrl driver doesn't do anything with the port LEDs. My patches got mixed
up though, but I'll correct that in v2.

>  Many of the LED drivers in Linux work similarly and provide a 
> standard interface to users.

My driver produces a node in /sys/class/leds for every defined port LED; that's
about as standard as it gets. The only non-standard thing is HW offloading of
the port status, but there is no existing framework for that. With these
patches, a user can switch port LEDs between different modes at will, during
runtime.

>  Also the LEDs are actually controlled by 
> the RTL8231 via a serial link on many models, not the Switch Core. 
> Placing the LED control into the switch core is a bit confusing, should 
> there not at least be a node for the RTL8231 in the .dts reflecting the 
> actual HW setup? Why not a small setup function for automatic HW control 
> of all LEDs as already done for the other SoC generations and a 
> dedicated LED driver. This might reduce code complexity and reflect the 
> actual hardware setup.

If you find the location of the LED support inside the switch core confusing,
you should go complain to Realtek. That's where it is, that's how we have to
deal with it.

The port LED peripheral manages how many bits are shifted out or written to a
bus, so that peripheral is what needs to be managed. It doesn't matter that the
external hardware sometimes happens to be an RTL8231 (in shift register mode),
if we don't need to manage it. For all I care, the hardware driving the LEDs
could be a daughter board full of discrete transistors, we would still need to
control the LED peripheral in the same way.

Best,
Sander