Hello.
On 06/10/2014 02:43 PM, Kishon Vijay Abraham I wrote:
Sorry for delayed reply, I was busy with other things. Now it's time to
revisit the infamous USB PHY driver...
This PHY, though formally being a part of Renesas USBHS controller, contains
the
UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls
them
channels) to the different USB controllers: channel 0 can be connected to
either
PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI EHCI/OHCI
or xHCI controllers.
This is a new driver for this USB PHY currently already supported under
drivers/
usb/phy/. The reason for writing the new driver was the requirement that the
multiplexing of USB channels to the controller be dynamic, depending on what
USB drivers are loaded, rather than static as provided by the old driver.
The infrastructure provided by drivers/phy/phy-core.c seems to fit that purpose
ideally. The new driver only supports device tree probing for now.
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
[...]
Index: linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
===================================================================
--- /dev/null
+++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
@@ -0,0 +1,60 @@
+* Renesas R-Car generation 2 USB PHY
+
+This file provides information on what the device node for the R-Car
generation
+2 USB PHY contains.
+
+Required properties:
+- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790
SoC.
+ "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.
+- reg: offset and length of the register block.
+- #phy-cells: see phy-bindings.txt in the same directory, must be 2.
+- clocks: clock phandle and specifier pair.
+- clock-names: string, clock input name, must be "usbhs".
+
+The phandle's first argument in the PHY specifier identifies the USB channel,
+the second one is the USB controller selector and depends on the first:
+
++-----------+---------------+---------------+
+|\ Selector | | |
++ --------- + 0 | 1 |
+| Channel \| | |
++-----------+---------------+---------------+
+| 0 | PCI EHCI/OHCI | HS-USB |
+| 1 | PCI EHCI/OHCI | xHCI |
++-----------+---------------+---------------+
+
+The USB PHY device tree node should be the subnodes corresponding to the USB
+channels and the controllers connected to them. These subnodes must contain
the
+following properties:
+- renesas,phy-select: the first cell identifies the USB channel, the second
cell
+ is the USB controller selector; see the table above for the values.
+- renesas,ugctrl2-masks: the first cell is the UGCTRL2 mask corresponding to
+ the USB channel, the second cell is the UGCTRL2 value corresponding to the
+ USB controller selected for that channel.
+
+Example (Lager board):
+
+ usb-phy@e6590100 {
+ compatible = "renesas,usb-phy-r8a7790";
+ reg = <0 0xe6590100 0 0x100>;
+ #phy-cells = <2>;
+ clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
+ clock-names = "usbhs";
+
+ usb-phy@0,0 {
+ renesas,phy-select = <0 0>;
+ renesas,ugctrl2-masks = <0x00000030 0x00000010>;
+ };
+ usb-phy@0,1 {
+ renesas,phy-select = <0 1>;
+ renesas,ugctrl2-masks = <0x00000030 0x00000030>;
+ };
+ usb-phy@1,0 {
+ renesas,phy-select = <1 0>;
+ renesas,ugctrl2-masks = <0x80000000 0x00000000>;
+ };
+ usb-phy@1,1 {
+ renesas,phy-select = <1 1>;
+ renesas,ugctrl2-masks = <0x80000000 0x80000000>;
+ };
+ };
IIUC, channel 0 can be configured for either EHCI/OHCI or HS-USB but can't be
used for both. And channel 1 can be configured for either PCI EHCI/OHCI or
xHCI. right?
Yes. However that depends on the driver load order: if e.g. xHCI driver is
loaded later than PCI USB drivers,
it will override the channel routing.
So will the PCI USB drivers will be notified of that?
Unfortunately, no. But this is also the case with the other multi-PHY
drivers...
So ideally only two sub-nodes should be created for channel '0' and channel
'1'.
Hm, but I need to perform a special PHY power up sequence for the USBHS PHY
itself (corresponding to channel #0, selector #1).
You can configure a channel to a particular mode by passing the mode in
PHY specifier
I already have "#phy-cells" prop equal to 2.
(The channel should be configured to a particualr mode in xlate).
I have even considered using the of_xlate() method at first but then
abandoned that idea for the phy_init() method...
If you want to configure the PHY to a particular mode, xlate should be the best
place.
I tried to move the code there from the init() method but then I realized
that I need to prepare/enable the USBHS clock before writing to the UGCTRL2
register and there's no place I can disable/unprepare this clock if I do the
channel routing in the xlate() method. So no, I don't agree here.
Btw I'm not sure if it is recommended to pass register mask values from dt.
Neither am I. I did that because you requested the device tree
representation for each of the sub-PHYs under that part of the driver which
initialized the register masks, so that I thought that you wanted those masks
to be represented in the device tree as well...
No I didn't mean that. Maybe for now we can have a identifier in the sub-node
and select mask and value based on that.
Sigh. Yes, having some sort of selecting property in the subnodes is the
only way...
[...]
Index: linux-phy/drivers/phy/phy-rcar-gen2.c
===================================================================
--- /dev/null
+++ linux-phy/drivers/phy/phy-rcar-gen2.c
@@ -0,0 +1,288 @@
[...]
+#define USBHS_LPSTS 0x02
+#define USBHS_UGCTRL 0x80
+#define USBHS_UGCTRL2 0x84
+#define USBHS_UGSTS 0x88
+
+/* Low Power Status register (LPSTS) */
+#define USBHS_LPSTS_SUSPM 0x4000
+
+/* USB General control register (UGCTRL) */
+#define USBHS_UGCTRL_CONNECT 0x00000004
+#define USBHS_UGCTRL_PLLRESET 0x00000001
+
+/* USB General control register 2 (UGCTRL2) */
+#define USBHS_UGCTRL2_USB2SEL 0x80000000
+#define USBHS_UGCTRL2_USB2SEL_PCI 0x00000000
+#define USBHS_UGCTRL2_USB2SEL_USB30 0x80000000
+#define USBHS_UGCTRL2_USB0SEL 0x00000030
+#define USBHS_UGCTRL2_USB0SEL_PCI 0x00000010
+#define USBHS_UGCTRL2_USB0SEL_HS_USB 0x00000030
+
+/* USB General status register (UGSTS) */
+#define USBHS_UGSTS_LOCK 0x00000300
+
+#define NUM_USB_CHANNELS 2
+
+struct rcar_gen2_phy {
+ struct phy *phy;
+ struct rcar_gen2_phy_driver *drv;
+ u32 select_mask;
+ u32 select_value;
+};
+
+struct rcar_gen2_phy_driver {
+ void __iomem *base;
+ struct clk *clk;
+ spinlock_t lock;
+ struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];
This can be created dynamically based on the number of sub-nodes. In this case
Did you mean that I'll need to use linked list here instead of an array?
Hm, that sounds to me like another complication of the driver with no
apparent win...
dynamic allocation is in no way going to complicate the driver.
Since when the dynamic data allocation is as simple as the statically
allocated data? :-O
it'll be only rcar_gen2_phy phys[2], one for each channel.
By this we need not hard code NUM_USB_CHANNELS.
I don't quite understand what's up with hard-coding it -- this constant is
dictated by the UGCTRL2 register layout anyway.
right but you don't want to change the driver a whole lot when they change the
no of channels in the next version
They have already done so: R8A7790 has 3 USB channels, R8A7791 has only 2.
However, the number of controllable channels didn't change.
or they use a slightly modified version of
this IP in a different SoC. And finding the number of channels dynamically is
not complicated anyway IMO.
Sorry, but what you're saying here just doesn't make sense to me. I'd need
to modify the driver for the different number of the controllable channels in
any case since the UGCTRL2 masks/values have to be hard coded in the driver as
you said. If they were read from the device tree, that would have made sense
but you seem to be against that...
Thanks
Kishon
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
