Hi, On Thursday 05 June 2014 03:24 AM, 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 will the PCI USB drivers will be notified of that? > >> 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. > >> 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. > > [...] >>> 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... dynamic allocation is in no way going to complicate the driver. > >> 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 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. Thanks Kishon -- 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