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
