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 clock that indicates the hardware
is active, remove the VDEC_SYS register space from the binding and
describe an extra clock 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>
---
.../media/mediatek,vcodec-decoder.yaml | 29 +++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
index 6447e6c86f29..36a53b2484d6 100644
--- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
+++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
@@ -21,17 +21,21 @@ properties:
- mediatek,mt8183-vcodec-dec
reg:
+ minItems: 11
maxItems: 12
+ reg-names:
+ minItems: 11
+
interrupts:
maxItems: 1
clocks:
- minItems: 1
+ minItems: 2
maxItems: 8
clock-names:
- minItems: 1
+ minItems: 2
maxItems: 8
assigned-clocks: true
@@ -84,6 +88,24 @@ allOf:
clock-names:
items:
- const: vdec
+ - const: active
+
+ 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
- if:
properties:
@@ -108,6 +130,9 @@ allOf:
- const: venc_lt_sel
- const: vdec_bus_clk_src
+ reg:
+ minItems: 12
+
additionalProperties: false
examples:
--
2.40.1
On 05/06/2023 18:20, 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 clock that indicates the hardware > is active, remove the VDEC_SYS register space from the binding and > describe an extra clock 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> > --- > > .../media/mediatek,vcodec-decoder.yaml | 29 +++++++++++++++++-- > 1 file changed, 27 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml > index 6447e6c86f29..36a53b2484d6 100644 > --- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml > @@ -21,17 +21,21 @@ properties: > - mediatek,mt8183-vcodec-dec > > reg: > + minItems: 11 > maxItems: 12 > > + reg-names: > + minItems: 11 maxItems > + > interrupts: > maxItems: 1 > > clocks: > - minItems: 1 > + minItems: 2 It does not make any sense. Just two patches ago you made it 1! Don't add incorrect values which are immediately changed in the same patchset. > maxItems: 8 > > clock-names: > - minItems: 1 > + minItems: 2 > maxItems: 8 > > assigned-clocks: true > @@ -84,6 +88,24 @@ allOf: > clock-names: > items: > - const: vdec > + - const: active > + > + 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 > > - if: > properties: > @@ -108,6 +130,9 @@ allOf: > - const: venc_lt_sel > - const: vdec_bus_clk_src > > + reg: > + minItems: 12 so max can be 1000? Best regards, Krzysztof
On Tue, Jun 06, 2023 at 11:16:12AM +0200, Krzysztof Kozlowski wrote: > On 05/06/2023 18:20, 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 clock that indicates the hardware > > is active, remove the VDEC_SYS register space from the binding and > > describe an extra clock 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> > > --- > > > > .../media/mediatek,vcodec-decoder.yaml | 29 +++++++++++++++++-- > > 1 file changed, 27 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml > > index 6447e6c86f29..36a53b2484d6 100644 > > --- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml > > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml > > @@ -21,17 +21,21 @@ properties: > > - mediatek,mt8183-vcodec-dec > > > > reg: > > + minItems: 11 > > maxItems: 12 > > > > + reg-names: > > + minItems: 11 > > maxItems > > > + > > interrupts: > > maxItems: 1 > > > > clocks: > > - minItems: 1 > > + minItems: 2 > > It does not make any sense. Just two patches ago you made it 1! Don't > add incorrect values which are immediately changed in the same patchset. It's not an incorrect value. At the point that commit was added, the first reg was still VDEC_SYS, so an active clock wasn't required. This commit removes the VDEC_SYS reg, and so the active clock becomes necessary. Splitting it like this made the most sense to me, and I thought it would also simplify review. But since it seems to be a problem I'll merge commit 1 with this one in v2 to avoid changing the number of clocks twice. > > > maxItems: 8 > > > > clock-names: > > - minItems: 1 > > + minItems: 2 > > maxItems: 8 > > > > assigned-clocks: true > > @@ -84,6 +88,24 @@ allOf: > > clock-names: > > items: > > - const: vdec > > + - const: active > > + > > + 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 > > > > - if: > > properties: > > @@ -108,6 +130,9 @@ allOf: > > - const: venc_lt_sel > > - const: vdec_bus_clk_src > > > > + reg: > > + minItems: 12 > > so max can be 1000? No, there's 'maxItems: 12' up above in the general case. Thanks, Nícolas
Il 05/06/23 18:20, Nícolas F. R. A. Prado ha scritto: > 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 clock that indicates the hardware > is active, remove the VDEC_SYS register space from the binding and > describe an extra clock 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> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
© 2016 - 2024 Red Hat, Inc.