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] [day] [month] [year] [list]

Date:   Fri, 09 Mar 2018 11:04:58 +0200
From:   Felipe Balbi <balbi@...nel.org>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>,
        linux-usb@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        devicetree@...r.kernel.org,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <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,

Masahiro Yamada <yamada.masahiro@...ionext.com> writes:
>>>> > +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
>
>
> Slightly related question.
>
>
> Why don't we put clocks and resets to dwc3/core.c?

We can do that for the simpler platforms, no worries.

> dwc3-of-simple.c only handles clocks and resets.
> This is generic enough to be added to dwc3/core.c, I think.
>
>
> I checked the two instances of dwc3-of-simple.
>
> "qcom,dwc3"
> https://github.com/torvalds/linux/blob/v4.16-rc3/arch/arm64/boot/dts/qcom/msm8996.dtsi#L780
>
> "rockchip,rk3399-dwc3"
> https://github.com/torvalds/linux/blob/v4.16-rc3/arch/arm64/boot/dts/rockchip/rk3399.dtsi#L395
>
>
> They just contain clocks, resets, and "snps,dwc3" sub-node.
>
>
> If we do that,
>
> usb@...0000 {
>         compatible = "qcom,dwc3";
>         clocks = ...;
>
>         dwc3@...0000 {
>                 compatible = "snps,dwc3";
>                 reg = ...;
>                 interrupts = ...;
>                 phys = ...;
>         }
> };
>
>
> will be turned into
>
>
> dwc3@...0000 {
>         compatible = "qcom,dwc3", "snps,dwc3";
>         reg = ...;
>         clocks = ...;
>         interrupts = ...;
>         phys = ...;
> };
>
>
> This looks simpler.

yup. This will only work for the simpler platforms, though. TI platforms
and PCI-based platforms, this really won't work :-)

> Also, dwc3/core.c and dwc3-of-simple.c
> duplicate runtime PM.

they "kinda" duplicate :-) Some platforms have platform-specific clocks
which are not generic enough to be stuffed into dwc3/core.c. Some of
those clocks may need special handling of some sorts. It's best to keep
the option for peculiar clock tree setups available.

If your platform is simple enough that you can get away without a glue
layer, sure thing. More power for you :-)

-- 
balbi

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