Re: [PATCH v5 2/8] dt-bindings: phy: rockchip,rk3588-mipi-dcphy: Add a cell to select TX or RX

[Michael Riesch <michael.riesch@collabora.com>]

Hi Jason,

On 10/21/26 10:03, Heiko Stübner wrote:
> Am Mittwoch, 21. Oktober 2026, 04:42:39 Mitteleuropäische Sommerzeit schrieb Rob Herring:
>> On Tue, Oct 20, 2026 at 04:57:34PM +0200, Heiko Stübner wrote:
>>> Am Dienstag, 20. Oktober 2026, 16:10:39 Mitteleuropäische Sommerzeit schrieb Rob Herring:
>>>> On Tue, Oct 20, 2026 at 03:37:35PM +0200, Heiko Stübner wrote:
>>>>> Am Dienstag, 20. Oktober 2026, 00:20:47 Mitteleuropäische Sommerzeit schrieb Michael Riesch:
>>>>>> Hi all,
>>>>>>
>>>>>> On 10/19/26 17:06, Rob Herring wrote:
>>>>>>> On Mon, Oct 19, 2026 at 04:15:03PM +0800, 楊智成 wrote:
>>>>>>>> Hi Krzysztof, Rob,
>>>>>>>>
>>>>>>>>> Existing OS should ignore surplus cell if the driver is coded properly.
>>>>>>>>> Same as adding new properties or new front-compatible.  The question is
>>>>>>>>> how the existing driver behaves?
>>>>>>>>
>>>>>>>> As you pointed out on 5/8, the xlate checks args_count != 1. The DC-PHY
>>>>>>>> driver was merged in v6.15, so every kernel since 6.15 fails on a second cell.
>>>>>>>>
>>>>>>>> 1. Keep the second cell. Add the patch as follows:
>>>>>>>>
>>>>>>>> - if (args->args_count != 1) {
>>>>>>>> + if (args->args_count < 1) {
>>>>>>>>     dev_err(dev, "invalid number of arguments\n");
>>>>>>>>     return ERR_PTR(-EINVAL);
>>>>>>>>   }
>>>>>>>>
>>>>>>>> + if (args->args_count > 1 && args->args[1]) {
>>>>>>>> +   dev_err(dev, "only the transmitter is supported\n");
>>>>>>>> +   return ERR_PTR(-EINVAL);
>>>>>>>> + }
>>>>>>>>
>>>>>>>>   With Fixes: and Cc: stable, the stable kernels maintained at that
>>>>>>>>   time (currently 6.18.y and 7.2.y) keep DSI working with the new
>>>>>>>>   dts, and the receiver fails cleanly instead of getting the transmitter.
>>>>>>>>   But EOL kernels (6.15, 6.16, 6.17, 6.19, 7.0, 7.1) stay affected.
>>>>>>>>   It goes in with the series, since two-cell dts only exist after it.
>>>>>>>>
>>>>>>>> 2. Use a flag in the upper bits of the existing cell, as Rob
>>>>>>>>     suggested. Existing kernels are not affected, unless a board enables
>>>>>>>>     the DC-PHY CSI-2 receiver on 7.0 or later without this series.
>>>>>>>>
>>>>>>>> I will write the impact clearly whichever  approach is selected in
>>>>>>>> the related commit messages and the cover letter.
>>>>>>>>
>>>>>>>> I think the cell approach is easier to read and more symmetric in the
>>>>>>>> long term,
>>>>>>>> but it breaks more. Which one do you think is better? Thanks.
>>>>>>>
>>>>>>> I'm not a user of this platform, so I don't care. Up to the platform 
>>>>>>> maintainers to decide if breakage is acceptible. You just need to 
>>>>>>> document the issue in the commit message.
>>>>>>>
>>>>>>> Rob
>>>>>>
>>>>>> It's a bit of a pickle, all this.
>>>>>>
>>>>>> I gave it some more thought. In the end this is an IP block with two
>>>>>> PHYs. No more, no less. One cell is sufficient to address those, and it
>>>>>> could be 0 for the transmitting PHY (DSI, display) and 1 for the
>>>>>> receiving PHY (CSI, camera).
>>>>>>
>>>>>> Of course the two PHYs can operate in either DPHY or CPHY mode. The
>>>>>> phy-cells have been (mis?)used to select the proper mode of combo PHYs,
>>>>>> and this principle has been applied here too.
>>>>>>
>>>>>> However, we have the special situation here that the mode can only be
>>>>>> changed for the complete IP block (and thus for both PHYs at the same
>>>>>> time). As Bryan points out, we need to check and complain if the PHY
>>>>>> mode is not set to the same value for both PHYs.
>>>>>>
>>>>>> I suggest using a property
>>>>>>     samsung,cphy-mode;
>>>>>
>>>>> I guess, a maybe nicer way would be mimicing
>>>>> 	Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
>>>>>
>>>>> with its
>>>>> 	intel,phy-mode = <x>
>>>>>
>>>>> with x coming from include/dt-bindings/phy/phy.h
>>>>> for the distinction between cphy and dphy, so we don't introduce yet
>>>>> another way to describe this.
>>>>
>>>> We already have the plain 'phy-type' property which takes the same 
>>>> PHY_MODE_x values. We don't need yet another vendor specific property.
>>>
>>> even better then ... and I didn't look deep enough it seems.
>>>
>>>
>>>>>> (name to be bikeshedded) that sets the complete IP block into CPHY mode
>>>>>> (DPHY being the default) to distinguish between the modes. Since both
>>>>>> modes use a different electrical interface, this is clearly a hardware
>>>>>> thing and thus suitable for device tree.
>>>>>>
>>>>>> Then, we can address the two PHYs with the help of the already existing
>>>>>> phy cell (without any bitflag tweaks to the value).
>>>>>>
>>>>>> The sole drawback is that we need to accept the PHY_TYPE_DPHY and
>>>>>> PHY_TYPE_CPHY values that some OS in the field might pass. (For Linux,
>>>>>> it should only be the former.) And we need to handle those values
>>>>>> correctly. But I think this is not more painful than the other
>>>>>> approaches. At the same time, this design seems to be the most correct
>>>>>
>>>>> The current cell values are 10 + 11, so as long as we don't re-use those
>>>>> exact values for the other constants the driver should somewhat easily
>>>>> be able to recognize an old DT?
>>>>
>>>> I think you will still have an ABI issue of what does old OS do with new 
>>>> values?
>>>
>>> wasn't the main contract, a newer kernel should support old DTs ?
>>
>> Well, yes, it was. That was a position I inherited. I hear complaints 
>> about the opposite though.
>>
>>> Aka the DT is parked in some firmware part (u-boot?) and gets handed
>>> to the newer kernel.
>>>
>>> I don't see deliberately handing a newer DTB to an older kernel as a
>>> very common occurence?
>>
>> Do you want things to break when you do a firmware update that's picked 
>> up a newer DT? I don't.
>>
>>
>>> And from a practical standpoint:
>>>
>>> We're talking about devices with DSI displays that are at stake here,
>>> so we're in the embedded world.
>>>
>>> In other areas we're still fighting basic functionality like actually
>>> handling the type-c orientation correctly in the usbphy.
>>>
>>> So I really don't see a device with a user-updateable mainline kernel out
>>> in the field, apart from people hacking around on their devices to try
>>> out new features and new kernels, simply because core functionality
>>> is still evolving in mainline.
>>>
>>>
>>> RK3576/RK3588 devices still ship with a vendor u-boot/kernel, with
>>> completely incompatible DTB afterall.
>>
>> I can only keep my own house clean...
> 
> ok :-) 
> 
> 
>> I only ask that compatibility issues be documented. Up to individual 
>> platforms to decide if they care or not. So, your decision.
> 
> The discussion would be a lot different for SoCs like the RK3399 or PX30
> that can and have run mainline software in a usable way for years now.
> RK3568 very likely falls in that area too.
> 
> But for the in-progress SoCs RK3576 and RK3588 anybody wanting to run
> a generic userspace would need a heavily patched kernel anyway, with
> other unreviewed/unmerged dt-changes, so personally I can live with it.
> 
> Especially if we keep the (in my eyes) more common case
> (old dtb, new kernel) working.
> 
> 
> Heiko
> 
> 

(when) could you post a new iteration of your series that implements the
suggestion above?

Thanks and best regards,
Michael