| Message ID | 1386350983-13281-8-git-send-email-wens@csie.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/12/6 Chen-Yu Tsai <wens@csie.org>: > The CubieTruck uses the GMAC with an RGMII phy. > > Signed-off-by: Chen-Yu Tsai <wens@csie.org> > --- > arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts > index 8a1009d..af212a2 100644 > --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts > +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts > @@ -33,6 +33,14 @@ > pinctrl-0 = <&uart0_pins_a>; > status = "okay"; > }; > + > + gmac: ethernet@01c50000 { > + pinctrl-names = "default"; > + pinctrl-0 = <&gmac_pins_rgmii>; > + snps,phy-addr = <1>; What is this snps,phy-addr property? Why is not a standard device tree node for an Ethernet PHY node used? > + phy-mode = "rgmii"; Especially since you use the relatively standard "phy-mode" property here? -- Florian
On Sat, Dec 7, 2013 at 5:09 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: > 2013/12/6 Chen-Yu Tsai <wens@csie.org>: >> The CubieTruck uses the GMAC with an RGMII phy. >> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >> --- >> arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >> index 8a1009d..af212a2 100644 >> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >> @@ -33,6 +33,14 @@ >> pinctrl-0 = <&uart0_pins_a>; >> status = "okay"; >> }; >> + >> + gmac: ethernet@01c50000 { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&gmac_pins_rgmii>; >> + snps,phy-addr = <1>; > > What is this snps,phy-addr property? Why is not a standard device tree > node for an Ethernet PHY node used? This property is implemented by stmmac and documented in the DT bindings. stmmac has not been updated to use Ethernet PHY nodes. Removing this property will not affect the function of the driver. The driver probes its MDIO bus and selects the lowest available address if not specified. >> + phy-mode = "rgmii"; > > Especially since you use the relatively standard "phy-mode" property here? Used as specified in stmmac DT bindings. Wens
2013/12/6 Chen-Yu Tsai <wens@csie.org>: > On Sat, Dec 7, 2013 at 5:09 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >> 2013/12/6 Chen-Yu Tsai <wens@csie.org>: >>> The CubieTruck uses the GMAC with an RGMII phy. >>> >>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>> --- >>> arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>> index 8a1009d..af212a2 100644 >>> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>> @@ -33,6 +33,14 @@ >>> pinctrl-0 = <&uart0_pins_a>; >>> status = "okay"; >>> }; >>> + >>> + gmac: ethernet@01c50000 { >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&gmac_pins_rgmii>; >>> + snps,phy-addr = <1>; >> >> What is this snps,phy-addr property? Why is not a standard device tree >> node for an Ethernet PHY node used? > > This property is implemented by stmmac and documented in the DT > bindings. stmmac has not been updated to use Ethernet PHY nodes. This driver property should be removed and deprecated since there is an ePAPR standardized Ethernet PHY node. What I am worried here is the loss of information, the standard Ethernet DT node allows to specify much more information (clause, maximum speed, compatible string etc...). > > Removing this property will not affect the function of the driver. > The driver probes its MDIO bus and selects the lowest available > address if not specified. So if this is just giving the driver a hint on where to probe for a PHY on the MDIO bus, then let's drop it and use the standard DT node no? > >>> + phy-mode = "rgmii"; >> >> Especially since you use the relatively standard "phy-mode" property here? > > Used as specified in stmmac DT bindings. That one is kind of okay because of_get_phy_mode() knows about it, even though ePAPR states this should be "phy-connection-type", so I am okay with keeping that one because current practice and helpers are here.
Florian, Giuseppe: On Sat, Dec 7, 2013 at 9:57 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: > 2013/12/6 Chen-Yu Tsai <wens@csie.org>: >> On Sat, Dec 7, 2013 at 5:09 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >>> 2013/12/6 Chen-Yu Tsai <wens@csie.org>: >>>> The CubieTruck uses the GMAC with an RGMII phy. >>>> >>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>>> --- >>>> arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>>> index 8a1009d..af212a2 100644 >>>> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>>> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>>> @@ -33,6 +33,14 @@ >>>> pinctrl-0 = <&uart0_pins_a>; >>>> status = "okay"; >>>> }; >>>> + >>>> + gmac: ethernet@01c50000 { >>>> + pinctrl-names = "default"; >>>> + pinctrl-0 = <&gmac_pins_rgmii>; >>>> + snps,phy-addr = <1>; >>> >>> What is this snps,phy-addr property? Why is not a standard device tree >>> node for an Ethernet PHY node used? >> >> This property is implemented by stmmac and documented in the DT >> bindings. stmmac has not been updated to use Ethernet PHY nodes. > > This driver property should be removed and deprecated since there is > an ePAPR standardized Ethernet PHY node. What I am worried here is the > loss of information, the standard Ethernet DT node allows to specify > much more information (clause, maximum speed, compatible string > etc...). Giuseppe, any thoughts on this? >> Removing this property will not affect the function of the driver. >> The driver probes its MDIO bus and selects the lowest available >> address if not specified. > > So if this is just giving the driver a hint on where to probe for a > PHY on the MDIO bus, then let's drop it and use the standard DT node > no? Sure. I will remove it from the DT. The stmmac driver does not have a seperate MDIO bus driver, nor does it support Ethernet PHY node bindings. So I will not add a phy node at this moment. >>>> + phy-mode = "rgmii"; >>> >>> Especially since you use the relatively standard "phy-mode" property here? >> >> Used as specified in stmmac DT bindings. > > That one is kind of okay because of_get_phy_mode() knows about it, > even though ePAPR states this should be "phy-connection-type", so I am > okay with keeping that one because current practice and helpers are > here. It seems all ARM DTs currently use "phy-mode". > -- > Florian Chen-Yu
2013/12/8 Chen-Yu Tsai <wens@csie.org>: > Florian, Giuseppe: > > On Sat, Dec 7, 2013 at 9:57 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >> 2013/12/6 Chen-Yu Tsai <wens@csie.org>: >>> On Sat, Dec 7, 2013 at 5:09 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >>>> 2013/12/6 Chen-Yu Tsai <wens@csie.org>: >>>>> The CubieTruck uses the GMAC with an RGMII phy. >>>>> >>>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>>>> --- >>>>> arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++ >>>>> 1 file changed, 8 insertions(+) >>>>> >>>>> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>>>> index 8a1009d..af212a2 100644 >>>>> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>>>> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>>>> @@ -33,6 +33,14 @@ >>>>> pinctrl-0 = <&uart0_pins_a>; >>>>> status = "okay"; >>>>> }; >>>>> + >>>>> + gmac: ethernet@01c50000 { >>>>> + pinctrl-names = "default"; >>>>> + pinctrl-0 = <&gmac_pins_rgmii>; >>>>> + snps,phy-addr = <1>; >>>> >>>> What is this snps,phy-addr property? Why is not a standard device tree >>>> node for an Ethernet PHY node used? >>> >>> This property is implemented by stmmac and documented in the DT >>> bindings. stmmac has not been updated to use Ethernet PHY nodes. >> >> This driver property should be removed and deprecated since there is >> an ePAPR standardized Ethernet PHY node. What I am worried here is the >> loss of information, the standard Ethernet DT node allows to specify >> much more information (clause, maximum speed, compatible string >> etc...). > > Giuseppe, any thoughts on this? > >>> Removing this property will not affect the function of the driver. >>> The driver probes its MDIO bus and selects the lowest available >>> address if not specified. >> >> So if this is just giving the driver a hint on where to probe for a >> PHY on the MDIO bus, then let's drop it and use the standard DT node >> no? > > Sure. I will remove it from the DT. > > The stmmac driver does not have a seperate MDIO bus driver, nor > does it support Ethernet PHY node bindings. So I will not add > a phy node at this moment. This will create needless churn in the DT if you do not do it now, worse actually, we switch from a DT which specifically described Ethernet PHY nodes properly to a version where it does, to ultimately a newer version which does. Considering that the absence of a "snps,phy-addr" property will still result in the MDIO bus to be probed, keeping the existing Ethernet PHY nodes, referencing them correclty with a "phy-handle" property, but having no explicit support for these in the driver will not result in a functional change, and will reduce the DT churn. Also, you could still sneak a patch in this patchset which parses the standard EThernet PHY node binding. > >>>>> + phy-mode = "rgmii"; >>>> >>>> Especially since you use the relatively standard "phy-mode" property here? >>> >>> Used as specified in stmmac DT bindings. >> >> That one is kind of okay because of_get_phy_mode() knows about it, >> even though ePAPR states this should be "phy-connection-type", so I am >> okay with keeping that one because current practice and helpers are >> here. > > It seems all ARM DTs currently use "phy-mode". Which is why I said "is kind of okay", but this is not what the standard came up with, ARM is not exactly the best DT-citizen out there as people are prone to making up new properties when the standard already defines some, anyway.... -- Florian
On Tue, Dec 10, 2013 at 1:48 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: > 2013/12/8 Chen-Yu Tsai <wens@csie.org>: >> Florian, Giuseppe: >> >> On Sat, Dec 7, 2013 at 9:57 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >>> 2013/12/6 Chen-Yu Tsai <wens@csie.org>: >>>> On Sat, Dec 7, 2013 at 5:09 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >>>>> 2013/12/6 Chen-Yu Tsai <wens@csie.org>: >>>>>> The CubieTruck uses the GMAC with an RGMII phy. >>>>>> >>>>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>>>>> --- >>>>>> arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++ >>>>>> 1 file changed, 8 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>>>>> index 8a1009d..af212a2 100644 >>>>>> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>>>>> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>>>>> @@ -33,6 +33,14 @@ >>>>>> pinctrl-0 = <&uart0_pins_a>; >>>>>> status = "okay"; >>>>>> }; >>>>>> + >>>>>> + gmac: ethernet@01c50000 { >>>>>> + pinctrl-names = "default"; >>>>>> + pinctrl-0 = <&gmac_pins_rgmii>; >>>>>> + snps,phy-addr = <1>; >>>>> >>>>> What is this snps,phy-addr property? Why is not a standard device tree >>>>> node for an Ethernet PHY node used? >>>> >>>> This property is implemented by stmmac and documented in the DT >>>> bindings. stmmac has not been updated to use Ethernet PHY nodes. >>> >>> This driver property should be removed and deprecated since there is >>> an ePAPR standardized Ethernet PHY node. What I am worried here is the >>> loss of information, the standard Ethernet DT node allows to specify >>> much more information (clause, maximum speed, compatible string >>> etc...). >> >> Giuseppe, any thoughts on this? >> >>>> Removing this property will not affect the function of the driver. >>>> The driver probes its MDIO bus and selects the lowest available >>>> address if not specified. >>> >>> So if this is just giving the driver a hint on where to probe for a >>> PHY on the MDIO bus, then let's drop it and use the standard DT node >>> no? >> >> Sure. I will remove it from the DT. >> >> The stmmac driver does not have a seperate MDIO bus driver, nor >> does it support Ethernet PHY node bindings. So I will not add >> a phy node at this moment. > > This will create needless churn in the DT if you do not do it now, > worse actually, we switch from a DT which specifically described > Ethernet PHY nodes properly to a version where it does, to ultimately > a newer version which does. > > Considering that the absence of a "snps,phy-addr" property will still > result in the MDIO bus to be probed, keeping the existing Ethernet PHY > nodes, referencing them correclty with a "phy-handle" property, but > having no explicit support for these in the driver will not result in > a functional change, and will reduce the DT churn. Also, you could > still sneak a patch in this patchset which parses the standard > EThernet PHY node binding. I see. Adding the PHY node should be no trouble at all. Is there a requirement for a seperate mdio bus in the DT? I do not see it in the ePAPR. The stmmac mdio is not a seperate driver, as is for most network controllers I presume. Would attaching the PHY nodes directly under the ethernet controller suffice? We can then introduce PHY node support to the driver and remove the "snps,phy-addr" property at the same time. ChenYu >> >>>>>> + phy-mode = "rgmii"; >>>>> >>>>> Especially since you use the relatively standard "phy-mode" property here? >>>> >>>> Used as specified in stmmac DT bindings. >>> >>> That one is kind of okay because of_get_phy_mode() knows about it, >>> even though ePAPR states this should be "phy-connection-type", so I am >>> okay with keeping that one because current practice and helpers are >>> here. >> >> It seems all ARM DTs currently use "phy-mode". > > Which is why I said "is kind of okay", but this is not what the > standard came up with, ARM is not exactly the best DT-citizen out > there as people are prone to making up new properties when the > standard already defines some, anyway.... > -- > Florian
2013/12/9 Chen-Yu Tsai <wens@csie.org>: > On Tue, Dec 10, 2013 at 1:48 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >> 2013/12/8 Chen-Yu Tsai <wens@csie.org>: >>> Florian, Giuseppe: >>> >>> On Sat, Dec 7, 2013 at 9:57 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >>>> 2013/12/6 Chen-Yu Tsai <wens@csie.org>: >>>>> On Sat, Dec 7, 2013 at 5:09 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >>>>>> 2013/12/6 Chen-Yu Tsai <wens@csie.org>: >>>>>>> The CubieTruck uses the GMAC with an RGMII phy. >>>>>>> >>>>>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>>>>>> --- >>>>>>> arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++ >>>>>>> 1 file changed, 8 insertions(+) >>>>>>> >>>>>>> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>>>>>> index 8a1009d..af212a2 100644 >>>>>>> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>>>>>> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>>>>>> @@ -33,6 +33,14 @@ >>>>>>> pinctrl-0 = <&uart0_pins_a>; >>>>>>> status = "okay"; >>>>>>> }; >>>>>>> + >>>>>>> + gmac: ethernet@01c50000 { >>>>>>> + pinctrl-names = "default"; >>>>>>> + pinctrl-0 = <&gmac_pins_rgmii>; >>>>>>> + snps,phy-addr = <1>; >>>>>> >>>>>> What is this snps,phy-addr property? Why is not a standard device tree >>>>>> node for an Ethernet PHY node used? >>>>> >>>>> This property is implemented by stmmac and documented in the DT >>>>> bindings. stmmac has not been updated to use Ethernet PHY nodes. >>>> >>>> This driver property should be removed and deprecated since there is >>>> an ePAPR standardized Ethernet PHY node. What I am worried here is the >>>> loss of information, the standard Ethernet DT node allows to specify >>>> much more information (clause, maximum speed, compatible string >>>> etc...). >>> >>> Giuseppe, any thoughts on this? >>> >>>>> Removing this property will not affect the function of the driver. >>>>> The driver probes its MDIO bus and selects the lowest available >>>>> address if not specified. >>>> >>>> So if this is just giving the driver a hint on where to probe for a >>>> PHY on the MDIO bus, then let's drop it and use the standard DT node >>>> no? >>> >>> Sure. I will remove it from the DT. >>> >>> The stmmac driver does not have a seperate MDIO bus driver, nor >>> does it support Ethernet PHY node bindings. So I will not add >>> a phy node at this moment. >> >> This will create needless churn in the DT if you do not do it now, >> worse actually, we switch from a DT which specifically described >> Ethernet PHY nodes properly to a version where it does, to ultimately >> a newer version which does. >> >> Considering that the absence of a "snps,phy-addr" property will still >> result in the MDIO bus to be probed, keeping the existing Ethernet PHY >> nodes, referencing them correclty with a "phy-handle" property, but >> having no explicit support for these in the driver will not result in >> a functional change, and will reduce the DT churn. Also, you could >> still sneak a patch in this patchset which parses the standard >> EThernet PHY node binding. > > I see. Adding the PHY node should be no trouble at all. > Is there a requirement for a seperate mdio bus in the DT? Not really no, in that case the MDIO bus is implicitely implemented as part of the Ethernet MAC controller, this is just fine. > I do not see it in the ePAPR. The stmmac mdio is not a seperate > driver, as is for most network controllers I presume. > Would attaching the PHY nodes directly under the ethernet controller > suffice? Sure, that is what most Ethernet driver do actually. They register their own MDIO bus and make sure PHYs get attached to it. > > We can then introduce PHY node support to the driver and > remove the "snps,phy-addr" property at the same time. Sure, sounds good, thanks!
On 12/7/2013 2:57 AM, Florian Fainelli wrote: > 2013/12/6 Chen-Yu Tsai <wens@csie.org>: >> On Sat, Dec 7, 2013 at 5:09 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >>> 2013/12/6 Chen-Yu Tsai <wens@csie.org>: >>>> The CubieTruck uses the GMAC with an RGMII phy. >>>> >>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>>> --- >>>> arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>>> index 8a1009d..af212a2 100644 >>>> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>>> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts >>>> @@ -33,6 +33,14 @@ >>>> pinctrl-0 = <&uart0_pins_a>; >>>> status = "okay"; >>>> }; >>>> + >>>> + gmac: ethernet@01c50000 { >>>> + pinctrl-names = "default"; >>>> + pinctrl-0 = <&gmac_pins_rgmii>; >>>> + snps,phy-addr = <1>; >>> >>> What is this snps,phy-addr property? Why is not a standard device tree >>> node for an Ethernet PHY node used? >> >> This property is implemented by stmmac and documented in the DT >> bindings. stmmac has not been updated to use Ethernet PHY nodes. > > This driver property should be removed and deprecated since there is > an ePAPR standardized Ethernet PHY node. What I am worried here is the > loss of information, the standard Ethernet DT node allows to specify > much more information (clause, maximum speed, compatible string > etc...). yes Florian you are right and I have already started updating the driver. I do not sure that I'll be able to send all the patches soon but for sure I can re-base them. >> >> Removing this property will not affect the function of the driver. >> The driver probes its MDIO bus and selects the lowest available >> address if not specified. > > So if this is just giving the driver a hint on where to probe for a > PHY on the MDIO bus, then let's drop it and use the standard DT node > no? > >> >>>> + phy-mode = "rgmii"; >>> >>> Especially since you use the relatively standard "phy-mode" property here? >> >> Used as specified in stmmac DT bindings. > > That one is kind of okay because of_get_phy_mode() knows about it, > even though ePAPR states this should be "phy-connection-type", so I am > okay with keeping that one because current practice and helpers are > here. >
diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts index 8a1009d..af212a2 100644 --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts @@ -33,6 +33,14 @@ pinctrl-0 = <&uart0_pins_a>; status = "okay"; }; + + gmac: ethernet@01c50000 { + pinctrl-names = "default"; + pinctrl-0 = <&gmac_pins_rgmii>; + snps,phy-addr = <1>; + phy-mode = "rgmii"; + status = "okay"; + }; }; leds {
The CubieTruck uses the GMAC with an RGMII phy. Signed-off-by: Chen-Yu Tsai <wens@csie.org> --- arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++ 1 file changed, 8 insertions(+)