Re: [linux-sunxi] Re: [PATCH 2/4] clk: sunxi-ng: Add sun7i-a20 CCU driver

[Chen-Yu Tsai <wens@csie.org>]

On Thu, Mar 2, 2017 at 10:21 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi Priit,
>
> On Wed, Mar 01, 2017 at 11:38:14PM +0200, Priit Laes wrote:
>> > > +/* PLL2 - Audio clock */
>> > > +static struct ccu_nm pll_audio_base_clk = {
>> > > > > > > + .enable         = BIT(31),
>> > > > > > > + .n              = _SUNXI_CCU_MULT_OFFSET(8, 7, 0),
>> > > > > > > + .m              = _SUNXI_CCU_DIV_OFFSET(0, 5, 0),
>> > > > > > > + .common         = {
>> > > > > > > +         .reg            = 0x008,
>> > > > > > > +         .hw.init        = CLK_HW_INIT("pll-audio-base",
>> > > > > +                                           "hosc",
>> > > > > +                                           &ccu_nm_ops,
>> > > > > +                                           0),
>> > > > > +     },
>> > > +
>> > > +};
>> >
>> > You're forgetting the post-divider here
>>
>> It's hardcoded to 4 during ccu initialization, similar to what is done
>> on the other SoCs (A13, A31..).
>
> Right, sorry, I only saw it later. Please move that define you've been
> using here, and it will be fine :)
>
>> > > +/* TODO: pll8 gpu 0x040 */
>> >
>> > Please add all the clocks.
>>
>> I'm not really comfortable adding clocks for blocks that currently lack
>> drivers.
>
> Yet, you did it for quite a significant amount already (VE, NAND,
> AC97, DE MP, etc.). Don't get me wrong, this is definitely not a
> criticism. One of the point of the switch to sunxi-ng was that we
> would get out of the previous situation where every time you wanted to
> do something, you needed to add some clocks.
>
> We have some generic code that, if fed the right data, will
> (hopefully) just work. So it's pretty safe to add (and this is better
> to be consistent, and that's the policy we had for all the other CCU
> drivers).
>
>> > > +/* BIT(21 .. 31) - reserved */
>> >
>> > I'm not sure we need those comments either.
>> >
>> > > +/*
>> > > + * TODO: SATA clock also supports external clock as parent.
>> > > + * Currently we default to using PLL6 SATA gate.
>> > > + */
>> >
>> > Which external clock? It should be modelled anyway. If we have a
>> > dependency on some other clock, it should be in our DT binding, and
>> > listed in the mux there.
>> >
>> > Otherwise, the clock framework will not be able to deal with that mux
>> > being already set by the bootloader, and if we need to support that
>> > clock in the future, our binding will be ready for it.
>>
>> I wish I knew which clock they're talking about..
>>
>> User manuals (A10/A20) only specify following in the clock register
>> description:
>>
>> BIT(24) - CLK_SRC_GATING, default 0x0
>> Clock Source Select:
>> 0: PLL6 for SATA(100MHz)
>> 1: External Clock
>>
>> There's no section for SATA (called NC) in A10 manual, and in A20
>> manual only contains list of SATA/AHCI features.
>
> Hmmmm, ok :/

The external clock is probably an optional crystal or oscillator
that can be connected to the SATA-CLKM / SATA-CLKP pins.

The datasheet does not say what the frequency or any other parameters
should be though. And to my knowledge no board uses it.

ChenYu

>> > > +/* Some AHB gates are exported */
>> > > > > +#define CLK_AHB_BIST         31
>> > > > > +#define CLK_AHB_MS           36
>> > > > > +#define CLK_AHB_SDRAM                38
>> > > > > +#define CLK_AHB_ACE          39
>> > > > > +#define CLK_AHB_TS           41
>> > > > > +#define CLK_AHB_VE           48
>> > > > > +#define CLK_AHB_TVD          49
>> > > > > +#define CLK_AHB_TVE1         51
>> > > > > +#define CLK_AHB_LCD1         53
>> > > > > +#define CLK_AHB_CSI0         54
>> > > > > +#define CLK_AHB_CSI1         55
>> > > > > +#define CLK_AHB_HDMI0                56
>> > > > > +#define CLK_AHB_DE_BE1               59
>> > > > > +#define CLK_AHB_DE_FE0               60
>> > > > > +#define CLK_AHB_DE_FE1               61
>> > > > > +#define CLK_AHB_MP           63
>> > > > > +#define CLK_AHB_GPU          64
>> > > +
>> > > +/* Some APB0 gates are exported */
>> > > > > +#define CLK_APB0_AC97                67
>> > > > > +#define CLK_APB0_KEYPAD              74
>> > > +
>> > > +/* Some APB1 gates are exported */
>> > > > > +#define CLK_APB1_CAN         79
>> > > > > +#define CLK_APB1_SCR         80
>> > > +
>> > > +/* Some IP module clocks are exported */
>> > > > > +#define CLK_MS                       93
>> > > > > +#define CLK_TS                       106
>> > > > > +#define CLK_PATA             111
>> > > > > +#define CLK_AC97             115
>> > > > > +#define CLK_KEYPAD           117
>> > > > > +#define CLK_SATA             118
>> > > +
>> > > +/* Some DRAM gates are exported */
>> > > > > +#define CLK_DRAM_VE          125
>> > > > > +#define CLK_DRAM_CSI0                126
>> > > > > +#define CLK_DRAM_CSI1                127
>> > > > > +#define CLK_DRAM_TS          128
>> > > > > +#define CLK_DRAM_TVD         129
>> > > > > +#define CLK_DRAM_TVE1                131
>> > > > > +#define CLK_DRAM_OUT         132
>> > > > > +#define CLK_DRAM_DE_FE1              133
>> > > > > +#define CLK_DRAM_DE_FE0              134
>> > > > > +#define CLK_DRAM_DE_BE1              136
>> > > > > +#define CLK_DRAM_MP          137
>> > > > > +#define CLK_DRAM_ACE         138
>> > > +
>> > > > > +#define CLK_DE_BE1           140
>> > > > > +#define CLK_DE_FE0           141
>> > > > > +#define CLK_DE_FE1           142
>> > > > > +#define CLK_DE_MP            143
>> > > > > +#define CLK_TCON1_CH0                145
>> > > > > +#define CLK_CSI_SPECIAL              146
>> > > > > +#define CLK_TVD                      147
>> > > > > +#define CLK_TCON0_CH1_SCLK2  148
>> > > > > +#define CLK_TCON1_CH1_SCLK2  150
>> > > > > +#define CLK_TCON1_CH1                151
>> > > > > +#define CLK_CSI0             152
>> > > > > +#define CLK_CSI1             153
>> > > > > +#define CLK_VE                       154
>> > > > > +#define CLK_AVS                      156
>> > > > > +#define CLK_ACE                      157
>> > > > > +#define CLK_HDMI             158
>> > > > > +#define CLK_GPU                      159
>> > > > > +#define CLK_MBUS             160
>> > > > > +#define CLK_HDMI1_SLOW               161
>> > > > > +#define CLK_HDMI1_REPEAT     162
>> > > > > +#define CLK_OUT_A            163
>> > > +#define CLK_OUT_B                164
>> >
>> > Is there a reason not to expose these clocks?
>>
>> I exposed them on need to have basis. And basically did one-to-one
>> conversion from devicetree.
>
> I guess we can still make some pretty good assumptions on what's going
> to be needed at some point.
>
> All the mod clocks will be, the bus gates too, the CPU too. All the
> internal ones (PLL, AXI, APB, etc) can remain hidden though.
>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com