| Message ID | 1531154338-3998-3-git-send-email-avienamo@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Multiplex sdmmc low jitter clock path | expand |
On 09/07/18 17:38, Aapo Vienamo wrote: > From: Peter De Schrijver <pdeschrijver@nvidia.com> > > Move this to a separate file so it can be used to calculate the sdmmc > clock dividers. Sorry for not commenting sooner, but what is the motivation for moving this to its own file? I don't see why we need to do this in order to use elsewhere. Furthermore, the original file is quite aptly named 'clk-divider.c' and now we have a div71.c which seems quite specific. > Signed-off-by: Peter De-Schrijver <pdeschrijver@nvidia.com> > Signed-off-by: Aapo Vienamo <avienamo@nvidia.com> > Acked-by: Peter De Schrijver <pdeschrijver@nvidia.com> > --- > drivers/clk/tegra/Makefile | 1 + > drivers/clk/tegra/clk-divider.c | 30 +++++----------------------- > drivers/clk/tegra/clk.h | 3 +++ > drivers/clk/tegra/div71.c | 43 +++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 52 insertions(+), 25 deletions(-) > create mode 100644 drivers/clk/tegra/div71.c > > diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile > index b716923..6d4f563 100644 > --- a/drivers/clk/tegra/Makefile > +++ b/drivers/clk/tegra/Makefile > @@ -24,3 +24,4 @@ obj-$(CONFIG_ARCH_TEGRA_132_SOC) += clk-tegra124.o > obj-y += cvb.o > obj-$(CONFIG_ARCH_TEGRA_210_SOC) += clk-tegra210.o > obj-$(CONFIG_CLK_TEGRA_BPMP) += clk-bpmp.o > +obj-y += div71.o > diff --git a/drivers/clk/tegra/clk-divider.c b/drivers/clk/tegra/clk-divider.c > index 16e0aee..ad87858 100644 > --- a/drivers/clk/tegra/clk-divider.c > +++ b/drivers/clk/tegra/clk-divider.c > @@ -32,35 +32,15 @@ > static int get_div(struct tegra_clk_frac_div *divider, unsigned long rate, > unsigned long parent_rate) > { > - u64 divider_ux1 = parent_rate; > - u8 flags = divider->flags; > - int mul; > - > - if (!rate) > - return 0; > - > - mul = get_mul(divider); > - > - if (!(flags & TEGRA_DIVIDER_INT)) > - divider_ux1 *= mul; > - > - if (flags & TEGRA_DIVIDER_ROUND_UP) > - divider_ux1 += rate - 1; > - > - do_div(divider_ux1, rate); > - > - if (flags & TEGRA_DIVIDER_INT) > - divider_ux1 *= mul; > + int div; > > - divider_ux1 -= mul; > + div = div71_get(rate, parent_rate, divider->width, divider->frac_width, > + divider->flags); > > - if ((s64)divider_ux1 < 0) > + if (div < 0) > return 0; > > - if (divider_ux1 > get_max_div(divider)) > - return get_max_div(divider); > - > - return divider_ux1; > + return div; > } > > static unsigned long clk_frac_div_recalc_rate(struct clk_hw *hw, > diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h > index e3b9c22..149cc70 100644 > --- a/drivers/clk/tegra/clk.h > +++ b/drivers/clk/tegra/clk.h > @@ -812,6 +812,9 @@ extern tegra_clk_apply_init_table_func tegra_clk_apply_init_table; > int tegra_pll_wait_for_lock(struct tegra_clk_pll *pll); > u16 tegra_pll_get_fixed_mdiv(struct clk_hw *hw, unsigned long input_rate); > int tegra_pll_p_div_to_hw(struct tegra_clk_pll *pll, u8 p_div); > +int div71_get(unsigned long rate, unsigned parent_rate, u8 width, > + u8 frac_width, u8 flags); > + > > /* Combined read fence with delay */ > #define fence_udelay(delay, reg) \ > diff --git a/drivers/clk/tegra/div71.c b/drivers/clk/tegra/div71.c > new file mode 100644 > index 0000000..1eecc84 > --- /dev/null > +++ b/drivers/clk/tegra/div71.c > @@ -0,0 +1,43 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved. > + */ > + > +#include <asm/div64.h> > + > +#include "clk.h" > + > +#define div_mask(w) ((1 << (w)) - 1) > + > +int div71_get(unsigned long rate, unsigned parent_rate, u8 width, > + u8 frac_width, u8 flags) > +{ > + u64 divider_ux1 = parent_rate; > + int mul; > + > + if (!rate) > + return 0; > + > + mul = 1 << frac_width; > + > + if (!(flags & TEGRA_DIVIDER_INT)) > + divider_ux1 *= mul; > + > + if (flags & TEGRA_DIVIDER_ROUND_UP) > + divider_ux1 += rate - 1; > + > + do_div(divider_ux1, rate); > + > + if (flags & TEGRA_DIVIDER_INT) > + divider_ux1 *= mul; > + > + if (divider_ux1 < mul) > + return 0; > + > + divider_ux1 -= mul; > + > + if (divider_ux1 > div_mask(width)) > + return div_mask(width); > + > + return divider_ux1; > +} I don't see anything in the above that makes this 7.1? It seems that the fractional width is being passed meaning it could be m.n unless I am missing something. Cheers Jon
On Tue, Jul 10, 2018 at 05:17:05PM +0100, Jon Hunter wrote: > > On 09/07/18 17:38, Aapo Vienamo wrote: > > From: Peter De Schrijver <pdeschrijver@nvidia.com> > > > > Move this to a separate file so it can be used to calculate the sdmmc > > clock dividers. > > Sorry for not commenting sooner, but what is the motivation for moving > this to its own file? I don't see why we need to do this in order to use > elsewhere. Furthermore, the original file is quite aptly named 'clk-divider.c' > and now we have a div71.c which seems quite specific. How else would you do it? Peter. > > Signed-off-by: Peter De-Schrijver <pdeschrijver@nvidia.com> > > Signed-off-by: Aapo Vienamo <avienamo@nvidia.com> > > Acked-by: Peter De Schrijver <pdeschrijver@nvidia.com> > > --- > > drivers/clk/tegra/Makefile | 1 + > > drivers/clk/tegra/clk-divider.c | 30 +++++----------------------- > > drivers/clk/tegra/clk.h | 3 +++ > > drivers/clk/tegra/div71.c | 43 +++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 52 insertions(+), 25 deletions(-) > > create mode 100644 drivers/clk/tegra/div71.c > > > > diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile > > index b716923..6d4f563 100644 > > --- a/drivers/clk/tegra/Makefile > > +++ b/drivers/clk/tegra/Makefile > > @@ -24,3 +24,4 @@ obj-$(CONFIG_ARCH_TEGRA_132_SOC) += clk-tegra124.o > > obj-y += cvb.o > > obj-$(CONFIG_ARCH_TEGRA_210_SOC) += clk-tegra210.o > > obj-$(CONFIG_CLK_TEGRA_BPMP) += clk-bpmp.o > > +obj-y += div71.o > > diff --git a/drivers/clk/tegra/clk-divider.c b/drivers/clk/tegra/clk-divider.c > > index 16e0aee..ad87858 100644 > > --- a/drivers/clk/tegra/clk-divider.c > > +++ b/drivers/clk/tegra/clk-divider.c > > @@ -32,35 +32,15 @@ > > static int get_div(struct tegra_clk_frac_div *divider, unsigned long rate, > > unsigned long parent_rate) > > { > > - u64 divider_ux1 = parent_rate; > > - u8 flags = divider->flags; > > - int mul; > > - > > - if (!rate) > > - return 0; > > - > > - mul = get_mul(divider); > > - > > - if (!(flags & TEGRA_DIVIDER_INT)) > > - divider_ux1 *= mul; > > - > > - if (flags & TEGRA_DIVIDER_ROUND_UP) > > - divider_ux1 += rate - 1; > > - > > - do_div(divider_ux1, rate); > > - > > - if (flags & TEGRA_DIVIDER_INT) > > - divider_ux1 *= mul; > > + int div; > > > > - divider_ux1 -= mul; > > + div = div71_get(rate, parent_rate, divider->width, divider->frac_width, > > + divider->flags); > > > > - if ((s64)divider_ux1 < 0) > > + if (div < 0) > > return 0; > > > > - if (divider_ux1 > get_max_div(divider)) > > - return get_max_div(divider); > > - > > - return divider_ux1; > > + return div; > > } > > > > static unsigned long clk_frac_div_recalc_rate(struct clk_hw *hw, > > diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h > > index e3b9c22..149cc70 100644 > > --- a/drivers/clk/tegra/clk.h > > +++ b/drivers/clk/tegra/clk.h > > @@ -812,6 +812,9 @@ extern tegra_clk_apply_init_table_func tegra_clk_apply_init_table; > > int tegra_pll_wait_for_lock(struct tegra_clk_pll *pll); > > u16 tegra_pll_get_fixed_mdiv(struct clk_hw *hw, unsigned long input_rate); > > int tegra_pll_p_div_to_hw(struct tegra_clk_pll *pll, u8 p_div); > > +int div71_get(unsigned long rate, unsigned parent_rate, u8 width, > > + u8 frac_width, u8 flags); > > + > > > > /* Combined read fence with delay */ > > #define fence_udelay(delay, reg) \ > > diff --git a/drivers/clk/tegra/div71.c b/drivers/clk/tegra/div71.c > > new file mode 100644 > > index 0000000..1eecc84 > > --- /dev/null > > +++ b/drivers/clk/tegra/div71.c > > @@ -0,0 +1,43 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved. > > + */ > > + > > +#include <asm/div64.h> > > + > > +#include "clk.h" > > + > > +#define div_mask(w) ((1 << (w)) - 1) > > + > > +int div71_get(unsigned long rate, unsigned parent_rate, u8 width, > > + u8 frac_width, u8 flags) > > +{ > > + u64 divider_ux1 = parent_rate; > > + int mul; > > + > > + if (!rate) > > + return 0; > > + > > + mul = 1 << frac_width; > > + > > + if (!(flags & TEGRA_DIVIDER_INT)) > > + divider_ux1 *= mul; > > + > > + if (flags & TEGRA_DIVIDER_ROUND_UP) > > + divider_ux1 += rate - 1; > > + > > + do_div(divider_ux1, rate); > > + > > + if (flags & TEGRA_DIVIDER_INT) > > + divider_ux1 *= mul; > > + > > + if (divider_ux1 < mul) > > + return 0; > > + > > + divider_ux1 -= mul; > > + > > + if (divider_ux1 > div_mask(width)) > > + return div_mask(width); > > + > > + return divider_ux1; > > +} > > I don't see anything in the above that makes this 7.1? It seems that the > fractional width is being passed meaning it could be m.n unless I am missing > something. > > Cheers > Jon > > -- > nvpublic > -- > To unsubscribe from this list: send the line "unsubscribe linux-clk" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/07/18 09:00, Peter De Schrijver wrote: > On Tue, Jul 10, 2018 at 05:17:05PM +0100, Jon Hunter wrote: >> >> On 09/07/18 17:38, Aapo Vienamo wrote: >>> From: Peter De Schrijver <pdeschrijver@nvidia.com> >>> >>> Move this to a separate file so it can be used to calculate the sdmmc >>> clock dividers. >> >> Sorry for not commenting sooner, but what is the motivation for moving >> this to its own file? I don't see why we need to do this in order to use >> elsewhere. Furthermore, the original file is quite aptly named 'clk-divider.c' >> and now we have a div71.c which seems quite specific. > > How else would you do it? Keep it in the same file? Jon -- nvpublic -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 11, 2018 at 09:42:20AM +0100, Jon Hunter wrote: > > On 11/07/18 09:00, Peter De Schrijver wrote: > > On Tue, Jul 10, 2018 at 05:17:05PM +0100, Jon Hunter wrote: > >> > >> On 09/07/18 17:38, Aapo Vienamo wrote: > >>> From: Peter De Schrijver <pdeschrijver@nvidia.com> > >>> > >>> Move this to a separate file so it can be used to calculate the sdmmc > >>> clock dividers. > >> > >> Sorry for not commenting sooner, but what is the motivation for moving > >> this to its own file? I don't see why we need to do this in order to use > >> elsewhere. Furthermore, the original file is quite aptly named 'clk-divider.c' > >> and now we have a div71.c which seems quite specific. > > > > How else would you do it? > > Keep it in the same file? > That seems odd. clk-divider.c is meant to implement a clock type, not utility functions we happen to need in several types. Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/07/18 12:17, Peter De Schrijver wrote: > On Wed, Jul 11, 2018 at 09:42:20AM +0100, Jon Hunter wrote: >> >> On 11/07/18 09:00, Peter De Schrijver wrote: >>> On Tue, Jul 10, 2018 at 05:17:05PM +0100, Jon Hunter wrote: >>>> >>>> On 09/07/18 17:38, Aapo Vienamo wrote: >>>>> From: Peter De Schrijver <pdeschrijver@nvidia.com> >>>>> >>>>> Move this to a separate file so it can be used to calculate the sdmmc >>>>> clock dividers. >>>> >>>> Sorry for not commenting sooner, but what is the motivation for moving >>>> this to its own file? I don't see why we need to do this in order to use >>>> elsewhere. Furthermore, the original file is quite aptly named 'clk-divider.c' >>>> and now we have a div71.c which seems quite specific. >>> >>> How else would you do it? >> >> Keep it in the same file? >> > > That seems odd. clk-divider.c is meant to implement a clock type, not > utility functions we happen to need in several types. I see, then why not have a clk-utils.c for stuff like this. I am painting the bikeshed here, but div71.c seems very specific and I still don't understand the 7.1 bit. Cheers Jon
On Wed, 11 Jul 2018 16:14:13 +0100 Jon Hunter <jonathanh@nvidia.com> wrote: > On 11/07/18 12:17, Peter De Schrijver wrote: > > On Wed, Jul 11, 2018 at 09:42:20AM +0100, Jon Hunter wrote: > >> > >> On 11/07/18 09:00, Peter De Schrijver wrote: > >>> On Tue, Jul 10, 2018 at 05:17:05PM +0100, Jon Hunter wrote: > >>>> > >>>> On 09/07/18 17:38, Aapo Vienamo wrote: > >>>>> From: Peter De Schrijver <pdeschrijver@nvidia.com> > >>>>> > >>>>> Move this to a separate file so it can be used to calculate the sdmmc > >>>>> clock dividers. > >>>> > >>>> Sorry for not commenting sooner, but what is the motivation for moving > >>>> this to its own file? I don't see why we need to do this in order to use > >>>> elsewhere. Furthermore, the original file is quite aptly named 'clk-divider.c' > >>>> and now we have a div71.c which seems quite specific. > >>> > >>> How else would you do it? > >> > >> Keep it in the same file? > >> > > > > That seems odd. clk-divider.c is meant to implement a clock type, not > > utility functions we happen to need in several types. > > I see, then why not have a clk-utils.c for stuff like this. I am painting > the bikeshed here, but div71.c seems very specific and I still don't > understand the 7.1 bit. While the code could work with other dividers, it was called 7.1 because the current usecases were only on 7.1 dividers. I can submit another version with a different filename if other changes to the series come about or if it's seen necessary to change div-frac.c from the latest version of the series to something else. -Aapo -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile index b716923..6d4f563 100644 --- a/drivers/clk/tegra/Makefile +++ b/drivers/clk/tegra/Makefile @@ -24,3 +24,4 @@ obj-$(CONFIG_ARCH_TEGRA_132_SOC) += clk-tegra124.o obj-y += cvb.o obj-$(CONFIG_ARCH_TEGRA_210_SOC) += clk-tegra210.o obj-$(CONFIG_CLK_TEGRA_BPMP) += clk-bpmp.o +obj-y += div71.o diff --git a/drivers/clk/tegra/clk-divider.c b/drivers/clk/tegra/clk-divider.c index 16e0aee..ad87858 100644 --- a/drivers/clk/tegra/clk-divider.c +++ b/drivers/clk/tegra/clk-divider.c @@ -32,35 +32,15 @@ static int get_div(struct tegra_clk_frac_div *divider, unsigned long rate, unsigned long parent_rate) { - u64 divider_ux1 = parent_rate; - u8 flags = divider->flags; - int mul; - - if (!rate) - return 0; - - mul = get_mul(divider); - - if (!(flags & TEGRA_DIVIDER_INT)) - divider_ux1 *= mul; - - if (flags & TEGRA_DIVIDER_ROUND_UP) - divider_ux1 += rate - 1; - - do_div(divider_ux1, rate); - - if (flags & TEGRA_DIVIDER_INT) - divider_ux1 *= mul; + int div; - divider_ux1 -= mul; + div = div71_get(rate, parent_rate, divider->width, divider->frac_width, + divider->flags); - if ((s64)divider_ux1 < 0) + if (div < 0) return 0; - if (divider_ux1 > get_max_div(divider)) - return get_max_div(divider); - - return divider_ux1; + return div; } static unsigned long clk_frac_div_recalc_rate(struct clk_hw *hw, diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h index e3b9c22..149cc70 100644 --- a/drivers/clk/tegra/clk.h +++ b/drivers/clk/tegra/clk.h @@ -812,6 +812,9 @@ extern tegra_clk_apply_init_table_func tegra_clk_apply_init_table; int tegra_pll_wait_for_lock(struct tegra_clk_pll *pll); u16 tegra_pll_get_fixed_mdiv(struct clk_hw *hw, unsigned long input_rate); int tegra_pll_p_div_to_hw(struct tegra_clk_pll *pll, u8 p_div); +int div71_get(unsigned long rate, unsigned parent_rate, u8 width, + u8 frac_width, u8 flags); + /* Combined read fence with delay */ #define fence_udelay(delay, reg) \ diff --git a/drivers/clk/tegra/div71.c b/drivers/clk/tegra/div71.c new file mode 100644 index 0000000..1eecc84 --- /dev/null +++ b/drivers/clk/tegra/div71.c @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved. + */ + +#include <asm/div64.h> + +#include "clk.h" + +#define div_mask(w) ((1 << (w)) - 1) + +int div71_get(unsigned long rate, unsigned parent_rate, u8 width, + u8 frac_width, u8 flags) +{ + u64 divider_ux1 = parent_rate; + int mul; + + if (!rate) + return 0; + + mul = 1 << frac_width; + + if (!(flags & TEGRA_DIVIDER_INT)) + divider_ux1 *= mul; + + if (flags & TEGRA_DIVIDER_ROUND_UP) + divider_ux1 += rate - 1; + + do_div(divider_ux1, rate); + + if (flags & TEGRA_DIVIDER_INT) + divider_ux1 *= mul; + + if (divider_ux1 < mul) + return 0; + + divider_ux1 -= mul; + + if (divider_ux1 > div_mask(width)) + return div_mask(width); + + return divider_ux1; +}