Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Andy Green <andy.green-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On 04/12/12 10:39, the mail apparently from Ming Lei included:
> On Mon, Dec 3, 2012 at 12:52 PM, Andy Green <andy.green-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>> +static void ehci_hub_power_off(struct power_controller *pc, struct
>>>>> device
>>>>> *dev)
>>>>> +{
>>>>> +       gpio_set_value(GPIO_HUB_NRESET, 0);
>>>>> +       gpio_set_value(GPIO_HUB_POWER, 0);
>>>>> +}
>>>>> +
>>>>> +static struct usb_port_power_switch_data root_hub_port_data = {
>>>>> +       .hub_tier       = 0,
>>>>> +       .port_number = 1,
>>>>> +       .type = USB_PORT_CONNECT_TYPE_HARD_WIRED,
>>>>> +};
>>>>> +
>>>>> +static struct usb_port_power_switch_data smsc_hub_port_data = {
>>>>> +       .hub_tier       = 1,
>>>>> +       .port_number = 1,
>>>>> +       .type = USB_PORT_CONNECT_TYPE_HARD_WIRED,
>>>>> +};
>>>>> +
>>>>> +static struct power_controller pc = {
>>>>> +       .name = "omap_hub_eth_pc",
>>>>> +       .count = ATOMIC_INIT(0),
>>>>> +       .power_on = ehci_hub_power_on,
>>>>> +       .power_off = ehci_hub_power_off,
>>>>> +};
>>>>> +
>>>>> +static inline int omap_ehci_hub_port(struct device *dev)
>>>>> +{
>>>>> +       /* we expect dev->parent points to ehcd controller */
>>>>> +       if (dev->parent && !strcmp(dev_name(dev->parent),
>>>>> "ehci-omap.0"))
>>>>> +               return 1;
>>>>> +       return 0;
>>>>> +}
>>>>> +
>>>>> +static inline int dev_pc_match(struct device *dev)
>>>>> +{
>>>>> +       struct device *anc;
>>>>> +       int ret = 0;
>>>>> +
>>>>> +       if (likely(strcmp(dev_name(dev), "port1")))
>>>>> +               goto exit;
>>>>> +
>>>>> +       if (dev->parent && (anc = dev->parent->parent)) {
>>>>> +               if (omap_ehci_hub_port(anc)) {
>>>>> +                       ret = 1;
>>>>> +                       goto exit;
>>>>> +               }
>>>>> +
>>>>> +               /* is it port of lan95xx hub? */
>>>>> +               if ((anc = anc->parent) && omap_ehci_hub_port(anc)) {
>>>>> +                       ret = 2;
>>>>> +                       goto exit;
>>>>> +               }
>>>>> +       }
>>>>> +exit:
>>>>> +       return ret;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * Notifications of device registration
>>>>> + */
>>>>> +static int device_notify(struct notifier_block *nb, unsigned long
>>>>> action,
>>>>> void *data)
>>>>> +{
>>>>> +       struct device *dev = data;
>>>>> +       int ret;
>>>>> +
>>>>> +       switch (action) {
>>>>> +       case DEV_NOTIFY_ADD_DEVICE:
>>>>> +               ret = dev_pc_match(dev);
>>>>> +               if (likely(!ret))
>>>>> +                       goto exit;
>>>>> +               if (ret == 1)
>>>>> +                       dev_pc_bind(&pc, dev, &root_hub_port_data,
>>>>> sizeof(root_hub_port_data));
>>>>> +               else
>>>>> +                       dev_pc_bind(&pc, dev, &smsc_hub_port_data,
>>>>> sizeof(smsc_hub_port_data));
>>>>> +               break;
>>>>> +
>>>>> +       case DEV_NOTIFY_DEL_DEVICE:
>>>>> +               break;
>>>>> +       }
>>>>> +exit:
>>>>> +       return 0;
>>>>> +}
>>>>> +
>>>>> +static struct notifier_block usb_port_nb = {
>>>>> +       .notifier_call = device_notify,
>>>>> +};
>>>>> +
>>>>
>>>>
>>>>
>>>> Some thoughts on trying to make this functionality specific to power only
>>>> and ehci hub port only:
>>>>
>>>>    - Quite a few boards have smsc95xx... they're all going to carry these
>>>> additions in the board file?  Surely you'll have to generalize the pieces
>>>
>>>
>>> All things are board dependent because we are discussing peripheral
>>> device(not builtin SoC devices), so it is proper to put it in the board
>>> file.
>>> If some boards want to share it, we can put it in a single .c file and
>>> let board file include it.
>>
>>
>> Where would the .c file go... SMSC is not platform, or even arch specific
>> chip (eg, iMX or MIPS or even x86 boards can have it), so not
>> arch/arm/mach-xxxx or arch/arm/plat-xxx or arch/arm.  I guess it would go in
>> drivers/usb or drivers/net.
>
> How does drivers/usb or drivers/net know the SMSC is used on beagle or
> panda? Different power control approach is used in the two boards even
> same SMSC chip is shipped in the two boards.

You mention you're going to "put it in a single .c file and let the 
board file include it", I am just wondering where that .c file will 
live.  It seems it would have to live down drivers/ somewhere so MIPS, 
x86 etc that might have an SMSC chip onboard can also use it if they want.

>> Push in ARM-Land is towards single kernels which support many platforms, a
>> good case in point is omap2plus_defconfg which wants to allow to support all
>> OMAP2/3/4/5 platforms in one kernel.  If you "include" that C file over and
>> over in board files, it's not very nice for that.  They anyway want to
>> eliminate board files for the single kernel binary reason, and just have one
>> load of config come in by dtb.
>
> I only mean it is reasonable to put the power control code into board
> file because
> each board may take different power control approach even same SMSC chip
> is used. I understand DT only describes the difference of the board via device
> node, and I am not sure if the current DT is enough to convert the board file
> into data as far as the problem is concerned.

No the approach with DT is to provide a dummy SoC "board file" with all 
data provided by dtb.  This is already established, see 
./arch/arm/mach-omap2/board-generic.c for example.  You can see there's 
nothing in it other than minimum dt match business.

People with new boards are getting pointed at that and told to sort it 
out in dtb and disallowed from creating new board files.

So whatever support or helper scheme you're adding needs a story about 
how dtb can express it (in dt language, "has bindings for it") or it 
can't be used on new boards.  That's not a minor ding any more either.

>> So it guides you towards having static helper code once in drivers/ for the
>> scenarios you support... if that's where you end up smsc is less good a
>> target for a helper than to have helpers for classes of object like
>> regulator and clk, that you can combine and reuse on all sorts of target
>> devices, which is device_asset approach.
>>
>> It also guides you to having the special platform sauce be something that
>> can go into the dtb: per-board code can't.  That's why device_asset stuff
>> only places asset structs in the board file and is removing code from there.
>>
>>
>>>> that perform device_path business out of the omap4panda board file at
>>>> least.
>>>> At that point the path matching code becomes generic
>>>> end-of-the-device-path
>>>> matching code.
>>>
>>>
>>> Looks Alan has mentioned, there might be no generic way, and any device's
>>> name change in the path may make the way fragile.
>>
>>
>> What you have provided is no less fragile if you allow "port1" and the
>> ehci-omap.0 to be set from the outside.
>>
>> Unless someone NAKs it for sure already (if you're already sure you're going
>> to, please do so to avoid wasting time), I'll issue a try#2 of my code later
>> which demonstrates what I mean.  At least I guess it's useful for
>> comparative purposes.
>>
>>
>>>>    - How could these literals like "port1" etc be nicely provided by
>>>> Device
>>>> Tree?  In ARM-land there's pressure to eventually eliminate board files
>>>> completely and pass in everything from dtb.  device_asset can neatly grow
>>>> DT
>>>> bindings in a generic way, since the footprint in the board file is some
>>>
>>>
>>> IMO, it isn't necessary to expose these assets to device or users, from
>>> the
>>> view of device or user, which only cares two actions(poweron and poweroff)
>>> about the discussed problem. Also it should be better to put these assets
>>> into device resource list, instead of introducing them in 'struct device'.
>>
>>
>>  From the point of view of allowing it to be reused on different boards /
>> platforms / arches, you are going to have to do something better than have
>> literals in the code.
>>
>>
>>>> regulators that already have dt bindings and some device_asset structs.
>>>> Similarly there's pressure for magic code to service a board (rather than
>>>> SoC) to go elsewhere than the board file.
>>>
>>>
>>> Looks you associate these assets with ehci-omap device, which mightn't be
>>> enough, because we need to control port's power for supporting port
>>> power off mechanism. Do you have generic way to associate these assets
>>> with usb port device and let port use it generally?
>>
>>
>> Yes, you need a parent device pointer (ehci host controller in this case)
>> and the path rhs, but only stuff that is defined by usb stack code.  Needing
>> a parent pointer is OK because this stuff only has meaning for hardwired
>> assets on the platform, so the parent device will always be known as a
>> platform_device at boot time anyway.
>
> parent device pointer may work on the panda problem, but may not work
> on other case if one hardwired device is powered by another power domain.
>
> So it is not a general solution on usb port power off.

