linux-kernel - Re: [PATCH v2 5/8] phy: amlogic: add Amlogic G12A USB2 PHY Driver

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: <CAFBinCCuWvm203nQFw6y3Nwoj_TxqgRk7X-N8O_P=SjAGJwDWA@mail.gmail.com>
Date:   Mon, 11 Mar 2019 22:04:47 +0100
From:   Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To:     Neil Armstrong <narmstrong@...libre.com>
Cc:     gregkh@...uxfoundation.org, hminas@...opsys.com, balbi@...nel.org,
        kishon@...com, linux-amlogic@...ts.infradead.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 5/8] phy: amlogic: add Amlogic G12A USB2 PHY Driver

Hi Neil,

On Thu, Mar 7, 2019 at 9:41 AM Neil Armstrong <narmstrong@...libre.com> wrote:
[...]
> >> +static int phy_meson_g12a_usb2_exit(struct phy *phy)
> >> +{
> >> +       struct phy_meson_g12a_usb2_priv *priv = phy_get_drvdata(phy);
> >> +
> >> +       return reset_control_reset(priv->reset);
> > do you know whether we should reset_control_assert here instead of
> > reset_control_reset?
> > the probe function below already uses reset_control_deassert, so the
> > current implementation is inconsistent. in v1 you replied with "Maybe
> > it would be better, indeed." - if there's a reason why
> > reset_control_assert doesn't work here then I would like to have a
> > comment stating why
>
> It's not clear yet, I implemented it safe here since in my tests, when
> I left the USB2 PHYs resetted, it was kept resetted on a soft system reset
> and the ROM was not able to setup the PHY correctly.
that's probably why Amlogic's kernel also uses a reset pulse

> So maybe it's wrong for power management, it's safer to simply to keep the
> PHYs unresetted when unused.
if you re-send this patch anyways (to clean up the #include) can you
please add a comment with the explanation from above?

> > Apart from these two this is looking good!
> > Human readable BIT/GENMASK #defines for the register bits would be
> > nice, but I'm not sure if you have the details to add these.
>
> I have the registers set in the doc, but it's much longer than copying
> the registers structs from the vendor kernel, so I postponed it.
>
> I'll try adding these, but for now it's low priority unless the PHY maintainer
> asks for them.
ACK, it's not high priority, so it's fine for now

with the #include changed and a comment for the reset pulse you can add my:
Reviewed-by: Martin Blumenstingl <martin.blumenstingl@...glemail.com>


Regards
Martin