Re: [PATCH 1/2] dt-bindings: mfd: Add Realtek switch

[Chris Packham]

Hi Krzysztof,

(sorry, re-sending as plain text)

On 10/09/24 19:26, Krzysztof Kozlowski wrote:
> On 09/09/2024 22:36, Chris Packham wrote:
> > Hi Krzysztof,
> >
> > On 9/09/24 18:38, Krzysztof Kozlowski wrote:
> > > On Mon, Sep 09, 2024 at 01:47:06PM +1200, Chris Packham wrote:
> > > > Add device tree schema for the Realtek switch. Currently the only
> > > > supported feature is the syscon-reboot which is needed to be able to
> > > > reboot the board.
> > > >
> > > > Signed-off-by: Chris Packham<chris.packham@xxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > >    .../bindings/mfd/realtek,switch.yaml          | 50 +++++++++++++++++++
> > > >    1 file changed, 50 insertions(+)
> > > >    create mode 100644 Documentation/devicetree/bindings/mfd/realtek,switch.yaml
> > > Use compatible as filename.
> > My hope was eventually that this would support multiple Realtek
> > switches. But sure for now at least I can name it after the one in front
> > of me.
> This might never happen, so unless you document more models now, I
> strongly suggest using compatible as filename.
Agreed.
> > > > diff --git a/Documentation/devicetree/bindings/mfd/realtek,switch.yaml b/Documentation/devicetree/bindings/mfd/realtek,switch.yaml
> > > > new file mode 100644
> > > > index 000000000000..84b57f87bd3a
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mfd/realtek,switch.yaml
> > > > @@ -0,0 +1,50 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id:http://scanmail.trustwave.com/?c=20988&d=s_Tf5n4B2HBkm1hFlKUTA3UKwK2Jg2EPHeQMRCQHXQ&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fmfd%2frealtek%2cswitch%2eyaml%23
> > > > +$schema:http://scanmail.trustwave.com/?c=20988&d=s_Tf5n4B2HBkm1hFlKUTA3UKwK2Jg2EPHbEEFSRQXw&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
> > > > +
> > > > +title: Realtek Switch with Internal CPU
> > > What sort of Switch? Like network switch? Then this should be placed in
> > > respective net (or deeper, e.g. net/dsa/) directory.
> > Yes network switch. But this is one of those all in one chips that has a
> > CPU, network switch and various peripherals. MFD seemed appropriate.
> There is no such device as MFD. MFD is a Linux framework.

What I meant was the RTL9302 is a similar class of device to the 
mscc,ocelot which has a binding in 
Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml. If it's a case 
of that being historical baggage then 
Documentation/devicetree/bindings/mips/ might be appropriate for the SoC 
type properties and Documentation/devicetree/bindings/net/ for the 
switch portion.

> > > Maintainers go here. See example-schema.
> > Ack.
> >
> > > > +
> > > > +description:
> > > > +  The RTL9302 ethernet switch has an internal CPU. The switch is a multi-port
> > > > +  networking switch that supports many interfaces. Additionally, the device can
> > > > +  support MDIO, SPI and I2C busses.
> > > I don't get why syscon node is called switch. This looks incomplete or
> > > you used description from some other device.
> > Yes I did take a lot of inspiration from the mscc,ocelot. I am working
> > on more support for the switch and some of the other peripherals so I
> > figured I'd word it towards that end goal. If you prefer I could word
> > this more towards the one function (reboot) that is supported right now.
> Your commit msg is not explaining here much. And "Currently the only
> supported" feels like a driver description. We expect bindings to be
> complete. It's fine to bring partial description of hardware, but this
> should be explained in the commit msg and entire binding should be still
> created to accommodate that full description.
>
> However such complex devices like Ocelot should be described fully so we
> can easily see how you organize entire binding.

I can definitely do a better job of explaining myself in the commit 
message. Right now I just want to have a working reboot.

I'm not really using the "realtek,rtl9302c-switch" compatible for 
anything but I gather using a "syscon" node without a more specific 
compatible is frowned upon (please tell me if I'm wrong). In terms of 
the binding should I just make the description something terse like "The 
RTL9302 ethernet switch has an internal CPU. Some peripherals are 
accessed via a common register block".

> > > But if this is DSA, then you miss dsa ref and dsa-related properties.
> > So far I'm resisting DSA. The usage of the RTL9300 as a SoC+Switch
> > doesn't really lend itself to the DSA architecture (there is a external
> > CPU mode that would). I think eventually we'd end up with something like
> > the mscc,oscelot where both switchdev and DSA usage is supported. There
> > would be some properties (e.g. port/phy arrangement) that apply to both
> > uses.
> This feels ok, although you really should create complete binding here.

This is a bit of a chicken and egg thing. I don't want to commit to a 
binding until I have more code to back it up, but of course I don't want 
to spend a bunch of time writing code for a binding that then needs to 
change when that binding is reviewed.

> > I have got a (kind of) working proof of concept switchdev driver which
> > has some of the support you've mentioned. It's not really ready so I
> > didn't include the dt-binding for that stuff in this patch.
> Best regards,
> Krzysztof