| Message ID | 1453749316-1848-10-git-send-email-wsa@the-dreams.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On 25 January 2016 at 20:15, Wolfram Sang <wsa@the-dreams.de> wrote: > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Registers are 64bit apart, so we refactor bus_shift handling a little > and set it based on the DT compatible. Also, EXT_ACC is different. > > Signed-off-by: Ai Kyuse <ai.kyuse.uw@renesas.com> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > @MMC maintainers: I set the MMC_CAP_WAIT_WHILE_BUSY flage here and it is needed > to probe the eMMC (implementing RSP_R1 without CRC would also work). However, I > can't find an explicit statement in the documentation saying that it really > waits when the bus is busy. Is my "it works, let's use it" approach enough, or > is there more I can do to verify that setting MMC_CAP_WAIT_WHILE_BUSY is valid > for this hardware? In general from the mmc core perspective, when a host announces MMC_CAP_WAIT_WHILE_BUSY it won't send a CMD13 to poll for busy. I think you should try without MMC_CAP_WAIT_WHILE_BUSY, and then check that a following CMD13 command always states that the card isn't busy. I think the best path to try this is when sending a big write data request, as in that case you can be quite certain that the card gets busy between the requests. So somewhere in the mmc block layer add some debug prints, that should do it. Kind regards Uffe > > Documentation/devicetree/bindings/mmc/tmio_mmc.txt | 1 + > drivers/mmc/host/Kconfig | 2 +- > drivers/mmc/host/sh_mobile_sdhi.c | 47 ++++++++++++++++------ > 3 files changed, 36 insertions(+), 14 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt > index 400b640fabc768..7fb746dd1a68ca 100644 > --- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt > +++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt > @@ -22,6 +22,7 @@ Required properties: > "renesas,sdhi-r8a7792" - SDHI IP on R8A7792 SoC > "renesas,sdhi-r8a7793" - SDHI IP on R8A7793 SoC > "renesas,sdhi-r8a7794" - SDHI IP on R8A7794 SoC > + "renesas,sdhi-r8a7795" - SDHI IP on R8A7795 SoC > > Optional properties: > - toshiba,mmc-wrprotect-disable: write-protect detection is unavailable > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig > index 1526b8a10b094e..dd1499bd218b16 100644 > --- a/drivers/mmc/host/Kconfig > +++ b/drivers/mmc/host/Kconfig > @@ -560,7 +560,7 @@ config MMC_TMIO > > config MMC_SDHI > tristate "SH-Mobile SDHI SD/SDIO controller support" > - depends on SUPERH || ARM > + depends on SUPERH || ARM || ARM64 > depends on SUPERH || ARCH_SHMOBILE || COMPILE_TEST > select MMC_TMIO_CORE > help > diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c > index 557e2b9dadeec7..f7eff5f53e0013 100644 > --- a/drivers/mmc/host/sh_mobile_sdhi.c > +++ b/drivers/mmc/host/sh_mobile_sdhi.c > @@ -1,6 +1,8 @@ > /* > * SuperH Mobile SDHI > * > + * Copyright (C) 2016 Sang Engineering, Wolfram Sang > + * Copyright (C) 2015-16 Renesas Electronics Corporation > * Copyright (C) 2009 Magnus Damm > * > * This program is free software; you can redistribute it and/or modify > @@ -43,6 +45,7 @@ struct sh_mobile_sdhi_of_data { > unsigned long capabilities2; > enum dma_slave_buswidth dma_buswidth; > dma_addr_t dma_rx_offset; > + unsigned bus_shift; > }; > > static const struct sh_mobile_sdhi_of_data sh_mobile_sdhi_of_cfg[] = { > @@ -65,6 +68,13 @@ static const struct sh_mobile_sdhi_of_data of_rcar_gen2_compatible = { > .dma_rx_offset = 0x2000, > }; > > +static const struct sh_mobile_sdhi_of_data of_rcar_gen3_compatible = { > + .tmio_flags = TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_WRPROTECT_DISABLE | > + TMIO_MMC_CLK_ACTUAL | TMIO_MMC_FAST_CLK_CHG, > + .capabilities = MMC_CAP_SD_HIGHSPEED | MMC_CAP_WAIT_WHILE_BUSY, > + .bus_shift = 2, > +}; > + > static const struct of_device_id sh_mobile_sdhi_of_match[] = { > { .compatible = "renesas,sdhi-shmobile" }, > { .compatible = "renesas,sdhi-sh7372" }, > @@ -78,6 +88,7 @@ static const struct of_device_id sh_mobile_sdhi_of_match[] = { > { .compatible = "renesas,sdhi-r8a7792", .data = &of_rcar_gen2_compatible, }, > { .compatible = "renesas,sdhi-r8a7793", .data = &of_rcar_gen2_compatible, }, > { .compatible = "renesas,sdhi-r8a7794", .data = &of_rcar_gen2_compatible, }, > + { .compatible = "renesas,sdhi-r8a7795", .data = &of_rcar_gen3_compatible, }, > {}, > }; > MODULE_DEVICE_TABLE(of, sh_mobile_sdhi_of_match); > @@ -103,6 +114,15 @@ static void sh_mobile_sdhi_sdbuf_width(struct tmio_mmc_host *host, int width) > case 0xCB0D: > val = (width == 32) ? 0x0000 : 0x0001; > break; > + case 0xCC10: /* Gen3, SD only */ > + case 0xCD10: /* Gen3, SD + MMC */ > + if (width == 64) > + val = 0x0000; > + else if (width == 32) > + val = 0x0101; > + else > + val = 0x0001; > + break; > default: > /* nothing to do */ > return; > @@ -233,16 +253,26 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev) > goto eprobe; > } > > + if (of_id && of_id->data) { > + const struct sh_mobile_sdhi_of_data *of_data = of_id->data; > + > + mmc_data->flags |= of_data->tmio_flags; > + mmc_data->capabilities |= of_data->capabilities; > + mmc_data->capabilities2 |= of_data->capabilities2; > + mmc_data->dma_rx_offset = of_data->dma_rx_offset; > + dma_priv->dma_buswidth = of_data->dma_buswidth; > + host->bus_shift = of_data->bus_shift; > + } > + > host->dma = dma_priv; > host->write16_hook = sh_mobile_sdhi_write16_hook; > host->clk_enable = sh_mobile_sdhi_clk_enable; > host->clk_disable = sh_mobile_sdhi_clk_disable; > host->multi_io_quirk = sh_mobile_sdhi_multi_io_quirk; > - /* SD control register space size is 0x100, 0x200 for bus_shift=1 */ > - if (resource_size(res) > 0x100) > + > + /* Orginally registers were 16 bit apart, could be 32 or 64 nowadays */ > + if (!host->bus_shift && resource_size(res) > 0x100) /* old way to determine the shift */ > host->bus_shift = 1; > - else > - host->bus_shift = 0; > > if (mmd) > *mmc_data = *mmd; > @@ -274,15 +304,6 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev) > */ > mmc_data->flags |= TMIO_MMC_SDIO_STATUS_QUIRK; > > - if (of_id && of_id->data) { > - const struct sh_mobile_sdhi_of_data *of_data = of_id->data; > - mmc_data->flags |= of_data->tmio_flags; > - mmc_data->capabilities |= of_data->capabilities; > - mmc_data->capabilities2 |= of_data->capabilities2; > - mmc_data->dma_rx_offset = of_data->dma_rx_offset; > - dma_priv->dma_buswidth = of_data->dma_buswidth; > - } > - > ret = tmio_mmc_host_probe(host, mmc_data); > if (ret < 0) > goto efree; > -- > 2.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
^ > I think you should try without MMC_CAP_WAIT_WHILE_BUSY, and then check > that a following CMD13 command always states that the card isn't busy. > I think the best path to try this is when sending a big write data > request, as in that case you can be quite certain that the card gets > busy between the requests. > > So somewhere in the mmc block layer add some debug prints, that should do it. I'd think the mmc_test driver already helped me with this. I ran tests like 31 (Consecutive write performance by transfer size) or 36 (Large sequential write from scattered pages) which both succeeded without any warnings printed. And the code explicitly sends a CMD13 after transfer, checks for busy and prints a warning when MMC_CAP_WAIT_WHILE_BUSY is set and a busy state is detected. Nice test thing this driver is :) So, it looks to me that patch 9 is fine to go in? -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 February 2016 at 17:36, Wolfram Sang <wsa@the-dreams.de> wrote: > ^ >> I think you should try without MMC_CAP_WAIT_WHILE_BUSY, and then check >> that a following CMD13 command always states that the card isn't busy. >> I think the best path to try this is when sending a big write data >> request, as in that case you can be quite certain that the card gets >> busy between the requests. >> >> So somewhere in the mmc block layer add some debug prints, that should do it. > > I'd think the mmc_test driver already helped me with this. I ran tests > like 31 (Consecutive write performance by transfer size) or 36 (Large > sequential write from scattered pages) which both succeeded without any > warnings printed. And the code explicitly sends a CMD13 after transfer, > checks for busy and prints a warning when MMC_CAP_WAIT_WHILE_BUSY is > set and a busy state is detected. Nice test thing this driver is :) That should do it! > > So, it looks to me that patch 9 is fine to go in? > Not yet. :-) I suspect you are using a delayed work in this driver to deal with request timeouts. The value for the delay that is used, needs to be informed towards the mmc core via the "->max_busy_timeout". One more thing, as the documentation of your host controllers lacks some information about the busy mode, perhaps it's worth to add some comments about this in the code and as well in the change-log!? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I suspect you are using a delayed work in this driver to deal with > request timeouts. > > The value for the delay that is used, needs to be informed towards the > mmc core via the "->max_busy_timeout". Shouldn't that be a seperate patch? In patch 9, I add support for another SoC. But your requested change will affect all SoCs. > One more thing, as the documentation of your host controllers lacks > some information about the busy mode, perhaps it's worth to add some > comments about this in the code and as well in the change-log!? Do you mean documenting my findings with the mmc_test driver? Or something else? Thanks, Wolfram
On 11 February 2016 at 01:07, Wolfram Sang <wsa@the-dreams.de> wrote: >> I suspect you are using a delayed work in this driver to deal with >> request timeouts. >> >> The value for the delay that is used, needs to be informed towards the >> mmc core via the "->max_busy_timeout". > > Shouldn't that be a seperate patch? In patch 9, I add support for > another SoC. But your requested change will affect all SoCs. This is related to how the core treat hosts that announces MMC_CAP_WAIT_WHILE_BUSY. Therefore max_busy_timeout needs to be set within the same patch. > >> One more thing, as the documentation of your host controllers lacks >> some information about the busy mode, perhaps it's worth to add some >> comments about this in the code and as well in the change-log!? > > Do you mean documenting my findings with the mmc_test driver? Or > something else? It doesn't have to be very much data, just that the documentation isn't clear and therefore some tests has been done to verify that it works as expected. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 11, 2016 at 10:00:50AM +0100, Ulf Hansson wrote: > On 11 February 2016 at 01:07, Wolfram Sang <wsa@the-dreams.de> wrote: > >> I suspect you are using a delayed work in this driver to deal with > >> request timeouts. > >> > >> The value for the delay that is used, needs to be informed towards the > >> mmc core via the "->max_busy_timeout". > > > > Shouldn't that be a seperate patch? In patch 9, I add support for > > another SoC. But your requested change will affect all SoCs. > > This is related to how the core treat hosts that announces > MMC_CAP_WAIT_WHILE_BUSY. > > Therefore max_busy_timeout needs to be set within the same patch. Right. I assumed previous SoC (Gen2) were using MMC_CAP_WAIT_WHILE_BUSY already, so this step would then have to be taken seperately to catch all SoC. But I was wrong. I meanwhile found the timeout bits which are present in Gen3 and also in Gen2 and are currently unused by the driver. While this helps explaining that the HW has internal busy detection, its full potential is not implemented yet. So, I am going to resend patch 9 without MMC_CAP_WAIT_WHILE_BUSY set and will implement timeout handling incrementally as a seperate series. Sounds good? -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 February 2016 at 14:32, Wolfram Sang <wsa@the-dreams.de> wrote: > On Thu, Feb 11, 2016 at 10:00:50AM +0100, Ulf Hansson wrote: >> On 11 February 2016 at 01:07, Wolfram Sang <wsa@the-dreams.de> wrote: >> >> I suspect you are using a delayed work in this driver to deal with >> >> request timeouts. >> >> >> >> The value for the delay that is used, needs to be informed towards the >> >> mmc core via the "->max_busy_timeout". >> > >> > Shouldn't that be a seperate patch? In patch 9, I add support for >> > another SoC. But your requested change will affect all SoCs. >> >> This is related to how the core treat hosts that announces >> MMC_CAP_WAIT_WHILE_BUSY. >> >> Therefore max_busy_timeout needs to be set within the same patch. > > Right. I assumed previous SoC (Gen2) were using MMC_CAP_WAIT_WHILE_BUSY > already, so this step would then have to be taken seperately to catch > all SoC. But I was wrong. > > I meanwhile found the timeout bits which are present in Gen3 and also in > Gen2 and are currently unused by the driver. While this helps explaining > that the HW has internal busy detection, its full potential is not > implemented yet. > > So, I am going to resend patch 9 without MMC_CAP_WAIT_WHILE_BUSY set and > will implement timeout handling incrementally as a seperate series. > > Sounds good? Yes, please! Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt index 400b640fabc768..7fb746dd1a68ca 100644 --- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt +++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt @@ -22,6 +22,7 @@ Required properties: "renesas,sdhi-r8a7792" - SDHI IP on R8A7792 SoC "renesas,sdhi-r8a7793" - SDHI IP on R8A7793 SoC "renesas,sdhi-r8a7794" - SDHI IP on R8A7794 SoC + "renesas,sdhi-r8a7795" - SDHI IP on R8A7795 SoC Optional properties: - toshiba,mmc-wrprotect-disable: write-protect detection is unavailable diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig index 1526b8a10b094e..dd1499bd218b16 100644 --- a/drivers/mmc/host/Kconfig +++ b/drivers/mmc/host/Kconfig @@ -560,7 +560,7 @@ config MMC_TMIO config MMC_SDHI tristate "SH-Mobile SDHI SD/SDIO controller support" - depends on SUPERH || ARM + depends on SUPERH || ARM || ARM64 depends on SUPERH || ARCH_SHMOBILE || COMPILE_TEST select MMC_TMIO_CORE help diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c index 557e2b9dadeec7..f7eff5f53e0013 100644 --- a/drivers/mmc/host/sh_mobile_sdhi.c +++ b/drivers/mmc/host/sh_mobile_sdhi.c @@ -1,6 +1,8 @@ /* * SuperH Mobile SDHI * + * Copyright (C) 2016 Sang Engineering, Wolfram Sang + * Copyright (C) 2015-16 Renesas Electronics Corporation * Copyright (C) 2009 Magnus Damm * * This program is free software; you can redistribute it and/or modify @@ -43,6 +45,7 @@ struct sh_mobile_sdhi_of_data { unsigned long capabilities2; enum dma_slave_buswidth dma_buswidth; dma_addr_t dma_rx_offset; + unsigned bus_shift; }; static const struct sh_mobile_sdhi_of_data sh_mobile_sdhi_of_cfg[] = { @@ -65,6 +68,13 @@ static const struct sh_mobile_sdhi_of_data of_rcar_gen2_compatible = { .dma_rx_offset = 0x2000, }; +static const struct sh_mobile_sdhi_of_data of_rcar_gen3_compatible = { + .tmio_flags = TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_WRPROTECT_DISABLE | + TMIO_MMC_CLK_ACTUAL | TMIO_MMC_FAST_CLK_CHG, + .capabilities = MMC_CAP_SD_HIGHSPEED | MMC_CAP_WAIT_WHILE_BUSY, + .bus_shift = 2, +}; + static const struct of_device_id sh_mobile_sdhi_of_match[] = { { .compatible = "renesas,sdhi-shmobile" }, { .compatible = "renesas,sdhi-sh7372" }, @@ -78,6 +88,7 @@ static const struct of_device_id sh_mobile_sdhi_of_match[] = { { .compatible = "renesas,sdhi-r8a7792", .data = &of_rcar_gen2_compatible, }, { .compatible = "renesas,sdhi-r8a7793", .data = &of_rcar_gen2_compatible, }, { .compatible = "renesas,sdhi-r8a7794", .data = &of_rcar_gen2_compatible, }, + { .compatible = "renesas,sdhi-r8a7795", .data = &of_rcar_gen3_compatible, }, {}, }; MODULE_DEVICE_TABLE(of, sh_mobile_sdhi_of_match); @@ -103,6 +114,15 @@ static void sh_mobile_sdhi_sdbuf_width(struct tmio_mmc_host *host, int width) case 0xCB0D: val = (width == 32) ? 0x0000 : 0x0001; break; + case 0xCC10: /* Gen3, SD only */ + case 0xCD10: /* Gen3, SD + MMC */ + if (width == 64) + val = 0x0000; + else if (width == 32) + val = 0x0101; + else + val = 0x0001; + break; default: /* nothing to do */ return; @@ -233,16 +253,26 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev) goto eprobe; } + if (of_id && of_id->data) { + const struct sh_mobile_sdhi_of_data *of_data = of_id->data; + + mmc_data->flags |= of_data->tmio_flags; + mmc_data->capabilities |= of_data->capabilities; + mmc_data->capabilities2 |= of_data->capabilities2; + mmc_data->dma_rx_offset = of_data->dma_rx_offset; + dma_priv->dma_buswidth = of_data->dma_buswidth; + host->bus_shift = of_data->bus_shift; + } + host->dma = dma_priv; host->write16_hook = sh_mobile_sdhi_write16_hook; host->clk_enable = sh_mobile_sdhi_clk_enable; host->clk_disable = sh_mobile_sdhi_clk_disable; host->multi_io_quirk = sh_mobile_sdhi_multi_io_quirk; - /* SD control register space size is 0x100, 0x200 for bus_shift=1 */ - if (resource_size(res) > 0x100) + + /* Orginally registers were 16 bit apart, could be 32 or 64 nowadays */ + if (!host->bus_shift && resource_size(res) > 0x100) /* old way to determine the shift */ host->bus_shift = 1; - else - host->bus_shift = 0; if (mmd) *mmc_data = *mmd; @@ -274,15 +304,6 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev) */ mmc_data->flags |= TMIO_MMC_SDIO_STATUS_QUIRK; - if (of_id && of_id->data) { - const struct sh_mobile_sdhi_of_data *of_data = of_id->data; - mmc_data->flags |= of_data->tmio_flags; - mmc_data->capabilities |= of_data->capabilities; - mmc_data->capabilities2 |= of_data->capabilities2; - mmc_data->dma_rx_offset = of_data->dma_rx_offset; - dma_priv->dma_buswidth = of_data->dma_buswidth; - } - ret = tmio_mmc_host_probe(host, mmc_data); if (ret < 0) goto efree;