[PATCH] soc: imx: gpcv2: keep i.MX8MM VPU-H1 bus clock active

[PATCH] soc: imx: gpcv2: keep i.MX8MM VPU-H1 bus clock active

[Adam Ford]

Enable the vpu-h1 clock when the domain is active because reading
or writing to the VPU-H1 IP block cause the system to hang.

Fixes: 656ade7aa42a ("soc: imx: gpcv2: keep i.MX8M* bus clocks enabled")
Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index b8d52d8d29db..7b6dfa33dcb9 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -734,6 +734,7 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
 			.map = IMX8MM_VPUH1_A53_DOMAIN,
 		},
 		.pgc   = BIT(IMX8MM_PGC_VPUH1),
+		.keep_clocks = true,
 	},
 
 	[IMX8MM_POWER_DOMAIN_DISPMIX] = {
-- 
2.32.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: imx: gpcv2: keep i.MX8MM VPU-H1 bus clock active

[Fabio Estevam]

Hi Adam,

On Sat, Nov 20, 2021 at 4:39 PM Adam Ford <aford173@gmail.com> wrote:
>
> Enable the vpu-h1 clock when the domain is active because reading
> or writing to the VPU-H1 IP block cause the system to hang.
>
> Fixes: 656ade7aa42a ("soc: imx: gpcv2: keep i.MX8M* bus clocks enabled")
> Signed-off-by: Adam Ford <aford173@gmail.com>

This makes sense:

Reviewed-by: Fabio Estevam <festevam@gmail.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: imx: gpcv2: keep i.MX8MM VPU-H1 bus clock active

[Shawn Guo]

On Sat, Nov 20, 2021 at 01:39:16PM -0600, Adam Ford wrote:
> Enable the vpu-h1 clock when the domain is active because reading
> or writing to the VPU-H1 IP block cause the system to hang.
> 
> Fixes: 656ade7aa42a ("soc: imx: gpcv2: keep i.MX8M* bus clocks enabled")
> Signed-off-by: Adam Ford <aford173@gmail.com>

Applied, thanks!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: imx: gpcv2: keep i.MX8MM VPU-H1 bus clock active

[Lucas Stach]

Hi Adam, hi Shawn,

Am Samstag, dem 20.11.2021 um 13:39 -0600 schrieb Adam Ford:
> Enable the vpu-h1 clock when the domain is active because reading
> or writing to the VPU-H1 IP block cause the system to hang.
> 
> Fixes: 656ade7aa42a ("soc: imx: gpcv2: keep i.MX8M* bus clocks enabled")
> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> index b8d52d8d29db..7b6dfa33dcb9 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -734,6 +734,7 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
>  			.map = IMX8MM_VPUH1_A53_DOMAIN,
>  		},
>  		.pgc   = BIT(IMX8MM_PGC_VPUH1),
> +		.keep_clocks = true,
>  	},
> 
I missed this patch and just stumbled across it when looking at the git
history. I don't think this patch is correct. The H1 GPC domain does
not even have clocks assigned in the DT, so there is nothing to keep
active. Also H1 is not a MIX domain, so it should not keep any bus
clocks active, that is the job of the VPUMIX domain.

While this patch is a no-op, as far as I can see, it still seems wrong
and I think it should be reverted.

Regards,
Lucas


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: imx: gpcv2: keep i.MX8MM VPU-H1 bus clock active

[Adam Ford]

On Thu, Mar 31, 2022 at 4:28 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi Adam, hi Shawn,
>
> Am Samstag, dem 20.11.2021 um 13:39 -0600 schrieb Adam Ford:
> > Enable the vpu-h1 clock when the domain is active because reading
> > or writing to the VPU-H1 IP block cause the system to hang.
> >
> > Fixes: 656ade7aa42a ("soc: imx: gpcv2: keep i.MX8M* bus clocks enabled")
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > index b8d52d8d29db..7b6dfa33dcb9 100644
> > --- a/drivers/soc/imx/gpcv2.c
> > +++ b/drivers/soc/imx/gpcv2.c
> > @@ -734,6 +734,7 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
> >                       .map = IMX8MM_VPUH1_A53_DOMAIN,
> >               },
> >               .pgc   = BIT(IMX8MM_PGC_VPUH1),
> > +             .keep_clocks = true,
> >       },
> >
> I missed this patch and just stumbled across it when looking at the git
> history. I don't think this patch is correct. The H1 GPC domain does
> not even have clocks assigned in the DT, so there is nothing to keep
> active. Also H1 is not a MIX domain, so it should not keep any bus
> clocks active, that is the job of the VPUMIX domain.
>
> While this patch is a no-op, as far as I can see, it still seems wrong
> and I think it should be reverted.

At the time I sent this, I was working with some people in the media
group to split the G1 and G2 up in the imx8mq and add G1 and G2
support in the imx8mm.  I had inquired about the feasibility of using
the H1 encoder on the imx8mm, but I needed to read some registers from
the IP block to see which features were fused out.  I tried several
different options to get the H1 to not hang when reading registers,
and that was the only solution I found that worked.  I thought it odd
as well since the G1 and G2 decoders didn't appear to need this.
However, during the course of my investigation, I learned that the
JPEG encoder was fused out of the imx8mm, and there wasn't a plan to
add VP8 or H.264 encodering any time soon.   Since it is, as you put
it, a no-op, I have no objections to reverting it.

adam
>
> Regards,
> Lucas
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: imx: gpcv2: keep i.MX8MM VPU-H1 bus clock active

[Shawn Guo]

On Thu, Mar 31, 2022 at 06:32:13AM -0500, Adam Ford wrote:
> On Thu, Mar 31, 2022 at 4:28 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> >
> > Hi Adam, hi Shawn,
> >
> > Am Samstag, dem 20.11.2021 um 13:39 -0600 schrieb Adam Ford:
> > > Enable the vpu-h1 clock when the domain is active because reading
> > > or writing to the VPU-H1 IP block cause the system to hang.
> > >
> > > Fixes: 656ade7aa42a ("soc: imx: gpcv2: keep i.MX8M* bus clocks enabled")
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > >
> > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > > index b8d52d8d29db..7b6dfa33dcb9 100644
> > > --- a/drivers/soc/imx/gpcv2.c
> > > +++ b/drivers/soc/imx/gpcv2.c
> > > @@ -734,6 +734,7 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
> > >                       .map = IMX8MM_VPUH1_A53_DOMAIN,
> > >               },
> > >               .pgc   = BIT(IMX8MM_PGC_VPUH1),
> > > +             .keep_clocks = true,
> > >       },
> > >
> > I missed this patch and just stumbled across it when looking at the git
> > history. I don't think this patch is correct. The H1 GPC domain does
> > not even have clocks assigned in the DT, so there is nothing to keep
> > active. Also H1 is not a MIX domain, so it should not keep any bus
> > clocks active, that is the job of the VPUMIX domain.
> >
> > While this patch is a no-op, as far as I can see, it still seems wrong
> > and I think it should be reverted.
> 
> At the time I sent this, I was working with some people in the media
> group to split the G1 and G2 up in the imx8mq and add G1 and G2
> support in the imx8mm.  I had inquired about the feasibility of using
> the H1 encoder on the imx8mm, but I needed to read some registers from
> the IP block to see which features were fused out.  I tried several
> different options to get the H1 to not hang when reading registers,
> and that was the only solution I found that worked.  I thought it odd
> as well since the G1 and G2 decoders didn't appear to need this.
> However, during the course of my investigation, I learned that the
> JPEG encoder was fused out of the imx8mm, and there wasn't a plan to
> add VP8 or H.264 encodering any time soon.   Since it is, as you put
> it, a no-op, I have no objections to reverting it.

I do not quite follow.  You claimed that the change fixes a system hang.
Are we getting the hang back if we revert the change?

Shawn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: imx: gpcv2: keep i.MX8MM VPU-H1 bus clock active

[Adam Ford]

On Tue, Apr 5, 2022 at 1:45 AM Shawn Guo <shawnguo@kernel.org> wrote:
>
> On Thu, Mar 31, 2022 at 06:32:13AM -0500, Adam Ford wrote:
> > On Thu, Mar 31, 2022 at 4:28 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > >
> > > Hi Adam, hi Shawn,
> > >
> > > Am Samstag, dem 20.11.2021 um 13:39 -0600 schrieb Adam Ford:
> > > > Enable the vpu-h1 clock when the domain is active because reading
> > > > or writing to the VPU-H1 IP block cause the system to hang.
> > > >
> > > > Fixes: 656ade7aa42a ("soc: imx: gpcv2: keep i.MX8M* bus clocks enabled")
> > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > >
> > > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > > > index b8d52d8d29db..7b6dfa33dcb9 100644
> > > > --- a/drivers/soc/imx/gpcv2.c
> > > > +++ b/drivers/soc/imx/gpcv2.c
> > > > @@ -734,6 +734,7 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
> > > >                       .map = IMX8MM_VPUH1_A53_DOMAIN,
> > > >               },
> > > >               .pgc   = BIT(IMX8MM_PGC_VPUH1),
> > > > +             .keep_clocks = true,
> > > >       },
> > > >
> > > I missed this patch and just stumbled across it when looking at the git
> > > history. I don't think this patch is correct. The H1 GPC domain does
> > > not even have clocks assigned in the DT, so there is nothing to keep
> > > active. Also H1 is not a MIX domain, so it should not keep any bus
> > > clocks active, that is the job of the VPUMIX domain.
> > >
> > > While this patch is a no-op, as far as I can see, it still seems wrong
> > > and I think it should be reverted.
> >
> > At the time I sent this, I was working with some people in the media
> > group to split the G1 and G2 up in the imx8mq and add G1 and G2
> > support in the imx8mm.  I had inquired about the feasibility of using
> > the H1 encoder on the imx8mm, but I needed to read some registers from
> > the IP block to see which features were fused out.  I tried several
> > different options to get the H1 to not hang when reading registers,
> > and that was the only solution I found that worked.  I thought it odd
> > as well since the G1 and G2 decoders didn't appear to need this.
> > However, during the course of my investigation, I learned that the
> > JPEG encoder was fused out of the imx8mm, and there wasn't a plan to
> > add VP8 or H.264 encodering any time soon.   Since it is, as you put
> > it, a no-op, I have no objections to reverting it.
>
> I do not quite follow.  You claimed that the change fixes a system hang.
> Are we getting the hang back if we revert the change?

