Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

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@cogentembedded.com>

[...]

>> 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...

[...]
>> 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.

> Thanks
> Kishon

WBR, Sergei