On 11/23/2011 08:54 PM, Simon Glass wrote:
This adds basic support for the Tegra2 USB controller. Board files should call board_usb_init() to set things up.
Just a very brief review:
+/* Put the port into host mode (this only works for USB1) */ +static void set_host_mode(struct usb_ctlr *usbctlr) +{
/* Check whether remote host from USB1 is driving VBus */if (readl(&usbctlr->phy_vbus_sensors) & VBUS_VLD_STS)return;/** If not driving, we set GPIO USB1_VBus_En. Seaboard platform uses* PAD SLXK (GPIO D.00) as USB1_VBus_En Config as GPIO*/gpio_direction_output(GPIO_PD0, 1);
The GPIO to use here needs to be parameterized; there's no reason to believe it'll be the same on all boards (or even that boards will have any such GPIO).
+/* Put our ports into host mode */ +void usb_set_host_mode(void) +{
if (host_dev_ctlr)set_host_mode(host_dev_ctlr);+}
Why not just call set_host_mode() directly from board_usb_init()?
+#ifndef CONFIG_OF_CONTROL +static int probe_port(struct usb_ctlr *usbctlr, const int params[]) +{
enum periph_id id;int utmi = 0;/** Get the periph id. Port 1 has an internal transceiver, port 3 is* external*/switch ((u32)usbctlr) {case TEGRA_USB1_BASE:id = PERIPH_ID_USBD;break;case TEGRA_USB3_BASE:id = PERIPH_ID_USB3;utmi = 1;break;default:printf("tegrausb: probe_port: no such port %p\n", usbctlr);return -1;}
What about the other port (USB2)?
+#ifdef CONFIG_OF_CONTROL +int fdt_decode_usb(const void *blob, int node, unsigned osc_frequency_mhz,
struct fdt_usb *config)+{
int clk_node = 0, rate;/* Find the parameters for our oscillator frequency */do {clk_node = fdt_node_offset_by_compatible(blob, clk_node,"nvidia,tegra20-usbparams");if (clk_node < 0) {debug("Cannot find USB params for clock %u",osc_frequency_mhz);return -FDT_ERR_NOTFOUND;}rate = fdtdec_get_int(blob, clk_node, "osc-frequency", 0);} while (rate != osc_frequency_mhz);config->reg = (struct usb_ctlr *)fdtdec_get_addr(blob, node, "reg");config->host_mode = fdtdec_get_bool(blob, node, "support-host-mode");
Property "support-host-mode" isn't something that's supported by the kernel binding, and needs discussion/ack there.
config->utmi = fdtdec_lookup_phandle(blob, node, "utmi") >= 0;
In the kernel DT binding, this is property "phy_type" with legal values "utmi" or "ulpi."
config->enabled = fdtdec_get_is_enabled(blob, node);config->periph_id = fdtdec_get_int(blob, node, "periph-id", -1);
periph-id is a U-Boot specific concept, not HW description. The DT shouldn't contain that value.
if (config->periph_id == -1)return -FDT_ERR_NOTFOUND;return fdtdec_get_int_array(blob, clk_node, "params", config->params,PARAM_COUNT);
Property "params" (which should probably be named something better anyway) isn't something that's supported by the kernel binding, and needs discussion/ack there. Instead, I suggest following the kernel's approach for now - don't specify these PHY parameters in the binding, but rather just apply the defaults, which are apparently suitable for all the boards supported by the mainline kernel at least.
+int board_usb_init(const void *blob) +{ +#ifdef CONFIG_OF_CONTROL
struct fdt_usb config;int clk_done = 0;int node, upto = 0;unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);do {node = fdtdec_next_alias(blob, "usb",COMPAT_NVIDIA_TEGRA20_USB, &upto);
Why only initialize USB controllers with aliases? Surely this should enumerate all nodes with a specific compatible flag?
/* The first port we find gets to set the clocks */
Ick.
/* Set up our two ports */+#ifdef CONFIG_TEGRA_USB1_HOST
host_dev_ctlr = (struct usb_ctlr *)TEGRA_USB1_BASE;+#endif
That port is always the host/device switchable controller. Why not always make that assignment? The issue here is probably that set_host_mode() isn't suitable for all boards. The solution seems to be to fix that.