| Message ID | 1424200821-16368-2-git-send-email-arun.ramamurthy@broadcom.com |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
On Tuesday 17 February 2015 11:20:20 Arun Ramamurthy wrote: > + /* This nodes declares port 0 > + and port 1 as host*/ > + > + ehci0: usb@0x18048000 { > + compatible = "generic-ehci"; > + reg = <0x18048000 0x100>; > + interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>; > + phys = <&usbphy0 0 1 &usbphy0 1>; The second reference in the example is missing the last cell, as you have #phy-cells = <2>. > + phy-names = "usbp0","usbp1"; > + status = "okay"; > + }; Further, the binding for "generic-ehci" requires the name to be "usb", not "usbp0". You should probably update that binding if you can have multiple references to mention that. Can you make both names "usb"? If not, we should document a common naming scheme that the driver can use. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thank you for the review Arnd. On 15-02-17 11:41 AM, Arnd Bergmann wrote: > On Tuesday 17 February 2015 11:20:20 Arun Ramamurthy wrote: >> + /* This nodes declares port 0 >> + and port 1 as host*/ >> + >> + ehci0: usb@0x18048000 { >> + compatible = "generic-ehci"; >> + reg = <0x18048000 0x100>; >> + interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>; >> + phys = <&usbphy0 0 1 &usbphy0 1>; > > The second reference in the example is missing the last cell, > as you have #phy-cells = <2>. > I am missing a 1 for the port number, will update in next patchset, thank you >> + phy-names = "usbp0","usbp1"; >> + status = "okay"; >> + }; > > Further, the binding for "generic-ehci" requires the name to be "usb", > not "usbp0". You should probably update that binding if you can have > multiple references to mention that. > > Can you make both names "usb"? If not, we should document a common > naming scheme that the driver can use. > Arnd, I patched the ehci and ohci driver to accept multiple phys so they require different names and cannot both be "usb". That patch was accepted by Alen Stern but I did not update the bindings documentation. I will send out another patch for that. Could we go with the naming scheme of "usb" + "p" + port number or do you have other suggestions? > Arnd > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 17 February 2015 12:00:49 Arun Ramamurthy wrote: > Arnd, I patched the ehci and ohci driver to accept multiple phys so they > require different names and cannot both be "usb". That patch was > accepted by Alen Stern but I did not update the bindings documentation. > I will send out another patch for that. Could we go with the naming > scheme of "usb" + "p" + port number or do you have other suggestions? I don't have a good idea, but I think it would be best if the first phy could remain named "usb" for compatibility with the existing binding. What is the reason for having two phys in your case? Are these identical phy devices connected to a single controller or do they server different purposes? Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15-02-17 12:53 PM, Arnd Bergmann wrote: > On Tuesday 17 February 2015 12:00:49 Arun Ramamurthy wrote: >> Arnd, I patched the ehci and ohci driver to accept multiple phys so they >> require different names and cannot both be "usb". That patch was >> accepted by Alen Stern but I did not update the bindings documentation. >> I will send out another patch for that. Could we go with the naming >> scheme of "usb" + "p" + port number or do you have other suggestions? > > I don't have a good idea, but I think it would be best if the first > phy could remain named "usb" for compatibility with the existing binding. > The patch was written in a way that all the existing and new drivers can continue to use "usb" if they are using only one phy so that we remain compatible. The names need to be different only if more than one phy is specified. In such cases i don't think the first phy should be "usb" as it would be confusing to have phy-names = "usb","usbp1" Should I run this by Alan Stern? > What is the reason for having two phys in your case? Are these > identical phy devices connected to a single controller or do they > server different purposes? > Yes, we have three identical phys connected to a single host controller and one of the phys is also connected to the device controller > Arnd > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 17 February 2015 13:05:50 Arun Ramamurthy wrote: > On 15-02-17 12:53 PM, Arnd Bergmann wrote: > > On Tuesday 17 February 2015 12:00:49 Arun Ramamurthy wrote: > >> Arnd, I patched the ehci and ohci driver to accept multiple phys so they > >> require different names and cannot both be "usb". That patch was > >> accepted by Alen Stern but I did not update the bindings documentation. > >> I will send out another patch for that. Could we go with the naming > >> scheme of "usb" + "p" + port number or do you have other suggestions? > > > > I don't have a good idea, but I think it would be best if the first > > phy could remain named "usb" for compatibility with the existing binding. > > > The patch was written in a way that all the existing and new drivers can > continue to use "usb" if they are using only one phy so that we remain > compatible. The names need to be different only if more than one phy is > specified. In such cases i don't think the first phy should be "usb" as > it would be confusing to have > phy-names = "usb","usbp1" I see your patch now, as 7e7a0e67f2c ("usb: ehci-platform: add support for multiple phys per controller"), and I'm not too happy about the way you did this. We already concluded that there should have been a binding change to go along with this, and that would have caught the fact that you circumvent the API here by reading the phy names manually. That part should never have made it into the kernel. I think we can do this either by defining specific names for the phy, or by changing the generic PHY binding to allow anonymous phy references (leaving out "phy-names" entirely), and adding a proper API for that. > Should I run this by Alan Stern? I've added him to Cc here. He clearly didn't know the background about the DT binding change, and should not need to, but he may have an opinion on what names we should use. > > What is the reason for having two phys in your case? Are these > > identical phy devices connected to a single controller or do they > > server different purposes? > > > Yes, we have three identical phys connected to a single host controller > and one of the phys is also connected to the device controller Ok, no problem with that, let's just make sure we come up with a good binding for it. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15-02-18 07:15 AM, Arnd Bergmann wrote: > On Tuesday 17 February 2015 13:05:50 Arun Ramamurthy wrote: >> On 15-02-17 12:53 PM, Arnd Bergmann wrote: >>> On Tuesday 17 February 2015 12:00:49 Arun Ramamurthy wrote: >>>> Arnd, I patched the ehci and ohci driver to accept multiple phys so they >>>> require different names and cannot both be "usb". That patch was >>>> accepted by Alen Stern but I did not update the bindings documentation. >>>> I will send out another patch for that. Could we go with the naming >>>> scheme of "usb" + "p" + port number or do you have other suggestions? >>> >>> I don't have a good idea, but I think it would be best if the first >>> phy could remain named "usb" for compatibility with the existing binding. >>> >> The patch was written in a way that all the existing and new drivers can >> continue to use "usb" if they are using only one phy so that we remain >> compatible. The names need to be different only if more than one phy is >> specified. In such cases i don't think the first phy should be "usb" as >> it would be confusing to have >> phy-names = "usb","usbp1" > > I see your patch now, as 7e7a0e67f2c ("usb: ehci-platform: add support for > multiple phys per controller"), and I'm not too happy about the way you > did this. > We already concluded that there should have been a binding change > to go along with this, and that would have caught the fact that you > circumvent the API here by reading the phy names manually. That > part should never have made it into the kernel. > > I think we can do this either by defining specific names for the > phy, or by changing the generic PHY binding to allow anonymous > phy references (leaving out "phy-names" entirely), and adding a > proper API for that. > Thanks Arnd, I will wait for Alan's comments before proceeding. I am happy to patch the ehci-platform driver to use a new api instead of devm_phy_get if that is the best option. >> Should I run this by Alan Stern? > > I've added him to Cc here. He clearly didn't know the background about > the DT binding change, and should not need to, but he may have an opinion > on what names we should use. > >>> What is the reason for having two phys in your case? Are these >>> identical phy devices connected to a single controller or do they >>> server different purposes? >>> >> Yes, we have three identical phys connected to a single host controller >> and one of the phys is also connected to the device controller > > Ok, no problem with that, let's just make sure we come up with a > good binding for it. > > Arnd > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Alan and Arnd I wanted to follow up on this patch and ascertain what I would have to change. Please see below for my questions On 15-02-18 04:46 PM, Arun Ramamurthy wrote: > > > On 15-02-18 07:15 AM, Arnd Bergmann wrote: >> On Tuesday 17 February 2015 13:05:50 Arun Ramamurthy wrote: >>> On 15-02-17 12:53 PM, Arnd Bergmann wrote: >>>> On Tuesday 17 February 2015 12:00:49 Arun Ramamurthy wrote: >>>>> Arnd, I patched the ehci and ohci driver to accept multiple phys so >>>>> they >>>>> require different names and cannot both be "usb". That patch was >>>>> accepted by Alen Stern but I did not update the bindings >>>>> documentation. >>>>> I will send out another patch for that. Could we go with the naming >>>>> scheme of "usb" + "p" + port number or do you have other suggestions? >>>> >>>> I don't have a good idea, but I think it would be best if the first >>>> phy could remain named "usb" for compatibility with the existing >>>> binding. >>>> >>> The patch was written in a way that all the existing and new drivers can >>> continue to use "usb" if they are using only one phy so that we remain >>> compatible. The names need to be different only if more than one phy is >>> specified. In such cases i don't think the first phy should be "usb" as >>> it would be confusing to have >>> phy-names = "usb","usbp1" >> >> I see your patch now, as 7e7a0e67f2c ("usb: ehci-platform: add support >> for >> multiple phys per controller"), and I'm not too happy about the way you >> did this. > >> We already concluded that there should have been a binding change >> to go along with this, and that would have caught the fact that you >> circumvent the API here by reading the phy names manually. That >> part should never have made it into the kernel. >> >> I think we can do this either by defining specific names for the >> phy, or by changing the generic PHY binding to allow anonymous >> phy references (leaving out "phy-names" entirely), and adding a >> proper API for that. >> > Thanks Arnd, I will wait for Alan's comments before proceeding. I am > happy to patch the ehci-platform driver to use a new api instead of > devm_phy_get if that is the best option. > >>> Should I run this by Alan Stern? >> >> I've added him to Cc here. He clearly didn't know the background about >> the DT binding change, and should not need to, but he may have an opinion >> on what names we should use. >> > Arnd, should I re patch the ehci-platform driver to avoid phy-names entirely? Alan, if not do you have an opinion on what the usb phy names should be? The current patch uses "usbp" + port number such as "usbp0" , "usbp1" etc >>>> What is the reason for having two phys in your case? Are these >>>> identical phy devices connected to a single controller or do they >>>> server different purposes? >>>> >>> Yes, we have three identical phys connected to a single host controller >>> and one of the phys is also connected to the device controller >> >> Ok, no problem with that, let's just make sure we come up with a >> good binding for it. >> Arnd do you have any other comments on the phy driver itself? Thank you >> Arnd >> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 25 February 2015 16:24:06 Arun Ramamurthy wrote: > >>> Should I run this by Alan Stern? > >> > >> I've added him to Cc here. He clearly didn't know the background about > >> the DT binding change, and should not need to, but he may have an opinion > >> on what names we should use. > >> > > > Arnd, should I re patch the ehci-platform driver to avoid phy-names > entirely? Alan, if not do you have an opinion on what the usb phy names > should be? The current patch uses "usbp" + port number such as "usbp0" , > "usbp1" etc I think avoiding the phy names would be best here, but it requires a kernel API change first, because we do not have a way to get a phy by index as we do for other subsystems (e.g. clocks or gpios). Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15-03-10 01:27 PM, Arnd Bergmann wrote: > On Wednesday 25 February 2015 16:24:06 Arun Ramamurthy wrote: >>>>> Should I run this by Alan Stern? >>>> >>>> I've added him to Cc here. He clearly didn't know the background about >>>> the DT binding change, and should not need to, but he may have an opinion >>>> on what names we should use. >>>> >>> >> Arnd, should I re patch the ehci-platform driver to avoid phy-names >> entirely? Alan, if not do you have an opinion on what the usb phy names >> should be? The current patch uses "usbp" + port number such as "usbp0" , >> "usbp1" etc > > I think avoiding the phy names would be best here, but it requires a > kernel API change first, because we do not have a way to get a phy > by index as we do for other subsystems (e.g. clocks or gpios). > Arnd , there is an existing api _of_phy_get that gets a phy by index. However it is not exported and is called from of_phy_get which is in turn called from devm_of_phy_get. My plan is to create a new function maybe devm_of_phy_get_by_index that calls _of_phy_get directly? What are your thoughts? > Arnd > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 11 March 2015 14:37:58 Arun Ramamurthy wrote: > On 15-03-10 01:27 PM, Arnd Bergmann wrote: > > On Wednesday 25 February 2015 16:24:06 Arun Ramamurthy wrote: > >>>>> Should I run this by Alan Stern? > >>>> > >>>> I've added him to Cc here. He clearly didn't know the background about > >>>> the DT binding change, and should not need to, but he may have an opinion > >>>> on what names we should use. > >>>> > >>> > >> Arnd, should I re patch the ehci-platform driver to avoid phy-names > >> entirely? Alan, if not do you have an opinion on what the usb phy names > >> should be? The current patch uses "usbp" + port number such as "usbp0" , > >> "usbp1" etc > > > > I think avoiding the phy names would be best here, but it requires a > > kernel API change first, because we do not have a way to get a phy > > by index as we do for other subsystems (e.g. clocks or gpios). > > > Arnd , there is an existing api _of_phy_get that gets a phy by index. > However it is not exported and is called from of_phy_get which is in > turn called from devm_of_phy_get. > > My plan is to create a new function maybe devm_of_phy_get_by_index that > calls _of_phy_get directly? What are your thoughts? Sounds good to me. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt b/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt new file mode 100644 index 0000000..f218fae --- /dev/null +++ b/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt @@ -0,0 +1,50 @@ +BROADCOM CYGNUS USB PHY + +Required Properties: + - compatible: brcm,cygnus-usb-phy + - reg : usbphy_regs - Base address of phy registers + usb2h_idm_regs - Base address of host idm registers + usb2d_idm_regs - Base address of device idm registers + - num_phys : Number of phys available + - #phy-cells must be 2 +The node that uses the phy must provide two integers, the first indicates the +port and the second indicates whether its host or device. The second integer +is 0 for device and 1 for host + +NOTE: port 0 and port 1 are host only and port 2 can be configured for host or device. + +Example of phy : + usbphy0: usbphy@0x0301c000 { + #phy-cells = <2>; + num_phys = <3>; + compatible = "brcm,cygnus-usb-phy"; + reg = <0x0301c000 0x2000>, + <0x18115000 0x1000>, + <0x18111000 0x1000>; + status = "okay"; + }; + +Example of node using the phy: + + /* This nodes declares port 0 + and port 1 as host*/ + + ehci0: usb@0x18048000 { + compatible = "generic-ehci"; + reg = <0x18048000 0x100>; + interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>; + phys = <&usbphy0 0 1 &usbphy0 1>; + phy-names = "usbp0","usbp1"; + status = "okay"; + }; + + /* This node declares port 2 phy + and configures it for device */ + + usbd_udc_dwc1: usbd_udc_dwc@0x1804c000 { + compatible = "iproc-udc"; + reg = <0x1804c000 0x2000>; + interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>; + phys = <&usbphy0 2 0>; + phy-names = "usb"; + };