[query] about recent mvneta patches

[Jisheng Zhang]

Hi,

Today I have a look at recent mvneta patches in net-next, I worried two patches:

1. commit 562e2f467e71 ("net: mvneta: Improve the buffer allocation method
for SWBM") sets rx_offset_correction as 0 for SW BM, but IIRC, the offset
is introduced to support 64bit platforms. So in theory the commit could break
arm64 platforms with mvneta SW BM.

Fortunately, commit d93277b9839b ("Revert "arm64: Increase the max granular
size") makes arm64 L1_CACHE_BYTES equal to 64 again, thus NET_SKB_PAD is 64
too, so the commit 562e2f467e71 doesn't introduce regression, but it hides
the bug we tried to fix in commit 8d5047cf9ca ("net: mvneta: Convert to be 64
bits compatible)

IMHO, this patch need to be updated to not introduce regression even the
L1_CACHE_BYTES is larger than 64B, what do you think?

2. commit f945cec88cb ("net: mvneta: Verify hardware checksum only when
offload checksum feature is set"), I agree with the point. But
MVNETA_RX_CSUM_WITH_PSEUDO_HDR bit is always set, so the RX CSUM is always
generated no matter we set NETIF_F_RXCSUM or not, so IMHO, we should
always set NETIF_F_RXCSUM. And since we enabled GRO, so what about something
as below:

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index bc80a678abc3..a5043b27bf37 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -4598,7 +4598,8 @@ static int mvneta_probe(struct platform_device *pdev)
}
}
 
-   dev->features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | 
NETIF_F_TSO;
+   dev->features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+   NETIF_F_RXCSUM | NETIF_F_TSO | NETIF_F_GRO;
dev->hw_features |= dev->features;
dev->vlan_features |= dev->features;
dev->priv_flags |= IFF_LIVE_ADDR_CHANGE;

Re: [PATCH v1 6/7] net: mvneta: Don't use GRO on Armada 3720

[Jisheng Zhang]

On Thu, 9 Aug 2018 19:27:55 +0800 Jisheng Zhang wrote:

> Hi,
> 
> On Thu, 9 Aug 2018 12:40:41 +0800 Jisheng Zhang wrote:
> 
> > + more people
> > 
> > On Wed,  8 Aug 2018 17:27:05 +0200 Marek Behún wrote:
> >   
> > > For some reason on Armada 3720 boards (EspressoBin and Turris Mox) the
> > > networking driver behaves weirdly when using napi_gro_receive.
> > > 
> > > For example downloading a big file from a local network (low ping) is
> > > fast, but when downloading from a remote server (higher ping), the
> > > download speed is at first high but drops rapidly to almost nothing or
> > > absolutely nothing.
> > 
> > We also met this issue on some berlin platforms. I tried to fix the bug,
> > but no clue so far.
> >   
> > > 
> > > This is fixed when using netif_receive_skb instead of napi_gro_receive.   
> > >  
> > 
> > This is a workaround. The good news is this workaround also fixes the issue
> > we saw on berlin.
> >   
> > > 
> > > Signed-off-by: Marek Behun 
> > > Cc: Russell King - ARM Linux 
> > > Cc: netdev@vger.kernel.org
> > > 
> > > diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> > > b/drivers/net/ethernet/marvell/mvneta.c
> > > index 0ad2f3f7da85..27f3017d94c5 100644
> > > --- a/drivers/net/ethernet/marvell/mvneta.c
> > > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > > @@ -1959,7 +1959,10 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, 
> > > int rx_todo,
> > >  
> > >   skb->protocol = eth_type_trans(skb, dev);
> > >   mvneta_rx_csum(pp, rx_status, skb);
> > > - napi_gro_receive(>napi, skb);
> > > + if (pp->neta_armada3700)
> > > + netif_receive_skb(skb);
> > > + else
> > > + napi_gro_receive(>napi, skb);  
> 
> I think I found the root cause, if neta_armada3700 is true, the port got from
> this_cpu_ptr(pp->ports) is invalid, this is bug... I'll cook a patch for this

correct it as:

the port's(port is got from this_cpu_ptr(pp->ports) napi is invalid.

Patch is sent out. Could you please try?

Per my test, it solves the issue we saw on berlin.

> 
> Thanks
> 
> > >  
> > >   rcvd_pkts++;
> > >   rcvd_bytes += rx_bytes;
> > > @@ -2001,7 +2004,10 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, 
> > > int rx_todo,
> > >  
> > >   mvneta_rx_csum(pp, rx_status, skb);
> > >  
> > > - napi_gro_receive(>napi, skb);
> > > + if (pp->neta_armada3700)
> > > + netif_receive_skb(skb);
> > > + else
> > > + napi_gro_receive(>napi, skb);
> > >   }
> > >  
> > >   if (rcvd_pkts) {
> >   
> 

Re: [PATCH v1 6/7] net: mvneta: Don't use GRO on Armada 3720

[Jisheng Zhang]

Hi,

On Thu, 9 Aug 2018 12:40:41 +0800 Jisheng Zhang wrote:

> + more people
> 
> On Wed,  8 Aug 2018 17:27:05 +0200 Marek Behún wrote:
> 
> > For some reason on Armada 3720 boards (EspressoBin and Turris Mox) the
> > networking driver behaves weirdly when using napi_gro_receive.
> > 
> > For example downloading a big file from a local network (low ping) is
> > fast, but when downloading from a remote server (higher ping), the
> > download speed is at first high but drops rapidly to almost nothing or
> > absolutely nothing.  
> 
> We also met this issue on some berlin platforms. I tried to fix the bug,
> but no clue so far.
> 
> > 
> > This is fixed when using netif_receive_skb instead of napi_gro_receive.  
> 
> This is a workaround. The good news is this workaround also fixes the issue
> we saw on berlin.
> 
> > 
> > Signed-off-by: Marek Behun 
> > Cc: Russell King - ARM Linux 
> > Cc: netdev@vger.kernel.org
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> > b/drivers/net/ethernet/marvell/mvneta.c
> > index 0ad2f3f7da85..27f3017d94c5 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -1959,7 +1959,10 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, 
> > int rx_todo,
> >  
> > skb->protocol = eth_type_trans(skb, dev);
> > mvneta_rx_csum(pp, rx_status, skb);
> > -   napi_gro_receive(>napi, skb);
> > +   if (pp->neta_armada3700)
> > +   netif_receive_skb(skb);
> > +   else
> > +   napi_gro_receive(>napi, skb);

I think I found the root cause, if neta_armada3700 is true, the port got from
this_cpu_ptr(pp->ports) is invalid, this is bug... I'll cook a patch for this

Thanks

> >  
> > rcvd_pkts++;
> > rcvd_bytes += rx_bytes;
> > @@ -2001,7 +2004,10 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, 
> > int rx_todo,
> >  
> > mvneta_rx_csum(pp, rx_status, skb);
> >  
> > -   napi_gro_receive(>napi, skb);
> > +   if (pp->neta_armada3700)
> > +   netif_receive_skb(skb);
> > +   else
> > +   napi_gro_receive(>napi, skb);
> > }
> >  
> > if (rcvd_pkts) {  
> 

Re: [PATCH v1 6/7] net: mvneta: Don't use GRO on Armada 3720

[Jisheng Zhang]

+ more people

On Wed,  8 Aug 2018 17:27:05 +0200 Marek Behún wrote:

> For some reason on Armada 3720 boards (EspressoBin and Turris Mox) the
> networking driver behaves weirdly when using napi_gro_receive.
> 
> For example downloading a big file from a local network (low ping) is
> fast, but when downloading from a remote server (higher ping), the
> download speed is at first high but drops rapidly to almost nothing or
> absolutely nothing.

We also met this issue on some berlin platforms. I tried to fix the bug,
but no clue so far.

> 
> This is fixed when using netif_receive_skb instead of napi_gro_receive.

This is a workaround. The good news is this workaround also fixes the issue
we saw on berlin.

> 
> Signed-off-by: Marek Behun 
> Cc: Russell King - ARM Linux 
> Cc: netdev@vger.kernel.org
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> b/drivers/net/ethernet/marvell/mvneta.c
> index 0ad2f3f7da85..27f3017d94c5 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -1959,7 +1959,10 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, int 
> rx_todo,
>  
>   skb->protocol = eth_type_trans(skb, dev);
>   mvneta_rx_csum(pp, rx_status, skb);
> - napi_gro_receive(>napi, skb);
> + if (pp->neta_armada3700)
> + netif_receive_skb(skb);
> + else
> + napi_gro_receive(>napi, skb);
>  
>   rcvd_pkts++;
>   rcvd_bytes += rx_bytes;
> @@ -2001,7 +2004,10 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, int 
> rx_todo,
>  
>   mvneta_rx_csum(pp, rx_status, skb);
>  
> - napi_gro_receive(>napi, skb);
> + if (pp->neta_armada3700)
> + netif_receive_skb(skb);
> + else
> + napi_gro_receive(>napi, skb);
>   }
>  
>   if (rcvd_pkts) {

[PATCH] drivers: net: replace UINT64_MAX with U64_MAX

[Jisheng Zhang]

U64_MAX is well defined now while the UINT64_MAX is not, so we fall
back to drivers' own definition as below:

#ifndef UINT64_MAX
#define UINT64_MAX (u64)(~((u64)0))
#endif

I believe this is in one phy driver then copied and pasted to other phy
drivers.

Replace the UINT64_MAX with U64_MAX to clean up the source code.

Signed-off-by: Jisheng Zhang <jisheng.zh...@synaptics.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 6 +++---
 drivers/net/dsa/mv88e6xxx/chip.h | 4 
 drivers/net/phy/bcm-phy-lib.c| 6 +-
 drivers/net/phy/marvell.c| 5 +
 drivers/net/phy/micrel.c | 5 +
 drivers/net/phy/smsc.c   | 5 +
 6 files changed, 7 insertions(+), 24 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 3d2091099f7f..1a2dd340853f 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -665,13 +665,13 @@ static uint64_t _mv88e6xxx_get_ethtool_stat(struct 
mv88e6xxx_chip *chip,
case STATS_TYPE_PORT:
err = mv88e6xxx_port_read(chip, port, s->reg, );
if (err)
-   return UINT64_MAX;
+   return U64_MAX;
 
low = reg;
if (s->size == 4) {
err = mv88e6xxx_port_read(chip, port, s->reg + 1, );
if (err)
-   return UINT64_MAX;
+   return U64_MAX;
high = reg;
}
break;
@@ -685,7 +685,7 @@ static uint64_t _mv88e6xxx_get_ethtool_stat(struct 
mv88e6xxx_chip *chip,
mv88e6xxx_g1_stats_read(chip, reg + 1, );
break;
default:
-   return UINT64_MAX;
+   return U64_MAX;
}
value = (((u64)high) << 16) | low;
return value;
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 80490f66bc06..4163c8099d0b 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -21,10 +21,6 @@
 #include 
 #include 
 
-#ifndef UINT64_MAX
-#define UINT64_MAX (u64)(~((u64)0))
-#endif
-
 #define SMI_CMD0x00
 #define SMI_CMD_BUSY   BIT(15)
 #define SMI_CMD_CLAUSE_22  BIT(12)
diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index 5ad130c3da43..0876aec7328c 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -346,10 +346,6 @@ void bcm_phy_get_strings(struct phy_device *phydev, u8 
*data)
 }
 EXPORT_SYMBOL_GPL(bcm_phy_get_strings);
 
-#ifndef UINT64_MAX
-#define UINT64_MAX  (u64)(~((u64)0))
-#endif
-
 /* Caller is supposed to provide appropriate storage for the library code to
  * access the shadow copy
  */
@@ -362,7 +358,7 @@ static u64 bcm_phy_get_stat(struct phy_device *phydev, u64 
*shadow,
 
val = phy_read(phydev, stat.reg);
if (val < 0) {
-   ret = UINT64_MAX;
+   ret = U64_MAX;
} else {
val >>= stat.shift;
val = val & ((1 << stat.bits) - 1);
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 25e2a099b71c..b8f57e9b9379 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -1482,9 +1482,6 @@ static void marvell_get_strings(struct phy_device 
*phydev, u8 *data)
}
 }
 
-#ifndef UINT64_MAX
-#define UINT64_MAX (u64)(~((u64)0))
-#endif
 static u64 marvell_get_stat(struct phy_device *phydev, int i)
 {
struct marvell_hw_stat stat = marvell_hw_stats[i];
@@ -1494,7 +1491,7 @@ static u64 marvell_get_stat(struct phy_device *phydev, 
int i)
 
val = phy_read_paged(phydev, stat.page, stat.reg);
if (val < 0) {
-   ret = UINT64_MAX;
+   ret = U64_MAX;
} else {
val = val & ((1 << stat.bits) - 1);
priv->stats[i] += val;
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index f41b224a9cdb..de31c5170a5b 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -650,9 +650,6 @@ static void kszphy_get_strings(struct phy_device *phydev, 
u8 *data)
}
 }
 
-#ifndef UINT64_MAX
-#define UINT64_MAX  (u64)(~((u64)0))
-#endif
 static u64 kszphy_get_stat(struct phy_device *phydev, int i)
 {
struct kszphy_hw_stat stat = kszphy_hw_stats[i];
@@ -662,7 +659,7 @@ static u64 kszphy_get_stat(struct phy_device *phydev, int i)
 
val = phy_read(phydev, stat.reg);
if (val < 0) {
-   ret = UINT64_MAX;
+   ret = U64_MAX;
} else {
val = val & ((1 << stat.bits) - 1);
priv->stats[i] += val;
diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index be399d645224..c328208388da 100644
--

Re: [PATCH] net: phy: marvell: clear wol event before setting it

[Jisheng Zhang]

On Thu, 26 Apr 2018 12:39:59 +0530 Bhadram Varka wrote:

> >
> > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> > index c22e8e383247..b6abe1cbc84b 100644
> > --- a/drivers/net/phy/marvell.c
> > +++ b/drivers/net/phy/marvell.c
> > @@ -115,6 +115,9 @@
> >    /* WOL Event Interrupt Enable */
> >    #define MII_88E1318S_PHY_CSIER_WOL_EIE BIT(7)
> >    +/* Copper Specific Interrupt Status Register */
> > +#define MII_88E1318S_PHY_CSISR    0x13
> > +
> >    /* LED Timer Control Register */
> >    #define MII_88E1318S_PHY_LED_TCR    0x12
> >    #define MII_88E1318S_PHY_LED_TCR_FORCE_INT BIT(15)
> > @@ -1393,6 +1396,12 @@ static int m88e1318_set_wol(struct 
> > phy_device *phydev,
> >    if (err < 0)
> >    goto error;
> >    +    /* If WOL event happened once, the LED[2] interrupt pin
> > + * will not be cleared unless reading the CSISR register.
> > + * So clear the WOL event first before enabling it.
> > + */
> > +    phy_read(phydev, MII_88E1318S_PHY_CSISR);
> > +  
>  Hi Jisheng
> 
>  The problem with this is, you could be clearing a real interrupt, link
>  down/up etc. If interrupts are in use, i think the normal interrupt
>  handling will clear the WOL interrupt? So can you make this read
>  conditional on !phy_interrupt_is_valid()?  
> >>> So this will clear WoL interrupt bit from Copper Interrupt status 
> >>> register.
> >>>
> >>> How about clearing WoL status (Page 17, register 17) for every WOL 
> >>> event ?
> >>>  
> >> This is already properly done by setting 
> >> MII_88E1318S_PHY_WOL_CTRL_CLEAR_WOL_STATUS
> >> in m88e1318_set_wol()  
> > This part of the code executes only when we enable WOL through ethtool 
> > (ethtool -s eth0 wol g)
> >
> > Lets say once WOL enabled through magic packet - HW generates WOL 
> > interrupt once magic packet received.
> > The problem that I see here is that for the next immediate magic 
> > packet I don't see WOL interrupt generated by the HW.
> > I need to explicitly clear WOL status for HW to generate WOL interrupt.  
> 
> With the below patch I see WOL event interrupt for every magic packet 
> that HW receives...
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index ed8a67d..5d3d138 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -55,6 +55,7 @@
> 
>   #define MII_M1011_IEVENT   0x13
>   #define MII_M1011_IEVENT_CLEAR 0x
> +#define MII_M1011_IEVENT_WOL_EVENT BIT(7)
> 
>   #define MII_M1011_IMASK    0x12
> - #define MII_M1011_IMASK_INIT   0x6400
> + #define MII_M1011_IMASK_INIT   0x6480
> 
> @@ -195,13 +196,40 @@ struct marvell_priv {
>      bool copper;
>   };
> 
> +static int marvell_clear_wol_status(struct phy_device *phydev)
> +{
> +   int err, temp, oldpage;
> +
> +   oldpage = phy_read(phydev, MII_MARVELL_PHY_PAGE);
> +   if (oldpage < 0)
> +   return oldpage;
> +
> +   err = phy_write(phydev, MII_MARVELL_PHY_PAGE,
> +    MII_88E1318S_PHY_WOL_PAGE);
> +   if (err < 0)
> +   return err;
> +
> +   /*
> +    * Clear WOL status so that for next WOL event
> +    * interrupt will be generated by HW
> +    */
> +   temp = phy_read(phydev, MII_88E1318S_PHY_WOL_CTRL);
> +   temp |= MII_88E1318S_PHY_WOL_CTRL_CLEAR_WOL_STATUS;
> +   err = phy_write(phydev, MII_88E1318S_PHY_WOL_CTRL, temp);

is it better to reuse __phy_write()?

> +   if (err < 0)
> +   return err;
> +
> +
> +   phy_write(phydev, MII_MARVELL_PHY_PAGE, oldpage);
> +
> +   return 0;
> +}
> +
>   static int marvell_ack_interrupt(struct phy_device *phydev)
>   {
>      int err;
> 
>      /* Clear the interrupts by reading the reg */
>      err = phy_read(phydev, MII_M1011_IEVENT);
> -
>      if (err < 0)
>      return err;
> 
> @@ -1454,12 +1482,18 @@ static int marvell_aneg_done(struct phy_device 
> *phydev)
> 
>   static int m88e1121_did_interrupt(struct phy_device *phydev)
>   {
> -   int imask;
> +   int imask, err;
> 
>      imask = phy_read(phydev, MII_M1011_IEVENT);
> 
> -   if (imask & MII_M1011_IMASK_INIT)
> +   if (imask & MII_M1011_IMASK_INIT) {
> +   if (imask & MII_M1011_IEVENT_WOL_EVENT) {
> +   err = marvell_clear_wol_status(phydev);
> +   if (err < 0)
> +   return 0;
> +   }
>      return 1;
> +   }
> 
>      return 0;
>   }

Re: [PATCH] net: phy: marvell: clear wol event before setting it

[Jisheng Zhang]

On Fri, 27 Apr 2018 09:22:34 +0530 Bhadram Varka wrote:

> Hi Andrew/Jisheng,
> 
> On 4/26/2018 6:10 PM, Andrew Lunn wrote:
> >> hmm, so you want a "stick" WOL feature, I dunno whether Linux kernel
> >> requires WOL should be "stick".  
> > I see two different cases:
> >
> > Suspend/resume: The WoL state in the kernel is probably kept across
> > such a cycle. If so, you would expect another suspend/resume to also
> > work, without needs to reconfigure it.  
> Trying this scenario (suspend/resume) from my side. In this case WoL 
> should be enabled in the HW. For Marvell PHY to generate WoL interrupt 
> we need to clear WoL status.
> Above mentioned change required to make this happen. Please share your 
> thoughts on this.

I'm fine with that patch. Maybe you could send out a patch?

Thanks

Re: [PATCH] net: phy: marvell: clear wol event before setting it

[Jisheng Zhang]

On Thu, 26 Apr 2018 11:56:33 +0530 Bhadram Varka wrote:

> Hi,
> On 4/26/2018 11:45 AM, Jisheng Zhang wrote:
> > Hi,
> >
> > On Thu, 26 Apr 2018 11:10:21 +0530 Bhadram Varka wrote:
> >  
> >> Hi,
> >>
> >> On 4/19/2018 5:48 PM, Andrew Lunn wrote:  
> >>> On Thu, Apr 19, 2018 at 04:02:32PM +0800, Jisheng Zhang wrote:  



> >>>>  if (err < 0)
> >>>>  goto error;
> >>>>
> >>>> +/* If WOL event happened once, the LED[2] interrupt pin
> >>>> + * will not be cleared unless reading the CSISR 
> >>>> register.
> >>>> + * So clear the WOL event first before enabling it.
> >>>> + */
> >>>> +phy_read(phydev, MII_88E1318S_PHY_CSISR);
> >>>> +  
> >>> Hi Jisheng
> >>>
> >>> The problem with this is, you could be clearing a real interrupt, link
> >>> down/up etc. If interrupts are in use, i think the normal interrupt
> >>> handling will clear the WOL interrupt? So can you make this read
> >>> conditional on !phy_interrupt_is_valid()?  
> >> So this will clear WoL interrupt bit from Copper Interrupt status register.
> >>
> >> How about clearing WoL status (Page 17, register 17) for every WOL event ?
> >>  
> > This is already properly done by setting 
> > MII_88E1318S_PHY_WOL_CTRL_CLEAR_WOL_STATUS
> > in m88e1318_set_wol()  
> This part of the code executes only when we enable WOL through ethtool 
> (ethtool -s eth0 wol g)
> 
> Lets say once WOL enabled through magic packet - HW generates WOL 
> interrupt once magic packet received.
> The problem that I see here is that for the next immediate magic packet 
> I don't see WOL interrupt generated by the HW.

hmm, so you want a "stick" WOL feature, I dunno whether Linux kernel
requires WOL should be "stick".

Re: [PATCH] net: phy: marvell: clear wol event before setting it

[Jisheng Zhang]

Hi,

On Thu, 26 Apr 2018 11:10:21 +0530 Bhadram Varka wrote:

> Hi,
> 
> On 4/19/2018 5:48 PM, Andrew Lunn wrote:
> > On Thu, Apr 19, 2018 at 04:02:32PM +0800, Jisheng Zhang wrote:  
> >> From: Jingju Hou <jingju@synaptics.com>
> >>
> >> If WOL event happened once, the LED[2] interrupt pin will not be
> >> cleared unless reading the CSISR register. So clear the WOL event
> >> before enabling it.
> >>
> >> Signed-off-by: Jingju Hou <jingju@synaptics.com>
> >> Signed-off-by: Jisheng Zhang <jisheng.zh...@synaptics.com>
> >> ---
> >>   drivers/net/phy/marvell.c | 9 +
> >>   1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> >> index c22e8e383247..b6abe1cbc84b 100644
> >> --- a/drivers/net/phy/marvell.c
> >> +++ b/drivers/net/phy/marvell.c
> >> @@ -115,6 +115,9 @@
> >>   /* WOL Event Interrupt Enable */
> >>   #define MII_88E1318S_PHY_CSIER_WOL_EIE   BIT(7)
> >>   
> >> +/* Copper Specific Interrupt Status Register */
> >> +#define MII_88E1318S_PHY_CSISR0x13
> >> +
> >>   /* LED Timer Control Register */
> >>   #define MII_88E1318S_PHY_LED_TCR 0x12
> >>   #define MII_88E1318S_PHY_LED_TCR_FORCE_INT   BIT(15)
> >> @@ -1393,6 +1396,12 @@ static int m88e1318_set_wol(struct phy_device 
> >> *phydev,
> >>if (err < 0)
> >>goto error;
> >>   
> >> +  /* If WOL event happened once, the LED[2] interrupt pin
> >> +   * will not be cleared unless reading the CSISR register.
> >> +   * So clear the WOL event first before enabling it.
> >> +   */
> >> +  phy_read(phydev, MII_88E1318S_PHY_CSISR);
> >> +  
> > Hi Jisheng
> >
> > The problem with this is, you could be clearing a real interrupt, link
> > down/up etc. If interrupts are in use, i think the normal interrupt
> > handling will clear the WOL interrupt? So can you make this read
> > conditional on !phy_interrupt_is_valid()?  
> So this will clear WoL interrupt bit from Copper Interrupt status register.
> 
> How about clearing WoL status (Page 17, register 17) for every WOL event ?
> 

This is already properly done by setting 
MII_88E1318S_PHY_WOL_CTRL_CLEAR_WOL_STATUS
in m88e1318_set_wol()

Thanks

[PATCH v2] net: phy: marvell: clear wol event before setting it

[Jisheng Zhang]

From: Jingju Hou <jingju@synaptics.com>

If WOL event happened once, the LED[2] interrupt pin will not be
cleared unless we read the CSISR register. If interrupts are in use,
the normal interrupt handling will clear the WOL event. Let's clear the
WOL event before enabling it if !phy_interrupt_is_valid().

Signed-off-by: Jingju Hou <jingju@synaptics.com>
Signed-off-by: Jisheng Zhang <jisheng.zh...@synaptics.com>
---
Since v1:
 - reuse MII_M1011_IEVENT, suggested by Bhadram Varka
 - make read conditional on !phy_interrupt_is_valid(), suggested by
   Andrew
 drivers/net/phy/marvell.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index c22e8e383247..25e2a099b71c 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -1393,6 +1393,15 @@ static int m88e1318_set_wol(struct phy_device *phydev,
if (err < 0)
goto error;
 
+   /* If WOL event happened once, the LED[2] interrupt pin
+* will not be cleared unless we reading the interrupt status
+* register. If interrupts are in use, the normal interrupt
+* handling will clear the WOL event. Clear the WOL event
+* before enabling it if !phy_interrupt_is_valid()
+*/
+   if (!phy_interrupt_is_valid(phydev))
+   phy_read(phydev, MII_M1011_IEVENT);
+
/* Enable the WOL interrupt */
err = __phy_modify(phydev, MII_88E1318S_PHY_CSIER, 0,
   MII_88E1318S_PHY_CSIER_WOL_EIE);
-- 
2.17.0

Re: [PATCH] net: phy: marvell: clear wol event before setting it

[Jisheng Zhang]

On Thu, 19 Apr 2018 09:00:40 + Bhadram Varka wrote:

> Hi,
> 
> > -Original Message-
> > From: Jisheng Zhang <jisheng.zh...@synaptics.com>
> > Sent: Thursday, April 19, 2018 2:24 PM
> > To: Bhadram Varka <vbhad...@nvidia.com>
> > Cc: Andrew Lunn <and...@lunn.ch>; Florian Fainelli <f.faine...@gmail.com>;
> > David S. Miller <da...@davemloft.net>; netdev@vger.kernel.org; linux-
> > ker...@vger.kernel.org; Jingju Hou <jingju@synaptics.com>
> > Subject: Re: [PATCH] net: phy: marvell: clear wol event before setting it
> > 
> > Hi,
> > 
> > On Thu, 19 Apr 2018 08:38:45 + Bhadram Varka wrote:
> >   
> > > Hi,
> > >  
> > > > -Original Message-
> > > > From: netdev-ow...@vger.kernel.org <netdev-ow...@vger.kernel.org> On
> > > > Behalf Of Jisheng Zhang
> > > > Sent: Thursday, April 19, 2018 1:33 PM
> > > > To: Andrew Lunn <and...@lunn.ch>; Florian Fainelli
> > > > <f.faine...@gmail.com>; David S. Miller <da...@davemloft.net>
> > > > Cc: netdev@vger.kernel.org; linux-ker...@vger.kernel.org; Jingju Hou
> > > > <jingju@synaptics.com>
> > > > Subject: [PATCH] net: phy: marvell: clear wol event before setting
> > > > it
> > > >
> > > > From: Jingju Hou <jingju....@synaptics.com>
> > > >
> > > > If WOL event happened once, the LED[2] interrupt pin will not be
> > > > cleared unless reading the CSISR register. So clear the WOL event 
> > > > before  
> > enabling it.  
> > > >
> > > > Signed-off-by: Jingju Hou <jingju@synaptics.com>
> > > > Signed-off-by: Jisheng Zhang <jisheng.zh...@synaptics.com>
> > > > ---
> > > >  drivers/net/phy/marvell.c | 9 +
> > > >  1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> > > > index c22e8e383247..b6abe1cbc84b 100644
> > > > --- a/drivers/net/phy/marvell.c
> > > > +++ b/drivers/net/phy/marvell.c
> > > > @@ -115,6 +115,9 @@
> > > >  /* WOL Event Interrupt Enable */
> > > >  #define MII_88E1318S_PHY_CSIER_WOL_EIE BIT(7)
> > > >
> > > > +/* Copper Specific Interrupt Status Register */
> > > > +#define MII_88E1318S_PHY_CSISR 0x13
> > > > +  
> > >
> > > There is already macro to represent this register - MII_M1011_IEVENT. Do 
> > > we  
> > need this macro ?
> > 
> > Good point. Will use MII_M1011_IEVENT instead in v2.
> >   
> > >  
> > > >  /* LED Timer Control Register */
> > > >  #define MII_88E1318S_PHY_LED_TCR   0x12
> > > >  #define MII_88E1318S_PHY_LED_TCR_FORCE_INT BIT(15)
> > > > @@ -1393,6 +1396,12 @@ static int m88e1318_set_wol(struct phy_device
> > > > *phydev,
> > > > if (err < 0)
> > > > goto error;
> > > >
> > > > +   /* If WOL event happened once, the LED[2] interrupt pin
> > > > +* will not be cleared unless reading the CSISR 
> > > > register.
> > > > +* So clear the WOL event first before enabling it.
> > > > +*/
> > > > +   phy_read(phydev, MII_88E1318S_PHY_CSISR);  
> > >
> > > This part of the operation already taken care by ack_interrupt and
> > > did_interrupt [] .ack_interrupt = _ack_interrupt,
> > > .did_interrupt = _did_interrupt, [...]
> > >
> > > If at all WOL event occurred marvell_ack_interrupt will take care of 
> > > clearing the  
> > interrupt status register.  
> > > Am I missing anything here ?  
> > 
> > If there's no valid irq for phy, the ack_interrupt/did_interrupt won't be 
> > called.  
> 
> Which means that the PHY is not having Interrupt pin ?

No valid irq doesn't mean "not having interrupt pin". they are different

> 
> Generally through PHY interrupt will wake up the system right. If there is no 
> interrupt pin then how the system will wake up the from suspend for the magic 
> packet.?
> 

IIRC, the phy irq isn't necessary for WOL. The phy interrupt pin isn't
necessarily taken as "interrupt"

PS: Did you use outlook as your email client? it's not suitable
for kernel mail list.

Thanks

Re: [PATCH] net: phy: marvell: clear wol event before setting it

[Jisheng Zhang]

Hi,

On Thu, 19 Apr 2018 08:38:45 + Bhadram Varka wrote:

> Hi,
> 
> > -Original Message-
> > From: netdev-ow...@vger.kernel.org <netdev-ow...@vger.kernel.org> On
> > Behalf Of Jisheng Zhang
> > Sent: Thursday, April 19, 2018 1:33 PM
> > To: Andrew Lunn <and...@lunn.ch>; Florian Fainelli <f.faine...@gmail.com>;
> > David S. Miller <da...@davemloft.net>
> > Cc: netdev@vger.kernel.org; linux-ker...@vger.kernel.org; Jingju Hou
> > <jingju@synaptics.com>
> > Subject: [PATCH] net: phy: marvell: clear wol event before setting it
> > 
> > From: Jingju Hou <jingju@synaptics.com>
> > 
> > If WOL event happened once, the LED[2] interrupt pin will not be cleared 
> > unless
> > reading the CSISR register. So clear the WOL event before enabling it.
> > 
> > Signed-off-by: Jingju Hou <jingju@synaptics.com>
> > Signed-off-by: Jisheng Zhang <jisheng.zh...@synaptics.com>
> > ---
> >  drivers/net/phy/marvell.c | 9 +
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index
> > c22e8e383247..b6abe1cbc84b 100644
> > --- a/drivers/net/phy/marvell.c
> > +++ b/drivers/net/phy/marvell.c
> > @@ -115,6 +115,9 @@
> >  /* WOL Event Interrupt Enable */
> >  #define MII_88E1318S_PHY_CSIER_WOL_EIE BIT(7)
> > 
> > +/* Copper Specific Interrupt Status Register */
> > +#define MII_88E1318S_PHY_CSISR 0x13
> > +  
> 
> There is already macro to represent this register - MII_M1011_IEVENT. Do we 
> need this macro ?

Good point. Will use MII_M1011_IEVENT instead in v2.

> 
> >  /* LED Timer Control Register */
> >  #define MII_88E1318S_PHY_LED_TCR   0x12
> >  #define MII_88E1318S_PHY_LED_TCR_FORCE_INT BIT(15)
> > @@ -1393,6 +1396,12 @@ static int m88e1318_set_wol(struct phy_device
> > *phydev,
> > if (err < 0)
> > goto error;
> > 
> > +   /* If WOL event happened once, the LED[2] interrupt pin
> > +* will not be cleared unless reading the CSISR register.
> > +* So clear the WOL event first before enabling it.
> > +*/
> > +   phy_read(phydev, MII_88E1318S_PHY_CSISR);  
> 
> This part of the operation already taken care by ack_interrupt and 
> did_interrupt
> []
> .ack_interrupt = _ack_interrupt,
> .did_interrupt = _did_interrupt,
> [...]
> 
> If at all WOL event occurred marvell_ack_interrupt will take care of clearing 
> the interrupt status register.
> Am I missing anything here ?

If there's no valid irq for phy, the ack_interrupt/did_interrupt won't
be called.


Thanks

[PATCH] net: phy: marvell: clear wol event before setting it

[Jisheng Zhang]

From: Jingju Hou <jingju@synaptics.com>

If WOL event happened once, the LED[2] interrupt pin will not be
cleared unless reading the CSISR register. So clear the WOL event
before enabling it.

Signed-off-by: Jingju Hou <jingju@synaptics.com>
Signed-off-by: Jisheng Zhang <jisheng.zh...@synaptics.com>
---
 drivers/net/phy/marvell.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index c22e8e383247..b6abe1cbc84b 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -115,6 +115,9 @@
 /* WOL Event Interrupt Enable */
 #define MII_88E1318S_PHY_CSIER_WOL_EIE BIT(7)
 
+/* Copper Specific Interrupt Status Register */
+#define MII_88E1318S_PHY_CSISR 0x13
+
 /* LED Timer Control Register */
 #define MII_88E1318S_PHY_LED_TCR   0x12
 #define MII_88E1318S_PHY_LED_TCR_FORCE_INT BIT(15)
@@ -1393,6 +1396,12 @@ static int m88e1318_set_wol(struct phy_device *phydev,
if (err < 0)
goto error;
 
+   /* If WOL event happened once, the LED[2] interrupt pin
+* will not be cleared unless reading the CSISR register.
+* So clear the WOL event first before enabling it.
+*/
+   phy_read(phydev, MII_88E1318S_PHY_CSISR);
+
/* Enable the WOL interrupt */
err = __phy_modify(phydev, MII_88E1318S_PHY_CSIER, 0,
   MII_88E1318S_PHY_CSIER_WOL_EIE);
-- 
2.17.0

[PATCH v3 2/2] net: mvneta: improve suspend/resume

[Jisheng Zhang]

Current suspend/resume implementation reuses the mvneta_open() and
mvneta_close(), but it could be optimized to take only necessary
actions during suspend/resume.

One obvious problem of current implementation is: after hundreds of
system suspend/resume cycles, the resume of mvneta could fail due to
fragmented dma coherent memory. After this patch, the non-necessary
memory alloc/free is optimized out.

Signed-off-by: Jisheng Zhang <jisheng.zh...@synaptics.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 69 +++
 1 file changed, 62 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index f96815853108..8999a9a52ca2 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -4586,16 +4586,45 @@ static int mvneta_remove(struct platform_device *pdev)
 #ifdef CONFIG_PM_SLEEP
 static int mvneta_suspend(struct device *device)
 {
+   int queue;
struct net_device *dev = dev_get_drvdata(device);
struct mvneta_port *pp = netdev_priv(dev);
 
+   if (!netif_running(dev))
+   goto clean_exit;
+
+   if (!pp->neta_armada3700) {
+   spin_lock(>lock);
+   pp->is_stopped = true;
+   spin_unlock(>lock);
+
+   cpuhp_state_remove_instance_nocalls(online_hpstate,
+   >node_online);
+   cpuhp_state_remove_instance_nocalls(CPUHP_NET_MVNETA_DEAD,
+   >node_dead);
+   }
+
rtnl_lock();
-   if (netif_running(dev))
-   mvneta_stop(dev);
+   mvneta_stop_dev(pp);
rtnl_unlock();
+
+   for (queue = 0; queue < rxq_number; queue++) {
+   struct mvneta_rx_queue *rxq = >rxqs[queue];
+
+   mvneta_rxq_drop_pkts(pp, rxq);
+   }
+
+   for (queue = 0; queue < txq_number; queue++) {
+   struct mvneta_tx_queue *txq = >txqs[queue];
+
+   mvneta_txq_hw_deinit(pp, txq);
+   }
+
+clean_exit:
netif_device_detach(dev);
clk_disable_unprepare(pp->clk_bus);
clk_disable_unprepare(pp->clk);
+
return 0;
 }
 
@@ -4604,7 +4633,7 @@ static int mvneta_resume(struct device *device)
struct platform_device *pdev = to_platform_device(device);
struct net_device *dev = dev_get_drvdata(device);
struct mvneta_port *pp = netdev_priv(dev);
-   int err;
+   int err, queue;
 
clk_prepare_enable(pp->clk);
if (!IS_ERR(pp->clk_bus))
@@ -4626,12 +4655,38 @@ static int mvneta_resume(struct device *device)
}
 
netif_device_attach(dev);
-   rtnl_lock();
-   if (netif_running(dev)) {
-   mvneta_open(dev);
-   mvneta_set_rx_mode(dev);
+
+   if (!netif_running(dev))
+   return 0;
+
+   for (queue = 0; queue < rxq_number; queue++) {
+   struct mvneta_rx_queue *rxq = >rxqs[queue];
+
+   rxq->next_desc_to_proc = 0;
+   mvneta_rxq_hw_init(pp, rxq);
+   }
+
+   for (queue = 0; queue < txq_number; queue++) {
+   struct mvneta_tx_queue *txq = >txqs[queue];
+
+   txq->next_desc_to_proc = 0;
+   mvneta_txq_hw_init(pp, txq);
}
+
+   if (!pp->neta_armada3700) {
+   spin_lock(>lock);
+   pp->is_stopped = false;
+   spin_unlock(>lock);
+   cpuhp_state_add_instance_nocalls(online_hpstate,
+>node_online);
+   cpuhp_state_add_instance_nocalls(CPUHP_NET_MVNETA_DEAD,
+>node_dead);
+   }
+
+   rtnl_lock();
+   mvneta_start_dev(pp);
rtnl_unlock();
+   mvneta_set_rx_mode(dev);
 
return 0;
 }
-- 
2.16.3

[PATCH v3 1/2] net: mvneta: split rxq/txq init and txq deinit into SW and HW parts

[Jisheng Zhang]

This is to prepare the suspend/resume improvement in next patch. The
SW parts can be optimized out during resume.

As for rxq handling during suspend, we'd like to drop packets by
calling mvneta_rxq_drop_pkts() which is both SW and HW operation,
so we don't split rxq deinit.

Signed-off-by: Jisheng Zhang <jisheng.zh...@synaptics.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 85 +++
 1 file changed, 66 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 30aab9bf77cc..f96815853108 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -2796,10 +2796,8 @@ static void mvneta_rx_reset(struct mvneta_port *pp)
 
 /* Rx/Tx queue initialization/cleanup methods */
 
-/* Create a specified RX queue */
-static int mvneta_rxq_init(struct mvneta_port *pp,
-  struct mvneta_rx_queue *rxq)
-
+static int mvneta_rxq_sw_init(struct mvneta_port *pp,
+ struct mvneta_rx_queue *rxq)
 {
rxq->size = pp->rx_ring_size;
 
@@ -2812,6 +2810,12 @@ static int mvneta_rxq_init(struct mvneta_port *pp,
 
rxq->last_desc = rxq->size - 1;
 
+   return 0;
+}
+
+static void mvneta_rxq_hw_init(struct mvneta_port *pp,
+  struct mvneta_rx_queue *rxq)
+{
/* Set Rx descriptors queue starting address */
mvreg_write(pp, MVNETA_RXQ_BASE_ADDR_REG(rxq->id), rxq->descs_phys);
mvreg_write(pp, MVNETA_RXQ_SIZE_REG(rxq->id), rxq->size);
@@ -2835,6 +2839,20 @@ static int mvneta_rxq_init(struct mvneta_port *pp,
mvneta_rxq_short_pool_set(pp, rxq);
mvneta_rxq_non_occup_desc_add(pp, rxq, rxq->size);
}
+}
+
+/* Create a specified RX queue */
+static int mvneta_rxq_init(struct mvneta_port *pp,
+  struct mvneta_rx_queue *rxq)
+
+{
+   int ret;
+
+   ret = mvneta_rxq_sw_init(pp, rxq);
+   if (ret < 0)
+   return ret;
+
+   mvneta_rxq_hw_init(pp, rxq);
 
return 0;
 }
@@ -2857,9 +2875,8 @@ static void mvneta_rxq_deinit(struct mvneta_port *pp,
rxq->descs_phys= 0;
 }
 
-/* Create and initialize a tx queue */
-static int mvneta_txq_init(struct mvneta_port *pp,
-  struct mvneta_tx_queue *txq)
+static int mvneta_txq_sw_init(struct mvneta_port *pp,
+ struct mvneta_tx_queue *txq)
 {
int cpu;
 
@@ -2872,7 +2889,6 @@ static int mvneta_txq_init(struct mvneta_port *pp,
txq->tx_stop_threshold = txq->size - MVNETA_MAX_SKB_DESCS;
txq->tx_wake_threshold = txq->tx_stop_threshold / 2;
 
-
/* Allocate memory for TX descriptors */
txq->descs = dma_alloc_coherent(pp->dev->dev.parent,
txq->size * MVNETA_DESC_ALIGNED_SIZE,
@@ -2882,14 +2898,6 @@ static int mvneta_txq_init(struct mvneta_port *pp,
 
txq->last_desc = txq->size - 1;
 
-   /* Set maximum bandwidth for enabled TXQs */
-   mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0x03ff);
-   mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0x3fff);
-
-   /* Set Tx descriptors queue starting address */
-   mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), txq->descs_phys);
-   mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), txq->size);
-
txq->tx_skb = kmalloc_array(txq->size, sizeof(*txq->tx_skb),
GFP_KERNEL);
if (!txq->tx_skb) {
@@ -2910,7 +2918,6 @@ static int mvneta_txq_init(struct mvneta_port *pp,
  txq->descs, txq->descs_phys);
return -ENOMEM;
}
-   mvneta_tx_done_pkts_coal_set(pp, txq, txq->done_pkts_coal);
 
/* Setup XPS mapping */
if (txq_number > 1)
@@ -2923,9 +2930,38 @@ static int mvneta_txq_init(struct mvneta_port *pp,
return 0;
 }
 
+static void mvneta_txq_hw_init(struct mvneta_port *pp,
+  struct mvneta_tx_queue *txq)
+{
+   /* Set maximum bandwidth for enabled TXQs */
+   mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0x03ff);
+   mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0x3fff);
+
+   /* Set Tx descriptors queue starting address */
+   mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), txq->descs_phys);
+   mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), txq->size);
+
+   mvneta_tx_done_pkts_coal_set(pp, txq, txq->done_pkts_coal);
+}
+
+/* Create and initialize a tx queue */
+static int mvneta_txq_init(struct mvneta_port *pp,
+  struct mvneta_tx_queue *txq)
+{
+   int ret;
+
+   ret = mvneta_txq_sw_init(pp, txq);
+   if (ret < 0)
+   return ret;
+
+   mvneta_txq

[PATCH v3 0/2] net: mvneta: improve suspend/resume

[Jisheng Zhang]

This series tries to optimize the mvneta's suspend/resume
implementation by only taking necessary actions.

Since v2:
 - keep rtnl lock when calling mvneta_start_dev() and mvneta_stop_dev()
   Thank Russell for pointing this out

Since v1:
 - unify ret check
 - try best to keep the suspend/resume behavior
 - split txq deinit into sw/hw parts as well
 - adjust mvneta_stop_dev() location

I didn't add Thomas's Ack tag to patch1, because in v2, I add new code
to split the txq deinit into two parts.

Jisheng Zhang (2):
  net: mvneta: split rxq/txq init and txq deinit into SW and HW parts
  net: mvneta: improve suspend/resume

 drivers/net/ethernet/marvell/mvneta.c | 154 --
 1 file changed, 128 insertions(+), 26 deletions(-)

-- 
2.16.3

[PATCH v2 2/2] net: mvneta: improve suspend/resume

[Jisheng Zhang]

Current suspend/resume implementation reuses the mvneta_open() and
mvneta_close(), but it could be optimized to take only necessary
actions during suspend/resume.

One obvious problem of current implementation is: after hundreds of
system suspend/resume cycles, the resume of mvneta could fail due to
fragmented dma coherent memory. After this patch, the non-necessary
memory alloc/free is optimized out.

Signed-off-by: Jisheng Zhang <jisheng.zh...@synaptics.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 71 ++-
 1 file changed, 61 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index f96815853108..cb7fce99ed6d 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -4586,16 +4586,43 @@ static int mvneta_remove(struct platform_device *pdev)
 #ifdef CONFIG_PM_SLEEP
 static int mvneta_suspend(struct device *device)
 {
+   int queue;
struct net_device *dev = dev_get_drvdata(device);
struct mvneta_port *pp = netdev_priv(dev);
 
-   rtnl_lock();
-   if (netif_running(dev))
-   mvneta_stop(dev);
-   rtnl_unlock();
+   if (!netif_running(dev))
+   goto clean_exit;
+
+   if (!pp->neta_armada3700) {
+   spin_lock(>lock);
+   pp->is_stopped = true;
+   spin_unlock(>lock);
+
+   cpuhp_state_remove_instance_nocalls(online_hpstate,
+   >node_online);
+   cpuhp_state_remove_instance_nocalls(CPUHP_NET_MVNETA_DEAD,
+   >node_dead);
+   }
+
+   mvneta_stop_dev(pp);
+
+   for (queue = 0; queue < rxq_number; queue++) {
+   struct mvneta_rx_queue *rxq = >rxqs[queue];
+
+   mvneta_rxq_drop_pkts(pp, rxq);
+   }
+
+   for (queue = 0; queue < txq_number; queue++) {
+   struct mvneta_tx_queue *txq = >txqs[queue];
+
+   mvneta_txq_hw_deinit(pp, txq);
+   }
+
+clean_exit:
netif_device_detach(dev);
clk_disable_unprepare(pp->clk_bus);
clk_disable_unprepare(pp->clk);
+
return 0;
 }
 
@@ -4604,7 +4631,7 @@ static int mvneta_resume(struct device *device)
struct platform_device *pdev = to_platform_device(device);
struct net_device *dev = dev_get_drvdata(device);
struct mvneta_port *pp = netdev_priv(dev);
-   int err;
+   int err, queue;
 
clk_prepare_enable(pp->clk);
if (!IS_ERR(pp->clk_bus))
@@ -4626,12 +4653,36 @@ static int mvneta_resume(struct device *device)
}
 
netif_device_attach(dev);
-   rtnl_lock();
-   if (netif_running(dev)) {
-   mvneta_open(dev);
-   mvneta_set_rx_mode(dev);
+
+   if (!netif_running(dev))
+   return 0;
+
+   for (queue = 0; queue < rxq_number; queue++) {
+   struct mvneta_rx_queue *rxq = >rxqs[queue];
+
+   rxq->next_desc_to_proc = 0;
+   mvneta_rxq_hw_init(pp, rxq);
}
-   rtnl_unlock();
+
+   for (queue = 0; queue < txq_number; queue++) {
+   struct mvneta_tx_queue *txq = >txqs[queue];
+
+   txq->next_desc_to_proc = 0;
+   mvneta_txq_hw_init(pp, txq);
+   }
+
+   if (!pp->neta_armada3700) {
+   spin_lock(>lock);
+   pp->is_stopped = false;
+   spin_unlock(>lock);
+   cpuhp_state_add_instance_nocalls(online_hpstate,
+>node_online);
+   cpuhp_state_add_instance_nocalls(CPUHP_NET_MVNETA_DEAD,
+>node_dead);
+   }
+
+   mvneta_start_dev(pp);
+   mvneta_set_rx_mode(dev);
 
return 0;
 }
-- 
2.16.3

[PATCH v2 1/2] net: mvneta: split rxq/txq init and txq deinit into SW and HW parts

[Jisheng Zhang]

This is to prepare the suspend/resume improvement in next patch. The
SW parts can be optimized out during resume.

As for rxq handling during suspend, we'd like to drop packets by
calling mvneta_rxq_drop_pkts() which is both SW and HW operation,
so we don't split rxq deinit.

Signed-off-by: Jisheng Zhang <jisheng.zh...@synaptics.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 85 +++
 1 file changed, 66 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 30aab9bf77cc..f96815853108 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -2796,10 +2796,8 @@ static void mvneta_rx_reset(struct mvneta_port *pp)
 
 /* Rx/Tx queue initialization/cleanup methods */
 
-/* Create a specified RX queue */
-static int mvneta_rxq_init(struct mvneta_port *pp,
-  struct mvneta_rx_queue *rxq)
-
+static int mvneta_rxq_sw_init(struct mvneta_port *pp,
+ struct mvneta_rx_queue *rxq)
 {
rxq->size = pp->rx_ring_size;
 
@@ -2812,6 +2810,12 @@ static int mvneta_rxq_init(struct mvneta_port *pp,
 
rxq->last_desc = rxq->size - 1;
 
+   return 0;
+}
+
+static void mvneta_rxq_hw_init(struct mvneta_port *pp,
+  struct mvneta_rx_queue *rxq)
+{
/* Set Rx descriptors queue starting address */
mvreg_write(pp, MVNETA_RXQ_BASE_ADDR_REG(rxq->id), rxq->descs_phys);
mvreg_write(pp, MVNETA_RXQ_SIZE_REG(rxq->id), rxq->size);
@@ -2835,6 +2839,20 @@ static int mvneta_rxq_init(struct mvneta_port *pp,
mvneta_rxq_short_pool_set(pp, rxq);
mvneta_rxq_non_occup_desc_add(pp, rxq, rxq->size);
}
+}
+
+/* Create a specified RX queue */
+static int mvneta_rxq_init(struct mvneta_port *pp,
+  struct mvneta_rx_queue *rxq)
+
+{
+   int ret;
+
+   ret = mvneta_rxq_sw_init(pp, rxq);
+   if (ret < 0)
+   return ret;
+
+   mvneta_rxq_hw_init(pp, rxq);
 
return 0;
 }
@@ -2857,9 +2875,8 @@ static void mvneta_rxq_deinit(struct mvneta_port *pp,
rxq->descs_phys= 0;
 }
 
-/* Create and initialize a tx queue */
-static int mvneta_txq_init(struct mvneta_port *pp,
-  struct mvneta_tx_queue *txq)
+static int mvneta_txq_sw_init(struct mvneta_port *pp,
+ struct mvneta_tx_queue *txq)
 {
int cpu;
 
@@ -2872,7 +2889,6 @@ static int mvneta_txq_init(struct mvneta_port *pp,
txq->tx_stop_threshold = txq->size - MVNETA_MAX_SKB_DESCS;
txq->tx_wake_threshold = txq->tx_stop_threshold / 2;
 
-
/* Allocate memory for TX descriptors */
txq->descs = dma_alloc_coherent(pp->dev->dev.parent,
txq->size * MVNETA_DESC_ALIGNED_SIZE,
@@ -2882,14 +2898,6 @@ static int mvneta_txq_init(struct mvneta_port *pp,
 
txq->last_desc = txq->size - 1;
 
-   /* Set maximum bandwidth for enabled TXQs */
-   mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0x03ff);
-   mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0x3fff);
-
-   /* Set Tx descriptors queue starting address */
-   mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), txq->descs_phys);
-   mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), txq->size);
-
txq->tx_skb = kmalloc_array(txq->size, sizeof(*txq->tx_skb),
GFP_KERNEL);
if (!txq->tx_skb) {
@@ -2910,7 +2918,6 @@ static int mvneta_txq_init(struct mvneta_port *pp,
  txq->descs, txq->descs_phys);
return -ENOMEM;
}
-   mvneta_tx_done_pkts_coal_set(pp, txq, txq->done_pkts_coal);
 
/* Setup XPS mapping */
if (txq_number > 1)
@@ -2923,9 +2930,38 @@ static int mvneta_txq_init(struct mvneta_port *pp,
return 0;
 }
 
+static void mvneta_txq_hw_init(struct mvneta_port *pp,
+  struct mvneta_tx_queue *txq)
+{
+   /* Set maximum bandwidth for enabled TXQs */
+   mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0x03ff);
+   mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0x3fff);
+
+   /* Set Tx descriptors queue starting address */
+   mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), txq->descs_phys);
+   mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), txq->size);
+
+   mvneta_tx_done_pkts_coal_set(pp, txq, txq->done_pkts_coal);
+}
+
+/* Create and initialize a tx queue */
+static int mvneta_txq_init(struct mvneta_port *pp,
+  struct mvneta_tx_queue *txq)
+{
+   int ret;
+
+   ret = mvneta_txq_sw_init(pp, txq);
+   if (ret < 0)
+   return ret;
+
+   mvneta_txq

[PATCH v2 0/2] net: mvneta: improve suspend/resume

[Jisheng Zhang]

This series tries to optimize the mvneta's suspend/resume
implementation by only taking necessary actions.

Since v1:
 - unify ret check
 - try best to keep the suspend/resume behavior
 - split txq deinit into sw/hw parts as well
 - adjust mvneta_stop_dev() location

I didn't add Thomas's Ack tag to patch1, because in v2, I added new code
to split the txq deinit into two parts.

Jisheng Zhang (2):
  net: mvneta: split rxq/txq init and txq deinit into SW and HW parts
  net: mvneta: improve suspend/resume

 drivers/net/ethernet/marvell/mvneta.c | 156 +++---
 1 file changed, 127 insertions(+), 29 deletions(-)

-- 
2.16.3

Re: [PATCH 2/2] net: mvneta: improve suspend/resume

[Jisheng Zhang]

On Thu, 29 Mar 2018 13:54:32 +0200 Thomas Petazzoni wrote:

> Hello Jisheng,

Hi Thomas,

> 
> On Thu, 29 Mar 2018 18:15:36 +0800, Jisheng Zhang wrote:
> > Current suspend/resume implementation reuses the mvneta_open() and
> > mvneta_close(), but it could be optimized to take only necessary
> > actions during suspend/resume.
> > 
> > One obvious problem of current implementation is: after hundreds of
> > system suspend/resume cycles, the resume of mvneta could fail due to
> > fragmented dma coherent memory. After this patch, the non-necessary
> > memory alloc/free is optimized out.  
> 
> Indeed, this needs to be fixed, you're totally right.
> 
> > Signed-off-by: Jisheng Zhang <jisheng.zh...@synaptics.com>
> > ---
> >  drivers/net/ethernet/marvell/mvneta.c | 76 
> > ++-
> >  1 file changed, 66 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> > b/drivers/net/ethernet/marvell/mvneta.c
> > index 4ec69bbd1eb4..1870f1dd7093 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -4575,14 +4575,46 @@ static int mvneta_remove(struct platform_device 
> > *pdev)
> >  #ifdef CONFIG_PM_SLEEP
> >  static int mvneta_suspend(struct device *device)
> >  {
> > +   int queue;
> > struct net_device *dev = dev_get_drvdata(device);
> > struct mvneta_port *pp = netdev_priv(dev);
> >  
> > -   rtnl_lock();
> > -   if (netif_running(dev))
> > -   mvneta_stop(dev);
> > -   rtnl_unlock();
> > +   if (!netif_running(dev))
> > +   return 0;  
> 
> This is changing the behavior I believe. The current code is:
> 
> rtnl_lock();
> if (netif_running(dev))
> mvneta_stop(dev);
> rtnl_unlock();
> netif_device_detach(dev);
> clk_disable_unprepare(pp->clk_bus);
> clk_disable_unprepare(pp->clk);
> return 0;
> 
> So, when netif_running(dev) is false, we're indeed not calling
> mvneta_stop(), but we're still doing netif_device_detach(), and
> disabling the clocks. With your change, we're no longer doing these
> steps.

Indeed, will try to keep the behavior in v2

> 
> > +
> > netif_device_detach(dev);
> > +
> > +   mvneta_stop_dev(pp);
> > +
> > +   if (!pp->neta_armada3700) {
> > +   spin_lock(>lock);
> > +   pp->is_stopped = true;
> > +   spin_unlock(>lock);  
> 
> Real question: is it OK to set pp->is_stopped *after* calling
> mvneta_stop_dev(), while it was set before calling mvneta_stop_dev() in
> the current code ?

oops, you are right. Fixed in v2

> 
> > +
> > +   cpuhp_state_remove_instance_nocalls(online_hpstate,
> > +   >node_online);
> > +   cpuhp_state_remove_instance_nocalls(CPUHP_NET_MVNETA_DEAD,
> > +   >node_dead);  
> 
> Do we need to remove/add those CPU notifiers when suspending/resuming ?

Take mvneta_cpu_online() as an example, if we don't remove it during
suspend, when system is resume back, it will touch mac when secondary
cpu is ON, but at this point the mvneta isn't resumed, this is not safe.

> 
> > +   }
> > +
> > +   for (queue = 0; queue < rxq_number; queue++) {
> > +   struct mvneta_rx_queue *rxq = >rxqs[queue];
> > +
> > +   mvneta_rxq_drop_pkts(pp, rxq);
> > +   }  
> 
> Wouldn't it make sense to have
> mvneta_rxq_sw_deinit/mvneta_rxq_hw_deinit(), like you did for the
> initialization ?

For rxq deinit, we'd like to drop rx pkts, this is both HW and SW operation.
So we reuse mvneta_rxq_drop_pkts() here.

> 
> > +
> > +   for (queue = 0; queue < txq_number; queue++) {
> > +   struct mvneta_tx_queue *txq = >txqs[queue];
> > +
> > +   /* Set minimum bandwidth for disabled TXQs */
> > +   mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0);
> > +   mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0);
> > +
> > +   /* Set Tx descriptors queue starting address and size */
> > +   mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), 0);
> > +   mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), 0);
> > +   }  
> 
> Same comment here: a mvneta_txq_sw_deinit()/mvneta_txq_hw_deinit()
> would be good, and would avoid duplicating this logic.

