[PATCH 0/4] mmc: meson-gx: smaller functional extensions

[PATCH 0/4] mmc: meson-gx: smaller functional extensions

[Heiner Kallweit]

This series includes the switch to bitfield macros and a few smaller
functional extensions.

Heiner Kallweit (4):
  mmc: meson-gx: use bitfield macros
  mmc: meson-gx: use per port interrupt names
  mmc: meson-gx: switch to dynamic timeout values
  mmc: meson-gx: add CMD23 mode

 drivers/mmc/host/meson-gx-mmc.c | 119 +++++++++++++++++++++++-----------------
 1 file changed, 68 insertions(+), 51 deletions(-)

-- 
2.12.0

[PATCH 1/4] mmc: meson-gx: use bitfield macros

[Heiner Kallweit]

Use GENMASK consistently for all bit masks and switch to using the
bitfield macros GET_FIELD and PREP_FIELD. This hides parts of the
complexity of dealing with bit fields.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/mmc/host/meson-gx-mmc.c | 84 +++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 46 deletions(-)

diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index b917765c..cf2ccc67 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -36,23 +36,25 @@
 #include <linux/clk-provider.h>
 #include <linux/regulator/consumer.h>
 #include <linux/interrupt.h>
+#include <linux/bitfield.h>
 
 #define DRIVER_NAME "meson-gx-mmc"
 
 #define SD_EMMC_CLOCK 0x0
 #define   CLK_DIV_SHIFT 0
 #define   CLK_DIV_WIDTH 6
-#define   CLK_DIV_MASK 0x3f
+#define   CLK_DIV_MASK GENMASK(5, 0)
 #define   CLK_DIV_MAX 63
 #define   CLK_SRC_SHIFT 6
 #define   CLK_SRC_WIDTH 2
-#define   CLK_SRC_MASK 0x3
+#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_PHASE_SHIFT 8
-#define   CLK_PHASE_MASK 0x3
+#define   CLK_CORE_PHASE_MASK GENMASK(9, 8)
+#define   CLK_TX_PHASE_MASK GENMASK(11, 10)
+#define   CLK_RX_PHASE_MASK GENMASK(13, 12)
 #define   CLK_PHASE_0 0
 #define   CLK_PHASE_90 1
 #define   CLK_PHASE_180 2
@@ -65,22 +67,17 @@
 #define SD_EMMC_START 0x40
 #define   START_DESC_INIT BIT(0)
 #define   START_DESC_BUSY BIT(1)
-#define   START_DESC_ADDR_SHIFT 2
-#define   START_DESC_ADDR_MASK (~0x3)
+#define   START_DESC_ADDR_MASK GENMASK(31, 2)
 
 #define SD_EMMC_CFG 0x44
-#define   CFG_BUS_WIDTH_SHIFT 0
-#define   CFG_BUS_WIDTH_MASK 0x3
+#define   CFG_BUS_WIDTH_MASK GENMASK(1, 0)
 #define   CFG_BUS_WIDTH_1 0x0
 #define   CFG_BUS_WIDTH_4 0x1
 #define   CFG_BUS_WIDTH_8 0x2
 #define   CFG_DDR BIT(2)
-#define   CFG_BLK_LEN_SHIFT 4
-#define   CFG_BLK_LEN_MASK 0xf
-#define   CFG_RESP_TIMEOUT_SHIFT 8
-#define   CFG_RESP_TIMEOUT_MASK 0xf
-#define   CFG_RC_CC_SHIFT 12
-#define   CFG_RC_CC_MASK 0xf
+#define   CFG_BLK_LEN_MASK GENMASK(7, 4)
+#define   CFG_RESP_TIMEOUT_MASK GENMASK(11, 8)
+#define   CFG_RC_CC_MASK GENMASK(15, 12)
 #define   CFG_STOP_CLOCK BIT(22)
 #define   CFG_CLK_ALWAYS_ON BIT(18)
 #define   CFG_CHK_DS BIT(20)
@@ -90,9 +87,8 @@
 #define   STATUS_BUSY BIT(31)
 
 #define SD_EMMC_IRQ_EN 0x4c
-#define   IRQ_EN_MASK 0x3fff
-#define   IRQ_RXD_ERR_SHIFT 0
-#define   IRQ_RXD_ERR_MASK 0xff
+#define   IRQ_EN_MASK GENMASK(13, 0)
+#define   IRQ_RXD_ERR_MASK GENMASK(7, 0)
 #define   IRQ_TXD_ERR BIT(8)
 #define   IRQ_DESC_ERR BIT(9)
 #define   IRQ_RESP_ERR BIT(10)
