On Tue, Oct 28, 2014 at 03:27:53PM -0700, Andrew Bresticker wrote:
[...]
> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
[...]
> +#define TEGRA_XHCI_NUM_SUPPLIES 8
> +static const char *tegra_xhci_supply_names[TEGRA_XHCI_NUM_SUPPLIES] = {
> + "avddio-pex",
> + "dvddio-pex",
> + "avdd-usb",
> + "avdd-pll-utmip",
> + "avdd-pll-erefe",
> + "avdd-pex-pll",
> + "hvdd-pex",
> + "hvdd-pex-plle",
> +};
This could be in a per-SoC structure since it's likely to change in a
future SoC. That could be done later on when it really becomes relevant,
though.
> +
> +static const struct {
> + const char *name;
> + int num;
unsigned?
> +} tegra_xhci_phy_types[] = {
> + {
> + .name = "usb3",
> + .num = TEGRA_XUSB_USB3_PHYS,
> + }, {
> + .name = "utmi",
> + .num = TEGRA_XUSB_UTMI_PHYS,
> + }, {
> + .name = "hsic",
> + .num = TEGRA_XUSB_HSIC_PHYS,
> + },
> +};
Should these constants perhaps be in a per-SoC structure like
tegra_xhci_soc_config rather than defined in a global header?
> +static int tegra_xhci_load_firmware(struct tegra_xhci_hcd *tegra)
> +{
[...]
> + /* Start Falcon CPU. */
> + csb_writel(tegra, CPUCTL_STARTCPU, XUSB_FALC_CPUCTL);
> + usleep_range(1000, 2000);
> +
> + fw_time = le32_to_cpu(cfg_tbl->fwimg_created_time);
> + time_to_tm(fw_time, 0, &fw_tm);
> + dev_info(dev,
> + "Firmware timestamp: %ld-%02d-%02d %02d:%02d:%02d UTC, "
> + "Falcon state 0x%x\n", fw_tm.tm_year + 1900,
> + fw_tm.tm_mon + 1, fw_tm.tm_mday, fw_tm.tm_hour,
> + fw_tm.tm_min, fw_tm.tm_sec,
> + csb_readl(tegra, XUSB_FALC_CPUCTL));
> +
> + /* Make sure Falcon CPU is now running. */
> + if (csb_readl(tegra, XUSB_FALC_CPUCTL) == CPUCTL_STATE_HALTED)
> + return -EIO;
It seems somewhat strange to output the dev_info() message when in fact
it could be that the Falcon wasn't successfully booted. Also is it
guaranteed that the Falcon will always be up after 1 ms? Perhaps better
would be to use a timed loop?
> +static int tegra_xhci_set_ss_clk(struct tegra_xhci_hcd *tegra,
> + unsigned long rate)
> +{
> + unsigned long new_parent_rate, old_parent_rate;
> + int ret, div;
> + struct clk *clk = tegra->ss_src_clk;
> +
> + if (clk_get_rate(clk) == rate)
> + return 0;
> +
> + switch (rate) {
> + case TEGRA_XHCI_SS_CLK_HIGH_SPEED:
> + /*
> + * Reparent to PLLU_480M. Set divider first to avoid
> + * overclocking.
> + */
> + old_parent_rate = clk_get_rate(clk_get_parent(clk));
> + new_parent_rate = clk_get_rate(tegra->pll_u_480m);
> + div = new_parent_rate / rate;
> + ret = clk_set_rate(clk, old_parent_rate / div);
> + if (ret)
> + return ret;
> + ret = clk_set_parent(clk, tegra->pll_u_480m);
> + if (ret)
> + return ret;
> + /*
> + * The rate should already be correct, but set it again just
> + * to be sure.
> + */
> + ret = clk_set_rate(clk, rate);
> + if (ret)
> + return ret;
> + break;
> + case TEGRA_XHCI_SS_CLK_LOW_SPEED:
> + /* Reparent to CLK_M */
> + ret = clk_set_parent(clk, tegra->clk_m);
> + if (ret)
> + return ret;
> + ret = clk_set_rate(clk, rate);
> + if (ret)
> + return ret;
> + break;
> + default:
> + dev_err(tegra->dev, "Invalid SS rate: %lu\n", rate);
> + return -EINVAL;
> + }
> +
> + if (clk_get_rate(clk) != rate) {
> + dev_err(tegra->dev, "SS clock doesn't match requested rate\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
So this is why you need pllu_480m and clk_m clocks. I would've thought
it nice to use something like the assigned-clocks properties to take
care of this, but it seems like this may actually be required to be
updated dynamically at runtime, so a fixed property is not going to be
an option.
> +static int tegra_xhci_clk_enable(struct tegra_xhci_hcd *tegra)
> +{
> + clk_prepare_enable(tegra->pll_e);
> + clk_prepare_enable(tegra->host_clk);
> + clk_prepare_enable(tegra->ss_clk);
> + clk_prepare_enable(tegra->falc_clk);
> + clk_prepare_enable(tegra->fs_src_clk);
> + clk_prepare_enable(tegra->hs_src_clk);
You should error-check these.
> +static int tegra_xhci_phy_enable(struct tegra_xhci_hcd *tegra)
> +{
> + int ret;
> + int i;
I prefer unsigned when the value can't be negative as in this case.
> +
> + for (i = 0; i < ARRAY_SIZE(tegra->phys); i++) {
> + ret = phy_init(tegra->phys[i]);
> + if (ret)
> + goto disable_phy;
> + ret = phy_power_on(tegra->phys[i]);
> + if (ret) {
> + phy_exit(tegra->phys[i]);
> + goto disable_phy;
> + }
> + }
Perhaps a phy_init_and_power_on() helper would be useful. Nothing that
needs to be done as part of this patch, though.
> +
> + return 0;
> +disable_phy:
> + for (i = i - 1; i >= 0; i--) {
> + phy_power_off(tegra->phys[i]);
> + phy_exit(tegra->phys[i]);
You could write this as:
for (i = i; i > 0; i--) {
phy_power_off(tegra->phys[i - 1]);
...
}
for the unsigned case. But I guess this would be a reasonable exception
to let i be signed.
> +static void tegra_xhci_phy_disable(struct tegra_xhci_hcd *tegra)
> +{
> + int i;
There's no reason for it to be signed here, though.
> +
> + for (i = 0; i < ARRAY_SIZE(tegra->phys); i++) {
> + phy_power_off(tegra->phys[i]);
> + phy_exit(tegra->phys[i]);
> + }
> +}
> +
> +static bool is_host_mbox_message(u32 cmd)
> +{
> + switch (cmd) {
> + case MBOX_CMD_INC_SSPI_CLOCK:
> + case MBOX_CMD_DEC_SSPI_CLOCK:
> + case MBOX_CMD_INC_FALC_CLOCK:
> + case MBOX_CMD_DEC_FALC_CLOCK:
> + case MBOX_CMD_SET_BW:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static void tegra_xhci_mbox_work(struct work_struct *work)
> +{
> + struct tegra_xhci_hcd *tegra = container_of(work, struct tegra_xhci_hcd,
> + mbox_req_work);
There's only a single instance of this, but it might still be useful to
wrap it in a static inline for readability.
> + /*
> + * Set the xHCI pointer before xhci_plat_setup() (aka hcd_driver.reset)
I don't think this driver calls xhci_plat_setup() (anymore?).
> + * is called by usb_add_hcd().
> + */
> + *((struct xhci_hcd **) xhci->shared_hcd->hcd_priv) = xhci;
This makes me a little uneasy. Perhaps this should be an XHCI wrapper to
make it look less like you're doing something you shouldn't.
> +static int tegra_xhci_probe(struct platform_device *pdev)
> +{
> + struct tegra_xhci_hcd *tegra;
> + struct usb_hcd *hcd;
> + struct resource *res;
There's a tab between resource and *res which should be a space.
> + struct phy *phy;
> + const struct of_device_id *match;
> + int ret, i, j, k;
> +
> + BUILD_BUG_ON(sizeof(struct tegra_xhci_fw_cfgtbl) != 256);
> +
> + tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> + if (!tegra)
> + return -ENOMEM;
> + tegra->dev = &pdev->dev;
> + platform_set_drvdata(pdev, tegra);
> +
> + match = of_match_device(tegra_xhci_of_match, &pdev->dev);
> + if (!match) {
> + dev_err(&pdev->dev, "No device match found\n");
> + return -ENODEV;
> + }
I don't think this can happen. If there's no match in the table then
this function shouldn't have been called in the first place.
> + tegra->soc_config = match->data;
> +
> + /*
> + * Right now device-tree probed devices don't get dma_mask set.
> + * Since shared usb code relies on it, set it here for now.
> + * Once we have dma capability bindings this can go away.
> + */
> + ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> + if (ret)
> + return ret;
I think that's no longer necessary. of_dma_configure() should take care
of this now.
> + k = 0;
> + for (i = 0; i < ARRAY_SIZE(tegra_xhci_phy_types); i++) {
I think a more idiomatic way to write this would be:
for (i = 0, k = 0; ...)
Thierry
Attachment:
pgpE4SrdsfZow.pgp
Description: PGP signature
