On Wed, May 06, 2020 at 01:48:45PM +0100, Will Deacon wrote:
> On Wed, May 06, 2020 at 02:02:37PM +0200, Greg KH wrote:
> > On Wed, May 06, 2020 at 05:03:31PM +0530, Mukesh, Savaliya wrote:
> > > +static void msm_geni_serial_wr_char(struct uart_port *uport, int ch)
> > > +{
> > > + writel_relaxed(ch, uport->membase+SE_GENI_TX_FIFOn);
> > > + /*
> > > + * Ensure FIFO write clear goes through before
> > > + * next iteration.
> > > + */
> > > + mb();
> >
> > Can't you just write the above two lines as:
> > writel(ch, uport->membase+SE_GENI_TX_FIFOn);
> > ?
> >
> > Why put a mb() after a _relaxed() call?
>
> writel() usually puts the barrier /before/ the I/O write, since it's
> normally used to signal the readiness of a DMA buffer, e.g.:
>
> ptr = dma_map(...);
> ptr->data = tx_data;
> writel(dev->ctrl_reg, SEND_DATA); // Device must see tx_data
>
> but this driver looks like it only cares about PIO rather than DMA, in which
> case there's no need for mb() or writel(); writel_relaxed() should do the
> trick because we just need to ensure ordering of the writes hitting the
> device. From memory-barriers.txt:
>
> ... they [relaxed accesses] are still guaranteed to be ordered with
> respect to other accesses from the same CPU thread to the same
> peripheral when operating on __iomem pointers mapped with the default
> I/O attributes.
Ok, that makes more sense, many thanks.
So, as writes are ordered here, Savaliya, I think all of the calls to
mb() can be dropped from this driver, right?
thanks,
greg k-h
