Re: [PATCH 2/5] mmc: add stm32 sdmmc controller driver

[Ludovic BARRE]

hi Shawn

thanks for your review

On 02/22/2018 05:20 PM, Shawn Lin wrote:

On 2018/2/15 21:34, Ludovic Barre wrote:

From: Ludovic Barre 



...


+
+static ssize_t stm32_sdmmc_stat_reset(struct file *filp,
+  const char __user *ubuf,
+  size_t count, loff_t *ppos)
+{
+    struct seq_file *seqf = filp->private_data;
+    struct sdmmc_host *host = seqf->private;
+
+    mutex_lock(>lock);
+    memset(>stat, 0, sizeof(host->stat));
+    mutex_unlock(>lock);
+
+    return count;
+}
+
+static int stm32_sdmmc_stat_open(struct inode *inode, struct file *file)
+{
+    return single_open(file, stm32_sdmmc_stat_show, inode->i_private);
+}
+
+static const struct file_operations stm32_sdmmc_stat_fops = {
+    .owner    = THIS_MODULE,
+    .open    = stm32_sdmmc_stat_open,
+    .read    = seq_read,
+    .write    = stm32_sdmmc_stat_reset,
+    .llseek    = seq_lseek,
+    .release    = single_release,
+};
+


Could you simply use DEFINE_SHOW_ATTRIBUTE(stm32_sdmmc_stat) instead?


DEFINE_SHOW_ATTRIBUTE has no ".write" file_operations.
It's very useful to reset the statistic structure.
So if it's possible to keep this feature, I would prefer.




+static void stm32_sdmmc_stat_init(struct sdmmc_host *host)
+{
+    struct mmc_host    *mmc = host->mmc;
+    struct dentry *root;
+
+    root = mmc->debugfs_root;
+    if (!root)
+    return;
+
+    if (!debugfs_create_file("stat", 0600, root, host,
+ _sdmmc_stat_fops))
+    dev_err(mmc_dev(host->mmc), "failed to initialize debugfs\n");
+}
+
+#define STAT_INC(stat) ((stat)++)
+#else
+static void stm32_sdmmc_stat_init(struct sdmmc_host *host)
+{
+}
+
+#define STAT_INC(stat)
+#endif
+
+static inline u32 enable_imask(struct sdmmc_host *host, u32 imask)
+{
+    u32 newmask;
+
+    newmask = readl_relaxed(host->base + SDMMC_MASKR);
+    newmask |= imask;
+
+    dev_vdbg(mmc_dev(host->mmc), "mask:%#x\n", newmask);
+
+    writel_relaxed(newmask, host->base + SDMMC_MASKR);
+
+    return newmask;
+}
+


I don't see you use the return value eleswhere, perhaps
remove it?


yes your right, I remove the return.




+static inline u32 disable_imask(struct sdmmc_host *host, u32 imask)
+{
+    u32 newmask;
+
+    newmask = readl_relaxed(host->base + SDMMC_MASKR);
+    newmask &= ~imask;
+
+    dev_vdbg(mmc_dev(host->mmc), "mask:%#x\n", newmask);
+
+    writel_relaxed(newmask, host->base + SDMMC_MASKR);
+
+    return newmask;
+}
+


Ditto?


yes your right, I remove the return.




+static inline void clear_imask(struct sdmmc_host *host)
+{
+    u32 mask = readl_relaxed(host->base + SDMMC_MASKR);
+
+    /* preserve the SDIO IRQ mask state */
+    mask &= MASKR_SDIOITIE;
+
+    dev_vdbg(mmc_dev(host->mmc), "mask:%#x\n", mask);
+
+    writel_relaxed(mask, host->base + SDMMC_MASKR);
+}
+


Not clear to me why couldn't you use :
imask = 0x ^ MASKR_SDIOITIE;
disable_imask(imask)


In fact, I wish keep SDIOITIE enabled if and only if the SDIOTIE was 
already enabled (so SDIOITIE mask is not always set)