@@ -149,13 +145,12 @@ struct sd_emmc_desc {
 	u32 cmd_data;
 	u32 cmd_resp;
 };
-#define CMD_CFG_LENGTH_SHIFT 0
-#define CMD_CFG_LENGTH_MASK 0x1ff
+
+#define CMD_CFG_LENGTH_MASK GENMASK(8, 0)
 #define CMD_CFG_BLOCK_MODE BIT(9)
 #define CMD_CFG_R1B BIT(10)
 #define CMD_CFG_END_OF_CHAIN BIT(11)
-#define CMD_CFG_TIMEOUT_SHIFT 12
-#define CMD_CFG_TIMEOUT_MASK 0xf
+#define CMD_CFG_TIMEOUT_MASK GENMASK(15, 12)
 #define CMD_CFG_NO_RESP BIT(16)
 #define CMD_CFG_NO_CMD BIT(17)
 #define CMD_CFG_DATA_IO BIT(18)
@@ -164,15 +159,14 @@ struct sd_emmc_desc {
 #define CMD_CFG_RESP_128 BIT(21)
 #define CMD_CFG_RESP_NUM BIT(22)
 #define CMD_CFG_DATA_NUM BIT(23)
-#define CMD_CFG_CMD_INDEX_SHIFT 24
-#define CMD_CFG_CMD_INDEX_MASK 0x3f
+#define CMD_CFG_CMD_INDEX_MASK GENMASK(29, 24)
 #define CMD_CFG_ERROR BIT(30)
 #define CMD_CFG_OWNER BIT(31)
 
-#define CMD_DATA_MASK (~0x3)
+#define CMD_DATA_MASK GENMASK(31, 2)
 #define CMD_DATA_BIG_ENDIAN BIT(1)
 #define CMD_DATA_SRAM BIT(0)
-#define CMD_RESP_MASK (~0x1)
+#define CMD_RESP_MASK GENMASK(31, 1)
 #define CMD_RESP_SRAM BIT(0)
 
 static int meson_mmc_clk_set(struct meson_host *host, unsigned long clk_rate)
@@ -302,9 +296,9 @@ static int meson_mmc_clk_init(struct meson_host *host)
 
 	/* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
 	clk_reg = 0;
-	clk_reg |= CLK_PHASE_180 << CLK_PHASE_SHIFT;
-	clk_reg |= CLK_SRC_XTAL << CLK_SRC_SHIFT;
-	clk_reg |= CLK_DIV_MAX << CLK_DIV_SHIFT;
+	clk_reg |= FIELD_PREP(CLK_CORE_PHASE_MASK, CLK_PHASE_180);
+	clk_reg |= FIELD_PREP(CLK_SRC_MASK, CLK_SRC_XTAL);
+	clk_reg |= FIELD_PREP(CLK_DIV_MASK, CLK_DIV_MAX);
 	clk_reg &= ~CLK_ALWAYS_ON;
 	writel(clk_reg, host->regs + SD_EMMC_CLOCK);
 
@@ -392,8 +386,8 @@ static void meson_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	val = readl(host->regs + SD_EMMC_CFG);
 	orig = val;
 
-	val &= ~(CFG_BUS_WIDTH_MASK << CFG_BUS_WIDTH_SHIFT);
-	val |= bus_width << CFG_BUS_WIDTH_SHIFT;
+	val &= ~CFG_BUS_WIDTH_MASK;
+	val |= FIELD_PREP(CFG_BUS_WIDTH_MASK, bus_width);
 
 	val &= ~CFG_DDR;
 	if (ios->timing == MMC_TIMING_UHS_DDR50 ||
@@ -432,8 +426,7 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)
 	/* Setup descriptors */
 	dma_rmb();
 
-	cmd_cfg |= (cmd->opcode & CMD_CFG_CMD_INDEX_MASK) <<
-		   CMD_CFG_CMD_INDEX_SHIFT;
+	cmd_cfg |= FIELD_PREP(CMD_CFG_CMD_INDEX_MASK, cmd->opcode);
 	cmd_cfg |= CMD_CFG_OWNER;  /* owned by CPU */
 
 	/* Response */
@@ -454,30 +447,27 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)
 	/* data? */
 	if (data) {
 		cmd_cfg |= CMD_CFG_DATA_IO;
-		cmd_cfg |= ilog2(SD_EMMC_CMD_TIMEOUT_DATA) <<
-			   CMD_CFG_TIMEOUT_SHIFT;
+		cmd_cfg |= FIELD_PREP(CMD_CFG_TIMEOUT_MASK,
+				      ilog2(SD_EMMC_CMD_TIMEOUT_DATA));
 
 		if (data->blocks > 1) {
 			cmd_cfg |= CMD_CFG_BLOCK_MODE;
-			cmd_cfg |= (data->blocks & CMD_CFG_LENGTH_MASK) <<
-				   CMD_CFG_LENGTH_SHIFT;
+			cmd_cfg |= FIELD_PREP(CMD_CFG_LENGTH_MASK, data->blocks);
 
 			/* check if block-size matches, if not update */
 			cfg = readl(host->regs + SD_EMMC_CFG);
-			blk_len = cfg & (CFG_BLK_LEN_MASK << CFG_BLK_LEN_SHIFT);
-			blk_len >>= CFG_BLK_LEN_SHIFT;
+			blk_len = FIELD_GET(CFG_BLK_LEN_MASK, cfg);
 			if (blk_len != ilog2(data->blksz)) {
 				dev_dbg(host->dev, "%s: update blk_len %d -> %d\n",
 					__func__, blk_len,
 					ilog2(data->blksz));
 				blk_len = ilog2(data->blksz);
-				cfg &= ~(CFG_BLK_LEN_MASK << CFG_BLK_LEN_SHIFT);
-				cfg |= blk_len << CFG_BLK_LEN_SHIFT;
+				cfg &= ~CFG_BLK_LEN_MASK;
+				cfg |= FIELD_PREP(CFG_BLK_LEN_MASK, blk_len);
 				writel(cfg, host->regs + SD_EMMC_CFG);
 			}
 		} else {
-			cmd_cfg |= (data->blksz & CMD_CFG_LENGTH_MASK) <<
-				   CMD_CFG_LENGTH_SHIFT;
+			cmd_cfg |= FIELD_PREP(CMD_CFG_LENGTH_MASK, data->blksz);
 		}
 
 		data->bytes_xfered = 0;
@@ -492,7 +482,8 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)
 
 		cmd_data = host->bounce_dma_addr & CMD_DATA_MASK;
 	} else {
-		cmd_cfg |= ilog2(SD_EMMC_CMD_TIMEOUT) << CMD_CFG_TIMEOUT_SHIFT;
+		cmd_cfg |= FIELD_PREP(CMD_CFG_TIMEOUT_MASK,
+				      ilog2(SD_EMMC_CMD_TIMEOUT));
 	}
 
 	host->cmd = cmd;
