diff mbox

[1/2] phy: usbphy: Add dt documentation for Broadcom Cygnus USB PHY driver

Message ID 1424200821-16368-2-git-send-email-arun.ramamurthy@broadcom.com
State New, archived
Headers show

Commit Message

Arun Ramamurthy Feb. 17, 2015, 7:20 p.m. UTC
Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
---
 .../bindings/phy/brcm,cygnus-usb-phy.txt           | 50 ++++++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt

Comments

Arnd Bergmann Feb. 17, 2015, 7:41 p.m. UTC | #1
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
Arun Ramamurthy Feb. 17, 2015, 8 p.m. UTC | #2
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
Arnd Bergmann Feb. 17, 2015, 8:53 p.m. UTC | #3
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
Arun Ramamurthy Feb. 17, 2015, 9:05 p.m. UTC | #4
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
Arnd Bergmann Feb. 18, 2015, 3:15 p.m. UTC | #5
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
Arun Ramamurthy Feb. 19, 2015, 12:46 a.m. UTC | #6
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
Arun Ramamurthy Feb. 26, 2015, 12:24 a.m. UTC | #7
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
Arnd Bergmann March 10, 2015, 8:27 p.m. UTC | #8
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
Arun Ramamurthy March 11, 2015, 9:37 p.m. UTC | #9
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
Arnd Bergmann March 11, 2015, 9:47 p.m. UTC | #10
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 mbox

Patch

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";
+	};