| Message ID | 1572868495-84816-1-git-send-email-jianxin.pan@amlogic.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2] mmc: meson-gx: fix mmc dma operation | expand |
On Mon 04 Nov 2019 at 12:54, Jianxin Pan <jianxin.pan@amlogic.com> wrote: > From: Nan Li <nan.li@amlogic.com> > > In MMC dma transfer, the region requested by dma_map_sg() may be released > by dma_unmap_sg() before the transfer is completed. > > Put the unmap operation in front of mmc_request_done() to avoid this. In the previous thread, you have described what was the issue you found. It would be nice to have this information here > > Fixes: 79ed05e329c3 ("mmc: meson-gx: add support for descriptor chain mode") > Signed-off-by: Nan Li <nan.li@amlogic.com> > Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com> > --- > drivers/mmc/host/meson-gx-mmc.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) Based on Uffe comment I tried something else. Basically, it enables chained mode in the driver only when the framework calls pre/post_req callback. As far as understood, the framework calls this when there is more than one request pending ... which seems to be when chained mode actually make sense ----8<----- diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c index e712315c7e8d..399604b4124d 100644 --- a/drivers/mmc/host/meson-gx-mmc.c +++ b/drivers/mmc/host/meson-gx-mmc.c @@ -126,8 +126,7 @@ #define SD_EMMC_CFG_CMD_GAP 16 /* in clock cycles */ #define SD_EMMC_DESC_BUF_LEN PAGE_SIZE -#define SD_EMMC_PRE_REQ_DONE BIT(0) -#define SD_EMMC_DESC_CHAIN_MODE BIT(1) +#define SD_EMMC_DESC_CHAIN_MODE BIT(0) #define MUX_CLK_NUM_PARENTS 2 @@ -228,7 +227,6 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc, struct mmc_data *data = mrq->data; struct scatterlist *sg; int i; - bool use_desc_chain_mode = true; /* * When Controller DMA cannot directly access DDR memory, disable @@ -251,12 +249,11 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc, /* check for 8 byte alignment */ if (sg->offset & 7) { WARN_ONCE(1, "unaligned scatterlist buffer\n"); - use_desc_chain_mode = false; - break; + return; } - if (use_desc_chain_mode) - data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE; + /* The planets are aligned, let's chain them up */ + data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE; } static inline bool meson_mmc_desc_chain_mode(const struct mmc_data *data) @@ -278,7 +275,6 @@ static void meson_mmc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq) return; meson_mmc_get_transfer_mode(mmc, mrq); - data->host_cookie |= SD_EMMC_PRE_REQ_DONE; if (!meson_mmc_desc_chain_mode(data)) return; @@ -803,25 +799,11 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd) static void meson_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq) { struct meson_host *host = mmc_priv(mmc); - bool needs_pre_post_req = mrq->data && - !(mrq->data->host_cookie & SD_EMMC_PRE_REQ_DONE); - - if (needs_pre_post_req) { - meson_mmc_get_transfer_mode(mmc, mrq); - if (!meson_mmc_desc_chain_mode(mrq->data)) - needs_pre_post_req = false; - } - - if (needs_pre_post_req) - meson_mmc_pre_req(mmc, mrq); /* Stop execution */ writel(0, host->regs + SD_EMMC_START); meson_mmc_start_cmd(mmc, mrq->sbc ?: mrq->cmd); - - if (needs_pre_post_req) - meson_mmc_post_req(mmc, mrq, 0); } static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd) ---->8----- No performance hit AFAICT. From your description, it should address your problem too. > > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > index e712315..7667e8a 100644 > --- a/drivers/mmc/host/meson-gx-mmc.c > +++ b/drivers/mmc/host/meson-gx-mmc.c > @@ -173,6 +173,7 @@ struct meson_host { > int irq; > > bool vqmmc_enabled; > + bool needs_pre_post_req; > }; > > #define CMD_CFG_LENGTH_MASK GENMASK(8, 0) > @@ -654,6 +655,8 @@ static void meson_mmc_request_done(struct mmc_host *mmc, > struct meson_host *host = mmc_priv(mmc); > > host->cmd = NULL; > + if (host->needs_pre_post_req) > + meson_mmc_post_req(mmc, mrq, 0); > mmc_request_done(host->mmc, mrq); > } > > @@ -803,25 +806,23 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd) > static void meson_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq) > { > struct meson_host *host = mmc_priv(mmc); > - bool needs_pre_post_req = mrq->data && > + > + host->needs_pre_post_req = mrq->data && > !(mrq->data->host_cookie & SD_EMMC_PRE_REQ_DONE); > > - if (needs_pre_post_req) { > + if (host->needs_pre_post_req) { > meson_mmc_get_transfer_mode(mmc, mrq); > if (!meson_mmc_desc_chain_mode(mrq->data)) > - needs_pre_post_req = false; > + host->needs_pre_post_req = false; > } > > - if (needs_pre_post_req) > + if (host->needs_pre_post_req) > meson_mmc_pre_req(mmc, mrq); > > /* Stop execution */ > writel(0, host->regs + SD_EMMC_START); > > meson_mmc_start_cmd(mmc, mrq->sbc ?: mrq->cmd); > - > - if (needs_pre_post_req) > - meson_mmc_post_req(mmc, mrq, 0); > } > > static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd)
On Tue 05 Nov 2019 at 02:45, Nan Li <Nan.Li@amlogic.com> wrote: > 在 2019/11/5 0:46, Jerome Brunet 写道: > > > On Mon 04 Nov 2019 at 12:54, Jianxin Pan <jianxin.pan@amlogic.com><mailto:jianxin.pan@amlogic.com> wrote: > > > > From: Nan Li <nan.li@amlogic.com><mailto:nan.li@amlogic.com> > > In MMC dma transfer, the region requested by dma_map_sg() may be released > by dma_unmap_sg() before the transfer is completed. > > Put the unmap operation in front of mmc_request_done() to avoid this. > > > > In the previous thread, you have described what was the issue you found. > It would be nice to have this information here > > > > > Fixes: 79ed05e329c3 ("mmc: meson-gx: add support for descriptor chain mode") > Signed-off-by: Nan Li <nan.li@amlogic.com><mailto:nan.li@amlogic.com> > Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com><mailto:jianxin.pan@amlogic.com> > --- > drivers/mmc/host/meson-gx-mmc.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > > > Based on Uffe comment I tried something else. > > Basically, it enables chained mode in the driver only when the framework > calls pre/post_req callback. As far as understood, the framework calls > this when there is more than one request pending ... which seems to be > when chained mode actually make sense > > Jerome, what you are talking about is the system framework problem when you call pre/post_req, > > which is not related to the patch I submitted. I strongly disagree. As far as I understand from your description, the problem was with the life cycle of the dma mapping. This is tighly related with pre/post req. Just the variable names you have picked clearly show that. > As you said, pre/post_req is called only when there is data to implement the chained mode, > > but it is also possible to cause memory consistency problems, > resulting in incorrect data. The life cycle of the mapping is also taken care of with patch, since dma mapping is no longer handled in .request(). IOW the mapping, if any, will be released after the request is done using .post_req() If you think otherwise, please elaborate. > > Therefore, this patch is added to make memory consistent and obtain real effective information. > > > > ----8<----- > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > index e712315c7e8d..399604b4124d 100644 > --- a/drivers/mmc/host/meson-gx-mmc.c > +++ b/drivers/mmc/host/meson-gx-mmc.c > @@ -126,8 +126,7 @@ > #define SD_EMMC_CFG_CMD_GAP 16 /* in clock cycles */ > #define SD_EMMC_DESC_BUF_LEN PAGE_SIZE > > -#define SD_EMMC_PRE_REQ_DONE BIT(0) > -#define SD_EMMC_DESC_CHAIN_MODE BIT(1) > +#define SD_EMMC_DESC_CHAIN_MODE BIT(0) > > #define MUX_CLK_NUM_PARENTS 2 > > @@ -228,7 +227,6 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc, > struct mmc_data *data = mrq->data; > struct scatterlist *sg; > int i; > - bool use_desc_chain_mode = true; > > /* > * When Controller DMA cannot directly access DDR memory, disable > @@ -251,12 +249,11 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc, > /* check for 8 byte alignment */ > if (sg->offset & 7) { > WARN_ONCE(1, "unaligned scatterlist buffer\n"); > - use_desc_chain_mode = false; > - break; > + return; > } > > - if (use_desc_chain_mode) > - data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE; > + /* The planets are aligned, let's chain them up */ > + data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE; > } > > static inline bool meson_mmc_desc_chain_mode(const struct mmc_data *data) > @@ -278,7 +275,6 @@ static void meson_mmc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq) > return; > > meson_mmc_get_transfer_mode(mmc, mrq); > - data->host_cookie |= SD_EMMC_PRE_REQ_DONE; > > if (!meson_mmc_desc_chain_mode(data)) > return; > @@ -803,25 +799,11 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd) > static void meson_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq) > { > struct meson_host *host = mmc_priv(mmc); > - bool needs_pre_post_req = mrq->data && > - !(mrq->data->host_cookie & SD_EMMC_PRE_REQ_DONE); > - > - if (needs_pre_post_req) { > - meson_mmc_get_transfer_mode(mmc, mrq); > - if (!meson_mmc_desc_chain_mode(mrq->data)) > - needs_pre_post_req = false; > - } > - > - if (needs_pre_post_req) > - meson_mmc_pre_req(mmc, mrq); > > /* Stop execution */ > writel(0, host->regs + SD_EMMC_START); > > meson_mmc_start_cmd(mmc, mrq->sbc ?: mrq->cmd); > - > - if (needs_pre_post_req) > - meson_mmc_post_req(mmc, mrq, 0); > } > > static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd) > ---->8----- > > No performance hit AFAICT. > From your description, it should address your problem too. > > > > > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > index e712315..7667e8a 100644 > --- a/drivers/mmc/host/meson-gx-mmc.c > +++ b/drivers/mmc/host/meson-gx-mmc.c > @@ -173,6 +173,7 @@ struct meson_host { > int irq; > > bool vqmmc_enabled; > + bool needs_pre_post_req; > }; > > #define CMD_CFG_LENGTH_MASK GENMASK(8, 0) > @@ -654,6 +655,8 @@ static void meson_mmc_request_done(struct mmc_host *mmc, > struct meson_host *host = mmc_priv(mmc); > > host->cmd = NULL; > + if (host->needs_pre_post_req) > + meson_mmc_post_req(mmc, mrq, 0); > mmc_request_done(host->mmc, mrq); > } > > @@ -803,25 +806,23 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd) > static void meson_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq) > { > struct meson_host *host = mmc_priv(mmc); > - bool needs_pre_post_req = mrq->data && > + > + host->needs_pre_post_req = mrq->data && > !(mrq->data->host_cookie & SD_EMMC_PRE_REQ_DONE); > > - if (needs_pre_post_req) { > + if (host->needs_pre_post_req) { > meson_mmc_get_transfer_mode(mmc, mrq); > if (!meson_mmc_desc_chain_mode(mrq->data)) > - needs_pre_post_req = false; > + host->needs_pre_post_req = false; > } > > - if (needs_pre_post_req) > + if (host->needs_pre_post_req) > meson_mmc_pre_req(mmc, mrq); > > /* Stop execution */ > writel(0, host->regs + SD_EMMC_START); > > meson_mmc_start_cmd(mmc, mrq->sbc ?: mrq->cmd); > - > - if (needs_pre_post_req) > - meson_mmc_post_req(mmc, mrq, 0); > } > > static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd)
On Tue 05 Nov 2019 at 09:30, Nan Li <Nan.Li@amlogic.com> wrote: > > Based on Uffe comment I tried something else. > > Basically, it enables chained mode in the driver only when the framework > calls pre/post_req callback. As far as understood, the framework calls > this when there is more than one request pending ... which seems to be > when chained mode actually make sense > > Jerome, what you are talking about is the system framework problem when you call pre/post_req, > > which is not related to the patch I submitted. > > > > I strongly disagree. > As far as I understand from your description, the problem was with the > life cycle of the dma mapping. This is tighly related with pre/post req. > Just the variable names you have picked clearly show that. > > > > As you said, pre/post_req is called only when there is data to implement the chained mode, > > but it is also possible to cause memory consistency problems, > resulting in incorrect data. > > > > The life cycle of the mapping is also taken care of with patch, > since dma mapping is no longer handled in .request(). IOW the mapping, > if any, will be released after the request is done using .post_req() > > If you think otherwise, please elaborate. > > > I see what you mean. You want to pull the pre/post_req operation out of the request interface and invoke it at the top. > > I didn't notice the following modification of patch in your last email and reply in time. I'm really sorry. > > The following patch removes all pre/post_req operations, No it does not. Callbacks are still provided to the MMC framework. > but you do not send out the operation patch added to the upper layer > together. There is no modification needed in the upper layer > > Then the patch is incomplete, which will affect the dma data transfer function in start_cmd function and affect the multi-block write operation. > No it is not incomplete. pre and post request are correctly called. You can check that with ftrace if you want. Maybe you could try it ? > Please send your complete patch, including core layer modification, > thank you. > > > > > > Therefore, this patch is added to make memory consistent and obtain real effective information. > > > > ----8<----- > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > index e712315c7e8d..399604b4124d 100644 > --- a/drivers/mmc/host/meson-gx-mmc.c > +++ b/drivers/mmc/host/meson-gx-mmc.c > @@ -126,8 +126,7 @@ > #define SD_EMMC_CFG_CMD_GAP 16 /* in clock cycles */ > #define SD_EMMC_DESC_BUF_LEN PAGE_SIZE > > -#define SD_EMMC_PRE_REQ_DONE BIT(0) > -#define SD_EMMC_DESC_CHAIN_MODE BIT(1) > +#define SD_EMMC_DESC_CHAIN_MODE BIT(0) > > #define MUX_CLK_NUM_PARENTS 2 > > @@ -228,7 +227,6 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc, > struct mmc_data *data = mrq->data; > struct scatterlist *sg; > int i; > - bool use_desc_chain_mode = true; > > /* > * When Controller DMA cannot directly access DDR memory, disable > @@ -251,12 +249,11 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc, > /* check for 8 byte alignment */ > if (sg->offset & 7) { > WARN_ONCE(1, "unaligned scatterlist buffer\n"); > - use_desc_chain_mode = false; > - break; > + return; > } > > - if (use_desc_chain_mode) > - data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE; > + /* The planets are aligned, let's chain them up */ > + data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE; > } > > static inline bool meson_mmc_desc_chain_mode(const struct mmc_data *data) > @@ -278,7 +275,6 @@ static void meson_mmc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq) > return; > > meson_mmc_get_transfer_mode(mmc, mrq); > - data->host_cookie |= SD_EMMC_PRE_REQ_DONE; > > if (!meson_mmc_desc_chain_mode(data)) > return; > @@ -803,25 +799,11 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd) > static void meson_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq) > { > struct meson_host *host = mmc_priv(mmc); > - bool needs_pre_post_req = mrq->data && > - !(mrq->data->host_cookie & SD_EMMC_PRE_REQ_DONE); > - > - if (needs_pre_post_req) { > - meson_mmc_get_transfer_mode(mmc, mrq); > - if (!meson_mmc_desc_chain_mode(mrq->data)) > - needs_pre_post_req = false; > - } > - > - if (needs_pre_post_req) > - meson_mmc_pre_req(mmc, mrq); > > /* Stop execution */ > writel(0, host->regs + SD_EMMC_START); > > meson_mmc_start_cmd(mmc, mrq->sbc ?: mrq->cmd); > - > - if (needs_pre_post_req) > - meson_mmc_post_req(mmc, mrq, 0); > } > > static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd) > ---->8----- > > No performance hit AFAICT. > From your description, it should address your problem too. > > > > > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > index e712315..7667e8a 100644 > --- a/drivers/mmc/host/meson-gx-mmc.c > +++ b/drivers/mmc/host/meson-gx-mmc.c > @@ -173,6 +173,7 @@ struct meson_host { > int irq; > > bool vqmmc_enabled; > + bool needs_pre_post_req; > }; > > #define CMD_CFG_LENGTH_MASK GENMASK(8, 0) > @@ -654,6 +655,8 @@ static void meson_mmc_request_done(struct mmc_host *mmc, > struct meson_host *host = mmc_priv(mmc); > > host->cmd = NULL; > + if (host->needs_pre_post_req) > + meson_mmc_post_req(mmc, mrq, 0); > mmc_request_done(host->mmc, mrq); > } > > @@ -803,25 +806,23 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd) > static void meson_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq) > { > struct meson_host *host = mmc_priv(mmc); > - bool needs_pre_post_req = mrq->data && > + > + host->needs_pre_post_req = mrq->data && > !(mrq->data->host_cookie & SD_EMMC_PRE_REQ_DONE); > > - if (needs_pre_post_req) { > + if (host->needs_pre_post_req) { > meson_mmc_get_transfer_mode(mmc, mrq); > if (!meson_mmc_desc_chain_mode(mrq->data)) > - needs_pre_post_req = false; > + host->needs_pre_post_req = false; > } > > - if (needs_pre_post_req) > + if (host->needs_pre_post_req) > meson_mmc_pre_req(mmc, mrq); > > /* Stop execution */ > writel(0, host->regs + SD_EMMC_START); > > meson_mmc_start_cmd(mmc, mrq->sbc ?: mrq->cmd); > - > - if (needs_pre_post_req) > - meson_mmc_post_req(mmc, mrq, 0); > } > > static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd)
On Tue 05 Nov 2019 at 10:05, Nan Li <Nan.Li@amlogic.com> wrote: Nan Li, please fix your mailer to use plain text properly, your reply are near impossible to read > 在 2019/11/5 16:54, Jerome Brunet 写道: > > > On Tue 05 Nov 2019 at 09:30, Nan Li <Nan.Li@amlogic.com><mailto:Nan.Li@amlogic.com> wrote: > > > > > > Based on Uffe comment I tried something else. > > Basically, it enables chained mode in the driver only when the framework > calls pre/post_req callback. As far as understood, the framework calls > this when there is more than one request pending ... which seems to be > when chained mode actually make sense > > Jerome, what you are talking about is the system framework problem when you call pre/post_req, > > which is not related to the patch I submitted. > > > > I strongly disagree. > As far as I understand from your description, the problem was with the > life cycle of the dma mapping. This is tighly related with pre/post req. > Just the variable names you have picked clearly show that. > > > > As you said, pre/post_req is called only when there is data to implement the chained mode, > > but it is also possible to cause memory consistency problems, > resulting in incorrect data. > > > > The life cycle of the mapping is also taken care of with patch, > since dma mapping is no longer handled in .request(). IOW the mapping, > if any, will be released after the request is done using .post_req() > > If you think otherwise, please elaborate. > > > I see what you mean. You want to pull the pre/post_req operation out of the request interface and invoke it at the top. > > I didn't notice the following modification of patch in your last email and reply in time. I'm really sorry. > > The following patch removes all pre/post_req operations, > > > > No it does not. Callbacks are still provided to the MMC framework. > > > > but you do not send out the operation patch added to the upper layer > together. > > > > There is no modification needed in the upper layer > > > > > Then the patch is incomplete, which will affect the dma data transfer function in start_cmd function and affect the multi-block write operation. > > > > > No it is not incomplete. pre and post request are correctly called. You > can check that with ftrace if you want. > > Maybe you could try it ? > > > I'm sorry, I didn't catch your meaning.How do I use pre/post_req > properly? Those callbacks are provided to the framework as well. If you have trouble following, please apply the patch and look at the driver Have a look at "meson_mmc_ops" > > Do you mean I need to add a call to pre/post_req on top of the changes > you provided below? No, you should not > > If not, your changes have deleted all pre/post_req calls. No, it does not. It relies on the framework to activate the chained mode or not. Again, if you have trouble following you can try to enable ftrace and trace the mmc functions. > > > > > Please send your complete patch, including core layer modification, > thank you. > > > > > > Therefore, this patch is added to make memory consistent and obtain real effective information. > > > > ----8<----- > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > index e712315c7e8d..399604b4124d 100644 > --- a/drivers/mmc/host/meson-gx-mmc.c > +++ b/drivers/mmc/host/meson-gx-mmc.c > @@ -126,8 +126,7 @@ > #define SD_EMMC_CFG_CMD_GAP 16 /* in clock cycles */ > #define SD_EMMC_DESC_BUF_LEN PAGE_SIZE > > -#define SD_EMMC_PRE_REQ_DONE BIT(0) > -#define SD_EMMC_DESC_CHAIN_MODE BIT(1) > +#define SD_EMMC_DESC_CHAIN_MODE BIT(0) > > #define MUX_CLK_NUM_PARENTS 2 > > @@ -228,7 +227,6 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc, > struct mmc_data *data = mrq->data; > struct scatterlist *sg; > int i; > - bool use_desc_chain_mode = true; > > /* > * When Controller DMA cannot directly access DDR memory, disable > @@ -251,12 +249,11 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc, > /* check for 8 byte alignment */ > if (sg->offset & 7) { > WARN_ONCE(1, "unaligned scatterlist buffer\n"); > - use_desc_chain_mode = false; > - break; > + return; > } > > - if (use_desc_chain_mode) > - data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE; > + /* The planets are aligned, let's chain them up */ > + data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE; > } > > static inline bool meson_mmc_desc_chain_mode(const struct mmc_data *data) > @@ -278,7 +275,6 @@ static void meson_mmc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq) > return; > > meson_mmc_get_transfer_mode(mmc, mrq); > - data->host_cookie |= SD_EMMC_PRE_REQ_DONE; > > if (!meson_mmc_desc_chain_mode(data)) > return; > @@ -803,25 +799,11 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd) > static void meson_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq) > { > struct meson_host *host = mmc_priv(mmc); > - bool needs_pre_post_req = mrq->data && > - !(mrq->data->host_cookie & SD_EMMC_PRE_REQ_DONE); > - > - if (needs_pre_post_req) { > - meson_mmc_get_transfer_mode(mmc, mrq); > - if (!meson_mmc_desc_chain_mode(mrq->data)) > - needs_pre_post_req = false; > - } > - > - if (needs_pre_post_req) > - meson_mmc_pre_req(mmc, mrq); > > /* Stop execution */ > writel(0, host->regs + SD_EMMC_START); > > meson_mmc_start_cmd(mmc, mrq->sbc ?: mrq->cmd); > - > - if (needs_pre_post_req) > - meson_mmc_post_req(mmc, mrq, 0); > } > > static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd) > ---->8----- > > No performance hit AFAICT. > From your description, it should address your problem too. > > > > > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > index e712315..7667e8a 100644 > --- a/drivers/mmc/host/meson-gx-mmc.c > +++ b/drivers/mmc/host/meson-gx-mmc.c > @@ -173,6 +173,7 @@ struct meson_host { > int irq; > > bool vqmmc_enabled; > + bool needs_pre_post_req; > }; > > #define CMD_CFG_LENGTH_MASK GENMASK(8, 0) > @@ -654,6 +655,8 @@ static void meson_mmc_request_done(struct mmc_host *mmc, > struct meson_host *host = mmc_priv(mmc); > > host->cmd = NULL; > + if (host->needs_pre_post_req) > + meson_mmc_post_req(mmc, mrq, 0); > mmc_request_done(host->mmc, mrq); > } > > @@ -803,25 +806,23 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd) > static void meson_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq) > { > struct meson_host *host = mmc_priv(mmc); > - bool needs_pre_post_req = mrq->data && > + > + host->needs_pre_post_req = mrq->data && > !(mrq->data->host_cookie & SD_EMMC_PRE_REQ_DONE); > > - if (needs_pre_post_req) { > + if (host->needs_pre_post_req) { > meson_mmc_get_transfer_mode(mmc, mrq); > if (!meson_mmc_desc_chain_mode(mrq->data)) > - needs_pre_post_req = false; > + host->needs_pre_post_req = false; > } > > - if (needs_pre_post_req) > + if (host->needs_pre_post_req) > meson_mmc_pre_req(mmc, mrq); > > /* Stop execution */ > writel(0, host->regs + SD_EMMC_START); > > meson_mmc_start_cmd(mmc, mrq->sbc ?: mrq->cmd); > - > - if (needs_pre_post_req) > - meson_mmc_post_req(mmc, mrq, 0); > } > > static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd)
On 2019/11/5 21:30, Jerome Brunet wrote: > > On Tue 05 Nov 2019 at 10:05, Nan Li <Nan.Li@amlogic.com> wrote: > > Nan Li, please fix your mailer to use plain text properly, your reply > are near impossible to read > Sorry, maybe there is something wrong with my email address. Please help me check whether my reply is in accordance with the format. >> 在 2019/11/5 16:54, Jerome Brunet 写道: >> >> >> On Tue 05 Nov 2019 at 09:30, Nan Li <Nan.Li@amlogic.com><mailto:Nan.Li@amlogic.com> wrote: >> >> >> >> >> >> Based on Uffe comment I tried something else. >> >> Basically, it enables chained mode in the driver only when the framework >> calls pre/post_req callback. As far as understood, the framework calls >> this when there is more than one request pending ... which seems to be >> when chained mode actually make sense >> >> Jerome, what you are talking about is the system framework problem when you call pre/post_req, >> >> which is not related to the patch I submitted. >> >> >> >> I strongly disagree. >> As far as I understand from your description, the problem was with the >> life cycle of the dma mapping. This is tighly related with pre/post req. >> Just the variable names you have picked clearly show that. >> >> >> >> As you said, pre/post_req is called only when there is data to implement the chained mode, >> >> but it is also possible to cause memory consistency problems, >> resulting in incorrect data. >> >> >> >> The life cycle of the mapping is also taken care of with patch, >> since dma mapping is no longer handled in .request(). IOW the mapping, >> if any, will be released after the request is done using .post_req() >> >> If you think otherwise, please elaborate. >> >> >> I see what you mean. You want to pull the pre/post_req operation out of the request interface and invoke it at the top. >> >> I didn't notice the following modification of patch in your last email and reply in time. I'm really sorry. >> >> The following patch removes all pre/post_req operations, >> >> >> >> No it does not. Callbacks are still provided to the MMC framework. >> >> >> >> but you do not send out the operation patch added to the upper layer >> together. >> >> >> >> There is no modification needed in the upper layer >> >> >> >> >> Then the patch is incomplete, which will affect the dma data transfer function in start_cmd function and affect the multi-block write operation. >> >> >> >> >> No it is not incomplete. pre and post request are correctly called. You >> can check that with ftrace if you want. >> >> Maybe you could try it ? >> >> >> I'm sorry, I didn't catch your meaning.How do I use pre/post_req >> properly? > > Those callbacks are provided to the framework as well. If you have > trouble following, please apply the patch and look at the driver > > Have a look at "meson_mmc_ops" > >> >> Do you mean I need to add a call to pre/post_req on top of the changes >> you provided below? > > No, you should not > >> >> If not, your changes have deleted all pre/post_req calls. > > No, it does not. It relies on the framework to activate the chained mode > or not. Again, if you have trouble following you can try to enable > ftrace and trace the mmc functions. > >> >> >> >> >> Please send your complete patch, including core layer modification, >> thank you. >> >> >> >> >> >> Therefore, this patch is added to make memory consistent and obtain real effective information. >> >> >> >> ----8<----- >> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c >> index e712315c7e8d..399604b4124d 100644 >> --- a/drivers/mmc/host/meson-gx-mmc.c >> +++ b/drivers/mmc/host/meson-gx-mmc.c >> @@ -126,8 +126,7 @@ >> #define SD_EMMC_CFG_CMD_GAP 16 /* in clock cycles */ >> #define SD_EMMC_DESC_BUF_LEN PAGE_SIZE >> >> -#define SD_EMMC_PRE_REQ_DONE BIT(0) >> -#define SD_EMMC_DESC_CHAIN_MODE BIT(1) >> +#define SD_EMMC_DESC_CHAIN_MODE BIT(0) >> >> #define MUX_CLK_NUM_PARENTS 2 >> >> @@ -228,7 +227,6 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc, >> struct mmc_data *data = mrq->data; >> struct scatterlist *sg; >> int i; >> - bool use_desc_chain_mode = true; >> >> /* >> * When Controller DMA cannot directly access DDR memory, disable >> @@ -251,12 +249,11 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc, >> /* check for 8 byte alignment */ >> if (sg->offset & 7) { >> WARN_ONCE(1, "unaligned scatterlist buffer\n"); >> - use_desc_chain_mode = false; >> - break; >> + return; >> } >> >> - if (use_desc_chain_mode) >> - data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE; >> + /* The planets are aligned, let's chain them up */ >> + data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE; >> } >> >> static inline bool meson_mmc_desc_chain_mode(const struct mmc_data *data) >> @@ -278,7 +275,6 @@ static void meson_mmc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq) >> return; >> >> meson_mmc_get_transfer_mode(mmc, mrq); >> - data->host_cookie |= SD_EMMC_PRE_REQ_DONE; >> >> if (!meson_mmc_desc_chain_mode(data)) >> return; >> @@ -803,25 +799,11 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd) >> static void meson_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq) >> { >> struct meson_host *host = mmc_priv(mmc); >> - bool needs_pre_post_req = mrq->data && >> - !(mrq->data->host_cookie & SD_EMMC_PRE_REQ_DONE); >> - >> - if (needs_pre_post_req) { >> - meson_mmc_get_transfer_mode(mmc, mrq); >> - if (!meson_mmc_desc_chain_mode(mrq->data)) >> - needs_pre_post_req = false; >> - } >> - >> - if (needs_pre_post_req) >> - meson_mmc_pre_req(mmc, mrq); >> >> /* Stop execution */ >> writel(0, host->regs + SD_EMMC_START); >> >> meson_mmc_start_cmd(mmc, mrq->sbc ?: mrq->cmd); >> - >> - if (needs_pre_post_req) >> - meson_mmc_post_req(mmc, mrq, 0); >> } >> >> static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd) >> ---->8----- >> >> No performance hit AFAICT. >> From your description, it should address your problem too. >> >> >> >> >> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c >> index e712315..7667e8a 100644 >> --- a/drivers/mmc/host/meson-gx-mmc.c >> +++ b/drivers/mmc/host/meson-gx-mmc.c >> @@ -173,6 +173,7 @@ struct meson_host { >> int irq; >> >> bool vqmmc_enabled; >> + bool needs_pre_post_req; >> }; >> >> #define CMD_CFG_LENGTH_MASK GENMASK(8, 0) >> @@ -654,6 +655,8 @@ static void meson_mmc_request_done(struct mmc_host *mmc, >> struct meson_host *host = mmc_priv(mmc); >> >> host->cmd = NULL; >> + if (host->needs_pre_post_req) >> + meson_mmc_post_req(mmc, mrq, 0); >> mmc_request_done(host->mmc, mrq); >> } >> >> @@ -803,25 +806,23 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd) >> static void meson_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq) >> { >> struct meson_host *host = mmc_priv(mmc); >> - bool needs_pre_post_req = mrq->data && >> + >> + host->needs_pre_post_req = mrq->data && >> !(mrq->data->host_cookie & SD_EMMC_PRE_REQ_DONE); >> >> - if (needs_pre_post_req) { >> + if (host->needs_pre_post_req) { >> meson_mmc_get_transfer_mode(mmc, mrq); >> if (!meson_mmc_desc_chain_mode(mrq->data)) >> - needs_pre_post_req = false; >> + host->needs_pre_post_req = false; >> } >> >> - if (needs_pre_post_req) >> + if (host->needs_pre_post_req) >> meson_mmc_pre_req(mmc, mrq); >> >> /* Stop execution */ >> writel(0, host->regs + SD_EMMC_START); >> >> meson_mmc_start_cmd(mmc, mrq->sbc ?: mrq->cmd); >> - >> - if (needs_pre_post_req) >> - meson_mmc_post_req(mmc, mrq, 0); >> } >> >> static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd) >
On Thu 07 Nov 2019 at 04:07, Nan Li <Nan.Li@amlogic.com> wrote: > On 2019/11/5 21:30, Jerome Brunet wrote: >> >> On Tue 05 Nov 2019 at 10:05, Nan Li <Nan.Li@amlogic.com> wrote: >> >> Nan Li, please fix your mailer to use plain text properly, your reply >> are near impossible to read >> > Sorry, maybe there is something wrong with my email address. Please help > me check whether my reply is in accordance with the format. > Please use plain text instead on html when replying on these mailing list.
On Wed 06 Nov 2019 at 04:28, Nan Li <Nan.Li@amlogic.com> wrote: > > I see what you mean, pre/post_req already provides callbacks in meson_mmc_ops for the framework to decide whether to invoke the chain mode or not. > > However, I searched the frame of MMC and found the use of mmc_pre_req() for this callback in the block layer mmc_blk_mq_issue_rw_rq(). > > Block layer mmc_blk_mq_issue_rw_rq() may be useful for emmc and SD card devices. > > But it may not be useful for reading and writing operations of sdio wifi, and the sdio device communication may not use the chain mode. > > > Our chain-mode is a way to transfer data using dma. > > This approach is very efficient for reading and writing large amounts of data. > > If you don't do it that way, you'll do it the other way, the bounce buf way, which is limited by the size of the buf, so when you do big data reads and writes, it affects the transfer rate. > > Therefore, our chain mode means that emmc, SD card or sdio device will use dma to transfer data when reading and writing operations, so our previous driver and the patch behind me all ensure this. > I see. The problem is that you are providing the same function to pre/post_req callbacks and the request() IOW, things mapped in the pre_req() callback might be unmapped by request_done() instead post_req() which, I think, is not great. It's been like that so far, your patch is not making much worse, so I guess you can go ahead with it but we need to look a this before it blows again In the future, we should probably use the cookie to distinguish the 2 cases ... or drop pre/post_req in the ops. Regards Jerome
On 2019/11/13 1:12, Jerome Brunet wrote: > > On Wed 06 Nov 2019 at 04:28, Nan Li <Nan.Li@amlogic.com> wrote: > >> >> I see what you mean, pre/post_req already provides callbacks in meson_mmc_ops for the framework to decide whether to invoke the chain mode or not. >> >> However, I searched the frame of MMC and found the use of mmc_pre_req() for this callback in the block layer mmc_blk_mq_issue_rw_rq(). >> >> Block layer mmc_blk_mq_issue_rw_rq() may be useful for emmc and SD card devices. >> >> But it may not be useful for reading and writing operations of sdio wifi, and the sdio device communication may not use the chain mode. >> >> >> Our chain-mode is a way to transfer data using dma. >> >> This approach is very efficient for reading and writing large amounts of data. >> >> If you don't do it that way, you'll do it the other way, the bounce buf way, which is limited by the size of the buf, so when you do big data reads and writes, it affects the transfer rate. >> >> Therefore, our chain mode means that emmc, SD card or sdio device will use dma to transfer data when reading and writing operations, so our previous driver and the patch behind me all ensure this. >> > > I see. > The problem is that you are providing the same function to pre/post_req > callbacks and the request() > > IOW, things mapped in the pre_req() callback might be unmapped by > request_done() instead post_req() which, I think, is not great. > > It's been like that so far, your patch is not making much worse, so I > guess you can go ahead with it but we need to look a this before it > blows again > > In the future, we should probably use the cookie to distinguish the 2 > cases ... or drop pre/post_req in the ops. > Yes, you are right. The previous version of kernel did not have the callback of pre/post_req, so our drivers all took dma operation, which would improve the interaction efficiency. I still have a doubt. In kernel, block layer calls the callback of pre/post_req to complete the mode selection, so the call of sdio is omitted. Since the read and write operation of sdio does not go through the interface of block layer, is this the function loss of the framework? You are concerned that the umap_sg() operation will be repeated in request_done(), which is risky.So we can restrict that. For example: 1. Add conditions in the driver. When only operating on sdio devices, pre/post_req interface is called in the driver. 2. Determine whether host_cookie has been assigned SD_EMMC_DESC_CHAIN_MODE before calling pre/post_req. If host_cookie has been assigned, it means that the block layer has called pre/post_req callback. This should prevent duplicate calls.What do you think? ----8<----- diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c index f7ac88c..3eaae4d 100644 --- a/drivers/mmc/host/meson-gx-mmc.c +++ b/drivers/mmc/host/meson-gx-mmc.c @@ -282,6 +282,11 @@ static void meson_mmc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq) if (!data) return; + if (meson_mmc_desc_chain_mode(data)) { + host->needs_pre_post_req = false; + return; + } + meson_mmc_get_transfer_mode(mmc, mrq); data->host_cookie |= SD_EMMC_PRE_REQ_DONE; ---->8----- > Regards > Jerome >
diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c index e712315..7667e8a 100644 --- a/drivers/mmc/host/meson-gx-mmc.c +++ b/drivers/mmc/host/meson-gx-mmc.c @@ -173,6 +173,7 @@ struct meson_host { int irq; bool vqmmc_enabled; + bool needs_pre_post_req; }; #define CMD_CFG_LENGTH_MASK GENMASK(8, 0) @@ -654,6 +655,8 @@ static void meson_mmc_request_done(struct mmc_host *mmc, struct meson_host *host = mmc_priv(mmc); host->cmd = NULL; + if (host->needs_pre_post_req) + meson_mmc_post_req(mmc, mrq, 0); mmc_request_done(host->mmc, mrq); } @@ -803,25 +806,23 @@ static void meson_mmc_start_cmd(struct mmc_host *mmc, struct mmc_command *cmd) static void meson_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq) { struct meson_host *host = mmc_priv(mmc); - bool needs_pre_post_req = mrq->data && + + host->needs_pre_post_req = mrq->data && !(mrq->data->host_cookie & SD_EMMC_PRE_REQ_DONE); - if (needs_pre_post_req) { + if (host->needs_pre_post_req) { meson_mmc_get_transfer_mode(mmc, mrq); if (!meson_mmc_desc_chain_mode(mrq->data)) - needs_pre_post_req = false; + host->needs_pre_post_req = false; } - if (needs_pre_post_req) + if (host->needs_pre_post_req) meson_mmc_pre_req(mmc, mrq); /* Stop execution */ writel(0, host->regs + SD_EMMC_START); meson_mmc_start_cmd(mmc, mrq->sbc ?: mrq->cmd); - - if (needs_pre_post_req) - meson_mmc_post_req(mmc, mrq, 0); } static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd)