| Message ID | 1400852087-32442-1-git-send-email-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 23 May 2014 15:34, Sascha Hauer <s.hauer@pengutronix.de> wrote: > The eMMC and the SD-Card specifications describe the optional SET_DSR command. > During measurements at our lab we found that some cards implementing this feature > having really strong driver strengts per default. This can lead to voltage peaks > above the specification of the host on signal edges for data sent from a card to > the host. > > Since availability of a given card type may be shorter than the time a certain > hardware will be produced it is useful to have support for this command (Alternative > would be changing termination resistors and adapting the driver strength of the > host to the used card.) > > As the value of the dsr register is board specific this adds a 'dsr' devicetree > property which is handled in mmc_of_parse. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > Documentation/devicetree/bindings/mmc/mmc.txt | 3 +++ > drivers/mmc/core/host.c | 3 +++ > drivers/mmc/core/mmc.c | 4 ++++ > drivers/mmc/core/mmc_ops.c | 17 +++++++++++++++++ > drivers/mmc/core/mmc_ops.h | 1 + > include/linux/mmc/card.h | 3 ++- > include/linux/mmc/host.h | 2 ++ > 7 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt > index 9dce540..737203e 100644 > --- a/Documentation/devicetree/bindings/mmc/mmc.txt > +++ b/Documentation/devicetree/bindings/mmc/mmc.txt > @@ -38,6 +38,9 @@ Optional properties: > - mmc-highspeed-ddr-1_2v: eMMC high-speed DDR mode(1.2V I/O) is supported > - mmc-hs200-1_8v: eMMC HS200 mode(1.8V I/O) is supported > - mmc-hs200-1_2v: eMMC HS200 mode(1.2V I/O) is supported > +- dsr: Content of the optional driver stage register (DSR) as described > + in JEDEC Standard No. 84-A44, 12.4: Programmable card output driver. Not > + all MMC cards implement this register and the actual value is card specific. I get the impression of that this is (e)MMC specific, but this is relevant for SD cards as well, right!? I don't find the part's that should be implemented for sd/sdio, could we add that in this patch as well? > > *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line > polarity properties, we have to fix the meaning of the "normal" and "inverted" > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c > index fdea825..eea272e 100644 > --- a/drivers/mmc/core/host.c > +++ b/drivers/mmc/core/host.c > @@ -447,6 +447,7 @@ int mmc_of_parse(struct mmc_host *host) > host->caps2 |= MMC_CAP2_HS200_1_8V_SDR; > if (of_find_property(np, "mmc-hs200-1_2v", &len)) > host->caps2 |= MMC_CAP2_HS200_1_2V_SDR; > + of_property_read_u32(np, "dsr", &host->dsr); > > return 0; > > @@ -515,6 +516,8 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev) > host->max_blk_size = 512; > host->max_blk_count = PAGE_CACHE_SIZE / 512; > > + host->dsr = 0xffffffff; > + Why is this needed? > return host; > > free: > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > index 1ab5f3a..0ab1c44 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -162,6 +162,7 @@ static int mmc_decode_csd(struct mmc_card *card) > csd->read_partial = UNSTUFF_BITS(resp, 79, 1); > csd->write_misalign = UNSTUFF_BITS(resp, 78, 1); > csd->read_misalign = UNSTUFF_BITS(resp, 77, 1); > + csd->dsr_imp = UNSTUFF_BITS(resp, 76, 1); > csd->r2w_factor = UNSTUFF_BITS(resp, 26, 3); > csd->write_blkbits = UNSTUFF_BITS(resp, 22, 4); > csd->write_partial = UNSTUFF_BITS(resp, 21, 1); > @@ -970,6 +971,9 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, > goto free_card; > } > > + if (card->csd.dsr_imp && (host->dsr & 0xffff) == host->dsr) To clarify, maybe add an extra pair of parentheses around the above expression? Additionally I don't quite understand the comparison? Couldn't we just use a "u16" to simplify this? > + mmc_set_dsr(host); > + > /* > * Select card, as all following commands rely on that. > */ > diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c > index f51b5ba..f8af72a 100644 > --- a/drivers/mmc/core/mmc_ops.c > +++ b/drivers/mmc/core/mmc_ops.c > @@ -93,6 +93,23 @@ int mmc_deselect_cards(struct mmc_host *host) > return _mmc_select_card(host, NULL); > } > > +int mmc_set_dsr(struct mmc_host *host) > +{ > + int err; > + struct mmc_command cmd = {0}; > + > + cmd.opcode = MMC_SET_DSR; > + > + cmd.arg = (host->dsr << 16) | 0xffff; > + cmd.flags = MMC_RSP_NONE | MMC_CMD_AC; I think it should be MMC_CMD_BC instead of MMC_CMD_AC. > + > + err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES); > + if (err) > + return err; > + > + return 0; > +} > + > int mmc_go_idle(struct mmc_host *host) > { > int err; > diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h > index 80ae9f4..390dac6 100644 > --- a/drivers/mmc/core/mmc_ops.h > +++ b/drivers/mmc/core/mmc_ops.h > @@ -14,6 +14,7 @@ > > int mmc_select_card(struct mmc_card *card); > int mmc_deselect_cards(struct mmc_host *host); > +int mmc_set_dsr(struct mmc_host *host); > int mmc_go_idle(struct mmc_host *host); > int mmc_send_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr); > int mmc_all_send_cid(struct mmc_host *host, u32 *cid); > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h > index b730272..ced116f 100644 > --- a/include/linux/mmc/card.h > +++ b/include/linux/mmc/card.h > @@ -42,7 +42,8 @@ struct mmc_csd { > unsigned int read_partial:1, > read_misalign:1, > write_partial:1, > - write_misalign:1; > + write_misalign:1, > + dsr_imp:1; > }; > > struct mmc_ext_csd { > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index cb61ea4..d8d9d68 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -358,6 +358,8 @@ struct mmc_host { > > unsigned int slotno; /* used for sdio acpi binding */ > > + u32 dsr; /* optional driver stage register (dsr) value */ Again, the DSR register is 16 bits. Couldn't we use u16 instead? Moreover, feel free to to move this further up in the struct. I would put it between ocr_avail and f_init, since it's more related to these. Kind regards Uffe > + > unsigned long private[0] ____cacheline_aligned; > }; > > -- > 2.0.0.rc0 > > -- > 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-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 16, 2014 at 01:36:38PM +0200, Ulf Hansson wrote: > On 23 May 2014 15:34, Sascha Hauer <s.hauer@pengutronix.de> wrote: > > @@ -970,6 +971,9 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, > > goto free_card; > > } > > > > + if (card->csd.dsr_imp && (host->dsr & 0xffff) == host->dsr) > > To clarify, maybe add an extra pair of parentheses around the above expression? To which I say, don't you bloody dare. It's perfectly readable as it is, it doesn't need any additional parenthesis. Additional useless parenthesis detracts from readability. Don't encourage stuch stuff.
Ulf Hansson <ulf.hansson <at> linaro.org> writes: > > On 23 May 2014 15:34, Sascha Hauer <s.hauer <at> pengutronix.de> wrote: > > The eMMC and the SD-Card specifications describe the optional SET_DSR command. > > During measurements at our lab we found that some cards implementing this feature > > having really strong driver strengts per default. This can lead to voltage peaks > > above the specification of the host on signal edges for data sent from a card to > > the host. > > > > Since availability of a given card type may be shorter than the time a certain > > hardware will be produced it is useful to have support for this command (Alternative > > would be changing termination resistors and adapting the driver strength of the > > host to the used card.) > > > > As the value of the dsr register is board specific this adds a 'dsr' devicetree > > property which is handled in mmc_of_parse. > > > > Signed-off-by: Sascha Hauer <s.hauer <at> pengutronix.de> > > --- > > Documentation/devicetree/bindings/mmc/mmc.txt | 3 +++ > > drivers/mmc/core/host.c | 3 +++ > > drivers/mmc/core/mmc.c | 4 ++++ > > drivers/mmc/core/mmc_ops.c | 17 +++++++++++++++++ > > drivers/mmc/core/mmc_ops.h | 1 + > > include/linux/mmc/card.h | 3 ++- > > include/linux/mmc/host.h | 2 ++ > > 7 files changed, 32 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt > > index 9dce540..737203e 100644 > > --- a/Documentation/devicetree/bindings/mmc/mmc.txt > > +++ b/Documentation/devicetree/bindings/mmc/mmc.txt > > <at> <at> -38,6 +38,9 <at> <at> Optional properties: > > - mmc-highspeed-ddr-1_2v: eMMC high-speed DDR mode(1.2V I/O) is supported > > - mmc-hs200-1_8v: eMMC HS200 mode(1.8V I/O) is supported > > - mmc-hs200-1_2v: eMMC HS200 mode(1.2V I/O) is supported > > +- dsr: Content of the optional driver stage register (DSR) as described > > + in JEDEC Standard No. 84-A44, 12.4: Programmable card output driver. Not > > + all MMC cards implement this register and the actual value is card specific. > > I get the impression of that this is (e)MMC specific, but this is > relevant for SD cards as well, right!? > Yes, the feature is optional for SD memory cards but not supported by SDIO. (accordding to SDIO Simplified Specification Version 2.00) On the other hand, I've not seen an SD card that implements this feature. Just a second thought about the SD card side: as I read and understand the specs the mapping between register values and physical driver stage value is left over to the manufacturer. So the setting makes only sense for fix mounted cards like eMMC - correct me if I'm wrong. Markus > I don't find the part's that should be implemented for sd/sdio, could > we add that in this patch as well? > -- 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
diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt index 9dce540..737203e 100644 --- a/Documentation/devicetree/bindings/mmc/mmc.txt +++ b/Documentation/devicetree/bindings/mmc/mmc.txt @@ -38,6 +38,9 @@ Optional properties: - mmc-highspeed-ddr-1_2v: eMMC high-speed DDR mode(1.2V I/O) is supported - mmc-hs200-1_8v: eMMC HS200 mode(1.8V I/O) is supported - mmc-hs200-1_2v: eMMC HS200 mode(1.2V I/O) is supported +- dsr: Content of the optional driver stage register (DSR) as described + in JEDEC Standard No. 84-A44, 12.4: Programmable card output driver. Not + all MMC cards implement this register and the actual value is card specific. *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line polarity properties, we have to fix the meaning of the "normal" and "inverted" diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c index fdea825..eea272e 100644 --- a/drivers/mmc/core/host.c +++ b/drivers/mmc/core/host.c @@ -447,6 +447,7 @@ int mmc_of_parse(struct mmc_host *host) host->caps2 |= MMC_CAP2_HS200_1_8V_SDR; if (of_find_property(np, "mmc-hs200-1_2v", &len)) host->caps2 |= MMC_CAP2_HS200_1_2V_SDR; + of_property_read_u32(np, "dsr", &host->dsr); return 0; @@ -515,6 +516,8 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev) host->max_blk_size = 512; host->max_blk_count = PAGE_CACHE_SIZE / 512; + host->dsr = 0xffffffff; + return host; free: diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index 1ab5f3a..0ab1c44 100644 --- a/drivers/mmc/core/mmc.c +++ b/drivers/mmc/core/mmc.c @@ -162,6 +162,7 @@ static int mmc_decode_csd(struct mmc_card *card) csd->read_partial = UNSTUFF_BITS(resp, 79, 1); csd->write_misalign = UNSTUFF_BITS(resp, 78, 1); csd->read_misalign = UNSTUFF_BITS(resp, 77, 1); + csd->dsr_imp = UNSTUFF_BITS(resp, 76, 1); csd->r2w_factor = UNSTUFF_BITS(resp, 26, 3); csd->write_blkbits = UNSTUFF_BITS(resp, 22, 4); csd->write_partial = UNSTUFF_BITS(resp, 21, 1); @@ -970,6 +971,9 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, goto free_card; } + if (card->csd.dsr_imp && (host->dsr & 0xffff) == host->dsr) + mmc_set_dsr(host); + /* * Select card, as all following commands rely on that. */ diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c index f51b5ba..f8af72a 100644 --- a/drivers/mmc/core/mmc_ops.c +++ b/drivers/mmc/core/mmc_ops.c @@ -93,6 +93,23 @@ int mmc_deselect_cards(struct mmc_host *host) return _mmc_select_card(host, NULL); } +int mmc_set_dsr(struct mmc_host *host) +{ + int err; + struct mmc_command cmd = {0}; + + cmd.opcode = MMC_SET_DSR; + + cmd.arg = (host->dsr << 16) | 0xffff; + cmd.flags = MMC_RSP_NONE | MMC_CMD_AC; + + err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES); + if (err) + return err; + + return 0; +} + int mmc_go_idle(struct mmc_host *host) { int err; diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h index 80ae9f4..390dac6 100644 --- a/drivers/mmc/core/mmc_ops.h +++ b/drivers/mmc/core/mmc_ops.h @@ -14,6 +14,7 @@ int mmc_select_card(struct mmc_card *card); int mmc_deselect_cards(struct mmc_host *host); +int mmc_set_dsr(struct mmc_host *host); int mmc_go_idle(struct mmc_host *host); int mmc_send_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr); int mmc_all_send_cid(struct mmc_host *host, u32 *cid); diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h index b730272..ced116f 100644 --- a/include/linux/mmc/card.h +++ b/include/linux/mmc/card.h @@ -42,7 +42,8 @@ struct mmc_csd { unsigned int read_partial:1, read_misalign:1, write_partial:1, - write_misalign:1; + write_misalign:1, + dsr_imp:1; }; struct mmc_ext_csd { diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index cb61ea4..d8d9d68 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -358,6 +358,8 @@ struct mmc_host { unsigned int slotno; /* used for sdio acpi binding */ + u32 dsr; /* optional driver stage register (dsr) value */ + unsigned long private[0] ____cacheline_aligned; };
The eMMC and the SD-Card specifications describe the optional SET_DSR command. During measurements at our lab we found that some cards implementing this feature having really strong driver strengts per default. This can lead to voltage peaks above the specification of the host on signal edges for data sent from a card to the host. Since availability of a given card type may be shorter than the time a certain hardware will be produced it is useful to have support for this command (Alternative would be changing termination resistors and adapting the driver strength of the host to the used card.) As the value of the dsr register is board specific this adds a 'dsr' devicetree property which is handled in mmc_of_parse. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- Documentation/devicetree/bindings/mmc/mmc.txt | 3 +++ drivers/mmc/core/host.c | 3 +++ drivers/mmc/core/mmc.c | 4 ++++ drivers/mmc/core/mmc_ops.c | 17 +++++++++++++++++ drivers/mmc/core/mmc_ops.h | 1 + include/linux/mmc/card.h | 3 ++- include/linux/mmc/host.h | 2 ++ 7 files changed, 32 insertions(+), 1 deletion(-)