Re: [PATCH v4 1/2] clk: intel: Add CGU clock driver for a new SoC

[Tanwar, Rahul]

Hi Stephen,

Thanks a lot for taking time out to review and providing feedback. I have
tried to address all your review concerns except few below mentioned points
where i need more clarification.

On 31/1/2020 10:24 AM, Stephen Boyd wrote:
> Quoting Rahul Tanwar (2020-01-30 01:04:02)
>> From: rtanwar <rahul.tanwar@xxxxxxxxx>
>>
>> Clock Generation Unit(CGU) is a new clock controller IP of a forthcoming

(...)

>> +               raw_spin_unlock_irqrestore(&ctx->lock, flags);
>> +       }
>> +
>> +       return clk_hw_register_fixed_rate(NULL, list->name,
>> +                                         list->parent_names[0],
>> +                                         list->flags, list->mux_flags);
> Can fixed rate clks come from DT? Or does this value change sometimes?

Fixed rate clks may need enable/disable clk ops. That's the only reason
for making it visible to clock driver.

>> +lgm_clk_ddiv_set_rate(struct clk_hw *hw, unsigned long rate,
>> +                     unsigned long prate)
>> +{
>> +       struct lgm_clk_ddiv *ddiv = to_lgm_clk_ddiv(hw);
>> +       u32 div, ddiv1, ddiv2;
>> +       unsigned long flags;
>> +
>> +       div = DIV_ROUND_CLOSEST_ULL((u64)prate, rate);
>> +
>> +       raw_spin_lock_irqsave(&ddiv->lock, flags);
>> +       if (lgm_get_clk_val(ddiv->membase, ddiv->reg, ddiv->shift2, 1)) {
>> +               div = DIV_ROUND_CLOSEST_ULL((u64)div, 5);
>> +               div = div * 2;
>> +       }
>> +       raw_spin_unlock_irqrestore(&ddiv->lock, flags);
> Does the math need to be inside the spinlock? Should probably not have
> any spinlock at all and just read it with lgm_get_clk_val() and trust
> that the hardware will return us something sane?
>
>> +
>> +       if (div <= 0)
>> +               return -EINVAL;
>> +
>> +       if (lgm_clk_get_ddiv_val(div, &ddiv1, &ddiv2))
>> +               return -EINVAL;
>> +
>> +       raw_spin_lock_irqsave(&ddiv->lock, flags);
>> +       lgm_set_clk_val(ddiv->membase, ddiv->reg, ddiv->shift0, ddiv->width0,
>> +                       ddiv1 - 1);
>> +
>> +       lgm_set_clk_val(ddiv->membase, ddiv->reg,  ddiv->shift1, ddiv->width1,
>> +                       ddiv2 - 1);
> Can this not be combined? lgm_set_clk_val is sort of obfuscating the
> code. Please consider inlining it and then holding the spinlock across
> this entire function while doing the read/modify/write.

These two set_clk_val's can not be combined because they have different
non-contiguous bitmaps (shifts) and lgm_set_clk_val() is defined to handle
only one shift. However, i have addressed your other comment i.e. inline it
and hold spinlock across the function during read/modify/write.

>> +struct lgm_clk_mux {
>> +       struct clk_hw hw;
>> +       struct device *dev;
>> +       void __iomem *membase;
>> +       unsigned int reg;
>> +       u8 shift;
>> +       u8 width;
>> +       unsigned long flags;
>> +       raw_spinlock_t lock;
>> +};
>> +
>> +struct lgm_clk_divider {
>> +       struct clk_hw hw;
>> +       struct device *dev;
>> +       void __iomem *membase;
>> +       unsigned int reg;
>> +       u8 shift;
>> +       u8 width;
>> +       u8 shift_gate;
>> +       u8 width_gate;
>> +       unsigned long flags;
>> +       const struct clk_div_table *table;
>> +       raw_spinlock_t lock;
>> +};
>> +
>> +struct lgm_clk_ddiv {
>> +       struct clk_hw hw;
>> +       struct device *dev;
>> +       void __iomem *membase;
>> +       unsigned int reg;
>> +       u8 shift0;
>> +       u8 width0;
>> +       u8 shift1;
>> +       u8 width1;
>> +       u8 shift2;
>> +       u8 width2;
>> +       u8 shift_gate;
>> +       u8 width_gate;
>> +       unsigned int mult;
>> +       unsigned int div;
>> +       unsigned long flags;
>> +       raw_spinlock_t lock;
>> +};
>> +
>> +struct lgm_clk_gate {
>> +       struct clk_hw hw;
>> +       struct device *dev;
> These all have dev pointers, is it necessary? In theory we can have a
> clk_hw API that gets the dev pointer out for you if you want it, now
> that we store the dev pointer when a clk registers

We needed dev pointers just for dev_err() in clk_ops when they are called
back from core. I have now removed dev_err() calls from the driver clk_ops
as i believe the error info was not that useful. And so i have removed dev
pointers from above structures.

However, i think it would be good to have a clk_hw API which returns dev
pointer for drivers. Presently, dev pointer is stored inside clk_hw->clk_core
and clk_core is private to core, inaccessible from clk drivers.

>> +enum pll_type {
>> +       TYPE_ROPLL,
>> +       TYPE_LJPLL,
>> +       TYPE_NONE,
> Is this used? Remove it if not.

It is actually used.

>> +struct lgm_pll_clk_data {
>> +       unsigned int id;
>> +       const char *name;
>> +       const char *const *parent_names;
> Can you use the new way of specifying clk parents instead of using plain
> strings? Reminder to self, write that document.

I have changed it to new way i.e. by using fw_name. Only exception is where we
use clk API's clk_hw_register_fixed_rate() & clk_hw_register_fixed_factor().
In these API's, i am, for now, passing parent_data[0].name as parent_name.

>> +/* clock flags definition */
>> +#define CLOCK_FLAG_VAL_INIT    BIT(16)
>> +#define GATE_CLK_HW            BIT(17)
>> +#define GATE_CLK_SW            BIT(18)
>> +#define MUX_CLK_SW             GATE_CLK_SW
> Why do these bits start at 16? 

To avoid the conflict with clk_flags used across common struct clk
defined in clk-provider.h. Bits 0~13 are already used there. So
we keep some gap and start with 16 :). We don't maintain a separate
pure driver only flags bitmask as that would make it confusing.

> What does _HW and _SW mean? Is there
> hardware control of clks?

GATE_CLK_HW & GATE_CLK_SW is no longer needed for this SoC. So i have
removed it. MUX_CLK_SW is still needed. It is more of a workaround for
one particular combophy module for which get_parent/set_parent does not
apply (no hardware translation to select/query input clk source).
However, other MUX clks in this clk group have valid hardware translation
for parent clock sources.

>> +static const struct clk_div_table dcl_div[] = {
>> +       { .val = 0, .div = 6  },
>> +       { .val = 1, .div = 12 },
>> +       { .val = 2, .div = 24 },
>> +       { .val = 3, .div = 32 },
>> +       { .val = 4, .div = 48 },
>> +       { .val = 5, .div = 96 },
>> +       {}
> I guess 'div' being equal to 0 is the terminator?

Yes, but i am missing your point. Can you please elaborate more ?

>> +       ctx->np = np;
>> +       ctx->dev = dev;
>> +       raw_spin_lock_init(&ctx->lock);
> Why is it a raw spinlock?

Agree. We use CONFIG_SMP with no PREEMPT_RT. So i think it is same.
Have switched to normal spinlocks.

Regards,
Rahul