Message-ID: <20221222161806.mhqsr2ot64v34al2@skbuf>
Date:   Thu, 22 Dec 2022 18:18:06 +0200
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Lee Jones <lee@...nel.org>,
        Colin Foster <colin.foster@...advantage.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: Advice on MFD-style probing of DSA switch SoCs

On Thu, Dec 22, 2022 at 03:34:27PM +0100, Andrew Lunn wrote:
> > 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:
> 
> Have you looked at probe ordering issues? MFD devices i've played with
> are very independent. They are a bunch of IP blocks sharing a bus. A
> switch however is very interconnected.

Some bits are functionally fully independent in a switch SoC as well -
the GPIO controller might have nothing to do with the MDIO controller.
Sure, there might be interdependencies too. That being said, there
shouldn't be probe ordering issues. Children of the soc node can depend
on each other (not circularly), but they are probed in parallel by the
soc driver, so that's not a problem.

> > 	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>;
> 
> ...
> 
> > 
> > 				port@3 {
> > 					reg = <3>;
> > 					label = "1ge_p2";
> > 					phy-mode = "rgmii-id";
> > 					phy-handle = <&sw2_mii3_phy>;
> > 				};
> 
> So for the switch to probe, the PHY needs to probe first.

Yup. This is better/clearer compared to the original binding, where the
mdio was a child of the ethernet-switch, now they can probe truly in
parallel, and fw_devlink can even enforce any ordering it wants.

> > 		mdio@...000 {
> > 			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>;
> > 			};
> 
> For the PHY to probe requires that the interrupt controller probes first.

Yup. This was actually a problem with fw_devlink with the old bindings,
especially with mv88e6xxx-type bindings, where the interrupt-controller;
property gets applied to the ethernet-switch node, and so, the
interrupts-extended property is practically a backlink.

> > 		slir2: interrupt-controller@...fe0 {
> > 			compatible = "nxp,sja1110-acu-slir";
> > 			reg = <0x711fe0 0x10>;
> > 			interrupt-controller;
> > 			#interrupt-cells = <1>;
> > 			interrupt-parent = <&gpio 10>;
> > 		};
> 
> and the interrupt controller requires its parent gpio controller
> probes first. I assume this is the host SOC GPIO controller, not the
> switches GPIO controller.

Yup, the interrupt-parent is a host interrupt, not something handled by
the switch. Although I've added logic to this irqchip driver (not posted)
which makes the parent interrupt completely optional, and it falls back
to poll mode if the parent IRQ is missing.

> 
> > 		sw2_rgu: reset@...000 {
> > 			compatible = "nxp,sja1110-rgu";
> > 			reg = <0x718000 0x1000>;
> > 			#reset-cells = <1>;
> > 		};
> 
> and presumably something needs to hit the reset at some point? Will
> there be DT phandles to this?

Yes, there already is a phandle in the ethernet-switch node:

			resets = <&sw2_rgu SJA1110_RGU_ETHSW_RST>;

This is something which SJA1110 engineers did better than SJA1105: the
reset domains are much better separated. Via the RGU block, one can
individually reset different peripherals.

Here's the content of my #include <dt-bindings/reset/nxp-sja1110-rgu.h>:

/* SPDX-License-Identifier: GPL-2.0+ */
/*
 * Device Tree binding constants for NXP SJA1110 Reset Generation Unit
 *
 * Copyright 2022 NXP
 */

#ifndef __DT_BINDINGS_NXP_SJA1110_RGU_H
#define __DT_BINDINGS_NXP_SJA1110_RGU_H

