MMC: mmci: Provide bindings for Device Tree

This is a step in the right direction for future Device Tree support. It will allow variant specific attributes to be collected from a Device Tree without overloading the MMCI core. It will also provide additional future variants a cleaner way to add support.

Quoted 5 lines · Lee Jones

This is a step in the right direction for future Device Tree support. It will allow variant specific attributes to be collected from a Device Tree without overloading the MMCI core. It will also provide additional future variants a cleaner way to add support.

NAK. You can't have two driver structures with the same name. The names are sysfs object names.

Quoted 7 lines · Russell King - ARM Linux

Quoted 5 lines · Lee Jones

This is a step in the right direction for future Device Tree support. It will allow variant specific attributes to be collected from a Device Tree without overloading the MMCI core. It will also provide additional future variants a cleaner way to add support.

Quoted 2 lines

NAK. You can't have two driver structures with the same name. The names are sysfs object names.

Well spotted.

I've already changed this in v2. :)

Quoted 8 lines · Lee Jones

Quoted 7 lines · Russell King - ARM Linux

Quoted 5 lines · Lee Jones

This is a step in the right direction for future Device Tree support. It will allow variant specific attributes to be collected from a Device Tree without overloading the MMCI core. It will also provide additional future variants a cleaner way to add support.

Quoted 6 lines

Quoted 2 lines

NAK. You can't have two driver structures with the same name. The names are sysfs object names.

Well spotted.

I've already changed this in v2. :)

I'm not in favour of splitting the two variants either btw.

Can you elaborate? I suggested the split in order to keep the ux500 specific parts local to one file. With the device tree conversion, we really want to have them out of the platform code, but sticking them into the main driver seems wrong, too.

Quoted 4 lines · Arnd Bergmann

Can you elaborate? I suggested the split in order to keep the ux500 specific parts local to one file. With the device tree conversion, we really want to have them out of the platform code, but sticking them into the main driver seems wrong, too.

You're talking about the data structures which describe what quirks to apply for the ux500 parts, rather than the actual quirks themselves. That's not a particularly clever thing to do because it separates out the selection of the works from the rest of the driver, which means simply searching for the flags to find out what's applied for what has to span several files.

Quoted 6 lines · Russell King - ARM Linux

Quoted 4 lines · Arnd Bergmann

Can you elaborate? I suggested the split in order to keep the ux500 specific parts local to one file. With the device tree conversion, we really want to have them out of the platform code, but sticking them into the main driver seems wrong, too.

Quoted 6 lines

You're talking about the data structures which describe what quirks to apply for the ux500 parts, rather than the actual quirks themselves. That's not a particularly clever thing to do because it separates out the selection of the works from the rest of the driver, which means simply searching for the flags to find out what's applied for what has to span several files.

Right, that is a disadvantage, but it's also how a lot of other drivers work, in particular sdhci.

My reasoning is roughly this:

* we want to get rid of arch/arm/mach-ux500/board-mop500-sdi.c
* the sdi0_configure() and mop500_sdi0_vdd_handler() functions need to be moved somewhere in order to do that, so they should be with the driver.

* we don't want them in mmci.c, so we create a new file for these.
* in order to call do the setup from sdi0_configure right, it needs to be done in the probe() function

* if we want to have a separate probe function, we also need to have a separate amba_driver structure
* the variant_data in mmci.c belongs with the amba_id, so that also gets moved to the ux500 file.

If you have a better solution for one or more of these, I'm all ears.

The alternatives that I can see are:
a) keep using auxdata to supply a platform_data pointer and do everything in the main driver. Problem: we want to avoid auxdata if possible
b) move the code from patch 4 into mmci.c using #ifdef.
Problem: it's ugly code that has nothing to do with mmci in general.
c) use the regulator framework to do the voltage selection here, and have only generic code in mmci.c. This may be the best solution, but I have no idea if this is actually possible, or how to do it.

Quoted 5 lines · Arnd Bergmann

* we want to get rid of arch/arm/mach-ux500/board-mop500-sdi.c
* the sdi0_configure() and mop500_sdi0_vdd_handler() functions need to be moved somewhere in order to do that, so they should be with the driver.

I don't believe they should. MMCI _is_ the peripheral itself. All the stuff that's in board-mop500-sdi.c seems to be doing is configuration type stuff for the SoC that its been integrated into. Are you seriously advocating having multiple drivers for essentially the same hardware?

Quoted 7 lines

* if we want to have a separate probe function, we also need to have a separate amba_driver structure
* the variant_data in mmci.c belongs with the amba_id, so that also gets moved to the ux500 file.

