| Message ID | 20210301151754.104749-1-benjamin.gaignard@collabora.com |
|---|---|
| Headers | show |
| Series | Reset driver for IMX8MQ VPU hardware block | expand |
Hi Benjamin, On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote: > The two VPUs inside IMX8MQ share the same control block which can be see > as a reset hardware block. This isn't a reset controller though. The control block also contains clock gates of some sort and a filter register for the featureset fuses. Those shouldn't be manipulated via the reset API. > In order to be able to add the second VPU (for HECV decoding) it will be > more handy if the both VPU drivers instance don't have to share the > control block registers. This lead to implement it as an independ reset > driver and to change the VPU driver to use it. Why not switch to a syscon regmap for the control block? That should also allow to keep backwards compatibility with the old binding with minimal effort. > Please note that this series break the compatibility between the DTB and > kernel. This break is limited to IMX8MQ SoC and is done when the driver > is still in staging directory. I know in this case we are pretty sure there are no users of this binding except for a staging driver, but it would still be nice to keep support for the deprecated binding, to avoid the requirement of updating kernel and DT in lock-step. regards Philipp
On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote: > Rather use a reset like feature inside the driver use the reset > controller API to get the same result. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > drivers/staging/media/hantro/Kconfig | 1 + > drivers/staging/media/hantro/imx8m_vpu_hw.c | 61 ++++----------------- > 2 files changed, 12 insertions(+), 50 deletions(-) > > diff --git a/drivers/staging/media/hantro/Kconfig b/drivers/staging/media/hantro/Kconfig > index 5b6cf9f62b1a..dd1d4dde2658 100644 > --- a/drivers/staging/media/hantro/Kconfig > +++ b/drivers/staging/media/hantro/Kconfig > @@ -20,6 +20,7 @@ config VIDEO_HANTRO_IMX8M > bool "Hantro VPU i.MX8M support" > depends on VIDEO_HANTRO > depends on ARCH_MXC || COMPILE_TEST > + select RESET_VPU_IMX8MQ > default y > help > Enable support for i.MX8M SoCs. > diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c > index c222de075ef4..d5b4312b9391 100644 > --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c > +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c > @@ -7,49 +7,12 @@ > > #include <linux/clk.h> > #include <linux/delay.h> > +#include <linux/reset.h> > > #include "hantro.h" > #include "hantro_jpeg.h" > #include "hantro_g1_regs.h" > > -#define CTRL_SOFT_RESET 0x00 > -#define RESET_G1 BIT(1) > -#define RESET_G2 BIT(0) > - > -#define CTRL_CLOCK_ENABLE 0x04 > -#define CLOCK_G1 BIT(1) > -#define CLOCK_G2 BIT(0) > - > -#define CTRL_G1_DEC_FUSE 0x08 > -#define CTRL_G1_PP_FUSE 0x0c > -#define CTRL_G2_DEC_FUSE 0x10 > - > -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); > - > - udelay(2); > - > - /* Release */ > - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET); > - val |= reset_bits; > - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET); > -} > - > -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); The way it is implemented in the reset driver, the clocks are now ungated between assert and deassert instead of afterwards. Is this on purpose? regards Philipp
Le 03/03/2021 à 15:39, Philipp Zabel a écrit : > On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote: >> Rather use a reset like feature inside the driver use the reset >> controller API to get the same result. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> drivers/staging/media/hantro/Kconfig | 1 + >> drivers/staging/media/hantro/imx8m_vpu_hw.c | 61 ++++----------------- >> 2 files changed, 12 insertions(+), 50 deletions(-) >> >> diff --git a/drivers/staging/media/hantro/Kconfig b/drivers/staging/media/hantro/Kconfig >> index 5b6cf9f62b1a..dd1d4dde2658 100644 >> --- a/drivers/staging/media/hantro/Kconfig >> +++ b/drivers/staging/media/hantro/Kconfig >> @@ -20,6 +20,7 @@ config VIDEO_HANTRO_IMX8M >> bool "Hantro VPU i.MX8M support" >> depends on VIDEO_HANTRO >> depends on ARCH_MXC || COMPILE_TEST >> + select RESET_VPU_IMX8MQ >> default y >> help >> Enable support for i.MX8M SoCs. >> diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c >> index c222de075ef4..d5b4312b9391 100644 >> --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c >> +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c >> @@ -7,49 +7,12 @@ >> >> #include <linux/clk.h> >> #include <linux/delay.h> >> +#include <linux/reset.h> >> >> #include "hantro.h" >> #include "hantro_jpeg.h" >> #include "hantro_g1_regs.h" >> >> -#define CTRL_SOFT_RESET 0x00 >> -#define RESET_G1 BIT(1) >> -#define RESET_G2 BIT(0) >> - >> -#define CTRL_CLOCK_ENABLE 0x04 >> -#define CLOCK_G1 BIT(1) >> -#define CLOCK_G2 BIT(0) >> - >> -#define CTRL_G1_DEC_FUSE 0x08 >> -#define CTRL_G1_PP_FUSE 0x0c >> -#define CTRL_G2_DEC_FUSE 0x10 >> - >> -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); >> - >> - udelay(2); >> - >> - /* Release */ >> - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET); >> - val |= reset_bits; >> - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET); >> -} >> - >> -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); > The way it is implemented in the reset driver, the clocks are now > ungated between assert and deassert instead of afterwards. Is this on > purpose? No and that could be changed on next version. Benjamin > > regards > Philipp >
Le 03/03/2021 à 15:17, Philipp Zabel a écrit : > Hi Benjamin, > > On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote: >> The two VPUs inside IMX8MQ share the same control block which can be see >> as a reset hardware block. > This isn't a reset controller though. The control block also contains > clock gates of some sort and a filter register for the featureset fuses. > Those shouldn't be manipulated via the reset API. They are all part of the control block and of the reset process for this hardware that why I put them here. I guess it is border line :-) > >> In order to be able to add the second VPU (for HECV decoding) it will be >> more handy if the both VPU drivers instance don't have to share the >> control block registers. This lead to implement it as an independ reset >> driver and to change the VPU driver to use it. > Why not switch to a syscon regmap for the control block? That should > also allow to keep backwards compatibility with the old binding with > minimal effort. I will give a try in this direction. > >> Please note that this series break the compatibility between the DTB and >> kernel. This break is limited to IMX8MQ SoC and is done when the driver >> is still in staging directory. > I know in this case we are pretty sure there are no users of this > binding except for a staging driver, but it would still be nice to keep > support for the deprecated binding, to avoid the requirement of updating > kernel and DT in lock-step. If I want to use a syscon (or a reset) the driver must not ioremap the "ctrl" registers. It means that "ctrl" has to be removed from the driver requested reg-names (imx8mq_reg_names[]). Doing that break the kernel/DT compatibility. Somehow syscon and "ctrl" are exclusive. Benjamin > > regards > Philipp >
On Wed, 2021-03-03 at 16:20 +0100, Benjamin Gaignard wrote: > Le 03/03/2021 à 15:17, Philipp Zabel a écrit : > > Hi Benjamin, > > > > On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote: > > > The two VPUs inside IMX8MQ share the same control block which can be see > > > as a reset hardware block. > > This isn't a reset controller though. The control block also contains > > clock gates of some sort and a filter register for the featureset fuses. > > Those shouldn't be manipulated via the reset API. > > They are all part of the control block and of the reset process for this > hardware that why I put them here. I guess it is border line :-) I'm pushing back to keep the reset control framework focused on controlling reset lines. Every side effect (such as the asymmetric clock ungating) in a random driver makes it harder to reason about behaviour at the API level, and to review patches for hardware I am not familiar with. > > > In order to be able to add the second VPU (for HECV decoding) it will be > > > more handy if the both VPU drivers instance don't have to share the > > > control block registers. This lead to implement it as an independ reset > > > driver and to change the VPU driver to use it. > > Why not switch to a syscon regmap for the control block? That should > > also allow to keep backwards compatibility with the old binding with > > minimal effort. > > I will give a try in this direction. Thank you. > > > Please note that this series break the compatibility between the DTB and > > > kernel. This break is limited to IMX8MQ SoC and is done when the driver > > > is still in staging directory. > > I know in this case we are pretty sure there are no users of this > > binding except for a staging driver, but it would still be nice to keep > > support for the deprecated binding, to avoid the requirement of updating > > kernel and DT in lock-step. > > If I want to use a syscon (or a reset) the driver must not ioremap the "ctrl" > registers. It means that "ctrl" has to be removed from the driver requested > reg-names (imx8mq_reg_names[]). Doing that break the kernel/DT compatibility. > Somehow syscon and "ctrl" are exclusive. The way the driver is set up currently, yes. You could add a bit of platform specific probe code, though, that would set up the regmap either by calling syscon_regmap_lookup_by_phandle(); for the new binding, or, if the phandle is not available, fall back to platform_get_resource_byname(..., "ctrl"); devm_ioremap_resource(); devm_regmap_init_mmio(); for the old binding. The actual codec .reset and variant .runtime_resume ops could be identical then. regards Philipp
On Wed, Mar 3, 2021 at 5:24 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > On Wed, 2021-03-03 at 16:20 +0100, Benjamin Gaignard wrote: > > Le 03/03/2021 à 15:17, Philipp Zabel a écrit : > > > Hi Benjamin, > > > > > > On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote: > > > > The two VPUs inside IMX8MQ share the same control block which can be see > > > > as a reset hardware block. > > > This isn't a reset controller though. The control block also contains > > > clock gates of some sort and a filter register for the featureset fuses. > > > Those shouldn't be manipulated via the reset API. This driver is very similar to several other patches for clk_blk control [1] which contain both resets and clock-enables on the i.MX8MP, i.MX8MM and i.MX8MN. In those cases, there are some specific power domain controls that are needed, but I wonder if the approach to creating resets and clock enables could be used in a similar way if the IMX8MQ doesn't have the same quirks. In the case of the i.MX8M Mini, I think it has the same VPU. [1] - https://patchwork.kernel.org/project/linux-clk/patch/1599560691-3763-12-git-send-email-abel.vesa@nxp.com/ adam > > > > They are all part of the control block and of the reset process for this > > hardware that why I put them here. I guess it is border line :-) > > I'm pushing back to keep the reset control framework focused on > controlling reset lines. Every side effect (such as the asymmetric clock > ungating) in a random driver makes it harder to reason about behaviour > at the API level, and to review patches for hardware I am not familiar > with. > > > > > In order to be able to add the second VPU (for HECV decoding) it will be > > > > more handy if the both VPU drivers instance don't have to share the > > > > control block registers. This lead to implement it as an independ reset > > > > driver and to change the VPU driver to use it. > > > Why not switch to a syscon regmap for the control block? That should > > > also allow to keep backwards compatibility with the old binding with > > > minimal effort. > > > > I will give a try in this direction. > > Thank you. > > > > > Please note that this series break the compatibility between the DTB and > > > > kernel. This break is limited to IMX8MQ SoC and is done when the driver > > > > is still in staging directory. > > > I know in this case we are pretty sure there are no users of this > > > binding except for a staging driver, but it would still be nice to keep > > > support for the deprecated binding, to avoid the requirement of updating > > > kernel and DT in lock-step. > > > > If I want to use a syscon (or a reset) the driver must not ioremap the "ctrl" > > registers. It means that "ctrl" has to be removed from the driver requested > > reg-names (imx8mq_reg_names[]). Doing that break the kernel/DT compatibility. > > Somehow syscon and "ctrl" are exclusive. > > The way the driver is set up currently, yes. You could add a bit of > platform specific probe code, though, that would set up the regmap > either by calling > syscon_regmap_lookup_by_phandle(); > for the new binding, or, if the phandle is not available, fall back to > platform_get_resource_byname(..., "ctrl"); > devm_ioremap_resource(); > devm_regmap_init_mmio(); > for the old binding. > The actual codec .reset and variant .runtime_resume ops could be > identical then. > > regards > Philipp > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Le 03/03/2021 à 17:25, Philipp Zabel a écrit : > On Wed, 2021-03-03 at 16:20 +0100, Benjamin Gaignard wrote: >> Le 03/03/2021 à 15:17, Philipp Zabel a écrit : >>> Hi Benjamin, >>> >>> On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote: >>>> The two VPUs inside IMX8MQ share the same control block which can be see >>>> as a reset hardware block. >>> This isn't a reset controller though. The control block also contains >>> clock gates of some sort and a filter register for the featureset fuses. >>> Those shouldn't be manipulated via the reset API. >> They are all part of the control block and of the reset process for this >> hardware that why I put them here. I guess it is border line :-) > I'm pushing back to keep the reset control framework focused on > controlling reset lines. Every side effect (such as the asymmetric clock > ungating) in a random driver makes it harder to reason about behaviour > at the API level, and to review patches for hardware I am not familiar > with. > >>>> In order to be able to add the second VPU (for HECV decoding) it will be >>>> more handy if the both VPU drivers instance don't have to share the >>>> control block registers. This lead to implement it as an independ reset >>>> driver and to change the VPU driver to use it. >>> Why not switch to a syscon regmap for the control block? That should >>> also allow to keep backwards compatibility with the old binding with >>> minimal effort. >> I will give a try in this direction. > Thank you. > >>>> Please note that this series break the compatibility between the DTB and >>>> kernel. This break is limited to IMX8MQ SoC and is done when the driver >>>> is still in staging directory. >>> I know in this case we are pretty sure there are no users of this >>> binding except for a staging driver, but it would still be nice to keep >>> support for the deprecated binding, to avoid the requirement of updating >>> kernel and DT in lock-step. >> If I want to use a syscon (or a reset) the driver must not ioremap the "ctrl" >> registers. It means that "ctrl" has to be removed from the driver requested >> reg-names (imx8mq_reg_names[]). Doing that break the kernel/DT compatibility. >> Somehow syscon and "ctrl" are exclusive. > The way the driver is set up currently, yes. You could add a bit of > platform specific probe code, though, that would set up the regmap > either by calling > syscon_regmap_lookup_by_phandle(); > for the new binding, or, if the phandle is not available, fall back to > platform_get_resource_byname(..., "ctrl"); > devm_ioremap_resource(); > devm_regmap_init_mmio(); > for the old binding. > The actual codec .reset and variant .runtime_resume ops could be > identical then. I made it works with syscon and your proposal. The next version of the patches will be without reset and won't break DT compatibility. Thanks for your help, Benjamin > > regards > Philipp >
On Mon, Mar 01, 2021 at 04:17:49PM +0100, Benjamin Gaignard wrote: > The two VPUs inside IMX8MQ share the same control block which can be see > as a reset hardware block. > In order to be able to add the second VPU (for HECV decoding) it will be > more handy if the both VPU drivers instance don't have to share the > control block registers. This lead to implement it as an independ reset > driver and to change the VPU driver to use it. > > Please note that this series break the compatibility between the DTB and > kernel. This break is limited to IMX8MQ SoC and is done when the driver > is still in staging directory. As this information will be lost, please put in the binding and dts patch. > > version 3: > - Fix error in VPU example node > > version 2: > - Document the change in VPU bindings > > Benjamin Gaignard (5): > dt-bindings: reset: IMX8MQ VPU reset > dt-bindings: media: IMX8MQ VPU: document reset usage > reset: Add reset driver for IMX8MQ VPU block > media: hantro: Use reset driver > arm64: dts: imx8mq: Use reset driver for VPU hardware block > > .../bindings/media/nxp,imx8mq-vpu.yaml | 14 +- > .../bindings/reset/fsl,imx8mq-vpu-reset.yaml | 54 ++++++ > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 31 +++- > drivers/reset/Kconfig | 8 + > drivers/reset/Makefile | 1 + > drivers/reset/reset-imx8mq-vpu.c | 169 ++++++++++++++++++ > drivers/staging/media/hantro/Kconfig | 1 + > drivers/staging/media/hantro/imx8m_vpu_hw.c | 61 ++----- > include/dt-bindings/reset/imx8mq-vpu-reset.h | 16 ++ > 9 files changed, 294 insertions(+), 61 deletions(-) > create mode 100644 Documentation/devicetree/bindings/reset/fsl,imx8mq-vpu-reset.yaml > create mode 100644 drivers/reset/reset-imx8mq-vpu.c > create mode 100644 include/dt-bindings/reset/imx8mq-vpu-reset.h > > -- > 2.25.1 >
On Mon, Mar 08, 2021 at 11:22:17AM -0700, Rob Herring wrote: > On Mon, Mar 01, 2021 at 04:17:49PM +0100, Benjamin Gaignard wrote: > > The two VPUs inside IMX8MQ share the same control block which can be see > > as a reset hardware block. > > In order to be able to add the second VPU (for HECV decoding) it will be > > more handy if the both VPU drivers instance don't have to share the > > control block registers. This lead to implement it as an independ reset > > driver and to change the VPU driver to use it. > > > > Please note that this series break the compatibility between the DTB and > > kernel. This break is limited to IMX8MQ SoC and is done when the driver > > is still in staging directory. > > As this information will be lost, please put in the binding and dts > patch. Actually, the adding the VPU reset binding doesn't break compatibility, so just the dts file changes needs it. > > > > > version 3: > > - Fix error in VPU example node > > > > version 2: > > - Document the change in VPU bindings > > > > Benjamin Gaignard (5): > > dt-bindings: reset: IMX8MQ VPU reset > > dt-bindings: media: IMX8MQ VPU: document reset usage > > reset: Add reset driver for IMX8MQ VPU block > > media: hantro: Use reset driver > > arm64: dts: imx8mq: Use reset driver for VPU hardware block > > > > .../bindings/media/nxp,imx8mq-vpu.yaml | 14 +- > > .../bindings/reset/fsl,imx8mq-vpu-reset.yaml | 54 ++++++ > > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 31 +++- > > drivers/reset/Kconfig | 8 + > > drivers/reset/Makefile | 1 + > > drivers/reset/reset-imx8mq-vpu.c | 169 ++++++++++++++++++ > > drivers/staging/media/hantro/Kconfig | 1 + > > drivers/staging/media/hantro/imx8m_vpu_hw.c | 61 ++----- > > include/dt-bindings/reset/imx8mq-vpu-reset.h | 16 ++ > > 9 files changed, 294 insertions(+), 61 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/reset/fsl,imx8mq-vpu-reset.yaml > > create mode 100644 drivers/reset/reset-imx8mq-vpu.c > > create mode 100644 include/dt-bindings/reset/imx8mq-vpu-reset.h > > > > -- > > 2.25.1 > >
On Mon, 01 Mar 2021 16:17:51 +0100, Benjamin Gaignard wrote: > Document IMX8MQ VPU bindings to add the phandle to the reset driver. > > Provide an independent reset driver allow to the both VPUs to share > their control/reset hardware block. The reset driver replace what > was previously done be using the 'ctrl' registers inside the driver. > > This breaks the compatibility between DTB and kernel but the driver > is still in staging directory and limited to IMX8MQ SoC. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > version 3: > - Fix error in VPU example node > > .../devicetree/bindings/media/nxp,imx8mq-vpu.yaml | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > Reviewed-by: Rob Herring <robh@kernel.org>