From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752841AbcAVJ4N (ORCPT ); Fri, 22 Jan 2016 04:56:13 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:36210 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752599AbcAVJ4G convert rfc822-to-8bit (ORCPT ); Fri, 22 Jan 2016 04:56:06 -0500 MIME-Version: 1.0 In-Reply-To: <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> <56A1ED53.60309@huawei.com> From: Tomeu Vizoso Date: Fri, 22 Jan 2016 10:55:45 +0100 X-Google-Sender-Auth: -b_LJov-GisDW5vAhzEt38j8V90 Message-ID: Subject: Re: [PATCH v5 1/6] clk: hisilicon: add CRG driver for hi3519 soc To: xuejiancheng Cc: Rob Herring , 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, suwenping@hisilicon.com, ml.yang@hisilicon.com, gaofei@hisilicon.com, zhangzhenxing@hisilicon.com, xuejiancheng@hisilicon.com, Marek Szyprowski Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 22 January 2016 at 09:50, xuejiancheng wrote: > 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. 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 >> >> . >> > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomeu Vizoso Subject: Re: [PATCH v5 1/6] clk: hisilicon: add CRG driver for hi3519 soc Date: Fri, 22 Jan 2016 10:55:45 +0100 Message-ID: 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> <56A1ED53.60309@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <56A1ED53.60309@huawei.com> Sender: linux-clk-owner@vger.kernel.org To: xuejiancheng Cc: Rob Herring , 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 List-Id: devicetree@vger.kernel.org On 22 January 2016 at 09:50, xuejiancheng wro= te: > 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/hisi= licon/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 plat= form >>>>>> 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 cl= ock is treated >>>>> as a fixed-clock provider, this driver can be implemented as a pl= atform driver. >>>>> Then the crg device must be registered before other clock consume= r devices.Accordingly >>>>> the crg device node must be written above all other clock consume= r 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 mu= st be registered before >>>> amba_device_add. The clock of "arm,pl011" uart is registered in th= e 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 her= e. >>> That was part of the on-demand probing series, but maybe it needs t= o >>> 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 ser= ies of him: >> >> http://lkml.kernel.org/g/1450868368-5650-1-git-send-email-m.szyprows= ki@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 t= ested 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 o= r -ENODEV when apb_pclk is available. > In this condition=EF=BC=8Camba_match() returns a non zero value which= means the driver and device matches > and the amba_probe() will be called, but amba_device->periphid remain= s as 0. Then amba_lookup() called in > amba_probe() will return a null id pointer.The null pointer will be p= assed 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 >> >> . >> > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 Sender: tomeu.vizoso@gmail.com In-Reply-To: <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> <56A1ED53.60309@huawei.com> From: Tomeu Vizoso Date: Fri, 22 Jan 2016 10:55:45 +0100 Message-ID: Subject: Re: [PATCH v5 1/6] clk: hisilicon: add CRG driver for hi3519 soc To: xuejiancheng Cc: Rob Herring , 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, suwenping@hisilicon.com, ml.yang@hisilicon.com, gaofei@hisilicon.com, zhangzhenxing@hisilicon.com, xuejiancheng@hisilicon.com, Marek Szyprowski Content-Type: text/plain; charset=UTF-8 List-ID: On 22 January 2016 at 09:50, xuejiancheng wrote: > 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/hisilico= n/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 platfo= rm driver. >>>>> Then the crg device must be registered before other clock consumer de= vices.Accordingly >>>>> the crg device node must be written above all other clock consumer de= vices 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 pla= tform 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 b= e registered before >>>> amba_device_add. The clock of "arm,pl011" uart is registered in the pr= obe 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@s= amsung.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 teste= d on my hi3519-demb board. > It works even if the apb_pclk is registered later than the amba-pl011 dev= ice being registered. > > But I think it is a problem if amba_read_periphid() returns -ENOMEM or -E= NODEV when apb_pclk is available. > In this condition=EF=BC=8Camba_match() returns a non zero value which mea= ns 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 passe= d 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 >> >> . >> > From mboxrd@z Thu Jan 1 00:00:00 1970 From: tomeu.vizoso@collabora.com (Tomeu Vizoso) Date: Fri, 22 Jan 2016 10:55:45 +0100 Subject: [PATCH v5 1/6] clk: hisilicon: add CRG driver for hi3519 soc In-Reply-To: <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> <56A1ED53.60309@huawei.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 22 January 2016 at 09:50, xuejiancheng wrote: > 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. 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 >> >> . >> >