RE: [PATCH v2 1/3] clk: zynqmp: Use firmware specific common clock flags
From: Amit Sunil Dhamne
Date: Thu Jul 23 2020 - 19:46:37 EST
Hi Michael,
Thanks for the review. Replies inline.
Thanks,
Amit
> -----Original Message-----
> From: Michael Tretter <m.tretter@xxxxxxxxxxxxxx>
> Sent: Wednesday, July 22, 2020 6:23 AM
> To: Amit Sunil Dhamne <amitsuni@xxxxxxxxxx>
> Cc: mturquette@xxxxxxxxxxxx; sboyd@xxxxxxxxxxxxxx; sboyd@xxxxxxxxxx;
> Michal Simek <michals@xxxxxxxxxx>; mark.rutland@xxxxxxx; linux-
> clk@xxxxxxxxxxxxxxx; Rajan Vaja <RAJANV@xxxxxxxxxx>; Jolly Shah
> <JOLLYS@xxxxxxxxxx>; Tejas Patel <TEJASP@xxxxxxxxxx>; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Rajan Vaja
> <RAJANV@xxxxxxxxxx>; Tejas Patel <TEJASP@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Tejas Patel <tejas.patel@xxxxxxxxxx>
> > Signed-off-by: Amit Sunil Dhamne <amit.sunil.dhamne@xxxxxxxxxx>
> > ---
> > 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.
> >