Hi Stephen,
Here are my comments on the rest of your email.
On Mon, Nov 28, 2011 at 11:21 AM, Stephen Warren swarren@nvidia.com wrote:
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).
I have done this. It is tricky since so far we have no way of setting up the pinmux for a given GPIO. Rather than bite off that camel I have just set up the pinmux in the board file.
+/* 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()?
This function is exported, and the caller doesn't have a pointer to the host/device-switchable port.
+#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)?
I have removed this function.
+#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.
OK will await comments. The kernel bindings seem very basic at this stage.
- 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."
OK I have changed this.
- 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.
See my previous comments as to why this is desirable. We can change over to a clock-based approach once the kernel implements it.
- 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.
As discussed I have changed the name but otherwise left this as is. since you mentioned that some boards may need to change the parameters (it seems Tegra3 is different also).
+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?
See my other comments - we want to know that port 0 is USB3 on Seaboard.
- /* The first port we find gets to set the clocks */
Ick.
Well they all uses the same params anyway. It could be refactored to read the timing separately but it is convenient to have it in one structure.
- /* 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.
I have removed this code.
Regards, Simon
-- nvpublic