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.
CC: Peter Chen <[email protected]>
CC: Stephen Boyd <[email protected]>
CC: Fabien Lahoudere <[email protected]>
Signed-off-by: Peter Senna Tschudin <[email protected]>
---
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
Hello!
On 4/19/2017 9:14 AM, 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.
>
> CC: Peter Chen <[email protected]>
> CC: Stephen Boyd <[email protected]>
> CC: Fabien Lahoudere <[email protected]>
> Signed-off-by: Peter Senna Tschudin <[email protected]>
> ---
>
> 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
[...]
> @@ -216,18 +221,29 @@ static int nop_set_host(struct usb_otg *otg, struct usb_bus *host)
> otg->host = host;
> return 0;
> }
Need empty line here.
> +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)
You hadn't run the patch thru scripts/checkpatch.pl before posting -- need
space between *if* and (.
[...]
> @@ -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")) {
Same here.
[...]
MBR, Sergei
On Wed, Apr 19, 2017 at 01:03:23PM +0300, Sergei Shtylyov wrote:
> Hello!
>
> On 4/19/2017 9:14 AM, 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.
> >
> > CC: Peter Chen <[email protected]>
> > CC: Stephen Boyd <[email protected]>
> > CC: Fabien Lahoudere <[email protected]>
> > Signed-off-by: Peter Senna Tschudin <[email protected]>
> > ---
> >
> > 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
> [...]
> > @@ -216,18 +221,29 @@ static int nop_set_host(struct usb_otg *otg, struct usb_bus *host)
> > otg->host = host;
> > return 0;
> > }
>
> Need empty line here.
>
> > +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)
>
> You hadn't run the patch thru scripts/checkpatch.pl before posting --
> need space between *if* and (.
>
> [...]
> > @@ -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")) {
>
> Same here.
>
> [...]
>
> MBR, Sergei
Thank you for the review Sergei! Should I send V2 of this RFC fixing
these issues or wait for comments on the validity of this approach?
>
On 04/19/2017 01:24 PM, 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.
>>>
>>> CC: Peter Chen <[email protected]>
>>> CC: Stephen Boyd <[email protected]>
>>> CC: Fabien Lahoudere <[email protected]>
>>> Signed-off-by: Peter Senna Tschudin <[email protected]>
>>> ---
>>>
>>> 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
>> [...]
>>> @@ -216,18 +221,29 @@ static int nop_set_host(struct usb_otg *otg, struct usb_bus *host)
>>> otg->host = host;
>>> return 0;
>>> }
>>
>> Need empty line here.
>>
>>> +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)
>>
>> You hadn't run the patch thru scripts/checkpatch.pl before posting --
>> need space between *if* and (.
>>
>> [...]
>>> @@ -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")) {
>>
>> Same here.
>>
>> [...]
>>
>> MBR, Sergei
>
> Thank you for the review Sergei! Should I send V2 of this RFC fixing
> these issues or wait for comments on the validity of this approach?
Wait, of course, if this is RFC...
WBR, Sergei
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 <[email protected]>
> CC: Stephen Boyd <[email protected]>
> CC: Fabien Lahoudere <[email protected]>
> Signed-off-by: Peter Senna Tschudin <[email protected]>
> ---
>
> 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
>
--
Best Regards,
Peter Chen
On 05/26, Fabien Lahoudere wrote:
> Hello
>
> I modify ci_hrdc_imx_probe to bypass?"data->phy = devm_usb_get_phy_by_phandle(&pdev->dev,
> "fsl,usbphy", 0);". Everything works as expected and call ci_ulpi_init.
>
> The problem is that in ci_ulpi_init, before calling "ci->ulpi = ulpi_register_interface(ci->dev,
> &ci->ulpi_ops);" (to initialize our phy), "hw_phymode_configure(ci);" is called which is the
> original function that make our system to hang.
>
> Our phy is not initialised before calling ulpi_register_interface so I don't understand how the phy
> can reply if it is not out of reset state.
I haven't see any problem in hw_phymode_configure(). What's the
value of ci->platdata->phy_mode? USBPHY_INTERFACE_MODE_ULPI? If
you phy needs to be taken out of reset to reply to the ulpi reads
of the vendor/product ids, then it sounds like you have a similar
situation to what I had. I needed to turn on some regulators to
get those reads to work, otherwise they would fail, but knowing
what needed to be turned on basically meant I needed to probe the
ulpi driver so probing the ids wasn't going to be useful. So on
my device the reads for the ids go through, but they get all
zeroes back, which is actually ok because there aren't any bits
set on my devices anyway. After the reads see 0, we fallback to
DT matching, which avoids the "bring it out of reset/power it on"
sorts of problems entirely.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Fri, 2017-06-02 at 15:00 -0700, Stephen Boyd wrote:
> On 05/26, Fabien Lahoudere wrote:
> > Hello
> >
> > I modify ci_hrdc_imx_probe to bypass "data->phy = devm_usb_get_phy_by_phandle(&pdev->dev,
> > "fsl,usbphy", 0);". Everything works as expected and call ci_ulpi_init.
> >
> > The problem is that in ci_ulpi_init, before calling "ci->ulpi = ulpi_register_interface(ci->dev,
> > &ci->ulpi_ops);" (to initialize our phy), "hw_phymode_configure(ci);" is called which is the
> > original function that make our system to hang.
> >
> > Our phy is not initialised before calling ulpi_register_interface so I don't understand how the
> > phy
> > can reply if it is not out of reset state.
>
> I haven't see any problem in hw_phymode_configure(). What's the
> value of ci->platdata->phy_mode? USBPHY_INTERFACE_MODE_ULPI? If
> you phy needs to be taken out of reset to reply to the ulpi reads
> of the vendor/product ids, then it sounds like you have a similar
> situation to what I had. I needed to turn on some regulators to
> get those reads to work, otherwise they would fail, but knowing
> what needed to be turned on basically meant I needed to probe the
> ulpi driver so probing the ids wasn't going to be useful. So on
> my device the reads for the ids go through, but they get all
> zeroes back, which is actually ok because there aren't any bits
> set on my devices anyway. After the reads see 0, we fallback to
> DT matching, which avoids the "bring it out of reset/power it on"
> sorts of problems entirely.
>
Yes the phy mode is configured to USBPHY_INTERFACE_MODE_ULPI.
Indeed, this phy need to be out of reset to work. For example everything works fine if I call
"_ci_usb_phy_init(ci);" before calling "hw_phymode_configure(ci);"
This function only init reset GPIO and clock.
For information, the original patch I have to fix the issue:
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 79ad8e9..21aaff1 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -391,6 +391,7 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
case USBPHY_INTERFACE_MODE_UTMI:
case USBPHY_INTERFACE_MODE_UTMIW:
case USBPHY_INTERFACE_MODE_HSIC:
+ case USBPHY_INTERFACE_MODE_ULPI:
ret = _ci_usb_phy_init(ci);
if (!ret)
hw_wait_phy_stable();
@@ -398,7 +399,6 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
return ret;
hw_phymode_configure(ci);
break;
- case USBPHY_INTERFACE_MODE_ULPI:
case USBPHY_INTERFACE_MODE_SERIAL:
hw_phymode_configure(ci);
ret = _ci_usb_phy_init(ci);
--
So if some ULPI phys need to be initialised before calling "hw_phymode_configure", is it acceptable
if I separate "case USBPHY_INTERFACE_MODE_ULPI:" and add a DT binding ("init_phy_first") to define
the order to call both functions?
Something like:
case USBPHY_INTERFACE_MODE_ULPI:
if (ci->platdata->init_phy_first) {
ret = _ci_usb_phy_init(ci);
if (!ret)
hw_wait_phy_stable();
else
return ret;
}
hw_phymode_configure(ci);
if (!ci->platdata->init_phy_first) {
ret = _ci_usb_phy_init(ci);
if (ret)
return ret;
}
break;
This approach will not modify current behaviour but allow to initialize phy first on demand.
On Mon, Jun 05, 2017 at 10:57:00AM +0200, Fabien Lahoudere wrote:
> On Fri, 2017-06-02 at 15:00 -0700, Stephen Boyd wrote:
> > On 05/26, Fabien Lahoudere wrote:
> > > Hello
> > >
> > > I modify ci_hrdc_imx_probe to bypass?"data->phy = devm_usb_get_phy_by_phandle(&pdev->dev,
> > > "fsl,usbphy", 0);". Everything works as expected and call ci_ulpi_init.
> > >
> > > The problem is that in ci_ulpi_init, before calling "ci->ulpi = ulpi_register_interface(ci->dev,
> > > &ci->ulpi_ops);" (to initialize our phy), "hw_phymode_configure(ci);" is called which is the
> > > original function that make our system to hang.
> > >
> > > Our phy is not initialised before calling ulpi_register_interface so I don't understand how the
> > > phy
> > > can reply if it is not out of reset state.
> >
> > I haven't see any problem in hw_phymode_configure(). What's the
> > value of ci->platdata->phy_mode? USBPHY_INTERFACE_MODE_ULPI? If
> > you phy needs to be taken out of reset to reply to the ulpi reads
> > of the vendor/product ids, then it sounds like you have a similar
> > situation to what I had. I needed to turn on some regulators to
> > get those reads to work, otherwise they would fail, but knowing
> > what needed to be turned on basically meant I needed to probe the
> > ulpi driver so probing the ids wasn't going to be useful. So on
> > my device the reads for the ids go through, but they get all
> > zeroes back, which is actually ok because there aren't any bits
> > set on my devices anyway. After the reads see 0, we fallback to
> > DT matching, which avoids the "bring it out of reset/power it on"
> > sorts of problems entirely.
> >
>
> Yes the phy mode is configured to USBPHY_INTERFACE_MODE_ULPI.
> Indeed, this phy need to be out of reset to work. For example everything works fine if I call?
> "_ci_usb_phy_init(ci);" before calling "hw_phymode_configure(ci);"
> This function only init reset GPIO and clock.
>
> For information, the original patch I have to fix the issue:
>
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 79ad8e9..21aaff1 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -391,6 +391,7 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
> ? case USBPHY_INTERFACE_MODE_UTMI:
> ? case USBPHY_INTERFACE_MODE_UTMIW:
> ? case USBPHY_INTERFACE_MODE_HSIC:
> + case USBPHY_INTERFACE_MODE_ULPI:
> ? ret = _ci_usb_phy_init(ci);
> ? if (!ret)
> ? hw_wait_phy_stable();
> @@ -398,7 +399,6 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
> ? return ret;
> ? hw_phymode_configure(ci);
> ? break;
> - case USBPHY_INTERFACE_MODE_ULPI:
> ? case USBPHY_INTERFACE_MODE_SERIAL:
> ? hw_phymode_configure(ci);
> ? ret = _ci_usb_phy_init(ci);
> --?
Currently, the hw_phymode_configure is called twice for ULPI PHY, the
two execution are between _ci_usb_phy_init, would you test which one
causes hang? If the second causes hang, you can make a patch for
hw_phymode_configure that if the required PORTSC_PTS is the same
the value in register, do noop.
--
Best Regards,
Peter Chen
On Mon, 2017-06-05 at 17:43 +0800, Peter Chen wrote:
> On Mon, Jun 05, 2017 at 10:57:00AM +0200, Fabien Lahoudere wrote:
> > On Fri, 2017-06-02 at 15:00 -0700, Stephen Boyd wrote:
> > > On 05/26, Fabien Lahoudere wrote:
> > > > Hello
> > > >
> > > > I modify ci_hrdc_imx_probe to bypass "data->phy = devm_usb_get_phy_by_phandle(&pdev->dev,
> > > > "fsl,usbphy", 0);". Everything works as expected and call ci_ulpi_init.
> > > >
> > > > The problem is that in ci_ulpi_init, before calling "ci->ulpi = ulpi_register_interface(ci-
> > > > >dev,
> > > > &ci->ulpi_ops);" (to initialize our phy), "hw_phymode_configure(ci);" is called which is the
> > > > original function that make our system to hang.
> > > >
> > > > Our phy is not initialised before calling ulpi_register_interface so I don't understand how
> > > > the
> > > > phy
> > > > can reply if it is not out of reset state.
> > >
> > > I haven't see any problem in hw_phymode_configure(). What's the
> > > value of ci->platdata->phy_mode? USBPHY_INTERFACE_MODE_ULPI? If
> > > you phy needs to be taken out of reset to reply to the ulpi reads
> > > of the vendor/product ids, then it sounds like you have a similar
> > > situation to what I had. I needed to turn on some regulators to
> > > get those reads to work, otherwise they would fail, but knowing
> > > what needed to be turned on basically meant I needed to probe the
> > > ulpi driver so probing the ids wasn't going to be useful. So on
> > > my device the reads for the ids go through, but they get all
> > > zeroes back, which is actually ok because there aren't any bits
> > > set on my devices anyway. After the reads see 0, we fallback to
> > > DT matching, which avoids the "bring it out of reset/power it on"
> > > sorts of problems entirely.
> > >
> >
> > Yes the phy mode is configured to USBPHY_INTERFACE_MODE_ULPI.
> > Indeed, this phy need to be out of reset to work. For example everything works fine if I call
> > "_ci_usb_phy_init(ci);" before calling "hw_phymode_configure(ci);"
> > This function only init reset GPIO and clock.
> >
> > For information, the original patch I have to fix the issue:
> >
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index 79ad8e9..21aaff1 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -391,6 +391,7 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
> > case USBPHY_INTERFACE_MODE_UTMI:
> > case USBPHY_INTERFACE_MODE_UTMIW:
> > case USBPHY_INTERFACE_MODE_HSIC:
> > + case USBPHY_INTERFACE_MODE_ULPI:
> > ret = _ci_usb_phy_init(ci);
> > if (!ret)
> > hw_wait_phy_stable();
> > @@ -398,7 +399,6 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
> > return ret;
> > hw_phymode_configure(ci);
> > break;
> > - case USBPHY_INTERFACE_MODE_ULPI:
> > case USBPHY_INTERFACE_MODE_SERIAL:
> > hw_phymode_configure(ci);
> > ret = _ci_usb_phy_init(ci);
> > --
>
> Currently, the hw_phymode_configure is called twice for ULPI PHY, the
> two execution are between _ci_usb_phy_init, would you test which one
> causes hang? If the second causes hang, you can make a patch for
> hw_phymode_configure that if the required PORTSC_PTS is the same
> the value in register, do noop.
The first one hangs, _ci_usb_phy_init is not called due to hang.
On Mon, Jun 05, 2017 at 11:52:26AM +0200, Fabien Lahoudere wrote:
> On Mon, 2017-06-05 at 17:43 +0800, Peter Chen wrote:
> > On Mon, Jun 05, 2017 at 10:57:00AM +0200, Fabien Lahoudere wrote:
> > > On Fri, 2017-06-02 at 15:00 -0700, Stephen Boyd wrote:
> > > > On 05/26, Fabien Lahoudere wrote:
> > > > > Hello
> > > > >
> > > > > I modify ci_hrdc_imx_probe to bypass?"data->phy = devm_usb_get_phy_by_phandle(&pdev->dev,
> > > > > "fsl,usbphy", 0);". Everything works as expected and call ci_ulpi_init.
> > > > >
> > > > > The problem is that in ci_ulpi_init, before calling "ci->ulpi = ulpi_register_interface(ci-
> > > > > >dev,
> > > > > &ci->ulpi_ops);" (to initialize our phy), "hw_phymode_configure(ci);" is called which is the
> > > > > original function that make our system to hang.
> > > > >
> > > > > Our phy is not initialised before calling ulpi_register_interface so I don't understand how
> > > > > the
> > > > > phy
> > > > > can reply if it is not out of reset state.
> > > >
> > > > I haven't see any problem in hw_phymode_configure(). What's the
> > > > value of ci->platdata->phy_mode? USBPHY_INTERFACE_MODE_ULPI? If
> > > > you phy needs to be taken out of reset to reply to the ulpi reads
> > > > of the vendor/product ids, then it sounds like you have a similar
> > > > situation to what I had. I needed to turn on some regulators to
> > > > get those reads to work, otherwise they would fail, but knowing
> > > > what needed to be turned on basically meant I needed to probe the
> > > > ulpi driver so probing the ids wasn't going to be useful. So on
> > > > my device the reads for the ids go through, but they get all
> > > > zeroes back, which is actually ok because there aren't any bits
> > > > set on my devices anyway. After the reads see 0, we fallback to
> > > > DT matching, which avoids the "bring it out of reset/power it on"
> > > > sorts of problems entirely.
> > > >
> > >
> > > Yes the phy mode is configured to USBPHY_INTERFACE_MODE_ULPI.
> > > Indeed, this phy need to be out of reset to work. For example everything works fine if I call?
> > > "_ci_usb_phy_init(ci);" before calling "hw_phymode_configure(ci);"
> > > This function only init reset GPIO and clock.
> > >
> > > For information, the original patch I have to fix the issue:
> > >
> > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > > index 79ad8e9..21aaff1 100644
> > > --- a/drivers/usb/chipidea/core.c
> > > +++ b/drivers/usb/chipidea/core.c
> > > @@ -391,6 +391,7 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
> > > ? case USBPHY_INTERFACE_MODE_UTMI:
> > > ? case USBPHY_INTERFACE_MODE_UTMIW:
> > > ? case USBPHY_INTERFACE_MODE_HSIC:
> > > + case USBPHY_INTERFACE_MODE_ULPI:
> > > ? ret = _ci_usb_phy_init(ci);
> > > ? if (!ret)
> > > ? hw_wait_phy_stable();
> > > @@ -398,7 +399,6 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
> > > ? return ret;
> > > ? hw_phymode_configure(ci);
> > > ? break;
> > > - case USBPHY_INTERFACE_MODE_ULPI:
> > > ? case USBPHY_INTERFACE_MODE_SERIAL:
> > > ? hw_phymode_configure(ci);
> > > ? ret = _ci_usb_phy_init(ci);
> > > --?
> >
> > Currently, the hw_phymode_configure is called twice for ULPI PHY, the
> > two execution are between _ci_usb_phy_init, would you test which one
> > causes hang? If the second causes hang, you can make a patch for
> > hw_phymode_configure that if the required PORTSC_PTS is the same
> > the value in register, do noop.
>
> The first one hangs,?_ci_usb_phy_init is not called due to hang.
>
So, you need to comment out hw_phymode_configure at ci_ulpi_init, and you
can't get vid/pid correctly, right? If it is, we may need to add power on
sequence at chipidea core driver (ci_hdrc_probe) for clock and reset things.
http://www.spinics.net/lists/linux-usb/msg157134.html
I am wondering if we can call ci_usb_phy_init before calling ci_ulpi_init,
since we need to let hardware be ready before reading vid/pid.
Stephen & Fabien, does that work for you?
--
Best Regards,
Peter Chen
Hi Peter,
On Tue, 2017-06-06 at 09:55 +0800, Peter Chen wrote:
> On Mon, Jun 05, 2017 at 11:52:26AM +0200, Fabien Lahoudere wrote:
> > On Mon, 2017-06-05 at 17:43 +0800, Peter Chen wrote:
> > > On Mon, Jun 05, 2017 at 10:57:00AM +0200, Fabien Lahoudere wrote:
> > > > On Fri, 2017-06-02 at 15:00 -0700, Stephen Boyd wrote:
> > > > > On 05/26, Fabien Lahoudere wrote:
> > > > > > Hello
> > > > > >
> > > > > > I modify ci_hrdc_imx_probe to bypass "data->phy = devm_usb_get_phy_by_phandle(&pdev-
> > > > > > >dev,
> > > > > > "fsl,usbphy", 0);". Everything works as expected and call ci_ulpi_init.
> > > > > >
> > > > > > The problem is that in ci_ulpi_init, before calling "ci->ulpi =
> > > > > > ulpi_register_interface(ci-
> > > > > > > dev,
> > > > > >
> > > > > > &ci->ulpi_ops);" (to initialize our phy), "hw_phymode_configure(ci);" is called which is
> > > > > > the
> > > > > > original function that make our system to hang.
> > > > > >
> > > > > > Our phy is not initialised before calling ulpi_register_interface so I don't understand
> > > > > > how
> > > > > > the
> > > > > > phy
> > > > > > can reply if it is not out of reset state.
> > > > >
> > > > > I haven't see any problem in hw_phymode_configure(). What's the
> > > > > value of ci->platdata->phy_mode? USBPHY_INTERFACE_MODE_ULPI? If
> > > > > you phy needs to be taken out of reset to reply to the ulpi reads
> > > > > of the vendor/product ids, then it sounds like you have a similar
> > > > > situation to what I had. I needed to turn on some regulators to
> > > > > get those reads to work, otherwise they would fail, but knowing
> > > > > what needed to be turned on basically meant I needed to probe the
> > > > > ulpi driver so probing the ids wasn't going to be useful. So on
> > > > > my device the reads for the ids go through, but they get all
> > > > > zeroes back, which is actually ok because there aren't any bits
> > > > > set on my devices anyway. After the reads see 0, we fallback to
> > > > > DT matching, which avoids the "bring it out of reset/power it on"
> > > > > sorts of problems entirely.
> > > > >
> > > >
> > > > Yes the phy mode is configured to USBPHY_INTERFACE_MODE_ULPI.
> > > > Indeed, this phy need to be out of reset to work. For example everything works fine if I
> > > > call
> > > > "_ci_usb_phy_init(ci);" before calling "hw_phymode_configure(ci);"
> > > > This function only init reset GPIO and clock.
> > > >
> > > > For information, the original patch I have to fix the issue:
> > > >
> > > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > > > index 79ad8e9..21aaff1 100644
> > > > --- a/drivers/usb/chipidea/core.c
> > > > +++ b/drivers/usb/chipidea/core.c
> > > > @@ -391,6 +391,7 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
> > > > case USBPHY_INTERFACE_MODE_UTMI:
> > > > case USBPHY_INTERFACE_MODE_UTMIW:
> > > > case USBPHY_INTERFACE_MODE_HSIC:
> > > > + case USBPHY_INTERFACE_MODE_ULPI:
> > > > ret = _ci_usb_phy_init(ci);
> > > > if (!ret)
> > > > hw_wait_phy_stable();
> > > > @@ -398,7 +399,6 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
> > > > return ret;
> > > > hw_phymode_configure(ci);
> > > > break;
> > > > - case USBPHY_INTERFACE_MODE_ULPI:
> > > > case USBPHY_INTERFACE_MODE_SERIAL:
> > > > hw_phymode_configure(ci);
> > > > ret = _ci_usb_phy_init(ci);
> > > > --
> > >
> > > Currently, the hw_phymode_configure is called twice for ULPI PHY, the
> > > two execution are between _ci_usb_phy_init, would you test which one
> > > causes hang? If the second causes hang, you can make a patch for
> > > hw_phymode_configure that if the required PORTSC_PTS is the same
> > > the value in register, do noop.
> >
> > The first one hangs, _ci_usb_phy_init is not called due to hang.
> >
>
> So, you need to comment out hw_phymode_configure at ci_ulpi_init, and you
> can't get vid/pid correctly, right? If it is, we may need to add power on
> sequence at chipidea core driver (ci_hdrc_probe) for clock and reset things.
>
> http://www.spinics.net/lists/linux-usb/msg157134.html
>
> I am wondering if we can call ci_usb_phy_init before calling ci_ulpi_init,
> since we need to let hardware be ready before reading vid/pid.
> Stephen & Fabien, does that work for you?
>
I test the following patch and it works. But I am not sure that we can move safely ci_ulpi_init.
I will investigate more tomorrow if it is a problem for other phys.
Is it a good approach?
Subject: [PATCH 1/1] power on phy before getting vid/pid
Signed-off-by: Fabien Lahoudere <[email protected]>
---
drivers/usb/chipidea/core.c | 12 ++++++++----
drivers/usb/chipidea/ulpi.c | 19 +++++++++++++++++++
2 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 79ad8e9..26889e1 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -879,10 +879,6 @@ static int ci_hdrc_probe(struct platform_device *pdev)
return -ENODEV;
}
- ret = ci_ulpi_init(ci);
- if (ret)
- return ret;
-
if (ci->platdata->phy) {
ci->phy = ci->platdata->phy;
} else if (ci->platdata->usb_phy) {
@@ -909,6 +905,14 @@ static int ci_hdrc_probe(struct platform_device *pdev)
ci->usb_phy = NULL;
}
+ /*
+ We move this in order to have phy reset and gpio information
+ before calling ci_ulpi_init.
+ */
+ ret = ci_ulpi_init(ci);
+ if (ret)
+ return ret;
+
ret = ci_usb_phy_init(ci);
if (ret) {
dev_err(dev, "unable to init phy: %d\n", ret);
diff --git a/drivers/usb/chipidea/ulpi.c b/drivers/usb/chipidea/ulpi.c
index 1219583..1c272e4 100644
--- a/drivers/usb/chipidea/ulpi.c
+++ b/drivers/usb/chipidea/ulpi.c
@@ -73,9 +73,28 @@ static int ci_ulpi_write(struct device *dev, u8 addr, u8 val)
int ci_ulpi_init(struct ci_hdrc *ci)
{
+ int ret;
if (ci->platdata->phy_mode != USBPHY_INTERFACE_MODE_ULPI)
return 0;
+ /*
+ This is the content of _ci_usb_phy_init from core.c to power on the phy.
+ Duplicated for test purpose only.
+ */
+ if (ci->phy) {
+ ret = phy_init(ci->phy);
+ if (ret)
+ return ret;
+
+ ret = phy_power_on(ci->phy);
+ if (ret) {
+ phy_exit(ci->phy);
+ return ret;
+ }
+ } else {
+ ret = usb_phy_init(ci->usb_phy);
+ }
+
/*
* Set PORTSC correctly so we can read/write ULPI registers for
* identification purposes
--
1.8.3.1
Thanks
Fabien
On Tue, Jun 06, 2017 at 07:36:10PM +0200, Fabien Lahoudere wrote:
> Hi Peter,
>
> On Tue, 2017-06-06 at 09:55 +0800, Peter Chen wrote:
> > On Mon, Jun 05, 2017 at 11:52:26AM +0200, Fabien Lahoudere wrote:
> > > On Mon, 2017-06-05 at 17:43 +0800, Peter Chen wrote:
> > > > On Mon, Jun 05, 2017 at 10:57:00AM +0200, Fabien Lahoudere wrote:
> > > > > On Fri, 2017-06-02 at 15:00 -0700, Stephen Boyd wrote:
> > > > > > On 05/26, Fabien Lahoudere wrote:
> > > > > > > Hello
> > > > > > >
> > > > > > > I modify ci_hrdc_imx_probe to bypass?"data->phy = devm_usb_get_phy_by_phandle(&pdev-
> > > > > > > >dev,
> > > > > > > "fsl,usbphy", 0);". Everything works as expected and call ci_ulpi_init.
> > > > > > >
> > > > > > > The problem is that in ci_ulpi_init, before calling "ci->ulpi =
> > > > > > > ulpi_register_interface(ci-
> > > > > > > > dev,
> > > > > > >
> > > > > > > &ci->ulpi_ops);" (to initialize our phy), "hw_phymode_configure(ci);" is called which is
> > > > > > > the
> > > > > > > original function that make our system to hang.
> > > > > > >
> > > > > > > Our phy is not initialised before calling ulpi_register_interface so I don't understand
> > > > > > > how
> > > > > > > the
> > > > > > > phy
> > > > > > > can reply if it is not out of reset state.
> > > > > >
> > > > > > I haven't see any problem in hw_phymode_configure(). What's the
> > > > > > value of ci->platdata->phy_mode? USBPHY_INTERFACE_MODE_ULPI? If
> > > > > > you phy needs to be taken out of reset to reply to the ulpi reads
> > > > > > of the vendor/product ids, then it sounds like you have a similar
> > > > > > situation to what I had. I needed to turn on some regulators to
> > > > > > get those reads to work, otherwise they would fail, but knowing
> > > > > > what needed to be turned on basically meant I needed to probe the
> > > > > > ulpi driver so probing the ids wasn't going to be useful. So on
> > > > > > my device the reads for the ids go through, but they get all
> > > > > > zeroes back, which is actually ok because there aren't any bits
> > > > > > set on my devices anyway. After the reads see 0, we fallback to
> > > > > > DT matching, which avoids the "bring it out of reset/power it on"
> > > > > > sorts of problems entirely.
> > > > > >
> > > > >
> > > > > Yes the phy mode is configured to USBPHY_INTERFACE_MODE_ULPI.
> > > > > Indeed, this phy need to be out of reset to work. For example everything works fine if I
> > > > > call?
> > > > > "_ci_usb_phy_init(ci);" before calling "hw_phymode_configure(ci);"
> > > > > This function only init reset GPIO and clock.
> > > > >
> > > > > For information, the original patch I have to fix the issue:
> > > > >
> > > > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > > > > index 79ad8e9..21aaff1 100644
> > > > > --- a/drivers/usb/chipidea/core.c
> > > > > +++ b/drivers/usb/chipidea/core.c
> > > > > @@ -391,6 +391,7 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
> > > > > ? case USBPHY_INTERFACE_MODE_UTMI:
> > > > > ? case USBPHY_INTERFACE_MODE_UTMIW:
> > > > > ? case USBPHY_INTERFACE_MODE_HSIC:
> > > > > + case USBPHY_INTERFACE_MODE_ULPI:
> > > > > ? ret = _ci_usb_phy_init(ci);
> > > > > ? if (!ret)
> > > > > ? hw_wait_phy_stable();
> > > > > @@ -398,7 +399,6 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
> > > > > ? return ret;
> > > > > ? hw_phymode_configure(ci);
> > > > > ? break;
> > > > > - case USBPHY_INTERFACE_MODE_ULPI:
> > > > > ? case USBPHY_INTERFACE_MODE_SERIAL:
> > > > > ? hw_phymode_configure(ci);
> > > > > ? ret = _ci_usb_phy_init(ci);
> > > > > --?
> > > >
> > > > Currently, the hw_phymode_configure is called twice for ULPI PHY, the
> > > > two execution are between _ci_usb_phy_init, would you test which one
> > > > causes hang? If the second causes hang, you can make a patch for
> > > > hw_phymode_configure that if the required PORTSC_PTS is the same
> > > > the value in register, do noop.
> > >
> > > The first one hangs,?_ci_usb_phy_init is not called due to hang.
> > >
> >
> > So, you need to comment out hw_phymode_configure at ci_ulpi_init, and you
> > can't get vid/pid correctly, right? If it is, we may need to add power on
> > sequence at chipidea core driver (ci_hdrc_probe) for clock and reset things.
> >
> > http://www.spinics.net/lists/linux-usb/msg157134.html
> >
> > I am wondering if we can call ci_usb_phy_init before calling ci_ulpi_init,
> > since we need to let hardware be ready before reading vid/pid.
> > Stephen & Fabien, does that work for you?
> >
>
> I test the following patch and it works. But I am not sure that we can move safely ci_ulpi_init.
> I will investigate more tomorrow if it is a problem for other phys.
>
> Is it a good approach?
>
> Subject: [PATCH 1/1] power on phy before getting vid/pid
>
> Signed-off-by: Fabien Lahoudere <[email protected]>
> ---
> ?drivers/usb/chipidea/core.c | 12 ++++++++----
> ?drivers/usb/chipidea/ulpi.c | 19 +++++++++++++++++++
> ?2 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 79ad8e9..26889e1 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -879,10 +879,6 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> ? return -ENODEV;
> ? }
> ?
> - ret = ci_ulpi_init(ci);
> - if (ret)
> - return ret;
> -
> ? if (ci->platdata->phy) {
> ? ci->phy = ci->platdata->phy;
> ? } else if (ci->platdata->usb_phy) {
> @@ -909,6 +905,14 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> ? ci->usb_phy = NULL;
> ? }
> ?
> + /*?
> + ???We move this in order to have phy reset and gpio information
> + ???before calling ci_ulpi_init.
> + */
> + ret = ci_ulpi_init(ci);
> + if (ret)
> + return ret;
> +
> ? ret = ci_usb_phy_init(ci);
> ? if (ret) {
> ? dev_err(dev, "unable to init phy: %d\n", ret);
> diff --git a/drivers/usb/chipidea/ulpi.c b/drivers/usb/chipidea/ulpi.c
> index 1219583..1c272e4 100644
> --- a/drivers/usb/chipidea/ulpi.c
> +++ b/drivers/usb/chipidea/ulpi.c
> @@ -73,9 +73,28 @@ static int ci_ulpi_write(struct device *dev, u8 addr, u8 val)
> ?
> ?int ci_ulpi_init(struct ci_hdrc *ci)
> ?{
> +????????int ret;
> ? if (ci->platdata->phy_mode != USBPHY_INTERFACE_MODE_ULPI)
> ? return 0;
> ?
> + /*?
> + ???This is the content of _ci_usb_phy_init from core.c to power on the phy.
> + ???Duplicated for test purpose only.
> + */
> + if (ci->phy) {
> + ret = phy_init(ci->phy);
> + if (ret)
> + return ret;
> +
> + ret = phy_power_on(ci->phy);
> + if (ret) {
> + phy_exit(ci->phy);
> + return ret;
> + }
> + } else {
> + ret = usb_phy_init(ci->usb_phy);
> + }
> +
> ? /*
> ? ?* Set PORTSC correctly so we can read/write ULPI registers for
> ? ?* identification purposes
Your above patch is not accepted. I don't know ULPI PHY behavior at
imx53, would you please make clear below things:
- Before setting phy mode at portsc, which you need to do?
If they can't be in phy_init, you may register a power sequence
instance.
- Can you read pid/vid correctly, and which way you would
like to match your ulpi device, pid/vid or using device
tree, see ulpi_match
--
Best Regards,
Peter Chen
On Wed, 2017-06-07 at 09:43 +0800, Peter Chen wrote:
> On Tue, Jun 06, 2017 at 07:36:10PM +0200, Fabien Lahoudere wrote:
> > Hi Peter,
> >
> > On Tue, 2017-06-06 at 09:55 +0800, Peter Chen wrote:
> > > On Mon, Jun 05, 2017 at 11:52:26AM +0200, Fabien Lahoudere wrote:
> > > > On Mon, 2017-06-05 at 17:43 +0800, Peter Chen wrote:
> > > > > On Mon, Jun 05, 2017 at 10:57:00AM +0200, Fabien Lahoudere wrote:
> > > > > > On Fri, 2017-06-02 at 15:00 -0700, Stephen Boyd wrote:
> > > > > > > On 05/26, Fabien Lahoudere wrote:
> > > > > > > > Hello
> > > > > > > >
> > > > > > > > I modify ci_hrdc_imx_probe to bypass "data->phy = devm_usb_get_phy_by_phandle(&pdev-
> > > > > > > > > dev,
> > > > > > > >
> > > > > > > > "fsl,usbphy", 0);". Everything works as expected and call ci_ulpi_init.
> > > > > > > >
> > > > > > > > The problem is that in ci_ulpi_init, before calling "ci->ulpi =
> > > > > > > > ulpi_register_interface(ci-
> > > > > > > > > dev,
> > > > > > > >
> > > > > > > > &ci->ulpi_ops);" (to initialize our phy), "hw_phymode_configure(ci);" is called
> > > > > > > > which is
> > > > > > > > the
> > > > > > > > original function that make our system to hang.
> > > > > > > >
> > > > > > > > Our phy is not initialised before calling ulpi_register_interface so I don't
> > > > > > > > understand
> > > > > > > > how
> > > > > > > > the
> > > > > > > > phy
> > > > > > > > can reply if it is not out of reset state.
> > > > > > >
> > > > > > > I haven't see any problem in hw_phymode_configure(). What's the
> > > > > > > value of ci->platdata->phy_mode? USBPHY_INTERFACE_MODE_ULPI? If
> > > > > > > you phy needs to be taken out of reset to reply to the ulpi reads
> > > > > > > of the vendor/product ids, then it sounds like you have a similar
> > > > > > > situation to what I had. I needed to turn on some regulators to
> > > > > > > get those reads to work, otherwise they would fail, but knowing
> > > > > > > what needed to be turned on basically meant I needed to probe the
> > > > > > > ulpi driver so probing the ids wasn't going to be useful. So on
> > > > > > > my device the reads for the ids go through, but they get all
> > > > > > > zeroes back, which is actually ok because there aren't any bits
> > > > > > > set on my devices anyway. After the reads see 0, we fallback to
> > > > > > > DT matching, which avoids the "bring it out of reset/power it on"
> > > > > > > sorts of problems entirely.
> > > > > > >
> > > > > >
> > > > > > Yes the phy mode is configured to USBPHY_INTERFACE_MODE_ULPI.
> > > > > > Indeed, this phy need to be out of reset to work. For example everything works fine if I
> > > > > > call
> > > > > > "_ci_usb_phy_init(ci);" before calling "hw_phymode_configure(ci);"
> > > > > > This function only init reset GPIO and clock.
> > > > > >
> > > > > > For information, the original patch I have to fix the issue:
> > > > > >
> > > > > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > > > > > index 79ad8e9..21aaff1 100644
> > > > > > --- a/drivers/usb/chipidea/core.c
> > > > > > +++ b/drivers/usb/chipidea/core.c
> > > > > > @@ -391,6 +391,7 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
> > > > > > case USBPHY_INTERFACE_MODE_UTMI:
> > > > > > case USBPHY_INTERFACE_MODE_UTMIW:
> > > > > > case USBPHY_INTERFACE_MODE_HSIC:
> > > > > > + case USBPHY_INTERFACE_MODE_ULPI:
> > > > > > ret = _ci_usb_phy_init(ci);
> > > > > > if (!ret)
> > > > > > hw_wait_phy_stable();
> > > > > > @@ -398,7 +399,6 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
> > > > > > return ret;
> > > > > > hw_phymode_configure(ci);
> > > > > > break;
> > > > > > - case USBPHY_INTERFACE_MODE_ULPI:
> > > > > > case USBPHY_INTERFACE_MODE_SERIAL:
> > > > > > hw_phymode_configure(ci);
> > > > > > ret = _ci_usb_phy_init(ci);
> > > > > > --
> > > > >
> > > > > Currently, the hw_phymode_configure is called twice for ULPI PHY, the
> > > > > two execution are between _ci_usb_phy_init, would you test which one
> > > > > causes hang? If the second causes hang, you can make a patch for
> > > > > hw_phymode_configure that if the required PORTSC_PTS is the same
> > > > > the value in register, do noop.
> > > >
> > > > The first one hangs, _ci_usb_phy_init is not called due to hang.
> > > >
> > >
> > > So, you need to comment out hw_phymode_configure at ci_ulpi_init, and you
> > > can't get vid/pid correctly, right? If it is, we may need to add power on
> > > sequence at chipidea core driver (ci_hdrc_probe) for clock and reset things.
> > >
> > > http://www.spinics.net/lists/linux-usb/msg157134.html
> > >
> > > I am wondering if we can call ci_usb_phy_init before calling ci_ulpi_init,
> > > since we need to let hardware be ready before reading vid/pid.
> > > Stephen & Fabien, does that work for you?
> > >
> >
> > I test the following patch and it works. But I am not sure that we can move safely ci_ulpi_init.
> > I will investigate more tomorrow if it is a problem for other phys.
> >
> > Is it a good approach?
> >
> > Subject: [PATCH 1/1] power on phy before getting vid/pid
> >
> > Signed-off-by: Fabien Lahoudere <[email protected]>
> > ---
> > drivers/usb/chipidea/core.c | 12 ++++++++----
> > drivers/usb/chipidea/ulpi.c | 19 +++++++++++++++++++
> > 2 files changed, 27 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index 79ad8e9..26889e1 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -879,10 +879,6 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > return -ENODEV;
> > }
> >
> > - ret = ci_ulpi_init(ci);
> > - if (ret)
> > - return ret;
> > -
> > if (ci->platdata->phy) {
> > ci->phy = ci->platdata->phy;
> > } else if (ci->platdata->usb_phy) {
> > @@ -909,6 +905,14 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > ci->usb_phy = NULL;
> > }
> >
> > + /*
> > + We move this in order to have phy reset and gpio information
> > + before calling ci_ulpi_init.
> > + */
> > + ret = ci_ulpi_init(ci);
> > + if (ret)
> > + return ret;
> > +
> > ret = ci_usb_phy_init(ci);
> > if (ret) {
> > dev_err(dev, "unable to init phy: %d\n", ret);
> > diff --git a/drivers/usb/chipidea/ulpi.c b/drivers/usb/chipidea/ulpi.c
> > index 1219583..1c272e4 100644
> > --- a/drivers/usb/chipidea/ulpi.c
> > +++ b/drivers/usb/chipidea/ulpi.c
> > @@ -73,9 +73,28 @@ static int ci_ulpi_write(struct device *dev, u8 addr, u8 val)
> >
> > int ci_ulpi_init(struct ci_hdrc *ci)
> > {
> > + int ret;
> > if (ci->platdata->phy_mode != USBPHY_INTERFACE_MODE_ULPI)
> > return 0;
> >
> > + /*
> > + This is the content of _ci_usb_phy_init from core.c to power on the phy.
> > + Duplicated for test purpose only.
> > + */
> > + if (ci->phy) {
> > + ret = phy_init(ci->phy);
> > + if (ret)
> > + return ret;
> > +
> > + ret = phy_power_on(ci->phy);
> > + if (ret) {
> > + phy_exit(ci->phy);
> > + return ret;
> > + }
> > + } else {
> > + ret = usb_phy_init(ci->usb_phy);
> > + }
> > +
> > /*
> > * Set PORTSC correctly so we can read/write ULPI registers for
> > * identification purposes
>
> Your above patch is not accepted. I don't know ULPI PHY behavior at
> imx53, would you please make clear below things:
>
> - Before setting phy mode at portsc, which you need to do?
I need to prepare clock and enable vcc.
Calling "usb_phy_init" before "ci_ulpi_init" works.
In my case usb_phy_init points to usb_gen_phy_init() from phy-generic.c which basically do:
regulator_enable(nop->vcc);
clk_prepare_enable(nop->clk);
nop_reset(nop);
> If they can't be in phy_init, you may register a power sequence
> instance.
How can I register a power sequence? by adding clock in a subnode of usbh2 node?
> - Can you read pid/vid correctly, and which way you would
> like to match your ulpi device, pid/vid or using device
> tree, see ulpi_match
Yes I can read pid/vid (ulpi ci_hdrc.2.ulpi: registered ULPI PHY: vendor 0424, product 0006)
I am not sure which way I prefer. The better seems to be pid/vid.
>
Hi Peter
I have a look at your patches (http://www.spinics.net/lists/linux-usb/msg157134.html) and I wonder
if power sequence is applicable only on hub node? hub are probed too late (after ci_ulpi_init). Do
you think it is possible to read a power sequence in dts from ci_ulpi_init?
Thanks
Fabien
>
>I have a look at your patches (http://www.spinics.net/lists/linux-usb/msg157134.html)
>and I wonder if power sequence is applicable only on hub node?
No, power sequence can be used for any devices which needs to carry out power on
before probe.
> hub are probed too
>late (after ci_ulpi_init). Do you think it is possible to read a power sequence in dts
>from ci_ulpi_init?
>
I think the suitable place for power sequence is at ulpi_of_register, some ULPI PHYs
need to carry out power on before reading ID.
Peter
Hi Peter
On Fri, 2017-06-09 at 08:26 +0000, Peter Chen wrote:
>
> >
> > I have a look at your patches (http://www.spinics.net/lists/linux-usb/msg157134.html)
> > and I wonder if power sequence is applicable only on hub node?
>
> No, power sequence can be used for any devices which needs to carry out power on
> before probe.
>
> > hub are probed too
> > late (after ci_ulpi_init). Do you think it is possible to read a power sequence in dts
> > from ci_ulpi_init?
> >
>
> I think the suitable place for power sequence is at ulpi_of_register, some ULPI PHYs
> need to carry out power on before reading ID.
>
I do some test to verify that ulpi_of_register is called before hw_phymode_configure but it is not
the case.
Can you confirm that ULPI phys works with IMX6?
It seems that IMX53 and IMX6 use the same chipidea controller and should have the same behaviour. So
I wonder if the issue I encounter can be a SOC issue.
Thanks
> Peter
>
>Can you confirm that ULPI phys works with IMX6?
>
>It seems that IMX53 and IMX6 use the same chipidea controller and should have the
>same behaviour. So I wonder if the issue I encounter can be a SOC issue.
>
Fabien, all imx6 series uses UTMI PHYs, so I am afraid I can't verify it.
Peter