Hi Rob,
Thanks for the quick review.
On 18/01/24 01:43, Rob Herring wrote:
> On Tue, Jan 16, 2024 at 07:11:40PM +0530, Devarsh Thakkar wrote:
>> Add support for using TI Keystone DSS hardware present in display
>> sharing mode.
>>
>> TI Keystone DSS hardware supports partitioning of resources between
>> multiple hosts as it provides separate register space and unique
>> interrupt line to each host.
>>
>> The DSS hardware can be used in shared mode in such a way that one or
>> more of video planes can be owned by Linux wherease other planes can be
>> owned by remote cores.
>>
>> One or more of the video ports can be dedicated exclusively to a
>> processing core, wherease some of the video ports can be shared between
>> two hosts too with only one of them having write access.
>>
>> Signed-off-by: Devarsh Thakkar <devarsht@xxxxxx>
>> ---
>> .../bindings/display/ti/ti,am65x-dss.yaml | 82 +++++++++++++++++++
>> 1 file changed, 82 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> index 55e3e490d0e6..d9bc69fbf1fb 100644
>> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> @@ -112,6 +112,86 @@ properties:
>> Input memory (from main memory to dispc) bandwidth limit in
>> bytes per second
>>
>> + ti,dss-shared-mode:
>> + type: boolean
>> + description:
>> + TI DSS7 supports sharing of display between multiple hosts
>> + as it provides separate register space for display configuration and
>> + unique interrupt line to each host.
>
> If you care about line breaks, you need '|'.
>
Noted.
>> + One of the host is provided access to the global display
>> + configuration labelled as "common" region of DSS allows that host
>> + exclusive access to global registers of DSS while other host can
>> + configure the display for it's usage using a separate register
>> + space labelled as "common1".
>> + The DSS resources can be partitioned in such a way that one or more
>> + of the video planes are owned by Linux whereas other video planes
>
> Your h/w can only run Linux?
>
> What if you want to use this same binding to define the configuration to
> the 'remote processor'? You can easily s/Linux/the OS/, but it all
> should be reworded to describe things in terms of the local processor.
>
It can run both Linux and RTOS or for that matter any other OS too. But yes I
got your point, will reword accordingly.
>> + can be owned by a remote core.
>> + The video port controlling these planes acts as a shared video port
>> + and it can be configured with write access either by Linux or the
>> + remote core in which case Linux only has read-only access to that
>> + video port.
>
> What is the purpose of this property when all the other properties are
> required?
>
The ti,dss-shared-mode and below group of properties are optional. But
if ti,dss-shared-mode is set then only driver should parse below set of
properties.
>> +
>> + ti,dss-shared-mode-planes:
>> + description:
>> + The video layer that is owned by processing core running Linux.
>> + The display driver running from Linux has exclusive write access to
>> + this video layer.
>> + $ref: /schemas/types.yaml#/definitions/string
>> + enum: [vidl, vid]
>> +
>> + ti,dss-shared-mode-vp:
>> + description:
>> + The video port that is being used in context of processing core
>> + running Linux with display susbsytem being used in shared mode.
>> + This can be owned either by the processing core running Linux in
>> + which case Linux has the write access and the responsibility to
>> + configure this video port and the associated overlay manager or
>> + it can be shared between core running Linux and a remote core
>> + with remote core provided with write access to this video port and
>> + associated overlay managers and remote core configures and drives
>> + this video port also feeding data from one or more of the
>> + video planes owned by Linux, with Linux only having read-only access
>> + to this video port and associated overlay managers.
>> +
>> + $ref: /schemas/types.yaml#/definitions/string
>> + enum: [vp1, vp2]
>> +
>> + ti,dss-shared-mode-common:
>> + description:
>> + The DSS register region owned by processing core running Linux.
>> + $ref: /schemas/types.yaml#/definitions/string
>> + enum: [common, common1]
>> +
>> + ti,dss-shared-mode-vp-owned:
>> + description:
>> + This tells whether processing core running Linux has write access to
>> + the video ports enlisted in ti,dss-shared-mode-vps.
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + enum: [0, 1]
>
> This can be boolean. Do writes abort or just get ignored? The latter can
> be probed and doesn't need a property.
>
Although we have kept all these properties as enums, but actually in driver we
are treating them as array of enums and using device_property_read_u32_array.
The reason being that for SoCs using am65x-dss bindings they can only have
single entry either vp1 or vp2 or 0 or 1 as there are only two video ports. So
for them the device tree overlay would look like :
&dss0 {
ti,dss-shared-mode;
ti,dss-shared-mode-vp = "vp1";
ti,dss-shared-mode-vp-owned = <0>;
ti,dss-shared-mode-common = "common1";
ti,dss-shared-mode-planes = "vid";
ti,dss-shared-mode-plane-zorder = <0>;
interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_>;
}
But we also plan to extend these bindings to SoCs using
Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml where there are
multiple video ports. So in that the driver and bindings should support below
configuration :
&dss0 {
ti,dss-shared-mode;
ti,dss-shared-mode-vp = "vp1 vp2";
ti,dss-shared-mode-vp-owned = <0 1>;
ti,dss-shared-mode-common = "common_s1";
ti,dss-shared-mode-planes = "vid1 vidl1";
ti,dss-shared-mode-plane-zorder = <0 1>;
interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_>;
}
As I am using device_property_read_u32_array in driver I thought to keep this
as uint32 in enum for am65x.yaml which works well with the driver.
>> +
>> + ti,dss-shared-mode-plane-zorder:
>> + description:
>> + The zorder of the planes owned by Linux.
>> + For the scenario where Linux is not having write access to associated
>> + video port, this field is just for
>> + informational purpose to enumerate the zorder configuration
>> + being used by remote core.
>> +
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + enum: [0, 1]
>
> I don't understand how 0 or 1 defines Z-order.
>
As there are only two planes in total so z-order can be either 0 or 1 for the
shared mode plane as there is only a single entry of plane.
For e.g. if ti,dss-shared-mode-plane-zorder is 1 then it means the plane owned
by Linux is programmed as topmost plane wherease the plane owned by remote
core is programmed as the underneath one.
>> +
>> +dependencies:
>> + ti,dss-shared-mode: [ 'ti,dss-shared-mode-planes', 'ti,dss-shared-mode-vp',
>> + 'ti,dss-shared-mode-plane-zorder', 'ti,dss-shared-mode-vp-owned']
>> + ti,dss-shared-mode-vp: ['ti,dss-shared-mode', 'ti,dss-shared-mode-planes',
>> + 'ti,dss-shared-mode-plane-zorder', 'ti,dss-shared-mode-vp-owned']
>> + ti,dss-shared-mode-planes: ['ti,dss-shared-mode', 'ti,dss-shared-mode-vp',
>> + 'ti,dss-shared-mode-plane-zorder', 'ti,dss-shared-mode-vp-owned']
>> + ti,dss-shared-mode-plane-zorder: ['ti,dss-shared-mode-planes', 'ti,dss-shared-mode-vp',
>> + 'ti,dss-shared-mode', 'ti,dss-shared-mode-vp-owned']
>> + ti,dss-shared-mode-vp-owned: ['ti,dss-shared-mode-planes', 'ti,dss-shared-mode-vp',
>> + 'ti,dss-shared-mode', 'ti,dss-shared-mode-plane-zorder']
>> +
>> allOf:
>> - if:
>> properties:
>> @@ -123,6 +203,8 @@ allOf:
>> ports:
>> properties:
>> port@0: false
>> + ti,dss-shared-mode-vp:
>> + enum: [vp2]
>
> This should throw a warning. You just defined a property called 'enum'.
>
Oops will fix this.
Regards
Devarsh
> Rob
