Re: [PATCH v7 14/19] phy: sun4i-usb: Introduce port2 SIDDQ quirk

[Andre Przywara]

On Mon, 21 Jun 2021 10:06:31 +0530
Vinod Koul <vkoul@xxxxxxxxxx> wrote:

Hi Vinod,

thanks for having a look!

> On 15-06-21, 12:06, Andre Przywara wrote:
> > At least the Allwinner H616 SoC requires a weird quirk to make most
> > USB PHYs work: Only port2 works out of the box, but all other ports
> > need some help from this port2 to work correctly: The CLK_BUS_PHY2 and
> > RST_USB_PHY2 clock and reset need to be enabled, and the SIDDQ bit in
> > the PMU PHY control register needs to be cleared. For this register to
> > be accessible, CLK_BUS_ECHI2 needs to be ungated. Don't ask ....
> > 
> > Instead of disguising this as some generic feature, do exactly that
> > in our PHY init:
> > If the quirk bit is set, and we initialise a PHY other than PHY2, ungate
> > this one special clock, and clear the SIDDQ bit. We can pull in the
> > other required clocks via the DT.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > ---
> >  drivers/phy/allwinner/phy-sun4i-usb.c | 59 +++++++++++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> > 
> > diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
> > index 126ef74d013c..316ef5fca831 100644
> > --- a/drivers/phy/allwinner/phy-sun4i-usb.c
> > +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
> > @@ -120,6 +120,7 @@ struct sun4i_usb_phy_cfg {
> >  	u8 phyctl_offset;
> >  	bool dedicated_clocks;
> >  	bool phy0_dual_route;
> > +	bool needs_phy2_siddq;
> >  	int missing_phys;
> >  };
> >  
> > @@ -289,6 +290,50 @@ static int sun4i_usb_phy_init(struct phy *_phy)
> >  		return ret;
> >  	}
> >  
> > +	/* Some PHYs on some SoCs need the help of PHY2 to work. */
> > +	if (data->cfg->needs_phy2_siddq && phy->index != 2) {
> > +		struct sun4i_usb_phy *phy2 = &data->phys[2];
> > +
> > +		ret = clk_prepare_enable(phy2->clk);
> > +		if (ret) {
> > +			reset_control_assert(phy->reset);
> > +			clk_disable_unprepare(phy->clk2);
> > +			clk_disable_unprepare(phy->clk);
> > +			return ret;
> > +		}
> > +
> > +		ret = reset_control_deassert(phy2->reset);
> > +		if (ret) {
> > +			clk_disable_unprepare(phy2->clk);
> > +			reset_control_assert(phy->reset);
> > +			clk_disable_unprepare(phy->clk2);
> > +			clk_disable_unprepare(phy->clk);
> > +			return ret;
> > +		}  
> 
> no delay between deassert and assert... ?

Mmmh, not sure what you are after. This is just the clean-up path,
when the deassert failed, and we tear down what was brought up before.
And the assert is not for the same reset line that we tried to
deassert anyway, if that is what you mean?
Or do I miss something here?

Cheers,
Andre