Re: [U-Boot] [Patch V7 04/18] net/fm/eth: Use mb() to be compatible for both ARM and PowerPC

[Gong Q . Y .]

> -Original Message-
> From: Wood Scott-B07421
> Sent: Friday, October 23, 2015 9:42 PM
> To: Hou Zhiqiang-B48286
> Cc: Gong Qianyu-B52263; u-boot@lists.denx.de; Hu Mingkai-B21284; Sun
> York-R58495; Xie Shaohui-B21989; Song Wenbin-B53747; Kushwaha Prabhakar-
> B32579; Wang Huan-B18965
> Subject: Re: [Patch V7 04/18] net/fm/eth: Use mb() to be compatible for
> both ARM and PowerPC
> 
> On Fri, 2015-10-23 at 06:01 -0500, Hou Zhiqiang-B48286 wrote:
> >
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: 2015年10月23日 7:11
> > > To: Gong Qianyu-B52263
> > > Cc: u-boot@lists.denx.de; Hu Mingkai-B21284; Sun York-R58495; Hou
> > > Zhiqiang-B48286; Xie Shaohui-B21989; Song Wenbin-B53747; Wood Scott-
> > > B07421; Kushwaha Prabhakar-B32579; Wang Huan-B18965
> > > Subject: Re: [Patch V7 04/18] net/fm/eth: Use mb() to be compatible
> > > for both ARM and PowerPC
> > >
> > > On Thu, 2015-10-22 at 18:46 +0800, Gong Qianyu wrote:
> > > > From: Shaohui Xie 
> > > >
> > > > Use mb() instead of sync() to be compatible for both ARM and
> PowerPC.
> > > >
> > > > Signed-off-by: Shaohui Xie 
> > > > Signed-off-by: Mingkai Hu 
> > > > Signed-off-by: Gong Qianyu 
> > > > ---
> > > > V7:
> > > >  - No change.
> > > > V6:
> > > >  - No change.
> > > > V5:
> > > >  - No change.
> > > > V4:
> > > >  - No change.
> > > > V3:
> > > >  - New patch. Separated from patch 'net: Move some header files to
> > > include/'
> > > >
> > > >  drivers/net/fm/eth.c | 14 +++---
> > > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/net/fm/eth.c b/drivers/net/fm/eth.c index
> > > > 368d554..ad02c66 100644
> > > > --- a/drivers/net/fm/eth.c
> > > > +++ b/drivers/net/fm/eth.c
> > > > @@ -371,7 +371,7 @@ static void
> > > > fmc_tx_port_graceful_stop_enable(struct
> > > > fm_eth *fm_eth)
> > > >   pram = fm_eth->tx_pram;
> > > >   /* graceful stop transmission of frames */
> > > >   setbits_be32(>mode, PRAM_MODE_GRACEFUL_STOP);
> > > > - sync();
> > > > + mb();
> > > >  }
> > > >
> > > >  static void fmc_tx_port_graceful_stop_disable(struct fm_eth
> > > > *fm_eth) @@ -381,7 +381,7 @@ static void
> > > > fmc_tx_port_graceful_stop_disable(struct
> > > > fm_eth *fm_eth)
> > > >   pram = fm_eth->tx_pram;
> > > >   /* re-enable transmission of frames */
> > > >   clrbits_be32(>mode, PRAM_MODE_GRACEFUL_STOP);
> > > > - sync();
> > > > + mb();
> > > >  }
> > >
> > > Why is it needed at all?  The I/O accessors should include the
> > > necessary barriers.
> >
> > The I/O accessors of powerpc does include the barrier, but it absents
> > from ARMs'.
> 
> Then fix ARM's accessors if they're missing barriers that are required.
> But first, consider whether the barrier is required on ARM.  What is
> special about FM that it needs it where all other drivers don't?
> 
> -Scott

I tested on both LS1043ARDB and T2080RDB board just now.
FM still works well without the barrier.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [Patch V7 04/18] net/fm/eth: Use mb() to be compatible for both ARM and PowerPC

[Hou Zhiqiang]

