diff mbox series

[v2,1/2] dt-bindings: drm/bridge: MHDP8546 bridge binding changes for HDCP

Message ID 1614597746-4563-1-git-send-email-pthombar@cadence.com (mailing list archive)
State New, archived
Headers show
Series enable HDCP in Cadence MHDP bridge driver | expand

Commit Message

Parshuram Thombare March 1, 2021, 11:22 a.m. UTC
Add binding changes for HDCP in the MHDP8546 DPI/DP bridge binding.
This binding is not used in any upstreamed DTS yet, so changing
index of property 'j721e-intg' should not affect anything.

Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
---
 .../display/bridge/cdns,mhdp8546.yaml         | 29 ++++++++++++-------
 1 file changed, 19 insertions(+), 10 deletions(-)

Comments

Laurent Pinchart March 1, 2021, 3:41 p.m. UTC | #1
Hi Parshuram,

Thank you for the patch.

On Mon, Mar 01, 2021 at 12:22:26PM +0100, Parshuram Thombare wrote:
> Add binding changes for HDCP in the MHDP8546 DPI/DP bridge binding.
> This binding is not used in any upstreamed DTS yet, so changing
> index of property 'j721e-intg' should not affect anything.
> 
> Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
> ---
>  .../display/bridge/cdns,mhdp8546.yaml         | 29 ++++++++++++-------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
> index 63427878715e..5fdadadaac16 100644
> --- a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
> @@ -17,21 +17,24 @@ properties:
>        - ti,j721e-mhdp8546
>  
>    reg:
> -    minItems: 1
> -    maxItems: 2
> +    minItems: 2
> +    maxItems: 3
>      items:
>        - description:
>            Register block of mhdptx apb registers up to PHY mapped area (AUX_CONFIG_P).
>            The AUX and PMA registers are not part of this range, they are instead
>            included in the associated PHY.
> +      - description:
> +          Register block of mhdptx sapb registers.
>        - description:
>            Register block for DSS_EDP0_INTG_CFG_VP registers in case of TI J7 SoCs.
>  
>    reg-names:
> -    minItems: 1
> -    maxItems: 2
> +    minItems: 2
> +    maxItems: 3
>      items:
>        - const: mhdptx
> +      - const: mhdptx-sapb
>        - const: j721e-intg
>  
>    clocks:
> @@ -53,6 +56,11 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  hdcp-config:
> +    maxItems: 1
> +    description:
> +      HDCP version supported. Bit [0]:HDCP2.2 [1]:HDCP1.4.
> +

Is this a property of the hardware, that is, are there multiple versions
of this IP core covered by the same compatible string that support HDCP
1.4 only, DHCP 2.2 only or both ? Or is it a way to select what a given
system will offer ?

