Re: [PATCH 2/2] usb: chipidea: imx: Fix ULPI on imx53

Peter Chen
2018-02-07T01:48:24Z

On Tue, Feb 06, 2018 at 04:50:41PM +0100, Sebastian Reichel wrote:
> Hi Peter,
>
> On Mon, Jan 29, 2018 at 11:33:15AM +0800, Peter Chen wrote:
> > On Wed, Jan 24, 2018 at 06:14:39PM +0100, Sebastian Reichel wrote:
> > > Traditionally, PORTSC should be set before initializing ULPI phys. But
> > > setting PORTSC before powering on the phy results in a kernel freeze
> > > on imx53 based GE PPD. As a workaround this initializes the phy early
> > > in the imx platform code and disables phy power management from the
> > > core.
> > >
> > > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk>
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > > ---
> > > drivers/usb/chipidea/ci_hdrc_imx.c | 12 ++++++++++++
> > > 1 file changed, 12 insertions(+)
> > >
> > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > index de155c80eb70..e431c5aafe35 100644
> > > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > @@ -83,6 +83,7 @@ struct ci_hdrc_imx_data {
> > > struct clk *clk;
> > > struct imx_usbmisc_data *usbmisc_data;
> > > bool supports_runtime_pm;
> > > + bool override_phy_control;
> > > bool in_lpm;
> > > /* SoC before i.mx6 (except imx23/imx28) needs three clks */
> > > bool need_three_clks;
> > > @@ -254,6 +255,7 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
> > > int ret;
> > > const struct of_device_id *of_id;
> > > const struct ci_hdrc_imx_platform_flag *imx_platform_flag;
> > > + struct device_node *np = pdev->dev.of_node;
> > >
> > > of_id = of_match_device(ci_hdrc_imx_dt_ids, &pdev->dev);
> > > if (!of_id)
> > > @@ -288,6 +290,14 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
> > > }
> > >
> > > pdata.usb_phy = data->phy;
> > > +
> > > + if (of_device_is_compatible(np, "fsl,imx53-usb") && pdata.usb_phy &&
> > > + of_usb_get_phy_mode(np) == USBPHY_INTERFACE_MODE_ULPI) {
> > > + pdata.flags |= CI_HDRC_OVERRIDE_PHY_CONTROL;
> > > + data->override_phy_control = true;
> > > + usb_phy_init(pdata.usb_phy);
> > > + }
> > > +
> > > pdata.flags |= imx_platform_flag->flags;
> > > if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
> > > data->supports_runtime_pm = true;
> > > @@ -341,6 +351,8 @@ static int ci_hdrc_imx_remove(struct platform_device *pdev)
> > > pm_runtime_put_noidle(&pdev->dev);
> > > }
> > > ci_hdrc_remove_device(data->ci_pdev);
> > > + if (data->override_phy_control)
> > > + usb_phy_shutdown(data->phy);
> > > imx_disable_unprepare_clks(&pdev->dev);
> > >
> >
> > Sebastian, I have a question, do you have any USB or generic PHY drivers
> > for ULPI bus, any power controls are needed for your ULPI peripheral?
>
> The devicetree for GE PPD is available in the mainline kernel:
>
> $ grep -A9 "usbphy[23] {" arch/arm/boot/dts/imx53-ppd.dts
> usbphy2: usbphy2 {
> compatible = "usb-nop-xceiv";
> reset-gpios = <&gpio4 4 GPIO_ACTIVE_LOW>;
> clock-names = "main_clk";
> clock-frequency = <24000000>;
> clocks = <&clks IMX5_CLK_CKO2>;
> assigned-clocks = <&clks IMX5_CLK_CKO2_SEL>, <&clks IMX5_CLK_OSC>;
> assigned-clock-parents = <&clks IMX5_CLK_OSC>;
> };
>
> usbphy3: usbphy3 {
> compatible = "usb-nop-xceiv";
> reset-gpios = <&gpio2 19 GPIO_ACTIVE_LOW>;
> clock-names = "main_clk";
>
> clock-frequency = <24000000>;
> clocks = <&clks IMX5_CLK_CKO2>;
> assigned-clocks = <&clks IMX5_CLK_CKO2_SEL>, <&clks IMX5_CLK_OSC>;
> assigned-clock-parents = <&clks IMX5_CLK_OSC>;
> };
>
> So currently the machine only uses drivers/usb/phy/phy-generic.c. Both
> USB phys are actually SMSC USB3315, which is also detected by the kernel:
>
> root@csmon :~# cat /sys/bus/ulpi/devices/ci_hdrc.*.ulpi/uevent
> DEVTYPE=ulpi_device
> MODALIAS=ulpi:v0424p0006
> DEVTYPE=ulpi_device
> MODALIAS=ulpi:v0424p0006
>
> So maybe drivers/usb/phy/phy-ulpi.c should be used, but I don't see
> a simple way to do so and using the generic PHY works.
>

It is correct you use phy-generic.c if it can let your design
work, thanks.

--

Best Regards,
Peter Chen