Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support
On Fri, Jun 24, 2022 at 5:13 PM Vladimir Oltean wrote:
>
> On Fri, Jun 24, 2022 at 04:16:34PM -0700, Tim Harvey wrote:
> > On Fri, Jun 24, 2022 at 3:25 AM Vladimir Oltean
> > wrote:
> > >
> > > On Mon, May 23, 2022 at 11:25:49AM -0700, Tim Harvey wrote:
> > > > Add MV88E61XX DSA support:
> > > > - update dt: U-Boot dsa driver requires different device-tree syntax
> > > >than the linux driver in order to link the dsa ports to the mdio bus.
> > > > - update defconfig
> > > > - replace mv88e61xx_hw_reset weak override with board_phy_config
> > > > support
> > > >for mv88e61xx configuration that is outside the scope of the DSA
> > > > driver
> > > >
> > > > Signed-off-by: Tim Harvey
> > > > ---
> > > > v3:
> > > > - move mdio's mdio@0 subnode
> > > > v2: no changes
> > > > ---
> > > > arch/arm/dts/imx6qdl-gw5904.dtsi| 41
> > > > board/gateworks/gw_ventana/gw_ventana.c | 50 +
> > > > configs/gwventana_gw5904_defconfig | 7 ++--
> > > > 3 files changed, 62 insertions(+), 36 deletions(-)
> > > >
> > > > diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi
> > > > b/arch/arm/dts/imx6qdl-gw5904.dtsi
> > > > index 286c7a9924c2..1b2f70d1ccb2 100644
> > > > --- a/arch/arm/dts/imx6qdl-gw5904.dtsi
> > > > +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi
> > > > @@ -219,6 +219,33 @@
> > > > compatible = "marvell,mv88e6085";
> > > > reg = <0>;
> > > >
> > > > + mdios {
> > > > + #address-cells = <1>;
> > > > + #size-cells = <0>;
> > > > +
> > > > + mdio@0 {
> > >
> > > If you are going to follow this new model with a dedicated "mdios"
> > > subnode,
> > > I've consulted with Andrew Lunn and Florian Fainelli and there shouldn't
> > > be a problem to later make Linux accept this alternate binding format.
> > > But in that case, please match this OF node by a dedicated compatible
> > > string, like "marvell,mv88e6xxx-mdio-internal", so that there will be a
> > > way to differentiate this from the existing
> > > "marvell,mv88e6xxx-mdio-external"
> > > when support for that is added in U-Boot.
> > >
> > > Alternatively, to repeat myself, you can always follow the de-facto
> > > bindings for Linux mv88e6xxx which have:
> > >
> > > switch0: switch0@0 {
> > > compatible = "marvell,mv88e6190";
> > >
> > > ports {
> > > #address-cells = <1>;
> > > #size-cells = <0>;
> > >
> > > ...
> > > };
> > >
> > > mdio { // internal
> > > #address-cells = <1>;
> > > #size-cells = <0>;
> > >
> > > ...
> > > };
> > >
> > > mdio1 {
> > > compatible =
> > > "marvell,mv88e6xxx-mdio-external";
> > > #address-cells = <1>;
> > > #size-cells = <0>;
> > >
> > > ...
> > > };
> > > };
> > >
> >
> > Documentation/devicetree/bindings/net/dsa/marvell.txt shows en example
> > with just one child node under the internal mdio node:
> >
> > mdio {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > switch1phy0: switch1phy0@0 {
> > reg = <0>;
> > interrupt-parent = <>;
> > interrupts = <0
> > IRQ_TYPE_LEVEL_HIGH>;
> > };
> > };
> >
> > Am I to assume I can add additional nodes there for the other ports
> > such as the following?
> >
> > mdio {
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > switch1phy0: switch1phy0@0 {
> > reg = <0>;
> > };
> >
> > switch1phy1: switch1phy1@1 {
> > reg = <1>;
> > };
> >
> > switch1phy2: switch1phy2@2 {
> > reg = <2>;
> > };
> >
> > switch1phy3: switch1phy3@3 {
> > reg = <3>;
> > };
> >
> > ...
> > };
>
> Sure, but name those PHY nodes "ethernet-phy@N" rather than
Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support
On Fri, Jun 24, 2022 at 04:16:34PM -0700, Tim Harvey wrote:
> On Fri, Jun 24, 2022 at 3:25 AM Vladimir Oltean
> wrote:
> >
> > On Mon, May 23, 2022 at 11:25:49AM -0700, Tim Harvey wrote:
> > > Add MV88E61XX DSA support:
> > > - update dt: U-Boot dsa driver requires different device-tree syntax
> > >than the linux driver in order to link the dsa ports to the mdio bus.
> > > - update defconfig
> > > - replace mv88e61xx_hw_reset weak override with board_phy_config support
> > >for mv88e61xx configuration that is outside the scope of the DSA driver
> > >
> > > Signed-off-by: Tim Harvey
> > > ---
> > > v3:
> > > - move mdio's mdio@0 subnode
> > > v2: no changes
> > > ---
> > > arch/arm/dts/imx6qdl-gw5904.dtsi| 41
> > > board/gateworks/gw_ventana/gw_ventana.c | 50 +
> > > configs/gwventana_gw5904_defconfig | 7 ++--
> > > 3 files changed, 62 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi
> > > b/arch/arm/dts/imx6qdl-gw5904.dtsi
> > > index 286c7a9924c2..1b2f70d1ccb2 100644
> > > --- a/arch/arm/dts/imx6qdl-gw5904.dtsi
> > > +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi
> > > @@ -219,6 +219,33 @@
> > > compatible = "marvell,mv88e6085";
> > > reg = <0>;
> > >
> > > + mdios {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + mdio@0 {
> >
> > If you are going to follow this new model with a dedicated "mdios" subnode,
> > I've consulted with Andrew Lunn and Florian Fainelli and there shouldn't
> > be a problem to later make Linux accept this alternate binding format.
> > But in that case, please match this OF node by a dedicated compatible
> > string, like "marvell,mv88e6xxx-mdio-internal", so that there will be a
> > way to differentiate this from the existing
> > "marvell,mv88e6xxx-mdio-external"
> > when support for that is added in U-Boot.
> >
> > Alternatively, to repeat myself, you can always follow the de-facto
> > bindings for Linux mv88e6xxx which have:
> >
> > switch0: switch0@0 {
> > compatible = "marvell,mv88e6190";
> >
> > ports {
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > ...
> > };
> >
> > mdio { // internal
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > ...
> > };
> >
> > mdio1 {
> > compatible =
> > "marvell,mv88e6xxx-mdio-external";
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > ...
> > };
> > };
> >
>
> Documentation/devicetree/bindings/net/dsa/marvell.txt shows en example
> with just one child node under the internal mdio node:
>
> mdio {
> #address-cells = <1>;
> #size-cells = <0>;
> switch1phy0: switch1phy0@0 {
> reg = <0>;
> interrupt-parent = <>;
> interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> };
> };
>
> Am I to assume I can add additional nodes there for the other ports
> such as the following?
>
> mdio {
> #address-cells = <1>;
> #size-cells = <0>;
>
> switch1phy0: switch1phy0@0 {
> reg = <0>;
> };
>
> switch1phy1: switch1phy1@1 {
> reg = <1>;
> };
>
> switch1phy2: switch1phy2@2 {
> reg = <2>;
> };
>
> switch1phy3: switch1phy3@3 {
> reg = <3>;
> };
>
> ...
> };
Sure, but name those PHY nodes "ethernet-phy@N" rather than "switchMphyN",
as Documentation/devicetree/bindings/net/ethernet-phy.yaml requires.
Many mistakes were made in writing mv88e6xxx device trees, we don't need
to follow each and every one of them, only the important ones.
Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support
On Fri, Jun 24, 2022 at 3:25 AM Vladimir Oltean wrote:
>
> On Mon, May 23, 2022 at 11:25:49AM -0700, Tim Harvey wrote:
> > Add MV88E61XX DSA support:
> > - update dt: U-Boot dsa driver requires different device-tree syntax
> >than the linux driver in order to link the dsa ports to the mdio bus.
> > - update defconfig
> > - replace mv88e61xx_hw_reset weak override with board_phy_config support
> >for mv88e61xx configuration that is outside the scope of the DSA driver
> >
> > Signed-off-by: Tim Harvey
> > ---
> > v3:
> > - move mdio's mdio@0 subnode
> > v2: no changes
> > ---
> > arch/arm/dts/imx6qdl-gw5904.dtsi| 41
> > board/gateworks/gw_ventana/gw_ventana.c | 50 +
> > configs/gwventana_gw5904_defconfig | 7 ++--
> > 3 files changed, 62 insertions(+), 36 deletions(-)
> >
> > diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi
> > b/arch/arm/dts/imx6qdl-gw5904.dtsi
> > index 286c7a9924c2..1b2f70d1ccb2 100644
> > --- a/arch/arm/dts/imx6qdl-gw5904.dtsi
> > +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi
> > @@ -219,6 +219,33 @@
> > compatible = "marvell,mv88e6085";
> > reg = <0>;
> >
> > + mdios {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + mdio@0 {
>
> If you are going to follow this new model with a dedicated "mdios" subnode,
> I've consulted with Andrew Lunn and Florian Fainelli and there shouldn't
> be a problem to later make Linux accept this alternate binding format.
> But in that case, please match this OF node by a dedicated compatible
> string, like "marvell,mv88e6xxx-mdio-internal", so that there will be a
> way to differentiate this from the existing "marvell,mv88e6xxx-mdio-external"
> when support for that is added in U-Boot.
>
> Alternatively, to repeat myself, you can always follow the de-facto
> bindings for Linux mv88e6xxx which have:
>
> switch0: switch0@0 {
> compatible = "marvell,mv88e6190";
>
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> ...
> };
>
> mdio { // internal
> #address-cells = <1>;
> #size-cells = <0>;
>
> ...
> };
>
> mdio1 {
> compatible =
> "marvell,mv88e6xxx-mdio-external";
> #address-cells = <1>;
> #size-cells = <0>;
>
> ...
> };
> };
>
Documentation/devicetree/bindings/net/dsa/marvell.txt shows en example
with just one child node under the internal mdio node:
mdio {
#address-cells = <1>;
#size-cells = <0>;
switch1phy0: switch1phy0@0 {
reg = <0>;
interrupt-parent = <>;
interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
};
};
Am I to assume I can add additional nodes there for the other ports
such as the following?
mdio {
#address-cells = <1>;
#size-cells = <0>;
switch1phy0: switch1phy0@0 {
reg = <0>;
};
switch1phy1: switch1phy1@1 {
reg = <1>;
};
switch1phy2: switch1phy2@2 {
reg = <2>;
};
switch1phy3: switch1phy3@3 {
reg = <3>;
};
...
};
Best Regards,
Tim
> and the associated parsing code:
>
> static int mv88e6xxx_mdios_register(struct mv88e6xxx_chip *chip,
> struct device_node *np)
> {
> struct device_node *child;
> int err;
>
> /* Always register one mdio bus for the internal/default mdio
> * bus. This maybe represented in the device tree, but is
> * optional.
> */
> child = of_get_child_by_name(np, "mdio");
> err = mv88e6xxx_mdio_register(chip, child, false);
> of_node_put(child);
> if (err)
> return err;
>
> /* Walk the device tree, and see if
Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support
On Mon, May 23, 2022 at 11:25:49AM -0700, Tim Harvey wrote:
> Add MV88E61XX DSA support:
> - update dt: U-Boot dsa driver requires different device-tree syntax
>than the linux driver in order to link the dsa ports to the mdio bus.
> - update defconfig
> - replace mv88e61xx_hw_reset weak override with board_phy_config support
>for mv88e61xx configuration that is outside the scope of the DSA driver
>
> Signed-off-by: Tim Harvey
> ---
> v3:
> - move mdio's mdio@0 subnode
> v2: no changes
> ---
> arch/arm/dts/imx6qdl-gw5904.dtsi| 41
> board/gateworks/gw_ventana/gw_ventana.c | 50 +
> configs/gwventana_gw5904_defconfig | 7 ++--
> 3 files changed, 62 insertions(+), 36 deletions(-)
>
> diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi
> b/arch/arm/dts/imx6qdl-gw5904.dtsi
> index 286c7a9924c2..1b2f70d1ccb2 100644
> --- a/arch/arm/dts/imx6qdl-gw5904.dtsi
> +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi
> @@ -219,6 +219,33 @@
> compatible = "marvell,mv88e6085";
> reg = <0>;
>
> + mdios {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + mdio@0 {
If you are going to follow this new model with a dedicated "mdios" subnode,
I've consulted with Andrew Lunn and Florian Fainelli and there shouldn't
be a problem to later make Linux accept this alternate binding format.
But in that case, please match this OF node by a dedicated compatible
string, like "marvell,mv88e6xxx-mdio-internal", so that there will be a
way to differentiate this from the existing "marvell,mv88e6xxx-mdio-external"
when support for that is added in U-Boot.
Alternatively, to repeat myself, you can always follow the de-facto
bindings for Linux mv88e6xxx which have:
switch0: switch0@0 {
compatible = "marvell,mv88e6190";
ports {
#address-cells = <1>;
#size-cells = <0>;
...
};
mdio { // internal
#address-cells = <1>;
#size-cells = <0>;
...
};
mdio1 {
compatible = "marvell,mv88e6xxx-mdio-external";
#address-cells = <1>;
#size-cells = <0>;
...
};
};
and the associated parsing code:
static int mv88e6xxx_mdios_register(struct mv88e6xxx_chip *chip,
struct device_node *np)
{
struct device_node *child;
int err;
/* Always register one mdio bus for the internal/default mdio
* bus. This maybe represented in the device tree, but is
* optional.
*/
child = of_get_child_by_name(np, "mdio");
err = mv88e6xxx_mdio_register(chip, child, false);
of_node_put(child);
if (err)
return err;
/* Walk the device tree, and see if there are any other nodes
* which say they are compatible with the external mdio
* bus.
*/
for_each_available_child_of_node(np, child) {
if (of_device_is_compatible(
child, "marvell,mv88e6xxx-mdio-external")) {
err = mv88e6xxx_mdio_register(chip, child, true);
if (err) {
mv88e6xxx_mdios_unregister(chip);
of_node_put(child);
return err;
}
}
}
return 0;
}
Personally I believe that if you have limited amount of time to spend on
U-Boot DM_DSA and DT bindings in general, you should just follow the
format already accepted by Linux ("mdio" node is for internal MDIO,
doesn't have compatible string, is placed directly under "switch" node",
while external MDIO is matched by compatible string and its node can
have any name).
What we should try to accomplish is that the DT blob that U-Boot creates
for itself here to be coherent enough that Linux is able to understand
and use it, in case we decide to pass it to the operating system. With
your approach you'd have work to do in Linux as well to accept the
"mdios" subnode and parse controllers by compatible string inside, and
I'm not exactly sure you're willing to do that.
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + sw_phy0: ethernet-phy@0 {
> + reg =
Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support
On Thu, Jun 23, 2022 at 5:42 AM Vladimir Oltean wrote:
>
> On Tue, Jun 21, 2022 at 09:57:35AM -0700, Tim Harvey wrote:
> > > > diff --git a/board/gateworks/gw_ventana/gw_ventana.c
> > > > b/board/gateworks/gw_ventana/gw_ventana.c
> > > > index c06630a66b66..bef3f7ef0d2b 100644
> > > > --- a/board/gateworks/gw_ventana/gw_ventana.c
> > > > +++ b/board/gateworks/gw_ventana/gw_ventana.c
> > > > @@ -68,44 +68,30 @@ int board_phy_config(struct phy_device *phydev)
> > > > phy_write(phydev, MDIO_DEVAD_NONE, 14, val);
> > > > }
> > > >
> > > > + /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE
> > > > Switch */
> > > > + else if (phydev->phy_id == 0xa5a55a5a &&
> > > > + ((board_type == GW5904) || (board_type == GW5909))) {
> > > > + struct mii_dev *bus = miiphy_get_dev_by_name("mdio");
> > > > +
> > > > + puts("MV88E61XX ");
> > > > + /* GPIO[0] output CLK125 for RGMII_REFCLK */
> > > > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x62 << 8) |
> > > > 0xfe);
> > > > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x68 << 8) |
> > > > 7);
> > > > +
> > > > + /* Port 0-3 LED configuration: Table 80/82 */
> > > > + /* LED configuration: 7:4-green (8=Activity) 3:0 amber
> > > > (8=Link) */
> > > > + bus->write(bus, 0x10, 0, 0x16, 0x8088);
> > > > + bus->write(bus, 0x11, 0, 0x16, 0x8088);
> > > > + bus->write(bus, 0x12, 0, 0x16, 0x8088);
> > > > + bus->write(bus, 0x13, 0, 0x16, 0x8088);
> > > > + }
> > > > +
> > >
> > > There's nothing too board specific about this configuration, why do you
> > > feel it does not belong to the DSA driver?
> > >
> > > Some macros hiding away magic register addresses and values would be
> > > good either way.
> > >
> >
> > I don't much care for MAC/PHY drivers configuring GPIO's and LED's due
> > to the lack of consistent dt bindings for such a thing and I wasn't
> > planning on trying to enhance the capabilities of the mv88e6xxx
> > driver.
> >
> > There are 7 functions each of the 15 GPIO's can be set to:
> > 0 - GPIO
> > 1 - PTP_TRIG - Precise Timing Protocol Trigger Generate Output
> > 2 - PTP_EVREQ - Precise Timing Protocol Event Request Input
> > 3 - PTP_EXTCLK - Precise Timing Protocol ExternalClock Input
> > 4 - SE_RCLK0 - SyncE Receive Clock 0 Output
> > 5 - SE_RCLK1 - SyncE Receive Clock 1 Output
> > 6 - Reserved
> > 7 - CLK125 - Free running 125 MHz Clock Output
> >
> > There are two LED's per port each of which can be set to 16 different
> > functions also and these functions take a lot of words to describe
> > thus probably wouldn't lend well to #define names.
> >
> > Are there dt bindings that you would suggest I look at for per-port
> > LED config on a switch, or GPIO config on a switch? If I add
> > dt-bindings I'll have to update the kernel driver as well which again,
> > was not my goal here. Maybe moving these into the mv88e6xxx driver per
> > dt bindings could be a 'todo'.
> >
> > This patch isn't making what is already in the board file more
> > obscure, it is just updating it to work with the new dsa driver. The
> > following was what this patch replaced:
> > -#ifdef CONFIG_MV88E61XX_SWITCH
> > -int mv88e61xx_hw_reset(struct phy_device *phydev)
> > -{
> > - struct mii_dev *bus = phydev->bus;
> > -
> > - /* GPIO[0] output, CLK125 */
> > - debug("enabling RGMII_REFCLK\n");
> > - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0,
> > - 0x1a /*MV_SCRATCH_MISC*/,
> > - (1 << 15) | (0x62 /*MV_GPIO_DIR*/ << 8) | 0xfe);
> > - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0,
> > - 0x1a /*MV_SCRATCH_MISC*/,
> > - (1 << 15) | (0x68 /*MV_GPIO01_CNTL*/ << 8) | 7);
> > -
> > - /* RGMII delay - Physical Control register bit[15:14] */
> > - debug("setting port%d RGMII rx/tx delay\n",
> > CONFIG_MV88E61XX_CPU_PORT);
> > - /* forced 1000mbps full-duplex link */
> > - bus->write(bus, 0x10 + CONFIG_MV88E61XX_CPU_PORT, 0, 1, 0xc0fe);
> > - phydev->autoneg = AUTONEG_DISABLE;
> > - phydev->speed = SPEED_1000;
> > - phydev->duplex = DUPLEX_FULL;
> > -
> > - /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */
> > - bus->write(bus, 0x10, 0, 0x16, 0x8088);
> > - bus->write(bus, 0x11, 0, 0x16, 0x8088);
> > - bus->write(bus, 0x12, 0, 0x16, 0x8088);
> > - bus->write(bus, 0x13, 0, 0x16, 0x8088);
> > -
> > - return 0;
> > -}
> > -#endif // CONFIG_MV88E61XX_SWITCH
> >
> > Best Regards,
> >
> > Tim
>
> Ok, I was thinking PHY LED configuration could be hardcoded in the
> driver itself (no DT bindings) and nobody would probably even notice.
No, it is always a bad idea to hard code a specific configuration in a
driver. Most modern PHY's have 4 LEDs with 16 configurations and board
vendors vary on what 2 LED's they use and how. I have seen
Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support
On Tue, Jun 21, 2022 at 09:57:35AM -0700, Tim Harvey wrote:
> > > diff --git a/board/gateworks/gw_ventana/gw_ventana.c
> > > b/board/gateworks/gw_ventana/gw_ventana.c
> > > index c06630a66b66..bef3f7ef0d2b 100644
> > > --- a/board/gateworks/gw_ventana/gw_ventana.c
> > > +++ b/board/gateworks/gw_ventana/gw_ventana.c
> > > @@ -68,44 +68,30 @@ int board_phy_config(struct phy_device *phydev)
> > > phy_write(phydev, MDIO_DEVAD_NONE, 14, val);
> > > }
> > >
> > > + /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch
> > > */
> > > + else if (phydev->phy_id == 0xa5a55a5a &&
> > > + ((board_type == GW5904) || (board_type == GW5909))) {
> > > + struct mii_dev *bus = miiphy_get_dev_by_name("mdio");
> > > +
> > > + puts("MV88E61XX ");
> > > + /* GPIO[0] output CLK125 for RGMII_REFCLK */
> > > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x62 << 8) |
> > > 0xfe);
> > > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x68 << 8) | 7);
> > > +
> > > + /* Port 0-3 LED configuration: Table 80/82 */
> > > + /* LED configuration: 7:4-green (8=Activity) 3:0 amber
> > > (8=Link) */
> > > + bus->write(bus, 0x10, 0, 0x16, 0x8088);
> > > + bus->write(bus, 0x11, 0, 0x16, 0x8088);
> > > + bus->write(bus, 0x12, 0, 0x16, 0x8088);
> > > + bus->write(bus, 0x13, 0, 0x16, 0x8088);
> > > + }
> > > +
> >
> > There's nothing too board specific about this configuration, why do you
> > feel it does not belong to the DSA driver?
> >
> > Some macros hiding away magic register addresses and values would be
> > good either way.
> >
>
> I don't much care for MAC/PHY drivers configuring GPIO's and LED's due
> to the lack of consistent dt bindings for such a thing and I wasn't
> planning on trying to enhance the capabilities of the mv88e6xxx
> driver.
>
> There are 7 functions each of the 15 GPIO's can be set to:
> 0 - GPIO
> 1 - PTP_TRIG - Precise Timing Protocol Trigger Generate Output
> 2 - PTP_EVREQ - Precise Timing Protocol Event Request Input
> 3 - PTP_EXTCLK - Precise Timing Protocol ExternalClock Input
> 4 - SE_RCLK0 - SyncE Receive Clock 0 Output
> 5 - SE_RCLK1 - SyncE Receive Clock 1 Output
> 6 - Reserved
> 7 - CLK125 - Free running 125 MHz Clock Output
>
> There are two LED's per port each of which can be set to 16 different
> functions also and these functions take a lot of words to describe
> thus probably wouldn't lend well to #define names.
>
> Are there dt bindings that you would suggest I look at for per-port
> LED config on a switch, or GPIO config on a switch? If I add
> dt-bindings I'll have to update the kernel driver as well which again,
> was not my goal here. Maybe moving these into the mv88e6xxx driver per
> dt bindings could be a 'todo'.
>
> This patch isn't making what is already in the board file more
> obscure, it is just updating it to work with the new dsa driver. The
> following was what this patch replaced:
> -#ifdef CONFIG_MV88E61XX_SWITCH
> -int mv88e61xx_hw_reset(struct phy_device *phydev)
> -{
> - struct mii_dev *bus = phydev->bus;
> -
> - /* GPIO[0] output, CLK125 */
> - debug("enabling RGMII_REFCLK\n");
> - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0,
> - 0x1a /*MV_SCRATCH_MISC*/,
> - (1 << 15) | (0x62 /*MV_GPIO_DIR*/ << 8) | 0xfe);
> - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0,
> - 0x1a /*MV_SCRATCH_MISC*/,
> - (1 << 15) | (0x68 /*MV_GPIO01_CNTL*/ << 8) | 7);
> -
> - /* RGMII delay - Physical Control register bit[15:14] */
> - debug("setting port%d RGMII rx/tx delay\n",
> CONFIG_MV88E61XX_CPU_PORT);
> - /* forced 1000mbps full-duplex link */
> - bus->write(bus, 0x10 + CONFIG_MV88E61XX_CPU_PORT, 0, 1, 0xc0fe);
> - phydev->autoneg = AUTONEG_DISABLE;
> - phydev->speed = SPEED_1000;
> - phydev->duplex = DUPLEX_FULL;
> -
> - /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */
> - bus->write(bus, 0x10, 0, 0x16, 0x8088);
> - bus->write(bus, 0x11, 0, 0x16, 0x8088);
> - bus->write(bus, 0x12, 0, 0x16, 0x8088);
> - bus->write(bus, 0x13, 0, 0x16, 0x8088);
> -
> - return 0;
> -}
> -#endif // CONFIG_MV88E61XX_SWITCH
>
> Best Regards,
>
> Tim
Ok, I was thinking PHY LED configuration could be hardcoded in the
driver itself (no DT bindings) and nobody would probably even notice.
For pin configuration as RGMII 125 MHz clock or GPIO or otherwise,
there would probably need to be a "pinctrl" DT subnode with a specific
pinctrl driver attached. It's best if the development for that would be
done in concert with the Linux community, perhaps even in Linux first.
In any case, from my side it's ok if you leave a pinctrl sub-driver as TODO.
Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support
On Mon, Jun 20, 2022 at 4:42 AM Vladimir Oltean wrote:
>
> On Mon, May 23, 2022 at 11:25:49AM -0700, Tim Harvey wrote:
> > Add MV88E61XX DSA support:
> > - update dt: U-Boot dsa driver requires different device-tree syntax
> >than the linux driver in order to link the dsa ports to the mdio bus.
> > - update defconfig
> > - replace mv88e61xx_hw_reset weak override with board_phy_config support
> >for mv88e61xx configuration that is outside the scope of the DSA driver
> >
> > Signed-off-by: Tim Harvey
> > ---
> > v3:
> > - move mdio's mdio@0 subnode
> > v2: no changes
> > ---
> > arch/arm/dts/imx6qdl-gw5904.dtsi| 41
> > board/gateworks/gw_ventana/gw_ventana.c | 50 +
> > configs/gwventana_gw5904_defconfig | 7 ++--
> > 3 files changed, 62 insertions(+), 36 deletions(-)
> >
> > diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi
> > b/arch/arm/dts/imx6qdl-gw5904.dtsi
> > index 286c7a9924c2..1b2f70d1ccb2 100644
> > --- a/arch/arm/dts/imx6qdl-gw5904.dtsi
> > +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi
> > @@ -219,6 +219,33 @@
> > compatible = "marvell,mv88e6085";
> > reg = <0>;
> >
> > + mdios {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + mdio@0 {
> > + reg = <0>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + sw_phy0: ethernet-phy@0 {
> > + reg = <0x0>;
> > + };
> > +
> > + sw_phy1: ethernet-phy@1 {
> > + reg = <0x1>;
> > + };
> > +
> > + sw_phy2: ethernet-phy@2 {
> > + reg = <0x2>;
> > + };
> > +
> > + sw_phy3: ethernet-phy@3 {
> > + reg = <0x3>;
> > + };
> > + };
> > + };
> > +
> > ports {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > @@ -226,27 +253,41 @@
> > port@0 {
> > reg = <0>;
> > label = "lan4";
> > + phy-mode = "internal";
> > + phy-handle = <_phy0>;
> > };
> >
> > port@1 {
> > reg = <1>;
> > label = "lan3";
> > + phy-mode = "internal";
> > + phy-handle = <_phy1>;
> > };
> >
> > port@2 {
> > reg = <2>;
> > label = "lan2";
> > + phy-mode = "internal";
> > + phy-handle = <_phy2>;
> > };
> >
> > port@3 {
> > reg = <3>;
> > label = "lan1";
> > + phy-mode = "internal";
> > + phy-handle = <_phy3>;
> > };
> >
> > port@5 {
> > reg = <5>;
> > label = "cpu";
> > ethernet = <>;
> > + phy-mode = "rgmii-id";
> > +
> > + fixed-link {
> > + speed = <1000>;
> > + full-duplex;
> > + };
> > };
> > };
> > };
> > diff --git a/board/gateworks/gw_ventana/gw_ventana.c
> > b/board/gateworks/gw_ventana/gw_ventana.c
> > index c06630a66b66..bef3f7ef0d2b 100644
> > --- a/board/gateworks/gw_ventana/gw_ventana.c
> > +++ b/board/gateworks/gw_ventana/gw_ventana.c
> > @@ -68,44 +68,30 @@ int board_phy_config(struct phy_device *phydev)
> > phy_write(phydev, MDIO_DEVAD_NONE, 14, val);
> > }
> >
> > + /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */
> > + else if (phydev->phy_id == 0xa5a55a5a &&
>
> PHY_FIXED_ID, but see below.
Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support
On Mon, May 23, 2022 at 11:25:49AM -0700, Tim Harvey wrote:
> Add MV88E61XX DSA support:
> - update dt: U-Boot dsa driver requires different device-tree syntax
>than the linux driver in order to link the dsa ports to the mdio bus.
> - update defconfig
> - replace mv88e61xx_hw_reset weak override with board_phy_config support
>for mv88e61xx configuration that is outside the scope of the DSA driver
>
> Signed-off-by: Tim Harvey
> ---
> v3:
> - move mdio's mdio@0 subnode
> v2: no changes
> ---
> arch/arm/dts/imx6qdl-gw5904.dtsi| 41
> board/gateworks/gw_ventana/gw_ventana.c | 50 +
> configs/gwventana_gw5904_defconfig | 7 ++--
> 3 files changed, 62 insertions(+), 36 deletions(-)
>
> diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi
> b/arch/arm/dts/imx6qdl-gw5904.dtsi
> index 286c7a9924c2..1b2f70d1ccb2 100644
> --- a/arch/arm/dts/imx6qdl-gw5904.dtsi
> +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi
> @@ -219,6 +219,33 @@
> compatible = "marvell,mv88e6085";
> reg = <0>;
>
> + mdios {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + mdio@0 {
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + sw_phy0: ethernet-phy@0 {
> + reg = <0x0>;
> + };
> +
> + sw_phy1: ethernet-phy@1 {
> + reg = <0x1>;
> + };
> +
> + sw_phy2: ethernet-phy@2 {
> + reg = <0x2>;
> + };
> +
> + sw_phy3: ethernet-phy@3 {
> + reg = <0x3>;
> + };
> + };
> + };
> +
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
> @@ -226,27 +253,41 @@
> port@0 {
> reg = <0>;
> label = "lan4";
> + phy-mode = "internal";
> + phy-handle = <_phy0>;
> };
>
> port@1 {
> reg = <1>;
> label = "lan3";
> + phy-mode = "internal";
> + phy-handle = <_phy1>;
> };
>
> port@2 {
> reg = <2>;
> label = "lan2";
> + phy-mode = "internal";
> + phy-handle = <_phy2>;
> };
>
> port@3 {
> reg = <3>;
> label = "lan1";
> + phy-mode = "internal";
> + phy-handle = <_phy3>;
> };
>
> port@5 {
> reg = <5>;
> label = "cpu";
> ethernet = <>;
> + phy-mode = "rgmii-id";
> +
> + fixed-link {
> + speed = <1000>;
> + full-duplex;
> + };
> };
> };
> };
> diff --git a/board/gateworks/gw_ventana/gw_ventana.c
> b/board/gateworks/gw_ventana/gw_ventana.c
> index c06630a66b66..bef3f7ef0d2b 100644
> --- a/board/gateworks/gw_ventana/gw_ventana.c
> +++ b/board/gateworks/gw_ventana/gw_ventana.c
> @@ -68,44 +68,30 @@ int board_phy_config(struct phy_device *phydev)
> phy_write(phydev, MDIO_DEVAD_NONE, 14, val);
> }
>
> + /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */
> + else if (phydev->phy_id == 0xa5a55a5a &&
PHY_FIXED_ID, but see below.
> + ((board_type == GW5904) || (board_type == GW5909))) {
> + struct mii_dev *bus = miiphy_get_dev_by_name("mdio");
> +
> + puts("MV88E61XX ");
> + /* GPIO[0] output CLK125 for RGMII_REFCLK */
> +
Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support
Hi Tim, On Tue, Jun 14, 2022 at 10:00:57AM -0700, Tim Harvey wrote: > Vladimir, > > I'm not sure if you've had a chance to look at this latest patch > revision yet. I believe above is what you were describing as the > correct way to do this (without modifying the Linux driver and > improving bindings) > > Best Regards, > > Tim I'm currently on vacation but I'm slowly working through my inbox. This is certainly one of the reasonable ways of doing things, but it will require modifying the mv88e6xxx Linux driver to support an "mdios" subnode (something which nobody should object against, I believe). I'll follow up with more comments when I get to these patches.
Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support
On Mon, May 23, 2022 at 11:26 AM Tim Harvey wrote:
>
> Add MV88E61XX DSA support:
> - update dt: U-Boot dsa driver requires different device-tree syntax
>than the linux driver in order to link the dsa ports to the mdio bus.
> - update defconfig
> - replace mv88e61xx_hw_reset weak override with board_phy_config support
>for mv88e61xx configuration that is outside the scope of the DSA driver
>
> Signed-off-by: Tim Harvey
> ---
> v3:
> - move mdio's mdio@0 subnode
> v2: no changes
> ---
> arch/arm/dts/imx6qdl-gw5904.dtsi| 41
> board/gateworks/gw_ventana/gw_ventana.c | 50 +
> configs/gwventana_gw5904_defconfig | 7 ++--
> 3 files changed, 62 insertions(+), 36 deletions(-)
>
> diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi
> b/arch/arm/dts/imx6qdl-gw5904.dtsi
> index 286c7a9924c2..1b2f70d1ccb2 100644
> --- a/arch/arm/dts/imx6qdl-gw5904.dtsi
> +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi
> @@ -219,6 +219,33 @@
> compatible = "marvell,mv88e6085";
> reg = <0>;
>
> + mdios {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + mdio@0 {
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + sw_phy0: ethernet-phy@0 {
> + reg = <0x0>;
> + };
> +
> + sw_phy1: ethernet-phy@1 {
> + reg = <0x1>;
> + };
> +
> + sw_phy2: ethernet-phy@2 {
> + reg = <0x2>;
> + };
> +
> + sw_phy3: ethernet-phy@3 {
> + reg = <0x3>;
> + };
> + };
> + };
> +
Vladimir,
I'm not sure if you've had a chance to look at this latest patch
revision yet. I believe above is what you were describing as the
correct way to do this (without modifying the Linux driver and
improving bindings)
Best Regards,
Tim
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
> @@ -226,27 +253,41 @@
> port@0 {
> reg = <0>;
> label = "lan4";
> + phy-mode = "internal";
> + phy-handle = <_phy0>;
> };
>
> port@1 {
> reg = <1>;
> label = "lan3";
> + phy-mode = "internal";
> + phy-handle = <_phy1>;
> };
>
> port@2 {
> reg = <2>;
> label = "lan2";
> + phy-mode = "internal";
> + phy-handle = <_phy2>;
> };
>
> port@3 {
> reg = <3>;
> label = "lan1";
> + phy-mode = "internal";
> + phy-handle = <_phy3>;
> };
>
> port@5 {
> reg = <5>;
> label = "cpu";
> ethernet = <>;
> + phy-mode = "rgmii-id";
> +
> + fixed-link {
> + speed = <1000>;
> + full-duplex;
> + };
> };
> };
> };
> diff --git a/board/gateworks/gw_ventana/gw_ventana.c
> b/board/gateworks/gw_ventana/gw_ventana.c
> index c06630a66b66..bef3f7ef0d2b 100644
> --- a/board/gateworks/gw_ventana/gw_ventana.c
> +++ b/board/gateworks/gw_ventana/gw_ventana.c
> @@ -68,44 +68,30 @@ int board_phy_config(struct phy_device *phydev)
> phy_write(phydev, MDIO_DEVAD_NONE, 14, val);
> }
>
> + /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE
