Message-ID: <8ace674c-9d57-4500-b155-caa662903774@ti.com>
Date: Fri, 17 Oct 2025 16:28:15 +0530
From: Harikrishna shenoy <h-shenoy@...com>
To: Krzysztof Kozlowski <krzk@...nel.org>
CC: <andrzej.hajda@...el.com>, <neil.armstrong@...aro.org>, <rfoss@...nel.org>,
        <Laurent.pinchart@...asonboard.com>, <jonas@...boo.se>,
        <jernej.skrabec@...il.com>, <airlied@...il.com>, <simona@...ll.ch>,
        <maarten.lankhorst@...ux.intel.com>, <mripard@...nel.org>,
        <tzimmermann@...e.de>, <robh@...nel.org>, <krzk+dt@...nel.org>,
        <conor+dt@...nel.org>, <sjakhade@...ence.com>, <yamonkar@...ence.com>,
        <lumag@...nel.org>, <dianders@...omium.org>, <jani.nikula@...el.com>,
        <luca.ceresoli@...tlin.com>, <andy.yan@...k-chips.com>,
        <dri-devel@...ts.freedesktop.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <devarsht@...com>, <u-kumar1@...com>,
        <s-jain1@...com>, <tomi.valkeinen@...asonboard.com>
Subject: Re: [PATCH v5 1/2] dt-bindings: drm/bridge: MHDP8546 bridge binding
 changes for DSC



On 22/09/25 13:11, Harikrishna Shenoy wrote:
> 
> 
> On 9/18/25 07:00, Krzysztof Kozlowski wrote:
>> On Mon, Sep 15, 2025 at 04:00:40PM +0530, Harikrishna Shenoy wrote:
>>> From: Swapnil Jakhade<sjakhade@...ence.com>
>>>
>>> Add binding changes for DSC(Display Stream Compression) in the MHDP8546
>>> DPI/DP bridge.
>>>
>>> Signed-off-by: Swapnil Jakhade<sjakhade@...ence.com>
>>> Signed-off-by: Harikrishna Shenoy<h-shenoy@...com>
>>> ---
>>>   .../display/bridge/cdns,mhdp8546.yaml         | 24 ++++++++++++-------
>>>   1 file changed, 15 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
>>> index c2b369456e4e..2a05a7d5847f 100644
>>> --- a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
>>> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
>>> @@ -27,13 +27,12 @@ properties:
>>>             Register block for DSS_EDP0_INTG_CFG_VP registers in case of TI J7 SoCs.
>>>         - description:
>>>             Register block of mhdptx sapb registers.
>>> +      - description:
>>> +          Register block for mhdptx DSC encoder registers.
>>>   
>>>     reg-names:
>>> -    minItems: 1
>>> -    items:
>>> -      - const: mhdptx
>>> -      - const: j721e-intg
>>> -      - const: mhdptx-sapb
>>> +    description:
>>> +      Names corresponding to entries in the reg property.
>> No, top-level should have broadest constraints. In your case it is
>> min/maxItems.
>>
>> Description is completely redundant. Wasn't here before, so why adding
>> it?
>>
> Noted, will remove description and add minItems:1.
>>>   
>>>     clocks:
>>>       maxItems: 1
>>> @@ -100,18 +99,25 @@ allOf:
>>>         properties:
>>>           reg:
>>>             minItems: 2
>>> -          maxItems: 3
>>> +          maxItems: 4
>>>           reg-names:
>>>             minItems: 2
>>> -          maxItems: 3
>>> +          items:
>>> +            - const: mhdptx
>>> +            - const: j721e-intg
>>> +            - const: mhdptx-sapb
>>> +            - const: dsc
>>>       else:
>>>         properties:
>>>           reg:
>>>             minItems: 1
>>> -          maxItems: 2
>>> +          maxItems: 3
>>>           reg-names:
>>>             minItems: 1
>>> -          maxItems: 2
>>> +          items:
>>> +            - const: mhdptx
>>> +            - const: mhdptx-sapb
>> This is wrong. Previously CDNS variant had two items means it had
>> "j721e-intg". Now it's something else.
>>
>> First, this is an ABI break.
>>
>> Second, there is no explanation at all for it in the commit msg! Looks
>> like random change.
>>
>> Read carefully writing-bindings doc.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Hi Krzysztof,
> 
> 
> Keeping this patch series aside, The existing binding-docs clearly have 
> a bug.
> 
> Since even for cadence specific compatible "cdns,mhdp8546" it 
> compulsorily expects "j721e-intg" register space
> 
> which is NOT part of the cadence IP block mhdp8546 and hence not 
> applicable to "cdns,mhdp8546".
> 
> This was also discussed here [1] and can also be referred in this TRM 
> section [2],
> 
> which clearly show that "j721e-intg" is part of TI wrapper IP block and 
> should be
> 
> applicable to "ti,j721e-mhdp8546" compatible.
> 
> Yes agreed it breaks the ABI but it also fixes a bug and I don't see any 
> one using only "cdns,mhdp8546" yet.
> 
> so I am thinking it's more appropriate to fix this as a separate patch 
> independent of this series.
> 
> Kindly let me know if I should submit a separate patch to fix this bug 
> or I should just ignore this bug.
> 
> Depending on your suggestion, if it's agreed upon to send the bug fix 
> patch first, I will send out an independent
> 
> bug fix to remove "j721e-intg" for compatible "cdns,mhdp8546" and then 
> rebase the series for adding DSC reg blocks
> 
> on top of bug fix.
> 
> [1]: https://lore.kernel.org/all/20250903220312.GA2903503- 
> robh@...nel.org/ <https://lore.kernel.org/all/20250903220312.GA2903503- 
> robh@...nel.org/>
> [2]: Link to TRM ZIP:https://www.ti.com/lit/zip/spruil1 <https:// 
> www.ti.com/lit/zip/spruil1>
> Table 2-1. MAIN Domain Memory Map
> DSS_EDP0_V2A_CORE_VP_REGS_APB are EDP core register identified by name 
> mhdptx in DT.
> DSS_EDP0_INTG_CFG_VP identified by j721e-intg in DT.
> 
> Section 12.6.6.16.4: EDP_CFG Registers
> 
> Driver use: TI j721e Cadence MHDP8546 DP wrapper(drivers/gpu/drm/bridge/ 
> cadence/cdns-mhdp8546-j721e.c)
> 
> 
> Regards.

Hi Krzysztof,

Could you please let us know your thoughts on above, will re-spin it 
accordingly.

Regards.

> 

Powered by blists - more mailing lists