| Message ID | 1456745963-2403-6-git-send-email-kernel@martin.sperl.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
kernel@martin.sperl.org writes: > From: Martin Sperl <kernel@martin.sperl.org> > > The current driver calculates the clock divider with > fractional support enabled. > > But it does not enable fractional support in the > control register itself resulting in an integer only divider, > but in clk_set_rate responds back the fractionally divided > clock frequency. > > This patch enables fractional support in the control register > whenever there is a fractional bit set in the requested clock divider. > > Mash clock limits are are also handled for the PWM clock > applying the correct divider limits (2 and max_int) applicable to > basic fractional divider support (mash order of 1). > > It also adds locking to protect the read/modify/write cycle of > the register modification. > > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the > audio domain clocks") > > Signed-off-by: Martin Sperl <kernel@martin.sperl.org> Rewrite of the commit message: The driver has been using fractional dividers for all clocks with fractional bits. However, MASH clocks (those used to drive audio) only do fractional division when in MASH mode, which is used to push clock jitter out of the audio band. The PWM clock (used to drive i2s audio) is the only MASH clock currently enabled in the driver. Additional MASH modes are available that widen the frequency range, but only 1-level MASH is used for now, until we characterize some better policy. Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks") Signed-off-by: Martin Sperl <kernel@martin.sperl.org> > --- > drivers/clk/bcm/clk-bcm2835.c | 45 +++++++++++++++++++++++++++++++++++------ > 1 file changed, 39 insertions(+), 6 deletions(-) > > diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c > index edb1f74..da77069 100644 > --- a/drivers/clk/bcm/clk-bcm2835.c > +++ b/drivers/clk/bcm/clk-bcm2835.c > @@ -52,6 +52,7 @@ > #define CM_GNRICCTL 0x000 > #define CM_GNRICDIV 0x004 > # define CM_DIV_FRAC_BITS 12 > +# define CM_DIV_FRAC_MASK GENMASK(CM_DIV_FRAC_BITS - 1, 0) > > #define CM_VPUCTL 0x008 > #define CM_VPUDIV 0x00c > @@ -129,6 +130,7 @@ > # define CM_GATE BIT(CM_GATE_BIT) > # define CM_BUSY BIT(7) > # define CM_BUSYD BIT(8) > +# define CM_FRAC BIT(9) This is just the low bit of the 2-bit CM_MASH field at 9:10. So you're enabling MASH mode 1 in this patch. Similarly, the subject of the patch should be something like "Fix MASH-based fractional clock support for PWM" since all the other clocks work with fractional already. > # define CM_SRC_SHIFT 0 > # define CM_SRC_BITS 4 > # define CM_SRC_MASK 0xf > @@ -648,6 +650,7 @@ struct bcm2835_clock_data { > u32 frac_bits; > > bool is_vpu_clock; > + bool is_mash_clock; > }; > > static const char *const bcm2835_clock_per_parents[] = { > @@ -829,6 +832,7 @@ static const struct bcm2835_clock_data bcm2835_clock_pwm_data = { > .div_reg = CM_PWMDIV, > .int_bits = 12, > .frac_bits = 12, > + .is_mash_clock = true, > }; > > struct bcm2835_pll { > @@ -1184,7 +1188,7 @@ static u32 bcm2835_clock_choose_div(struct clk_hw *hw, > GENMASK(CM_DIV_FRAC_BITS - data->frac_bits, 0) >> 1; > u64 temp = (u64)parent_rate << CM_DIV_FRAC_BITS; > u64 rem; > - u32 div; > + u32 div, mindiv, maxdiv; > > rem = do_div(temp, rate); > div = temp; > @@ -1194,11 +1198,23 @@ static u32 bcm2835_clock_choose_div(struct clk_hw *hw, > div += unused_frac_mask + 1; > div &= ~unused_frac_mask; > > - /* clamp to min divider of 1 */ > - div = max_t(u32, div, 1 << CM_DIV_FRAC_BITS); > - /* clamp to the highest possible fractional divider */ > - div = min_t(u32, div, GENMASK(data->int_bits + CM_DIV_FRAC_BITS - 1, > - CM_DIV_FRAC_BITS - data->frac_bits)); > + /* different clamping limits apply for a mash clock */ > + if (data->is_mash_clock) { > + /* clamp to min divider of 2 */ > + mindiv = 2 << CM_DIV_FRAC_BITS; > + /* clamp to the highest possible integer divider */ > + maxdiv = (BIT(data->int_bits) - 1) << CM_DIV_FRAC_BITS; > + } else { > + /* clamp to min divider of 1 */ > + mindiv = 1 << CM_DIV_FRAC_BITS; > + /* clamp to the highest possible fractional divider */ > + maxdiv = GENMASK(data->int_bits + CM_DIV_FRAC_BITS - 1, > + CM_DIV_FRAC_BITS - data->frac_bits); > + } > + > + /* apply the clamping limits */ > + div = max_t(u32, div, mindiv); > + div = min_t(u32, div, maxdiv); > > return div; > } > @@ -1292,9 +1308,26 @@ static int bcm2835_clock_set_rate(struct clk_hw *hw, > struct bcm2835_cprman *cprman = clock->cprman; > const struct bcm2835_clock_data *data = clock->data; > u32 div = bcm2835_clock_choose_div(hw, rate, parent_rate, false); > + u32 ctl; > + > + spin_lock(&cprman->regs_lock); > + > + /* > + * Setting up frac support > + * > + * In principle it is recommended to stop/start the clock first, > + * but as we set CLK_SET_RATE_GATE during registration of the > + * clock this requirement should be take care of by the > + * clk-framework. > + */ > + ctl = cprman_read(cprman, data->ctl_reg) & ~CM_FRAC; > + ctl |= (div & CM_DIV_FRAC_MASK) ? CM_FRAC : 0; > + cprman_write(cprman, data->ctl_reg, ctl); This should all be under "if (data->is_mash_clock) {}" since it doesn't do anything on non-mash clocks. > > cprman_write(cprman, data->div_reg, div); > > + spin_unlock(&cprman->regs_lock); > + > return 0; > }
> On 29.02.2016, at 21:33, Eric Anholt <eric@anholt.net> wrote: > > kernel@martin.sperl.org writes: > >> From: Martin Sperl <kernel@martin.sperl.org> >> >> The current driver calculates the clock divider with >> fractional support enabled. >> >> But it does not enable fractional support in the >> control register itself resulting in an integer only divider, >> but in clk_set_rate responds back the fractionally divided >> clock frequency. >> >> This patch enables fractional support in the control register >> whenever there is a fractional bit set in the requested clock divider. >> >> Mash clock limits are are also handled for the PWM clock >> applying the correct divider limits (2 and max_int) applicable to >> basic fractional divider support (mash order of 1). >> >> It also adds locking to protect the read/modify/write cycle of >> the register modification. >> >> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the >> audio domain clocks") >> >> Signed-off-by: Martin Sperl <kernel@martin.sperl.org> > > Rewrite of the commit message: > > The driver has been using fractional dividers for all clocks with > fractional bits. However, MASH clocks (those used to drive audio) only > do fractional division when in MASH mode, which is used to push clock > jitter out of the audio band. The PWM clock (used to drive i2s audio) > is the only MASH clock currently enabled in the driver. > > Additional MASH modes are available that widen the frequency range, but > only 1-level MASH is used for now, until we characterize some better > policy. > > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the > audio domain clocks”) That is not 100% true - non mash modes have CM_FRAC and CM_FRAC = CM_MASH0 so these map each other And for a non-mash clock if CM_FRAC is not set then the factual divider support is not used and only the integer portion is used. This results in a frequency that is to high (as the frac bits are missing). Just to giv an example of firmware set clock for the timer: root@raspcm:~# head /sys/kernel/debug/clk/timer/current_* ==> /sys/kernel/debug/clk/timer/current_divf <== 819 ==> /sys/kernel/debug/clk/timer/current_divi <== 19 ==> /sys/kernel/debug/clk/timer/current_frac <== 1 ==> /sys/kernel/debug/clk/timer/current_parent <== xosc root@raspcm:~# head /sys/kernel/debug/clk/timer/regdump ctl = 0x00000291 div = 0x00013333 So this - non mash - clock has frac set by default and this clock is set up by the firmware! Similar things for: hsm and uart - all of them are “normal” clocks and they have the CM_FRAC bit set and divf > 0. So the code essentially sets: * CM_FRAC for “normal” clocks * CM_MASH0 for mash clocks when the divider has a fract component. So I believe my description is correct. But anyway: except for the pcm clocks clk_set_rate is not really being used right now, as we still consume the clocks set up by the firmware. The comment portion about mash-order=1 could probably get added. As for enabling higher order mash modes - at least in the audio domain using I2S I have done some interresting measurements making use of all the variations (and clock selections). The result there was that - at least with the DAC that I am using - the noise level varies widely between different mash modes and clock dividers. So the “lowest” noise showed the following combinations: osc-mash3 pllc-mash2 osc-mash1 osc-non-frac (optimized so that we may use integer divider) osc-mash2 pllc-mash1 pllc-mash3 So the "selection” of clocks/divider/mash is really requiring fine tuning and measurement - because there is no real pattern visible. Here a graph showing the audio spectrum for the different modes with a 400Hz signal: https://cloud.githubusercontent.com/assets/2638784/13278003/20e09002-dacd-11e5-8d62-17eb7118a4fa.jpg > > This is just the low bit of the 2-bit CM_MASH field at 9:10. So you're > enabling MASH mode 1 in this patch. Similarly, the subject of the patch > should be something like "Fix MASH-based fractional clock support for > PWM" since all the other clocks work with fractional already. > As explained above: no they do not work without setting the FRAC bit! Take again the example of timer: when not using FRAC you get a timer clock of: 1010526Hz while with FRAC you get 1000002 Hz So you NEED to set FRAC for all. >> >> >> + ctl = cprman_read(cprman, data->ctl_reg) & ~CM_FRAC; >> + ctl |= (div & CM_DIV_FRAC_MASK) ? CM_FRAC : 0; >> + cprman_write(cprman, data->ctl_reg, ctl); > > This should all be under "if (data->is_mash_clock) {}" since it doesn't > do anything on non-mash clocks. Again: without CM_FRAC you get a frequency that is too high, so it needs to be implemented for all - independent of if it is a mash clock or a FRAC clock. The only cases where it does not apply is for clocks that have no fract bits at all.
Martin Sperl <kernel@martin.sperl.org> writes: >> On 29.02.2016, at 21:33, Eric Anholt <eric@anholt.net> wrote: >> >> kernel@martin.sperl.org writes: >> >>> From: Martin Sperl <kernel@martin.sperl.org> >>> >>> The current driver calculates the clock divider with >>> fractional support enabled. >>> >>> But it does not enable fractional support in the >>> control register itself resulting in an integer only divider, >>> but in clk_set_rate responds back the fractionally divided >>> clock frequency. >>> >>> This patch enables fractional support in the control register >>> whenever there is a fractional bit set in the requested clock divider. >>> >>> Mash clock limits are are also handled for the PWM clock >>> applying the correct divider limits (2 and max_int) applicable to >>> basic fractional divider support (mash order of 1). >>> >>> It also adds locking to protect the read/modify/write cycle of >>> the register modification. >>> >>> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the >>> audio domain clocks") >>> >>> Signed-off-by: Martin Sperl <kernel@martin.sperl.org> >> >> Rewrite of the commit message: >> >> The driver has been using fractional dividers for all clocks with >> fractional bits. However, MASH clocks (those used to drive audio) only >> do fractional division when in MASH mode, which is used to push clock >> jitter out of the audio band. The PWM clock (used to drive i2s audio) >> is the only MASH clock currently enabled in the driver. >> >> Additional MASH modes are available that widen the frequency range, but >> only 1-level MASH is used for now, until we characterize some better >> policy. >> >> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the >> audio domain clocks”) > > That is not 100% true - non mash modes have CM_FRAC and CM_FRAC = CM_MASH0 > so these map each other Once again, trusting the docs turns out to be a bad idea. You're right, the non-MASH clocks *do* have a bit 9 to enable fractional mode. Sigh. So, this patch is: Reviewed-by: Eric Anholt <eric@anholt.net>
> On 01.03.2016, at 00:44, Eric Anholt <eric@anholt.net> wrote: > > Once again, trusting the docs turns out to be a bad idea. You're right, > the non-MASH clocks *do* have a bit 9 to enable fractional mode. Sigh. > > So, this patch is: > > Reviewed-by: Eric Anholt <eric@anholt.net> The only really reliable “docs” on registers I have found outside of probably signing a NDA are those header files for the VC4 provided by broadcom. That is why I have taken the effort to translate those into something much more easily readable/searchable (i.e html pages on the web). And that is where I have “guessed” all the information on the clock tree structure. The main thing that is sometimes missing from those documents is some extra context like: what is that pllt clock with a mux of 8 parent clocks (not 4 or 16 as typical) and what do those corresponding 4 counter really do? See: https://github.com/msperl/rpi-registers/blob/master/md/Region_CM.md#cm_plltctl Martin
diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c index edb1f74..da77069 100644 --- a/drivers/clk/bcm/clk-bcm2835.c +++ b/drivers/clk/bcm/clk-bcm2835.c @@ -52,6 +52,7 @@ #define CM_GNRICCTL 0x000 #define CM_GNRICDIV 0x004 # define CM_DIV_FRAC_BITS 12 +# define CM_DIV_FRAC_MASK GENMASK(CM_DIV_FRAC_BITS - 1, 0) #define CM_VPUCTL 0x008 #define CM_VPUDIV 0x00c @@ -129,6 +130,7 @@ # define CM_GATE BIT(CM_GATE_BIT) # define CM_BUSY BIT(7) # define CM_BUSYD BIT(8) +# define CM_FRAC BIT(9) # define CM_SRC_SHIFT 0 # define CM_SRC_BITS 4 # define CM_SRC_MASK 0xf @@ -648,6 +650,7 @@ struct bcm2835_clock_data { u32 frac_bits; bool is_vpu_clock; + bool is_mash_clock; }; static const char *const bcm2835_clock_per_parents[] = { @@ -829,6 +832,7 @@ static const struct bcm2835_clock_data bcm2835_clock_pwm_data = { .div_reg = CM_PWMDIV, .int_bits = 12, .frac_bits = 12, + .is_mash_clock = true, }; struct bcm2835_pll { @@ -1184,7 +1188,7 @@ static u32 bcm2835_clock_choose_div(struct clk_hw *hw, GENMASK(CM_DIV_FRAC_BITS - data->frac_bits, 0) >> 1; u64 temp = (u64)parent_rate << CM_DIV_FRAC_BITS; u64 rem; - u32 div; + u32 div, mindiv, maxdiv; rem = do_div(temp, rate); div = temp; @@ -1194,11 +1198,23 @@ static u32 bcm2835_clock_choose_div(struct clk_hw *hw, div += unused_frac_mask + 1; div &= ~unused_frac_mask; - /* clamp to min divider of 1 */ - div = max_t(u32, div, 1 << CM_DIV_FRAC_BITS); - /* clamp to the highest possible fractional divider */ - div = min_t(u32, div, GENMASK(data->int_bits + CM_DIV_FRAC_BITS - 1, - CM_DIV_FRAC_BITS - data->frac_bits)); + /* different clamping limits apply for a mash clock */ + if (data->is_mash_clock) { + /* clamp to min divider of 2 */ + mindiv = 2 << CM_DIV_FRAC_BITS; + /* clamp to the highest possible integer divider */ + maxdiv = (BIT(data->int_bits) - 1) << CM_DIV_FRAC_BITS; + } else { + /* clamp to min divider of 1 */ + mindiv = 1 << CM_DIV_FRAC_BITS; + /* clamp to the highest possible fractional divider */ + maxdiv = GENMASK(data->int_bits + CM_DIV_FRAC_BITS - 1, + CM_DIV_FRAC_BITS - data->frac_bits); + } + + /* apply the clamping limits */ + div = max_t(u32, div, mindiv); + div = min_t(u32, div, maxdiv); return div; } @@ -1292,9 +1308,26 @@ static int bcm2835_clock_set_rate(struct clk_hw *hw, struct bcm2835_cprman *cprman = clock->cprman; const struct bcm2835_clock_data *data = clock->data; u32 div = bcm2835_clock_choose_div(hw, rate, parent_rate, false); + u32 ctl; + + spin_lock(&cprman->regs_lock); + + /* + * Setting up frac support + * + * In principle it is recommended to stop/start the clock first, + * but as we set CLK_SET_RATE_GATE during registration of the + * clock this requirement should be take care of by the + * clk-framework. + */ + ctl = cprman_read(cprman, data->ctl_reg) & ~CM_FRAC; + ctl |= (div & CM_DIV_FRAC_MASK) ? CM_FRAC : 0; + cprman_write(cprman, data->ctl_reg, ctl); cprman_write(cprman, data->div_reg, div); + spin_unlock(&cprman->regs_lock); + return 0; }