It was hanging when I has trying to use the H1 VPU, and this did fix
it, but it seemed weird to me.  Unfortunately, Hantro H1 driver that
is currently in Linux only supports JPEG, and the H1 VPU in the imx8mm
has JPEG fused out.  As of right now, there is no H1 VPU consumer, so
there is nobody attempting to enable the H1 power domain, so it
remains a no-op.  If it's reverted, we'll have to revisit the issue in
the future to prevent the hanging, but that will also require a new
Hantro driver.

adam
>
> Shawn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: imx: gpcv2: keep i.MX8MM VPU-H1 bus clock active

[Lucas Stach]

Am Dienstag, dem 05.04.2022 um 04:55 -0500 schrieb Adam Ford:
> On Tue, Apr 5, 2022 at 1:45 AM Shawn Guo <shawnguo@kernel.org> wrote:
> > 
> > On Thu, Mar 31, 2022 at 06:32:13AM -0500, Adam Ford wrote:
> > > On Thu, Mar 31, 2022 at 4:28 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > 
> > > > Hi Adam, hi Shawn,
> > > > 
> > > > Am Samstag, dem 20.11.2021 um 13:39 -0600 schrieb Adam Ford:
> > > > > Enable the vpu-h1 clock when the domain is active because reading
> > > > > or writing to the VPU-H1 IP block cause the system to hang.
> > > > > 
> > > > > Fixes: 656ade7aa42a ("soc: imx: gpcv2: keep i.MX8M* bus clocks enabled")
> > > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > > 
> > > > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > > > > index b8d52d8d29db..7b6dfa33dcb9 100644
> > > > > --- a/drivers/soc/imx/gpcv2.c
> > > > > +++ b/drivers/soc/imx/gpcv2.c
> > > > > @@ -734,6 +734,7 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
> > > > >                       .map = IMX8MM_VPUH1_A53_DOMAIN,
> > > > >               },
> > > > >               .pgc   = BIT(IMX8MM_PGC_VPUH1),
> > > > > +             .keep_clocks = true,
> > > > >       },
> > > > > 
> > > > I missed this patch and just stumbled across it when looking at the git
> > > > history. I don't think this patch is correct. The H1 GPC domain does
> > > > not even have clocks assigned in the DT, so there is nothing to keep
> > > > active. Also H1 is not a MIX domain, so it should not keep any bus
> > > > clocks active, that is the job of the VPUMIX domain.
> > > > 
> > > > While this patch is a no-op, as far as I can see, it still seems wrong
> > > > and I think it should be reverted.
> > > 
> > > At the time I sent this, I was working with some people in the media
> > > group to split the G1 and G2 up in the imx8mq and add G1 and G2
> > > support in the imx8mm.  I had inquired about the feasibility of using
> > > the H1 encoder on the imx8mm, but I needed to read some registers from
> > > the IP block to see which features were fused out.  I tried several
> > > different options to get the H1 to not hang when reading registers,
> > > and that was the only solution I found that worked.  I thought it odd
> > > as well since the G1 and G2 decoders didn't appear to need this.
> > > However, during the course of my investigation, I learned that the
> > > JPEG encoder was fused out of the imx8mm, and there wasn't a plan to
> > > add VP8 or H.264 encodering any time soon.   Since it is, as you put
> > > it, a no-op, I have no objections to reverting it.
> > 
> > I do not quite follow.  You claimed that the change fixes a system hang.
> > Are we getting the hang back if we revert the change?
> 
> It was hanging when I has trying to use the H1 VPU, and this did fix
> it, but it seemed weird to me.  Unfortunately, Hantro H1 driver that
> is currently in Linux only supports JPEG, and the H1 VPU in the imx8mm
> has JPEG fused out.  As of right now, there is no H1 VPU consumer, so
> there is nobody attempting to enable the H1 power domain, so it
> remains a no-op.  If it's reverted, we'll have to revisit the issue in
> the future to prevent the hanging, but that will also require a new
> Hantro driver.

The H1 clock is not a bus clock, so there is nothing else that depends
on this clock than the H1 encoder peripheral, which is why it should be
handled by the future driver and not the power domain.

Regards,
Lucas


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel