From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932397AbdHWR2e (ORCPT ); Wed, 23 Aug 2017 13:28:34 -0400 Received: from mail-pg0-f46.google.com ([74.125.83.46]:37960 "EHLO mail-pg0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932204AbdHWR2c (ORCPT ); Wed, 23 Aug 2017 13:28:32 -0400 Date: Wed, 23 Aug 2017 10:28:28 -0700 From: Bjorn Andersson To: Ulf Hansson Cc: Adrian Hunter , "linux-mmc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-arm-msm@vger.kernel.org" , Venkat Gopalakrishnan , Ritesh Harjani Subject: Re: [PATCH 2/2] mmc: sdhci-msm: Enable delay circuit calibration clocks Message-ID: <20170823172828.GA29306@minitux> References: <20170818175506.5035-1-bjorn.andersson@linaro.org> <20170818175506.5035-3-bjorn.andersson@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.8.3 (2017-05-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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