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

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

Hello.

On 07/01/2014 05:11 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.

> .
> .
> <snip>
> .
> .

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

> IIRC, in the case of other existing multi-phy drivers, all the PHYs can
> co-exist without actually overriding anything that was configured previously.

    'phy-exynos-mipi-video' driver looked somewhat suspicious in this respect 
(I didn't understand why they used "#phy-cells" of 1, having 2 channels with 
two PHYs each) but upon further scrutiny it appears that the PHYs on one 
channel function quite independently...

[...]

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

    Unless I prepare/enable the clock when probing, and undo it on removal, 
that is.

>> channel routing in the xlate() method. So no, I don't agree here.

> enabling clock from init() seems correct to me. We need a better way to avoid
> overriding the PHY to a particular mode.

    In fact, in my case such override may be rather desirable.

[...]
> .
> .
> <snip>
> .
> .
>
>>>>>> +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?

> Nope. I meant something like below.

> struct rcar_gen2_phy_driver {
> 	.
> 	.
> 	struct rcar_gen2_phy **phys;
> }
>
> probe()
> {
> 	<snip>
> 	int i = 0, channel_count;
> 	struct rcar_gen2_phy **phys;
> 	channel_count = of_get_child_count(np);

    Didn't know of such function...

> 	phys = kzalloc(sizeof(*phys) * channel_count, GFP_KERNEL);

    Rather kcalloc().

> 	for_each_child_of_node(dev->of_node, np) {
> 		struct rcar_gen2_phy *phy;
> 		.
> 		.
> 		phy = kzalloc(sizeof(*phy), GFP_KERNEL);
> 		.
> 		.
> 		phy->phy = devm_phy_create(dev, &rcar_gen2_phy_ops, NULL);
> 		phys[i++] = phy;
> 	}
> 	drv->phys = phys;
> 	<snip>
> }

> Then you can access 'phys' just like how you access an array.

    Aren't you over-engineering things? I'd rather have just an array of 
'struct rcar_gen2_phy' dynamically allocated at once, instead of an array of 
pointers to struct rcar_gen2_phy' and then PHYs allocated piecemeal...

    Anyway, this means that I'll have to do linear search for the needed PHY 
in the xlate() method, just like it would have been with a linked list. 
Complication. :-)

[...]

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

> right.. that's where I'd like to have status = "disabled" for that channel in
> your dt node.

    I disagree here. First, channel #1 is not controllable anyway, so of no 
interest to us. Anyway, if more controllable channel appear, may point is that 
should be a matter of introducing and properly handling a new "compatible" 
property, not just adding/removing subnodes.

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

> R8A7790 has 3 USB channels and R8A7791 has only 2. So what should be the
> NUM_CHANNELS in this driver?

    Two; we have only two controllable channels in any case.

> Modifying the driver _can_ be adding macros for
> registers, bit masks etc.. and maybe appropriately modifying of_device data.

    s/_can_/must/.

> Cheers
> Kishon

WBR, Sergei