Re: [PATCH 2/2] clk: X1000: Add support for calculat REFCLK of USB PHY.

[Zhou Yanjie]

Hi Paul,

å 2020/6/27 äå1:36, Paul Cercueil åé:
> Hi Zhou,
>
> Le sam. 27 juin 2020 Ã 0:48, åçæ (Zhou Yanjie) 
> <zhouyanjie@xxxxxxxxxxxxxx> a Ãcrit :
> > Add new functions to "x1000_otg_phy_ops" to calculat the rate of REFCLK,
> > which is needed by USB PHY in the Ingenic X1000 SoC.
> >
> > Tested-by: åæ (Zhou Zheng) <sernia.zhou@xxxxxxxxxxx>
> > Signed-off-by: åçæ (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx>
> > ---
> > Âdrivers/clk/ingenic/x1000-cgu.c | 113 
> > ++++++++++++++++++++++++++++++++++++++++
> > Â1 file changed, 113 insertions(+)
> >
> > diff --git a/drivers/clk/ingenic/x1000-cgu.c 
> > b/drivers/clk/ingenic/x1000-cgu.c
> > index 453f3323cb99..a61c16f98a11 100644
> > --- a/drivers/clk/ingenic/x1000-cgu.c
> > +++ b/drivers/clk/ingenic/x1000-cgu.c
> > @@ -48,8 +48,114 @@
> > Â#define USBPCR_SIDDQÂÂÂÂÂÂÂ BIT(21)
> > Â#define USBPCR_OTG_DISABLEÂÂÂ BIT(20)
> >
> > +/* bits within the USBPCR1 register */
> > +#define USBPCR1_REFCLKSEL_SHIFTÂÂÂ 26
> > +#define USBPCR1_REFCLKSEL_MASKÂÂÂ (0x3 << USBPCR1_REFCLKSEL_SHIFT)
> > +#define USBPCR1_REFCLKSEL_COREÂÂÂ (0x2 << USBPCR1_REFCLKSEL_SHIFT)
> > +#define USBPCR1_REFCLKDIV_SHIFTÂÂÂ 24
> > +#define USBPCR1_REFCLKDIV_MASKÂÂÂ (0x3 << USBPCR1_REFCLKDIV_SHIFT)
> > +#define USBPCR1_REFCLKDIV_48ÂÂÂ (0x2 << USBPCR1_REFCLKDIV_SHIFT)
> > +#define USBPCR1_REFCLKDIV_24ÂÂÂ (0x1 << USBPCR1_REFCLKDIV_SHIFT)
> > +#define USBPCR1_REFCLKDIV_12ÂÂÂ (0x0 << USBPCR1_REFCLKDIV_SHIFT)
> > +
> > Âstatic struct ingenic_cgu *cgu;
> >
> > +static u8 x1000_otg_phy_get_parent(struct clk_hw *hw)
> > +{
> > +ÂÂÂ /* we only use CLKCORE, revisit if that ever changes */
> > +ÂÂÂ return 0;
> > +}
> > +
> > +static int x1000_otg_phy_set_parent(struct clk_hw *hw, u8 idx)
> > +{
> > +ÂÂÂ unsigned long flags;
> > +ÂÂÂ u32 usbpcr1;
> > +
> > +ÂÂÂ if (idx > 0)
> > +ÂÂÂÂÂÂÂ return -EINVAL;
> > +
> > +ÂÂÂ spin_lock_irqsave(&cgu->lock, flags);
> > +
> > +ÂÂÂ usbpcr1 = readl(cgu->base + CGU_REG_USBPCR1);
> > +ÂÂÂ usbpcr1 &= ~USBPCR1_REFCLKSEL_MASK;
> > +ÂÂÂ /* we only use CLKCORE */
> > +ÂÂÂ usbpcr1 |= USBPCR1_REFCLKSEL_CORE;
> > +ÂÂÂ writel(usbpcr1, cgu->base + CGU_REG_USBPCR1);
> > +
> > +ÂÂÂ spin_unlock_irqrestore(&cgu->lock, flags);
> > +ÂÂÂ return 0;
> > +}
>
> If you only support one parent, maybe set that bit in the 
> x1000_cgu_init(), then you can drop the get_parent/set_parent.

Sure.


> > +
> > +static unsigned long x1000_otg_phy_recalc_rate(struct clk_hw *hw,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long parent_rate)
> > +{
> > +ÂÂÂ u32 usbpcr1;
> > +ÂÂÂ unsigned refclk_div;
> > +
> > +ÂÂÂ usbpcr1 = readl(cgu->base + CGU_REG_USBPCR1);
> > +ÂÂÂ refclk_div = usbpcr1 & USBPCR1_REFCLKDIV_MASK;
> > +
> > +ÂÂÂ switch (refclk_div) {
> > +ÂÂÂ case USBPCR1_REFCLKDIV_12:
> > +ÂÂÂÂÂÂÂ return 12000000;
> > +
> > +ÂÂÂ case USBPCR1_REFCLKDIV_24:
> > +ÂÂÂÂÂÂÂ return 24000000;
> > +
> > +ÂÂÂ case USBPCR1_REFCLKDIV_48:
> > +ÂÂÂÂÂÂÂ return 48000000;
> > +ÂÂÂ }
>
> On your setup, what frequency is configured for the "otg" clock? Is it 
> 48 MHz?
>
> I believe CLKCORE is the OTG core's clock (aka "otg"), and I'm pretty 
> sure that these fields only represent CLKCORE/4, CLKCORE/2, CLKCORE/1, 
> but the doc expects CLKCORE==48MHz.

This is the REFCLKDIV in USBPCR1 reg, it's for the usb phy, so it is not 
the otg clock. In X1000 and X1500 it is 24MHz, in JZ4780 it is 48MHz.


> In that case the "otg_phy" should be parented to "otg", and the rate 
> should be computed according to the parent rate and the divider 
> configured.
>
> > +
> > +ÂÂÂ BUG();
>
> Don't use BUG() - it pisses off Linus :)
> And it's reserved for bugs that will take the whole system down, I 
> think. Better use WARN().

Sure, I will change it in the next version.

Thanks and best regards!


> Cheers,
> -Paul
>
> > +ÂÂÂ return parent_rate;
> > +}
> > +
> > +static long x1000_otg_phy_round_rate(struct clk_hw *hw, unsigned 
> > long req_rate,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long *parent_rate)
> > +{
> > +ÂÂÂ if (req_rate < 18000000)
> > +ÂÂÂÂÂÂÂ return 12000000;
> > +
> > +ÂÂÂ if (req_rate < 36000000)
> > +ÂÂÂÂÂÂÂ return 24000000;
> > +
> > +ÂÂÂ return 48000000;
> > +}
> > +
> > +static int x1000_otg_phy_set_rate(struct clk_hw *hw, unsigned long 
> > req_rate,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long parent_rate)
> > +{
> > +ÂÂÂ unsigned long flags;
> > +ÂÂÂ u32 usbpcr1, div_bits;
> > +
> > +ÂÂÂ switch (req_rate) {
> > +ÂÂÂ case 12000000:
> > +ÂÂÂÂÂÂÂ div_bits = USBPCR1_REFCLKDIV_12;
> > +ÂÂÂÂÂÂÂ break;
> > +
> > +ÂÂÂ case 24000000:
> > +ÂÂÂÂÂÂÂ div_bits = USBPCR1_REFCLKDIV_24;
> > +ÂÂÂÂÂÂÂ break;
> > +
> > +ÂÂÂ case 48000000:
> > +ÂÂÂÂÂÂÂ div_bits = USBPCR1_REFCLKDIV_48;
> > +ÂÂÂÂÂÂÂ break;
> > +
> > +ÂÂÂ default:
> > +ÂÂÂÂÂÂÂ return -EINVAL;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ spin_lock_irqsave(&cgu->lock, flags);
> > +
> > +ÂÂÂ usbpcr1 = readl(cgu->base + CGU_REG_USBPCR1);
> > +ÂÂÂ usbpcr1 &= ~USBPCR1_REFCLKDIV_MASK;
> > +ÂÂÂ usbpcr1 |= div_bits;
> > +ÂÂÂ writel(usbpcr1, cgu->base + CGU_REG_USBPCR1);
> > +
> > +ÂÂÂ spin_unlock_irqrestore(&cgu->lock, flags);
> > +ÂÂÂ return 0;
> > +}
> > +
> > Âstatic int x1000_usb_phy_enable(struct clk_hw *hw)
> > Â{
> > ÂÂÂÂ void __iomem *reg_opcrÂÂÂÂÂÂÂ = cgu->base + CGU_REG_OPCR;
> > @@ -80,6 +186,13 @@ static int x1000_usb_phy_is_enabled(struct clk_hw 
> > *hw)
> > Â}
> >
> > Âstatic const struct clk_ops x1000_otg_phy_ops = {
> > +ÂÂÂ .get_parent = x1000_otg_phy_get_parent,
> > +ÂÂÂ .set_parent = x1000_otg_phy_set_parent,
> > +
> > +ÂÂÂ .recalc_rate = x1000_otg_phy_recalc_rate,
> > +ÂÂÂ .round_rate = x1000_otg_phy_round_rate,
> > +ÂÂÂ .set_rate = x1000_otg_phy_set_rate,
> > +
> > ÂÂÂÂ .enableÂÂÂÂÂÂÂ = x1000_usb_phy_enable,
> > ÂÂÂÂ .disableÂÂÂ = x1000_usb_phy_disable,
> > ÂÂÂÂ .is_enabledÂÂÂ = x1000_usb_phy_is_enabled,
> > --
> > 2.11.0