Hi Heiko,

On 06/02/2016 07:46 AM, Heiko Stübner wrote:
Hi Frank,

Am Dienstag, 31. Mai 2016, 14:40:11 schrieb Frank Wang:
The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
than rk3288 and before, and most of phy-related registers are also
different from the past, so a new phy driver is required necessarily.

Signed-off-by: Frank Wang <frank.w...@rock-chips.com>
---

[...]

+static void rockchip_usb2phy_clk480m_disable(struct clk_hw *hw)
+{
+       struct rockchip_usb2phy *rphy =
+               container_of(hw, struct rockchip_usb2phy, clk480m_hw);
+       int index;
+
+       /* make sure all ports in suspended mode */
+       for (index = 0; index != rphy->phy_cfg->num_ports; index++)
+               if (!rphy->ports[index].suspended)
+                       return;

This function can only get called when all clk-references have disabled the
clock, so you should never reach that point that one phy port is not
suspended?


Yeah, you are right, I will clean them up in the next patches.

+
+       /* turn off 480m clk output */
+       property_enable(rphy, &rphy->phy_cfg->clkout_ctl, false);
+}
+

[...]

add something like:

static void rockchip_usb2phy_clk480m_unregister(void *data)
{
        struct rockchip_usb2phy *rphy = data;

        of_clk_del_provider(rphy->dev->of_node);
        clk_unregister(rphy->clk480m);
}

+static struct clk *
+rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
+{
+       struct device_node *node = rphy->dev->of_node;
+       struct clk *clk;
+       struct clk_init_data init;
        int ret;

+
+       init.name = "clk_usbphy_480m";
+       init.ops = &rockchip_usb2phy_clkout_ops;
+       init.flags = CLK_IS_ROOT;
+       init.parent_names = NULL;
+       init.num_parents = 0;
+       rphy->clk480m_hw.init = &init;
+
+       /* optional override of the clockname */
+       of_property_read_string(node, "clock-output-names", &init.name);
+
+       /* register the clock */
+       clk = clk_register(rphy->dev, &rphy->clk480m_hw);
        if (IS_ERR(clk))
                return clk:

        ret = of_clk_add_provider(node, of_clk_src_simple_get, clk);
        if (ret < 0)
                goto err_clk_provider;

        ret = devm_add_action(rphy->dev, rockchip_usb2phy_clk480m_unregister,
rphy);
        if (ret < 0)
                goto err_unreg_action;

        return clk;

err_unreg_action:
        of_clk_del_provider(node);
err_clk_provider:
        clk_unregister(clk);
        return ERR_PTR(ret);

+}


Good comments! Those codes will be included in the next.

[...]

+/*
+ * The function manage host-phy port state and suspend/resume phy port
+ * to save power.
+ *
+ * we rely on utmi_linestate and utmi_hostdisconnect to identify whether
+ * FS/HS is disconnect or not. Besides, we do not need care it is FS
+ * disconnected or HS disconnected, actually, we just only need get the
+ * device is disconnected at last through rearm the delayed work,
+ * to suspend the phy port in _PHY_STATE_DISCONNECT_ case.
+ *
+ * NOTE: It will invoke some clk related APIs, so do not invoke it from
+ * interrupt context.

This does not seem to match the code, as I don't see any clk_* calls,

Sorry for not describing the comment clearly. In a fact, *_sm_work will invoke *phy_suspend or *phy_resume which will invoke some *clk APIs.

Anyway, I will correct it to be more clear.


+ */
+static void rockchip_usb2phy_sm_work(struct work_struct *work)
+{
>> +
 [...]
>> +
+               /* fall through */
+       case PHY_STATE_HS_CONNECT:
+               if (rport->suspended) {
+                       dev_dbg(&rport->phy->dev, "HS/FS connected\n");
+                       rockchip_usb2phy_resume(rport->phy);
+                       rport->suspended = false;
+               }
+               break;
+       case PHY_STATE_DISCONNECT:
+               if (!rport->suspended) {
+                       dev_dbg(&rport->phy->dev, "HS/FS disconnected\n");
+                       rockchip_usb2phy_suspend(rport->phy);
+                       rport->suspended = true;
+               }
 [...]
>> +
+       }
+
+next_schedule:
+       mutex_unlock(&rport->mutex);
+       schedule_delayed_work(&rport->sm_work, SCHEDULE_DELAY);
+}

[...]

+static int rockchip_usb2phy_probe(struct platform_device *pdev)
+{
>> +
>>  [...]
>> +
+
+       provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+       return PTR_ERR_OR_ZERO(provider);
+
+put_child:
+       of_node_put(child_np);

+       of_clk_del_provider(np);
+       clk_unregister(rphy->clk480m);

these two can go away, once you have the devm_action described
near your clk_register function.

Done


+       return ret;
+}

BR.
Frank

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

Reply via email to