Hi Prashant,
On Thursday 28 of February 2013 11:20:31 Stephen Warren wrote:
> On 02/28/2013 12:58 AM, Prashant Gaikwad wrote:
> > On Wednesday 06 February 2013 03:36 PM, Tomasz Figa wrote:
> >> On Wednesday 06 of February 2013 08:34:32 Prashant Gaikwad wrote:
> >>> On Tuesday 05 February 2013 03:45 PM, Tomasz Figa wrote:
> >>>> Hi Prashant,
> >>>>
> >>>> Thank you for your patch. Please see some comments inline.
> >>>>
> >>>> On Monday 04 of February 2013 13:41:22 Prashant Gaikwad wrote:
> >>>>> Not all clocks are required to be decomposed into basic clock
> >>>>> types but at the same time want to use the functionality
> >>>>> provided by these basic clock types instead of duplicating.
> >>>>>
> >>>>> For example, Tegra SoC has ~100 clocks which can be decomposed
> >>>>> into Mux -> Div -> Gate clock types making the clock count to
> >>>>> ~300. Also, parent change operation can not be performed on gate
> >>>>> clock which forces to use mux clock in driver if want to change
> >>>>> the parent.
> >>>>>
> >>>>> Instead aggregate the basic clock types functionality into one
> >>>>> clock and just use this clock for all operations. This clock
> >>>>> type re-uses the functionality of basic clock types and not
> >>>>> limited to basic clock types but any hardware-specific
> >>>>> implementation.
> >>>>>
> >>>>> diff --git a/drivers/clk/clk-composite.c
> >>>>>
> >>>>> +static u8 clk_composite_get_parent(struct clk_hw *hw)
> >>>>> +{
> >>>>> + struct clk_composite *composite = to_clk_composite(hw);
> >>>>> + const struct clk_ops *mux_ops = composite->mux_ops;
> >>>>> + struct clk_hw *mux_hw = composite->mux_hw;
> >>>>> +
> >>>>> + mux_hw->clk = hw->clk;
> >>>>
> >>>> Why is this needed? Looks like this filed is already being initialized
> >>>> in clk_register_composite.
> >>>
> >>> Some ops will get called during clk_init where this clk is not populated
> >>> hence doing here. I have done it for all ops to make sure that any
> >>> future change in clock framework don't break this clock.
> >>> Now, why duplicate it in clk_register_composite? It is possible that
> >>> none of these ops get called in clk_init.
> >>> For example, recalc_rate is called during init and this ops is supported
> >>> by div clock type, but what if div clock is not added.
> >>>
> >>> I hope this explains the need.
> >>
> >> Sorry, I don't understand your explanation.
> >>
> >> I have asked why mux_hw->clk field has to be reinitialized in all the
> >> ops.
> >>
> >> In other words, is it even possible that this clk pointer changes since
> >> calling clk_register from clk_register_composite and if yes, why?
> >
> > Tomasz,
> >
> > calling sequence is as
> >
> > clk_register(struct clk_hw *hw)
> >
> > clk_init(struct clk_hw *hw)
> >
> > .
> > .
> > hw->clk = clk;
> > clk->ops.recalc_rate(hw) => clk_composite_recalc_rate(hw) =>
> >
> > composite->div_ops.recalc_rate(div_hw) => clk_divider_recalc_rate(hw)
> >
> > Now if clk_divider_recalc_rate tries to access clk from hw then it will
> > get NULL since this is not assigned yet and that is what I am doing in
> > clk_composite_recalc_rate.
> >
> > I am doing it in all ops because I can not assume which one will get
> > called first and always. If in future something changes the calling
> > sequence in ccf framework then it will break this clock.
>
> Surely the CCF core should be taking care of this as part of
> clk_register() or clk_init()?
Any news on this? It would be nice if this patch could be merged soon, because
we'd like to rework Exynos clock code to use composite clocks before merge
window, to have that merged for 3.10.
If you don't have time to work on this, would you mind if I made any necessary
fixes, added my sign-off next to yours and posted next version myself?
Best regards,
--
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Kernel and System Framework
On Wednesday 13 March 2013 10:00 PM, Tomasz Figa wrote:
> Hi Prashant,
>
> On Thursday 28 of February 2013 11:20:31 Stephen Warren wrote:
>> On 02/28/2013 12:58 AM, Prashant Gaikwad wrote:
>>> On Wednesday 06 February 2013 03:36 PM, Tomasz Figa wrote:
>>>> On Wednesday 06 of February 2013 08:34:32 Prashant Gaikwad wrote:
>>>>> On Tuesday 05 February 2013 03:45 PM, Tomasz Figa wrote:
>>>>>> Hi Prashant,
>>>>>>
>>>>>> Thank you for your patch. Please see some comments inline.
>>>>>>
>>>>>> On Monday 04 of February 2013 13:41:22 Prashant Gaikwad wrote:
>>>>>>> Not all clocks are required to be decomposed into basic clock
>>>>>>> types but at the same time want to use the functionality
>>>>>>> provided by these basic clock types instead of duplicating.
>>>>>>>
>>>>>>> For example, Tegra SoC has ~100 clocks which can be decomposed
>>>>>>> into Mux -> Div -> Gate clock types making the clock count to
>>>>>>> ~300. Also, parent change operation can not be performed on gate
>>>>>>> clock which forces to use mux clock in driver if want to change
>>>>>>> the parent.
>>>>>>>
>>>>>>> Instead aggregate the basic clock types functionality into one
>>>>>>> clock and just use this clock for all operations. This clock
>>>>>>> type re-uses the functionality of basic clock types and not
>>>>>>> limited to basic clock types but any hardware-specific
>>>>>>> implementation.
>>>>>>>
>>>>>>> diff --git a/drivers/clk/clk-composite.c
>>>>>>>
>>>>>>> +static u8 clk_composite_get_parent(struct clk_hw *hw)
>>>>>>> +{
>>>>>>> + struct clk_composite *composite = to_clk_composite(hw);
>>>>>>> + const struct clk_ops *mux_ops = composite->mux_ops;
>>>>>>> + struct clk_hw *mux_hw = composite->mux_hw;
>>>>>>> +
>>>>>>> + mux_hw->clk = hw->clk;
>>>>>> Why is this needed? Looks like this filed is already being initialized
>>>>>> in clk_register_composite.
>>>>> Some ops will get called during clk_init where this clk is not populated
>>>>> hence doing here. I have done it for all ops to make sure that any
>>>>> future change in clock framework don't break this clock.
>>>>> Now, why duplicate it in clk_register_composite? It is possible that
>>>>> none of these ops get called in clk_init.
>>>>> For example, recalc_rate is called during init and this ops is supported
>>>>> by div clock type, but what if div clock is not added.
>>>>>
>>>>> I hope this explains the need.
>>>> Sorry, I don't understand your explanation.
>>>>
>>>> I have asked why mux_hw->clk field has to be reinitialized in all the
>>>> ops.
>>>>
>>>> In other words, is it even possible that this clk pointer changes since
>>>> calling clk_register from clk_register_composite and if yes, why?
>>> Tomasz,
>>>
>>> calling sequence is as
>>>
>>> clk_register(struct clk_hw *hw)
>>>
>>> clk_init(struct clk_hw *hw)
>>>
>>> .
>>> .
>>> hw->clk = clk;
>>> clk->ops.recalc_rate(hw) => clk_composite_recalc_rate(hw) =>
>>>
>>> composite->div_ops.recalc_rate(div_hw) => clk_divider_recalc_rate(hw)
>>>
>>> Now if clk_divider_recalc_rate tries to access clk from hw then it will
>>> get NULL since this is not assigned yet and that is what I am doing in
>>> clk_composite_recalc_rate.
>>>
>>> I am doing it in all ops because I can not assume which one will get
>>> called first and always. If in future something changes the calling
>>> sequence in ccf framework then it will break this clock.
>> Surely the CCF core should be taking care of this as part of
>> clk_register() or clk_init()?
> Any news on this? It would be nice if this patch could be merged soon, because
> we'd like to rework Exynos clock code to use composite clocks before merge
> window, to have that merged for 3.10.
>
> If you don't have time to work on this, would you mind if I made any necessary
> fixes, added my sign-off next to yours and posted next version myself?
Tomasz,
Sorry for delayed reply. I will send the updated patch in next 2-3 days.
After that if any rework required you can make the changes and add your
sign-off.
Thanks for the patience!!
Regards,
PrashantG
> Best regards,