[PATCH 1/5] mmc: uniphier-sd: Factor out register IO

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 1/5] mmc: uniphier-sd: Factor out register IO

Marek Vasut
This patch prepares the driver to support controller(s) with registers
at locations shifted by constant. Pull out the readl()/writel() from
the driver into separate functions, where the adjustment of the register
offset can be easily contained.

Signed-off-by: Marek Vasut <[hidden email]>
Cc: Masahiro Yamada <[hidden email]>
Cc: Jaehoon Chung <[hidden email]>
---
 drivers/mmc/uniphier-sd.c | 115 +++++++++++++++++++++++++---------------------
 1 file changed, 63 insertions(+), 52 deletions(-)

diff --git a/drivers/mmc/uniphier-sd.c b/drivers/mmc/uniphier-sd.c
index e272b14153..70090522bd 100644
--- a/drivers/mmc/uniphier-sd.c
+++ b/drivers/mmc/uniphier-sd.c
@@ -134,6 +134,17 @@ struct uniphier_sd_priv {
 #define UNIPHIER_SD_CAP_DIV1024 BIT(2) /* divisor 1024 is available */
 };
 
+static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
+{
+ return readl(priv->regbase + reg);
+}
+
+static void uniphier_sd_writel(struct uniphier_sd_priv *priv,
+       const u32 val, const u32 reg)
+{
+ writel(val, priv->regbase + reg);
+}
+
 static dma_addr_t __dma_map_single(void *ptr, size_t size,
    enum dma_data_direction dir)
 {
@@ -157,7 +168,7 @@ static void __dma_unmap_single(dma_addr_t addr, size_t size,
 static int uniphier_sd_check_error(struct udevice *dev)
 {
  struct uniphier_sd_priv *priv = dev_get_priv(dev);
- u32 info2 = readl(priv->regbase + UNIPHIER_SD_INFO2);
+ u32 info2 = uniphier_sd_readl(priv, UNIPHIER_SD_INFO2);
 
  if (info2 & UNIPHIER_SD_INFO2_ERR_RTO) {
  /*
@@ -195,7 +206,7 @@ static int uniphier_sd_wait_for_irq(struct udevice *dev, unsigned int reg,
  long wait = 1000000;
  int ret;
 
- while (!(readl(priv->regbase + reg) & flag)) {
+ while (!(uniphier_sd_readl(priv, reg) & flag)) {
  if (wait-- < 0) {
  dev_err(dev, "timeout\n");
  return -ETIMEDOUT;
@@ -227,14 +238,14 @@ static int uniphier_sd_pio_read_one_block(struct udevice *dev, u32 **pbuf,
  * Clear the status flag _before_ read the buffer out because
  * UNIPHIER_SD_INFO2_BRE is edge-triggered, not level-triggered.
  */
- writel(0, priv->regbase + UNIPHIER_SD_INFO2);
+ uniphier_sd_writel(priv, 0, UNIPHIER_SD_INFO2);
 
  if (likely(IS_ALIGNED((unsigned long)*pbuf, 4))) {
  for (i = 0; i < blocksize / 4; i++)
- *(*pbuf)++ = readl(priv->regbase + UNIPHIER_SD_BUF);
+ *(*pbuf)++ = uniphier_sd_readl(priv, UNIPHIER_SD_BUF);
  } else {
  for (i = 0; i < blocksize / 4; i++)
- put_unaligned(readl(priv->regbase + UNIPHIER_SD_BUF),
+ put_unaligned(uniphier_sd_readl(priv, UNIPHIER_SD_BUF),
       (*pbuf)++);
  }
 
@@ -253,15 +264,15 @@ static int uniphier_sd_pio_write_one_block(struct udevice *dev,
  if (ret)
  return ret;
 
- writel(0, priv->regbase + UNIPHIER_SD_INFO2);
+ uniphier_sd_writel(priv, 0, UNIPHIER_SD_INFO2);
 
  if (likely(IS_ALIGNED((unsigned long)*pbuf, 4))) {
  for (i = 0; i < blocksize / 4; i++)
- writel(*(*pbuf)++, priv->regbase + UNIPHIER_SD_BUF);
+ uniphier_sd_writel(priv, *(*pbuf)++, UNIPHIER_SD_BUF);
  } else {
  for (i = 0; i < blocksize / 4; i++)
- writel(get_unaligned((*pbuf)++),
-       priv->regbase + UNIPHIER_SD_BUF);
+ uniphier_sd_writel(priv, get_unaligned((*pbuf)++),
+   UNIPHIER_SD_BUF);
  }
 
  return 0;
@@ -292,22 +303,22 @@ static void uniphier_sd_dma_start(struct uniphier_sd_priv *priv,
 {
  u32 tmp;
 
- writel(0, priv->regbase + UNIPHIER_SD_DMA_INFO1);
- writel(0, priv->regbase + UNIPHIER_SD_DMA_INFO2);
+ uniphier_sd_writel(priv, 0, UNIPHIER_SD_DMA_INFO1);
+ uniphier_sd_writel(priv, 0, UNIPHIER_SD_DMA_INFO2);
 
  /* enable DMA */
- tmp = readl(priv->regbase + UNIPHIER_SD_EXTMODE);
+ tmp = uniphier_sd_readl(priv, UNIPHIER_SD_EXTMODE);
  tmp |= UNIPHIER_SD_EXTMODE_DMA_EN;
- writel(tmp, priv->regbase + UNIPHIER_SD_EXTMODE);
+ uniphier_sd_writel(priv, tmp, UNIPHIER_SD_EXTMODE);
 
- writel(dma_addr & U32_MAX, priv->regbase + UNIPHIER_SD_DMA_ADDR_L);
+ uniphier_sd_writel(priv, dma_addr & U32_MAX, UNIPHIER_SD_DMA_ADDR_L);
 
  /* suppress the warning "right shift count >= width of type" */
  dma_addr >>= min_t(int, 32, 8 * sizeof(dma_addr));
 
- writel(dma_addr & U32_MAX, priv->regbase + UNIPHIER_SD_DMA_ADDR_H);
+ uniphier_sd_writel(priv, dma_addr & U32_MAX, UNIPHIER_SD_DMA_ADDR_H);
 
- writel(UNIPHIER_SD_DMA_CTL_START, priv->regbase + UNIPHIER_SD_DMA_CTL);
+ uniphier_sd_writel(priv, UNIPHIER_SD_DMA_CTL_START, UNIPHIER_SD_DMA_CTL);
 }
 
 static int uniphier_sd_dma_wait_for_irq(struct udevice *dev, u32 flag,
@@ -316,7 +327,7 @@ static int uniphier_sd_dma_wait_for_irq(struct udevice *dev, u32 flag,
  struct uniphier_sd_priv *priv = dev_get_priv(dev);
  long wait = 1000000 + 10 * blocks;
 
- while (!(readl(priv->regbase + UNIPHIER_SD_DMA_INFO1) & flag)) {
+ while (!(uniphier_sd_readl(priv, UNIPHIER_SD_DMA_INFO1) & flag)) {
  if (wait-- < 0) {
  dev_err(dev, "timeout during DMA\n");
  return -ETIMEDOUT;
@@ -325,7 +336,7 @@ static int uniphier_sd_dma_wait_for_irq(struct udevice *dev, u32 flag,
  udelay(10);
  }
 
- if (readl(priv->regbase + UNIPHIER_SD_DMA_INFO2)) {
+ if (uniphier_sd_readl(priv, UNIPHIER_SD_DMA_INFO2)) {
  dev_err(dev, "error during DMA\n");
  return -EIO;
  }
@@ -343,7 +354,7 @@ static int uniphier_sd_dma_xfer(struct udevice *dev, struct mmc_data *data)
  u32 poll_flag, tmp;
  int ret;
 
- tmp = readl(priv->regbase + UNIPHIER_SD_DMA_MODE);
+ tmp = uniphier_sd_readl(priv, UNIPHIER_SD_DMA_MODE);
 
  if (data->flags & MMC_DATA_READ) {
  buf = data->dest;
@@ -357,7 +368,7 @@ static int uniphier_sd_dma_xfer(struct udevice *dev, struct mmc_data *data)
  tmp &= ~UNIPHIER_SD_DMA_MODE_DIR_RD;
  }
 
- writel(tmp, priv->regbase + UNIPHIER_SD_DMA_MODE);
+ uniphier_sd_writel(priv, tmp, UNIPHIER_SD_DMA_MODE);
 
  dma_addr = __dma_map_single(buf, len, dir);
 
@@ -396,27 +407,27 @@ static int uniphier_sd_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
  int ret;
  u32 tmp;
 
- if (readl(priv->regbase + UNIPHIER_SD_INFO2) & UNIPHIER_SD_INFO2_CBSY) {
+ if (uniphier_sd_readl(priv, UNIPHIER_SD_INFO2) & UNIPHIER_SD_INFO2_CBSY) {
  dev_err(dev, "command busy\n");
  return -EBUSY;
  }
 
  /* clear all status flags */
- writel(0, priv->regbase + UNIPHIER_SD_INFO1);
- writel(0, priv->regbase + UNIPHIER_SD_INFO2);
+ uniphier_sd_writel(priv, 0, UNIPHIER_SD_INFO1);
+ uniphier_sd_writel(priv, 0, UNIPHIER_SD_INFO2);
 
  /* disable DMA once */
- tmp = readl(priv->regbase + UNIPHIER_SD_EXTMODE);
+ tmp = uniphier_sd_readl(priv, UNIPHIER_SD_EXTMODE);
  tmp &= ~UNIPHIER_SD_EXTMODE_DMA_EN;
- writel(tmp, priv->regbase + UNIPHIER_SD_EXTMODE);
+ uniphier_sd_writel(priv, tmp, UNIPHIER_SD_EXTMODE);
 
- writel(cmd->cmdarg, priv->regbase + UNIPHIER_SD_ARG);
+ uniphier_sd_writel(priv, cmd->cmdarg, UNIPHIER_SD_ARG);
 
  tmp = cmd->cmdidx;
 
  if (data) {
- writel(data->blocksize, priv->regbase + UNIPHIER_SD_SIZE);
- writel(data->blocks, priv->regbase + UNIPHIER_SD_SECCNT);
+ uniphier_sd_writel(priv, data->blocksize, UNIPHIER_SD_SIZE);
+ uniphier_sd_writel(priv, data->blocks, UNIPHIER_SD_SECCNT);
 
  /* Do not send CMD12 automatically */
  tmp |= UNIPHIER_SD_CMD_NOSTOP | UNIPHIER_SD_CMD_DATA;
@@ -457,7 +468,7 @@ static int uniphier_sd_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
 
  dev_dbg(dev, "sending CMD%d (SD_CMD=%08x, SD_ARG=%08x)\n",
  cmd->cmdidx, tmp, cmd->cmdarg);
- writel(tmp, priv->regbase + UNIPHIER_SD_CMD);
+ uniphier_sd_writel(priv, tmp, UNIPHIER_SD_CMD);
 
  ret = uniphier_sd_wait_for_irq(dev, UNIPHIER_SD_INFO1,
        UNIPHIER_SD_INFO1_RSP);
@@ -465,10 +476,10 @@ static int uniphier_sd_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
  return ret;
 
  if (cmd->resp_type & MMC_RSP_136) {
- u32 rsp_127_104 = readl(priv->regbase + UNIPHIER_SD_RSP76);
- u32 rsp_103_72 = readl(priv->regbase + UNIPHIER_SD_RSP54);
- u32 rsp_71_40 = readl(priv->regbase + UNIPHIER_SD_RSP32);
- u32 rsp_39_8 = readl(priv->regbase + UNIPHIER_SD_RSP10);
+ u32 rsp_127_104 = uniphier_sd_readl(priv, UNIPHIER_SD_RSP76);
+ u32 rsp_103_72 = uniphier_sd_readl(priv, UNIPHIER_SD_RSP54);
+ u32 rsp_71_40 = uniphier_sd_readl(priv, UNIPHIER_SD_RSP32);
+ u32 rsp_39_8 = uniphier_sd_readl(priv, UNIPHIER_SD_RSP10);
 
  cmd->response[0] = ((rsp_127_104 & 0x00ffffff) << 8) |
    ((rsp_103_72  & 0xff000000) >> 24);
@@ -479,7 +490,7 @@ static int uniphier_sd_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
  cmd->response[3] = (rsp_39_8     & 0xffffff)   << 8;
  } else {
  /* bit 39-8 */
- cmd->response[0] = readl(priv->regbase + UNIPHIER_SD_RSP10);
+ cmd->response[0] = uniphier_sd_readl(priv, UNIPHIER_SD_RSP10);
  }
 
  if (data) {
@@ -518,10 +529,10 @@ static int uniphier_sd_set_bus_width(struct uniphier_sd_priv *priv,
  return -EINVAL;
  }
 
- tmp = readl(priv->regbase + UNIPHIER_SD_OPTION);
+ tmp = uniphier_sd_readl(priv, UNIPHIER_SD_OPTION);
  tmp &= ~UNIPHIER_SD_OPTION_WIDTH_MASK;
  tmp |= val;
- writel(tmp, priv->regbase + UNIPHIER_SD_OPTION);
+ uniphier_sd_writel(priv, tmp, UNIPHIER_SD_OPTION);
 
  return 0;
 }
@@ -531,12 +542,12 @@ static void uniphier_sd_set_ddr_mode(struct uniphier_sd_priv *priv,
 {
  u32 tmp;
 
- tmp = readl(priv->regbase + UNIPHIER_SD_IF_MODE);
+ tmp = uniphier_sd_readl(priv, UNIPHIER_SD_IF_MODE);
  if (mmc->ddr_mode)
  tmp |= UNIPHIER_SD_IF_MODE_DDR;
  else
  tmp &= ~UNIPHIER_SD_IF_MODE_DDR;
- writel(tmp, priv->regbase + UNIPHIER_SD_IF_MODE);
+ uniphier_sd_writel(priv, tmp, UNIPHIER_SD_IF_MODE);
 }
 
 static void uniphier_sd_set_clk_rate(struct uniphier_sd_priv *priv,
@@ -573,21 +584,21 @@ static void uniphier_sd_set_clk_rate(struct uniphier_sd_priv *priv,
  else
  val = UNIPHIER_SD_CLKCTL_DIV1024;
 
- tmp = readl(priv->regbase + UNIPHIER_SD_CLKCTL);
+ tmp = uniphier_sd_readl(priv, UNIPHIER_SD_CLKCTL);
  if (tmp & UNIPHIER_SD_CLKCTL_SCLKEN &&
     (tmp & UNIPHIER_SD_CLKCTL_DIV_MASK) == val)
  return;
 
  /* stop the clock before changing its rate to avoid a glitch signal */
  tmp &= ~UNIPHIER_SD_CLKCTL_SCLKEN;
- writel(tmp, priv->regbase + UNIPHIER_SD_CLKCTL);
+ uniphier_sd_writel(priv, tmp, UNIPHIER_SD_CLKCTL);
 
  tmp &= ~UNIPHIER_SD_CLKCTL_DIV_MASK;
  tmp |= val | UNIPHIER_SD_CLKCTL_OFFEN;
- writel(tmp, priv->regbase + UNIPHIER_SD_CLKCTL);
+ uniphier_sd_writel(priv, tmp, UNIPHIER_SD_CLKCTL);
 
  tmp |= UNIPHIER_SD_CLKCTL_SCLKEN;
- writel(tmp, priv->regbase + UNIPHIER_SD_CLKCTL);
+ uniphier_sd_writel(priv, tmp, UNIPHIER_SD_CLKCTL);
 
  udelay(1000);
 }
@@ -617,7 +628,7 @@ static int uniphier_sd_get_cd(struct udevice *dev)
  if (priv->caps & UNIPHIER_SD_CAP_NONREMOVABLE)
  return 1;
 
- return !!(readl(priv->regbase + UNIPHIER_SD_INFO1) &
+ return !!(uniphier_sd_readl(priv, UNIPHIER_SD_INFO1) &
   UNIPHIER_SD_INFO1_CD);
 }
 
@@ -632,28 +643,28 @@ static void uniphier_sd_host_init(struct uniphier_sd_priv *priv)
  u32 tmp;
 
  /* soft reset of the host */
- tmp = readl(priv->regbase + UNIPHIER_SD_SOFT_RST);
+ tmp = uniphier_sd_readl(priv, UNIPHIER_SD_SOFT_RST);
  tmp &= ~UNIPHIER_SD_SOFT_RST_RSTX;
- writel(tmp, priv->regbase + UNIPHIER_SD_SOFT_RST);
+ uniphier_sd_writel(priv, tmp, UNIPHIER_SD_SOFT_RST);
  tmp |= UNIPHIER_SD_SOFT_RST_RSTX;
- writel(tmp, priv->regbase + UNIPHIER_SD_SOFT_RST);
+ uniphier_sd_writel(priv, tmp, UNIPHIER_SD_SOFT_RST);
 
  /* FIXME: implement eMMC hw_reset */
 
- writel(UNIPHIER_SD_STOP_SEC, priv->regbase + UNIPHIER_SD_STOP);
+ uniphier_sd_writel(priv, UNIPHIER_SD_STOP_SEC, UNIPHIER_SD_STOP);
 
  /*
  * Connected to 32bit AXI.
  * This register dropped backward compatibility at version 0x10.
  * Write an appropriate value depending on the IP version.
  */
- writel(priv->version >= 0x10 ? 0x00000101 : 0x00000000,
-       priv->regbase + UNIPHIER_SD_HOST_MODE);
+ uniphier_sd_writel(priv, priv->version >= 0x10 ? 0x00000101 : 0x00000000,
+   UNIPHIER_SD_HOST_MODE);
 
  if (priv->caps & UNIPHIER_SD_CAP_DMA_INTERNAL) {
- tmp = readl(priv->regbase + UNIPHIER_SD_DMA_MODE);
+ tmp = uniphier_sd_readl(priv, UNIPHIER_SD_DMA_MODE);
  tmp |= UNIPHIER_SD_DMA_MODE_ADDR_INC;
- writel(tmp, priv->regbase + UNIPHIER_SD_DMA_MODE);
+ uniphier_sd_writel(priv, tmp, UNIPHIER_SD_DMA_MODE);
  }
 }
 
@@ -724,7 +735,7 @@ static int uniphier_sd_probe(struct udevice *dev)
      NULL))
  priv->caps |= UNIPHIER_SD_CAP_NONREMOVABLE;
 
- priv->version = readl(priv->regbase + UNIPHIER_SD_VERSION) &
+ priv->version = uniphier_sd_readl(priv, UNIPHIER_SD_VERSION) &
  UNIPHIER_SD_VERSION_IP;
  dev_dbg(dev, "version %x\n", priv->version);
  if (priv->version >= 0x10) {
--
2.11.0

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 2/5] mmc: uniphier-sd: Add support for 64bit controller

Marek Vasut
The Renesas RCar Gen3 contains the same controller, originally
Matsushita, yet the register addresses are shifted by 1 to the
left. The whole controller is also 64bit, including the data
FIFOs and RSP registers. This patch adds support for handling
the register IO by shifting the register offset by 1 in the IO
accessor functions.

Signed-off-by: Marek Vasut <[hidden email]>
Cc: Masahiro Yamada <[hidden email]>
Cc: Jaehoon Chung <[hidden email]>
---
 drivers/mmc/uniphier-sd.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/uniphier-sd.c b/drivers/mmc/uniphier-sd.c
index 70090522bd..93a2c6becd 100644
--- a/drivers/mmc/uniphier-sd.c
+++ b/drivers/mmc/uniphier-sd.c
@@ -132,17 +132,24 @@ struct uniphier_sd_priv {
 #define UNIPHIER_SD_CAP_NONREMOVABLE BIT(0) /* Nonremovable e.g. eMMC */
 #define UNIPHIER_SD_CAP_DMA_INTERNAL BIT(1) /* have internal DMA engine */
 #define UNIPHIER_SD_CAP_DIV1024 BIT(2) /* divisor 1024 is available */
+#define UNIPHIER_SD_CAP_64BIT BIT(3) /* Controller is 64bit */
 };
 
 static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
 {
- return readl(priv->regbase + reg);
+ if (priv->caps & UNIPHIER_SD_CAP_64BIT)
+ return readl(priv->regbase + (reg << 1));
+ else
+ return readl(priv->regbase + reg);
 }
 
 static void uniphier_sd_writel(struct uniphier_sd_priv *priv,
        const u32 val, const u32 reg)
 {
- writel(val, priv->regbase + reg);
+ if (priv->caps & UNIPHIER_SD_CAP_64BIT)
+ writel(val, priv->regbase + (reg << 1));
+ else
+ writel(val, priv->regbase + reg);
 }
 
 static dma_addr_t __dma_map_single(void *ptr, size_t size,
--
2.11.0

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 3/5] mmc: uniphier-sd: Add support for 64bit FIFO

Marek Vasut
In reply to this post by Marek Vasut
The Renesas RCar Gen3 contains the same controller, originally
Matsushita. This patch adds support for handling of the 64bit
FIFO on this controller.

Signed-off-by: Marek Vasut <[hidden email]>
Cc: Masahiro Yamada <[hidden email]>
Cc: Jaehoon Chung <[hidden email]>
---
 drivers/mmc/uniphier-sd.c | 84 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 74 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/uniphier-sd.c b/drivers/mmc/uniphier-sd.c
index 93a2c6becd..07436f6ef6 100644
--- a/drivers/mmc/uniphier-sd.c
+++ b/drivers/mmc/uniphier-sd.c
@@ -135,6 +135,23 @@ struct uniphier_sd_priv {
 #define UNIPHIER_SD_CAP_64BIT BIT(3) /* Controller is 64bit */
 };
 
+static u64 uniphier_sd_readq(struct uniphier_sd_priv *priv, const u32 reg)
+{
+ if (priv->caps & UNIPHIER_SD_CAP_64BIT)
+ return readq(priv->regbase + (reg << 1));
+ else
+ return readq(priv->regbase + reg);
+}
+
+static void uniphier_sd_writeq(struct uniphier_sd_priv *priv,
+       const u64 val, const u32 reg)
+{
+ if (priv->caps & UNIPHIER_SD_CAP_64BIT)
+ writeq(val, priv->regbase + (reg << 1));
+ else
+ writeq(val, priv->regbase + reg);
+}
+
 static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
 {
  if (priv->caps & UNIPHIER_SD_CAP_64BIT)
@@ -248,12 +265,37 @@ static int uniphier_sd_pio_read_one_block(struct udevice *dev, u32 **pbuf,
  uniphier_sd_writel(priv, 0, UNIPHIER_SD_INFO2);
 
  if (likely(IS_ALIGNED((unsigned long)*pbuf, 4))) {
- for (i = 0; i < blocksize / 4; i++)
- *(*pbuf)++ = uniphier_sd_readl(priv, UNIPHIER_SD_BUF);
+ if (priv->caps & UNIPHIER_SD_CAP_64BIT) {
+ for (i = 0; i < blocksize / 8; i++) {
+ u64 data;
+ data = uniphier_sd_readq(priv,
+ UNIPHIER_SD_BUF);
+ *(*pbuf)++ = data;
+ *(*pbuf)++ = data >> 32;
+ }
+ } else {
+ for (i = 0; i < blocksize / 4; i++) {
+ u32 data;
+ data = uniphier_sd_readl(priv, UNIPHIER_SD_BUF);
+ *(*pbuf)++ = data;
+ }
+ }
  } else {
- for (i = 0; i < blocksize / 4; i++)
- put_unaligned(uniphier_sd_readl(priv, UNIPHIER_SD_BUF),
-      (*pbuf)++);
+ if (priv->caps & UNIPHIER_SD_CAP_64BIT) {
+ for (i = 0; i < blocksize / 8; i++) {
+ u64 data;
+ data = uniphier_sd_readq(priv,
+ UNIPHIER_SD_BUF);
+ put_unaligned(data, (*pbuf)++);
+ put_unaligned(data >> 32, (*pbuf)++);
+ }
+ } else {
+ for (i = 0; i < blocksize / 4; i++) {
+ u32 data;
+ data = uniphier_sd_readl(priv, UNIPHIER_SD_BUF);
+ put_unaligned(data, (*pbuf)++);
+ }
+ }
  }
 
  return 0;
@@ -274,12 +316,34 @@ static int uniphier_sd_pio_write_one_block(struct udevice *dev,
  uniphier_sd_writel(priv, 0, UNIPHIER_SD_INFO2);
 
  if (likely(IS_ALIGNED((unsigned long)*pbuf, 4))) {
- for (i = 0; i < blocksize / 4; i++)
- uniphier_sd_writel(priv, *(*pbuf)++, UNIPHIER_SD_BUF);
+ if (priv->caps & UNIPHIER_SD_CAP_64BIT) {
+ for (i = 0; i < blocksize / 8; i++) {
+ u64 data = *(*pbuf)++;
+ data |= (u64)*(*pbuf)++ << 32;
+ uniphier_sd_writeq(priv, data,
+   UNIPHIER_SD_BUF);
+ }
+ } else {
+ for (i = 0; i < blocksize / 4; i++) {
+ uniphier_sd_writel(priv, *(*pbuf)++,
+   UNIPHIER_SD_BUF);
+ }
+ }
  } else {
- for (i = 0; i < blocksize / 4; i++)
- uniphier_sd_writel(priv, get_unaligned((*pbuf)++),
-   UNIPHIER_SD_BUF);
+ if (priv->caps & UNIPHIER_SD_CAP_64BIT) {
+ for (i = 0; i < blocksize / 8; i++) {
+ u64 data = get_unaligned((*pbuf)++);
+ data |= (u64)get_unaligned((*pbuf)++) << 32;
+ uniphier_sd_writeq(priv, data,
+   UNIPHIER_SD_BUF);
+ }
+ } else {
+ for (i = 0; i < blocksize / 4; i++) {
+ u32 data = get_unaligned((*pbuf)++);
+ uniphier_sd_writel(priv, data,
+   UNIPHIER_SD_BUF);
+ }
+ }
  }
 
  return 0;
--
2.11.0

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 4/5] mmc: uniphier-sd: Add support for quirks

Marek Vasut
In reply to this post by Marek Vasut
Check if the OF match has any associated data and if so, use those
data as the controller quirks, otherwise fallback to the old method
of reading the controller version register to figure out the quirks.
This allows us to supply controller quirks on controllers which ie.
do not have version register.

Signed-off-by: Marek Vasut <[hidden email]>
Cc: Masahiro Yamada <[hidden email]>
Cc: Jaehoon Chung <[hidden email]>
---
 drivers/mmc/uniphier-sd.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/uniphier-sd.c b/drivers/mmc/uniphier-sd.c
index 07436f6ef6..c22c64cdfb 100644
--- a/drivers/mmc/uniphier-sd.c
+++ b/drivers/mmc/uniphier-sd.c
@@ -751,6 +751,7 @@ static int uniphier_sd_probe(struct udevice *dev)
  struct uniphier_sd_plat *plat = dev_get_platdata(dev);
  struct uniphier_sd_priv *priv = dev_get_priv(dev);
  struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
+ const u32 quirks = dev_get_driver_data(dev);
  fdt_addr_t base;
  struct clk clk;
  int ret;
@@ -802,18 +803,22 @@ static int uniphier_sd_probe(struct udevice *dev)
  return -EINVAL;
  }
 
+ if (quirks) {
+ priv->caps = quirks;
+ } else {
+ priv->version = uniphier_sd_readl(priv, UNIPHIER_SD_VERSION) &
+ UNIPHIER_SD_VERSION_IP;
+ dev_dbg(dev, "version %x\n", priv->version);
+ if (priv->version >= 0x10) {
+ priv->caps |= UNIPHIER_SD_CAP_DMA_INTERNAL;
+ priv->caps |= UNIPHIER_SD_CAP_DIV1024;
+ }
+ }
+
  if (fdt_get_property(gd->fdt_blob, dev_of_offset(dev), "non-removable",
      NULL))
  priv->caps |= UNIPHIER_SD_CAP_NONREMOVABLE;
 
- priv->version = uniphier_sd_readl(priv, UNIPHIER_SD_VERSION) &
- UNIPHIER_SD_VERSION_IP;
- dev_dbg(dev, "version %x\n", priv->version);
- if (priv->version >= 0x10) {
- priv->caps |= UNIPHIER_SD_CAP_DMA_INTERNAL;
- priv->caps |= UNIPHIER_SD_CAP_DIV1024;
- }
-
  uniphier_sd_host_init(priv);
 
  plat->cfg.voltages = MMC_VDD_165_195 | MMC_VDD_32_33 | MMC_VDD_33_34;
@@ -828,7 +833,7 @@ static int uniphier_sd_probe(struct udevice *dev)
 }
 
 static const struct udevice_id uniphier_sd_match[] = {
- { .compatible = "socionext,uniphier-sdhc" },
+ { .compatible = "socionext,uniphier-sdhc", .data = 0 },
  { /* sentinel */ }
 };
 
--
2.11.0

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 5/5] mmc: uniphier-sd: Add support for R8A7795 and R7A7796

Marek Vasut
In reply to this post by Marek Vasut
Add OF match entries and quirks for Renesas RCar Gen3 controllers
into the driver. The IP this driver handles is in fact Matsushita
one and in used both in Socionext and Renesas chips.

Signed-off-by: Marek Vasut <[hidden email]>
Cc: Masahiro Yamada <[hidden email]>
Cc: Jaehoon Chung <[hidden email]>
---
 drivers/mmc/Kconfig       | 7 ++++---
 drivers/mmc/uniphier-sd.c | 2 ++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
index 82b8d75686..3eed8e66e6 100644
--- a/drivers/mmc/Kconfig
+++ b/drivers/mmc/Kconfig
@@ -161,12 +161,13 @@ config SH_SDHI
   Support for the on-chip SDHI host controller on SuperH/Renesas ARM SoCs platform
 
 config MMC_UNIPHIER
- bool "UniPhier SD/MMC Host Controller support"
- depends on ARCH_UNIPHIER
+ bool "UniPhier/RCar SD/MMC Host Controller support"
+ depends on ARCH_UNIPHIER || ARCH_RMOBILE
  depends on BLK && DM_MMC_OPS
  depends on OF_CONTROL
  help
-  This selects support for the SD/MMC Host Controller on UniPhier SoCs.
+  This selects support for the Matsushita SD/MMC Host Controller on
+  SocioNext UniPhier and Renesas RCar SoCs.
 
 config MMC_SANDBOX
  bool "Sandbox MMC support"
diff --git a/drivers/mmc/uniphier-sd.c b/drivers/mmc/uniphier-sd.c
index c22c64cdfb..d9f7ea52b2 100644
--- a/drivers/mmc/uniphier-sd.c
+++ b/drivers/mmc/uniphier-sd.c
@@ -833,6 +833,8 @@ static int uniphier_sd_probe(struct udevice *dev)
 }
 
 static const struct udevice_id uniphier_sd_match[] = {
+ { .compatible = "renesas,sdhi-r8a7795", .data = UNIPHIER_SD_CAP_64BIT },
+ { .compatible = "renesas,sdhi-r8a7796", .data = UNIPHIER_SD_CAP_64BIT },
  { .compatible = "socionext,uniphier-sdhc", .data = 0 },
  { /* sentinel */ }
 };
--
2.11.0

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/5] mmc: uniphier-sd: Factor out register IO

Masahiro Yamada-2
In reply to this post by Marek Vasut
Hi Marek,

Very sorry for my late reply.

I agree we should share the driver
to avoid code duplication.

I tested the series on my board and working.
Basically, I'd say "go for it".

A little minor comments below.


2017-07-22 6:24 GMT+09:00 Marek Vasut <[hidden email]>:

> This patch prepares the driver to support controller(s) with registers
> at locations shifted by constant. Pull out the readl()/writel() from
> the driver into separate functions, where the adjustment of the register
> offset can be easily contained.
>
> Signed-off-by: Marek Vasut <[hidden email]>
> Cc: Masahiro Yamada <[hidden email]>
> Cc: Jaehoon Chung <[hidden email]>
> ---
>  drivers/mmc/uniphier-sd.c | 115 +++++++++++++++++++++++++---------------------
>  1 file changed, 63 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/mmc/uniphier-sd.c b/drivers/mmc/uniphier-sd.c
> index e272b14153..70090522bd 100644
> --- a/drivers/mmc/uniphier-sd.c
> +++ b/drivers/mmc/uniphier-sd.c
> @@ -134,6 +134,17 @@ struct uniphier_sd_priv {
>  #define UNIPHIER_SD_CAP_DIV1024                BIT(2)  /* divisor 1024 is available */
>  };
>
> +static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)

"const" is unneeded here.

Also, could you use "unsigned int" or "int" for reg?




> +{
> +       return readl(priv->regbase + reg);
> +}
> +
> +static void uniphier_sd_writel(struct uniphier_sd_priv *priv,
> +                              const u32 val, const u32 reg)

Same here.  Please drop "const".

Please use "unsigned int" or "int" for reg

It is OK to use u32 for val.




--
Best Regards
Masahiro Yamada
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 3/5] mmc: uniphier-sd: Add support for 64bit FIFO

Masahiro Yamada-2
In reply to this post by Marek Vasut
Hi Marek,


2017-07-22 6:24 GMT+09:00 Marek Vasut <[hidden email]>:

> The Renesas RCar Gen3 contains the same controller, originally
> Matsushita. This patch adds support for handling of the 64bit
> FIFO on this controller.
>
> Signed-off-by: Marek Vasut <[hidden email]>
> Cc: Masahiro Yamada <[hidden email]>
> Cc: Jaehoon Chung <[hidden email]>
> ---
>  drivers/mmc/uniphier-sd.c | 84 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 74 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mmc/uniphier-sd.c b/drivers/mmc/uniphier-sd.c
> index 93a2c6becd..07436f6ef6 100644
> --- a/drivers/mmc/uniphier-sd.c
> +++ b/drivers/mmc/uniphier-sd.c
> @@ -135,6 +135,23 @@ struct uniphier_sd_priv {
>  #define UNIPHIER_SD_CAP_64BIT          BIT(3)  /* Controller is 64bit */
>  };
>
> +static u64 uniphier_sd_readq(struct uniphier_sd_priv *priv, const u32 reg)
> +{
> +       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
> +               return readq(priv->regbase + (reg << 1));
> +       else
> +               return readq(priv->regbase + reg);
> +}
> +
> +static void uniphier_sd_writeq(struct uniphier_sd_priv *priv,
> +                              const u64 val, const u32 reg)
> +{
> +       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
> +               writeq(val, priv->regbase + (reg << 1));
> +       else
> +               writeq(val, priv->regbase + reg);
> +}
> +
>  static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>  {
>         if (priv->caps & UNIPHIER_SD_CAP_64BIT)
> @@ -248,12 +265,37 @@ static int uniphier_sd_pio_read_one_block(struct udevice *dev, u32 **pbuf,


This 'u32 **pbuf' was optimized for my case.
This is pointless for 64bit platform.

you can change it 'char **pbuf'



>         uniphier_sd_writel(priv, 0, UNIPHIER_SD_INFO2);
>
>         if (likely(IS_ALIGNED((unsigned long)*pbuf, 4))) {
> -               for (i = 0; i < blocksize / 4; i++)
> -                       *(*pbuf)++ = uniphier_sd_readl(priv, UNIPHIER_SD_BUF);
> +               if (priv->caps & UNIPHIER_SD_CAP_64BIT) {
> +                       for (i = 0; i < blocksize / 8; i++) {
> +                               u64 data;
> +                               data = uniphier_sd_readq(priv,
> +                                                        UNIPHIER_SD_BUF);
> +                               *(*pbuf)++ = data;
> +                               *(*pbuf)++ = data >> 32;


I do not think this code is efficient for your 64 bit platform.

Perhaps, you can do as follows:


Prepare a 64bit pointer.

      u64 **pbuf64 = (u64 **)pbuf;

Then,

*(*pbuf64)++ = uniphier_sd_readq(priv, UNIPHIER_SD_BUF);



In this case, please be careful for

if (likely(IS_ALIGNED((unsigned long)*pbuf, 4))) {

You will need 8 byte alignment.


--
Best Regards
Masahiro Yamada
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/5] mmc: uniphier-sd: Factor out register IO

Marek Vasut
In reply to this post by Masahiro Yamada-2
On 08/03/2017 02:36 PM, Masahiro Yamada wrote:
> Hi Marek,

Hi,

[...]

>> +static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>
> "const" is unneeded here.

Why? The function should not modify reg , so it is const.

> Also, could you use "unsigned int" or "int" for reg?

Why?

>> +{
>> +       return readl(priv->regbase + reg);
>> +}
>> +
>> +static void uniphier_sd_writel(struct uniphier_sd_priv *priv,
>> +                              const u32 val, const u32 reg)
>
> Same here.  Please drop "const".
>
> Please use "unsigned int" or "int" for reg
>
> It is OK to use u32 for val.
>
>
>
>


--
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/5] mmc: uniphier-sd: Factor out register IO

Masahiro Yamada-2
Hi Marek,


2017-08-06 4:23 GMT+09:00 Marek Vasut <[hidden email]>:

> On 08/03/2017 02:36 PM, Masahiro Yamada wrote:
>> Hi Marek,
>
> Hi,
>
> [...]
>
>>> +static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>>
>> "const" is unneeded here.
>
> Why? The function should not modify reg , so it is const.


Because "const" is useless here.

The "reg" is not a pointer, so it is obvious
that there is no impact to the callers.



Moreover, whether "reg" is constant or not
depends on how you implement the function.


If you force "const" to the argument, the only choice for the implementation
will be as follows:



static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
{
      if (priv->caps & UNIPHIER_SD_CAP_64BIT)
             return readl(priv->regbase + (reg << 1));
      else
             return readl(priv->regbase + reg);
}



If you want to implement the function as follows, you need to drop "const".

static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, u32 reg)
{
      if (priv->caps & UNIPHIER_SD_CAP_64BIT)
              reg <<= 1;

      return readl(priv->regbase + reg);
}





>> Also, could you use "unsigned int" or "int" for reg?
>
> Why?


Because "unsigned int" or "int" is more natural.

No reason to use a fixed width variable for the address offset.






--
Best Regards
Masahiro Yamada
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/5] mmc: uniphier-sd: Factor out register IO

Marek Vasut
On 08/07/2017 04:30 AM, Masahiro Yamada wrote:
> Hi Marek,

Hi Masahiro,

This is gonna be a great discussion, let's wrestle about consts and ints :-)

> 2017-08-06 4:23 GMT+09:00 Marek Vasut <[hidden email]>:
>> On 08/03/2017 02:36 PM, Masahiro Yamada wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>> [...]
>>
>>>> +static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>>>
>>> "const" is unneeded here.
>>
>> Why? The function should not modify reg , so it is const.
>
>
> Because "const" is useless here.
>
> The "reg" is not a pointer, so it is obvious
> that there is no impact to the callers.
>
>
>
> Moreover, whether "reg" is constant or not
> depends on how you implement the function.
>
>
> If you force "const" to the argument, the only choice for the implementation
> will be as follows:
>
>
>
> static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
> {
>       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
>              return readl(priv->regbase + (reg << 1));
>       else
>              return readl(priv->regbase + reg);
> }
>
>
>
> If you want to implement the function as follows, you need to drop "const".
>
> static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, u32 reg)
> {
>       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
>               reg <<= 1;
>
>       return readl(priv->regbase + reg);
> }

My argument would be that the const prevents you from accidentally
modifying the $reg inside the function.

>>> Also, could you use "unsigned int" or "int" for reg?
>>
>> Why?
>
>
> Because "unsigned int" or "int" is more natural.
>
> No reason to use a fixed width variable for the address offset.

You're loosing the benefit of fixed width with using unsigned int though?

--
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/5] mmc: uniphier-sd: Factor out register IO

Masahiro Yamada-2
Hi.


2017-08-07 17:30 GMT+09:00 Marek Vasut <[hidden email]>:

> On 08/07/2017 04:30 AM, Masahiro Yamada wrote:
>> Hi Marek,
>
> Hi Masahiro,
>
> This is gonna be a great discussion, let's wrestle about consts and ints :-)
>
>> 2017-08-06 4:23 GMT+09:00 Marek Vasut <[hidden email]>:
>>> On 08/03/2017 02:36 PM, Masahiro Yamada wrote:
>>>> Hi Marek,
>>>
>>> Hi,
>>>
>>> [...]
>>>
>>>>> +static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>>>>
>>>> "const" is unneeded here.
>>>
>>> Why? The function should not modify reg , so it is const.
>>
>>
>> Because "const" is useless here.
>>
>> The "reg" is not a pointer, so it is obvious
>> that there is no impact to the callers.
>>
>>
>>
>> Moreover, whether "reg" is constant or not
>> depends on how you implement the function.
>>
>>
>> If you force "const" to the argument, the only choice for the implementation
>> will be as follows:
>>
>>
>>
>> static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>> {
>>       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
>>              return readl(priv->regbase + (reg << 1));
>>       else
>>              return readl(priv->regbase + reg);
>> }
>>
>>
>>
>> If you want to implement the function as follows, you need to drop "const".
>>
>> static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, u32 reg)
>> {
>>       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
>>               reg <<= 1;
>>
>>       return readl(priv->regbase + reg);
>> }
>
> My argument would be that the const prevents you from accidentally
> modifying the $reg inside the function.


No.
The arguments of a functions are local variables.
No impact on the outside of the scope of the function.

If you accidentally modify a variable where it should not,
it is a bug.  Just fix it.




If you want to wrestle more, please do it in LKML
by adding around "const".

For example,

drivers/base/regmap/regmap.c:
int regmap_write(struct regmap *map, unsigned int reg, unsigned int val)

arch/arm/include/asm/io.h:
static inline void __raw_writel(u32 val, volatile void __iomem *addr)





--
Best Regards
Masahiro Yamada
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/5] mmc: uniphier-sd: Factor out register IO

Marek Vasut
On 08/10/2017 09:49 AM, Masahiro Yamada wrote:

> Hi.
>
>
> 2017-08-07 17:30 GMT+09:00 Marek Vasut <[hidden email]>:
>> On 08/07/2017 04:30 AM, Masahiro Yamada wrote:
>>> Hi Marek,
>>
>> Hi Masahiro,
>>
>> This is gonna be a great discussion, let's wrestle about consts and ints :-)
>>
>>> 2017-08-06 4:23 GMT+09:00 Marek Vasut <[hidden email]>:
>>>> On 08/03/2017 02:36 PM, Masahiro Yamada wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi,
>>>>
>>>> [...]
>>>>
>>>>>> +static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>>>>>
>>>>> "const" is unneeded here.
>>>>
>>>> Why? The function should not modify reg , so it is const.
>>>
>>>
>>> Because "const" is useless here.
>>>
>>> The "reg" is not a pointer, so it is obvious
>>> that there is no impact to the callers.
>>>
>>>
>>>
>>> Moreover, whether "reg" is constant or not
>>> depends on how you implement the function.
>>>
>>>
>>> If you force "const" to the argument, the only choice for the implementation
>>> will be as follows:
>>>
>>>
>>>
>>> static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>>> {
>>>       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
>>>              return readl(priv->regbase + (reg << 1));
>>>       else
>>>              return readl(priv->regbase + reg);
>>> }
>>>
>>>
>>>
>>> If you want to implement the function as follows, you need to drop "const".
>>>
>>> static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, u32 reg)
>>> {
>>>       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
>>>               reg <<= 1;
>>>
>>>       return readl(priv->regbase + reg);
>>> }
>>
>> My argument would be that the const prevents you from accidentally
>> modifying the $reg inside the function.
>
>
> No.
> The arguments of a functions are local variables.
> No impact on the outside of the scope of the function.

I'm not arguing about that.

> If you accidentally modify a variable where it should not,
> it is a bug.  Just fix it.

If it's const, compiler will warn the user he's doing something stupid.

> If you want to wrestle more, please do it in LKML
> by adding around "const".
>
> For example,
>
> drivers/base/regmap/regmap.c:
> int regmap_write(struct regmap *map, unsigned int reg, unsigned int val)
>
> arch/arm/include/asm/io.h:
> static inline void __raw_writel(u32 val, volatile void __iomem *addr)

Well, there were some cocci patches adding const recently :)

--
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/5] mmc: uniphier-sd: Factor out register IO

Jaehoon Chung
On 08/13/2017 01:55 AM, Marek Vasut wrote:

> On 08/10/2017 09:49 AM, Masahiro Yamada wrote:
>> Hi.
>>
>>
>> 2017-08-07 17:30 GMT+09:00 Marek Vasut <[hidden email]>:
>>> On 08/07/2017 04:30 AM, Masahiro Yamada wrote:
>>>> Hi Marek,
>>>
>>> Hi Masahiro,
>>>
>>> This is gonna be a great discussion, let's wrestle about consts and ints :-)
>>>
>>>> 2017-08-06 4:23 GMT+09:00 Marek Vasut <[hidden email]>:
>>>>> On 08/03/2017 02:36 PM, Masahiro Yamada wrote:
>>>>>> Hi Marek,
>>>>>
>>>>> Hi,
>>>>>
>>>>> [...]
>>>>>
>>>>>>> +static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>>>>>>
>>>>>> "const" is unneeded here.
>>>>>
>>>>> Why? The function should not modify reg , so it is const.
>>>>
>>>>
>>>> Because "const" is useless here.
>>>>
>>>> The "reg" is not a pointer, so it is obvious
>>>> that there is no impact to the callers.
>>>>
>>>>
>>>>
>>>> Moreover, whether "reg" is constant or not
>>>> depends on how you implement the function.
>>>>
>>>>
>>>> If you force "const" to the argument, the only choice for the implementation
>>>> will be as follows:
>>>>
>>>>
>>>>
>>>> static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>>>> {
>>>>       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
>>>>              return readl(priv->regbase + (reg << 1));
>>>>       else
>>>>              return readl(priv->regbase + reg);
>>>> }
>>>>
>>>>
>>>>
>>>> If you want to implement the function as follows, you need to drop "const".
>>>>
>>>> static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, u32 reg)
>>>> {
>>>>       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
>>>>               reg <<= 1;
>>>>
>>>>       return readl(priv->regbase + reg);
>>>> }
>>>
>>> My argument would be that the const prevents you from accidentally
>>> modifying the $reg inside the function.

Is there any case about modifying the regs value in this function?
Well..I think that it makes sense about both. (Masahiro and Marek opinion)

But There is nothing wrong to prevent from accidentally.

Best Regards,
Jaehoon Chung

>>
>>
>> No.
>> The arguments of a functions are local variables.
>> No impact on the outside of the scope of the function.
>
> I'm not arguing about that.
>
>> If you accidentally modify a variable where it should not,
>> it is a bug.  Just fix it.
>
> If it's const, compiler will warn the user he's doing something stupid.
>
>> If you want to wrestle more, please do it in LKML
>> by adding around "const".
>>
>> For example,
>>
>> drivers/base/regmap/regmap.c:
>> int regmap_write(struct regmap *map, unsigned int reg, unsigned int val)
>>
>> arch/arm/include/asm/io.h:
>> static inline void __raw_writel(u32 val, volatile void __iomem *addr)
>
> Well, there were some cocci patches adding const recently :)
>

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/5] mmc: uniphier-sd: Factor out register IO

Jaehoon Chung
In reply to this post by Marek Vasut
Hi,

On 07/22/2017 06:24 AM, Marek Vasut wrote:
> This patch prepares the driver to support controller(s) with registers
> at locations shifted by constant. Pull out the readl()/writel() from
> the driver into separate functions, where the adjustment of the register
> offset can be easily contained.
>
> Signed-off-by: Marek Vasut <[hidden email]>
> Cc: Masahiro Yamada <[hidden email]>
> Cc: Jaehoon Chung <[hidden email]>

I will wait for Masahiro's acked-tag or reviewing..

Best Regards,
Jaehoon Chung

> ---
>  drivers/mmc/uniphier-sd.c | 115 +++++++++++++++++++++++++---------------------
>  1 file changed, 63 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/mmc/uniphier-sd.c b/drivers/mmc/uniphier-sd.c
> index e272b14153..70090522bd 100644
> --- a/drivers/mmc/uniphier-sd.c
> +++ b/drivers/mmc/uniphier-sd.c
> @@ -134,6 +134,17 @@ struct uniphier_sd_priv {
>  #define UNIPHIER_SD_CAP_DIV1024 BIT(2) /* divisor 1024 is available */
>  };
>  
> +static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
> +{
> + return readl(priv->regbase + reg);
> +}
> +
> +static void uniphier_sd_writel(struct uniphier_sd_priv *priv,
> +       const u32 val, const u32 reg)
> +{
> + writel(val, priv->regbase + reg);
> +}
> +
>  static dma_addr_t __dma_map_single(void *ptr, size_t size,
>     enum dma_data_direction dir)
>  {
> @@ -157,7 +168,7 @@ static void __dma_unmap_single(dma_addr_t addr, size_t size,
>  static int uniphier_sd_check_error(struct udevice *dev)
>  {
>   struct uniphier_sd_priv *priv = dev_get_priv(dev);
> - u32 info2 = readl(priv->regbase + UNIPHIER_SD_INFO2);
> + u32 info2 = uniphier_sd_readl(priv, UNIPHIER_SD_INFO2);
>  
>   if (info2 & UNIPHIER_SD_INFO2_ERR_RTO) {
>   /*
> @@ -195,7 +206,7 @@ static int uniphier_sd_wait_for_irq(struct udevice *dev, unsigned int reg,
>   long wait = 1000000;
>   int ret;
>  
> - while (!(readl(priv->regbase + reg) & flag)) {
> + while (!(uniphier_sd_readl(priv, reg) & flag)) {
>   if (wait-- < 0) {
>   dev_err(dev, "timeout\n");
>   return -ETIMEDOUT;
> @@ -227,14 +238,14 @@ static int uniphier_sd_pio_read_one_block(struct udevice *dev, u32 **pbuf,
>   * Clear the status flag _before_ read the buffer out because
>   * UNIPHIER_SD_INFO2_BRE is edge-triggered, not level-triggered.
>   */
> - writel(0, priv->regbase + UNIPHIER_SD_INFO2);
> + uniphier_sd_writel(priv, 0, UNIPHIER_SD_INFO2);
>  
>   if (likely(IS_ALIGNED((unsigned long)*pbuf, 4))) {
>   for (i = 0; i < blocksize / 4; i++)
> - *(*pbuf)++ = readl(priv->regbase + UNIPHIER_SD_BUF);
> + *(*pbuf)++ = uniphier_sd_readl(priv, UNIPHIER_SD_BUF);
>   } else {
>   for (i = 0; i < blocksize / 4; i++)
> - put_unaligned(readl(priv->regbase + UNIPHIER_SD_BUF),
> + put_unaligned(uniphier_sd_readl(priv, UNIPHIER_SD_BUF),
>        (*pbuf)++);
>   }
>  
> @@ -253,15 +264,15 @@ static int uniphier_sd_pio_write_one_block(struct udevice *dev,
>   if (ret)
>   return ret;
>  
> - writel(0, priv->regbase + UNIPHIER_SD_INFO2);
> + uniphier_sd_writel(priv, 0, UNIPHIER_SD_INFO2);
>  
>   if (likely(IS_ALIGNED((unsigned long)*pbuf, 4))) {
>   for (i = 0; i < blocksize / 4; i++)
> - writel(*(*pbuf)++, priv->regbase + UNIPHIER_SD_BUF);
> + uniphier_sd_writel(priv, *(*pbuf)++, UNIPHIER_SD_BUF);
>   } else {
>   for (i = 0; i < blocksize / 4; i++)
> - writel(get_unaligned((*pbuf)++),
> -       priv->regbase + UNIPHIER_SD_BUF);
> + uniphier_sd_writel(priv, get_unaligned((*pbuf)++),
> +   UNIPHIER_SD_BUF);
>   }
>  
>   return 0;
> @@ -292,22 +303,22 @@ static void uniphier_sd_dma_start(struct uniphier_sd_priv *priv,
>  {
>   u32 tmp;
>  
> - writel(0, priv->regbase + UNIPHIER_SD_DMA_INFO1);
> - writel(0, priv->regbase + UNIPHIER_SD_DMA_INFO2);
> + uniphier_sd_writel(priv, 0, UNIPHIER_SD_DMA_INFO1);
> + uniphier_sd_writel(priv, 0, UNIPHIER_SD_DMA_INFO2);
>  
>   /* enable DMA */
> - tmp = readl(priv->regbase + UNIPHIER_SD_EXTMODE);
> + tmp = uniphier_sd_readl(priv, UNIPHIER_SD_EXTMODE);
>   tmp |= UNIPHIER_SD_EXTMODE_DMA_EN;
> - writel(tmp, priv->regbase + UNIPHIER_SD_EXTMODE);
> + uniphier_sd_writel(priv, tmp, UNIPHIER_SD_EXTMODE);
>  
> - writel(dma_addr & U32_MAX, priv->regbase + UNIPHIER_SD_DMA_ADDR_L);
> + uniphier_sd_writel(priv, dma_addr & U32_MAX, UNIPHIER_SD_DMA_ADDR_L);
>  
>   /* suppress the warning "right shift count >= width of type" */
>   dma_addr >>= min_t(int, 32, 8 * sizeof(dma_addr));
>  
> - writel(dma_addr & U32_MAX, priv->regbase + UNIPHIER_SD_DMA_ADDR_H);
> + uniphier_sd_writel(priv, dma_addr & U32_MAX, UNIPHIER_SD_DMA_ADDR_H);
>  
> - writel(UNIPHIER_SD_DMA_CTL_START, priv->regbase + UNIPHIER_SD_DMA_CTL);
> + uniphier_sd_writel(priv, UNIPHIER_SD_DMA_CTL_START, UNIPHIER_SD_DMA_CTL);
>  }
>  
>  static int uniphier_sd_dma_wait_for_irq(struct udevice *dev, u32 flag,
> @@ -316,7 +327,7 @@ static int uniphier_sd_dma_wait_for_irq(struct udevice *dev, u32 flag,
>   struct uniphier_sd_priv *priv = dev_get_priv(dev);
>   long wait = 1000000 + 10 * blocks;
>  
> - while (!(readl(priv->regbase + UNIPHIER_SD_DMA_INFO1) & flag)) {
> + while (!(uniphier_sd_readl(priv, UNIPHIER_SD_DMA_INFO1) & flag)) {
>   if (wait-- < 0) {
>   dev_err(dev, "timeout during DMA\n");
>   return -ETIMEDOUT;
> @@ -325,7 +336,7 @@ static int uniphier_sd_dma_wait_for_irq(struct udevice *dev, u32 flag,
>   udelay(10);
>   }
>  
> - if (readl(priv->regbase + UNIPHIER_SD_DMA_INFO2)) {
> + if (uniphier_sd_readl(priv, UNIPHIER_SD_DMA_INFO2)) {
>   dev_err(dev, "error during DMA\n");
>   return -EIO;
>   }
> @@ -343,7 +354,7 @@ static int uniphier_sd_dma_xfer(struct udevice *dev, struct mmc_data *data)
>   u32 poll_flag, tmp;
>   int ret;
>  
> - tmp = readl(priv->regbase + UNIPHIER_SD_DMA_MODE);
> + tmp = uniphier_sd_readl(priv, UNIPHIER_SD_DMA_MODE);
>  
>   if (data->flags & MMC_DATA_READ) {
>   buf = data->dest;
> @@ -357,7 +368,7 @@ static int uniphier_sd_dma_xfer(struct udevice *dev, struct mmc_data *data)
>   tmp &= ~UNIPHIER_SD_DMA_MODE_DIR_RD;
>   }
>  
> - writel(tmp, priv->regbase + UNIPHIER_SD_DMA_MODE);
> + uniphier_sd_writel(priv, tmp, UNIPHIER_SD_DMA_MODE);
>  
>   dma_addr = __dma_map_single(buf, len, dir);
>  
> @@ -396,27 +407,27 @@ static int uniphier_sd_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
>   int ret;
>   u32 tmp;
>  
> - if (readl(priv->regbase + UNIPHIER_SD_INFO2) & UNIPHIER_SD_INFO2_CBSY) {
> + if (uniphier_sd_readl(priv, UNIPHIER_SD_INFO2) & UNIPHIER_SD_INFO2_CBSY) {
>   dev_err(dev, "command busy\n");
>   return -EBUSY;
>   }
>  
>   /* clear all status flags */
> - writel(0, priv->regbase + UNIPHIER_SD_INFO1);
> - writel(0, priv->regbase + UNIPHIER_SD_INFO2);
> + uniphier_sd_writel(priv, 0, UNIPHIER_SD_INFO1);
> + uniphier_sd_writel(priv, 0, UNIPHIER_SD_INFO2);
>  
>   /* disable DMA once */
> - tmp = readl(priv->regbase + UNIPHIER_SD_EXTMODE);
> + tmp = uniphier_sd_readl(priv, UNIPHIER_SD_EXTMODE);
>   tmp &= ~UNIPHIER_SD_EXTMODE_DMA_EN;
> - writel(tmp, priv->regbase + UNIPHIER_SD_EXTMODE);
> + uniphier_sd_writel(priv, tmp, UNIPHIER_SD_EXTMODE);
>  
> - writel(cmd->cmdarg, priv->regbase + UNIPHIER_SD_ARG);
> + uniphier_sd_writel(priv, cmd->cmdarg, UNIPHIER_SD_ARG);
>  
>   tmp = cmd->cmdidx;
>  
>   if (data) {
> - writel(data->blocksize, priv->regbase + UNIPHIER_SD_SIZE);
> - writel(data->blocks, priv->regbase + UNIPHIER_SD_SECCNT);
> + uniphier_sd_writel(priv, data->blocksize, UNIPHIER_SD_SIZE);
> + uniphier_sd_writel(priv, data->blocks, UNIPHIER_SD_SECCNT);
>  
>   /* Do not send CMD12 automatically */
>   tmp |= UNIPHIER_SD_CMD_NOSTOP | UNIPHIER_SD_CMD_DATA;
> @@ -457,7 +468,7 @@ static int uniphier_sd_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
>  
>   dev_dbg(dev, "sending CMD%d (SD_CMD=%08x, SD_ARG=%08x)\n",
>   cmd->cmdidx, tmp, cmd->cmdarg);
> - writel(tmp, priv->regbase + UNIPHIER_SD_CMD);
> + uniphier_sd_writel(priv, tmp, UNIPHIER_SD_CMD);
>  
>   ret = uniphier_sd_wait_for_irq(dev, UNIPHIER_SD_INFO1,
>         UNIPHIER_SD_INFO1_RSP);
> @@ -465,10 +476,10 @@ static int uniphier_sd_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
>   return ret;
>  
>   if (cmd->resp_type & MMC_RSP_136) {
> - u32 rsp_127_104 = readl(priv->regbase + UNIPHIER_SD_RSP76);
> - u32 rsp_103_72 = readl(priv->regbase + UNIPHIER_SD_RSP54);
> - u32 rsp_71_40 = readl(priv->regbase + UNIPHIER_SD_RSP32);
> - u32 rsp_39_8 = readl(priv->regbase + UNIPHIER_SD_RSP10);
> + u32 rsp_127_104 = uniphier_sd_readl(priv, UNIPHIER_SD_RSP76);
> + u32 rsp_103_72 = uniphier_sd_readl(priv, UNIPHIER_SD_RSP54);
> + u32 rsp_71_40 = uniphier_sd_readl(priv, UNIPHIER_SD_RSP32);
> + u32 rsp_39_8 = uniphier_sd_readl(priv, UNIPHIER_SD_RSP10);
>  
>   cmd->response[0] = ((rsp_127_104 & 0x00ffffff) << 8) |
>     ((rsp_103_72  & 0xff000000) >> 24);
> @@ -479,7 +490,7 @@ static int uniphier_sd_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
>   cmd->response[3] = (rsp_39_8     & 0xffffff)   << 8;
>   } else {
>   /* bit 39-8 */
> - cmd->response[0] = readl(priv->regbase + UNIPHIER_SD_RSP10);
> + cmd->response[0] = uniphier_sd_readl(priv, UNIPHIER_SD_RSP10);
>   }
>  
>   if (data) {
> @@ -518,10 +529,10 @@ static int uniphier_sd_set_bus_width(struct uniphier_sd_priv *priv,
>   return -EINVAL;
>   }
>  
> - tmp = readl(priv->regbase + UNIPHIER_SD_OPTION);
> + tmp = uniphier_sd_readl(priv, UNIPHIER_SD_OPTION);
>   tmp &= ~UNIPHIER_SD_OPTION_WIDTH_MASK;
>   tmp |= val;
> - writel(tmp, priv->regbase + UNIPHIER_SD_OPTION);
> + uniphier_sd_writel(priv, tmp, UNIPHIER_SD_OPTION);
>  
>   return 0;
>  }
> @@ -531,12 +542,12 @@ static void uniphier_sd_set_ddr_mode(struct uniphier_sd_priv *priv,
>  {
>   u32 tmp;
>  
> - tmp = readl(priv->regbase + UNIPHIER_SD_IF_MODE);
> + tmp = uniphier_sd_readl(priv, UNIPHIER_SD_IF_MODE);
>   if (mmc->ddr_mode)
>   tmp |= UNIPHIER_SD_IF_MODE_DDR;
>   else
>   tmp &= ~UNIPHIER_SD_IF_MODE_DDR;
> - writel(tmp, priv->regbase + UNIPHIER_SD_IF_MODE);
> + uniphier_sd_writel(priv, tmp, UNIPHIER_SD_IF_MODE);
>  }
>  
>  static void uniphier_sd_set_clk_rate(struct uniphier_sd_priv *priv,
> @@ -573,21 +584,21 @@ static void uniphier_sd_set_clk_rate(struct uniphier_sd_priv *priv,
>   else
>   val = UNIPHIER_SD_CLKCTL_DIV1024;
>  
> - tmp = readl(priv->regbase + UNIPHIER_SD_CLKCTL);
> + tmp = uniphier_sd_readl(priv, UNIPHIER_SD_CLKCTL);
>   if (tmp & UNIPHIER_SD_CLKCTL_SCLKEN &&
>      (tmp & UNIPHIER_SD_CLKCTL_DIV_MASK) == val)
>   return;
>  
>   /* stop the clock before changing its rate to avoid a glitch signal */
>   tmp &= ~UNIPHIER_SD_CLKCTL_SCLKEN;
> - writel(tmp, priv->regbase + UNIPHIER_SD_CLKCTL);
> + uniphier_sd_writel(priv, tmp, UNIPHIER_SD_CLKCTL);
>  
>   tmp &= ~UNIPHIER_SD_CLKCTL_DIV_MASK;
>   tmp |= val | UNIPHIER_SD_CLKCTL_OFFEN;
> - writel(tmp, priv->regbase + UNIPHIER_SD_CLKCTL);
> + uniphier_sd_writel(priv, tmp, UNIPHIER_SD_CLKCTL);
>  
>   tmp |= UNIPHIER_SD_CLKCTL_SCLKEN;
> - writel(tmp, priv->regbase + UNIPHIER_SD_CLKCTL);
> + uniphier_sd_writel(priv, tmp, UNIPHIER_SD_CLKCTL);
>  
>   udelay(1000);
>  }
> @@ -617,7 +628,7 @@ static int uniphier_sd_get_cd(struct udevice *dev)
>   if (priv->caps & UNIPHIER_SD_CAP_NONREMOVABLE)
>   return 1;
>  
> - return !!(readl(priv->regbase + UNIPHIER_SD_INFO1) &
> + return !!(uniphier_sd_readl(priv, UNIPHIER_SD_INFO1) &
>    UNIPHIER_SD_INFO1_CD);
>  }
>  
> @@ -632,28 +643,28 @@ static void uniphier_sd_host_init(struct uniphier_sd_priv *priv)
>   u32 tmp;
>  
>   /* soft reset of the host */
> - tmp = readl(priv->regbase + UNIPHIER_SD_SOFT_RST);
> + tmp = uniphier_sd_readl(priv, UNIPHIER_SD_SOFT_RST);
>   tmp &= ~UNIPHIER_SD_SOFT_RST_RSTX;
> - writel(tmp, priv->regbase + UNIPHIER_SD_SOFT_RST);
> + uniphier_sd_writel(priv, tmp, UNIPHIER_SD_SOFT_RST);
>   tmp |= UNIPHIER_SD_SOFT_RST_RSTX;
> - writel(tmp, priv->regbase + UNIPHIER_SD_SOFT_RST);
> + uniphier_sd_writel(priv, tmp, UNIPHIER_SD_SOFT_RST);
>  
>   /* FIXME: implement eMMC hw_reset */
>  
> - writel(UNIPHIER_SD_STOP_SEC, priv->regbase + UNIPHIER_SD_STOP);
> + uniphier_sd_writel(priv, UNIPHIER_SD_STOP_SEC, UNIPHIER_SD_STOP);
>  
>   /*
>   * Connected to 32bit AXI.
>   * This register dropped backward compatibility at version 0x10.
>   * Write an appropriate value depending on the IP version.
>   */
> - writel(priv->version >= 0x10 ? 0x00000101 : 0x00000000,
> -       priv->regbase + UNIPHIER_SD_HOST_MODE);
> + uniphier_sd_writel(priv, priv->version >= 0x10 ? 0x00000101 : 0x00000000,
> +   UNIPHIER_SD_HOST_MODE);
>  
>   if (priv->caps & UNIPHIER_SD_CAP_DMA_INTERNAL) {
> - tmp = readl(priv->regbase + UNIPHIER_SD_DMA_MODE);
> + tmp = uniphier_sd_readl(priv, UNIPHIER_SD_DMA_MODE);
>   tmp |= UNIPHIER_SD_DMA_MODE_ADDR_INC;
> - writel(tmp, priv->regbase + UNIPHIER_SD_DMA_MODE);
> + uniphier_sd_writel(priv, tmp, UNIPHIER_SD_DMA_MODE);
>   }
>  }
>  
> @@ -724,7 +735,7 @@ static int uniphier_sd_probe(struct udevice *dev)
>       NULL))
>   priv->caps |= UNIPHIER_SD_CAP_NONREMOVABLE;
>  
> - priv->version = readl(priv->regbase + UNIPHIER_SD_VERSION) &
> + priv->version = uniphier_sd_readl(priv, UNIPHIER_SD_VERSION) &
>   UNIPHIER_SD_VERSION_IP;
>   dev_dbg(dev, "version %x\n", priv->version);
>   if (priv->version >= 0x10) {
>

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/5] mmc: uniphier-sd: Factor out register IO

Masahiro Yamada-2
In reply to this post by Marek Vasut
2017-08-13 1:55 GMT+09:00 Marek Vasut <[hidden email]>:

> On 08/10/2017 09:49 AM, Masahiro Yamada wrote:
>> Hi.
>>
>>
>> 2017-08-07 17:30 GMT+09:00 Marek Vasut <[hidden email]>:
>>> On 08/07/2017 04:30 AM, Masahiro Yamada wrote:
>>>> Hi Marek,
>>>
>>> Hi Masahiro,
>>>
>>> This is gonna be a great discussion, let's wrestle about consts and ints :-)
>>>
>>>> 2017-08-06 4:23 GMT+09:00 Marek Vasut <[hidden email]>:
>>>>> On 08/03/2017 02:36 PM, Masahiro Yamada wrote:
>>>>>> Hi Marek,
>>>>>
>>>>> Hi,
>>>>>
>>>>> [...]
>>>>>
>>>>>>> +static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>>>>>>
>>>>>> "const" is unneeded here.
>>>>>
>>>>> Why? The function should not modify reg , so it is const.
>>>>
>>>>
>>>> Because "const" is useless here.
>>>>
>>>> The "reg" is not a pointer, so it is obvious
>>>> that there is no impact to the callers.
>>>>
>>>>
>>>>
>>>> Moreover, whether "reg" is constant or not
>>>> depends on how you implement the function.
>>>>
>>>>
>>>> If you force "const" to the argument, the only choice for the implementation
>>>> will be as follows:
>>>>
>>>>
>>>>
>>>> static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>>>> {
>>>>       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
>>>>              return readl(priv->regbase + (reg << 1));
>>>>       else
>>>>              return readl(priv->regbase + reg);
>>>> }
>>>>
>>>>
>>>>
>>>> If you want to implement the function as follows, you need to drop "const".
>>>>
>>>> static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, u32 reg)
>>>> {
>>>>       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
>>>>               reg <<= 1;
>>>>
>>>>       return readl(priv->regbase + reg);
>>>> }
>>>
>>> My argument would be that the const prevents you from accidentally
>>> modifying the $reg inside the function.
>>
>>
>> No.
>> The arguments of a functions are local variables.
>> No impact on the outside of the scope of the function.
>
> I'm not arguing about that.
>
>> If you accidentally modify a variable where it should not,
>> it is a bug.  Just fix it.
>
> If it's const, compiler will warn the user he's doing something stupid.
>
>> If you want to wrestle more, please do it in LKML
>> by adding around "const".
>>
>> For example,
>>
>> drivers/base/regmap/regmap.c:
>> int regmap_write(struct regmap *map, unsigned int reg, unsigned int val)
>>
>> arch/arm/include/asm/io.h:
>> static inline void __raw_writel(u32 val, volatile void __iomem *addr)
>
> Well, there were some cocci patches adding const recently :)
>


Do you mean cocci scripts in scripts/coccinelle ?
I checked linux-next, but I could not find it.




--
Best Regards
Masahiro Yamada
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/5] mmc: uniphier-sd: Factor out register IO

Masahiro Yamada-2
In reply to this post by Jaehoon Chung
2017-08-17 15:39 GMT+09:00 Jaehoon Chung <[hidden email]>:

> On 08/13/2017 01:55 AM, Marek Vasut wrote:
>> On 08/10/2017 09:49 AM, Masahiro Yamada wrote:
>>> Hi.
>>>
>>>
>>> 2017-08-07 17:30 GMT+09:00 Marek Vasut <[hidden email]>:
>>>> On 08/07/2017 04:30 AM, Masahiro Yamada wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi Masahiro,
>>>>
>>>> This is gonna be a great discussion, let's wrestle about consts and ints :-)
>>>>
>>>>> 2017-08-06 4:23 GMT+09:00 Marek Vasut <[hidden email]>:
>>>>>> On 08/03/2017 02:36 PM, Masahiro Yamada wrote:
>>>>>>> Hi Marek,
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>> +static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>>>>>>>
>>>>>>> "const" is unneeded here.
>>>>>>
>>>>>> Why? The function should not modify reg , so it is const.
>>>>>
>>>>>
>>>>> Because "const" is useless here.
>>>>>
>>>>> The "reg" is not a pointer, so it is obvious
>>>>> that there is no impact to the callers.
>>>>>
>>>>>
>>>>>
>>>>> Moreover, whether "reg" is constant or not
>>>>> depends on how you implement the function.
>>>>>
>>>>>
>>>>> If you force "const" to the argument, the only choice for the implementation
>>>>> will be as follows:
>>>>>
>>>>>
>>>>>
>>>>> static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>>>>> {
>>>>>       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
>>>>>              return readl(priv->regbase + (reg << 1));
>>>>>       else
>>>>>              return readl(priv->regbase + reg);
>>>>> }
>>>>>
>>>>>
>>>>>
>>>>> If you want to implement the function as follows, you need to drop "const".
>>>>>
>>>>> static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, u32 reg)
>>>>> {
>>>>>       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
>>>>>               reg <<= 1;
>>>>>
>>>>>       return readl(priv->regbase + reg);
>>>>> }
>>>>
>>>> My argument would be that the const prevents you from accidentally
>>>> modifying the $reg inside the function.
>
> Is there any case about modifying the regs value in this function?
> Well..I think that it makes sense about both. (Masahiro and Marek opinion)
>
> But There is nothing wrong to prevent from accidentally.


In my opinion, const is useful for pointer dereference,
but unneeded in this case.

I believe it is the taste
from what I saw from Linux code.  So, I follow it.



--
Best Regards
Masahiro Yamada
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/5] mmc: uniphier-sd: Factor out register IO

Marek Vasut
On 08/17/2017 09:01 AM, Masahiro Yamada wrote:

> 2017-08-17 15:39 GMT+09:00 Jaehoon Chung <[hidden email]>:
>> On 08/13/2017 01:55 AM, Marek Vasut wrote:
>>> On 08/10/2017 09:49 AM, Masahiro Yamada wrote:
>>>> Hi.
>>>>
>>>>
>>>> 2017-08-07 17:30 GMT+09:00 Marek Vasut <[hidden email]>:
>>>>> On 08/07/2017 04:30 AM, Masahiro Yamada wrote:
>>>>>> Hi Marek,
>>>>>
>>>>> Hi Masahiro,
>>>>>
>>>>> This is gonna be a great discussion, let's wrestle about consts and ints :-)
>>>>>
>>>>>> 2017-08-06 4:23 GMT+09:00 Marek Vasut <[hidden email]>:
>>>>>>> On 08/03/2017 02:36 PM, Masahiro Yamada wrote:
>>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>>> +static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>>>>>>>>
>>>>>>>> "const" is unneeded here.
>>>>>>>
>>>>>>> Why? The function should not modify reg , so it is const.
>>>>>>
>>>>>>
>>>>>> Because "const" is useless here.
>>>>>>
>>>>>> The "reg" is not a pointer, so it is obvious
>>>>>> that there is no impact to the callers.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Moreover, whether "reg" is constant or not
>>>>>> depends on how you implement the function.
>>>>>>
>>>>>>
>>>>>> If you force "const" to the argument, the only choice for the implementation
>>>>>> will be as follows:
>>>>>>
>>>>>>
>>>>>>
>>>>>> static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>>>>>> {
>>>>>>       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
>>>>>>              return readl(priv->regbase + (reg << 1));
>>>>>>       else
>>>>>>              return readl(priv->regbase + reg);
>>>>>> }
>>>>>>
>>>>>>
>>>>>>
>>>>>> If you want to implement the function as follows, you need to drop "const".
>>>>>>
>>>>>> static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, u32 reg)
>>>>>> {
>>>>>>       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
>>>>>>               reg <<= 1;
>>>>>>
>>>>>>       return readl(priv->regbase + reg);
>>>>>> }
>>>>>
>>>>> My argument would be that the const prevents you from accidentally
>>>>> modifying the $reg inside the function.
>>
>> Is there any case about modifying the regs value in this function?
>> Well..I think that it makes sense about both. (Masahiro and Marek opinion)
>>
>> But There is nothing wrong to prevent from accidentally.
>
>
> In my opinion, const is useful for pointer dereference,
> but unneeded in this case.
>
> I believe it is the taste
> from what I saw from Linux code.  So, I follow it.

I don't care either way, I was just wrestling Masahiro for the sake of
it (you know the thing about wrestling engineers). There's a V2, so feel
free to collect that if Masahiro can give me an AB/RB.

--
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/5] mmc: uniphier-sd: Factor out register IO

Masahiro Yamada-2
Hi Marek,


2017-08-17 20:56 GMT+09:00 Marek Vasut <[hidden email]>:

> On 08/17/2017 09:01 AM, Masahiro Yamada wrote:
>> 2017-08-17 15:39 GMT+09:00 Jaehoon Chung <[hidden email]>:
>>> On 08/13/2017 01:55 AM, Marek Vasut wrote:
>>>> On 08/10/2017 09:49 AM, Masahiro Yamada wrote:
>>>>> Hi.
>>>>>
>>>>>
>>>>> 2017-08-07 17:30 GMT+09:00 Marek Vasut <[hidden email]>:
>>>>>> On 08/07/2017 04:30 AM, Masahiro Yamada wrote:
>>>>>>> Hi Marek,
>>>>>>
>>>>>> Hi Masahiro,
>>>>>>
>>>>>> This is gonna be a great discussion, let's wrestle about consts and ints :-)
>>>>>>
>>>>>>> 2017-08-06 4:23 GMT+09:00 Marek Vasut <[hidden email]>:
>>>>>>>> On 08/03/2017 02:36 PM, Masahiro Yamada wrote:
>>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>>> +static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>>>>>>>>>
>>>>>>>>> "const" is unneeded here.
>>>>>>>>
>>>>>>>> Why? The function should not modify reg , so it is const.
>>>>>>>
>>>>>>>
>>>>>>> Because "const" is useless here.
>>>>>>>
>>>>>>> The "reg" is not a pointer, so it is obvious
>>>>>>> that there is no impact to the callers.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Moreover, whether "reg" is constant or not
>>>>>>> depends on how you implement the function.
>>>>>>>
>>>>>>>
>>>>>>> If you force "const" to the argument, the only choice for the implementation
>>>>>>> will be as follows:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, const u32 reg)
>>>>>>> {
>>>>>>>       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
>>>>>>>              return readl(priv->regbase + (reg << 1));
>>>>>>>       else
>>>>>>>              return readl(priv->regbase + reg);
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> If you want to implement the function as follows, you need to drop "const".
>>>>>>>
>>>>>>> static u32 uniphier_sd_readl(struct uniphier_sd_priv *priv, u32 reg)
>>>>>>> {
>>>>>>>       if (priv->caps & UNIPHIER_SD_CAP_64BIT)
>>>>>>>               reg <<= 1;
>>>>>>>
>>>>>>>       return readl(priv->regbase + reg);
>>>>>>> }
>>>>>>
>>>>>> My argument would be that the const prevents you from accidentally
>>>>>> modifying the $reg inside the function.
>>>
>>> Is there any case about modifying the regs value in this function?
>>> Well..I think that it makes sense about both. (Masahiro and Marek opinion)
>>>
>>> But There is nothing wrong to prevent from accidentally.
>>
>>
>> In my opinion, const is useful for pointer dereference,
>> but unneeded in this case.
>>
>> I believe it is the taste
>> from what I saw from Linux code.  So, I follow it.
>
> I don't care either way, I was just wrestling Masahiro for the sake of
> it (you know the thing about wrestling engineers). There's a V2, so feel
> free to collect that if Masahiro can give me an AB/RB.


I still see const in 1/5 and 3/5.

Please remove them if my AB/RB is needed.



--
Best Regards
Masahiro Yamada
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Loading...