Re: [PATCH v2 1/2] phy: spacemit: add K1 USB 2.0 PHY driver

[Yixun Lan <dlan@kernel.org>]

Hi Dave,

On 09:04 Mon 12 Oct     , Yixun Lan wrote:
> Hi Dave,
> 
> On 12:32 Tue 06 Oct     , Dave Klotz wrote:
> > The UTMI PHY at c0a30000 ("spacemit,k1-usb2-phy") feeds the K1's DWC3
> > USB 3 controller. Port the Linux driver: enable the PHY clock, program
> > the PLL divider for the 24 MHz reference, wait for the PLL, then release
> > the PHY's resets and enable its clocks, the HS TX clock and the
> > automatic host-disconnect clear.
> > 
> ..
> > The divider has to be written before the PLL is polled. On the OrangePi
> Right, the upstream kernel driver also did this way
> 
> > R2S under FreeBSD, a driver that polled first and never wrote it left
> > the PLL stopped: the clock enables did not latch (0x60ef read back as
> Sorry, I've not fully understood here, "polled first and never write"..
> Could you elaborate more on this?
> 
> > 0x60e9), the PHY gave the controller no UTMI clock and the xHCI reset
> > (USBCMD.HCRST) never completed. Warn if the enables do not latch.
> Will comment at code below
> 
> > 
> > Also apply the vendor's HS DAC current setting (+15 %), which the
> > FreeBSD driver that runs the R2S port does.
> > 
> Any actual problem without above settings? I'm just curious,
> as upstream code miss this part
> 
> > Signed-off-by: Dave Klotz <DaveWK225@protonmail.com>
> > ---
> >  MAINTAINERS                        |   1 +
> >  drivers/phy/spacemit/Kconfig       |   9 ++
> >  drivers/phy/spacemit/Makefile      |   1 +
> >  drivers/phy/spacemit/phy-k1-usb2.c | 173 +++++++++++++++++++++++++++++
> >  4 files changed, 184 insertions(+)
> >  create mode 100644 drivers/phy/spacemit/phy-k1-usb2.c
> > 
> > +
..
> > +	/* Release the PHY's internal resets and enable its clocks */
> > +	writel(PHY_INIT_MODE_BITS | PHY_CLK_ENABLE_BITS | PHY_DEASSERT_RST_BITS,
> > +	       base + PHY_RST_MODE_CTRL);
> ..
> > +	val = readl(base + PHY_RST_MODE_CTRL);
> > +	if ((val & PHY_CLK_ENABLE_BITS) != PHY_CLK_ENABLE_BITS)
> > +		dev_warn(phy->dev, "clock enables did not latch (%#x)\n", val);
> So read back and check.. if this situation is happening, a warning
> message won't save life.
> 
> But, I need to check if the "enable bits" actually reflect the clock
> enabling status from the hardware perspective.. but even leave that alone,
> I think using a poll() instead of a readl() is a more proper way to go?
> usually it will take few time for the hardware to settle down..
> 
I've asked SpacemiT internal to check, and got this info:

Setting "enable bits" will take effect immediately, it doesn't reflect
the clock status, so this "readl() & check" isn't useful, drop it

If you still see problem, then probably need investigate other way..

> > +
> > +	writel(PHY_HSTXP_RSTN | PHY_CLK_HSTXP_EN | PHY_HSTXP_MODE,
> > +	       base + PHY_HSTXP_HW_CTRL);
> > +
> > +	clrsetbits_le32(base + PHY_ANALOG_REG14_13, PHY_HSDAC_ISEL,
> > +			FIELD_PREP(PHY_HSDAC_ISEL, PHY_HSDAC_ISEL_15_INC) |
> > +			PHY_HSDAC_IREG_EN);
> > +
> > +	/* Auto-clear the HS host disconnect state on reconnect */
> > +	setbits_le32(base + PHY_TX_HOST_CTRL, PHY_HST_DISC_AUTO_CLR);
> > +
> > +	return 0;
> > +}

-- 
Yixun Lan (dlan)