[PATCH 1/3 v2] usb: tegra20-ehci: Add devicetree support.

Grant Likely grant.likely at secretlab.ca
Thu Jul 21 06:02:11 EST 2011


On Wed, Jul 20, 2011 at 1:38 PM,  <achew at nvidia.com> wrote:
> From: Andrew Chew <achew at nvidia.com>
>
> Add code to try to get platform data information (register base, irq,
> modes, various tuning parameters) from device tree, if not present in board
> files.
>
> Signed-off-by: Andrew Chew <achew at nvidia.com>
> ---
>  .../devicetree/bindings/usb/tegra20-ehci.txt       |   28 +++
>  drivers/usb/host/ehci-tegra.c                      |  221 ++++++++++++++++++++
>  2 files changed, 249 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/tegra20-ehci.txt
>
> diff --git a/Documentation/devicetree/bindings/usb/tegra20-ehci.txt b/Documentation/devicetree/bindings/usb/tegra20-ehci.txt
> new file mode 100644
> index 0000000..5a73a96
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/tegra20-ehci.txt
> @@ -0,0 +1,28 @@
> +NVIDIA Tegra20 SOC USB controllers
> +
> +The device node for a USB controller that is part of a Tegra20
> +SOC is as described in the document "Open Firmware Recommended
> +Practice: Universal Serial Bus" with the following modifications
> +and additions:
> +
> +Required properties:
> + - compatible: Should be "nvidia,tegra20-ehci".
> + - mode: Should be one of "device", "host", or "otg".
> + - power_down_on_bus_suspend: For host mode only, should be <1> if you
> +   want the USB phy to power down when the host is suspended.  Else, it
> +   should be <0>.

We use '-' in property names, not '_'

Boolean values like this should be based on whether or not the
property is present.  ie. default to not powering down the phy unless
an empty "power-down-on-suspend" property is present.

> + - type: Should be one of "utmi" or "ulpi".

phy-type perhaps?

> +
> +Required properties for type = "utmi".  These values are derived from
> +characterization by system engineering.
> + - hssync_start_delay
> + - idle_wait_delay
> + - elastic_limit
> + - term_range_adj
> + - xcvr_setup
> + - xcvr_lsfslew
> + - xcvr_lsrslew

All of these custom properties should be prefixed with "nvidia," to
avoid namespace collisions.

> +
> +Required properties for type = "ulpi":
> + - reset_gpio: The GPIO used to drive reset
> + - clk
> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index 02b2bfd..89144d5 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -21,10 +21,20 @@
>  #include <linux/platform_data/tegra_usb.h>
>  #include <linux/irq.h>
>  #include <linux/usb/otg.h>
> +
> +#if defined(CONFIG_OF)
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#endif
> +

linux/of* headers are always safe to include.  Don't separate them
from the rest of the #includes.

>  #include <mach/usb_phy.h>
>
>  #define TEGRA_USB_DMA_ALIGN 32
>
> +static u64 tegra_ehci_dmamask = DMA_BIT_MASK(TEGRA_USB_DMA_ALIGN);
> +
>  struct tegra_ehci_hcd {
>        struct ehci_hcd *ehci;
>        struct tegra_usb_phy *phy;
> @@ -574,9 +584,181 @@ static const struct hc_driver tegra_ehci_hc_driver = {
>        .port_handed_over       = ehci_port_handed_over,
>  };
>
> +#if defined(CONFIG_OF)
> +static int tegra_ehci_parse_dt_node_utmi(struct device_node *dn,
> +                                        struct platform_device *pdev,
> +                                        struct tegra_ehci_platform_data *pdata)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct tegra_utmip_config *phy_config;
> +       int retval;
> +
> +       phy_config = devm_kzalloc(dev, sizeof(struct tegra_utmip_config),
> +                                 GFP_KERNEL);
> +       if (!phy_config)
> +               return -ENOMEM;
> +
> +       retval = of_property_read_u32(dn, "hssync_start_delay",
> +                                     (u32 *)&phy_config->hssync_start_delay);

Gah!  Don't cast pointers that way.  It will break badly if the sizes
don't match.  The variable *must* be a u32.

> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"hssync_start_delay\"\n");
> +               return retval;
> +       }
> +
> +       retval = of_property_read_u32(dn, "elastic_limit",
> +                                     (u32 *)&phy_config->elastic_limit);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"elastic_limit\"\n");
> +               return retval;
> +       }
> +
> +       retval = of_property_read_u32(dn, "idle_wait_delay",
> +                                     (u32 *)&phy_config->idle_wait_delay);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"idle_wait_delay\"\n");
> +               return retval;
> +       }
> +
> +       retval = of_property_read_u32(dn, "term_range_adj",
> +                                     (u32 *)&phy_config->term_range_adj);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"term_range_adj\"\n");
> +               return retval;
> +       }
> +
> +       retval = of_property_read_u32(dn, "xcvr_setup",
> +                                     (u32 *)&phy_config->xcvr_setup);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"xcvr_setup\"\n");
> +               return retval;
> +       }
> +
> +       retval = of_property_read_u32(dn, "xcvr_lsfslew",
> +                                     (u32 *)&phy_config->xcvr_lsfslew);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"xcvr_lsfslew\"\n");
> +               return retval;
> +       }
> +
> +       retval = of_property_read_u32(dn, "xcvr_lsrslew",
> +                                     (u32 *)&phy_config->xcvr_lsrslew);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"xcvr_lsrslew\"\n");
> +               return retval;
> +       }

