Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1312438 > unrolled thread

Re: [PATCH v5 1/6] clk: hisilicon: add CRG driver for hi3519 soc

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 1/6] clk: hisilicon: add CRG driver for hi3519 soc Rob Herring <robh+dt@kernel.org> - 2016-01-19 19:30 +0100
    Re: [PATCH v5 1/6] clk: hisilicon: add CRG driver for hi3519 soc Tomeu Vizoso <tomeu.vizoso@collabora.com> - 2016-01-20 07:40 +0100
      Re: [PATCH v5 1/6] clk: hisilicon: add CRG driver for hi3519 soc xuejiancheng <xuejiancheng@huawei.com> - 2016-01-22 10:00 +0100
        Re: [PATCH v5 1/6] clk: hisilicon: add CRG driver for hi3519 soc Tomeu Vizoso <tomeu.vizoso@collabora.com> - 2016-01-22 11:00 +0100

#1312438 — Re: [PATCH v5 1/6] clk: hisilicon: add CRG driver for hi3519 soc

On Fri, Jan 15, 2016 at 1:57 AM, xuejiancheng <xuejiancheng@huawei.com> wrote:
>
> On 2016/1/14 21:16, xuejiancheng wrote:
>> Hi Mike,
>>
>> On 2016/1/14 2:57, Michael Turquette wrote:
>>> Quoting xuejiancheng (2016-01-12 19:03:01)
>>>> Hi Stephen,
>>>>    Thank you very much for your reply.
>>>>
>>>> On 2016/1/13 6:12, Stephen Boyd wrote:
>>>>> On 01/08, Jiancheng Xue wrote:
>>>>>> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
>>>>>> index e434854..b6baebf 100644
>>>>>> --- a/drivers/clk/hisilicon/Kconfig
>>>>>> +++ b/drivers/clk/hisilicon/Kconfig
>>>>>> @@ -1,3 +1,10 @@
>>>>>> +config COMMON_CLK_HI3519
>>>>>> +    tristate "Clock Driver for Hi3519"
>>>>>
>>>>> It looks like this has to be bool. Otherwise it needs to be a
>>>>> platform driver and the hisilicon APIs need to be exported and
>>>>> lose their __init markings.
>>>>>
>>>> Yes,it's a problem. I will fix it in next version. Thank you.
>>>
>>> The best solution would be to make this clock driver a real platform
>>> driver.
>>>
>> Now the work clock of the clocksource timer-sp804 is provided by this driver. So
>> it need to be registered early by CLK_OF_DECLARE. If the timer clock is treated
>> as a fixed-clock provider, this driver can be implemented as a platform driver.
>> Then the crg device must be registered before other clock consumer devices.Accordingly
>> the crg device node must be written above all other clock consumer devices node in dts files.
>> I think it is also a dependence.
>>
>> Can you help me understand why it is better to make this driver a platform driver?
>> Thank you very much!
>>
> arch_initcall(customize_machine)
> -->of_platform_populate
>    -->of_platform_bus_create
>      -->of_amba_device_create
>         -->amba_device_add
>            -->amba_get_enable_pclk
> The call sequence above shows that the clock of the amba device must be registered before
> amba_device_add. The clock of "arm,pl011" uart is registered in the probe function of the
> platform driver "hi3519-crg". So the platform device "hi3519-crg" must be created before
> the amba device "arm,pl011" uart.

It is a problem, but Tomeu had a fix to support deferred probes here.
That was part of the on-demand probing series, but maybe it needs to
be applied separately if we are moving clock drivers to platform
drivers.

Rob

[toc] | [next] | [standalone]

#1312916

On 19 January 2016 at 19:20, Rob Herring <robh+dt@kernel.org> wrote:
> On Fri, Jan 15, 2016 at 1:57 AM, xuejiancheng <xuejiancheng@huawei.com> wrote:
>>
>> On 2016/1/14 21:16, xuejiancheng wrote:
>>> Hi Mike,
>>>
>>> On 2016/1/14 2:57, Michael Turquette wrote:
>>>> Quoting xuejiancheng (2016-01-12 19:03:01)
>>>>> Hi Stephen,
>>>>>    Thank you very much for your reply.
>>>>>
>>>>> On 2016/1/13 6:12, Stephen Boyd wrote:
>>>>>> On 01/08, Jiancheng Xue wrote:
>>>>>>> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
>>>>>>> index e434854..b6baebf 100644
>>>>>>> --- a/drivers/clk/hisilicon/Kconfig
>>>>>>> +++ b/drivers/clk/hisilicon/Kconfig
>>>>>>> @@ -1,3 +1,10 @@
>>>>>>> +config COMMON_CLK_HI3519
>>>>>>> +    tristate "Clock Driver for Hi3519"
>>>>>>
>>>>>> It looks like this has to be bool. Otherwise it needs to be a
>>>>>> platform driver and the hisilicon APIs need to be exported and
>>>>>> lose their __init markings.
>>>>>>
>>>>> Yes,it's a problem. I will fix it in next version. Thank you.
>>>>
>>>> The best solution would be to make this clock driver a real platform
>>>> driver.
>>>>
>>> Now the work clock of the clocksource timer-sp804 is provided by this driver. So
>>> it need to be registered early by CLK_OF_DECLARE. If the timer clock is treated
>>> as a fixed-clock provider, this driver can be implemented as a platform driver.
>>> Then the crg device must be registered before other clock consumer devices.Accordingly
>>> the crg device node must be written above all other clock consumer devices node in dts files.
>>> I think it is also a dependence.
>>>
>>> Can you help me understand why it is better to make this driver a platform driver?
>>> Thank you very much!
>>>
>> arch_initcall(customize_machine)
>> -->of_platform_populate
>>    -->of_platform_bus_create
>>      -->of_amba_device_create
>>         -->amba_device_add
>>            -->amba_get_enable_pclk
>> The call sequence above shows that the clock of the amba device must be registered before
>> amba_device_add. The clock of "arm,pl011" uart is registered in the probe function of the
>> platform driver "hi3519-crg". So the platform device "hi3519-crg" must be created before
>> the amba device "arm,pl011" uart.
>
> It is a problem, but Tomeu had a fix to support deferred probes here.
> That was part of the on-demand probing series, but maybe it needs to
> be applied separately if we are moving clock drivers to platform
> drivers.

