| Message ID | 20210628192826.1855132-2-kurt@x64architecture.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Possible Issue Setting the Delay Flags in the Marvell Net PHY Driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | fail | 5 maintainers not CCed: linux@armlinux.org.uk andrew@lunn.ch hkallweit1@gmail.com davem@davemloft.net kuba@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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
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
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
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
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 ?
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
> ?
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
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
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index e6721c1c26c2..a5a9d76b6bab 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -546,7 +546,8 @@ static int m88e1121_config_aneg_rgmii_delays(struct phy_device *phydev) { int mscr; - if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) + if (phydev->interface == PHY_INTERFACE_MODE_RGMII || + phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) mscr = MII_88E1121_PHY_MSCR_RX_DELAY | MII_88E1121_PHY_MSCR_TX_DELAY; else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
This patch changes the default behavior to enable RX and TX delays for the PHY_INTERFACE_MODE_RGMII case by default. Signed-off-by: Kurt Cancemi <kurt@x64architecture.com> --- drivers/net/phy/marvell.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)