Re: [PATCH 4/6] clk: sunxi-ng: Add A33 CCU support

[Chen-Yu Tsai]

On Mon, Sep 5, 2016 at 5:51 PM, Maxime Ripard
<maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote:
> Hi Chen-Yu,
>
> On Mon, Sep 05, 2016 at 02:34:21PM +0800, Chen-Yu Tsai wrote:
>> > +static SUNXI_CCU_NKMP_WITH_GATE_LOCK(pll_cpux_clk, "pll-cpux",
>> > +                                    "osc24M", 0x000,
>> > +                                    8, 5,              /* N */
>> > +                                    4, 2,              /* K */
>> > +                                    0, 2,              /* M */
>> > +                                    16, 2,             /* P */
>>
>> Manual says there's no divider for P = 0x3.
>> You'll need a table here.
>
> Hmmm, Indeed. Or rather, a max value. We discussed that in the past, I
> guess it's time to introduce it.
>

Cool.

>> > +                                    BIT(31),           /* gate */
>> > +                                    BIT(28),           /* lock */
>> > +                                    0);
>> > +
>> > +/*
>> > + * The Audio PLL is supposed to have 4 outputs: 3 fixed factors from
>> > + * the base (2x, 4x and 8x), and one variable divider (the one true
>> > + * pll audio).
>> > + *
>> > + * We don't have any need for the variable divider for now, so we just
>> > + * hardcode it to match with the clock names
>> > + */
>> > +#define SUN8I_A33_PLL_AUDIO_REG        0x008
>> > +
>> > +static SUNXI_CCU_NM_WITH_GATE_LOCK(pll_audio_base_clk, "pll-audio-base",
>> > +                                  "osc24M", 0x008,
>> > +                                  8, 7,                /* N */
>> > +                                  0, 5,                /* M */
>> > +                                  BIT(31),             /* gate */
>> > +                                  BIT(28),             /* lock */
>> > +                                  CLK_SET_RATE_UNGATE);
>>
>> Are you sure about this? I suspect CLK_SET_RATE_GATE (clk must be
>> gated before set rate) is what you want, given the non-DVFS
>> nature of the PLL. Same for the other PLLs.
>
> Yes, I'm sure about it. The lock, on the A33 at least, this has to be
> confirmed on other SoCs, won't work if the clock is gated.
>
> So, every time we call clk_set_rate, we will hit the WARN_ON because
> of the lock timeout unless the clock runs.

I see. That's a relief. It would be problematic if it were
actually CLK_SET_RATE_GATE, and multiple blocks were using
the same PLL, and you tried to switch the display resolution.

It also makes sense. If the PLL is disabled, there is no way
it would lock. Maybe we could skip the lock check if the PLL
is disabled?

>> > +static SUNXI_CCU_GATE(bus_mipi_dsi_clk,        "bus-mipi-dsi", "ahb1",
>> > +                     0x060, BIT(1), 0);
>>
>> Nit: we know which bus these peripherals are on.
>> Can we be more explicit with the names?
>
> I wanted to be consistent with the datasheet, and would really prefer
> to stick to it.

OK. I guess it's just eye candy for some of us. The user or
board developer (who is going to use the bindings) shouldn't
care.

>> > +static SUNXI_CCU_GATE(bus_ce_clk,      "bus-ce",       "ahb1",
>> > +                     0x060, BIT(5), 0);
>>
>> Nit: manual says Security System.
>>
>> (Maybe I should change the name on sun6i to say Crypto Engine
>>  if that's what we're going with?)
>
> But I can't really use reject that argument there then :)

:)

> The rationale was that it's called CE in the H3 which was already
> merged, but I'll change it.
>
>> [...]
>>
>> > +static SUNXI_CCU_GATE(usb_hsic_12M_clk,        "usb-hsic-12M", "osc24M",
>> > +                     0x0cc, BIT(11), 0);
>>
>> A TODO note saying we should have a fixed-factor-gate for this
>> would be nice.
>
> Hmmm? I'm not sure what you're saying.

Some of the USB clocks have known clock rates. The HSIC 12M
clock here is obviously 12 MHz.

Others such as the OHCI special clocks might run at 48 MHz
from a dedicated PLL. This number is given in the H3 datasheet,
in the USB Host Clock Requirement section.

It's not really a blocking issue as we only ever do enable/disable
on them.

Regards
ChenYu

> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com