> -Original Message-
> From: Wood Scott-B07421
> Sent: 2015年10月23日 7:11
> To: Gong Qianyu-B52263
> Cc: u-boot@lists.denx.de; Hu Mingkai-B21284; Sun York-R58495; Hou
> Zhiqiang-B48286; Xie Shaohui-B21989; Song Wenbin-B53747; Wood Scott-
> B07421; Kushwaha Prabhakar-B32579; Wang Huan-B18965
> Subject: Re: [Patch V7 04/18] net/fm/eth: Use mb() to be compatible for
> both ARM and PowerPC
> 
> On Thu, 2015-10-22 at 18:46 +0800, Gong Qianyu wrote:
> > From: Shaohui Xie 
> >
> > Use mb() instead of sync() to be compatible for both ARM and PowerPC.
> >
> > Signed-off-by: Shaohui Xie 
> > Signed-off-by: Mingkai Hu 
> > Signed-off-by: Gong Qianyu 
> > ---
> > V7:
> >  - No change.
> > V6:
> >  - No change.
> > V5:
> >  - No change.
> > V4:
> >  - No change.
> > V3:
> >  - New patch. Separated from patch 'net: Move some header files to
> include/'
> >
> >  drivers/net/fm/eth.c | 14 +++---
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/fm/eth.c b/drivers/net/fm/eth.c index
> > 368d554..ad02c66 100644
> > --- a/drivers/net/fm/eth.c
> > +++ b/drivers/net/fm/eth.c
> > @@ -371,7 +371,7 @@ static void
> > fmc_tx_port_graceful_stop_enable(struct
> > fm_eth *fm_eth)
> >   pram = fm_eth->tx_pram;
> >   /* graceful stop transmission of frames */
> >   setbits_be32(>mode, PRAM_MODE_GRACEFUL_STOP);
> > - sync();
> > + mb();
> >  }
> >
> >  static void fmc_tx_port_graceful_stop_disable(struct fm_eth *fm_eth)
> > @@ -381,7 +381,7 @@ static void
> > fmc_tx_port_graceful_stop_disable(struct
> > fm_eth *fm_eth)
> >   pram = fm_eth->tx_pram;
> >   /* re-enable transmission of frames */
> >   clrbits_be32(>mode, PRAM_MODE_GRACEFUL_STOP);
> > - sync();
> > + mb();
> >  }
> 
> Why is it needed at all?  The I/O accessors should include the necessary
> barriers.

The I/O accessors of powerpc does include the barrier, but it absents from 
ARMs'.

Thanks,
Zhiqiang

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [Patch V7 04/18] net/fm/eth: Use mb() to be compatible for both ARM and PowerPC

[Scott Wood]

On Fri, 2015-10-23 at 06:01 -0500, Hou Zhiqiang-B48286 wrote:
> 
> > -Original Message-
> > From: Wood Scott-B07421
> > Sent: 2015年10月23日 7:11
> > To: Gong Qianyu-B52263
> > Cc: u-boot@lists.denx.de; Hu Mingkai-B21284; Sun York-R58495; Hou
> > Zhiqiang-B48286; Xie Shaohui-B21989; Song Wenbin-B53747; Wood Scott-
> > B07421; Kushwaha Prabhakar-B32579; Wang Huan-B18965
> > Subject: Re: [Patch V7 04/18] net/fm/eth: Use mb() to be compatible for
> > both ARM and PowerPC
> > 
> > On Thu, 2015-10-22 at 18:46 +0800, Gong Qianyu wrote:
> > > From: Shaohui Xie 
> > > 
> > > Use mb() instead of sync() to be compatible for both ARM and PowerPC.
> > > 
> > > Signed-off-by: Shaohui Xie 
> > > Signed-off-by: Mingkai Hu 
> > > Signed-off-by: Gong Qianyu 
> > > ---
> > > V7:
> > >  - No change.
> > > V6:
> > >  - No change.
> > > V5:
> > >  - No change.
> > > V4:
> > >  - No change.
> > > V3:
> > >  - New patch. Separated from patch 'net: Move some header files to
> > include/'
> > > 
> > >  drivers/net/fm/eth.c | 14 +++---
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/net/fm/eth.c b/drivers/net/fm/eth.c index
> > > 368d554..ad02c66 100644
> > > --- a/drivers/net/fm/eth.c
> > > +++ b/drivers/net/fm/eth.c
> > > @@ -371,7 +371,7 @@ static void
> > > fmc_tx_port_graceful_stop_enable(struct
> > > fm_eth *fm_eth)
> > >   pram = fm_eth->tx_pram;
> > >   /* graceful stop transmission of frames */
> > >   setbits_be32(>mode, PRAM_MODE_GRACEFUL_STOP);
> > > - sync();
> > > + mb();
> > >  }
> > > 
> > >  static void fmc_tx_port_graceful_stop_disable(struct fm_eth *fm_eth)
> > > @@ -381,7 +381,7 @@ static void
> > > fmc_tx_port_graceful_stop_disable(struct
> > > fm_eth *fm_eth)
> > >   pram = fm_eth->tx_pram;
> > >   /* re-enable transmission of frames */
> > >   clrbits_be32(>mode, PRAM_MODE_GRACEFUL_STOP);
> > > - sync();
> > > + mb();
> > >  }
> > 
> > Why is it needed at all?  The I/O accessors should include the necessary
> > barriers.
> 
> The I/O accessors of powerpc does include the barrier, but it absents from 
> ARMs'.

Then fix ARM's accessors if they're missing barriers that are required.  But 
first, consider whether the barrier is required on ARM.  What is special 
about FM that it needs it where all other drivers don't?

-Scott

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [Patch V7 04/18] net/fm/eth: Use mb() to be compatible for both ARM and PowerPC

[Gong Qianyu]

From: Shaohui Xie 

