Re: [PATCH 16/16] ARM: OMAP: omap4panda: Power down the USB PHY and ETH when not in use

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3911 bytes --]

Hi,

On Thu, Nov 22, 2012 at 09:49:05PM +0800, Andy Green wrote:
> >Again it sounds like something that should be done at the hub driver
> >level. I mean using the GPIO (for reset) and Power Regulator.
> >
> >not implementing the regulator part itself, but using it.
> 
> If you mean change the regulator managing code from living in
> omap-hsusb to ehci-hcd, it sounds like a good idea.

I mean that drivers/usb/core/hub.c should manage it, since that's what
actually manages the HUB entity.

> >>easily be a PMIC regulator channel: it boils down to controlling a
> >>power rail.  So using struct regulator as the abstraction for the
> >>power is the right way already.
> >
> >I agree with you here. Nevertheless, I'm arguing that this all should be
> >handled by the hub driver, not ehci-omap.
> 
> If "hub driver" means ehci-hcd then I agree, why not let all the ehci
> platforms have the same regulator management option instead of just
> OMAP.

the reasoning is in alignment with mine, but the "target driver" is
wrong :-)

> From the POV of the original goal of the patch, it was just to give
> us a way to control static power consumption by modprobe.  It would
> work fine to do that by modprobe [-r] smsc95xx same as ehci-hcd
> actually, although I agree it's backward from usual discover -> udev
> -> modprobe POV.  Anyway that is not what we ended up with so no need
> to worry about it.

fair enough, but I would only accept $SUBJECT if in the same series came
the patch moving the code to the right place, otherwise things such as
those never get done because other tasks are prioritized over a "simple
cleanup".

> >>offers a struct regulator.  But there is a quirk on Panda that means
> >>that is not workable.
> >>
> >>On Panda, they share one SoC gpio to reset both an external ULPI PHY
> >>chip and the smsc95xx that is downstream of it.
> >
> >of course. the Network part is attached to one of the Hub ports, that's
> >why the hub exposes only two ports.
> 
> I am not sure how that makes it an "of course".  It's not like
> there's a shortage of SoC gpio to separate them and allow cleaner
> software.  But never mind that either.

fair enough ;-)

> >>After you power up the smsc95xx, you must reset it in order for
> >>correct operation.  This actually resets the ULPI PHY too.
> >>
> >>The ULPI PHY is permanently powered, only nRESET is provided to
> >>control it.
> >>
> >>For that reason, as an attribute of being on a Panda board, we need
> >>to do the (shared) reset meddling once at hsusb init, and that is why
> >>the patch is as it is.
> >
> >yeah, yeah, but it's not correct to push all that code is the OMAP USB
> >Part when the hub driver is the only one who knows when the hub is going
> >to be reset and the like.
> >
> >You're poking into a HUB, not into the EHCI controller.
> 
> My patch is just using what's there at the moment.  It only touches
> the board file and does not "push all that code is the OMAP USB
> part".
> 
> I agree with you what's in the OMAP USB part is better in the generic
> part, but I didn't put it there.

not saying you did, merely checking if Alan would be ok with that code
living in the hub driver, and if he would, I'd ask you or Roger to move
the to it's proper location ;-)

> If you want that moved and nobody else wants to do it, I can probably
> do that in a new, additional patch.  If so you might want to confirm

as long as Alan is ok and it comes in the same series, I'd be really,
really glad to see such a patch and would happily review it.

> you're OK with the magic naming convention for regulators or (as
> Roger suggested earlier) pass it in by platform_data.

I'm not sure if regulator names should be passed via platform_data. I
think Mark Brown would get quite upset if we did such a thing, but I
could be wrong.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]