mbox series

[0/1] Possible Issue Setting the Delay Flags in the Marvell Net PHY Driver

Message ID 20210628192826.1855132-1-kurt@x64architecture.com
Headers show
Series Possible Issue Setting the Delay Flags in the Marvell Net PHY Driver | expand

Message

Kurt Cancemi June 28, 2021, 7:28 p.m. UTC 
Hi,

I believe there is an issue setting the RX and TX delay flags in the Marvell
net PHY driver. This patch fixes the issue for me but I am not convinced that
this is the right way to fix the issue or that this patch will not cause side
effects for other models. Feedback and comments are greatly appreciated.

Backstory:

I have been troubleshooting getting ethernet to work on a board based off of
the NXP T2080RDB (with DPAA ethernet). It has a Marvell 88E1510 PHY chip.
When attempting to use ping to verify that the ethernet was working I was
only getting RX and TX errors. Upon further debugging I discovered that the
RX and TX delay flags were not being set.

I believe there is an issue because of the following:

* The DPAA memac driver correctly reports that the device tree ethernet
  "phy-connection-type" is set to "rgmii-id" and the of_get_phy_mode()
  function correctly returns 0x8 "PHY_INTERFACE_MODE_RGMII_ID"

* A similar fix for this same issue was incorporated into U-Boot back in 2018:
  https://github.com/u-boot/u-boot/commit/431be621c6cbc72efd1d45fa36686a682cbb470a

* The ethernet works with the attached patch.

Kurt

Kurt Cancemi (1):
  net: phy: marvell: Fixed handing of delays with plain RGMII interface

 drivers/net/phy/marvell.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Marek Behún June 28, 2021, 10:49 p.m. UTC | #1 
On Mon, 28 Jun 2021 15:28:26 -0400
Kurt Cancemi <kurt@x64architecture.com> wrote:

> This patch changes the default behavior to enable RX and TX delays for
> the PHY_INTERFACE_MODE_RGMII case by default.

And why would we want that? I was under the impression that we have the
_ID variants for enabling these delays.

Marek
Kurt Cancemi June 29, 2021, 12:05 a.m. UTC | #2 
Well I'm sorry for the noise I was running into a lot of other DPAA
ethernet related issues and overlooked adding phy-mode = "rgmii-id";
that fixed the issue. I knew my patch was not correct (as I explained
in the cover email) but I was not sure why it was necessary but now I
see it was not necessary I just had "phy-connection-mode" instead of
"phy-mode".

May I ask what is the purpose of phy-connection-mode? And why did the
DPAA driver recognize the PHY interface mode as RGMII ID but the
Marvell PHY driver didn't?

On Mon, Jun 28, 2021 at 7:01 PM Marcin Wojtas <mw@semihalf.com> wrote:
>

> Hi Kurt,

>

>

> wt., 29 cze 2021 o 00:50 Marek Behún <kabel@kernel.org> napisał(a):

> >

> > On Mon, 28 Jun 2021 15:28:26 -0400

> > Kurt Cancemi <kurt@x64architecture.com> wrote:

> >

> > > This patch changes the default behavior to enable RX and TX delays for

> > > the PHY_INTERFACE_MODE_RGMII case by default.

> >

> > And why would we want that? I was under the impression that we have the

> > _ID variants for enabling these delays.

> >

>

> +1

>

> From Documentation/devicetree/bindings/net/ethernet-controller.yaml

>

>       # RGMII with internal RX and TX delays provided by the PHY,

>       # the MAC should not add the RX or TX delays in this case

>       - rgmii-id

>

> I guess you should rather fix the hardware description of your board,

> i.e. use `phy-mode = "rgmii-id"` instead of tweaking the PHY driver

> itself.

>

> Best regards,

> Marcin
Marek Behún June 29, 2021, 12:21 a.m. UTC | #3 
On Mon, 28 Jun 2021 20:05:19 -0400
Kurt Cancemi <kurt@x64architecture.com> wrote:

> Well I'm sorry for the noise I was running into a lot of other DPAA

> ethernet related issues and overlooked adding phy-mode = "rgmii-id";

> that fixed the issue. I knew my patch was not correct (as I explained

> in the cover email) but I was not sure why it was necessary but now I

> see it was not necessary I just had "phy-connection-mode" instead of

> "phy-mode".

> 

> May I ask what is the purpose of phy-connection-mode? And why did the


phy-connection-type, not mode

> DPAA driver recognize the PHY interface mode as RGMII ID but the

> Marvell PHY driver didn't?


phy-mode and phy-connection-type are synonyms. phy-mode takes
precedence. Look at drivers/of/of_net.c function of_get_phy_mode().

If your device tree declares both, it can lead to confusion. For
example if dtsi file says
  phy-mode = "rgmii";
and you include this dtsi file but than you say
  phy-connection-type = "rgmii-id";
the kernel code will use rgmii, not rgmii-id, because phy-mode takes
precedence over phy-connection-type.