>    interrupts:
>      maxItems: 1
>  
> @@ -98,15 +106,15 @@ allOf:
>      then:
>        properties:
>          reg:
> -          minItems: 2
> +          minItems: 3
>          reg-names:
> -          minItems: 2
> +          minItems: 3
>      else:
>        properties:
>          reg:
> -          maxItems: 1
> +          maxItems: 2
>          reg-names:
> -          maxItems: 1
> +          maxItems: 2
>  
>  required:
>    - compatible
> @@ -129,8 +137,9 @@ examples:
>  
>          mhdp: dp-bridge@f0fb000000 {
>              compatible = "cdns,mhdp8546";
> -            reg = <0xf0 0xfb000000 0x0 0x1000000>;
> -            reg-names = "mhdptx";
> +            reg = <0xf0 0xfb000000 0x0 0x1000000>,
> +                  <0x0 0x4f48000 0x0 0x74>;
> +            reg-names = "mhdptx", "mhdptx-sapb";
>              clocks = <&mhdp_clock>;
>              phys = <&dp_phy>;
>              phy-names = "dpphy";
Parshuram Thombare March 1, 2021, 4:14 p.m. UTC | #2
Hi Laurent,

>Is this a property of the hardware, that is, are there multiple versions
>of this IP core covered by the same compatible string that support HDCP
>1.4 only, DHCP 2.2 only or both ? Or is it a way to select what a given
>system will offer ?[] 

MHDP hardware supports both HDCP 2.2 and 1.4. So, this is a way
to select the version of HDCP, system wish to support.

Regards,
Parshuram Thombare
Laurent Pinchart March 1, 2021, 4:27 p.m. UTC | #3
Hi Parshuram,

On Mon, Mar 01, 2021 at 04:14:54PM +0000, Parshuram Raju Thombare wrote:
> Hi Laurent,
> 
> > Is this a property of the hardware, that is, are there multiple versions
> > of this IP core covered by the same compatible string that support HDCP
> > 1.4 only, DHCP 2.2 only or both ? Or is it a way to select what a given
> > system will offer ?[] 
> 
> MHDP hardware supports both HDCP 2.2 and 1.4. So, this is a way
> to select the version of HDCP, system wish to support.

Then I'm not sure this qualifies as a DT property, which should describe
the system, not configure it. A way for userspace to configure this
would be better.
Parshuram Thombare March 2, 2021, 12:53 p.m. UTC | #4
Hi Laurent,

>> > Is this a property of the hardware, that is, are there multiple versions
>> > of this IP core covered by the same compatible string that support HDCP
>> > 1.4 only, DHCP 2.2 only or both ? Or is it a way to select what a given
>> > system will offer ?[]
>>
>> MHDP hardware supports both HDCP 2.2 and 1.4. So, this is a way
>> to select the version of HDCP, system wish to support.
>
>Then I'm not sure this qualifies as a DT property, which should describe
>the system, not configure it. A way for userspace to configure this
>would be better.

Since this is for source device, I am not sure how useful it is to allow
user to change HDCP version supported. I think doing it in DTS
gives more control over HDCP to system designer/integrator.

Regards,
Parshuram Thombare
Rob Herring March 8, 2021, 5:36 p.m. UTC | #5
On Mon, Mar 01, 2021 at 12:22:26PM +0100, Parshuram Thombare wrote:
> Add binding changes for HDCP in the MHDP8546 DPI/DP bridge binding.
> This binding is not used in any upstreamed DTS yet, so changing
> index of property 'j721e-intg' should not affect anything.

TI folks might disagree, but weren't Cc'ed.

> 
> Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
> ---
>  .../display/bridge/cdns,mhdp8546.yaml         | 29 ++++++++++++-------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
> index 63427878715e..5fdadadaac16 100644
> --- a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
> @@ -17,21 +17,24 @@ properties:
>        - ti,j721e-mhdp8546
>  
>    reg:
> -    minItems: 1
> -    maxItems: 2
> +    minItems: 2
> +    maxItems: 3
>      items:
>        - description:
>            Register block of mhdptx apb registers up to PHY mapped area (AUX_CONFIG_P).
>            The AUX and PMA registers are not part of this range, they are instead
>            included in the associated PHY.
> +      - description:
> +          Register block of mhdptx sapb registers.
>        - description:
>            Register block for DSS_EDP0_INTG_CFG_VP registers in case of TI J7 SoCs.
>  
>    reg-names:
> -    minItems: 1
> -    maxItems: 2
> +    minItems: 2
> +    maxItems: 3
>      items:
>        - const: mhdptx
> +      - const: mhdptx-sapb
>        - const: j721e-intg
>  
>    clocks:
> @@ -53,6 +56,11 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  hdcp-config:
> +    maxItems: 1
> +    description:
> +      HDCP version supported. Bit [0]:HDCP2.2 [1]:HDCP1.4.

2.2 is not backwards compatible with 1.4? What's the setting if not 
present? Maybe just a 'disable 2.2 boolean' if that's the non-common 
case.

In any case, it needs a type and constraints on the values.


> +
>    interrupts:
>      maxItems: 1
>  
> @@ -98,15 +106,15 @@ allOf:
>      then:
>        properties:
>          reg:
> -          minItems: 2
> +          minItems: 3
>          reg-names:
> -          minItems: 2
> +          minItems: 3
>      else:
>        properties:
>          reg:
> -          maxItems: 1
> +          maxItems: 2
>          reg-names:
> -          maxItems: 1
> +          maxItems: 2
>  
>  required:
>    - compatible
> @@ -129,8 +137,9 @@ examples:
>  
>          mhdp: dp-bridge@f0fb000000 {
>              compatible = "cdns,mhdp8546";
> -            reg = <0xf0 0xfb000000 0x0 0x1000000>;
> -            reg-names = "mhdptx";
> +            reg = <0xf0 0xfb000000 0x0 0x1000000>,
> +                  <0x0 0x4f48000 0x0 0x74>;
> +            reg-names = "mhdptx", "mhdptx-sapb";
>              clocks = <&mhdp_clock>;
>              phys = <&dp_phy>;
>              phy-names = "dpphy";
> -- 
> 2.25.1
>
Laurent Pinchart March 8, 2021, 8:42 p.m. UTC | #6
Hi Parshuram,

On Tue, Mar 02, 2021 at 12:53:50PM +0000, Parshuram Raju Thombare wrote:
> Hi Laurent,
> 
> >>> Is this a property of the hardware, that is, are there multiple versions
> >>> of this IP core covered by the same compatible string that support HDCP
> >>> 1.4 only, DHCP 2.2 only or both ? Or is it a way to select what a given
> >>> system will offer ?[]
> >>
> >> MHDP hardware supports both HDCP 2.2 and 1.4. So, this is a way
> >> to select the version of HDCP, system wish to support.
> >
> > Then I'm not sure this qualifies as a DT property, which should describe
> > the system, not configure it. A way for userspace to configure this
> > would be better.
> 
> Since this is for source device, I am not sure how useful it is to allow
> user to change HDCP version supported. I think doing it in DTS
> gives more control over HDCP to system designer/integrator.

But how would they do so ? What would be the rationale for selecting a
particular version in DT ?

I'm not thinking about giving control of this parameter to the end-user,
but in the context of an embedded system, it may be useful to select
which HDCP versions to offer based on different constraints at runtime.
This really seems like a system configuration parameter to me, not a
system description.
Parshuram Thombare March 9, 2021, 8:02 a.m. UTC | #7
>> >>> Is this a property of the hardware, that is, are there multiple versions
>> >>> of this IP core covered by the same compatible string that support HDCP
>> >>> 1.4 only, DHCP 2.2 only or both ? Or is it a way to select what a given
>> >>> system will offer ?[]
>> >>
>> >> MHDP hardware supports both HDCP 2.2 and 1.4. So, this is a way
>> >> to select the version of HDCP, system wish to support.
>> >
>> > Then I'm not sure this qualifies as a DT property, which should describe
>> > the system, not configure it. A way for userspace to configure this
>> > would be better.
>>
>> Since this is for source device, I am not sure how useful it is to allow
>> user to change HDCP version supported. I think doing it in DTS
>> gives more control over HDCP to system designer/integrator.
>
>But how would they do so ? What would be the rationale for selecting a
>particular version in DT ?
>
>I'm not thinking about giving control of this parameter to the end-user,
>but in the context of an embedded system, it may be useful to select
>which HDCP versions to offer based on different constraints at runtime.
>This really seems like a system configuration parameter to me, not a
>system description.

Ok, we can remove this parameter from DTS and let driver attempt both
HDCP 2.2 and HDCP 1.4 in the same sequence so that HDCP version is
selected based on sink capabilities. But then user space application will
not have information about the HDCP version with which content will be
protected. I suppose most UHD 4k content would need HDCP 2.2.
So, forcing HDCP version in DTS was the easiest way.

Regards,
Parshuram Thombare
Parshuram Thombare March 9, 2021, 3:35 p.m. UTC | #8
Hi Rob,

Thanks for your comments.

>> Add binding changes for HDCP in the MHDP8546 DPI/DP bridge binding.
>> This binding is not used in any upstreamed DTS yet, so changing
>> index of property 'j721e-intg' should not affect anything.
>
>TI folks might disagree, but weren't Cc'ed.
Kishon and Nikhil from TI are Cc'ed.
Hi Kishon / Nikhil, 
Do you think this can be an issue ?

>In any case, it needs a type and constraints on the values.
Ok, I will add type and constraints on the values.

Regards,
Parshuram Thombare
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
index 63427878715e..5fdadadaac16 100644
--- a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
@@ -17,21 +17,24 @@  properties:
       - ti,j721e-mhdp8546
 
   reg:
-    minItems: 1
-    maxItems: 2
+    minItems: 2
+    maxItems: 3
     items:
       - description:
           Register block of mhdptx apb registers up to PHY mapped area (AUX_CONFIG_P).
           The AUX and PMA registers are not part of this range, they are instead
           included in the associated PHY.
+      - description:
+          Register block of mhdptx sapb registers.
       - description:
           Register block for DSS_EDP0_INTG_CFG_VP registers in case of TI J7 SoCs.
 
   reg-names:
-    minItems: 1
-    maxItems: 2
+    minItems: 2
+    maxItems: 3
     items:
       - const: mhdptx
+      - const: mhdptx-sapb
       - const: j721e-intg
 
   clocks:
@@ -53,6 +56,11 @@  properties:
   power-domains:
     maxItems: 1
 
+  hdcp-config:
+    maxItems: 1
+    description:
+      HDCP version supported. Bit [0]:HDCP2.2 [1]:HDCP1.4.
+
   interrupts:
     maxItems: 1
 
@@ -98,15 +106,15 @@  allOf:
     then:
       properties:
         reg:
-          minItems: 2
+          minItems: 3
         reg-names:
-          minItems: 2
+          minItems: 3
     else:
       properties:
         reg:
-          maxItems: 1
+          maxItems: 2
         reg-names:
-          maxItems: 1
+          maxItems: 2
 
 required:
   - compatible
@@ -129,8 +137,9 @@  examples:
 
         mhdp: dp-bridge@f0fb000000 {
             compatible = "cdns,mhdp8546";
-            reg = <0xf0 0xfb000000 0x0 0x1000000>;
-            reg-names = "mhdptx";
+            reg = <0xf0 0xfb000000 0x0 0x1000000>,
+                  <0x0 0x4f48000 0x0 0x74>;
+            reg-names = "mhdptx", "mhdptx-sapb";
             clocks = <&mhdp_clock>;
             phys = <&dp_phy>;
             phy-names = "dpphy";