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

[Birger Koblitz]

Hi Sander,

On 7/17/22 15:37, Sander Vanheule wrote:
> 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.
I don't see that you actually do the hardware setup for the LEDs, or am 
I missing something? You rely on u-boot or the existing led_init() 
functions (e.g. 
https://github.com/openwrt/openwrt/commit/3190361ace26cc63fe64a166067c7c543d568337) 
to configure the HW offload settings, to setup the LED topology and 
attributing it to a LED-set and define what port type it is (fibre, 
ethernet, combo,...), right? For that one needs to have access to the 
PHY-information. Would you add that into the LED driver, or should that 
be a property of the switch driver?
On the RTL93xx devices that correctly initializes the LEDs in u-boot are 
an exception, that was BTW the reason I pointed to the other SoCs.

> 
> 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.
I don't really see the hurry. There are different design options for 
this, I would like to understand implications, first. At least we are 
now in a position to understand all SoC generations and can take that 
into account.

> 
>>
>> 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.
We should be sure this will work. Ethernet performance is not fantastic 
as it is, further degradation would be really bad. The driver should 
also anticipate more of the possible offloading, e.g. not copying over 
TX buffers as MIPS can do DMA from anywhere.

> 
>> 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.
I had a discussion with Sebastian and Daniel on this at some point. We 
came to the conclusion that a global platform structure would be the 
best choice. Note that this was more a way of providing global locking 
and MII topology, which is needed in several drivers for MDIO busses, 
including locking of the polling, probably table space coordination (at 
the moment everything is in the switch driver, but routing offload might 
want to live in a separate driver). The discussion was not so much about 
providing a regmap for the wast and chaotic switch-core.

> 
>> 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.
I am willing to buy that, I hope that will also fly upstream.

> 
> 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.
Since you are not actually doing the LED hardware topology setup (I 
believe) it is not really visible, anyway. But I believe that should be 
taken into account, because frankly that existing code should be taken 
into account, as without it several devices will not work.

> 
> Best,
> Sander
> 

Cheers,
   Birger