| Message ID | 20170804174353.16486-1-jbrunet@baylibre.com |
|---|---|
| Headers | show |
| Series | mmc: meson-gx: driver fixups and upgrade | expand |
On Fri, 2017-08-04 at 19:43 +0200, Jerome Brunet wrote: > The patchset features several bugfixes, rework and upgrade for the > meson-gx MMC driver. > > The main goal is to improve readability and enable new high speed > modes, such as eMMC DDR52 and sdcard UHS modes up to SDR50 (100Mhz) > > While full speed SDR104 is stable with most cards, a few seems to > require an even more precise tuning. For this, we'll probably have > to implement per-line delay calibration. > > This series has been tested on gxbb-p200, gxbb-nanopi-k2 and > gxl-s905x-libretech > Ulf, Kevin, Please ignore this series. I'll resend it soon I've discovered new things around the tuning capabilities of the SoC. I'll probably rework the tuning function again. Sorry for the noise Regards Jerome > Jerome Brunet (14): > mmc: meson-gx: fix mux mask definition > mmc: meson-gx: remove CLK_DIVIDER_ALLOW_ZERO clock flag > mmc: meson-gx: clean up some constants > mmc: meson-gx: use _irqsave variant of spinlock > mmc: meson-gx: cfg init overwrite values > mmc: meson-gx: rework set_ios function > mmc: meson-gx: rework clk_set function > mmc: meson-gx: rework clock init function > mmc: meson-gx: simplify interrupt handler > mmc: meson-gx: implement card_busy callback > mmc: meson-gx: rework tuning function > mmc: meson-gx: fix dual data rate mode frequencies > mmc: meson-gx: work around clk-stop issue > mmc: meson-gx: implement voltage switch callback > > drivers/mmc/host/meson-gx-mmc.c | 611 ++++++++++++++++++++++++++------------- > - > 1 file changed, 396 insertions(+), 215 deletions(-) > -- 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
Jerome Brunet <jbrunet@baylibre.com> writes: > Remove useless clock rate defines. These should not be defined but To be more precise, they're also unused, so maybe s/useless/unused/ ? > equested from the clock framework. s/equested/requested/ > Also correct typo on the DELAY register > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> Otherwise, Reviewed-by: Kevin Hilman <khilman@baylibre.com> > --- > drivers/mmc/host/meson-gx-mmc.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > index d480a8052a06..8a74a048db88 100644 > --- a/drivers/mmc/host/meson-gx-mmc.c > +++ b/drivers/mmc/host/meson-gx-mmc.c > @@ -45,9 +45,7 @@ > #define CLK_DIV_MAX 63 > #define CLK_SRC_MASK GENMASK(7, 6) > #define CLK_SRC_XTAL 0 /* external crystal */ > -#define CLK_SRC_XTAL_RATE 24000000 > #define CLK_SRC_PLL 1 /* FCLK_DIV2 */ > -#define CLK_SRC_PLL_RATE 1000000000 > #define CLK_CORE_PHASE_MASK GENMASK(9, 8) > #define CLK_TX_PHASE_MASK GENMASK(11, 10) > #define CLK_RX_PHASE_MASK GENMASK(13, 12) > @@ -57,7 +55,7 @@ > #define CLK_PHASE_270 3 > #define CLK_ALWAYS_ON BIT(24) > > -#define SD_EMMC_DElAY 0x4 > +#define SD_EMMC_DELAY 0x4 > #define SD_EMMC_ADJUST 0x8 > #define SD_EMMC_CALOUT 0x10 > #define SD_EMMC_START 0x40 -- 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
Jerome Brunet <jbrunet@baylibre.com> writes: > Perform basic initialisation of the clk register before providing it to > the CCF. > > Thanks to devm, carrying the clock structure around after init is not > necessary. Rework the function to remove these from the controller host > data. > > Finally, set initial mmc clock rate before enabling it, simplifying the > exit condition. > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/mmc/host/meson-gx-mmc.c | 101 +++++++++++++++++++--------------------- > 1 file changed, 49 insertions(+), 52 deletions(-) > > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > index 8f9ba5190c18..4cc7d6530536 100644 > --- a/drivers/mmc/host/meson-gx-mmc.c > +++ b/drivers/mmc/host/meson-gx-mmc.c > @@ -42,10 +42,7 @@ > > #define SD_EMMC_CLOCK 0x0 > #define CLK_DIV_MASK GENMASK(5, 0) > -#define CLK_DIV_MAX 63 > #define CLK_SRC_MASK GENMASK(7, 6) > -#define CLK_SRC_XTAL 0 /* external crystal */ > -#define CLK_SRC_PLL 1 /* FCLK_DIV2 */ > #define CLK_CORE_PHASE_MASK GENMASK(9, 8) > #define CLK_TX_PHASE_MASK GENMASK(11, 10) > #define CLK_RX_PHASE_MASK GENMASK(13, 12) > @@ -137,13 +134,9 @@ struct meson_host { > spinlock_t lock; > void __iomem *regs; > struct clk *core_clk; > - struct clk_mux mux; > - struct clk *mux_clk; > + struct clk *signal_clk; > unsigned long req_rate; > > - struct clk_divider cfg_div; > - struct clk *cfg_div_clk; > - > unsigned int bounce_buf_size; > void *bounce_buf; > dma_addr_t bounce_dma_addr; > @@ -291,7 +284,7 @@ static int meson_mmc_clk_set(struct meson_host *host, unsigned long clk_rate) > return 0; > } > > - ret = clk_set_rate(host->cfg_div_clk, clk_rate); > + ret = clk_set_rate(host->signal_clk, clk_rate); minor nit: where does the name "signal" come from? I called this "div_clk" because it's the output of the divider right before the sd/emmc IP block. Admittedly, that's not a great name either, and I'm not too picky about the naming, just curious... Looking at the diagram we have since I initially wrote the driver, this is more commonly referred to as device_clk. Anyways, if you're going to rename... [...] > static void meson_mmc_set_tuning_params(struct mmc_host *mmc) > @@ -987,7 +984,7 @@ static int meson_mmc_probe(struct platform_device *pdev) > dma_free_coherent(host->dev, host->bounce_buf_size, > host->bounce_buf, host->bounce_dma_addr); > err_div_clk: ... probably should rename this too. Otherwise, Reviewed-by: Kevin Hilman <khilman@baylibre.com> Kevin -- 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
Jerome Brunet <jbrunet@baylibre.com> writes: > It seems that the signal clock is also used and required, somehow, by > the controller it self. > > It is shown during init, when writing to CFG while the divider is set > to 0 will crash the SoC. During voltage switch, the controller may crash > and the card may then fail to exit busy state if the clock is stopped. > > To avoid this, it is best to keep the clock running for the controller, > except during rate change. However, we still need to be able to gate > the clock out of the SoC. Let's use the pinmux for this, and fallback > to gpio mode (pulled-down) when we need to gate the clock > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> Curious "feature" of the IP, but the solution looks good to me. Reviewed-by: Kevin Hilman <khilman@baylibre.com> Kevin -- 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
Jerome Brunet <jbrunet@baylibre.com> writes: > On Fri, 2017-08-04 at 19:43 +0200, Jerome Brunet wrote: >> The patchset features several bugfixes, rework and upgrade for the >> meson-gx MMC driver. >> >> The main goal is to improve readability and enable new high speed >> modes, such as eMMC DDR52 and sdcard UHS modes up to SDR50 (100Mhz) >> >> While full speed SDR104 is stable with most cards, a few seems to >> require an even more precise tuning. For this, we'll probably have >> to implement per-line delay calibration. >> >> This series has been tested on gxbb-p200, gxbb-nanopi-k2 and >> gxl-s905x-libretech >> > > Ulf, Kevin, > > Please ignore this series. I'll resend it soon > > I've discovered new things around the tuning capabilities of the SoC. > I'll probably rework the tuning function again. > > Sorry for the noise No worries, I had a couple very minor comments on some individual patches. Other than that, LGTM. Feel free to add: Reviewed-by: Kevin Hilman <khilman@baylibre.com> to anything that doesn't need major rework. Also, You might want to separate out the fixups/cleanups from the voltage/clock changes stuff. Kevin -- 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 Mon, 2017-08-07 at 14:06 -0700, Kevin Hilman wrote: > Jerome Brunet <jbrunet@baylibre.com> writes: > > > Remove useless clock rate defines. These should not be defined but > > To be more precise, they're also unused, so maybe s/useless/unused/ ? > > > equested from the clock framework. > > s/equested/requested/ > Thx Kevin ! > > Also correct typo on the DELAY register > > > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > > Otherwise, > > Reviewed-by: Kevin Hilman <khilman@baylibre.com> > > > --- > > drivers/mmc/host/meson-gx-mmc.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx- > > mmc.c > > index d480a8052a06..8a74a048db88 100644 > > --- a/drivers/mmc/host/meson-gx-mmc.c > > +++ b/drivers/mmc/host/meson-gx-mmc.c > > @@ -45,9 +45,7 @@ > > #define CLK_DIV_MAX 63 > > #define CLK_SRC_MASK GENMASK(7, 6) > > #define CLK_SRC_XTAL 0 /* external crystal */ > > -#define CLK_SRC_XTAL_RATE 24000000 > > #define CLK_SRC_PLL 1 /* FCLK_DIV2 */ > > -#define CLK_SRC_PLL_RATE 1000000000 > > #define CLK_CORE_PHASE_MASK GENMASK(9, 8) > > #define CLK_TX_PHASE_MASK GENMASK(11, 10) > > #define CLK_RX_PHASE_MASK GENMASK(13, 12) > > @@ -57,7 +55,7 @@ > > #define CLK_PHASE_270 3 > > #define CLK_ALWAYS_ON BIT(24) > > > > -#define SD_EMMC_DElAY 0x4 > > +#define SD_EMMC_DELAY 0x4 > > #define SD_EMMC_ADJUST 0x8 > > #define SD_EMMC_CALOUT 0x10 > > #define SD_EMMC_START 0x40 -- 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