Hi,

Marek Szyprowski has kindly taken those two patches as part of a series of him:

http://lkml.kernel.org/g/1450868368-5650-1-git-send-email-m.szyprowski@samsung.com

I think it would be great if you could test them and report.

Thanks,

Tomeu

[toc] | [prev] | [next] | [standalone]

#1314830

On 2016/1/20 14:38, Tomeu Vizoso wrote:
> On 19 January 2016 at 19:20, Rob Herring <robh+dt@kernel.org> wrote:
>> On Fri, Jan 15, 2016 at 1:57 AM, xuejiancheng <xuejiancheng@huawei.com> wrote:
>>>
>>> On 2016/1/14 21:16, xuejiancheng wrote:
>>>> Hi Mike,
>>>>
>>>> On 2016/1/14 2:57, Michael Turquette wrote:
>>>>> Quoting xuejiancheng (2016-01-12 19:03:01)
>>>>>> Hi Stephen,
>>>>>>    Thank you very much for your reply.
>>>>>>
>>>>>> On 2016/1/13 6:12, Stephen Boyd wrote:
>>>>>>> On 01/08, Jiancheng Xue wrote:
>>>>>>>> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
>>>>>>>> index e434854..b6baebf 100644
>>>>>>>> --- a/drivers/clk/hisilicon/Kconfig
>>>>>>>> +++ b/drivers/clk/hisilicon/Kconfig
>>>>>>>> @@ -1,3 +1,10 @@
>>>>>>>> +config COMMON_CLK_HI3519
>>>>>>>> +    tristate "Clock Driver for Hi3519"
>>>>>>>
>>>>>>> It looks like this has to be bool. Otherwise it needs to be a
>>>>>>> platform driver and the hisilicon APIs need to be exported and
>>>>>>> lose their __init markings.
>>>>>>>
>>>>>> Yes,it's a problem. I will fix it in next version. Thank you.
>>>>>
>>>>> The best solution would be to make this clock driver a real platform
>>>>> driver.
>>>>>
>>>> Now the work clock of the clocksource timer-sp804 is provided by this driver. So
>>>> it need to be registered early by CLK_OF_DECLARE. If the timer clock is treated
>>>> as a fixed-clock provider, this driver can be implemented as a platform driver.
>>>> Then the crg device must be registered before other clock consumer devices.Accordingly
>>>> the crg device node must be written above all other clock consumer devices node in dts files.
>>>> I think it is also a dependence.
>>>>
>>>> Can you help me understand why it is better to make this driver a platform driver?
>>>> Thank you very much!
>>>>
>>> arch_initcall(customize_machine)
>>> -->of_platform_populate
>>>    -->of_platform_bus_create
>>>      -->of_amba_device_create
>>>         -->amba_device_add
>>>            -->amba_get_enable_pclk
>>> The call sequence above shows that the clock of the amba device must be registered before
>>> amba_device_add. The clock of "arm,pl011" uart is registered in the probe function of the
>>> platform driver "hi3519-crg". So the platform device "hi3519-crg" must be created before
>>> the amba device "arm,pl011" uart.
>>
>> It is a problem, but Tomeu had a fix to support deferred probes here.
>> That was part of the on-demand probing series, but maybe it needs to
>> be applied separately if we are moving clock drivers to platform
>> drivers.
> 
> Hi,
> 
> Marek Szyprowski has kindly taken those two patches as part of a series of him:
> 
> http://lkml.kernel.org/g/1450868368-5650-1-git-send-email-m.szyprowski@samsung.com
> 
> I think it would be great if you could test them and report.
> 
Hi Tomeu,

I have applied the patch "https://lkml.org/lkml/2015/12/23/105" and tested on my hi3519-demb board.
It works even if the apb_pclk is registered later than the amba-pl011 device being registered.

