Re: [PATCH v3 0/5] Reset driver for IMX8MQ VPU hardware block
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 > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 0/5] Reset driver for IMX8MQ VPU hardware block
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 > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 0/5] Reset driver for IMX8MQ VPU hardware block
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 0/5] Reset driver for IMX8MQ VPU hardware block
On Wed, Mar 3, 2021 at 5:24 PM Philipp Zabel 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.v...@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-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 0/5] Reset driver for IMX8MQ VPU hardware block
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 0/5] Reset driver for IMX8MQ VPU hardware block
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 0/5] Reset driver for IMX8MQ VPU hardware block
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 0/5] Reset driver for IMX8MQ VPU hardware block
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. 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel