Re: [PATCH v3 0/5] Reset driver for IMX8MQ VPU hardware block

[Adam Ford]

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 v9 02/13] dt-bindings: media: nxp, imx8mq-vpu: Update the bindings for G2 support

[Adam Ford]

On Wed, Apr 7, 2021 at 2:37 AM Benjamin Gaignard
 wrote:
>
> Introducing the G2 hevc video decoder requires modifications of the bindings 
> to allow
> one node per VPU.
>
> VPUs share one hardware control block which is provided as a phandle on
> a syscon.
> Each node has now one reg and one interrupt.
> Add a compatible for G2 hardware block: nxp,imx8mq-vpu-g2.
>
> To be compatible with older DT the driver is still capable to use the 'ctrl'
> reg-name even if it is deprecated now.
>
> Signed-off-by: Benjamin Gaignard 
> Reviewed-by: Rob Herring 
> Reviewed-by: Philipp Zabel 

I need to edit the yaml file to add support the imx8mm, but it doesn't
appear that this series has gone anywhere.  I know there is still some
waiting on the vpu-blk-ctrl driver, but it seems like the 8mq could
split the codecs out using syscon in place of the blk-ctrl until that
driver is available.  If that doesn't work, I might have to introduce
a separate yaml file for mini which could be somehow merged with the
8mq in the future.  I am just not sure which way to go right now.

adam
> ---
> version 9:
>  - Corrections in commit message
>
> version 7:
>  - Add Rob and Philipp reviewed-by tag
>  - Change syscon phandle name to nxp,imx8m-vpu-ctrl (remove 'q' to be
>usable for iMX8MM too)
>
> version 5:
> - This version doesn't break the backward compatibilty between kernel
>   and DT.
>
>  .../bindings/media/nxp,imx8mq-vpu.yaml| 53 ---
>  1 file changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml 
> b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> index 762be3f96ce9..18e7d40a5f24 100644
> --- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> +++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> @@ -15,22 +15,18 @@ description:
>
>  properties:
>compatible:
> -const: nxp,imx8mq-vpu
> +oneOf:
> +  - const: nxp,imx8mq-vpu
> +  - const: nxp,imx8mq-vpu-g2
>
>reg:
> -maxItems: 3
> -
> -  reg-names:
> -items:
> -  - const: g1
> -  - const: g2
> -  - const: ctrl
> +maxItems: 1
>
>interrupts:
> -maxItems: 2
> +maxItems: 1
>
>interrupt-names:
> -items:
> +oneOf:
>- const: g1
>- const: g2
>
> @@ -46,14 +42,18 @@ properties:
>power-domains:
>  maxItems: 1
>
> +  nxp,imx8m-vpu-ctrl:
> +description: Specifies a phandle to syscon VPU hardware control block
> +$ref: "/schemas/types.yaml#/definitions/phandle"
> +
>  required:
>- compatible
>- reg
> -  - reg-names
>- interrupts
>- interrupt-names
>- clocks
>- clock-names
> +  - nxp,imx8m-vpu-ctrl
>
>  additionalProperties: false
>
> @@ -62,18 +62,33 @@ examples:
>  #include 
>  #include 
>
> -vpu: video-codec@3830 {
> +vpu_ctrl: syscon@3832 {
> + compatible = "nxp,imx8mq-vpu-ctrl", "syscon";
> + reg = <0x3832 0x1>;
> +};
> +
> +vpu_g1: video-codec@3830 {
>  compatible = "nxp,imx8mq-vpu";
> -reg = <0x3830 0x1>,
> -  <0x3831 0x1>,
> -  <0x3832 0x1>;
> -reg-names = "g1", "g2", "ctrl";
> -interrupts = ,
> - ;
> -interrupt-names = "g1", "g2";
> +reg = <0x3830 0x1>;
> +interrupts = ;
> +interrupt-names = "g1";
> +clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
> + <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
> + <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
> +clock-names = "g1", "g2", "bus";
> +power-domains = <&pgc_vpu>;
> +nxp,imx8m-vpu-ctrl = <&vpu_ctrl>;
> +};
> +
> +vpu_g2: video-codec@3831 {
> +compatible = "nxp,imx8mq-vpu-g2";
> +reg = <0x3830 0x1>;
> +interrupts = ;
> +interrupt-names = "g2";
>  clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
>   <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
>   <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
>  clock-names = "g1", "g2", "bus";
>  power-domains = <&pgc_vpu>;
> +nxp,imx8m-vpu-ctrl = <&vpu_ctrl>;
>  };
> --
> 2.25.1
>
>
> ___
> 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