svn commit: r353493 - head/sys/dev/mmc/host

[Emmanuel Vadot]

On Wed, 16 Oct 2019 16:06:34 +0100
Ruslan Bukin <br at freebsd.org> wrote:

> On Mon, Oct 14, 2019 at 06:45:26PM +0200, Emmanuel Vadot wrote:
> > On Mon, 14 Oct 2019 17:27:51 +0100
> > Ruslan Bukin <br at freebsd.org> wrote:
> > 
> > > On Mon, Oct 14, 2019 at 06:10:51PM +0200, Emmanuel Vadot wrote:
> > > > 
> > > > On Mon, 14 Oct 2019 15:53:00 +0000 (UTC)
> > > > Ruslan Bukin <br at FreeBSD.org> wrote:
> > > > 
> > > > > Author: br
> > > > > Date: Mon Oct 14 15:52:59 2019
> > > > > New Revision: 353493
> > > > > URL: https://svnweb.freebsd.org/changeset/base/353493
> > > > > 
> > > > > Log:
> > > > >   Fix the driver attachment in cases when the external resource devices
> > > > >   (resets, regulators, clocks) are not available.
> > > > >   
> > > > >   Rely on a system initialization done by a bootloader in that cases.
> > > > >   
> > > > >   This fixes operation on Terasic DE10-Pro (an Intel Stratix 10
> > > > >   development kit).
> > > > >   
> > > > >   Sponsored by:	DARPA, AFRL
> > > > > 
> > > > > Modified:
> > > > >   head/sys/dev/mmc/host/dwmmc.c
> > > > > 
> > > > > Modified: head/sys/dev/mmc/host/dwmmc.c
> > > > > ==============================================================================
> > > > > --- head/sys/dev/mmc/host/dwmmc.c	Mon Oct 14 15:33:53 2019	(r353492)
> > > > > +++ head/sys/dev/mmc/host/dwmmc.c	Mon Oct 14 15:52:59 2019	(r353493)
> > > > > @@ -1,5 +1,5 @@
> > > > >  /*-
> > > > > - * Copyright (c) 2014 Ruslan Bukin <br at bsdpad.com>
> > > > > + * Copyright (c) 2014-2019 Ruslan Bukin <br at bsdpad.com>
> > > > >   * All rights reserved.
> > > > >   *
> > > > >   * This software was developed by SRI International and the University of
> > > > > @@ -457,26 +457,20 @@ parse_fdt(struct dwmmc_softc *sc)
> > > > >  
> > > > >  	/* IP block reset is optional */
> > > > >  	error = hwreset_get_by_ofw_name(sc->dev, 0, "reset", &sc->hwreset);
> > > > > -	if (error != 0 && error != ENOENT) {
> > > > > +	if (error != 0 && error != ENOENT)
> > > > >  		device_printf(sc->dev, "Cannot get reset\n");
> > > > > -		goto fail;
> > > > > -	}
> > > > 
> > > >  This is not correct, on a system without reset/clock/regulator support
> > > > you will get ENODEV as the phandle is present but no device is
> > > > associated with it. This is the case that you want to test. Currently
> > > > this hide all errors. 
> > > 
> > > The change means that the driver will be attached regardless of the return value from ext resources.
> > 
> >  Yes and this is a problem.
> > 
> > > Why it is not correct?
> > 
> >  Because if a reset/clock/regulator is present but we failed to parse
> > the node (bad #clock-cell for example) this just assume that the
> > resource isn't present which is not correct. Or worse the underlying
> > gpio cannot be mapped, you now have a non functional driver because you
> > cannot switch the regulator.
> > 
> 
> Could an error in DTS convert a device from being optional to become
> mandatory? It is still not clear if any support present on a stage of
> parsing.
> 
> Since the knowledge of registered support is at extres, then it should
> be mandatory from a driver perspective (if enabled in kernel config).
> And then the framework should make a decision based on any support present.
> 
> What do you think ?
> 
> Ruslan

 I didnt' quite understood what you are suggesting ? Could you
elaborate a bit more please ?

-- 
Emmanuel Vadot <manu at bidouilliste.com>