[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 at 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