| Message ID | 20170605200107.7692-1-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> +&cps_eth1 { > + status = "okay"; > + phy = <&phy0>; > + phy-mode = "sgmii"; > +}; Hi Marc Documentation/devicetree/bindings/net/ethernet.txt says: - phy-handle: phandle, specifies a reference to a node representing a PHY device; this property is described in ePAPR and so preferred; - phy: the same as "phy-handle" property, not recommended for new bindings. Andrew
Hi Andrew, On 05/06/17 22:16, Andrew Lunn wrote: >> +&cps_eth1 { >> + status = "okay"; >> + phy = <&phy0>; >> + phy-mode = "sgmii"; >> +}; > > Hi Marc > > Documentation/devicetree/bindings/net/ethernet.txt says: > > - phy-handle: phandle, specifies a reference to a node representing a PHY > device; this property is described in ePAPR and so preferred; > - phy: the same as "phy-handle" property, not recommended for new bindings. Happy to amend the patch, though armada-8040-db.dts is already using "phy" (and not "phy-handle"). Should that one be fixed as well in the name of consistency? Also, Documentation/devicetree/bindings/net/marvell-pp2.txt only mentions the "phy" property. If feels a bit odd not to at least recommend the new property in the binding documentation. Thanks, M.
Hello, On Mon, 5 Jun 2017 21:01:07 +0100, Marc Zyngier wrote: > Enable the 1GB Ethernet interface that lives on the slave CP110, > with its corresponding phy (that oddly lives on the master CP110). > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> This patch is somewhat redundant with https://patchwork.kernel.org/patch/9728207/, which despite the flaming discussion that followed, does the same thing (and more). Best regards, Thomas
Hello, On Tue, 6 Jun 2017 11:02:34 +0200, Thomas Petazzoni wrote: > This patch is somewhat redundant with > https://patchwork.kernel.org/patch/9728207/, which despite the flaming > discussion that followed, does the same thing (and more). That being said, the patch from Marcin was also enabling SDHCI, which has already been enabled in the mean time by a patch from Russell. So perhaps we should indeed keep Marc's patch, enabling just the 1GB Ethernet interface. Best regards, Thomas
On Tue, Jun 06 2017 at 11:02:34 am BST, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Hello, > > On Mon, 5 Jun 2017 21:01:07 +0100, Marc Zyngier wrote: >> Enable the 1GB Ethernet interface that lives on the slave CP110, >> with its corresponding phy (that oddly lives on the master CP110). >> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > This patch is somewhat redundant with > https://patchwork.kernel.org/patch/9728207/, which despite the flaming > discussion that followed, does the same thing (and more). It's great that there is a patch for that already. How far is that from being merged (given how much controversy seem to surround this)? Thanks,
On Tue, Jun 06 2017 at 11:06:03 am BST, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Hello, > > On Tue, 6 Jun 2017 11:02:34 +0200, Thomas Petazzoni wrote: > >> This patch is somewhat redundant with >> https://patchwork.kernel.org/patch/9728207/, which despite the flaming >> discussion that followed, does the same thing (and more). > > That being said, the patch from Marcin was also enabling SDHCI, which > has already been enabled in the mean time by a patch from Russell. > > So perhaps we should indeed keep Marc's patch, enabling just the 1GB > Ethernet interface. Whichever patch we merge, Andrew's comment remains. Should we stick with "phy", or switch to "phy-handle" everywhere? Thanks, M.
Hello, On Tue, 06 Jun 2017 10:16:08 +0100, Marc Zyngier wrote: > > This patch is somewhat redundant with > > https://patchwork.kernel.org/patch/9728207/, which despite the flaming > > discussion that followed, does the same thing (and more). > > It's great that there is a patch for that already. How far is that from > being merged (given how much controversy seem to surround this)? As I said in my second reply, I believe a v2 of your patch would in fact be better. Marcin's patch is no longer applicable, and was doing too many things at once. So let's merge your patch instead. Best regards, Thomas
Hello, On Tue, 06 Jun 2017 10:18:11 +0100, Marc Zyngier wrote: > > So perhaps we should indeed keep Marc's patch, enabling just the 1GB > > Ethernet interface. > > Whichever patch we merge, Andrew's comment remains. Should we stick with > "phy", or switch to "phy-handle" everywhere? Since the binding says to use phy-handle, we should use phy-handle for your patch. Of course, progressively, we should migrate all our bindings to this new property name, but I don't think this should be a prerequisite for your patch to go in. Side note: I don't understand this change from "phy" to "phy-handle". It really doesn't make sense to me to encode the type of the property in the name of the property. Are we going to use: phy-mode-string = "sgmii"; pinctrl-0-handle = <&foo>; reg-u32s = <0xdeadbeef 0xbadcafe>; is there a pointer to the reasoning for this change? Best regards, Thomas
On Tue, Jun 06, 2017 at 11:36:26AM +0200, Thomas Petazzoni wrote: > Side note: I don't understand this change from "phy" to "phy-handle". > It really doesn't make sense to me to encode the type of the property > in the name of the property. Are we going to use: ePAPR 1.1 has specified phy-handle, which as the binding document says is the preferred version (probably because it's in ePAPR as the official name of the property.) We've ended up with "phy" and "phy-handle" because there was never a central binding document for ethernet devices until 2014, so drivers randomly picked between the two property names. Which one should be used is driver dependent, since drivers parse this property. Many drivers parse just one of the names and not the other, so you have to use the right one for the driver.
On Tue, Jun 06, 2017 at 09:21:58AM +0100, Marc Zyngier wrote: > Hi Andrew, > > On 05/06/17 22:16, Andrew Lunn wrote: > >> +&cps_eth1 { > >> + status = "okay"; > >> + phy = <&phy0>; > >> + phy-mode = "sgmii"; > >> +}; > > > > Hi Marc > > > > Documentation/devicetree/bindings/net/ethernet.txt says: > > > > - phy-handle: phandle, specifies a reference to a node representing a PHY > > device; this property is described in ePAPR and so preferred; > > - phy: the same as "phy-handle" property, not recommended for new bindings. > > Happy to amend the patch, though armada-8040-db.dts is already using > "phy" (and not "phy-handle"). Should that one be fixed as well in the > name of consistency? Also, > Documentation/devicetree/bindings/net/marvell-pp2.txt only mentions the > "phy" property. If feels a bit odd not to at least recommend the new > property in the binding documentation. Ah. As Russell pointed out, parsing this property is done by the driver, not a central helper. mvpp2 only supports phy, not phy-handle. This is not a new binding, it has been around since the middle of 2014. So "phy" is O.K. Sometime in the future, it would be nice to centralise the parsing of this and phy-mode. Andrew
Hello, On Mon, 5 Jun 2017 21:01:07 +0100, Marc Zyngier wrote: > Enable the 1GB Ethernet interface that lives on the slave CP110, > with its corresponding phy (that oddly lives on the master CP110). > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
On Mon, Jun 05, 2017 at 09:01:07PM +0100, Marc Zyngier wrote: > Enable the 1GB Ethernet interface that lives on the slave CP110, > with its corresponding phy (that oddly lives on the master CP110). > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts > index f7bb0cc03147..1eb3cd5332c1 100644 > --- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts > +++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts > @@ -100,6 +100,12 @@ > status = "okay"; > }; > > +&cpm_mdio { > + phy0: ethernet-phy@0 { As the board has multiple phys, can we use a more descriptive label here, such as "ge_phy" (for gigabit ethernet phy) to distinugish it from the 10G PHYs? > +&cps_ethernet { > + status = "okay"; > +}; > + > +&cps_eth1 { Labelling up the connector as is done elsewhere in the file would be nice: /* CPS Lane 0 - J5 (Gigabit RJ45) */ > + status = "okay"; > + phy = <&phy0>; > + phy-mode = "sgmii";
On 06/06/17 14:34, Russell King - ARM Linux wrote: > On Mon, Jun 05, 2017 at 09:01:07PM +0100, Marc Zyngier wrote: >> Enable the 1GB Ethernet interface that lives on the slave CP110, >> with its corresponding phy (that oddly lives on the master CP110). >> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts >> index f7bb0cc03147..1eb3cd5332c1 100644 >> --- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts >> +++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts >> @@ -100,6 +100,12 @@ >> status = "okay"; >> }; >> >> +&cpm_mdio { >> + phy0: ethernet-phy@0 { > > As the board has multiple phys, can we use a more descriptive label > here, such as "ge_phy" (for gigabit ethernet phy) to distinugish it > from the 10G PHYs? > >> +&cps_ethernet { >> + status = "okay"; >> +}; >> + >> +&cps_eth1 { > > Labelling up the connector as is done elsewhere in the file would be > nice: > > /* CPS Lane 0 - J5 (Gigabit RJ45) */ > > >> + status = "okay"; >> + phy = <&phy0>; >> + phy-mode = "sgmii"; > Yup, agreed on both counts. I'll update the patch. Thanks, M.
diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts index f7bb0cc03147..1eb3cd5332c1 100644 --- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts +++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts @@ -100,6 +100,12 @@ status = "okay"; }; +&cpm_mdio { + phy0: ethernet-phy@0 { + reg = <0>; + }; +}; + &cpm_sata0 { /* CPM Lane 0 - U29 */ status = "okay"; @@ -115,6 +121,16 @@ status = "okay"; }; +&cps_ethernet { + status = "okay"; +}; + +&cps_eth1 { + status = "okay"; + phy = <&phy0>; + phy-mode = "sgmii"; +}; + &cps_sata0 { /* CPS Lane 1 - U32 */ /* CPS Lane 3 - U31 */
Enable the 1GB Ethernet interface that lives on the slave CP110, with its corresponding phy (that oddly lives on the master CP110). Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)