devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 3/4] RFC: net: dsa: Add bindings for Realtek SMI DSAs
       [not found] <20171105231909.5599-1-linus.walleij@linaro.org>
@ 2017-11-05 23:19 ` Linus Walleij
  2017-11-05 23:48   ` Andrew Lunn
  0 siblings, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2017-11-05 23:19 UTC (permalink / raw)
  To: Andrew Lunn, Vivien Didelot, Florian Fainelli
  Cc: netdev, Linus Walleij, Antti Seppälä,
	Roman Yeryomin, Colin Leitner, Gabor Juhos, devicetree

The Realtek SMI family is a set of DSA chips that provide
switching in routers. This binding just follows the pattern
set by other switches but with the introduction of an embedded
irqchip to demux and handle the interrupts fired by the single
line from the chip.

This interrupt construction is similar to how we handle
interrupt controllers inside PCI bridges etc.

Cc: Antti Seppälä <a.seppala@gmail.com>
Cc: Roman Yeryomin <roman@advem.lv>
Cc: Colin Leitner <colin.leitner@googlemail.com>
Cc: Gabor Juhos <juhosg@openwrt.org>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../devicetree/bindings/net/dsa/realtek-smi.txt    | 104 +++++++++++++++++++++
 1 file changed, 104 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/dsa/realtek-smi.txt

