| Message ID | 20210222122406.41782-9-benjamin.gaignard@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add HANTRO G2/HEVC decoder support for IMX8MQ | expand |
On Mon, Feb 22, 2021 at 01:24:05PM +0100, Benjamin Gaignard wrote: > The current bindings seem to make the assumption that the > two VPUs hardware blocks (G1 and G2) are only one set of > registers. > After implementing the VPU reset driver and G2 decoder driver > it shows that all the VPUs are independent and don't need to > know about the registers of the other blocks. > Remove from the bindings the need to set all blocks register > but keep reg-names property because removing it from the driver > may affect other variants. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > version 2: > - be more verbose about why I change the bindings > Keep in mind that series comes after: https://www.spinics.net/lists/arm-kernel/msg875766.html > without that review and ack it won't work Better, but you've still mentioned nothing about breaking compatibility. Why is that okay?
Le 23/02/2021 à 01:34, Rob Herring a écrit : > On Mon, Feb 22, 2021 at 01:24:05PM +0100, Benjamin Gaignard wrote: >> The current bindings seem to make the assumption that the >> two VPUs hardware blocks (G1 and G2) are only one set of >> registers. >> After implementing the VPU reset driver and G2 decoder driver >> it shows that all the VPUs are independent and don't need to >> know about the registers of the other blocks. >> Remove from the bindings the need to set all blocks register >> but keep reg-names property because removing it from the driver >> may affect other variants. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> version 2: >> - be more verbose about why I change the bindings >> Keep in mind that series comes after: https://www.spinics.net/lists/arm-kernel/msg875766.html >> without that review and ack it won't work > Better, but you've still mentioned nothing about breaking compatibility. > Why is that okay? Because this reg-names wasn't used before for this variant so remove it won't change anything. >
On Tue, Feb 23, 2021 at 2:04 AM Benjamin Gaignard <benjamin.gaignard@collabora.com> wrote: > > > Le 23/02/2021 à 01:34, Rob Herring a écrit : > > On Mon, Feb 22, 2021 at 01:24:05PM +0100, Benjamin Gaignard wrote: > >> The current bindings seem to make the assumption that the > >> two VPUs hardware blocks (G1 and G2) are only one set of > >> registers. > >> After implementing the VPU reset driver and G2 decoder driver > >> it shows that all the VPUs are independent and don't need to > >> know about the registers of the other blocks. > >> Remove from the bindings the need to set all blocks register > >> but keep reg-names property because removing it from the driver > >> may affect other variants. > >> > >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > >> --- > >> version 2: > >> - be more verbose about why I change the bindings > >> Keep in mind that series comes after: https://www.spinics.net/lists/arm-kernel/msg875766.html > >> without that review and ack it won't work > > Better, but you've still mentioned nothing about breaking compatibility. > > Why is that okay? > > Because this reg-names wasn't used before for this variant so remove it won't change anything. It is the reset changes in the driver that break. The driver previously got the 'ctrl' registers whether it went by name or index, right? With an old DTB and a kernel with the changes (and vice-versa), you'll have nothing to handle the VPU resets because the VPU reset node doesn't exist. It could work if the default state is not held in reset. At least the removal of 'ctrl' registers belongs in the reset changes series. Rob
Hi Rob, On Tue, 2021-02-23 at 08:31 -0600, Rob Herring wrote: > On Tue, Feb 23, 2021 at 2:04 AM Benjamin Gaignard > <benjamin.gaignard@collabora.com> wrote: > > > > > > Le 23/02/2021 à 01:34, Rob Herring a écrit : > > > On Mon, Feb 22, 2021 at 01:24:05PM +0100, Benjamin Gaignard wrote: > > > > The current bindings seem to make the assumption that the > > > > two VPUs hardware blocks (G1 and G2) are only one set of > > > > registers. > > > > After implementing the VPU reset driver and G2 decoder driver > > > > it shows that all the VPUs are independent and don't need to > > > > know about the registers of the other blocks. > > > > Remove from the bindings the need to set all blocks register > > > > but keep reg-names property because removing it from the driver > > > > may affect other variants. > > > > > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > > > > --- > > > > version 2: > > > > - be more verbose about why I change the bindings > > > > Keep in mind that series comes after: https://www.spinics.net/lists/arm-kernel/msg875766.html > > > > without that review and ack it won't work > > > Better, but you've still mentioned nothing about breaking compatibility. > > > Why is that okay? > > Indeed, the commit description should be clearer about breaking compatibility. > > Because this reg-names wasn't used before for this variant so remove it won't change anything. > > It is the reset changes in the driver that break. The driver > previously got the 'ctrl' registers whether it went by name or index, > right? With an old DTB and a kernel with the changes (and vice-versa), > you'll have nothing to handle the VPU resets because the VPU reset > node doesn't exist. It could work if the default state is not held in > reset. > > At least the removal of 'ctrl' registers belongs in the reset changes series. > > Considering that FFMPEG patches that are required to support this driver are still floating around, and GStreamer's implementation is also still a bit under discussion, we are certain there aren't many upstreams users (leaving ChromiumOS aside which mostly care for Rockchip variants). So, given the driver is in staging, and that there aren't users of the i.MX8MQ G1 variant just yet, I think we are safe breaking the compatibility (and I'm not taking it lightly). It would be important to detect an old devicetree and do some pr_warn about the driver not supporting it. Thanks, Ezequiel
diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml index 762be3f96ce9..468435c70eef 100644 --- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml +++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml @@ -15,24 +15,25 @@ description: properties: compatible: - const: nxp,imx8mq-vpu + enum: + - nxp,imx8mq-vpu + - nxp,imx8mq-vpu-g2 reg: - maxItems: 3 + maxItems: 1 reg-names: - items: - - const: g1 - - const: g2 - - const: ctrl + enum: + - g1 + - g2 interrupts: - maxItems: 2 + maxItems: 1 interrupt-names: - items: - - const: g1 - - const: g2 + enum: + - g1 + - g2 clocks: maxItems: 3 @@ -46,6 +47,9 @@ properties: power-domains: maxItems: 1 + resets: + maxItems: 1 + required: - compatible - reg @@ -54,6 +58,7 @@ required: - interrupt-names - clocks - clock-names + - resets additionalProperties: false @@ -61,19 +66,32 @@ examples: - | #include <dt-bindings/clock/imx8mq-clock.h> #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/reset/imx8mq-vpu-reset.h> - vpu: video-codec@38300000 { + vpu_g1: video-codec@38300000 { compatible = "nxp,imx8mq-vpu"; - reg = <0x38300000 0x10000>, - <0x38310000 0x10000>, - <0x38320000 0x10000>; - reg-names = "g1", "g2", "ctrl"; - interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>, - <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; - interrupt-names = "g1", "g2"; + reg = <0x38300000 0x10000>; + reg-names = "g1"; + interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "g1"; + clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>, + <&clk IMX8MQ_CLK_VPU_G2_ROOT>, + <&clk IMX8MQ_CLK_VPU_DEC_ROOT>; + clock-names = "g1", "g2", "bus"; + power-domains = <&pgc_vpu>; + resets = <&vpu_reset IMX8MQ_RESET_VPU_RESET_G1>; + }; + + vpu_g2: video-codec@38310000 { + compatible = "nxp,imx8mq-vpu-g2"; + reg = <0x38310000 0x10000>; + reg-names = "g2"; + interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "g2"; clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>, <&clk IMX8MQ_CLK_VPU_G2_ROOT>, <&clk IMX8MQ_CLK_VPU_DEC_ROOT>; clock-names = "g1", "g2", "bus"; power-domains = <&pgc_vpu>; + resets = <&vpu_reset IMX8MQ_RESET_VPU_RESET_G2>; };
The current bindings seem to make the assumption that the two VPUs hardware blocks (G1 and G2) are only one set of registers. After implementing the VPU reset driver and G2 decoder driver it shows that all the VPUs are independent and don't need to know about the registers of the other blocks. Remove from the bindings the need to set all blocks register but keep reg-names property because removing it from the driver may affect other variants. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> --- version 2: - be more verbose about why I change the bindings Keep in mind that series comes after: https://www.spinics.net/lists/arm-kernel/msg875766.html without that review and ack it won't work .../bindings/media/nxp,imx8mq-vpu.yaml | 54 ++++++++++++------- 1 file changed, 36 insertions(+), 18 deletions(-)