From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Andrew Lunn <andrew@lunn.ch>,
Florian Fainelli <f.fainelli@gmail.com>,
Lee Jones <lee@kernel.org>,
Colin Foster <colin.foster@in-advantage.com>,
Alexandre Belloni <alexandre.belloni@bootlin.com>,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org
Subject: Advice on MFD-style probing of DSA switch SoCs
Date: Thu, 22 Dec 2022 15:48:44 +0200 [thread overview]
Message-ID: <20221222134844.lbzyx5hz7z5n763n@skbuf> (raw)
Hi,
Many DSA switches (drivers/net/dsa/) are in fact complex SoCs with more
embedded peripherals than just the Ethernet switching IP. For example,
I was trying to add interrupt support for the internal PHYs of the NXP
SJA1110 switch. For context, one SJA1110 switch as seen in fsl-lx2160a-bluebox3.dts
currently has bindings which look like this:
sw2: ethernet-switch@2 {
compatible = "nxp,sja1110a";
reg = <2>;
spi-max-frequency = <4000000>;
spi-cpol;
dsa,member = <0 1>;
ethernet-ports {
#address-cells = <1>;
#size-cells = <0>;
/* Microcontroller port */
port@0 {
reg = <0>;
status = "disabled";
};
sw2p1: port@1 {
reg = <1>;
link = <&sw1p4>;
phy-mode = "sgmii";
fixed-link {
speed = <1000>;
full-duplex;
};
};
port@2 {
reg = <2>;
ethernet = <&dpmac18>;
phy-mode = "rgmii-id";
rx-internal-delay-ps = <2000>;
tx-internal-delay-ps = <2000>;
fixed-link {
speed = <1000>;
full-duplex;
};
};
port@3 {
reg = <3>;
label = "1ge_p2";
phy-mode = "rgmii-id";
phy-handle = <&sw2_mii3_phy>;
};
port@4 {
reg = <4>;
label = "to_sw3";
phy-mode = "2500base-x";
fixed-link {
speed = <2500>;
full-duplex;
};
};
port@5 {
reg = <5>;
label = "trx7";
phy-mode = "internal";
phy-handle = <&sw2_port5_base_t1_phy>;
};
port@6 {
reg = <6>;
label = "trx8";
phy-mode = "internal";
phy-handle = <&sw2_port6_base_t1_phy>;
};
port@7 {
reg = <7>;
label = "trx9";
phy-mode = "internal";
phy-handle = <&sw2_port7_base_t1_phy>;
};
port@8 {
reg = <8>;
label = "trx10";
phy-mode = "internal";
phy-handle = <&sw2_port8_base_t1_phy>;
};
port@9 {
reg = <9>;
label = "trx11";
phy-mode = "internal";
phy-handle = <&sw2_port9_base_t1_phy>;
};
port@a {
reg = <10>;
label = "trx12";
phy-mode = "internal";
phy-handle = <&sw2_port10_base_t1_phy>;
};
};
mdios {
#address-cells = <1>;
#size-cells = <0>;
mdio@0 {
compatible = "nxp,sja1110-base-t1-mdio";
#address-cells = <1>;
#size-cells = <0>;
reg = <0>;
sw2_port5_base_t1_phy: ethernet-phy@1 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x1>;
};
sw2_port6_base_t1_phy: ethernet-phy@2 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x2>;
};
sw2_port7_base_t1_phy: ethernet-phy@3 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x3>;
};
sw2_port8_base_t1_phy: ethernet-phy@4 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x4>;
};
sw2_port9_base_t1_phy: ethernet-phy@5 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x5>;
};
sw2_port10_base_t1_phy: ethernet-phy@6 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x6>;
};
};
};
};
To add interrupts in a naive way, similar to how other DSA drivers have
done it, it would have to be done like this:
#include <dt-bindings/interrupt-controller/nxp-sja1110-acu-slir.h>
sw2: ethernet-switch@2 {
compatible = "nxp,sja1110a";
reg = <2>;
spi-max-frequency = <4000000>;
spi-cpol;
dsa,member = <0 1>;
slir2: interrupt-controller {
compatible = "nxp,sja1110-acu-slir";
interrupt-controller;
#interrupt-cells = <1>;
interrupt-parent = <&gpio 10>;
};
ethernet-ports {
#address-cells = <1>;
#size-cells = <0>;
/* Microcontroller port */
port@0 {
reg = <0>;
status = "disabled";
};
sw2p1: port@1 {
reg = <1>;
link = <&sw1p4>;
phy-mode = "sgmii";
fixed-link {
speed = <1000>;
full-duplex;
};
};
port@2 {
reg = <2>;
ethernet = <&dpmac18>;
phy-mode = "rgmii-id";
rx-internal-delay-ps = <2000>;
tx-internal-delay-ps = <2000>;
fixed-link {
speed = <1000>;
full-duplex;
};
};
port@3 {
reg = <3>;
label = "1ge_p2";
phy-mode = "rgmii-id";
phy-handle = <&sw2_mii3_phy>;
};
port@4 {
reg = <4>;
label = "to_sw3";
phy-mode = "2500base-x";
fixed-link {
speed = <2500>;
full-duplex;
};
};
port@5 {
reg = <5>;
label = "trx7";
phy-mode = "internal";
phy-handle = <&sw2_port5_base_t1_phy>;
};
port@6 {
reg = <6>;
label = "trx8";
phy-mode = "internal";
phy-handle = <&sw2_port6_base_t1_phy>;
};
port@7 {
reg = <7>;
label = "trx9";
phy-mode = "internal";
phy-handle = <&sw2_port7_base_t1_phy>;
};
port@8 {
reg = <8>;
label = "trx10";
phy-mode = "internal";
phy-handle = <&sw2_port8_base_t1_phy>;
};
port@9 {
reg = <9>;
label = "trx11";
phy-mode = "internal";
phy-handle = <&sw2_port9_base_t1_phy>;
};
port@a {
reg = <10>;
label = "trx12";
phy-mode = "internal";
phy-handle = <&sw2_port10_base_t1_phy>;
};
};
mdios {
#address-cells = <1>;
#size-cells = <0>;
mdio@0 {
compatible = "nxp,sja1110-base-t1-mdio";
#address-cells = <1>;
#size-cells = <0>;
reg = <0>;
sw2_port5_base_t1_phy: ethernet-phy@1 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x1>;
interrupts-extended = <&slir2 SJA1110_IRQ_CBT1_PHY1>;
};
sw2_port6_base_t1_phy: ethernet-phy@2 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x2>;
interrupts-extended = <&slir2 SJA1110_IRQ_CBT1_PHY2>;
};
sw2_port7_base_t1_phy: ethernet-phy@3 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x3>;
interrupts-extended = <&slir2 SJA1110_IRQ_CBT1_PHY3>;
};
sw2_port8_base_t1_phy: ethernet-phy@4 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x4>;
interrupts-extended = <&slir2 SJA1110_IRQ_CBT1_PHY4>;
};
sw2_port9_base_t1_phy: ethernet-phy@5 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x5>;
interrupts-extended = <&slir2 SJA1110_IRQ_CBT1_PHY5>;
};
sw2_port10_base_t1_phy: ethernet-phy@6 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x6>;
interrupts-extended = <&slir2 SJA1110_IRQ_CBT1_PHY6>;
};
};
};
};
However, the irq_domain/irqchip handling code in this case will go to
drivers/net/dsa/, and it won't really be a "driver" (there is no struct
device of its own). I don't really like that, the drivers/net/dsa/
folder should ideally contain only drivers for the switching IP.
It doesn't scale to find here code for GPIO, interrupts, MDIO, hwmon and
what have you; not only because the folder gets bloated with irrelevant
stuff, but also because DSA maintainers are not the best reviewers of
drivers which really belong to (and make use of infrastructure from)
other subsystems.
Not only that, but "interrupt-controller" cannot even have a unit
address with these bindings, because it's on the same level as
"ethernet-ports" which requires not having it. But there are multiple
interrupt controllers in the SJA1110 block (I could count 3). Not clear
how the other 3 would be defined in the device tree in this format.
The logical continuation of the existing bindings would be to do what
was done for the multiple MDIO controllers: an "interrupt-controllers"
container node, with children with #address-cells = <1>.
I think that doesn't scale very well either, so I was looking into
transitioning the sja1105 bindings to something similar to what Colin
Foster has done with vsc7512 (ocelot). For this switch, new-style
bindings would look like this:
soc@2 {
compatible = "nxp,sja1110-soc";
reg = <2>;
spi-max-frequency = <4000000>;
spi-cpol;
#address-cells = <1>;
#size-cells = <1>;
sw2: ethernet-switch@0 {
compatible = "nxp,sja1110a";
reg = <0x000000 0x400000>;
resets = <&sw2_rgu SJA1110_RGU_ETHSW_RST>;
dsa,member = <0 1>;
ethernet-ports {
#address-cells = <1>;
#size-cells = <0>;
/* Microcontroller port */
port@0 {
reg = <0>;
status = "disabled";
};
sw2p1: port@1 {
reg = <1>;
link = <&sw1p4>;
phy-mode = "sgmii";
fixed-link {
speed = <1000>;
full-duplex;
};
};
port@2 {
reg = <2>;
ethernet = <&dpmac18>;
phy-mode = "rgmii-id";
rx-internal-delay-ps = <2000>;
tx-internal-delay-ps = <2000>;
fixed-link {
speed = <1000>;
full-duplex;
};
};
port@3 {
reg = <3>;
label = "1ge_p2";
phy-mode = "rgmii-id";
phy-handle = <&sw2_mii3_phy>;
};
port@4 {
reg = <4>;
label = "to_sw3";
phy-mode = "2500base-x";
fixed-link {
speed = <2500>;
full-duplex;
};
};
port@5 {
reg = <5>;
label = "trx7";
phy-mode = "internal";
phy-handle = <&sw2_port5_base_t1_phy>;
};
port@6 {
reg = <6>;
label = "trx8";
phy-mode = "internal";
phy-handle = <&sw2_port6_base_t1_phy>;
};
port@7 {
reg = <7>;
label = "trx9";
phy-mode = "internal";
phy-handle = <&sw2_port7_base_t1_phy>;
};
port@8 {
reg = <8>;
label = "trx10";
phy-mode = "internal";
phy-handle = <&sw2_port8_base_t1_phy>;
};
port@9 {
reg = <9>;
label = "trx11";
phy-mode = "internal";
phy-handle = <&sw2_port9_base_t1_phy>;
};
port@a {
reg = <10>;
label = "trx12";
phy-mode = "internal";
phy-handle = <&sw2_port10_base_t1_phy>;
};
};
};
mdio@704000 {
compatible = "nxp,sja1110-base-t1-mdio";
#address-cells = <1>;
#size-cells = <0>;
reg = <0x704000 0x1000>;
sw2_port5_base_t1_phy: ethernet-phy@1 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x1>;
interrupts-extended = <&slir2 SJA1110_IRQ_CBT1_PHY1>;
};
sw2_port6_base_t1_phy: ethernet-phy@2 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x2>;
interrupts-extended = <&slir2 SJA1110_IRQ_CBT1_PHY2>;
};
sw2_port7_base_t1_phy: ethernet-phy@3 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x3>;
interrupts-extended = <&slir2 SJA1110_IRQ_CBT1_PHY3>;
};
sw2_port8_base_t1_phy: ethernet-phy@4 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x4>;
interrupts-extended = <&slir2 SJA1110_IRQ_CBT1_PHY4>;
};
sw2_port9_base_t1_phy: ethernet-phy@5 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x5>;
interrupts-extended = <&slir2 SJA1110_IRQ_CBT1_PHY5>;
};
sw2_port10_base_t1_phy: ethernet-phy@6 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x6>;
interrupts-extended = <&slir2 SJA1110_IRQ_CBT1_PHY6>;
};
};
mdio@709000 {
compatible = "nxp,sja1110-base-tx-mdio";
#address-cells = <1>;
#size-cells = <0>;
reg = <0x709000 0x1000>;
};
slir2: interrupt-controller@711fe0 {
compatible = "nxp,sja1110-acu-slir";
reg = <0x711fe0 0x10>;
interrupt-controller;
#interrupt-cells = <1>;
interrupt-parent = <&gpio 10>;
};
gpio@712000 {
compatible = "nxp,sja1110-gpio";
reg = <0x712000 0x1000>;
gpio-controller;
};
slir2_gpio: interrupt-controller@712fe0 {
compatible = "nxp,sja1110-gpio-slir";
reg = <0x712fe0 0x10>;
interrupt-controller;
#interrupt-cells = <1>;
};
sw2_rgu: reset@718000 {
compatible = "nxp,sja1110-rgu";
reg = <0x718000 0x1000>;
#reset-cells = <1>;
};
sw2_cgu: clock-controller@719000 {
compatible = "nxp,sja1110-cgu";
reg = <0x719000 0x1000>;
#clock-cells = <1>;
};
slir2_pmu: interrupt-controller@71afe0 {
compatible = "nxp,sja1110-pmu-slir";
reg = <0x71afe0 0x10>;
interrupt-controller;
#interrupt-cells = <1>;
};
};
In this model, the Ethernet switch is just one of the children of the
SoC driver (the one owning the spi_device, and creating regmaps for its
children). The DSA driver doesn't have to concern itself with the other
drivers, which can live in their own folders.
It seems like mfd is a tool which could help with the driver for
"nxp,sja1110-soc", but mfd_add_devices() requires an array of struct
mfd_cell, and I don't really like that. I want a driver which just
creates IORESOURCE_REG arrays of resources for each child OF node
according to their "reg" properties, creates regmaps for each resource
using a regmap_config that I give it, associates the regmaps with the
parent using devres, and allocates/probes a platform device driver for
each of these child OF nodes. I don't want to spell out a different
mfd_cell for each driver that I'd like the SoC to probe.
It looks like of_platform_populate() would be an alternative option for
this task, but that doesn't live up to the task either. It will assume
that the addresses of the SoC children are in the CPU's address space
(IORESOURCE_MEM), and attempt to translate them. It simply doesn't have
the concept of IORESOURCE_REG. The MFD drivers which call
of_platform_populate() (simple-mfd-i2c.c) simply don't have unit
addresses for their children, and this is why address translation isn't
a problem for them.
In fact, this seems to be a rather large limitation of include/linux/of_address.h.
Even something as simple as of_address_count() will end up trying to
translate the address into the CPU memory space, so not even open-coding
the resource creation in the SoC driver is as simple as it appears.
Is there a better way than completely open-coding the parsing of the OF
addresses when turning them into IORESOURCE_REG resources (or open-coding
mfd_cells for each child)? Would there be a desire in creating a generic
set of helpers which create platform devices with IORESOURCE_REG resources,
based solely on OF addresses of children? What would be the correct
scope for these helpers?
next reply other threads:[~2022-12-22 13:49 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-22 13:48 Vladimir Oltean [this message]
2022-12-22 14:34 ` Advice on MFD-style probing of DSA switch SoCs Andrew Lunn
2022-12-22 16:18 ` Vladimir Oltean
2022-12-22 16:36 ` Andrew Lunn
2022-12-23 8:44 ` Krzysztof Kozlowski
2022-12-23 13:44 ` Vladimir Oltean
2023-02-07 6:49 ` Saravana Kannan
2023-02-11 1:27 ` Vladimir Oltean
2023-02-11 15:50 ` Andrew Lunn
2023-02-11 21:36 ` Vladimir Oltean
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221222134844.lbzyx5hz7z5n763n@skbuf \
--to=vladimir.oltean@nxp.com \
--cc=alexandre.belloni@bootlin.com \
--cc=andrew@lunn.ch \
--cc=colin.foster@in-advantage.com \
--cc=devicetree@vger.kernel.org \
--cc=f.fainelli@gmail.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lee@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox