| Message ID | 1528809280-31116-1-git-send-email-ludovic.Barre@st.com |
|---|---|
| Headers | show |
| Series | mmc: mmci: add stm32 sdmmc variant | expand |
hi Ulf I know that you very busy on other task, but did you have time to look my serie. do you have first feedback ? BR Ludo On 06/12/2018 03:14 PM, Ludovic Barre wrote: > From: Ludovic Barre <ludovic.barre@st.com> > > This patch series adapts mmci driver to add support for stm32 > sdmmc variant. stm32h7 SoC integrates the first revision of > stm32 sdmmc. > > This series is composed of 3 parts: > -Prepare mmci driver to manage dma interfaces by adding property. > New mmci dma API is defined according to the legacy needs. > -Adapt mmci driver to dedicated constraints of stm32 sdmmc variant, > defined under some specific properties. > -Add stm32 sdmmc variant. As Internal DMA way satisfies data > transfer, the mmci driver hasn't been modified for pio_read/write. > Specific adds-ons to stm32 sdmmc: > + registers > + clk/power functions > + idma interface > > Ludovic Barre (19): > mmc: mmci: regroup and define dma operations > mmc: mmci: merge qcom dml feature into mmci dma > mmc: mmci: add datactrl block size variant property > mmc: mmci: expand startbiterr to irqmask and error check > mmc: mmci: allow to overwrite clock/power procedure to specific > variant > mmc: mmci: add variant properties to define cpsm & cmdresp bits > mmc: mmci: add variant property to define dpsm bit > mmc: mmci: add variant property to define irq pio mask > mmc: mmci: add variant property to write datactrl before command > mmc: mmci: add variant property to allow remain data > mmc: mmci: add variant property to check specific data constraint > mmc: mmci: add variant property to request a reset > mmc: mmci: send stop cmd if a data command fail > mmc: mmci: add clock divider for stm32 sdmmc > mmc: mmci: add stm32 sdmmc registers > mmc: mmci: add DT bindings for STM32 sdmmc > mmc: mmci: add stm32 sdmmc idma support > mmc: mmci: add specific clk/pwr procedure for stm32 sdmmc > mmc: mmci: add stm32 sdmmc variant > > Documentation/devicetree/bindings/mmc/mmci.txt | 11 + > drivers/mmc/host/Makefile | 3 +- > drivers/mmc/host/mmci.c | 846 +++++++++++-------------- > drivers/mmc/host/mmci.h | 237 ++++++- > drivers/mmc/host/mmci_dma.c | 780 +++++++++++++++++++++++ > drivers/mmc/host/mmci_dma.h | 33 + > drivers/mmc/host/mmci_qcom_dml.c | 177 ------ > drivers/mmc/host/mmci_qcom_dml.h | 31 - > 8 files changed, 1410 insertions(+), 708 deletions(-) > create mode 100644 drivers/mmc/host/mmci_dma.c > create mode 100644 drivers/mmc/host/mmci_dma.h > delete mode 100644 drivers/mmc/host/mmci_qcom_dml.c > delete mode 100644 drivers/mmc/host/mmci_qcom_dml.h > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29 June 2018 at 15:51, Ludovic BARRE <ludovic.barre@st.com> wrote: > hi Ulf > > I know that you very busy on other task, > but did you have time to look my serie. > do you have first feedback ? Apologize for the delay. I am going to catch up on mmc next week. At first glance, this looks reasonable. Kind regards Uffe > > BR > Ludo > > > On 06/12/2018 03:14 PM, Ludovic Barre wrote: >> >> From: Ludovic Barre <ludovic.barre@st.com> >> >> This patch series adapts mmci driver to add support for stm32 >> sdmmc variant. stm32h7 SoC integrates the first revision of >> stm32 sdmmc. >> >> This series is composed of 3 parts: >> -Prepare mmci driver to manage dma interfaces by adding property. >> New mmci dma API is defined according to the legacy needs. >> -Adapt mmci driver to dedicated constraints of stm32 sdmmc variant, >> defined under some specific properties. >> -Add stm32 sdmmc variant. As Internal DMA way satisfies data >> transfer, the mmci driver hasn't been modified for pio_read/write. >> Specific adds-ons to stm32 sdmmc: >> + registers >> + clk/power functions >> + idma interface >> >> Ludovic Barre (19): >> mmc: mmci: regroup and define dma operations >> mmc: mmci: merge qcom dml feature into mmci dma >> mmc: mmci: add datactrl block size variant property >> mmc: mmci: expand startbiterr to irqmask and error check >> mmc: mmci: allow to overwrite clock/power procedure to specific >> variant >> mmc: mmci: add variant properties to define cpsm & cmdresp bits >> mmc: mmci: add variant property to define dpsm bit >> mmc: mmci: add variant property to define irq pio mask >> mmc: mmci: add variant property to write datactrl before command >> mmc: mmci: add variant property to allow remain data >> mmc: mmci: add variant property to check specific data constraint >> mmc: mmci: add variant property to request a reset >> mmc: mmci: send stop cmd if a data command fail >> mmc: mmci: add clock divider for stm32 sdmmc >> mmc: mmci: add stm32 sdmmc registers >> mmc: mmci: add DT bindings for STM32 sdmmc >> mmc: mmci: add stm32 sdmmc idma support >> mmc: mmci: add specific clk/pwr procedure for stm32 sdmmc >> mmc: mmci: add stm32 sdmmc variant >> >> Documentation/devicetree/bindings/mmc/mmci.txt | 11 + >> drivers/mmc/host/Makefile | 3 +- >> drivers/mmc/host/mmci.c | 846 >> +++++++++++-------------- >> drivers/mmc/host/mmci.h | 237 ++++++- >> drivers/mmc/host/mmci_dma.c | 780 >> +++++++++++++++++++++++ >> drivers/mmc/host/mmci_dma.h | 33 + >> drivers/mmc/host/mmci_qcom_dml.c | 177 ------ >> drivers/mmc/host/mmci_qcom_dml.h | 31 - >> 8 files changed, 1410 insertions(+), 708 deletions(-) >> create mode 100644 drivers/mmc/host/mmci_dma.c >> create mode 100644 drivers/mmc/host/mmci_dma.h >> delete mode 100644 drivers/mmc/host/mmci_qcom_dml.c >> delete mode 100644 drivers/mmc/host/mmci_qcom_dml.h >> > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote: > From: Ludovic Barre <ludovic.barre@st.com> > > The mmc framework follows the requirement of SD_Specification: > the STOP_TRANSMISSION is sent on multiple write/read commands > and the stop command (alone), not needed on other ADTC commands. > > But, some variants require a stop command "STOP_TRANSMISION" to clear > the DPSM "Data Path State Machine" if an error happens on command or data > step. If it's not done the next data command will freeze hardware block. > Needed to support the STM32 sdmmc variant. > > Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > --- > drivers/mmc/host/mmci.c | 36 +++++++++++++++++++++++++++++++----- > drivers/mmc/host/mmci.h | 3 +++ > 2 files changed, 34 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index 87724e1..9af7db8 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -24,6 +24,7 @@ > #include <linux/mmc/pm.h> > #include <linux/mmc/host.h> > #include <linux/mmc/card.h> > +#include <linux/mmc/mmc.h> > #include <linux/mmc/slot-gpio.h> > #include <linux/amba/bus.h> > #include <linux/clk.h> > @@ -522,11 +523,28 @@ static void mmci_set_mask1(struct mmci_host *host, unsigned int mask) > host->mask1_reg = mask; > } > > -static void mmci_stop_data(struct mmci_host *host) > +static int mmci_stop_data(struct mmci_host *host) Let's leave this as void function and instead add a check in mmci_start_command(), to see if the command is a MMC_STOP_TRANSMISSION. If so, then add host->variant->cmdreg_stop to the register that is written in it. And actually, I think this change alone should be a separate patch coming before $subject patch in the series, as it addresses the first problem of two. > { > + struct mmc_command *stop = &host->stop_abort; > + struct mmc_data *data = host->data; > + unsigned int cmd = 0; > + > mmci_write_datactrlreg(host, 0); > mmci_set_mask1(host, 0); > host->data = NULL; > + > + if (host->variant->cmdreg_stop) { > + cmd |= host->variant->cmdreg_stop; > + if (!data->stop) { > + memset(stop, 0, sizeof(struct mmc_command)); > + stop->opcode = MMC_STOP_TRANSMISSION; > + stop->arg = 0; > + stop->flags = MMC_RSP_R1B | MMC_CMD_AC; > + data->stop = stop; I don't understand why you want the host->stop_abort being present in the mmci host struct? Can we get rid of that? Anyway, re-using the data->stop pointer seems reasonable. > + } > + } > + > + return cmd; > } > > static void mmci_init_sg(struct mmci_host *host, struct mmc_data *data) > @@ -703,6 +721,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data, > unsigned int status) > { > unsigned int status_err; > + unsigned int cmd_reg = 0; > > /* Make sure we have data to handle */ > if (!data) > @@ -761,7 +780,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data, > if (status & MCI_DATAEND || data->error) { > mmci_dma_finalize(host, data); > > - mmci_stop_data(host); > + cmd_reg = mmci_stop_data(host); > > if (!data->error) > /* The error clause is handled above, success! */ > @@ -770,7 +789,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data, > if (!data->stop || host->mrq->sbc) { > mmci_request_end(host, data->mrq); > } else { > - mmci_start_command(host, data->stop, 0); > + mmci_start_command(host, data->stop, cmd_reg); > } > } > } > @@ -780,6 +799,8 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, > unsigned int status) > { > void __iomem *base = host->base; > + struct mmc_data *data = host->data; > + unsigned int cmd_reg = 0; > bool sbc; > > if (!cmd) > @@ -865,11 +886,16 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, > } > > if ((!sbc && !cmd->data) || cmd->error) { > - if (host->data) { > + if (data) { > /* Terminate the DMA transfer */ > mmci_dma_error(host); > > - mmci_stop_data(host); > + cmd_reg = mmci_stop_data(host); > + > + if (data->stop) { This means a change in behavior for !host->variant->cmdreg_stop variants, because the mmc core may have assigned data->stop. Did you test this path for any of the old variants? The change as such anyway seems like a good idea. If we had data, we may play safe and send a stop command to let the card to try to close down the transfer. On the other hand, we don't want this to come as side-effect, but rather it deserves is own standalone patch. I guess there are two options going forward. 1) Split the patch and see if this works for other variants first and the add your changes on top for the new ST variant. 2) Make this change only to affect the new ST variant. I am fine with both. > + mmci_start_command(host, data->stop, cmd_reg); > + return; > + } > } > mmci_request_end(host, host->mrq); > } else if (sbc) { > diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h > index 2d7e901..4b4cd1d 100644 > --- a/drivers/mmc/host/mmci.h > +++ b/drivers/mmc/host/mmci.h > @@ -243,6 +243,7 @@ struct mmci_host; > * @cmdreg_lrsp_crc: enable value for long response with crc > * @cmdreg_srsp_crc: enable value for short response with crc > * @cmdreg_srsp: enable value for short response without crc > + * @cmdreg_stop: enable value for stop and abort transmission > * @datalength_bits: number of bits in the MMCIDATALENGTH register > * @fifosize: number of bytes that can be written when MMCI_TXFIFOEMPTY > * is asserted (likewise for RX) > @@ -298,6 +299,7 @@ struct variant_data { > unsigned int cmdreg_lrsp_crc; > unsigned int cmdreg_srsp_crc; > unsigned int cmdreg_srsp; > + unsigned int cmdreg_stop; > unsigned int datalength_bits; > unsigned int fifosize; > unsigned int fifohalfsize; > @@ -343,6 +345,7 @@ struct mmci_host { > struct mmc_request *mrq; > struct mmc_command *cmd; > struct mmc_data *data; > + struct mmc_command stop_abort; > struct mmc_host *mmc; > struct clk *clk; > bool singleirq; > -- > 2.7.4 > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote: > From: Ludovic Barre <ludovic.barre@st.com> > > A specific variant could have different power or clock procedures. > This patch allows to overwrite the default mmci_set_clkreg and > mmci_set_pwrreg for a specific variant. > > Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > --- > drivers/mmc/host/mmci.c | 96 +++++++++++++++++++++++++++++-------------------- > drivers/mmc/host/mmci.h | 7 ++++ > 2 files changed, 64 insertions(+), 39 deletions(-) > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index ede95b7..801c86b 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -374,6 +374,52 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired) > mmci_write_clkreg(host, clk); > } > > +static void mmci_set_pwrreg(struct mmci_host *host, unsigned char power_mode, > + unsigned int pwr) > +{ > + struct variant_data *variant = host->variant; > + struct mmc_host *mmc = host->mmc; > + > + switch (power_mode) { > + case MMC_POWER_OFF: > + if (!IS_ERR(mmc->supply.vmmc)) > + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0); > + > + if (!IS_ERR(mmc->supply.vqmmc) && host->vqmmc_enabled) { > + regulator_disable(mmc->supply.vqmmc); > + host->vqmmc_enabled = false; > + } > + > + break; > + case MMC_POWER_UP: > + if (!IS_ERR(mmc->supply.vmmc)) > + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, > + mmc->ios.vdd); > + > + /* > + * The ST Micro variant doesn't have the PL180s MCI_PWR_UP > + * and instead uses MCI_PWR_ON so apply whatever value is > + * configured in the variant data. > + */ > + pwr |= variant->pwrreg_powerup; > + > + break; > + case MMC_POWER_ON: > + if (!IS_ERR(mmc->supply.vqmmc) && !host->vqmmc_enabled) { > + if (regulator_enable(mmc->supply.vqmmc) < 0) > + dev_err(mmc_dev(mmc), > + "failed to enable vqmmc regulator\n"); > + else > + host->vqmmc_enabled = true; > + } > + > + pwr |= MCI_PWR_ON; > + break; > + } > + > + mmci_write_pwrreg(host, pwr); > +} > + > static void > mmci_request_end(struct mmci_host *host, struct mmc_request *mrq) > { > @@ -1031,7 +1077,7 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > { > struct mmci_host *host = mmc_priv(mmc); > struct variant_data *variant = host->variant; > - u32 pwr = 0; > + unsigned int pwr = 0; ? > unsigned long flags; > int ret; > > @@ -1039,42 +1085,6 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > host->plat->ios_handler(mmc_dev(mmc), ios)) > dev_err(mmc_dev(mmc), "platform ios_handler failed\n"); > > - switch (ios->power_mode) { > - case MMC_POWER_OFF: > - if (!IS_ERR(mmc->supply.vmmc)) > - mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0); > - > - if (!IS_ERR(mmc->supply.vqmmc) && host->vqmmc_enabled) { > - regulator_disable(mmc->supply.vqmmc); > - host->vqmmc_enabled = false; > - } > - > - break; > - case MMC_POWER_UP: > - if (!IS_ERR(mmc->supply.vmmc)) > - mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd); > - > - /* > - * The ST Micro variant doesn't have the PL180s MCI_PWR_UP > - * and instead uses MCI_PWR_ON so apply whatever value is > - * configured in the variant data. > - */ > - pwr |= variant->pwrreg_powerup; > - > - break; > - case MMC_POWER_ON: > - if (!IS_ERR(mmc->supply.vqmmc) && !host->vqmmc_enabled) { > - ret = regulator_enable(mmc->supply.vqmmc); > - if (ret < 0) > - dev_err(mmc_dev(mmc), > - "failed to enable vqmmc regulator\n"); > - else > - host->vqmmc_enabled = true; > - } > - > - pwr |= MCI_PWR_ON; > - break; > - } This above looks like pure re-factoring. Please make the above change a separate patch. > > if (variant->signal_direction && ios->power_mode != MMC_POWER_OFF) { > /* > @@ -1126,8 +1136,16 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > > spin_lock_irqsave(&host->lock, flags); > > - mmci_set_clkreg(host, ios->clock); > - mmci_write_pwrreg(host, pwr); > + if (variant->set_clkreg) > + variant->set_clkreg(host, ios->clock); > + else > + mmci_set_clkreg(host, ios->clock); This means a change in behavior, which I don't think is what you want. 1) The spin lock will be held while doing the regulator operations. 2) The clock register will be written to before the regulator operations have been done. Not sure if that works fine!? An overall comment, I think we should create a mmci_host_ops structure and put the needed callbacks in there (kept to a minimum of course), rather than putting them as a part of the variant struct. More importantly, also the legacy mmci variants should get a mmci_host_ops structure assigned during probe. The point is, I think it makes the code above (and future wise) more flexible. It should also allow us to better share functions between the variants. In principle, I expect that we end up with a bunch of "library" mmci functions that can be invoked from variant's mmci_host_ops (if not assigned directly). > + > + if (variant->set_pwrreg) > + variant->set_pwrreg(host, ios->power_mode, pwr); > + else > + mmci_set_pwrreg(host, ios->power_mode, pwr); > + > mmci_reg_delay(host); > > spin_unlock_irqrestore(&host->lock, flags); > diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h > index 2ba9640..7265ca6 100644 > --- a/drivers/mmc/host/mmci.h > +++ b/drivers/mmc/host/mmci.h > @@ -231,6 +231,7 @@ > > struct clk; > struct dma_chan; > +struct mmci_host; > > /** > * struct variant_data - MMCI variant-specific quirks > @@ -273,6 +274,8 @@ struct dma_chan; > * register. > * @opendrain: bitmask identifying the OPENDRAIN bit inside MMCIPOWER register > * @mmci_dma: Pointer to platform-specific DMA callbacks. > + * @set_clk_ios: if clock procedure of variant is specific > + * @set_pwr_ios: if power procedure of variant is specific > */ > struct variant_data { > unsigned int clkreg; > @@ -307,6 +310,9 @@ struct variant_data { > u32 start_err; > u32 opendrain; > struct mmci_dma_ops *mmci_dma; > + void (*set_clkreg)(struct mmci_host *host, unsigned int desired); > + void (*set_pwrreg)(struct mmci_host *host, unsigned char power_mode, > + unsigned int pwr); > }; > > struct mmci_host { > @@ -328,6 +334,7 @@ struct mmci_host { > u32 pwr_reg; > u32 pwr_reg_add; > u32 clk_reg; > + u32 clk_reg_add; What's this? Some leftover I guess? > u32 datactrl_reg; > u32 busy_status; > u32 mask1_reg; > -- > 2.7.4 > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote: > From: Ludovic Barre <ludovic.barre@st.com> > > This patch adds a boolean property to read remaining data. > Needed to support the STM32 sdmmc variant. MMCIDATACNT > register should be read only after the data transfer is complete. > When reading after an error event the read data count value may be > different from the real number of data bytes transferred. > > Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > --- > drivers/mmc/host/mmci.c | 17 +++++++++++++++-- > drivers/mmc/host/mmci.h | 3 +++ > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index 83c3572..abddad7 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -58,6 +58,7 @@ static struct variant_data variant_arm = { > .datalength_bits = 16, > .datactrl_blocksz = 11, > .datactrl_dpsm_enable = MCI_DPSM_ENABLE, > + .datacnt_remain = true, According to the description in the changelog, it sounds like MMCIDATACNT is broken (or useless for the STM32 variant). I suggest to rename the new variant to something along those lines instead. Like "datacnt_broken", then you only need to set it to true for the STM32 variant and legacy variants can remain as is. > .pwrreg_powerup = MCI_PWR_UP, > .f_max = 100000000, > .reversed_irq_handling = true, > @@ -78,6 +79,7 @@ static struct variant_data variant_arm_extended_fifo = { > .datalength_bits = 16, > .datactrl_blocksz = 11, > .datactrl_dpsm_enable = MCI_DPSM_ENABLE, > + .datacnt_remain = true, > .pwrreg_powerup = MCI_PWR_UP, > .f_max = 100000000, > .mmcimask1 = true, > @@ -98,6 +100,7 @@ static struct variant_data variant_arm_extended_fifo_hwfc = { > .datalength_bits = 16, > .datactrl_blocksz = 11, > .datactrl_dpsm_enable = MCI_DPSM_ENABLE, > + .datacnt_remain = true, > .pwrreg_powerup = MCI_PWR_UP, > .f_max = 100000000, > .mmcimask1 = true, > @@ -120,6 +123,7 @@ static struct variant_data variant_u300 = { > .datactrl_blocksz = 11, > .datactrl_dpsm_enable = MCI_DPSM_ENABLE, > .datactrl_mask_sdio = MCI_DPSM_ST_SDIOEN, > + .datacnt_remain = true, > .st_sdio = true, > .pwrreg_powerup = MCI_PWR_ON, > .f_max = 100000000, > @@ -146,6 +150,7 @@ static struct variant_data variant_nomadik = { > .datactrl_blocksz = 11, > .datactrl_dpsm_enable = MCI_DPSM_ENABLE, > .datactrl_mask_sdio = MCI_DPSM_ST_SDIOEN, > + .datacnt_remain = true, > .st_sdio = true, > .st_clkdiv = true, > .pwrreg_powerup = MCI_PWR_ON, > @@ -175,6 +180,7 @@ static struct variant_data variant_ux500 = { > .datactrl_blocksz = 11, > .datactrl_dpsm_enable = MCI_DPSM_ENABLE, > .datactrl_mask_sdio = MCI_DPSM_ST_SDIOEN, > + .datacnt_remain = true, > .st_sdio = true, > .st_clkdiv = true, > .pwrreg_powerup = MCI_PWR_ON, > @@ -209,6 +215,7 @@ static struct variant_data variant_ux500v2 = { > .datactrl_blocksz = 11, > .datactrl_dpsm_enable = MCI_DPSM_ENABLE, > .datactrl_mask_sdio = MCI_DPSM_ST_SDIOEN, > + .datacnt_remain = true, > .st_sdio = true, > .st_clkdiv = true, > .blksz_datactrl16 = true, > @@ -244,6 +251,7 @@ static struct variant_data variant_stm32 = { > .datactrl_blocksz = 11, > .datactrl_dpsm_enable = MCI_DPSM_ENABLE, > .datactrl_mask_sdio = MCI_DPSM_ST_SDIOEN, > + .datacnt_remain = true, > .st_sdio = true, > .st_clkdiv = true, > .pwrreg_powerup = MCI_PWR_ON, > @@ -270,6 +278,7 @@ static struct variant_data variant_qcom = { > .datalength_bits = 24, > .datactrl_blocksz = 11, > .datactrl_dpsm_enable = MCI_DPSM_ENABLE, > + .datacnt_remain = true, > .pwrreg_powerup = MCI_PWR_UP, > .f_max = 208000000, > .explicit_mclk_control = true, > @@ -711,8 +720,12 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data, > * can be as much as a FIFO-worth of data ahead. This > * matters for FIFO overruns only. > */ > - remain = readl(host->base + MMCIDATACNT); > - success = data->blksz * data->blocks - remain; > + if (host->variant->datacnt_remain) { > + remain = readl(host->base + MMCIDATACNT); > + success = data->blksz * data->blocks - remain; > + } else { > + success = 0; > + } > > dev_dbg(mmc_dev(host->mmc), "MCI ERROR IRQ, status 0x%08x at 0x%08x\n", > status_err, success); > diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h > index 5091025..12ee2e6 100644 > --- a/drivers/mmc/host/mmci.h > +++ b/drivers/mmc/host/mmci.h > @@ -260,6 +260,8 @@ struct mmci_host; > * @datactrl_blksz: block size in power of two > * @datactrl_dpsm_enable: enable value for DPSM > * @datactrl_first: true if data must be setup before send command > + * @datacnt_remain: true if you could read datacnt register > + * to define remain data > * @pwrreg_powerup: power up value for MMCIPOWER register > * @f_max: maximum clk frequency supported by the controller. > * @signal_direction: input/out direction of bus signals can be indicated > @@ -303,6 +305,7 @@ struct variant_data { > unsigned int datactrl_blocksz; > unsigned int datactrl_dpsm_enable; > bool datactrl_first; > + bool datacnt_remain; > bool st_sdio; > bool st_clkdiv; > bool blksz_datactrl16; > -- > 2.7.4 > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote: > From: Ludovic Barre <ludovic.barre@st.com> > > This patch adds command variant properties to define > cpsm enable bit and responses. > Needed to support the STM32 variant (shift of cpsm bit, > specific definition of commands response). > > Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > --- > drivers/mmc/host/mmci.c | 47 +++++++++++++++++++++++++++++++++++++++++++---- > drivers/mmc/host/mmci.h | 8 ++++++++ > 2 files changed, 51 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index 801c86b..52562fc 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -51,6 +51,10 @@ static unsigned int fmax = 515633; > static struct variant_data variant_arm = { > .fifosize = 16 * 4, > .fifohalfsize = 8 * 4, > + .cmdreg_cpsm_enable = MCI_CPSM_ENABLE, > + .cmdreg_lrsp_crc = MCI_CPSM_RESPONSE | MCI_CPSM_LONGRSP, > + .cmdreg_srsp_crc = MCI_CPSM_RESPONSE, > + .cmdreg_srsp = MCI_CPSM_RESPONSE, Do these really needs to be a superset of each other? Maybe it becomes easier for the STM32 variant later though... [...] > @@ -603,16 +639,19 @@ mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c) While at it, would you mind folding in a cleanup patch removing the "u32 c" as in-parameter? It's currently always set to "0" by the caller, so it's not needed. > dev_dbg(mmc_dev(host->mmc), "op %02x arg %08x flags %08x\n", > cmd->opcode, cmd->arg, cmd->flags); > > - if (readl(base + MMCICOMMAND) & MCI_CPSM_ENABLE) { > + if (readl(base + MMCICOMMAND) & host->variant->cmdreg_cpsm_enable) { > writel(0, base + MMCICOMMAND); > mmci_reg_delay(host); > } > > - c |= cmd->opcode | MCI_CPSM_ENABLE; > + c |= cmd->opcode | host->variant->cmdreg_cpsm_enable; > if (cmd->flags & MMC_RSP_PRESENT) { > if (cmd->flags & MMC_RSP_136) > - c |= MCI_CPSM_LONGRSP; > - c |= MCI_CPSM_RESPONSE; > + c |= host->variant->cmdreg_lrsp_crc; This looks weird. Probably because of the naming of the variant data. Perhaps rename to "cmdreg_lrsp", thus skipping the "_crc" suffix? > + else if (cmd->flags & MMC_RSP_CRC) > + c |= host->variant->cmdreg_srsp_crc; Why is here an else? We can have both MMC_RSP_136 and MMC_RSP_CRC bits set, right!? > + else > + c |= host->variant->cmdreg_srsp; What do you think about using a switch-case, perhaps with fall-through - and then adding those bits that are needed for each response bit+variant instead? Could that make this more readable? > } > if (/*interrupt*/0) > c |= MCI_CPSM_INTERRUPT; > diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h > index 7265ca6..e173305 100644 > --- a/drivers/mmc/host/mmci.h > +++ b/drivers/mmc/host/mmci.h > @@ -239,6 +239,10 @@ struct mmci_host; > * @clkreg_enable: enable value for MMCICLOCK register > * @clkreg_8bit_bus_enable: enable value for 8 bit bus > * @clkreg_neg_edge_enable: enable value for inverted data/cmd output > + * @cmdreg_cpsm_enable: enable value for CPSM > + * @cmdreg_lrsp_crc: enable value for long response with crc > + * @cmdreg_srsp_crc: enable value for short response with crc > + * @cmdreg_srsp: enable value for short response without crc > * @datalength_bits: number of bits in the MMCIDATALENGTH register > * @fifosize: number of bytes that can be written when MMCI_TXFIFOEMPTY > * is asserted (likewise for RX) > @@ -282,6 +286,10 @@ struct variant_data { > unsigned int clkreg_enable; > unsigned int clkreg_8bit_bus_enable; > unsigned int clkreg_neg_edge_enable; > + unsigned int cmdreg_cpsm_enable; > + unsigned int cmdreg_lrsp_crc; > + unsigned int cmdreg_srsp_crc; > + unsigned int cmdreg_srsp; > unsigned int datalength_bits; > unsigned int fifosize; > unsigned int fifohalfsize; > -- > 2.7.4 > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote: > From: Ludovic Barre <ludovic.barre@st.com> > > This patch adds specific clock and power ios for stm32 sdmmc variant. > power ios: stm32 dedicated procedure must be done to perform power > off/on procedures. To power off, the sdmmc must be reset and set > to power cycle state before to disabling vqmmc. This drives low > SDMMC_D[7:0], SDMMC_CMD and SDMMC_CK to prevent the Card from > being supplied through the signal lines. > To power on, set the SDMMC in power-off SDMMC_D[7:0], SDMMC_CMD > and SDMMC_CK are driven high. Then we can set the SDMMC to > Power-on state. > > clock ios: specific bits behavior: > -clock divider card_clk = mclk / (2 * clkdiv) > -ddr activation > -wide bus 1/4/8bits > -bus speed > -receive clock selection (in_ck/ck_in/fb_ck) > > Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > --- > drivers/mmc/host/mmci.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 112 insertions(+) > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index 86aef4f..af27a0a 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -50,6 +50,10 @@ > > static unsigned int fmax = 515633; > > +static void mmci_sdmmc_set_pwrreg(struct mmci_host *host, > + unsigned char power_mode, unsigned int pwr); > +static void mmci_sdmmc_set_clkreg(struct mmci_host *host, unsigned int desired); > + > static struct variant_data variant_arm = { > .fifosize = 16 * 4, > .fifohalfsize = 8 * 4, > @@ -490,6 +494,114 @@ static void mmci_set_pwrreg(struct mmci_host *host, unsigned char power_mode, > mmci_write_pwrreg(host, pwr); > } > > +static void mmci_sdmmc_set_clkreg(struct mmci_host *host, unsigned int desired) > +{ > + unsigned int clk = 0, ddr = 0; > + > + if (host->mmc->ios.timing == MMC_TIMING_MMC_DDR52 || > + host->mmc->ios.timing == MMC_TIMING_UHS_DDR50) > + ddr = MCI_STM32_CLK_DDR; > + > + /* > + * cclk = mclk / (2 * clkdiv) > + * clkdiv 0 => bypass > + * in ddr mode bypass is not possible > + */ > + if (desired) { > + if (desired >= host->mclk && !ddr) { > + host->cclk = host->mclk; > + } else { > + clk = DIV_ROUND_UP(host->mclk, 2 * desired); > + if (clk > MCI_STM32_CLK_CLKDIV_MSK) > + clk = MCI_STM32_CLK_CLKDIV_MSK; > + host->cclk = host->mclk / (2 * clk); > + } > + } > + > + if (host->mmc->ios.bus_width == MMC_BUS_WIDTH_4) > + clk |= MCI_STM32_CLK_WIDEBUS_4; > + if (host->mmc->ios.bus_width == MMC_BUS_WIDTH_8) > + clk |= MCI_STM32_CLK_WIDEBUS_8; > + > + clk |= MCI_STM32_CLK_HWFCEN; > + clk |= host->clk_reg_add; > + clk |= ddr; > + > + /* > + * SDMMC_FBCK is selected when an external Delay Block is needed > + * with SDR104. > + */ > + if (host->mmc->ios.timing >= MMC_TIMING_UHS_SDR50) { > + clk |= MCI_STM32_CLK_BUSSPEED; > + if (host->mmc->ios.timing == MMC_TIMING_UHS_SDR104) { > + clk &= ~MCI_STM32_CLK_SEL_MSK; > + clk |= MCI_STM32_CLK_SELFBCK; > + } > + } > + > + mmci_write_clkreg(host, clk); > +} > + > +static void mmci_sdmmc_set_pwrreg(struct mmci_host *host, > + unsigned char power_mode, unsigned int pwr) > +{ > + struct mmc_host *mmc = host->mmc; > + > + pwr |= host->pwr_reg_add; > + > + switch (power_mode) { > + case MMC_POWER_OFF: > + if (!IS_ERR(mmc->supply.vmmc)) > + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0); > + > + /* Only a reset could disable sdmmc */ > + reset_control_assert(host->rst); > + udelay(2); > + reset_control_deassert(host->rst); Could you please elaborate on what goes on here. Do you need to reset-the controller when powering off the card? > + > + /* default mask (probe) must be activated */ > + writel(MCI_IRQENABLE | host->variant->start_err, > + host->base + MMCIMASK0); The above seems like it could be made generic. There is no reason to keep IRQs enabled when the card is powered off. > + > + /* > + * Set the SDMMC in Power-cycle state before to disabling vqmmc. > + * This will make that the SDMMC_D[7:0], SDMMC_CMD and SDMMC_CK > + * are driven low, to prevent the Card from being supplied > + * through the signal lines. > + */ > + mmci_write_pwrreg(host, MCI_STM32_PWR_CYC | pwr); > + > + if (!IS_ERR(host->mmc->supply.vqmmc) && host->vqmmc_enabled) { > + regulator_disable(host->mmc->supply.vqmmc); > + host->vqmmc_enabled = false; > + } > + break; > + case MMC_POWER_UP: > + if (!IS_ERR(mmc->supply.vmmc)) > + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, > + mmc->ios.vdd); > + break; > + case MMC_POWER_ON: > + if (!IS_ERR(host->mmc->supply.vqmmc) && !host->vqmmc_enabled) { > + if (regulator_enable(host->mmc->supply.vqmmc) < 0) > + dev_err(mmc_dev(host->mmc), > + "failed to enable vqmmc regulator\n"); > + else > + host->vqmmc_enabled = true; > + } > + > + /* > + * 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 > + */ > + mmci_write_pwrreg(host, MCI_PWR_OFF | pwr); > + mdelay(1); > + mmci_write_pwrreg(host, MCI_PWR_ON | pwr); This looks odd. MMC_POWER_ON is the last power phase. For every single additional ios change (timing, clock, etc), you will hit this piece of code. In other words, there will be a mdelay(1) each time, which is probably not needed. > + break; > + } > +} > + > static void > mmci_request_end(struct mmci_host *host, struct mmc_request *mrq) > { > -- > 2.7.4 > Overall I am wondering if mmci_sdmmc_set_pwrreg() can be made generic inside the common mmci ->set_ios() functions instead. For example, the reset can be optional and checked with if(!IS_ERR(host->rst)) - and the others can probably be conditional based on a variant data. What do you think? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote: > From: Ludovic Barre <ludovic.barre@st.com> > > Prepare mmci driver to manage dma interface by new property. > This patch defines and regroups dma operations for mmci drivers. > mmci_dma_XX prototypes are added to call member of mmci_dma_ops > if not null. Based on legacy need, a mmci dma interface has been > defined with: > -mmci_dma_setup > -mmci_dma_release > -mmci_dma_pre_req > -mmci_dma_start > -mmci_dma_finalize > -mmci_dma_post_req > -mmci_dma_error > -mmci_dma_get_next_data As I suggested for one of the other patches, I would rather turn core mmci functions into library functions, which can be either invoked from variant callbacks or assigned directly to them. In other words, I would leave the functions that you move in this patch to stay in mmci.c. Although some needs to be re-factored and we also needs to make some of them available to be called from another file, hence the functions needs to be shared via mmci.h rather than being declared static. Let me take a concrete example on how I would move forward, hopefully that explains it a bit better. Please see below. [...] > -/* > - * All the DMA operation mode stuff goes inside this ifdef. > - * This assumes that you have a generic DMA device interface, > - * no custom DMA interfaces are supported. > - */ > -#ifdef CONFIG_DMA_ENGINE > -static void mmci_dma_setup(struct mmci_host *host) > -{ > - const char *rxname, *txname; > - struct variant_data *variant = host->variant; > - > - host->dma_rx_channel = dma_request_slave_channel(mmc_dev(host->mmc), "rx"); > - host->dma_tx_channel = dma_request_slave_channel(mmc_dev(host->mmc), "tx"); > - > - /* initialize pre request cookie */ > - host->next_data.cookie = 1; > - > - /* > - * If only an RX channel is specified, the driver will > - * attempt to use it bidirectionally, however if it is > - * is specified but cannot be located, DMA will be disabled. > - */ > - if (host->dma_rx_channel && !host->dma_tx_channel) > - host->dma_tx_channel = host->dma_rx_channel; > - > - if (host->dma_rx_channel) > - rxname = dma_chan_name(host->dma_rx_channel); > - else > - rxname = "none"; > - > - if (host->dma_tx_channel) > - txname = dma_chan_name(host->dma_tx_channel); > - else > - txname = "none"; > - > - dev_info(mmc_dev(host->mmc), "DMA channels RX %s, TX %s\n", > - rxname, txname); > - > - /* > - * Limit the maximum segment size in any SG entry according to > - * the parameters of the DMA engine device. > - */ > - if (host->dma_tx_channel) { > - struct device *dev = host->dma_tx_channel->device->dev; > - unsigned int max_seg_size = dma_get_max_seg_size(dev); > - > - if (max_seg_size < host->mmc->max_seg_size) > - host->mmc->max_seg_size = max_seg_size; > - } > - if (host->dma_rx_channel) { > - struct device *dev = host->dma_rx_channel->device->dev; > - unsigned int max_seg_size = dma_get_max_seg_size(dev); > - > - if (max_seg_size < host->mmc->max_seg_size) > - host->mmc->max_seg_size = max_seg_size; > - } Everything above shall be left as generic library function, mmci_dma_setup() and I would share it via mmci.h and thus change it from being static. > - > - if (variant->qcom_dml && host->dma_rx_channel && host->dma_tx_channel) > - if (dml_hw_init(host, host->mmc->parent->of_node)) > - variant->qcom_dml = false; This piece of code, should be made specific to the qcom variant and managed though a "mmci_host_ops" callback. The corresponding code in that callback would then start by invoking mmci_dma_setup(), before it continues with the qcom specific operations. For legacy variants, the corresponding callback would be set directly to mmci_dma_setup() and called through the callback from mmci.c when needed. There is no need to have a separate file for DMA for the legacy variants, I think. [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote: > From: Ludovic Barre <ludovic.barre@st.com> > > This patch integrates qcom dml feature into mmci_dma file. > Qualcomm Data Mover lite/local is already a variant of mmci dmaengine. > > Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > --- > drivers/mmc/host/Makefile | 1 - > drivers/mmc/host/mmci.c | 1 - > drivers/mmc/host/mmci.h | 35 ++++++++ > drivers/mmc/host/mmci_dma.c | 135 ++++++++++++++++++++++++++++- > drivers/mmc/host/mmci_qcom_dml.c | 177 --------------------------------------- > drivers/mmc/host/mmci_qcom_dml.h | 31 ------- > 6 files changed, 169 insertions(+), 211 deletions(-) > delete mode 100644 drivers/mmc/host/mmci_qcom_dml.c > delete mode 100644 drivers/mmc/host/mmci_qcom_dml.h No, this is not the way to go. Instead I I think there are two options. 1) Keep mmci_qcom_dml.c|h and thus add new files for the stm32 dma variant. 2) Start by renaming mmci_qcom_dml.* to mmc_dma.* and then in the next step add the code for stm32 dma into the renamed files. I guess if there is some overlap in functionality, 2) may be best as it could easier avoid open coding. However, I am fine with whatever option and I expect that you knows what is best. Kind regards Uffe > > diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile > index daecaa98..608a020 100644 > --- a/drivers/mmc/host/Makefile > +++ b/drivers/mmc/host/Makefile > @@ -5,7 +5,6 @@ > > obj-$(CONFIG_MMC_ARMMMCI) += armmmci.o > armmmci-y := mmci.o mmci_dma.o > -armmmci-$(CONFIG_MMC_QCOM_DML) += mmci_qcom_dml.o > obj-$(CONFIG_MMC_PXA) += pxamci.o > obj-$(CONFIG_MMC_MXC) += mxcmmc.o > obj-$(CONFIG_MMC_MXS) += mxs-mmc.o > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index 8868be0..7a15afd 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -43,7 +43,6 @@ > > #include "mmci.h" > #include "mmci_dma.h" > -#include "mmci_qcom_dml.h" > > #define DRIVER_NAME "mmci-pl18x" > > diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h > index a73bb98..f7cba35 100644 > --- a/drivers/mmc/host/mmci.h > +++ b/drivers/mmc/host/mmci.h > @@ -194,6 +194,41 @@ > > #define MMCI_PINCTRL_STATE_OPENDRAIN "opendrain" > > +/* QCOM DML Registers */ > +#define DML_CONFIG 0x00 > +#define PRODUCER_CRCI_MSK GENMASK(1, 0) > +#define PRODUCER_CRCI_DISABLE 0 > +#define PRODUCER_CRCI_X_SEL BIT(0) > +#define PRODUCER_CRCI_Y_SEL BIT(1) > +#define CONSUMER_CRCI_MSK GENMASK(3, 2) > +#define CONSUMER_CRCI_DISABLE 0 > +#define CONSUMER_CRCI_X_SEL BIT(2) > +#define CONSUMER_CRCI_Y_SEL BIT(3) > +#define PRODUCER_TRANS_END_EN BIT(4) > +#define BYPASS BIT(16) > +#define DIRECT_MODE BIT(17) > +#define INFINITE_CONS_TRANS BIT(18) > + > +#define DML_SW_RESET 0x08 > +#define DML_PRODUCER_START 0x0c > +#define DML_CONSUMER_START 0x10 > +#define DML_PRODUCER_PIPE_LOGICAL_SIZE 0x14 > +#define DML_CONSUMER_PIPE_LOGICAL_SIZE 0x18 > +#define DML_PIPE_ID 0x1c > +#define PRODUCER_PIPE_ID_SHFT 0 > +#define PRODUCER_PIPE_ID_MSK GENMASK(4, 0) > +#define CONSUMER_PIPE_ID_SHFT 16 > +#define CONSUMER_PIPE_ID_MSK GENMASK(20, 16) > + > +#define DML_PRODUCER_BAM_BLOCK_SIZE 0x24 > +#define DML_PRODUCER_BAM_TRANS_SIZE 0x28 > + > +/* other definitions */ > +#define PRODUCER_PIPE_LOGICAL_SIZE 4096 > +#define CONSUMER_PIPE_LOGICAL_SIZE 4096 > + > +#define DML_OFFSET 0x800 > + > struct clk; > struct dma_chan; > > diff --git a/drivers/mmc/host/mmci_dma.c b/drivers/mmc/host/mmci_dma.c > index 98a542d..dd7dae5 100644 > --- a/drivers/mmc/host/mmci_dma.c > +++ b/drivers/mmc/host/mmci_dma.c > @@ -8,11 +8,11 @@ > #include <linux/dmaengine.h> > #include <linux/mmc/host.h> > #include <linux/mmc/card.h> > +#include <linux/of.h> > #include <linux/scatterlist.h> > > #include "mmci.h" > #include "mmci_dma.h" > -#include "mmci_qcom_dml.h" > > int mmci_dma_setup(struct mmci_host *host) > { > @@ -101,6 +101,139 @@ struct dmaengine_priv { > > #define dma_inprogress(dmae) ((dmae)->dma_in_progress) > > +#ifdef CONFIG_MMC_QCOM_DML > +void dml_start_xfer(struct mmci_host *host, struct mmc_data *data) > +{ > + u32 config; > + void __iomem *base = host->base + DML_OFFSET; > + > + if (data->flags & MMC_DATA_READ) { > + /* Read operation: configure DML for producer operation */ > + /* Set producer CRCI-x and disable consumer CRCI */ > + config = readl_relaxed(base + DML_CONFIG); > + config = (config & ~PRODUCER_CRCI_MSK) | PRODUCER_CRCI_X_SEL; > + config = (config & ~CONSUMER_CRCI_MSK) | CONSUMER_CRCI_DISABLE; > + writel_relaxed(config, base + DML_CONFIG); > + > + /* Set the Producer BAM block size */ > + writel_relaxed(data->blksz, base + DML_PRODUCER_BAM_BLOCK_SIZE); > + > + /* Set Producer BAM Transaction size */ > + writel_relaxed(data->blocks * data->blksz, > + base + DML_PRODUCER_BAM_TRANS_SIZE); > + /* Set Producer Transaction End bit */ > + config = readl_relaxed(base + DML_CONFIG); > + config |= PRODUCER_TRANS_END_EN; > + writel_relaxed(config, base + DML_CONFIG); > + /* Trigger producer */ > + writel_relaxed(1, base + DML_PRODUCER_START); > + } else { > + /* Write operation: configure DML for consumer operation */ > + /* Set consumer CRCI-x and disable producer CRCI*/ > + config = readl_relaxed(base + DML_CONFIG); > + config = (config & ~CONSUMER_CRCI_MSK) | CONSUMER_CRCI_X_SEL; > + config = (config & ~PRODUCER_CRCI_MSK) | PRODUCER_CRCI_DISABLE; > + writel_relaxed(config, base + DML_CONFIG); > + /* Clear Producer Transaction End bit */ > + config = readl_relaxed(base + DML_CONFIG); > + config &= ~PRODUCER_TRANS_END_EN; > + writel_relaxed(config, base + DML_CONFIG); > + /* Trigger consumer */ > + writel_relaxed(1, base + DML_CONSUMER_START); > + } > + > + /* make sure the dml is configured before dma is triggered */ > + wmb(); > +} > + > +static int of_get_dml_pipe_index(struct device_node *np, const char *name) > +{ > + int index; > + struct of_phandle_args dma_spec; > + > + index = of_property_match_string(np, "dma-names", name); > + > + if (index < 0) > + return -ENODEV; > + > + if (of_parse_phandle_with_args(np, "dmas", "#dma-cells", index, > + &dma_spec)) > + return -ENODEV; > + > + if (dma_spec.args_count) > + return dma_spec.args[0]; > + > + return -ENODEV; > +} > + > +/* Initialize the dml hardware connected to SD Card controller */ > +int dml_hw_init(struct mmci_host *host, struct device_node *np) > +{ > + u32 config; > + void __iomem *base; > + int consumer_id, producer_id; > + > + consumer_id = of_get_dml_pipe_index(np, "tx"); > + producer_id = of_get_dml_pipe_index(np, "rx"); > + > + if (producer_id < 0 || consumer_id < 0) > + return -ENODEV; > + > + base = host->base + DML_OFFSET; > + > + /* Reset the DML block */ > + writel_relaxed(1, base + DML_SW_RESET); > + > + /* Disable the producer and consumer CRCI */ > + config = (PRODUCER_CRCI_DISABLE | CONSUMER_CRCI_DISABLE); > + /* > + * Disable the bypass mode. Bypass mode will only be used > + * if data transfer is to happen in PIO mode and don't > + * want the BAM interface to connect with SDCC-DML. > + */ > + config &= ~BYPASS; > + /* > + * Disable direct mode as we don't DML to MASTER the AHB bus. > + * BAM connected with DML should MASTER the AHB bus. > + */ > + config &= ~DIRECT_MODE; > + /* > + * Disable infinite mode transfer as we won't be doing any > + * infinite size data transfers. All data transfer will be > + * of finite data size. > + */ > + config &= ~INFINITE_CONS_TRANS; > + writel_relaxed(config, base + DML_CONFIG); > + > + /* > + * Initialize the logical BAM pipe size for producer > + * and consumer. > + */ > + writel_relaxed(PRODUCER_PIPE_LOGICAL_SIZE, > + base + DML_PRODUCER_PIPE_LOGICAL_SIZE); > + writel_relaxed(CONSUMER_PIPE_LOGICAL_SIZE, > + base + DML_CONSUMER_PIPE_LOGICAL_SIZE); > + > + /* Initialize Producer/consumer pipe id */ > + writel_relaxed(producer_id | (consumer_id << CONSUMER_PIPE_ID_SHFT), > + base + DML_PIPE_ID); > + > + /* Make sure dml initialization is finished */ > + mb(); > + > + return 0; > +} > +#else > +static inline int dml_hw_init(struct mmci_host *host, struct device_node *np) > +{ > + return -EINVAL; > +} > + > +static inline void dml_start_xfer(struct mmci_host *host, struct mmc_data *data) > +{ > +} > +#endif /* CONFIG_MMC_QCOM_DML */ > + > static int dmaengine_setup(struct mmci_host *host) > { > const char *rxname, *txname; > diff --git a/drivers/mmc/host/mmci_qcom_dml.c b/drivers/mmc/host/mmci_qcom_dml.c > deleted file mode 100644 > index 00750c9..0000000 > --- a/drivers/mmc/host/mmci_qcom_dml.c > +++ /dev/null > @@ -1,177 +0,0 @@ > -/* > - * > - * Copyright (c) 2011, The Linux Foundation. All rights reserved. > - * > - * This program is free software; you can redistribute it and/or modify > - * it under the terms of the GNU General Public License version 2 and > - * only version 2 as published by the Free Software Foundation. > - * > - * This program is distributed in the hope that it will be useful, > - * but WITHOUT ANY WARRANTY; without even the implied warranty of > - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > - * GNU General Public License for more details. > - * > - */ > -#include <linux/of.h> > -#include <linux/of_dma.h> > -#include <linux/bitops.h> > -#include <linux/mmc/host.h> > -#include <linux/mmc/card.h> > -#include "mmci.h" > - > -/* Registers */ > -#define DML_CONFIG 0x00 > -#define PRODUCER_CRCI_MSK GENMASK(1, 0) > -#define PRODUCER_CRCI_DISABLE 0 > -#define PRODUCER_CRCI_X_SEL BIT(0) > -#define PRODUCER_CRCI_Y_SEL BIT(1) > -#define CONSUMER_CRCI_MSK GENMASK(3, 2) > -#define CONSUMER_CRCI_DISABLE 0 > -#define CONSUMER_CRCI_X_SEL BIT(2) > -#define CONSUMER_CRCI_Y_SEL BIT(3) > -#define PRODUCER_TRANS_END_EN BIT(4) > -#define BYPASS BIT(16) > -#define DIRECT_MODE BIT(17) > -#define INFINITE_CONS_TRANS BIT(18) > - > -#define DML_SW_RESET 0x08 > -#define DML_PRODUCER_START 0x0c > -#define DML_CONSUMER_START 0x10 > -#define DML_PRODUCER_PIPE_LOGICAL_SIZE 0x14 > -#define DML_CONSUMER_PIPE_LOGICAL_SIZE 0x18 > -#define DML_PIPE_ID 0x1c > -#define PRODUCER_PIPE_ID_SHFT 0 > -#define PRODUCER_PIPE_ID_MSK GENMASK(4, 0) > -#define CONSUMER_PIPE_ID_SHFT 16 > -#define CONSUMER_PIPE_ID_MSK GENMASK(20, 16) > - > -#define DML_PRODUCER_BAM_BLOCK_SIZE 0x24 > -#define DML_PRODUCER_BAM_TRANS_SIZE 0x28 > - > -/* other definitions */ > -#define PRODUCER_PIPE_LOGICAL_SIZE 4096 > -#define CONSUMER_PIPE_LOGICAL_SIZE 4096 > - > -#define DML_OFFSET 0x800 > - > -void dml_start_xfer(struct mmci_host *host, struct mmc_data *data) > -{ > - u32 config; > - void __iomem *base = host->base + DML_OFFSET; > - > - if (data->flags & MMC_DATA_READ) { > - /* Read operation: configure DML for producer operation */ > - /* Set producer CRCI-x and disable consumer CRCI */ > - config = readl_relaxed(base + DML_CONFIG); > - config = (config & ~PRODUCER_CRCI_MSK) | PRODUCER_CRCI_X_SEL; > - config = (config & ~CONSUMER_CRCI_MSK) | CONSUMER_CRCI_DISABLE; > - writel_relaxed(config, base + DML_CONFIG); > - > - /* Set the Producer BAM block size */ > - writel_relaxed(data->blksz, base + DML_PRODUCER_BAM_BLOCK_SIZE); > - > - /* Set Producer BAM Transaction size */ > - writel_relaxed(data->blocks * data->blksz, > - base + DML_PRODUCER_BAM_TRANS_SIZE); > - /* Set Producer Transaction End bit */ > - config = readl_relaxed(base + DML_CONFIG); > - config |= PRODUCER_TRANS_END_EN; > - writel_relaxed(config, base + DML_CONFIG); > - /* Trigger producer */ > - writel_relaxed(1, base + DML_PRODUCER_START); > - } else { > - /* Write operation: configure DML for consumer operation */ > - /* Set consumer CRCI-x and disable producer CRCI*/ > - config = readl_relaxed(base + DML_CONFIG); > - config = (config & ~CONSUMER_CRCI_MSK) | CONSUMER_CRCI_X_SEL; > - config = (config & ~PRODUCER_CRCI_MSK) | PRODUCER_CRCI_DISABLE; > - writel_relaxed(config, base + DML_CONFIG); > - /* Clear Producer Transaction End bit */ > - config = readl_relaxed(base + DML_CONFIG); > - config &= ~PRODUCER_TRANS_END_EN; > - writel_relaxed(config, base + DML_CONFIG); > - /* Trigger consumer */ > - writel_relaxed(1, base + DML_CONSUMER_START); > - } > - > - /* make sure the dml is configured before dma is triggered */ > - wmb(); > -} > - > -static int of_get_dml_pipe_index(struct device_node *np, const char *name) > -{ > - int index; > - struct of_phandle_args dma_spec; > - > - index = of_property_match_string(np, "dma-names", name); > - > - if (index < 0) > - return -ENODEV; > - > - if (of_parse_phandle_with_args(np, "dmas", "#dma-cells", index, > - &dma_spec)) > - return -ENODEV; > - > - if (dma_spec.args_count) > - return dma_spec.args[0]; > - > - return -ENODEV; > -} > - > -/* Initialize the dml hardware connected to SD Card controller */ > -int dml_hw_init(struct mmci_host *host, struct device_node *np) > -{ > - u32 config; > - void __iomem *base; > - int consumer_id, producer_id; > - > - consumer_id = of_get_dml_pipe_index(np, "tx"); > - producer_id = of_get_dml_pipe_index(np, "rx"); > - > - if (producer_id < 0 || consumer_id < 0) > - return -ENODEV; > - > - base = host->base + DML_OFFSET; > - > - /* Reset the DML block */ > - writel_relaxed(1, base + DML_SW_RESET); > - > - /* Disable the producer and consumer CRCI */ > - config = (PRODUCER_CRCI_DISABLE | CONSUMER_CRCI_DISABLE); > - /* > - * Disable the bypass mode. Bypass mode will only be used > - * if data transfer is to happen in PIO mode and don't > - * want the BAM interface to connect with SDCC-DML. > - */ > - config &= ~BYPASS; > - /* > - * Disable direct mode as we don't DML to MASTER the AHB bus. > - * BAM connected with DML should MASTER the AHB bus. > - */ > - config &= ~DIRECT_MODE; > - /* > - * Disable infinite mode transfer as we won't be doing any > - * infinite size data transfers. All data transfer will be > - * of finite data size. > - */ > - config &= ~INFINITE_CONS_TRANS; > - writel_relaxed(config, base + DML_CONFIG); > - > - /* > - * Initialize the logical BAM pipe size for producer > - * and consumer. > - */ > - writel_relaxed(PRODUCER_PIPE_LOGICAL_SIZE, > - base + DML_PRODUCER_PIPE_LOGICAL_SIZE); > - writel_relaxed(CONSUMER_PIPE_LOGICAL_SIZE, > - base + DML_CONSUMER_PIPE_LOGICAL_SIZE); > - > - /* Initialize Producer/consumer pipe id */ > - writel_relaxed(producer_id | (consumer_id << CONSUMER_PIPE_ID_SHFT), > - base + DML_PIPE_ID); > - > - /* Make sure dml initialization is finished */ > - mb(); > - > - return 0; > -} > diff --git a/drivers/mmc/host/mmci_qcom_dml.h b/drivers/mmc/host/mmci_qcom_dml.h > deleted file mode 100644 > index 6e405d0..0000000 > --- a/drivers/mmc/host/mmci_qcom_dml.h > +++ /dev/null > @@ -1,31 +0,0 @@ > -/* > - * > - * Copyright (c) 2011, The Linux Foundation. All rights reserved. > - * > - * This program is free software; you can redistribute it and/or modify > - * it under the terms of the GNU General Public License version 2 and > - * only version 2 as published by the Free Software Foundation. > - * > - * This program is distributed in the hope that it will be useful, > - * but WITHOUT ANY WARRANTY; without even the implied warranty of > - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > - * GNU General Public License for more details. > - * > - */ > -#ifndef __MMC_QCOM_DML_H__ > -#define __MMC_QCOM_DML_H__ > - > -#ifdef CONFIG_MMC_QCOM_DML > -int dml_hw_init(struct mmci_host *host, struct device_node *np); > -void dml_start_xfer(struct mmci_host *host, struct mmc_data *data); > -#else > -static inline int dml_hw_init(struct mmci_host *host, struct device_node *np) > -{ > - return -ENOSYS; > -} > -static inline void dml_start_xfer(struct mmci_host *host, struct mmc_data *data) > -{ > -} > -#endif /* CONFIG_MMC_QCOM_DML */ > - > -#endif /* __MMC_QCOM_DML_H__ */ > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
hi Ulf thanks for review, comments below On 07/04/2018 03:37 PM, Ulf Hansson wrote: > On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote: >> From: Ludovic Barre <ludovic.barre@st.com> >> >> The mmc framework follows the requirement of SD_Specification: >> the STOP_TRANSMISSION is sent on multiple write/read commands >> and the stop command (alone), not needed on other ADTC commands. >> >> But, some variants require a stop command "STOP_TRANSMISION" to clear >> the DPSM "Data Path State Machine" if an error happens on command or data >> step. If it's not done the next data command will freeze hardware block. >> Needed to support the STM32 sdmmc variant. >> >> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> >> --- >> drivers/mmc/host/mmci.c | 36 +++++++++++++++++++++++++++++++----- >> drivers/mmc/host/mmci.h | 3 +++ >> 2 files changed, 34 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c >> index 87724e1..9af7db8 100644 >> --- a/drivers/mmc/host/mmci.c >> +++ b/drivers/mmc/host/mmci.c >> @@ -24,6 +24,7 @@ >> #include <linux/mmc/pm.h> >> #include <linux/mmc/host.h> >> #include <linux/mmc/card.h> >> +#include <linux/mmc/mmc.h> >> #include <linux/mmc/slot-gpio.h> >> #include <linux/amba/bus.h> >> #include <linux/clk.h> >> @@ -522,11 +523,28 @@ static void mmci_set_mask1(struct mmci_host *host, unsigned int mask) >> host->mask1_reg = mask; >> } >> >> -static void mmci_stop_data(struct mmci_host *host) >> +static int mmci_stop_data(struct mmci_host *host) > > Let's leave this as void function and instead add a check in > mmci_start_command(), to see if the command is a > MMC_STOP_TRANSMISSION. If so, then add host->variant->cmdreg_stop to > the register that is written in it. In fact: the cmdreg_stop bit (which abort and reinit the DPSM) should have been activated only in error case. But I see a mistake in this piece of code => cmdreg_stop bit is set for all MMC_STOP_TRANSMISSION. So, I read closely the sdmmc datasheet and it seems possible to set cmdreg_stop bit for all MMC_STOP_TRANSMISSION (error or not). So, I will try your proposition to set cmdreg_stop bit in mmci_start_command() if the command is MMC_STOP_TRANSMISSION. > > And actually, I think this change alone should be a separate patch > coming before $subject patch in the series, as it addresses the first > problem of two. > >> { >> + struct mmc_command *stop = &host->stop_abort; >> + struct mmc_data *data = host->data; >> + unsigned int cmd = 0; >> + >> mmci_write_datactrlreg(host, 0); >> mmci_set_mask1(host, 0); >> host->data = NULL; >> + >> + if (host->variant->cmdreg_stop) { >> + cmd |= host->variant->cmdreg_stop; >> + if (!data->stop) { >> + memset(stop, 0, sizeof(struct mmc_command)); >> + stop->opcode = MMC_STOP_TRANSMISSION; >> + stop->arg = 0; >> + stop->flags = MMC_RSP_R1B | MMC_CMD_AC; >> + data->stop = stop; > > I don't understand why you want the host->stop_abort being present in > the mmci host struct? Can we get rid of that? > > Anyway, re-using the data->stop pointer seems reasonable. In fact mrq->data->stop could be not referenced (NULL). for data command, the stop command is allocated in mmc_blk_request struct mmc_blk_request { struct mmc_command stop; }; struct mmc_data { struct mmc_command *stop; }; struct mmc_request { struct mmc_command *stop; }; in mmc_blk_rw_rq_prep function: if data.blocks > 1 brq->mrq.stop = &brq->stop; else brq->mrq.stop = NULL; in mmc_mrq_prep function: if (mrq->stop) { mrq->data->stop = mrq->stop; Update about this topic: This last week I found a new specific need for sdmmc variant. On response type MMC_RSP_BUSY (example mmc_switch command cmd6, AC command without data): SDMMC needs to set MMCIDATATIMER to define busy_timeout. If timeout expires Datatimeout and DPSM flags occur. So a MMC_STOP_TRANSMISSION with cmdreg_stop bit is required to abort and reinitialize the DPSM. So, I think we could keep "host->stop_abort" which may be used in cmd or data requests. The stop_abort command is always the same, so stop_abort command could be initialized only while probe function (under variant->cmdreg_stop). What do you think about it ? > >> + } >> + } >> + >> + return cmd; >> } >> >> static void mmci_init_sg(struct mmci_host *host, struct mmc_data *data) >> @@ -703,6 +721,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data, >> unsigned int status) >> { >> unsigned int status_err; >> + unsigned int cmd_reg = 0; >> >> /* Make sure we have data to handle */ >> if (!data) >> @@ -761,7 +780,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data, >> if (status & MCI_DATAEND || data->error) { >> mmci_dma_finalize(host, data); >> >> - mmci_stop_data(host); >> + cmd_reg = mmci_stop_data(host); >> >> if (!data->error) >> /* The error clause is handled above, success! */ >> @@ -770,7 +789,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data, >> if (!data->stop || host->mrq->sbc) { >> mmci_request_end(host, data->mrq); >> } else { >> - mmci_start_command(host, data->stop, 0); >> + mmci_start_command(host, data->stop, cmd_reg); >> } >> } >> } >> @@ -780,6 +799,8 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, >> unsigned int status) >> { >> void __iomem *base = host->base; >> + struct mmc_data *data = host->data; >> + unsigned int cmd_reg = 0; >> bool sbc; >> >> if (!cmd) >> @@ -865,11 +886,16 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, >> } >> >> if ((!sbc && !cmd->data) || cmd->error) { >> - if (host->data) { >> + if (data) { >> /* Terminate the DMA transfer */ >> mmci_dma_error(host); >> >> - mmci_stop_data(host); >> + cmd_reg = mmci_stop_data(host); >> + >> + if (data->stop) { > > This means a change in behavior for !host->variant->cmdreg_stop > variants, because the mmc core may have assigned data->stop. Did you > test this path for any of the old variants? I just tested on: -stm32f7 => close to pl180 variant. -stm32h7 and mp1 => sdmmc variant. -I like try to snowball but I've not yet find this board. -no arm or qcom boards that seem ok for the both (f7/mp1). > > The change as such anyway seems like a good idea. If we had data, we > may play safe and send a stop command to let the card to try to close > down the transfer. On the other hand, we don't want this to come as > side-effect, but rather it deserves is own standalone patch. > > I guess there are two options going forward. > 1) Split the patch and see if this works for other variants first and > the add your changes on top for the new ST variant. > 2) Make this change only to affect the new ST variant. At first time, I prefer not to have a side effect on other variant. This part must be rework, like I note above in "Update about this topic", the stop transmission with cmdreg_stop bit could be send on command without data (like cmd6: mmc_switch) > > I am fine with both. > >> + mmci_start_command(host, data->stop, cmd_reg); >> + return; >> + } >> } >> mmci_request_end(host, host->mrq); >> } else if (sbc) { >> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h >> index 2d7e901..4b4cd1d 100644 >> --- a/drivers/mmc/host/mmci.h >> +++ b/drivers/mmc/host/mmci.h >> @@ -243,6 +243,7 @@ struct mmci_host; >> * @cmdreg_lrsp_crc: enable value for long response with crc >> * @cmdreg_srsp_crc: enable value for short response with crc >> * @cmdreg_srsp: enable value for short response without crc >> + * @cmdreg_stop: enable value for stop and abort transmission >> * @datalength_bits: number of bits in the MMCIDATALENGTH register >> * @fifosize: number of bytes that can be written when MMCI_TXFIFOEMPTY >> * is asserted (likewise for RX) >> @@ -298,6 +299,7 @@ struct variant_data { >> unsigned int cmdreg_lrsp_crc; >> unsigned int cmdreg_srsp_crc; >> unsigned int cmdreg_srsp; >> + unsigned int cmdreg_stop; >> unsigned int datalength_bits; >> unsigned int fifosize; >> unsigned int fifohalfsize; >> @@ -343,6 +345,7 @@ struct mmci_host { >> struct mmc_request *mrq; >> struct mmc_command *cmd; >> struct mmc_data *data; >> + struct mmc_command stop_abort; >> struct mmc_host *mmc; >> struct clk *clk; >> bool singleirq; >> -- >> 2.7.4 >> > > Kind regards > Uffe > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/05/2018 05:17 PM, Ulf Hansson wrote: > On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote: >> From: Ludovic Barre <ludovic.barre@st.com> >> >> Prepare mmci driver to manage dma interface by new property. >> This patch defines and regroups dma operations for mmci drivers. >> mmci_dma_XX prototypes are added to call member of mmci_dma_ops >> if not null. Based on legacy need, a mmci dma interface has been >> defined with: >> -mmci_dma_setup >> -mmci_dma_release >> -mmci_dma_pre_req >> -mmci_dma_start >> -mmci_dma_finalize >> -mmci_dma_post_req >> -mmci_dma_error >> -mmci_dma_get_next_data > > As I suggested for one of the other patches, I would rather turn core > mmci functions into library functions, which can be either invoked > from variant callbacks or assigned directly to them. > > In other words, I would leave the functions that you move in this > patch to stay in mmci.c. Although some needs to be re-factored and we > also needs to make some of them available to be called from another > file, hence the functions needs to be shared via mmci.h rather than > being declared static. In previous exchange mail "STM32MP1 SDMMC driver review" we are said: >>> -dma variant à should fit in Qualcomm implementation, reuse (rename) >>> mmci_qcom_dml.c file and integrate ST dma in. >> >> stm32 sdmmc has an internal dma, no need to use dmaengine API; >> So some modifications in mmci (pre/post request, mmci_dma_xx). perhaps >> should be done with an ops or not. > >Yes. > >The Qualcomm variant is also using an internal DMA, hence I thought >there may be something we could re-use, or at least have some new >common ops for. It's not crystal clear for me. Do you always agree with a dma ops which allow to address different DMA transfer: -with dmaengine API -sdmmc idma, without dmaengine API -... > > Let me take a concrete example on how I would move forward, hopefully > that explains it a bit better. Please see below. > > [...] > >> -/* >> - * All the DMA operation mode stuff goes inside this ifdef. >> - * This assumes that you have a generic DMA device interface, >> - * no custom DMA interfaces are supported. >> - */ >> -#ifdef CONFIG_DMA_ENGINE >> -static void mmci_dma_setup(struct mmci_host *host) >> -{ >> - const char *rxname, *txname; >> - struct variant_data *variant = host->variant; >> - >> - host->dma_rx_channel = dma_request_slave_channel(mmc_dev(host->mmc), "rx"); >> - host->dma_tx_channel = dma_request_slave_channel(mmc_dev(host->mmc), "tx"); >> - >> - /* initialize pre request cookie */ >> - host->next_data.cookie = 1; >> - >> - /* >> - * If only an RX channel is specified, the driver will >> - * attempt to use it bidirectionally, however if it is >> - * is specified but cannot be located, DMA will be disabled. >> - */ >> - if (host->dma_rx_channel && !host->dma_tx_channel) >> - host->dma_tx_channel = host->dma_rx_channel; >> - >> - if (host->dma_rx_channel) >> - rxname = dma_chan_name(host->dma_rx_channel); >> - else >> - rxname = "none"; >> - >> - if (host->dma_tx_channel) >> - txname = dma_chan_name(host->dma_tx_channel); >> - else >> - txname = "none"; >> - >> - dev_info(mmc_dev(host->mmc), "DMA channels RX %s, TX %s\n", >> - rxname, txname); >> - >> - /* >> - * Limit the maximum segment size in any SG entry according to >> - * the parameters of the DMA engine device. >> - */ >> - if (host->dma_tx_channel) { >> - struct device *dev = host->dma_tx_channel->device->dev; >> - unsigned int max_seg_size = dma_get_max_seg_size(dev); >> - >> - if (max_seg_size < host->mmc->max_seg_size) >> - host->mmc->max_seg_size = max_seg_size; >> - } >> - if (host->dma_rx_channel) { >> - struct device *dev = host->dma_rx_channel->device->dev; >> - unsigned int max_seg_size = dma_get_max_seg_size(dev); >> - >> - if (max_seg_size < host->mmc->max_seg_size) >> - host->mmc->max_seg_size = max_seg_size; >> - } > > Everything above shall be left as generic library function, > mmci_dma_setup() and I would share it via mmci.h and thus change it > from being static. > each interfaces mmci_dma_XXX have very different needs depending dma_ops (legacy, sdmmc idma) >> - >> - if (variant->qcom_dml && host->dma_rx_channel && host->dma_tx_channel) >> - if (dml_hw_init(host, host->mmc->parent->of_node)) >> - variant->qcom_dml = false; > > This piece of code, should be made specific to the qcom variant and > managed though a "mmci_host_ops" callback. The corresponding code in > that callback would then start by invoking mmci_dma_setup(), before it > continues with the qcom specific operations. > > For legacy variants, the corresponding callback would be set directly > to mmci_dma_setup() and called through the callback from mmci.c when > needed. There is no need to have a separate file for DMA for the > legacy variants, I think. > > [...] > > Kind regards > Uffe > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 July 2018 at 11:41, Ludovic BARRE <ludovic.barre@st.com> wrote: > > > On 07/05/2018 05:17 PM, Ulf Hansson wrote: >> >> On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote: >>> >>> From: Ludovic Barre <ludovic.barre@st.com> >>> >>> Prepare mmci driver to manage dma interface by new property. >>> This patch defines and regroups dma operations for mmci drivers. >>> mmci_dma_XX prototypes are added to call member of mmci_dma_ops >>> if not null. Based on legacy need, a mmci dma interface has been >>> defined with: >>> -mmci_dma_setup >>> -mmci_dma_release >>> -mmci_dma_pre_req >>> -mmci_dma_start >>> -mmci_dma_finalize >>> -mmci_dma_post_req >>> -mmci_dma_error >>> -mmci_dma_get_next_data >> >> >> As I suggested for one of the other patches, I would rather turn core >> mmci functions into library functions, which can be either invoked >> from variant callbacks or assigned directly to them. >> >> In other words, I would leave the functions that you move in this >> patch to stay in mmci.c. Although some needs to be re-factored and we >> also needs to make some of them available to be called from another >> file, hence the functions needs to be shared via mmci.h rather than >> being declared static. > > > In previous exchange mail "STM32MP1 SDMMC driver review" > we are said: > >>>> -dma variant à should fit in Qualcomm implementation, reuse (rename) >>>> mmci_qcom_dml.c file and integrate ST dma in. Apologize if I may have lead you in a wrong direction, that was not my intent. However, by looking at $subject patch, your seems to be unnecessarily shuffling code around. I would like to avoid that. >>> >>> stm32 sdmmc has an internal dma, no need to use dmaengine API; >>> So some modifications in mmci (pre/post request, mmci_dma_xx). perhaps >>> should be done with an ops or not. >> >>Yes. >> >>The Qualcomm variant is also using an internal DMA, hence I thought >>there may be something we could re-use, or at least have some new >>common ops for. > > It's not crystal clear for me. > Do you always agree with a dma ops which allow to address different > DMA transfer: > -with dmaengine API > -sdmmc idma, without dmaengine API > -... If we can use a mmci ops callback to manage the variant differences, that would be perfect. That combined with making the existing DMA functions in mmci.c converted to "library" functions, which the mmci ops callbacks can call, in order to re-use code. When that isn't really suitable, we may need to add a "quirk" instead, which would be specific for that particular variant. Along the lines of what we already do for variant specifics inside mmci.c. I think we have to decide on case by case basis, what fits best. Hope this makes a better explanation. If not, please tell, and I can take an initial stab and post a patch to show you with code how I mean to move forward. > > >> >> Let me take a concrete example on how I would move forward, hopefully >> that explains it a bit better. Please see below. >> >> [...] >> >>> -/* >>> - * All the DMA operation mode stuff goes inside this ifdef. >>> - * This assumes that you have a generic DMA device interface, >>> - * no custom DMA interfaces are supported. >>> - */ >>> -#ifdef CONFIG_DMA_ENGINE >>> -static void mmci_dma_setup(struct mmci_host *host) >>> -{ >>> - const char *rxname, *txname; >>> - struct variant_data *variant = host->variant; >>> - >>> - host->dma_rx_channel = >>> dma_request_slave_channel(mmc_dev(host->mmc), "rx"); >>> - host->dma_tx_channel = >>> dma_request_slave_channel(mmc_dev(host->mmc), "tx"); >>> - >>> - /* initialize pre request cookie */ >>> - host->next_data.cookie = 1; >>> - >>> - /* >>> - * If only an RX channel is specified, the driver will >>> - * attempt to use it bidirectionally, however if it is >>> - * is specified but cannot be located, DMA will be disabled. >>> - */ >>> - if (host->dma_rx_channel && !host->dma_tx_channel) >>> - host->dma_tx_channel = host->dma_rx_channel; >>> - >>> - if (host->dma_rx_channel) >>> - rxname = dma_chan_name(host->dma_rx_channel); >>> - else >>> - rxname = "none"; >>> - >>> - if (host->dma_tx_channel) >>> - txname = dma_chan_name(host->dma_tx_channel); >>> - else >>> - txname = "none"; >>> - >>> - dev_info(mmc_dev(host->mmc), "DMA channels RX %s, TX %s\n", >>> - rxname, txname); >>> - >>> - /* >>> - * Limit the maximum segment size in any SG entry according to >>> - * the parameters of the DMA engine device. >>> - */ >>> - if (host->dma_tx_channel) { >>> - struct device *dev = host->dma_tx_channel->device->dev; >>> - unsigned int max_seg_size = dma_get_max_seg_size(dev); >>> - >>> - if (max_seg_size < host->mmc->max_seg_size) >>> - host->mmc->max_seg_size = max_seg_size; >>> - } >>> - if (host->dma_rx_channel) { >>> - struct device *dev = host->dma_rx_channel->device->dev; >>> - unsigned int max_seg_size = dma_get_max_seg_size(dev); >>> - >>> - if (max_seg_size < host->mmc->max_seg_size) >>> - host->mmc->max_seg_size = max_seg_size; >>> - } >> >> >> Everything above shall be left as generic library function, >> mmci_dma_setup() and I would share it via mmci.h and thus change it >> from being static. >> > > each interfaces mmci_dma_XXX have very different needs depending > dma_ops (legacy, sdmmc idma) Right. This was just one example. If I understand, what you are suggesting is to make all of them being variant specific callbacks, so I assume that would solve the problems. Just to be clear, I have no problem with that. Although, that doesn't mean we can't re-use existing dma functions in mmci.c, when that makes sense. [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/05/2018 03:48 PM, Ulf Hansson wrote: > On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote: >> From: Ludovic Barre <ludovic.barre@st.com> >> >> A specific variant could have different power or clock procedures. >> This patch allows to overwrite the default mmci_set_clkreg and >> mmci_set_pwrreg for a specific variant. >> >> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> >> --- >> drivers/mmc/host/mmci.c | 96 +++++++++++++++++++++++++++++-------------------- >> drivers/mmc/host/mmci.h | 7 ++++ >> 2 files changed, 64 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c >> index ede95b7..801c86b 100644 >> --- a/drivers/mmc/host/mmci.c >> +++ b/drivers/mmc/host/mmci.c >> @@ -374,6 +374,52 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired) >> mmci_write_clkreg(host, clk); >> } >> >> +static void mmci_set_pwrreg(struct mmci_host *host, unsigned char power_mode, >> + unsigned int pwr) >> +{ >> + struct variant_data *variant = host->variant; >> + struct mmc_host *mmc = host->mmc; >> + >> + switch (power_mode) { >> + case MMC_POWER_OFF: >> + if (!IS_ERR(mmc->supply.vmmc)) >> + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0); >> + >> + if (!IS_ERR(mmc->supply.vqmmc) && host->vqmmc_enabled) { >> + regulator_disable(mmc->supply.vqmmc); >> + host->vqmmc_enabled = false; >> + } >> + >> + break; >> + case MMC_POWER_UP: >> + if (!IS_ERR(mmc->supply.vmmc)) >> + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, >> + mmc->ios.vdd); >> + >> + /* >> + * The ST Micro variant doesn't have the PL180s MCI_PWR_UP >> + * and instead uses MCI_PWR_ON so apply whatever value is >> + * configured in the variant data. >> + */ >> + pwr |= variant->pwrreg_powerup; >> + >> + break; >> + case MMC_POWER_ON: >> + if (!IS_ERR(mmc->supply.vqmmc) && !host->vqmmc_enabled) { >> + if (regulator_enable(mmc->supply.vqmmc) < 0) >> + dev_err(mmc_dev(mmc), >> + "failed to enable vqmmc regulator\n"); >> + else >> + host->vqmmc_enabled = true; >> + } >> + >> + pwr |= MCI_PWR_ON; >> + break; >> + } >> + >> + mmci_write_pwrreg(host, pwr); >> +} >> + >> static void >> mmci_request_end(struct mmci_host *host, struct mmc_request *mrq) >> { >> @@ -1031,7 +1077,7 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >> { >> struct mmci_host *host = mmc_priv(mmc); >> struct variant_data *variant = host->variant; >> - u32 pwr = 0; >> + unsigned int pwr = 0; > > ? yes not needed rewritten due to re-factoring > >> unsigned long flags; >> int ret; >> >> @@ -1039,42 +1085,6 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >> host->plat->ios_handler(mmc_dev(mmc), ios)) >> dev_err(mmc_dev(mmc), "platform ios_handler failed\n"); >> >> - switch (ios->power_mode) { >> - case MMC_POWER_OFF: >> - if (!IS_ERR(mmc->supply.vmmc)) >> - mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0); >> - >> - if (!IS_ERR(mmc->supply.vqmmc) && host->vqmmc_enabled) { >> - regulator_disable(mmc->supply.vqmmc); >> - host->vqmmc_enabled = false; >> - } >> - >> - break; >> - case MMC_POWER_UP: >> - if (!IS_ERR(mmc->supply.vmmc)) >> - mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd); >> - >> - /* >> - * The ST Micro variant doesn't have the PL180s MCI_PWR_UP >> - * and instead uses MCI_PWR_ON so apply whatever value is >> - * configured in the variant data. >> - */ >> - pwr |= variant->pwrreg_powerup; >> - >> - break; >> - case MMC_POWER_ON: >> - if (!IS_ERR(mmc->supply.vqmmc) && !host->vqmmc_enabled) { >> - ret = regulator_enable(mmc->supply.vqmmc); >> - if (ret < 0) >> - dev_err(mmc_dev(mmc), >> - "failed to enable vqmmc regulator\n"); >> - else >> - host->vqmmc_enabled = true; >> - } >> - >> - pwr |= MCI_PWR_ON; >> - break; >> - } > > This above looks like pure re-factoring. Please make the above change > a separate patch. OK > >> >> if (variant->signal_direction && ios->power_mode != MMC_POWER_OFF) { >> /* >> @@ -1126,8 +1136,16 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >> >> spin_lock_irqsave(&host->lock, flags); >> >> - mmci_set_clkreg(host, ios->clock); >> - mmci_write_pwrreg(host, pwr); >> + if (variant->set_clkreg) >> + variant->set_clkreg(host, ios->clock); >> + else >> + mmci_set_clkreg(host, ios->clock); > > This means a change in behavior, which I don't think is what you want. > > 1) The spin lock will be held while doing the regulator operations. Yes, you are right, the behavior around spin lock has been modified for legacy. I will try to find a solution to avoid behavior change for legacy. For power, sdmmc variant have specific need to reset the sdmmc variant and set power register before to disable the vqmmc regulator. that's why I move the regulator operation in mmci_set_pwrreg. > 2) The clock register will be written to before the regulator > operations have been done. Not sure if that works fine!? you are right, it's probably better if the clock is done after power. do you agree if I change the order? > > An overall comment, I think we should create a mmci_host_ops structure > and put the needed callbacks in there (kept to a minimum of course), > rather than putting them as a part of the variant struct. More > importantly, also the legacy mmci variants should get a mmci_host_ops > structure assigned during probe. > > The point is, I think it makes the code above (and future wise) more > flexible. It should also allow us to better share functions between > the variants. In principle, I expect that we end up with a bunch of > "library" mmci functions that can be invoked from variant's > mmci_host_ops (if not assigned directly). After "Linaro connect" we have exchanged some emails (subject: "STM32MP1 SDMMC driver review") and the start point was "Goal is not to redesign mmci.c like sdhci.c." This it's why I'm surprise by "library" and "mmci_host_ops" and comment of patch 01. So, it's not clear for me on where we wish to go. Could you clarify the way, if it's with a mmci_ops like sdhci_ops? > >> + >> + if (variant->set_pwrreg) >> + variant->set_pwrreg(host, ios->power_mode, pwr); >> + else >> + mmci_set_pwrreg(host, ios->power_mode, pwr); >> + >> mmci_reg_delay(host); >> >> spin_unlock_irqrestore(&host->lock, flags); >> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h >> index 2ba9640..7265ca6 100644 >> --- a/drivers/mmc/host/mmci.h >> +++ b/drivers/mmc/host/mmci.h >> @@ -231,6 +231,7 @@ >> >> struct clk; >> struct dma_chan; >> +struct mmci_host; >> >> /** >> * struct variant_data - MMCI variant-specific quirks >> @@ -273,6 +274,8 @@ struct dma_chan; >> * register. >> * @opendrain: bitmask identifying the OPENDRAIN bit inside MMCIPOWER register >> * @mmci_dma: Pointer to platform-specific DMA callbacks. >> + * @set_clk_ios: if clock procedure of variant is specific >> + * @set_pwr_ios: if power procedure of variant is specific >> */ >> struct variant_data { >> unsigned int clkreg; >> @@ -307,6 +310,9 @@ struct variant_data { >> u32 start_err; >> u32 opendrain; >> struct mmci_dma_ops *mmci_dma; >> + void (*set_clkreg)(struct mmci_host *host, unsigned int desired); >> + void (*set_pwrreg)(struct mmci_host *host, unsigned char power_mode, >> + unsigned int pwr); >> }; >> >> struct mmci_host { >> @@ -328,6 +334,7 @@ struct mmci_host { >> u32 pwr_reg; >> u32 pwr_reg_add; >> u32 clk_reg; >> + u32 clk_reg_add; > > What's this? Some leftover I guess? > >> u32 datactrl_reg; >> u32 busy_status; >> u32 mask1_reg; >> -- >> 2.7.4 >> > > Kind regards > Uffe > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] >> 2) The clock register will be written to before the regulator >> operations have been done. Not sure if that works fine!? > > > you are right, it's probably better if the clock is done after power. > do you agree if I change the order? If the new ST variant can cope with the existing order in the current code, then let's stick to that to avoid breaking legacy variants. > >> >> An overall comment, I think we should create a mmci_host_ops structure >> and put the needed callbacks in there (kept to a minimum of course), >> rather than putting them as a part of the variant struct. More >> importantly, also the legacy mmci variants should get a mmci_host_ops >> structure assigned during probe. >> >> The point is, I think it makes the code above (and future wise) more >> flexible. It should also allow us to better share functions between >> the variants. In principle, I expect that we end up with a bunch of >> "library" mmci functions that can be invoked from variant's >> mmci_host_ops (if not assigned directly). > > > After "Linaro connect" we have exchanged some emails (subject: "STM32MP1 > SDMMC driver review") and the start point was > "Goal is not to redesign mmci.c like sdhci.c." > This it's why I'm surprise by "library" and "mmci_host_ops" > and comment of patch 01. Sometimes it's easier to look at patches/code, instead of giving qualified guesses what is the best solution. Try something, realize what is better, then try again... The point is, we need callbacks to cope with the variants as we can't have quirks for everything, and you are demonstrating that in $subject patch. Since this is the case, then I think it's better to make it more flexible, already from the beginning and thus put the callbacks in a new mmci host ops structure instead. > > So, it's not clear for me on where we wish to go. > Could you clarify the way, if it's with a mmci_ops like sdhci_ops? Yep, something along those lines. However, we don't want to introduce unnecessary callbacks and complexity. We should strive towards sharing common functionality through library functions in mmci.c, rather than having callbacks for every single difference. Does it makes sense? [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/05/2018 05:26 PM, Ulf Hansson wrote: > On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote: >> From: Ludovic Barre <ludovic.barre@st.com> >> >> This patch integrates qcom dml feature into mmci_dma file. >> Qualcomm Data Mover lite/local is already a variant of mmci dmaengine. >> >> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> >> --- >> drivers/mmc/host/Makefile | 1 - >> drivers/mmc/host/mmci.c | 1 - >> drivers/mmc/host/mmci.h | 35 ++++++++ >> drivers/mmc/host/mmci_dma.c | 135 ++++++++++++++++++++++++++++- >> drivers/mmc/host/mmci_qcom_dml.c | 177 --------------------------------------- >> drivers/mmc/host/mmci_qcom_dml.h | 31 ------- >> 6 files changed, 169 insertions(+), 211 deletions(-) >> delete mode 100644 drivers/mmc/host/mmci_qcom_dml.c >> delete mode 100644 drivers/mmc/host/mmci_qcom_dml.h > > No, this is not the way to go. Instead I I think there are two options. > > 1) Keep mmci_qcom_dml.c|h and thus add new files for the stm32 dma variant. > > 2) Start by renaming mmci_qcom_dml.* to mmc_dma.* and then in the next > step add the code for stm32 dma into the renamed files. > > I guess if there is some overlap in functionality, 2) may be best as > it could easier avoid open coding. However, I am fine with whatever > option and I expect that you knows what is best. After patch 01 & 05 comments: I will try to define a mmci_ops which contain some functions pointer called by mmci.c (core). A variant defines its mmci_ops. where do you define the specific function: -in a single file, mmci-ops.c or other (for the name, I'm not inspirated) -or in specific file for each variant mmci-qcom.c or mmci-stm32.c following the comment (above), I think we define a single file? > > Kind regards > Uffe > >> >> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile >> index daecaa98..608a020 100644 >> --- a/drivers/mmc/host/Makefile >> +++ b/drivers/mmc/host/Makefile >> @@ -5,7 +5,6 @@ >> >> obj-$(CONFIG_MMC_ARMMMCI) += armmmci.o >> armmmci-y := mmci.o mmci_dma.o >> -armmmci-$(CONFIG_MMC_QCOM_DML) += mmci_qcom_dml.o >> obj-$(CONFIG_MMC_PXA) += pxamci.o >> obj-$(CONFIG_MMC_MXC) += mxcmmc.o >> obj-$(CONFIG_MMC_MXS) += mxs-mmc.o >> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c >> index 8868be0..7a15afd 100644 >> --- a/drivers/mmc/host/mmci.c >> +++ b/drivers/mmc/host/mmci.c >> @@ -43,7 +43,6 @@ >> >> #include "mmci.h" >> #include "mmci_dma.h" >> -#include "mmci_qcom_dml.h" >> >> #define DRIVER_NAME "mmci-pl18x" >> >> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h >> index a73bb98..f7cba35 100644 >> --- a/drivers/mmc/host/mmci.h >> +++ b/drivers/mmc/host/mmci.h >> @@ -194,6 +194,41 @@ >> >> #define MMCI_PINCTRL_STATE_OPENDRAIN "opendrain" >> >> +/* QCOM DML Registers */ >> +#define DML_CONFIG 0x00 >> +#define PRODUCER_CRCI_MSK GENMASK(1, 0) >> +#define PRODUCER_CRCI_DISABLE 0 >> +#define PRODUCER_CRCI_X_SEL BIT(0) >> +#define PRODUCER_CRCI_Y_SEL BIT(1) >> +#define CONSUMER_CRCI_MSK GENMASK(3, 2) >> +#define CONSUMER_CRCI_DISABLE 0 >> +#define CONSUMER_CRCI_X_SEL BIT(2) >> +#define CONSUMER_CRCI_Y_SEL BIT(3) >> +#define PRODUCER_TRANS_END_EN BIT(4) >> +#define BYPASS BIT(16) >> +#define DIRECT_MODE BIT(17) >> +#define INFINITE_CONS_TRANS BIT(18) >> + >> +#define DML_SW_RESET 0x08 >> +#define DML_PRODUCER_START 0x0c >> +#define DML_CONSUMER_START 0x10 >> +#define DML_PRODUCER_PIPE_LOGICAL_SIZE 0x14 >> +#define DML_CONSUMER_PIPE_LOGICAL_SIZE 0x18 >> +#define DML_PIPE_ID 0x1c >> +#define PRODUCER_PIPE_ID_SHFT 0 >> +#define PRODUCER_PIPE_ID_MSK GENMASK(4, 0) >> +#define CONSUMER_PIPE_ID_SHFT 16 >> +#define CONSUMER_PIPE_ID_MSK GENMASK(20, 16) >> + >> +#define DML_PRODUCER_BAM_BLOCK_SIZE 0x24 >> +#define DML_PRODUCER_BAM_TRANS_SIZE 0x28 >> + >> +/* other definitions */ >> +#define PRODUCER_PIPE_LOGICAL_SIZE 4096 >> +#define CONSUMER_PIPE_LOGICAL_SIZE 4096 >> + >> +#define DML_OFFSET 0x800 >> + >> struct clk; >> struct dma_chan; >> >> diff --git a/drivers/mmc/host/mmci_dma.c b/drivers/mmc/host/mmci_dma.c >> index 98a542d..dd7dae5 100644 >> --- a/drivers/mmc/host/mmci_dma.c >> +++ b/drivers/mmc/host/mmci_dma.c >> @@ -8,11 +8,11 @@ >> #include <linux/dmaengine.h> >> #include <linux/mmc/host.h> >> #include <linux/mmc/card.h> >> +#include <linux/of.h> >> #include <linux/scatterlist.h> >> >> #include "mmci.h" >> #include "mmci_dma.h" >> -#include "mmci_qcom_dml.h" >> >> int mmci_dma_setup(struct mmci_host *host) >> { >> @@ -101,6 +101,139 @@ struct dmaengine_priv { >> >> #define dma_inprogress(dmae) ((dmae)->dma_in_progress) >> >> +#ifdef CONFIG_MMC_QCOM_DML >> +void dml_start_xfer(struct mmci_host *host, struct mmc_data *data) >> +{ >> + u32 config; >> + void __iomem *base = host->base + DML_OFFSET; >> + >> + if (data->flags & MMC_DATA_READ) { >> + /* Read operation: configure DML for producer operation */ >> + /* Set producer CRCI-x and disable consumer CRCI */ >> + config = readl_relaxed(base + DML_CONFIG); >> + config = (config & ~PRODUCER_CRCI_MSK) | PRODUCER_CRCI_X_SEL; >> + config = (config & ~CONSUMER_CRCI_MSK) | CONSUMER_CRCI_DISABLE; >> + writel_relaxed(config, base + DML_CONFIG); >> + >> + /* Set the Producer BAM block size */ >> + writel_relaxed(data->blksz, base + DML_PRODUCER_BAM_BLOCK_SIZE); >> + >> + /* Set Producer BAM Transaction size */ >> + writel_relaxed(data->blocks * data->blksz, >> + base + DML_PRODUCER_BAM_TRANS_SIZE); >> + /* Set Producer Transaction End bit */ >> + config = readl_relaxed(base + DML_CONFIG); >> + config |= PRODUCER_TRANS_END_EN; >> + writel_relaxed(config, base + DML_CONFIG); >> + /* Trigger producer */ >> + writel_relaxed(1, base + DML_PRODUCER_START); >> + } else { >> + /* Write operation: configure DML for consumer operation */ >> + /* Set consumer CRCI-x and disable producer CRCI*/ >> + config = readl_relaxed(base + DML_CONFIG); >> + config = (config & ~CONSUMER_CRCI_MSK) | CONSUMER_CRCI_X_SEL; >> + config = (config & ~PRODUCER_CRCI_MSK) | PRODUCER_CRCI_DISABLE; >> + writel_relaxed(config, base + DML_CONFIG); >> + /* Clear Producer Transaction End bit */ >> + config = readl_relaxed(base + DML_CONFIG); >> + config &= ~PRODUCER_TRANS_END_EN; >> + writel_relaxed(config, base + DML_CONFIG); >> + /* Trigger consumer */ >> + writel_relaxed(1, base + DML_CONSUMER_START); >> + } >> + >> + /* make sure the dml is configured before dma is triggered */ >> + wmb(); >> +} >> + >> +static int of_get_dml_pipe_index(struct device_node *np, const char *name) >> +{ >> + int index; >> + struct of_phandle_args dma_spec; >> + >> + index = of_property_match_string(np, "dma-names", name); >> + >> + if (index < 0) >> + return -ENODEV; >> + >> + if (of_parse_phandle_with_args(np, "dmas", "#dma-cells", index, >> + &dma_spec)) >> + return -ENODEV; >> + >> + if (dma_spec.args_count) >> + return dma_spec.args[0]; >> + >> + return -ENODEV; >> +} >> + >> +/* Initialize the dml hardware connected to SD Card controller */ >> +int dml_hw_init(struct mmci_host *host, struct device_node *np) >> +{ >> + u32 config; >> + void __iomem *base; >> + int consumer_id, producer_id; >> + >> + consumer_id = of_get_dml_pipe_index(np, "tx"); >> + producer_id = of_get_dml_pipe_index(np, "rx"); >> + >> + if (producer_id < 0 || consumer_id < 0) >> + return -ENODEV; >> + >> + base = host->base + DML_OFFSET; >> + >> + /* Reset the DML block */ >> + writel_relaxed(1, base + DML_SW_RESET); >> + >> + /* Disable the producer and consumer CRCI */ >> + config = (PRODUCER_CRCI_DISABLE | CONSUMER_CRCI_DISABLE); >> + /* >> + * Disable the bypass mode. Bypass mode will only be used >> + * if data transfer is to happen in PIO mode and don't >> + * want the BAM interface to connect with SDCC-DML. >> + */ >> + config &= ~BYPASS; >> + /* >> + * Disable direct mode as we don't DML to MASTER the AHB bus. >> + * BAM connected with DML should MASTER the AHB bus. >> + */ >> + config &= ~DIRECT_MODE; >> + /* >> + * Disable infinite mode transfer as we won't be doing any >> + * infinite size data transfers. All data transfer will be >> + * of finite data size. >> + */ >> + config &= ~INFINITE_CONS_TRANS; >> + writel_relaxed(config, base + DML_CONFIG); >> + >> + /* >> + * Initialize the logical BAM pipe size for producer >> + * and consumer. >> + */ >> + writel_relaxed(PRODUCER_PIPE_LOGICAL_SIZE, >> + base + DML_PRODUCER_PIPE_LOGICAL_SIZE); >> + writel_relaxed(CONSUMER_PIPE_LOGICAL_SIZE, >> + base + DML_CONSUMER_PIPE_LOGICAL_SIZE); >> + >> + /* Initialize Producer/consumer pipe id */ >> + writel_relaxed(producer_id | (consumer_id << CONSUMER_PIPE_ID_SHFT), >> + base + DML_PIPE_ID); >> + >> + /* Make sure dml initialization is finished */ >> + mb(); >> + >> + return 0; >> +} >> +#else >> +static inline int dml_hw_init(struct mmci_host *host, struct device_node *np) >> +{ >> + return -EINVAL; >> +} >> + >> +static inline void dml_start_xfer(struct mmci_host *host, struct mmc_data *data) >> +{ >> +} >> +#endif /* CONFIG_MMC_QCOM_DML */ >> + >> static int dmaengine_setup(struct mmci_host *host) >> { >> const char *rxname, *txname; >> diff --git a/drivers/mmc/host/mmci_qcom_dml.c b/drivers/mmc/host/mmci_qcom_dml.c >> deleted file mode 100644 >> index 00750c9..0000000 >> --- a/drivers/mmc/host/mmci_qcom_dml.c >> +++ /dev/null >> @@ -1,177 +0,0 @@ >> -/* >> - * >> - * Copyright (c) 2011, The Linux Foundation. All rights reserved. >> - * >> - * This program is free software; you can redistribute it and/or modify >> - * it under the terms of the GNU General Public License version 2 and >> - * only version 2 as published by the Free Software Foundation. >> - * >> - * This program is distributed in the hope that it will be useful, >> - * but WITHOUT ANY WARRANTY; without even the implied warranty of >> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> - * GNU General Public License for more details. >> - * >> - */ >> -#include <linux/of.h> >> -#include <linux/of_dma.h> >> -#include <linux/bitops.h> >> -#include <linux/mmc/host.h> >> -#include <linux/mmc/card.h> >> -#include "mmci.h" >> - >> -/* Registers */ >> -#define DML_CONFIG 0x00 >> -#define PRODUCER_CRCI_MSK GENMASK(1, 0) >> -#define PRODUCER_CRCI_DISABLE 0 >> -#define PRODUCER_CRCI_X_SEL BIT(0) >> -#define PRODUCER_CRCI_Y_SEL BIT(1) >> -#define CONSUMER_CRCI_MSK GENMASK(3, 2) >> -#define CONSUMER_CRCI_DISABLE 0 >> -#define CONSUMER_CRCI_X_SEL BIT(2) >> -#define CONSUMER_CRCI_Y_SEL BIT(3) >> -#define PRODUCER_TRANS_END_EN BIT(4) >> -#define BYPASS BIT(16) >> -#define DIRECT_MODE BIT(17) >> -#define INFINITE_CONS_TRANS BIT(18) >> - >> -#define DML_SW_RESET 0x08 >> -#define DML_PRODUCER_START 0x0c >> -#define DML_CONSUMER_START 0x10 >> -#define DML_PRODUCER_PIPE_LOGICAL_SIZE 0x14 >> -#define DML_CONSUMER_PIPE_LOGICAL_SIZE 0x18 >> -#define DML_PIPE_ID 0x1c >> -#define PRODUCER_PIPE_ID_SHFT 0 >> -#define PRODUCER_PIPE_ID_MSK GENMASK(4, 0) >> -#define CONSUMER_PIPE_ID_SHFT 16 >> -#define CONSUMER_PIPE_ID_MSK GENMASK(20, 16) >> - >> -#define DML_PRODUCER_BAM_BLOCK_SIZE 0x24 >> -#define DML_PRODUCER_BAM_TRANS_SIZE 0x28 >> - >> -/* other definitions */ >> -#define PRODUCER_PIPE_LOGICAL_SIZE 4096 >> -#define CONSUMER_PIPE_LOGICAL_SIZE 4096 >> - >> -#define DML_OFFSET 0x800 >> - >> -void dml_start_xfer(struct mmci_host *host, struct mmc_data *data) >> -{ >> - u32 config; >> - void __iomem *base = host->base + DML_OFFSET; >> - >> - if (data->flags & MMC_DATA_READ) { >> - /* Read operation: configure DML for producer operation */ >> - /* Set producer CRCI-x and disable consumer CRCI */ >> - config = readl_relaxed(base + DML_CONFIG); >> - config = (config & ~PRODUCER_CRCI_MSK) | PRODUCER_CRCI_X_SEL; >> - config = (config & ~CONSUMER_CRCI_MSK) | CONSUMER_CRCI_DISABLE; >> - writel_relaxed(config, base + DML_CONFIG); >> - >> - /* Set the Producer BAM block size */ >> - writel_relaxed(data->blksz, base + DML_PRODUCER_BAM_BLOCK_SIZE); >> - >> - /* Set Producer BAM Transaction size */ >> - writel_relaxed(data->blocks * data->blksz, >> - base + DML_PRODUCER_BAM_TRANS_SIZE); >> - /* Set Producer Transaction End bit */ >> - config = readl_relaxed(base + DML_CONFIG); >> - config |= PRODUCER_TRANS_END_EN; >> - writel_relaxed(config, base + DML_CONFIG); >> - /* Trigger producer */ >> - writel_relaxed(1, base + DML_PRODUCER_START); >> - } else { >> - /* Write operation: configure DML for consumer operation */ >> - /* Set consumer CRCI-x and disable producer CRCI*/ >> - config = readl_relaxed(base + DML_CONFIG); >> - config = (config & ~CONSUMER_CRCI_MSK) | CONSUMER_CRCI_X_SEL; >> - config = (config & ~PRODUCER_CRCI_MSK) | PRODUCER_CRCI_DISABLE; >> - writel_relaxed(config, base + DML_CONFIG); >> - /* Clear Producer Transaction End bit */ >> - config = readl_relaxed(base + DML_CONFIG); >> - config &= ~PRODUCER_TRANS_END_EN; >> - writel_relaxed(config, base + DML_CONFIG); >> - /* Trigger consumer */ >> - writel_relaxed(1, base + DML_CONSUMER_START); >> - } >> - >> - /* make sure the dml is configured before dma is triggered */ >> - wmb(); >> -} >> - >> -static int of_get_dml_pipe_index(struct device_node *np, const char *name) >> -{ >> - int index; >> - struct of_phandle_args dma_spec; >> - >> - index = of_property_match_string(np, "dma-names", name); >> - >> - if (index < 0) >> - return -ENODEV; >> - >> - if (of_parse_phandle_with_args(np, "dmas", "#dma-cells", index, >> - &dma_spec)) >> - return -ENODEV; >> - >> - if (dma_spec.args_count) >> - return dma_spec.args[0]; >> - >> - return -ENODEV; >> -} >> - >> -/* Initialize the dml hardware connected to SD Card controller */ >> -int dml_hw_init(struct mmci_host *host, struct device_node *np) >> -{ >> - u32 config; >> - void __iomem *base; >> - int consumer_id, producer_id; >> - >> - consumer_id = of_get_dml_pipe_index(np, "tx"); >> - producer_id = of_get_dml_pipe_index(np, "rx"); >> - >> - if (producer_id < 0 || consumer_id < 0) >> - return -ENODEV; >> - >> - base = host->base + DML_OFFSET; >> - >> - /* Reset the DML block */ >> - writel_relaxed(1, base + DML_SW_RESET); >> - >> - /* Disable the producer and consumer CRCI */ >> - config = (PRODUCER_CRCI_DISABLE | CONSUMER_CRCI_DISABLE); >> - /* >> - * Disable the bypass mode. Bypass mode will only be used >> - * if data transfer is to happen in PIO mode and don't >> - * want the BAM interface to connect with SDCC-DML. >> - */ >> - config &= ~BYPASS; >> - /* >> - * Disable direct mode as we don't DML to MASTER the AHB bus. >> - * BAM connected with DML should MASTER the AHB bus. >> - */ >> - config &= ~DIRECT_MODE; >> - /* >> - * Disable infinite mode transfer as we won't be doing any >> - * infinite size data transfers. All data transfer will be >> - * of finite data size. >> - */ >> - config &= ~INFINITE_CONS_TRANS; >> - writel_relaxed(config, base + DML_CONFIG); >> - >> - /* >> - * Initialize the logical BAM pipe size for producer >> - * and consumer. >> - */ >> - writel_relaxed(PRODUCER_PIPE_LOGICAL_SIZE, >> - base + DML_PRODUCER_PIPE_LOGICAL_SIZE); >> - writel_relaxed(CONSUMER_PIPE_LOGICAL_SIZE, >> - base + DML_CONSUMER_PIPE_LOGICAL_SIZE); >> - >> - /* Initialize Producer/consumer pipe id */ >> - writel_relaxed(producer_id | (consumer_id << CONSUMER_PIPE_ID_SHFT), >> - base + DML_PIPE_ID); >> - >> - /* Make sure dml initialization is finished */ >> - mb(); >> - >> - return 0; >> -} >> diff --git a/drivers/mmc/host/mmci_qcom_dml.h b/drivers/mmc/host/mmci_qcom_dml.h >> deleted file mode 100644 >> index 6e405d0..0000000 >> --- a/drivers/mmc/host/mmci_qcom_dml.h >> +++ /dev/null >> @@ -1,31 +0,0 @@ >> -/* >> - * >> - * Copyright (c) 2011, The Linux Foundation. All rights reserved. >> - * >> - * This program is free software; you can redistribute it and/or modify >> - * it under the terms of the GNU General Public License version 2 and >> - * only version 2 as published by the Free Software Foundation. >> - * >> - * This program is distributed in the hope that it will be useful, >> - * but WITHOUT ANY WARRANTY; without even the implied warranty of >> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> - * GNU General Public License for more details. >> - * >> - */ >> -#ifndef __MMC_QCOM_DML_H__ >> -#define __MMC_QCOM_DML_H__ >> - >> -#ifdef CONFIG_MMC_QCOM_DML >> -int dml_hw_init(struct mmci_host *host, struct device_node *np); >> -void dml_start_xfer(struct mmci_host *host, struct mmc_data *data); >> -#else >> -static inline int dml_hw_init(struct mmci_host *host, struct device_node *np) >> -{ >> - return -ENOSYS; >> -} >> -static inline void dml_start_xfer(struct mmci_host *host, struct mmc_data *data) >> -{ >> -} >> -#endif /* CONFIG_MMC_QCOM_DML */ >> - >> -#endif /* __MMC_QCOM_DML_H__ */ >> -- >> 2.7.4 >> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/11/2018 02:16 PM, Ulf Hansson wrote: > On 11 July 2018 at 11:41, Ludovic BARRE <ludovic.barre@st.com> wrote: >> >> >> On 07/05/2018 05:17 PM, Ulf Hansson wrote: >>> >>> On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote: >>>> >>>> From: Ludovic Barre <ludovic.barre@st.com> >>>> >>>> Prepare mmci driver to manage dma interface by new property. >>>> This patch defines and regroups dma operations for mmci drivers. >>>> mmci_dma_XX prototypes are added to call member of mmci_dma_ops >>>> if not null. Based on legacy need, a mmci dma interface has been >>>> defined with: >>>> -mmci_dma_setup >>>> -mmci_dma_release >>>> -mmci_dma_pre_req >>>> -mmci_dma_start >>>> -mmci_dma_finalize >>>> -mmci_dma_post_req >>>> -mmci_dma_error >>>> -mmci_dma_get_next_data >>> >>> >>> As I suggested for one of the other patches, I would rather turn core >>> mmci functions into library functions, which can be either invoked >>> from variant callbacks or assigned directly to them. >>> >>> In other words, I would leave the functions that you move in this >>> patch to stay in mmci.c. Although some needs to be re-factored and we >>> also needs to make some of them available to be called from another >>> file, hence the functions needs to be shared via mmci.h rather than >>> being declared static. >> >> >> In previous exchange mail "STM32MP1 SDMMC driver review" >> we are said: >> >>>>> -dma variant à should fit in Qualcomm implementation, reuse (rename) >>>>> mmci_qcom_dml.c file and integrate ST dma in. > > Apologize if I may have lead you in a wrong direction, that was not my intent. > > However, by looking at $subject patch, your seems to be unnecessarily > shuffling code around. I would like to avoid that. > >>>> >>>> stm32 sdmmc has an internal dma, no need to use dmaengine API; >>>> So some modifications in mmci (pre/post request, mmci_dma_xx). perhaps >>>> should be done with an ops or not. >>> >>> Yes. >>> >>> The Qualcomm variant is also using an internal DMA, hence I thought >>> there may be something we could re-use, or at least have some new >>> common ops for. >> >> It's not crystal clear for me. >> Do you always agree with a dma ops which allow to address different >> DMA transfer: >> -with dmaengine API >> -sdmmc idma, without dmaengine API >> -... > > If we can use a mmci ops callback to manage the variant differences, > that would be perfect. That combined with making the existing DMA > functions in mmci.c converted to "library" functions, which the mmci > ops callbacks can call, in order to re-use code. > > When that isn't really suitable, we may need to add a "quirk" instead, > which would be specific for that particular variant. Along the lines > of what we already do for variant specifics inside mmci.c. > > I think we have to decide on case by case basis, what fits best. > > Hope this makes a better explanation. If not, please tell, and I can > take an initial stab and post a patch to show you with code how I mean > to move forward. > >> >> >>> >>> Let me take a concrete example on how I would move forward, hopefully >>> that explains it a bit better. Please see below. >>> >>> [...] >>> >>>> -/* >>>> - * All the DMA operation mode stuff goes inside this ifdef. >>>> - * This assumes that you have a generic DMA device interface, >>>> - * no custom DMA interfaces are supported. >>>> - */ >>>> -#ifdef CONFIG_DMA_ENGINE >>>> -static void mmci_dma_setup(struct mmci_host *host) >>>> -{ >>>> - const char *rxname, *txname; >>>> - struct variant_data *variant = host->variant; >>>> - >>>> - host->dma_rx_channel = >>>> dma_request_slave_channel(mmc_dev(host->mmc), "rx"); >>>> - host->dma_tx_channel = >>>> dma_request_slave_channel(mmc_dev(host->mmc), "tx"); >>>> - >>>> - /* initialize pre request cookie */ >>>> - host->next_data.cookie = 1; >>>> - >>>> - /* >>>> - * If only an RX channel is specified, the driver will >>>> - * attempt to use it bidirectionally, however if it is >>>> - * is specified but cannot be located, DMA will be disabled. >>>> - */ >>>> - if (host->dma_rx_channel && !host->dma_tx_channel) >>>> - host->dma_tx_channel = host->dma_rx_channel; >>>> - >>>> - if (host->dma_rx_channel) >>>> - rxname = dma_chan_name(host->dma_rx_channel); >>>> - else >>>> - rxname = "none"; >>>> - >>>> - if (host->dma_tx_channel) >>>> - txname = dma_chan_name(host->dma_tx_channel); >>>> - else >>>> - txname = "none"; >>>> - >>>> - dev_info(mmc_dev(host->mmc), "DMA channels RX %s, TX %s\n", >>>> - rxname, txname); >>>> - >>>> - /* >>>> - * Limit the maximum segment size in any SG entry according to >>>> - * the parameters of the DMA engine device. >>>> - */ >>>> - if (host->dma_tx_channel) { >>>> - struct device *dev = host->dma_tx_channel->device->dev; >>>> - unsigned int max_seg_size = dma_get_max_seg_size(dev); >>>> - >>>> - if (max_seg_size < host->mmc->max_seg_size) >>>> - host->mmc->max_seg_size = max_seg_size; >>>> - } >>>> - if (host->dma_rx_channel) { >>>> - struct device *dev = host->dma_rx_channel->device->dev; >>>> - unsigned int max_seg_size = dma_get_max_seg_size(dev); >>>> - >>>> - if (max_seg_size < host->mmc->max_seg_size) >>>> - host->mmc->max_seg_size = max_seg_size; >>>> - } >>> >>> >>> Everything above shall be left as generic library function, >>> mmci_dma_setup() and I would share it via mmci.h and thus change it >>> from being static. >>> >> >> each interfaces mmci_dma_XXX have very different needs depending >> dma_ops (legacy, sdmmc idma) > > Right. This was just one example. > > If I understand, what you are suggesting is to make all of them being > variant specific callbacks, so I assume that would solve the problems. > Just to be clear, I have no problem with that. > > Although, that doesn't mean we can't re-use existing dma functions in > mmci.c, when that makes sense. Yes, when examine dmaengine_XX ops and sdmmc_idma_XX ops (in patch 01 and 17) there are few common piece of code. So I think we will have same dma functions pointer in mmci_ops. However, the cookie management may be shared > > [...] > > Kind regards > Uffe > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 July 2018 at 17:19, Ludovic BARRE <ludovic.barre@st.com> wrote: > > > On 07/05/2018 05:26 PM, Ulf Hansson wrote: >> >> On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote: >>> >>> From: Ludovic Barre <ludovic.barre@st.com> >>> >>> This patch integrates qcom dml feature into mmci_dma file. >>> Qualcomm Data Mover lite/local is already a variant of mmci dmaengine. >>> >>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> >>> --- >>> drivers/mmc/host/Makefile | 1 - >>> drivers/mmc/host/mmci.c | 1 - >>> drivers/mmc/host/mmci.h | 35 ++++++++ >>> drivers/mmc/host/mmci_dma.c | 135 ++++++++++++++++++++++++++++- >>> drivers/mmc/host/mmci_qcom_dml.c | 177 >>> --------------------------------------- >>> drivers/mmc/host/mmci_qcom_dml.h | 31 ------- >>> 6 files changed, 169 insertions(+), 211 deletions(-) >>> delete mode 100644 drivers/mmc/host/mmci_qcom_dml.c >>> delete mode 100644 drivers/mmc/host/mmci_qcom_dml.h >> >> >> No, this is not the way to go. Instead I I think there are two options. >> >> 1) Keep mmci_qcom_dml.c|h and thus add new files for the stm32 dma >> variant. >> >> 2) Start by renaming mmci_qcom_dml.* to mmc_dma.* and then in the next >> step add the code for stm32 dma into the renamed files. >> >> I guess if there is some overlap in functionality, 2) may be best as >> it could easier avoid open coding. However, I am fine with whatever >> option and I expect that you knows what is best. > > > After patch 01 & 05 comments: > I will try to define a mmci_ops which contain some functions pointer > called by mmci.c (core). > A variant defines its mmci_ops. > where do you define the specific function: > -in a single file, mmci-ops.c or other (for the name, I'm not inspirated) > -or in specific file for each variant mmci-qcom.c or mmci-stm32.c > > following the comment (above), I think we define a single file? If I understand the question, the problem is how we should assign the mmc host ops, which corresponds to the probed variant data!? I took a stub at it and posted two patches which I think you should be able to build upon. Please have a look. [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/13/2018 01:17 PM, Ulf Hansson wrote: > On 11 July 2018 at 17:19, Ludovic BARRE <ludovic.barre@st.com> wrote: >> >> >> On 07/05/2018 05:26 PM, Ulf Hansson wrote: >>> >>> On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote: >>>> >>>> From: Ludovic Barre <ludovic.barre@st.com> >>>> >>>> This patch integrates qcom dml feature into mmci_dma file. >>>> Qualcomm Data Mover lite/local is already a variant of mmci dmaengine. >>>> >>>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> >>>> --- >>>> drivers/mmc/host/Makefile | 1 - >>>> drivers/mmc/host/mmci.c | 1 - >>>> drivers/mmc/host/mmci.h | 35 ++++++++ >>>> drivers/mmc/host/mmci_dma.c | 135 ++++++++++++++++++++++++++++- >>>> drivers/mmc/host/mmci_qcom_dml.c | 177 >>>> --------------------------------------- >>>> drivers/mmc/host/mmci_qcom_dml.h | 31 ------- >>>> 6 files changed, 169 insertions(+), 211 deletions(-) >>>> delete mode 100644 drivers/mmc/host/mmci_qcom_dml.c >>>> delete mode 100644 drivers/mmc/host/mmci_qcom_dml.h >>> >>> >>> No, this is not the way to go. Instead I I think there are two options. >>> >>> 1) Keep mmci_qcom_dml.c|h and thus add new files for the stm32 dma >>> variant. >>> >>> 2) Start by renaming mmci_qcom_dml.* to mmc_dma.* and then in the next >>> step add the code for stm32 dma into the renamed files. >>> >>> I guess if there is some overlap in functionality, 2) may be best as >>> it could easier avoid open coding. However, I am fine with whatever >>> option and I expect that you knows what is best. >> >> >> After patch 01 & 05 comments: >> I will try to define a mmci_ops which contain some functions pointer >> called by mmci.c (core). >> A variant defines its mmci_ops. >> where do you define the specific function: >> -in a single file, mmci-ops.c or other (for the name, I'm not inspirated) >> -or in specific file for each variant mmci-qcom.c or mmci-stm32.c >> >> following the comment (above), I think we define a single file? > > If I understand the question, the problem is how we should assign the > mmc host ops, which corresponds to the probed variant data!? > > I took a stub at it and posted two patches which I think you should be > able to build upon. Please have a look. I review your patch on mmci_host_ops and init, I agree with your series, I was going in the same direction. The comment above was on file organization, what do you prefer? -a single file with: all callback and all mmci_host_ops of each variant -or each variant in specific file (like sdhci): mmci-qcom.c | mmci-stm32.c ... > > [...] > > Kind regards > Uffe > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13 July 2018 at 15:08, Ludovic BARRE <ludovic.barre@st.com> wrote: > > > On 07/13/2018 01:17 PM, Ulf Hansson wrote: >> >> On 11 July 2018 at 17:19, Ludovic BARRE <ludovic.barre@st.com> wrote: >>> >>> >>> >>> On 07/05/2018 05:26 PM, Ulf Hansson wrote: >>>> >>>> >>>> On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@st.com> wrote: >>>>> >>>>> >>>>> From: Ludovic Barre <ludovic.barre@st.com> >>>>> >>>>> This patch integrates qcom dml feature into mmci_dma file. >>>>> Qualcomm Data Mover lite/local is already a variant of mmci dmaengine. >>>>> >>>>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> >>>>> --- >>>>> drivers/mmc/host/Makefile | 1 - >>>>> drivers/mmc/host/mmci.c | 1 - >>>>> drivers/mmc/host/mmci.h | 35 ++++++++ >>>>> drivers/mmc/host/mmci_dma.c | 135 ++++++++++++++++++++++++++++- >>>>> drivers/mmc/host/mmci_qcom_dml.c | 177 >>>>> --------------------------------------- >>>>> drivers/mmc/host/mmci_qcom_dml.h | 31 ------- >>>>> 6 files changed, 169 insertions(+), 211 deletions(-) >>>>> delete mode 100644 drivers/mmc/host/mmci_qcom_dml.c >>>>> delete mode 100644 drivers/mmc/host/mmci_qcom_dml.h >>>> >>>> >>>> >>>> No, this is not the way to go. Instead I I think there are two options. >>>> >>>> 1) Keep mmci_qcom_dml.c|h and thus add new files for the stm32 dma >>>> variant. >>>> >>>> 2) Start by renaming mmci_qcom_dml.* to mmc_dma.* and then in the next >>>> step add the code for stm32 dma into the renamed files. >>>> >>>> I guess if there is some overlap in functionality, 2) may be best as >>>> it could easier avoid open coding. However, I am fine with whatever >>>> option and I expect that you knows what is best. >>> >>> >>> >>> After patch 01 & 05 comments: >>> I will try to define a mmci_ops which contain some functions pointer >>> called by mmci.c (core). >>> A variant defines its mmci_ops. >>> where do you define the specific function: >>> -in a single file, mmci-ops.c or other (for the name, I'm not inspirated) >>> -or in specific file for each variant mmci-qcom.c or mmci-stm32.c >>> >>> following the comment (above), I think we define a single file? >> >> >> If I understand the question, the problem is how we should assign the >> mmc host ops, which corresponds to the probed variant data!? >> >> I took a stub at it and posted two patches which I think you should be >> able to build upon. Please have a look. > > > I review your patch on mmci_host_ops and init, I agree with your series, > I was going in the same direction. > The comment above was on file organization, what do you prefer? > -a single file with: all callback and all mmci_host_ops of each variant > -or each variant in specific file (like sdhci): mmci-qcom.c | mmci-stm32.c The latter seems better. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Ludovic Barre <ludovic.barre@st.com> This patch series adapts mmci driver to add support for stm32 sdmmc variant. stm32h7 SoC integrates the first revision of stm32 sdmmc. This series is composed of 3 parts: -Prepare mmci driver to manage dma interfaces by adding property. New mmci dma API is defined according to the legacy needs. -Adapt mmci driver to dedicated constraints of stm32 sdmmc variant, defined under some specific properties. -Add stm32 sdmmc variant. As Internal DMA way satisfies data transfer, the mmci driver hasn't been modified for pio_read/write. Specific adds-ons to stm32 sdmmc: + registers + clk/power functions + idma interface Ludovic Barre (19): mmc: mmci: regroup and define dma operations mmc: mmci: merge qcom dml feature into mmci dma mmc: mmci: add datactrl block size variant property mmc: mmci: expand startbiterr to irqmask and error check mmc: mmci: allow to overwrite clock/power procedure to specific variant mmc: mmci: add variant properties to define cpsm & cmdresp bits mmc: mmci: add variant property to define dpsm bit mmc: mmci: add variant property to define irq pio mask mmc: mmci: add variant property to write datactrl before command mmc: mmci: add variant property to allow remain data mmc: mmci: add variant property to check specific data constraint mmc: mmci: add variant property to request a reset mmc: mmci: send stop cmd if a data command fail mmc: mmci: add clock divider for stm32 sdmmc mmc: mmci: add stm32 sdmmc registers mmc: mmci: add DT bindings for STM32 sdmmc mmc: mmci: add stm32 sdmmc idma support mmc: mmci: add specific clk/pwr procedure for stm32 sdmmc mmc: mmci: add stm32 sdmmc variant Documentation/devicetree/bindings/mmc/mmci.txt | 11 + drivers/mmc/host/Makefile | 3 +- drivers/mmc/host/mmci.c | 846 +++++++++++-------------- drivers/mmc/host/mmci.h | 237 ++++++- drivers/mmc/host/mmci_dma.c | 780 +++++++++++++++++++++++ drivers/mmc/host/mmci_dma.h | 33 + drivers/mmc/host/mmci_qcom_dml.c | 177 ------ drivers/mmc/host/mmci_qcom_dml.h | 31 - 8 files changed, 1410 insertions(+), 708 deletions(-) create mode 100644 drivers/mmc/host/mmci_dma.c create mode 100644 drivers/mmc/host/mmci_dma.h delete mode 100644 drivers/mmc/host/mmci_qcom_dml.c delete mode 100644 drivers/mmc/host/mmci_qcom_dml.h