yep, will do in v2.

Thanks a lot for the kind review.

Re: [PATCH 1/2] net: mvneta: split rxq/txq init into SW and HW parts

[Jisheng Zhang]

Hi,

On Thu, 29 Mar 2018 13:42:59 +0200 Thomas Petazzoni wrote:

> Hello,
> 
> On Thu, 29 Mar 2018 18:13:56 +0800, Jisheng Zhang wrote:
> > This is to prepare the suspend/resume improvement in next patch. The
> > SW parts can be optimized out during resume.
> > 
> > Signed-off-by: Jisheng Zhang <jisheng.zh...@synaptics.com>  
> 
> Thanks, I have two very minor nits below, but otherwise:
> 
> Acked-by: Thomas Petazzoni <thomas.petazz...@bootlin.com>

Thanks for reviewing.

> 
> > +/* Create a specified RX queue */
> > +static int mvneta_rxq_init(struct mvneta_port *pp,
> > +  struct mvneta_rx_queue *rxq)
> > +
> > +{
> > +   int ret;
> > +
> > +   ret = mvneta_rxq_sw_init(pp, rxq);
> > +   if (ret)  
> 
> Here you're testing if (ret), while in mvneta_txq_init(), in the same
> situation, you're doing if (ret < 0). I don't have a preference for one
> or the other, but having them consistent between the two lpaces would
> be nice.

updated in v2.

> 
> > -/* Create and initialize a tx queue */
> > -static int mvneta_txq_init(struct mvneta_port *pp,
> > -  struct mvneta_tx_queue *txq)
> > +static int mvneta_txq_sw_init(struct mvneta_port *pp,
> > + struct mvneta_tx_queue *txq)
> >  {
> > int cpu;
> >  
> > @@ -2872,7 +2889,6 @@ static int mvneta_txq_init(struct mvneta_port *pp,
> > txq->tx_stop_threshold = txq->size - MVNETA_MAX_SKB_DESCS;
> > txq->tx_wake_threshold = txq->tx_stop_threshold / 2;
> >  
> > -  
> 
> Spurious change.

There's an extra blank line here, so I removed it ;)

Thanks

[PATCH 2/2] net: mvneta: improve suspend/resume

[Jisheng Zhang]

Current suspend/resume implementation reuses the mvneta_open() and
mvneta_close(), but it could be optimized to take only necessary
actions during suspend/resume.

One obvious problem of current implementation is: after hundreds of
system suspend/resume cycles, the resume of mvneta could fail due to
fragmented dma coherent memory. After this patch, the non-necessary
memory alloc/free is optimized out.

Signed-off-by: Jisheng Zhang <jisheng.zh...@synaptics.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 76 ++-
 1 file changed, 66 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 4ec69bbd1eb4..1870f1dd7093 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -4575,14 +4575,46 @@ static int mvneta_remove(struct platform_device *pdev)
 #ifdef CONFIG_PM_SLEEP
 static int mvneta_suspend(struct device *device)
 {
+   int queue;
struct net_device *dev = dev_get_drvdata(device);
struct mvneta_port *pp = netdev_priv(dev);
 
-   rtnl_lock();
-   if (netif_running(dev))
-   mvneta_stop(dev);
-   rtnl_unlock();
+   if (!netif_running(dev))
+   return 0;
+
netif_device_detach(dev);
+
+   mvneta_stop_dev(pp);
+
+   if (!pp->neta_armada3700) {
+   spin_lock(>lock);
+   pp->is_stopped = true;
+   spin_unlock(>lock);
+
+   cpuhp_state_remove_instance_nocalls(online_hpstate,
+   >node_online);
+   cpuhp_state_remove_instance_nocalls(CPUHP_NET_MVNETA_DEAD,
+   >node_dead);
+   }
+
+   for (queue = 0; queue < rxq_number; queue++) {
+   struct mvneta_rx_queue *rxq = >rxqs[queue];
+
+   mvneta_rxq_drop_pkts(pp, rxq);
+   }
+
+   for (queue = 0; queue < txq_number; queue++) {
+   struct mvneta_tx_queue *txq = >txqs[queue];
+
+   /* Set minimum bandwidth for disabled TXQs */
+   mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0);
+   mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0);
+
+   /* Set Tx descriptors queue starting address and size */
+   mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), 0);
+   mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), 0);
+   }
+
clk_disable_unprepare(pp->clk_bus);
clk_disable_unprepare(pp->clk);
return 0;
@@ -4593,7 +4625,7 @@ static int mvneta_resume(struct device *device)
struct platform_device *pdev = to_platform_device(device);
struct net_device *dev = dev_get_drvdata(device);
struct mvneta_port *pp = netdev_priv(dev);
-   int err;
+   int err, queue;
 
