RE: [PATCH v9 10/10] drivers: clk: Add ZynqMP clock driver

[Stephen Boyd]

Quoting Jolly Shah (2018-07-17 13:09:01)
> Hi Stephen,
> 
> Thanks for the review,
> 
> > -----Original Message-----
> > From: Stephen Boyd [mailto:sboyd@xxxxxxxxxx]
> > Sent: Sunday, July 08, 2018 10:27 PM
> > To: Jolly Shah <JOLLYS@xxxxxxxxxx>; ard.biesheuvel@xxxxxxxxxx;
> > dmitry.torokhov@xxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx;
> > hkallweit1@xxxxxxxxx; keescook@xxxxxxxxxxxx; linux-clk@xxxxxxxxxxxxxxx;
> > mark.rutland@xxxxxxx; matt@xxxxxxxxxxxxxxxxxxx; Michal Simek
> > <michals@xxxxxxxxxx>; mingo@xxxxxxxxxx; mturquette@xxxxxxxxxxxx;
> > robh+dt@xxxxxxxxxx; sboyd@xxxxxxxxxxxxxx; sudeep.holla@xxxxxxx
> > Cc: Rajan Vaja <RAJANV@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Jolly Shah
> > <JOLLYS@xxxxxxxxxx>; Tejas Patel <TEJASP@xxxxxxxxxx>; Shubhrajyoti Datta
> > <shubhraj@xxxxxxxxxx>; Jolly Shah <JOLLYS@xxxxxxxxxx>
> > Subject: Re: [PATCH v9 10/10] drivers: clk: Add ZynqMP clock driver
> > 
> > > +/**
> > > + * zynqmp_pm_clock_get_parents() - Get the first 3 parents of clock for given
> > id
> > > + * @clock_id:  Clock ID
> > > + * @index:     Parent index
> > > + * @parents:   3 parents of the given clock
> > > + *
> > > + * This function is used to get 3 parents for the clock specified by
> > > + * given clock ID.
> > > + *
> > > + * This API will return 3 parents with a single response. To get
> > > + * other parents, master should call same API in loop with new
> > > + * parent index till error is returned. E.g First call should have
> > > + * index 0 which will return parents 0,1 and 2. Next call, index
> > > + * should be 3 which will return parent 3,4 and 5 and so on.
> > > + *
> > > + * Return: Returns status, either success or error+reason
> > > + */
> > > +static int zynqmp_pm_clock_get_parents(u32 clock_id, u32 index, u32
> > *parents)
> > > +{
> > > +       struct zynqmp_pm_query_data qdata = {0};
> > > +       u32 ret_payload[PAYLOAD_ARG_CNT];
> > 
> > What's the endianness of this payload? Is it little endian? Or do the
> > eemi_ops convert to CPU native endianness?
> 
> Its little endian

Is it CPU native? This might need to be marked as __le32 for proper
endianess code.

> 
> > > +
> > > +/**
> > > + * zynqmp_clock_init() - Initialize zynqmp clocks
> > > + *
> > > + * Return: 0 on success else error code
> > > + */
> > > +static int __init zynqmp_clock_init(void)
> > > +{
> > > +       int ret;
> > > +       struct device_node *np;
> > > +
> > > +       np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp");
> > > +       if (!np)
> > > +               return -ENOENT;
> > > +       of_node_put(np);
> > > +
> > > +       np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp-clk");
> > 
> > Why can't this be a platform device driver?
> 
> Platform driver may probe later(an actually probing later in our case). This will results in clock get failure in clock consumer peripherals. So clock registration needs to be done earlier.

That's fine though? If a clk_get() fails because the provider isn't
registered yet the consumer will see -EPROBE_DEFER and try again later.