Re: [PATCH 3/7] mmc: dw_mmc: add device tree support

[Kyungmin Park]

On 5/10/12, Thomas Abraham <thomas.abraham@xxxxxxxxxx> wrote:
> Dear Mr. Park,
>
> On 2 May 2012 12:25, Kyungmin Park <kmpark@xxxxxxxxxxxxx> wrote:
>> Hi,
>>
>> On 5/2/12, Thomas Abraham <thomas.abraham@xxxxxxxxxx> wrote:
>>> Add device tree based discovery support.
>>>
>>> Signed-off-by: Thomas Abraham <thomas.abraham@xxxxxxxxxx>
>>> ---
>>>  .../devicetree/bindings/mmc/synposis-dw-mshc.txt   |   85 +++++++++
>>>  drivers/mmc/host/dw_mmc-pltfm.c                    |   24 +++
>>>  drivers/mmc/host/dw_mmc.c                          |  181
>>> +++++++++++++++++++-
>>>  drivers/mmc/host/dw_mmc.h                          |   10 +
>>>  include/linux/mmc/dw_mmc.h                         |    2 +
>>>  5 files changed, 296 insertions(+), 6 deletions(-)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>>> b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>>> new file mode 100644
>>> index 0000000..c1ed70e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>>> @@ -0,0 +1,85 @@
>>> +* Synopsis Designware Mobile Storage Host Controller
>>> +
>>> +The Synopsis designware mobile storage host controller is used to
>>> interface
>>> +a SoC with storage medium such as eMMC or SD/MMC cards.
>>> +
>>> +Required Properties:
>>> +
>>> +* compatible: should be one of the following
>>> +     - synopsis,dw-mshc: for controllers compliant with synopsis
>>> dw-mshc.
>> I googled the "Synopsis Designware Mobile Storage Host Controller" and
>> Synopsis released it as this name. but still I like the 'dw-mmc'
>> instead of'dw-mshc'.
>
> Ok. Synopsis abbreviates this controller as MSHC in their datasheets
> available online. Since device tree is more about describing the
> hardware, using MSHC as the abbreviation will help with correlating
> hardware specs with the dts file. So I would prefer to continue using
> mshc as the abbreviation.

Then why author of this file uses "dw-mmc" instead of "dw-mshc"? and
it uses 'dw_mci' prefix at functions. I just worried and don't want to
give confusion to users who uses this device.

