| Message ID | 20170401131001.9988-2-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 2017-04-01 at 15:10 +0200, Martin Blumenstingl wrote: > This adds the NAND clocks (from the HHI_NAND_CLK_CNTL register) to the > Meson8b clock driver. There are three NAND clocks: a gate which enables > or disables the NAND clock, a mux and a divider (which divides the mux > output). > Unfortunately the public S805 datasheet does not document the mux > parents. However, the vendor kernel has a few hints for us which allows > us to make an educated guess about the clock parents. To do this we need > to have a look at set_nand_core_clk() from the vendor's NAND driver (see > [0]): > - XTAL = (4<<9) | (1<<8) | 0 > - 160MHz = (0<<9) | (1<<8) | 3) > - 182MHz = (3<<9) | (1<<8) | 1) > - 212MHz = (1<<9) | (1<<8) | 3) > - 255MHz = (2<<9) | (1<<8) | 1) > > should only be used for debugging) we have to do a bit of math for the > other parents: target_freq * divider = rate of parent clock > Bit 8 above is the enable bit, so we can ignore it here. Bits 11:9 are > the mux index and bits 6:0 are the 0-based divider (so we need to add > 1). This gives us: > - mux 0 (160MHz * 4) = fclk_div4 (actual rate = 637.5MHz, off by 2.5MHz) > - mux 1 (212MHz * 4) = fclk_div3 (actual rate = 850MHz, off by 2MHz) > - mux 2 (255MHz * 2) = fclk_div5 (matches exactly 510MHz) > - mux 3 (182MHz * 2) = fclk_div7 (actual rate = 346.3MHz, off by 0.3MHz) > Good job detective ;) What is strange is that amlogic seems to have change the index of these parents on the gxbb. I have a documentation (which I'm not at liberty to share unfortunately) which gives the following index on the S905: 0: xtal 1: fclk_div2 2: fclk_div3 3: flck_div5 4: fclk_div7 5: mpll2 6: mpll3 7: gp0 Did they change the ids, or is the documentation wrong ? I don't know, but we'll to pay attention to that if you port your NAND work the gx family. > [0] > https://github.com/khadas/linux/blob/9587681285cb/drivers/amlogic/amlnf/dev/am > lnf_ctrl.c#L314 > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > drivers/clk/meson/meson8b.c | 53 > +++++++++++++++++++++++++++++++++++++++++++++ > drivers/clk/meson/meson8b.h | 6 ++++- > 2 files changed, 58 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c > index e9985503165c..b7f3cf8ed386 100644 > --- a/drivers/clk/meson/meson8b.c > +++ b/drivers/clk/meson/meson8b.c > @@ -403,6 +403,53 @@ struct clk_gate meson8b_clk81 = { > }, > }; > > +static u32 mux_table_nand[] = { 0, 1, 2, 3, 4 }; You can drop this table, you don't need it. > + > +struct clk_mux meson8b_nand_clk_sel = { > + .reg = (void *)HHI_NAND_CLK_CNTL, > + .mask = 0x7, > + .shift = 9, > + .table = mux_table_nand, > + .flags = CLK_MUX_ROUND_CLOSEST, If you go for CLK_SET_RATE_NO_REPARENT, I think this flag is not really useful. > + .lock = &clk_lock, > + .hw.init = &(struct clk_init_data){ > + .name = "nand_clk_sel", > + .ops = &clk_mux_ops, > + /* FIXME all other parents are unknown: */ > + .parent_names = (const char *[]){ "fclk_div4", "fclk_div3", > + "fclk_div5", "fclk_div7", "xtal" }, > + .num_parents = 5, > + .flags = CLK_SET_RATE_NO_REPARENT, PSB > + }, > +}; > + > +struct clk_divider meson8b_nand_clk_div = { > + .reg = (void *)HHI_NAND_CLK_CNTL, > + .shift = 0, > + .width = 6, > + .flags = CLK_DIVIDER_ROUND_CLOSEST, > + .lock = &clk_lock, > + .hw.init = &(struct clk_init_data){ > + .name = "nand_clk_div", > + .ops = &clk_divider_ops, > + .parent_names = (const char *[]){ "nand_clk_sel" }, > + .num_parents = 1, I think you are trying to tightly control all those elements in your nand driver, aren't you ? To mimic the vendor driver settings, maybe ? While it would surely work, another way is possible. You could give the flag CLK_SET_RATE_PARENT (and drop the CLK_SET_RATE_NO_REPARENT) to all those elements and let the CCF do the work for you. There is a good chance it will come with same settings anyway (if that's the best math can provide) The good thing about it is that it hides those quirks from your driver. So if the gxbb has slighly different clock setup for the nand, you won't have to deal with it in your driver, just call clk_set_rate on the gate and be done with it. Could you give it try ? > + }, > +}; > + > +struct clk_gate meson8b_nand_clk_gate = { > + .reg = (void *)HHI_NAND_CLK_CNTL, > + .bit_idx = 8, > + .lock = &clk_lock, > + .hw.init = &(struct clk_init_data){ > + .name = "nand_clk_gate", > + .ops = &clk_gate_ops, > + .parent_names = (const char *[]){ "nand_clk_div" }, > + .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT, > + }, > +}; > + > /* Everything Else (EE) domain gates */ > > static MESON_GATE(meson8b_ddr, HHI_GCLK_MPEG0, 0); > @@ -584,6 +631,9 @@ static struct clk_hw_onecell_data meson8b_hw_onecell_data > = { > [CLKID_MPLL0] = &meson8b_mpll0.hw, > [CLKID_MPLL1] = &meson8b_mpll1.hw, > [CLKID_MPLL2] = &meson8b_mpll2.hw, > + [CLKID_NAND_SEL] = &meson8b_nand_clk_sel.hw, > + [CLKID_NAND_DIV] = &meson8b_nand_clk_div.hw, > + [CLKID_NAND_CLK] = &meson8b_nand_clk_gate.hw, > }, > .num = CLK_NR_CLKS, > }; > @@ -679,14 +729,17 @@ static struct clk_gate *const meson8b_clk_gates[] = { > &meson8b_ao_ahb_sram, > &meson8b_ao_ahb_bus, > &meson8b_ao_iface, > + &meson8b_nand_clk_gate, > }; > > static struct clk_mux *const meson8b_clk_muxes[] = { > &meson8b_mpeg_clk_sel, > + &meson8b_nand_clk_sel, > }; > > static struct clk_divider *const meson8b_clk_dividers[] = { > &meson8b_mpeg_clk_div, > + &meson8b_nand_clk_div, > }; > > static int meson8b_clkc_probe(struct platform_device *pdev) > diff --git a/drivers/clk/meson/meson8b.h b/drivers/clk/meson/meson8b.h > index 3881defc8644..cbb8001b5fe8 100644 > --- a/drivers/clk/meson/meson8b.h > +++ b/drivers/clk/meson/meson8b.h > @@ -37,6 +37,7 @@ > #define HHI_GCLK_AO 0x154 /* 0x55 offset in data sheet > */ > #define HHI_SYS_CPU_CLK_CNTL1 0x15c /* 0x57 offset in data > sheet */ > #define HHI_MPEG_CLK_CNTL 0x174 /* 0x5d offset in data sheet > */ > +#define HHI_NAND_CLK_CNTL 0x25c /* 0x97 offset in data sheet > */ > #define HHI_MPLL_CNTL 0x280 /* 0xa0 offset in data > sheet */ > #define HHI_SYS_PLL_CNTL 0x300 /* 0xc0 offset in data sheet */ > #define HHI_VID_PLL_CNTL 0x320 /* 0xc8 offset in data sheet */ > @@ -160,8 +161,11 @@ > #define CLKID_MPLL0 93 > #define CLKID_MPLL1 94 > #define CLKID_MPLL2 95 > +#define CLKID_NAND_SEL 96 > +#define CLKID_NAND_DIV 97 > +#define CLKID_NAND_CLK 98 > > -#define CLK_NR_CLKS 96 > +#define CLK_NR_CLKS 99 > > /* include the CLKIDs that have been made part of the stable DT binding */ > #include <dt-bindings/clock/meson8b-clkc.h>
Hi Jerome, thanks for taking time to review this! On Sun, Apr 2, 2017 at 5:38 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > On Sat, 2017-04-01 at 15:10 +0200, Martin Blumenstingl wrote: >> This adds the NAND clocks (from the HHI_NAND_CLK_CNTL register) to the >> Meson8b clock driver. There are three NAND clocks: a gate which enables >> or disables the NAND clock, a mux and a divider (which divides the mux >> output). >> Unfortunately the public S805 datasheet does not document the mux >> parents. However, the vendor kernel has a few hints for us which allows >> us to make an educated guess about the clock parents. To do this we need >> to have a look at set_nand_core_clk() from the vendor's NAND driver (see >> [0]): >> - XTAL = (4<<9) | (1<<8) | 0 >> - 160MHz = (0<<9) | (1<<8) | 3) >> - 182MHz = (3<<9) | (1<<8) | 1) >> - 212MHz = (1<<9) | (1<<8) | 3) >> - 255MHz = (2<<9) | (1<<8) | 1) >> >> should only be used for debugging) we have to do a bit of math for the >> other parents: target_freq * divider = rate of parent clock >> Bit 8 above is the enable bit, so we can ignore it here. Bits 11:9 are >> the mux index and bits 6:0 are the 0-based divider (so we need to add >> 1). This gives us: >> - mux 0 (160MHz * 4) = fclk_div4 (actual rate = 637.5MHz, off by 2.5MHz) >> - mux 1 (212MHz * 4) = fclk_div3 (actual rate = 850MHz, off by 2MHz) >> - mux 2 (255MHz * 2) = fclk_div5 (matches exactly 510MHz) >> - mux 3 (182MHz * 2) = fclk_div7 (actual rate = 346.3MHz, off by 0.3MHz) >> > > Good job detective ;) > What is strange is that amlogic seems to have change the index of these parents > on the gxbb. I have a documentation (which I'm not at liberty to share > unfortunately) which gives the following index on the S905: > 0: xtal > 1: fclk_div2 > 2: fclk_div3 > 3: flck_div5 > 4: fclk_div7 > 5: mpll2 > 6: mpll3 > 7: gp0 > > Did they change the ids, or is the documentation wrong ? I don't know, but we'll > to pay attention to that if you port your NAND work the gx family. I guess they changed the IDs. on Meson8b the only NAND clock rates that are actually used are 212MHz and 255MHz (hardcoded in their driver). however, on GXBB there's a tiny difference: there 200MHz and 250MHz are used. my assumption was: if they change even the clock frequencies then it's likely that the mux parents are different as well >> [0] >> https://github.com/khadas/linux/blob/9587681285cb/drivers/amlogic/amlnf/dev/am >> lnf_ctrl.c#L314 >> >> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> >> --- >> drivers/clk/meson/meson8b.c | 53 >> +++++++++++++++++++++++++++++++++++++++++++++ >> drivers/clk/meson/meson8b.h | 6 ++++- >> 2 files changed, 58 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c >> index e9985503165c..b7f3cf8ed386 100644 >> --- a/drivers/clk/meson/meson8b.c >> +++ b/drivers/clk/meson/meson8b.c >> @@ -403,6 +403,53 @@ struct clk_gate meson8b_clk81 = { >> }, >> }; >> >> +static u32 mux_table_nand[] = { 0, 1, 2, 3, 4 }; > You can drop this table, you don't need it. > >> + >> +struct clk_mux meson8b_nand_clk_sel = { >> + .reg = (void *)HHI_NAND_CLK_CNTL, >> + .mask = 0x7, >> + .shift = 9, >> + .table = mux_table_nand, >> + .flags = CLK_MUX_ROUND_CLOSEST, > > If you go for CLK_SET_RATE_NO_REPARENT, I think this flag is not really useful. yep, I'd rather leave it and remove CLK_SET_RATE_NO_REPARENT below >> + .lock = &clk_lock, >> + .hw.init = &(struct clk_init_data){ >> + .name = "nand_clk_sel", >> + .ops = &clk_mux_ops, >> + /* FIXME all other parents are unknown: */ >> + .parent_names = (const char *[]){ "fclk_div4", "fclk_div3", >> + "fclk_div5", "fclk_div7", "xtal" }, >> + .num_parents = 5, >> + .flags = CLK_SET_RATE_NO_REPARENT, > > PSB > >> + }, >> +}; >> + >> +struct clk_divider meson8b_nand_clk_div = { >> + .reg = (void *)HHI_NAND_CLK_CNTL, >> + .shift = 0, >> + .width = 6, >> + .flags = CLK_DIVIDER_ROUND_CLOSEST, >> + .lock = &clk_lock, >> + .hw.init = &(struct clk_init_data){ >> + .name = "nand_clk_div", >> + .ops = &clk_divider_ops, >> + .parent_names = (const char *[]){ "nand_clk_sel" }, >> + .num_parents = 1, > > I think you are trying to tightly control all those elements in your nand > driver, aren't you ? To mimic the vendor driver settings, maybe ? > > While it would surely work, another way is possible. You could give the flag > CLK_SET_RATE_PARENT (and drop the CLK_SET_RATE_NO_REPARENT) to all those > elements and let the CCF do the work for you. > > There is a good chance it will come with same settings anyway (if that's the > best math can provide) > > The good thing about it is that it hides those quirks from your driver. So if > the gxbb has slighly different clock setup for the nand, you won't have to deal > with it in your driver, just call clk_set_rate on the gate and be done with it. > > Could you give it try ? it seems that I missed this: at first I thought that I have to set the mux parent manually. but after playing around with my NAND driver code I figured out that (like you suggested) I could probably get away by only setting the rate on the gate and let the framework do the rest. I'll send an update as soon as I have time >> + }, >> +}; >> + >> +struct clk_gate meson8b_nand_clk_gate = { >> + .reg = (void *)HHI_NAND_CLK_CNTL, >> + .bit_idx = 8, >> + .lock = &clk_lock, >> + .hw.init = &(struct clk_init_data){ >> + .name = "nand_clk_gate", >> + .ops = &clk_gate_ops, >> + .parent_names = (const char *[]){ "nand_clk_div" }, >> + .num_parents = 1, >> + .flags = CLK_SET_RATE_PARENT, >> + }, >> +}; >> + >> /* Everything Else (EE) domain gates */ >> >> static MESON_GATE(meson8b_ddr, HHI_GCLK_MPEG0, 0); >> @@ -584,6 +631,9 @@ static struct clk_hw_onecell_data meson8b_hw_onecell_data >> = { >> [CLKID_MPLL0] = &meson8b_mpll0.hw, >> [CLKID_MPLL1] = &meson8b_mpll1.hw, >> [CLKID_MPLL2] = &meson8b_mpll2.hw, >> + [CLKID_NAND_SEL] = &meson8b_nand_clk_sel.hw, >> + [CLKID_NAND_DIV] = &meson8b_nand_clk_div.hw, >> + [CLKID_NAND_CLK] = &meson8b_nand_clk_gate.hw, >> }, >> .num = CLK_NR_CLKS, >> }; >> @@ -679,14 +729,17 @@ static struct clk_gate *const meson8b_clk_gates[] = { >> &meson8b_ao_ahb_sram, >> &meson8b_ao_ahb_bus, >> &meson8b_ao_iface, >> + &meson8b_nand_clk_gate, >> }; >> >> static struct clk_mux *const meson8b_clk_muxes[] = { >> &meson8b_mpeg_clk_sel, >> + &meson8b_nand_clk_sel, >> }; >> >> static struct clk_divider *const meson8b_clk_dividers[] = { >> &meson8b_mpeg_clk_div, >> + &meson8b_nand_clk_div, >> }; >> >> static int meson8b_clkc_probe(struct platform_device *pdev) >> diff --git a/drivers/clk/meson/meson8b.h b/drivers/clk/meson/meson8b.h >> index 3881defc8644..cbb8001b5fe8 100644 >> --- a/drivers/clk/meson/meson8b.h >> +++ b/drivers/clk/meson/meson8b.h >> @@ -37,6 +37,7 @@ >> #define HHI_GCLK_AO 0x154 /* 0x55 offset in data sheet >> */ >> #define HHI_SYS_CPU_CLK_CNTL1 0x15c /* 0x57 offset in data >> sheet */ >> #define HHI_MPEG_CLK_CNTL 0x174 /* 0x5d offset in data sheet >> */ >> +#define HHI_NAND_CLK_CNTL 0x25c /* 0x97 offset in data sheet >> */ >> #define HHI_MPLL_CNTL 0x280 /* 0xa0 offset in data >> sheet */ >> #define HHI_SYS_PLL_CNTL 0x300 /* 0xc0 offset in data sheet */ >> #define HHI_VID_PLL_CNTL 0x320 /* 0xc8 offset in data sheet */ >> @@ -160,8 +161,11 @@ >> #define CLKID_MPLL0 93 >> #define CLKID_MPLL1 94 >> #define CLKID_MPLL2 95 >> +#define CLKID_NAND_SEL 96 >> +#define CLKID_NAND_DIV 97 >> +#define CLKID_NAND_CLK 98 >> >> -#define CLK_NR_CLKS 96 >> +#define CLK_NR_CLKS 99 >> >> /* include the CLKIDs that have been made part of the stable DT binding */ >> #include <dt-bindings/clock/meson8b-clkc.h>
diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c index e9985503165c..b7f3cf8ed386 100644 --- a/drivers/clk/meson/meson8b.c +++ b/drivers/clk/meson/meson8b.c @@ -403,6 +403,53 @@ struct clk_gate meson8b_clk81 = { }, }; +static u32 mux_table_nand[] = { 0, 1, 2, 3, 4 }; + +struct clk_mux meson8b_nand_clk_sel = { + .reg = (void *)HHI_NAND_CLK_CNTL, + .mask = 0x7, + .shift = 9, + .table = mux_table_nand, + .flags = CLK_MUX_ROUND_CLOSEST, + .lock = &clk_lock, + .hw.init = &(struct clk_init_data){ + .name = "nand_clk_sel", + .ops = &clk_mux_ops, + /* FIXME all other parents are unknown: */ + .parent_names = (const char *[]){ "fclk_div4", "fclk_div3", + "fclk_div5", "fclk_div7", "xtal" }, + .num_parents = 5, + .flags = CLK_SET_RATE_NO_REPARENT, + }, +}; + +struct clk_divider meson8b_nand_clk_div = { + .reg = (void *)HHI_NAND_CLK_CNTL, + .shift = 0, + .width = 6, + .flags = CLK_DIVIDER_ROUND_CLOSEST, + .lock = &clk_lock, + .hw.init = &(struct clk_init_data){ + .name = "nand_clk_div", + .ops = &clk_divider_ops, + .parent_names = (const char *[]){ "nand_clk_sel" }, + .num_parents = 1, + }, +}; + +struct clk_gate meson8b_nand_clk_gate = { + .reg = (void *)HHI_NAND_CLK_CNTL, + .bit_idx = 8, + .lock = &clk_lock, + .hw.init = &(struct clk_init_data){ + .name = "nand_clk_gate", + .ops = &clk_gate_ops, + .parent_names = (const char *[]){ "nand_clk_div" }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + }, +}; + /* Everything Else (EE) domain gates */ static MESON_GATE(meson8b_ddr, HHI_GCLK_MPEG0, 0); @@ -584,6 +631,9 @@ static struct clk_hw_onecell_data meson8b_hw_onecell_data = { [CLKID_MPLL0] = &meson8b_mpll0.hw, [CLKID_MPLL1] = &meson8b_mpll1.hw, [CLKID_MPLL2] = &meson8b_mpll2.hw, + [CLKID_NAND_SEL] = &meson8b_nand_clk_sel.hw, + [CLKID_NAND_DIV] = &meson8b_nand_clk_div.hw, + [CLKID_NAND_CLK] = &meson8b_nand_clk_gate.hw, }, .num = CLK_NR_CLKS, }; @@ -679,14 +729,17 @@ static struct clk_gate *const meson8b_clk_gates[] = { &meson8b_ao_ahb_sram, &meson8b_ao_ahb_bus, &meson8b_ao_iface, + &meson8b_nand_clk_gate, }; static struct clk_mux *const meson8b_clk_muxes[] = { &meson8b_mpeg_clk_sel, + &meson8b_nand_clk_sel, }; static struct clk_divider *const meson8b_clk_dividers[] = { &meson8b_mpeg_clk_div, + &meson8b_nand_clk_div, }; static int meson8b_clkc_probe(struct platform_device *pdev) diff --git a/drivers/clk/meson/meson8b.h b/drivers/clk/meson/meson8b.h index 3881defc8644..cbb8001b5fe8 100644 --- a/drivers/clk/meson/meson8b.h +++ b/drivers/clk/meson/meson8b.h @@ -37,6 +37,7 @@ #define HHI_GCLK_AO 0x154 /* 0x55 offset in data sheet */ #define HHI_SYS_CPU_CLK_CNTL1 0x15c /* 0x57 offset in data sheet */ #define HHI_MPEG_CLK_CNTL 0x174 /* 0x5d offset in data sheet */ +#define HHI_NAND_CLK_CNTL 0x25c /* 0x97 offset in data sheet */ #define HHI_MPLL_CNTL 0x280 /* 0xa0 offset in data sheet */ #define HHI_SYS_PLL_CNTL 0x300 /* 0xc0 offset in data sheet */ #define HHI_VID_PLL_CNTL 0x320 /* 0xc8 offset in data sheet */ @@ -160,8 +161,11 @@ #define CLKID_MPLL0 93 #define CLKID_MPLL1 94 #define CLKID_MPLL2 95 +#define CLKID_NAND_SEL 96 +#define CLKID_NAND_DIV 97 +#define CLKID_NAND_CLK 98 -#define CLK_NR_CLKS 96 +#define CLK_NR_CLKS 99 /* include the CLKIDs that have been made part of the stable DT binding */ #include <dt-bindings/clock/meson8b-clkc.h>
This adds the NAND clocks (from the HHI_NAND_CLK_CNTL register) to the Meson8b clock driver. There are three NAND clocks: a gate which enables or disables the NAND clock, a mux and a divider (which divides the mux output). Unfortunately the public S805 datasheet does not document the mux parents. However, the vendor kernel has a few hints for us which allows us to make an educated guess about the clock parents. To do this we need to have a look at set_nand_core_clk() from the vendor's NAND driver (see [0]): - XTAL = (4<<9) | (1<<8) | 0 - 160MHz = (0<<9) | (1<<8) | 3) - 182MHz = (3<<9) | (1<<8) | 1) - 212MHz = (1<<9) | (1<<8) | 3) - 255MHz = (2<<9) | (1<<8) | 1) While there is a comment for the XTAL parent (which indicates that it should only be used for debugging) we have to do a bit of math for the other parents: target_freq * divider = rate of parent clock Bit 8 above is the enable bit, so we can ignore it here. Bits 11:9 are the mux index and bits 6:0 are the 0-based divider (so we need to add 1). This gives us: - mux 0 (160MHz * 4) = fclk_div4 (actual rate = 637.5MHz, off by 2.5MHz) - mux 1 (212MHz * 4) = fclk_div3 (actual rate = 850MHz, off by 2MHz) - mux 2 (255MHz * 2) = fclk_div5 (matches exactly 510MHz) - mux 3 (182MHz * 2) = fclk_div7 (actual rate = 346.3MHz, off by 0.3MHz) [0] https://github.com/khadas/linux/blob/9587681285cb/drivers/amlogic/amlnf/dev/amlnf_ctrl.c#L314 Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- drivers/clk/meson/meson8b.c | 53 +++++++++++++++++++++++++++++++++++++++++++++ drivers/clk/meson/meson8b.h | 6 ++++- 2 files changed, 58 insertions(+), 1 deletion(-)