| Message ID | 20230620000349.2122191-4-nfraprado@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/6] media: dt-bindings: mediatek,vcodec: Allow single clock for mt8183 | expand |
On 20/06/2023 18:31, Nícolas F. R. A. Prado wrote: > On Tue, Jun 20, 2023 at 03:00:00PM +0200, Krzysztof Kozlowski wrote: >> On 20/06/2023 14:46, Nícolas F. R. A. Prado wrote: >>> On Tue, Jun 20, 2023 at 10:12:14AM +0200, Krzysztof Kozlowski wrote: >>>> On 20/06/2023 02:03, Nícolas F. R. A. Prado wrote: >>>>> The binding expects the first register space to be VDEC_SYS. But on >>>>> mt8183, which uses the stateless decoders, this space is used only for >>>>> controlling clocks and resets, which are better described as separate >>>>> clock-controller and reset-controller nodes. >>>>> >>>>> In fact, in mt8173's devicetree there are already such separate >>>>> clock-controller nodes, which cause duplicate addresses between the >>>>> vdecsys node and the vcodec node. But for this SoC, since the stateful >>>>> decoder code makes other uses of the VDEC_SYS register space, it's not >>>>> straightforward to remove it. >>>>> >>>>> In order to avoid the same address conflict to happen on mt8183, >>>>> since the only current use of the VDEC_SYS register space in >>>>> the driver is to read the status of a hardware controlled clock, remove >>>>> the VDEC_SYS register space from the binding and describe an extra >>>>> syscon that will be used to directly check the hardware status. >>>>> >>>>> Also add reg-names to be able to tell that this new register schema is >>>>> used, so the driver can keep backward compatibility. >>>>> >>>>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> >>>>> >>>>> --- >>>>> I dropped the tags from this commit since a syscon is now used instead >>>>> of an extra clock. >>>>> >>>>> Changes in v3: >>>>> - Removed the active clock >>>>> - Added a mediatek,vdecsys syscon property >>>>> >>>>> Changes in v2: >>>>> - Merged with patch 1 (media: dt-bindings: mediatek,vcodec: Allow single >>>>> clock for mt8183) to avoid changing number of clocks twice >>>>> - Added maxItems to reg-names >>>>> - Constrained clocks for each compatible >>>>> - Reordered properties for each compatible >>>>> >>>>> .../media/mediatek,vcodec-decoder.yaml | 30 +++++++++++++++++++ >>>>> 1 file changed, 30 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml >>>>> index 1e56ece44aee..2f625c50bbfe 100644 >>>>> --- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml >>>>> +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml >>>>> @@ -21,8 +21,13 @@ properties: >>>>> - mediatek,mt8183-vcodec-dec >>>>> >>>>> reg: >>>>> + minItems: 11 >>>>> maxItems: 12 >>>>> >>>>> + reg-names: >>>>> + minItems: 11 >>>>> + maxItems: 11 >>>> >>>> maxItems: 12 >>>> >>>>> + >>>>> interrupts: >>>>> maxItems: 1 >>>>> >>>>> @@ -60,6 +65,10 @@ properties: >>>>> description: >>>>> Describes point to scp. >>>>> >>>>> + mediatek,vdecsys: >>>>> + $ref: /schemas/types.yaml#/definitions/phandle >>>>> + description: Phandle to the vdecsys syscon node. >>>>> + >>>>> required: >>>>> - compatible >>>>> - reg >>>>> @@ -79,8 +88,26 @@ allOf: >>>>> then: >>>>> required: >>>>> - mediatek,scp >>>>> + - mediatek,vdecsys >>>>> >>>>> properties: >>>>> + reg: >>>>> + maxItems: 11 >>>>> + >>>>> + reg-names: >>>>> + items: >>>>> + - const: misc >>>>> + - const: ld >>>>> + - const: top >>>>> + - const: cm >>>>> + - const: ad >>>>> + - const: av >>>>> + - const: pp >>>>> + - const: hwd >>>>> + - const: hwq >>>>> + - const: hwb >>>>> + - const: hwg >>>>> + >>>>> clocks: >>>>> minItems: 1 >>>>> maxItems: 1 >>>>> @@ -101,6 +128,9 @@ allOf: >>>>> - mediatek,vpu >>>>> >>>>> properties: >>>>> + reg: >>>>> + minItems: 12 >>>> >>>> >>>> What about reg-names here? They should be also defined and in sync with >>>> regs. >>> >>> That's intentional. As described in the commit message, mt8173 will keep having >>> the VDEC_SYS iospace, while mt8183 won't. And we use the presence of reg-names >>> to tell them apart. >>> >>> So, mt8173 has 12 regs, no reg-names and no syscon, while mt8183 has 11 regs, >>> with reg-names and the syscon. >> >> reg-names is not the way to tell apart variants. Compatible is. If you >> add reg-names for one variant, it's expected to have it defined for >> other as well, because the order of items in reg is always fixed. > > But differentiating with compatible in this case would be wrong, since it's not > not something inherent to the SoC. We really just want to be able to tell > whether the vdecsys iospace is supplied as a reg or as a syscon. Wait, you should not have one device or even family of devices taking their IO space with two different methods. It's exactly the same device, the same bus. > > This series focuses on getting the mt8183 decoder working, and as part of that > introduces the binding and DT node for mt8183 with vdecsys as a syscon instead > of a reg, to avoid introducing new 'duplicate unit-address' DT warnings. I got patches 1, 2, 3 and 6, nothing more so I cannot comment on what else you are trying to do here. Since you did not cc me, it's not relevant. Your DTS change does nothing like switching from MMIO to syscon. But anyway this variant comes with some set of regs and reg-names. Other variant comes with different set. In all cases they should be defined, even by "defined" means not allowed. > > But in a separate series we could drop vdecsys from mt8173's reg as well, > passing it as a syscon instead, which would solve the warning on that platform, > though some more driver changes would be needed to be able to handle it for that > SoC. The newer SoCs like mt8192, mt8195, etc, should also get vdecsys dropped > from their regs to have a correct memory description. > Sure, but I don't understand how does it affect defining and making specific regs/reg-names or keeping them loose. Best regards, Krzysztof
On 21/06/2023 20:00, Nícolas F. R. A. Prado wrote: >> >> But anyway this variant comes with some set of regs and reg-names. Other >> variant comes with different set. In all cases they should be defined, >> even by "defined" means not allowed. > > I'm not sure what you mean. Are you suggesting to disable reg-names on mt8173? That's one of the options if for some reason you don't want to define them. > >> >>> >>> But in a separate series we could drop vdecsys from mt8173's reg as well, >>> passing it as a syscon instead, which would solve the warning on that platform, >>> though some more driver changes would be needed to be able to handle it for that >>> SoC. The newer SoCs like mt8192, mt8195, etc, should also get vdecsys dropped >>> from their regs to have a correct memory description. >>> >> >> Sure, but I don't understand how does it affect defining and making >> specific regs/reg-names or keeping them loose. > > We need some way to tell in the driver whether the first reg is VDEC_SYS or not. > Since so far reg-names have not been used for the vcodec, the simplest, and > cleanest, way to do it, is to add reg-names when VDEC_SYS is not present. When > the other SoCs are updated to no longer have the first reg as VDEC_SYS, they > would also have reg-names added to their binding, to clearly indicate that. Don't use reg-names for that. The order of entries is anyway strict. > > For example, for mt8173 we currently have > > vcodec_dec: vcodec@16000000 { > compatible = "mediatek,mt8173-vcodec-dec"; > reg = <0 0x16000000 0 0x100>, /* VDEC_SYS */ > <0 0x16020000 0 0x1000>, /* VDEC_MISC */ > <0 0x16021000 0 0x800>, /* VDEC_LD */ > <0 0x16021800 0 0x800>, /* VDEC_TOP */ > <0 0x16022000 0 0x1000>, /* VDEC_CM */ > <0 0x16023000 0 0x1000>, /* VDEC_AD */ > <0 0x16024000 0 0x1000>, /* VDEC_AV */ > <0 0x16025000 0 0x1000>, /* VDEC_PP */ > <0 0x16026800 0 0x800>, /* VDEC_HWD */ > <0 0x16027000 0 0x800>, /* VDEC_HWQ */ > <0 0x16027800 0 0x800>, /* VDEC_HWB */ > <0 0x16028400 0 0x400>; /* VDEC_HWG */ > > In a future series, when removing VDEC_SYS from it, it would become > > vcodec_dec: vcodec@16020000 { > compatible = "mediatek,mt8173-vcodec-dec"; > reg = <0 0x16020000 0 0x1000>, /* VDEC_MISC */ > <0 0x16021000 0 0x800>, /* VDEC_LD */ > <0 0x16021800 0 0x800>, /* VDEC_TOP */ > <0 0x16022000 0 0x1000>, /* VDEC_CM */ > <0 0x16023000 0 0x1000>, /* VDEC_AD */ > <0 0x16024000 0 0x1000>, /* VDEC_AV */ > <0 0x16025000 0 0x1000>, /* VDEC_PP */ > <0 0x16026800 0 0x800>, /* VDEC_HWD */ > <0 0x16027000 0 0x800>, /* VDEC_HWQ */ > <0 0x16027800 0 0x800>, /* VDEC_HWB */ > <0 0x16028400 0 0x400>; /* VDEC_HWG */ > reg-names = "misc", "ld", "top", "cm", "ad", "av", "pp", > "hwd", "hwq", "hwb", "hwg"; So you want to use reg-names to avoid ABI break. This is not the reason not to define reg-names for other case. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml index 1e56ece44aee..2f625c50bbfe 100644 --- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml @@ -21,8 +21,13 @@ properties: - mediatek,mt8183-vcodec-dec reg: + minItems: 11 maxItems: 12 + reg-names: + minItems: 11 + maxItems: 11 + interrupts: maxItems: 1 @@ -60,6 +65,10 @@ properties: description: Describes point to scp. + mediatek,vdecsys: + $ref: /schemas/types.yaml#/definitions/phandle + description: Phandle to the vdecsys syscon node. + required: - compatible - reg @@ -79,8 +88,26 @@ allOf: then: required: - mediatek,scp + - mediatek,vdecsys properties: + reg: + maxItems: 11 + + reg-names: + items: + - const: misc + - const: ld + - const: top + - const: cm + - const: ad + - const: av + - const: pp + - const: hwd + - const: hwq + - const: hwb + - const: hwg + clocks: minItems: 1 maxItems: 1 @@ -101,6 +128,9 @@ allOf: - mediatek,vpu properties: + reg: + minItems: 12 + clocks: minItems: 8 maxItems: 8
The binding expects the first register space to be VDEC_SYS. But on mt8183, which uses the stateless decoders, this space is used only for controlling clocks and resets, which are better described as separate clock-controller and reset-controller nodes. In fact, in mt8173's devicetree there are already such separate clock-controller nodes, which cause duplicate addresses between the vdecsys node and the vcodec node. But for this SoC, since the stateful decoder code makes other uses of the VDEC_SYS register space, it's not straightforward to remove it. In order to avoid the same address conflict to happen on mt8183, since the only current use of the VDEC_SYS register space in the driver is to read the status of a hardware controlled clock, remove the VDEC_SYS register space from the binding and describe an extra syscon that will be used to directly check the hardware status. Also add reg-names to be able to tell that this new register schema is used, so the driver can keep backward compatibility. Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- I dropped the tags from this commit since a syscon is now used instead of an extra clock. Changes in v3: - Removed the active clock - Added a mediatek,vdecsys syscon property Changes in v2: - Merged with patch 1 (media: dt-bindings: mediatek,vcodec: Allow single clock for mt8183) to avoid changing number of clocks twice - Added maxItems to reg-names - Constrained clocks for each compatible - Reordered properties for each compatible .../media/mediatek,vcodec-decoder.yaml | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+)