clk_prepare_enable(pp->clk);
if (!IS_ERR(pp->clk_bus))
@@ -4614,13 +4646,37 @@ static int mvneta_resume(struct device *device)
return err;
}
 
+   if (!netif_running(dev))
+   return 0;
+
netif_device_attach(dev);
-   rtnl_lock();
-   if (netif_running(dev)) {
-   mvneta_open(dev);
-   mvneta_set_rx_mode(dev);
+
+   for (queue = 0; queue < rxq_number; queue++) {
+   struct mvneta_rx_queue *rxq = >rxqs[queue];
+
+   rxq->next_desc_to_proc = 0;
+   mvneta_rxq_hw_init(pp, rxq);
}
-   rtnl_unlock();
+
+   for (queue = 0; queue < txq_number; queue++) {
+   struct mvneta_tx_queue *txq = >txqs[queue];
+
+   txq->next_desc_to_proc = 0;
+   mvneta_txq_hw_init(pp, txq);
+   }
+
+   if (!pp->neta_armada3700) {
+   spin_lock(>lock);
+   pp->is_stopped = false;
+   spin_unlock(>lock);
+   cpuhp_state_add_instance_nocalls(online_hpstate,
+>node_online);
+   cpuhp_state_add_instance_nocalls(CPUHP_NET_MVNETA_DEAD,
+>node_dead);
+   }
+
+   mvneta_set_rx_mode(dev);
+   mvneta_start_dev(pp);
 
return 0;
 }
-- 
2.16.3

[PATCH 1/2] net: mvneta: split rxq/txq init into SW and HW parts

[Jisheng Zhang]

This is to prepare the suspend/resume improvement in next patch. The
SW parts can be optimized out during resume.

Signed-off-by: Jisheng Zhang <jisheng.zh...@synaptics.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 70 ++-
 1 file changed, 53 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 30aab9bf77cc..4ec69bbd1eb4 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -2796,10 +2796,8 @@ static void mvneta_rx_reset(struct mvneta_port *pp)
 
 /* Rx/Tx queue initialization/cleanup methods */
 
-/* Create a specified RX queue */
-static int mvneta_rxq_init(struct mvneta_port *pp,
-  struct mvneta_rx_queue *rxq)
-
+static int mvneta_rxq_sw_init(struct mvneta_port *pp,
+ struct mvneta_rx_queue *rxq)
 {
rxq->size = pp->rx_ring_size;
 
@@ -2812,6 +2810,12 @@ static int mvneta_rxq_init(struct mvneta_port *pp,
 
rxq->last_desc = rxq->size - 1;
 
+   return 0;
+}
+
+static void mvneta_rxq_hw_init(struct mvneta_port *pp,
+  struct mvneta_rx_queue *rxq)
+{
/* Set Rx descriptors queue starting address */
mvreg_write(pp, MVNETA_RXQ_BASE_ADDR_REG(rxq->id), rxq->descs_phys);
mvreg_write(pp, MVNETA_RXQ_SIZE_REG(rxq->id), rxq->size);
@@ -2835,6 +2839,20 @@ static int mvneta_rxq_init(struct mvneta_port *pp,
mvneta_rxq_short_pool_set(pp, rxq);
mvneta_rxq_non_occup_desc_add(pp, rxq, rxq->size);
}
+}
+
+/* Create a specified RX queue */
+static int mvneta_rxq_init(struct mvneta_port *pp,
+  struct mvneta_rx_queue *rxq)
+
+{
+   int ret;
+
+   ret = mvneta_rxq_sw_init(pp, rxq);
+   if (ret)
+   return ret;
+
+   mvneta_rxq_hw_init(pp, rxq);
 
return 0;
 }
@@ -2857,9 +2875,8 @@ static void mvneta_rxq_deinit(struct mvneta_port *pp,
rxq->descs_phys= 0;
 }
 
-/* Create and initialize a tx queue */
-static int mvneta_txq_init(struct mvneta_port *pp,
-  struct mvneta_tx_queue *txq)
+static int mvneta_txq_sw_init(struct mvneta_port *pp,
+ struct mvneta_tx_queue *txq)
 {
int cpu;
 
@@ -2872,7 +2889,6 @@ static int mvneta_txq_init(struct mvneta_port *pp,
txq->tx_stop_threshold = txq->size - MVNETA_MAX_SKB_DESCS;
txq->tx_wake_threshold = txq->tx_stop_threshold / 2;
 
-
/* Allocate memory for TX descriptors */
txq->descs = dma_alloc_coherent(pp->dev->dev.parent,
txq->size * MVNETA_DESC_ALIGNED_SIZE,
@@ -2882,14 +2898,6 @@ static int mvneta_txq_init(struct mvneta_port *pp,
 
txq->last_desc = txq->size - 1;
 
-   /* Set maximum bandwidth for enabled TXQs */
-   mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0x03ff);
-   mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0x3fff);
-
-   /* Set Tx descriptors queue starting address */
-   mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), txq->descs_phys);
-   mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), txq->size);
-
txq->tx_skb = kmalloc_array(txq->size, sizeof(*txq->tx_skb),
GFP_KERNEL);
if (!txq->tx_skb) {
@@ -2910,7 +2918,6 @@ static int mvneta_txq_init(struct mvneta_port *pp,
  txq->descs, txq->descs_phys);
return -ENOMEM;
}
-   mvneta_tx_done_pkts_coal_set(pp, txq, txq->done_pkts_coal);
 
/* Setup XPS mapping */
if (txq_number > 1)
@@ -2923,6 +2930,35 @@ static int mvneta_txq_init(struct mvneta_port *pp,
return 0;
 }
 
+static void mvneta_txq_hw_init(struct mvneta_port *pp,
+  struct mvneta_tx_queue *txq)
+{
+   /* Set maximum bandwidth for enabled TXQs */
+   mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0x03ff);
+   mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0x3fff);
+
+   /* Set Tx descriptors queue starting address */
+   mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), txq->descs_phys);
+   mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), txq->size);
+
+   mvneta_tx_done_pkts_coal_set(pp, txq, txq->done_pkts_coal);
+}
+
+/* Create and initialize a tx queue */
+static int mvneta_txq_init(struct mvneta_port *pp,
+  struct mvneta_tx_queue *txq)
+{
+   int ret;
+
+   ret = mvneta_txq_sw_init(pp, txq);
+   if (ret < 0)
+   return ret;
+
+   mvneta_txq_hw_init(pp, txq);
+
+   return 0;
+}
+
 /* Free allocated resources when mvneta_txq_init() fails to allocate memory*/
 static void mvneta_txq_deinit(struct mvneta_port *pp,
  struct mvneta_tx_queue *txq)
-- 
2.16.3

[PATCH 0/2] net: mvneta: improve suspend/resume

[Jisheng Zhang]

This series tries to optimize the mvneta's suspend/resume
implementation by only taking necessary actions.

Jisheng Zhang (2):
  net: mvneta: split rxq/txq init into SW and HW parts
  net: mvneta: improve suspend/resume

 drivers/net/ethernet/marvell/mvneta.c | 146 +++---
 1 file changed, 119 insertions(+), 27 deletions(-)

-- 
2.16.3

[PATCH] net: mvneta: remove duplicate *_coal assignment

[Jisheng Zhang]

The style of the rx/tx queue's *_coal member assignment is:

static void foo_coal_set(...)
{
set the coal in hw;
update queue's foo_coal member; [1]
}

In other place, we call foo_coal_set(pp, queue->foo_coal), so the above [1]
is duplicated and could be removed.

Signed-off-by: Jisheng Zhang <jisheng.zh...@synaptics.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index eaa4bb80f1c9..30aab9bf77cc 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -1555,7 +1555,6 @@ static void mvneta_rx_pkts_coal_set(struct mvneta_port 
*pp,
 {
mvreg_write(pp, MVNETA_RXQ_THRESHOLD_REG(rxq->id),
value | MVNETA_RXQ_NON_OCCUPIED(0));
-   rxq->pkts_coal = value;
 }
 
 /* Set the time delay in usec before RX interrupt will be generated by
@@ -1571,7 +1570,6 @@ static void mvneta_rx_time_coal_set(struct mvneta_port 
*pp,
val = (clk_rate / 100) * value;
 
mvreg_write(pp, MVNETA_RXQ_TIME_COAL_REG(rxq->id), val);
-   rxq->time_coal = value;
 }
 
 /* Set threshold for TX_DONE pkts coalescing */
@@ -1586,8 +1584,6 @@ static void mvneta_tx_done_pkts_coal_set(struct 
mvneta_port *pp,
val |= MVNETA_TXQ_SENT_THRESH_MASK(value);
 
mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), val);
-
-   txq->done_pkts_coal = value;
 }
 
 /* Handle rx descriptor fill by setting buf_cookie and buf_phys_addr */
-- 
2.16.3

[PATCH net-next] net: mvneta: fix failed to suspend if WOL is enabled

[Jisheng Zhang]

Recently, suspend/resume and WOL support are added into mvneta driver.
If we enable WOL, then we get some error as below on Marvell BG4CT
platforms during suspend:

[  184.149723] dpm_run_callback(): mdio_bus_suspend+0x0/0x50 returns -16
[  184.149727] PM: Device f7b62004.mdio-mi:00 failed to suspend: error -16

-16 means -EBUSY, phy_suspend() will return -EBUSY if it finds the
device has WOL enabled.

We fix this issue by properly setting the netdev's power.can_wakeup
and power.wakeup, i.e

1. in mvneta_mdio_probe(), call device_set_wakeup_capable() to set
power.can_wakeup if the phy support WOL.

2. in mvneta_ethtool_set_wol(), call device_set_wakeup_enable() to
set power.wakeup if WOL has been successfully enabled in phy.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 34a3686d2ce6..0992db47070f 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -3318,6 +3318,7 @@ static void mvneta_adjust_link(struct net_device *ndev)
 static int mvneta_mdio_probe(struct mvneta_port *pp)
 {
struct phy_device *phy_dev;
+   struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
 
phy_dev = of_phy_connect(pp->dev, pp->phy_node, mvneta_adjust_link, 0,
 pp->phy_interface);
@@ -3326,6 +3327,9 @@ static int mvneta_mdio_probe(struct mvneta_port *pp)
return -ENODEV;
}
 
+   phy_ethtool_get_wol(phy_dev, );
+   device_set_wakeup_capable(>dev->dev, !!wol.supported);
+
phy_dev->supported &= PHY_GBIT_FEATURES;
phy_dev->advertising = phy_dev->supported;
 
@@ -3942,10 +3946,16 @@ static void mvneta_ethtool_get_wol(struct net_device 
*dev,
 static int mvneta_ethtool_set_wol(struct net_device *dev,
  struct ethtool_wolinfo *wol)
 {
+   int ret;
+
if (!dev->phydev)
return -EOPNOTSUPP;
 
-   return phy_ethtool_set_wol(dev->phydev, wol);
+   ret = phy_ethtool_set_wol(dev->phydev, wol);
+   if (!ret)
+   device_set_wakeup_enable(>dev, !!wol->wolopts);
+
+   return ret;
 }
 
 static const struct net_device_ops mvneta_netdev_ops = {
-- 
2.11.0

[PATCH net-next v2] net: mvneta: set rx mode during resume if interface is running

[Jisheng Zhang]

I found a bug by:

0. boot and start dhcp client
1. echo mem > /sys/power/state
2. resume back immediately
3. don't touch dhcp client to renew the lease
4. ping the gateway. No acks

Usually, after step2, the DHCP lease isn't expired, so in theory we
should resume all back. But in fact, it doesn't. It turns out
the rx mode isn't resumed correctly. This patch fixes it by adding
mvneta_set_rx_mode(dev) in the resume hook if interface is running.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
Since v1:
 - rebased to the latest net-next tree and explictly mention it

 drivers/net/ethernet/marvell/mvneta.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index aebbc5399a06..cc126204dc4d 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -4449,8 +4449,11 @@ static int mvneta_resume(struct device *device)
mvneta_fixed_link_update(pp, dev->phydev);
 
netif_device_attach(dev);
-   if (netif_running(dev))
+   if (netif_running(dev)) {
mvneta_open(dev);
+   mvneta_set_rx_mode(dev);
+   }
+
return 0;
 }
 #endif
-- 
2.11.0

[PATCH net-next] net: mvneta: add RGMII_RXID and RGMII_TXID support

[Jisheng Zhang]

RGMII_RXID and RGMII_TX_ID share the same GMAC CTRL setting as RGMII
or RGMII_ID.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index aebbc5399a06..7a6c65b44d7e 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -4099,6 +4099,8 @@ static int mvneta_port_power_up(struct mvneta_port *pp, 
int phy_mode)
break;
case PHY_INTERFACE_MODE_RGMII:
case PHY_INTERFACE_MODE_RGMII_ID:
+   case PHY_INTERFACE_MODE_RGMII_RXID:
+   case PHY_INTERFACE_MODE_RGMII_TXID:
ctrl |= MVNETA_GMAC2_PORT_RGMII;
break;
default:
-- 
2.11.0

Re: [PATCH] net: mvneta: set rx mode during resume if interface is running

[Jisheng Zhang]

Dear David,

On Mon, 27 Mar 2017 16:15:34 -0700 David Miller wrote:

> From: Jisheng Zhang <jszh...@marvell.com>
> Date: Mon, 27 Mar 2017 18:59:05 +0800
> 
> > I found a bug by:
> > 
> > 0. boot and start dhcp client
> > 1. echo mem > /sys/power/state
> > 2. resume back immediately
> > 3. don't touch dhcp client to renew the lease
> > 4. ping the gateway. No acks
> > 
> > Usually, after step2, the DHCP lease isn't expired, so in theory we
> > should resume all back. But in fact, it doesn't. It turns out
> > the rx mode isn't resumed correctly. This patch fixes it by adding
> > mvneta_set_rx_mode(dev) in the resume hook if interface is running.
> > 
> > Signed-off-by: Jisheng Zhang <jszh...@marvell.com>  
> 
> This doesn't apply cleanly to the net tree, please respin.

This patch is generated against net-next, for mvneta suspend/resume support
is added into net-next recently. I did need to use the "[PATCH net-next]" for
the patch title, will take care in the future.

Sorry for confusion,
Jisheng

[PATCH] net: mvneta: set rx mode during resume if interface is running

[Jisheng Zhang]

I found a bug by:

0. boot and start dhcp client
1. echo mem > /sys/power/state
2. resume back immediately
3. don't touch dhcp client to renew the lease
4. ping the gateway. No acks

Usually, after step2, the DHCP lease isn't expired, so in theory we
should resume all back. But in fact, it doesn't. It turns out
the rx mode isn't resumed correctly. This patch fixes it by adding
mvneta_set_rx_mode(dev) in the resume hook if interface is running.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index aebbc5399a06..cc126204dc4d 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -4449,8 +4449,11 @@ static int mvneta_resume(struct device *device)
mvneta_fixed_link_update(pp, dev->phydev);
 
netif_device_attach(dev);
-   if (netif_running(dev))
+   if (netif_running(dev)) {
mvneta_open(dev);
+   mvneta_set_rx_mode(dev);
+   }
+
return 0;
 }
 #endif
-- 
2.11.0

Re: [PATCH v3] net: mvneta: support suspend and resume

[Jisheng Zhang]

On Thu, 16 Mar 2017 15:11:48 +0800 Jane Li wrote:

