Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

From: Fabien Lahoudere
Date: Tue May 23 2017 - 14:16:45 EST

Next message: Lino Sanfilippo: "Re: [PATCH v6 net-next 17/17] net: qualcomm: add QCA7000 UART driver"
Previous message: Chris Metcalf: "Re: Updating kernel.org cross compilers?"
Next in thread: Stephen Boyd: "Re: [RFC] usb-phy-generic: Add support to SMSC USB3315"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

We investigate on the topic and now our device tree look like:

in imx53.dtsi:

usbh2: usb@53f80400 {
compatible = "fsl,imx53-usb", "fsl,imx27-usb";
ÂÂÂÂÂÂÂÂreg = <0x53f80400 0x0200>;
ÂÂÂÂÂÂÂÂinterrupts = <16>;
ÂÂÂÂÂÂÂÂclocks = <&clks IMX5_CLK_USBOH3_GATE>;
ÂÂÂÂÂÂÂÂfsl,usbmisc = <&usbmisc 2>;
ÂÂÂÂÂÂÂÂdr_mode = "host";
ÂÂÂÂÂÂÂÂstatus = "disabled";
};

usbmisc: usbmisc@53f80800 {
#index-cells = <1>;
compatible = "fsl,imx53-usbmisc";
reg = <0x53f80800 0x200>;
clocks = <&clks IMX5_CLK_USBOH3_GATE>;
};

and in our dts:

&usbh2 {
ÂÂÂÂÂÂÂÂpinctrl-names = "default";
pinctrl-0 = <&pinctrl_usbh2>;
disable-int60ck;
ÂÂÂÂÂÂÂÂdr_mode = "host";
ÂÂÂÂÂÂÂÂ//fsl,usbphy = <&usbphy2>;
ÂÂÂÂÂÂÂÂvbus-supply = <&reg_usbh2_vbus>;
ÂÂÂÂÂÂÂÂstatus = "okay";
ulpi {
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂphy {
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "smsc,usb3315-ulpi";
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreset-gpios = <&gpio4 4 GPIO_ACTIVE_LOW>;
clock-names = "main_clk";
/*
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* Hardware uses CKO2 at 24MHz at several places. Set the parent
Â*ÂÂclock of CKO2 to OSC.
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ*/
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>;
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstatus = "okay";
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
ÂÂÂÂÂÂÂÂ};
};

And we create a basic driver to check what happened:

static int smsc_usb3315_phy_probe(struct ulpi *ulpi)
{
ÂÂÂÂÂÂÂÂprintk(KERN_ERR "Fabien: %s:%d-%s\n", __FILE__, __LINE__, __func__);

ÂÂÂÂÂÂÂÂreturn 0;
}

static const struct of_device_id smsc_usb3315_phy_match[] = {
ÂÂÂÂÂÂÂÂ{ .compatible = "smsc,usb3315-phy", },
ÂÂÂÂÂÂÂÂ{ }
};
MODULE_DEVICE_TABLE(of, smsc_usb3315_phy_match);

static struct ulpi_driver smsc_usb3315_phy_driver = {
ÂÂÂÂÂÂÂÂ.probe = smsc_usb3315_phy_probe,
ÂÂÂÂÂÂÂÂ.driver = {
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ.name = "smsc_usb3315_phy",
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ.of_match_table = smsc_usb3315_phy_match,
ÂÂÂÂÂÂÂÂ},
};
module_ulpi_driver(smsc_usb3315_phy_driver);

/*MODULE_ALIAS("platform:usb_phy_generic");*/
MODULE_AUTHOR("GE Healthcare");
MODULE_DESCRIPTION("SMSC USB 3315 ULPI Phy driver");
MODULE_LICENSE("GPL v2");

I checked that the driver is registered by drivers/usb/common/ulpi.c:__ulpi_register_driver
successfully.

But our probe function (smsc_usb3315_phy_probe) is never called.

Our understanding is that we need to use the ulpi_bus instead of devm_usb_get_phy_by_phandle(&pdev-
>dev, "fsl,usbphy", 0); in drivers/usb/chipidea/ci_hdrc_imx.c.

Is our approach good?
How can we use this bus from our controller probe function ?

Thanks
Fabien

On Thu, 2017-04-20 at 16:50 +0800, Peter Chen wrote:
> On Wed, Apr 19, 2017 at 06:14:13AM +0000, Peter Senna Tschudin wrote:
> > We need the SMSC USB3315 clock and regulator to always be initialized.
> > We also need the PHY driver to take the PHY out of reset. This patch
> > extends the existing USB generic nop phy driver to include a new
> > initialization path.
> >
> > A new compatible string "smsc,usb3315" is used to decide which
> > initialization path to use.
> >
>
> Hi Peter,
>
> This is an ULPI PHY, so it is not suitable using generic USB PHY.
> Taking a look of drivers/phy/phy-qcom-usb-hs.c, you may have some
> clues.
>
> Peter
>
> > CC: Peter Chen <peter.chen@xxxxxxx>
> > CC: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
> > CC: Fabien Lahoudere <fabien.lahoudere@xxxxxxxxxxxxxxx>
> > Signed-off-by: Peter Senna Tschudin <peter.senna@xxxxxxxxxxxxx>
> > ---
> >
> > This is a follow-up of previous discussion:
> > Â https://www.spinics.net/lists/linux-usb/msg146680.html
> >
> > Âdrivers/usb/phy/phy-generic.c | 33 +++++++++++++++++++++++++++++----
> > Âdrivers/usb/phy/phy-generic.h |ÂÂ1 +
> > Â2 files changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
> > index 89d6e7a..6ea9ce4 100644
> > --- a/drivers/usb/phy/phy-generic.c
> > +++ b/drivers/usb/phy/phy-generic.c
> > @@ -151,6 +151,9 @@ int usb_gen_phy_init(struct usb_phy *phy)
> > Â struct usb_phy_generic *nop = dev_get_drvdata(phy->dev);
> > Â int ret;
> > Â
> > + if (nop->init_done)
> > + return 0;
> > +
> > Â if (!IS_ERR(nop->vcc)) {
> > Â if (regulator_enable(nop->vcc))
> > Â dev_err(phy->dev, "Failed to enable power\n");
> > @@ -164,6 +167,8 @@ int usb_gen_phy_init(struct usb_phy *phy)
> > Â
> > Â nop_reset(nop);
> > Â
> > + nop->init_done = true;
> > +
> > Â return 0;
> > Â}
> > ÂEXPORT_SYMBOL_GPL(usb_gen_phy_init);
> > @@ -216,18 +221,29 @@ static int nop_set_host(struct usb_otg *otg, struct usb_bus *host)
> > Â otg->host = host;
> > Â return 0;
> > Â}
> > +int smsc_usb3315_init(struct usb_phy_generic *nop)
> > +{
> > + /*
> > + Â* If the gpio for controlling reset state is not available, try again
> > + Â* later
> > + Â*/
> > + if(!nop->gpiod_reset)
> > + return -EPROBE_DEFER;
> > +
> > + return usb_gen_phy_init(&nop->phy);
> > +}
> > Â
> > Âint usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop,
> > Â struct usb_phy_generic_platform_data *pdata)
> > Â{
> > + struct device_node *node = NULL;
> > Â enum usb_phy_type type = USB_PHY_TYPE_USB2;
> > Â int err = 0;
> > -
> > Â u32 clk_rate = 0;
> > Â bool needs_vcc = false;
> > Â
> > Â if (dev->of_node) {
> > - struct device_node *node = dev->of_node;
> > + node = dev->of_node;
> > Â
> > Â if (of_property_read_u32(node, "clock-frequency", &clk_rate))
> > Â clk_rate = 0;
> > @@ -304,6 +320,12 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop,
> > Â nop->phy.otg->set_host = nop_set_host;
> > Â nop->phy.otg->set_peripheral = nop_set_peripheral;
> > Â
> > + if(node && of_device_is_compatible(node, "smsc,usb3315")) {
> > + err = smsc_usb3315_init(nop);
> > + if (err)
> > + return err;
> > + }
> > +
> > Â return 0;
> > Â}
> > ÂEXPORT_SYMBOL_GPL(usb_phy_gen_create_phy);
> > @@ -318,6 +340,10 @@ static int usb_phy_generic_probe(struct platform_device *pdev)
> > Â if (!nop)
> > Â return -ENOMEM;
> > Â
> > + platform_set_drvdata(pdev, nop);
> > +
> > + nop->init_done = false;
> > +
> > Â err = usb_phy_gen_create_phy(dev, nop, dev_get_platdata(&pdev->dev));
> > Â if (err)
> > Â return err;
> > @@ -346,8 +372,6 @@ static int usb_phy_generic_probe(struct platform_device *pdev)
> > Â return err;
> > Â }
> > Â
> > - platform_set_drvdata(pdev, nop);
> > -
> > Â return 0;
> > Â}
> > Â
> > @@ -362,6 +386,7 @@ static int usb_phy_generic_remove(struct platform_device *pdev)
> > Â
> > Âstatic const struct of_device_id nop_xceiv_dt_ids[] = {
> > Â { .compatible = "usb-nop-xceiv" },
> > + { .compatible = "smsc,usb3315" },
> > Â { }
> > Â};
> > Â
> > diff --git a/drivers/usb/phy/phy-generic.h b/drivers/usb/phy/phy-generic.h
> > index 0d0eadd..db4ade6 100644
> > --- a/drivers/usb/phy/phy-generic.h
> > +++ b/drivers/usb/phy/phy-generic.h
> > @@ -14,6 +14,7 @@ struct usb_phy_generic {
> > Â struct gpio_desc *gpiod_vbus;
> > Â struct regulator *vbus_draw;
> > Â bool vbus_draw_enabled;
> > + bool init_done;
> > Â unsigned long mA;
> > Â unsigned int vbus;
> > Â};
> > --Â
> > 2.9.3
> >
>
>

Next message: Lino Sanfilippo: "Re: [PATCH v6 net-next 17/17] net: qualcomm: add QCA7000 UART driver"
Previous message: Chris Metcalf: "Re: Updating kernel.org cross compilers?"
Next in thread: Stephen Boyd: "Re: [RFC] usb-phy-generic: Add support to SMSC USB3315"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]