From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
To: Kishon Vijay Abraham I <kishon@ti.com>,
robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com,
ijc+devicetree@hellion.org.uk, galak@codeaurora.org,
grant.likely@linaro.org, devicetree@vger.kernel.org
Cc: rdunlap@infradead.org, linux-doc@vger.kernel.org,
linux-sh@vger.kernel.org
Subject: Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
Date: Fri, 04 Jul 2014 20:53:13 +0000 [thread overview]
Message-ID: <53B71439.8040709@cogentembedded.com> (raw)
In-Reply-To: <53B2B37F.6080300@ti.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
next prev parent reply other threads:[~2014-07-04 20:53 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-05-23 22:06 [PATCH v4] phy: Renesas R-Car Gen2 PHY driver Sergei Shtylyov
2014-05-26 7:28 ` Geert Uytterhoeven
2014-05-26 7:48 ` Simon Horman
2014-05-26 8:04 ` Geert Uytterhoeven
2014-05-26 9:00 ` Simon Horman
2014-05-27 9:29 ` Yoshihiro Shimoda
2014-05-27 19:38 ` Sergei Shtylyov
2014-05-28 1:12 ` Yoshihiro Shimoda
2014-06-04 11:52 ` Kishon Vijay Abraham I
2014-06-04 21:54 ` Sergei Shtylyov
2014-06-09 11:43 ` Laurent Pinchart
2014-06-10 10:55 ` Kishon Vijay Abraham I
2014-06-25 22:16 ` Sergei Shtylyov
2014-07-01 13:23 ` Kishon Vijay Abraham I
2014-07-04 20:53 ` Sergei Shtylyov [this message]
2014-07-08 12:32 ` Kishon Vijay Abraham I
2014-07-08 21:27 ` Sergei Shtylyov
2014-06-09 11:44 ` Laurent Pinchart
2014-06-09 12:34 ` Sergei Shtylyov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53B71439.8040709@cogentembedded.com \
--to=sergei.shtylyov@cogentembedded.com \
--cc=devicetree@vger.kernel.org \
--cc=galak@codeaurora.org \
--cc=grant.likely@linaro.org \
--cc=ijc+devicetree@hellion.org.uk \
--cc=kishon@ti.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-sh@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=pawel.moll@arm.com \
--cc=rdunlap@infradead.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).