> Add basic support for handling suspend and resume.
> 
> Signed-off-by: Jane Li <j...@marvell.com>
> ---
> Since v2:
> - use SIMPLE_DEV_PM_OPS instead of SET_LATE_SYSTEM_SLEEP_PM_OPS
> 
> Since v1:
> - add mvneta_conf_mbus_windows() and mvneta_bm_port_init() in mvneta_resume()
> 
>  drivers/net/ethernet/marvell/mvneta.c | 60 
> ---
>  1 file changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> b/drivers/net/ethernet/marvell/mvneta.c
> index 61dd446..b0fea26 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -431,6 +431,7 @@ struct mvneta_port {
>   /* Flags for special SoC configurations */
>   bool neta_armada3700;
>   u16 rx_offset_correction;
> + const struct mbus_dram_target_info *dram_target_info;
>  };
>  
>  /* The mvneta_tx_desc and mvneta_rx_desc structures describe the
> @@ -4118,7 +4119,6 @@ static int mvneta_port_power_up(struct mvneta_port *pp, 
> int phy_mode)
>  /* Device initialization routine */
>  static int mvneta_probe(struct platform_device *pdev)
>  {
> - const struct mbus_dram_target_info *dram_target_info;
>   struct resource *res;
>   struct device_node *dn = pdev->dev.of_node;
>   struct device_node *phy_node;
> @@ -4267,13 +4267,13 @@ static int mvneta_probe(struct platform_device *pdev)
>  
>   pp->tx_csum_limit = tx_csum_limit;
>  
> - dram_target_info = mv_mbus_dram_info();
> + pp->dram_target_info = mv_mbus_dram_info();
>   /* Armada3700 requires setting default configuration of Mbus
>* windows, however without using filled mbus_dram_target_info
>* structure.
>*/
> - if (dram_target_info || pp->neta_armada3700)
> - mvneta_conf_mbus_windows(pp, dram_target_info);
> + if (pp->dram_target_info || pp->neta_armada3700)
> + mvneta_conf_mbus_windows(pp, pp->dram_target_info);
>  
>   pp->tx_ring_size = MVNETA_MAX_TXD;
>   pp->rx_ring_size = MVNETA_MAX_RXD;
> @@ -4405,6 +4405,57 @@ static int mvneta_remove(struct platform_device *pdev)
>   return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int mvneta_suspend(struct device *device)
> +{
> + struct net_device *dev = dev_get_drvdata(device);
> + struct mvneta_port *pp = netdev_priv(dev);
> +
> + if (netif_running(dev))
> + mvneta_stop(dev);
> + netif_device_detach(dev);
> + clk_disable_unprepare(pp->clk_bus);
> + clk_disable_unprepare(pp->clk);
> + return 0;
> +}
> +
> +static int mvneta_resume(struct device *device)
> +{
> + struct platform_device *pdev = to_platform_device(device);
> + struct net_device *dev = dev_get_drvdata(device);
> + struct mvneta_port *pp = netdev_priv(dev);
> + int err;
> +
> + clk_prepare_enable(pp->clk);
> + clk_prepare_enable(pp->clk_bus);

hmm, since clk_bus is optional, it's better to add check here.

Except the above, you could add

Reviewed-by: Jisheng Zhang <jszh...@marvell.com>

> + if (pp->dram_target_info || pp->neta_armada3700)
> + mvneta_conf_mbus_windows(pp, pp->dram_target_info);
> + if (pp->bm_priv) {
> + err = mvneta_bm_port_init(pdev, pp);
> + if (err < 0) {
> + dev_info(>dev, "use SW buffer management\n");
> + pp->bm_priv = NULL;
> + }
> + }
> + mvneta_defaults_set(pp);
> + err = mvneta_port_power_up(pp, pp->phy_interface);
> + if (err < 0) {
> + dev_err(device, "can't power up port\n");
> + return err;
> + }
> +
> + if (pp->use_inband_status)
> + mvneta_fixed_link_update(pp, dev->phydev);
> +
> + netif_device_attach(dev);
> + if (netif_running(dev))
> + mvneta_open(dev);
> + return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(mvneta_pm_ops, mvneta_suspend, mvneta_resume);
> +
>  static const struct of_device_id mvneta_match[] = {
>   { .compatible = "marvell,armada-370-neta" },
>   { .compatible = "marvell,armada-xp-neta" },
> @@ -4419,6 +4470,7 @@ static int mvneta_remove(struct platform_device *pdev)
>   .driver = {
>   .name = MVNETA_DRIVER_NAME,
>   .of_match_table = mvneta_match,
> + .pm = _pm_ops,
>   },
>  };
>  

Re: [PATCH v2] net: mvneta: support suspend and resume

[Jisheng Zhang]

On Thu, 16 Mar 2017 11:19:10 +0800 Jane Li wrote:

> Add basic support for handling suspend and resume.
> 
> Signed-off-by: Jane Li 
> ---
> Since v1:
> - add mvneta_conf_mbus_windows() and mvneta_bm_port_init() in mvneta_resume()
> 
>  drivers/net/ethernet/marvell/mvneta.c | 62 
> ---
>  1 file changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> b/drivers/net/ethernet/marvell/mvneta.c
> index 61dd446..250017b 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -431,6 +431,7 @@ struct mvneta_port {
>   /* Flags for special SoC configurations */
>   bool neta_armada3700;
>   u16 rx_offset_correction;
> + const struct mbus_dram_target_info *dram_target_info;
>  };
>  
>  /* The mvneta_tx_desc and mvneta_rx_desc structures describe the
> @@ -4118,7 +4119,6 @@ static int mvneta_port_power_up(struct mvneta_port *pp, 
> int phy_mode)
>  /* Device initialization routine */
>  static int mvneta_probe(struct platform_device *pdev)
>  {
> - const struct mbus_dram_target_info *dram_target_info;
>   struct resource *res;
>   struct device_node *dn = pdev->dev.of_node;
>   struct device_node *phy_node;
> @@ -4267,13 +4267,13 @@ static int mvneta_probe(struct platform_device *pdev)
>  
>   pp->tx_csum_limit = tx_csum_limit;
>  
> - dram_target_info = mv_mbus_dram_info();
> + pp->dram_target_info = mv_mbus_dram_info();
>   /* Armada3700 requires setting default configuration of Mbus
>* windows, however without using filled mbus_dram_target_info
>* structure.
>*/
> - if (dram_target_info || pp->neta_armada3700)
> - mvneta_conf_mbus_windows(pp, dram_target_info);
> + if (pp->dram_target_info || pp->neta_armada3700)
> + mvneta_conf_mbus_windows(pp, pp->dram_target_info);
>  
>   pp->tx_ring_size = MVNETA_MAX_TXD;
>   pp->rx_ring_size = MVNETA_MAX_RXD;
> @@ -4405,6 +4405,59 @@ static int mvneta_remove(struct platform_device *pdev)
>   return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int mvneta_suspend(struct device *device)
> +{
> + struct net_device *dev = dev_get_drvdata(device);
> + struct mvneta_port *pp = netdev_priv(dev);
> +
> + if (netif_running(dev))
> + mvneta_stop(dev);
> + netif_device_detach(dev);
> + clk_disable_unprepare(pp->clk_bus);
> + clk_disable_unprepare(pp->clk);
> + return 0;
> +}
> +
> +static int mvneta_resume(struct device *device)
> +{
> + struct platform_device *pdev = to_platform_device(device);
> + struct net_device *dev = dev_get_drvdata(device);
> + struct mvneta_port *pp = netdev_priv(dev);
> + int err;
> +
> + clk_prepare_enable(pp->clk);
> + clk_prepare_enable(pp->clk_bus);
> + if (pp->dram_target_info || pp->neta_armada3700)
> + mvneta_conf_mbus_windows(pp, pp->dram_target_info);
> + if (pp->bm_priv) {
> + err = mvneta_bm_port_init(pdev, pp);
> + if (err < 0) {
> + dev_info(>dev, "use SW buffer management\n");
> + pp->bm_priv = NULL;
> + }
> + }
> + mvneta_defaults_set(pp);
> + err = mvneta_port_power_up(pp, pp->phy_interface);
> + if (err < 0) {
> + dev_err(device, "can't power up port\n");
> + return err;
> + }
> +
> + if (pp->use_inband_status)
> + mvneta_fixed_link_update(pp, dev->phydev);
> +
> + netif_device_attach(dev);
> + if (netif_running(dev))
> + mvneta_open(dev);
> + return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops mvneta_pm_ops = {
> + SET_LATE_SYSTEM_SLEEP_PM_OPS(mvneta_suspend, mvneta_resume)
> +};

we could make use of SIMPLE_DEV_PM_OPS to simplify the code

Thanks

Re: [PATCH] net: mvneta: support suspend and resume

[Jisheng Zhang]

Hi Jane,

On Wed, 15 Mar 2017 15:08:34 +0800 Jane Li  wrote:

> Add basic support for handling suspend and resume.
> 
> Signed-off-by: Jane Li 
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 44 
> +++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> b/drivers/net/ethernet/marvell/mvneta.c
> index 61dd446..4f16342 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -4405,6 +4405,49 @@ static int mvneta_remove(struct platform_device *pdev)
>   return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int mvneta_suspend(struct device *device)
> +{
> + struct net_device *dev = dev_get_drvdata(device);
> + struct mvneta_port *pp = netdev_priv(dev);
> +
> + if (netif_running(dev))
> + mvneta_stop(dev);
> + netif_device_detach(dev);
> + clk_disable_unprepare(pp->clk_bus);
> + clk_disable_unprepare(pp->clk);
> + return 0;
> +}
> +
> +static int mvneta_resume(struct device *device)
> +{
> + struct net_device *dev = dev_get_drvdata(device);
> + struct mvneta_port *pp = netdev_priv(dev);
> + int err;
> +
> + clk_prepare_enable(pp->clk);
> + clk_prepare_enable(pp->clk_bus);

we may miss the necessary registers setting in mvneta_bm_port_init() and
mvneta_conf_mbus_windows(). those registers also need to be restored.


> + mvneta_defaults_set(pp);

before restore the default setting, is it safer to mvneta_port_disable()?

Thanks,
Jisheng

> + err = mvneta_port_power_up(pp, pp->phy_interface);
> + if (err < 0) {
> + dev_err(device, "can't power up port\n");
> + return err;
> + }
> +
> + if (pp->use_inband_status)
> + mvneta_fixed_link_update(pp, dev->phydev);
> +
> + netif_device_attach(dev);
> + if (netif_running(dev))
> + mvneta_open(dev);
> + return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops mvneta_pm_ops = {
> + SET_LATE_SYSTEM_SLEEP_PM_OPS(mvneta_suspend, mvneta_resume)
> +};
> +
>  static const struct of_device_id mvneta_match[] = {
>   { .compatible = "marvell,armada-370-neta" },
>   { .compatible = "marvell,armada-xp-neta" },
> @@ -4419,6 +4462,7 @@ static int mvneta_remove(struct platform_device *pdev)
>   .driver = {
>   .name = MVNETA_DRIVER_NAME,
>   .of_match_table = mvneta_match,
> + .pm = _pm_ops,
>   },
>  };
>  

Re: [PATCH net-next v3 0/4] net: mvneta: improve rx/tx performance

[Jisheng Zhang]

Hi David, Marcin,

On Tue, 21 Feb 2017 11:16:02 -0500 David Miller wrote:

> From: Jisheng Zhang <jszh...@marvell.com>
> Date: Tue, 21 Feb 2017 12:37:40 +0800
> 
> > Thanks for your review.
> > 
> > The measurement is simple: record how much time we spent in mvneta_rx_swbm()
> > for receiving 1GB data, something as below:  
> 
> Please use a standard tool for measuring performance, rather than profiling
> the driver and trying to derive numbers that way.

Got your point. I will try to get performance with standard tool and cook a
v4 once rc1 is released.

Thanks,
Jisheng

Re: [PATCH net-next v3 0/4] net: mvneta: improve rx/tx performance

[Jisheng Zhang]

Hi Gregory,

On Mon, 20 Feb 2017 15:21:35 +0100 Gregory CLEMENT wrote:

> Hi Jisheng,
>  
>  On lun., févr. 20 2017, Jisheng Zhang <jszh...@marvell.com> wrote:
> 
> > In hot code path such as mvneta_rx_swbm(), we access fields of rx_desc
> > and tx_desc. These DMA descs are allocated by dma_alloc_coherent, they
> > are uncacheable if the device isn't cache coherent, reading from
> > uncached memory is fairly slow.
> >
> > patch1 reuses the read out status to getting status field of rx_desc
> > again.
> >
> > patch2 avoids getting buf_phys_addr from rx_desc again in
> > mvneta_rx_hwbm by reusing the phys_addr variable.
> >
> > patch3 avoids reading from tx_desc as much as possible by store what
> > we need in local variable.
> >
> > We get the following performance data on Marvell BG4CT Platforms
> > (tested with iperf):
> >
> > before the patch:
> > sending 1GB in mvneta_tx()(disabled TSO) costs 793553760ns
> >
> > after the patch:
> > sending 1GB in mvneta_tx()(disabled TSO) costs 719953800ns
> >
> > we saved 9.2% time.
> >
> > patch4 uses cacheable memory to store the rx buffer DMA address.
> >
> > We get the following performance data on Marvell BG4CT Platforms
> > (tested with iperf):
> >
> > before the patch:
> > recving 1GB in mvneta_rx_swbm() costs 1492659600 ns
> >
> > after the patch:
> > recving 1GB in mvneta_rx_swbm() costs 1421565640 ns  
> 
> Could you explain who you get this number?

Thanks for your review.

The measurement is simple: record how much time we spent in mvneta_rx_swbm()
for receiving 1GB data, something as below:

mvneta_rx_swbm()
{
static u64 total_time;
u64 t1, t2;
static u64 count;

t1 = sched_clock();
...

if (rcvd_pkts) {
...
t2 = sched_clock() - t1;
total_time += t2;
count += rcvd_bytes;;
if (count >= 0x4000) {
printk(" %lld %lld\n", total_time, count);
total_time = 0;
count = 0;
}
...
}

> 
> receiving 1GB in 1.42 second means having a bandwidth of
> 8/1.42=5.63 Gb/s, that means that you are using at least a 10Gb
> interface.

hmmm, we just measured the time spent in mvneta_rx_swbm(), so we can't solve
the bandwidth as 8/1.42, what do you think?

> 
> When I used iperf I didn't have this kind of granularity:
> iperf -c 192.168.10.1 -n 1024M
> 
> Client connecting to 192.168.10.19, TCP port 5001
> TCP window size: 43.8 KByte (default)
> 
> [  3] local 192.168.10.28 port 53086 connected with 192.168.10.1 port 5001
> [ ID] Interval   Transfer Bandwidth
> [  3]  0.0- 9.1 sec  1.00 GBytes   942 Mbits/sec
> 
> Also without HWBM enabled (so with the same configuration of your test),
> I didn't noticed any improvement with the patch set applied. But at

>From bandwidth point of view, yes, there's no improvement. But from cpu
time/load point of view, I do see a trivial improvement. Could you also
did a simple test from your side to see whether we have similar improvement
data?

Thanks,
Jisheng



> least I didn't see any regression with or without HWBM.
> 
> Gregory
> 
> >
> > We saved 4.76% time.
> >
> > Basically, patch1 and patch4 do what Arnd mentioned in [1].
> >
> > Hi Arnd,
> >
> > I added "Suggested-by you" tag, I hope you don't mind ;)
> >
> > Thanks
> >
> > [1] https://www.spinics.net/lists/netdev/msg405889.html
> >
> > Since v2:
> >   - add Gregory's ack to patch1
> >   - only get rx buffer DMA address from cacheable memory for 
> > mvneta_rx_swbm()
> >   - add patch 2 to read rx_desc->buf_phys_addr once in mvneta_rx_hwbm()
> >   - add patch 3 to avoid reading from tx_desc as much as possible
> >
> > Since v1:
> >   - correct the performance data typo
> >
> >
> > Jisheng Zhang (4):
> >   net: mvneta: avoid getting status from rx_desc as much as possible
> >   net: mvneta: avoid getting buf_phys_addr from rx_desc again
> >   net: mvneta: avoid reading from tx_desc as much as possible
> >   net: mvneta: Use cacheable memory to store the rx buffer DMA address
> >
> >  drivers/net/ethernet/marvell/mvneta.c | 80 
> > +++
> >  1 file changed, 43 insertions(+), 37 deletions(-)
> >
> > -- 
> > 2.11.0
> >  
> 

[PATCH net-next v3 1/4] net: mvneta: avoid getting status from rx_desc as much as possible

[Jisheng Zhang]

In hot code path mvneta_rx_hwbm(), the rx_desc->status is read twice.
The rx_desc is allocated by dma_alloc_coherent, it's uncacheable if
the device isn't cache-coherent, reading from uncached memory is
fairly slow. So reuse the read out rx_status to avoid the second
reading from uncached memory.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
Suggested-by: Arnd Bergmann <a...@arndb.de>
Tested-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 61dd4462411c..06df72b8da85 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -313,8 +313,8 @@
((addr >= txq->tso_hdrs_phys) && \
 (addr < txq->tso_hdrs_phys + txq->size * TSO_HEADER_SIZE))
 
-#define MVNETA_RX_GET_BM_POOL_ID(rxd) \
-   (((rxd)->status & MVNETA_RXD_BM_POOL_MASK) >> MVNETA_RXD_BM_POOL_SHIFT)
+#define MVNETA_RX_GET_BM_POOL_ID(status) \
+   (((status) & MVNETA_RXD_BM_POOL_MASK) >> MVNETA_RXD_BM_POOL_SHIFT)
 
 struct mvneta_statistic {
unsigned short offset;
@@ -1900,7 +1900,7 @@ static void mvneta_rxq_drop_pkts(struct mvneta_port *pp,
for (i = 0; i < rx_done; i++) {
struct mvneta_rx_desc *rx_desc =
  mvneta_rxq_next_desc_get(rxq);
-   u8 pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc);
+   u8 pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc->status);
struct mvneta_bm_pool *bm_pool;
 
bm_pool = >bm_priv->bm_pools[pool_id];
@@ -2075,7 +2075,7 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int 
rx_todo,
rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
data = (u8 *)(uintptr_t)rx_desc->buf_cookie;
phys_addr = rx_desc->buf_phys_addr;
-   pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc);
+   pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_status);
bm_pool = >bm_priv->bm_pools[pool_id];
 
if (!mvneta_rxq_desc_is_first_last(rx_status) ||
-- 
2.11.0

[PATCH net-next v3 0/4] net: mvneta: improve rx/tx performance

[Jisheng Zhang]

In hot code path such as mvneta_rx_swbm(), we access fields of rx_desc
and tx_desc. These DMA descs are allocated by dma_alloc_coherent, they
are uncacheable if the device isn't cache coherent, reading from
uncached memory is fairly slow.

patch1 reuses the read out status to getting status field of rx_desc
again.

patch2 avoids getting buf_phys_addr from rx_desc again in
mvneta_rx_hwbm by reusing the phys_addr variable.

patch3 avoids reading from tx_desc as much as possible by store what
we need in local variable.

We get the following performance data on Marvell BG4CT Platforms
(tested with iperf):

before the patch:
sending 1GB in mvneta_tx()(disabled TSO) costs 793553760ns

after the patch:
sending 1GB in mvneta_tx()(disabled TSO) costs 719953800ns

we saved 9.2% time.

patch4 uses cacheable memory to store the rx buffer DMA address.

We get the following performance data on Marvell BG4CT Platforms
(tested with iperf):

before the patch:
recving 1GB in mvneta_rx_swbm() costs 1492659600 ns

after the patch:
recving 1GB in mvneta_rx_swbm() costs 1421565640 ns

We saved 4.76% time.

Basically, patch1 and patch4 do what Arnd mentioned in [1].

Hi Arnd,

I added "Suggested-by you" tag, I hope you don't mind ;)

Thanks

[1] https://www.spinics.net/lists/netdev/msg405889.html

Since v2:
  - add Gregory's ack to patch1
  - only get rx buffer DMA address from cacheable memory for mvneta_rx_swbm()
  - add patch 2 to read rx_desc->buf_phys_addr once in mvneta_rx_hwbm()
  - add patch 3 to avoid reading from tx_desc as much as possible

Since v1:
  - correct the performance data typo


Jisheng Zhang (4):
  net: mvneta: avoid getting status from rx_desc as much as possible
  net: mvneta: avoid getting buf_phys_addr from rx_desc again
  net: mvneta: avoid reading from tx_desc as much as possible
  net: mvneta: Use cacheable memory to store the rx buffer DMA address

 drivers/net/ethernet/marvell/mvneta.c | 80 +++
 1 file changed, 43 insertions(+), 37 deletions(-)

-- 
2.11.0

[PATCH net-next v3 4/4] net: mvneta: Use cacheable memory to store the rx buffer DMA address

[Jisheng Zhang]

In hot code path such as mvneta_rx_swbm(), the buf_phys_addr field of
rx_dec is accessed. The rx_desc is allocated by dma_alloc_coherent,
it's uncacheable if the device isn't cache coherent, reading from
uncached memory is fairly slow. This patch uses cacheable memory to
store the rx buffer DMA address. We get the following performance data
on Marvell BG4CT Platforms (tested with iperf):

before the patch:
recving 1GB in mvneta_rx_swbm() costs 1492659600 ns

after the patch:
recving 1GB in mvneta_rx_swbm() costs 1421565640 ns

We saved 4.76% time.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
Suggested-by: Arnd Bergmann <a...@arndb.de>
---
 drivers/net/ethernet/marvell/mvneta.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index b6cda4131c78..ccd3f2601446 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -580,6 +580,9 @@ struct mvneta_rx_queue {
/* Virtual address of the RX buffer */
void  **buf_virt_addr;
 
+   /* DMA address of the RX buffer */
+   dma_addr_t *buf_dma_addr;
+
/* Virtual address of the RX DMA descriptors array */
struct mvneta_rx_desc *descs;
 
@@ -1617,6 +1620,7 @@ static void mvneta_rx_desc_fill(struct mvneta_rx_desc 
*rx_desc,
 
rx_desc->buf_phys_addr = phys_addr;
i = rx_desc - rxq->descs;
+   rxq->buf_dma_addr[i] = phys_addr;
rxq->buf_virt_addr[i] = virt_addr;
 }
 
@@ -1912,10 +1916,9 @@ static void mvneta_rxq_drop_pkts(struct mvneta_port *pp,
}
 
for (i = 0; i < rxq->size; i++) {
-   struct mvneta_rx_desc *rx_desc = rxq->descs + i;
void *data = rxq->buf_virt_addr[i];
 
-   dma_unmap_single(pp->dev->dev.parent, rx_desc->buf_phys_addr,
+   dma_unmap_single(pp->dev->dev.parent, rxq->buf_dma_addr[i],
 MVNETA_RX_BUF_SIZE(pp->pkt_size), 
DMA_FROM_DEVICE);
mvneta_frag_free(pp->frag_size, data);
}
@@ -1953,7 +1956,7 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, int 
rx_todo,
rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
index = rx_desc - rxq->descs;
data = rxq->buf_virt_addr[index];
-   phys_addr = rx_desc->buf_phys_addr;
+   phys_addr = rxq->buf_dma_addr[index];
 
if (!mvneta_rxq_desc_is_first_last(rx_status) ||
(rx_status & MVNETA_RXD_ERR_SUMMARY)) {
@@ -4019,7 +4022,10 @@ static int mvneta_init(struct device *dev, struct 
mvneta_port *pp)
rxq->buf_virt_addr = devm_kmalloc(pp->dev->dev.parent,
  rxq->size * sizeof(void *),
  GFP_KERNEL);
-   if (!rxq->buf_virt_addr)
+   rxq->buf_dma_addr = devm_kmalloc(pp->dev->dev.parent,
+rxq->size * sizeof(dma_addr_t),
+GFP_KERNEL);
+   if (!rxq->buf_virt_addr || !rxq->buf_dma_addr)
return -ENOMEM;
}
 
-- 
2.11.0

[PATCH net-next v3 2/4] net: mvneta: avoid getting buf_phys_addr from rx_desc again

[Jisheng Zhang]

In hot code path mvneta_rx_hwbm(), the rx_desc->buf_phys_addr is read
four times. The rx_desc is allocated by dma_alloc_coherent, it's
uncacheable if the device isn't cache-coherent, reading from uncached
memory is fairly slow. So reuse the read out phys_addr variable to
avoid the extra reading from uncached memory.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
Suggested-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 06df72b8da85..a25042801eec 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -2082,8 +2082,7 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int 
rx_todo,
(rx_status & MVNETA_RXD_ERR_SUMMARY)) {
 err_drop_frame_ret_pool:
/* Return the buffer to the pool */
-   mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool,
- rx_desc->buf_phys_addr);
+   mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool, phys_addr);
 err_drop_frame:
dev->stats.rx_errors++;
mvneta_rx_error(pp, rx_desc);
@@ -2098,7 +2097,7 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int 
rx_todo,
goto err_drop_frame_ret_pool;
 
dma_sync_single_range_for_cpu(dev->dev.parent,
- rx_desc->buf_phys_addr,
+ phys_addr,
  MVNETA_MH_SIZE + 
NET_SKB_PAD,
  rx_bytes,
  DMA_FROM_DEVICE);
@@ -2114,8 +2113,7 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int 
rx_todo,
rcvd_bytes += rx_bytes;
 
/* Return the buffer to the pool */
-   mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool,
- rx_desc->buf_phys_addr);
+   mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool, phys_addr);
 
/* leave the descriptor and buffer untouched */
continue;
-- 
2.11.0

[PATCH net-next v3 3/4] net: mvneta: avoid reading from tx_desc as much as possible

[Jisheng Zhang]

In hot code path such as mvneta_tx(), mvneta_txq_bufs_free() etc. we
access tx_desc several times. The tx_desc is allocated by
dma_alloc_coherent, it's uncacheable if the device isn't cache-coherent,
reading from uncached memory is fairly slow. So use local variable to
store what we need to avoid extra reading from uncached memory.

We get the following performance data on Marvell BG4CT Platforms
(tested with iperf):

before the patch:
sending 1GB in mvneta_tx()(disabled TSO) costs 793553760ns

after the patch:
sending 1GB in mvneta_tx()(disabled TSO) costs 719953800ns

we saved 9.2% time.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 50 ++-
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index a25042801eec..b6cda4131c78 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -1770,6 +1770,7 @@ static void mvneta_txq_bufs_free(struct mvneta_port *pp,
struct mvneta_tx_desc *tx_desc = txq->descs +
txq->txq_get_index;
struct sk_buff *skb = txq->tx_skb[txq->txq_get_index];
+   u32 dma_addr = tx_desc->buf_phys_addr;
 
if (skb) {
bytes_compl += skb->len;
@@ -1778,9 +1779,8 @@ static void mvneta_txq_bufs_free(struct mvneta_port *pp,
 
mvneta_txq_inc_get(txq);
 
-   if (!IS_TSO_HEADER(txq, tx_desc->buf_phys_addr))
-   dma_unmap_single(pp->dev->dev.parent,
-tx_desc->buf_phys_addr,
+   if (!IS_TSO_HEADER(txq, dma_addr))
+   dma_unmap_single(pp->dev->dev.parent, dma_addr,
 tx_desc->data_size, DMA_TO_DEVICE);
if (!skb)
continue;
@@ -2191,17 +2191,18 @@ mvneta_tso_put_data(struct net_device *dev, struct 
mvneta_tx_queue *txq,
bool last_tcp, bool is_last)
 {
struct mvneta_tx_desc *tx_desc;
+   dma_addr_t dma_addr;
 
tx_desc = mvneta_txq_next_desc_get(txq);
tx_desc->data_size = size;
-   tx_desc->buf_phys_addr = dma_map_single(dev->dev.parent, data,
-   size, DMA_TO_DEVICE);
-   if (unlikely(dma_mapping_error(dev->dev.parent,
-tx_desc->buf_phys_addr))) {
+
+   dma_addr = dma_map_single(dev->dev.parent, data, size, DMA_TO_DEVICE);
+   if (unlikely(dma_mapping_error(dev->dev.parent, dma_addr))) {
mvneta_txq_desc_put(txq);
return -ENOMEM;
}
 
+   tx_desc->buf_phys_addr = dma_addr;
tx_desc->command = 0;
txq->tx_skb[txq->txq_put_index] = NULL;
 
@@ -2278,9 +2279,10 @@ static int mvneta_tx_tso(struct sk_buff *skb, struct 
net_device *dev,
 */
for (i = desc_count - 1; i >= 0; i--) {
struct mvneta_tx_desc *tx_desc = txq->descs + i;
-   if (!IS_TSO_HEADER(txq, tx_desc->buf_phys_addr))
+   u32 dma_addr = tx_desc->buf_phys_addr;
+   if (!IS_TSO_HEADER(txq, dma_addr))
dma_unmap_single(pp->dev->dev.parent,
-tx_desc->buf_phys_addr,
+dma_addr,
 tx_desc->data_size,
 DMA_TO_DEVICE);
mvneta_txq_desc_put(txq);
@@ -2296,21 +2298,20 @@ static int mvneta_tx_frag_process(struct mvneta_port 
*pp, struct sk_buff *skb,
int i, nr_frags = skb_shinfo(skb)->nr_frags;
 
for (i = 0; i < nr_frags; i++) {
+   dma_addr_t dma_addr;
skb_frag_t *frag = _shinfo(skb)->frags[i];
void *addr = page_address(frag->page.p) + frag->page_offset;
 
tx_desc = mvneta_txq_next_desc_get(txq);
tx_desc->data_size = frag->size;
 
-   tx_desc->buf_phys_addr =
-   dma_map_single(pp->dev->dev.parent, addr,
-  tx_desc->data_size, DMA_TO_DEVICE);
-
-   if (dma_mapping_error(pp->dev->dev.parent,
- tx_desc->buf_phys_addr)) {
+   dma_addr = dma_map_single(pp->dev->dev.parent, addr,
+ frag->size, DMA_TO_DEVICE);
+   if (dma_mapping_error(pp->dev->dev.parent, dma_addr)) {
mvneta_txq_desc_put(txq);
goto error;
}
+   tx_desc->buf_phys_addr = dma_addr;
 
if (i == nr_frags - 1) {

Re: [PATCH net-next v2 0/2] net: mvneta: improve rx performance

[Jisheng Zhang]

On Fri, 17 Feb 2017 11:37:21 +0100 Gregory CLEMENT wrote:

> Hi Jisheng,
>  
>  On ven., févr. 17 2017, Jisheng Zhang <jszh...@marvell.com> wrote:
> 
> > In hot code path such as mvneta_rx_hwbm() and mvneta_rx_swbm(), we may
> > access fields of rx_desc. The rx_desc is allocated by
> > dma_alloc_coherent, it's uncacheable if the device isn't cache
> > coherent, reading from uncached memory is fairly slow.  
> 
> Did you test it with HWBM support?

No I didn't test it for lacking of such HW, so it's appreciated if someone
can test with HWBM capable HW.

> 
> I am not sure ti will work in this case.

IMHO, if mvneta HW doesn't update rx_desc->buf_phys_addr, it can still work.
I don't have HWBM background, so above may be wrong. If this case doesn't
work for HWBM, I'll submit v3 to modify mvneta_rx_swbm() only.

Thanks,
Jisheng

> 
> Gregory
> 
> >
> > patch1 reuses the read out status to getting status field of rx_desc
> > again.
> >
> > patch2 uses cacheable memory to store the rx buffer DMA address.
> >
> > We get the following performance data on Marvell BG4CT Platforms
> > (tested with iperf):
> >
> > before the patch:
> > recving 1GB in mvneta_rx_swbm() costs 149265960 ns
> >
> > after the patch:
> > recving 1GB in mvneta_rx_swbm() costs 1421565640 ns
> >
> > We saved 4.76% time.
> >
> > RFC: can we do similar modification for tx? If yes, I can prepare a v2.
> >
> >
> > Basically, these two patches do what Arnd mentioned in [1].
> >
> > Hi Arnd,
> >
> > I added "Suggested-by you" tag, I hope you don't mind ;)
> >
> > Thanks
> >
> > [1] https://www.spinics.net/lists/netdev/msg405889.html
> >
> > Since v1:
> >   - correct the performance data typo
> >
> > Jisheng Zhang (2):
> >   net: mvneta: avoid getting status from rx_desc as much as possible
> >   net: mvneta: Use cacheable memory to store the rx buffer DMA address
> >
> >  drivers/net/ethernet/marvell/mvneta.c | 36 
> > ---
> >  1 file changed, 21 insertions(+), 15 deletions(-)
> >
> > -- 
> > 2.11.0
> >
> >
> > ___
> > linux-arm-kernel mailing list
> > linux-arm-ker...@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel  
> 

Re: [PATCH net-next v2 0/2] net: mvneta: improve rx performance

[Jisheng Zhang]

On Fri, 17 Feb 2017 18:02:31 +0800
Jisheng Zhang <jszh...@marvell.com> wrote:

> In hot code path such as mvneta_rx_hwbm() and mvneta_rx_swbm(), we may
> access fields of rx_desc. The rx_desc is allocated by
> dma_alloc_coherent, it's uncacheable if the device isn't cache
> coherent, reading from uncached memory is fairly slow.
> 
> patch1 reuses the read out status to getting status field of rx_desc
> again.
> 
> patch2 uses cacheable memory to store the rx buffer DMA address.
> 
> We get the following performance data on Marvell BG4CT Platforms
> (tested with iperf):
> 
> before the patch:
> recving 1GB in mvneta_rx_swbm() costs 149265960 ns

oops, I still didn't correct the typo here, it should be 1492659600 ns

Sorry, but I think there must be comments, I'll fix this typo in v3 when
address comments.

> 
> after the patch:
> recving 1GB in mvneta_rx_swbm() costs 1421565640 ns
> 
> We saved 4.76% time.
> 
> RFC: can we do similar modification for tx? If yes, I can prepare a v2.
> 
> 
> Basically, these two patches do what Arnd mentioned in [1].
> 
> Hi Arnd,
> 
> I added "Suggested-by you" tag, I hope you don't mind ;)
> 
> Thanks
> 
> [1] https://www.spinics.net/lists/netdev/msg405889.html
> 
> Since v1:
>   - correct the performance data typo
> 
> Jisheng Zhang (2):
>   net: mvneta: avoid getting status from rx_desc as much as possible
>   net: mvneta: Use cacheable memory to store the rx buffer DMA address
> 
>  drivers/net/ethernet/marvell/mvneta.c | 36 
> ---
>  1 file changed, 21 insertions(+), 15 deletions(-)
> 

[PATCH net-next v2 1/2] net: mvneta: avoid getting status from rx_desc as much as possible

[Jisheng Zhang]

In hot code path mvneta_rx_hwbm(), the rx_desc->status is read twice.
The rx_desc is allocated by dma_alloc_coherent, it's uncacheable if
the device isn't cache-coherent, reading from uncached memory is
fairly slow. So reuse the read out rx_status to avoid the second
reading from uncached memory.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
Suggested-by: Arnd Bergmann <a...@arndb.de>
---
 drivers/net/ethernet/marvell/mvneta.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 61dd4462411c..06df72b8da85 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -313,8 +313,8 @@
((addr >= txq->tso_hdrs_phys) && \
 (addr < txq->tso_hdrs_phys + txq->size * TSO_HEADER_SIZE))
 
-#define MVNETA_RX_GET_BM_POOL_ID(rxd) \
-   (((rxd)->status & MVNETA_RXD_BM_POOL_MASK) >> MVNETA_RXD_BM_POOL_SHIFT)
+#define MVNETA_RX_GET_BM_POOL_ID(status) \
+   (((status) & MVNETA_RXD_BM_POOL_MASK) >> MVNETA_RXD_BM_POOL_SHIFT)
 
 struct mvneta_statistic {
unsigned short offset;
@@ -1900,7 +1900,7 @@ static void mvneta_rxq_drop_pkts(struct mvneta_port *pp,
for (i = 0; i < rx_done; i++) {
struct mvneta_rx_desc *rx_desc =
  mvneta_rxq_next_desc_get(rxq);
-   u8 pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc);
+   u8 pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc->status);
struct mvneta_bm_pool *bm_pool;
 
bm_pool = >bm_priv->bm_pools[pool_id];
@@ -2075,7 +2075,7 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int 
rx_todo,
rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
data = (u8 *)(uintptr_t)rx_desc->buf_cookie;
phys_addr = rx_desc->buf_phys_addr;
-   pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc);
+   pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_status);
bm_pool = >bm_priv->bm_pools[pool_id];
 
if (!mvneta_rxq_desc_is_first_last(rx_status) ||
-- 
2.11.0

[PATCH net-next v2 2/2] net: mvneta: Use cacheable memory to store the rx buffer DMA address

[Jisheng Zhang]

In hot code path such as mvneta_rx_hwbm() and mvneta_rx_swbm, the
buf_phys_addr field of rx_dec is accessed. The rx_desc is allocated by
dma_alloc_coherent, it's uncacheable if the device isn't cache
coherent, reading from uncached memory is fairly slow. This patch uses
cacheable memory to store the rx buffer DMA address. We get the
following performance data on Marvell BG4CT Platforms (tested with
iperf):

before the patch:
recving 1GB in mvneta_rx_swbm() costs 1492659600 ns

after the patch:
recving 1GB in mvneta_rx_swbm() costs 1421565640 ns

We saved 4.76% time.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
Suggested-by: Arnd Bergmann <a...@arndb.de>
---
 drivers/net/ethernet/marvell/mvneta.c | 28 +---
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 06df72b8da85..e24c3028fe1d 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -580,6 +580,9 @@ struct mvneta_rx_queue {
/* Virtual address of the RX buffer */
void  **buf_virt_addr;
 
+   /* DMA address of the RX buffer */
+   dma_addr_t *buf_dma_addr;
+
/* Virtual address of the RX DMA descriptors array */
struct mvneta_rx_desc *descs;
 
@@ -1617,6 +1620,7 @@ static void mvneta_rx_desc_fill(struct mvneta_rx_desc 
*rx_desc,
 
rx_desc->buf_phys_addr = phys_addr;
i = rx_desc - rxq->descs;
+   rxq->buf_dma_addr[i] = phys_addr;
rxq->buf_virt_addr[i] = virt_addr;
 }
 
@@ -1900,22 +1904,22 @@ static void mvneta_rxq_drop_pkts(struct mvneta_port *pp,
for (i = 0; i < rx_done; i++) {
struct mvneta_rx_desc *rx_desc =
  mvneta_rxq_next_desc_get(rxq);
+   int index = rx_desc - rxq->descs;
u8 pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc->status);
struct mvneta_bm_pool *bm_pool;
 
bm_pool = >bm_priv->bm_pools[pool_id];
/* Return dropped buffer to the pool */
mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool,
- rx_desc->buf_phys_addr);
+ rxq->buf_dma_addr[index]);
}
return;
}
 
for (i = 0; i < rxq->size; i++) {
-   struct mvneta_rx_desc *rx_desc = rxq->descs + i;
void *data = rxq->buf_virt_addr[i];
 
-   dma_unmap_single(pp->dev->dev.parent, rx_desc->buf_phys_addr,
+   dma_unmap_single(pp->dev->dev.parent, rxq->buf_dma_addr[i],
 MVNETA_RX_BUF_SIZE(pp->pkt_size), 
DMA_FROM_DEVICE);
mvneta_frag_free(pp->frag_size, data);
}
@@ -1953,7 +1957,7 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, int 
rx_todo,
rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
index = rx_desc - rxq->descs;
data = rxq->buf_virt_addr[index];
-   phys_addr = rx_desc->buf_phys_addr;
+   phys_addr = rxq->buf_dma_addr[index];
 
if (!mvneta_rxq_desc_is_first_last(rx_status) ||
(rx_status & MVNETA_RXD_ERR_SUMMARY)) {
@@ -2062,6 +2066,7 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int 
rx_todo,
/* Fairness NAPI loop */
while (rx_done < rx_todo) {
struct mvneta_rx_desc *rx_desc = mvneta_rxq_next_desc_get(rxq);
+   int index = rx_desc - rxq->descs;
struct mvneta_bm_pool *bm_pool = NULL;
struct sk_buff *skb;
unsigned char *data;
@@ -2074,7 +2079,7 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int 
rx_todo,
rx_status = rx_desc->status;
rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
data = (u8 *)(uintptr_t)rx_desc->buf_cookie;
-   phys_addr = rx_desc->buf_phys_addr;
+   phys_addr = rxq->buf_dma_addr[index];
pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_status);
bm_pool = >bm_priv->bm_pools[pool_id];
 
@@ -2082,8 +2087,7 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int 
rx_todo,
(rx_status & MVNETA_RXD_ERR_SUMMARY)) {
 err_drop_frame_ret_pool:
/* Return the buffer to the pool */
-   mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool,
- rx_desc->buf_phys_addr);
+   mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool, phys_addr);
 err_drop_frame:
dev->stats.rx_er

[PATCH net-next v2 0/2] net: mvneta: improve rx performance

[Jisheng Zhang]

In hot code path such as mvneta_rx_hwbm() and mvneta_rx_swbm(), we may
access fields of rx_desc. The rx_desc is allocated by
dma_alloc_coherent, it's uncacheable if the device isn't cache
coherent, reading from uncached memory is fairly slow.

patch1 reuses the read out status to getting status field of rx_desc
again.

patch2 uses cacheable memory to store the rx buffer DMA address.

We get the following performance data on Marvell BG4CT Platforms
(tested with iperf):

before the patch:
recving 1GB in mvneta_rx_swbm() costs 149265960 ns

after the patch:
recving 1GB in mvneta_rx_swbm() costs 1421565640 ns

We saved 4.76% time.

RFC: can we do similar modification for tx? If yes, I can prepare a v2.


Basically, these two patches do what Arnd mentioned in [1].

Hi Arnd,

I added "Suggested-by you" tag, I hope you don't mind ;)

Thanks

[1] https://www.spinics.net/lists/netdev/msg405889.html

Since v1:
  - correct the performance data typo

Jisheng Zhang (2):
  net: mvneta: avoid getting status from rx_desc as much as possible
  net: mvneta: Use cacheable memory to store the rx buffer DMA address

 drivers/net/ethernet/marvell/mvneta.c | 36 ---
 1 file changed, 21 insertions(+), 15 deletions(-)

-- 
2.11.0

[PATCH net-next 0/2] net: mvneta: improve rx performance

[Jisheng Zhang]

In hot code path such as mvneta_rx_hwbm() and mvneta_rx_swbm(), we may
access fields of rx_desc. The rx_desc is allocated by
dma_alloc_coherent, it's uncacheable if the device isn't cache
coherent, reading from uncached memory is fairly slow.

patch1 reuses the read out status to getting status field of rx_desc
again.

patch2 uses cacheable memory to store the rx buffer DMA address.

We get the following performance data on Marvell BG4CT Platforms
(tested with iperf):

before the patch:
recving 1GB in mvneta_rx_swbm() costs 149265960 ns

after the patch:
recving 1GB in mvneta_rx_swbm() costs 1421565640 ns

We saved 4.76% time.

RFC: can we do similar modification for tx? If yes, I can prepare a v2.


Basically, these two patches do what Arnd mentioned in [1].

Hi Arnd,

I added "Suggested-by you" tag, I hope you don't mind ;)

Thanks

[1] https://www.spinics.net/lists/netdev/msg405889.html

Jisheng Zhang (2):
  net: mvneta: avoid getting status from rx_desc as much as possible
  net: mvneta: Use cacheable memory to store the rx buffer DMA address

 drivers/net/ethernet/marvell/mvneta.c | 36 ---
 1 file changed, 21 insertions(+), 15 deletions(-)

-- 
2.11.0

[PATCH net-next 2/2] net: mvneta: Use cacheable memory to store the rx buffer DMA address

[Jisheng Zhang]

In hot code path such as mvneta_rx_hwbm() and mvneta_rx_swbm, the
buf_phys_addr field of rx_dec is accessed. The rx_desc is allocated by
dma_alloc_coherent, it's uncacheable if the device isn't cache
coherent, reading from uncached memory is fairly slow. This patch uses
cacheable memory to store the rx buffer DMA address. We get the
following performance data on Marvell BG4CT Platforms (tested with
iperf):

before the patch:
recving 1GB in mvneta_rx_swbm() costs 149265960 ns

after the patch:
recving 1GB in mvneta_rx_swbm() costs 1421565640 ns

We saved 4.76% time.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
Suggested-by: Arnd Bergmann <a...@arndb.de>
---
 drivers/net/ethernet/marvell/mvneta.c | 28 +---
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 06df72b8da85..e24c3028fe1d 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -580,6 +580,9 @@ struct mvneta_rx_queue {
/* Virtual address of the RX buffer */
void  **buf_virt_addr;
 
+   /* DMA address of the RX buffer */
+   dma_addr_t *buf_dma_addr;
+
/* Virtual address of the RX DMA descriptors array */
struct mvneta_rx_desc *descs;
 
@@ -1617,6 +1620,7 @@ static void mvneta_rx_desc_fill(struct mvneta_rx_desc 
*rx_desc,
 
rx_desc->buf_phys_addr = phys_addr;
i = rx_desc - rxq->descs;
+   rxq->buf_dma_addr[i] = phys_addr;
rxq->buf_virt_addr[i] = virt_addr;
 }
 
@@ -1900,22 +1904,22 @@ static void mvneta_rxq_drop_pkts(struct mvneta_port *pp,
for (i = 0; i < rx_done; i++) {
struct mvneta_rx_desc *rx_desc =
  mvneta_rxq_next_desc_get(rxq);
+   int index = rx_desc - rxq->descs;
u8 pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc->status);
struct mvneta_bm_pool *bm_pool;
 
bm_pool = >bm_priv->bm_pools[pool_id];
/* Return dropped buffer to the pool */
mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool,
- rx_desc->buf_phys_addr);
+ rxq->buf_dma_addr[index]);
}
return;
}
 
for (i = 0; i < rxq->size; i++) {
-   struct mvneta_rx_desc *rx_desc = rxq->descs + i;
void *data = rxq->buf_virt_addr[i];
 
-   dma_unmap_single(pp->dev->dev.parent, rx_desc->buf_phys_addr,
+   dma_unmap_single(pp->dev->dev.parent, rxq->buf_dma_addr[i],
 MVNETA_RX_BUF_SIZE(pp->pkt_size), 
DMA_FROM_DEVICE);
mvneta_frag_free(pp->frag_size, data);
}
@@ -1953,7 +1957,7 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, int 
rx_todo,
rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
index = rx_desc - rxq->descs;
data = rxq->buf_virt_addr[index];
-   phys_addr = rx_desc->buf_phys_addr;
+   phys_addr = rxq->buf_dma_addr[index];
 
if (!mvneta_rxq_desc_is_first_last(rx_status) ||
(rx_status & MVNETA_RXD_ERR_SUMMARY)) {
@@ -2062,6 +2066,7 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int 
rx_todo,
/* Fairness NAPI loop */
while (rx_done < rx_todo) {
struct mvneta_rx_desc *rx_desc = mvneta_rxq_next_desc_get(rxq);
+   int index = rx_desc - rxq->descs;
struct mvneta_bm_pool *bm_pool = NULL;
struct sk_buff *skb;
unsigned char *data;
@@ -2074,7 +2079,7 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int 
rx_todo,
rx_status = rx_desc->status;
rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
data = (u8 *)(uintptr_t)rx_desc->buf_cookie;
-   phys_addr = rx_desc->buf_phys_addr;
+   phys_addr = rxq->buf_dma_addr[index];
pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_status);
bm_pool = >bm_priv->bm_pools[pool_id];
 
@@ -2082,8 +2087,7 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int 
rx_todo,
(rx_status & MVNETA_RXD_ERR_SUMMARY)) {
 err_drop_frame_ret_pool:
/* Return the buffer to the pool */
-   mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool,
- rx_desc->buf_phys_addr);
+   mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool, phys_addr);
 err_drop_frame:
dev->stats.rx_er

[PATCH net-next 1/2] net: mvneta: avoid getting status from rx_desc as much as possible

[Jisheng Zhang]

In hot code path mvneta_rx_hwbm(), the rx_desc->status is read twice.
The rx_desc is allocated by dma_alloc_coherent, it's uncacheable if
the device isn't cache-coherent, reading from uncached memory is
fairly slow. So reuse the read out rx_status to avoid the second
reading from uncached memory.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
Suggested-by: Arnd Bergmann <a...@arndb.de>
---
 drivers/net/ethernet/marvell/mvneta.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 61dd4462411c..06df72b8da85 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -313,8 +313,8 @@
((addr >= txq->tso_hdrs_phys) && \
 (addr < txq->tso_hdrs_phys + txq->size * TSO_HEADER_SIZE))
 
-#define MVNETA_RX_GET_BM_POOL_ID(rxd) \
-   (((rxd)->status & MVNETA_RXD_BM_POOL_MASK) >> MVNETA_RXD_BM_POOL_SHIFT)
+#define MVNETA_RX_GET_BM_POOL_ID(status) \
+   (((status) & MVNETA_RXD_BM_POOL_MASK) >> MVNETA_RXD_BM_POOL_SHIFT)
 
 struct mvneta_statistic {
unsigned short offset;
@@ -1900,7 +1900,7 @@ static void mvneta_rxq_drop_pkts(struct mvneta_port *pp,
for (i = 0; i < rx_done; i++) {
struct mvneta_rx_desc *rx_desc =
  mvneta_rxq_next_desc_get(rxq);
-   u8 pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc);
+   u8 pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc->status);
struct mvneta_bm_pool *bm_pool;
 
bm_pool = >bm_priv->bm_pools[pool_id];
@@ -2075,7 +2075,7 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int 
rx_todo,
rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
data = (u8 *)(uintptr_t)rx_desc->buf_cookie;
phys_addr = rx_desc->buf_phys_addr;
-   pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc);
+   pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_status);
bm_pool = >bm_priv->bm_pools[pool_id];
 
if (!mvneta_rxq_desc_is_first_last(rx_status) ||
-- 
2.11.0

[PATCH] net: mvneta: make mvneta_eth_tool_ops static

[Jisheng Zhang]

The mvneta_eth_tool_ops is only used internally in mvneta driver, so
make it static.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index fdf71720e707..61dd4462411c 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -3959,7 +3959,7 @@ static const struct net_device_ops mvneta_netdev_ops = {
.ndo_do_ioctl= mvneta_ioctl,
 };
 
-const struct ethtool_ops mvneta_eth_tool_ops = {
+static const struct ethtool_ops mvneta_eth_tool_ops = {
.nway_reset = phy_ethtool_nway_reset,
.get_link   = ethtool_op_get_link,
.set_coalesce   = mvneta_ethtool_set_coalesce,
-- 
2.11.0

Re: [PATCH v4 net-next] net: mvneta: implement .set_wol and .get_wol

[Jisheng Zhang]

On Mon, 6 Feb 2017 15:08:48 +0800 Jisheng Zhang wrote:

> Hi Andrew,
> 
> On Mon, 23 Jan 2017 19:10:34 +0100 Andrew Lunn wrote:
> 
> > 
> > On Mon, Jan 23, 2017 at 02:55:07PM +0800, Jisheng Zhang wrote:  
> > > From: Jingju Hou <houji...@marvell.com>
> > > 
> > > From: Jingju Hou <houji...@marvell.com>
> > > 
> > > The mvneta itself does not support WOL, but the PHY might.
> > > So pass the calls to the PHY
> > > 
> > > Signed-off-by: Jingju Hou <houji...@marvell.com>
> > > Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
> > > ---
> > > since v3:
> > >  - really fix the build error
> > 
> > Keep trying
> > 
> > But maybe tomorrow, after you have taken the pause Dave said you
> > should take, and maybe ask Jingju to really review it, in detail.  
> 
> Jingju is a newbie in the Linux kernel community. She made a mistake
> when trying to send the old patch. I picked up her patch when she went
> on vacation, fixed the error and send it out on behalf of her.
> 
> >   
> > > 
> > > since v2,v1:
> > >  - using phy_dev member in struct net_device
> > >  - add commit msg
> > > 
> > >  drivers/net/ethernet/marvell/mvneta.c | 21 +
> > >  1 file changed, 21 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> > > b/drivers/net/ethernet/marvell/mvneta.c
> > > index 6dcc951af0ff..02611fa1c3b8 100644
> > > --- a/drivers/net/ethernet/marvell/mvneta.c
> > > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > > @@ -3929,6 +3929,25 @@ static int mvneta_ethtool_get_rxfh(struct 
> > > net_device *dev, u32 *indir, u8 *key,
> > >   return 0;
> > >  }
> > >  
> > > +static void mvneta_ethtool_get_wol(struct net_device *dev,
> > > +struct ethtool_wolinfo *wol)
> > > +{
> > > + wol->supported = 0;
> > > + wol->wolopts = 0;
> > > +
> > > + if (dev->phydev)
> > > + return phy_ethtool_get_wol(dev->phydev, wol);
> > 
> > This is a void function.  And you are returning a value.  And
> > phy_ethtool_get_wol() is also a void function, so does not actually
> > return anything.  
> 
> Thanks for catching it, fixed in v4, can you please review?

typo, fixed in v5. 

Re: [PATCH v4 net-next] net: mvneta: implement .set_wol and .get_wol

[Jisheng Zhang]

Hi Andrew,

On Mon, 23 Jan 2017 19:10:34 +0100 Andrew Lunn wrote:

> 
> On Mon, Jan 23, 2017 at 02:55:07PM +0800, Jisheng Zhang wrote:
> > From: Jingju Hou <houji...@marvell.com>
> > 
> > From: Jingju Hou <houji...@marvell.com>
> > 
> > The mvneta itself does not support WOL, but the PHY might.
> > So pass the calls to the PHY
> > 
> > Signed-off-by: Jingju Hou <houji...@marvell.com>
> > Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
> > ---
> > since v3:
> >  - really fix the build error  
> 
> Keep trying
> 
> But maybe tomorrow, after you have taken the pause Dave said you
> should take, and maybe ask Jingju to really review it, in detail.

Jingju is a newbie in the Linux kernel community. She made a mistake
when trying to send the old patch. I picked up her patch when she went
on vacation, fixed the error and send it out on behalf of her.

> 
> > 
> > since v2,v1:
> >  - using phy_dev member in struct net_device
> >  - add commit msg
> > 
> >  drivers/net/ethernet/marvell/mvneta.c | 21 +
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> > b/drivers/net/ethernet/marvell/mvneta.c
> > index 6dcc951af0ff..02611fa1c3b8 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -3929,6 +3929,25 @@ static int mvneta_ethtool_get_rxfh(struct net_device 
> > *dev, u32 *indir, u8 *key,
> > return 0;
> >  }
> >  
> > +static void mvneta_ethtool_get_wol(struct net_device *dev,
> > +  struct ethtool_wolinfo *wol)
> > +{
> > +   wol->supported = 0;
> > +   wol->wolopts = 0;
> > +
> > +   if (dev->phydev)
> > +   return phy_ethtool_get_wol(dev->phydev, wol);  
> 
> This is a void function.  And you are returning a value.  And
> phy_ethtool_get_wol() is also a void function, so does not actually
> return anything.

Thanks for catching it, fixed in v4, can you please review?

[PATCH v5 net-next] net: mvneta: implement .set_wol and .get_wol

[Jisheng Zhang]

From: Jingju Hou <houji...@marvell.com>

The mvneta itself does not support WOL, but the PHY might.
So pass the calls to the PHY

Signed-off-by: Jingju Hou <houji...@marvell.com>
Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
since v4:
 - address Andrew Lunn's comment

since v3:
 - really fix the build error

since v2,v1:
 - using phy_dev member in struct net_device
 - add commit msg

 drivers/net/ethernet/marvell/mvneta.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index de6c47744b8e..0f4d1697be46 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -3927,6 +3927,25 @@ static int mvneta_ethtool_get_rxfh(struct net_device 
*dev, u32 *indir, u8 *key,
return 0;
 }
 
+static void mvneta_ethtool_get_wol(struct net_device *dev,
+  struct ethtool_wolinfo *wol)
+{
+   wol->supported = 0;
+   wol->wolopts = 0;
+
+   if (dev->phydev)
+   phy_ethtool_get_wol(dev->phydev, wol);
+}
+
+static int mvneta_ethtool_set_wol(struct net_device *dev,
+ struct ethtool_wolinfo *wol)
+{
+   if (!dev->phydev)
+   return -EOPNOTSUPP;
+
+   return phy_ethtool_set_wol(dev->phydev, wol);
+}
+
 static const struct net_device_ops mvneta_netdev_ops = {
.ndo_open= mvneta_open,
.ndo_stop= mvneta_stop,
@@ -3956,6 +3975,8 @@ const struct ethtool_ops mvneta_eth_tool_ops = {
.set_rxfh   = mvneta_ethtool_set_rxfh,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = mvneta_ethtool_set_link_ksettings,
+   .get_wol= mvneta_ethtool_get_wol,
+   .set_wol= mvneta_ethtool_set_wol,
 };
 
 /* Initialize hw */
-- 
2.11.0

[PATCH v4 net-next] net: mvneta: implement .set_wol and .get_wol

[Jisheng Zhang]

From: Jingju Hou <houji...@marvell.com>

From: Jingju Hou <houji...@marvell.com>

The mvneta itself does not support WOL, but the PHY might.
So pass the calls to the PHY

Signed-off-by: Jingju Hou <houji...@marvell.com>
Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
since v3:
 - really fix the build error

since v2,v1:
 - using phy_dev member in struct net_device
 - add commit msg

 drivers/net/ethernet/marvell/mvneta.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 6dcc951af0ff..02611fa1c3b8 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -3929,6 +3929,25 @@ static int mvneta_ethtool_get_rxfh(struct net_device 
*dev, u32 *indir, u8 *key,
return 0;
 }
 
+static void mvneta_ethtool_get_wol(struct net_device *dev,
+  struct ethtool_wolinfo *wol)
+{
+   wol->supported = 0;
+   wol->wolopts = 0;
+
+   if (dev->phydev)
+   return phy_ethtool_get_wol(dev->phydev, wol);
+}
+
+static int mvneta_ethtool_set_wol(struct net_device *dev,
+ struct ethtool_wolinfo *wol)
+{
+   if (!dev->phydev)
+   return -EOPNOTSUPP;
+
+   return phy_ethtool_set_wol(dev->phydev, wol);
+}
+
 static const struct net_device_ops mvneta_netdev_ops = {
.ndo_open= mvneta_open,
.ndo_stop= mvneta_stop,
@@ -3958,6 +3977,8 @@ const struct ethtool_ops mvneta_eth_tool_ops = {
.set_rxfh   = mvneta_ethtool_set_rxfh,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = mvneta_ethtool_set_link_ksettings,
+   .get_wol= mvneta_ethtool_get_wol,
+   .set_wol= mvneta_ethtool_set_wol,
 };
 
 /* Initialize hw */
-- 
2.11.0

Re: [PATCH v2 net-next] net: phy: marvell: Add Wake from LAN support for 88E1510 PHY

[Jisheng Zhang]

On Mon, 23 Jan 2017 10:58:15 +0800 wrote:

> This is test on BG4CT platform with 88E1518 marvell PHY.
> 
> Signed-off-by: Jingju Hou <houji...@marvell.com>

Reviewed-by: Jisheng Zhang <jszh...@marvell.com>

> ---
> Since v1:
> - add some commit messages
> 
>  drivers/net/phy/marvell.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 0b78210..ed0d235 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -1679,6 +1679,8 @@ static int marvell_probe(struct phy_device *phydev)
>   .ack_interrupt = _ack_interrupt,
>   .config_intr = _config_intr,
>   .did_interrupt = _did_interrupt,
> + .get_wol = _get_wol,
> + .set_wol = _set_wol,
>   .resume = _resume,
>   .suspend = _suspend,
>   .get_sset_count = marvell_get_sset_count,

Re: [PATCH] net: mvneta: implement .set_wol and .get_wol

[Jisheng Zhang]

Hi Jingju,

On Mon, 23 Jan 2017 10:43:08 +0800 wrote:

> The mvneta itself does not support WOL, but the PHY might.
> So pass the calls to the PHY
> 
> Signed-off-by: Jingju Hou 
> ---
> Since v1:
> - using phy_dev member in struct net_device

I noticed that you send a new v2 patch. So this patch should be ignored.
Some tips:

*the v2 patch title should be like:

[PATCH v2] net: mvneta: implement .set_wol and .get_wol

*you also add a commit msg in v2, you'd better mention it
in changes since v1.

Thanks,
Jisheng

> 
>  drivers/net/ethernet/marvell/mvneta.c | 21 +
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> b/drivers/net/ethernet/marvell/mvneta.c
> index e05e227..78869fa 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -3908,6 +3908,25 @@ static int mvneta_ethtool_get_rxfh(struct net_device 
> *dev, u32 *indir, u8 *key,
>   return 0;
>  }
>  
> +static void
> +mvneta_ethtool_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
> +{
> + wol->supported = 0;
> + wol->wolopts = 0;
> +
> + if (dev->phy_dev)
> + return phy_ethtool_get_wol(dev->phy_dev, wol);
> +}
> +
> +static int
> +mvneta_ethtool_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
> +{
> + if (!dev->phy_dev)
> + return -EOPNOTSUPP;
> +
> + return phy_ethtool_set_wol(dev->phy_dev, wol);
> +}
> +
>  static const struct net_device_ops mvneta_netdev_ops = {
>   .ndo_open= mvneta_open,
>   .ndo_stop= mvneta_stop,
> @@ -3937,6 +3956,8 @@ static int mvneta_ethtool_get_rxfh(struct net_device 
> *dev, u32 *indir, u8 *key,
>   .set_rxfh   = mvneta_ethtool_set_rxfh,
>   .get_link_ksettings = phy_ethtool_get_link_ksettings,
>   .set_link_ksettings = mvneta_ethtool_set_link_ksettings,
> + .get_wol= mvneta_ethtool_get_wol,
> + .set_wol= mvneta_ethtool_set_wol,
>  };
>  
>  /* Initialize hw */

Re: [PATCHv2 net-next] net: mvneta: implement .set_wol and .get_wol

[Jisheng Zhang]

On Mon, 23 Jan 2017 10:44:07 +0800
Jingju Hou <houji...@marvell.com> wrote:

> The mvneta itself does not support WOL, but the PHY might.
> So pass the calls to the PHY
> 
> Signed-off-by: Jingju Hou <houji...@marvell.com>

Reviewed-by: Jisheng Zhang <jszh...@marvell.com>

> ---
> Since v1:
> - using phy_dev member in struct net_device
> 
>  drivers/net/ethernet/marvell/mvneta.c | 21 +
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> b/drivers/net/ethernet/marvell/mvneta.c
> index e05e227..78869fa 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -3908,6 +3908,25 @@ static int mvneta_ethtool_get_rxfh(struct net_device 
> *dev, u32 *indir, u8 *key,
>   return 0;
>  }
>  
> +static void
> +mvneta_ethtool_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
> +{
> + wol->supported = 0;
> + wol->wolopts = 0;
> +
> + if (dev->phy_dev)
> + return phy_ethtool_get_wol(dev->phy_dev, wol);
> +}
> +
> +static int
> +mvneta_ethtool_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
> +{
> + if (!dev->phy_dev)
> + return -EOPNOTSUPP;
> +
> + return phy_ethtool_set_wol(dev->phy_dev, wol);
> +}
> +
>  static const struct net_device_ops mvneta_netdev_ops = {
>   .ndo_open= mvneta_open,
>   .ndo_stop= mvneta_stop,
> @@ -3937,6 +3956,8 @@ static int mvneta_ethtool_get_rxfh(struct net_device 
> *dev, u32 *indir, u8 *key,
>   .set_rxfh   = mvneta_ethtool_set_rxfh,
>   .get_link_ksettings = phy_ethtool_get_link_ksettings,
>   .set_link_ksettings = mvneta_ethtool_set_link_ksettings,
> + .get_wol= mvneta_ethtool_get_wol,
> + .set_wol= mvneta_ethtool_set_wol,
>  };
>  
>  /* Initialize hw */

Re: [PATCH v5 net-next 4/7] net: mvneta: Convert to be 64 bits compatible

[Jisheng Zhang]

On Thu, 1 Dec 2016 20:02:05 +0800 Jisheng Zhang wrote:

> Hi Marcin,
> 
> On Thu, 1 Dec 2016 12:48:39 +0100 Marcin Wojtas wrote:
> 
> > Hi Jisheng,
> > 
> > Which baseline do you use?
> > 
> > It took me really lot of time to catch why RX broke after rebase from
> > LKv4.1 to LKv4.4. Between those two, in commit:
> > 97303480753e ("arm64: Increase the max granular size")
> > L1_CACHE_BYTES for all ARMv8 platforms was increased to 128B and so
> > did NET_SKB_PAD.
> > 
> > And 128 is more than the maximum that can fit into packet offset
> > [11:8]@0x1400. In such case this correction is needed. Did it answer
> > your doubts?  
> 
> That's key! Thanks a lot. In my repo, we don't have commit 97303480753e
> ("arm64: Increase the max granular size")
> 
> I think it would be great if this information can be added into the commit
> msg.
> 
> IIRC, arm64 maintainers considered to let L1_CACHE_BYTES the _minimum_ of
> cache line sizes of arm64. If that's implemented and merged, then we can

I just searched and found the email.

"We may have to revisit this logic and consider L1_CACHE_BYTES the
_minimum_ of cache line sizes in arm64 systems supported by the kernel.
Do you have any benchmarks on Cavium boards that would show significant
degradation with 64-byte L1_CACHE_BYTES vs 128?"

https://patchwork.kernel.org/patch/8634481/


> revert this patch later.
> 
> Thanks,
> Jisheng
> 
> > 
> > Best regards,
> > Marcin
> > 
> > 
> > 
> > 2016-12-01 12:26 GMT+01:00 Jisheng Zhang <jszh...@marvell.com>:  
> > > Hi Gregory, Marcin,
> > >
> > > On Wed, 30 Nov 2016 22:42:49 +0100 Gregory CLEMENT wrote:
> > >
> > >> From: Marcin Wojtas <m...@semihalf.com>
> > >>
> > >> Prepare the mvneta driver in order to be usable on the 64 bits platform
> > >> such as the Armada 3700.
> > >>
> > >> [gregory.clem...@free-electrons.com]: this patch was extract from a 
> > >> larger
> > >> one to ease review and maintenance.
> > >>
> > >> Signed-off-by: Marcin Wojtas <m...@semihalf.com>
> > >> Signed-off-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
> > >> ---
> > >>  drivers/net/ethernet/marvell/mvneta.c | 17 -
> > >>  1 file changed, 16 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> > >> b/drivers/net/ethernet/marvell/mvneta.c
> > >> index 92b9af14c352..8ef03fb69bcd 100644
> > >> --- a/drivers/net/ethernet/marvell/mvneta.c
> > >> +++ b/drivers/net/ethernet/marvell/mvneta.c
> > >> @@ -296,6 +296,12 @@
> > >>  /* descriptor aligned size */
> > >>  #define MVNETA_DESC_ALIGNED_SIZE 32
> > >>
> > >> +/* Number of bytes to be taken into account by HW when putting incoming 
> > >> data
> > >> + * to the buffers. It is needed in case NET_SKB_PAD exceeds maximum 
> > >> packet
> > >> + * offset supported in MVNETA_RXQ_CONFIG_REG(q) registers.
> > >
> > > We also brought up this driver on 64bit platforms, we doesn't have this
> > > patch. Maybe I'm wrong, I'm trying to understand why we need this
> > > modification. Let's assume the NET_SKB_PAD is 64B, we call
> > > mvneta_rxq_offset_set(pp, rxq, 64),
> > >
> > > {
> > > u32 val;
> > >
> > > val = mvreg_read(pp, MVNETA_RXQ_CONFIG_REG(rxq->id));
> > > val &= ~MVNETA_RXQ_PKT_OFFSET_ALL_MASK;
> > >
> > > /* Offset is in */
> > > val |= MVNETA_RXQ_PKT_OFFSET_MASK(offset >> 3);
> > > // then this will be "val |= 8;" it doesn't exceeds the max offset of
> > > MVNETA_RXQ_CONFIG_REG(q) register.
> > >
> > > Could you please kindly point out where I am wrong?
> > >
> > >> + */
> > >> +#define MVNETA_RX_PKT_OFFSET_CORRECTION  64
> > >> +
> > >>  #define MVNETA_RX_PKT_SIZE(mtu) \
> > >>   ALIGN((mtu) + MVNETA_MH_SIZE + MVNETA_VLAN_TAG_LEN + \
> > >> ETH_HLEN + ETH_FCS_LEN,\
> > >> @@ -416,6 +422,7 @@ struct mvneta_port {
> > >>   u64 ethtool_stats[ARRAY_SIZE(mvneta_statistics)];
> > >>
> > >>   u32 indir[MVNETA_RSS_LU_TABLE_SIZE];
> > >> + u16 rx_offset_correction;
> > >>  };
> > 

Re: [PATCH v5 net-next 4/7] net: mvneta: Convert to be 64 bits compatible

[Jisheng Zhang]

Hi Marcin,

On Thu, 1 Dec 2016 12:48:39 +0100 Marcin Wojtas wrote:

> Hi Jisheng,
> 
> Which baseline do you use?
> 
> It took me really lot of time to catch why RX broke after rebase from
> LKv4.1 to LKv4.4. Between those two, in commit:
> 97303480753e ("arm64: Increase the max granular size")
> L1_CACHE_BYTES for all ARMv8 platforms was increased to 128B and so
> did NET_SKB_PAD.
> 
> And 128 is more than the maximum that can fit into packet offset
> [11:8]@0x1400. In such case this correction is needed. Did it answer
> your doubts?

That's key! Thanks a lot. In my repo, we don't have commit 97303480753e
("arm64: Increase the max granular size")

I think it would be great if this information can be added into the commit
msg.

IIRC, arm64 maintainers considered to let L1_CACHE_BYTES the _minimum_ of
cache line sizes of arm64. If that's implemented and merged, then we can
revert this patch later.

Thanks,
Jisheng

> 
> Best regards,
> Marcin
> 
> 
> 
> 2016-12-01 12:26 GMT+01:00 Jisheng Zhang <jszh...@marvell.com>:
> > Hi Gregory, Marcin,
> >
> > On Wed, 30 Nov 2016 22:42:49 +0100 Gregory CLEMENT wrote:
> >  
> >> From: Marcin Wojtas <m...@semihalf.com>
> >>
> >> Prepare the mvneta driver in order to be usable on the 64 bits platform
> >> such as the Armada 3700.
> >>
> >> [gregory.clem...@free-electrons.com]: this patch was extract from a larger
> >> one to ease review and maintenance.
> >>
> >> Signed-off-by: Marcin Wojtas <m...@semihalf.com>
> >> Signed-off-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
> >> ---
> >>  drivers/net/ethernet/marvell/mvneta.c | 17 -
> >>  1 file changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> >> b/drivers/net/ethernet/marvell/mvneta.c
> >> index 92b9af14c352..8ef03fb69bcd 100644
> >> --- a/drivers/net/ethernet/marvell/mvneta.c
> >> +++ b/drivers/net/ethernet/marvell/mvneta.c
> >> @@ -296,6 +296,12 @@
> >>  /* descriptor aligned size */
> >>  #define MVNETA_DESC_ALIGNED_SIZE 32
> >>
> >> +/* Number of bytes to be taken into account by HW when putting incoming 
> >> data
> >> + * to the buffers. It is needed in case NET_SKB_PAD exceeds maximum packet
> >> + * offset supported in MVNETA_RXQ_CONFIG_REG(q) registers.  
> >
> > We also brought up this driver on 64bit platforms, we doesn't have this
> > patch. Maybe I'm wrong, I'm trying to understand why we need this
> > modification. Let's assume the NET_SKB_PAD is 64B, we call
> > mvneta_rxq_offset_set(pp, rxq, 64),
> >
> > {
> > u32 val;
> >
> > val = mvreg_read(pp, MVNETA_RXQ_CONFIG_REG(rxq->id));
> > val &= ~MVNETA_RXQ_PKT_OFFSET_ALL_MASK;
> >
> > /* Offset is in */
> > val |= MVNETA_RXQ_PKT_OFFSET_MASK(offset >> 3);
> > // then this will be "val |= 8;" it doesn't exceeds the max offset of
> > MVNETA_RXQ_CONFIG_REG(q) register.
> >
> > Could you please kindly point out where I am wrong?
> >  
> >> + */
> >> +#define MVNETA_RX_PKT_OFFSET_CORRECTION  64
> >> +
> >>  #define MVNETA_RX_PKT_SIZE(mtu) \
> >>   ALIGN((mtu) + MVNETA_MH_SIZE + MVNETA_VLAN_TAG_LEN + \
> >> ETH_HLEN + ETH_FCS_LEN,\
> >> @@ -416,6 +422,7 @@ struct mvneta_port {
> >>   u64 ethtool_stats[ARRAY_SIZE(mvneta_statistics)];
> >>
> >>   u32 indir[MVNETA_RSS_LU_TABLE_SIZE];
> >> + u16 rx_offset_correction;
> >>  };
> >>
> >>  /* The mvneta_tx_desc and mvneta_rx_desc structures describe the
> >> @@ -1807,6 +1814,7 @@ static int mvneta_rx_refill(struct mvneta_port *pp,
> >>   return -ENOMEM;
> >>   }
> >>
> >> + phys_addr += pp->rx_offset_correction;
> >>   mvneta_rx_desc_fill(rx_desc, phys_addr, data, rxq);
> >>   return 0;
> >>  }
> >> @@ -2782,7 +2790,7 @@ static int mvneta_rxq_init(struct mvneta_port *pp,
> >>   mvreg_write(pp, MVNETA_RXQ_SIZE_REG(rxq->id), rxq->size);
> >>
> >>   /* Set Offset */
> >> - mvneta_rxq_offset_set(pp, rxq, NET_SKB_PAD);
> >> + mvneta_rxq_offset_set(pp, rxq, NET_SKB_PAD - 
> >> pp->rx_offset_correction);
> >>
> >>   /* Set coalescing pkts and time */
> >>   mvneta_rx_pkts_coal_set(pp, rxq, rxq->pkts_coal);
> >> @@ -4033,6 +4041,13 @@ static int mvneta_probe(struct platform_device 
> >> *pdev)
> >>
> >>   pp->rxq_def = rxq_def;
> >>
> >> + /* Set RX packet offset correction for platforms, whose
> >> +  * NET_SKB_PAD, exceeds 64B. It should be 64B for 64-bit
> >> +  * platforms and 0B for 32-bit ones.  
> >
> > Even we need this patch, I'm not sure this last comment is correct or not.
> > NET_SKB_PAD is defined as:
> >
> > #define NET_SKB_PAD max(32, L1_CACHE_BYTES)
> >
> > we have 64B cacheline 32bit platforms, on this platforms, the NET_SKB_PAD
> > should be 64B as well.
> >
> > Thanks,
> > Jisheng  

Re: [PATCH v5 net-next 4/7] net: mvneta: Convert to be 64 bits compatible

[Jisheng Zhang]

On Thu, 1 Dec 2016 19:26:04 +0800
Jisheng Zhang <jszh...@marvell.com> wrote:

> Hi Gregory, Marcin,
> 
> On Wed, 30 Nov 2016 22:42:49 +0100 Gregory CLEMENT wrote:
> 
> > From: Marcin Wojtas <m...@semihalf.com>
> > 
> > Prepare the mvneta driver in order to be usable on the 64 bits platform
> > such as the Armada 3700.
> > 
> > [gregory.clem...@free-electrons.com]: this patch was extract from a larger
> > one to ease review and maintenance.
> > 
> > Signed-off-by: Marcin Wojtas <m...@semihalf.com>
> > Signed-off-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
> > ---
> >  drivers/net/ethernet/marvell/mvneta.c | 17 -
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> > b/drivers/net/ethernet/marvell/mvneta.c
> > index 92b9af14c352..8ef03fb69bcd 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -296,6 +296,12 @@
> >  /* descriptor aligned size */
> >  #define MVNETA_DESC_ALIGNED_SIZE   32
> >  
> > +/* Number of bytes to be taken into account by HW when putting incoming 
> > data
> > + * to the buffers. It is needed in case NET_SKB_PAD exceeds maximum packet
> > + * offset supported in MVNETA_RXQ_CONFIG_REG(q) registers.  
> 
> We also brought up this driver on 64bit platforms, we doesn't have this
> patch. Maybe I'm wrong, I'm trying to understand why we need this
> modification. Let's assume the NET_SKB_PAD is 64B, we call
> mvneta_rxq_offset_set(pp, rxq, 64),
> 
> {
> u32 val;
> 
> val = mvreg_read(pp, MVNETA_RXQ_CONFIG_REG(rxq->id));
> val &= ~MVNETA_RXQ_PKT_OFFSET_ALL_MASK;
> 
> /* Offset is in */
> val |= MVNETA_RXQ_PKT_OFFSET_MASK(offset >> 3);
> // then this will be "val |= 8;" it doesn't exceeds the max offset of

sorry, this will be "val |= MVNETA_RXQ_PKT_OFFSET_MASK(8);"

> MVNETA_RXQ_CONFIG_REG(q) register.
> 
> Could you please kindly point out where I am wrong?
> 
> > + */
> > +#define MVNETA_RX_PKT_OFFSET_CORRECTION64
> > +
> >  #define MVNETA_RX_PKT_SIZE(mtu) \
> > ALIGN((mtu) + MVNETA_MH_SIZE + MVNETA_VLAN_TAG_LEN + \
> >   ETH_HLEN + ETH_FCS_LEN,\
> > @@ -416,6 +422,7 @@ struct mvneta_port {
> > u64 ethtool_stats[ARRAY_SIZE(mvneta_statistics)];
> >  
> > u32 indir[MVNETA_RSS_LU_TABLE_SIZE];
> > +   u16 rx_offset_correction;
> >  };
> >  
> >  /* The mvneta_tx_desc and mvneta_rx_desc structures describe the
> > @@ -1807,6 +1814,7 @@ static int mvneta_rx_refill(struct mvneta_port *pp,
> > return -ENOMEM;
> > }
> >  
> > +   phys_addr += pp->rx_offset_correction;
> > mvneta_rx_desc_fill(rx_desc, phys_addr, data, rxq);
> > return 0;
> >  }
> > @@ -2782,7 +2790,7 @@ static int mvneta_rxq_init(struct mvneta_port *pp,
> > mvreg_write(pp, MVNETA_RXQ_SIZE_REG(rxq->id), rxq->size);
> >  
> > /* Set Offset */
> > -   mvneta_rxq_offset_set(pp, rxq, NET_SKB_PAD);
> > +   mvneta_rxq_offset_set(pp, rxq, NET_SKB_PAD - pp->rx_offset_correction);
> >  
> > /* Set coalescing pkts and time */
> > mvneta_rx_pkts_coal_set(pp, rxq, rxq->pkts_coal);
> > @@ -4033,6 +4041,13 @@ static int mvneta_probe(struct platform_device *pdev)
> >  
> > pp->rxq_def = rxq_def;
> >  
> > +   /* Set RX packet offset correction for platforms, whose
> > +* NET_SKB_PAD, exceeds 64B. It should be 64B for 64-bit
> > +* platforms and 0B for 32-bit ones.  
> 
> Even we need this patch, I'm not sure this last comment is correct or not.
> NET_SKB_PAD is defined as:
> 
> #define NET_SKB_PAD max(32, L1_CACHE_BYTES)
> 
> we have 64B cacheline 32bit platforms, on this platforms, the NET_SKB_PAD
> should be 64B as well.
> 
> Thanks,
> Jisheng
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 net-next 3/7] net: mvneta: Use cacheable memory to store the rx buffer virtual address

[Jisheng Zhang]

On Wed, 30 Nov 2016 22:42:48 +0100 Gregory CLEMENT wrote:

> Until now the virtual address of the received buffer were stored in the
> cookie field of the rx descriptor. However, this field is 32-bits only
> which prevents to use the driver on a 64-bits architecture.
> 
> With this patch the virtual address is stored in an array not shared with
> the hardware (no more need to use the DMA API). Thanks to this, it is
> possible to use cache contrary to the access of the rx descriptor member.
> 
> The change is done in the swbm path only because the hwbm uses the cookie
> field, this also means that currently the hwbm is not usable in 64-bits.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>

Reviewed-by: Jisheng Zhang <jszh...@marvell.com>

Thanks,
Jisheng

> ---
>  drivers/net/ethernet/marvell/mvneta.c | 34 +++-
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> b/drivers/net/ethernet/marvell/mvneta.c
> index f5319c50f8d9..92b9af14c352 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -561,6 +561,9 @@ struct mvneta_rx_queue {
>   u32 pkts_coal;
>   u32 time_coal;
>  
> + /* Virtual address of the RX buffer */
> + void  **buf_virt_addr;
> +
>   /* Virtual address of the RX DMA descriptors array */
>   struct mvneta_rx_desc *descs;
>  
> @@ -1573,10 +1576,14 @@ static void mvneta_tx_done_pkts_coal_set(struct 
> mvneta_port *pp,
>  
>  /* Handle rx descriptor fill by setting buf_cookie and buf_phys_addr */
>  static void mvneta_rx_desc_fill(struct mvneta_rx_desc *rx_desc,
> - u32 phys_addr, u32 cookie)
> + u32 phys_addr, void *virt_addr,
> + struct mvneta_rx_queue *rxq)
>  {
> - rx_desc->buf_cookie = cookie;
> + int i;
> +
>   rx_desc->buf_phys_addr = phys_addr;
> + i = rx_desc - rxq->descs;
> + rxq->buf_virt_addr[i] = virt_addr;
>  }
>  
>  /* Decrement sent descriptors counter */
> @@ -1781,7 +1788,8 @@ EXPORT_SYMBOL_GPL(mvneta_frag_free);
>  
>  /* Refill processing for SW buffer management */
>  static int mvneta_rx_refill(struct mvneta_port *pp,
> - struct mvneta_rx_desc *rx_desc)
> + struct mvneta_rx_desc *rx_desc,
> + struct mvneta_rx_queue *rxq)
>  
>  {
>   dma_addr_t phys_addr;
> @@ -1799,7 +1807,7 @@ static int mvneta_rx_refill(struct mvneta_port *pp,
>   return -ENOMEM;
>   }
>  
> - mvneta_rx_desc_fill(rx_desc, phys_addr, (u32)data);
> + mvneta_rx_desc_fill(rx_desc, phys_addr, data, rxq);
>   return 0;
>  }
>  
> @@ -1861,7 +1869,7 @@ static void mvneta_rxq_drop_pkts(struct mvneta_port *pp,
>  
>   for (i = 0; i < rxq->size; i++) {
>   struct mvneta_rx_desc *rx_desc = rxq->descs + i;
> - void *data = (void *)rx_desc->buf_cookie;
> + void *data = rxq->buf_virt_addr[i];
>  
>   dma_unmap_single(pp->dev->dev.parent, rx_desc->buf_phys_addr,
>MVNETA_RX_BUF_SIZE(pp->pkt_size), 
> DMA_FROM_DEVICE);
> @@ -1894,12 +1902,13 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, int 
> rx_todo,
>   unsigned char *data;
>   dma_addr_t phys_addr;
>   u32 rx_status, frag_size;
> - int rx_bytes, err;
> + int rx_bytes, err, index;
>  
>   rx_done++;
>   rx_status = rx_desc->status;
>   rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
> - data = (unsigned char *)rx_desc->buf_cookie;
> + index = rx_desc - rxq->descs;
> + data = rxq->buf_virt_addr[index];
>   phys_addr = rx_desc->buf_phys_addr;
>  
>   if (!mvneta_rxq_desc_is_first_last(rx_status) ||
> @@ -1938,7 +1947,7 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, int 
> rx_todo,
>   }
>  
>   /* Refill processing */
> - err = mvneta_rx_refill(pp, rx_desc);
> + err = mvneta_rx_refill(pp, rx_desc, rxq);
>   if (err) {
>   netdev_err(dev, "Linux processing - Can't refill\n");
>   rxq->missed++;
> @@ -2020,7 +2029,7 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int 
> rx_todo,
>   rx_done++;
>   rx_status = rx_desc->status;
>   rx_bytes = rx_desc->data_size - (ETH_FCS_

Re: [PATCH v5 net-next 4/7] net: mvneta: Convert to be 64 bits compatible

[Jisheng Zhang]

Hi Gregory, Marcin,

On Wed, 30 Nov 2016 22:42:49 +0100 Gregory CLEMENT wrote:

> From: Marcin Wojtas 
> 
> Prepare the mvneta driver in order to be usable on the 64 bits platform
> such as the Armada 3700.
> 
> [gregory.clem...@free-electrons.com]: this patch was extract from a larger
> one to ease review and maintenance.
> 
> Signed-off-by: Marcin Wojtas 
> Signed-off-by: Gregory CLEMENT 
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 17 -
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> b/drivers/net/ethernet/marvell/mvneta.c
> index 92b9af14c352..8ef03fb69bcd 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -296,6 +296,12 @@
>  /* descriptor aligned size */
>  #define MVNETA_DESC_ALIGNED_SIZE 32
>  
> +/* Number of bytes to be taken into account by HW when putting incoming data
> + * to the buffers. It is needed in case NET_SKB_PAD exceeds maximum packet
> + * offset supported in MVNETA_RXQ_CONFIG_REG(q) registers.

We also brought up this driver on 64bit platforms, we doesn't have this
patch. Maybe I'm wrong, I'm trying to understand why we need this
modification. Let's assume the NET_SKB_PAD is 64B, we call
mvneta_rxq_offset_set(pp, rxq, 64),

{
u32 val;

val = mvreg_read(pp, MVNETA_RXQ_CONFIG_REG(rxq->id));
val &= ~MVNETA_RXQ_PKT_OFFSET_ALL_MASK;

/* Offset is in */
val |= MVNETA_RXQ_PKT_OFFSET_MASK(offset >> 3);
// then this will be "val |= 8;" it doesn't exceeds the max offset of
MVNETA_RXQ_CONFIG_REG(q) register.

Could you please kindly point out where I am wrong?

> + */
> +#define MVNETA_RX_PKT_OFFSET_CORRECTION  64
> +
>  #define MVNETA_RX_PKT_SIZE(mtu) \
>   ALIGN((mtu) + MVNETA_MH_SIZE + MVNETA_VLAN_TAG_LEN + \
> ETH_HLEN + ETH_FCS_LEN,\
> @@ -416,6 +422,7 @@ struct mvneta_port {
>   u64 ethtool_stats[ARRAY_SIZE(mvneta_statistics)];
>  
>   u32 indir[MVNETA_RSS_LU_TABLE_SIZE];
> + u16 rx_offset_correction;
>  };
>  
>  /* The mvneta_tx_desc and mvneta_rx_desc structures describe the
> @@ -1807,6 +1814,7 @@ static int mvneta_rx_refill(struct mvneta_port *pp,
>   return -ENOMEM;
>   }
>  
> + phys_addr += pp->rx_offset_correction;
>   mvneta_rx_desc_fill(rx_desc, phys_addr, data, rxq);
>   return 0;
>  }
> @@ -2782,7 +2790,7 @@ static int mvneta_rxq_init(struct mvneta_port *pp,
>   mvreg_write(pp, MVNETA_RXQ_SIZE_REG(rxq->id), rxq->size);
>  
>   /* Set Offset */
> - mvneta_rxq_offset_set(pp, rxq, NET_SKB_PAD);
> + mvneta_rxq_offset_set(pp, rxq, NET_SKB_PAD - pp->rx_offset_correction);
>  
>   /* Set coalescing pkts and time */
>   mvneta_rx_pkts_coal_set(pp, rxq, rxq->pkts_coal);
> @@ -4033,6 +4041,13 @@ static int mvneta_probe(struct platform_device *pdev)
>  
>   pp->rxq_def = rxq_def;
>  
> + /* Set RX packet offset correction for platforms, whose
> +  * NET_SKB_PAD, exceeds 64B. It should be 64B for 64-bit
> +  * platforms and 0B for 32-bit ones.

Even we need this patch, I'm not sure this last comment is correct or not.
NET_SKB_PAD is defined as:

#define NET_SKB_PAD max(32, L1_CACHE_BYTES)

we have 64B cacheline 32bit platforms, on this platforms, the NET_SKB_PAD
should be 64B as well.

Thanks,
Jisheng

Re: [PATCH net-next 0/5] Support Armada 37xx SoC (ARMv8 64-bits) in mvneta driver

[Jisheng Zhang]

Hi Gregory,

On Fri, 25 Nov 2016 16:30:13 +0100 Gregory CLEMENT wrote:

> Hi,
> 
> The Armada 37xx is a new ARMv8 SoC from Marvell using same network
> controller as the older Armada 370/38x/XP SoCs. This series adapts the
> driver in order to be able to use it on this new SoC. The main changes
> are:
> 
> - 64-bits support: the first patches allow using the driver on a 64-bit
>   architecture.
> 
> - MBUS support: the mbus configuration is different on Armada 37xx
>   from the older SoCs.
> 
> - per cpu interrupt: Armada 37xx do not support per cpu interrupt for
>   the NETA IP, the non-per-CPU behavior was added back.
> 
> The first item is solved by patches 1 to 3.
> The 2 last items are solved by patch 4.
> In patch 5 the dt support is added.
> 
> Beside Armada 37xx, the series have been tested on Armada XP and
> Armada 38x (with Hardware Buffer Management and with Software Buffer
> Managment).

AFAICT, this is a V2? seems no Change log ;)

Thanks,
Jisheng

Re: [PATCH net-next 1/5] net: mvneta: Use cacheable memory to store the rx buffer virtual address

[Jisheng Zhang]

Hi Gregory,

On Fri, 25 Nov 2016 16:30:14 +0100 Gregory CLEMENT wrote:

> Until now the virtual address of the received buffer were stored in the
> cookie field of the rx descriptor. However, this field is 32-bits only
> which prevents to use the driver on a 64-bits architecture.
> 
> With this patch the virtual address is stored in an array not shared with
> the hardware (no more need to use the DMA API). Thanks to this, it is
> possible to use cache contrary to the access of the rx descriptor member.
> 
> The change is done in the swbm path only because the hwbm uses the cookie
> field, this also means that currently the hwbm is not usable in 64-bits.
> 
> Signed-off-by: Gregory CLEMENT 
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 96 
>  1 file changed, 84 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> b/drivers/net/ethernet/marvell/mvneta.c
> index 87274d4ab102..b6849f88cab7 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -561,6 +561,9 @@ struct mvneta_rx_queue {
>   u32 pkts_coal;
>   u32 time_coal;
>  
> + /* Virtual address of the RX buffer */
> + void  **buf_virt_addr;

can we store buf_phys_addr in cacheable memory as well?

> +
>   /* Virtual address of the RX DMA descriptors array */
>   struct mvneta_rx_desc *descs;
>  
> @@ -1573,10 +1576,14 @@ static void mvneta_tx_done_pkts_coal_set(struct 
> mvneta_port *pp,
>  
>  /* Handle rx descriptor fill by setting buf_cookie and buf_phys_addr */
>  static void mvneta_rx_desc_fill(struct mvneta_rx_desc *rx_desc,
> - u32 phys_addr, u32 cookie)
> + u32 phys_addr, void *virt_addr,
> + struct mvneta_rx_queue *rxq)
>  {
> - rx_desc->buf_cookie = cookie;
> + int i;
> +
>   rx_desc->buf_phys_addr = phys_addr;
> + i = rx_desc - rxq->descs;
> + rxq->buf_virt_addr[i] = virt_addr;
>  }
>  
>  /* Decrement sent descriptors counter */
> @@ -1781,7 +1788,8 @@ EXPORT_SYMBOL_GPL(mvneta_frag_free);
>  
>  /* Refill processing for SW buffer management */
>  static int mvneta_rx_refill(struct mvneta_port *pp,
> - struct mvneta_rx_desc *rx_desc)
> + struct mvneta_rx_desc *rx_desc,
> + struct mvneta_rx_queue *rxq)
>  
>  {
>   dma_addr_t phys_addr;
> @@ -1799,7 +1807,7 @@ static int mvneta_rx_refill(struct mvneta_port *pp,
>   return -ENOMEM;
>   }
>  
> - mvneta_rx_desc_fill(rx_desc, phys_addr, (u32)data);
> + mvneta_rx_desc_fill(rx_desc, phys_addr, data, rxq);
>   return 0;
>  }
>  
> @@ -1861,7 +1869,12 @@ static void mvneta_rxq_drop_pkts(struct mvneta_port 
> *pp,
>  
>   for (i = 0; i < rxq->size; i++) {
>   struct mvneta_rx_desc *rx_desc = rxq->descs + i;
> - void *data = (void *)rx_desc->buf_cookie;
> + void *data;
> +
> + if (!pp->bm_priv)
> + data = rxq->buf_virt_addr[i];
> + else
> + data = (void *)(uintptr_t)rx_desc->buf_cookie;
>  
>   dma_unmap_single(pp->dev->dev.parent, rx_desc->buf_phys_addr,
>MVNETA_RX_BUF_SIZE(pp->pkt_size), 
> DMA_FROM_DEVICE);
> @@ -1894,12 +1907,13 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, int 
> rx_todo,
>   unsigned char *data;
>   dma_addr_t phys_addr;
>   u32 rx_status, frag_size;
> - int rx_bytes, err;
> + int rx_bytes, err, index;
>  
>   rx_done++;
>   rx_status = rx_desc->status;
>   rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
> - data = (unsigned char *)rx_desc->buf_cookie;
> + index = rx_desc - rxq->descs;
> + data = (unsigned char *)rxq->buf_virt_addr[index];
>   phys_addr = rx_desc->buf_phys_addr;
>  
>   if (!mvneta_rxq_desc_is_first_last(rx_status) ||
> @@ -1938,7 +1952,7 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, int 
> rx_todo,
>   }
>  
>   /* Refill processing */
> - err = mvneta_rx_refill(pp, rx_desc);
> + err = mvneta_rx_refill(pp, rx_desc, rxq);
>   if (err) {
>   netdev_err(dev, "Linux processing - Can't refill\n");
>   rxq->missed++;
> @@ -2020,7 +2034,7 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int 
> rx_todo,
>   rx_done++;
>   rx_status = rx_desc->status;
>   rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
> - data = (unsigned char *)rx_desc->buf_cookie;
> + data = (u8 *)(uintptr_t)rx_desc->buf_cookie;
>   phys_addr = rx_desc->buf_phys_addr;
>   pool_id = 

Re: [PATCH net-next 1/4] net: mvneta: Convert to be 64 bits compatible

[Jisheng Zhang]

On Thu, 24 Nov 2016 10:00:36 +0100
Arnd Bergmann <a...@arndb.de> wrote:

> On Thursday, November 24, 2016 4:37:36 PM CET Jisheng Zhang wrote:
> > solB (a SW shadow cookie) perhaps gives a better performance: in hot path,
> > such as mvneta_rx(), the driver accesses buf_cookie and buf_phys_addr of
> > rx_desc which is allocated by dma_alloc_coherent, it's noncacheable if the
> > device isn't cache-coherent. I didn't measure the performance difference,
> > because in fact we take solA as well internally. From your experience,
> > can the performance gain deserve the complex code?  
> 
> Yes, a read from uncached memory is fairly slow, so if you have a chance
> to avoid that it will probably help. When adding complexity to the code,
> it probably makes sense to take a runtime profile anyway quantify how
> much it gains.
> 
> On machines that have cache-coherent DMA, accessing the descriptor
> should be fine, as you already have to load the entire cache line
> to read the status field.
> 
> Looking at this snippet:
> 
> rx_status = rx_desc->status;
> rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + 
> MVNETA_MH_SIZE);
> data = (unsigned char *)rx_desc->buf_cookie;
> phys_addr = rx_desc->buf_phys_addr;
> pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc);
> bm_pool = >bm_priv->bm_pools[pool_id];
> 
> if (!mvneta_rxq_desc_is_first_last(rx_status) ||
> (rx_status & MVNETA_RXD_ERR_SUMMARY)) {
> err_drop_frame_ret_pool:
> /* Return the buffer to the pool */
> mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool,
>   rx_desc->buf_phys_addr);
> err_drop_frame:
> 
> 
> I think there is more room for optimizing if you start: you read
> the status field twice (the second one in MVNETA_RX_GET_BM_POOL_ID)
> and you can cache the buf_phys_addr along with the virtual address
> once you add that.

oh, yeah! buf_phy_addr could be included too.

> 
> Generally speaking, I'd recommend using READ_ONCE()/WRITE_ONCE()
> to access the descriptor fields, to ensure the compiler doesn't
> add extra references as well as to annotate the expensive
> operations.
> 
>   Arnd

Got it. Thanks so much for the detailed guide. 

Re: [PATCH net-next 1/4] net: mvneta: Convert to be 64 bits compatible

[Jisheng Zhang]

Hi Marcin, Gregory, Arnd,

On Wed, 23 Nov 2016 17:02:11 +0100 Marcin Wojtas  wrote:

> Hi Gregory,
> 
> 2016-11-23 14:07 GMT+01:00 Gregory CLEMENT:
> > Hi Jisheng, Arnd,
> >
> >
> > Thanks for your feedback.
> >
> >
> >  On mer., nov. 23 2016, Arnd Bergmann wrote:
> >  
> >> On Wednesday, November 23, 2016 5:53:41 PM CET Jisheng Zhang wrote:  
> >>> On Tue, 22 Nov 2016 22:04:12 +0100 Arnd Bergmann wrote:
> >>>  
> >>> > On Tuesday, November 22, 2016 5:48:41 PM CET Gregory CLEMENT wrote:  
> >>> > > +#ifdef CONFIG_64BIT
> >>> > > +   void *data_tmp;
> >>> > > +
> >>> > > +   /* In Neta HW only 32 bits data is supported, so in order to
> >>> > > +* obtain whole 64 bits address from RX descriptor, we store
> >>> > > +* the upper 32 bits when allocating buffer, and put it back
> >>> > > +* when using buffer cookie for accessing packet in memory.
> >>> > > +* Frags should be allocated from single 'memory' region,
> >>> > > +* hence common upper address half should be sufficient.
> >>> > > +*/
> >>> > > +   data_tmp = mvneta_frag_alloc(pp->frag_size);
> >>> > > +   if (data_tmp) {
> >>> > > +   pp->data_high = (u64)upper_32_bits((u64)data_tmp) << 
> >>> > > 32;
> >>> > > +   mvneta_frag_free(pp->frag_size, data_tmp);
> >>> > > +   }
> >>> > >  
> >>> >
> >>> > How does this work when the region spans a n*4GB address boundary?  
> >>>
> >>> indeed. We also make use of this driver on 64bit platforms. We use
> >>> different solution to make the driver 64bit safe.
> >>>
> >>> solA: make use of the reserved field in the mvneta_rx_desc, such
> >>> as reserved2 etc. Yes, the field is marked as "for future use, PnC", but
> >>> now it's not used at all. This is one possible solution however.  
> >>
> >> Right, this sounds like the most straightforward choice.  
> >
> > The PnC (which stands for Parsing and Classification) is not used yet
> > indeed but this field will be needed when we will enable it. It is
> > something we want to do but it is not planned in a near future. However
> > from the datasheets I have it seems only present on the Armada XP. It is
> > not mentioned on datasheets for the Armada 38x or the Armada 3700.
> >  
> 
> It is not mentioned in A38x spec, but this SoC has exactly the same
> PnC as Armada XP (they differ only with used SRAM details). I wouldn't
> be surprised if it was supported on A3700 as well.
> 
> > That would mean it was safe to use on of this field in 64-bits mode on
> > the Armada 3700.
> >
> > So I am going to take this approach.
> >  
> 
> I think for now it's safe and is much easier than handling extra
> software ring for virtual addresses.
> 

solB (a SW shadow cookie) perhaps gives a better performance: in hot path,
such as mvneta_rx(), the driver accesses buf_cookie and buf_phys_addr of
rx_desc which is allocated by dma_alloc_coherent, it's noncacheable if the
device isn't cache-coherent. I didn't measure the performance difference,
because in fact we take solA as well internally. From your experience,
can the performance gain deserve the complex code?

Thanks,
Jisheng

Re: [PATCH net-next 1/4] net: mvneta: Convert to be 64 bits compatible

[Jisheng Zhang]

Hi Arnd,

On Wed, 23 Nov 2016 11:15:32 +0100 Arnd Bergmann wrote:

> On Wednesday, November 23, 2016 5:53:41 PM CET Jisheng Zhang wrote:
> > On Tue, 22 Nov 2016 22:04:12 +0100 Arnd Bergmann wrote:
> >   
> > > On Tuesday, November 22, 2016 5:48:41 PM CET Gregory CLEMENT wrote:  
> > > > +#ifdef CONFIG_64BIT
> > > > +   void *data_tmp;
> > > > +
> > > > +   /* In Neta HW only 32 bits data is supported, so in order to
> > > > +* obtain whole 64 bits address from RX descriptor, we store
> > > > +* the upper 32 bits when allocating buffer, and put it back
> > > > +* when using buffer cookie for accessing packet in memory.
> > > > +* Frags should be allocated from single 'memory' region,
> > > > +* hence common upper address half should be sufficient.
> > > > +*/
> > > > +   data_tmp = mvneta_frag_alloc(pp->frag_size);
> > > > +   if (data_tmp) {
> > > > +   pp->data_high = (u64)upper_32_bits((u64)data_tmp) << 32;
> > > > +   mvneta_frag_free(pp->frag_size, data_tmp);
> > > > +   }
> > > > 
> > > 
> > > How does this work when the region spans a n*4GB address boundary?  
> > 
> > indeed. We also make use of this driver on 64bit platforms. We use
> > different solution to make the driver 64bit safe.
> > 
> > solA: make use of the reserved field in the mvneta_rx_desc, such
> > as reserved2 etc. Yes, the field is marked as "for future use, PnC", but
> > now it's not used at all. This is one possible solution however.  
> 
> Right, this sounds like the most straightforward choice.
> 
> > solB: allocate a shadow buf cookie during init, e.g
> > 
> > rxq->descs_bufcookie = kmalloc(rxq->size * sizeof(void*), GFP_KERNEL);
> > 
> > then modify mvneta_rx_desc_fill a bit to save the 64bit pointer in
> > the shadow buf cookie, e.g
> > static void mvneta_rx_desc_fill(struct mvneta_rx_desc *rx_desc,
> > u32 phys_addr, u32 cookie,

sorry, this line should be:
u32 phys_addr, void *cookie

> > struct mvneta_rx_queue *rxq)
> > 
> > {
> > int i;
> > 
> > rx_desc->buf_cookie = cookie;
> > rx_desc->buf_phys_addr = phys_addr;
> > i = rx_desc - rxq->descs;
> > rxq->descs_bufcookie[i] = cookie;
> > }
> > 
> > then fetch the desc from the shadow buf cookie in all code path, such
> > as mvneta_rx() etc.
> > 
> > Both solutions should not have the problems pointed out by Arnd.  
> 
> Wait, since you compute an index 'i' here, can't you just store 'i'
> directly in the descriptor instead of the pointer?
> 

we need to store the pointer, it's to store the buffer allocated by
mvneta_frag_alloc()

Thanks,
Jisheng

Re: [PATCH net-next 1/4] net: mvneta: Convert to be 64 bits compatible

[Jisheng Zhang]

On Tue, 22 Nov 2016 22:04:12 +0100 Arnd Bergmann wrote:

> On Tuesday, November 22, 2016 5:48:41 PM CET Gregory CLEMENT wrote:
> > +#ifdef CONFIG_64BIT
> > +   void *data_tmp;
> > +
> > +   /* In Neta HW only 32 bits data is supported, so in order to
> > +* obtain whole 64 bits address from RX descriptor, we store
> > +* the upper 32 bits when allocating buffer, and put it back
> > +* when using buffer cookie for accessing packet in memory.
> > +* Frags should be allocated from single 'memory' region,
> > +* hence common upper address half should be sufficient.
> > +*/
> > +   data_tmp = mvneta_frag_alloc(pp->frag_size);
> > +   if (data_tmp) {
> > +   pp->data_high = (u64)upper_32_bits((u64)data_tmp) << 32;
> > +   mvneta_frag_free(pp->frag_size, data_tmp);
> > +   }
> >   
> 
> How does this work when the region spans a n*4GB address boundary?

indeed. We also make use of this driver on 64bit platforms. We use
different solution to make the driver 64bit safe.

solA: make use of the reserved field in the mvneta_rx_desc, such
as reserved2 etc. Yes, the field is marked as "for future use, PnC", but
now it's not used at all. This is one possible solution however.

solB: allocate a shadow buf cookie during init, e.g

rxq->descs_bufcookie = kmalloc(rxq->size * sizeof(void*), GFP_KERNEL);

then modify mvneta_rx_desc_fill a bit to save the 64bit pointer in
the shadow buf cookie, e.g
static void mvneta_rx_desc_fill(struct mvneta_rx_desc *rx_desc,
u32 phys_addr, u32 cookie,
struct mvneta_rx_queue *rxq)

{
int i;

rx_desc->buf_cookie = cookie;
rx_desc->buf_phys_addr = phys_addr;
i = rx_desc - rxq->descs;
rxq->descs_bufcookie[i] = cookie;
}

then fetch the desc from the shadow buf cookie in all code path, such
as mvneta_rx() etc.

Both solutions should not have the problems pointed out by Arnd.

Thanks,
Jisheng

Re: [PATCH 1/2] net: mv643xx_eth: use {readl|writel}_relaxed instead of readl/writel

[Jisheng Zhang]

Dear Arnd,

On Fri, 13 May 2016 14:09:43 +0200 Arnd Bergmann wrote:

> On Friday 13 May 2016 19:59:19 Jisheng Zhang wrote:
> >  /* port register accessors 
> > **/
> >  static inline u32 rdl(struct mv643xx_eth_private *mp, int offset)
> >  {
> > -   return readl(mp->shared->base + offset);
> > +   return readl_relaxed(mp->shared->base + offset);
> >  }
> >  
> >  static inline u32 rdlp(struct mv643xx_eth_private *mp, int offset)
> >  {
> > -   return readl(mp->base + offset);
> > +   return readl_relaxed(mp->base + offset);
> >  }  
> 
> I'd recommend not changing these in general, but introducing new 
> 'rdl_relaxed()'
> and 'rdlp_relaxed()' etc variants that you use in the code that actually
> is performance sensitive, but use the normal non-relaxed versions by
> default.
> 
> Then add a comment to each use of the relaxed accessors on how you know
> that it's safe for that caller. This usually is just needed for the xmit()
> function and for the interrupt handler.

Got your points and I do think it makes sense. But could we always use the
relaxed version to save some LoCs, although I have no mv643xx_eth platform
but I can confirm similar relaxed version changes in pxa168_eth is safe and
this is what we do in product's kernel.

Above is just my humble opinion, comments are welcome.

Thanks,
Jisheng

>   
> > @@ -2642,10 +2642,10 @@ mv643xx_eth_conf_mbus_windows(struct 
> > mv643xx_eth_shared_private *msp,
> > int i;
> >  
> > for (i = 0; i < 6; i++) {
> > -   writel(0, base + WINDOW_BASE(i));
> > -   writel(0, base + WINDOW_SIZE(i));
> > +   writel_relaxed(0, base + WINDOW_BASE(i));
> > +   writel_relaxed(0, base + WINDOW_SIZE(i));
> > if (i < 4)
> > -   writel(0, base + WINDOW_REMAP_HIGH(i));
> > +   writel_relaxed(0, base + WINDOW_REMAP_HIGH(i));
> > }
> >  
> > win_enable = 0x3f;  
> 
> Configuring the mbus for instance is clearly not an operation in which
> performance matters at all, so better not touch that.
> 
> > @@ -2674,8 +2675,8 @@ static void infer_hw_params(struct 
> > mv643xx_eth_shared_private *msp)
> >  * [21:8], or a 16-bit coal limit in bits [25,21:7] of the
> >  * SDMA config register.
> >  */
> > -   writel(0x0200, msp->base + 0x0400 + SDMA_CONFIG);
> > -   if (readl(msp->base + 0x0400 + SDMA_CONFIG) & 0x0200)
> > +   writel_relaxed(0x0200, msp->base + 0x0400 + SDMA_CONFIG);
> > +   if (readl_relaxed(msp->base + 0x0400 + SDMA_CONFIG) & 0x0200)
> > msp->extended_rx_coal_limit = 1;
> > else
> > msp->extended_rx_coal_limit = 0;  
> 
> 
> This also seems to be configuration, rather than in the packet rx/tx hotpath.
> 
>   Arnd

[PATCH 1/2] net: mv643xx_eth: use {readl|writel}_relaxed instead of readl/writel

[Jisheng Zhang]

Since appropriate memory barriers are already there, use the relaxed
version to improve performance a bit.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
 drivers/net/ethernet/marvell/mv643xx_eth.c | 33 +++---
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c 
b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 5583118..c6d8124 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -433,22 +433,22 @@ struct mv643xx_eth_private {
 /* port register accessors **/
 static inline u32 rdl(struct mv643xx_eth_private *mp, int offset)
 {
-   return readl(mp->shared->base + offset);
+   return readl_relaxed(mp->shared->base + offset);
 }
 
 static inline u32 rdlp(struct mv643xx_eth_private *mp, int offset)
 {
-   return readl(mp->base + offset);
+   return readl_relaxed(mp->base + offset);
 }
 
 static inline void wrl(struct mv643xx_eth_private *mp, int offset, u32 data)
 {
-   writel(data, mp->shared->base + offset);
+   writel_relaxed(data, mp->shared->base + offset);
 }
 
 static inline void wrlp(struct mv643xx_eth_private *mp, int offset, u32 data)
 {
-   writel(data, mp->base + offset);
+   writel_relaxed(data, mp->base + offset);
 }
 
 
@@ -2642,10 +2642,10 @@ mv643xx_eth_conf_mbus_windows(struct 
mv643xx_eth_shared_private *msp,
int i;
 
for (i = 0; i < 6; i++) {
-   writel(0, base + WINDOW_BASE(i));
-   writel(0, base + WINDOW_SIZE(i));
+   writel_relaxed(0, base + WINDOW_BASE(i));
+   writel_relaxed(0, base + WINDOW_SIZE(i));
if (i < 4)
-   writel(0, base + WINDOW_REMAP_HIGH(i));
+   writel_relaxed(0, base + WINDOW_REMAP_HIGH(i));
}
 
win_enable = 0x3f;
@@ -2654,16 +2654,17 @@ mv643xx_eth_conf_mbus_windows(struct 
mv643xx_eth_shared_private *msp,
for (i = 0; i < dram->num_cs; i++) {
const struct mbus_dram_window *cs = dram->cs + i;
 
-   writel((cs->base & 0x) |
+   writel_relaxed((cs->base & 0x) |
(cs->mbus_attr << 8) |
dram->mbus_dram_target_id, base + WINDOW_BASE(i));
-   writel((cs->size - 1) & 0x, base + WINDOW_SIZE(i));
+   writel_relaxed((cs->size - 1) & 0x,
+   base + WINDOW_SIZE(i));
 
win_enable &= ~(1 << i);
win_protect |= 3 << (2 * i);
}
 
-   writel(win_enable, base + WINDOW_BAR_ENABLE);
+   writel_relaxed(win_enable, base + WINDOW_BAR_ENABLE);
msp->win_protect = win_protect;
 }
 
@@ -2674,8 +2675,8 @@ static void infer_hw_params(struct 
mv643xx_eth_shared_private *msp)
 * [21:8], or a 16-bit coal limit in bits [25,21:7] of the
 * SDMA config register.
 */
-   writel(0x0200, msp->base + 0x0400 + SDMA_CONFIG);
-   if (readl(msp->base + 0x0400 + SDMA_CONFIG) & 0x0200)
+   writel_relaxed(0x0200, msp->base + 0x0400 + SDMA_CONFIG);
+   if (readl_relaxed(msp->base + 0x0400 + SDMA_CONFIG) & 0x0200)
msp->extended_rx_coal_limit = 1;
else
msp->extended_rx_coal_limit = 0;
@@ -2685,12 +2686,12 @@ static void infer_hw_params(struct 
mv643xx_eth_shared_private *msp)
 * yes, whether its associated registers are in the old or
 * the new place.
 */
-   writel(1, msp->base + 0x0400 + TX_BW_MTU_MOVED);
-   if (readl(msp->base + 0x0400 + TX_BW_MTU_MOVED) & 1) {
+   writel_relaxed(1, msp->base + 0x0400 + TX_BW_MTU_MOVED);
+   if (readl_relaxed(msp->base + 0x0400 + TX_BW_MTU_MOVED) & 1) {
msp->tx_bw_control = TX_BW_CONTROL_NEW_LAYOUT;
} else {
-   writel(7, msp->base + 0x0400 + TX_BW_RATE);
-   if (readl(msp->base + 0x0400 + TX_BW_RATE) & 7)
+   writel_relaxed(7, msp->base + 0x0400 + TX_BW_RATE);
+   if (readl_relaxed(msp->base + 0x0400 + TX_BW_RATE) & 7)
msp->tx_bw_control = TX_BW_CONTROL_OLD_LAYOUT;
else
msp->tx_bw_control = TX_BW_CONTROL_ABSENT;
-- 
2.8.1

[PATCH 0/2] net: mv643xx_eth: improve performance

[Jisheng Zhang]

This series is to improve the mv643xx_eth driver performance by using
{readl|writel}_relaxed or appropriate memory barriers.

Since I have no mv643xx_eth platforms, tests are appreciated!

Jisheng Zhang (2):
  net: mv643xx_eth: use {readl|writel}_relaxed instead of readl/writel
  net: mv643xx_eth: use dma_wmb/rmb where appropriate

 drivers/net/ethernet/marvell/mv643xx_eth.c | 43 +++---
 1 file changed, 22 insertions(+), 21 deletions(-)

-- 
2.8.1

[PATCH 2/2] net: mv643xx_eth: use dma_wmb/rmb where appropriate

[Jisheng Zhang]

Update the mv643xx_eth driver to use the dma_rmb/wmb calls instead of
the full barriers in order to improve performance.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
 drivers/net/ethernet/marvell/mv643xx_eth.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c 
b/drivers/net/ethernet/marvell/mv643xx_eth.c
index c6d8124..13b71e3 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -536,7 +536,7 @@ static int rxq_process(struct rx_queue *rxq, int budget)
cmd_sts = rx_desc->cmd_sts;
if (cmd_sts & BUFFER_OWNED_BY_DMA)
break;
-   rmb();
+   dma_rmb();
 
skb = rxq->rx_skb[rxq->rx_curr_desc];
rxq->rx_skb[rxq->rx_curr_desc] = NULL;
@@ -647,9 +647,9 @@ static int rxq_refill(struct rx_queue *rxq, int budget)
  DMA_FROM_DEVICE);
rx_desc->buf_size = size;
rxq->rx_skb[rx] = skb;
-   wmb();
+   dma_wmb();
rx_desc->cmd_sts = BUFFER_OWNED_BY_DMA | RX_ENABLE_INTERRUPT;
-   wmb();
+   dma_wmb();
 
/*
 * The hardware automatically prepends 2 bytes of
@@ -889,7 +889,7 @@ static int txq_submit_tso(struct tx_queue *txq, struct 
sk_buff *skb,
skb_tx_timestamp(skb);
 
/* ensure all other descriptors are written before first cmd_sts */
-   wmb();
+   dma_wmb();
first_tx_desc->cmd_sts = first_cmd_sts;
 
/* clear TX_END status */
@@ -994,7 +994,7 @@ static int txq_submit_skb(struct tx_queue *txq, struct 
sk_buff *skb,
skb_tx_timestamp(skb);
 
/* ensure all other descriptors are written before first cmd_sts */
-   wmb();
+   dma_wmb();
desc->cmd_sts = cmd_sts;
 
/* clear TX_END status */
-- 
2.8.1

[PATCH 2/2] net: pxa168_eth: Use dma_wmb/rmb where appropriate

[Jisheng Zhang]

Update the pxa168_eth driver to use the dma_rmb/wmb calls instead of the
full barriers in order to improve performance: reduced 97ns/39ns on
average in tx/rx path on Marvell BG4CT platform.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
 drivers/net/ethernet/marvell/pxa168_eth.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c 
b/drivers/net/ethernet/marvell/pxa168_eth.c
index db774e3..728a115 100644
--- a/drivers/net/ethernet/marvell/pxa168_eth.c
+++ b/drivers/net/ethernet/marvell/pxa168_eth.c
@@ -342,9 +342,9 @@ static void rxq_refill(struct net_device *dev)
pep->rx_skb[used_rx_desc] = skb;
 
/* Return the descriptor to DMA ownership */
-   wmb();
+   dma_wmb();
p_used_rx_desc->cmd_sts = BUF_OWNED_BY_DMA | RX_EN_INT;
-   wmb();
+   dma_wmb();
 
/* Move the used descriptor pointer to the next descriptor */
pep->rx_used_desc_q = (used_rx_desc + 1) % pep->rx_ring_size;
@@ -794,7 +794,7 @@ static int rxq_process(struct net_device *dev, int budget)
rx_used_desc = pep->rx_used_desc_q;
rx_desc = >p_rx_desc_area[rx_curr_desc];
cmd_sts = rx_desc->cmd_sts;
-   rmb();
+   dma_rmb();
if (cmd_sts & (BUF_OWNED_BY_DMA))
break;
skb = pep->rx_skb[rx_curr_desc];
@@ -1289,7 +1289,7 @@ static int pxa168_eth_start_xmit(struct sk_buff *skb, 
struct net_device *dev)
 
skb_tx_timestamp(skb);
 
-   wmb();
+   dma_wmb();
desc->cmd_sts = BUF_OWNED_BY_DMA | TX_GEN_CRC | TX_FIRST_DESC |
TX_ZERO_PADDING | TX_LAST_DESC | TX_EN_INT;
wmb();
-- 
2.8.1

[PATCH 0/2] net: pxa168_eth: improve performance

[Jisheng Zhang]

This series is to improve the pxa168_eth driver performance by using
{readl|writel}_relaxed or appropriate memory barriers.

Jisheng Zhang (2):
  net: pxa168_eth: use {readl|writel}_relaxed instead of readl/writel
  net: pxa168_eth: Use dma_wmb/rmb where appropriate

 drivers/net/ethernet/marvell/pxa168_eth.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

-- 
2.8.1

[PATCH 1/2] net: pxa168_eth: use {readl|writel}_relaxed instead of readl/writel

[Jisheng Zhang]

Since appropriate memory barriers are already there, use the relaxed
version to improve performance a bit.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
 drivers/net/ethernet/marvell/pxa168_eth.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c 
b/drivers/net/ethernet/marvell/pxa168_eth.c
index c442f6a..db774e3 100644
--- a/drivers/net/ethernet/marvell/pxa168_eth.c
+++ b/drivers/net/ethernet/marvell/pxa168_eth.c
@@ -286,12 +286,12 @@ static int pxa168_eth_stop(struct net_device *dev);
 
 static inline u32 rdl(struct pxa168_eth_private *pep, int offset)
 {
-   return readl(pep->base + offset);
+   return readl_relaxed(pep->base + offset);
 }
 
 static inline void wrl(struct pxa168_eth_private *pep, int offset, u32 data)
 {
-   writel(data, pep->base + offset);
+   writel_relaxed(data, pep->base + offset);
 }
 
 static void abort_dma(struct pxa168_eth_private *pep)
-- 
2.8.1

[PATCH] net: mvneta: use cache_line_size() to get cacheline size

[Jisheng Zhang]

L1_CACHE_BYTES may not be the real cacheline size, use cache_line_size
to determine the cacheline size in runtime.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
Suggested-by: Marcin Wojtas <m...@semihalf.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 5880871..b1db000 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -299,7 +299,7 @@
 #define MVNETA_RX_PKT_SIZE(mtu) \
ALIGN((mtu) + MVNETA_MH_SIZE + MVNETA_VLAN_TAG_LEN + \
  ETH_HLEN + ETH_FCS_LEN,\
- L1_CACHE_BYTES)
+ cache_line_size())
 
 #define IS_TSO_HEADER(txq, addr) \
((addr >= txq->tso_hdrs_phys) && \
-- 
2.8.0.rc3

[PATCH] net: mvpp2: use cache_line_size() to get cacheline size

[Jisheng Zhang]

L1_CACHE_BYTES may not be the real cacheline size, use cache_line_size
to determine the cacheline size in runtime.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
Suggested-by: Marcin Wojtas <m...@semihalf.com>
---
 drivers/net/ethernet/marvell/mvpp2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2.c 
b/drivers/net/ethernet/marvell/mvpp2.c
index 05f358b..23f3c08 100644
--- a/drivers/net/ethernet/marvell/mvpp2.c
+++ b/drivers/net/ethernet/marvell/mvpp2.c
@@ -376,7 +376,7 @@
 
 #define MVPP2_RX_PKT_SIZE(mtu) \
ALIGN((mtu) + MVPP2_MH_SIZE + MVPP2_VLAN_TAG_LEN + \
- ETH_HLEN + ETH_FCS_LEN, L1_CACHE_BYTES)
+ ETH_HLEN + ETH_FCS_LEN, cache_line_size())
 
 #define MVPP2_RX_BUF_SIZE(pkt_size)((pkt_size) + NET_SKB_PAD)
 #define MVPP2_RX_TOTAL_SIZE(buf_size)  ((buf_size) + MVPP2_SKB_SHINFO_SIZE)
-- 
2.8.0.rc3

Re: [PATCH] net: mvneta: replace MVNETA_CPU_D_CACHE_LINE_SIZE with L1_CACHE_BYTES

[Jisheng Zhang]

Hi David, Thomas,

On Thu, 31 Mar 2016 16:47:10 -0400 David Miller  wrote:

> From: Thomas Petazzoni <thomas.petazz...@free-electrons.com>
> Date: Thu, 31 Mar 2016 22:37:35 +0200
> 
> > Hello,
> > 
> > On Thu, 31 Mar 2016 15:15:47 -0400 (EDT), David Miller wrote:  
> >> From: Jisheng Zhang <jszh...@marvell.com>
> >> Date: Wed, 30 Mar 2016 19:55:21 +0800
> >>   
> >> > The mvneta is also used in some Marvell berlin family SoCs which may
> >> > have 64bytes cacheline size. Replace the MVNETA_CPU_D_CACHE_LINE_SIZE
> >> > usage with L1_CACHE_BYTES.
> >> > 
> >> > And since dma_alloc_coherent() is always cacheline size aligned, so
> >> > remove the align checks.
> >> > 
> >> > Signed-off-by: Jisheng Zhang <jszh...@marvell.com>  
> >> 
> >> Applied.  
> > 
> > A new version of the patch was sent, which more rightfully uses
> > cache_line_size(), see:
> > 
> >  "[PATCH v2] net: mvneta: replace MVNETA_CPU_D_CACHE_LINE_SIZE with 
> > cache_line_size"  
> 
> Sorry about that.
> 
> Send me a realtive fixup patch if you like.
> 

Sorry about inconvenience, I'll send out fixup patch.

Thanks,
Jisheng

[PATCH v2] net: mvpp2: replace MVPP2_CPU_D_CACHE_LINE_SIZE with cache_line_size

[Jisheng Zhang]

The mvpp2 ip maybe used in SoCs which may have have different cacheline
size. Replace the MVPP2_CPU_D_CACHE_LINE_SIZE with cache_line_size.

And since dma_alloc_coherent() is always cacheline size aligned, so
remove the align checks.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
Since v1:
 - use cache_line_size() suggested by Marcin

 drivers/net/ethernet/marvell/mvpp2.c | 14 +-
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2.c 
b/drivers/net/ethernet/marvell/mvpp2.c
index e9aa8d9..868a957 100644
--- a/drivers/net/ethernet/marvell/mvpp2.c
+++ b/drivers/net/ethernet/marvell/mvpp2.c
@@ -321,7 +321,6 @@
 /* Lbtd 802.3 type */
 #define MVPP2_IP_LBDT_TYPE 0xfffa
 
-#define MVPP2_CPU_D_CACHE_LINE_SIZE32
 #define MVPP2_TX_CSUM_MAX_SIZE 9800
 
 /* Timeout constants */
@@ -377,7 +376,7 @@
 
 #define MVPP2_RX_PKT_SIZE(mtu) \
ALIGN((mtu) + MVPP2_MH_SIZE + MVPP2_VLAN_TAG_LEN + \
- ETH_HLEN + ETH_FCS_LEN, MVPP2_CPU_D_CACHE_LINE_SIZE)
+ ETH_HLEN + ETH_FCS_LEN, cache_line_size())
 
 #define MVPP2_RX_BUF_SIZE(pkt_size)((pkt_size) + NET_SKB_PAD)
 #define MVPP2_RX_TOTAL_SIZE(buf_size)  ((buf_size) + MVPP2_SKB_SHINFO_SIZE)
@@ -4493,10 +4492,6 @@ static int mvpp2_aggr_txq_init(struct platform_device 
*pdev,
if (!aggr_txq->descs)
return -ENOMEM;
 
-   /* Make sure descriptor address is cache line size aligned  */
-   BUG_ON(aggr_txq->descs !=
-  PTR_ALIGN(aggr_txq->descs, MVPP2_CPU_D_CACHE_LINE_SIZE));
-
aggr_txq->last_desc = aggr_txq->size - 1;
 
/* Aggr TXQ no reset WA */
@@ -4526,9 +4521,6 @@ static int mvpp2_rxq_init(struct mvpp2_port *port,
if (!rxq->descs)
return -ENOMEM;
 
-   BUG_ON(rxq->descs !=
-  PTR_ALIGN(rxq->descs, MVPP2_CPU_D_CACHE_LINE_SIZE));
-
rxq->last_desc = rxq->size - 1;
 
/* Zero occupied and non-occupied counters - direct access */
@@ -4616,10 +4608,6 @@ static int mvpp2_txq_init(struct mvpp2_port *port,
if (!txq->descs)
return -ENOMEM;
 
-   /* Make sure descriptor address is cache line size aligned  */
-   BUG_ON(txq->descs !=
-  PTR_ALIGN(txq->descs, MVPP2_CPU_D_CACHE_LINE_SIZE));
-
txq->last_desc = txq->size - 1;
 
/* Set Tx descriptors queue starting address - indirect access */
-- 
2.8.0.rc3

[PATCH] net: mvpp2: fix maybe-uninitialized warning

[Jisheng Zhang]

This is to fix the following maybe-uninitialized warning:

drivers/net/ethernet/marvell/mvpp2.c:6007:18: warning: 'err' may be
used uninitialized in this function [-Wmaybe-uninitialized]

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
 drivers/net/ethernet/marvell/mvpp2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2.c 
b/drivers/net/ethernet/marvell/mvpp2.c
index c797971a..e9aa8d9 100644
--- a/drivers/net/ethernet/marvell/mvpp2.c
+++ b/drivers/net/ethernet/marvell/mvpp2.c
@@ -6059,8 +6059,10 @@ static int mvpp2_port_init(struct mvpp2_port *port)
 
/* Map physical Rx queue to port's logical Rx queue */
rxq = devm_kzalloc(dev, sizeof(*rxq), GFP_KERNEL);
-   if (!rxq)
+   if (!rxq) {
+   err = -ENOMEM;
goto err_free_percpu;
+   }
/* Map this Rx queue to a physical queue */
rxq->id = port->first_rxq + queue;
rxq->port = port->id;
-- 
2.8.0.rc3

[PATCH v2] net: mvneta: replace MVNETA_CPU_D_CACHE_LINE_SIZE with cache_line_size

[Jisheng Zhang]

The mvneta is also used in some Marvell berlin family SoCs which may
have different cacheline size. Replace the MVNETA_CPU_D_CACHE_LINE_SIZE
usage with cache_line_size().

And since dma_alloc_coherent() is always cacheline size aligned, so
remove the align checks.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
Since v1:
 - use cache_line_size() suggested by Marcin

 drivers/net/ethernet/marvell/mvneta.c | 10 +-
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 577f7ca..b1db000 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -260,7 +260,6 @@
 
 #define MVNETA_VLAN_TAG_LEN 4
 
-#define MVNETA_CPU_D_CACHE_LINE_SIZE32
 #define MVNETA_TX_CSUM_DEF_SIZE1600
 #define MVNETA_TX_CSUM_MAX_SIZE9800
 #define MVNETA_ACC_MODE_EXT1   1
@@ -300,7 +299,7 @@
 #define MVNETA_RX_PKT_SIZE(mtu) \
ALIGN((mtu) + MVNETA_MH_SIZE + MVNETA_VLAN_TAG_LEN + \
  ETH_HLEN + ETH_FCS_LEN,\
- MVNETA_CPU_D_CACHE_LINE_SIZE)
+ cache_line_size())
 
 #define IS_TSO_HEADER(txq, addr) \
((addr >= txq->tso_hdrs_phys) && \
@@ -2764,9 +2763,6 @@ static int mvneta_rxq_init(struct mvneta_port *pp,
if (rxq->descs == NULL)
return -ENOMEM;
 
-   BUG_ON(rxq->descs !=
-  PTR_ALIGN(rxq->descs, MVNETA_CPU_D_CACHE_LINE_SIZE));
-
rxq->last_desc = rxq->size - 1;
 
/* Set Rx descriptors queue starting address */
@@ -2837,10 +2833,6 @@ static int mvneta_txq_init(struct mvneta_port *pp,
if (txq->descs == NULL)
return -ENOMEM;
 
-   /* Make sure descriptor address is cache line size aligned  */
-   BUG_ON(txq->descs !=
-  PTR_ALIGN(txq->descs, MVNETA_CPU_D_CACHE_LINE_SIZE));
-
txq->last_desc = txq->size - 1;
 
/* Set maximum bandwidth for enabled TXQs */
-- 
2.8.0.rc3

Re: [PATCH] net: mvneta: explicitly disable BM on 64bit platform

[Jisheng Zhang]

Hi Marcin,

On Thu, 31 Mar 2016 08:49:19 +0200 Marcin Wojtas wrote:

> Hi Jisheng,
> 
> 2016-03-31 7:53 GMT+02:00 Jisheng Zhang <jszh...@marvell.com>:
> > Hi Gregory,
> >
> > On Wed, 30 Mar 2016 17:11:41 +0200 Gregory CLEMENT wrote:
> >  
> >> Hi Jisheng,
> >>
> >>  On mer., mars 30 2016, Jisheng Zhang <jszh...@marvell.com> wrote:
> >>  
> >> > The mvneta BM can't work on 64bit platform, as the BM hardware expects
> >> > buf virtual address to be placed in the first four bytes of mapped
> >> > buffer, but obviously the virtual address on 64bit platform can't be
> >> > stored in 4 bytes. So we have to explicitly disable BM on 64bit
> >> > platform.  
> >>
> >> Actually mvneta is used on Armada 3700 which is a 64bits platform.
> >> Is it true that the driver needs some change to use BM in 64 bits, but
> >> we don't have to disable it.
> >>
> >> Here is the 64 bits part of the patch we have currently on the hardware
> >> prototype. We have more things which are really related to the way the
> >> mvneta is connected to the Armada 3700 SoC. This code was not ready for  
> >
> > Thanks for the sharing.
> >
> > I think we could commit easy parts firstly, for example: the cacheline size
> > hardcoding, either piece of your diff or my version:
> >
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/418513.html
> >   
> 
> Since the commit:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm64/include/asm/cache.h?id=97303480753e48fb313dc0e15daaf11b0451cdb8
> detached L1_CACHE_BYTES from real cache size, I suggest, the macro should be:
> #define MVNETA_CPU_D_CACHE_LINE_SIZE cache_line_size()

Thanks for the hint. I'll send out updated version to address the cacheline size
issue.

> 
> Regarding check after dma_alloc_coherent, I agree it's not necessary.
> 
> >  
> >> mainline but I prefer share it now instead of having the HWBM blindly  
> >
> > I have looked through the diff, it is for the driver itself on 64bit 
> > platforms,
> > and it doesn't touch BM. The BM itself need to be disabled for 64bit, I'm 
> > not
> > sure the BM could work on 64bit even with your diff. Per my understanding, 
> > the BM
> > can't work on 64 bit, let's have a look at some piece of the 
> > mvneta_bm_construct()
> >
> > *(u32 *)buf = (u32)buf;  
> 
> Indeed this particular part is different and unclear, I tried
> different options - with no success. I'm checking with design team
> now. Anyway, I managed to enable operation for HWBM on A3700 with one
> work-around in mvneta_hwbm_rx():
> data = phys_to_virt(rx_desc->buf_phys_addr);

oh yes! This seems a good idea. And If we replace all

data = (void *)rx_desc->buf_cookie

with

data = phys_to_virt(rx_desc->buf_phys_addr);

we also resolve the buf_cookie issue on 64bit platforms! no need to introduce
data_high or use existing reserved member to store virtual address' higher 
32bits

> 
> Of course mvneta_bm, due to some silicone differences needed also a rework.
> 
> Actually I'd wait with updating 64-bit parts of mvneta, until real
> support for such machine's controller is introduced. Basing on my
> experience with enabling neta on A3700, it turns out to be more
> changes.

I agree with you. And I need one more rework: berlin SoCs don't have mbus
concept at all ;)

Thanks for your hints,
Jisheng

Re: [PATCH] net: mvneta: remove useless RX descriptor prefetch

[Jisheng Zhang]

Hi all,

On Thu, 31 Mar 2016 14:45:37 +0800 Jisheng Zhang wrote:

> Hi,
> 
> + linux arm kernel
> 
> On Thu, 31 Mar 2016 14:36:30 +0800 Jisheng Zhang wrote:
> 
> > The rx descriptors are allocated using dma_alloc_coherent, so prefetch
> > doesn't really happen at all.  
> 
> This is for RFC, I'm sorry to send it without changing its title -- 
> s/PATCH/RFC.
> 
> I'm not sure whether there's any benefit to prefetch on space allocated from
> dma_alloc_coherent.

After more consideration, I think my patch is wrong.

As for coherent platforms, the space allocated from dma_alloc_coherent is
cacheable, so prefetch would definitely benefit us.

As for noncoherent platforms, the space allocated from dma_alloc_coherent is
uncacheable, but prefetch on arm/arm64 is implemented via. pld/prfm, the op
would be nop if target address is uncacheable.

So let's drop this patch.

Thanks,
Jisheng


> 
> Thanks
> 
> > 
> > Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
> > ---
> >  drivers/net/ethernet/marvell/mvneta.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> > b/drivers/net/ethernet/marvell/mvneta.c
> > index 5880871..6c09a27 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -757,7 +757,6 @@ mvneta_rxq_next_desc_get(struct mvneta_rx_queue *rxq)
> > int rx_desc = rxq->next_desc_to_proc;
> >  
> > rxq->next_desc_to_proc = MVNETA_QUEUE_NEXT_DESC(rxq, rx_desc);
> > -   prefetch(rxq->descs + rxq->next_desc_to_proc);
> > return rxq->descs + rx_desc;
> >  }
> >
> 
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] net: mvneta: remove useless RX descriptor prefetch

[Jisheng Zhang]

Hi,

+ linux arm kernel

On Thu, 31 Mar 2016 14:36:30 +0800 Jisheng Zhang wrote:

> The rx descriptors are allocated using dma_alloc_coherent, so prefetch
> doesn't really happen at all.

This is for RFC, I'm sorry to send it without changing its title -- s/PATCH/RFC.

I'm not sure whether there's any benefit to prefetch on space allocated from
dma_alloc_coherent.

Thanks

> 
> Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> b/drivers/net/ethernet/marvell/mvneta.c
> index 5880871..6c09a27 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -757,7 +757,6 @@ mvneta_rxq_next_desc_get(struct mvneta_rx_queue *rxq)
>   int rx_desc = rxq->next_desc_to_proc;
>  
>   rxq->next_desc_to_proc = MVNETA_QUEUE_NEXT_DESC(rxq, rx_desc);
> - prefetch(rxq->descs + rxq->next_desc_to_proc);
>   return rxq->descs + rx_desc;
>  }
>  

[PATCH] net: mvneta: remove useless RX descriptor prefetch

[Jisheng Zhang]

The rx descriptors are allocated using dma_alloc_coherent, so prefetch
doesn't really happen at all.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 5880871..6c09a27 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -757,7 +757,6 @@ mvneta_rxq_next_desc_get(struct mvneta_rx_queue *rxq)
int rx_desc = rxq->next_desc_to_proc;
 
rxq->next_desc_to_proc = MVNETA_QUEUE_NEXT_DESC(rxq, rx_desc);
-   prefetch(rxq->descs + rxq->next_desc_to_proc);
return rxq->descs + rx_desc;
 }
 
-- 
2.8.0.rc3

Re: [PATCH] net: mvneta: explicitly disable BM on 64bit platform

[Jisheng Zhang]

Hi Gregory,

On Wed, 30 Mar 2016 17:11:41 +0200 Gregory CLEMENT wrote:

> Hi Jisheng,
>  
>  On mer., mars 30 2016, Jisheng Zhang <jszh...@marvell.com> wrote:
> 
> > The mvneta BM can't work on 64bit platform, as the BM hardware expects
> > buf virtual address to be placed in the first four bytes of mapped
> > buffer, but obviously the virtual address on 64bit platform can't be
> > stored in 4 bytes. So we have to explicitly disable BM on 64bit
> > platform.  
> 
> Actually mvneta is used on Armada 3700 which is a 64bits platform.
> Is it true that the driver needs some change to use BM in 64 bits, but
> we don't have to disable it.
> 
> Here is the 64 bits part of the patch we have currently on the hardware
> prototype. We have more things which are really related to the way the
> mvneta is connected to the Armada 3700 SoC. This code was not ready for

Thanks for the sharing.

I think we could commit easy parts firstly, for example: the cacheline size
hardcoding, either piece of your diff or my version:

http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/418513.html

> mainline but I prefer share it now instead of having the HWBM blindly

I have looked through the diff, it is for the driver itself on 64bit platforms,
and it doesn't touch BM. The BM itself need to be disabled for 64bit, I'm not
sure the BM could work on 64bit even with your diff. Per my understanding, the 
BM
can't work on 64 bit, let's have a look at some piece of the 
mvneta_bm_construct()

*(u32 *)buf = (u32)buf;

Am I misunderstanding?

Thanks,
Jisheng

> disable for 64 bits platform:
> 
> --- a/drivers/net/ethernet/marvell/Kconfig
> +++ b/drivers/net/ethernet/marvell/Kconfig
> @@ -55,7 +55,7 @@ config MVNETA_BM_ENABLE
>  
>  config MVNETA
>   tristate "Marvell Armada 370/38x/XP network interface support"
> - depends on PLAT_ORION
> + depends on ARCH_MVEBU || COMPILE_TEST
>   select MVMDIO
>   select FIXED_PHY
>   ---help---
> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> b/drivers/net/ethernet/marvell/mvneta.c
> index 577f7ca7deba..6929ad112b64 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -260,7 +260,7 @@
>  
>  #define MVNETA_VLAN_TAG_LEN 4
>  
> -#define MVNETA_CPU_D_CACHE_LINE_SIZE32
> +#define MVNETA_CPU_D_CACHE_LINE_SIZEcache_line_size()
>  #define MVNETA_TX_CSUM_DEF_SIZE  1600
>  #define MVNETA_TX_CSUM_MAX_SIZE  9800
>  #define MVNETA_ACC_MODE_EXT1 1
> @@ -297,6 +297,12 @@
>  /* descriptor aligned size */
>  #define MVNETA_DESC_ALIGNED_SIZE 32
>  
> +/* Number of bytes to be taken into account by HW when putting incoming data
> + * to the buffers. It is needed in case NET_SKB_PAD exceeds maximum packet
> + * offset supported in MVNETA_RXQ_CONFIG_REG(q) registers.
> + */
> +#define MVNETA_RX_PKT_OFFSET_CORRECTION  64
> +
>  #define MVNETA_RX_PKT_SIZE(mtu) \
>   ALIGN((mtu) + MVNETA_MH_SIZE + MVNETA_VLAN_TAG_LEN + \
> ETH_HLEN + ETH_FCS_LEN,\
> @@ -417,6 +423,10 @@ struct mvneta_port {
>   u64 ethtool_stats[ARRAY_SIZE(mvneta_statistics)];
>  
>   u32 indir[MVNETA_RSS_LU_TABLE_SIZE];
> +#ifdef CONFIG_64BIT
> + u64 data_high;
> +#endif
> + u16 rx_offset_correction;
>  };
>  
>  /* The mvneta_tx_desc and mvneta_rx_desc structures describe the
> @@ -961,7 +971,9 @@ static int mvneta_bm_port_init(struct platform_device 
> *pdev,
>  struct mvneta_port *pp)
>  {
>   struct device_node *dn = pdev->dev.of_node;
> - u32 long_pool_id, short_pool_id, wsize;
> + u32 long_pool_id, short_pool_id;
> +#ifndef CONFIG_64BIT
> + u32 wsize;
>   u8 target, attr;
>   int err;
>  
> @@ -985,7 +997,7 @@ static int mvneta_bm_port_init(struct platform_device 
> *pdev,
>   netdev_info(pp->dev, "missing long pool id\n");
>   return -EINVAL;
>   }
> -
> +#endif
>   /* Create port's long pool depending on mtu */
>   pp->pool_long = mvneta_bm_pool_use(pp->bm_priv, long_pool_id,
>  MVNETA_BM_LONG, pp->id,
> @@ -1790,6 +1802,10 @@ static int mvneta_rx_refill(struct mvneta_port *pp,
>   if (!data)
>   return -ENOMEM;
>  
> +#ifdef CONFIG_64BIT
> + if (unlikely(pp->data_high != ((u64)data & 0x)))
> + return -ENOMEM;
> +#endif
>   phys_addr = dma_map_single(pp->dev->dev.parent, data,
>  MVNETA_RX_BUF_SIZE(pp->pkt_size),
>

Re: [RFC] net: mvneta: 64bit platform support

[Jisheng Zhang]

On Wed, 30 Mar 2016 21:37:00 +0800 Jisheng Zhang wrote:

> Hi all,
> 
> Obviously, current mvneta driver can't work on 64bit platforms. For one thing
> the BM feature should be explicitly disabled, I just sent out one patch for
> this purpose. 
> 
> What's more, the buf_cookie in mvneta_rx_desc need to be carefully considered.
> The driver use the buf_cookie(u32 type) to store the buffer virtual address,
> obviously it can't store the virtual address on 64bit platforms. I have two
> solutions:
> 
> solution A: let one reserved type in current mvneta_rx_desc, e.g reserved5
> to store the high 32bit virt address, and hack code as the following:
> #ifdef CONFIG_64BIT
> rx_desc->reserved5 = high32(data);
> #endif

oh, missing some code:
#ifdef CONFIG_64BIT
data = ((u64)rx_desc->reserved5 << 32) | rx_desc->buf_cookie;
#else
data = (void*)rx_desc->buf_cookie;
#endif

> 
> solution B: add one member void **buf_virt_ptrs in mvneta_rx_queue, and point
> all buf_cookie usage to the according buf_virt_ptrs[i]
> 
> 
> Is there any elegant solutions?
> 
> Thanks,
> Jisheng
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC] net: mvneta: 64bit platform support

[Jisheng Zhang]

Hi all,

Obviously, current mvneta driver can't work on 64bit platforms. For one thing
the BM feature should be explicitly disabled, I just sent out one patch for
this purpose. 

What's more, the buf_cookie in mvneta_rx_desc need to be carefully considered.
The driver use the buf_cookie(u32 type) to store the buffer virtual address,
obviously it can't store the virtual address on 64bit platforms. I have two
solutions:

solution A: let one reserved type in current mvneta_rx_desc, e.g reserved5
to store the high 32bit virt address, and hack code as the following:
#ifdef CONFIG_64BIT
rx_desc->reserved5 = high32(data);
#endif

solution B: add one member void **buf_virt_ptrs in mvneta_rx_queue, and point
all buf_cookie usage to the according buf_virt_ptrs[i]


Is there any elegant solutions?

Thanks,
Jisheng

[PATCH] net: mvneta: explicitly disable BM on 64bit platform

[Jisheng Zhang]

The mvneta BM can't work on 64bit platform, as the BM hardware expects
buf virtual address to be placed in the first four bytes of mapped
buffer, but obviously the virtual address on 64bit platform can't be
stored in 4 bytes. So we have to explicitly disable BM on 64bit
platform.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
 drivers/net/ethernet/marvell/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/Kconfig 
b/drivers/net/ethernet/marvell/Kconfig
index b5c6d42..53d6572 100644
--- a/drivers/net/ethernet/marvell/Kconfig
+++ b/drivers/net/ethernet/marvell/Kconfig
@@ -42,7 +42,7 @@ config MVMDIO
 
 config MVNETA_BM_ENABLE
tristate "Marvell Armada 38x/XP network interface BM support"
-   depends on MVNETA
+   depends on MVNETA && !64BIT
---help---
  This driver supports auxiliary block of the network
  interface units in the Marvell ARMADA XP and ARMADA 38x SoC
-- 
2.8.0.rc3

[PATCH] net: mvneta: replace MVNETA_CPU_D_CACHE_LINE_SIZE with L1_CACHE_BYTES

[Jisheng Zhang]

The mvneta is also used in some Marvell berlin family SoCs which may
have 64bytes cacheline size. Replace the MVNETA_CPU_D_CACHE_LINE_SIZE
usage with L1_CACHE_BYTES.

And since dma_alloc_coherent() is always cacheline size aligned, so
remove the align checks.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 10 +-
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 577f7ca..5880871 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -260,7 +260,6 @@
 
 #define MVNETA_VLAN_TAG_LEN 4
 
-#define MVNETA_CPU_D_CACHE_LINE_SIZE32
 #define MVNETA_TX_CSUM_DEF_SIZE1600
 #define MVNETA_TX_CSUM_MAX_SIZE9800
 #define MVNETA_ACC_MODE_EXT1   1
@@ -300,7 +299,7 @@
 #define MVNETA_RX_PKT_SIZE(mtu) \
ALIGN((mtu) + MVNETA_MH_SIZE + MVNETA_VLAN_TAG_LEN + \
  ETH_HLEN + ETH_FCS_LEN,\
- MVNETA_CPU_D_CACHE_LINE_SIZE)
+ L1_CACHE_BYTES)
 
 #define IS_TSO_HEADER(txq, addr) \
((addr >= txq->tso_hdrs_phys) && \
@@ -2764,9 +2763,6 @@ static int mvneta_rxq_init(struct mvneta_port *pp,
if (rxq->descs == NULL)
return -ENOMEM;
 
-   BUG_ON(rxq->descs !=
-  PTR_ALIGN(rxq->descs, MVNETA_CPU_D_CACHE_LINE_SIZE));
-
rxq->last_desc = rxq->size - 1;
 
/* Set Rx descriptors queue starting address */
@@ -2837,10 +2833,6 @@ static int mvneta_txq_init(struct mvneta_port *pp,
if (txq->descs == NULL)
return -ENOMEM;
 
-   /* Make sure descriptor address is cache line size aligned  */
-   BUG_ON(txq->descs !=
-  PTR_ALIGN(txq->descs, MVNETA_CPU_D_CACHE_LINE_SIZE));
-
txq->last_desc = txq->size - 1;
 
/* Set maximum bandwidth for enabled TXQs */
-- 
2.8.0.rc3

[PATCH] net: mvpp2: replace MVPP2_CPU_D_CACHE_LINE_SIZE with L1_CACHE_BYTES

[Jisheng Zhang]

The mvpp2 ip maybe used in SoCs which may have have 64bytes cacheline
size. Replace the MVPP2_CPU_D_CACHE_LINE_SIZE with L1_CACHE_BYTES.

And since dma_alloc_coherent() is always cacheline size aligned, so
remove the align checks.

Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
---
 drivers/net/ethernet/marvell/mvpp2.c | 14 +-
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2.c 
b/drivers/net/ethernet/marvell/mvpp2.c
index c797971a..05f358b 100644
--- a/drivers/net/ethernet/marvell/mvpp2.c
+++ b/drivers/net/ethernet/marvell/mvpp2.c
@@ -321,7 +321,6 @@
 /* Lbtd 802.3 type */
 #define MVPP2_IP_LBDT_TYPE 0xfffa
 
-#define MVPP2_CPU_D_CACHE_LINE_SIZE32
 #define MVPP2_TX_CSUM_MAX_SIZE 9800
 
 /* Timeout constants */
@@ -377,7 +376,7 @@
 
 #define MVPP2_RX_PKT_SIZE(mtu) \
ALIGN((mtu) + MVPP2_MH_SIZE + MVPP2_VLAN_TAG_LEN + \
- ETH_HLEN + ETH_FCS_LEN, MVPP2_CPU_D_CACHE_LINE_SIZE)
+ ETH_HLEN + ETH_FCS_LEN, L1_CACHE_BYTES)
 
 #define MVPP2_RX_BUF_SIZE(pkt_size)((pkt_size) + NET_SKB_PAD)
 #define MVPP2_RX_TOTAL_SIZE(buf_size)  ((buf_size) + MVPP2_SKB_SHINFO_SIZE)
@@ -4493,10 +4492,6 @@ static int mvpp2_aggr_txq_init(struct platform_device 
*pdev,
if (!aggr_txq->descs)
return -ENOMEM;
 
-   /* Make sure descriptor address is cache line size aligned  */
-   BUG_ON(aggr_txq->descs !=
-  PTR_ALIGN(aggr_txq->descs, MVPP2_CPU_D_CACHE_LINE_SIZE));
-
aggr_txq->last_desc = aggr_txq->size - 1;
 
/* Aggr TXQ no reset WA */
@@ -4526,9 +4521,6 @@ static int mvpp2_rxq_init(struct mvpp2_port *port,
if (!rxq->descs)
return -ENOMEM;
 
-   BUG_ON(rxq->descs !=
-  PTR_ALIGN(rxq->descs, MVPP2_CPU_D_CACHE_LINE_SIZE));
-
rxq->last_desc = rxq->size - 1;
 
/* Zero occupied and non-occupied counters - direct access */
@@ -4616,10 +4608,6 @@ static int mvpp2_txq_init(struct mvpp2_port *port,
if (!txq->descs)
return -ENOMEM;
 
-   /* Make sure descriptor address is cache line size aligned  */
-   BUG_ON(txq->descs !=
-  PTR_ALIGN(txq->descs, MVPP2_CPU_D_CACHE_LINE_SIZE));
-
txq->last_desc = txq->size - 1;
 
/* Set Tx descriptors queue starting address - indirect access */
-- 
2.8.0.rc3

Re: [PATCH net 1/3] net: mvneta: Fix spinlock usage

[Jisheng Zhang]

Dear Gregory,

On Wed, 9 Mar 2016 08:49:40 +0100 Gregory CLEMENT wrote:

> Hi Jisheng,
>  
>  On mer., mars 09 2016, Jisheng Zhang <jszh...@marvell.com> wrote:
> 
> > Dear Gregory,
> >
> > On Tue, 8 Mar 2016 13:57:04 +0100 Gregory CLEMENT wrote:
> >  
> >> In the previous patch, the spinlock was not initialized. While it didn't
> >> cause any trouble yet it could be a problem to use it uninitialized.
> >> 
> >> The most annoying part was the critical section protected by the spinlock
> >> in mvneta_stop(). Some of the functions could sleep as pointed when
> >> activated CONFIG_DEBUG_ATOMIC_SLEEP. Actually, in mvneta_stop() we only
> >> need to protect the is_stopped flagged, indeed the code of the notifier
> >> for CPU online is protected by the same spinlock, so when we get the
> >> lock, the notifer work is done.
> >> 
> >> Reported-by: Patrick Uiterwijk <patr...@puiterwijk.org>
> >> Signed-off-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
> >> ---
> >>  drivers/net/ethernet/marvell/mvneta.c | 11 ++-
> >>  1 file changed, 6 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> >> b/drivers/net/ethernet/marvell/mvneta.c
> >> index b0ae69f84493..8dc7df2edff6 100644
> >> --- a/drivers/net/ethernet/marvell/mvneta.c
> >> +++ b/drivers/net/ethernet/marvell/mvneta.c
> >> @@ -3070,17 +3070,17 @@ static int mvneta_stop(struct net_device *dev)
> >>struct mvneta_port *pp = netdev_priv(dev);
> >>  
> >>/* Inform that we are stopping so we don't want to setup the
> >> -   * driver for new CPUs in the notifiers
> >> +   * driver for new CPUs in the notifiers. The code of the
> >> +   * notifier for CPU online is protected by the same spinlock,
> >> +   * so when we get the lock, the notifer work is done.
> >> */
> >>spin_lock(>lock);
> >>pp->is_stopped = true;
> >> +  spin_unlock(>lock);  
> >
> > This fix sleep in atomic issue. But
> > I see race here. Let's assume is_stopped is false.  
> 
> You forgot that the lock was hold in the mvneta_percpu_notifier so your
> scenario can't happen.
> 
> >
> > cpu0:   cpu1:
> > mvneta_percpu_notifier():   mvneta_stop():
> >  
> 
> spin_lock(>lock);
> 
> > if (pp->is_stopped) {
> > spin_unlock(>lock);
> > break;
> > }

OOPS, I misread the code here as "the lock will be unlocked" ;)
Sorry for noise.

If you want, feel free to add

Reviewed-by: Jisheng Zhang <jszh...@marvell.com>

> >  
> 
>   the lock is hold in
>   mvneta_percpu_notifier(), so as
>   said in the comment this cpu is
>   waiting for on the following
>   line:
>   spin_lock(>lock);
> 
>   This code will be executed only
>   when the lock will be released
> > pp->is_stopped = true;
> > spin_unlock(>lock);
> >
> >
> > netif_tx_stop_all_queues(pp->dev);
> > for_each_online_cpu(other_cpu) {
> > 
> >  
> So what will happen is:
> cpu0: cpu1:
> mvneta_percpu_notifier(): mvneta_stop():
> 
> spin_lock(>lock);
> if (pp->is_stopped) {
>   spin_unlock(>lock);
>   break;
> }
> spin_lock(>lock);
> 
> netif_tx_stop_all_queues(pp->dev);
> for_each_online_cpu(other_cpu) {
> 
> spin_unlock(>lock);
>   pp->is_stopped = true;
>   spin_unlock(>lock);
> 
> 
> Gregory
> 

Re: [PATCH net 1/3] net: mvneta: Fix spinlock usage

[Jisheng Zhang]

Dear Gregory,

On Tue, 8 Mar 2016 13:57:04 +0100 Gregory CLEMENT wrote:

> In the previous patch, the spinlock was not initialized. While it didn't
> cause any trouble yet it could be a problem to use it uninitialized.
> 
> The most annoying part was the critical section protected by the spinlock
> in mvneta_stop(). Some of the functions could sleep as pointed when
> activated CONFIG_DEBUG_ATOMIC_SLEEP. Actually, in mvneta_stop() we only
> need to protect the is_stopped flagged, indeed the code of the notifier
> for CPU online is protected by the same spinlock, so when we get the
> lock, the notifer work is done.
> 
> Reported-by: Patrick Uiterwijk 
> Signed-off-by: Gregory CLEMENT 
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> b/drivers/net/ethernet/marvell/mvneta.c
> index b0ae69f84493..8dc7df2edff6 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -3070,17 +3070,17 @@ static int mvneta_stop(struct net_device *dev)
>   struct mvneta_port *pp = netdev_priv(dev);
>  
>   /* Inform that we are stopping so we don't want to setup the
> -  * driver for new CPUs in the notifiers
> +  * driver for new CPUs in the notifiers. The code of the
> +  * notifier for CPU online is protected by the same spinlock,
> +  * so when we get the lock, the notifer work is done.
>*/
>   spin_lock(>lock);
>   pp->is_stopped = true;
> + spin_unlock(>lock);

This fix sleep in atomic issue. But
I see race here. Let's assume is_stopped is false.

cpu0:   cpu1:
mvneta_percpu_notifier():   mvneta_stop():

if (pp->is_stopped) {
spin_unlock(>lock);
break;
}

pp->is_stopped = true;
spin_unlock(>lock);


netif_tx_stop_all_queues(pp->dev);
for_each_online_cpu(other_cpu) {


Thanks,
Jisheng

> +
>   mvneta_stop_dev(pp);
>   mvneta_mdio_remove(pp);
>   unregister_cpu_notifier(>cpu_notifier);
> - /* Now that the notifier are unregistered, we can release le
> -  * lock
> -  */
> - spin_unlock(>lock);
>   on_each_cpu(mvneta_percpu_disable, pp, true);
>   free_percpu_irq(dev->irq, pp->ports);
>   mvneta_cleanup_rxqs(pp);
> @@ -3612,6 +3612,7 @@ static int mvneta_probe(struct platform_device *pdev)
>   dev->ethtool_ops = _eth_tool_ops;
>  
>   pp = netdev_priv(dev);
> + spin_lock_init(>lock);
>   pp->phy_node = phy_node;
>   pp->phy_interface = phy_mode;
>  

Re: [PATCH v2 net 6/6] net: mvneta: Fix race condition during stopping

[Jisheng Zhang]

On Mon, 1 Feb 2016 14:07:47 +0100 Gregory CLEMENT wrote:

> When stopping the port, the CPU notifier are still there whereas the
> mvneta_stop_dev function calls mvneta_percpu_disable() on each CPUs.
> It was possible to have a new CPU coming at this point which could be
> racy.
> 
> This patch adds a flag preventing executing the code notifier for a new CPU
> when the port is stopping.
> 
> Signed-off-by: Gregory CLEMENT 
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> b/drivers/net/ethernet/marvell/mvneta.c
> index 4d40d2fde7ca..2f53975aa6ec 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -374,6 +374,7 @@ struct mvneta_port {
>* ensuring that the configuration remains coherent.
>*/
>   spinlock_t lock;
> + bool is_stopping;
>  
>   /* Core clock */
>   struct clk *clk;
> @@ -2916,6 +2917,11 @@ static int mvneta_percpu_notifier(struct 
> notifier_block *nfb,
>   switch (action) {
>   case CPU_ONLINE:
>   case CPU_ONLINE_FROZEN:
> + /* Configuring the driver for a new CPU while the
> +  * driver is stopping is racy, so just avoid it.
> +  */
> + if (pp->is_stopping)
> + break;

I still see race. What about another cpu set is_stopping at this point?

Thanks


>   netif_tx_stop_all_queues(pp->dev);
>  
>   /* We have to synchronise on tha napi of each CPU
> @@ -3054,9 +3060,17 @@ static int mvneta_stop(struct net_device *dev)
>  {
>   struct mvneta_port *pp = netdev_priv(dev);
>  
> + /* Inform that we are stopping so we don't want to setup the
> +  * driver for new CPUs in the notifiers
> +  */
> + pp->is_stopping = true;
>   mvneta_stop_dev(pp);
>   mvneta_mdio_remove(pp);
>   unregister_cpu_notifier(>cpu_notifier);
> + /* Now that the notifier are unregistered, we can clear the
> +  * flag
> +  */
> + pp->is_stopping = false;
>   on_each_cpu(mvneta_percpu_disable, pp, true);
>   free_percpu_irq(dev->irq, pp->ports);
>   mvneta_cleanup_rxqs(pp);