If you have a better solution for one or more of these, I'm all ears.

Are you really advocating having mmci_foo.c where foo is an ever increasing number of SoCs with the MMCI integrated because you want to integrate SoC specific details into the MMCI peripheral driver?

Quoted 11 lines

The alternatives that I can see are:
a) keep using auxdata to supply a platform_data pointer and do everything in the main driver. Problem: we want to avoid auxdata if possible
b) move the code from patch 4 into mmci.c using #ifdef.
Problem: it's ugly code that has nothing to do with mmci in general.
c) use the regulator framework to do the voltage selection here, and have only generic code in mmci.c. This may be the best solution, but I have no idea if this is actually possible, or how to do it.

Let's ignore the DMA stuff for the time being - that's a separate problem which needs to be addressed once we work out how to deal with how DMA engine stuff gets resolved with DT based systems.

The remainder of it (mmci_platform_data) looks like it can be easily represented as DT properties, the exception to that is the ios_handler. I think the right solution for that is to move to the regulator framework, but allow the driver to continue having the ios callback if there is no attached regulator. It needs that because of the four integrated regulator control bits (which are entirely undefined.)

We _do_ need a solution for GPIO-controlled voltage regulators, as these are actually quite common - see the various soc-common using PCMCIA drivers.

So I think actually a bit more thought is required here, and there's a need to use some of the facilities such as regulator which are now present that weren't when the driver was originally written some 9 years ago.

Quoted 7 lines · Russell King - ARM Linux

We _do_ need a solution for GPIO-controlled voltage regulators, as these are actually quite common - see the various soc-common using PCMCIA drivers.

So I think actually a bit more thought is required here, and there's a need to use some of the facilities such as regulator which are now present that weren't when the driver was originally written some 9 years ago.

Ok. Let's see what Mark has to say about this.

Quoted 7 lines · Russell King - ARM Linux

Quoted 5 lines · Lee Jones

This is a step in the right direction for future Device Tree support. It will allow variant specific attributes to be collected from a Device Tree without overloading the MMCI core. It will also provide additional future variants a cleaner way to add support.

Quoted 2 lines

NAK. You can't have two driver structures with the same name. The names are sysfs object names.

Well spotted.

I've already changed this in v2. :)

This adds the necessary bindings for collection of shared attributes used in the mmci driver.

Signed-off-by Lee Jones <[email protected]>

---
?drivers/mmc/host/mmci.c | ? 43 +++++++++++++++++++++++++++++++++++++++++++ ?1 files changed, 43 insertions(+), 0 deletions(-)

drivers/mmc/host/mmci.c +0 -0

index 23b41a5..9132ca8 100644
@@ -30,6 +30,7 @@

?#include <linux/dma-mapping.h>
?#include <linux/amba/mmci.h>
?#include <linux/pm_runtime.h>
+#include <linux/of_gpio.h>

?#include <asm/div64.h>
?#include <asm/io.h>

drivers/mmc/host/mmci.c +15 -0

@@ -1056,11 +1057,47 @@ static const struct mmc_host_ops mmci_ops = {
 ? ? ? ?.get_cd ? ? ? ? = mmci_get_cd,
 ?};
 
