Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
Quoting Shawn Guo (2018-11-13 06:25:36) > On Sat, Nov 10, 2018 at 04:05:44PM +, A.s. Dong wrote: > > Hi Stephen, > > > > [...] > > > > I already sent the 12th version of this current patch series and I > > > > would really like to get this in ASAP so that the booting up of imx8mq > > > > will > > > not be delayed. > > > > > > > > > > Ok. Well we're in rc1 right now, and so we're not merging new drivers into > > > mainline. I can merge the clk driver into clk-next, but you'll have to > > > wait for the > > > stabilization period to end (approximately 6 or 7 weeks) before this can > > > get > > > into the next kernel version. It will be in linux-next much sooner of > > > course. > > > > That would be great if you can help that. > > We're now working with SUSE to enable i.MX8 support. > > Their criteria is only backporting patches which must be at least in > > maintainer's > > next tree already. So either picked up by you or Shawn would help a lot on > > it. > > > > BTW, one simple question is that because MX8MQ DTS patches depends on > > this clock driver series. How would you suggest this series to go through > > your tree or Shawn's tree? > > Once Stephen has a topic branch for the patches, I can pull it into my > tree to resolve the DT dependency. > Sounds like a plan. Expect something next week.
Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
Quoting Shawn Guo (2018-11-13 06:25:36) > On Sat, Nov 10, 2018 at 04:05:44PM +, A.s. Dong wrote: > > Hi Stephen, > > > > [...] > > > > I already sent the 12th version of this current patch series and I > > > > would really like to get this in ASAP so that the booting up of imx8mq > > > > will > > > not be delayed. > > > > > > > > > > Ok. Well we're in rc1 right now, and so we're not merging new drivers into > > > mainline. I can merge the clk driver into clk-next, but you'll have to > > > wait for the > > > stabilization period to end (approximately 6 or 7 weeks) before this can > > > get > > > into the next kernel version. It will be in linux-next much sooner of > > > course. > > > > That would be great if you can help that. > > We're now working with SUSE to enable i.MX8 support. > > Their criteria is only backporting patches which must be at least in > > maintainer's > > next tree already. So either picked up by you or Shawn would help a lot on > > it. > > > > BTW, one simple question is that because MX8MQ DTS patches depends on > > this clock driver series. How would you suggest this series to go through > > your tree or Shawn's tree? > > Once Stephen has a topic branch for the patches, I can pull it into my > tree to resolve the DT dependency. > Sounds like a plan. Expect something next week.
Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
On Sat, Nov 10, 2018 at 04:05:44PM +, A.s. Dong wrote: > Hi Stephen, > > [...] > > > I already sent the 12th version of this current patch series and I > > > would really like to get this in ASAP so that the booting up of imx8mq > > > will > > not be delayed. > > > > > > > Ok. Well we're in rc1 right now, and so we're not merging new drivers into > > mainline. I can merge the clk driver into clk-next, but you'll have to wait > > for the > > stabilization period to end (approximately 6 or 7 weeks) before this can get > > into the next kernel version. It will be in linux-next much sooner of > > course. > > That would be great if you can help that. > We're now working with SUSE to enable i.MX8 support. > Their criteria is only backporting patches which must be at least in > maintainer's > next tree already. So either picked up by you or Shawn would help a lot on it. > > BTW, one simple question is that because MX8MQ DTS patches depends on > this clock driver series. How would you suggest this series to go through > your tree or Shawn's tree? Once Stephen has a topic branch for the patches, I can pull it into my tree to resolve the DT dependency. Shawn
Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
On Sat, Nov 10, 2018 at 04:05:44PM +, A.s. Dong wrote: > Hi Stephen, > > [...] > > > I already sent the 12th version of this current patch series and I > > > would really like to get this in ASAP so that the booting up of imx8mq > > > will > > not be delayed. > > > > > > > Ok. Well we're in rc1 right now, and so we're not merging new drivers into > > mainline. I can merge the clk driver into clk-next, but you'll have to wait > > for the > > stabilization period to end (approximately 6 or 7 weeks) before this can get > > into the next kernel version. It will be in linux-next much sooner of > > course. > > That would be great if you can help that. > We're now working with SUSE to enable i.MX8 support. > Their criteria is only backporting patches which must be at least in > maintainer's > next tree already. So either picked up by you or Shawn would help a lot on it. > > BTW, one simple question is that because MX8MQ DTS patches depends on > this clock driver series. How would you suggest this series to go through > your tree or Shawn's tree? Once Stephen has a topic branch for the patches, I can pull it into my tree to resolve the DT dependency. Shawn
RE: [PATCH v9 3/5] clk: imx: add SCCG PLL type
Hi Stephen, [...] > > I already sent the 12th version of this current patch series and I > > would really like to get this in ASAP so that the booting up of imx8mq will > not be delayed. > > > > Ok. Well we're in rc1 right now, and so we're not merging new drivers into > mainline. I can merge the clk driver into clk-next, but you'll have to wait > for the > stabilization period to end (approximately 6 or 7 weeks) before this can get > into the next kernel version. It will be in linux-next much sooner of course. That would be great if you can help that. We're now working with SUSE to enable i.MX8 support. Their criteria is only backporting patches which must be at least in maintainer's next tree already. So either picked up by you or Shawn would help a lot on it. BTW, one simple question is that because MX8MQ DTS patches depends on this clock driver series. How would you suggest this series to go through your tree or Shawn's tree? Regards Dong Aisheng
RE: [PATCH v9 3/5] clk: imx: add SCCG PLL type
Hi Stephen, [...] > > I already sent the 12th version of this current patch series and I > > would really like to get this in ASAP so that the booting up of imx8mq will > not be delayed. > > > > Ok. Well we're in rc1 right now, and so we're not merging new drivers into > mainline. I can merge the clk driver into clk-next, but you'll have to wait > for the > stabilization period to end (approximately 6 or 7 weeks) before this can get > into the next kernel version. It will be in linux-next much sooner of course. That would be great if you can help that. We're now working with SUSE to enable i.MX8 support. Their criteria is only backporting patches which must be at least in maintainer's next tree already. So either picked up by you or Shawn would help a lot on it. BTW, one simple question is that because MX8MQ DTS patches depends on this clock driver series. How would you suggest this series to go through your tree or Shawn's tree? Regards Dong Aisheng
Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
Quoting Abel Vesa (2018-11-08 04:29:39) > On Wed, Nov 07, 2018 at 04:18:35PM -0800, Stephen Boyd wrote: > > Quoting Abel Vesa (2018-11-07 12:26:25) > > > On Wed, Nov 07, 2018 at 11:01:02AM -0800, Stephen Boyd wrote: > > > > > > > > > > > > What's the plan to clean it up? > > > > > > So I'm doing this in our internal tree first to make sure I don't break > > > the > > > other (newer) socs. > > > > > > I already have a prototype in testing but it's a long way to upstream it. > > > > > > Basically, I'm replacing all of this with a single, more like a composite, > > > more complex, clock type that does all the magic inside. > > > > > > One of the problems is the fact that the bypasses can have the same > > > sources > > > and in my case, I'm implementing that as a list of parents name, but the > > > parent names list doesn't work with duplicates, so I have to find some > > > other > > > way to do it. > > > > > > Once I have something clean and tested enough I'll send it upstream. > > > > Ok. Thanks for the info. > > > > Just to avoid any kind of confusion. > > The whole refactoring of the SCCG clock will be done in a separate (later) > change > and will not be part of this patchset. > > I already sent the 12th version of this current patch series and I would > really > like to get this in ASAP so that the booting up of imx8mq will not be delayed. > Ok. Well we're in rc1 right now, and so we're not merging new drivers into mainline. I can merge the clk driver into clk-next, but you'll have to wait for the stabilization period to end (approximately 6 or 7 weeks) before this can get into the next kernel version. It will be in linux-next much sooner of course.
Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
Quoting Abel Vesa (2018-11-08 04:29:39) > On Wed, Nov 07, 2018 at 04:18:35PM -0800, Stephen Boyd wrote: > > Quoting Abel Vesa (2018-11-07 12:26:25) > > > On Wed, Nov 07, 2018 at 11:01:02AM -0800, Stephen Boyd wrote: > > > > > > > > > > > > What's the plan to clean it up? > > > > > > So I'm doing this in our internal tree first to make sure I don't break > > > the > > > other (newer) socs. > > > > > > I already have a prototype in testing but it's a long way to upstream it. > > > > > > Basically, I'm replacing all of this with a single, more like a composite, > > > more complex, clock type that does all the magic inside. > > > > > > One of the problems is the fact that the bypasses can have the same > > > sources > > > and in my case, I'm implementing that as a list of parents name, but the > > > parent names list doesn't work with duplicates, so I have to find some > > > other > > > way to do it. > > > > > > Once I have something clean and tested enough I'll send it upstream. > > > > Ok. Thanks for the info. > > > > Just to avoid any kind of confusion. > > The whole refactoring of the SCCG clock will be done in a separate (later) > change > and will not be part of this patchset. > > I already sent the 12th version of this current patch series and I would > really > like to get this in ASAP so that the booting up of imx8mq will not be delayed. > Ok. Well we're in rc1 right now, and so we're not merging new drivers into mainline. I can merge the clk driver into clk-next, but you'll have to wait for the stabilization period to end (approximately 6 or 7 weeks) before this can get into the next kernel version. It will be in linux-next much sooner of course.
Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
On Wed, Nov 07, 2018 at 04:18:35PM -0800, Stephen Boyd wrote: > Quoting Abel Vesa (2018-11-07 12:26:25) > > On Wed, Nov 07, 2018 at 11:01:02AM -0800, Stephen Boyd wrote: > > > > > > > > > What's the plan to clean it up? > > > > So I'm doing this in our internal tree first to make sure I don't break the > > other (newer) socs. > > > > I already have a prototype in testing but it's a long way to upstream it. > > > > Basically, I'm replacing all of this with a single, more like a composite, > > more complex, clock type that does all the magic inside. > > > > One of the problems is the fact that the bypasses can have the same sources > > and in my case, I'm implementing that as a list of parents name, but the > > parent names list doesn't work with duplicates, so I have to find some other > > way to do it. > > > > Once I have something clean and tested enough I'll send it upstream. > > Ok. Thanks for the info. > Just to avoid any kind of confusion. The whole refactoring of the SCCG clock will be done in a separate (later) change and will not be part of this patchset. I already sent the 12th version of this current patch series and I would really like to get this in ASAP so that the booting up of imx8mq will not be delayed. Thanks Abel --
Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
On Wed, Nov 07, 2018 at 04:18:35PM -0800, Stephen Boyd wrote: > Quoting Abel Vesa (2018-11-07 12:26:25) > > On Wed, Nov 07, 2018 at 11:01:02AM -0800, Stephen Boyd wrote: > > > > > > > > > What's the plan to clean it up? > > > > So I'm doing this in our internal tree first to make sure I don't break the > > other (newer) socs. > > > > I already have a prototype in testing but it's a long way to upstream it. > > > > Basically, I'm replacing all of this with a single, more like a composite, > > more complex, clock type that does all the magic inside. > > > > One of the problems is the fact that the bypasses can have the same sources > > and in my case, I'm implementing that as a list of parents name, but the > > parent names list doesn't work with duplicates, so I have to find some other > > way to do it. > > > > Once I have something clean and tested enough I'll send it upstream. > > Ok. Thanks for the info. > Just to avoid any kind of confusion. The whole refactoring of the SCCG clock will be done in a separate (later) change and will not be part of this patchset. I already sent the 12th version of this current patch series and I would really like to get this in ASAP so that the booting up of imx8mq will not be delayed. Thanks Abel --
Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
Quoting Abel Vesa (2018-11-07 12:26:25) > On Wed, Nov 07, 2018 at 11:01:02AM -0800, Stephen Boyd wrote: > > > > > > What's the plan to clean it up? > > So I'm doing this in our internal tree first to make sure I don't break the > other (newer) socs. > > I already have a prototype in testing but it's a long way to upstream it. > > Basically, I'm replacing all of this with a single, more like a composite, > more complex, clock type that does all the magic inside. > > One of the problems is the fact that the bypasses can have the same sources > and in my case, I'm implementing that as a list of parents name, but the > parent names list doesn't work with duplicates, so I have to find some other > way to do it. > > Once I have something clean and tested enough I'll send it upstream. Ok. Thanks for the info.
Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
Quoting Abel Vesa (2018-11-07 12:26:25) > On Wed, Nov 07, 2018 at 11:01:02AM -0800, Stephen Boyd wrote: > > > > > > What's the plan to clean it up? > > So I'm doing this in our internal tree first to make sure I don't break the > other (newer) socs. > > I already have a prototype in testing but it's a long way to upstream it. > > Basically, I'm replacing all of this with a single, more like a composite, > more complex, clock type that does all the magic inside. > > One of the problems is the fact that the bypasses can have the same sources > and in my case, I'm implementing that as a list of parents name, but the > parent names list doesn't work with duplicates, so I have to find some other > way to do it. > > Once I have something clean and tested enough I'll send it upstream. Ok. Thanks for the info.
Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
On Wed, Nov 07, 2018 at 11:01:02AM -0800, Stephen Boyd wrote:
> Quoting Abel Vesa (2018-11-07 03:54:45)
> > On Wed, Oct 17, 2018 at 12:55:52PM -0700, Stephen Boyd wrote:
> > > Quoting Abel Vesa (2018-09-24 03:39:55)
> > > > +static unsigned long clk_pll2_recalc_rate(struct clk_hw *hw,
> > > > +unsigned long parent_rate)
> > > > +{
> > > > + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > > > + u32 val, ref, divr1, divf1, divr2, divf2;
> > > > + u64 temp64;
> > > > +
> > > > + val = readl_relaxed(pll->base + PLL_CFG0);
> > > > + switch (FIELD_GET(PLL_REF_MASK, val)) {
> > > > + case 0:
> > > > + ref = OSC_25M;
> > > > + break;
> > > > + case 1:
> > > > + ref = OSC_27M;
> > > > + break;
> > > > + default:
> > > > + ref = OSC_25M;
> > >
> > > Does this information not come through 'parent_rate'?
> > >
> >
> > No. So basically both pll1 and pll2 and the divider after it form together
> > this SCCG:
> >
> > https://www.nxp.com/docs/en/reference-manual/IMX8MDQLQRM.pdf#page=834
> >
> > See: Figure 5-8. SSCG PLL Block Diagram
>
> Thanks for the link!
>
> >
> > We're basically reading the input of the pll 1 in order to compute the
> > output of the entire SCCG.
> >
> > I know it's a mess. I'm working on cleaning it up, but for now we need this
> > in in order to boot up.
>
> What's the plan to clean it up?
So I'm doing this in our internal tree first to make sure I don't break the
other (newer) socs.
I already have a prototype in testing but it's a long way to upstream it.
Basically, I'm replacing all of this with a single, more like a composite,
more complex, clock type that does all the magic inside.
One of the problems is the fact that the bypasses can have the same sources
and in my case, I'm implementing that as a list of parents name, but the
parent names list doesn't work with duplicates, so I have to find some other
way to do it.
Once I have something clean and tested enough I'll send it upstream.
>
> >
> > > > + break;
> > > > + }
> > > > +
> > > > + val = readl_relaxed(pll->base + PLL_CFG2);
> > > > + divr1 = FIELD_GET(PLL_DIVR1_MASK, val);
> > > > + divr2 = FIELD_GET(PLL_DIVR2_MASK, val);
> > > > + divf1 = FIELD_GET(PLL_DIVF1_MASK, val);
> > > > + divf2 = FIELD_GET(PLL_DIVF2_MASK, val);
> > > > +
> > > > + temp64 = ref * 2;
> > > > + temp64 *= (divf1 + 1) * (divf2 + 1);
> > > > +
> > > > + do_div(temp64, (divr1 + 1) * (divr2 + 1));
> > >
> > > Nitpicks: A comment with the equation may be helpful to newcomers.
> >
> > Since the SCCG is contructed by multiple different types of clocks here,
> > the equation doesn't help
> > since it is spread in all constructing blocks.
>
> Ok.
>
--
Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
On Wed, Nov 07, 2018 at 11:01:02AM -0800, Stephen Boyd wrote:
> Quoting Abel Vesa (2018-11-07 03:54:45)
> > On Wed, Oct 17, 2018 at 12:55:52PM -0700, Stephen Boyd wrote:
> > > Quoting Abel Vesa (2018-09-24 03:39:55)
> > > > +static unsigned long clk_pll2_recalc_rate(struct clk_hw *hw,
> > > > +unsigned long parent_rate)
> > > > +{
> > > > + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > > > + u32 val, ref, divr1, divf1, divr2, divf2;
> > > > + u64 temp64;
> > > > +
> > > > + val = readl_relaxed(pll->base + PLL_CFG0);
> > > > + switch (FIELD_GET(PLL_REF_MASK, val)) {
> > > > + case 0:
> > > > + ref = OSC_25M;
> > > > + break;
> > > > + case 1:
> > > > + ref = OSC_27M;
> > > > + break;
> > > > + default:
> > > > + ref = OSC_25M;
> > >
> > > Does this information not come through 'parent_rate'?
> > >
> >
> > No. So basically both pll1 and pll2 and the divider after it form together
> > this SCCG:
> >
> > https://www.nxp.com/docs/en/reference-manual/IMX8MDQLQRM.pdf#page=834
> >
> > See: Figure 5-8. SSCG PLL Block Diagram
>
> Thanks for the link!
>
> >
> > We're basically reading the input of the pll 1 in order to compute the
> > output of the entire SCCG.
> >
> > I know it's a mess. I'm working on cleaning it up, but for now we need this
> > in in order to boot up.
>
> What's the plan to clean it up?
So I'm doing this in our internal tree first to make sure I don't break the
other (newer) socs.
I already have a prototype in testing but it's a long way to upstream it.
Basically, I'm replacing all of this with a single, more like a composite,
more complex, clock type that does all the magic inside.
One of the problems is the fact that the bypasses can have the same sources
and in my case, I'm implementing that as a list of parents name, but the
parent names list doesn't work with duplicates, so I have to find some other
way to do it.
Once I have something clean and tested enough I'll send it upstream.
>
> >
> > > > + break;
> > > > + }
> > > > +
> > > > + val = readl_relaxed(pll->base + PLL_CFG2);
> > > > + divr1 = FIELD_GET(PLL_DIVR1_MASK, val);
> > > > + divr2 = FIELD_GET(PLL_DIVR2_MASK, val);
> > > > + divf1 = FIELD_GET(PLL_DIVF1_MASK, val);
> > > > + divf2 = FIELD_GET(PLL_DIVF2_MASK, val);
> > > > +
> > > > + temp64 = ref * 2;
> > > > + temp64 *= (divf1 + 1) * (divf2 + 1);
> > > > +
> > > > + do_div(temp64, (divr1 + 1) * (divr2 + 1));
> > >
> > > Nitpicks: A comment with the equation may be helpful to newcomers.
> >
> > Since the SCCG is contructed by multiple different types of clocks here,
> > the equation doesn't help
> > since it is spread in all constructing blocks.
>
> Ok.
>
--
Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
Quoting Abel Vesa (2018-11-07 03:54:45)
> On Wed, Oct 17, 2018 at 12:55:52PM -0700, Stephen Boyd wrote:
> > Quoting Abel Vesa (2018-09-24 03:39:55)
> > > +static unsigned long clk_pll2_recalc_rate(struct clk_hw *hw,
> > > +unsigned long parent_rate)
> > > +{
> > > + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > > + u32 val, ref, divr1, divf1, divr2, divf2;
> > > + u64 temp64;
> > > +
> > > + val = readl_relaxed(pll->base + PLL_CFG0);
> > > + switch (FIELD_GET(PLL_REF_MASK, val)) {
> > > + case 0:
> > > + ref = OSC_25M;
> > > + break;
> > > + case 1:
> > > + ref = OSC_27M;
> > > + break;
> > > + default:
> > > + ref = OSC_25M;
> >
> > Does this information not come through 'parent_rate'?
> >
>
> No. So basically both pll1 and pll2 and the divider after it form together
> this SCCG:
>
> https://www.nxp.com/docs/en/reference-manual/IMX8MDQLQRM.pdf#page=834
>
> See: Figure 5-8. SSCG PLL Block Diagram
Thanks for the link!
>
> We're basically reading the input of the pll 1 in order to compute the output
> of the entire SCCG.
>
> I know it's a mess. I'm working on cleaning it up, but for now we need this
> in in order to boot up.
What's the plan to clean it up?
>
> > > + break;
> > > + }
> > > +
> > > + val = readl_relaxed(pll->base + PLL_CFG2);
> > > + divr1 = FIELD_GET(PLL_DIVR1_MASK, val);
> > > + divr2 = FIELD_GET(PLL_DIVR2_MASK, val);
> > > + divf1 = FIELD_GET(PLL_DIVF1_MASK, val);
> > > + divf2 = FIELD_GET(PLL_DIVF2_MASK, val);
> > > +
> > > + temp64 = ref * 2;
> > > + temp64 *= (divf1 + 1) * (divf2 + 1);
> > > +
> > > + do_div(temp64, (divr1 + 1) * (divr2 + 1));
> >
> > Nitpicks: A comment with the equation may be helpful to newcomers.
>
> Since the SCCG is contructed by multiple different types of clocks here, the
> equation doesn't help
> since it is spread in all constructing blocks.
Ok.
Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
Quoting Abel Vesa (2018-11-07 03:54:45)
> On Wed, Oct 17, 2018 at 12:55:52PM -0700, Stephen Boyd wrote:
> > Quoting Abel Vesa (2018-09-24 03:39:55)
> > > +static unsigned long clk_pll2_recalc_rate(struct clk_hw *hw,
> > > +unsigned long parent_rate)
> > > +{
> > > + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > > + u32 val, ref, divr1, divf1, divr2, divf2;
> > > + u64 temp64;
> > > +
> > > + val = readl_relaxed(pll->base + PLL_CFG0);
> > > + switch (FIELD_GET(PLL_REF_MASK, val)) {
> > > + case 0:
> > > + ref = OSC_25M;
> > > + break;
> > > + case 1:
> > > + ref = OSC_27M;
> > > + break;
> > > + default:
> > > + ref = OSC_25M;
> >
> > Does this information not come through 'parent_rate'?
> >
>
> No. So basically both pll1 and pll2 and the divider after it form together
> this SCCG:
>
> https://www.nxp.com/docs/en/reference-manual/IMX8MDQLQRM.pdf#page=834
>
> See: Figure 5-8. SSCG PLL Block Diagram
Thanks for the link!
>
> We're basically reading the input of the pll 1 in order to compute the output
> of the entire SCCG.
>
> I know it's a mess. I'm working on cleaning it up, but for now we need this
> in in order to boot up.
What's the plan to clean it up?
>
> > > + break;
> > > + }
> > > +
> > > + val = readl_relaxed(pll->base + PLL_CFG2);
> > > + divr1 = FIELD_GET(PLL_DIVR1_MASK, val);
> > > + divr2 = FIELD_GET(PLL_DIVR2_MASK, val);
> > > + divf1 = FIELD_GET(PLL_DIVF1_MASK, val);
> > > + divf2 = FIELD_GET(PLL_DIVF2_MASK, val);
> > > +
> > > + temp64 = ref * 2;
> > > + temp64 *= (divf1 + 1) * (divf2 + 1);
> > > +
> > > + do_div(temp64, (divr1 + 1) * (divr2 + 1));
> >
> > Nitpicks: A comment with the equation may be helpful to newcomers.
>
> Since the SCCG is contructed by multiple different types of clocks here, the
> equation doesn't help
> since it is spread in all constructing blocks.
Ok.
Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
On Wed, Oct 17, 2018 at 12:55:52PM -0700, Stephen Boyd wrote:
> Quoting Abel Vesa (2018-09-24 03:39:55)
> > diff --git a/drivers/clk/imx/clk-sccg-pll.c b/drivers/clk/imx/clk-sccg-pll.c
> > new file mode 100644
> > index 000..a9837fa
> > --- /dev/null
> > +++ b/drivers/clk/imx/clk-sccg-pll.c
> > @@ -0,0 +1,237 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright 2018 NXP.
> > + */
> > +
> > +#include
>
> Is this include used? Otherwise should see clk-provider.h included here.
>
Fixed in the next version.
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "clk.h"
> > +
> > +/* PLL CFGs */
> > +#define PLL_CFG0 0x0
> > +#define PLL_CFG1 0x4
> > +#define PLL_CFG2 0x8
> > +
> > +#define PLL_DIVF1_MASK GENMASK(18, 13)
> > +#define PLL_DIVF2_MASK GENMASK(12, 7)
> > +#define PLL_DIVR1_MASK GENMASK(27, 25)
> > +#define PLL_DIVR2_MASK GENMASK(24, 19)
> > +#define PLL_REF_MASK GENMASK(2, 0)
> > +
> > +#define PLL_LOCK_MASK BIT(31)
> > +#define PLL_PD_MASKBIT(7)
> > +
> > +#define OSC_25M2500
> > +#define OSC_27M2700
> > +
> > +#define PLL_SCCG_LOCK_TIMEOUT 70
> > +
> > +struct clk_sccg_pll {
> > + struct clk_hw hw;
> > + void __iomem*base;
> > +};
> > +
> > +#define to_clk_sccg_pll(_hw) container_of(_hw, struct clk_sccg_pll, hw)
> > +
> > +static int clk_pll_wait_lock(struct clk_sccg_pll *pll)
> > +{
> > + u32 val;
> > +
> > + return readl_poll_timeout(pll->base, val, val & PLL_LOCK_MASK, 0,
> > + PLL_SCCG_LOCK_TIMEOUT);
> > +}
> > +
> > +static int clk_pll1_is_prepared(struct clk_hw *hw)
> > +{
> > + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > + u32 val;
> > +
> > + val = readl_relaxed(pll->base + PLL_CFG0);
> > + return (val & PLL_PD_MASK) ? 0 : 1;
> > +}
> > +
> > +static unsigned long clk_pll1_recalc_rate(struct clk_hw *hw,
> > +unsigned long parent_rate)
> > +{
> > + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > + u32 val, divf;
> > +
> > + val = readl_relaxed(pll->base + PLL_CFG2);
> > + divf = FIELD_GET(PLL_DIVF1_MASK, val);
> > +
> > + return parent_rate * 2 * (divf + 1);
> > +}
> > +
> > +static long clk_pll1_round_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long *prate)
> > +{
> > + unsigned long parent_rate = *prate;
> > + u32 div;
> > +
> > + div = rate / (parent_rate * 2);
>
> Can parent_rate be 0?
>
Fixed in the next version.
> > +
> > + return parent_rate * div * 2;
> > +}
> > +
> > +static int clk_pll1_set_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long parent_rate)
> > +{
> > + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > + u32 val;
> > + u32 divf;
> > +
> > + divf = rate / (parent_rate * 2);
>
> Can parent_rate be 0?
>
Fixed in the next version.
> > +
> > + val = readl_relaxed(pll->base + PLL_CFG2);
> > + val &= ~PLL_DIVF1_MASK;
> > + val |= FIELD_PREP(PLL_DIVF1_MASK, divf - 1);
> > + writel_relaxed(val, pll->base + PLL_CFG2);
> > +
> > + return clk_pll_wait_lock(pll);
> > +}
> > +
> > +static int clk_pll1_prepare(struct clk_hw *hw)
> > +{
> > + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > + u32 val;
> > +
> > + val = readl_relaxed(pll->base + PLL_CFG0);
> > + val &= ~PLL_PD_MASK;
> > + writel_relaxed(val, pll->base + PLL_CFG0);
> > +
> > + return clk_pll_wait_lock(pll);
> > +}
> > +
> > +static void clk_pll1_unprepare(struct clk_hw *hw)
> > +{
> > + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > + u32 val;
> > +
> > + val = readl_relaxed(pll->base + PLL_CFG0);
> > + val |= PLL_PD_MASK;
> > + writel_relaxed(val, pll->base + PLL_CFG0);
> > +
> > +}
> > +
> > +static unsigned long clk_pll2_recalc_rate(struct clk_hw *hw,
> > +unsigned long parent_rate)
> > +{
> > + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > + u32 val, ref, divr1, divf1, divr2, divf2;
> > + u64 temp64;
> > +
> > + val = readl_relaxed(pll->base + PLL_CFG0);
> > + switch (FIELD_GET(PLL_REF_MASK, val)) {
> > + case 0:
> > + ref = OSC_25M;
> > + break;
> > + case 1:
> > + ref = OSC_27M;
> > + break;
> > + default:
> > + ref = OSC_25M;
>
> Does this information not come through 'parent_rate'?
>
No. So basically both pll1 and pll2 and the divider after it form together this
SCCG:
https://www.nxp.com/docs/en/reference-manual/IMX8MDQLQRM.pdf#page=834
See: Figure 5-8. SSCG PLL Block Diagram
We're
Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
On Wed, Oct 17, 2018 at 12:55:52PM -0700, Stephen Boyd wrote:
> Quoting Abel Vesa (2018-09-24 03:39:55)
> > diff --git a/drivers/clk/imx/clk-sccg-pll.c b/drivers/clk/imx/clk-sccg-pll.c
> > new file mode 100644
> > index 000..a9837fa
> > --- /dev/null
> > +++ b/drivers/clk/imx/clk-sccg-pll.c
> > @@ -0,0 +1,237 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright 2018 NXP.
> > + */
> > +
> > +#include
>
> Is this include used? Otherwise should see clk-provider.h included here.
>
Fixed in the next version.
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "clk.h"
> > +
> > +/* PLL CFGs */
> > +#define PLL_CFG0 0x0
> > +#define PLL_CFG1 0x4
> > +#define PLL_CFG2 0x8
> > +
> > +#define PLL_DIVF1_MASK GENMASK(18, 13)
> > +#define PLL_DIVF2_MASK GENMASK(12, 7)
> > +#define PLL_DIVR1_MASK GENMASK(27, 25)
> > +#define PLL_DIVR2_MASK GENMASK(24, 19)
> > +#define PLL_REF_MASK GENMASK(2, 0)
> > +
> > +#define PLL_LOCK_MASK BIT(31)
> > +#define PLL_PD_MASKBIT(7)
> > +
> > +#define OSC_25M2500
> > +#define OSC_27M2700
> > +
> > +#define PLL_SCCG_LOCK_TIMEOUT 70
> > +
> > +struct clk_sccg_pll {
> > + struct clk_hw hw;
> > + void __iomem*base;
> > +};
> > +
> > +#define to_clk_sccg_pll(_hw) container_of(_hw, struct clk_sccg_pll, hw)
> > +
> > +static int clk_pll_wait_lock(struct clk_sccg_pll *pll)
> > +{
> > + u32 val;
> > +
> > + return readl_poll_timeout(pll->base, val, val & PLL_LOCK_MASK, 0,
> > + PLL_SCCG_LOCK_TIMEOUT);
> > +}
> > +
> > +static int clk_pll1_is_prepared(struct clk_hw *hw)
> > +{
> > + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > + u32 val;
> > +
> > + val = readl_relaxed(pll->base + PLL_CFG0);
> > + return (val & PLL_PD_MASK) ? 0 : 1;
> > +}
> > +
> > +static unsigned long clk_pll1_recalc_rate(struct clk_hw *hw,
> > +unsigned long parent_rate)
> > +{
> > + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > + u32 val, divf;
> > +
> > + val = readl_relaxed(pll->base + PLL_CFG2);
> > + divf = FIELD_GET(PLL_DIVF1_MASK, val);
> > +
> > + return parent_rate * 2 * (divf + 1);
> > +}
> > +
> > +static long clk_pll1_round_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long *prate)
> > +{
> > + unsigned long parent_rate = *prate;
> > + u32 div;
> > +
> > + div = rate / (parent_rate * 2);
>
> Can parent_rate be 0?
>
Fixed in the next version.
> > +
> > + return parent_rate * div * 2;
> > +}
> > +
> > +static int clk_pll1_set_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long parent_rate)
> > +{
> > + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > + u32 val;
> > + u32 divf;
> > +
> > + divf = rate / (parent_rate * 2);
>
> Can parent_rate be 0?
>
Fixed in the next version.
> > +
> > + val = readl_relaxed(pll->base + PLL_CFG2);
> > + val &= ~PLL_DIVF1_MASK;
> > + val |= FIELD_PREP(PLL_DIVF1_MASK, divf - 1);
> > + writel_relaxed(val, pll->base + PLL_CFG2);
> > +
> > + return clk_pll_wait_lock(pll);
> > +}
> > +
> > +static int clk_pll1_prepare(struct clk_hw *hw)
> > +{
> > + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > + u32 val;
> > +
> > + val = readl_relaxed(pll->base + PLL_CFG0);
> > + val &= ~PLL_PD_MASK;
> > + writel_relaxed(val, pll->base + PLL_CFG0);
> > +
> > + return clk_pll_wait_lock(pll);
> > +}
> > +
> > +static void clk_pll1_unprepare(struct clk_hw *hw)
> > +{
> > + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > + u32 val;
> > +
> > + val = readl_relaxed(pll->base + PLL_CFG0);
> > + val |= PLL_PD_MASK;
> > + writel_relaxed(val, pll->base + PLL_CFG0);
> > +
> > +}
> > +
> > +static unsigned long clk_pll2_recalc_rate(struct clk_hw *hw,
> > +unsigned long parent_rate)
> > +{
> > + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > + u32 val, ref, divr1, divf1, divr2, divf2;
> > + u64 temp64;
> > +
> > + val = readl_relaxed(pll->base + PLL_CFG0);
> > + switch (FIELD_GET(PLL_REF_MASK, val)) {
> > + case 0:
> > + ref = OSC_25M;
> > + break;
> > + case 1:
> > + ref = OSC_27M;
> > + break;
> > + default:
> > + ref = OSC_25M;
>
> Does this information not come through 'parent_rate'?
>
No. So basically both pll1 and pll2 and the divider after it form together this
SCCG:
https://www.nxp.com/docs/en/reference-manual/IMX8MDQLQRM.pdf#page=834
See: Figure 5-8. SSCG PLL Block Diagram
We're
Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
Quoting Abel Vesa (2018-09-24 03:39:55)
> diff --git a/drivers/clk/imx/clk-sccg-pll.c b/drivers/clk/imx/clk-sccg-pll.c
> new file mode 100644
> index 000..a9837fa
> --- /dev/null
> +++ b/drivers/clk/imx/clk-sccg-pll.c
> @@ -0,0 +1,237 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright 2018 NXP.
> + */
> +
> +#include
Is this include used? Otherwise should see clk-provider.h included here.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "clk.h"
> +
> +/* PLL CFGs */
> +#define PLL_CFG0 0x0
> +#define PLL_CFG1 0x4
> +#define PLL_CFG2 0x8
> +
> +#define PLL_DIVF1_MASK GENMASK(18, 13)
> +#define PLL_DIVF2_MASK GENMASK(12, 7)
> +#define PLL_DIVR1_MASK GENMASK(27, 25)
> +#define PLL_DIVR2_MASK GENMASK(24, 19)
> +#define PLL_REF_MASK GENMASK(2, 0)
> +
> +#define PLL_LOCK_MASK BIT(31)
> +#define PLL_PD_MASKBIT(7)
> +
> +#define OSC_25M2500
> +#define OSC_27M2700
> +
> +#define PLL_SCCG_LOCK_TIMEOUT 70
> +
> +struct clk_sccg_pll {
> + struct clk_hw hw;
> + void __iomem*base;
> +};
> +
> +#define to_clk_sccg_pll(_hw) container_of(_hw, struct clk_sccg_pll, hw)
> +
> +static int clk_pll_wait_lock(struct clk_sccg_pll *pll)
> +{
> + u32 val;
> +
> + return readl_poll_timeout(pll->base, val, val & PLL_LOCK_MASK, 0,
> + PLL_SCCG_LOCK_TIMEOUT);
> +}
> +
> +static int clk_pll1_is_prepared(struct clk_hw *hw)
> +{
> + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> + u32 val;
> +
> + val = readl_relaxed(pll->base + PLL_CFG0);
> + return (val & PLL_PD_MASK) ? 0 : 1;
> +}
> +
> +static unsigned long clk_pll1_recalc_rate(struct clk_hw *hw,
> +unsigned long parent_rate)
> +{
> + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> + u32 val, divf;
> +
> + val = readl_relaxed(pll->base + PLL_CFG2);
> + divf = FIELD_GET(PLL_DIVF1_MASK, val);
> +
> + return parent_rate * 2 * (divf + 1);
> +}
> +
> +static long clk_pll1_round_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long *prate)
> +{
> + unsigned long parent_rate = *prate;
> + u32 div;
> +
> + div = rate / (parent_rate * 2);
Can parent_rate be 0?
> +
> + return parent_rate * div * 2;
> +}
> +
> +static int clk_pll1_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> + u32 val;
> + u32 divf;
> +
> + divf = rate / (parent_rate * 2);
Can parent_rate be 0?
> +
> + val = readl_relaxed(pll->base + PLL_CFG2);
> + val &= ~PLL_DIVF1_MASK;
> + val |= FIELD_PREP(PLL_DIVF1_MASK, divf - 1);
> + writel_relaxed(val, pll->base + PLL_CFG2);
> +
> + return clk_pll_wait_lock(pll);
> +}
> +
> +static int clk_pll1_prepare(struct clk_hw *hw)
> +{
> + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> + u32 val;
> +
> + val = readl_relaxed(pll->base + PLL_CFG0);
> + val &= ~PLL_PD_MASK;
> + writel_relaxed(val, pll->base + PLL_CFG0);
> +
> + return clk_pll_wait_lock(pll);
> +}
> +
> +static void clk_pll1_unprepare(struct clk_hw *hw)
> +{
> + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> + u32 val;
> +
> + val = readl_relaxed(pll->base + PLL_CFG0);
> + val |= PLL_PD_MASK;
> + writel_relaxed(val, pll->base + PLL_CFG0);
> +
> +}
> +
> +static unsigned long clk_pll2_recalc_rate(struct clk_hw *hw,
> +unsigned long parent_rate)
> +{
> + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> + u32 val, ref, divr1, divf1, divr2, divf2;
> + u64 temp64;
> +
> + val = readl_relaxed(pll->base + PLL_CFG0);
> + switch (FIELD_GET(PLL_REF_MASK, val)) {
> + case 0:
> + ref = OSC_25M;
> + break;
> + case 1:
> + ref = OSC_27M;
> + break;
> + default:
> + ref = OSC_25M;
Does this information not come through 'parent_rate'?
> + break;
> + }
> +
> + val = readl_relaxed(pll->base + PLL_CFG2);
> + divr1 = FIELD_GET(PLL_DIVR1_MASK, val);
> + divr2 = FIELD_GET(PLL_DIVR2_MASK, val);
> + divf1 = FIELD_GET(PLL_DIVF1_MASK, val);
> + divf2 = FIELD_GET(PLL_DIVF2_MASK, val);
> +
> + temp64 = ref * 2;
> + temp64 *= (divf1 + 1) * (divf2 + 1);
> +
> + do_div(temp64, (divr1 + 1) * (divr2 + 1));
Nitpicks: A comment with the equation may be helpful to newcomers.
> +
> + return (unsigned long)temp64;
Drop useless cast please.
> +}
> +
> +static long clk_pll2_round_rate(struct clk_hw *hw, unsigned long rate,
> +
Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type
Quoting Abel Vesa (2018-09-24 03:39:55)
> diff --git a/drivers/clk/imx/clk-sccg-pll.c b/drivers/clk/imx/clk-sccg-pll.c
> new file mode 100644
> index 000..a9837fa
> --- /dev/null
> +++ b/drivers/clk/imx/clk-sccg-pll.c
> @@ -0,0 +1,237 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright 2018 NXP.
> + */
> +
> +#include
Is this include used? Otherwise should see clk-provider.h included here.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "clk.h"
> +
> +/* PLL CFGs */
> +#define PLL_CFG0 0x0
> +#define PLL_CFG1 0x4
> +#define PLL_CFG2 0x8
> +
> +#define PLL_DIVF1_MASK GENMASK(18, 13)
> +#define PLL_DIVF2_MASK GENMASK(12, 7)
> +#define PLL_DIVR1_MASK GENMASK(27, 25)
> +#define PLL_DIVR2_MASK GENMASK(24, 19)
> +#define PLL_REF_MASK GENMASK(2, 0)
> +
> +#define PLL_LOCK_MASK BIT(31)
> +#define PLL_PD_MASKBIT(7)
> +
> +#define OSC_25M2500
> +#define OSC_27M2700
> +
> +#define PLL_SCCG_LOCK_TIMEOUT 70
> +
> +struct clk_sccg_pll {
> + struct clk_hw hw;
> + void __iomem*base;
> +};
> +
> +#define to_clk_sccg_pll(_hw) container_of(_hw, struct clk_sccg_pll, hw)
> +
> +static int clk_pll_wait_lock(struct clk_sccg_pll *pll)
> +{
> + u32 val;
> +
> + return readl_poll_timeout(pll->base, val, val & PLL_LOCK_MASK, 0,
> + PLL_SCCG_LOCK_TIMEOUT);
> +}
> +
> +static int clk_pll1_is_prepared(struct clk_hw *hw)
> +{
> + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> + u32 val;
> +
> + val = readl_relaxed(pll->base + PLL_CFG0);
> + return (val & PLL_PD_MASK) ? 0 : 1;
> +}
> +
> +static unsigned long clk_pll1_recalc_rate(struct clk_hw *hw,
> +unsigned long parent_rate)
> +{
> + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> + u32 val, divf;
> +
> + val = readl_relaxed(pll->base + PLL_CFG2);
> + divf = FIELD_GET(PLL_DIVF1_MASK, val);
> +
> + return parent_rate * 2 * (divf + 1);
> +}
> +
> +static long clk_pll1_round_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long *prate)
> +{
> + unsigned long parent_rate = *prate;
> + u32 div;
> +
> + div = rate / (parent_rate * 2);
Can parent_rate be 0?
> +
> + return parent_rate * div * 2;
> +}
> +
> +static int clk_pll1_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> + u32 val;
> + u32 divf;
> +
> + divf = rate / (parent_rate * 2);
Can parent_rate be 0?
> +
> + val = readl_relaxed(pll->base + PLL_CFG2);
> + val &= ~PLL_DIVF1_MASK;
> + val |= FIELD_PREP(PLL_DIVF1_MASK, divf - 1);
> + writel_relaxed(val, pll->base + PLL_CFG2);
> +
> + return clk_pll_wait_lock(pll);
> +}
> +
> +static int clk_pll1_prepare(struct clk_hw *hw)
> +{
> + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> + u32 val;
> +
> + val = readl_relaxed(pll->base + PLL_CFG0);
> + val &= ~PLL_PD_MASK;
> + writel_relaxed(val, pll->base + PLL_CFG0);
> +
> + return clk_pll_wait_lock(pll);
> +}
> +
> +static void clk_pll1_unprepare(struct clk_hw *hw)
> +{
> + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> + u32 val;
> +
> + val = readl_relaxed(pll->base + PLL_CFG0);
> + val |= PLL_PD_MASK;
> + writel_relaxed(val, pll->base + PLL_CFG0);
> +
> +}
> +
> +static unsigned long clk_pll2_recalc_rate(struct clk_hw *hw,
> +unsigned long parent_rate)
> +{
> + struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> + u32 val, ref, divr1, divf1, divr2, divf2;
> + u64 temp64;
> +
> + val = readl_relaxed(pll->base + PLL_CFG0);
> + switch (FIELD_GET(PLL_REF_MASK, val)) {
> + case 0:
> + ref = OSC_25M;
> + break;
> + case 1:
> + ref = OSC_27M;
> + break;
> + default:
> + ref = OSC_25M;
Does this information not come through 'parent_rate'?
> + break;
> + }
> +
> + val = readl_relaxed(pll->base + PLL_CFG2);
> + divr1 = FIELD_GET(PLL_DIVR1_MASK, val);
> + divr2 = FIELD_GET(PLL_DIVR2_MASK, val);
> + divf1 = FIELD_GET(PLL_DIVF1_MASK, val);
> + divf2 = FIELD_GET(PLL_DIVF2_MASK, val);
> +
> + temp64 = ref * 2;
> + temp64 *= (divf1 + 1) * (divf2 + 1);
> +
> + do_div(temp64, (divr1 + 1) * (divr2 + 1));
Nitpicks: A comment with the equation may be helpful to newcomers.
> +
> + return (unsigned long)temp64;
Drop useless cast please.
> +}
> +
> +static long clk_pll2_round_rate(struct clk_hw *hw, unsigned long rate,
> +
