| Message ID | cover.1558346019.git.baolin.wang@linaro.org |
|---|---|
| Headers | show |
| Series | Add SD host controller support for SC9860 platform | expand |
Hi Adrian & Ulf, On Mon, 20 May 2019 at 18:12, Baolin Wang <baolin.wang@linaro.org> wrote: > > This patch set adds optional clock support, HS400 enhanced strobe mode support, > PHY DLL configuration and other optimization to make the SD host controller > can work well on the Spreadtrum SC9860 platform. Do you have any comments for this patch set? Thanks. > > Baolin Wang (9): > mmc: sdhci-sprd: Check the enable clock's return value correctly > dt-bindings: mmc: sprd: Add another optional clock documentation > mmc: sdhci-sprd: Add optional gate clock support > mmc: sdhci-sprd: Implement the get_max_timeout_count() interface > mmc: sdhci-sprd: Add HS400 enhanced strobe mode > mmc: sdhci-sprd: Enable PHY DLL to make clock stable > dt-bindings: mmc: sprd: Add PHY DLL delay documentation > mmc: sdhci-sprd: Add PHY DLL delay configuration > arm64: dts: sprd: Add Spreadtrum SD host controller support > > .../devicetree/bindings/mmc/sdhci-sprd.txt | 19 +++ > arch/arm64/boot/dts/sprd/whale2.dtsi | 35 ++++ > drivers/mmc/host/sdhci-sprd.c | 171 +++++++++++++++++++- > 3 files changed, 217 insertions(+), 8 deletions(-) > > -- > 1.7.9.5 > -- Baolin Wang Best Regards
On 20/05/19 1:11 PM, Baolin Wang wrote: > For the Spreadtrum SD host controller, when we changed the clock to be > more than 52M, we should enable the PHY DLL which is used to track the > clock frequency to make the clock work more stable. Otherwise deviation > may occur of the higher clock. > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/host/sdhci-sprd.c | 44 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 43 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c > index edec197..e6eda13 100644 > --- a/drivers/mmc/host/sdhci-sprd.c > +++ b/drivers/mmc/host/sdhci-sprd.c > @@ -22,6 +22,13 @@ > /* SDHCI_ARGUMENT2 register high 16bit */ > #define SDHCI_SPRD_ARG2_STUFF GENMASK(31, 16) > > +#define SDHCI_SPRD_REG_32_DLL_CFG 0x200 > +#define SDHCI_SPRD_DLL_ALL_CPST_EN (BIT(18) | BIT(24) | BIT(25) | BIT(26) | BIT(27)) > +#define SDHCI_SPRD_DLL_EN BIT(21) > +#define SDHCI_SPRD_DLL_SEARCH_MODE BIT(16) > +#define SDHCI_SPRD_DLL_INIT_COUNT 0xc00 > +#define SDHCI_SPRD_DLL_PHASE_INTERNAL 0x3 > + > #define SDHCI_SPRD_REG_32_DLL_DLY_OFFSET 0x208 > #define SDHCIBSPRD_IT_WR_DLY_INV BIT(5) > #define SDHCI_SPRD_BIT_CMD_DLY_INV BIT(13) > @@ -56,6 +63,7 @@ > #define SDHCI_SPRD_CLK_MAX_DIV 1023 > > #define SDHCI_SPRD_CLK_DEF_RATE 26000000 > +#define SDHCI_SPRD_PHY_DLL_CLK 52000000 > > struct sdhci_sprd_host { > u32 version; > @@ -200,9 +208,33 @@ static inline void _sdhci_sprd_set_clock(struct sdhci_host *host, > } > } > > +static void sdhci_sprd_enable_phy_dll(struct sdhci_host *host) > +{ > + u32 tmp; > + > + tmp = sdhci_readl(host, SDHCI_SPRD_REG_32_DLL_CFG); > + tmp &= ~(SDHCI_SPRD_DLL_EN | SDHCI_SPRD_DLL_ALL_CPST_EN); > + sdhci_writel(host, tmp, SDHCI_SPRD_REG_32_DLL_CFG); > + /* wait 1ms */ > + usleep_range(1000, 1250); > + > + tmp = sdhci_readl(host, SDHCI_SPRD_REG_32_DLL_CFG); > + tmp |= SDHCI_SPRD_DLL_ALL_CPST_EN | SDHCI_SPRD_DLL_SEARCH_MODE | > + SDHCI_SPRD_DLL_INIT_COUNT | SDHCI_SPRD_DLL_PHASE_INTERNAL; > + sdhci_writel(host, tmp, SDHCI_SPRD_REG_32_DLL_CFG); > + /* wait 1ms */ > + usleep_range(1000, 1250); > + > + tmp = sdhci_readl(host, SDHCI_SPRD_REG_32_DLL_CFG); > + tmp |= SDHCI_SPRD_DLL_EN; > + sdhci_writel(host, tmp, SDHCI_SPRD_REG_32_DLL_CFG); > + /* wait 1ms */ > + usleep_range(1000, 1250); > +} > + > static void sdhci_sprd_set_clock(struct sdhci_host *host, unsigned int clock) > { > - bool en = false; > + bool en = false, clk_changed = false; > > if (clock == 0) { > sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); > @@ -214,9 +246,19 @@ static void sdhci_sprd_set_clock(struct sdhci_host *host, unsigned int clock) > en = true; > sdhci_sprd_set_dll_invert(host, SDHCI_SPRD_BIT_CMD_DLY_INV | > SDHCI_SPRD_BIT_POSRD_DLY_INV, en); > + clk_changed = true; > } else { > _sdhci_sprd_set_clock(host, clock); > } > + > + /* > + * According to the Spreadtrum SD host specification, when we changed > + * the clock to be more than 52M, we should enable the PHY DLL which > + * is used to track the clock frequency to make the clock work more > + * stable. Otherwise deviation may occur of the higher clock. > + */ > + if (clk_changed && clock > SDHCI_SPRD_PHY_DLL_CLK) > + sdhci_sprd_enable_phy_dll(host); > } > > static unsigned int sdhci_sprd_get_max_clock(struct sdhci_host *host) >
Hi Adrian, On Mon, 3 Jun 2019 at 21:03, Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 20/05/19 1:12 PM, Baolin Wang wrote: > > Set the PHY DLL delay for each timing mode, which is used to sample the clock > > accurately and make the clock more stable. > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > One comment, nevertheless: > > Acked-by: Adrian Hunter <adrian.hunter@intel.com> > > > --- > > drivers/mmc/host/sdhci-sprd.c | 51 +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 51 insertions(+) > > > > diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c > > index e6eda13..911a09b 100644 > > --- a/drivers/mmc/host/sdhci-sprd.c > > +++ b/drivers/mmc/host/sdhci-sprd.c > > @@ -29,6 +29,8 @@ > > #define SDHCI_SPRD_DLL_INIT_COUNT 0xc00 > > #define SDHCI_SPRD_DLL_PHASE_INTERNAL 0x3 > > > > +#define SDHCI_SPRD_REG_32_DLL_DLY 0x204 > > + > > #define SDHCI_SPRD_REG_32_DLL_DLY_OFFSET 0x208 > > #define SDHCIBSPRD_IT_WR_DLY_INV BIT(5) > > #define SDHCI_SPRD_BIT_CMD_DLY_INV BIT(13) > > @@ -72,6 +74,24 @@ struct sdhci_sprd_host { > > struct clk *clk_2x_enable; > > u32 base_rate; > > int flags; /* backup of host attribute */ > > + u32 phy_delay[MMC_TIMING_MMC_HS400 + 2]; > > +}; > > + > > +struct sdhci_sprd_phy_cfg { > > + const char *property; > > + u8 timing; > > +}; > > + > > +static const struct sdhci_sprd_phy_cfg sdhci_sprd_phy_cfgs[] = { > > + { "sprd,phy-delay-legacy", MMC_TIMING_LEGACY, }, > > + { "sprd,phy-delay-sd-highspeed", MMC_TIMING_MMC_HS, }, > > Did you mean MMC_TIMING_SD_HS Ah, yes, my copy mistake and will fix it in next version. Thanks for your reviewing and comments. -- Baolin Wang Best Regards
On Mon, 3 Jun 2019 at 10:42, Baolin Wang <baolin.wang@linaro.org> wrote: > > Hi Adrian & Ulf, > > On Mon, 20 May 2019 at 18:12, Baolin Wang <baolin.wang@linaro.org> wrote: > > > > This patch set adds optional clock support, HS400 enhanced strobe mode support, > > PHY DLL configuration and other optimization to make the SD host controller > > can work well on the Spreadtrum SC9860 platform. > > Do you have any comments for this patch set? Thanks. > Seems like the series is almost ready to go. However, due to a few the minor comments/questions from Adrian, I am expecting a new version from you before applying. Kind regards Uffe > > > > Baolin Wang (9): > > mmc: sdhci-sprd: Check the enable clock's return value correctly > > dt-bindings: mmc: sprd: Add another optional clock documentation > > mmc: sdhci-sprd: Add optional gate clock support > > mmc: sdhci-sprd: Implement the get_max_timeout_count() interface > > mmc: sdhci-sprd: Add HS400 enhanced strobe mode > > mmc: sdhci-sprd: Enable PHY DLL to make clock stable > > dt-bindings: mmc: sprd: Add PHY DLL delay documentation > > mmc: sdhci-sprd: Add PHY DLL delay configuration > > arm64: dts: sprd: Add Spreadtrum SD host controller support > > > > .../devicetree/bindings/mmc/sdhci-sprd.txt | 19 +++ > > arch/arm64/boot/dts/sprd/whale2.dtsi | 35 ++++ > > drivers/mmc/host/sdhci-sprd.c | 171 +++++++++++++++++++- > > 3 files changed, 217 insertions(+), 8 deletions(-) > > > > -- > > 1.7.9.5 > > > > > -- > Baolin Wang > Best Regards
On Tue, 4 Jun 2019 at 15:14, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Mon, 3 Jun 2019 at 10:42, Baolin Wang <baolin.wang@linaro.org> wrote: > > > > Hi Adrian & Ulf, > > > > On Mon, 20 May 2019 at 18:12, Baolin Wang <baolin.wang@linaro.org> wrote: > > > > > > This patch set adds optional clock support, HS400 enhanced strobe mode support, > > > PHY DLL configuration and other optimization to make the SD host controller > > > can work well on the Spreadtrum SC9860 platform. > > > > Do you have any comments for this patch set? Thanks. > > > > Seems like the series is almost ready to go. However, due to a few the > minor comments/questions from Adrian, I am expecting a new version > from you before applying. Okay, I am preparing the V2 with addressing the minor comment. Thanks. -- Baolin Wang Best Regards
On Mon, 3 Jun 2019 at 20:35, Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 20/05/19 1:11 PM, Baolin Wang wrote: > > Implement the get_max_timeout_count() interface to set the Spredtrum SD > > host controller actual maximum timeout count. > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > Seems surprising that there isn't a custom ->set_timeout() as well. Until now we did not find issues when using sdhci_calc_timeout(). Thanks for your reviewing. > Nevertheless: > > Acked-by: Adrian Hunter <adrian.hunter@intel.com> > > > --- > > drivers/mmc/host/sdhci-sprd.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c > > index 31ba7d6..d91281d 100644 > > --- a/drivers/mmc/host/sdhci-sprd.c > > +++ b/drivers/mmc/host/sdhci-sprd.c > > @@ -285,6 +285,12 @@ static void sdhci_sprd_hw_reset(struct sdhci_host *host) > > usleep_range(300, 500); > > } > > > > +static unsigned int sdhci_sprd_get_max_timeout_count(struct sdhci_host *host) > > +{ > > + /* The Spredtrum controller actual maximum timeout count is 1 << 31 */ > > + return 1 << 31; > > +} > > + > > static struct sdhci_ops sdhci_sprd_ops = { > > .read_l = sdhci_sprd_readl, > > .write_l = sdhci_sprd_writel, > > @@ -296,6 +302,7 @@ static void sdhci_sprd_hw_reset(struct sdhci_host *host) > > .reset = sdhci_reset, > > .set_uhs_signaling = sdhci_sprd_set_uhs_signaling, > > .hw_reset = sdhci_sprd_hw_reset, > > + .get_max_timeout_count = sdhci_sprd_get_max_timeout_count, > > }; > > > > static void sdhci_sprd_request(struct mmc_host *mmc, struct mmc_request *mrq) > > > -- Baolin Wang Best Regards