From: Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: sdhc(4): zap Intel bus power quirk
To: James Hastings <moosetek4@gmail.com>
Cc: moosetek4@gmail.com,tech@openbsd.org
Date: Tue, 24 Sep 2024 10:34:43 +0200

> From: James Hastings <moosetek4@gmail.com>
> Date: Sun, 22 Sep 2024 02:00:51 -0400 (EDT)
> 
> Diff below addresses issue with Intel SD controllers
> failing to enable SD cards and eMMC devices.
> 
>   sdmmc0: can't enable card
> 
> This happens after sdmmc(4) makes a bus_power request for the
> same voltage that the card is currently operating.
> 
> Change sdhc_bus_power() behavior to return successfully and not
> do a power-off voltage switch sequence if both conditions are met:
>  1) bus is powered on.
>  2) bus is already at requested voltage.
> 
> With this change we can zap the Intel NOPWR0 quirk.
> 
> Tested on 400-series-LP (SD) and Apollo Lake (SD*,eMMC) SoCs.
> (* card probed only at boot; needs GPIO card detect interrupt.)
> 
> ok?

The diff makes sense to me, but this needs to be tested on a wider
range of hardware, especially on some of the arm/arm64 machines where
this code is used for the boot/root device.

(I don't feel confident pushing this into the release last-minute).

> Index: dev/sdmmc/sdhc.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/sdmmc/sdhc.c,v
> retrieving revision 1.77
> diff -u -p -r1.77 sdhc.c
> --- dev/sdmmc/sdhc.c	6 Aug 2024 15:03:36 -0000	1.77
> +++ dev/sdmmc/sdhc.c	20 Sep 2024 09:30:06 -0000
> @@ -591,14 +591,9 @@ sdhc_bus_power(sdmmc_chipset_handle_t sc
>  
>  	s = splsdmmc();
>  
> -	/*
> -	 * Disable bus power before voltage change.
> -	 */
> -	if (!(hp->sc->sc_flags & SDHC_F_NOPWR0))
> -		HWRITE1(hp, SDHC_POWER_CTL, 0);
> -
>  	/* If power is disabled, reset the host and return now. */
>  	if (ocr == 0) {
> +		HWRITE1(hp, SDHC_POWER_CTL, 0);
>  		splx(s);
>  		(void)sdhc_host_reset(hp);
>  		return 0;
> @@ -619,6 +614,21 @@ sdhc_bus_power(sdmmc_chipset_handle_t sc
>  		splx(s);
>  		return EINVAL;
>  	}
> +
> +	/*
> +	 * Return if no change to powered bus voltage.
> +	 */
> +	if (HREAD1(hp, SDHC_POWER_CTL) ==
> +	    ((vdd << SDHC_VOLTAGE_SHIFT) | SDHC_BUS_POWER)) {
> +		splx(s);
> +		return 0;
> +	}
> +
> +	/*
> +	 * Disable bus power before voltage change.
> +	 */
> +	if (!(hp->sc->sc_flags & SDHC_F_NOPWR0))
> +		HWRITE1(hp, SDHC_POWER_CTL, 0);
>  
>  	/*
>  	 * Enable bus power.  Wait at least 1 ms (or 74 clocks) plus
> Index: dev/pci/sdhc_pci.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/sdhc_pci.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 sdhc_pci.c
> --- dev/pci/sdhc_pci.c	29 Mar 2024 02:36:49 -0000	1.26
> +++ dev/pci/sdhc_pci.c	20 Sep 2024 09:30:06 -0000
> @@ -126,16 +126,6 @@ sdhc_pci_attach(struct device *parent, s
>  	    PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_ENE_SDCARD)
>  		sc->sc.sc_flags |= SDHC_F_NOPWR0;
>  
> -	/* Some Intel controllers break if set to 0V bus power. */
> -	if (PCI_VENDOR(pa->pa_id) == PCI_VENDOR_INTEL &&
> -	    (PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_INTEL_100SERIES_LP_EMMC ||
> -	    PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_INTEL_APOLLOLAKE_EMMC ||
> -	    PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_INTEL_GLK_EMMC ||
> -	    PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_INTEL_JSL_EMMC ||
> -	    PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_INTEL_EHL_EMMC ||
> -	    PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_INTEL_ADL_N_EMMC))
> -		sc->sc.sc_flags |= SDHC_F_NOPWR0;
> -
>  	/* Some RICOH controllers need to be bumped into the right mode. */
>  	if (PCI_VENDOR(pa->pa_id) == PCI_VENDOR_RICOH &&
>  	    (PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_RICOH_R5U822 ||
> 
>