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