diff mbox

mmc: renesas_sdhi: fix WP detection

Message ID 20180305204842.25391-1-wsa+renesas@sang-engineering.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wolfram Sang March 5, 2018, 8:48 p.m. UTC 
Commit "mmc: renesas_sdhi: use MMC_CAP2_NO_WRITE_PROTECT instead of
TMIO own flag" activated MMC_CAP2_NO_WRITE_PROTECT for Renesas SDHI
which incorrectly disabled WP altogether instead of only disabling the
internal mechanism. Since the whole WP handling has been reworked, we
can simply disable this capability to re-enable WP GPIOs.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

More testing revealed this. This time, I don't think squashing makes sense but
I am open to discussion here.

 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 1 -
 drivers/mmc/host/renesas_sdhi_sys_dmac.c      | 3 ---
 2 files changed, 4 deletions(-)

Comments

Masahiro Yamada March 6, 2018, 3:48 a.m. UTC | #1 
2018-03-06 5:48 GMT+09:00 Wolfram Sang <wsa+renesas@sang-engineering.com>:
> Commit "mmc: renesas_sdhi: use MMC_CAP2_NO_WRITE_PROTECT instead of
> TMIO own flag" activated MMC_CAP2_NO_WRITE_PROTECT for Renesas SDHI
> which incorrectly disabled WP altogether instead of only disabling the
> internal mechanism.


I am not opposed to this patch,
but I have a question.

Is this is a real problem in the upstream kernel?
(If so, how do renesas boards set-up WP GPIOs?)


When I wrote the addressed patch,
I checked all renesas drivers and device trees,
and confirmed no one set WP-GPIOs.


Then, I described the following in my commit log:

    Only the difference is the TMIO_... makes tmio_mmc_get_ro() return 0
    (i.e. it does not affect mmc_gpio_get_ro() at all), while MMC_CAP2_...
    returns 0 before calling ->get_ro() hook (i.e. it affects both IP own
    logic and GPIO detection).

    The TMIO MMC drivers do not set-up gpio_ro by themselves.  Only the
    possibility, if any, would be DT specifies "wp-gpios" property, and
    gpio_ro is set by mmc_gpiod_request_ro() called from mmc_of_parse().
    However, it does not make sense to specify "wp-gpios" property and
    TMIO_MMC_WRPROTECT_DISABLE at the same time.

    I checked under arch/arm/boot/dts/ and arch/arm64/boot/dts/renesas/,
    and I did not see any Renesas boards with "wp-gpios".  So, this
    conversion should be safe.

> Since the whole WP handling has been reworked, we
> can simply disable this capability to re-enable WP GPIOs.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> More testing revealed this. This time, I don't think squashing makes sense but
> I am open to discussion here.
Wolfram Sang March 6, 2018, 8:53 a.m. UTC | #2 
On Tue, Mar 06, 2018 at 12:48:19PM +0900, Masahiro Yamada wrote:
> 2018-03-06 5:48 GMT+09:00 Wolfram Sang <wsa+renesas@sang-engineering.com>:
> > Commit "mmc: renesas_sdhi: use MMC_CAP2_NO_WRITE_PROTECT instead of
> > TMIO own flag" activated MMC_CAP2_NO_WRITE_PROTECT for Renesas SDHI
> > which incorrectly disabled WP altogether instead of only disabling the
> > internal mechanism.
> 
> 
> I am not opposed to this patch,
> but I have a question.
> 
> Is this is a real problem in the upstream kernel?

Not upstream, but mmc/next. The patch I mentioned in the above commit
message is in mmc/next only. Yes, it is a real problem. As I said below,
"more testing revealed this" on Gen3, at least.

> (If so, how do renesas boards set-up WP GPIOs?)

Quite the standard way, I'd think:

