| Message ID | HK0PR01MB3521C806FE109E04FA72858CFA3F0@HK0PR01MB3521.apcprd01.prod.exchangelabs.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | gianfar: Solve ethernet TX/RX problems for ls1021a | expand |
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c > index 72868a28b621..ab4e45199df9 100644 > --- a/drivers/net/ethernet/freescale/gianfar.c > +++ b/drivers/net/ethernet/freescale/gianfar.c > @@ -833,6 +833,7 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev) > > /* Find the TBI PHY. If it's not there, we don't support SGMII */ > priv->tbi_node = of_parse_phandle(np, "tbi-handle", 0); > + priv->dma_endian_le = of_property_read_bool(np, "fsl,dma-endian-le"); Hi Johnson You need to document this new property in the binding. Thanks Andrew
On Tue, Jan 7, 2020 at 11:13 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c > > index 72868a28b621..ab4e45199df9 100644 > > --- a/drivers/net/ethernet/freescale/gianfar.c > > +++ b/drivers/net/ethernet/freescale/gianfar.c > > @@ -833,6 +833,7 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev) > > > > /* Find the TBI PHY. If it's not there, we don't support SGMII */ > > priv->tbi_node = of_parse_phandle(np, "tbi-handle", 0); > > + priv->dma_endian_le = of_property_read_bool(np, "fsl,dma-endian-le"); > > Hi Johnson > > You need to document this new property in the binding. Yes, but what about calling it 'little-endian' which is commonly used in arch/arm64/boot/dts/freescale/fsl-lsxxx device trees?
Hi Chen, On Tue, 7 Jan 2020 at 12:37, Johnson CH Chen (陳昭勳) <JohnsonCH.Chen@moxa.com> wrote: > > Add dma_endian_le to solve ethernet TX/RX problems for freescale ls1021a. Without this, it will result in > rx-busy-errors by ethtool, and transmit queue timeout in ls1021a's platforms. > > Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com> > --- This patch is not valid. The endianness configuration in eTSECx_DMACTRL is reserved and not applicable. What is the value of SCFG_ETSECDMAMCR bits ETSEC_BD and ETSEC_FR_DATA on your board? Typically this is configured by the bootloader. > drivers/net/ethernet/freescale/gianfar.c | 3 +++ > drivers/net/ethernet/freescale/gianfar.h | 4 ++++ > 2 files changed, 7 insertions(+) > > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c > index 72868a28b621..ab4e45199df9 100644 > --- a/drivers/net/ethernet/freescale/gianfar.c > +++ b/drivers/net/ethernet/freescale/gianfar.c > @@ -833,6 +833,7 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev) > > /* Find the TBI PHY. If it's not there, we don't support SGMII */ > priv->tbi_node = of_parse_phandle(np, "tbi-handle", 0); > + priv->dma_endian_le = of_property_read_bool(np, "fsl,dma-endian-le"); > > return 0; > > @@ -1209,6 +1210,8 @@ static void gfar_start(struct gfar_private *priv) > /* Initialize DMACTRL to have WWR and WOP */ > tempval = gfar_read(®s->dmactrl); > tempval |= DMACTRL_INIT_SETTINGS; > + if (priv->dma_endian_le) > + tempval |= DMACTRL_LE; > gfar_write(®s->dmactrl, tempval); > > /* Make sure we aren't stopped */ > diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h > index 432c6a818ae5..aae07db5206b 100644 > --- a/drivers/net/ethernet/freescale/gianfar.h > +++ b/drivers/net/ethernet/freescale/gianfar.h > @@ -215,6 +215,7 @@ extern const char gfar_driver_version[]; > #define DMACTRL_INIT_SETTINGS 0x000000c3 > #define DMACTRL_GRS 0x00000010 > #define DMACTRL_GTS 0x00000008 > +#define DMACTRL_LE 0x00008000 > > #define TSTAT_CLEAR_THALT_ALL 0xFF000000 > #define TSTAT_CLEAR_THALT 0x80000000 > @@ -1140,6 +1141,9 @@ struct gfar_private { > tx_pause_en:1, > rx_pause_en:1; > > + /* little endian dma buffer and descriptor host interface */ > + unsigned int dma_endian_le; > + > /* The total tx and rx ring size for the enabled queues */ > unsigned int total_tx_ring_size; > unsigned int total_rx_ring_size; > -- > 2.11.0 > > Best regards, > Johnson Regards, -Vladimir
Hi, Fabio Estevam <festevam@gmail.com> 於 2020年1月7日 週二 下午10:17寫道： > > On Tue, Jan 7, 2020 at 11:13 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > diff --git a/drivers/net/ethernet/freescale/gianfar.c > > > b/drivers/net/ethernet/freescale/gianfar.c > > > index 72868a28b621..ab4e45199df9 100644 > > > --- a/drivers/net/ethernet/freescale/gianfar.c > > > +++ b/drivers/net/ethernet/freescale/gianfar.c > > > @@ -833,6 +833,7 @@ static int gfar_of_init(struct platform_device > > > *ofdev, struct net_device **pdev) > > > > > > /* Find the TBI PHY. If it's not there, we don't support SGMII */ > > > priv->tbi_node = of_parse_phandle(np, "tbi-handle", 0); > > > + priv->dma_endian_le = of_property_read_bool(np, > > > + "fsl,dma-endian-le"); > > > > Hi Johnson > > > > You need to document this new property in the binding. Thanks yuor remind, I'll take care of it later > > Yes, but what about calling it 'little-endian' which is commonly used > in arch/arm64/boot/dts/freescale/fsl-lsxxx device trees? It sounds good, use "dma-endian-le" because it's from freescale's SDK for arm (32bit). Thanks your suggestions! Best regards, Johnson
Hi Vladimir, Vladimir Oltean <olteanv@gmail.com> 於 2020年1月7日 週二 下午11:49寫道： > > Hi Chen, > > On Tue, 7 Jan 2020 at 12:37, Johnson CH Chen (陳昭勳) > <JohnsonCH.Chen@moxa.com> wrote: > > > > Add dma_endian_le to solve ethernet TX/RX problems for freescale > > ls1021a. Without this, it will result in rx-busy-errors by ethtool, and transmit queue timeout in ls1021a's platforms. > > > > Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com> > > --- > > This patch is not valid. The endianness configuration in > eTSECx_DMACTRL is reserved and not applicable. > What is the value of SCFG_ETSECDMAMCR bits ETSEC_BD and ETSEC_FR_DATA > on your board? Typically this is configured by the bootloader. > Thanks your suggestion. I use linux-fsl-sdk-v1.7, and find "dma-endian-le" is used in ls1021a.dtsi and gianfar.c/.h. For bootloader, version is U-Boot version is 2015.01-dirty and it seems old and not includes "SCFG_ETSECDMAMCR bits". It seems solution is included in bootloader, not in device tree for freescale/NXP: https://lxr.missinglinkelectronics.com/uboot/board/freescale/ls1021aiot/ls1021aiot.c It means bootloader provides functions are the same as device tree's. So what's benefit for this desgin? It seems we need to upgrade kernel and bootloader to satisfy our need, not just upgrade kernel only. So many thanks! > > drivers/net/ethernet/freescale/gianfar.c | 3 +++ > > drivers/net/ethernet/freescale/gianfar.h | 4 ++++ > > 2 files changed, 7 insertions(+) > > > > diff --git a/drivers/net/ethernet/freescale/gianfar.c > > b/drivers/net/ethernet/freescale/gianfar.c > > index 72868a28b621..ab4e45199df9 100644 > > --- a/drivers/net/ethernet/freescale/gianfar.c > > +++ b/drivers/net/ethernet/freescale/gianfar.c > > @@ -833,6 +833,7 @@ static int gfar_of_init(struct platform_device > > *ofdev, struct net_device **pdev) > > > > /* Find the TBI PHY. If it's not there, we don't support SGMII */ > > priv->tbi_node = of_parse_phandle(np, "tbi-handle", 0); > > + priv->dma_endian_le = of_property_read_bool(np, > > + "fsl,dma-endian-le"); > > > > return 0; > > > > @@ -1209,6 +1210,8 @@ static void gfar_start(struct gfar_private *priv) > > /* Initialize DMACTRL to have WWR and WOP */ > > tempval = gfar_read(®s->dmactrl); > > tempval |= DMACTRL_INIT_SETTINGS; > > + if (priv->dma_endian_le) > > + tempval |= DMACTRL_LE; > > gfar_write(®s->dmactrl, tempval); > > > > /* Make sure we aren't stopped */ diff --git > > a/drivers/net/ethernet/freescale/gianfar.h > > b/drivers/net/ethernet/freescale/gianfar.h > > index 432c6a818ae5..aae07db5206b 100644 > > --- a/drivers/net/ethernet/freescale/gianfar.h > > +++ b/drivers/net/ethernet/freescale/gianfar.h > > @@ -215,6 +215,7 @@ extern const char gfar_driver_version[]; > > #define DMACTRL_INIT_SETTINGS 0x000000c3 > > #define DMACTRL_GRS 0x00000010 > > #define DMACTRL_GTS 0x00000008 > > +#define DMACTRL_LE 0x00008000 > > > > #define TSTAT_CLEAR_THALT_ALL 0xFF000000 > > #define TSTAT_CLEAR_THALT 0x80000000 > > @@ -1140,6 +1141,9 @@ struct gfar_private { > > tx_pause_en:1, > > rx_pause_en:1; > > > > + /* little endian dma buffer and descriptor host interface */ > > + unsigned int dma_endian_le; > > + > > /* The total tx and rx ring size for the enabled queues */ > > unsigned int total_tx_ring_size; > > unsigned int total_rx_ring_size; > > -- > > 2.11.0 > > > > Best regards, > > Johnson > > Regards, > -Vladimir Best regards, Johnson
Hi Johnson, On Wed, 8 Jan 2020 at 09:15, Johnson CH Chen (陳昭勳) <JohnsonCH.Chen@moxa.com> wrote: > > Hi Vladimir, > > Vladimir Oltean <olteanv@gmail.com> 於 2020年1月7日 週二 下午11:49寫道： > > > > Hi Chen, > > > > On Tue, 7 Jan 2020 at 12:37, Johnson CH Chen (陳昭勳) > > <JohnsonCH.Chen@moxa.com> wrote: > > > > > > Add dma_endian_le to solve ethernet TX/RX problems for freescale > > > ls1021a. Without this, it will result in rx-busy-errors by ethtool, and transmit queue timeout in ls1021a's platforms. > > > > > > Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com> > > > --- > > > > This patch is not valid. The endianness configuration in > > eTSECx_DMACTRL is reserved and not applicable. > > What is the value of SCFG_ETSECDMAMCR bits ETSEC_BD and ETSEC_FR_DATA > > on your board? Typically this is configured by the bootloader. > > > > Thanks your suggestion. I use linux-fsl-sdk-v1.7, and find "dma-endian-le" is used in ls1021a.dtsi and gianfar.c/.h. For bootloader, version is U-Boot version is 2015.01-dirty and it seems old and not includes "SCFG_ETSECDMAMCR bits". > > It seems solution is included in bootloader, not in device tree for > freescale/NXP: https://lxr.missinglinkelectronics.com/uboot/board/freescale/ls1021aiot/ls1021aiot.c > > It means bootloader provides functions are the same as device tree's. > So what's benefit for this desgin? It seems we need to upgrade kernel and bootloader to satisfy our need, not just upgrade kernel only. So many thanks! > I'm not sure that the Freescale SDK 1.7 is of any relevance here. The point is that this patch is breaking Ethernet for every other LS1021A board except yours. Regards, -Vladimir
diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c index 72868a28b621..ab4e45199df9 100644 --- a/drivers/net/ethernet/freescale/gianfar.c +++ b/drivers/net/ethernet/freescale/gianfar.c @@ -833,6 +833,7 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev) /* Find the TBI PHY. If it's not there, we don't support SGMII */ priv->tbi_node = of_parse_phandle(np, "tbi-handle", 0); + priv->dma_endian_le = of_property_read_bool(np, "fsl,dma-endian-le"); return 0; @@ -1209,6 +1210,8 @@ static void gfar_start(struct gfar_private *priv) /* Initialize DMACTRL to have WWR and WOP */ tempval = gfar_read(®s->dmactrl); tempval |= DMACTRL_INIT_SETTINGS; + if (priv->dma_endian_le) + tempval |= DMACTRL_LE; gfar_write(®s->dmactrl, tempval); /* Make sure we aren't stopped */ diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h index 432c6a818ae5..aae07db5206b 100644 --- a/drivers/net/ethernet/freescale/gianfar.h +++ b/drivers/net/ethernet/freescale/gianfar.h @@ -215,6 +215,7 @@ extern const char gfar_driver_version[]; #define DMACTRL_INIT_SETTINGS 0x000000c3 #define DMACTRL_GRS 0x00000010 #define DMACTRL_GTS 0x00000008 +#define DMACTRL_LE 0x00008000 #define TSTAT_CLEAR_THALT_ALL 0xFF000000 #define TSTAT_CLEAR_THALT 0x80000000 @@ -1140,6 +1141,9 @@ struct gfar_private { tx_pause_en:1, rx_pause_en:1; + /* little endian dma buffer and descriptor host interface */ + unsigned int dma_endian_le; + /* The total tx and rx ring size for the enabled queues */ unsigned int total_tx_ring_size; unsigned int total_rx_ring_size;
Add dma_endian_le to solve ethernet TX/RX problems for freescale ls1021a. Without this, it will result in rx-busy-errors by ethtool, and transmit queue timeout in ls1021a's platforms. Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com> --- drivers/net/ethernet/freescale/gianfar.c | 3 +++ drivers/net/ethernet/freescale/gianfar.h | 4 ++++ 2 files changed, 7 insertions(+)