git: 8a8166e5bcfb - main - mmc: switch mmc_helper to device_ api
Jessica Clarke
jrtc27 at freebsd.org
Tue Sep 28 18:00:43 UTC 2021
On 20 Sep 2021, at 16:18, Marcin Wojtas <mw at FreeBSD.org> wrote:
>
> The branch main has been updated by mw:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=8a8166e5bcfb50e2b7280581b600d098fa6c9fc7
>
> commit 8a8166e5bcfb50e2b7280581b600d098fa6c9fc7
> Author: Bartlomiej Grzesik <bag at semihalf.com>
> AuthorDate: 2021-08-02 14:27:23 +0000
> Commit: Marcin Wojtas <mw at FreeBSD.org>
> CommitDate: 2021-09-20 15:18:02 +0000
>
> mmc: switch mmc_helper to device_ api
>
> Add generic mmc_helper which uses newly introduced device_*_property
> api. Thanks to this change the sd/mmc drivers will be capable
> of parsing both DT and ACPI description.
>
> Ensure backward compatibility for all mmc_fdt_helper users.
>
> Reviewed by: manu, mw
> Sponsored by: Semihalf
> Differential revision: https://reviews.freebsd.org/D31598
> ---
> sys/arm/allwinner/aw_mmc.c | 2 +-
> sys/arm/broadcom/bcm2835/bcm2835_sdhci.c | 2 +-
> sys/conf/files | 1 +
> sys/dev/mmc/host/dwmmc_var.h | 2 +-
> sys/dev/mmc/mmc_fdt_helpers.c | 116 ++++----------------------
> sys/dev/mmc/mmc_fdt_helpers.h | 42 ++--------
> sys/dev/mmc/mmc_helpers.c | 134 +++++++++++++++++++++++++++++++
> sys/dev/mmc/mmc_helpers.h | 70 ++++++++++++++++
> sys/dev/sdhci/sdhci_fsl_fdt.c | 2 +-
> sys/dev/sdhci/sdhci_xenon.c | 2 +-
> 10 files changed, 234 insertions(+), 139 deletions(-)
>
> diff --git a/sys/arm/allwinner/aw_mmc.c b/sys/arm/allwinner/aw_mmc.c
> index 9f215399e62b..3271090f19e3 100644
> --- a/sys/arm/allwinner/aw_mmc.c
> +++ b/sys/arm/allwinner/aw_mmc.c
> @@ -130,7 +130,7 @@ struct aw_mmc_softc {
> int aw_timeout;
> struct callout aw_timeoutc;
> struct mmc_host aw_host;
> - struct mmc_fdt_helper mmc_helper;
> + struct mmc_helper mmc_helper;
> #ifdef MMCCAM
> union ccb * ccb;
> struct mmc_sim mmc_sim;
> diff --git a/sys/arm/broadcom/bcm2835/bcm2835_sdhci.c b/sys/arm/broadcom/bcm2835/bcm2835_sdhci.c
> index cd9b60743be3..38617dcd38eb 100644
> --- a/sys/arm/broadcom/bcm2835/bcm2835_sdhci.c
> +++ b/sys/arm/broadcom/bcm2835/bcm2835_sdhci.c
> @@ -156,7 +156,7 @@ struct bcm_sdhci_softc {
> void * sc_intrhand;
> struct mmc_request * sc_req;
> struct sdhci_slot sc_slot;
> - struct mmc_fdt_helper sc_mmc_helper;
> + struct mmc_helper sc_mmc_helper;
> int sc_dma_ch;
> bus_dma_tag_t sc_dma_tag;
> bus_dmamap_t sc_dma_map;
> diff --git a/sys/conf/files b/sys/conf/files
> index bf1c680093d1..1d3a36ac6bc7 100644
> --- a/sys/conf/files
> +++ b/sys/conf/files
> @@ -2483,6 +2483,7 @@ dev/mmc/mmcbr_if.m standard
> dev/mmc/mmcbus_if.m standard
> dev/mmc/mmcsd.c optional mmcsd !mmccam
> dev/mmc/mmc_fdt_helpers.c optional ext_resources mmc fdt | ext_resources mmccam fdt
> +dev/mmc/mmc_helpers.c optional ext_resources mmc | ext_resources mmccam
> dev/mmc/mmc_pwrseq.c optional ext_resources mmc fdt | ext_resources mmccam fdt
> dev/mmc/mmc_pwrseq_if.m optional ext_resources mmc fdt | ext_resources mmccam fdt
> dev/mmcnull/mmcnull.c optional mmcnull
> diff --git a/sys/dev/mmc/host/dwmmc_var.h b/sys/dev/mmc/host/dwmmc_var.h
> index ef7c91fa628e..a3f20278ad2a 100644
> --- a/sys/dev/mmc/host/dwmmc_var.h
> +++ b/sys/dev/mmc/host/dwmmc_var.h
> @@ -56,7 +56,7 @@ struct dwmmc_softc {
> device_t dev;
> void *intr_cookie;
> struct mmc_host host;
> - struct mmc_fdt_helper mmc_helper;
> + struct mmc_helper mmc_helper;
> struct mtx sc_mtx;
> #ifdef MMCCAM
> union ccb * ccb;
> diff --git a/sys/dev/mmc/mmc_fdt_helpers.c b/sys/dev/mmc/mmc_fdt_helpers.c
> index 291a4bc72ff2..249a430311fa 100644
> --- a/sys/dev/mmc/mmc_fdt_helpers.c
> +++ b/sys/dev/mmc/mmc_fdt_helpers.c
> @@ -45,105 +45,21 @@ __FBSDID("$FreeBSD$");
> #include <dev/extres/regulator/regulator.h>
> #endif
>
> -#include "mmc_pwrseq_if.h"
> -
> -static inline void
> -mmc_fdt_parse_sd_speed(phandle_t node, struct mmc_host *host)
> -{
> - bool no_18v = false;
> -
> - /*
> - * Parse SD supported modes
> - * All UHS-I modes requires 1.8V signaling.
> - */
> - if (OF_hasprop(node, "no-1-8-v"))
> - no_18v = true;
> - if (OF_hasprop(node, "cap-sd-highspeed"))
> - host->caps |= MMC_CAP_HSPEED;
> - if (OF_hasprop(node, "sd-uhs-sdr12") && no_18v == false)
> - host->caps |= MMC_CAP_UHS_SDR12 | MMC_CAP_SIGNALING_180;
> - if (OF_hasprop(node, "sd-uhs-sdr25") && no_18v == false)
> - host->caps |= MMC_CAP_UHS_SDR25 | MMC_CAP_SIGNALING_180;
> - if (OF_hasprop(node, "sd-uhs-sdr50") && no_18v == false)
> - host->caps |= MMC_CAP_UHS_SDR50 | MMC_CAP_SIGNALING_180;
> - if (OF_hasprop(node, "sd-uhs-sdr104") && no_18v == false)
> - host->caps |= MMC_CAP_UHS_SDR104 | MMC_CAP_SIGNALING_180;
> - if (OF_hasprop(node, "sd-uhs-ddr50") && no_18v == false)
> - host->caps |= MMC_CAP_UHS_DDR50 | MMC_CAP_SIGNALING_180;
> -}
> +#include <dev/mmc/mmc_helpers.h>
>
> -static inline void
> -mmc_fdt_parse_mmc_speed(phandle_t node, struct mmc_host *host)
> -{
> -
> - /* Parse eMMC supported modes */
> - if (OF_hasprop(node, "cap-mmc-highspeed"))
> - host->caps |= MMC_CAP_HSPEED;
> - if (OF_hasprop(node, "mmc-ddr-1_2v"))
> - host->caps |= MMC_CAP_MMC_DDR52_120 | MMC_CAP_SIGNALING_120;
> - if (OF_hasprop(node, "mmc-ddr-1_8v"))
> - host->caps |= MMC_CAP_MMC_DDR52_180 | MMC_CAP_SIGNALING_180;
> - if (OF_hasprop(node, "mmc-ddr-3_3v"))
> - host->caps |= MMC_CAP_SIGNALING_330;
> - if (OF_hasprop(node, "mmc-hs200-1_2v"))
> - host->caps |= MMC_CAP_MMC_HS200_120 | MMC_CAP_SIGNALING_120;
> - if (OF_hasprop(node, "mmc-hs200-1_8v"))
> - host->caps |= MMC_CAP_MMC_HS200_180 | MMC_CAP_SIGNALING_180;
> - if (OF_hasprop(node, "mmc-hs400-1_2v"))
> - host->caps |= MMC_CAP_MMC_HS400_120 | MMC_CAP_SIGNALING_120;
> - if (OF_hasprop(node, "mmc-hs400-1_8v"))
> - host->caps |= MMC_CAP_MMC_HS400_180 | MMC_CAP_SIGNALING_180;
> - if (OF_hasprop(node, "mmc-hs400-enhanced-strobe"))
> - host->caps |= MMC_CAP_MMC_ENH_STROBE;
> -}
> +#include "mmc_pwrseq_if.h"
>
> int
> -mmc_fdt_parse(device_t dev, phandle_t node, struct mmc_fdt_helper *helper,
> +mmc_fdt_parse(device_t dev, phandle_t node, struct mmc_helper *helper,
> struct mmc_host *host)
> {
> - uint32_t bus_width;
> + struct mmc_helper mmc_helper;
> phandle_t pwrseq_xref;
>
> - if (node <= 0)
> - node = ofw_bus_get_node(dev);
> - if (node <= 0)
> - return (ENXIO);
> -
> - if (OF_getencprop(node, "bus-width", &bus_width, sizeof(uint32_t)) <= 0)
> - bus_width = 1;
> -
> - if (bus_width >= 4)
> - host->caps |= MMC_CAP_4_BIT_DATA;
> - if (bus_width >= 8)
> - host->caps |= MMC_CAP_8_BIT_DATA;
> + memset(&mmc_helper, 0, sizeof(mmc_helper));
> + mmc_parse(dev, &mmc_helper, host);
>
> - /*
> - * max-frequency is optional, drivers should tweak this value
> - * if it's not present based on the clock that the mmc controller
> - * operates on
> - */
> - OF_getencprop(node, "max-frequency", &host->f_max, sizeof(uint32_t));
> -
> - if (OF_hasprop(node, "broken-cd"))
> - helper->props |= MMC_PROP_BROKEN_CD;
> - if (OF_hasprop(node, "non-removable"))
> - helper->props |= MMC_PROP_NON_REMOVABLE;
> - if (OF_hasprop(node, "wp-inverted"))
> - helper->props |= MMC_PROP_WP_INVERTED;
> - if (OF_hasprop(node, "cd-inverted"))
> - helper->props |= MMC_PROP_CD_INVERTED;
> - if (OF_hasprop(node, "no-sdio"))
> - helper->props |= MMC_PROP_NO_SDIO;
> - if (OF_hasprop(node, "no-sd"))
> - helper->props |= MMC_PROP_NO_SD;
> - if (OF_hasprop(node, "no-mmc"))
> - helper->props |= MMC_PROP_NO_MMC;
> -
> - if (!(helper->props & MMC_PROP_NO_SD))
> - mmc_fdt_parse_sd_speed(node, host);
> -
> - if (!(helper->props & MMC_PROP_NO_MMC))
> - mmc_fdt_parse_mmc_speed(node, host);
> + helper->props = mmc_helper.props;
>
> #ifdef EXT_RESOURCES
> /*
> @@ -200,7 +116,7 @@ mmc_fdt_parse(device_t dev, phandle_t node, struct mmc_fdt_helper *helper,
> static void
> cd_intr(void *arg)
> {
> - struct mmc_fdt_helper *helper = arg;
> + struct mmc_helper *helper = arg;
>
> taskqueue_enqueue_timeout(taskqueue_swi_giant,
> &helper->cd_delayed_task, -(hz / 2));
> @@ -209,7 +125,7 @@ cd_intr(void *arg)
> static void
> cd_card_task(void *arg, int pending __unused)
> {
> - struct mmc_fdt_helper *helper = arg;
> + struct mmc_helper *helper = arg;
> bool cd_present;
>
> cd_present = mmc_fdt_gpio_get_present(helper);
> @@ -228,7 +144,7 @@ cd_card_task(void *arg, int pending __unused)
> * Card detect setup.
> */
> static void
> -cd_setup(struct mmc_fdt_helper *helper, phandle_t node)
> +cd_setup(struct mmc_helper *helper, phandle_t node)
> {
> int pincaps;
> device_t dev;
> @@ -330,7 +246,7 @@ without_interrupts:
> * Write protect setup.
> */
> static void
> -wp_setup(struct mmc_fdt_helper *helper, phandle_t node)
> +wp_setup(struct mmc_helper *helper, phandle_t node)
> {
> device_t dev;
>
> @@ -352,7 +268,7 @@ wp_setup(struct mmc_fdt_helper *helper, phandle_t node)
> }
>
> int
> -mmc_fdt_gpio_setup(device_t dev, phandle_t node, struct mmc_fdt_helper *helper,
> +mmc_fdt_gpio_setup(device_t dev, phandle_t node, struct mmc_helper *helper,
> mmc_fdt_cd_handler handler)
> {
>
> @@ -377,7 +293,7 @@ mmc_fdt_gpio_setup(device_t dev, phandle_t node, struct mmc_fdt_helper *helper,
> }
>
> void
> -mmc_fdt_gpio_teardown(struct mmc_fdt_helper *helper)
> +mmc_fdt_gpio_teardown(struct mmc_helper *helper)
> {
>
> if (helper == NULL)
> @@ -396,7 +312,7 @@ mmc_fdt_gpio_teardown(struct mmc_fdt_helper *helper)
> }
>
> bool
> -mmc_fdt_gpio_get_present(struct mmc_fdt_helper *helper)
> +mmc_fdt_gpio_get_present(struct mmc_helper *helper)
> {
> bool pinstate;
>
> @@ -411,7 +327,7 @@ mmc_fdt_gpio_get_present(struct mmc_fdt_helper *helper)
> }
>
> bool
> -mmc_fdt_gpio_get_readonly(struct mmc_fdt_helper *helper)
> +mmc_fdt_gpio_get_readonly(struct mmc_helper *helper)
> {
> bool pinstate;
>
> @@ -427,7 +343,7 @@ mmc_fdt_gpio_get_readonly(struct mmc_fdt_helper *helper)
> }
>
> void
> -mmc_fdt_set_power(struct mmc_fdt_helper *helper, enum mmc_power_mode power_mode)
> +mmc_fdt_set_power(struct mmc_helper *helper, enum mmc_power_mode power_mode)
> {
> int reg_status;
> int rv;
> diff --git a/sys/dev/mmc/mmc_fdt_helpers.h b/sys/dev/mmc/mmc_fdt_helpers.h
> index e6d6f3fbfd84..f07ca1684440 100644
> --- a/sys/dev/mmc/mmc_fdt_helpers.h
> +++ b/sys/dev/mmc/mmc_fdt_helpers.h
> @@ -37,43 +37,17 @@
> #include <dev/extres/regulator/regulator.h>
> #endif
>
> -struct mmc_fdt_helper {
> - device_t dev;
> - gpio_pin_t wp_pin;
> - gpio_pin_t cd_pin;
> - void * cd_ihandler;
> - struct resource * cd_ires;
> - int cd_irid;
> - void (*cd_handler)(device_t, bool);
> - struct timeout_task cd_delayed_task;
> - bool cd_disabled;
> - bool wp_disabled;
> - bool cd_present;
> - uint32_t props;
> -#define MMC_PROP_BROKEN_CD (1 << 0)
> -#define MMC_PROP_NON_REMOVABLE (1 << 1)
> -#define MMC_PROP_WP_INVERTED (1 << 2)
> -#define MMC_PROP_CD_INVERTED (1 << 3)
> -#define MMC_PROP_DISABLE_WP (1 << 4)
> -#define MMC_PROP_NO_SDIO (1 << 5)
> -#define MMC_PROP_NO_SD (1 << 6)
> -#define MMC_PROP_NO_MMC (1 << 7)
> +#include <dev/mmc/mmc_helpers.h>
>
> -#ifdef EXT_RESOURCES
> - regulator_t vmmc_supply;
> - regulator_t vqmmc_supply;
> -#endif
> -
> - device_t mmc_pwrseq;
> -};
> +#define mmc_fdt_helper mmc_helper /* For backwards compatibility */
>
> typedef void (*mmc_fdt_cd_handler)(device_t dev, bool present);
>
> -int mmc_fdt_parse(device_t dev, phandle_t node, struct mmc_fdt_helper *helper, struct mmc_host *host);
> -int mmc_fdt_gpio_setup(device_t dev, phandle_t node, struct mmc_fdt_helper *helper, mmc_fdt_cd_handler handler);
> -void mmc_fdt_gpio_teardown(struct mmc_fdt_helper *helper);
> -bool mmc_fdt_gpio_get_present(struct mmc_fdt_helper *helper);
> -bool mmc_fdt_gpio_get_readonly(struct mmc_fdt_helper *helper);
> -void mmc_fdt_set_power(struct mmc_fdt_helper *helper, enum mmc_power_mode power_mode);
> +int mmc_fdt_parse(device_t dev, phandle_t node, struct mmc_helper *helper, struct mmc_host *host);
> +int mmc_fdt_gpio_setup(device_t dev, phandle_t node, struct mmc_helper *helper, mmc_fdt_cd_handler handler);
> +void mmc_fdt_gpio_teardown(struct mmc_helper *helper);
> +bool mmc_fdt_gpio_get_present(struct mmc_helper *helper);
> +bool mmc_fdt_gpio_get_readonly(struct mmc_helper *helper);
> +void mmc_fdt_set_power(struct mmc_helper *helper, enum mmc_power_mode power_mode);
>
> #endif
> diff --git a/sys/dev/mmc/mmc_helpers.c b/sys/dev/mmc/mmc_helpers.c
> new file mode 100644
> index 000000000000..0e1e16666367
> --- /dev/null
> +++ b/sys/dev/mmc/mmc_helpers.c
> @@ -0,0 +1,134 @@
> +/*
> + * Copyright 2019 Emmanuel Vadot <manu at freebsd.org>
> + * Copyright (c) 2017 Ian Lepore <ian at freebsd.org> All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
> + * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
> + * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
> + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <sys/cdefs.h>
> +__FBSDID("$FreeBSD$");
> +
> +#include <sys/param.h>
> +#include <sys/bus.h>
> +#include <sys/kernel.h>
> +#include <sys/gpio.h>
> +#include <sys/taskqueue.h>
> +
> +#include <dev/mmc/bridge.h>
> +#include <dev/mmc/mmc_helpers.h>
> +
> +static inline void
> +mmc_parse_sd_speed(device_t dev, struct mmc_host *host)
> +{
> + bool no_18v = false;
> +
> + /*
> + * Parse SD supported modes
> + * All UHS-I modes requires 1.8V signaling.
> + */
> + if (device_has_property(dev, "no-1-8-v"))
> + no_18v = true;
> + if (device_has_property(dev, "cap-sd-highspeed"))
> + host->caps |= MMC_CAP_HSPEED;
> + if (device_has_property(dev, "sd-uhs-sdr12") && !no_18v)
> + host->caps |= MMC_CAP_UHS_SDR12 | MMC_CAP_SIGNALING_180;
> + if (device_has_property(dev, "sd-uhs-sdr25") && !no_18v)
> + host->caps |= MMC_CAP_UHS_SDR25 | MMC_CAP_SIGNALING_180;
> + if (device_has_property(dev, "sd-uhs-sdr50") && !no_18v)
> + host->caps |= MMC_CAP_UHS_SDR50 | MMC_CAP_SIGNALING_180;
> + if (device_has_property(dev, "sd-uhs-sdr104") && !no_18v)
> + host->caps |= MMC_CAP_UHS_SDR104 | MMC_CAP_SIGNALING_180;
> + if (device_has_property(dev, "sd-uhs-ddr50") && !no_18v)
> + host->caps |= MMC_CAP_UHS_DDR50 | MMC_CAP_SIGNALING_180;
> +}
> +
> +static inline void
> +mmc_parse_mmc_speed(device_t dev, struct mmc_host *host)
> +{
> + /* Parse eMMC supported modes */
> + if (device_has_property(dev, "cap-mmc-highspeed"))
> + host->caps |= MMC_CAP_HSPEED;
> + if (device_has_property(dev, "mmc-ddr-1_2v"))
> + host->caps |= MMC_CAP_MMC_DDR52_120 | MMC_CAP_SIGNALING_120;
> + if (device_has_property(dev, "mmc-ddr-1_8v"))
> + host->caps |= MMC_CAP_MMC_DDR52_180 | MMC_CAP_SIGNALING_180;
> + if (device_has_property(dev, "mmc-ddr-3_3v"))
> + host->caps |= MMC_CAP_SIGNALING_330;
> + if (device_has_property(dev, "mmc-hs200-1_2v"))
> + host->caps |= MMC_CAP_MMC_HS200_120 | MMC_CAP_SIGNALING_120;
> + if (device_has_property(dev, "mmc-hs200-1_8v"))
> + host->caps |= MMC_CAP_MMC_HS200_180 | MMC_CAP_SIGNALING_180;
> + if (device_has_property(dev, "mmc-hs400-1_2v"))
> + host->caps |= MMC_CAP_MMC_HS400_120 | MMC_CAP_SIGNALING_120;
> + if (device_has_property(dev, "mmc-hs400-1_8v"))
> + host->caps |= MMC_CAP_MMC_HS400_180 | MMC_CAP_SIGNALING_180;
> + if (device_has_property(dev, "mmc-hs400-enhanced-strobe"))
> + host->caps |= MMC_CAP_MMC_ENH_STROBE;
> +}
> +
> +int
> +mmc_parse(device_t dev, struct mmc_helper *helper, struct mmc_host *host)
> +{
> + uint64_t bus_width, max_freq;
> +
> + bus_width = 0;
> + if (device_get_property(dev, "bus-width", &bus_width, sizeof(uint64_t)) <= 0)
> + bus_width = 1;
> +
> + if (bus_width >= 4)
> + host->caps |= MMC_CAP_4_BIT_DATA;
> + if (bus_width >= 8)
> + host->caps |= MMC_CAP_8_BIT_DATA;
> +
> + /*
> + * max-frequency is optional, drivers should tweak this value
> + * if it's not present based on the clock that the mmc controller
> + * operates on
> + */
> + max_freq = 0;
> + device_get_property(dev, "max-frequency", &max_freq, sizeof(uint64_t));
> + host->f_max = max_freq;
Previously OF_getencprop was called with &host->f_max so it would only
be written to if it existed. Now it is instead written to with the
value of 0 if the property doesn’t exist. Both aw_mmc and dwmmc set
f_max to a default value *before* calling mmc_fdt_parse so that is now
clobbered. This commit breaks booting on the Stratix 10 which uses
dwmmc (boot hangs at the point where da0 would normally attach), and
reverting it succeeds. I don’t know if f_max being clobbered is the
cause of that (I’ll try tomorrow), but it seems likely, and is
certainly a bug in this commit.
Jess
More information about the dev-commits-src-all
mailing list