Messages in this thread | | | Date | Sun, 28 Jun 2020 19:03:54 +0200 | From | Paul Cercueil <> | Subject | Re: [PATCH 2/2] clk: X1000: Add support for calculat REFCLK of USB PHY. |
| |
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) >> <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 >>> ++++++++++++++++++++++++++++++++++++++++ >>> 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.
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 >> 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 >>> >>
| |