Re: [PATCH V2] serial: msm_geni_serial_console : Add Earlycon support

["Mukesh, Savaliya" <msavaliy@codeaurora.org>]

On 5/6/2020 6:29 PM, Greg KH wrote:
> 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?
yes, makes sense. I have removed it, kept *_relaxed() calls and uploaded 
the patch.
>
> thanks,
>
> greg k-h