[PATCH v2 1/3] clk: zynqmp: Use firmware specific common clock flags

Amit Sunil Dhamne amitsuni at xilinx.com
Thu Jul 23 19:46:31 EDT 2020


Hi Michael,
Thanks for the review. Replies inline.

Thanks,
Amit

> -----Original Message-----
> From: Michael Tretter <m.tretter at pengutronix.de>
> Sent: Wednesday, July 22, 2020 6:23 AM
> To: Amit Sunil Dhamne <amitsuni at xilinx.com>
> Cc: mturquette at baylibre.com; sboyd at codeaurora.org; sboyd at kernel.org;
> Michal Simek <michals at xilinx.com>; mark.rutland at arm.com; linux-
> clk at vger.kernel.org; Rajan Vaja <RAJANV at xilinx.com>; Jolly Shah
> <JOLLYS at xilinx.com>; Tejas Patel <TEJASP at xilinx.com>; linux-arm-
> kernel at lists.infradead.org; linux-kernel at vger.kernel.org; Rajan Vaja
> <RAJANV at xilinx.com>; Tejas Patel <TEJASP at xilinx.com>
> Subject: Re: [PATCH v2 1/3] clk: zynqmp: Use firmware specific common clock
> flags
> 
> On Tue, 21 Jul 2020 23:55:30 -0700, Amit Sunil Dhamne wrote:
> > From: Rajan Vaja <rajan.vaja at xilinx.com>
> >
> > Currently firmware passes CCF specific flags to ZynqMP clock driver.
> > So firmware needs to be updated if CCF flags are changed. The firmware
> > should have its own 'flag number space' that is distinct from the
> > common clk framework's 'flag number space'. So define and use ZynqMP
> > specific common clock flags instead of using CCF flags.
> >
> > Signed-off-by: Rajan Vaja <rajan.vaja at xilinx.com>
> > Signed-off-by: Tejas Patel <tejas.patel at xilinx.com>
> > Signed-off-by: Amit Sunil Dhamne <amit.sunil.dhamne at xilinx.com>
> > ---
> >  drivers/clk/zynqmp/clk-gate-zynqmp.c |  4 +++-
> >  drivers/clk/zynqmp/clk-mux-zynqmp.c  |  4 +++-
> >  drivers/clk/zynqmp/clk-zynqmp.h      | 25 +++++++++++++++++++++++++
> >  drivers/clk/zynqmp/clkc.c            | 31 ++++++++++++++++++++++++++++++-
> >  drivers/clk/zynqmp/divider.c         |  5 +++--
> >  drivers/clk/zynqmp/pll.c             |  4 +++-
> >  6 files changed, 67 insertions(+), 6 deletions(-)
> >
> [snip]
> > diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
> > index db8d0d7..11351f6 100644
> > --- a/drivers/clk/zynqmp/clkc.c
> > +++ b/drivers/clk/zynqmp/clkc.c
> > @@ -271,6 +271,32 @@ static int zynqmp_pm_clock_get_topology(u32
> clock_id, u32 index,
> >         return ret;
> >  }
> >
> > +void zynqmp_clk_map_common_ccf_flags(const u32 zynqmp_flag,
> > +                                    unsigned long *ccf_flag)
> > +{
> > +       *ccf_flag = 0;
> > +       *ccf_flag |= (zynqmp_flag & ZYNQMP_CLK_SET_RATE_GATE) ?
> > +                     CLK_SET_RATE_GATE : 0;
> > +       *ccf_flag |= (zynqmp_flag & ZYNQMP_CLK_SET_PARENT_GATE) ?
> > +                     CLK_SET_PARENT_GATE : 0;
> > +       *ccf_flag |= (zynqmp_flag & ZYNQMP_CLK_SET_RATE_PARENT) ?
> > +                     CLK_SET_RATE_PARENT : 0;
> > +       *ccf_flag |= (zynqmp_flag & ZYNQMP_CLK_IGNORE_UNUSED) ?
> > +                     CLK_IGNORE_UNUSED : 0;
> > +       *ccf_flag |= (zynqmp_flag & ZYNQMP_CLK_GET_RATE_NOCACHE) ?
> > +                     CLK_GET_RATE_NOCACHE : 0;
> > +       *ccf_flag |= (zynqmp_flag & ZYNQMP_CLK_SET_RATE_NO_REPARENT)
> ?
> > +                     CLK_SET_RATE_NO_REPARENT : 0;
> > +       *ccf_flag |= (zynqmp_flag &
> ZYNQMP_CLK_GET_ACCURACY_NOCACHE) ?
> > +                     CLK_GET_ACCURACY_NOCACHE : 0;
> > +       *ccf_flag |= (zynqmp_flag & ZYNQMP_CLK_RECALC_NEW_RATES) ?
> > +                     CLK_RECALC_NEW_RATES : 0;
> > +       *ccf_flag |= (zynqmp_flag & ZYNQMP_CLK_SET_RATE_UNGATE) ?
> > +                     CLK_SET_RATE_UNGATE : 0;
> > +       *ccf_flag |= (zynqmp_flag & ZYNQMP_CLK_IS_CRITICAL) ?
> > +                     CLK_IS_CRITICAL : 0;
> > +}
> 
> What is the reason for returning the resulting flags via pointer? I would have
> expected something like the following function:
> 
> unsigned long zynqmp_clk_flags_to_clk_flags(const u32 zyqnmp_flags)
> {
> 	unsigned long flags = 0;
> 
> 	if (zynqmp_flag & ZYNQMP_CLK_SET_RATE_GATE)
> 		flags |= CLK_SET_RATE_GATE;
> 	/* ... */
> 
> 	return flags;
> }
> 
> Michael
> 
[Amit] There's no particular reason for returning values through pointer. I will send a next version of patch with the format you suggested.
> > +
> >  /**
> >   * zynqmp_clk_register_fixed_factor() - Register fixed factor with the
> >   *                                     clock framework
> > @@ -292,6 +318,7 @@ struct clk_hw
> *zynqmp_clk_register_fixed_factor(const char *name, u32 clk_id,
> >         struct zynqmp_pm_query_data qdata = {0};
> >         u32 ret_payload[PAYLOAD_ARG_CNT];
> >         int ret;
> > +       unsigned long flag;
> >
> >         qdata.qid = PM_QID_CLOCK_GET_FIXEDFACTOR_PARAMS;
> >         qdata.arg1 = clk_id;
> > @@ -303,9 +330,11 @@ struct clk_hw
> *zynqmp_clk_register_fixed_factor(const char *name, u32 clk_id,
> >         mult = ret_payload[1];
> >         div = ret_payload[2];
> >
> > +       zynqmp_clk_map_common_ccf_flags(nodes->flag, &flag);
> > +
> >         hw = clk_hw_register_fixed_factor(NULL, name,
> >                                           parents[0],
> > -                                         nodes->flag, mult,
> > +                                         flag, mult,
> >                                           div);
> >
> >         return hw;
> > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
> > index 66da02b..3ab57d9 100644
> > --- a/drivers/clk/zynqmp/divider.c
> > +++ b/drivers/clk/zynqmp/divider.c
> > @@ -311,8 +311,9 @@ struct clk_hw *zynqmp_clk_register_divider(const
> char *name,
> >
> >         init.name = name;
> >         init.ops = &zynqmp_clk_divider_ops;
> > -       /* CLK_FRAC is not defined in the common clk framework */
> > -       init.flags = nodes->flag & ~CLK_FRAC;
> > +
> > +       zynqmp_clk_map_common_ccf_flags(nodes->flag, &init.flags);
> > +
> >         init.parent_names = parents;
> >         init.num_parents = 1;
> >
> > diff --git a/drivers/clk/zynqmp/pll.c b/drivers/clk/zynqmp/pll.c
> > index 92f449e..1b7e231 100644
> > --- a/drivers/clk/zynqmp/pll.c
> > +++ b/drivers/clk/zynqmp/pll.c
> > @@ -302,7 +302,9 @@ struct clk_hw *zynqmp_clk_register_pll(const char
> *name, u32 clk_id,
> >
> >         init.name = name;
> >         init.ops = &zynqmp_pll_ops;
> > -       init.flags = nodes->flag;
> > +
> > +       zynqmp_clk_map_common_ccf_flags(nodes->flag, &init.flags);
> > +
> >         init.parent_names = parents;
> >         init.num_parents = 1;
> >
> > --
> > 2.7.4
> >
> > This email and any attachments are intended for the sole use of the named
> recipient(s) and contain(s) confidential information that may be proprietary,
> privileged or copyrighted under applicable law. If you are not the intended
> recipient, do not read, copy, or forward this email message or any
> attachments. Delete this email message and any attachments immediately.
> >


More information about the linux-arm-kernel mailing list