Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Felipe Balbi <balbi@ti.com>]

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

On Mon, Feb 24, 2014 at 03:21:05PM +0530, Kishon Vijay Abraham I wrote:
> Hi Roger,
> 
> On Friday 21 February 2014 05:59 PM, Roger Quadros wrote:
> > On 02/21/2014 02:25 PM, Kishon Vijay Abraham I wrote:
> >> Hi Roger,
> >>
> >> On Wednesday 19 February 2014 06:07 PM, Roger Quadros wrote:
> >>> Hi,
> >>>
> >>> On 02/12/2014 11:46 AM, Kishon Vijay Abraham I wrote:
> >>>> On Wednesday 29 January 2014 08:17 PM, Heikki Krogerus wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Tue, Jan 28, 2014 at 10:30:36AM -0600, Felipe Balbi wrote:
> >>>>>> On Tue, Jan 28, 2014 at 05:32:30PM +0200, Heikki Krogerus wrote:
> >>>>>>> On Mon, Jan 27, 2014 at 10:05:20AM -0600, Felipe Balbi wrote:
> >>>>>>> For the controller drivers the PHYs are just a resource like any
> >>>>>>> other. The controller drivers can't have any responsibility of
> >>>>>>> them. They should not care if PHY drivers are available for them or
> >>>>>>> not, or even if the PHY framework is available or not.
> >>>>>>
> >>>>>> huh? If memory isn't available you don't continue probing, right ? If
> >>>>>> your IORESOURCE_MEM is missing, you also don't continue probing, if your
> >>>>>> IRQ line is missing, you bail too. Those are also nothing but resources
> >>>>>> to the driver, what you're asking here is to treat PHY as a _different_
> >>>>>> resource; which might be fine, but we need to make sure we don't
> >>>>>> continue probing when a PHY is missing in a platform that certainly
> >>>>>> needs a PHY.
> >>>>>
> >>>>> Yes, true. In my head I was comparing the PHY only to resources like
> >>>>> gpios, clocks, dma channels, etc. that are often optional to the
> >>>>> drivers.
> >>>>>
> >>>>>>>>>> But I really want to see the argument against using no-op. As far as I
> >>>>>>>>>> could see, everybody needs a PHY driver one way or another, some
> >>>>>>>>>> platforms just haven't sent any PHY driver upstream and have their own
> >>>>>>>>>> hacked up solution to avoid using the PHY layer.
> >>>>>>>>>
> >>>>>>>>> Not true in our case. Platforms using Intel's SoCs and chip sets may
> >>>>>>>>> or may not have controllable USB PHY. Quite often they don't. The
> >>>>>>>>> Baytrails have usually ULPI PHY for USB2, but that does not mean they
> >>>>>>>>> provide any vendor specific functions or any need for a driver in any
> >>>>>>>>> case.
> >>>>>>>>
> >>>>>>>> that's different from what I heard.
> >>>>>>>
> >>>>>>> I don't know where you got that impression, but it's not true. The
> >>>>>>> Baytrail SoCs for example don't have internal USB PHYs, which means
> >>>>>>> the manufacturers using it can select what they want. So we have
> >>>>>>> boards where PHY driver(s) is needed and boards where it isn't.
> >>>>>>
> >>>>>> alright, that explains it ;-) So you have external USB2 and USB3 PHYs ?
> >>>>>> You have an external PIPE3 interface ? That's quite an achievement,
> >>>>>> kudos to your HW designers. Getting timing closure on PIPE3 is a
> >>>>>> difficult task.
> >>>>>
> >>>>> No, only the USB2 PHY is external. I'm giving you wrong information,
> >>>>> I'm sorry about that. Need to concentrate on what I'm writing.
> >>>>>
> >>>>> <snip>
> >>>>>
> >>>>>>> This is really good to get. We have some projects where we are dealing
> >>>>>>> with more embedded environments, like IVI, where the kernel should be
> >>>>>>> stripped of everything useless. Since the PHYs are autonomous, we
> >>>>>>> should be able to disable the PHY libraries/frameworks.
> >>>>>>
> >>>>>> hmmm, in that case it's a lot easier to treat. We can use
> >>>>>> ERR_PTR(-ENXIO) as an indication that the framework is disabled, or
> >>>>>> something like that.
> >>>>>>
> >>>>>> The difficult is really reliably supporting e.g. OMAP5 (which won't work
> >>>>>> without a PHY) and your BayTrail with autonomous PHYs. What can we use
> >>>>>> as an indication ?
> >>>>>
> >>>>> OMAP has it's own glue driver, so shouldn't it depend on the PHY
> >>>>> layer?
> >>>>
> >>>> right, but the PHY is connected to the dwc3 core and not to the glue.
> >>>>>
> >>>>>> I mean, I need to know that a particular platform depends on a PHY
> >>>>>> driver before I decide to return -EPROBE_DEFER or just assume the PHY
> >>>>>> isn't needed ;-)
> >>>>>
> >>>>> I don't think dwc3 (core) should care about that. The PHY layer needs
> >>>>> to tell us that. If the PHY driver that the platform depends is not
> >>>>> available yet, the PHY layer returns -EPROBE_DEFER and dwc3 ends up
> >>>>> returning -EPROBE_DEFER.
> >>>>
> >>>> I don't think the PHY layer can 'reliably' tell if PHY driver is available or
> >>>> not. Consider when the phy_provider_register fails, there is no way to know if
> >>>> PHY driver is available or not. There are a few cases where PHY layer returns
> >>>> -EPROBE_DEFER but none of them can tell for sure that PHY driver is either
> >>>> available and failed or not available at all. It would be best for us to leave
> >>>> that to the platforms if we want to be sure if the platform needs a PHY or not.
> >>>>
> >>>
> >>> Just to summarize this thread on what we need
> >>
> >> Thanks for summarizing.
> >>>
> >>> 1) dwc3 core shouldn't worry about platform specific stuff i.e.
> >>> PHY needed or not.  It should be as generic as possible.
> >>>
> >>> 2) dwc3 core should continue probe even if PHY layer is not
> >>> enabled, as not all platforms need it.
> >>>
> >>> 3) dwc3 core should continue probe if PHY device is not available.
> >>> (-ENODEV?)
> >>>
> >>> 4) dwc3 core should error out on any error condition if PHY device
> >>> is available and caused some error, e.g. init error.
> >>>
> >>> 5) dwc3 core should return EPROBE_DEFER if PHY device is available
> >>> but device driver is not yet loaded.
> >>>
> >>> 6) platform glue should do the necessary sanity checks for
> >>> availability of all resources like PHY device, PHY layer, etc,
> >>> before populating the dwc3 device. e.g. in OMAP5 case we could
> >>> check if both usb2 and usb3 PHY nodes are available in the DT and
> >>> PHY layer is enabled, from dwc3-omap.c? In J6 case we could check
> >>> that at least usb2 phy node is there for the High-Speed only
> >>> controller, and so on.
> >>
> >> The PHY is connected to the dwc3 core. So I'm not sure if we should
> >> be doing checks for PHY in the glue layer.
> > 
> > Sorry, I didn't get you. My reasoning was that since OMAP platform
> > has this strict requirement of requiring explicit PHY control in
> > order to work, we must do the sanity checks in OMAP specific code
> > and not in the dwc3 core code. It has nothing to do with how
> > hardware is laid out.
> 
> What kind of sanity check do you think can be done in OMAP code? We don't use
> any of the PHY API's in glue code. If we add the same PHY APIs in glue code it
> will be duplication of the same code without much value besides breaking the
> design guideline of the software to be modelled similar to hardware.
> 
> However in Kconfig of dwc3 glue we can add 'select GENERIC_PHY, select
> PHY_OMAP_USB2, select OMAP_USB3' I guess.

ick, please don't. We have suffered too much from selects being
sprinkled all over the place.

-- 
balbi

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