@@ -664,9 +655,10 @@ static void meson_mmc_cfg_init(struct meson_host *host)
 {
 	u32 cfg = 0;
 
-	cfg |= ilog2(SD_EMMC_CFG_RESP_TIMEOUT) << CFG_RESP_TIMEOUT_SHIFT;
-	cfg |= ilog2(SD_EMMC_CFG_CMD_GAP) << CFG_RC_CC_SHIFT;
-	cfg |= ilog2(SD_EMMC_CFG_BLK_SIZE) << CFG_BLK_LEN_SHIFT;
+	cfg |= FIELD_PREP(CFG_RESP_TIMEOUT_MASK,
+			  ilog2(SD_EMMC_CFG_RESP_TIMEOUT));
+	cfg |= FIELD_PREP(CFG_RC_CC_MASK, ilog2(SD_EMMC_CFG_CMD_GAP));
+	cfg |= FIELD_PREP(CFG_BLK_LEN_MASK, ilog2(SD_EMMC_CFG_BLK_SIZE));
 
 	writel(cfg, host->regs + SD_EMMC_CFG);
 }
-- 
2.12.0

[PATCH 2/4] mmc: meson-gx: use per port interrupt names

[Heiner Kallweit]

So far the driver name is used as interrupt description, therefore in
/proc/interrupts it's not possible to tell which interrupt belongs to
which port. Change this by switching to NULL what causes the default
(device name) to be used. In our case that's the DT node name.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/mmc/host/meson-gx-mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index cf2ccc67..b0a9317b 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -740,7 +740,7 @@ static int meson_mmc_probe(struct platform_device *pdev)
 
 	ret = devm_request_threaded_irq(&pdev->dev, irq, meson_mmc_irq,
 					meson_mmc_irq_thread, IRQF_SHARED,
-					DRIVER_NAME, host);
+					NULL, host);
 	if (ret)
 		goto err_div_clk;
 