+static int stm32_sdmmc_card_busy(struct mmc_host *mmc)
+{
+    struct sdmmc_host *host = mmc_priv(mmc);
+    unsigned long flags;
+    u32 status;
+
+    spin_lock_irqsave(>lock, flags);
+    status = readl_relaxed(host->base + SDMMC_STAR);
+    spin_unlock_irqrestore(>lock, flags);
+
+    return !!(status & STAR_BUSYD0);
+}
+


I don't think you need to hold the lock here.


just a protection with "stm32_sdmmc_irq" which could modify status value




+static void stm32_sdmmc_request_end(struct sdmmc_host *host,
+    struct mmc_request *mrq)
+{
+    writel_relaxed(0, host->base + SDMMC_CMDR);
+    writel_relaxed(ICR_STATIC_FLAG, host->base + SDMMC_ICR);
+
+    host->mrq = NULL;
+    host->cmd = NULL;
+    host->data = NULL;
+
+    clear_imask(host);
+
+    mmc_request_done(host->mmc, mrq);
+}
+
+static void stm32_sdmmc_pwroff(struct sdmmc_host *host)
+{
+    /* Only a reset could disable sdmmc */
+    reset_control_assert(host->rst);
+    udelay(2);
+    reset_control_deassert(host->rst);
+
+    /*
+ * Set the SDMMC in Power-cycle state. This will make that the
+ * SDMMC_D[7:0], SDMMC_CMD and SDMMC_CK are driven low,
+ * to prevent the Card from being powered through the signal lines.
+ */
+    writel_relaxed(POWERCTRL_CYC | host->pwr_reg_add,
+   host->base + SDMMC_POWER);
+}
+
+static void stm32_sdmmc_pwron(struct sdmmc_host *host)
+{
+    /*
+ * After a power-cycle state, we must set the SDMMC in Power-off.
+ * The SDMMC_D[7:0], SDMMC_CMD and SDMMC_CK are driven high.
+ * Then we can set the SDMMC to Power-on state
+ */
+    writel_relaxed(POWERCTRL_OFF | host->pwr_reg_add,
+   host->base + SDMMC_POWER);
+    mdelay(1);
+    writel_relaxed(POWERCTRL_ON | host->pwr_reg_add,
+   host->base + SDMMC_POWER);
+}
+
+static void stm32_sdmmc_set_clkreg(struct sdmmc_host *host, struct 
mmc_ios *ios)