diff --git a/Documentation/devicetree/bindings/net/dsa/realtek-smi.txt b/Documentation/devicetree/bindings/net/dsa/realtek-smi.txt
new file mode 100644
index 000000000000..95e96d49c0be
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/dsa/realtek-smi.txt
@@ -0,0 +1,104 @@
+Realtek SMI-based Switches
+==========================
+
+The SMI "Simple Management Interface" is a two-wire protocol using
+bit-banged GPIO that while it reuses the MDIO lines MCK and MDIO does
+not use the MDIO protocol. This binding defines how to specify the
+SMI-based Realtek devices.
+
+Required properties:
+
+- compatible: must be exactly one of:
+      "realtek,rtl8366"
+      "realtek,rtl8369"
+      "realtek,rtl8366rb"
+      "realtek,rtl8366s"
+      "realtek,rtl8367"
+      "realtek,rtl8367b"
+
+Required subnode:
+
+- interrupt-controller
+
+  This defines an interrupt controller with an IRQ line (typically
+  a GPIO) that will demultiplex and handle the interrupt from the single
+  interrupt line coming out of one of the SMI-based chips. It most
+  importantly provides link up/down interrupts to the PHY blocks inside
+  the ASIC.
+
+Required properties of interrupt-controller:
+
+- interrupt: parent interrupt, see interrupt-controller/interrupts.txt
+- interrupt-controller: see interrupt-controller/interrupts.txt
+- #address-cells: should be <0>
+- #interrupt-cells: should be <1>
+
+See net/dsa/dsa.txt for a list of additional required and optional properties
+and subnodes.
+
+
+Examples:
+
+switch {
+	compatible = "realtek,rtl8366rb";
+	reg = <0>;
+	/* 22 = MDIO (has input reads), 21 = MDC (clock, output only) */
+	mdc-gpios = <&gpio0 21 GPIO_ACTIVE_HIGH>;
+	mdio-gpios = <&gpio0 22 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&gpio0 14 GPIO_ACTIVE_LOW>;
+
+	switch_intc: interrupt-controller {
+		/* GPIO 15 provides the interrupt */
+		interrupt-parent = <&gpio0>;
+		interrupts = <15 IRQ_TYPE_LEVEL_LOW>;
+		interrupt-controller;
+		#address-cells = <0>;
+		#interrupt-cells = <1>;
+	};
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0>;
+		port@0 {
+			reg = <0>;
+			label = "lan0";
+			interrupt-parent = <&switch_intc>;
+			interrupts = <0>;
+		};
+		port@1 {
+			reg = <1>;
+			label = "lan1";
+			interrupt-parent = <&switch_intc>;
+			interrupts = <1>;
+		};
+		port@2 {
+			reg = <2>;
+			label = "lan2";
+			interrupt-parent = <&switch_intc>;
+			interrupts = <2>;
+		};
+		port@3 {
+			reg = <3>;
+			label = "lan3";
+			interrupt-parent = <&switch_intc>;
+			interrupts = <3>;
+		};
+		port@4 {
+			reg = <4>;
+			label = "wan";
+			interrupt-parent = <&switch_intc>;
+			interrupts = <4>;
+		};
+		phy0: port@5 {
+			reg = <5>;
+			label = "cpu";
+			ethernet = <&gmac0>;
+			phy-mode = "rgmii";
+			fixed-link {
+				speed = <1000>;
+				full-duplex;
+			};
+		};
+	};
+};
-- 
2.13.6

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] RFC: net: dsa: Add bindings for Realtek SMI DSAs
  2017-11-05 23:19 ` [PATCH 3/4] RFC: net: dsa: Add bindings for Realtek SMI DSAs Linus Walleij
@ 2017-11-05 23:48   ` Andrew Lunn
       [not found]     ` <20171105234831.GA24822-g2DYL2Zd6BY@public.gmane.org>
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Lunn @ 2017-11-05 23:48 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Vivien Didelot, Florian Fainelli, netdev, Antti Seppälä,
	Roman Yeryomin, Colin Leitner, Gabor Juhos, devicetree

> This interrupt construction is similar to how we handle
> interrupt controllers inside PCI bridges etc.

Hi Linus

Your interrupt handling is going in the right direction, but needs
further work. The PHY interrupt is a phy property, so should be in the
PHY node in device tree.

The Marvell driver gives an example of this, and
vf610-zii-dev-rev-c.dts is an example DT blob you can look at.

> +	ports {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		reg = <0>;
> +		port@0 {
> +			reg = <0>;
> +			label = "lan0";

So here, you should have a

   	     	    	phy-handle = <&phy0>;

linking this MAC to the PHY connected to it.


> +		};

And then an MDIO bus, listing the PHYs

               mdio {
                        #address-cells = <1>;
                        #size-cells = <0>;

                        phy0: phy@0 {
                                reg = <0>;
				interrupt-parent = <&switch_intc>;
				interrupts = <0>;
                        };

It is here you list the interrupts. And the PHY subsystem will link
the interrupt to the PHY when it enumerate the MDIO bus.

You have most of the code already for implementing the MDIO bus. The
rest you can probably borrow from the mv88e6xxx driver.

     Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] RFC: net: dsa: Add bindings for Realtek SMI DSAs
       [not found]     ` <20171105234831.GA24822-g2DYL2Zd6BY@public.gmane.org>
@ 2017-11-29 12:24       ` Linus Walleij
  2017-11-29 15:56         ` Andrew Lunn
  0 siblings, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2017-11-29 12:24 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Vivien Didelot, Florian Fainelli, netdev-u79uwXL29TY76Z2rM5mHXA,
	Antti Seppälä,
	Roman Yeryomin, Colin Leitner, Gabor Juhos,
	devicetree-u79uwXL29TY76Z2rM5mHXA

