| Message ID | 1422271175-19445-2-git-send-email-addy.ke@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote: > We need to take the card pointer in execute_tuning() for mmc_send_status(), mmc_send_status() is an mmc core function, not intended for host's to call. > but mmc->card is NULL in tuning state. So we need change the first parameter > of execute_tuning() to card pointer(struct mmc_card * card). So, why do we need this? Kind regards Uffe > > Signed-off-by: Addy Ke <addy.ke@rock-chips.com> > --- > drivers/mmc/core/core.c | 2 +- > drivers/mmc/host/dw_mmc.c | 3 ++- > drivers/mmc/host/rtsx_pci_sdmmc.c | 3 ++- > drivers/mmc/host/rtsx_usb_sdmmc.c | 3 ++- > include/linux/mmc/host.h | 2 +- > 5 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 1be7055..271f024 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -1101,7 +1101,7 @@ int mmc_execute_tuning(struct mmc_card *card) > opcode = MMC_SEND_TUNING_BLOCK; > > mmc_host_clk_hold(host); > - err = host->ops->execute_tuning(host, opcode); > + err = host->ops->execute_tuning(card, opcode); > mmc_host_clk_release(host); > > if (err) > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 4bd22af..e54e656 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -1485,8 +1485,9 @@ free_blk_test: > return ret; > } > > -static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode) > +static int dw_mci_execute_tuning(struct mmc_card *card, u32 opcode) > { > + struct mmc_host *mmc = card->host; > struct dw_mci_slot *slot = mmc_priv(mmc); > struct dw_mci *host = slot->host; > const struct dw_mci_drv_data *drv_data = host->drv_data; > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c > index 1d3d6c4..230bd2f 100644 > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c > @@ -1270,8 +1270,9 @@ out: > return err; > } > > -static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode) > +static int sdmmc_execute_tuning(struct mmc_card *card, u32 opcode) > { > + struct mmc_host *mmc = card->host; > struct realtek_pci_sdmmc *host = mmc_priv(mmc); > struct rtsx_pcr *pcr = host->pcr; > int err = 0; > diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c > index 88af827..c494c06 100644 > --- a/drivers/mmc/host/rtsx_usb_sdmmc.c > +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c > @@ -1265,8 +1265,9 @@ out: > return 0; > } > > -static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode) > +static int sdmmc_execute_tuning(struct mmc_card *card, u32 opcode) > { > + struct mmc_host *mmc = card->host; > struct rtsx_usb_sdmmc *host = mmc_priv(mmc); > struct rtsx_ucr *ucr = host->ucr; > int err = 0; > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index 0c8cbe5..ec4128e 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -133,7 +133,7 @@ struct mmc_host_ops { > int (*card_busy)(struct mmc_host *host); > > /* The tuning command opcode value is different for SD and eMMC cards */ > - int (*execute_tuning)(struct mmc_host *host, u32 opcode); > + int (*execute_tuning)(struct mmc_card *card, u32 opcode); > > /* Prepare HS400 target operating frequency depending host driver */ > int (*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios); > -- > 1.8.3.2 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
Ulf, On Mon, Jan 26, 2015 at 7:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote: >> We need to take the card pointer in execute_tuning() for mmc_send_status(), > > mmc_send_status() is an mmc core function, not intended for host's to call. > >> but mmc->card is NULL in tuning state. So we need change the first parameter >> of execute_tuning() to card pointer(struct mmc_card * card). > > So, why do we need this? I asked Addy to post upstream against mmc_send_tuning(), but I guess he didn't (he posted against Alex's NAKed patch instead). ...when I talked to him about it, Addy was asserting that when tuning fails it is important (at least on dw_mmc on rk3288) that we wait for the card to stop being busy and that the way to detect was using mmc_send_status(). That would mean that against upstream you'd need to change mmc_send_tuning() to take in the card as well (or move the "host->card = card" assignment to before UHS init, which seems less desirable?) What do you think about that? Is there a better solution? -Doug
On Mon, Jan 26, 2015 at 09:45:07AM -0800, Doug Anderson wrote: > Ulf, > > On Mon, Jan 26, 2015 at 7:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote: > >> We need to take the card pointer in execute_tuning() for mmc_send_status(), > > > > mmc_send_status() is an mmc core function, not intended for host's to call. > > > >> but mmc->card is NULL in tuning state. So we need change the first parameter > >> of execute_tuning() to card pointer(struct mmc_card * card). > > > > So, why do we need this? > > I asked Addy to post upstream against mmc_send_tuning(), but I guess > he didn't (he posted against Alex's NAKed patch instead). > > ...when I talked to him about it, Addy was asserting that when tuning > fails it is important (at least on dw_mmc on rk3288) that we wait for > the card to stop being busy and that the way to detect was using > mmc_send_status(). > > That would mean that against upstream you'd need to change > mmc_send_tuning() to take in the card as well (or move the "host->card > = card" assignment to before UHS init, which seems less desirable?) > > What do you think about that? Is there a better solution? That sounds like a generic thing though - in which case, what do the specs have to say on this, and does the code implement what it says?
On 26 January 2015 at 18:45, Doug Anderson <dianders@chromium.org> wrote: > Ulf, > > On Mon, Jan 26, 2015 at 7:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 26 January 2015 at 12:19, Addy Ke <addy.ke@rock-chips.com> wrote: >>> We need to take the card pointer in execute_tuning() for mmc_send_status(), >> >> mmc_send_status() is an mmc core function, not intended for host's to call. >> >>> but mmc->card is NULL in tuning state. So we need change the first parameter >>> of execute_tuning() to card pointer(struct mmc_card * card). >> >> So, why do we need this? > > I asked Addy to post upstream against mmc_send_tuning(), but I guess > he didn't (he posted against Alex's NAKed patch instead). > > ...when I talked to him about it, Addy was asserting that when tuning > fails it is important (at least on dw_mmc on rk3288) that we wait for > the card to stop being busy and that the way to detect was using > mmc_send_status(). So, could that be due to the internal logic of the error handling in dw_mmc driver? Or you think this is a generic issue? According to the specifications (eMMC and SD) both states that the tuning command has an R1 response. So, there shouldn't be any busy signalling involved - at least according to spec. > > That would mean that against upstream you'd need to change > mmc_send_tuning() to take in the card as well (or move the "host->card > = card" assignment to before UHS init, which seems less desirable?) > > What do you think about that? Is there a better solution? Why do we need to change mmc_send_tuning()? I thought the issue was that mmc_send_status(), which currently takes "card" as a parameter. Kind regards Uffe
Ulf, On Tue, Jan 27, 2015 at 7:18 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> I asked Addy to post upstream against mmc_send_tuning(), but I guess >> he didn't (he posted against Alex's NAKed patch instead). >> >> ...when I talked to him about it, Addy was asserting that when tuning >> fails it is important (at least on dw_mmc on rk3288) that we wait for >> the card to stop being busy and that the way to detect was using >> mmc_send_status(). > > So, could that be due to the internal logic of the error handling in > dw_mmc driver? Or you think this is a generic issue? > > According to the specifications (eMMC and SD) both states that the > tuning command has an R1 response. So, there shouldn't be any busy > signalling involved - at least according to spec. I did a bit of digging into this issue myself. What I found was that a "response CRC" and "end of transfer". This was why I posted up <https://patchwork.kernel.org/patch/5623071/>. From that patch: > Specifically it looks like in certain error conditions (I saw this > with Response CRC errors) that data keeps showing up in the FIFO even > after the error is reported and the CD (command done) bit is set. If > we don't wait for this data to finish transferring then it confuses > the next transaction. In the specific failure case I ran into I found > that I could monitor the data_state_mc_busy bit and wait for it to > clear, but in other failure cases this bit was stuck at busy when we > saw an error. Hence a generic big delay seems like the only option. ...Addy instead fixed the problem using mmc_send_status() to try to detect when the transfer was all done and it apparently worked, but it seemed odd to me. My MMC "expertise" pretty much ends with looking for simple logic errors in the MMC driver, so my hope was that one of you guys would know this better... >> That would mean that against upstream you'd need to change >> mmc_send_tuning() to take in the card as well (or move the "host->card >> = card" assignment to before UHS init, which seems less desirable?) >> >> What do you think about that? Is there a better solution? > > Why do we need to change mmc_send_tuning()? I thought the issue was > that mmc_send_status(), which currently takes "card" as a parameter. Well, if mmc_send_tuning() needed to call mmc_send_status() then mmc_send_tuning() would need the card parameter, right? Doug
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 1be7055..271f024 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -1101,7 +1101,7 @@ int mmc_execute_tuning(struct mmc_card *card) opcode = MMC_SEND_TUNING_BLOCK; mmc_host_clk_hold(host); - err = host->ops->execute_tuning(host, opcode); + err = host->ops->execute_tuning(card, opcode); mmc_host_clk_release(host); if (err) diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 4bd22af..e54e656 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -1485,8 +1485,9 @@ free_blk_test: return ret; } -static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode) +static int dw_mci_execute_tuning(struct mmc_card *card, u32 opcode) { + struct mmc_host *mmc = card->host; struct dw_mci_slot *slot = mmc_priv(mmc); struct dw_mci *host = slot->host; const struct dw_mci_drv_data *drv_data = host->drv_data; diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c index 1d3d6c4..230bd2f 100644 --- a/drivers/mmc/host/rtsx_pci_sdmmc.c +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c @@ -1270,8 +1270,9 @@ out: return err; } -static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode) +static int sdmmc_execute_tuning(struct mmc_card *card, u32 opcode) { + struct mmc_host *mmc = card->host; struct realtek_pci_sdmmc *host = mmc_priv(mmc); struct rtsx_pcr *pcr = host->pcr; int err = 0; diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c index 88af827..c494c06 100644 --- a/drivers/mmc/host/rtsx_usb_sdmmc.c +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c @@ -1265,8 +1265,9 @@ out: return 0; } -static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode) +static int sdmmc_execute_tuning(struct mmc_card *card, u32 opcode) { + struct mmc_host *mmc = card->host; struct rtsx_usb_sdmmc *host = mmc_priv(mmc); struct rtsx_ucr *ucr = host->ucr; int err = 0; diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index 0c8cbe5..ec4128e 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -133,7 +133,7 @@ struct mmc_host_ops { int (*card_busy)(struct mmc_host *host); /* The tuning command opcode value is different for SD and eMMC cards */ - int (*execute_tuning)(struct mmc_host *host, u32 opcode); + int (*execute_tuning)(struct mmc_card *card, u32 opcode); /* Prepare HS400 target operating frequency depending host driver */ int (*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios);
We need to take the card pointer in execute_tuning() for mmc_send_status(), but mmc->card is NULL in tuning state. So we need change the first parameter of execute_tuning() to card pointer(struct mmc_card * card). Signed-off-by: Addy Ke <addy.ke@rock-chips.com> --- drivers/mmc/core/core.c | 2 +- drivers/mmc/host/dw_mmc.c | 3 ++- drivers/mmc/host/rtsx_pci_sdmmc.c | 3 ++- drivers/mmc/host/rtsx_usb_sdmmc.c | 3 ++- include/linux/mmc/host.h | 2 +- 5 files changed, 8 insertions(+), 5 deletions(-)