Hello,
On Thursday 05 June 2014 01:54:47 Sergei Shtylyov wrote:
> Hello.
>
> On 06/04/2014 03:40 PM, Kishon Vijay Abraham I wrote:
> >> 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 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...
>
> > 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...
Given that this is a new driver, and that we can't know how future generations
of the device will behave and what information will be required, wouldn't it
make more sense to move the information represented by the subnodes to the
driver ?
> [...]
>
> >> 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
>
> Hm, that sounds to me like another complication of the driver with no
> apparent win...
>
> > 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.
--
Regards,
Laurent Pinchart
--
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