Marek
Marek Behún June 29, 2021, 12:23 a.m. UTC | #4 
On Mon, 28 Jun 2021 19:09:49 -0400
Kurt Cancemi <kurt@x64architecture.com> wrote:

> I’m sorry if I proposed this wrong (I am new to the kernel mailing list), I

> acknowledge that this is probably not the way to fix the problem, I wanted

> to discuss why my fix is necessary. In the cover email I explained that I

> used (in the device tree) “rgmii-id” for the “phy-connection-type” and the

> DPAA memac correctly reports that the PHY type is “PHY_INTERFACE_MODE_RGMII_ID”

> but without my patch the RX and TX delay flags are not set on the

> underlying Marvell PHY and I receive RX and TX errors on every network

> request. Maybe there is some type of incompatibility between the Freescale

> DPAA1 Ethernet driver and the Marvell PHY?


Which driver again?
  drivers/net/ethernet/freescale/dpaa
or
  drivers/net/ethernet/freescale/dpaa2
?
Kurt Cancemi June 29, 2021, 1:12 a.m. UTC | #5 
I am using drivers/net/ethernet/freescale/dpaa. This is a T2080 soc.

The following is where I added a dev_info statement for "phy_if", it
correctly returned -> PHY_INTERFACE_MODE_RGMII_ID.
https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/freescale/fman/mac.c#L774

I will clarify things better, I am using an almost verbatim device
tree "arch/powerpc/boot/dts/fsl/t2080rdb.dts" the only changes I made
are the following:

Doesn't Work (RX and TX errors on every packet):
--- t2080rdb.dts    2021-06-28 21:08:06.571758700 -0400
+++ t2080rdb-doesnt_work.dts    2021-06-28 21:08:10.704915200 -0400
@@ -68,7 +68,7 @@

         ethernet@e4000 {
             phy-handle = <&rgmii_phy1>;
-            phy-connection-type = "rgmii";
+            phy-connection-type = "rgmii-id";
         };

         ethernet@e6000 {

Works (Your suggestion):
--- t2080rdb.dts    2021-06-28 21:08:06.571758700 -0400
+++ t2080rdb-works.dts    2021-06-28 21:09:49.415971900 -0400
@@ -68,7 +68,7 @@

         ethernet@e4000 {
             phy-handle = <&rgmii_phy1>;
-            phy-connection-type = "rgmii";
+            phy-mode = "rgmii-id";
         };

         ethernet@e6000 {

On Mon, Jun 28, 2021 at 8:23 PM Marek Behún <kabel@kernel.org> wrote:
>

> On Mon, 28 Jun 2021 19:09:49 -0400

> Kurt Cancemi <kurt@x64architecture.com> wrote:

>

> > I’m sorry if I proposed this wrong (I am new to the kernel mailing list), I

> > acknowledge that this is probably not the way to fix the problem, I wanted

> > to discuss why my fix is necessary. In the cover email I explained that I

> > used (in the device tree) “rgmii-id” for the “phy-connection-type” and the

> > DPAA memac correctly reports that the PHY type is “PHY_INTERFACE_MODE_RGMII_ID”

> > but without my patch the RX and TX delay flags are not set on the

> > underlying Marvell PHY and I receive RX and TX errors on every network

> > request. Maybe there is some type of incompatibility between the Freescale

> > DPAA1 Ethernet driver and the Marvell PHY?

>

> Which driver again?

>   drivers/net/ethernet/freescale/dpaa

> or

>   drivers/net/ethernet/freescale/dpaa2

> ?
Marek Behún June 29, 2021, 10:52 a.m. UTC | #6 
On Mon, 28 Jun 2021 21:12:41 -0400
Kurt Cancemi <kurt@x64architecture.com> wrote:

> I am using drivers/net/ethernet/freescale/dpaa. This is a T2080 soc.

> 

> The following is where I added a dev_info statement for "phy_if", it

> correctly returned -> PHY_INTERFACE_MODE_RGMII_ID.

> https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/freescale/fman/mac.c#L774


It seem that dpaa / fman drivers do the same thing for both
"rgmii" and "rgmii-id". There should be code that enables the delays
for the "rgmii-id" variant...

Marek
Andrew Lunn June 29, 2021, 3:08 p.m. UTC | #7 
On Tue, Jun 29, 2021 at 12:52:34PM +0200, Marek Behún wrote:
> On Mon, 28 Jun 2021 21:12:41 -0400

> Kurt Cancemi <kurt@x64architecture.com> wrote:

> 

> > I am using drivers/net/ethernet/freescale/dpaa. This is a T2080 soc.

> > 

> > The following is where I added a dev_info statement for "phy_if", it

> > correctly returned -> PHY_INTERFACE_MODE_RGMII_ID.

> > https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/freescale/fman/mac.c#L774

> 

> It seem that dpaa / fman drivers do the same thing for both

> "rgmii" and "rgmii-id". There should be code that enables the delays

> for the "rgmii-id" variant...


Generally, the MAC does nothing, and asks the PHY to do the delay. So
in the MAC driver there should be nothing, apart from pass phy-mode to
the PHY.

    Andrew