| Message ID | 20170818175506.5035-3-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[...] > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c > index 71e01cbc38b6..7b47906ba447 100644 > --- a/drivers/mmc/host/sdhci-msm.c > +++ b/drivers/mmc/host/sdhci-msm.c > @@ -131,7 +131,7 @@ struct sdhci_msm_host { > struct clk *pclk; /* SDHC peripheral bus clock */ > struct clk *bus_clk; /* SDHC bus voter clock */ > struct clk *xo_clk; /* TCXO clk needed for FLL feature of cm_dll*/ > - struct clk_bulk_data bulk_clks[2]; > + struct clk_bulk_data bulk_clks[4]; > unsigned long clk_rate; > struct mmc_host *mmc; > bool use_14lpp_dll_reset; > @@ -1125,6 +1125,7 @@ static int sdhci_msm_probe(struct platform_device *pdev) > struct sdhci_pltfm_host *pltfm_host; > struct sdhci_msm_host *msm_host; > struct resource *core_memres; > + struct clk *clk; > int ret; > u16 host_version, core_minor; > u32 core_version, config; > @@ -1194,6 +1195,14 @@ static int sdhci_msm_probe(struct platform_device *pdev) > msm_host->bulk_clks[0].clk = msm_host->clk; > msm_host->bulk_clks[1].clk = msm_host->pclk; > > + clk = devm_clk_get(&pdev->dev, "cal"); > + if (!IS_ERR(clk)) > + msm_host->bulk_clks[2].clk = clk; > + > + clk = devm_clk_get(&pdev->dev, "sleep"); > + if (!IS_ERR(clk)) > + msm_host->bulk_clks[3].clk = clk; > + First, both these clocks needs to be documented in DT doc. Second, I think you should initialize bulk_clks[2|3] to NULL, in case the new optional clocks can't be fetched. > ret = clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks), > msm_host->bulk_clks); > if (ret) > -- > 2.12.0 > Another observation is the number of clocks for this device. In some cases, now six clocks are needed!? Is that really correct? Just wanted to point it out as it seems a bit too much. :-) Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 22 Aug 03:45 PDT 2017, Ulf Hansson wrote: > [...] > > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c > > index 71e01cbc38b6..7b47906ba447 100644 > > --- a/drivers/mmc/host/sdhci-msm.c > > +++ b/drivers/mmc/host/sdhci-msm.c > > @@ -131,7 +131,7 @@ struct sdhci_msm_host { > > struct clk *pclk; /* SDHC peripheral bus clock */ > > struct clk *bus_clk; /* SDHC bus voter clock */ > > struct clk *xo_clk; /* TCXO clk needed for FLL feature of cm_dll*/ > > - struct clk_bulk_data bulk_clks[2]; > > + struct clk_bulk_data bulk_clks[4]; > > unsigned long clk_rate; > > struct mmc_host *mmc; > > bool use_14lpp_dll_reset; > > @@ -1125,6 +1125,7 @@ static int sdhci_msm_probe(struct platform_device *pdev) > > struct sdhci_pltfm_host *pltfm_host; > > struct sdhci_msm_host *msm_host; > > struct resource *core_memres; > > + struct clk *clk; > > int ret; > > u16 host_version, core_minor; > > u32 core_version, config; > > @@ -1194,6 +1195,14 @@ static int sdhci_msm_probe(struct platform_device *pdev) > > msm_host->bulk_clks[0].clk = msm_host->clk; > > msm_host->bulk_clks[1].clk = msm_host->pclk; > > > > + clk = devm_clk_get(&pdev->dev, "cal"); > > + if (!IS_ERR(clk)) > > + msm_host->bulk_clks[2].clk = clk; > > + > > + clk = devm_clk_get(&pdev->dev, "sleep"); > > + if (!IS_ERR(clk)) > > + msm_host->bulk_clks[3].clk = clk; > > + > > First, both these clocks needs to be documented in DT doc. > Of course, sorry for missing this part. > Second, I think you should initialize bulk_clks[2|3] to NULL, in case > the new optional clocks can't be fetched. > msm_host does come from a kzalloc() in mmc_alloc_host(), but I can write this differently to not rely on this "fact". > > ret = clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks), > > msm_host->bulk_clks); > > if (ret) > > -- > > 2.12.0 > > > > Another observation is the number of clocks for this device. In some > cases, now six clocks are needed!? Is that really correct? Just wanted > to point it out as it seems a bit too much. :-) > * we need "iface" and "core" for normal operation * "xo" is the base clock of the entire system and is always present, question is why its clock rate isn't hard coded in the driver. * "bus" should probably not be handled directly in the driver, but rather through the upcoming "interconnect" framework * And finally these two new clocks are needed on some HS400-enabled platforms, for calibrating the separate (RCLK) clock delay circuit So I believe the right answer should have been 2 required and 2 optional clocks. But we need the interconnect framework and I'll look into why we need to specify "xo". Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23 August 2017 at 19:28, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Tue 22 Aug 03:45 PDT 2017, Ulf Hansson wrote: > >> [...] >> >> > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c >> > index 71e01cbc38b6..7b47906ba447 100644 >> > --- a/drivers/mmc/host/sdhci-msm.c >> > +++ b/drivers/mmc/host/sdhci-msm.c >> > @@ -131,7 +131,7 @@ struct sdhci_msm_host { >> > struct clk *pclk; /* SDHC peripheral bus clock */ >> > struct clk *bus_clk; /* SDHC bus voter clock */ >> > struct clk *xo_clk; /* TCXO clk needed for FLL feature of cm_dll*/ >> > - struct clk_bulk_data bulk_clks[2]; >> > + struct clk_bulk_data bulk_clks[4]; >> > unsigned long clk_rate; >> > struct mmc_host *mmc; >> > bool use_14lpp_dll_reset; >> > @@ -1125,6 +1125,7 @@ static int sdhci_msm_probe(struct platform_device *pdev) >> > struct sdhci_pltfm_host *pltfm_host; >> > struct sdhci_msm_host *msm_host; >> > struct resource *core_memres; >> > + struct clk *clk; >> > int ret; >> > u16 host_version, core_minor; >> > u32 core_version, config; >> > @@ -1194,6 +1195,14 @@ static int sdhci_msm_probe(struct platform_device *pdev) >> > msm_host->bulk_clks[0].clk = msm_host->clk; >> > msm_host->bulk_clks[1].clk = msm_host->pclk; >> > >> > + clk = devm_clk_get(&pdev->dev, "cal"); >> > + if (!IS_ERR(clk)) >> > + msm_host->bulk_clks[2].clk = clk; >> > + >> > + clk = devm_clk_get(&pdev->dev, "sleep"); >> > + if (!IS_ERR(clk)) >> > + msm_host->bulk_clks[3].clk = clk; >> > + >> >> First, both these clocks needs to be documented in DT doc. >> > > Of course, sorry for missing this part. > >> Second, I think you should initialize bulk_clks[2|3] to NULL, in case >> the new optional clocks can't be fetched. >> > > msm_host does come from a kzalloc() in mmc_alloc_host(), but I can write > this differently to not rely on this "fact". No, it's fine as is, we often relies on that fact. Sorry about the noise. > >> > ret = clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks), >> > msm_host->bulk_clks); >> > if (ret) >> > -- >> > 2.12.0 >> > >> >> Another observation is the number of clocks for this device. In some >> cases, now six clocks are needed!? Is that really correct? Just wanted >> to point it out as it seems a bit too much. :-) >> > > * we need "iface" and "core" for normal operation > > * "xo" is the base clock of the entire system and is always present, > question is why its clock rate isn't hard coded in the driver. > > * "bus" should probably not be handled directly in the driver, but > rather through the upcoming "interconnect" framework > > * And finally these two new clocks are needed on some HS400-enabled > platforms, for calibrating the separate (RCLK) clock delay circuit > > So I believe the right answer should have been 2 required and 2 optional > clocks. But we need the interconnect framework and I'll look into why > we need to specify "xo". Thanks for the explanation so far. Looking forward to further clarifications. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c index 71e01cbc38b6..7b47906ba447 100644 --- a/drivers/mmc/host/sdhci-msm.c +++ b/drivers/mmc/host/sdhci-msm.c @@ -131,7 +131,7 @@ struct sdhci_msm_host { struct clk *pclk; /* SDHC peripheral bus clock */ struct clk *bus_clk; /* SDHC bus voter clock */ struct clk *xo_clk; /* TCXO clk needed for FLL feature of cm_dll*/ - struct clk_bulk_data bulk_clks[2]; + struct clk_bulk_data bulk_clks[4]; unsigned long clk_rate; struct mmc_host *mmc; bool use_14lpp_dll_reset; @@ -1125,6 +1125,7 @@ static int sdhci_msm_probe(struct platform_device *pdev) struct sdhci_pltfm_host *pltfm_host; struct sdhci_msm_host *msm_host; struct resource *core_memres; + struct clk *clk; int ret; u16 host_version, core_minor; u32 core_version, config; @@ -1194,6 +1195,14 @@ static int sdhci_msm_probe(struct platform_device *pdev) msm_host->bulk_clks[0].clk = msm_host->clk; msm_host->bulk_clks[1].clk = msm_host->pclk; + clk = devm_clk_get(&pdev->dev, "cal"); + if (!IS_ERR(clk)) + msm_host->bulk_clks[2].clk = clk; + + clk = devm_clk_get(&pdev->dev, "sleep"); + if (!IS_ERR(clk)) + msm_host->bulk_clks[3].clk = clk; + ret = clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks), msm_host->bulk_clks); if (ret)
The delay circuit used to support HS400 is calibrated based on two additional clocks. When these clocks are not available and FF_CLK_SW_RST_DIS is not set in CORE_HC_MODE, reset might fail. But on some platforms this doesn't work properly and below dump can be seen in the kernel log. mmc0: Reset 0x1 never completed. mmc0: sdhci: ============ SDHCI REGISTER DUMP =========== mmc0: sdhci: Sys addr: 0x00000000 | Version: 0x00001102 mmc0: sdhci: Blk size: 0x00004000 | Blk cnt: 0x00000000 mmc0: sdhci: Argument: 0x00000000 | Trn mode: 0x00000000 mmc0: sdhci: Present: 0x01f80000 | Host ctl: 0x00000000 mmc0: sdhci: Power: 0x00000000 | Blk gap: 0x00000000 mmc0: sdhci: Wake-up: 0x00000000 | Clock: 0x00000002 mmc0: sdhci: Timeout: 0x00000000 | Int stat: 0x00000000 mmc0: sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000 mmc0: sdhci: AC12 err: 0x00000000 | Slot int: 0x00000000 mmc0: sdhci: Caps: 0x742dc8b2 | Caps_1: 0x00008007 mmc0: sdhci: Cmd: 0x00000000 | Max curr: 0x00000000 mmc0: sdhci: Resp[0]: 0x00000000 | Resp[1]: 0x00000000 mmc0: sdhci: Resp[2]: 0x00000000 | Resp[3]: 0x00000000 mmc0: sdhci: Host ctl2: 0x00000000 mmc0: sdhci: ============================================ Add support for the additional calibration clocks to allow these platforms to be configured appropriately. Cc: Venkat Gopalakrishnan <venkatg@codeaurora.org> Cc: Ritesh Harjani <riteshh@codeaurora.org> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- drivers/mmc/host/sdhci-msm.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)