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

[Paul Cercueil <paul@crapouillou.net>]

Hi Zhou,

Le lun. 29 juin 2020 à 0:18, Zhou Yanjie <zhouyanjie@wanyeetech.com> a 
écrit :
> Hi Paul,
> 
> 在 2020/6/27 上午1:36, Paul Cercueil 写道:
>> Hi Zhou,
>> 
>> Le sam. 27 juin 2020 à 0:48, 周琰杰 (Zhou Yanjie) 
>> \x7f<zhouyanjie@wanyeetech.com> 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@foxmail.com>
>>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>>> ---
>>>  drivers/clk/ingenic/x1000-cgu.c | 113 
>>> \x7f\x7f++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 113 insertions(+)
>>> 
>>> diff --git a/drivers/clk/ingenic/x1000-cgu.c 
>>> \x7f\x7fb/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 
>> \x7fx1000_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 \x7f48 MHz?
>> 
>> I believe CLKCORE is the OTG core's clock (aka "otg"), and I'm 
>> pretty \x7fsure that these fields only represent CLKCORE/4, CLKCORE/2, 
>> CLKCORE/1, \x7fbut 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.

I know it is for the OTG PHY clock, what I'm saying is that the OTG PHY 
clock is derived from CLKCORE which I believe is the "otg" clock. 
Unless the clock represents a crystal, it is derived from another 
clock, and as a result the clock rate should be computed from the 
parent clock rate.

-Paul

> 
>> In that case the "otg_phy" should be parented to "otg", and the rate 
>> \x7fshould be computed according to the parent rate and the divider 
>> \x7fconfigured.
>> 
>>> +
>>> +    BUG();
>> 
>> Don't use BUG() - it pisses off Linus :)
>> And it's reserved for bugs that will take the whole system down, I 
>> \x7fthink. 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 
>>> \x7f\x7flong 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 
>>> \x7f\x7freq_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 \x7f\x7f*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
>>> 
>>