[PATCH 2/4] drm/rcar-du: dsi: Remove fixed PPI lane count setup

[Tomi Valkeinen]

Hi,

On 14/08/2025 00:06, Marek Vasut wrote:
> On 8/13/25 9:34 AM, Tomi Valkeinen wrote:
> 
> Hi,
> 
>>>>>> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/
>>>>>> drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>>>>> index b3e57217ae63..cefa7e92b5b8 100644
>>>>>> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>>>>> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
>>>>>> @@ -80,10 +80,7 @@
>>>>>>     * PHY-Protocol Interface (PPI) Registers
>>>>>>     */
>>>>>>    #define PPISETR                0x700
>>>>>> -#define PPISETR_DLEN_0            (0x1 << 0)
>>>>>> -#define PPISETR_DLEN_1            (0x3 << 0)
>>>>>> -#define PPISETR_DLEN_2            (0x7 << 0)
>>>>>> -#define PPISETR_DLEN_3            (0xf << 0)
>>>>>> +#define PPISETR_DLEN_MASK        (0xf << 0)
>>>>>>    #define PPISETR_CLEN            BIT(8)
>>>>>
>>>>> Looks fine, but do you know what the TXSETR register does? It also has
>>>>> LANECNT, but I don't see the driver touching that register at all.
>>>>> TXSETR:LANECNT default value is 3 (4 lanes), which matches with the
>>>>> old
>>>>> hardcoded behavior for PPISETR... So I wonder if that register should
>>>>> also be set?
>>>>
>>>> Ah, never mind, I now saw the patch 3 =). But should it be before patch
>>>> 2? Hmm, I guess that ordering is no better. Should they be combined
>>>> into
>>>> "support 1,2,3 datalanes" patch?
>>> I think each patch fixes slighly different issue, even if the issues are
>>> related. I tried to keep the issue description in each patch commit
>>> message for posterity. I can squash them if you think that's better, I
>>> don't mind either way.
>>
>> I was thinking about this the user's or backporting point of view.
>> Neither of the commits (clearly) say that they add support for 1/2/3
>> lane modes.
> 
> The 1/2/3 lane mode was already implemented in the driver, except it was
> broken.

If it never worked, was it broken or not implemented? How much code the
original driver must have for the feature to have the feature
"implemented, just broken"? If it reads the num lanes from the DT, and
allows the driver to probe with 1-4 lanes, is it then "implemented, but
broken"? Or does the driver have to have a clear intent on having the
feature (even if it doesn't work) for it to be implemented?

I'm not trying to be annoying here, and I'm fine with the new patch you
sent. I bring this topic up as I just had a similar discussion in a
thread for another patch series, and the answer is not clear to me.

stable-kernel-rules.rst doesn't really cover this case, so, afaics,
someone could argue that this (well, the new one you sent) is not valid
stable patch: it doesn't fix a crash/hang/etc, it adds support for more
lane setups.

In this particular case I'm fine calling this a fix, and backporting to
stable, as the patch is such a small one and "obviously correct" (as
stable-kernel-rules.rst says) because with the only working lane setup,
4-lanes, we can easily see that the values written to registers are
identical to default/old values.

Still, if someone has feedback on how to approach this question, I
wouldn't mind hearing the thoughts =).

>> You say they "fix", but they're not quite fixes either. The
>> patch 3 could be considered a fix, but at the moment it just writes the
>> default value to the register, so no point in marking it as a fix to be
>> backported.
> 
> 3/4 does write the DSI lane count into TXSETR , not the default value.

I meant that as only the 4-lane mode works, I must assume all the users
use 4-lane mode. Thus with patch 3, a value identical to the default
value gets written, as everyone uses 4 lanes. So, if it's backported to
kernels where everyone uses 4 lanes, it won't change anything.

 Tomi