linux-kernel - Re: [PATCH 2/4] usb: dwc3: add dwc3 glue layer for UniPhier SoCs

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]

Message-ID: <87a7x3l8gr.fsf@linux.intel.com>
Date:   Wed, 24 Jan 2018 14:58:12 +0200
From:   Felipe Balbi <balbi@...nel.org>
To:     Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>
Cc:     linux-usb@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org,
        Jassi Brar <jaswinder.singh@...aro.org>,
        Masami Hiramatsu <masami.hiramatsu@...aro.org>,
        Kishon Vijay Abraham I <kishon@...com>
Subject: Re: [PATCH 2/4] usb: dwc3: add dwc3 glue layer for UniPhier SoCs


Hi,

Kunihiko Hayashi <hayashi.kunihiko@...ionext.com> writes:
> Hello Felipe,
>
> Thank you for your comments.
>
> On Tue, 23 Jan 2018 15:12:36 +0200 <balbi@...nel.org> wrote:
>
>> 
>> Hi,
>> 
>> Kunihiko Hayashi <hayashi.kunihiko@...ionext.com> writes:
>> > Add a specific glue layer for UniPhier SoC platform to support
>> > USB host mode. It manages hardware operating sequences to enable multiple
>> > clock gates and assert resets, and to prepare to use dwc3 controller
>> > on the SoC.
>> >
>> > This patch also handles the physical layer that has same register space
>> > as the glue layer, because it needs to integrate initialziation sequence
>> > between glue and phy.
>> >
>> > In case of some SoCs, since some initialization values for PHY are
>> > included in nvmem, this patch includes the way to get the values from nvmem.
>> >
>> > It supports PXs2 and LD20 SoCs.
>> >
>> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>
>> > Signed-off-by: Motoya Tanigawa <tanigawa.motoya@...ionext.com>
>> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@...aro.org>
>> > ---
>> >  drivers/usb/dwc3/Kconfig         |   9 +
>> >  drivers/usb/dwc3/Makefile        |   1 +
>> >  drivers/usb/dwc3/dwc3-uniphier.c | 554 +++++++++++++++++++++++++++++++++++++++
>> >  3 files changed, 564 insertions(+)
>> >  create mode 100644 drivers/usb/dwc3/dwc3-uniphier.c
>
> ...snip...
>
>> > +
>> > +static void dwc3u_ssphy_testio_write(struct dwc3u_priv *priv, int port,
>> > +				     u32 data)
>> 
>> anything with sshphy or hsphy in the name should probably be part of a
>> PHY driver using drivers/phy/ framework.
>
> I can try to separate phy control from this driver.
> However, phy registers belongs to "dwc3-glue" IO map area (65b00000),
> and this area also contains a reset bit for "dwc3-core" hardware.
>
> Although the phy driver is called from dwc3-core driver,
> we need to deassert the reset bit before probing dwc3-core driver.
>
> As shown later, I think that it's difficut to determine the order of
> initializing the registers in this area.
>
>> > +static void dwc3u_vbus_disable(struct dwc3u_priv *priv)
>> > +{
>> > +	int i;
>> > +
>> > +	for (i = 0; i < priv->nvbus; i++) {
>> > +		dwc3u_maskwrite(priv, VBUS_CONTROL(i),
>> > +				DRVVBUS_REG_EN | DRVVBUS_REG,
>> > +				DRVVBUS_REG_EN | 0);
>> > +	}
>> > +}
>> 
>> drivers/regulator maybe?
>
> VBUS_CONTROL register is used for determing whether "dwc3-glue" hardware
> enables vbus connected with "regulator" hardware.
>
> The regulator driver should manage "regulator" hardware, and
> I don't think that the driver should manage this register.
>
>> > +static void dwc3u_reset_init(struct dwc3u_priv *priv)
>> > +{
>> > +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
>> > +	usleep_range(1000, 2000);
>> > +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, LINK_RESET);
>> > +}
>> > +
>> > +static void dwc3u_reset_clear(struct dwc3u_priv *priv)
>> > +{
>> > +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
>> > +}
>> 
>> drivers/reset ?
>
> The reset driver manages "sysctrl" IO map area in our SoC.
>
> RESET_CTL register belongs to "dwc3-glue" IO map area,
> and the kernel can't access this area until enabling usb clocks and
> deasserting usb resets in "sysctrl".
>
> I think that "dwc3-glue" register control should be separated from
> "sysctrl".

Just split your address space and treat your glue as a device with
several children:

glue@...00000 {
	compatible = "foo"

	phy@bar {
        	...
	};

	sysctrl@baz {
        	...
	};

	dwc3@foo {
        	compatible = "snps, dwc3";
                ...
	};
};

Then you know that you can let dwc3/core.c handle the PHY for you. If we
need to teach dwc3/core.c about regulators, we can do that. But we don't
need SoC-specific hacks ;-)

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)