| Message ID | 1475933892-16021-1-git-send-email-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Oct 08, 2016 at 09:38:12PM +0800, Shawn Guo wrote: > The hi6220-sysctrl and hi6220-mediactrl are not only clock provider but > also reset controller. It worked fine that single sysctrl/mediactrl > device node in DT can be used to initialize clock driver and populate > platform device for reset controller. But it stops working after > commit 989eafd0b609 ("clk: core: Avoid double initialization of clocks") > gets merged. The commit sets flag OF_POPULATED during clock > initialization to skip the platform device populating for the same > device node. On hi6220, it effectively makes hi6220-sysctrl reset > driver not probe any more. > > The patch changes hi6220 sysctrl and mediactrl clock init macro from > CLK_OF_DECLARE to CLK_OF_DECLARE_DRIVER, so that the reset driver using > the same hardware block can continue working. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > --- > It fixes an issue that is seen on linux-next, i.e. the new added > hi6220-sysctrl reset driver doesn't probe at all, and consequently the > mmc driver fails to register. Correction: the hi6220-sysctrl has been there for a while, and the issue is discovered by mmc driver which adds reset support recently. So technically, this is a regression fix. Shawn
On Sat, Oct 8, 2016 at 6:38 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > The hi6220-sysctrl and hi6220-mediactrl are not only clock provider but > also reset controller. It worked fine that single sysctrl/mediactrl > device node in DT can be used to initialize clock driver and populate > platform device for reset controller. But it stops working after > commit 989eafd0b609 ("clk: core: Avoid double initialization of clocks") > gets merged. The commit sets flag OF_POPULATED during clock > initialization to skip the platform device populating for the same > device node. On hi6220, it effectively makes hi6220-sysctrl reset > driver not probe any more. > > The patch changes hi6220 sysctrl and mediactrl clock init macro from > CLK_OF_DECLARE to CLK_OF_DECLARE_DRIVER, so that the reset driver using > the same hardware block can continue working. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> Tested-by: John Stultz <john.stultz@linaro.org> I hit this as well last week when 989eafd0b609 ("clk: core: Avoid double initialization of clocks") landed, which killed graphics on my HiKey. My workaround was a bit hackish, as I don't really know when one should use OF_DECLARE vs OF_DECLARE_DRIVER, but I also converted the hi6220_clk_ao and hi6220_clk_power to the _DRIVER side. Your patch seems to work just as well for me, but I wanted to double check with you that the ao/power clks didn't need the conversion as well. thanks -john
Hi John, On Mon, Oct 10, 2016 at 10:39:12AM -0700, John Stultz wrote: > On Sat, Oct 8, 2016 at 6:38 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > > The hi6220-sysctrl and hi6220-mediactrl are not only clock provider but > > also reset controller. It worked fine that single sysctrl/mediactrl > > device node in DT can be used to initialize clock driver and populate > > platform device for reset controller. But it stops working after > > commit 989eafd0b609 ("clk: core: Avoid double initialization of clocks") > > gets merged. The commit sets flag OF_POPULATED during clock > > initialization to skip the platform device populating for the same > > device node. On hi6220, it effectively makes hi6220-sysctrl reset > > driver not probe any more. > > > > The patch changes hi6220 sysctrl and mediactrl clock init macro from > > CLK_OF_DECLARE to CLK_OF_DECLARE_DRIVER, so that the reset driver using > > the same hardware block can continue working. > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > Tested-by: John Stultz <john.stultz@linaro.org> > > I hit this as well last week when 989eafd0b609 ("clk: core: Avoid > double initialization of clocks") landed, which killed graphics on my > HiKey. > > My workaround was a bit hackish, as I don't really know when one > should use OF_DECLARE vs OF_DECLARE_DRIVER, but I also converted the > hi6220_clk_ao and hi6220_clk_power to the _DRIVER side. Your patch > seems to work just as well for me, but I wanted to double check with > you that the ao/power clks didn't need the conversion as well. For now, clock driver is the only one matching compatible "hisilicon,hi6220-aoctrl" and "hisilicon,hi6220-pmctrl". Whoever adding a platform driver probing the same compatible later will have to change it CLK_OF_DECLARE_DRIVER. Otherwise, the platform driver simply doesn't probe. Shawn
On 10/08, Shawn Guo wrote: > The hi6220-sysctrl and hi6220-mediactrl are not only clock provider but > also reset controller. It worked fine that single sysctrl/mediactrl > device node in DT can be used to initialize clock driver and populate > platform device for reset controller. But it stops working after > commit 989eafd0b609 ("clk: core: Avoid double initialization of clocks") > gets merged. The commit sets flag OF_POPULATED during clock > initialization to skip the platform device populating for the same > device node. On hi6220, it effectively makes hi6220-sysctrl reset > driver not probe any more. > > The patch changes hi6220 sysctrl and mediactrl clock init macro from > CLK_OF_DECLARE to CLK_OF_DECLARE_DRIVER, so that the reset driver using > the same hardware block can continue working. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > --- Applied to clk-fixes
diff --git a/drivers/clk/hisilicon/clk-hi6220.c b/drivers/clk/hisilicon/clk-hi6220.c index fe364e63f8de..c0e8e1f196aa 100644 --- a/drivers/clk/hisilicon/clk-hi6220.c +++ b/drivers/clk/hisilicon/clk-hi6220.c @@ -195,7 +195,7 @@ static void __init hi6220_clk_sys_init(struct device_node *np) hi6220_clk_register_divider(hi6220_div_clks_sys, ARRAY_SIZE(hi6220_div_clks_sys), clk_data); } -CLK_OF_DECLARE(hi6220_clk_sys, "hisilicon,hi6220-sysctrl", hi6220_clk_sys_init); +CLK_OF_DECLARE_DRIVER(hi6220_clk_sys, "hisilicon,hi6220-sysctrl", hi6220_clk_sys_init); /* clocks in media controller */ @@ -252,7 +252,7 @@ static void __init hi6220_clk_media_init(struct device_node *np) hi6220_clk_register_divider(hi6220_div_clks_media, ARRAY_SIZE(hi6220_div_clks_media), clk_data); } -CLK_OF_DECLARE(hi6220_clk_media, "hisilicon,hi6220-mediactrl", hi6220_clk_media_init); +CLK_OF_DECLARE_DRIVER(hi6220_clk_media, "hisilicon,hi6220-mediactrl", hi6220_clk_media_init); /* clocks in pmctrl */
The hi6220-sysctrl and hi6220-mediactrl are not only clock provider but also reset controller. It worked fine that single sysctrl/mediactrl device node in DT can be used to initialize clock driver and populate platform device for reset controller. But it stops working after commit 989eafd0b609 ("clk: core: Avoid double initialization of clocks") gets merged. The commit sets flag OF_POPULATED during clock initialization to skip the platform device populating for the same device node. On hi6220, it effectively makes hi6220-sysctrl reset driver not probe any more. The patch changes hi6220 sysctrl and mediactrl clock init macro from CLK_OF_DECLARE to CLK_OF_DECLARE_DRIVER, so that the reset driver using the same hardware block can continue working. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- It fixes an issue that is seen on linux-next, i.e. the new added hi6220-sysctrl reset driver doesn't probe at all, and consequently the mmc driver fails to register. Shawn drivers/clk/hisilicon/clk-hi6220.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)