Message-ID: <c7d4f87b-f3d4-4853-9960-e7faa560ce34@alliedtelesis.co.nz>
Date: Wed, 11 Sep 2024 08:48:44 +1200
From: Chris Packham <chris.packham@...iedtelesis.co.nz>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: lee@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 tsbogend@...ha.franken.de, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-mips@...r.kernel.org
Subject: Re: [PATCH 1/2] dt-bindings: mfd: Add Realtek switch

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@...iedtelesis.co.nz>
>>>> ---
>>>>    .../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
>

Powered by blists - more mailing lists