+{
+  

Re: [PATCH 2/5] mmc: add stm32 sdmmc controller driver

[Ludovic BARRE]

hi Shawn

thanks for your review

On 02/22/2018 05:20 PM, Shawn Lin wrote:

On 2018/2/15 21:34, Ludovic Barre wrote:

From: Ludovic Barre 



...


+
+static ssize_t stm32_sdmmc_stat_reset(struct file *filp,
+  const char __user *ubuf,
+  size_t count, loff_t *ppos)
+{
+    struct seq_file *seqf = filp->private_data;
+    struct sdmmc_host *host = seqf->private;
+
+    mutex_lock(>lock);
+    memset(>stat, 0, sizeof(host->stat));
+    mutex_unlock(>lock);
+
+    return count;
+}
+
+static int stm32_sdmmc_stat_open(struct inode *inode, struct file *file)
+{
+    return single_open(file, stm32_sdmmc_stat_show, inode->i_private);
+}
+
+static const struct file_operations stm32_sdmmc_stat_fops = {
+    .owner    = THIS_MODULE,
+    .open    = stm32_sdmmc_stat_open,
+    .read    = seq_read,
+    .write    = stm32_sdmmc_stat_reset,
+    .llseek    = seq_lseek,
+    .release    = single_release,
+};
+


Could you simply use DEFINE_SHOW_ATTRIBUTE(stm32_sdmmc_stat) instead?


DEFINE_SHOW_ATTRIBUTE has no ".write" file_operations.
It's very useful to reset the statistic structure.
So if it's possible to keep this feature, I would prefer.




+static void stm32_sdmmc_stat_init(struct sdmmc_host *host)
+{
+    struct mmc_host    *mmc = host->mmc;
+    struct dentry *root;
+
+    root = mmc->debugfs_root;
+    if (!root)
+    return;
+
+    if (!debugfs_create_file("stat", 0600, root, host,
+ _sdmmc_stat_fops))
+    dev_err(mmc_dev(host->mmc), "failed to initialize debugfs\n");
+}
+
+#define STAT_INC(stat) ((stat)++)
+#else
+static void stm32_sdmmc_stat_init(struct sdmmc_host *host)
+{
+}
+
+#define STAT_INC(stat)
+#endif
+
+static inline u32 enable_imask(struct sdmmc_host *host, u32 imask)
+{
+    u32 newmask;
+
+    newmask = readl_relaxed(host->base + SDMMC_MASKR);
+    newmask |= imask;
+
+    dev_vdbg(mmc_dev(host->mmc), "mask:%#x\n", newmask);
+
+    writel_relaxed(newmask, host->base + SDMMC_MASKR);
+
+    return newmask;
+}
+


I don't see you use the return value eleswhere, perhaps
remove it?


yes your right, I remove the return.




+static inline u32 disable_imask(struct sdmmc_host *host, u32 imask)
+{
+    u32 newmask;
+
+    newmask = readl_relaxed(host->base + SDMMC_MASKR);
+    newmask &= ~imask;
+
+    dev_vdbg(mmc_dev(host->mmc), "mask:%#x\n", newmask);
+
+    writel_relaxed(newmask, host->base + SDMMC_MASKR);
+
+    return newmask;
+}
+


Ditto?


yes your right, I remove the return.




+static inline void clear_imask(struct sdmmc_host *host)
+{
+    u32 mask = readl_relaxed(host->base + SDMMC_MASKR);
+
+    /* preserve the SDIO IRQ mask state */
+    mask &= MASKR_SDIOITIE;
+
+    dev_vdbg(mmc_dev(host->mmc), "mask:%#x\n", mask);
+
+    writel_relaxed(mask, host->base + SDMMC_MASKR);
+}
+


Not clear to me why couldn't you use :
imask = 0x ^ MASKR_SDIOITIE;
disable_imask(imask)


In fact, I wish keep SDIOITIE enabled if and only if the SDIOTIE was 
already enabled (so SDIOITIE mask is not always set)





+static int stm32_sdmmc_card_busy(struct mmc_host *mmc)
+{
+    struct sdmmc_host *host = mmc_priv(mmc);
+    unsigned long flags;
+    u32 status;
+
+    spin_lock_irqsave(>lock, flags);
+    status = readl_relaxed(host->base + SDMMC_STAR);
+    spin_unlock_irqrestore(>lock, flags);
+
+    return !!(status & STAR_BUSYD0);
+}
+


I don't think you need to hold the lock here.


just a protection with "stm32_sdmmc_irq" which could modify status value




+static void stm32_sdmmc_request_end(struct sdmmc_host *host,
+    struct mmc_request *mrq)
+{
+    writel_relaxed(0, host->base + SDMMC_CMDR);
+    writel_relaxed(ICR_STATIC_FLAG, host->base + SDMMC_ICR);
+
+    host->mrq = NULL;
+    host->cmd = NULL;
+    host->data = NULL;
+
+    clear_imask(host);
+
+    mmc_request_done(host->mmc, mrq);
+}
+
+static void stm32_sdmmc_pwroff(struct sdmmc_host *host)
+{
+    /* Only a reset could disable sdmmc */
+    reset_control_assert(host->rst);
+    udelay(2);
+    reset_control_deassert(host->rst);
+
+    /*
+ * Set the SDMMC in Power-cycle state. This will make that the
+ * SDMMC_D[7:0], SDMMC_CMD and SDMMC_CK are driven low,
+ * to prevent the Card from being powered through the signal lines.
+ */
+    writel_relaxed(POWERCTRL_CYC | host->pwr_reg_add,
+   host->base + SDMMC_POWER);
+}
+
+static void stm32_sdmmc_pwron(struct sdmmc_host *host)
+{
+    /*
+ * After a power-cycle state, we must set the SDMMC in Power-off.
+ * The SDMMC_D[7:0], SDMMC_CMD and SDMMC_CK are driven high.
+ * Then we can set the SDMMC to Power-on state
+ */
+    writel_relaxed(POWERCTRL_OFF | host->pwr_reg_add,
+   host->base + SDMMC_POWER);
+    mdelay(1);
+    writel_relaxed(POWERCTRL_ON | host->pwr_reg_add,
+   host->base + SDMMC_POWER);
+}
+
+static void stm32_sdmmc_set_clkreg(struct sdmmc_host *host, struct 
mmc_ios *ios)

+{
+    u32 desired = 

Re: [PATCH 2/5] mmc: add stm32 sdmmc controller driver

[Shawn Lin]

On 2018/2/15 21:34, Ludovic Barre wrote:

From: Ludovic Barre 



...


+
+static ssize_t stm32_sdmmc_stat_reset(struct file *filp,
+ const char __user *ubuf,
+ size_t count, loff_t *ppos)
+{
+   struct seq_file *seqf = filp->private_data;
+   struct sdmmc_host *host = seqf->private;
+
+   mutex_lock(>lock);
+   memset(>stat, 0, sizeof(host->stat));
+   mutex_unlock(>lock);
+
+   return count;
+}
+
+static int stm32_sdmmc_stat_open(struct inode *inode, struct file *file)
+{
+   return single_open(file, stm32_sdmmc_stat_show, inode->i_private);
+}
+
+static const struct file_operations stm32_sdmmc_stat_fops = {
+   .owner  = THIS_MODULE,
+   .open   = stm32_sdmmc_stat_open,
+   .read   = seq_read,
+   .write  = stm32_sdmmc_stat_reset,
+   .llseek = seq_lseek,
+   .release= single_release,
+};
+


Could you simply use DEFINE_SHOW_ATTRIBUTE(stm32_sdmmc_stat) instead?


+static void stm32_sdmmc_stat_init(struct sdmmc_host *host)
+{
+   struct mmc_host *mmc = host->mmc;
+   struct dentry *root;
+
+   root = mmc->debugfs_root;
+   if (!root)
+   return;
+
+   if (!debugfs_create_file("stat", 0600, root, host,
+_sdmmc_stat_fops))
+   dev_err(mmc_dev(host->mmc), "failed to initialize debugfs\n");
+}
+
+#define STAT_INC(stat) ((stat)++)
+#else
+static void stm32_sdmmc_stat_init(struct sdmmc_host *host)
+{
+}
+
+#define STAT_INC(stat)
+#endif
+
+static inline u32 enable_imask(struct sdmmc_host *host, u32 imask)
+{
+   u32 newmask;
+
+   newmask = readl_relaxed(host->base + SDMMC_MASKR);
+   newmask |= imask;
+
+   dev_vdbg(mmc_dev(host->mmc), "mask:%#x\n", newmask);
+
+   writel_relaxed(newmask, host->base + SDMMC_MASKR);
+
+   return newmask;
+}
+


I don't see you use the return value eleswhere, perhaps
remove it?


+static inline u32 disable_imask(struct sdmmc_host *host, u32 imask)
+{
+   u32 newmask;
+
+   newmask = readl_relaxed(host->base + SDMMC_MASKR);
+   newmask &= ~imask;
+
+   dev_vdbg(mmc_dev(host->mmc), "mask:%#x\n", newmask);
+
+   writel_relaxed(newmask, host->base + SDMMC_MASKR);
+
+   return newmask;
+}
+


Ditto?


+static inline void clear_imask(struct sdmmc_host *host)
+{
+   u32 mask = readl_relaxed(host->base + SDMMC_MASKR);
+
+   /* preserve the SDIO IRQ mask state */
+   mask &= MASKR_SDIOITIE;
+
+   dev_vdbg(mmc_dev(host->mmc), "mask:%#x\n", mask);
+
+   writel_relaxed(mask, host->base + SDMMC_MASKR);
+}
+


Not clear to me why couldn't you use :
imask = 0x ^ MASKR_SDIOITIE;
disable_imask(imask)


+static int stm32_sdmmc_card_busy(struct mmc_host *mmc)
+{
+   struct sdmmc_host *host = mmc_priv(mmc);
+   unsigned long flags;
+   u32 status;
+
+   spin_lock_irqsave(>lock, flags);
+   status = readl_relaxed(host->base + SDMMC_STAR);
+   spin_unlock_irqrestore(>lock, flags);
+
+   return !!(status & STAR_BUSYD0);
+}
+


I don't think you need to hold the lock here.


+static void stm32_sdmmc_request_end(struct sdmmc_host *host,
+   struct mmc_request *mrq)
+{
+   writel_relaxed(0, host->base + SDMMC_CMDR);
+   writel_relaxed(ICR_STATIC_FLAG, host->base + SDMMC_ICR);
+
+   host->mrq = NULL;
+   host->cmd = NULL;
+   host->data = NULL;
+
+   clear_imask(host);
+
+   mmc_request_done(host->mmc, mrq);
+}
+
+static void stm32_sdmmc_pwroff(struct sdmmc_host *host)
+{
+   /* Only a reset could disable sdmmc */
+   reset_control_assert(host->rst);
+   udelay(2);
+   reset_control_deassert(host->rst);
+
+   /*
+* Set the SDMMC in Power-cycle state. This will make that the
+* SDMMC_D[7:0], SDMMC_CMD and SDMMC_CK are driven low,
+* to prevent the Card from being powered through the signal lines.
+*/
+   writel_relaxed(POWERCTRL_CYC | host->pwr_reg_add,
+  host->base + SDMMC_POWER);
+}
+
+static void stm32_sdmmc_pwron(struct sdmmc_host *host)
+{
+   /*
+* After a power-cycle state, we must set the SDMMC in Power-off.
+* The SDMMC_D[7:0], SDMMC_CMD and SDMMC_CK are driven high.
+* Then we can set the SDMMC to Power-on state
+*/
+   writel_relaxed(POWERCTRL_OFF | host->pwr_reg_add,
+  host->base + SDMMC_POWER);
+   mdelay(1);
+   writel_relaxed(POWERCTRL_ON | host->pwr_reg_add,
+  host->base + SDMMC_POWER);
+}
+
+static void stm32_sdmmc_set_clkreg(struct sdmmc_host *host, struct mmc_ios 
*ios)
+{
+   u32 desired = ios->clock;
+   u32 clk = 0;
+
+   /*
+* sdmmc_ck = sdmmcclk/(2*clkdiv)
+* clkdiv 0 => bypass
+*/
+   if (desired) {
+   if (desired >= host->sdmmcclk) {
+  

Re: [PATCH 2/5] mmc: add stm32 sdmmc controller driver

[Shawn Lin]

On 2018/2/15 21:34, Ludovic Barre wrote:

From: Ludovic Barre 



...


+
+static ssize_t stm32_sdmmc_stat_reset(struct file *filp,
+ const char __user *ubuf,
+ size_t count, loff_t *ppos)
+{
+   struct seq_file *seqf = filp->private_data;
+   struct sdmmc_host *host = seqf->private;
+
+   mutex_lock(>lock);
+   memset(>stat, 0, sizeof(host->stat));
+   mutex_unlock(>lock);
+
+   return count;
+}
+
+static int stm32_sdmmc_stat_open(struct inode *inode, struct file *file)
+{
+   return single_open(file, stm32_sdmmc_stat_show, inode->i_private);
+}
+
+static const struct file_operations stm32_sdmmc_stat_fops = {
+   .owner  = THIS_MODULE,
+   .open   = stm32_sdmmc_stat_open,
+   .read   = seq_read,
+   .write  = stm32_sdmmc_stat_reset,
+   .llseek = seq_lseek,
+   .release= single_release,
+};
+


Could you simply use DEFINE_SHOW_ATTRIBUTE(stm32_sdmmc_stat) instead?


+static void stm32_sdmmc_stat_init(struct sdmmc_host *host)
+{
+   struct mmc_host *mmc = host->mmc;
+   struct dentry *root;
+
+   root = mmc->debugfs_root;
+   if (!root)
+   return;
+
+   if (!debugfs_create_file("stat", 0600, root, host,
+_sdmmc_stat_fops))
+   dev_err(mmc_dev(host->mmc), "failed to initialize debugfs\n");
+}
+
+#define STAT_INC(stat) ((stat)++)
+#else
+static void stm32_sdmmc_stat_init(struct sdmmc_host *host)
+{
+}
+
+#define STAT_INC(stat)
+#endif
+
+static inline u32 enable_imask(struct sdmmc_host *host, u32 imask)
+{
+   u32 newmask;
+
+   newmask = readl_relaxed(host->base + SDMMC_MASKR);
+   newmask |= imask;
+
+   dev_vdbg(mmc_dev(host->mmc), "mask:%#x\n", newmask);
+
+   writel_relaxed(newmask, host->base + SDMMC_MASKR);
+
+   return newmask;
+}
+


I don't see you use the return value eleswhere, perhaps
remove it?


+static inline u32 disable_imask(struct sdmmc_host *host, u32 imask)
+{
+   u32 newmask;
+
+   newmask = readl_relaxed(host->base + SDMMC_MASKR);
+   newmask &= ~imask;
+
+   dev_vdbg(mmc_dev(host->mmc), "mask:%#x\n", newmask);
+
+   writel_relaxed(newmask, host->base + SDMMC_MASKR);
+
+   return newmask;
+}
+


Ditto?


+static inline void clear_imask(struct sdmmc_host *host)
+{
+   u32 mask = readl_relaxed(host->base + SDMMC_MASKR);
+
+   /* preserve the SDIO IRQ mask state */
+   mask &= MASKR_SDIOITIE;
+
+   dev_vdbg(mmc_dev(host->mmc), "mask:%#x\n", mask);
+
+   writel_relaxed(mask, host->base + SDMMC_MASKR);
+}
+


Not clear to me why couldn't you use :
imask = 0x ^ MASKR_SDIOITIE;
disable_imask(imask)


+static int stm32_sdmmc_card_busy(struct mmc_host *mmc)
+{
+   struct sdmmc_host *host = mmc_priv(mmc);
+   unsigned long flags;
+   u32 status;
+
+   spin_lock_irqsave(>lock, flags);
+   status = readl_relaxed(host->base + SDMMC_STAR);
+   spin_unlock_irqrestore(>lock, flags);
+
+   return !!(status & STAR_BUSYD0);
+}
+


I don't think you need to hold the lock here.


+static void stm32_sdmmc_request_end(struct sdmmc_host *host,
+   struct mmc_request *mrq)
+{
+   writel_relaxed(0, host->base + SDMMC_CMDR);
+   writel_relaxed(ICR_STATIC_FLAG, host->base + SDMMC_ICR);
+
+   host->mrq = NULL;
+   host->cmd = NULL;
+   host->data = NULL;
+
+   clear_imask(host);
+
+   mmc_request_done(host->mmc, mrq);
+}
+
+static void stm32_sdmmc_pwroff(struct sdmmc_host *host)
+{
+   /* Only a reset could disable sdmmc */
+   reset_control_assert(host->rst);
+   udelay(2);
+   reset_control_deassert(host->rst);
+
+   /*
+* Set the SDMMC in Power-cycle state. This will make that the
+* SDMMC_D[7:0], SDMMC_CMD and SDMMC_CK are driven low,
+* to prevent the Card from being powered through the signal lines.
+*/
+   writel_relaxed(POWERCTRL_CYC | host->pwr_reg_add,
+  host->base + SDMMC_POWER);
+}
+
+static void stm32_sdmmc_pwron(struct sdmmc_host *host)
+{
+   /*
+* After a power-cycle state, we must set the SDMMC in Power-off.
+* The SDMMC_D[7:0], SDMMC_CMD and SDMMC_CK are driven high.
+* Then we can set the SDMMC to Power-on state
+*/
+   writel_relaxed(POWERCTRL_OFF | host->pwr_reg_add,
+  host->base + SDMMC_POWER);
+   mdelay(1);
+   writel_relaxed(POWERCTRL_ON | host->pwr_reg_add,
+  host->base + SDMMC_POWER);
+}
+
+static void stm32_sdmmc_set_clkreg(struct sdmmc_host *host, struct mmc_ios 
*ios)
+{
+   u32 desired = ios->clock;
+   u32 clk = 0;
+
+   /*
+* sdmmc_ck = sdmmcclk/(2*clkdiv)
+* clkdiv 0 => bypass
+*/
+   if (desired) {
+   if (desired >= host->sdmmcclk) {
+