I am talking about, well, "ancestor" pointer only to filter descendant 
children.  Not for any power control directly.

It gets us away from caring about what the device path looked like prior 
to that host controller, and since we have confidence it's exactly the 
host controller we intended by knowing its platform_device, we can 
ignore everything between than at the right-hand side path fragment 
identifying the child.  So it's a strong way to reduce fragility on the 
device_path stuff.

>> The code I'll provide will work the same in sdio or other bus case, just use
>> mmc host controller pointer and the sdio device name the same way.
>>
>>
>>>>    - Shouldn't this take care of enabling and disabling the ULPI PHY clock
>>>> on
>>>> Panda too?  There's no purpose leaving it running if the one thing the
>>>> ULPI
>>>> PHY is connected to is depowered, and when you do power it, on Panda you
>>>> will reset the ULPI PHY at the same time anyway (smsc reset and ULPI PHY
>>>> reset are connected together on Panda).  Then you can eliminate
>>>> omap4_ehci_init() in the board file.
>>>
>>>
>>> OK, we can include the ULPI PHY clock things easily in ->power_on() and
>>> ->power_off() of 'power controller'
>>
>>
>> Yes if the ARM people will accept establishing more code in board files that
>> doesn't have a DT story.
>
> As I explained above, it is reasonable to put the power control code in board
> file, but current DT could convert these board file into device node?

DT has lots of bindings now, you can describe regulators and things in 
there fully.  The point is whatever scheme is workable for this must be 
able to get soaked up using DT too to be acceptable.  Taking the 
approach you're going to drop literal strings in code in the board file, 
or throw code at the board file at all, will no longer fly.

I have not provided a DT solution for my approach yet either, but I have 
taken care that the only footprint in the boardfile version of it are 
*structs*.  That means translating them to dtb content by adding a 
binding for the asset stuff for example, will be a clean task.

That's also why back at the beginning of this discussion I gave dts 
version of the regulator structs I was introducing in that patch... it's 
proof that the boardfile footprint of that scheme is ready to go into dtb.

-Andy

-- 
Andy Green | TI Landing Team Leader
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  - 
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html