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

regards
Philipp

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip