Hi Neil,

On Thu, Mar 7, 2019 at 9:41 AM Neil Armstrong [off-list ref] wrote:
[...]

quoted

quoted

+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?

quoted

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@googlemail.com>


Regards
Martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel