Message-ID: <87mvkd6i65.fsf@linux.intel.com>
Date:	Tue, 16 Aug 2016 14:56:50 +0300
From:	Felipe Balbi <balbi@...nel.org>
To:	"William.wu" <William.wu@...k-chips.com>,
	gregkh@...uxfoundation.org, heiko@...ech.de
Cc:	linux-rockchip@...ts.infradead.org, briannorris@...gle.com,
	dianders@...gle.com, kever.yang@...k-chips.com,
	huangtao@...k-chips.com, frank.wang@...k-chips.com,
	eddie.cai@...k-chips.com, John.Youn@...opsys.com,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	sergei.shtylyov@...entembedded.com, robh+dt@...nel.org,
	mark.rutland@....com, devicetree@...r.kernel.org,
	zhengsq@...k-chips.com, zyw@...k-chips.com,
	Roger Quadros <rogerq@...com>
Subject: Re: [PATCH v10 5/5] usb: dwc3: add rockchip specific glue layer


Hi,

"William.wu" <William.wu@...k-chips.com> writes:
>> "William.wu" <William.wu@...k-chips.com> writes:
>>>> William Wu <william.wu@...k-chips.com> writes:
>>>>> Add rockchip specific glue layer to support USB3 Peripheral mode
>>>>> and Host mode on rockchip platforms (e.g. rk3399).
>>>>>
>>>>> The DesignWare USB3 integrated in rockchip SoCs is a configurable
>>>>> IP Core which can be instantiated as Dual-Role Device (DRD), Host
>>>>> Only (XHCI) and Peripheral Only configurations.
>>>>>
>>>>> We use extcon notifier to manage usb cable detection and mode switch.
>>>>> Enable DWC3 PM runtime auto suspend to allow core enter runtime_suspend
>>>>> if USB cable is dettached. For host mode, it requires to keep whole
>>>>> DWC3 controller in reset state to hold pipe power state in P2 before
>>>>> initializing PHY. And it need to reconfigure USB PHY interface of DWC3
>>>>> core after deassert DWC3 controller reset.
>>>>>
>>>>> The current driver supports Host only and Peripheral Only well, for
>>>>> now, we will add support for OTG after we have it all stabilized.
>>>>>
>>>>> Signed-off-by: William Wu <william.wu@...k-chips.com>
>>>>> ---
>>>>> Changes in v10:
>>>>> - fix building error
>>>>>
>>>>> Changes in v9:
>>>>> - Add a new dwc3-rockchip.c driver for rk3399, rather than use dwc3-of-simple.c
>>>>>
>>>>>    drivers/usb/dwc3/Kconfig         |   9 +
>>>>>    drivers/usb/dwc3/Makefile        |   1 +
>>>>>    drivers/usb/dwc3/core.c          |   2 +-
>>>>>    drivers/usb/dwc3/core.h          |   1 +
>>>>>    drivers/usb/dwc3/dwc3-rockchip.c | 441 +++++++++++++++++++++++++++++++++++++++
>>>> William, if you need to touch core dwc3 to introduce a glue layer,
>>>> you're doing it wrong.
>>> Sorry， I realized that it's not better to touch core dwc3 in a specific
>>> glue layer.
>>> I touched dwc3 in glue layer, because I want to support dual-role mode， and
>>> according to our dwc3 IP and usb3 PHY IP design， it need to reinit dwc3
>>> core
>>> whenever  usb cable attached.
>>>
>>> Anyway, it's wrong to do that.:-[
>>>
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index e887b38..3da6215 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -405,7 +405,7 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc)
>>>>>     * initialized. The PHY interfaces and the PHYs get initialized together with
>>>>>     * the core in dwc3_core_init.
>>>>>     */
>>>>> -static int dwc3_phy_setup(struct dwc3 *dwc)
>>>>> +int dwc3_phy_setup(struct dwc3 *dwc)
>>>> there's no way I'll let this slip through the cracks :-)
>>> Why I need  dwc3_phy_setup in  glue layer is because usb3 IP design
>>> in rockchip platform. If dwc3 works on host mode, it requires to put
>>> dwc3 controller in reset state before usb3 phy initializing，and after
>>> deassert reset,  we need to reconfigure UTMI+ PHY interface because
>>> our dwc3 core can't configure PHY interface correctly.
>>>
>>> Thank you for give me a chance to explain it.:-)
>>>
>>>>> diff --git a/drivers/usb/dwc3/dwc3-rockchip.c b/drivers/usb/dwc3/dwc3-rockchip.c
>>>>> new file mode 100644
>>>>> index 0000000..eeae1a9
>>>>> --- /dev/null
>>>>> +++ b/drivers/usb/dwc3/dwc3-rockchip.c
>>>>> @@ -0,0 +1,441 @@
>>>> [...]
>>>>
>>>>> +static int dwc3_rockchip_probe(struct platform_device *pdev)
>>>>> +{
>>>>> +	struct dwc3_rockchip	*rockchip;
>>>>> +	struct device		*dev = &pdev->dev;
>>>>> +	struct device_node	*np = dev->of_node, *child;
>>>>> +	struct platform_device	*child_pdev;
>>>>> +
>>>>> +	unsigned int		count;
>>>>> +	int			ret;
>>>>> +	int			i;
>>>>> +
>>>>> +	rockchip = devm_kzalloc(dev, sizeof(*rockchip), GFP_KERNEL);
>>>>> +
>>>>> +	if (!rockchip)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	count = of_clk_get_parent_count(np);
>>>>> +	if (!count)
>>>>> +		return -ENOENT;
>>>>> +
>>>>> +	rockchip->num_clocks = count;
>>>>> +
>>>>> +	rockchip->clks = devm_kcalloc(dev, rockchip->num_clocks,
>>>>> +				      sizeof(struct clk *), GFP_KERNEL);
>>>>> +	if (!rockchip->clks)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	platform_set_drvdata(pdev, rockchip);
>>>>> +
>>>>> +	rockchip->dev = dev;
>>>>> +	rockchip->edev = NULL;
>>>>> +
>>>>> +	pm_runtime_set_active(dev);
>>>>> +	pm_runtime_enable(dev);
>>>>> +	ret = pm_runtime_get_sync(dev);
>>>>> +	if (ret < 0) {
>>>>> +		dev_err(dev, "get_sync failed with err %d\n", ret);
>>>>> +		goto err1;
>>>>> +	}
>>>>> +
>>>>> +	for (i = 0; i < rockchip->num_clocks; i++) {
>>>>> +		struct clk	*clk;
>>>>> +
>>>>> +		clk = of_clk_get(np, i);
>>>>> +		if (IS_ERR(clk)) {
>>>>> +			while (--i >= 0)
>>>>> +				clk_put(rockchip->clks[i]);
>>>>> +			ret = PTR_ERR(clk);
>>>>> +
>>>>> +			goto err1;
>>>>> +		}
>>>>> +
>>>>> +		ret = clk_prepare_enable(clk);
>>>>> +		if (ret < 0) {
>>>>> +			while (--i >= 0) {
>>>>> +				clk_disable_unprepare(rockchip->clks[i]);
>>>>> +				clk_put(rockchip->clks[i]);
>>>>> +			}
>>>>> +			clk_put(clk);
>>>>> +
>>>>> +			goto err1;
>>>>> +		}
>>>>> +
>>>>> +		rockchip->clks[i] = clk;
>>>>> +	}
>>>>> +
>>>>> +	rockchip->otg_rst = devm_reset_control_get(dev, "usb3-otg");
>>>>> +	if (IS_ERR(rockchip->otg_rst)) {
>>>>> +		dev_err(dev, "could not get reset controller\n");
>>>>> +		ret = PTR_ERR(rockchip->otg_rst);
>>>>> +		goto err2;
>>>>> +	}
>>>>> +
>>>>> +	ret = dwc3_rockchip_extcon_register(rockchip);
>>>>> +	if (ret < 0)
>>>>> +		goto err2;
>>>>> +
>>>>> +	child = of_get_child_by_name(np, "dwc3");
>>>>> +	if (!child) {
>>>>> +		dev_err(dev, "failed to find dwc3 core node\n");
>>>>> +		ret = -ENODEV;
>>>>> +		goto err3;
>>>>> +	}
>>>>> +
>>>>> +	/* Allocate and initialize the core */
>>>>> +	ret = of_platform_populate(np, NULL, NULL, dev);
>>>>> +	if (ret) {
>>>>> +		dev_err(dev, "failed to create dwc3 core\n");
>>>>> +		goto err3;
>>>>> +	}
>>>>> +
>>>>> +	child_pdev = of_find_device_by_node(child);
>>>>> +	if (!child_pdev) {
>>>>> +		dev_err(dev, "failed to find dwc3 core device\n");
>>>>> +		ret = -ENODEV;
>>>>> +		goto err4;
>>>>> +	}
>>>>> +
>>>>> +	rockchip->dwc = platform_get_drvdata(child_pdev);
>>>> No! You will *NOT* the core struct device. Don't even try to come up
>>>> with tricks like this.
>>>>
>>>> Let's do this: introduce a glue layer that gets peripheral-only
>>>> working. Remove PM for now, too. Start with something simple, get the
>>>> bare minimum working upstream and add more stuff as you go.
>>>>
>>>> Trying to do everything in one patch just makes it much more likely for
>>>> your patch to be NAKed. What you're doing here is bypassing all the
>>>> layering we've built. That won't work very well. The only thing you'll
>>>> get is for your patches to continue to be NAKed.
>>>>
>>>> Avoid the tricks and abuses. Just because you _can_ do it somehow, it
>>>> doesn't mean you _should_ do it :-)
>>>>
>>>> Your best option right now, is to remove PM and dual-role support and a
>>>> minimal glue layer supported.
>>>>
>>>> In fact, all you *really* need is to add a compatible to
>>>> dwc3-of-simple.c. That should be enough to get your dwc3 working. Don't
>>>> do anything more than that. For dual-role and PM, we can add it
>>>> generically to dwc3-of-simple.c when all pieces fall into place.
>>>>
>>> Ah, thanks very much for your kind explanation. I think I just only need
>>> to add a compatible to dwc3-of-simple.c，for now， and I have tested
>>> my dwc3， it worked well on peripheral only mode and host only mode
>>> without PM. Further, if dwc3-of-simple.c adds generic handling of dual-role
>>> and PM, I can improve our dwc3 feature.:-)
>> that's my point exactly. We can add more support generically so that
>> other platforms can benefit from the work. PM should be simple for
>> dwc3-of-simple.c. Dual-role will take a little more effort. In almost
>> there actually. There are a few missing pieces but it should be doable
>> (hopefully) within the next two major releases.
>>
>> Your integration is no different than other companies' using DWC3 in
>> dual-role setup. For example TI's DWC3 have the same requirements as you
>> do, so it makes sense to add it straight to dwc3-core. Roger Quadros
>> (now in Cc) has been working on dual-role for TI's platforms and we've
>> been discussing about how to add missing pieces generically. Perhaps
>> you'd want to join the discussion.
> Thanks！ I'll upload a new patch later. And I have seen the dual-role patch
> uploaded by Roger Quadros, it's helpful for me. I'm interested in the 
> patch，
> but I need to understand the patch first， hope to be able to join the 
> discussion.:-)

cool, thanks. More users means we're more likely to make a trully
generic layer. We're discussing some simplification of that layer,
however, so that it doesn't take as much code to get DRD working.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (819 bytes)

Powered by blists - more mailing lists