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

[Zhou Yanjie <zhouyanjie@wanyeetech.com>]

Hi Paul,

在 2020/6/29 上午1:03, Paul Cercueil 写道:
> 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.
>

In the current Ingenic SoCs I have in hand, only JZ4780/X1000/X1500 has 
this REFCLKDIV.

At present, the relevant drivers in X1000 are written with reference to 
the drivers in jz4780-cgu.c ( the author is Paul Burton ). I have 
already tested on X1000 and JZ4780 today, the driver is effective and 
can be configured by setting assigned-clock-rates in DT ( 48000000 for 
JZ4780, 24000000 for X1000 and X1500 ).

Thanks and best regards!


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