Use mb() instead of sync() to be compatible for both ARM and PowerPC.

Signed-off-by: Shaohui Xie 
Signed-off-by: Mingkai Hu 
Signed-off-by: Gong Qianyu 
---
V7:
 - No change.
V6:
 - No change.
V5:
 - No change.
V4:
 - No change.
V3:
 - New patch. Separated from patch 'net: Move some header files to include/'

 drivers/net/fm/eth.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/fm/eth.c b/drivers/net/fm/eth.c
index 368d554..ad02c66 100644
--- a/drivers/net/fm/eth.c
+++ b/drivers/net/fm/eth.c
@@ -371,7 +371,7 @@ static void fmc_tx_port_graceful_stop_enable(struct fm_eth 
*fm_eth)
pram = fm_eth->tx_pram;
/* graceful stop transmission of frames */
setbits_be32(>mode, PRAM_MODE_GRACEFUL_STOP);
-   sync();
+   mb();
 }
 
 static void fmc_tx_port_graceful_stop_disable(struct fm_eth *fm_eth)
@@ -381,7 +381,7 @@ static void fmc_tx_port_graceful_stop_disable(struct fm_eth 
*fm_eth)
pram = fm_eth->tx_pram;
/* re-enable transmission of frames */
clrbits_be32(>mode, PRAM_MODE_GRACEFUL_STOP);
-   sync();
+   mb();
 }
 
 static int fm_eth_open(struct eth_device *dev, bd_t *bd)
@@ -483,9 +483,9 @@ static int fm_eth_send(struct eth_device *dev, void *buf, 
int len)
muram_writew(>buf_ptr_hi, 0);
out_be32(>buf_ptr_lo, (u32)buf);
muram_writew(>len, len);
-   sync();
+   mb();
muram_writew(>status, TxBD_READY | TxBD_LAST);
-   sync();
+   mb();
 
/* update TxQD, let RISC to send the packet */
offset_in = muram_readw(>txqd.offset_in);
@@ -493,7 +493,7 @@ static int fm_eth_send(struct eth_device *dev, void *buf, 
int len)
if (offset_in >= muram_readw(>txqd.bd_ring_size))
offset_in = 0;
muram_writew(>txqd.offset_in, offset_in);
-   sync();
+   mb();
 
/* wait for buffer to be transmitted */
for (i = 0; muram_readw(>status) & TxBD_READY; i++) {
@@ -544,7 +544,7 @@ static int fm_eth_recv(struct eth_device *dev)
/* clear the RxBDs */
muram_writew(>status, RxBD_EMPTY);
muram_writew(>len, 0);
-   sync();
+   mb();
 
/* advance RxBD */
rxbd++;
@@ -560,7 +560,7 @@ static int fm_eth_recv(struct eth_device *dev)
if (offset_out >= muram_readw(>rxqd.bd_ring_size))
offset_out = 0;
muram_writew(>rxqd.offset_out, offset_out);
-   sync();
+   mb();
}
fm_eth->cur_rxbd = (void *)rxbd;
 
-- 
2.1.0.27.g96db324

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [Patch V7 04/18] net/fm/eth: Use mb() to be compatible for both ARM and PowerPC

[Scott Wood]

On Thu, 2015-10-22 at 18:46 +0800, Gong Qianyu wrote:
> From: Shaohui Xie 
> 
> Use mb() instead of sync() to be compatible for both ARM and PowerPC.
> 
> Signed-off-by: Shaohui Xie 
> Signed-off-by: Mingkai Hu 
> Signed-off-by: Gong Qianyu 
> ---
> V7:
>  - No change.
> V6:
>  - No change.
> V5:
>  - No change.
> V4:
>  - No change.
> V3:
>  - New patch. Separated from patch 'net: Move some header files to include/'
> 
>  drivers/net/fm/eth.c | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/fm/eth.c b/drivers/net/fm/eth.c
> index 368d554..ad02c66 100644
> --- a/drivers/net/fm/eth.c
> +++ b/drivers/net/fm/eth.c
> @@ -371,7 +371,7 @@ static void fmc_tx_port_graceful_stop_enable(struct 
> fm_eth *fm_eth)
>   pram = fm_eth->tx_pram;
>   /* graceful stop transmission of frames */
>   setbits_be32(>mode, PRAM_MODE_GRACEFUL_STOP);
> - sync();
> + mb();
>  }
>  
>  static void fmc_tx_port_graceful_stop_disable(struct fm_eth *fm_eth)
> @@ -381,7 +381,7 @@ static void fmc_tx_port_graceful_stop_disable(struct 
> fm_eth *fm_eth)
>   pram = fm_eth->tx_pram;
>   /* re-enable transmission of frames */
>   clrbits_be32(>mode, PRAM_MODE_GRACEFUL_STOP);
> - sync();
> + mb();
>  }

Why is it needed at all?  The I/O accessors should include the necessary 
barriers.

-Scott

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot