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

[Kishon Vijay Abraham I <kishon@ti.com>]

Hi,

On Saturday 05 July 2014 02:23 AM, Sergei Shtylyov wrote:
> 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.

Don't understand how overriding is desirable. Won't it affect the first
controller that got the PHY?
> 
> [...]
>> .
>> .
>> <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...

yeah.. that can be done.
> 
>    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.

indeed. Unless we directly pass the index in the phy specifier (from dt). But I
would prefer linear search instead.
> 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.

That will lead to broken dt data. I think we have to do both.
> 
>>>> 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.

NAK.

-Kishon