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