[prev in list] [next in list] [prev in thread] [next in thread] 

List:       dri-devel
Subject:    Re: [PATCH] drm: mxsfb: Clear FIFO_CLEAR bit
From:       Marek Vasut <marex () denx ! de>
Date:       2021-06-30 22:50:03
Message-ID: 6ec45690-e85b-b267-b189-ee54de673692 () denx ! de
[Download RAW message or body]

On 6/29/21 10:02 AM, Lucas Stach wrote:
> Am Dienstag, dem 29.06.2021 um 05:04 +0200 schrieb Marek Vasut:
>> On 6/28/21 10:09 AM, Lucas Stach wrote:
>>> Am Samstag, dem 26.06.2021 um 20:15 +0200 schrieb Marek Vasut:
>>>> On 6/24/21 2:01 PM, Lucas Stach wrote:
>>>>> Am Dienstag, dem 22.06.2021 um 11:33 +0200 schrieb Marek Vasut:
>>>>>> On 6/22/21 9:28 AM, Lucas Stach wrote:
>>>>>>> Am Montag, dem 21.06.2021 um 18:30 +0200 schrieb Marek Vasut:
>>>>>>>> On 6/21/21 2:14 PM, Lucas Stach wrote:
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>>>>>>>>>> index 98d8ba0bae84..22cb749fc9bc 100644
>>>>>>>>>> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>>>>>>>>>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>>>>>>>>>> @@ -241,6 +241,9 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
>>>>>>>>>>       
>>>>>>>>>>       	/* Clear the FIFOs */
>>>>>>>>>>       	writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_SET);
>>>>>>>>>> +	readl(mxsfb->base + LCDC_CTRL1);
>>>>>>>>>
>>>>>>>>> Do you really need those readbacks? As both writes are targeting the
>>>>>>>>> same slave interface, the memory barrier in the clear write should push
>>>>>>>>> the set write.
>>>>>>>>
>>>>>>>> What would push the clear write then ? We can drop one of the readl()s,
>>>>>>>> but not the last one.
>>>>>>>
>>>>>>> There are a lot of more writes with barriers to the controller slave
>>>>>>> interface in that function after clearing the FIFO. I don't see why
>>>>>>> this readback would be required.
>>>>>>
>>>>>> Because you really do want to make sure the fifo is cleared before you
>>>>>> start doing any of those other writes or configuring the controller in
>>>>>> any way.
>>>>>
>>>>> I still don't see the reason. What additional properties do you think
>>>>> the readback provides that isn't already provided by the barriers in
>>>>> the following writes?
>>>>
>>>> See the paragraph above -- we have to make sure the writes that trigger
>>>> the FIFO clearing really take place before any other writes do.
>>>
>>> And they do, as there are write barriers prepended to the writes
>>> following the FIFO clear. The readback just lets the CPU wait until the
>>> write reached the peripheral, which I don't see a reason to do here.
>>> The ordering of the writes from the perspective of the peripheral is
>>> completely the same with or without the readback. The later writes can
>>> not overtake the FIFO clear writes due to the barriers.
>>>
>>> I'm strongly against adding stuff because it "might have an effect", if
>>> it isn't required by architectural rules. It clutters the code and some
>>> months/years down the line nobody dares to cleanup/remove this stuff
>>> anymore, because everyone assumes that there was a good reason for
>>> adding those things.
>>
>> Since there is no RTL for any of the iMXes or their IPs, how do you
>> propose anyone except NXP can validate what is and what is not required ?
>>
>> This patch helps with a problem where I sporadically observe shifted
>> image on boot on mx8mm.
> 
> The order of writes to a device mapped region are defined by the ARM
> architecture and the AMBA bus standard, not the peripheral. I'm not
> saying this patch isn't needed. I'm saying the readbacks look bogus.
> 
> Have you checked that just adding the write to the REG_CLR doesn't fix
> your issue?

No, it does not help with the issue.
[prev in list] [next in list] [prev in thread] [next in thread]