Hello. On 07/08/2014 04:20 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.
[...]
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.
Consider the scenario where OHCI/EHCI drivers load first: they would claim all channels for themselves (despite channel #0 might have a host connected instead of a device). With the channel exclusion mechanism in place, the USBHS driver loaded later won't be able to take control of the channel #0, while it's actually desirable.
Won't it affect the first controller that got the PHY?
It will, though it might be desirable. But I'd probably agree that making the host/UDC driver hold into its PHY forbidding the other drivers to bind to the same channel would be more straight-forward way: just load the drivers that you want to control a given channel first, not second. I guess you want me to invent something here?
[...]
+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.
That would a bit simpler and even faster, I think.
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
The DT subnodes are not guaranteed to be in a certain order or even have consecutive channel #'s, no?
would prefer linear search instead.
OK.
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.
Looking at the IEEE1275-1994 standard, I do not think "disabled" could mean that the device is not present:
“disabled” The device represented by this node is not operational, but it might become operational in the future (e.g., an external switch is turned off, or something isn’t plugged in.) So I would really prefer the non-existing channels to not be described in DT.
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.
Could you elaborate?
I think we have to do both.
Well, if we agree that we'd have subnodes, yes.
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?
I have now read the info on all members of the R-Car generation 2 SoCs, most will have the same two channels as R8A7791, only R8A7790 has 3 channels. There's still some hope that the future SoC family (a few years away from now) would have more sane PHY design...
Two; we have only two controllable channels in any case.
NAK.
Seriously, I don't want to waste any memory on non-switchable channel, even when it exists.
-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
