| Message ID | 1370358317-12768-4-git-send-email-haojian.zhuang@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Jun 4, 2013 at 5:05 PM, Haojian Zhuang <haojian.zhuang@linaro.org> wrote: > In Hisilicon Hi3620 clock divider register, 16-bit HIWORD is mask field. > Support the HIWORD mask to reuse clock divider driver. > > Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> I don't understand this... > + * CLK_DIVIDER_HIWORD_MASK - register contains high 16-bit as mask field Could you be a bit more verbose here on what this actually means, so other users of this flag will realize if they need to set it? In other cases in the clk subsystem where masks and shifts are used, the position and number of bits are stated, rather than a single flag indicating some 16 bits. Yours, Linus Walleij
On 7 June 2013 17:15, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Jun 4, 2013 at 5:05 PM, Haojian Zhuang > <haojian.zhuang@linaro.org> wrote: > >> In Hisilicon Hi3620 clock divider register, 16-bit HIWORD is mask field. >> Support the HIWORD mask to reuse clock divider driver. >> >> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> > > I don't understand this... The mux or divider register in Hi3620 is 32-bit. The lower 16-bit is used to configure mux or divider, and the higher 16-bit is used to set mask of the mux or divier. If I need to set b01 in mux/divider register, I also need to set (b11 << 16) for HIWORD mask in the same register. The reason to set (b11 << 16) is two bits are changed in the mux/divider register. > >> + * CLK_DIVIDER_HIWORD_MASK - register contains high 16-bit as mask field > > Could you be a bit more verbose here on what this actually means, > so other users of this flag will realize if they need to set it? > I didn't find a better name for this flag. :( > In other cases in the clk subsystem where masks and shifts are > used, the position and number of bits are stated, rather than a > single flag indicating some 16 bits. > Those masks are different from mine because those're used to calculate which bits should be set or clear. Regards Haojian
Hi Haojian, as Linus suggested, a bit of commentary for your side :-) The same also applies to the similar changes to the mux clock. Interestingly the gates on the hisilicon follow another paradigm altogether, where on the Rockchip they also use this mask-mechanism. Am Dienstag, 4. Juni 2013, 17:05:14 schrieb Haojian Zhuang: > In Hisilicon Hi3620 clock divider register, 16-bit HIWORD is mask field. > Support the HIWORD mask to reuse clock divider driver. > > Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> > --- > drivers/clk/clk-divider.c | 6 ++++++ > include/linux/clk-provider.h | 2 ++ > 2 files changed, 8 insertions(+) > > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > index 6d96741..4c344b4 100644 > --- a/drivers/clk/clk-divider.c > +++ b/drivers/clk/clk-divider.c > @@ -220,6 +220,12 @@ static int clk_divider_set_rate(struct clk_hw *hw, > unsigned long rate, val = readl(divider->reg); > val &= ~(div_mask(divider) << divider->shift); Do you really need to read the register value on the HiSilicon before changing it? On the Rockchip side, the mask is used just for just this, to indicate the bits that are set in the lower part, so there there really is no separate read necessary when changing bits. > val |= value << divider->shift; > + if (divider->flags & CLK_DIVIDER_HIWORD_MASK) { > + if (divider->width + divider->shift > 16) > + pr_warn("divider value exceeds LOWORD field\n"); This can be checked in the clock setup. If shift and mask are exceeding the low word there is something seriously wrong with the setup data and the clock shouldn't be created at all. This also saves the conditional on every set operation. > + else > + val |= div_mask(divider) << (divider->shift + 16); > + } > writel(val, divider->reg); > > if (divider->lock) > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 6ba32bc..dbb9bd9 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -257,6 +257,7 @@ struct clk_div_table { > * Some hardware implementations gracefully handle this case and allow a > * zero divisor by not modifying their input clock > * (divide by one / bypass). > + * CLK_DIVIDER_HIWORD_MASK - register contains high 16-bit as mask field > */ > struct clk_divider { > struct clk_hw hw; > @@ -271,6 +272,7 @@ struct clk_divider { > #define CLK_DIVIDER_ONE_BASED BIT(0) > #define CLK_DIVIDER_POWER_OF_TWO BIT(1) > #define CLK_DIVIDER_ALLOW_ZERO BIT(2) > +#define CLK_DIVIDER_HIWORD_MASK BIT(3) I like your naming a lot better than mine :-) Heiko
On 7 June 2013 20:31, Heiko Stübner <heiko@sntech.de> wrote: > Hi Haojian, > > as Linus suggested, a bit of commentary for your side :-) > > The same also applies to the similar changes to the mux clock. > > Interestingly the gates on the hisilicon follow another paradigm altogether, > where on the Rockchip they also use this mask-mechanism. > > > Am Dienstag, 4. Juni 2013, 17:05:14 schrieb Haojian Zhuang: >> In Hisilicon Hi3620 clock divider register, 16-bit HIWORD is mask field. >> Support the HIWORD mask to reuse clock divider driver. >> >> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> >> --- >> drivers/clk/clk-divider.c | 6 ++++++ >> include/linux/clk-provider.h | 2 ++ >> 2 files changed, 8 insertions(+) >> >> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c >> index 6d96741..4c344b4 100644 >> --- a/drivers/clk/clk-divider.c >> +++ b/drivers/clk/clk-divider.c >> @@ -220,6 +220,12 @@ static int clk_divider_set_rate(struct clk_hw *hw, >> unsigned long rate, val = readl(divider->reg); >> val &= ~(div_mask(divider) << divider->shift); > > Do you really need to read the register value on the HiSilicon before changing > it? Because there's a mask field in the register, it could work without reading. My purpose is only to do less change in common code. > > On the Rockchip side, the mask is used just for just this, to indicate the > bits that are set in the lower part, so there there really is no separate read > necessary when changing bits. > > >> val |= value << divider->shift; >> + if (divider->flags & CLK_DIVIDER_HIWORD_MASK) { >> + if (divider->width + divider->shift > 16) >> + pr_warn("divider value exceeds LOWORD field\n"); > > This can be checked in the clock setup. If shift and mask are exceeding the > low word there is something seriously wrong with the setup data and the clock > shouldn't be created at all. This also saves the conditional on every set > operation. OK. > > >> + else >> + val |= div_mask(divider) << (divider->shift + 16); >> + } >> writel(val, divider->reg); >> >> if (divider->lock) >> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h >> index 6ba32bc..dbb9bd9 100644 >> --- a/include/linux/clk-provider.h >> +++ b/include/linux/clk-provider.h >> @@ -257,6 +257,7 @@ struct clk_div_table { >> * Some hardware implementations gracefully handle this case and allow a >> * zero divisor by not modifying their input clock >> * (divide by one / bypass). >> + * CLK_DIVIDER_HIWORD_MASK - register contains high 16-bit as mask field >> */ >> struct clk_divider { >> struct clk_hw hw; >> @@ -271,6 +272,7 @@ struct clk_divider { >> #define CLK_DIVIDER_ONE_BASED BIT(0) >> #define CLK_DIVIDER_POWER_OF_TWO BIT(1) >> #define CLK_DIVIDER_ALLOW_ZERO BIT(2) >> +#define CLK_DIVIDER_HIWORD_MASK BIT(3) > > I like your naming a lot better than mine :-) OK. How about I sent the next version. And you can base on mine version. I'll append your comments & logic in my version. And I'll split your patches into three that are gate, mux and divider. Regards Haojian > > > Heiko
Am Samstag, 8. Juni 2013, 05:17:16 schrieb Haojian Zhuang: > On 7 June 2013 20:31, Heiko Stübner <heiko@sntech.de> wrote: > > Hi Haojian, > > > > as Linus suggested, a bit of commentary for your side :-) > > > > The same also applies to the similar changes to the mux clock. > > > > Interestingly the gates on the hisilicon follow another paradigm > > altogether, where on the Rockchip they also use this mask-mechanism. > > > > Am Dienstag, 4. Juni 2013, 17:05:14 schrieb Haojian Zhuang: > >> In Hisilicon Hi3620 clock divider register, 16-bit HIWORD is mask field. > >> Support the HIWORD mask to reuse clock divider driver. > >> > >> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> > >> --- > >> > >> drivers/clk/clk-divider.c | 6 ++++++ > >> include/linux/clk-provider.h | 2 ++ > >> 2 files changed, 8 insertions(+) > >> > >> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > >> index 6d96741..4c344b4 100644 > >> --- a/drivers/clk/clk-divider.c > >> +++ b/drivers/clk/clk-divider.c > >> @@ -220,6 +220,12 @@ static int clk_divider_set_rate(struct clk_hw *hw, > >> unsigned long rate, val = readl(divider->reg); > >> > >> val &= ~(div_mask(divider) << divider->shift); > > > > Do you really need to read the register value on the HiSilicon before > > changing it? > > Because there's a mask field in the register, it could work without > reading. My purpose is only to do less change in common code. > > > On the Rockchip side, the mask is used just for just this, to indicate > > the bits that are set in the lower part, so there there really is no > > separate read necessary when changing bits. > > > >> val |= value << divider->shift; > >> > >> + if (divider->flags & CLK_DIVIDER_HIWORD_MASK) { > >> + if (divider->width + divider->shift > 16) > >> + pr_warn("divider value exceeds LOWORD field\n"); > > > > This can be checked in the clock setup. If shift and mask are exceeding > > the low word there is something seriously wrong with the setup data and > > the clock shouldn't be created at all. This also saves the conditional > > on every set operation. > > OK. > > >> + else > >> + val |= div_mask(divider) << (divider->shift + 16); > >> + } > >> > >> writel(val, divider->reg); > >> > >> if (divider->lock) > >> > >> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > >> index 6ba32bc..dbb9bd9 100644 > >> --- a/include/linux/clk-provider.h > >> +++ b/include/linux/clk-provider.h > >> @@ -257,6 +257,7 @@ struct clk_div_table { > >> > >> * Some hardware implementations gracefully handle this case and > >> allow a * zero divisor by not modifying their input clock > >> * (divide by one / bypass). > >> > >> + * CLK_DIVIDER_HIWORD_MASK - register contains high 16-bit as mask > >> field > >> > >> */ > >> > >> struct clk_divider { > >> > >> struct clk_hw hw; > >> > >> @@ -271,6 +272,7 @@ struct clk_divider { > >> > >> #define CLK_DIVIDER_ONE_BASED BIT(0) > >> #define CLK_DIVIDER_POWER_OF_TWO BIT(1) > >> #define CLK_DIVIDER_ALLOW_ZERO BIT(2) > >> > >> +#define CLK_DIVIDER_HIWORD_MASK BIT(3) > > > > I like your naming a lot better than mine :-) > > OK. How about I sent the next version. And you can base on mine version. > I'll append your comments & logic in my version. And I'll split your > patches into three that are gate, mux and divider. sure, go ahead, but would say without the unecessary read we talked about above. Heiko
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c index 6d96741..4c344b4 100644 --- a/drivers/clk/clk-divider.c +++ b/drivers/clk/clk-divider.c @@ -220,6 +220,12 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate, val = readl(divider->reg); val &= ~(div_mask(divider) << divider->shift); val |= value << divider->shift; + if (divider->flags & CLK_DIVIDER_HIWORD_MASK) { + if (divider->width + divider->shift > 16) + pr_warn("divider value exceeds LOWORD field\n"); + else + val |= div_mask(divider) << (divider->shift + 16); + } writel(val, divider->reg); if (divider->lock) diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 6ba32bc..dbb9bd9 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -257,6 +257,7 @@ struct clk_div_table { * Some hardware implementations gracefully handle this case and allow a * zero divisor by not modifying their input clock * (divide by one / bypass). + * CLK_DIVIDER_HIWORD_MASK - register contains high 16-bit as mask field */ struct clk_divider { struct clk_hw hw; @@ -271,6 +272,7 @@ struct clk_divider { #define CLK_DIVIDER_ONE_BASED BIT(0) #define CLK_DIVIDER_POWER_OF_TWO BIT(1) #define CLK_DIVIDER_ALLOW_ZERO BIT(2) +#define CLK_DIVIDER_HIWORD_MASK BIT(3) extern const struct clk_ops clk_divider_ops; struct clk *clk_register_divider(struct device *dev, const char *name,
In Hisilicon Hi3620 clock divider register, 16-bit HIWORD is mask field. Support the HIWORD mask to reuse clock divider driver. Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> --- drivers/clk/clk-divider.c | 6 ++++++ include/linux/clk-provider.h | 2 ++ 2 files changed, 8 insertions(+)