| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 30 Jun 2020 20:26:30 +0800
From: Zhou Yanjie <zhouyanjie@...yeetech.com>
To: Paul Cercueil <paul@...pouillou.net>
Cc: linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
sboyd@...nel.org, mturquette@...libre.com,
dongsheng.qiu@...enic.com, aric.pzqi@...enic.com,
rick.tyliu@...enic.com, yanfei.li@...enic.com,
sernia.zhou@...mail.com, zhenwenjin@...il.com
Subject: Re: [PATCH 2/2] clk: X1000: Add support for calculat REFCLK of USB
PHY.
Hi Paul,
在 2020/6/29 上午1:03, Paul Cercueil 写道:
> Hi Zhou,
>
> Le lun. 29 juin 2020 à 0:18, Zhou Yanjie <zhouyanjie@...yeetech.com> a
> écrit :
>> Hi Paul,
>>
>> 在 2020/6/27 上午1:36, Paul Cercueil 写道:
>>> Hi Zhou,
>>>
>>> Le sam. 27 juin 2020 à 0:48, 周琰杰 (Zhou Yanjie)
>>> <zhouyanjie@...yeetech.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@...mail.com>
>>>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@...yeetech.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.
>
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
>>> 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
>>>>
>>>
>
Powered by blists - more mailing lists