diff mbox

[v3,1/3] mmc: sh_mobile_sdhi: add ocr_mask option

Message ID 20160912141507.6837-2-chris.brandt@renesas.com (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Brandt Sept. 12, 2016, 2:15 p.m. UTC
In moving platforms from board files to DT, there still needs to be a way
to set the ocr_mask setting for the tmio driver during probe. Without this
setting, the probe will fail because the supported voltages are not known.

This patch will also traditional platform registration platforms to
migrate to DT.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 drivers/mmc/host/sh_mobile_sdhi.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Ulf Hansson Sept. 13, 2016, 12:57 p.m. UTC | #1
On 12 September 2016 at 16:15, Chris Brandt <chris.brandt@renesas.com> wrote:
> In moving platforms from board files to DT, there still needs to be a way
> to set the ocr_mask setting for the tmio driver during probe. Without this
> setting, the probe will fail because the supported voltages are not known.

Regarding the ocr_mask; How do these SoCs provides the power to the mmc/sd card?

Do note, I am *not* talking about the I/O voltage but the core power
to the card.

The reason for raising the question is that we have infrastructures in
the mmc core which can create the ocr_mask, by parsing a regulator's
voltage range. This is the recommended method to use, instead of using
hard coded ocr mask values.

Kind regards
Uffe

>
> This patch will also traditional platform registration platforms to
> migrate to DT.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  drivers/mmc/host/sh_mobile_sdhi.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> index 5334f24..b033500 100644
> --- a/drivers/mmc/host/sh_mobile_sdhi.c
> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> @@ -59,6 +59,7 @@ enum tmio_mmc_dmac_type {
>
>  struct sh_mobile_sdhi_of_data {
>         unsigned long tmio_flags;
> +       u32           tmio_ocr_mask;
>         unsigned long capabilities;
>         unsigned long capabilities2;
>         enum dma_slave_buswidth dma_buswidth;
> @@ -630,6 +631,7 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
>                 const struct sh_mobile_sdhi_of_data *of_data = of_id->data;
>
>                 mmc_data->flags |= of_data->tmio_flags;
> +               mmc_data->ocr_mask = of_data->tmio_ocr_mask;
>                 mmc_data->capabilities |= of_data->capabilities;
>                 mmc_data->capabilities2 |= of_data->capabilities2;
>                 mmc_data->dma_rx_offset = of_data->dma_rx_offset;
> --
> 2.9.2
>
>
--
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
Geert Uytterhoeven Sept. 13, 2016, 1:26 p.m. UTC | #2
Hi Ulf,

On Tue, Sep 13, 2016 at 2:57 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 12 September 2016 at 16:15, Chris Brandt <chris.brandt@renesas.com> wrote:
>> In moving platforms from board files to DT, there still needs to be a way
>> to set the ocr_mask setting for the tmio driver during probe. Without this
>> setting, the probe will fail because the supported voltages are not known.
>
> Regarding the ocr_mask; How do these SoCs provides the power to the mmc/sd card?
>
> Do note, I am *not* talking about the I/O voltage but the core power
> to the card.
>
> The reason for raising the question is that we have infrastructures in
> the mmc core which can create the ocr_mask, by parsing a regulator's
> voltage range. This is the recommended method to use, instead of using
> hard coded ocr mask values.

On RSKRZA1, 3.3V is provided to the SD/MMC socket through an MIC2026
MOSFET switch.
On Genmai, 3.3V or 5V is provided through an LTC1471 switch.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
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
Chris Brandt Sept. 13, 2016, 1:28 p.m. UTC | #3
On 9/13/2016, Ulf Hansson wrote:
> Regarding the ocr_mask; How do these SoCs provides the power to the mmc/sd

> card?

> 

> Do note, I am *not* talking about the I/O voltage but the core power to

> the card.


The SoC does not *provide* power to the card. It's not that fancy. How the 3.3v is supplied to the card slot is up to the board designer. But, the SoC spec says the I/O should be 3.3v, so we might as well fix the OCR as 3.3v because they don't have a choice.

I did see all the DT regulator stuff for the other Renesas SoC, but all that is beyond how this chip is being used.

Also note that this SoC (Renesas RZ/A1), unlike the others, is being used with XIP_KERNEL because it's possible to run a full kernel without any external memory (it comes in a 3MB, 5MB, or 10RM internal RAM versions). So, I'm trying to avoid adding extra DT "stuff" that just takes up more RAM resources and has no value for these simple embedded systems. I know in other DDR based system with Megs and Megs of RAM to spare this isn't a problem, but in an XIP_KERNEL environment, it all adds up.

One of the reasons why I haven't been trying to push our BSP upstream and move everyone is because I'm afraid of what all the new DT bloat is going to do to the kernel RAM usage.

Chris
Chris Brandt Sept. 13, 2016, 1:50 p.m. UTC | #4
Here's a question:

The DT regulator method is good if you want to be able to control the regulator at run-time by the system.

But for MMC and SDHI, why isn't there a way to just set the OCR voltage in the board's DT if it's fixed (other than making a fixed regulator node)?
Why isn't there a mmc-ocr-mask property? That's really what I want and it seems like it would make a lot of dts files more simple.


Chris
Ulf Hansson Sept. 13, 2016, 3:10 p.m. UTC | #5
On 13 September 2016 at 15:50, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> Here's a question:
>
> The DT regulator method is good if you want to be able to control the regulator at run-time by the system.
>
> But for MMC and SDHI, why isn't there a way to just set the OCR voltage in the board's DT if it's fixed (other than making a fixed regulator node)?
> Why isn't there a mmc-ocr-mask property? That's really what I want and it seems like it would make a lot of dts files more simple.

Because, if you have an external regulator feeding the mmc with power
you should set it up and use it.

Now, we do have cases where it's actually the MMC controller that
manges the power to the card. So no external regulators being used.
For these case we have the "voltage-ranges" DT binding, but I don't
think you should be using that for these cases. :-)

Kind regards
Uffe
--
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
Chris Brandt Sept. 13, 2016, 3:59 p.m. UTC | #6
SGVsbG8gVWZmZQ0KDQpUaGFuayB5b3UgZm9yIHlvdXIgcmVwbHkuDQoNCk9uIDkvMTMvMjAxNiwg
VWxmIEhhbnNzb24gd3JvdGU6DQo+IEJlY2F1c2UsIGlmIHlvdSBoYXZlIGFuIGV4dGVybmFsIHJl
Z3VsYXRvciBmZWVkaW5nIHRoZSBtbWMgd2l0aCBwb3dlciB5b3UNCj4gc2hvdWxkIHNldCBpdCB1
cCBhbmQgdXNlIGl0Lg0KDQpCdXQgaW4gbWFueSBib2FyZCBkZXNpZ25zLCB0aGVyZSBpcyBubyBj
b250cm9sIG92ZXIgdGhlIHJlZ3VsYXRvciBzdXBwbHkgKG9yIHlvdSB3b3VsZCByYXRoZXIgaGF2
ZSB5b3VyIGJvb3Rsb2FkZXIgZG8gdGhhdCwgbm90IHRoZSBrZXJuZWwpLg0KDQpTbyB3aGF0IHRo
aXMgcGF0Y2ggaXMgZG9pbmcgaXMgc2V0dGluZyBpdCB0byBhIGRlZmF1bHQgb2YgMy4zdiAoZm9y
IGp1c3QgdGhpcyBTb0MpIGFuZCBpZiBzb21lb25lIHdhbnRzIHRvIGFkZCBhIHJlZ3VsYXRvciBp
biB0aGUgYm9hcmQncyBEVCBmaWxlLCB0aGF0IHdpbGwgb3ZlcnJpZGUgdGhlIG9jcl9tYXNrIHNl
dCBieSB0aGUgZHJpdmVyLg0KDQoNCkknbSBhc3N1bWluZyBJJ2xsIGhhdmUgdG8gZG8gdGhlIHNh
bWUgZm9yIE1NQyAoc2hfbW1jaWYuYykgb25jZSBJIGdldCB0byB0aGF0IGRyaXZlciAoSSBuZWVk
IHRvIGZpbmQgbXkgTU1DIFBMVVMgY2FyZCBmaXJzdCBmb3IgdGVzdGluZykuDQoNCg0KQ2hyaXMN
Cg0K
--
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
Ulf Hansson Sept. 17, 2016, 9:12 a.m. UTC | #7
On 13 September 2016 at 17:59, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> Hello Uffe
>
> Thank you for your reply.
>
> On 9/13/2016, Ulf Hansson wrote:
>> Because, if you have an external regulator feeding the mmc with power you
>> should set it up and use it.
>
> But in many board designs, there is no control over the regulator supply (or you would rather have your bootloader do that, not the kernel).
>
> So what this patch is doing is setting it to a default of 3.3v (for just this SoC) and if someone wants to add a regulator in the board's DT file, that will override the ocr_mask set by the driver.

That's perfectly okay to me! I just wanted to get clear picture of
*why* this change was needed.

>
>
> I'm assuming I'll have to do the same for MMC (sh_mmcif.c) once I get to that driver (I need to find my MMC PLUS card first for testing).

Okay!

Kind regards
Uffe
--
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
Chris Brandt Sept. 17, 2016, 1:38 p.m. UTC | #8
On 9/17/2016, Ulf Hansson wrote:
> > I'm assuming I'll have to do the same for MMC (sh_mmcif.c) once I get to

> that driver (I need to find my MMC PLUS card first for testing).

> 

> Okay!

> 

> Kind regards

> Uffe


Just FYI,
For the eMMC case,  I decided not to modify sh_mmcif.c to add the OCR and instead just create a 3.3 fixed regulator in the DT.

The reason is that if someone is using eMMC for their file system, they are probably using external RAM (since block data needs to be copied out of eMMC into RAM before they can be executed). So the extra RAM needed for the regular DT is not really an issue.


However, the people using SDHI are using it for SDIO for Wifi modules and they are the ones with the slimmed down RAM systems. So for them, removing the regulator DT will help squeeze their system into 3MB or 5MB of in-chip system RAM.


Chris
Wolfram Sang Oct. 20, 2016, 1:05 p.m. UTC | #9
On Mon, Sep 12, 2016 at 10:15:05AM -0400, Chris Brandt wrote:
> In moving platforms from board files to DT, there still needs to be a way
> to set the ocr_mask setting for the tmio driver during probe. Without this
> setting, the probe will fail because the supported voltages are not known.
> 
> This patch will also traditional platform registration platforms to
> migrate to DT.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

I'm fine with this change:

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

Maybe some details from this discussion could be added to the commit
message?
diff mbox

Patch

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 5334f24..b033500 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -59,6 +59,7 @@  enum tmio_mmc_dmac_type {
 
 struct sh_mobile_sdhi_of_data {
 	unsigned long tmio_flags;
+	u32	      tmio_ocr_mask;
 	unsigned long capabilities;
 	unsigned long capabilities2;
 	enum dma_slave_buswidth dma_buswidth;
@@ -630,6 +631,7 @@  static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 		const struct sh_mobile_sdhi_of_data *of_data = of_id->data;
 
 		mmc_data->flags |= of_data->tmio_flags;
+		mmc_data->ocr_mask = of_data->tmio_ocr_mask;
 		mmc_data->capabilities |= of_data->capabilities;
 		mmc_data->capabilities2 |= of_data->capabilities2;
 		mmc_data->dma_rx_offset = of_data->dma_rx_offset;