| Message ID | 20210301151754.104749-1-benjamin.gaignard@collabora.com (mailing list archive) |
|---|---|
| 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
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 > >