Can the driver use sane defaults for any of these values?  This patch
will be a lot smaller if there isn't the need to check all the return
values each time.

> +
> +       pdata->phy_config = phy_config;
> +
> +       return 0;
> +}
> +
> +static int tegra_ehci_parse_dt_node_ulpi(struct device_node *dn,
> +                                        struct platform_device *pdev,
> +                                        struct tegra_ehci_platform_data *pdata)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct tegra_ulpi_config *phy_config;
> +       int retval;
> +
> +       phy_config = devm_kzalloc(dev, sizeof(struct tegra_ulpi_config),
> +                                 GFP_KERNEL);
> +       if (!phy_config)
> +               return -ENOMEM;
> +
> +       retval = of_property_read_u32(dn, "reset_gpio",
> +                                     &phy_config->reset_gpio);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"reset_gpio\"\n");
> +               return retval;
> +       }
> +
> +       retval = of_property_read_string(dn, "clk",
> +                                        (char **)&phy_config->clk);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"clk\"\n");
> +               return retval;
> +       }
> +
> +       pdata->phy_config = phy_config;
> +
> +       return 0;
> +}
> +
> +static struct tegra_ehci_platform_data *
> +tegra_ehci_parse_dt_node(struct device_node *dn,
> +                        struct platform_device *pdev)

Nit: to be friendly for grepping, function name should remain on same
line as return value and annotation.  Keep line breaks in the
parameter list.