-- 
2.12.0

[PATCH 3/4] mmc: meson-gx: switch to dynamic timeout values

[Heiner Kallweit]

Currently we use a fixed timeout of 4s for all data transfers. Switch
to dynamic timeout values by making use of data->timeout_ns.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/mmc/host/meson-gx-mmc.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index b0a9317b..e637b3ba 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -169,6 +169,18 @@ struct sd_emmc_desc {
 #define CMD_RESP_MASK GENMASK(31, 1)
 #define CMD_RESP_SRAM BIT(0)
 
+static unsigned int meson_mmc_get_timeout(struct mmc_data *data)
+{
+	unsigned int timeout = data->timeout_ns / NSEC_PER_MSEC;
+
+	if (!timeout)
+		return SD_EMMC_CMD_TIMEOUT_DATA;
+
+	timeout = roundup_pow_of_two(timeout);
+
+	return min(timeout, 32768U); /* max. 2^15 ms */
+}
+
 static int meson_mmc_clk_set(struct meson_host *host, unsigned long clk_rate)
 {
 	struct mmc_host *mmc = host->mmc;
@@ -448,7 +460,7 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)
 	if (data) {
 		cmd_cfg |= CMD_CFG_DATA_IO;
 		cmd_cfg |= FIELD_PREP(CMD_CFG_TIMEOUT_MASK,
-				      ilog2(SD_EMMC_CMD_TIMEOUT_DATA));
+				      ilog2(meson_mmc_get_timeout(data)));
 
 		if (data->blocks > 1) {
 			cmd_cfg |= CMD_CFG_BLOCK_MODE;
-- 
2.12.0

[PATCH 4/4] mmc: meson-gx: add CMD23 mode

[Heiner Kallweit]

CMD23 mode (use "set block count" command before transferring multiple
data blocks) typically is more performant as host / card know upfront
how many data blocks to expect. Therefore add support for this mode to
the driver.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/mmc/host/meson-gx-mmc.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index e637b3ba..6e93270f 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -181,6 +181,17 @@ static unsigned int meson_mmc_get_timeout(struct mmc_data *data)
 	return min(timeout, 32768U); /* max. 2^15 ms */
 }
 
+static struct mmc_command *meson_mmc_get_next_command(struct mmc_command *cmd)
+{
+	if (cmd->opcode == MMC_SET_BLOCK_COUNT && !cmd->error)
+		return cmd->mrq->cmd;
+	else if (mmc_op_multi(cmd->opcode) &&
+		 (!cmd->mrq->sbc || cmd->error || cmd->data->error))
+		return cmd->mrq->stop;
+	else
+		return NULL;
+}
+
 static int meson_mmc_clk_set(struct meson_host *host, unsigned long clk_rate)
 {
 	struct mmc_host *mmc = host->mmc;
@@ -626,7 +637,7 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id)
 static irqreturn_t meson_mmc_irq_thread(int irq, void *dev_id)
 {
 	struct meson_host *host = dev_id;
-	struct mmc_command *cmd = host->cmd;
+	struct mmc_command *next_cmd, *cmd = host->cmd;
 	struct mmc_data *data;
 	unsigned int xfer_bytes;
 
@@ -641,10 +652,11 @@ static irqreturn_t meson_mmc_irq_thread(int irq, void *dev_id)
 				    host->bounce_buf, xfer_bytes);
 	}
 
-	if (!data || !data->stop || cmd->mrq->sbc)
-		meson_mmc_request_done(host->mmc, cmd->mrq);
+	next_cmd = meson_mmc_get_next_command(cmd);
+	if (next_cmd)
+		meson_mmc_start_cmd(host->mmc, next_cmd);
 	else
-		meson_mmc_start_cmd(host->mmc, data->stop);
+		meson_mmc_request_done(host->mmc, cmd->mrq);
 
 	return IRQ_HANDLED;
 }
@@ -756,6 +768,7 @@ static int meson_mmc_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_div_clk;
 
+	mmc->caps |= MMC_CAP_CMD23;
 	mmc->max_blk_count = CMD_CFG_LENGTH_MASK;
 	mmc->max_req_size = mmc->max_blk_count * mmc->max_blk_size;
 
-- 
2.12.0

Re: [PATCH 1/4] mmc: meson-gx: use bitfield macros