But I think it is a problem if amba_read_periphid() returns -ENOMEM or -ENODEV when apb_pclk is available.
In this condition，amba_match() returns a non zero value which means the driver and device matches
and the amba_probe() will be called, but amba_device->periphid remains as 0. Then amba_lookup() called in
amba_probe() will return a null id pointer.The null pointer will be passed to amba_driver->probe() and
this may cause a segment fault.

Regards,

Jiancheng

> Thanks,
> 
> Tomeu
> 
> .
> 

[toc] | [prev] | [next] | [standalone]

#1314861

On 22 January 2016 at 09:50, xuejiancheng <xuejiancheng@huawei.com> wrote:
> On 2016/1/20 14:38, Tomeu Vizoso wrote:
>> On 19 January 2016 at 19:20, Rob Herring <robh+dt@kernel.org> wrote:
>>> On Fri, Jan 15, 2016 at 1:57 AM, xuejiancheng <xuejiancheng@huawei.com> wrote:
>>>>
>>>> On 2016/1/14 21:16, xuejiancheng wrote:
>>>>> Hi Mike,
>>>>>
>>>>> On 2016/1/14 2:57, Michael Turquette wrote:
>>>>>> Quoting xuejiancheng (2016-01-12 19:03:01)
>>>>>>> Hi Stephen,
>>>>>>>    Thank you very much for your reply.
>>>>>>>
>>>>>>> On 2016/1/13 6:12, Stephen Boyd wrote:
>>>>>>>> On 01/08, Jiancheng Xue wrote:
>>>>>>>>> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
>>>>>>>>> index e434854..b6baebf 100644
>>>>>>>>> --- a/drivers/clk/hisilicon/Kconfig
>>>>>>>>> +++ b/drivers/clk/hisilicon/Kconfig
>>>>>>>>> @@ -1,3 +1,10 @@
>>>>>>>>> +config COMMON_CLK_HI3519
>>>>>>>>> +    tristate "Clock Driver for Hi3519"
>>>>>>>>
>>>>>>>> It looks like this has to be bool. Otherwise it needs to be a
>>>>>>>> platform driver and the hisilicon APIs need to be exported and
>>>>>>>> lose their __init markings.
>>>>>>>>
>>>>>>> Yes,it's a problem. I will fix it in next version. Thank you.
>>>>>>
>>>>>> The best solution would be to make this clock driver a real platform
>>>>>> driver.
>>>>>>
>>>>> Now the work clock of the clocksource timer-sp804 is provided by this driver. So
>>>>> it need to be registered early by CLK_OF_DECLARE. If the timer clock is treated
>>>>> as a fixed-clock provider, this driver can be implemented as a platform driver.
>>>>> Then the crg device must be registered before other clock consumer devices.Accordingly
>>>>> the crg device node must be written above all other clock consumer devices node in dts files.
>>>>> I think it is also a dependence.
>>>>>
>>>>> Can you help me understand why it is better to make this driver a platform driver?
>>>>> Thank you very much!
>>>>>
>>>> arch_initcall(customize_machine)
>>>> -->of_platform_populate
>>>>    -->of_platform_bus_create
>>>>      -->of_amba_device_create
>>>>         -->amba_device_add
>>>>            -->amba_get_enable_pclk
>>>> The call sequence above shows that the clock of the amba device must be registered before
>>>> amba_device_add. The clock of "arm,pl011" uart is registered in the probe function of the
>>>> platform driver "hi3519-crg". So the platform device "hi3519-crg" must be created before
>>>> the amba device "arm,pl011" uart.
>>>
>>> It is a problem, but Tomeu had a fix to support deferred probes here.
>>> That was part of the on-demand probing series, but maybe it needs to
>>> be applied separately if we are moving clock drivers to platform
>>> drivers.
>>
>> Hi,
>>
>> Marek Szyprowski has kindly taken those two patches as part of a series of him:
>>
>> http://lkml.kernel.org/g/1450868368-5650-1-git-send-email-m.szyprowski@samsung.com
>>
>> I think it would be great if you could test them and report.
>>
> Hi Tomeu,
>
> I have applied the patch "https://lkml.org/lkml/2015/12/23/105" and tested on my hi3519-demb board.
> It works even if the apb_pclk is registered later than the amba-pl011 device being registered.
>
> But I think it is a problem if amba_read_periphid() returns -ENOMEM or -ENODEV when apb_pclk is available.
> In this condition，amba_match() returns a non zero value which means the driver and device matches
> and the amba_probe() will be called, but amba_device->periphid remains as 0. Then amba_lookup() called in
> amba_probe() will return a null id pointer.The null pointer will be passed to amba_driver->probe() and
> this may cause a segment fault.

But, have you applied the other patches in the series? There's one
that should handle the other error codes.

In any case, any feedback you have on that series should be given in
the other thread.

Regards,

Tomeu

> Regards,
>
> Jiancheng
>
>> Thanks,
>>
>> Tomeu
>>
>> .
>>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web