[PATCH 0/2] ARM: OMAP DSS hwmod reset fix

[PATCH 0/2] ARM: OMAP DSS hwmod reset fix

[Tomi Valkeinen]

Hi,

Here are similar patches for AM43xx and DRA7xxx than was done earlier for
OMAP4/5 in

543b2847d4bdb07eb1b50003095bc65cf2a1e2c0
 ("ARM: OMAP4: hwmod: set DSS submodule parent hwmods")

and

9ed69650897e8738c959fe8242ec41499f3f3f35
("ARM: OMAP5: hwmod: set DSS submodule parent hwmods")

I don't think missing the reset causes any problems, but the boot time warnings
are annoying.

 Tomi

Tomi Valkeinen (2):
  ARM: AM43xx: hwmod: set DSS submodule parent hwmods
  ARM: DRA7xx: hwmod: set DSS submodule parent hwmods

 arch/arm/mach-omap2/omap_hwmod_43xx_data.c | 2 ++
 arch/arm/mach-omap2/omap_hwmod_7xx_data.c  | 2 ++
 2 files changed, 4 insertions(+)

-- 
2.2.0

[PATCH 0/2] ARM: OMAP DSS hwmod reset fix

[Tomi Valkeinen]

Hi,

Here are similar patches for AM43xx and DRA7xxx than was done earlier for
OMAP4/5 in

543b2847d4bdb07eb1b50003095bc65cf2a1e2c0
 ("ARM: OMAP4: hwmod: set DSS submodule parent hwmods")

and

9ed69650897e8738c959fe8242ec41499f3f3f35
("ARM: OMAP5: hwmod: set DSS submodule parent hwmods")

I don't think missing the reset causes any problems, but the boot time warnings
are annoying.

 Tomi

Tomi Valkeinen (2):
  ARM: AM43xx: hwmod: set DSS submodule parent hwmods
  ARM: DRA7xx: hwmod: set DSS submodule parent hwmods

 arch/arm/mach-omap2/omap_hwmod_43xx_data.c | 2 ++
 arch/arm/mach-omap2/omap_hwmod_7xx_data.c  | 2 ++
 2 files changed, 4 insertions(+)

-- 
2.2.0

[PATCH 1/2] ARM: AM43xx: hwmod: set DSS submodule parent hwmods

[Tomi Valkeinen]

Set DSS core hwmod as the parent for all the DSS submodules. This fixes
the boot time DSS reset, removing the following warnings:

omap_hwmod: dss_dispc: cannot be enabled for reset (3)
omap_hwmod: dss_rfbi: cannot be enabled for reset (3)

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_43xx_data.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-omap2/omap_hwmod_43xx_data.c b/arch/arm/mach-omap2/omap_hwmod_43xx_data.c
index 5c6c8410160e..8eb85925e444 100644
--- a/arch/arm/mach-omap2/omap_hwmod_43xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_43xx_data.c
@@ -498,6 +498,7 @@ static struct omap_hwmod am43xx_dss_dispc_hwmod = {
 		},
 	},
 	.dev_attr	= &am43xx_dss_dispc_dev_attr,
+	.parent_hwmod	= &am43xx_dss_core_hwmod,
 };
 
 /* rfbi */
@@ -512,6 +513,7 @@ static struct omap_hwmod am43xx_dss_rfbi_hwmod = {
 			.clkctrl_offs = AM43XX_CM_PER_DSS_CLKCTRL_OFFSET,
 		},
 	},
+	.parent_hwmod	= &am43xx_dss_core_hwmod,
 };
 
 /* Interfaces */
-- 
2.2.0

[PATCH 1/2] ARM: AM43xx: hwmod: set DSS submodule parent hwmods

[Tomi Valkeinen]

Set DSS core hwmod as the parent for all the DSS submodules. This fixes
the boot time DSS reset, removing the following warnings:

omap_hwmod: dss_dispc: cannot be enabled for reset (3)
omap_hwmod: dss_rfbi: cannot be enabled for reset (3)

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_43xx_data.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-omap2/omap_hwmod_43xx_data.c b/arch/arm/mach-omap2/omap_hwmod_43xx_data.c
index 5c6c8410160e..8eb85925e444 100644
--- a/arch/arm/mach-omap2/omap_hwmod_43xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_43xx_data.c
@@ -498,6 +498,7 @@ static struct omap_hwmod am43xx_dss_dispc_hwmod = {
 		},
 	},
 	.dev_attr	= &am43xx_dss_dispc_dev_attr,