[Kevin Hilman]

Heiner Kallweit <hkallweit1@gmail.com> writes:

> Use GENMASK consistently for all bit masks and switch to using the
> bitfield macros GET_FIELD and PREP_FIELD. This hides parts of the
> complexity of dealing with bit fields.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Very nice.  I should've used these from the beginning.

Some comments below...

> ---
>  drivers/mmc/host/meson-gx-mmc.c | 84 +++++++++++++++++++----------------------
>  1 file changed, 38 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
> index b917765c..cf2ccc67 100644
> --- a/drivers/mmc/host/meson-gx-mmc.c
> +++ b/drivers/mmc/host/meson-gx-mmc.c
> @@ -36,23 +36,25 @@
>  #include <linux/clk-provider.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/interrupt.h>
> +#include <linux/bitfield.h>
>  
>  #define DRIVER_NAME "meson-gx-mmc"
>  
>  #define SD_EMMC_CLOCK 0x0
>  #define   CLK_DIV_SHIFT 0
>  #define   CLK_DIV_WIDTH 6
> -#define   CLK_DIV_MASK 0x3f
> +#define   CLK_DIV_MASK GENMASK(5, 0)
>  #define   CLK_DIV_MAX 63
>  #define   CLK_SRC_SHIFT 6
>  #define   CLK_SRC_WIDTH 2

Shouldn't you get rid of the shift/width here too?

> -#define   CLK_SRC_MASK 0x3
> +#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_PHASE_SHIFT 8
> -#define   CLK_PHASE_MASK 0x3
> +#define   CLK_CORE_PHASE_MASK GENMASK(9, 8)
> +#define   CLK_TX_PHASE_MASK GENMASK(11, 10)
> +#define   CLK_RX_PHASE_MASK GENMASK(13, 12)

The latter 2 aren't used anywhere yet.  I prefer to keep this #defines
to only fields that are actually used.

>  #define   CLK_PHASE_0 0
>  #define   CLK_PHASE_90 1
>  #define   CLK_PHASE_180 2

Otherwise, looks good to me.

Reviewed-by: Kevin Hilman <khilman@baylibre.com>

Kevin

Re: [PATCH 2/4] mmc: meson-gx: use per port interrupt names

[Kevin Hilman]

Heiner Kallweit <hkallweit1@gmail.com> writes:

> So far the driver name is used as interrupt description, therefore in
> /proc/interrupts it's not possible to tell which interrupt belongs to
> which port. Change this by switching to NULL what causes the default
> (device name) to be used. In our case that's the DT node name.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Kevin Hilman <khilman@baylibre.com>

> ---
>  drivers/mmc/host/meson-gx-mmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
> index cf2ccc67..b0a9317b 100644
> --- a/drivers/mmc/host/meson-gx-mmc.c
> +++ b/drivers/mmc/host/meson-gx-mmc.c
> @@ -740,7 +740,7 @@ static int meson_mmc_probe(struct platform_device *pdev)
>  
>  	ret = devm_request_threaded_irq(&pdev->dev, irq, meson_mmc_irq,
>  					meson_mmc_irq_thread, IRQF_SHARED,
> -					DRIVER_NAME, host);
> +					NULL, host);
>  	if (ret)
>  		goto err_div_clk;

Re: [PATCH 3/4] mmc: meson-gx: switch to dynamic timeout values

[Kevin Hilman]

Heiner Kallweit <hkallweit1@gmail.com> writes:

