[PATCH] drm: mxsfb: Clear FIFO_CLEAR bit

[PATCH] drm: mxsfb: Clear FIFO_CLEAR bit

[Marek Vasut]

Make sure the FIFO_CLEAR bit is latched in when configuring the
controller, so that the FIFO is really cleared. And then clear
the FIFO_CLEAR bit, since it is not self-clearing.

Fixes: 45d59d704080 ("drm: Add new driver for MXSFB controller")
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Daniel Abrecht <public@danielabrecht.ch>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/mxsfb/mxsfb_kms.c | 3 +++
 1 file changed, 3 insertions(+)

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);
+	writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_CLR);
+	readl(mxsfb->base + LCDC_CTRL1);
 
 	if (mxsfb->devdata->has_overlay)
 		writel(0, mxsfb->base + LCDC_AS_CTRL);
-- 
2.30.2

Re: [PATCH] drm: mxsfb: Clear FIFO_CLEAR bit

[Lucas Stach]

Am Montag, dem 21.06.2021 um 00:49 +0200 schrieb Marek Vasut:
> Make sure the FIFO_CLEAR bit is latched in when configuring the
> controller, so that the FIFO is really cleared. And then clear
> the FIFO_CLEAR bit, since it is not self-clearing.
> 
> Fixes: 45d59d704080 ("drm: Add new driver for MXSFB controller")
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Daniel Abrecht <public@danielabrecht.ch>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/gpu/drm/mxsfb/mxsfb_kms.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> 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.

> +	writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_CLR);
> +	readl(mxsfb->base + LCDC_CTRL1);
>  
>  	if (mxsfb->devdata->has_overlay)
>  		writel(0, mxsfb->base + LCDC_AS_CTRL);

Re: [PATCH] drm: mxsfb: Clear FIFO_CLEAR bit

[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.

Re: [PATCH] drm: mxsfb: Clear FIFO_CLEAR bit

[Lucas Stach]

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.

Regards,
Lucas

Re: [PATCH] drm: mxsfb: Clear FIFO_CLEAR bit

[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.

Re: [PATCH] drm: mxsfb: Clear FIFO_CLEAR bit

[Jagan Teki]

On Mon, Jun 21, 2021 at 4:19 AM Marek Vasut <marex@denx.de> wrote:
>
> Make sure the FIFO_CLEAR bit is latched in when configuring the
> controller, so that the FIFO is really cleared. And then clear
> the FIFO_CLEAR bit, since it is not self-clearing.
>
> Fixes: 45d59d704080 ("drm: Add new driver for MXSFB controller")
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Daniel Abrecht <public@danielabrecht.ch>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Stefan Agner <stefan@agner.ch>
> ---

Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
Tested-by: Jagan Teki <jagan@amarulasolutions.com> # i.Core MX8MM

Re: [PATCH] drm: mxsfb: Clear FIFO_CLEAR bit

[Lucas Stach]

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?

Regards,
Lucas

Re: [PATCH] drm: mxsfb: Clear FIFO_CLEAR bit

[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.

Re: [PATCH] drm: mxsfb: Clear FIFO_CLEAR bit

[Lucas Stach]

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.

Regards,
Lucas

Re: [PATCH] drm: mxsfb: Clear FIFO_CLEAR bit

[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.

Re: [PATCH] drm: mxsfb: Clear FIFO_CLEAR bit

[Lucas Stach]

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?

Regards,
Lucas

Re: [PATCH] drm: mxsfb: Clear FIFO_CLEAR bit

[Marek Vasut]

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.

Re: [PATCH] drm: mxsfb: Clear FIFO_CLEAR bit

[Lucas Stach]

Am Donnerstag, dem 01.07.2021 um 00:50 +0200 schrieb Marek Vasut:
> 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.

Okay, i don't want to hold up this patch over technicalities if it
fixes the issue, in which case the readbacks probably provide just the
right amount of delay for the FIFO clear to happen in hardware. FWIW:

Acked-by: Lucas Stach <l.stach@pengutronix.de>

Regards,
Lucas