[PATCH] gianfar: Solve ethernet TX/RX problems for ls1021a

[PATCH] gianfar: Solve ethernet TX/RX problems for ls1021a

[Johnson CH Chen (陳昭勳)]

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(+)

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(&regs->dmactrl);
 	tempval |= DMACTRL_INIT_SETTINGS;
+	if (priv->dma_endian_le)
+		tempval |= DMACTRL_LE;
 	gfar_write(&regs->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

Re: [PATCH] gianfar: Solve ethernet TX/RX problems for ls1021a

[Andrew Lunn]

> 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

Re: [PATCH] gianfar: Solve ethernet TX/RX problems for ls1021a

[Fabio Estevam]

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?

Re: [PATCH] gianfar: Solve ethernet TX/RX problems for ls1021a

[Vladimir Oltean]

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(&regs->dmactrl);
>         tempval |= DMACTRL_INIT_SETTINGS;
> +       if (priv->dma_endian_le)
> +               tempval |= DMACTRL_LE;
>         gfar_write(&regs->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

RE: [PATCH] gianfar: Solve ethernet TX/RX problems for ls1021a

[Johnson CH Chen (陳昭勳)]

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

RE: [PATCH] gianfar: Solve ethernet TX/RX problems for ls1021a

[Johnson CH Chen (陳昭勳)]

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(&regs->dmactrl);
> >         tempval |= DMACTRL_INIT_SETTINGS;
> > +       if (priv->dma_endian_le)
> > +               tempval |= DMACTRL_LE;
> >         gfar_write(&regs->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

Re: [PATCH] gianfar: Solve ethernet TX/RX problems for ls1021a

[Vladimir Oltean]

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