+	.parent_hwmod	= &am43xx_dss_core_hwmod,
 };
 
 /* rfbi */
@@ -512,6 +513,7 @@ static struct omap_hwmod am43xx_dss_rfbi_hwmod = {
 			.clkctrl_offs = AM43XX_CM_PER_DSS_CLKCTRL_OFFSET,
 		},
 	},
+	.parent_hwmod	= &am43xx_dss_core_hwmod,
 };
 
 /* Interfaces */
-- 
2.2.0

[PATCH 2/2] ARM: DRA7xx: hwmod: set DSS submodule parent hwmods

[Tomi Valkeinen]

Set DSS core hwmod as the parent for all the DSS submodules. This fixes
the boot time DSS reset, removing the following warnings:

omap_hwmod: dss_dispc: cannot be enabled for reset (3)
omap_hwmod: dss_hdmi: cannot be enabled for reset (3)

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
index ffd6604cd546..41238d509cb5 100644
--- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
@@ -500,6 +500,7 @@ static struct omap_hwmod dra7xx_dss_dispc_hwmod = {
 		},
 	},
 	.dev_attr	= &dss_dispc_dev_attr,
+	.parent_hwmod	= &dra7xx_dss_hwmod,
 };
 
 /*
@@ -541,6 +542,7 @@ static struct omap_hwmod dra7xx_dss_hdmi_hwmod = {
 	},
 	.opt_clks	= dss_hdmi_opt_clks,
 	.opt_clks_cnt	= ARRAY_SIZE(dss_hdmi_opt_clks),
+	.parent_hwmod	= &dra7xx_dss_hwmod,
 };
 
 /*
-- 
2.2.0

[PATCH 2/2] ARM: DRA7xx: hwmod: set DSS submodule parent hwmods

[Tomi Valkeinen]

Set DSS core hwmod as the parent for all the DSS submodules. This fixes
the boot time DSS reset, removing the following warnings:

omap_hwmod: dss_dispc: cannot be enabled for reset (3)
omap_hwmod: dss_hdmi: cannot be enabled for reset (3)

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
index ffd6604cd546..41238d509cb5 100644
--- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
@@ -500,6 +500,7 @@ static struct omap_hwmod dra7xx_dss_dispc_hwmod = {
 		},
 	},
 	.dev_attr	= &dss_dispc_dev_attr,
+	.parent_hwmod	= &dra7xx_dss_hwmod,
 };
 
 /*
@@ -541,6 +542,7 @@ static struct omap_hwmod dra7xx_dss_hdmi_hwmod = {
 	},
 	.opt_clks	= dss_hdmi_opt_clks,
 	.opt_clks_cnt	= ARRAY_SIZE(dss_hdmi_opt_clks),
+	.parent_hwmod	= &dra7xx_dss_hwmod,
 };
 
 /*
-- 
2.2.0

Re: [PATCH 1/2] ARM: AM43xx: hwmod: set DSS submodule parent hwmods

[Paul Walmsley]

On Fri, 19 Dec 2014, Tomi Valkeinen wrote:

> Set DSS core hwmod as the parent for all the DSS submodules. This fixes
> the boot time DSS reset, removing the following warnings:
> 
> omap_hwmod: dss_dispc: cannot be enabled for reset (3)
> omap_hwmod: dss_rfbi: cannot be enabled for reset (3)
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Thanks, queued for v3.19-rc.

Note that I cannot test this patch due to lack of a AM43xx board here.


- Paul

[PATCH 1/2] ARM: AM43xx: hwmod: set DSS submodule parent hwmods

[Paul Walmsley]

On Fri, 19 Dec 2014, Tomi Valkeinen wrote:

> Set DSS core hwmod as the parent for all the DSS submodules. This fixes
> the boot time DSS reset, removing the following warnings:
> 
> omap_hwmod: dss_dispc: cannot be enabled for reset (3)
> omap_hwmod: dss_rfbi: cannot be enabled for reset (3)
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Thanks, queued for v3.19-rc.

Note that I cannot test this patch due to lack of a AM43xx board here.


- Paul

Re: [PATCH 2/2] ARM: DRA7xx: hwmod: set DSS submodule parent hwmods

[Paul Walmsley]

On Fri, 19 Dec 2014, Tomi Valkeinen wrote:

> Set DSS core hwmod as the parent for all the DSS submodules. This fixes
> the boot time DSS reset, removing the following warnings:
> 
> omap_hwmod: dss_dispc: cannot be enabled for reset (3)
> omap_hwmod: dss_hdmi: cannot be enabled for reset (3)
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Thanks, queued for v3.19-rc.                         

Note that I cannot test this patch due to lack of a DRA7xx board here.

- Paul

[PATCH 2/2] ARM: DRA7xx: hwmod: set DSS submodule parent hwmods

[Paul Walmsley]

On Fri, 19 Dec 2014, Tomi Valkeinen wrote:

> Set DSS core hwmod as the parent for all the DSS submodules. This fixes
> the boot time DSS reset, removing the following warnings:
> 
> omap_hwmod: dss_dispc: cannot be enabled for reset (3)
> omap_hwmod: dss_hdmi: cannot be enabled for reset (3)
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Thanks, queued for v3.19-rc.                         

Note that I cannot test this patch due to lack of a DRA7xx board here.

- Paul

Re: [PATCH 2/2] ARM: DRA7xx: hwmod: set DSS submodule parent hwmods

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2613 bytes --]

Hi Paul,

On 03/01/15 00:50, Paul Walmsley wrote:
> On Fri, 19 Dec 2014, Tomi Valkeinen wrote:
> 
>> Set DSS core hwmod as the parent for all the DSS submodules. This fixes
>> the boot time DSS reset, removing the following warnings:
>>
>> omap_hwmod: dss_dispc: cannot be enabled for reset (3)
>> omap_hwmod: dss_hdmi: cannot be enabled for reset (3)
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> Thanks, queued for v3.19-rc.                         
> 
> Note that I cannot test this patch due to lack of a DRA7xx board here.

Thanks. Unfortunately, I made a mistake with the DRA7xx patch. Well, the
patch itself is correct, but we have some insanity in the HW that I missed:

DRA7xx has a CTRL_CORE_CONTROL_IO_2 register in the control module,
which contains bits for various subsystems, including DCAN, PCIe, QSPI
and DSS. At the moment only DCAN driver uses the register via syscon +
regmap.

For DSS, the bit 0, DSS_DESHDCP_CLKEN is critical. While the bit is
rather undocumented, it presumably enables a clock for DSS's HDCP. Now,
we don't support HDPC in the display driver, but unfortunately the clock
is required for the DSS hardware to initialize.

Without this patch, we see only the few warnings about dss hwmods
"cannot be enabled", but with the patch, we see those and some WARN()s
from hwmod as the DSS HW module does not start. So it's a bit worse.

Why I didn't notice this is that I had an u-boot version that enables
the DSS_DESHDCP_CLKEN bit and leaves it enabled.

So what to do about the issue? You could drop/revert this patch if we
don't see a quick solution for the DSS_DESHDCP_CLKEN. It won't fix
anything as such, but lessens the boot-up spam.

I don't have a good idea how to manage the bit properly. As far as I
see, the bit has to be managed via syscon, using remap_update_bits, so
that we get proper locking. With a quick glance, I didn't see anything
for that in the current clock code. And can we presume that syscon is
probed before hwmods? I guess not.

For the time being, I think the easiest solution would be to just set
the bit and leave it enabled. My understanding (based on commits in TI's
product kernel) is that leaving the DSS_DESHDCP_CLKEN enabled doesn't
really affect much, and any increased power consumption would be too
small to measure.

If that's the route, the question is still where to enable it. As I
said, I have an u-boot which enables it, but I'd rather do the enabling
in the kernel. If in the kernel, where there? It has to happen before
the hwmod init is ran.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/2] ARM: DRA7xx: hwmod: set DSS submodule parent hwmods

[Tomi Valkeinen]

Hi Paul,

On 03/01/15 00:50, Paul Walmsley wrote:
> On Fri, 19 Dec 2014, Tomi Valkeinen wrote:
> 
>> Set DSS core hwmod as the parent for all the DSS submodules. This fixes
>> the boot time DSS reset, removing the following warnings:
>>
>> omap_hwmod: dss_dispc: cannot be enabled for reset (3)
>> omap_hwmod: dss_hdmi: cannot be enabled for reset (3)
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> Thanks, queued for v3.19-rc.                         
> 
> Note that I cannot test this patch due to lack of a DRA7xx board here.

Thanks. Unfortunately, I made a mistake with the DRA7xx patch. Well, the
patch itself is correct, but we have some insanity in the HW that I missed:

DRA7xx has a CTRL_CORE_CONTROL_IO_2 register in the control module,
which contains bits for various subsystems, including DCAN, PCIe, QSPI
and DSS. At the moment only DCAN driver uses the register via syscon +
regmap.

For DSS, the bit 0, DSS_DESHDCP_CLKEN is critical. While the bit is
rather undocumented, it presumably enables a clock for DSS's HDCP. Now,
we don't support HDPC in the display driver, but unfortunately the clock
is required for the DSS hardware to initialize.

Without this patch, we see only the few warnings about dss hwmods
"cannot be enabled", but with the patch, we see those and some WARN()s
from hwmod as the DSS HW module does not start. So it's a bit worse.

Why I didn't notice this is that I had an u-boot version that enables
the DSS_DESHDCP_CLKEN bit and leaves it enabled.

So what to do about the issue? You could drop/revert this patch if we
don't see a quick solution for the DSS_DESHDCP_CLKEN. It won't fix
anything as such, but lessens the boot-up spam.

I don't have a good idea how to manage the bit properly. As far as I
see, the bit has to be managed via syscon, using remap_update_bits, so
that we get proper locking. With a quick glance, I didn't see anything
for that in the current clock code. And can we presume that syscon is
probed before hwmods? I guess not.

For the time being, I think the easiest solution would be to just set
the bit and leave it enabled. My understanding (based on commits in TI's
product kernel) is that leaving the DSS_DESHDCP_CLKEN enabled doesn't
really affect much, and any increased power consumption would be too
small to measure.

If that's the route, the question is still where to enable it. As I
said, I have an u-boot which enables it, but I'd rather do the enabling
in the kernel. If in the kernel, where there? It has to happen before
the hwmod init is ran.

 Tomi


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150105/57514c2a/attachment.sig>

Re: [PATCH 2/2] ARM: DRA7xx: hwmod: set DSS submodule parent hwmods

[Paul Walmsley]

Hi Tomi,

your detailed description of the problem is really great; thanks for the 
summary.

On Mon, 5 Jan 2015, Tomi Valkeinen wrote:

> On 03/01/15 00:50, Paul Walmsley wrote:
> > On Fri, 19 Dec 2014, Tomi Valkeinen wrote:
> > 
> >> Set DSS core hwmod as the parent for all the DSS submodules. This fixes
> >> the boot time DSS reset, removing the following warnings:
> >>
> >> omap_hwmod: dss_dispc: cannot be enabled for reset (3)
> >> omap_hwmod: dss_hdmi: cannot be enabled for reset (3)
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > 
> > Thanks, queued for v3.19-rc.                         
> > 
> > Note that I cannot test this patch due to lack of a DRA7xx board here.
> 
> Thanks. Unfortunately, I made a mistake with the DRA7xx patch. Well, the
> patch itself is correct, but we have some insanity in the HW that I missed:
> 
> DRA7xx has a CTRL_CORE_CONTROL_IO_2 register in the control module,
> which contains bits for various subsystems, including DCAN, PCIe, QSPI
> and DSS. At the moment only DCAN driver uses the register via syscon +
> regmap.
> 
> For DSS, the bit 0, DSS_DESHDCP_CLKEN is critical. While the bit is
> rather undocumented, it presumably enables a clock for DSS's HDCP. Now,
> we don't support HDPC in the display driver, but unfortunately the clock
> is required for the DSS hardware to initialize.

Do you know where this DSS_DESHDCP clock appears in the clock tree?  Is it 
downstream of the main DSS functional clock, or does it come from 
somewhere else?
 
> Without this patch, we see only the few warnings about dss hwmods
> "cannot be enabled", but with the patch, we see those and some WARN()s
> from hwmod as the DSS HW module does not start. So it's a bit worse.
> 
> Why I didn't notice this is that I had an u-boot version that enables
> the DSS_DESHDCP_CLKEN bit and leaves it enabled.
> 
> So what to do about the issue? You could drop/revert this patch if we
> don't see a quick solution for the DSS_DESHDCP_CLKEN. It won't fix
> anything as such, but lessens the boot-up spam.

Sounds like the thing to do in the short term is to drop that patch from 
the fixes series.  Then we can add it back when DSS_DESHDCP_CLKEN is 
sorted.  Are you OK with that for the time being?

> I don't have a good idea how to manage the bit properly. As far as I
> see, the bit has to be managed via syscon, using remap_update_bits, so
> that we get proper locking. With a quick glance, I didn't see anything
> for that in the current clock code. And can we presume that syscon is
> probed before hwmods? I guess not.

Based on the description so far, it sounds like there should be a system 
control module driver that registers this clock, along with all of the 
other clocks in the CTRL_CORE_CONTROL_IO_2 register.  Just shooting from 
the hip here, I guess we'd probably list that DSS_DESHDCP_CLKEN clock as 
an optional clock for DSS in the hwmod data, and add some code to indicate 
that it should be enabled and disabled with the main_clk.

> For the time being, I think the easiest solution would be to just set
> the bit and leave it enabled. My understanding (based on commits in TI's
> product kernel) is that leaving the DSS_DESHDCP_CLKEN enabled doesn't
> really affect much, and any increased power consumption would be too
> small to measure.
> 
> If that's the route, the question is still where to enable it. As I
> said, I have an u-boot which enables it, but I'd rather do the enabling
> in the kernel. If in the kernel, where there? It has to happen before
> the hwmod init is ran.

Yeah, that's a tricky question to answer.  If DSS_DESHDCP_CLKEN is modeled 
as a clock, then it could be marked as ENABLE_ON_INIT, but that seems like 
kind of a hack.



- Paul

[PATCH 2/2] ARM: DRA7xx: hwmod: set DSS submodule parent hwmods

[Paul Walmsley]

Hi Tomi,

your detailed description of the problem is really great; thanks for the 
summary.

On Mon, 5 Jan 2015, Tomi Valkeinen wrote:

> On 03/01/15 00:50, Paul Walmsley wrote:
> > On Fri, 19 Dec 2014, Tomi Valkeinen wrote:
> > 
> >> Set DSS core hwmod as the parent for all the DSS submodules. This fixes
> >> the boot time DSS reset, removing the following warnings:
> >>
> >> omap_hwmod: dss_dispc: cannot be enabled for reset (3)
> >> omap_hwmod: dss_hdmi: cannot be enabled for reset (3)
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > 
> > Thanks, queued for v3.19-rc.                         
> > 
> > Note that I cannot test this patch due to lack of a DRA7xx board here.
> 
> Thanks. Unfortunately, I made a mistake with the DRA7xx patch. Well, the
> patch itself is correct, but we have some insanity in the HW that I missed:
> 
> DRA7xx has a CTRL_CORE_CONTROL_IO_2 register in the control module,
> which contains bits for various subsystems, including DCAN, PCIe, QSPI
> and DSS. At the moment only DCAN driver uses the register via syscon +
> regmap.
> 
> For DSS, the bit 0, DSS_DESHDCP_CLKEN is critical. While the bit is
> rather undocumented, it presumably enables a clock for DSS's HDCP. Now,
> we don't support HDPC in the display driver, but unfortunately the clock
> is required for the DSS hardware to initialize.

Do you know where this DSS_DESHDCP clock appears in the clock tree?  Is it 
downstream of the main DSS functional clock, or does it come from 
somewhere else?
 
> Without this patch, we see only the few warnings about dss hwmods
> "cannot be enabled", but with the patch, we see those and some WARN()s
> from hwmod as the DSS HW module does not start. So it's a bit worse.
> 
> Why I didn't notice this is that I had an u-boot version that enables
> the DSS_DESHDCP_CLKEN bit and leaves it enabled.
> 
> So what to do about the issue? You could drop/revert this patch if we
> don't see a quick solution for the DSS_DESHDCP_CLKEN. It won't fix
> anything as such, but lessens the boot-up spam.

Sounds like the thing to do in the short term is to drop that patch from 
the fixes series.  Then we can add it back when DSS_DESHDCP_CLKEN is 
sorted.  Are you OK with that for the time being?

> I don't have a good idea how to manage the bit properly. As far as I
> see, the bit has to be managed via syscon, using remap_update_bits, so
> that we get proper locking. With a quick glance, I didn't see anything
> for that in the current clock code. And can we presume that syscon is
> probed before hwmods? I guess not.

Based on the description so far, it sounds like there should be a system 
control module driver that registers this clock, along with all of the 
other clocks in the CTRL_CORE_CONTROL_IO_2 register.  Just shooting from 
the hip here, I guess we'd probably list that DSS_DESHDCP_CLKEN clock as 
an optional clock for DSS in the hwmod data, and add some code to indicate 
that it should be enabled and disabled with the main_clk.

> For the time being, I think the easiest solution would be to just set
> the bit and leave it enabled. My understanding (based on commits in TI's
> product kernel) is that leaving the DSS_DESHDCP_CLKEN enabled doesn't
> really affect much, and any increased power consumption would be too
> small to measure.
> 
> If that's the route, the question is still where to enable it. As I
> said, I have an u-boot which enables it, but I'd rather do the enabling
> in the kernel. If in the kernel, where there? It has to happen before
> the hwmod init is ran.

Yeah, that's a tricky question to answer.  If DSS_DESHDCP_CLKEN is modeled 
as a clock, then it could be marked as ENABLE_ON_INIT, but that seems like 
kind of a hack.



- Paul

Re: [PATCH 2/2] ARM: DRA7xx: hwmod: set DSS submodule parent hwmods

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3740 bytes --]

On 08/01/15 12:15, Paul Walmsley wrote:

>> For DSS, the bit 0, DSS_DESHDCP_CLKEN is critical. While the bit is
>> rather undocumented, it presumably enables a clock for DSS's HDCP. Now,
>> we don't support HDPC in the display driver, but unfortunately the clock
>> is required for the DSS hardware to initialize.
> 
> Do you know where this DSS_DESHDCP clock appears in the clock tree?  Is it 
> downstream of the main DSS functional clock, or does it come from 
> somewhere else?

Unfortunately not. It is not mentioned in any documentation I can find.
OMAP5 has the exact same HDMI block, but it does not have this bit in
the control module.

There's this commit from Archit (who is no longer in TI) with some data,
but I cannot find any of it in the documentation:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/247993.html

Maybe OMAP5 has this clock always enabled, i.e. direct L3 clock without
the gate, and the bit was added to DRA7x to give the tiny bit of power
saving it might produce. This sounds quite likely scenario to me.

If so, it would not sound too bad to set the bit at boot time to make
the HW work like OMAP5, so that the SW could be the same. But then
again, I'm purely guessing.

> Sounds like the thing to do in the short term is to drop that patch from 
> the fixes series.  Then we can add it back when DSS_DESHDCP_CLKEN is 
> sorted.  Are you OK with that for the time being?

Yes, I think it's fine to drop it. While resetting the DSS at boot time
would be nice, I think in practice the only diff with this patch (if it
worked) and the current situation are the few hwmod warnings we get at
boot time.

However, I'm working on DRA7 display support for omapdss, and it won't
work if the DSS_DESHDCP_CLKEN is not enabled. So we need some kind of
solution pretty soon.

>> I don't have a good idea how to manage the bit properly. As far as I
>> see, the bit has to be managed via syscon, using remap_update_bits, so
>> that we get proper locking. With a quick glance, I didn't see anything
>> for that in the current clock code. And can we presume that syscon is
>> probed before hwmods? I guess not.
> 
> Based on the description so far, it sounds like there should be a system 
> control module driver that registers this clock, along with all of the 
> other clocks in the CTRL_CORE_CONTROL_IO_2 register.  Just shooting from 
> the hip here, I guess we'd probably list that DSS_DESHDCP_CLKEN clock as 
> an optional clock for DSS in the hwmod data, and add some code to indicate 
> that it should be enabled and disabled with the main_clk.

Hmm, ok. So a syscon driver, that registers this clock (and maybe some
others), and allows access to the non-clock bits via regmap, and handles
locking between the clock and non-clock accesses?

>> For the time being, I think the easiest solution would be to just set
>> the bit and leave it enabled. My understanding (based on commits in TI's
>> product kernel) is that leaving the DSS_DESHDCP_CLKEN enabled doesn't
>> really affect much, and any increased power consumption would be too
>> small to measure.
>>
>> If that's the route, the question is still where to enable it. As I
>> said, I have an u-boot which enables it, but I'd rather do the enabling
>> in the kernel. If in the kernel, where there? It has to happen before
>> the hwmod init is ran.
> 
> Yeah, that's a tricky question to answer.  If DSS_DESHDCP_CLKEN is modeled 
> as a clock, then it could be marked as ENABLE_ON_INIT, but that seems like 
> kind of a hack.

True, but the HW here also seems like a kind of hack =). Random bits
from various subsystems in the same register... sigh...

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/2] ARM: DRA7xx: hwmod: set DSS submodule parent hwmods

[Tomi Valkeinen]

On 08/01/15 12:15, Paul Walmsley wrote:

>> For DSS, the bit 0, DSS_DESHDCP_CLKEN is critical. While the bit is
>> rather undocumented, it presumably enables a clock for DSS's HDCP. Now,
>> we don't support HDPC in the display driver, but unfortunately the clock
>> is required for the DSS hardware to initialize.
> 
> Do you know where this DSS_DESHDCP clock appears in the clock tree?  Is it 
> downstream of the main DSS functional clock, or does it come from 
> somewhere else?

Unfortunately not. It is not mentioned in any documentation I can find.
OMAP5 has the exact same HDMI block, but it does not have this bit in
the control module.

There's this commit from Archit (who is no longer in TI) with some data,
but I cannot find any of it in the documentation:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/247993.html

Maybe OMAP5 has this clock always enabled, i.e. direct L3 clock without
the gate, and the bit was added to DRA7x to give the tiny bit of power
saving it might produce. This sounds quite likely scenario to me.

If so, it would not sound too bad to set the bit at boot time to make
the HW work like OMAP5, so that the SW could be the same. But then
again, I'm purely guessing.

> Sounds like the thing to do in the short term is to drop that patch from 
> the fixes series.  Then we can add it back when DSS_DESHDCP_CLKEN is 
> sorted.  Are you OK with that for the time being?

Yes, I think it's fine to drop it. While resetting the DSS at boot time
would be nice, I think in practice the only diff with this patch (if it
worked) and the current situation are the few hwmod warnings we get at
boot time.

However, I'm working on DRA7 display support for omapdss, and it won't
work if the DSS_DESHDCP_CLKEN is not enabled. So we need some kind of
solution pretty soon.

>> I don't have a good idea how to manage the bit properly. As far as I
>> see, the bit has to be managed via syscon, using remap_update_bits, so
>> that we get proper locking. With a quick glance, I didn't see anything
>> for that in the current clock code. And can we presume that syscon is
>> probed before hwmods? I guess not.
> 
> Based on the description so far, it sounds like there should be a system 
> control module driver that registers this clock, along with all of the 
> other clocks in the CTRL_CORE_CONTROL_IO_2 register.  Just shooting from 
> the hip here, I guess we'd probably list that DSS_DESHDCP_CLKEN clock as 
> an optional clock for DSS in the hwmod data, and add some code to indicate 
> that it should be enabled and disabled with the main_clk.

Hmm, ok. So a syscon driver, that registers this clock (and maybe some
others), and allows access to the non-clock bits via regmap, and handles
locking between the clock and non-clock accesses?

>> For the time being, I think the easiest solution would be to just set
>> the bit and leave it enabled. My understanding (based on commits in TI's
>> product kernel) is that leaving the DSS_DESHDCP_CLKEN enabled doesn't
>> really affect much, and any increased power consumption would be too
>> small to measure.
>>
>> If that's the route, the question is still where to enable it. As I
>> said, I have an u-boot which enables it, but I'd rather do the enabling
>> in the kernel. If in the kernel, where there? It has to happen before
>> the hwmod init is ran.
> 
> Yeah, that's a tricky question to answer.  If DSS_DESHDCP_CLKEN is modeled 
> as a clock, then it could be marked as ENABLE_ON_INIT, but that seems like 
> kind of a hack.

True, but the HW here also seems like a kind of hack =). Random bits
from various subsystems in the same register... sigh...

 Tomi


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150108/efa115b8/attachment-0001.sig>