+#ifdef CONFIG_OF
+static void mmci_dt_populate_generic_pdata(struct device_node *np,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct mmci_platform_data *pdata)
+{
+ ? ? ? const void *prop;
+ ? ? ? int len;
+
+ ? ? ? of_property_read_u32(np, "wp-gpios", &pdata->gpio_wp);
+ ? ? ? if (!pdata->gpio_wp)
+ ? ? ? ? ? ? ? pdata->gpio_wp = -1;
+
+ ? ? ? of_property_read_u32(np, "cd-gpios", &pdata->gpio_cd);
+ ? ? ? if (!pdata->gpio_cd)
+ ? ? ? ? ? ? ? pdata->gpio_cd = -1;
+

Quoted 44 lines · Lee Jones

This adds the necessary bindings for collection of shared attributes used in the mmci driver.

Signed-off-by Lee Jones <[email protected]>

---
 drivers/mmc/host/mmci.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 43 insertions(+), 0 deletions(-)

drivers/mmc/host/mmci.c +21 -0

index 23b41a5..9132ca8 100644
@@ -30,6 +30,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/amba/mmci.h>
 #include <linux/pm_runtime.h>
+#include <linux/of_gpio.h>
 
 #include <asm/div64.h>
 #include <asm/io.h>

@@ -1056,11 +1057,47 @@ static const struct mmc_host_ops mmci_ops = {
       .get_cd         = mmci_get_cd,
 };
 
+#ifdef CONFIG_OF
+static void mmci_dt_populate_generic_pdata(struct device_node *np,
+                                       struct mmci_platform_data *pdata)
+{
+       const void *prop;
+       int len;
+
+       of_property_read_u32(np, "wp-gpios", &pdata->gpio_wp);
+       if (!pdata->gpio_wp)
+               pdata->gpio_wp = -1;
+
+       of_property_read_u32(np, "cd-gpios", &pdata->gpio_cd);
+       if (!pdata->gpio_cd)
+               pdata->gpio_cd = -1;
+
+       if (of_get_property(np, "cd-invert", NULL))
+               pdata->cd_invert = true;
+       else
+               pdata->cd_invert = false;
+

drivers/mmc/host/mmci.c static const struct mmc_host_ops mmci_ops = { · line 1084

Quoted 5 lines

drivers/mmc/host/mmci.c +5 -0

+       of_property_read_u32(np, "clock_frequency", &pdata->f_max);
+       if (!pdata->f_max)
+               pr_warning("%s has no 'clock_frequency' property\n", np->full_name);
+
+       if (of_get_property(np, "mmc_cap_4_bit_data", NULL))

I have no previous experience with DT. Could you please bring some light on this.
Is it really necessary to represent each bit in the CAP with a string?

To add CAP_ERASE for instance I need to change the code here and update the DT, right?

+ux500 variant specific properties:

+- gpio-en                : contains the level-shifter enable gpio
+- gpio-vsel              : contains the level-shifter voltage select gpio
+- tx_dst_dev_type        : contains the tx destination device (DMA) 
+- rx_src_dev_type        : contains the rx source device (DMA)

Quoted 5 lines · Lee Jones

+ux500 variant specific properties:

+- gpio-en                : contains the level-shifter enable gpio
+- gpio-vsel              : contains the level-shifter voltage select gpio
+- tx_dst_dev_type        : contains the tx destination device (DMA) 
+- rx_src_dev_type        : contains the rx source device (DMA)

So what about the DMA support being proposed elsewhere?

This sounds like we're inventing one DMA solution for one bit of the tree and another DMA solution for another bit. This is madness.

Quoted 8 lines · Russell King - ARM Linux

Quoted 5 lines · Lee Jones

+ux500 variant specific properties:

+- gpio-en                : contains the level-shifter enable gpio
+- gpio-vsel              : contains the level-shifter voltage select gpio
+- tx_dst_dev_type        : contains the tx destination device (DMA) 
+- rx_src_dev_type        : contains the rx source device (DMA)

Quoted 4 lines

So what about the DMA support being proposed elsewhere?

This sounds like we're inventing one DMA solution for one bit of the tree and another DMA solution for another bit. This is madness.

Agreed. I originally made a similar comment for an early version but had not noticed that it made it into this one. This should be using the proper DMA bindings, or just auxdata while we're not entirely there yet.

+Required properties:
+- compatible : contains "arm,pl18x", "arm,primecell".

+- reg                    : contains pl18x registers and length.
+- interrupts             : contains the device IRQ(s).

+- arm,primecell-periphid : contains the PrimeCell Peripheral ID.

+- wp-gpios               : contains any write protect (ro) gpios
+- cd-gpios               : contains any card detection gpios
+- cd-invert              : indicates whether the cd gpio is inverted 
+- clock_frequency        : contains the maximum operating frequency

+- mmc_cap_4_bit_data     : indicates whether device is 4 bit capable
+- mmc_cap_8_bit_data     : indicates whether device is 8 bit capable
+- mmc_cap_mmc_highspeed  : indicates whether MMC is high speed capable
+- mmc_cap_sd_highspeed   : indicates whether SD is high speed capable

+ux500 variant specific properties:

+- gpio-en                : contains the level-shifter enable gpio
+- gpio-vsel              : contains the level-shifter voltage select gpio

+- tx_dst_dev_type        : contains the tx destination device (DMA) 
+- rx_src_dev_type        : contains the rx source device (DMA)

Documentation/devicetree/bindings/mmc/mmci.txt line 13

Quoted 3 lines · Lee Jones

Documentation/devicetree/bindings/mmc/mmci.txt new file +3 -0

+
+Optional properties:
+- ocr_mask               : contains the regulator oscillator mask

Not sure about this one. If it's for the regulators, we should be using the regulator bindings.

This patch-set splits out ux500 and ARM specific variants from mmci core code, applies generic bindings for use by all variants, then enables the ux500 variant for use with Device Tree.

?Documentation/devicetree/bindings/mmc/mmci.txt | ? 27 +++
?arch/arm/boot/dts/db8500.dtsi ? ? ? ? ? ? ? ? ?| ? ?6 +-
?arch/arm/boot/dts/snowball.dts ? ? ? ? ? ? ? ? | ? 30 +++-
?arch/arm/mach-ux500/board-mop500.c ? ? ? ? ? ? | ? ?3 +-
?drivers/mmc/host/Makefile ? ? ? ? ? ? ? ? ? ? ?| ? ?2 +-
?drivers/mmc/host/mmci-arm.c ? ? ? ? ? ? ? ? ? ?| ? 65 ++++++
?drivers/mmc/host/mmci-ux500.c ? ? ? ? ? ? ? ? ?| ?260 ++++++++++++++++++++++++
?drivers/mmc/host/mmci.c ? ? ? ? ? ? ? ? ? ? ? ?| ?194 +++++-------------
?drivers/mmc/host/mmci.h ? ? ? ? ? ? ? ? ? ? ? ?| ? 37 ++++

?9 files changed, 473 insertions(+), 151 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at ?http://vger.kernel.org/majordomo-info.html

I would like to see what the minimal required change is to support DT for mmci without factorization.
1. Minimal change in mmci.
2. Add mmci_dt.c which contains the DT-populate code.

The factorization could be done as step 2 I think.

What do you say?

Quoted 19 lines

This patch-set splits out ux500 and ARM specific variants from mmci core code, applies generic bindings for use by all variants, then enables the ux500 variant for use with Device Tree.

 Documentation/devicetree/bindings/mmc/mmci.txt |   27 +++
 arch/arm/boot/dts/db8500.dtsi                  |    6 +-
 arch/arm/boot/dts/snowball.dts                 |   30 +++-
 arch/arm/mach-ux500/board-mop500.c             |    3 +-
 drivers/mmc/host/Makefile                      |    2 +-
 drivers/mmc/host/mmci-arm.c                    |   65 ++++++
 drivers/mmc/host/mmci-ux500.c                  |  260 ++++++++++++++++++++++++
 drivers/mmc/host/mmci.c                        |  194 +++++-------------
 drivers/mmc/host/mmci.h                        |   37 ++++
 9 files changed, 473 insertions(+), 151 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines

I would like to see what the minimal required change is to support DT for mmci without factorization.
1. Minimal change in mmci.
2. Add mmci_dt.c which contains the DT-populate code.

Quoted 3 lines

The factorization could be done as step 2 I think.

What do you say?

I'm wondering what the difference is as the work has already been done.

It was Arnd's suggestion to separate out the two types of variants, and I'm quite fond of the new (fully featured) layout.

Quoted 5 lines · Lee Jones

Quoted 5 lines

I would like to see what the minimal required change is to support DT for mmci without factorization.
1. Minimal change in mmci.
2. Add mmci_dt.c which contains the DT-populate code.

Quoted 8 lines

Quoted 3 lines

The factorization could be done as step 2 I think.

What do you say?

I'm wondering what the difference is as the work has already been done.

It was Arnd's suggestion to separate out the two types of variants, and I'm quite fond of the new (fully featured) layout.

Right, I usually prefer cleanups or other refactoring to be done first, and then features added on top.

You could in theory add have just patches 3/4/5 all applied without the refactoring, but that I would be worried that this causes dependencies between the mmci driver and ux500 specific functionality like the stedma40_filter function.

Quoted 5 lines · Lee Jones

Quoted 5 lines

I would like to see what the minimal required change is to support DT for mmci without factorization.
1. Minimal change in mmci.
2. Add mmci_dt.c which contains the DT-populate code.

Quoted 8 lines

Quoted 3 lines

The factorization could be done as step 2 I think.

What do you say?

I'm wondering what the difference is as the work has already been done.

It was Arnd's suggestion to separate out the two types of variants, and I'm quite fond of the new (fully featured) layout.

Right, I usually prefer cleanups or other refactoring to be done first, and then features added on top.

You could in theory add have just patches 3/4/5 all applied without the refactoring, but that I would be worried that this causes dependencies between the mmci driver and ux500 specific functionality like the stedma40_filter function.

About refactoring
There are numbers of patches stashed up for MMCI waiting for verdict
here http://git.kernel.org/?p=linux/kernel/git/linusw/linux-stericsson.git;a=shortlog;h=refs/heads/mmci.
If doing a refactorization please rebase it on top of these patches.

Quoted 15 lines · Arnd Bergmann

Quoted 5 lines · Lee Jones

Quoted 5 lines

I would like to see what the minimal required change is to support DT for mmci without factorization.
1. Minimal change in mmci.
2. Add mmci_dt.c which contains the DT-populate code.

Quoted 8 lines

Quoted 3 lines

The factorization could be done as step 2 I think.

What do you say?

I'm wondering what the difference is as the work has already been done.

It was Arnd's suggestion to separate out the two types of variants, and I'm quite fond of the new (fully featured) layout.

Quoted 8 lines

Right, I usually prefer cleanups or other refactoring to be done first, and then features added on top.

You could in theory add have just patches 3/4/5 all applied without the refactoring, but that I would be worried that this causes dependencies between the mmci driver and ux500 specific functionality like the stedma40_filter function.

About DT
I think I need to catch up on the DT-design a bit. I will try to catch up on the DT-implementation and maybe then the refactorization will make sense to me :)

Board specific data in mmci-driver
The stedma40 filter function is not really specific for ux500. ux500 use stedma40 but it should be possible to replace this DMA.IP with some other DMA-controller. This is board specific configuration. You should not need to change the mmci-driver just because the dma-driver has changed, right?
Or will the board-configuration now be placed in mmci-ux500?

Common DT populate code for all mmc host drivers
Some parts of the DT-population is common for all the host driver, for instance setting the CAP and CAP2. This code should be moved to a common place to be used by other host drivers as well.

About refactoring
There are numbers of patches stashed up for MMCI waiting for verdict
here http://git.kernel.org/?p=linux/kernel/git/linusw/linux-stericsson.git;a=shortlog;h=refs/heads/mmci.
If doing a refactorization please rebase it on top of these patches.
This needs to be done carefully, there may be more to considerations
than just DT. Maybe the timing is better to do this for 4.5, just
after 4.4 is closed. Then we can make sure that all new patches are
made on top of the refactorized base.

[Per]

Quoted 6 lines

The stedma40 filter function is not really specific for ux500. ux500 use stedma40 but it should be possible to replace this DMA.IP with some other DMA-controller. This is board specific configuration. You should not need to change the mmci-driver just because the dma-driver has changed, right?
Or will the board-configuration now be placed in mmci-ux500?

Right, the DMA configuration does not really belong in there, but the voltage setup might (unless we convert that to the regulator setup).

Quoted 11 lines · Arnd Bergmann

[Per]

Quoted 6 lines

The stedma40 filter function is not really specific for ux500. ux500 use stedma40 but it should be possible to replace this DMA.IP with some other DMA-controller. This is board specific configuration. You should not need to change the mmci-driver just because the dma-driver has changed, right?
Or will the board-configuration now be placed in mmci-ux500?

Right, the DMA configuration does not really belong in there, but the voltage setup might (unless we convert that to the regulator setup).

The voltage to power the card is already using the regulator framework with the MMC-specific helpers in the MMCI driver.

But I guess you're after modelling the levelshifter as a regulator?

Basically the level shifter is a separate device has two voltage inputs A and B (from other regulators) that is controlled by a simple GPIO to select voltage A or B to drive the signals to the card.

That could probably be modelled as a regulator with two volategs for sure, but then we should maybe create a more generic "struct level_shifter_regulator" (or whatever) for the concept as a whole.

Let's page Mark about what to do with levelshifters and whether they are regulators of sorts in his book.

Quoted 8 lines · Linus Walleij

On Thu, Mar 15, 2012 at 9:58 PM, Arnd Bergmann <[email protected]> wrote:

But I guess you're after modelling the levelshifter as a regulator?

Basically the level shifter is a separate device has two voltage inputs A and B (from other regulators) that is controlled by a simple GPIO to select voltage A or B to drive the signals to the card.

Quoted 4 lines

That could probably be modelled as a regulator with two volategs for sure, but then we should maybe create a more generic "struct level_shifter_regulator" (or whatever) for the concept as a whole.

Ok, thanks for the explanation.

Quoted 2 lines

Let's page Mark about what to do with levelshifters and whether they are regulators of sorts in his book.

It does sound appealing, especially because this one could be done completely generically by defining a regulator that has a bunch of other regulators as well as a set of gpio lines as inputs and one output that can be used in other devices. We would probably only use this one together with device tree then.