Message ID | 20210318082046.51546-1-benjamin.gaignard@collabora.com |
---|---|
Headers | show |
Series | Add HANTRO G2/HEVC decoder support for IMX8MQ | expand |
On Thu, Mar 18, 2021 at 09:20:36AM +0100, Benjamin Gaignard wrote: > In order to be able to share the control hardware block between > VPUs use a syscon instead a ioremap it in the driver. > To keep the compatibility with older DT if 'nxp,imx8mq-vpu-ctrl' > phandle is not found look at 'ctrl' reg-name. > With the method it becomes useless to provide a list of register > names so remove it. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > version 5: > - use syscon instead of VPU reset driver. > - if DT doesn't provide syscon keep backward compatibilty by using > 'ctrl' reg-name. > > drivers/staging/media/hantro/hantro.h | 5 +- > drivers/staging/media/hantro/imx8m_vpu_hw.c | 52 ++++++++++++--------- > 2 files changed, 34 insertions(+), 23 deletions(-) > > diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h > index 65f9f7ea7dcf..a99a96b84b5e 100644 > --- a/drivers/staging/media/hantro/hantro.h > +++ b/drivers/staging/media/hantro/hantro.h > @@ -13,6 +13,7 @@ > #define HANTRO_H_ > > #include <linux/platform_device.h> > +#include <linux/regmap.h> > #include <linux/videodev2.h> > #include <linux/wait.h> > #include <linux/clk.h> > @@ -167,7 +168,7 @@ hantro_vdev_to_func(struct video_device *vdev) > * @reg_bases: Mapped addresses of VPU registers. > * @enc_base: Mapped address of VPU encoder register for convenience. > * @dec_base: Mapped address of VPU decoder register for convenience. > - * @ctrl_base: Mapped address of VPU control block. > + * @ctrl_base: Regmap of VPU control block. > * @vpu_mutex: Mutex to synchronize V4L2 calls. > * @irqlock: Spinlock to synchronize access to data structures > * shared with interrupt handlers. > @@ -186,7 +187,7 @@ struct hantro_dev { > void __iomem **reg_bases; > void __iomem *enc_base; > void __iomem *dec_base; > - void __iomem *ctrl_base; > + struct regmap *ctrl_base; > > struct mutex vpu_mutex; /* video_device lock */ > spinlock_t irqlock; > diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c > index c222de075ef4..bd9d135dd440 100644 > --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c > +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c > @@ -7,6 +7,7 @@ > > #include <linux/clk.h> > #include <linux/delay.h> > +#include <linux/mfd/syscon.h> > > #include "hantro.h" > #include "hantro_jpeg.h" > @@ -24,30 +25,28 @@ > #define CTRL_G1_PP_FUSE 0x0c > #define CTRL_G2_DEC_FUSE 0x10 > > +static const struct regmap_config ctrl_regmap_ctrl = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 0x14, > +}; > + > static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits) > { > - u32 val; > - > /* Assert */ > - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET); > - val &= ~reset_bits; > - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET); > + regmap_update_bits(vpu->ctrl_base, CTRL_SOFT_RESET, reset_bits, 0); > > udelay(2); > > /* Release */ > - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET); > - val |= reset_bits; > - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET); > + regmap_update_bits(vpu->ctrl_base, CTRL_SOFT_RESET, > + reset_bits, reset_bits); > } > > static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits) > { > - u32 val; > - > - val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE); > - val |= clock_bits; > - writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE); > + regmap_update_bits(vpu->ctrl_base, CTRL_CLOCK_ENABLE, > + clock_bits, clock_bits); > } > > static int imx8mq_runtime_resume(struct hantro_dev *vpu) > @@ -64,9 +63,9 @@ static int imx8mq_runtime_resume(struct hantro_dev *vpu) > imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2); > > /* Set values of the fuse registers */ > - writel(0xffffffff, vpu->ctrl_base + CTRL_G1_DEC_FUSE); > - writel(0xffffffff, vpu->ctrl_base + CTRL_G1_PP_FUSE); > - writel(0xffffffff, vpu->ctrl_base + CTRL_G2_DEC_FUSE); > + regmap_write(vpu->ctrl_base, CTRL_G1_DEC_FUSE, 0xffffffff); > + regmap_write(vpu->ctrl_base, CTRL_G1_PP_FUSE, 0xffffffff); > + regmap_write(vpu->ctrl_base, CTRL_G2_DEC_FUSE, 0xffffffff); > > clk_bulk_disable_unprepare(vpu->variant->num_clocks, vpu->clocks); > > @@ -150,8 +149,22 @@ static irqreturn_t imx8m_vpu_g1_irq(int irq, void *dev_id) > > static int imx8mq_vpu_hw_init(struct hantro_dev *vpu) > { > - vpu->dec_base = vpu->reg_bases[0]; > - vpu->ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1]; > + struct device_node *np = vpu->dev->of_node; > + > + vpu->ctrl_base = syscon_regmap_lookup_by_phandle(np, "nxp,imx8mq-vpu-ctrl"); I think calling this nxp,imx8m-vpu-ctrl would allow to share this with i.MX8MM later. Otherwise, Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> thanks Philipp
On Thu, Mar 18, 2021 at 09:20:45AM +0100, Benjamin Gaignard wrote: > Add variant to IMX8M to enable G2/HEVC codec. > Define the capabilities for the hardware up to 3840x2160. > G2 doesn't have postprocessor, use the same clocks and got it > own interruption. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> regards Philipp
On Thu, Mar 18, 2021 at 09:20:46AM +0100, Benjamin Gaignard wrote: > Split VPU node in two: one for G1 and one for G2 since they are > different hardware blocks. > Add syscon for hardware control block. > Remove reg-names property that is useless. > Each VPU node only need one interrupt. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > version 5: > - use syscon instead of VPU reset > > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 43 ++++++++++++++++++----- > 1 file changed, 34 insertions(+), 9 deletions(-) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > index 17c449e12c2e..b537d153ebbd 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > @@ -1329,15 +1329,16 @@ usb3_phy1: usb-phy@382f0040 { > status = "disabled"; > }; > > - vpu: video-codec@38300000 { > + vpu_ctrl: syscon@38320000 { > + compatible = "nxp,imx8mq-vpu-ctrl", "syscon"; > + reg = <0x38320000 0x10000>; > + }; > + > + 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>; > + 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>; > @@ -1350,9 +1351,33 @@ vpu: video-codec@38300000 { > <&clk IMX8MQ_VPU_PLL_OUT>, > <&clk IMX8MQ_SYS1_PLL_800M>, > <&clk IMX8MQ_VPU_PLL>; > - assigned-clock-rates = <600000000>, <600000000>, > + assigned-clock-rates = <600000000>, <300000000>, I'd like to see this mentioned in the commit message. > + <800000000>, <0>; > + power-domains = <&pgc_vpu>; > + nxp,imx8mq-vpu-ctrl = <&vpu_ctrl>; > + }; > + > + vpu_g2: video-codec@38310000 { > + compatible = "nxp,imx8mq-vpu-g2"; > + reg = <0x38310000 0x10000>; > + 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"; > + assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>, Can the G1 clock configuration be dropped from the G2 device node and the G2 clock configuration from the G1 device node? It looks weird that these devices configure each other's clocks. regards Philipp
Le 26/03/2021 à 15:24, Philipp Zabel a écrit : > On Thu, Mar 18, 2021 at 09:20:46AM +0100, Benjamin Gaignard wrote: >> Split VPU node in two: one for G1 and one for G2 since they are >> different hardware blocks. >> Add syscon for hardware control block. >> Remove reg-names property that is useless. >> Each VPU node only need one interrupt. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> version 5: >> - use syscon instead of VPU reset >> >> arch/arm64/boot/dts/freescale/imx8mq.dtsi | 43 ++++++++++++++++++----- >> 1 file changed, 34 insertions(+), 9 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi >> index 17c449e12c2e..b537d153ebbd 100644 >> --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi >> +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi >> @@ -1329,15 +1329,16 @@ usb3_phy1: usb-phy@382f0040 { >> status = "disabled"; >> }; >> >> - vpu: video-codec@38300000 { >> + vpu_ctrl: syscon@38320000 { >> + compatible = "nxp,imx8mq-vpu-ctrl", "syscon"; >> + reg = <0x38320000 0x10000>; >> + }; >> + >> + 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>; >> + 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>; >> @@ -1350,9 +1351,33 @@ vpu: video-codec@38300000 { >> <&clk IMX8MQ_VPU_PLL_OUT>, >> <&clk IMX8MQ_SYS1_PLL_800M>, >> <&clk IMX8MQ_VPU_PLL>; >> - assigned-clock-rates = <600000000>, <600000000>, >> + assigned-clock-rates = <600000000>, <300000000>, > I'd like to see this mentioned in the commit message. Yes I would do that. The value comes from the datasheet. > >> + <800000000>, <0>; >> + power-domains = <&pgc_vpu>; >> + nxp,imx8mq-vpu-ctrl = <&vpu_ctrl>; >> + }; >> + >> + vpu_g2: video-codec@38310000 { >> + compatible = "nxp,imx8mq-vpu-g2"; >> + reg = <0x38310000 0x10000>; >> + 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"; >> + assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>, > Can the G1 clock configuration be dropped from the G2 device node and > the G2 clock configuration from the G1 device node? It looks weird that > these devices configure each other's clocks. No because if only one device node is enabled we need to configure the both clocks anyway. Benjamin > > regards > Philipp >
On Fri, 2021-03-26 at 15:33 +0100, Benjamin Gaignard wrote: > > Le 26/03/2021 à 15:24, Philipp Zabel a écrit : > > On Thu, Mar 18, 2021 at 09:20:46AM +0100, Benjamin Gaignard wrote: > > > Split VPU node in two: one for G1 and one for G2 since they are > > > different hardware blocks. > > > Add syscon for hardware control block. > > > Remove reg-names property that is useless. > > > Each VPU node only need one interrupt. > > > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > > > --- > > > version 5: > > > - use syscon instead of VPU reset > > > > > > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 43 ++++++++++++++++++----- > > > 1 file changed, 34 insertions(+), 9 deletions(-) > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > index 17c449e12c2e..b537d153ebbd 100644 > > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > @@ -1329,15 +1329,16 @@ usb3_phy1: usb-phy@382f0040 { > > > status = "disabled"; > > > }; > > > > > > - vpu: video-codec@38300000 { > > > + vpu_ctrl: syscon@38320000 { > > > + compatible = "nxp,imx8mq-vpu-ctrl", "syscon"; > > > + reg = <0x38320000 0x10000>; > > > + }; > > > + > > > + 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>; > > > + 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>; > > > @@ -1350,9 +1351,33 @@ vpu: video-codec@38300000 { > > > <&clk IMX8MQ_VPU_PLL_OUT>, > > > <&clk IMX8MQ_SYS1_PLL_800M>, > > > <&clk IMX8MQ_VPU_PLL>; > > > - assigned-clock-rates = <600000000>, <600000000>, > > > + assigned-clock-rates = <600000000>, <300000000>, > > I'd like to see this mentioned in the commit message. > > Yes I would do that. > The value comes from the datasheet. > > > > > > + <800000000>, <0>; > > > + power-domains = <&pgc_vpu>; > > > + nxp,imx8mq-vpu-ctrl = <&vpu_ctrl>; > > > + }; > > > + > > > + vpu_g2: video-codec@38310000 { > > > + compatible = "nxp,imx8mq-vpu-g2"; > > > + reg = <0x38310000 0x10000>; > > > + 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"; > > > + assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>, > > Can the G1 clock configuration be dropped from the G2 device node and > > the G2 clock configuration from the G1 device node? It looks weird that > > these devices configure each other's clocks. > > No because if only one device node is enabled we need to configure the both > clocks anyway. > Since this is akward, how about adding a comment here in the dtsi to clarify it? Thanks, Ezequiel