On Fri, May 2, 2014 at 2:55 PM, Vivek Gautam <gautam.vivek@xxxxxxxxxxx> wrote:
> Hi Tomasz,
>
>
> On Thu, May 1, 2014 at 10:46 PM, Tomasz Figa <tomasz.figa@xxxxxxxxx> wrote:
>> Hi Vivek,
>>
>> Please see my comments inline.
>
> Thanks for the review, please find my answers inline.
>
>>
>>
>> On 30.04.2014 07:19, Vivek Gautam wrote:
>>>
>>> Add support to consume phy provided by Generic phy framework.
>>> Keeping the support for older usb-phy intact right now, in order
>>> to prevent any functionality break in absence of relevant
>>> device tree side change for ohci-exynos.
>>> Once we move to new phy in the device nodes for ohci, we can
>>> remove the support for older phys.
>>>
>>> Signed-off-by: Vivek Gautam <gautam.vivek@xxxxxxxxxxx>
>>> Cc: Jingoo Han <jg1.han@xxxxxxxxxxx>
>>> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
>>> ---
>>>
>>> Changes from v3:
>>> - Calling usb_phy_shutdown() when exynos_ohci_phy_enable() is failing.
>>> - Made exynos_ohci_phy_disable() return void, since its return value
>>> did not serve any purpose.
>>> - Calling clk_disable_unprepare() in exynos_ohci_resume() when
>>> exynos_ohci_phy_enable() is failed.
>>>
>>> .../devicetree/bindings/usb/exynos-usb.txt | 19 +++
>>> drivers/usb/host/ohci-exynos.c | 128
>>> +++++++++++++++++---
>>> 2 files changed, 132 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt
>>> b/Documentation/devicetree/bindings/usb/exynos-usb.txt
>>> index d967ba1..a90c973 100644
>>> --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
>>> +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
>>> @@ -38,6 +38,15 @@ Required properties:
>>> - interrupts: interrupt number to the cpu.
>>> - clocks: from common clock binding: handle to usb clock.
>>> - clock-names: from common clock binding: Shall be "usbhost".
>>> + - port: if in the SoC there are OHCI phys, they should be listed here.
>>> + One phy per port. Each port should have following entries:
>>> + - reg: port number on OHCI controller, e.g
>>> + On Exynos5250, port 0 is USB2.0 otg phy
>>> + port 1 is HSIC phy0
>>> + port 2 is HSIC phy1
>>> + - phys: from the *Generic PHY* bindings, specifying phy used by
>>> port.
>>> + - phy-names: from the *Generic PHY* bindings, specifying name of
>>> phy
>>> + used by the port.
>>
>>
>> I think you don't need this property for this binding, as the PHYs are being
>> requested by indices (in fact only index 0 is used).
>
> True, that phy-names property is not used, since PHYs are being
> requested using indices.
> We can remove this.
>
>>
>>
>>>
>>> Example:
>>> usb@12120000 {
>>> @@ -47,6 +56,16 @@ Example:
>>>
>>> clocks = <&clock 285>;
>>> clock-names = "usbhost";
>>> +
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + port@0 {
>>> + reg = <0>;
>>> + phys = <&usb2phy 1>;
>>> + phy-names = "host";
>>
>>
>> Ditto.
> will remove this.
>
>>
>>
>>> + status = "disabled";
>>> + };
>>> +
>>> };
>>>
>>> DWC3
>>> diff --git a/drivers/usb/host/ohci-exynos.c
>>> b/drivers/usb/host/ohci-exynos.c
>>> index 05f00e3..f90bf9a 100644
>>> --- a/drivers/usb/host/ohci-exynos.c
>>> +++ b/drivers/usb/host/ohci-exynos.c
>>> @@ -18,6 +18,7 @@
>>> #include <linux/module.h>
>>> #include <linux/of.h>
>>> #include <linux/platform_device.h>
>>> +#include <linux/phy/phy.h>
>>> #include <linux/usb/phy.h>
>>> #include <linux/usb/samsung_usb_phy.h>
>>> #include <linux/usb.h>
>>> @@ -33,28 +34,122 @@ static struct hc_driver __read_mostly
>>> exynos_ohci_hc_driver;
>>>
>>> #define to_exynos_ohci(hcd) (struct exynos_ohci_hcd
>>> *)(hcd_to_ohci(hcd)->priv)
>>>
>>> +#define PHY_NUMBER 3
>>
>>
>> nit: A blank line would be nice here to separate from the struct below.
>
> Ok, will add one.
>
>>
>> By the way, doesn't the number of PHY ports depend on platform? Of course
>> right now the driver supports only Exynos >= 4210 SoCs with the generic PHY
>> interface, so it might be changed in further patches.
>
> Yes, the number of PHY ports will be platform dependent feature.
> In subsequent patches we can add support to count the number of PHYs,
> or rather in this patch
> itself, when we do -
> for_each_available_child_of_node(dev->of_node, child) {
> we can keep a count of how many child nodes we found, and then
> configure those many.
> As such the host controller drivers will have 'host' and 'hsic' PHYs.
>
>>
>>
>>> struct exynos_ohci_hcd {
>>> struct clk *clk;
>>> struct usb_phy *phy;
>>> struct usb_otg *otg;
>>> + struct phy *phy_g[PHY_NUMBER];
>>> };
>>>
>>> -static void exynos_ohci_phy_enable(struct device *dev)
>>> +static int exynos_ohci_get_phy(struct device *dev,
>>> + struct exynos_ohci_hcd *exynos_ohci)
>>> +{
>>> + struct device_node *child;
>>> + struct phy *phy;
>>> + int phy_number;
>>> + int ret = 0;
>>> +
>>> + exynos_ohci->phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
>>> + if (IS_ERR(exynos_ohci->phy)) {
>>> + ret = PTR_ERR(exynos_ohci->phy);
>>> + /* This is the case when PHY config is disabled */
>>> + if (ret == -ENXIO || ret == -ENODEV) {
>>> + dev_dbg(dev, "Failed to get usb2 phy\n");
>>> + exynos_ohci->phy = NULL;
>>
>>
>> I think you should keep this an ERR_PTR() and just use IS_ERR() check
>> further in the driver instead of checking for NULL.
>
> Yea, that's also one way to check for phy, i can modify this.
>
>>
>>> + ret = 0;
>>
>>
>> Do you need to set ret to 0 here? The code for getting generic PHYs will
>> either leave it unchanged when there are no port nodes defined or overwrite
>> it with value returned by of_property_read_u32(). In the first case, an
>> error code should be returned, not zero, as the driver was unable to get any
>> PHY.
>
> The idea was to not fail exynos_ohci_get_phy() call when the PHY
> configs are not even enabled.
> Since this would mean, that the driver will never be able to get a PHY
> in future, and there will be no point in
> failing the driver probe.
>
> In a case when the host controller dts is still on older PHY bindings,
> this 'ret' will *not* be over-wriiten, since the
> generic phy nodes will not be there.
> And in case the host dts is moved to the new generic PHY based
> bindings. then the part of this function exynos_ohci_get_phy()
> related to older usb-phy, shall not execute.
>
> This is reason why i did not really add a fall-back mechanism for getting PHY.
> Since from DT either of the two bindings be supplied, and then things
> here will be handles almost independently.
>
>>
>>
>>> + } else if (ret == -EPROBE_DEFER) {
>>
>>
>> I think you could merge this case with the else clause below, as most driver
>> do. Moreover, since the only thing done after the fail_phy label is
>> returning the error code, you could just immediately return from here. So
>> the whole block of code would end up like this:
>>
>> if (IS_ERR(exynos_ohci->phy)) {
>> ret = PTR_ERR(exynos_ohci->phy);
>> if (ret != -ENXIO && ret != -ENODEV) {
>>
>> dev_err(dev, "no usb2 phy configured\n");
>> return ret;
>>
>> }
>> dev_dbg(dev, "Failed to get usb2 phy\n");
>> exynos_ohci->phy = NULL;
>> } else {
>>
>> exynos_ohci->otg = exynos_ohci->phy->otg;
>> }
>
> Ok, this seems a good restructuring. I shall change this.
>
>>
>>> + goto fail_phy;
>>>
>>> + } else {
>>> + dev_err(dev, "no usb2 phy configured\n");
>>> + goto fail_phy;
>>> + }
>>> + } else {
>>> + exynos_ohci->otg = exynos_ohci->phy->otg;
>>> + }
>>> +
>>> + /* Getting generic phy:
>>
>>
>> CodingStyle: Multi-line comments should begin and end with a single empty
>> line:
>>
>> /*
>> * Getting generic phy:
>> * ...
>> */
>>
>> also nit: s/phy/PHY/
>>
>
> Ok, will correct this.
>
>>
>>> + * We are keeping both types of phys as a part of transiting OHCI
>>> + * to generic phy framework, so that in absence of supporting dts
>>> + * changes the functionality doesn't break.
>>> + * Once we move the ohci dt nodes to use new generic phys,
>>> + * we can remove support for older PHY in this driver.
>>
>>
>> Well, this is not entirely true. The problem here is not caused by existing
>> DTS files, but rather a chance that there are existing devices using DTB
>> files built from them. So to remove the support for old bindings, we need to
>> make sure that such devices have their DTBs updated to ones built from new
>> DTS.
>
> Fair enough, thanks for explaining. :-)
> I shall modify this comment.
>
>>
>>
>>> + */
>>> + for_each_available_child_of_node(dev->of_node, child) {
>>> + ret = of_property_read_u32(child, "reg", &phy_number);
>>> + if (ret) {
>>> + dev_err(dev, "Failed to parse device tree\n");
>>> + of_node_put(child);
>>> + goto fail_phy;
>>
>>
>> Why not just return ret here?
>>
>>> + }
>>
>>
>> nit: Blank line here would be nice.
>
> ok
>
>>
>>
>>> + if (phy_number >= PHY_NUMBER) {
>>> + dev_err(dev, "Invalid number of PHYs\n");
>>> + of_node_put(child);
>>> + ret = -EINVAL;
>>> + goto fail_phy;
>>
>>
>> What about just return -EINVAL;
>
> Yea, just another way of doing things. ;-)
> I felt 'goto' to be a bit clean than adding number of returns in
> between statements.
>
>>
>>> + }
>>
>>
>> nit: Here too.
> yea, will add a blank line.
>
>>
>>> + phy = devm_of_phy_get(dev, child, 0);
>>> + of_node_put(child);
>>> + if (IS_ERR(phy)) {
>>> + ret = PTR_ERR(phy);
>>> + /* This is the case when PHY config is disabled */
>>> + if (ret == -ENOSYS || ret == -ENODEV) {
>>> + dev_dbg(dev, "Failed to get usb2 phy\n");
>>> + phy = NULL;
>>>
>>> + ret = 0;
>>> + } else if (ret == -EPROBE_DEFER) {
>>> + goto fail_phy;
>>> + } else {
>>> + dev_err(dev, "no usb2 phy configured\n");
>>> + goto fail_phy;
>>> + }
>>> + }
>>
>>
>> Similar comments to this block apply as for the block getting legacy USB
>> PHY.
>
> Ok, i will restructure this same as 'legacy PHY block'
>
>>
>>
>>> + exynos_ohci->phy_g[phy_number] = phy;
>>> + }
>>> +
>>> +fail_phy:
>>> + return ret;
>>> +}
>>> +
>>> +static int exynos_ohci_phy_enable(struct device *dev)
>>> {
>>> struct usb_hcd *hcd = dev_get_drvdata(dev);
>>> struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
>>> + int i;
>>> + int ret = 0;
>>>
>>> - if (exynos_ohci->phy)
>>> - usb_phy_init(exynos_ohci->phy);
>>> + if (exynos_ohci->phy) {
>>
>>
>> !IS_ERR() should be used to check for validity.
>
> Ok, this with the earlier change of setting exynos_ohci->phy as
> ERR_PTR(), should be good then.
>
>>
>>
>>> + ret = usb_phy_init(exynos_ohci->phy);
>>> + if (ret)
>>> + return ret;
>>
>>
>> IMHO a simple return usb_phy_init(...) could be used here, if we are using
>> the legacy PHY interface.
>
> Right, with legacy PHY, just a 'return usb_phy_init(...)' should do;
> since those devices will not have the generic
> PHYs.
>
>>
>>
>>> + }
>>> +
>>> + for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
>>> + if (exynos_ohci->phy_g[i])
>>
>>
>> !IS_ERR(). Just make sure that the array is initialized with an ERR_PTR()
>> with some error code, (-ENODEV) probably
>
> Ok.
I think we don't need to initialize this array to any ERR_PTR(), since
devm_of_phy_get()
will anyways assign an ERR_PTR() to phy in unsuccessful case, and a
valid phy pointer
when it successfully gets one.
Isn't it ?
I think the same goes with legacy usb-phy function devm_usb_get_phy().
[snip]
--
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