> +{
> +       struct device *dev = &pdev->dev;
> +       struct tegra_ehci_platform_data *pdata;
> +       char *mode;
> +       char *type;
> +       int retval;
> +
> +       pdata = devm_kzalloc(dev, sizeof(struct tegra_ehci_platform_data),
> +                            GFP_KERNEL);
> +       if (!pdata)
> +               return NULL;
> +
> +       retval = of_property_read_string(dn, "mode", &mode);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"mode\"\n");
> +               goto fail;
> +       }
> +
> +       if (strcmp(mode, "device") == 0)
> +               pdata->operating_mode = TEGRA_USB_DEVICE;
> +       else if (strcmp(mode, "host") == 0)
> +               pdata->operating_mode = TEGRA_USB_HOST;
> +       else if (strcmp(mode, "otg") == 0)
> +               pdata->operating_mode = TEGRA_USB_OTG;
> +       else {
> +               dev_err(dev, "Invalid dt property \"mode\" value %s\n", mode);
> +               goto fail;
> +       }
> +
> +       retval = of_property_read_u32(dn, "power_down_on_bus_suspend",
> +                                     &pdata->power_down_on_bus_suspend);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property "
> +                            "\"power_down_on_bus_suspend\"\n");
> +               goto fail;
> +       }
> +
> +       retval = of_property_read_string(dn, "type", &type);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"type\"\n");
> +               goto fail;
> +       }
> +
> +       if (strcmp(type, "utmi") == 0) {
> +               retval = tegra_ehci_parse_dt_node_utmi(dn, pdev, pdata);
> +               if (retval)
> +                       goto fail;
> +       } else if (strcmp(type, "ulpi") == 0) {
> +               retval = tegra_ehci_parse_dt_node_ulpi(dn, pdev, pdata);
> +               if (retval)
> +                       goto fail;
> +       } else {
> +               dev_err(dev, "Invalid dt property \"type\" value %s\n", type);
> +               goto fail;
> +       }
> +
> +       return pdata;
> +
> +fail:
> +       devm_kfree(dev, pdata);
> +
> +       return NULL;
> +}
> +#endif
> +
>  static int tegra_ehci_probe(struct platform_device *pdev)
>  {
>        struct resource *res;
> +#if defined(CONFIG_OF)
> +       struct device_node *dn = pdev->dev.of_node;
> +       struct resource of_res;
> +#endif
>        struct usb_hcd *hcd;
>        struct tegra_ehci_hcd *tegra;
>        struct tegra_ehci_platform_data *pdata;
> @@ -584,7 +766,20 @@ static int tegra_ehci_probe(struct platform_device *pdev)
>        int irq;
>        int instance = pdev->id;
>
> +       /*
> +        * See if there's any platform data passed via board files.
> +        * If there isn't, then allocate one and fill it by parsing
> +        * device tree node.
> +        */
>        pdata = pdev->dev.platform_data;
> +#if defined(CONFIG_OF)
> +       if (!pdata) {
> +               pdata = tegra_ehci_parse_dt_node(dn, pdev);
> +
> +               pdev->dev.dma_mask = &tegra_ehci_dmamask;
> +               pdev->dev.coherent_dma_mask = tegra_ehci_dmamask;
> +       }
> +#endif

Drop the #if/#endif protection, and create an empty version of
tegra_ehci_parse_dt_node() when CONFIG_OF is not selected.  It will
make the code cleaner.

>        if (!pdata) {
>                dev_err(&pdev->dev, "Platform data missing\n");
>                return -EINVAL;
> @@ -625,7 +820,15 @@ static int tegra_ehci_probe(struct platform_device *pdev)
>        clk_enable(tegra->emc_clk);
>        clk_set_rate(tegra->emc_clk, 400000000);
>
> +       /*
> +        * If there isn't an IORESOURCE_MEM defined in the board file,
> +        * then try to get that information from the device tree node.
> +        */
>        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +#if defined(CONFIG_OF)
> +       if (!res && (of_address_to_resource(dn, 0, &of_res) == 0))
> +               res = &of_res;
> +#endif

Unnecessary.  The register ranges are populated into the resource
table by default.

>        if (!res) {
>                dev_err(&pdev->dev, "Failed to get I/O memory\n");
>                err = -ENXIO;
> @@ -658,7 +861,15 @@ static int tegra_ehci_probe(struct platform_device *pdev)
>        tegra->power_down_on_bus_suspend = pdata->power_down_on_bus_suspend;
>        tegra->ehci = hcd_to_ehci(hcd);
>
> +       /*
> +        * If there isn't an irq defined in the board file, then try to get
> +        * that information from the device tree node.
> +        */
>        irq = platform_get_irq(pdev, 0);
> +#if defined(CONFIG_OF)
> +       if (!irq)
> +               irq = irq_of_parse_and_map(dn, 0);
> +#endif

Same here; unnecessary.

>        if (!irq) {
>                dev_err(&pdev->dev, "Failed to get IRQ\n");
>                err = -ENODEV;
> @@ -773,6 +984,14 @@ static void tegra_ehci_hcd_shutdown(struct platform_device *pdev)
>                hcd->driver->shutdown(hcd);
>  }
>
> +static const struct of_device_id tegra_ehci_of_match[] = {
> +       {
> +               .compatible = "nvidia,tegra20-ehci",
> +       },

The following form is more concise and preferred for match tables:

        { .compatible = "nvidia,tegra20-ehci", },
        {},

> +};
> +MODULE_DEVICE_TABLE(of, tegra_ehci_of_match);
> +
>  static struct platform_driver tegra_ehci_driver = {
>        .probe          = tegra_ehci_probe,
>        .remove         = tegra_ehci_remove,
> @@ -783,5 +1002,7 @@ static struct platform_driver tegra_ehci_driver = {
>        .shutdown       = tegra_ehci_hcd_shutdown,
>        .driver         = {
>                .name   = "tegra-ehci",
> +               .owner  = THIS_MODULE,
> +               .of_match_table = tegra_ehci_of_match,
>        }
>  };
> --
> 1.7.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.


More information about the devicetree-discuss mailing list