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