> Currently we use a fixed timeout of 4s for all data transfers. Switch
> to dynamic timeout values by making use of data->timeout_ns.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/mmc/host/meson-gx-mmc.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
> index b0a9317b..e637b3ba 100644
> --- a/drivers/mmc/host/meson-gx-mmc.c
> +++ b/drivers/mmc/host/meson-gx-mmc.c
> @@ -169,6 +169,18 @@ struct sd_emmc_desc {
>  #define CMD_RESP_MASK GENMASK(31, 1)
>  #define CMD_RESP_SRAM BIT(0)
>  
> +static unsigned int meson_mmc_get_timeout(struct mmc_data *data)

nit: add an _msecs to the func name for clarity/readability.

> +{
> +	unsigned int timeout = data->timeout_ns / NSEC_PER_MSEC;
> +
> +	if (!timeout)
> +		return SD_EMMC_CMD_TIMEOUT_DATA;
> +
> +	timeout = roundup_pow_of_two(timeout);
> +
> +	return min(timeout, 32768U); /* max. 2^15 ms */
> +}
> +
>  static int meson_mmc_clk_set(struct meson_host *host, unsigned long clk_rate)
>  {
>  	struct mmc_host *mmc = host->mmc;
> @@ -448,7 +460,7 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd)
>  	if (data) {
>  		cmd_cfg |= CMD_CFG_DATA_IO;
>  		cmd_cfg |= FIELD_PREP(CMD_CFG_TIMEOUT_MASK,
> -				      ilog2(SD_EMMC_CMD_TIMEOUT_DATA));
> +				      ilog2(meson_mmc_get_timeout(data)));
>  
>  		if (data->blocks > 1) {
>  			cmd_cfg |= CMD_CFG_BLOCK_MODE;

Otherwise,

Reviewed-by: Kevin Hilman <khilman@baylibre.com>

Kevin

Re: [PATCH 4/4] mmc: meson-gx: add CMD23 mode

[Kevin Hilman]

Heiner Kallweit <hkallweit1@gmail.com> writes:

> CMD23 mode (use "set block count" command before transferring multiple
> data blocks) typically is more performant as host / card know upfront
> how many data blocks to expect. Therefore add support for this mode to
> the driver.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Kevin Hilman <khilman@baylibre.com>

Kevin

Re: [PATCH 0/4] mmc: meson-gx: smaller functional extensions

[Kevin Hilman]

Heiner Kallweit <hkallweit1@gmail.com> writes:

> This series includes the switch to bitfield macros and a few smaller
> functional extensions.

Again, would be most helpful to to list in the cover letter the
platforms where this was tested, and in the case of perf improvments
like this, describing any changes in performance.

Thanks,

Kevin

Re: [PATCH 1/4] mmc: meson-gx: use bitfield macros

[Heiner Kallweit]

Am 24.03.2017 um 23:52 schrieb Kevin Hilman:
> Heiner Kallweit <hkallweit1@gmail.com> writes:
> 
>> Use GENMASK consistently for all bit masks and switch to using the
>> bitfield macros GET_FIELD and PREP_FIELD. This hides parts of the
>> complexity of dealing with bit fields.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> Very nice.  I should've used these from the beginning.
> 
> Some comments below...
> 
>> ---
>>  drivers/mmc/host/meson-gx-mmc.c | 84 +++++++++++++++++++----------------------
>>  1 file changed, 38 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
>> index b917765c..cf2ccc67 100644
>> --- a/drivers/mmc/host/meson-gx-mmc.c
>> +++ b/drivers/mmc/host/meson-gx-mmc.c
>> @@ -36,23 +36,25 @@
>>  #include <linux/clk-provider.h>
>>  #include <linux/regulator/consumer.h>
>>  #include <linux/interrupt.h>
>> +#include <linux/bitfield.h>
>>  
>>  #define DRIVER_NAME "meson-gx-mmc"
>>  
>>  #define SD_EMMC_CLOCK 0x0
>>  #define   CLK_DIV_SHIFT 0
>>  #define   CLK_DIV_WIDTH 6
>> -#define   CLK_DIV_MASK 0x3f
>> +#define   CLK_DIV_MASK GENMASK(5, 0)
>>  #define   CLK_DIV_MAX 63
>>  #define   CLK_SRC_SHIFT 6
>>  #define   CLK_SRC_WIDTH 2
> 
> Shouldn't you get rid of the shift/width here too?
> 
Just had a look, yes, that's possible too. We just need some
built-in compiler magic to derive these value from the mask
in meson_mmc_clk_init.

>> -#define   CLK_SRC_MASK 0x3
>> +#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_PHASE_SHIFT 8
>> -#define   CLK_PHASE_MASK 0x3
>> +#define   CLK_CORE_PHASE_MASK GENMASK(9, 8)
>> +#define   CLK_TX_PHASE_MASK GENMASK(11, 10)
>> +#define   CLK_RX_PHASE_MASK GENMASK(13, 12)
> 
> The latter 2 aren't used anywhere yet.  I prefer to keep this #defines
> to only fields that are actually used.
> 
Right, the latter two are used in a later patch only. So I will
insert them when needed.

>>  #define   CLK_PHASE_0 0
>>  #define   CLK_PHASE_90 1
>>  #define   CLK_PHASE_180 2
> 
> Otherwise, looks good to me.
> 
> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> 
Thanks for the quick review, Heiner

> Kevin
>