$ grep 'wp-gpios' arch/arm64/boot/dts/renesas/*
arch/arm64/boot/dts/renesas/salvator-common.dtsi:	wp-gpios = <&gpio3 13 GPIO_ACTIVE_HIGH>;
arch/arm64/boot/dts/renesas/salvator-common.dtsi:	wp-gpios = <&gpio4 16 GPIO_ACTIVE_HIGH>;

$ grep 'wp-gpios' arch/arm/boot/dts/r8a*
arch/arm/boot/dts/r8a7778-bockw.dts:	wp-gpios = <&gpio3 18 GPIO_ACTIVE_HIGH>;
arch/arm/boot/dts/r8a7791-koelsch.dts:	wp-gpios = <&gpio6 7 GPIO_ACTIVE_HIGH>;
arch/arm/boot/dts/r8a7791-koelsch.dts:	wp-gpios = <&gpio6 15 GPIO_ACTIVE_HIGH>;
arch/arm/boot/dts/r8a7791-porter.dts:	wp-gpios = <&gpio6 7 GPIO_ACTIVE_HIGH>;
arch/arm/boot/dts/r8a7793-gose.dts:	wp-gpios = <&gpio6 7 GPIO_ACTIVE_HIGH>;
arch/arm/boot/dts/r8a7793-gose.dts:	wp-gpios = <&gpio6 15 GPIO_ACTIVE_HIGH>;
arch/arm/boot/dts/r8a7794-alt.dts:	wp-gpios = <&gpio6 7 GPIO_ACTIVE_HIGH>;
arch/arm/boot/dts/r8a7794-alt.dts:	wp-gpios = <&gpio6 15 GPIO_ACTIVE_HIGH>;

> When I wrote the addressed patch,
> I checked all renesas drivers and device trees,
> and confirmed no one set WP-GPIOs.

Yes, I saw this in the commit message and was confused, too. I seem to also
have overlooked it in my review. I can't really tell what changed or otherwise
went wrong, but fact is that Gen3 can't check RO currently im mmc/next and
after my patch it can. Well, this is why we do apply through testing, I
guess...
Masahiro Yamada March 6, 2018, 9:09 a.m. UTC | #3 
2018-03-06 17:53 GMT+09:00 Wolfram Sang <wsa@the-dreams.de>:
> On Tue, Mar 06, 2018 at 12:48:19PM +0900, Masahiro Yamada wrote:
>> 2018-03-06 5:48 GMT+09:00 Wolfram Sang <wsa+renesas@sang-engineering.com>:
>> > Commit "mmc: renesas_sdhi: use MMC_CAP2_NO_WRITE_PROTECT instead of
>> > TMIO own flag" activated MMC_CAP2_NO_WRITE_PROTECT for Renesas SDHI
>> > which incorrectly disabled WP altogether instead of only disabling the
>> > internal mechanism.
>>
>>
>> I am not opposed to this patch,
>> but I have a question.
>>
>> Is this is a real problem in the upstream kernel?
>
> Not upstream, but mmc/next. The patch I mentioned in the above commit
> message is in mmc/next only. Yes, it is a real problem. As I said below,
> "more testing revealed this" on Gen3, at least.

Seems already upstream.


>> (If so, how do renesas boards set-up WP GPIOs?)
>
> Quite the standard way, I'd think:
>
> $ grep 'wp-gpios' arch/arm64/boot/dts/renesas/*
> arch/arm64/boot/dts/renesas/salvator-common.dtsi:       wp-gpios = <&gpio3 13 GPIO_ACTIVE_HIGH>;
> arch/arm64/boot/dts/renesas/salvator-common.dtsi:       wp-gpios = <&gpio4 16 GPIO_ACTIVE_HIGH>;
>
> $ grep 'wp-gpios' arch/arm/boot/dts/r8a*
> arch/arm/boot/dts/r8a7778-bockw.dts:    wp-gpios = <&gpio3 18 GPIO_ACTIVE_HIGH>;
> arch/arm/boot/dts/r8a7791-koelsch.dts:  wp-gpios = <&gpio6 7 GPIO_ACTIVE_HIGH>;
> arch/arm/boot/dts/r8a7791-koelsch.dts:  wp-gpios = <&gpio6 15 GPIO_ACTIVE_HIGH>;
> arch/arm/boot/dts/r8a7791-porter.dts:   wp-gpios = <&gpio6 7 GPIO_ACTIVE_HIGH>;
> arch/arm/boot/dts/r8a7793-gose.dts:     wp-gpios = <&gpio6 7 GPIO_ACTIVE_HIGH>;
> arch/arm/boot/dts/r8a7793-gose.dts:     wp-gpios = <&gpio6 15 GPIO_ACTIVE_HIGH>;
> arch/arm/boot/dts/r8a7794-alt.dts:      wp-gpios = <&gpio6 7 GPIO_ACTIVE_HIGH>;
> arch/arm/boot/dts/r8a7794-alt.dts:      wp-gpios = <&gpio6 15 GPIO_ACTIVE_HIGH>;

Ugh.


>> When I wrote the addressed patch,
>> I checked all renesas drivers and device trees,
>> and confirmed no one set WP-GPIOs.
>
> Yes, I saw this in the commit message and was confused, too. I seem to also
> have overlooked it in my review. I can't really tell what changed or otherwise
> went wrong, but fact is that Gen3 can't check RO currently im mmc/next and
> after my patch it can. Well, this is why we do apply through testing, I
> guess...
>

You are right.

Sorry, this is my mistake, I do not know why it happened.

I would tedious to coping with git-history.
So, I think this can go in as a separate fix-up patch.


Reviewed-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Ulf Hansson March 15, 2018, 10:23 a.m. UTC | #4 
On 5 March 2018 at 21:48, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
> Commit "mmc: renesas_sdhi: use MMC_CAP2_NO_WRITE_PROTECT instead of
> TMIO own flag" activated MMC_CAP2_NO_WRITE_PROTECT for Renesas SDHI
> which incorrectly disabled WP altogether instead of only disabling the
> internal mechanism. Since the whole WP handling has been reworked, we
> can simply disable this capability to re-enable WP GPIOs.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks, applied for next!

Kind regards
Uffe

> ---
>
> More testing revealed this. This time, I don't think squashing makes sense but
> I am open to discussion here.
>
>  drivers/mmc/host/renesas_sdhi_internal_dmac.c | 1 -
>  drivers/mmc/host/renesas_sdhi_sys_dmac.c      | 3 ---
>  2 files changed, 4 deletions(-)
>
> diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> index a04f31fea72fd1..8e0acd197c43c0 100644
> --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> @@ -75,7 +75,6 @@ static const struct renesas_sdhi_of_data of_rcar_gen3_compatible = {
>                           TMIO_MMC_HAVE_CBSY | TMIO_MMC_MIN_RCAR2,
>         .capabilities   = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
>                           MMC_CAP_CMD23,
> -       .capabilities2  = MMC_CAP2_NO_WRITE_PROTECT,
>         .bus_shift      = 2,
>         .scc_offset     = 0x1000,
>         .taps           = rcar_gen3_scc_taps,
> diff --git a/drivers/mmc/host/renesas_sdhi_sys_dmac.c b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
> index 4bb46c489d71f8..848e50c1638aa6 100644
> --- a/drivers/mmc/host/renesas_sdhi_sys_dmac.c
> +++ b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
> @@ -42,7 +42,6 @@ static const struct renesas_sdhi_of_data of_rz_compatible = {
>  static const struct renesas_sdhi_of_data of_rcar_gen1_compatible = {
>         .tmio_flags     = TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_CLK_ACTUAL,
>         .capabilities   = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
> -       .capabilities2  = MMC_CAP2_NO_WRITE_PROTECT,
>  };
>
>  /* Definitions for sampling clocks */
> @@ -62,7 +61,6 @@ static const struct renesas_sdhi_of_data of_rcar_gen2_compatible = {
>                           TMIO_MMC_HAVE_CBSY | TMIO_MMC_MIN_RCAR2,
>         .capabilities   = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
>                           MMC_CAP_CMD23,
> -       .capabilities2  = MMC_CAP2_NO_WRITE_PROTECT,
>         .dma_buswidth   = DMA_SLAVE_BUSWIDTH_4_BYTES,
>         .dma_rx_offset  = 0x2000,
>         .scc_offset     = 0x0300,
> @@ -83,7 +81,6 @@ static const struct renesas_sdhi_of_data of_rcar_gen3_compatible = {
>                           TMIO_MMC_HAVE_CBSY | TMIO_MMC_MIN_RCAR2,
>         .capabilities   = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
>                           MMC_CAP_CMD23,
> -       .capabilities2  = MMC_CAP2_NO_WRITE_PROTECT,
>         .bus_shift      = 2,
>         .scc_offset     = 0x1000,
>         .taps           = rcar_gen3_scc_taps,
> --
> 2.11.0
>
--
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
Wolfram Sang June 1, 2018, 8:07 a.m. UTC | #5 
On Mon, Mar 05, 2018 at 09:48:42PM +0100, Wolfram Sang wrote:
> Commit "mmc: renesas_sdhi: use MMC_CAP2_NO_WRITE_PROTECT instead of
> TMIO own flag" activated MMC_CAP2_NO_WRITE_PROTECT for Renesas SDHI
> which incorrectly disabled WP altogether instead of only disabling the
> internal mechanism. Since the whole WP handling has been reworked, we
> can simply disable this capability to re-enable WP GPIOs.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Ulf, it seems this patch is also not proper yet, we still have
regressions. I am working on it right now and hope to have a fix ASAP. I
know it is super-last-minute, but I hope for an easy to parse two-liner
which we maybe can still have in 4.17.

Just so you know already...
diff mbox

Patch

diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index a04f31fea72fd1..8e0acd197c43c0 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -75,7 +75,6 @@  static const struct renesas_sdhi_of_data of_rcar_gen3_compatible = {
 			  TMIO_MMC_HAVE_CBSY | TMIO_MMC_MIN_RCAR2,
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
 			  MMC_CAP_CMD23,
-	.capabilities2	= MMC_CAP2_NO_WRITE_PROTECT,
 	.bus_shift	= 2,
 	.scc_offset	= 0x1000,
 	.taps		= rcar_gen3_scc_taps,
diff --git a/drivers/mmc/host/renesas_sdhi_sys_dmac.c b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
index 4bb46c489d71f8..848e50c1638aa6 100644
--- a/drivers/mmc/host/renesas_sdhi_sys_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
@@ -42,7 +42,6 @@  static const struct renesas_sdhi_of_data of_rz_compatible = {
 static const struct renesas_sdhi_of_data of_rcar_gen1_compatible = {
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_CLK_ACTUAL,
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
-	.capabilities2	= MMC_CAP2_NO_WRITE_PROTECT,
 };
 
 /* Definitions for sampling clocks */
@@ -62,7 +61,6 @@  static const struct renesas_sdhi_of_data of_rcar_gen2_compatible = {
 			  TMIO_MMC_HAVE_CBSY | TMIO_MMC_MIN_RCAR2,
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
 			  MMC_CAP_CMD23,
-	.capabilities2	= MMC_CAP2_NO_WRITE_PROTECT,
 	.dma_buswidth	= DMA_SLAVE_BUSWIDTH_4_BYTES,
 	.dma_rx_offset	= 0x2000,
 	.scc_offset	= 0x0300,
@@ -83,7 +81,6 @@  static const struct renesas_sdhi_of_data of_rcar_gen3_compatible = {
 			  TMIO_MMC_HAVE_CBSY | TMIO_MMC_MIN_RCAR2,
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
 			  MMC_CAP_CMD23,
-	.capabilities2	= MMC_CAP2_NO_WRITE_PROTECT,
 	.bus_shift	= 2,
 	.scc_offset	= 0x1000,
 	.taps		= rcar_gen3_scc_taps,