linux-kernel - Re: [PATCH 3/3] bus: ti-sysc: Implement display subsystem reset quirk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 3 Mar 2020 07:13:49 -0800
From:   Tony Lindgren <tony@...mide.com>
To:     Tomi Valkeinen <tomi.valkeinen@...com>
Cc:     linux-omap@...r.kernel.org, "Andrew F . Davis" <afd@...com>,
        Dave Gerlach <d-gerlach@...com>,
        Faiz Abbas <faiz_abbas@...com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Keerthy <j-keerthy@...com>, Nishanth Menon <nm@...com>,
        Peter Ujfalusi <peter.ujfalusi@...com>,
        Roger Quadros <rogerq@...com>, Suman Anna <s-anna@...com>,
        Tero Kristo <t-kristo@...com>, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, Jyri Sarha <jsarha@...com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 3/3] bus: ti-sysc: Implement display subsystem reset quirk

Hi,

* Tomi Valkeinen <tomi.valkeinen@...com> [200303 06:03]:
> On 24/02/2020 21:12, Tony Lindgren wrote:
> > +	/* Remap the whole module range to be able to reset dispc outputs */
> > +	devm_iounmap(ddata->dev, ddata->module_va);
> > +	ddata->module_va = devm_ioremap(ddata->dev,
> > +					ddata->module_pa,
> > +					ddata->module_size);
> 
> Why is this needed? The range is not mapped when sysc_pre_reset_quirk_dss()
> is called? This will unmap and remap twice, as this function is called
> twice. And then left mapped.

That's because by default we only ioremap the module revision, sysconfig
and sysstatus register are and provide the rest as a range for the child
nodes.

In the dss quirk case we need to tinker with registers also in the dispc
range, and at the parent dss probe time dispc has not probed yet.

We may be able to eventually move the reset quirk to dispc, but then
it won't happen in the current setup until after dss top level driver
has loaded.

We leave the module range ioremapped as we still need to access
sysconfig related registers for PM runtime.

> > +	if (!ddata->module_va)
> > +		return -EIO;
> > +
> > +	/* DISP_CONTROL */
> > +	val = sysc_read(ddata, dispc_offset + 0x40);
> 
> Defines for dss/dispc register offsets could have been copied from the
> platform display.c and used in this file.

Yeah I though about that, but decided to keep everything local to
the quirk handling. We could have them defined in some dss header
though.

> > +	/* Clear IRQSTATUS */
> > +	sysc_write(ddata, 0x1000 + 0x18, irq_mask);
> 
> dispc_offset instead of 0x1000.

OK

> > +
> > +	/* Disable outputs */
> > +	val = sysc_quirk_dispc(ddata, dispc_offset, true);
> > +
> > +	/* Poll IRQSTATUS */
> > +	error = readl_poll_timeout(ddata->module_va + dispc_offset + 0x18,
> > +				   val, val != irq_mask, 100, 50);
> > +	if (error)
> > +		dev_warn(ddata->dev, "%s: timed out %08x !+ %08x\n",
> > +			 __func__, val, irq_mask);
> > +
> > +	if (sysc_soc->soc == SOC_3430) {
> > +		/* Clear DSS_SDI_CONTROL */
> > +		sysc_write(ddata, dispc_offset + 0x44, 0);
> > +
> > +		/* Clear DSS_PLL_CONTROL */
> > +		sysc_write(ddata, dispc_offset + 0x48, 0);
> 
> These are not dispc registers, but dss registers.

Ouch. Thanks for catching this, will include in the fix.

> > +	}
> > +
> > +	/* Clear DSS_CONTROL to switch DSS clock sources to PRCM if not */
> > +	sysc_write(ddata, dispc_offset + 0x40, 0);
> 
> Same here.

OK

Regards,

Tony