linux-sh.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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


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