On Wed, Jun 21, 2023 at 10:41:49AM +0200, Krzysztof Kozlowski wrote: > 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@xxxxxxxxxxxxx> > >>>>> > >>>>> --- > >>>>> 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. The VDEC_SYS IO space is arguably not part of the vcodec IO space, since it is declared by a different node. Hence we shouldn't be getting it through reg, but instead through a syscon, to avoid clashing addresses. In other words, the VDEC_SYS IO space shouldn't have been in the binding as a 'reg' in the first place. And since we want to: 1. Keep backward compatibility 2. Fix the 'duplicate unit-address' DT warning We will need to support both ways - reg or syscon - of passing the VDEC_SYS IO space moving forward. > > > > > 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. That's ok, the driver changes aren't relevant to this discussion. > > Your DTS change does nothing like switching from MMIO to syscon. The original downstream DT node used MMIO for VDEC_SYS, but I've adapted that commit to use a syscon. So the commit is implicitly doing the switch, it just doesn't show because the node wasn't upstreamed before on mt8183. > > 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? > > > > > 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. 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"; mediatek,vdecsys = <&vdecsys>; Thanks, Nícolas