From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752831AbcAVI6q (ORCPT ); Fri, 22 Jan 2016 03:58:46 -0500 Received: from szxga03-in.huawei.com ([119.145.14.66]:58354 "EHLO szxga03-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751913AbcAVI6a (ORCPT ); Fri, 22 Jan 2016 03:58:30 -0500 Subject: Re: [PATCH v5 1/6] clk: hisilicon: add CRG driver for hi3519 soc To: Tomeu Vizoso , Rob Herring References: <1452219400-32478-1-git-send-email-xuejiancheng@huawei.com> <1452219400-32478-2-git-send-email-xuejiancheng@huawei.com> <20160112221211.GB22188@codeaurora.org> <5695BE65.3070409@huawei.com> <20160113185707.1168.85601@quark.deferred.io> <56979F9F.5080201@huawei.com> <5698A650.5010600@huawei.com> CC: Michael Turquette , Stephen Boyd , Philipp Zabel , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , "Russell King - ARM Linux" , Kevin Hilman , Arnd Bergmann , Olof Johansson , Wei Xu , Haojian Zhuang , Zhangfei Gao , Bintian Wang , "linux-kernel@vger.kernel.org" , linux-clk , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , , , , , , , From: xuejiancheng Message-ID: <56A1ED53.60309@huawei.com> Date: Fri, 22 Jan 2016 16:50:27 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.217.211] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020201.56A1ED67.0013,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-05-26 15:14:31, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: abe398ef104839baeaa11c5a8990fbe2 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2016/1/20 14:38, Tomeu Vizoso wrote: > On 19 January 2016 at 19:20, Rob Herring wrote: >> On Fri, Jan 15, 2016 at 1:57 AM, xuejiancheng 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 > > . > From mboxrd@z Thu Jan 1 00:00:00 1970 From: xuejiancheng Subject: Re: [PATCH v5 1/6] clk: hisilicon: add CRG driver for hi3519 soc Date: Fri, 22 Jan 2016 16:50:27 +0800 Message-ID: <56A1ED53.60309@huawei.com> References: <1452219400-32478-1-git-send-email-xuejiancheng@huawei.com> <1452219400-32478-2-git-send-email-xuejiancheng@huawei.com> <20160112221211.GB22188@codeaurora.org> <5695BE65.3070409@huawei.com> <20160113185707.1168.85601@quark.deferred.io> <56979F9F.5080201@huawei.com> <5698A650.5010600@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: Sender: linux-clk-owner@vger.kernel.org To: Tomeu Vizoso , Rob Herring Cc: Michael Turquette , Stephen Boyd , Philipp Zabel , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Russell King - ARM Linux , Kevin Hilman , Arnd Bergmann , Olof Johansson , Wei Xu , Haojian Zhuang , Zhangfei Gao , Bintian Wang , "linux-kernel@vger.kernel.org" , linux-clk , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , yanhaifeng@hisilicon.com, yanghongwei@hisilicon.com List-Id: devicetree@vger.kernel.org On 2016/1/20 14:38, Tomeu Vizoso wrote: > On 19 January 2016 at 19:20, Rob Herring wrote: >> On Fri, Jan 15, 2016 at 1:57 AM, xuejiancheng 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/hisil= icon/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 platf= orm >>>>> driver. >>>>> >>>> Now the work clock of the clocksource timer-sp804 is provided by t= his driver. So >>>> it need to be registered early by CLK_OF_DECLARE. If the timer clo= ck is treated >>>> as a fixed-clock provider, this driver can be implemented as a pla= tform 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 mus= t 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" m= ust be created before >>> the amba device "arm,pl011" uart. >> >> It is a problem, but Tomeu had a fix to support deferred probes here= =2E >> 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. >=20 > Hi, >=20 > Marek Szyprowski has kindly taken those two patches as part of a seri= es of him: >=20 > http://lkml.kernel.org/g/1450868368-5650-1-git-send-email-m.szyprowsk= i@samsung.com >=20 > I think it would be great if you could test them and report. >=20 Hi Tomeu, I have applied the patch "https://lkml.org/lkml/2015/12/23/105" and tes= ted on my hi3519-demb board. It works even if the apb_pclk is registered later than the amba-pl011 d= evice 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=EF=BC=8Camba_match() returns a non zero value which m= eans 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 pas= sed to amba_driver->probe() and this may cause a segment fault. Regards, Jiancheng > Thanks, >=20 > Tomeu >=20 > . >=20 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [PATCH v5 1/6] clk: hisilicon: add CRG driver for hi3519 soc To: Tomeu Vizoso , Rob Herring References: <1452219400-32478-1-git-send-email-xuejiancheng@huawei.com> <1452219400-32478-2-git-send-email-xuejiancheng@huawei.com> <20160112221211.GB22188@codeaurora.org> <5695BE65.3070409@huawei.com> <20160113185707.1168.85601@quark.deferred.io> <56979F9F.5080201@huawei.com> <5698A650.5010600@huawei.com> CC: Michael Turquette , Stephen Boyd , Philipp Zabel , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , "Russell King - ARM Linux" , Kevin Hilman , Arnd Bergmann , Olof Johansson , Wei Xu , Haojian Zhuang , Zhangfei Gao , Bintian Wang , "linux-kernel@vger.kernel.org" , linux-clk , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , , , , , , , From: xuejiancheng Message-ID: <56A1ED53.60309@huawei.com> Date: Fri, 22 Jan 2016 16:50:27 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" List-ID: On 2016/1/20 14:38, Tomeu Vizoso wrote: > On 19 January 2016 at 19:20, Rob Herring wrote: >> On Fri, Jan 15, 2016 at 1:57 AM, xuejiancheng 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 > > . > From mboxrd@z Thu Jan 1 00:00:00 1970 From: xuejiancheng@huawei.com (xuejiancheng) Date: Fri, 22 Jan 2016 16:50:27 +0800 Subject: [PATCH v5 1/6] clk: hisilicon: add CRG driver for hi3519 soc In-Reply-To: References: <1452219400-32478-1-git-send-email-xuejiancheng@huawei.com> <1452219400-32478-2-git-send-email-xuejiancheng@huawei.com> <20160112221211.GB22188@codeaurora.org> <5695BE65.3070409@huawei.com> <20160113185707.1168.85601@quark.deferred.io> <56979F9F.5080201@huawei.com> <5698A650.5010600@huawei.com> Message-ID: <56A1ED53.60309@huawei.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 2016/1/20 14:38, Tomeu Vizoso wrote: > On 19 January 2016 at 19:20, Rob Herring wrote: >> On Fri, Jan 15, 2016 at 1:57 AM, xuejiancheng 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 at 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 > > . >