On Tue, Feb 08, 2011 at 09:22:04PM -0800, Benoit Goby wrote:
> Interface used by Tegra's gadget driver and ehci driver
> to power on and configure the USB PHYs.
>
> Signed-off-by: Benoit Goby <benoit@xxxxxxxxxxx>
> ---
For some reason this patch (along with patch 3) appears to contain
spaces as indentation (no hard tabs), although patches 2 and 4 in this
series have the correct tabs.
[...]
> +#define USB_USBSTS 0x144
> +#define USB_USBSTS_PCI (1 << 2)
This doesn't appear to be used?
[...]
> +static DEFINE_SPINLOCK(utmip_pad_lock);
> +static int utmip_pad_count;
> +
> +static const int udc_freq_table[] = {
> + 12000000,
> + 13000000,
> + 19200000,
> + 26000000,
> +};
> +
> +static const u8 udc_delay_table[][4] = {
> + /* ENABLE_DLY, STABLE_CNT, ACTIVE_DLY, XTAL_FREQ_CNT */
> + {0x02, 0x2F, 0x04, 0x76}, /* 12 MHz */
> + {0x02, 0x33, 0x05, 0x7F}, /* 13 MHz */
> + {0x03, 0x4B, 0x06, 0xBB}, /* 19.2 MHz */
> + {0x04, 0x66, 0x09, 0xFE}, /* 26 Mhz */
> +};
Use an array of structs here? Writing
"UTMIP_PLL_ACTIVE_DLY_COUNT(udc_delay_table[phy->freq_sel][2])" below is a
lot less clear and more prone to bugs than, say,
"UTMIP_PLL_ACTIVE_DLY_COUNT(udc_delay_table[phy->freq_sel].active_dly)".
It'd be even better to combine all three tables into one; something
like:
static const struct udc_... udc_table[] = {
{ .freq = 12000000,
.delay = {
.enable = 0x02,
.stable = 0x2f,
.active = 0x04
},
.xtal_freq_cnt = 0x76,
.debounce = 0x7530,
},
[... etc]
};
[...]
> +static int utmip_pad_open(struct tegra_usb_phy *phy)
> +{
> + phy->pad_clk = clk_get_sys("utmip-pad", NULL);
> + if (IS_ERR(phy->pad_clk)) {
> + pr_err("%s: can't get utmip pad clock\n", __func__);
> + return -1;
return PTR_ERR(phy->pad_clk);
> + }
> +
> + if (phy->instance == 0) {
> + phy->pad_regs = phy->regs;
> + } else {
> + phy->pad_regs = ioremap(TEGRA_USB_BASE, TEGRA_USB_SIZE);
> + if (!phy->pad_regs) {
> + pr_err("%s: can't remap usb registers\n", __func__);
> + clk_put(phy->pad_clk);
> + return -ENOMEM;
> + }
> + }
> + return 0;
> +}
[...]
> +static int utmip_pad_power_off(struct tegra_usb_phy *phy)
> +{
> + unsigned long val, flags;
> + void __iomem *base = phy->pad_regs;
> +
> + if (!utmip_pad_count) {
> + pr_err("%s: utmip pad already powered off\n", __func__);
> + return -1;
return -EINVAL?
> + }
> +
> + clk_enable(phy->pad_clk);
> +
> + spin_lock_irqsave(&utmip_pad_lock, flags);
> +
> + if (--utmip_pad_count == 0) {
> + val = readl(base + UTMIP_BIAS_CFG0);
> + val |= UTMIP_OTGPD | UTMIP_BIASPD;
> + writel(val, base + UTMIP_BIAS_CFG0);
> + }
> +
> + spin_unlock_irqrestore(&utmip_pad_lock, flags);
> +
> + clk_disable(phy->pad_clk);
> +
> + return 0;
> +}
> +
[...]
> +static void ulpi_phy_power_on(struct tegra_usb_phy *phy)
> +{
> + unsigned long val;
> + void __iomem *base = phy->regs;
> + struct tegra_ulpi_config *config = phy->config;
> +
> + gpio_direction_output(config->reset_gpio, 0);
> + msleep(5);
> + gpio_direction_output(config->reset_gpio, 1);
> +
> + clk_enable(phy->clk);
> + msleep(1);
This msleep seems excessive. Does it take that long for the clock to
settle?
> +
> + val = readl(base + USB_SUSP_CTRL);
> + val |= UHSIC_RESET;
> + writel(val, base + USB_SUSP_CTRL);
> +
> + val = readl(base + ULPI_TIMING_CTRL_0);
> + val |= ULPI_OUTPUT_PINMUX_BYP | ULPI_CLKOUT_PINMUX_BYP;
> + writel(val, base + ULPI_TIMING_CTRL_0);
> +
> + val = readl(base + USB_SUSP_CTRL);
> + val |= ULPI_PHY_ENABLE;
> + writel(val, base + USB_SUSP_CTRL);
> +
> + val = 0;
> + writel(val, base + ULPI_TIMING_CTRL_1);
> +
> + val |= ULPI_DATA_TRIMMER_SEL(4);
> + val |= ULPI_STPDIRNXT_TRIMMER_SEL(4);
> + val |= ULPI_DIR_TRIMMER_SEL(4);
> + writel(val, base + ULPI_TIMING_CTRL_1);
> + udelay(10);
> +
> + val |= ULPI_DATA_TRIMMER_LOAD;
> + val |= ULPI_STPDIRNXT_TRIMMER_LOAD;
> + val |= ULPI_DIR_TRIMMER_LOAD;
> + writel(val, base + ULPI_TIMING_CTRL_1);
> +
> + val = ULPI_WAKEUP | ULPI_RD_RW_WRITE | ULPI_PORT(0);
> + writel(val, base + ULPI_VIEWPORT);
> +
> + if (utmi_wait_register(base + ULPI_VIEWPORT, ULPI_WAKEUP, 0)) {
> + pr_err("%s: timeout waiting for ulpi phy wakeup\n", __func__);
> + return;
Why does this function return void if it can fail?
tegra_usb_phy_power_on() can propagate the error.
> + }
> +
> + /* Fix VbusInvalid due to floating VBUS */
> + ulpi_viewport_write(phy, 0x08, 0x40);
> + ulpi_viewport_write(phy, 0x0B, 0x80);
> +
> + val = readl(base + USB_PORTSC1);
> + val |= USB_PORTSC1_WKOC | USB_PORTSC1_WKDS | USB_PORTSC1_WKCN;
> + writel(val, base + USB_PORTSC1);
> +
> + val = readl(base + USB_SUSP_CTRL);
> + val |= USB_SUSP_CLR;
> + writel(val, base + USB_SUSP_CTRL);
> + udelay(100);
> +
> + val = readl(base + USB_SUSP_CTRL);
> + val &= ~USB_SUSP_CLR;
> + writel(val, base + USB_SUSP_CTRL);
> +}
[...]
> +struct tegra_usb_phy *tegra_usb_phy_open(int instance, void __iomem *regs,
> + void *config, enum tegra_usb_phy_mode phy_mode)
> +{
> + struct tegra_usb_phy *phy;
> + struct tegra_ulpi_config *ulpi_config;
> + unsigned long parent_rate;
> + int freq_sel;
> + int err;
> +
> + phy = kmalloc(sizeof(struct tegra_usb_phy), GFP_KERNEL);
> + if (!phy)
> + return ERR_PTR(-ENOMEM);
> +
> + phy->instance = instance;
> + phy->regs = regs;
> + phy->config = config;
> + phy->mode = phy_mode;
> +
> + if (!phy->config) {
> + if (instance == 1) {
Instead of all of the (instance == 1) checks here and below (I count
eleven separate places this is checked), it might be more readable to
have a macro or static inline function such as bool phy_is_ulpi(phy).
> + pr_err("%s: ulpi phy configuration missing", __func__);
> + err = -EINVAL;
> + goto err0;
> + } else {
> + phy->config = &utmip_default[instance];
> + }
> + }
> +
> + phy->pll_u = clk_get_sys(NULL, "pll_u");
> + if (IS_ERR(phy->pll_u)) {
> + pr_err("Can't get pll_u clock\n");
> + err = PTR_ERR(phy->pll_u);
> + goto err0;
> + }
> + clk_enable(phy->pll_u);
> +
> + parent_rate = clk_get_rate(clk_get_parent(phy->pll_u));
> + for (freq_sel = 0; freq_sel < ARRAY_SIZE(udc_freq_table); freq_sel++) {
> + if (udc_freq_table[freq_sel] == parent_rate)
> + break;
> + }
> + if (freq_sel == ARRAY_SIZE(udc_freq_table)) {
> + pr_err("invalid pll_u parent rate %ld\n", parent_rate);
> + err = -EINVAL;
> + goto err1;
> + }
> + phy->freq_sel = freq_sel;
> +
> + if (phy->instance == 1) {
> + ulpi_config = config;
> + phy->clk = clk_get_sys(NULL, ulpi_config->clk);
> + if (IS_ERR(phy->clk)) {
> + pr_err("%s: can't get ulpi clock\n", __func__);
> + err = -ENXIO;
> + goto err1;
> + }
> + tegra_gpio_enable(ulpi_config->reset_gpio);
> + gpio_request(ulpi_config->reset_gpio, "ulpi_phy_reset_b");
> + gpio_direction_output(ulpi_config->reset_gpio, 0);
> + } else {
> + err = utmip_pad_open(phy);
> + if (err < 0)
> + goto err1;
> + }
> +
> + return phy;
> +
> +err1:
> + clk_disable(phy->pll_u);
> + clk_put(phy->pll_u);
> +err0:
> + kfree(phy);
> + return ERR_PTR(err);
> +}
[...]
The rest seems okay.
> --
> 1.7.3.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
