| Message ID | 20190312110016.29174-2-m.tretter@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: zynqmp: fix CLK_FRAC and various cleanups | expand |
Quoting Michael Tretter (2019-03-12 04:00:12) > CLK_FRAC is not set in the divider->flags, but in the hw->flags. > > The firmware sets CLK_FRAC for fractional clocks in the clkflag field. > When registering the devider, these clkflags are copied to hw->flags. > > Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So > this check would never work. > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > --- > drivers/clk/zynqmp/divider.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c > index a371c66e72ef..fc70950c1e24 100644 > --- a/drivers/clk/zynqmp/divider.c > +++ b/drivers/clk/zynqmp/divider.c > @@ -117,7 +117,7 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw, > bestdiv = zynqmp_divider_get_val(*prate, rate); > > if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && > - (divider->flags & CLK_FRAC)) > + (clk_hw_get_flags(hw) & CLK_FRAC)) CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's not a clk framework flag so it shouldn't go there. Please fix the user of this flag to place the CLK_FRAC flag somewhere else. Even adding it into divider::flags is not a good idea because that numberspace is for dividers, and this flag seems to be zynqmp driver specific, so maybe just add a bool to the zynqmp_clk_divider?
On Tue, 12 Mar 2019 09:49:21 -0700, Stephen Boyd wrote: > Quoting Michael Tretter (2019-03-12 04:00:12) > > CLK_FRAC is not set in the divider->flags, but in the hw->flags. > > > > The firmware sets CLK_FRAC for fractional clocks in the clkflag field. > > When registering the devider, these clkflags are copied to hw->flags. > > > > Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So > > this check would never work. > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > > --- > > drivers/clk/zynqmp/divider.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c > > index a371c66e72ef..fc70950c1e24 100644 > > --- a/drivers/clk/zynqmp/divider.c > > +++ b/drivers/clk/zynqmp/divider.c > > @@ -117,7 +117,7 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw, > > bestdiv = zynqmp_divider_get_val(*prate, rate); > > > > if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && > > - (divider->flags & CLK_FRAC)) > > + (clk_hw_get_flags(hw) & CLK_FRAC)) > > CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's > not a clk framework flag so it shouldn't go there. Please fix the user > of this flag to place the CLK_FRAC flag somewhere else. Even adding it > into divider::flags is not a good idea because that numberspace is for > dividers, and this flag seems to be zynqmp driver specific, so maybe > just add a bool to the zynqmp_clk_divider? > Thanks. The driver sets the clk_hw::core::flags based on a response from the ATF and this response includes this flag with other clk frameworks flags. I can test for the flag when registering the clock and set another flag or a bool for the zynqmp_clk_divider and will do so in v2. However, this merely sounds like a workaround for an issue in the ATF, which should not define and use this flag in the first place. Michael
Quoting Michael Tretter (2019-03-12 10:25:46) > On Tue, 12 Mar 2019 09:49:21 -0700, Stephen Boyd wrote: > > Quoting Michael Tretter (2019-03-12 04:00:12) > > > CLK_FRAC is not set in the divider->flags, but in the hw->flags. > > > > > > The firmware sets CLK_FRAC for fractional clocks in the clkflag field. > > > When registering the devider, these clkflags are copied to hw->flags. > > > > > > Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So > > > this check would never work. > > > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > > > --- > > > drivers/clk/zynqmp/divider.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c > > > index a371c66e72ef..fc70950c1e24 100644 > > > --- a/drivers/clk/zynqmp/divider.c > > > +++ b/drivers/clk/zynqmp/divider.c > > > @@ -117,7 +117,7 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw, > > > bestdiv = zynqmp_divider_get_val(*prate, rate); > > > > > > if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && > > > - (divider->flags & CLK_FRAC)) > > > + (clk_hw_get_flags(hw) & CLK_FRAC)) > > > > CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's > > not a clk framework flag so it shouldn't go there. Please fix the user > > of this flag to place the CLK_FRAC flag somewhere else. Even adding it > > into divider::flags is not a good idea because that numberspace is for > > dividers, and this flag seems to be zynqmp driver specific, so maybe > > just add a bool to the zynqmp_clk_divider? > > > > Thanks. The driver sets the clk_hw::core::flags based on a response > from the ATF and this response includes this flag with other clk > frameworks flags. I can test for the flag when registering the clock > and set another flag or a bool for the zynqmp_clk_divider and will do > so in v2. Cool. Thanks! > > However, this merely sounds like a workaround for an issue in the ATF, > which should not define and use this flag in the first place. > What is ATF doing with these flags? Hopefully ATF and the Linux kernel aren't using the same numberspace to describe these things. For example, I would be concerned if ATF was looking at the CLK_SET_RATE_PARENT flag and passing that value from firmware to the kernel, blindly assuming that the kernel wouldn't change those numbers to be something else. Obviously that type of kernel change would be invasive but it's not an ABI that we've ever published so we're free to do these sorts of things.
On Wed, 13 Mar 2019 09:24:04 -0700, Stephen Boyd wrote: > Quoting Michael Tretter (2019-03-12 10:25:46) > > On Tue, 12 Mar 2019 09:49:21 -0700, Stephen Boyd wrote: > > > Quoting Michael Tretter (2019-03-12 04:00:12) > > > > CLK_FRAC is not set in the divider->flags, but in the hw->flags. > > > > > > > > The firmware sets CLK_FRAC for fractional clocks in the clkflag field. > > > > When registering the devider, these clkflags are copied to hw->flags. > > > > > > > > Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So > > > > this check would never work. > > > > > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > > > > --- > > > > drivers/clk/zynqmp/divider.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c > > > > index a371c66e72ef..fc70950c1e24 100644 > > > > --- a/drivers/clk/zynqmp/divider.c > > > > +++ b/drivers/clk/zynqmp/divider.c > > > > @@ -117,7 +117,7 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw, > > > > bestdiv = zynqmp_divider_get_val(*prate, rate); > > > > > > > > if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && > > > > - (divider->flags & CLK_FRAC)) > > > > + (clk_hw_get_flags(hw) & CLK_FRAC)) > > > > > > CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's > > > not a clk framework flag so it shouldn't go there. Please fix the user > > > of this flag to place the CLK_FRAC flag somewhere else. Even adding it > > > into divider::flags is not a good idea because that numberspace is for > > > dividers, and this flag seems to be zynqmp driver specific, so maybe > > > just add a bool to the zynqmp_clk_divider? > > > > > > > Thanks. The driver sets the clk_hw::core::flags based on a response > > from the ATF and this response includes this flag with other clk > > frameworks flags. I can test for the flag when registering the clock > > and set another flag or a bool for the zynqmp_clk_divider and will do > > so in v2. > > Cool. Thanks! > > > > > However, this merely sounds like a workaround for an issue in the ATF, > > which should not define and use this flag in the first place. > > > > What is ATF doing with these flags? Hopefully ATF and the Linux kernel > aren't using the same numberspace to describe these things. For example, > I would be concerned if ATF was looking at the CLK_SET_RATE_PARENT flag > and passing that value from firmware to the kernel, blindly assuming > that the kernel wouldn't change those numbers to be something else. > Obviously that type of kernel change would be invasive but it's not an > ABI that we've ever published so we're free to do these sorts of things. You mean that the ATF defines macros like #define CLK_SET_RATE_GATE BIT(0) /* must be gated across rate change */ #define CLK_SET_PARENT_GATE BIT(1) /* must be gated across re-parent */ #define CLK_SET_RATE_PARENT BIT(2) /* propagate rate change up one level */ in plat/xilinx/zynqmp/pm_service/pm_api_clock.h, sets the flags in the response to the Linux driver, and the Linux driver copies the flags that it got from the ATF to clk_hw::core::flags like init.flags = nodes->flag; where nodes is the response from the ATF? That's exactly what is happening. So instead of only translating CLK_FRAC, the driver should actually translate all flags in the ATF response to proper clk framework flags instead of blindly copying them, right? Michael
Quoting Michael Tretter (2019-03-14 01:38:36) > On Wed, 13 Mar 2019 09:24:04 -0700, Stephen Boyd wrote: > > Quoting Michael Tretter (2019-03-12 10:25:46) > > > > > > However, this merely sounds like a workaround for an issue in the ATF, > > > which should not define and use this flag in the first place. > > > > > > > What is ATF doing with these flags? Hopefully ATF and the Linux kernel > > aren't using the same numberspace to describe these things. For example, > > I would be concerned if ATF was looking at the CLK_SET_RATE_PARENT flag > > and passing that value from firmware to the kernel, blindly assuming > > that the kernel wouldn't change those numbers to be something else. > > Obviously that type of kernel change would be invasive but it's not an > > ABI that we've ever published so we're free to do these sorts of things. > > You mean that the ATF defines macros like > > #define CLK_SET_RATE_GATE BIT(0) /* must be gated across rate change */ > #define CLK_SET_PARENT_GATE BIT(1) /* must be gated across re-parent */ > #define CLK_SET_RATE_PARENT BIT(2) /* propagate rate change up one level */ > > in plat/xilinx/zynqmp/pm_service/pm_api_clock.h, sets the flags in the > response to the Linux driver, and the Linux driver copies the flags > that it got from the ATF to clk_hw::core::flags like > > init.flags = nodes->flag; > > where nodes is the response from the ATF? That's exactly what is happening. Yes, that looks like a strong coupling between firmware and OS implemented in a non-obvious way. > > So instead of only translating CLK_FRAC, the driver should actually > translate all flags in the ATF response to proper clk framework flags > instead of blindly copying them, right? > Sure. The CLK_FRAC will have to be filtered out anyway it sounds like.
Hi Michael, > -----Original Message----- > From: Michael Tretter <m.tretter@pengutronix.de> > Sent: Thursday, March 14, 2019 1:39 AM > To: Stephen Boyd <sboyd@kernel.org> > Cc: linux-arm-kernel@lists.infradead.org; linux-clk@vger.kernel.org; > kernel@pengutronix.de; Michael Turquette <mturquette@baylibre.com>; > Michal Simek <michals@xilinx.com>; Jolly Shah <JOLLYS@xilinx.com> > Subject: Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock > > On Wed, 13 Mar 2019 09:24:04 -0700, Stephen Boyd wrote: > > Quoting Michael Tretter (2019-03-12 10:25:46) > > > On Tue, 12 Mar 2019 09:49:21 -0700, Stephen Boyd wrote: > > > > Quoting Michael Tretter (2019-03-12 04:00:12) > > > > > CLK_FRAC is not set in the divider->flags, but in the hw->flags. > > > > > > > > > > The firmware sets CLK_FRAC for fractional clocks in the clkflag field. > > > > > When registering the devider, these clkflags are copied to hw->flags. > > > > > > > > > > Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So > > > > > this check would never work. > > > > > > > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > > > > > --- > > > > > drivers/clk/zynqmp/divider.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c > > > > > index a371c66e72ef..fc70950c1e24 100644 > > > > > --- a/drivers/clk/zynqmp/divider.c > > > > > +++ b/drivers/clk/zynqmp/divider.c > > > > > @@ -117,7 +117,7 @@ static long > zynqmp_clk_divider_round_rate(struct clk_hw *hw, > > > > > bestdiv = zynqmp_divider_get_val(*prate, rate); > > > > > > > > > > if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && > > > > > - (divider->flags & CLK_FRAC)) > > > > > + (clk_hw_get_flags(hw) & CLK_FRAC)) > > > > > > > > CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's > > > > not a clk framework flag so it shouldn't go there. Please fix the user > > > > of this flag to place the CLK_FRAC flag somewhere else. Even adding it > > > > into divider::flags is not a good idea because that numberspace is for > > > > dividers, and this flag seems to be zynqmp driver specific, so maybe > > > > just add a bool to the zynqmp_clk_divider? > > > > > > > > > > Thanks. The driver sets the clk_hw::core::flags based on a response > > > from the ATF and this response includes this flag with other clk > > > frameworks flags. I can test for the flag when registering the clock > > > and set another flag or a bool for the zynqmp_clk_divider and will do > > > so in v2. > > > > Cool. Thanks! > > > > > > > > However, this merely sounds like a workaround for an issue in the ATF, > > > which should not define and use this flag in the first place. > > > > > > > What is ATF doing with these flags? Hopefully ATF and the Linux kernel > > aren't using the same numberspace to describe these things. For example, > > I would be concerned if ATF was looking at the CLK_SET_RATE_PARENT flag > > and passing that value from firmware to the kernel, blindly assuming > > that the kernel wouldn't change those numbers to be something else. > > Obviously that type of kernel change would be invasive but it's not an > > ABI that we've ever published so we're free to do these sorts of things. > > You mean that the ATF defines macros like > > #define CLK_SET_RATE_GATE BIT(0) /* must be gated across rate > change */ > #define CLK_SET_PARENT_GATE BIT(1) /* must be gated across re- > parent */ > #define CLK_SET_RATE_PARENT BIT(2) /* propagate rate change up > one level */ > > in plat/xilinx/zynqmp/pm_service/pm_api_clock.h, sets the flags in the > response to the Linux driver, and the Linux driver copies the flags > that it got from the ATF to clk_hw::core::flags like > > init.flags = nodes->flag; > > where nodes is the response from the ATF? That's exactly what is happening. > > So instead of only translating CLK_FRAC, the driver should actually > translate all flags in the ATF response to proper clk framework flags > instead of blindly copying them, right? Except CLK_FRAC, all flags are defined same as clk framework. I think we should add custom flags field and pass zynqmp specific flags there. Thanks, Jolly Shah > > Michael
On Tue, 19 Mar 2019 00:56:31 +0000, Jolly Shah wrote: > > -----Original Message----- > > From: Michael Tretter <m.tretter@pengutronix.de> > > Sent: Thursday, March 14, 2019 1:39 AM > > To: Stephen Boyd <sboyd@kernel.org> > > Cc: linux-arm-kernel@lists.infradead.org; linux-clk@vger.kernel.org; > > kernel@pengutronix.de; Michael Turquette <mturquette@baylibre.com>; > > Michal Simek <michals@xilinx.com>; Jolly Shah <JOLLYS@xilinx.com> > > Subject: Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock > > > > On Wed, 13 Mar 2019 09:24:04 -0700, Stephen Boyd wrote: > > > Quoting Michael Tretter (2019-03-12 10:25:46) > > > > On Tue, 12 Mar 2019 09:49:21 -0700, Stephen Boyd wrote: > > > > > Quoting Michael Tretter (2019-03-12 04:00:12) > > > > > > CLK_FRAC is not set in the divider->flags, but in the hw->flags. > > > > > > > > > > > > The firmware sets CLK_FRAC for fractional clocks in the clkflag field. > > > > > > When registering the devider, these clkflags are copied to hw->flags. > > > > > > > > > > > > Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So > > > > > > this check would never work. > > > > > > > > > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > > > > > > --- > > > > > > drivers/clk/zynqmp/divider.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c > > > > > > index a371c66e72ef..fc70950c1e24 100644 > > > > > > --- a/drivers/clk/zynqmp/divider.c > > > > > > +++ b/drivers/clk/zynqmp/divider.c > > > > > > @@ -117,7 +117,7 @@ static long > > zynqmp_clk_divider_round_rate(struct clk_hw *hw, > > > > > > bestdiv = zynqmp_divider_get_val(*prate, rate); > > > > > > > > > > > > if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && > > > > > > - (divider->flags & CLK_FRAC)) > > > > > > + (clk_hw_get_flags(hw) & CLK_FRAC)) > > > > > > > > > > CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's > > > > > not a clk framework flag so it shouldn't go there. Please fix the user > > > > > of this flag to place the CLK_FRAC flag somewhere else. Even adding it > > > > > into divider::flags is not a good idea because that numberspace is for > > > > > dividers, and this flag seems to be zynqmp driver specific, so maybe > > > > > just add a bool to the zynqmp_clk_divider? > > > > > > > > > > > > > Thanks. The driver sets the clk_hw::core::flags based on a response > > > > from the ATF and this response includes this flag with other clk > > > > frameworks flags. I can test for the flag when registering the clock > > > > and set another flag or a bool for the zynqmp_clk_divider and will do > > > > so in v2. > > > > > > Cool. Thanks! > > > > > > > > > > > However, this merely sounds like a workaround for an issue in the ATF, > > > > which should not define and use this flag in the first place. > > > > > > > > > > What is ATF doing with these flags? Hopefully ATF and the Linux kernel > > > aren't using the same numberspace to describe these things. For example, > > > I would be concerned if ATF was looking at the CLK_SET_RATE_PARENT flag > > > and passing that value from firmware to the kernel, blindly assuming > > > that the kernel wouldn't change those numbers to be something else. > > > Obviously that type of kernel change would be invasive but it's not an > > > ABI that we've ever published so we're free to do these sorts of things. > > > > You mean that the ATF defines macros like > > > > #define CLK_SET_RATE_GATE BIT(0) /* must be gated across rate > > change */ > > #define CLK_SET_PARENT_GATE BIT(1) /* must be gated across re- > > parent */ > > #define CLK_SET_RATE_PARENT BIT(2) /* propagate rate change up > > one level */ > > > > in plat/xilinx/zynqmp/pm_service/pm_api_clock.h, sets the flags in the > > response to the Linux driver, and the Linux driver copies the flags > > that it got from the ATF to clk_hw::core::flags like > > > > init.flags = nodes->flag; > > > > where nodes is the response from the ATF? That's exactly what is happening. > > > > So instead of only translating CLK_FRAC, the driver should actually > > translate all flags in the ATF response to proper clk framework flags > > instead of blindly copying them, right? > > Except CLK_FRAC, all flags are defined same as clk framework. I think > we should add custom flags field and pass zynqmp specific flags there. Using the same flags in the ATF and the common clk framework works now, because they have the same value. However, the flags are not actually the same, because the flags that the driver gets from the ATF are defined in the ATF and the flags that the driver passes to the common clk framework are defined in the common clk framework. If the flags in the common clk framework are changed (which is admittedly unlikely), the ZynqMP clock driver will break, because it still assumes that the common clk framework uses the same flags as defined in the ATF. Therefore, the driver should actually decouple the flags by defining its own flags that correspond to the flags as defined by the ATF (i.e. platform specific flags) and convert these flags into flags for the common clk framework when registering the clocks. The problem is that two things that have the same value are not necessarily the same thing. Michael > > Thanks, > Jolly Shah > > > > > Michael >
diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c index a371c66e72ef..fc70950c1e24 100644 --- a/drivers/clk/zynqmp/divider.c +++ b/drivers/clk/zynqmp/divider.c @@ -117,7 +117,7 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw, bestdiv = zynqmp_divider_get_val(*prate, rate); if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && - (divider->flags & CLK_FRAC)) + (clk_hw_get_flags(hw) & CLK_FRAC)) bestdiv = rate % *prate ? 1 : bestdiv; *prate = rate * bestdiv;
CLK_FRAC is not set in the divider->flags, but in the hw->flags. The firmware sets CLK_FRAC for fractional clocks in the clkflag field. When registering the devider, these clkflags are copied to hw->flags. Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So this check would never work. Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> --- drivers/clk/zynqmp/divider.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)