#define SJA1110_RGU_CBT1_RST		22
#define SJA1110_RGU_PERSS_RST		21
#define SJA1110_RGU_ETHSW_RST		20
#define SJA1110_RGU_MCSS_RST		19
#define SJA1110_RGU_NT1SS_PORT4_RST	18
#define SJA1110_RGU_NT1SS_PORT3_RST	17
#define SJA1110_RGU_NT1SS_PORT2_RST	16
#define SJA1110_RGU_NT1SS_PORT1_RST	15
#define SJA1110_RGU_CBT1_RESUME		14
#define SJA1110_RGU_CHIP_SYS_RST	13
#define SJA1110_RGU_PLL_DISABLE		12
#define SJA1110_RGU_DEVCFG_RST		11
#define SJA1110_RGU_OTP_RST		10
#define SJA1110_RGU_OSC_DISABLE		9
#define SJA1110_RGU_PMC_RST		8
#define SJA1110_RGU_SISS_RST		7
#define SJA1110_RGU_WARM_RST		6
#define SJA1110_RGU_COLD_RST		5
#define SJA1110_RGU_COLD_INP_RST	4
#define SJA1110_RGU_HW_RST		3
#define SJA1110_RGU_HW_INP_RST		2
#define SJA1110_RGU_POR_RST		1
#define SJA1110_RGU_PORTAP_RST		0

#endif /* __DT_BINDINGS_NXP_SJA1110_RGU_H */

I haven't yet discovered the mapping of all peripherals to reset
domains, but the switch reset really resets only the switch IP (this is
necessary to load a different static config into it).

This is different compared to SJA1105, where the XPCS also gets reset
when the switch reset is triggered. That led to workarounds such as me
needing to call xpcs_do_config() from sja1105_static_config_reload() -
every time the switching IP got reset. Food for thought, especially with
Sean Anderson's proposal to treat PCS devices using the regular device
model with independent probe and remove - how independent the PCS truly
is will depend on the hardware integration.

> > 
> > 		sw2_cgu: clock-controller@...000 {
> > 			compatible = "nxp,sja1110-cgu";
> > 			reg = <0x719000 0x1000>;
> > 			#clock-cells = <1>;
> > 		};
> 
> and phandles to the clock driver?

Yup. Consumers of CGU clocks can either be some other SJA1110
peripherals, or external devices altogether, which need to keep a clock
ticking. Currently I don't have a need for a CGU driver, it's there
mostly for illustrative purposes.

> Before doing too much in this direction, i would want to be sure you
> have sufficient control of ordering and the DT loops are not too
> complex, that the MFD core and the driver core can actually get
> everything probed.

Yup, I did think about that.

> The current way of doing it, with the drivers embedded inside the DSA
> driver is that DT blob only exposes what needs to be seen outside of
> the DSA driver. And the driver has full control over the order it
> probes its internal sub drivers, so ensuring they are probed in the
> correct order, and the linking is done in C, not DT, were again the
> driver is in full control.

Calling the sub-functions "drivers" is a bit too much, since in the
Linux device model sense, the old/standard way proposes only a single
driver for a single device structure: the spi_driver / i2c_driver /
mdio_driver / platform_driver for the switch chip as a whole. That
device driver calls dsa_register_switch(), but also optionally calls
mdiobus_register(), gpiochip_add_data(), irq_domain_add_linear(), etc
etc. Basically it registers a single struct device with all the
subsystems that the switching chip needs.

> I do however agree that being able to split sub drivers out of the
> main driver is a good idea, and putting them in the appropriated
> subsystem would help with code review.

Yup. Concretely, here, my problem is somewhat different. It is related
to OF addresses for all those SoC children. Somehow that was a problem
even in the old-style (current) bindings, but in a different way: see
the "mdios" subnode.

Some other mfd drivers which call of_platform_populate() and their
children have unit addresses are all memory-mapped in the CPU address
space. Not the case here.

> Maybe the media subsystem has some pointers how to do this. It also
> has complex devices made up from lots of sub devices.

You mean something like struct v4l2_subdev_ops? This seems like the
precise definition of what I'd like to avoid: a predefined set of
subfunctions decided by the DSA core.

Or maybe something else? To be honest, I don't know much about the media
subsystem. This is what I saw.

Powered by blists - more mailing lists