From: Benjamin Gaignard <benjamin.gaignard@collabora.com> Date: 2021-03-01 15:19:52
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Benjamin Gaignard <benjamin.gaignard@collabora.com> Date: 2021-03-01 15:20:15
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(-)
@@ -0,0 +1,54 @@+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)+%YAML1.2+---+$id:http://devicetree.org/schemas/reset/fsl,imx8mq-vpu-reset.yaml#+$schema:http://devicetree.org/meta-schemas/core.yaml#++title:Freescale i.MX8MQ VPU Reset Controller++maintainers:+-Benjamin Gaignard <benjamin.gaignard@collabora.com>++description:|+The VPU reset controller is used to reset the video processor+unit peripherals. Device nodes that need access to reset lines should+specify them as a reset phandle in their corresponding node as+specified in reset.txt.++For list of all valid reset indices see+<dt-bindings/reset/imx8mq-vpu-reset.h> for i.MX8MQ.++properties:+compatible:+items:+-const:fsl,imx8mq-vpu-reset+-const:syscon++reg:+maxItems:1++clocks:+minItems:1+maxItems:3++'#reset-cells':+const:1++required:+-compatible+-reg+-clocks+-'#reset-cells'++additionalProperties:false++examples:+-|+#include <dt-bindings/clock/imx8mq-clock.h>++vpu-reset@38320000 {+compatible = "fsl,imx8mq-vpu-reset", "syscon";+reg = <0x38320000 0x10000>;+clocks = <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;+#reset-cells = <1>;+};
From: Benjamin Gaignard <benjamin.gaignard@collabora.com> Date: 2021-03-01 15:22:07
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(-)
@@ -60,13 +23,10 @@ static int imx8mq_runtime_resume(struct hantro_dev *vpu)returnret;}-imx8m_soft_reset(vpu,RESET_G1|RESET_G2);-imx8m_clk_enable(vpu,CLOCK_G1|CLOCK_G2);+ret=device_reset(vpu->dev);+if(ret)+dev_err(vpu->dev,"Failed to reset Hantro VPU\n");-/* 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);clk_bulk_disable_unprepare(vpu->variant->num_clocks,vpu->clocks);
From: Philipp Zabel <p.zabel@pengutronix.de> Date: 2021-03-03 18:23:16
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Philipp Zabel <p.zabel@pengutronix.de> Date: 2021-03-03 18:40:11
On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote:
quoted hunk
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(-)
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Benjamin Gaignard <benjamin.gaignard@collabora.com> Date: 2021-03-03 18:40:49
Le 03/03/2021 à 15:39, Philipp Zabel a écrit :
On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote:
quoted
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(-)
From: Benjamin Gaignard <benjamin.gaignard@collabora.com> Date: 2021-03-03 18:42:35
Le 03/03/2021 à 15:17, Philipp Zabel a écrit :
Hi Benjamin,
On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote:
quoted
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 :-)
quoted
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.
quoted
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
Also I assume that only the VPU_DEC_ROOT clock is required to control
these registers. Enabling the VPU_G1_ROOT and VPU_G2_ROOT clocks
(presumably to make sure the resets propagate into the respective VPU
core) would be the reset consumer's responsibility.
regards
Philipp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Philipp Zabel <p.zabel@pengutronix.de> Date: 2021-03-03 19:09:31
On Wed, 2021-03-03 at 16:20 +0100, Benjamin Gaignard wrote:
Le 03/03/2021 à 15:17, Philipp Zabel a écrit :
quoted
Hi Benjamin,
On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote:
quoted
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.
quoted
quoted
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.
quoted
quoted
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
From: Adam Ford <hidden> Date: 2021-03-04 12:54:10
On Wed, Mar 3, 2021 at 5:24 PM Philipp Zabel [off-list ref] wrote:
On Wed, 2021-03-03 at 16:20 +0100, Benjamin Gaignard wrote:
quoted
Le 03/03/2021 à 15:17, Philipp Zabel a écrit :
quoted
Hi Benjamin,
On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote:
quoted
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
quoted
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.
quoted
quoted
quoted
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.
quoted
quoted
quoted
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
From: Benjamin Gaignard <benjamin.gaignard@collabora.com> Date: 2021-03-05 09:36:52
Le 03/03/2021 à 17:25, Philipp Zabel a écrit :
On Wed, 2021-03-03 at 16:20 +0100, Benjamin Gaignard wrote:
quoted
Le 03/03/2021 à 15:17, Philipp Zabel a écrit :
quoted
Hi Benjamin,
On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote:
quoted
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.
quoted
quoted
quoted
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.
quoted
quoted
quoted
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
From: Rob Herring <robh@kernel.org> Date: 2021-03-08 18:23:03
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.
@@ -0,0 +1,54 @@+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)+%YAML1.2+---+$id:http://devicetree.org/schemas/reset/fsl,imx8mq-vpu-reset.yaml#+$schema:http://devicetree.org/meta-schemas/core.yaml#++title:Freescale i.MX8MQ VPU Reset Controller++maintainers:+-Benjamin Gaignard <benjamin.gaignard@collabora.com>++description:|+The VPU reset controller is used to reset the video processor+unit peripherals. Device nodes that need access to reset lines should+specify them as a reset phandle in their corresponding node as+specified in reset.txt.++For list of all valid reset indices see+<dt-bindings/reset/imx8mq-vpu-reset.h> for i.MX8MQ.++properties:+compatible:+items:+-const:fsl,imx8mq-vpu-reset+-const:syscon
Is there other functionality in the block? If so, add some details in
'description' above.
From: Rob Herring <robh@kernel.org> Date: 2021-03-08 18:27:52
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(-)
From: Rob Herring <robh@kernel.org> Date: 2021-03-08 18:27:52
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:
quoted
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.