>
>>> +
>>> +* reg: physical base address of the dw-mshc controller and size of its
>>> memory
>>> +  region.
>>> +
>>> +* interrupts: interrupt specifier for the controller. The format and
>>> value
>>> of
>>> +  the interrupt specifier depends on the interrupt parent for the
>>> controller.
>>> +
>>> +# Slots: The slot specific information are contained within child-nodes
>>> with
>>> +  each child-node representing a supported slot. There should be
>>> atleast
>>> one
>>> +  child node representing a card slot. The name of the slot child node
>>> should
>>> +  be 'slot{n}' where n is the unique number of the slot connnected to
>>> the
>>> +  controller. The following are optional properties which can be
>>> included
>>> in
>>> +  the slot child node.
>>> +
>>> +     * bus-width: specifies the width of the data bus connected from
>>> the
>>> +       controller to the card slot. The value should be 1, 4 or 8. In
>>> case
>>> +       this property is not specified, a default value of 1 is assumed
>>> for
>>> +       this property.
>>> +
>>> +     * cd-gpios: specifies the card detect gpio line. The format of the
>>> +       gpio specifier depends on the gpio controller.
>>> +
>>> +     * wp-gpios: specifies the write protect gpio line. The format of
>>> the
>>> +       gpio specifier depends on the gpio controller.
>>> +
>>> +     * gpios: specifies a list of gpios used for command, clock and
>>> data
>>> +       bus. The first gpio is the command line and the second gpio is
>>> the
>>> +       clock line. The rest of the gpios (depending on the bus-width
>>> +       property) are the data lines in no particular order. The format
>>> of
>>> +       the gpio specifier depends on the gpio controller.
>>> +
>>> +Optional properties:
>>> +
>>> +* fifo-depth: The maximum size of the tx/rx fifo's. If this property is
>>> not
>>> +  specified, the default value of the fifo size is determined from the
>>> +  controller registers.
>>> +
>>> +*  card-detect-delay: Delay in milli-seconds before detecting card
>>> after
>>> card
>>> +   insert event. The default value is 0.
>>> +
>>> +* supports-highspeed: Enables support for high speed cards (upto 50MHz)
>>> +
>>> +* card-detection-broken: The card detection functionality is not
>>> available
>>> on
>>> +  any of the slots.
>>> +
>>> +* no-write-protect: The write protect pad of the controller is not
>>> connected
>>> +  to the write protect pin on the slot.
>>> +
>>> +Example:
>>> +
>>> +  The MSHC controller node can be split into two portions, SoC specific
>>> and
>>> +  board specific portions as listed below.
>>> +
>>> +     dwmmc0@12200000 {
>>> +             compatible = "synopsis,dw-mshc";
>>> +             reg = <0x12200000 0x1000>;
>>> +             interrupts = <0 75 0>;
>>> +     };
>>> +
>>> +     dwmmc0@12200000 {
>>> +             supports-highspeed;
>>> +             card-detection-broken;
>>> +             no-write-protect;
>>> +             fifo-depth = <0x80>;
>>> +             card-detect-delay = <200>;
>>> +
>>> +             slot0 {
>>> +                     bus-width = <8>;
>>> +                     cd-gpios = <&gpc0 2 2 3 3>;
>>> +                     gpios = <&gpc0 0 2 0 3>, <&gpc0 1 2 0 3>,
>>> +                             <&gpc1 0 2 3 3>, <&gpc1 1 2 3 3>,
>>> +                             <&gpc1 2 2 3 3>, <&gpc1 3 2 3 3>,
>>> +                             <&gpc0 3 2 3 3>, <&gpc0 4 2 3 3>,
>>> +                             <&gpc0 5 2 3 3>, <&gpc0 6 2 3 3>;
>>> +             };
>>> +     };
>>> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c
>>> b/drivers/mmc/host/dw_mmc-pltfm.c
>>> index 92ec3eb..2b2c9bd 100644
>>> --- a/drivers/mmc/host/dw_mmc-pltfm.c
>>> +++ b/drivers/mmc/host/dw_mmc-pltfm.c
>>> @@ -19,8 +19,24 @@
>>>  #include <linux/mmc/host.h>
>>>  #include <linux/mmc/mmc.h>
>>>  #include <linux/mmc/dw_mmc.h>
>>> +#include <linux/of.h>
>>>  #include "dw_mmc.h"
>>>
>>> +#ifdef CONFIG_OF
>>> +static struct dw_mci_drv_data synopsis_drv_data = {
>>> +     .ctrl_type      = DW_MCI_TYPE_SYNOPSIS,
>>> +};
>>> +
>>> +static const struct of_device_id dw_mci_pltfm_match[] = {
>>> +     { .compatible = "synopsis,dw-mshc",
>>> +                     .data = (void *)&synopsis_drv_data, },
>>> +     {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, dw_mci_pltfm_match);
>>> +#else
>>> +static const struct of_device_id dw_mci_pltfm_match[];
>>> +#endif
>>> +
>>>  static int dw_mci_pltfm_probe(struct platform_device *pdev)
>>>  {
>>>       struct dw_mci *host;
>>> @@ -51,6 +67,13 @@ static int dw_mci_pltfm_probe(struct platform_device
>>> *pdev)
>>>       if (!host->regs)
>>>               goto err_free;
>>>       platform_set_drvdata(pdev, host);
>>> +
>>> +     if (pdev->dev.of_node) {
>>> +             const struct of_device_id *match;
>>> +             match = of_match_node(dw_mci_pltfm_match,
>>> pdev->dev.of_node);
>>> +             host->drv_data = match->data;
>>> +     }
>>> +
>>>       ret = dw_mci_probe(host);
>>>       if (ret)
>>>               goto err_out;
>>> @@ -111,6 +134,7 @@ static struct platform_driver dw_mci_pltfm_driver =
>>> {
>>>       .remove         = __exit_p(dw_mci_pltfm_remove),
>>>       .driver         = {
>>>               .name           = "dw_mmc",
>>> +             .of_match_table = of_match_ptr(dw_mci_pltfm_match),
>>>               .pm             = &dw_mci_pltfm_pmops,
>>>       },
>>>  };
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index 38cb7f8..bcf66d7 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -33,9 +33,13 @@
>>>  #include <linux/bitops.h>
>>>  #include <linux/regulator/consumer.h>
>>>  #include <linux/workqueue.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_gpio.h>
>>>
>>>  #include "dw_mmc.h"
>>>
>>> +#define NUM_PINS(x) (x + 2)
>>> +
>>>  /* Common flag combinations */
>>>  #define DW_MCI_DATA_ERROR_FLAGS      (SDMMC_INT_DTO | SDMMC_INT_DCRC |
>>> \
>>>                                SDMMC_INT_HTO | SDMMC_INT_SBE  | \
>>> @@ -86,6 +90,8 @@ struct idmac_desc {
>>>  struct dw_mci_slot {
>>>       struct mmc_host         *mmc;
>>>       struct dw_mci           *host;
>>> +     int                     wp_gpio;
>>> +     int                     cd_gpio;
>>>
>>>       u32                     ctype;
>>>
>>> @@ -816,6 +822,8 @@ static int dw_mci_get_ro(struct mmc_host *mmc)
>>>               read_only = 0;
>>>       else if (brd->get_ro)
>>>               read_only = brd->get_ro(slot->id);
>>> +     else if (gpio_is_valid(slot->wp_gpio))
>>> +             read_only = gpio_get_value(slot->wp_gpio);
>>>       else
>>>               read_only =
>>>                       mci_readl(slot->host, WRTPRT) & (1 << slot->id) ? 1
>>> : 0;
>>> @@ -837,6 +845,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>>>               present = 1;
>>>       else if (brd->get_cd)
>>>               present = !brd->get_cd(slot->id);
>>> +     else if (gpio_is_valid(slot->cd_gpio))
>>> +             present = gpio_get_value(slot->cd_gpio);
>>>       else
>>>               present = (mci_readl(slot->host, CDETECT) & (1 <<
>>> slot->id))
>>>                       == 0 ? 1 : 0;
>>> @@ -1747,6 +1757,96 @@ static void dw_mci_work_routine_card(struct
>>> work_struct *work)
>>>       }
>>>  }
>>>
>>> +#ifdef CONFIG_OF
>>> +static struct device_node *dw_mci_of_find_slot_node(struct device *dev,
>>> u8
>>> slot)
>>> +{
>>> +     struct device_node *np;
>>> +     char name[7];
>>> +
>>> +     if (!dev || !dev->of_node)
>>> +             return NULL;
>>> +
>>> +     for_each_child_of_node(dev->of_node, np) {
>>> +             sprintf(name, "slot%d", slot);
>>> +             if (!strcmp(name, np->name))
>>> +                     return np;
>>> +     }
>>> +     return NULL;
>>> +}
>>> +
>>> +static u32 dw_mci_of_get_bus_wd(struct device *dev, u8 slot)
>>> +{
>>> +     struct device_node *np = dw_mci_of_find_slot_node(dev, slot);
>>> +     u32 bus_wd = 1;
>>> +
>>> +     if (!np)
>>> +             return 1;
>>> +
>>> +     if (of_property_read_u32(np, "bus-width", &bus_wd))
>>> +             dev_err(dev, "bus-width property not found, assuming
>>> width"
>>> +                             " as 1\n");
>>> +     return bus_wd;
>>> +}
>>> +
>>> +static int dw_mci_of_setup_bus(struct dw_mci *host, u8 slot, u32
>>> bus_wd)
>>> +{
>>> +     struct device_node *np = dw_mci_of_find_slot_node(&host->dev,
>>> slot);
>>> +     int idx, gpio, ret;
>>> +
>>> +     for (idx = 0; idx < NUM_PINS(bus_wd); idx++) {
>>> +             gpio = of_get_gpio(np, idx);
>>> +             if (!gpio_is_valid(gpio)) {
>>> +                     dev_err(&host->dev, "invalid gpio: %d\n", gpio);
>>> +                     return -EINVAL;
>>> +             }
>>> +
>>> +             ret = devm_gpio_request(&host->dev, gpio, "dw-mci-bus");
>>> +             if (ret) {
>>> +                     dev_err(&host->dev, "gpio [%d] request failed\n",
>>> gpio);
>>> +                     return -EBUSY;
>>> +             }
>>> +     }
>>> +
>>> +     host->slot[slot]->wp_gpio = -1;
>>> +     gpio = of_get_named_gpio(np, "wp_gpios", 0);
>>> +     if (!gpio_is_valid(gpio)) {
>>> +             dev_info(&host->dev, "wp gpio not available");
>>> +     } else {
>>> +             ret = devm_gpio_request(&host->dev, gpio, "dw-mci-wp");
>>> +             if (ret)
>>> +                     dev_info(&host->dev, "gpio [%d] request failed\n",
>>> +                                             gpio);
>>> +             else
>>> +                     host->slot[slot]->wp_gpio = gpio;
>>> +     }
>>> +
>>> +     host->slot[slot]->cd_gpio = -1;
>>> +     gpio = of_get_named_gpio(np, "cd-gpios", 0);
>>> +     if (!gpio_is_valid(gpio)) {
>>> +             dev_info(&host->dev, "cd gpio not available");
>>> +     } else {
>>> +             ret = devm_gpio_request(&host->dev, gpio, "dw-mci-cd");
>>> +             if (ret)
>>> +                     dev_err(&host->dev, "gpio [%d] request failed\n",
>>> gpio);
>>> +             else
>>> +                     host->slot[slot]->cd_gpio = gpio;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +#else /* CONFIG_OF */
>>> +static u32 dw_mci_of_get_bus_wd(struct device *dev, u8 slot)
>>> +{
>>> +     return 1;
>>> +}
>>> +
>>> +static int dw_mci_of_setup_bus(struct dw_mci *host, u8 slot, u32
>>> bus_wd)
>>> +{
>>> +     return -EINVAL;
>>> +}
>>> +#endif /* CONFIG_OF */
>>> +
>>>  static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int
>>> id)
>>>  {
>>>       struct mmc_host *mmc;
>>> @@ -1760,6 +1860,7 @@ static int __init dw_mci_init_slot(struct dw_mci
>>> *host, unsigned int id)
>>>       slot->id = id;
>>>       slot->mmc = mmc;
>>>       slot->host = host;
>>> +     host->slot[id] = slot;
>>>
>>>       mmc->ops = &dw_mci_ops;
>>>       mmc->f_min = DIV_ROUND_UP(host->bus_hz, 510);
>>> @@ -1780,12 +1881,21 @@ static int __init dw_mci_init_slot(struct dw_mci
>>> *host, unsigned int id)
>>>       if (host->pdata->caps)
>>>               mmc->caps = host->pdata->caps;
>>>
>>> +     mmc->caps |= host->drv_data->caps;
>>> +
>>>       if (host->pdata->caps2)
>>>               mmc->caps2 = host->pdata->caps2;
>>>
>>> -     if (host->pdata->get_bus_wd)
>>> +     if (host->pdata->get_bus_wd) {
>>>               if (host->pdata->get_bus_wd(slot->id) >= 4)
>>>                       mmc->caps |= MMC_CAP_4_BIT_DATA;
>>> +     } else if (host->dev.of_node) {
>>> +             unsigned int bus_width;
>>> +             bus_width = dw_mci_of_get_bus_wd(&host->dev, slot->id);
>>> +             if (bus_width >= 4)
>>> +                     mmc->caps |= MMC_CAP_4_BIT_DATA;
>> In case of bus_width has 8; then how to handle it? maybe it's missing
>> one.
>
>
> Yes, this was not correct. I will fix this. Thanks for pointing this out.
>
>
>>> +             dw_mci_of_setup_bus(host, slot->id, bus_width);
>>> +     }
>>>
>>>       if (host->pdata->quirks & DW_MCI_QUIRK_HIGHSPEED)
>>>               mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
>>> @@ -1830,7 +1940,6 @@ static int __init dw_mci_init_slot(struct dw_mci
>>> *host, unsigned int id)
>>>       else
>>>               clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
>>>
>>> -     host->slot[id] = slot;
>>>       mmc_add_host(mmc);
>>>
>>>  #if defined(CONFIG_DEBUG_FS)
>>> @@ -1923,15 +2032,75 @@ static bool mci_wait_reset(struct device *dev,
>>> struct dw_mci *host)
>>>       return false;
>>>  }
>>>
>>> +#ifdef CONFIG_OF
>>> +static struct dw_mci_of_quirks {
>>> +     char *quirk;
>>> +     int id;
>>> +} of_quriks[] = {
>>> +     {
>>> +             .quirk  = "supports-highspeed",
>>> +             .id     = DW_MCI_QUIRK_HIGHSPEED,
>>> +     }, {
>>> +             .quirk  = "card-detection-broken",
>>> +             .id     = DW_MCI_QUIRK_BROKEN_CARD_DETECTION,
>>> +     }, {
>>> +             .quirk  = "no-write-protect",
>>> +             .id     = DW_MCI_QUIRK_NO_WRITE_PROTECT,
>>> +     },
>>> +     { }
>>> +};
>>> +
>>> +static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>>> +{
>>> +     struct dw_mci_board *pdata;
>>> +     struct device *dev = &host->dev;
>>> +     struct device_node *np = dev->of_node, *slot;
>>> +     u32 timing[3];
>>> +     int idx, cnt;
>>> +
>>> +     pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>>> +     if (!pdata) {
>>> +             dev_err(dev, "could not allocate memory for pdata\n");
>>> +             return ERR_PTR(-ENOMEM);
>>> +     }
>>> +
>>> +     /* find out number of slots supported */
>>> +     for_each_child_of_node(np, slot)
>>> +             pdata->num_slots++;
>>> +
>>> +     /* get quirks */
>>> +     cnt = sizeof(of_quriks) / sizeof(struct dw_mci_of_quirks);
>>> +     for (idx = 0; idx < cnt; idx++)
>>> +             if (of_get_property(np, of_quriks[idx].quirk, NULL))
>>> +                     pdata->quirks |= of_quriks[idx].id;
>>> +
>>> +     if (of_property_read_u32(np, "fifo-depth", &pdata->fifo_depth))
>>> +             dev_info(dev, "fifo-depth property not found, using "
>>> +                             "value of FIFOTH register as default\n");
>>> +
>>> +     of_property_read_u32(np, "card-detect-delay",
>>> &pdata->detect_delay_ms);
>>> +
>>> +     return pdata;
>>> +}
>>> +
>>> +#else /* CONFIG_OF */
>>> +static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>>> +{
>>> +     return ERR_PTR(-EINVAL);
>>> +}
>>> +#endif /* CONFIG_OF */
>>> +
>>>  int dw_mci_probe(struct dw_mci *host)
>>>  {
>>>       int width, i, ret = 0;
>>>       u32 fifo_size;
>>>
>>> -     if (!host->pdata || !host->pdata->init) {
>>> -             dev_err(&host->dev,
>>> -                     "Platform data must supply init function\n");
>>> -             return -ENODEV;
>>> +     if (!host->pdata) {
>>> +             host->pdata = dw_mci_parse_dt(host);
>>> +             if (IS_ERR(host->pdata)) {
>>> +                     dev_err(&host->dev, "platform data not
>>> available\n");
>>> +                     return -EINVAL;
>>> +             }
>>>       }
>>>
>>>       if (!host->pdata->select_slot && host->pdata->num_slots > 1) {
>>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>>> index 15c27e1..8b8862b 100644
>>> --- a/drivers/mmc/host/dw_mmc.h
>>> +++ b/drivers/mmc/host/dw_mmc.h
>>> @@ -182,4 +182,14 @@ extern int dw_mci_suspend(struct dw_mci *host);
>>>  extern int dw_mci_resume(struct dw_mci *host);
>>>  #endif
>>>
>>> +/* Variations in the dw_mci controller */
>>> +#define DW_MCI_TYPE_SYNOPSIS         0
>>> +#define DW_MCI_TYPE_EXYNOS5250               1 /* Samsung Exynos5250
>>> Extensions */
>> Um. it's not good idea to add specific SOC version. And as you know,
>> exynos4 series has this controller.
>
> There are some differences between the Exynos4 and Exynos5 mshc
> controllers. For instance, the DIVRATIO field in the CLKSEL register
> is available only in Exynos5 and there are 8 phase shift values in
> Exynos5 when compared to 4 phase shift values in Exynos4. Likewise,
> there are some other differences. So to handle these specific
> implementations, we need to define types (or variants) of the
> controller. Using SoC names for the type would help in readability of
> the different types of implementations that are defined. So I would
> prefer to continue using SoC names for this.

You mean if it supports the exynos4, then it should be add exynos4 type?

Thank you,
Kyungmin Park
>
> Thanks,
> Thomas.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/