[linux-sunxi] Re: [PATCH 2/4] clk: sunxi-ng: Add sun7i-a20 CCU driver
Chen-Yu Tsai
wens at csie.org
Thu Mar 2 07:05:03 PST 2017
On Thu, Mar 2, 2017 at 10:21 PM, Maxime Ripard
<maxime.ripard at 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
More information about the linux-arm-kernel
mailing list