On Mon, Nov 6, 2017 at 12:48 AM, Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org> wrote:
>> This interrupt construction is similar to how we handle
>> interrupt controllers inside PCI bridges etc.
>
> Hi Linus
>
> Your interrupt handling is going in the right direction, but needs
> further work. The PHY interrupt is a phy property, so should be in the
> PHY node in device tree.
>
> The Marvell driver gives an example of this, and
> vf610-zii-dev-rev-c.dts is an example DT blob you can look at.
>
>> +     ports {
>> +             #address-cells = <1>;
>> +             #size-cells = <0>;
>> +             reg = <0>;
>> +             port@0 {
>> +                     reg = <0>;
>> +                     label = "lan0";
>
> So here, you should have a
>
>                         phy-handle = <&phy0>;
>
> linking this MAC to the PHY connected to it.

I have the phy-handle in the ethernet controller. This RTL8366RB
thing is just one big PHY as far as I know. So to give the complete picture
this is what I have in my tree right now with the RTL8366RB as PHY
and Gemini ethernet (yeah I'm upstreaming that too...) as the ethernet
controller:

/* This is a RealTek RTL8366RB switch and PHY using SMI over GPIO */
switch {
    compatible = "realtek,rtl8366rb";
    reg = <0>;
    /* 22 = MDIO (has input reads), 21 = MDC (clock, output only) */
    mdc-gpios = <&gpio0 21 GPIO_ACTIVE_HIGH>;
    mdio-gpios = <&gpio0 22 GPIO_ACTIVE_HIGH>;
    reset-gpios = <&gpio0 14 GPIO_ACTIVE_LOW>;
    realtek,disable-leds;

    switch_intc: interrupt-controller {
        /* GPIO 15 provides the interrupt */
        interrupt-parent = <&gpio0>;
        interrupts = <15 IRQ_TYPE_LEVEL_LOW>;
        interrupt-controller;
        #address-cells = <0>;
        #interrupt-cells = <1>;
    };

    ports {
        #address-cells = <1>;
        #size-cells = <0>;

        port@0 {
            reg = <0>;
            label = "lan0";
            interrupt-parent = <&switch_intc>;
            interrupts = <0>;
        };
        port@1 {
            reg = <1>;
            label = "lan1";
            interrupt-parent = <&switch_intc>;
            interrupts = <1>;
        };
        port@2 {
            reg = <2>;
            label = "lan2";
            interrupt-parent = <&switch_intc>;
            interrupts = <2>;
        };
        port@3 {
            reg = <3>;
            label = "lan3";
            interrupt-parent = <&switch_intc>;
            interrupts = <3>;
        };
        port@4 {
            reg = <4>;
            label = "wan";
            interrupt-parent = <&switch_intc>;
            interrupts = <12>;
        };
        phy0: port@5 {
            reg = <5>;
            label = "cpu";
            ethernet = <&gmac0>;
            phy-mode = "rgmii";
            fixed-link {
                speed = <1000>;
                full-duplex;
            };
    };
};


ethernet@60000000 {
    compatible = "cortina,gemini-ethernet";
    reg = <0x60000000 0x4000>;
    #address-cells = <1>;
    #size-cells = <1>;
    ranges;

    gmac0: port0 {
        compatible = "cortina,gemini-ethernet-port";
        reg = <0x60008000 0x2000>, /* Port 0 DMA/TOE */
        <0x6000a000 0x2000>; /* Port 0 GMAC */
        interrupt-parent = <&intcon>;
        interrupts = <1 IRQ_TYPE_LEVEL_HIGH>;
        resets = <&syscon GEMINI_RESET_GMAC0>;
        clocks = <&syscon GEMINI_CLK_GATE_GMAC0>;
        clock-names = "PCLK";
        phy-mode = "rgmii";
        phy-handle = <&phy0>;
    };
};


> And then an MDIO bus, listing the PHYs
>
>                mdio {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>
>                         phy0: phy@0 {
>                                 reg = <0>;
>                                 interrupt-parent = <&switch_intc>;
>                                 interrupts = <0>;
>                         };
>
> It is here you list the interrupts. And the PHY subsystem will link
> the interrupt to the PHY when it enumerate the MDIO bus.

I do get that to work with a lot of standard PHY drivers
in drivers/net/phy, that assume they have an IRQ line from
device tree or board files.

However when PHY slave children are spawn from the
internal DSA MDIO bus interrupts are not assigned from the
device tree, so that is why I have a separate patch for that.

> You have most of the code already for implementing the MDIO bus. The
> rest you can probably borrow from the mv88e6xxx driver.

I have a working MDIO bus coming out directly from the SDA core,
so that part is fine. I also patched in the corresponding PHY driver
(a Realtek derivative for this DSA only, so just a few lines add
in the Realtek PHY driver) and it works fine.

I will repost the series as a non-RFC when I have all parts working
and illustrate with a few examples so you see how I set it up.
I hope I didn't turn the entire subsystem on its head or something...

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] RFC: net: dsa: Add bindings for Realtek SMI DSAs
  2017-11-29 12:24       ` Linus Walleij
@ 2017-11-29 15:56         ` Andrew Lunn
  2017-11-29 21:28           ` Linus Walleij
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Lunn @ 2017-11-29 15:56 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Vivien Didelot, Florian Fainelli, netdev, Antti Seppälä,
	Roman Yeryomin, Colin Leitner, Gabor Juhos, devicetree

> I have the phy-handle in the ethernet controller. This RTL8366RB
> thing is just one big PHY as far as I know.

Hi Linus

We don't model switches as PHYs. They are their own device type.  And
the internal or external PHYs are just normal PHYs in the linux
model. Meaning their interrupt properties goes in the PHY node in
device tree, as documented in the phy.txt binding documentation.

       Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] RFC: net: dsa: Add bindings for Realtek SMI DSAs
  2017-11-29 15:56         ` Andrew Lunn
@ 2017-11-29 21:28           ` Linus Walleij
       [not found]             ` <CACRpkdZVXgFMiHpyUqw7ONYDcq6Htn3rTMRaBJkzd6T3WtX36A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2017-11-29 21:56             ` Andrew Lunn
  0 siblings, 2 replies; 12+ messages in thread
From: Linus Walleij @ 2017-11-29 21:28 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Vivien Didelot, Florian Fainelli, netdev, Antti Seppälä,
	Roman Yeryomin, Colin Leitner, Gabor Juhos,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Wed, Nov 29, 2017 at 4:56 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> I have the phy-handle in the ethernet controller. This RTL8366RB
>> thing is just one big PHY as far as I know.
>
> We don't model switches as PHYs. They are their own device type.  And
> the internal or external PHYs are just normal PHYs in the linux
> model. Meaning their interrupt properties goes in the PHY node in
> device tree, as documented in the phy.txt binding documentation.

I do model the PHYs on the switch as PHYs.
They are using the driver in drivers/phy/realtek.c.

The interrupts are assigned to the PHYs not to the Switch.
Just that the PHYs are on the MDIO bus inside the switch, of
course.

The switch however provides an irqchip to demux the interrupts.

I think there is some misunderstanding in what I'm trying to do..

I have tried learning the DSA ideas by reading e.g. your paper:
https://www.netdevconf.org/2.1/papers/distributed-switch-architecture.pdf

So I try my best to conform with these ideas.

I however have a hard time testing things since I don't really have a
system to compare to. What would be useful is to know how
commands like "ip" and "ifconfig" are used on a typical
say home router.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] RFC: net: dsa: Add bindings for Realtek SMI DSAs
       [not found]             ` <CACRpkdZVXgFMiHpyUqw7ONYDcq6Htn3rTMRaBJkzd6T3WtX36A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-11-29 21:48               ` Florian Fainelli
  0 siblings, 0 replies; 12+ messages in thread
From: Florian Fainelli @ 2017-11-29 21:48 UTC (permalink / raw)
  To: Linus Walleij, Andrew Lunn
  Cc: Vivien Didelot, netdev-u79uwXL29TY76Z2rM5mHXA,
	Antti Seppälä,
	Roman Yeryomin, Colin Leitner, Gabor Juhos,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On 11/29/2017 01:28 PM, Linus Walleij wrote:
> On Wed, Nov 29, 2017 at 4:56 PM, Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org> wrote:
>>> I have the phy-handle in the ethernet controller. This RTL8366RB
>>> thing is just one big PHY as far as I know.
>>
>> We don't model switches as PHYs. They are their own device type.  And
>> the internal or external PHYs are just normal PHYs in the linux
>> model. Meaning their interrupt properties goes in the PHY node in
>> device tree, as documented in the phy.txt binding documentation.
> 
> I do model the PHYs on the switch as PHYs.
> They are using the driver in drivers/phy/realtek.c.

That's good.

> 
> The interrupts are assigned to the PHYs not to the Switch.
> Just that the PHYs are on the MDIO bus inside the switch, of
> course.
> 
> The switch however provides an irqchip to demux the interrupts.
> 
> I think there is some misunderstanding in what I'm trying to do..
> 
> I have tried learning the DSA ideas by reading e.g. your paper:
> https://www.netdevconf.org/2.1/papers/distributed-switch-architecture.pdf
> 
> So I try my best to conform with these ideas.
> 
> I however have a hard time testing things since I don't really have a
> system to compare to. What would be useful is to know how
> commands like "ip" and "ifconfig" are used on a typical
> say home router.

There is a mock-up driver: drivers/net/dsa/dsa_loop.c which does not
pass any packets, but at least allows you to exercise user-space tools
and so on.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] RFC: net: dsa: Add bindings for Realtek SMI DSAs
  2017-11-29 21:28           ` Linus Walleij
       [not found]             ` <CACRpkdZVXgFMiHpyUqw7ONYDcq6Htn3rTMRaBJkzd6T3WtX36A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-11-29 21:56             ` Andrew Lunn
       [not found]               ` <20171129215659.GC1706-g2DYL2Zd6BY@public.gmane.org>
  1 sibling, 1 reply; 12+ messages in thread
From: Andrew Lunn @ 2017-11-29 21:56 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Vivien Didelot, Florian Fainelli, netdev, Antti Seppälä,
	Roman Yeryomin, Colin Leitner, Gabor Juhos,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Hi Linus

> Just that the PHYs are on the MDIO bus inside the switch, of
> course.

I think the problem might be, you are using the DSA provided MDIO bus.
The Marvell switches has a similar setup in terms of interrupts. The
PHY interrupts appear within the switch. So i implemented an interrupt
controller, just the same as you.

The problem is, the DSA provided MDIO bus is not linked to device
tree. So you cannot have phy nodes in device tree associated to it.

What i did for the Marvell driver is that driver itself implements an
MDIO bus (two actually in some chips), and the internal or external
PHYs are placed on the switch drivers MDIO bus, rather than the DSA
MDIO bus. The switch driver MDIO bus links to an mdio node in device
tree. I can then have interrupt properties in the phys on this MDIO
bus in device tree.

What actually might make sense, is to have the DSA MDIO bus look
inside the switches device tree node and see if there is an mdio
node. If so allow dsa_switch_setup() to use of_mdiobus_register()
instead of mdiobus_register().

      Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] RFC: net: dsa: Add bindings for Realtek SMI DSAs
       [not found]               ` <20171129215659.GC1706-g2DYL2Zd6BY@public.gmane.org>
@ 2017-11-29 23:19                 ` Linus Walleij
  2017-11-29 23:26                   ` Florian Fainelli
  0 siblings, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2017-11-29 23:19 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Vivien Didelot, Florian Fainelli, netdev-u79uwXL29TY76Z2rM5mHXA,
	Antti Seppälä,
	Roman Yeryomin, Colin Leitner, Gabor Juhos,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Wed, Nov 29, 2017 at 10:56 PM, Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org> wrote:

> I think the problem might be, you are using the DSA provided MDIO bus.
> The Marvell switches has a similar setup in terms of interrupts. The
> PHY interrupts appear within the switch. So i implemented an interrupt
> controller, just the same as you.
>
> The problem is, the DSA provided MDIO bus is not linked to device
> tree. So you cannot have phy nodes in device tree associated to it.
>
> What i did for the Marvell driver is that driver itself implements an
> MDIO bus (two actually in some chips), and the internal or external
> PHYs are placed on the switch drivers MDIO bus, rather than the DSA
> MDIO bus. The switch driver MDIO bus links to an mdio node in device
> tree. I can then have interrupt properties in the phys on this MDIO
> bus in device tree.
>
> What actually might make sense, is to have the DSA MDIO bus look
> inside the switches device tree node and see if there is an mdio
> node. If so allow dsa_switch_setup() to use of_mdiobus_register()
> instead of mdiobus_register().

Aha I think I see where my thinking went wrong.

I have been assuming (thought it was intuitive...) that ports and
PHYs are mapped 1:1.

So I assumed the port with reg = <N> is also the PHY with
reg = <N>

So naturally I added the PHY interrupt to the port node.

So you are saying that the PHY and the port are two
very disparate things in DSA terminology?

I guess all ports except the CPU port actually have
a 1:1 mapped PHY though, am I right?

Or are there in pracice things such that reg is different
on the port and the PHY connected to it? Then it makes
much sense to put an MDIO bus inside the switch DT
node and populate the PHY interrupts from there as you
say.

I can take a stab at fixing that if that is what we want.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] RFC: net: dsa: Add bindings for Realtek SMI DSAs
  2017-11-29 23:19                 ` Linus Walleij
@ 2017-11-29 23:26                   ` Florian Fainelli
       [not found]                     ` <f9bfa1e1-7f05-1e2b-6663-09d4d3bf6a12-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  2017-12-02 12:56                     ` Linus Walleij
  0 siblings, 2 replies; 12+ messages in thread
From: Florian Fainelli @ 2017-11-29 23:26 UTC (permalink / raw)
  To: Linus Walleij, Andrew Lunn
  Cc: Vivien Didelot, netdev, Antti Seppälä,
	Roman Yeryomin, Colin Leitner, Gabor Juhos,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On 11/29/2017 03:19 PM, Linus Walleij wrote:
> On Wed, Nov 29, 2017 at 10:56 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> 
>> I think the problem might be, you are using the DSA provided MDIO bus.
>> The Marvell switches has a similar setup in terms of interrupts. The
>> PHY interrupts appear within the switch. So i implemented an interrupt
>> controller, just the same as you.
>>
>> The problem is, the DSA provided MDIO bus is not linked to device
>> tree. So you cannot have phy nodes in device tree associated to it.
>>
>> What i did for the Marvell driver is that driver itself implements an
>> MDIO bus (two actually in some chips), and the internal or external
>> PHYs are placed on the switch drivers MDIO bus, rather than the DSA
>> MDIO bus. The switch driver MDIO bus links to an mdio node in device
>> tree. I can then have interrupt properties in the phys on this MDIO
>> bus in device tree.
>>
>> What actually might make sense, is to have the DSA MDIO bus look
>> inside the switches device tree node and see if there is an mdio
>> node. If so allow dsa_switch_setup() to use of_mdiobus_register()
>> instead of mdiobus_register().
> 
> Aha I think I see where my thinking went wrong.
> 
> I have been assuming (thought it was intuitive...) that ports and
> PHYs are mapped 1:1.
> 
> So I assumed the port with reg = <N> is also the PHY with
> reg = <N>
> 
> So naturally I added the PHY interrupt to the port node.
> 
> So you are saying that the PHY and the port are two
> very disparate things in DSA terminology?

Yes, because the port is some sort of simplified Ethernet MAC, whereas
the PHY is the PHY, and it usually exists in the same shape and size
irrespective of whether it's integrated into a switch, being external,
or being internal to a proper Ethernet NIC.

> 
> I guess all ports except the CPU port actually have
> a 1:1 mapped PHY though, am I right?

This is the typical case, but is not universally true.

> 
> Or are there in pracice things such that reg is different
> on the port and the PHY connected to it? Then it makes
> much sense to put an MDIO bus inside the switch DT
> node and populate the PHY interrupts from there as you
> say.

Yes, I have such systems here, Port 0 has its PHY at MDIO address 5 for
instance.

> 
> I can take a stab at fixing that if that is what we want.

While Andrew's suggestion to use of_mdiobus_register() even for the
built-in DSA created slave_mii_bus makes sense, I would rather recommend
you instantiate your own bus (ala mv88e6xxx), such that your DT will
likely look like:

switch@0 {
	compatible = "acme,switch";
	#address-cells = <1>;
	#size-cells = <0>;

	ports {

		port@0 {
			reg = <0>;
			phy-handle = <&phy0>;
		};

		port@1 {
			reg = <1>;
			phy-handle = <&phy1>;
		};

		port@8 {
			reg = <8>;
			ethernet = = <&eth0>;
		};
	};

	mdio {
		compatible = "acme,switch-mdio";

		phy@0 {
			reg = <0>;
		};

		phy@1 {
			reg = <1>;
		};
	};
};

That way it's clear which port maps to which PHY, and that the MDIO
controller is internal within the switch (and so are the PHYs).
-- 
Florian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] RFC: net: dsa: Add bindings for Realtek SMI DSAs
       [not found]                     ` <f9bfa1e1-7f05-1e2b-6663-09d4d3bf6a12-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-11-29 23:36                       ` Andrew Lunn
  0 siblings, 0 replies; 12+ messages in thread
From: Andrew Lunn @ 2017-11-29 23:36 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Linus Walleij, Vivien Didelot, netdev-u79uwXL29TY76Z2rM5mHXA,
	Antti Seppälä,
	Roman Yeryomin, Colin Leitner, Gabor Juhos,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

> While Andrew's suggestion to use of_mdiobus_register() even for the
> built-in DSA created slave_mii_bus makes sense, I would rather recommend
> you instantiate your own bus (ala mv88e6xxx), such that your DT will
> likely look like:

Hi Florian

I could still look like this, if the built in slave_mii_bus looked for
the mdio node.

Something like:

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 44e3fb7dec8c..6b64c09413bf 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -312,6 +312,7 @@ static void dsa_port_teardown(struct dsa_port *dp)
 
 static int dsa_switch_setup(struct dsa_switch *ds)
 {
+       struct device_node *node;
        int err;
 
        /* Initialize ds->phys_mii_mask before registering the slave MDIO bus
@@ -347,7 +348,11 @@ static int dsa_switch_setup(struct dsa_switch *ds)
 
                dsa_slave_mii_bus_init(ds);
 
-               err = mdiobus_register(ds->slave_mii_bus);
+               if (ds->dev->of_node &&
+                   node = of_get_child_by_name(pdev->dev.of_node, "mdio"))
+                       err = of_mdiobus_register(ds->slave_mii_bus, node);
+               else
+                       err = mdiobus_register(ds->slave_mii_bus);
                if (err < 0)
                        return err;
        }

	Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] RFC: net: dsa: Add bindings for Realtek SMI DSAs
  2017-11-29 23:26                   ` Florian Fainelli
       [not found]                     ` <f9bfa1e1-7f05-1e2b-6663-09d4d3bf6a12-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-12-02 12:56                     ` Linus Walleij
       [not found]                       ` <CACRpkdYoMVNh8eaTnaDQ59bsh4bC88biLaYSXyhnc4W83PMWzA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  1 sibling, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2017-12-02 12:56 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Andrew Lunn, Vivien Didelot, netdev, Antti Seppälä,
	Roman Yeryomin, Colin Leitner, Gabor Juhos,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Thu, Nov 30, 2017 at 12:26 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 11/29/2017 03:19 PM, Linus Walleij wrote:

>> Or are there in pracice things such that reg is different
>> on the port and the PHY connected to it? Then it makes
>> much sense to put an MDIO bus inside the switch DT
>> node and populate the PHY interrupts from there as you
>> say.
>
> Yes, I have such systems here, Port 0 has its PHY at MDIO address 5 for
> instance.

That explains it.

> switch@0 {
>         compatible = "acme,switch";
>         #address-cells = <1>;
>         #size-cells = <0>;
>
>         ports {
>
>                 port@0 {
>                         reg = <0>;
>                         phy-handle = <&phy0>;
>                 };
>
>                 port@1 {
>                         reg = <1>;
>                         phy-handle = <&phy1>;
>                 };
>
>                 port@8 {
>                         reg = <8>;
>                         ethernet = = <&eth0>;
>                 };
>         };
>
>         mdio {
>                 compatible = "acme,switch-mdio";
>
>                 phy@0 {
>                         reg = <0>;
>                 };
>
>                 phy@1 {
>                         reg = <1>;
>                 };
>         };
> };
>
> That way it's clear which port maps to which PHY, and that the MDIO
> controller is internal within the switch (and so are the PHYs).

So why not:

switch@0 {
        compatible = "acme,switch";
        #address-cells = <1>;
        #size-cells = <0>;

        ports {

                port@0 {
                        reg = <0>;
                        phy@0 {
                             reg = <0>;
                        };
                };

                port@1 {
                        reg = <1>;
                        phy@1 {
                             reg = <1>;
                        };
                };

                port@8 {
                        reg = <8>;
                        ethernet = = <&eth0>;
                };
        };

This avoids the cross-referencing of phandles.

Yours,
Linus Walleii

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] RFC: net: dsa: Add bindings for Realtek SMI DSAs
       [not found]                       ` <CACRpkdYoMVNh8eaTnaDQ59bsh4bC88biLaYSXyhnc4W83PMWzA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-12-04 22:50                         ` Andrew Lunn
  0 siblings, 0 replies; 12+ messages in thread
From: Andrew Lunn @ 2017-12-04 22:50 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Florian Fainelli, Vivien Didelot, netdev-u79uwXL29TY76Z2rM5mHXA,
	Antti Seppälä,
	Roman Yeryomin, Colin Leitner, Gabor Juhos,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

> So why not:
> 
> switch@0 {
>         compatible = "acme,switch";
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         ports {
> 
>                 port@0 {
>                         reg = <0>;
>                         phy@0 {
>                              reg = <0>;
>                         };
>                 };

Hi Linus

So you are suggesting put the PHY node inside the MAC node.

This is sometimes done, but does not describe the hardware. The PHYs
are on an MDIO bus, so device tree should show the MDIO bus and the
PHYs on it.

DSA does have an MDIO bus, so putting the PHYs in the MAC is just
confusing. Although that is not Florians preferred solution, he would
like the DSA driver to export its own MDIO bus and list the PHYs on
it.

	Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-12-04 22:50 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20171105231909.5599-1-linus.walleij@linaro.org>
2017-11-05 23:19 ` [PATCH 3/4] RFC: net: dsa: Add bindings for Realtek SMI DSAs Linus Walleij
2017-11-05 23:48   ` Andrew Lunn
     [not found]     ` <20171105234831.GA24822-g2DYL2Zd6BY@public.gmane.org>
2017-11-29 12:24       ` Linus Walleij
2017-11-29 15:56         ` Andrew Lunn
2017-11-29 21:28           ` Linus Walleij
     [not found]             ` <CACRpkdZVXgFMiHpyUqw7ONYDcq6Htn3rTMRaBJkzd6T3WtX36A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-11-29 21:48               ` Florian Fainelli
2017-11-29 21:56             ` Andrew Lunn
     [not found]               ` <20171129215659.GC1706-g2DYL2Zd6BY@public.gmane.org>
2017-11-29 23:19                 ` Linus Walleij
2017-11-29 23:26                   ` Florian Fainelli
     [not found]                     ` <f9bfa1e1-7f05-1e2b-6663-09d4d3bf6a12-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-11-29 23:36                       ` Andrew Lunn
2017-12-02 12:56                     ` Linus Walleij
     [not found]                       ` <CACRpkdYoMVNh8eaTnaDQ59bsh4bC88biLaYSXyhnc4W83PMWzA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-12-04 22:50                         ` Andrew Lunn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).