[2/2] USB: ehci-tegra: add probing through device tree

Rely on platform_data being passed through auxdata for now; more elaborate
bindings for phy config and tunings to be added.

Signed-off-by: Olof Johansson <olof@lixom.net>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 arch/arm/mach-tegra/board-dt.c |    8 +++++
 drivers/usb/host/ehci-tegra.c  |   62 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 0 deletions(-)

Olof Johansson wrote at Thursday, November 03, 2011 9:26 PM:
> Rely on platform_data being passed through auxdata for now; more elaborate
> bindings for phy config and tunings to be added.
...
> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
...
>  static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = {
>  	/* name		parent		rate		enabled */
>  	{ "uartd",	"pll_p",	216000000,	true },
> +	{ "usbd",	"clk_m",	12000000,	false },
> +	{ "usb3",	"clk_m",	12000000,	false },
>  	{ NULL,		NULL,		0,		0},
>  };

As woglinde mentioned on IRC, I think you do want to add "usb2" to the
table here; it's in board-paz00.c at least. It shouldn't hurt other boards.

> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
...
> @@ -590,6 +617,13 @@ static int tegra_ehci_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
> 
> +	if (!pdev->dev.dma_mask)
> +		pdev->dev.dma_mask = &tegra_ehci_dma_mask;

Should this come from DT, or is it some more system-level/internal thing
that doesn't make sense to represent there?

> +
> +	vbus_gpio = of_get_named_gpio(pdev->dev.of_node, "nvidia,vbus-gpio", 0);

of_get_named_gpio() does check for NULL node pointer in practice, but is
it defined to? I wonder if this needs to be conditional of of_node!=NULL
or not?

> +	if (vbus_gpio > 0)

Should that use gpio_is_valid()?

> +		setup_vbus_gpio(pdev, vbus_gpio);

On Fri, Nov 4, 2011 at 8:37 AM, Stephen Warren <swarren@nvidia.com> wrote:
> Olof Johansson wrote at Thursday, November 03, 2011 9:26 PM:
>> Rely on platform_data being passed through auxdata for now; more elaborate
>> bindings for phy config and tunings to be added.
> ...
>> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
> ...
>>  static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = {
>>       /* name         parent          rate            enabled */
>>       { "uartd",      "pll_p",        216000000,      true },
>> +     { "usbd",       "clk_m",        12000000,       false },
>> +     { "usb3",       "clk_m",        12000000,       false },
>>       { NULL,         NULL,           0,              0},
>>  };
>
> As woglinde mentioned on IRC, I think you do want to add "usb2" to the
> table here; it's in board-paz00.c at least. It shouldn't hurt other boards.

Yeah, I didn't catch it before posting this.

It might make sense to just move them to common.c then, especially
since the clocks aren't enabled in the table. But I'll add usb2 for
now, and we can move them later if it makes sense.

>
>> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> ...
>> @@ -590,6 +617,13 @@ static int tegra_ehci_probe(struct platform_device *pdev)
>>               return -EINVAL;
>>       }
>>
>> +     if (!pdev->dev.dma_mask)
>> +             pdev->dev.dma_mask = &tegra_ehci_dma_mask;
>
> Should this come from DT, or is it some more system-level/internal thing
> that doesn't make sense to represent there?

Ideally it should come from the device tree based on dma window
information, but there's currently no way to encode in there what the
dma address limitations of a device is, or at least nothing that is
used generically -- IBM has some custom extensions to encode what part
of the dma address space is mapped for a specific device through the
iommu.

The auxdata function has a comment saying that it is intentionally not
setting up the dma mapping functions and that it should be handled
through a system notifier instead (but it does set the
coherent_dma_mask). As a stepping stone on getting that all sorted out
I just did the simple solution above. I'll add a comment saying it
should go away.

>> +
>> +     vbus_gpio = of_get_named_gpio(pdev->dev.of_node, "nvidia,vbus-gpio", 0);
>
> of_get_named_gpio() does check for NULL node pointer in practice, but is
> it defined to? I wonder if this needs to be conditional of of_node!=NULL
> or not?

Sure, I can wrap it with a test of of_node.

>> +     if (vbus_gpio > 0)
>
> Should that use gpio_is_valid()?

Yep, fixing.



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