| Message ID | 20171204051912.7485-2-wens@csie.org (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
Hi Chen-Yu, On 04/12/17 05:19, Chen-Yu Tsai wrote: > On the A64, the MMC module clocks are fixed in the new timing mode, > i.e. they do not have a bit to select the mode. These clocks have > a 2x divider somewhere between the clock and the MMC module. > > To be consistent with other SoCs supporting the new timing mode, > we model the 2x divider as a fixed post-divider on the MMC module > clocks. > > To do this, we first add fixed post-divider to the MP style clocks, > which the MMC module clocks are. > > Signed-off-by: Chen-Yu Tsai <wens@csie.org> > --- > drivers/clk/sunxi-ng/ccu_mp.c | 20 ++++++++++++++++++-- > drivers/clk/sunxi-ng/ccu_mp.h | 24 ++++++++++++++++++++++++ > 2 files changed, 42 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/sunxi-ng/ccu_mp.c b/drivers/clk/sunxi-ng/ccu_mp.c > index 688855e7dc8c..5d0af4051737 100644 > --- a/drivers/clk/sunxi-ng/ccu_mp.c > +++ b/drivers/clk/sunxi-ng/ccu_mp.c > @@ -50,12 +50,19 @@ static unsigned long ccu_mp_round_rate(struct ccu_mux_internal *mux, > unsigned int max_m, max_p; > unsigned int m, p; > > + if (cmp->common.features & CCU_FEATURE_FIXED_POSTDIV) > + rate *= cmp->fixed_post_div; Can't you just initialise fixed_post_div to 1 normally and save the CCU_FEATURE_FIXED_POSTDIV? > + > max_m = cmp->m.max ?: 1 << cmp->m.width; > max_p = cmp->p.max ?: 1 << ((1 << cmp->p.width) - 1); > > ccu_mp_find_best(*parent_rate, rate, max_m, max_p, &m, &p); > + rate = *parent_rate / p / m; > + > + if (cmp->common.features & CCU_FEATURE_FIXED_POSTDIV) > + rate /= cmp->fixed_post_div; > > - return *parent_rate / p / m; > + return rate; > } > > static void ccu_mp_disable(struct clk_hw *hw) > @@ -83,6 +90,7 @@ static unsigned long ccu_mp_recalc_rate(struct clk_hw *hw, > unsigned long parent_rate) > { > struct ccu_mp *cmp = hw_to_ccu_mp(hw); > + unsigned long rate; > unsigned int m, p; > u32 reg; > > @@ -101,7 +109,11 @@ static unsigned long ccu_mp_recalc_rate(struct clk_hw *hw, > p = reg >> cmp->p.shift; > p &= (1 << cmp->p.width) - 1; > > - return (parent_rate >> p) / m; > + rate = (parent_rate >> p) / m; > + if (cmp->common.features & CCU_FEATURE_FIXED_POSTDIV) > + rate /= cmp->fixed_post_div; > + > + return rate; > } > > static int ccu_mp_determine_rate(struct clk_hw *hw, > @@ -129,6 +141,10 @@ static int ccu_mp_set_rate(struct clk_hw *hw, unsigned long rate, > max_m = cmp->m.max ?: 1 << cmp->m.width; > max_p = cmp->p.max ?: 1 << ((1 << cmp->p.width) - 1); > > + /* Adjust target rate according to post-dividers */ > + if (cmp->common.features & CCU_FEATURE_FIXED_POSTDIV) > + rate = rate * cmp->fixed_post_div; > + > ccu_mp_find_best(parent_rate, rate, max_m, max_p, &m, &p); > > spin_lock_irqsave(cmp->common.lock, flags); > diff --git a/drivers/clk/sunxi-ng/ccu_mp.h b/drivers/clk/sunxi-ng/ccu_mp.h > index aaef11d747ea..5107635e61de 100644 > --- a/drivers/clk/sunxi-ng/ccu_mp.h > +++ b/drivers/clk/sunxi-ng/ccu_mp.h > @@ -33,9 +33,33 @@ struct ccu_mp { > struct ccu_div_internal m; > struct ccu_div_internal p; > struct ccu_mux_internal mux; > + > + unsigned int fixed_post_div; > + > struct ccu_common common; > }; > > +#define SUNXI_CCU_MP_WITH_MUX_GATE_POSTDIV(_struct, _name, _parents, _reg, \ > + _mshift, _mwidth, \ > + _pshift, _pwidth, \ > + _muxshift, _muxwidth, \ > + _gate, _postdiv, _flags) \ > + struct ccu_mp _struct = { \ > + .enable = _gate, \ > + .m = _SUNXI_CCU_DIV(_mshift, _mwidth), \ > + .p = _SUNXI_CCU_DIV(_pshift, _pwidth), \ > + .mux = _SUNXI_CCU_MUX(_muxshift, _muxwidth), \ > + .fixed_post_div = _postdiv, \ > + .common = { \ > + .reg = _reg, \ > + .features = CCU_FEATURE_FIXED_POSTDIV, \ > + .hw.init = CLK_HW_INIT_PARENTS(_name, \ > + _parents, \ > + &ccu_mp_ops, \ > + _flags), \ > + } \ > + } > + This looks suspiciously like a copy of the macro below. What about you define the one below as a special case of this new one above? Should be even more straightforward with defaulting postdiv to 1 and loosing the feature flags. Cheers, Andre. > #define SUNXI_CCU_MP_WITH_MUX_GATE(_struct, _name, _parents, _reg, \ > _mshift, _mwidth, \ > _pshift, _pwidth, \ >
On Tue, Dec 5, 2017 at 7:18 AM, André Przywara <andre.przywara@arm.com> wrote: > Hi Chen-Yu, > > On 04/12/17 05:19, Chen-Yu Tsai wrote: >> On the A64, the MMC module clocks are fixed in the new timing mode, >> i.e. they do not have a bit to select the mode. These clocks have >> a 2x divider somewhere between the clock and the MMC module. >> >> To be consistent with other SoCs supporting the new timing mode, >> we model the 2x divider as a fixed post-divider on the MMC module >> clocks. >> >> To do this, we first add fixed post-divider to the MP style clocks, >> which the MMC module clocks are. >> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >> --- >> drivers/clk/sunxi-ng/ccu_mp.c | 20 ++++++++++++++++++-- >> drivers/clk/sunxi-ng/ccu_mp.h | 24 ++++++++++++++++++++++++ >> 2 files changed, 42 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/clk/sunxi-ng/ccu_mp.c b/drivers/clk/sunxi-ng/ccu_mp.c >> index 688855e7dc8c..5d0af4051737 100644 >> --- a/drivers/clk/sunxi-ng/ccu_mp.c >> +++ b/drivers/clk/sunxi-ng/ccu_mp.c >> @@ -50,12 +50,19 @@ static unsigned long ccu_mp_round_rate(struct ccu_mux_internal *mux, >> unsigned int max_m, max_p; >> unsigned int m, p; >> >> + if (cmp->common.features & CCU_FEATURE_FIXED_POSTDIV) >> + rate *= cmp->fixed_post_div; > > Can't you just initialise fixed_post_div to 1 normally and save the > CCU_FEATURE_FIXED_POSTDIV? I'll refer to Maxime about this. The feature flag was there from day one. We only started to implement support for it later on. I'm not sure if there was a reason to add them as feature flags, instead of a field that defaults to something (0 even). Otherwise it's a reasonable change. And we probably don't have to do a wholesale change for the other clocks in one go. Incidentally I have a A83T audio series that also adds post-dividers for another clock type. I'll wait for a conclusion on this end before posting it. > >> + >> max_m = cmp->m.max ?: 1 << cmp->m.width; >> max_p = cmp->p.max ?: 1 << ((1 << cmp->p.width) - 1); >> >> ccu_mp_find_best(*parent_rate, rate, max_m, max_p, &m, &p); >> + rate = *parent_rate / p / m; >> + >> + if (cmp->common.features & CCU_FEATURE_FIXED_POSTDIV) >> + rate /= cmp->fixed_post_div; >> >> - return *parent_rate / p / m; >> + return rate; >> } >> >> static void ccu_mp_disable(struct clk_hw *hw) >> @@ -83,6 +90,7 @@ static unsigned long ccu_mp_recalc_rate(struct clk_hw *hw, >> unsigned long parent_rate) >> { >> struct ccu_mp *cmp = hw_to_ccu_mp(hw); >> + unsigned long rate; >> unsigned int m, p; >> u32 reg; >> >> @@ -101,7 +109,11 @@ static unsigned long ccu_mp_recalc_rate(struct clk_hw *hw, >> p = reg >> cmp->p.shift; >> p &= (1 << cmp->p.width) - 1; >> >> - return (parent_rate >> p) / m; >> + rate = (parent_rate >> p) / m; >> + if (cmp->common.features & CCU_FEATURE_FIXED_POSTDIV) >> + rate /= cmp->fixed_post_div; >> + >> + return rate; >> } >> >> static int ccu_mp_determine_rate(struct clk_hw *hw, >> @@ -129,6 +141,10 @@ static int ccu_mp_set_rate(struct clk_hw *hw, unsigned long rate, >> max_m = cmp->m.max ?: 1 << cmp->m.width; >> max_p = cmp->p.max ?: 1 << ((1 << cmp->p.width) - 1); >> >> + /* Adjust target rate according to post-dividers */ >> + if (cmp->common.features & CCU_FEATURE_FIXED_POSTDIV) >> + rate = rate * cmp->fixed_post_div; >> + >> ccu_mp_find_best(parent_rate, rate, max_m, max_p, &m, &p); >> >> spin_lock_irqsave(cmp->common.lock, flags); >> diff --git a/drivers/clk/sunxi-ng/ccu_mp.h b/drivers/clk/sunxi-ng/ccu_mp.h >> index aaef11d747ea..5107635e61de 100644 >> --- a/drivers/clk/sunxi-ng/ccu_mp.h >> +++ b/drivers/clk/sunxi-ng/ccu_mp.h >> @@ -33,9 +33,33 @@ struct ccu_mp { >> struct ccu_div_internal m; >> struct ccu_div_internal p; >> struct ccu_mux_internal mux; >> + >> + unsigned int fixed_post_div; >> + >> struct ccu_common common; >> }; >> >> +#define SUNXI_CCU_MP_WITH_MUX_GATE_POSTDIV(_struct, _name, _parents, _reg, \ >> + _mshift, _mwidth, \ >> + _pshift, _pwidth, \ >> + _muxshift, _muxwidth, \ >> + _gate, _postdiv, _flags) \ >> + struct ccu_mp _struct = { \ >> + .enable = _gate, \ >> + .m = _SUNXI_CCU_DIV(_mshift, _mwidth), \ >> + .p = _SUNXI_CCU_DIV(_pshift, _pwidth), \ >> + .mux = _SUNXI_CCU_MUX(_muxshift, _muxwidth), \ >> + .fixed_post_div = _postdiv, \ >> + .common = { \ >> + .reg = _reg, \ >> + .features = CCU_FEATURE_FIXED_POSTDIV, \ >> + .hw.init = CLK_HW_INIT_PARENTS(_name, \ >> + _parents, \ >> + &ccu_mp_ops, \ >> + _flags), \ >> + } \ >> + } >> + > > This looks suspiciously like a copy of the macro below. What about you > define the one below as a special case of this new one above? Correct. But you can't unset the feature flag. So a copy is needed. > Should be even more straightforward with defaulting postdiv to 1 and > loosing the feature flags. See above about the feature flag. ChenYu > > Cheers, > Andre. > >> #define SUNXI_CCU_MP_WITH_MUX_GATE(_struct, _name, _parents, _reg, \ >> _mshift, _mwidth, \ >> _pshift, _pwidth, \ >> >
1;5002;0c On Tue, Dec 05, 2017 at 11:01:11AM +0800, Chen-Yu Tsai wrote: > On Tue, Dec 5, 2017 at 7:18 AM, André Przywara <andre.przywara@arm.com> wrote: > > Hi Chen-Yu, > > > > On 04/12/17 05:19, Chen-Yu Tsai wrote: > >> On the A64, the MMC module clocks are fixed in the new timing mode, > >> i.e. they do not have a bit to select the mode. These clocks have > >> a 2x divider somewhere between the clock and the MMC module. > >> > >> To be consistent with other SoCs supporting the new timing mode, > >> we model the 2x divider as a fixed post-divider on the MMC module > >> clocks. > >> > >> To do this, we first add fixed post-divider to the MP style clocks, > >> which the MMC module clocks are. > >> > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> > >> --- > >> drivers/clk/sunxi-ng/ccu_mp.c | 20 ++++++++++++++++++-- > >> drivers/clk/sunxi-ng/ccu_mp.h | 24 ++++++++++++++++++++++++ > >> 2 files changed, 42 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/clk/sunxi-ng/ccu_mp.c b/drivers/clk/sunxi-ng/ccu_mp.c > >> index 688855e7dc8c..5d0af4051737 100644 > >> --- a/drivers/clk/sunxi-ng/ccu_mp.c > >> +++ b/drivers/clk/sunxi-ng/ccu_mp.c > >> @@ -50,12 +50,19 @@ static unsigned long ccu_mp_round_rate(struct ccu_mux_internal *mux, > >> unsigned int max_m, max_p; > >> unsigned int m, p; > >> > >> + if (cmp->common.features & CCU_FEATURE_FIXED_POSTDIV) > >> + rate *= cmp->fixed_post_div; > > > > Can't you just initialise fixed_post_div to 1 normally and save the > > CCU_FEATURE_FIXED_POSTDIV? > > I'll refer to Maxime about this. The feature flag was there from day > one. We only started to implement support for it later on. I'm not > sure if there was a reason to add them as feature flags, instead of > a field that defaults to something (0 even). > > Otherwise it's a reasonable change. And we probably don't have to > do a wholesale change for the other clocks in one go. Incidentally > I have a A83T audio series that also adds post-dividers for another > clock type. I'll wait for a conclusion on this end before posting > it. We can definitely remove that feature flag. However, that would also mean going over all the clocks we define everywhere to set it to 1, which is going to be cumbersome and likely to introduce some bugs. I don't really want to tie those two patches to that effort. I applied both, thanks! Maxime
diff --git a/drivers/clk/sunxi-ng/ccu_mp.c b/drivers/clk/sunxi-ng/ccu_mp.c index 688855e7dc8c..5d0af4051737 100644 --- a/drivers/clk/sunxi-ng/ccu_mp.c +++ b/drivers/clk/sunxi-ng/ccu_mp.c @@ -50,12 +50,19 @@ static unsigned long ccu_mp_round_rate(struct ccu_mux_internal *mux, unsigned int max_m, max_p; unsigned int m, p; + if (cmp->common.features & CCU_FEATURE_FIXED_POSTDIV) + rate *= cmp->fixed_post_div; + max_m = cmp->m.max ?: 1 << cmp->m.width; max_p = cmp->p.max ?: 1 << ((1 << cmp->p.width) - 1); ccu_mp_find_best(*parent_rate, rate, max_m, max_p, &m, &p); + rate = *parent_rate / p / m; + + if (cmp->common.features & CCU_FEATURE_FIXED_POSTDIV) + rate /= cmp->fixed_post_div; - return *parent_rate / p / m; + return rate; } static void ccu_mp_disable(struct clk_hw *hw) @@ -83,6 +90,7 @@ static unsigned long ccu_mp_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) { struct ccu_mp *cmp = hw_to_ccu_mp(hw); + unsigned long rate; unsigned int m, p; u32 reg; @@ -101,7 +109,11 @@ static unsigned long ccu_mp_recalc_rate(struct clk_hw *hw, p = reg >> cmp->p.shift; p &= (1 << cmp->p.width) - 1; - return (parent_rate >> p) / m; + rate = (parent_rate >> p) / m; + if (cmp->common.features & CCU_FEATURE_FIXED_POSTDIV) + rate /= cmp->fixed_post_div; + + return rate; } static int ccu_mp_determine_rate(struct clk_hw *hw, @@ -129,6 +141,10 @@ static int ccu_mp_set_rate(struct clk_hw *hw, unsigned long rate, max_m = cmp->m.max ?: 1 << cmp->m.width; max_p = cmp->p.max ?: 1 << ((1 << cmp->p.width) - 1); + /* Adjust target rate according to post-dividers */ + if (cmp->common.features & CCU_FEATURE_FIXED_POSTDIV) + rate = rate * cmp->fixed_post_div; + ccu_mp_find_best(parent_rate, rate, max_m, max_p, &m, &p); spin_lock_irqsave(cmp->common.lock, flags); diff --git a/drivers/clk/sunxi-ng/ccu_mp.h b/drivers/clk/sunxi-ng/ccu_mp.h index aaef11d747ea..5107635e61de 100644 --- a/drivers/clk/sunxi-ng/ccu_mp.h +++ b/drivers/clk/sunxi-ng/ccu_mp.h @@ -33,9 +33,33 @@ struct ccu_mp { struct ccu_div_internal m; struct ccu_div_internal p; struct ccu_mux_internal mux; + + unsigned int fixed_post_div; + struct ccu_common common; }; +#define SUNXI_CCU_MP_WITH_MUX_GATE_POSTDIV(_struct, _name, _parents, _reg, \ + _mshift, _mwidth, \ + _pshift, _pwidth, \ + _muxshift, _muxwidth, \ + _gate, _postdiv, _flags) \ + struct ccu_mp _struct = { \ + .enable = _gate, \ + .m = _SUNXI_CCU_DIV(_mshift, _mwidth), \ + .p = _SUNXI_CCU_DIV(_pshift, _pwidth), \ + .mux = _SUNXI_CCU_MUX(_muxshift, _muxwidth), \ + .fixed_post_div = _postdiv, \ + .common = { \ + .reg = _reg, \ + .features = CCU_FEATURE_FIXED_POSTDIV, \ + .hw.init = CLK_HW_INIT_PARENTS(_name, \ + _parents, \ + &ccu_mp_ops, \ + _flags), \ + } \ + } + #define SUNXI_CCU_MP_WITH_MUX_GATE(_struct, _name, _parents, _reg, \ _mshift, _mwidth, \ _pshift, _pwidth, \
On the A64, the MMC module clocks are fixed in the new timing mode, i.e. they do not have a bit to select the mode. These clocks have a 2x divider somewhere between the clock and the MMC module. To be consistent with other SoCs supporting the new timing mode, we model the 2x divider as a fixed post-divider on the MMC module clocks. To do this, we first add fixed post-divider to the MP style clocks, which the MMC module clocks are. Signed-off-by: Chen-Yu Tsai <wens@csie.org> --- drivers/clk/sunxi-ng/ccu_mp.c | 20 ++++++++++++++++++-- drivers/clk/sunxi-ng/ccu_mp.h | 24 ++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-)