[PATCH v2 2/2] MMC: meson: initial support for GXBB platforms

[khilman@baylibre.com (Kevin Hilman)]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On 4 August 2016 at 01:18, Kevin Hilman <khilman@baylibre.com> wrote:
>> Initial support for the SD/eMMC controller in the Amlogic S905/GXBB
>> family of SoCs.
>>
>> Currently working for the SD and eMMC interfaces, but not yet tested
>> for SDIO.
>>
>> Tested external SD card and internal eMMC on meson-gxbb-p200 and
>> meson-gxbb-odroidc2.
>>
>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>> ---
>>  MAINTAINERS                   |   1 +
>>  drivers/mmc/host/Kconfig      |  10 +
>>  drivers/mmc/host/Makefile     |   1 +
>>  drivers/mmc/host/meson-gxbb.c | 918 ++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 930 insertions(+)
>>  create mode 100644 drivers/mmc/host/meson-gxbb.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7304d2e37a98..3762e46811f3 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -972,6 +972,7 @@ F:  arch/arm/mach-meson/
>>  F:     arch/arm/boot/dts/meson*
>>  F:     arch/arm64/boot/dts/amlogic/
>>  F:     drivers/pinctrl/meson/
>> +F:      drivers/mmc/host/meson*
>>  N:     meson
>>
>>  ARM/Annapurna Labs ALPINE ARCHITECTURE
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 0aa484c10c0a..4c3d091f7548 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -332,6 +332,16 @@ config MMC_SDHCI_IPROC
>>
>>           If unsure, say N.
>>
>> +config MMC_MESON_GXBB
>> +       tristate "Amlogic S905/GXBB SD/MMC Host Controller support"
>> +       depends on ARCH_MESON && MMC
>> +       help
>> +         This selects support for the Amlogic SD/MMC Host Controller
>> +         found on the S905/GXBB family of SoCs.  This controller is
>> +         MMC 5.1 compliant and support SD, eMMC and SDIO interfaces.
>> +
>> +         If you have a controller with this interface, say Y here.
>> +
>>  config MMC_MOXART
>>         tristate "MOXART SD/MMC Host Controller support"
>>         depends on ARCH_MOXART && MMC
>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>> index af918d261ff9..3e0de57f55b9 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -53,6 +53,7 @@ obj-$(CONFIG_MMC_JZ4740)      += jz4740_mmc.o
>>  obj-$(CONFIG_MMC_VUB300)       += vub300.o
>>  obj-$(CONFIG_MMC_USHC)         += ushc.o
>>  obj-$(CONFIG_MMC_WMT)          += wmt-sdmmc.o
>> +obj-$(CONFIG_MMC_MESON_GXBB)   += meson-gxbb.o
>>  obj-$(CONFIG_MMC_MOXART)       += moxart-mmc.o
>>  obj-$(CONFIG_MMC_SUNXI)                += sunxi-mmc.o
>>  obj-$(CONFIG_MMC_USDHI6ROL0)   += usdhi6rol0.o
>> diff --git a/drivers/mmc/host/meson-gxbb.c b/drivers/mmc/host/meson-gxbb.c
>> new file mode 100644
>> index 000000000000..10eac41cf31a
>> --- /dev/null
>> +++ b/drivers/mmc/host/meson-gxbb.c
>> @@ -0,0 +1,918 @@
>> +/*
>> + * This file is provided under a dual BSD/GPLv2 license.  When using or
>> + * redistributing this file, you may do so under either license.
>> + *
>> + * GPL LICENSE SUMMARY
>> + *
>> + * Copyright (c) 2016 BayLibre, SAS.
>> + * Author: Kevin Hilman <khilman@baylibre.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of version 2 of the GNU General Public License as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
>> + * The full GNU General Public License is included in this distribution
>> + * in the file called COPYING.
>> + *
>> + * BSD LICENSE
>> + *
>> + * Copyright (c) 2016 BayLibre, SAS.
>> + * Author: Kevin Hilman <khilman@baylibre.com>
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + *   * Redistributions of source code must retain the above copyright
>> + *     notice, this list of conditions and the following disclaimer.
>> + *   * 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.
>> + *   * Neither the name of Intel Corporation nor the names of its
>> + *     contributors may be used to endorse or promote products derived
>> + *     from this software without specific prior written permission.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS 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 COPYRIGHT
>> + * OWNER 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.
>> + */
>
> Lots of licence text. Isn't it enough to state dual BSD/GPLv2?
>
> [...]
>
>> +
>> +struct meson_host {
>> +       struct  device          *dev;
>> +       struct  mmc_host        *mmc;
>> +       struct  mmc_request     *mrq;
>> +       struct  mmc_command     *cmd;
>> +
>> +       spinlock_t lock;
>> +       void __iomem *regs;
>> +#ifdef USE_REGMAP
>> +       struct regmap *regmap;
>> +#endif
>> +       int irq;
>> +       u32 ocr_mask;
>> +       struct clk *core_clk;
>> +       struct clk_mux mux;
>> +       struct clk *mux_clk;
>> +       struct clk *mux_parent[MUX_CLK_NUM_PARENTS];
>> +       unsigned long mux_parent_rate[MUX_CLK_NUM_PARENTS];
>> +
>> +       struct clk_divider cfg_div;
>> +       struct clk *cfg_div_clk;
>> +
>> +       unsigned int bounce_buf_size;
>> +       void *bounce_buf;
>> +       dma_addr_t bounce_dma_addr;
>> +
>> +       unsigned long clk_rate;
>> +       unsigned long clk_src_rate;
>> +       unsigned short clk_src_div;
>> +};
>> +
>> +#define reg_read(host, offset) readl(host->regs + offset)
>> +#define reg_write(host, offset, val) writel(val, host->regs + offset)
>
> I am not a fan of macros, especially when I don't think they improves the code.
>
> Could we just stick to use readl/writel() directly instead!?
>

Yes, I'll remove these.  They are leftover from when I was using macros
to switch between readl/writel and regmap accessors.

>
>> +static int meson_mmc_clk_set(struct meson_host *host, unsigned long clk_rate)
>> +{
>> +       struct mmc_host *mmc = host->mmc;
>> +       int ret = 0;
>> +       u32 cfg;
>> +
>> +       if (clk_rate) {
>> +               if (WARN_ON(clk_rate > mmc->f_max))
>> +                       clk_rate = mmc->f_max;
>> +               else if (WARN_ON(clk_rate < mmc->f_min))
>> +                       clk_rate = mmc->f_min;
>> +       }
>> +
>> +       if (clk_rate == host->clk_rate)
>> +               return 0;
>> +
>> +       /* stop clock */
>> +       cfg = reg_read(host, SD_EMMC_CFG);
>> +       if (!(cfg & CFG_STOP_CLOCK)) {
>> +               cfg |= CFG_STOP_CLOCK;
>> +               reg_write(host, SD_EMMC_CFG, cfg);
>> +       }
>> +
>> +       dev_dbg(host->dev, "change clock rate %lu -> %lu\n",
>> +               host->clk_rate, clk_rate);
>> +       ret = clk_set_rate(host->cfg_div_clk, clk_rate);
>> +       if (clk_rate && clk_rate != clk_get_rate(host->cfg_div_clk))
>> +               dev_warn(host->dev, "divider requested rate %lu != actual rate %lu: ret=%d\n",
>> +                        clk_rate, clk_get_rate(host->cfg_div_clk), ret);
>> +       else
>> +               host->clk_rate = clk_rate;
>> +
>> +       /* (re)start clock, if non-zero */
>> +       if (clk_rate) {
>> +               cfg = reg_read(host, SD_EMMC_CFG);
>> +               cfg &= ~CFG_STOP_CLOCK;
>> +               reg_write(host, SD_EMMC_CFG, cfg);
>> +       }
>
> You probably want to update mmc->actual_clock, which is useful for
> debugging purpose.

OK, I hadn't noticed that field.  I'll just drop my own host->clk_rate
and use that.

>> +
>> +       return ret;
>> +}
>> +
>> +static int meson_mmc_clk_init(struct meson_host *host)
>> +{
>> +       struct clk_init_data init;
>> +       char clk_name[32];
>> +       int i, ret = 0;
>> +       const char *mux_parent_names[MUX_CLK_NUM_PARENTS];
>> +       unsigned int mux_parent_count = 0;
>> +       const char *clk_div_parents[1];
>> +       unsigned int f_min = UINT_MAX;
>> +       u32 clk_reg, cfg;
>> +
>> +       /* get the mux parents from DT */
>> +       for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
>> +               char name[16];
>> +
>> +               snprintf(name, sizeof(name), "clkin%d", i);
>> +               host->mux_parent[i] = devm_clk_get(host->dev, name);
>> +               if (IS_ERR(host->mux_parent[i])) {
>> +                       ret = PTR_ERR(host->mux_parent[i]);
>> +                       if (PTR_ERR(host->mux_parent[i]) != -EPROBE_DEFER)
>> +                               dev_err(host->dev, "Missing clock %s\n", name);
>> +                       host->mux_parent[i] = NULL;
>> +                       return ret;
>> +               }
>> +
>> +               host->mux_parent_rate[i] = clk_get_rate(host->mux_parent[i]);
>> +               mux_parent_names[i] = __clk_get_name(host->mux_parent[i]);
>> +               mux_parent_count++;
>> +               if (host->mux_parent_rate[i] < f_min)
>> +                       f_min = host->mux_parent_rate[i];
>> +       }
>> +
>> +       /* cacluate f_min based on input clocks, and max divider value */
>> +       if (f_min != UINT_MAX)
>> +               f_min = DIV_ROUND_UP(CLK_SRC_XTAL_RATE, CLK_DIV_MAX);
>> +       else
>> +               f_min = 4000000;  /* default min: 400 MHz */
>> +       host->mmc->f_min = f_min;
>> +
>> +       /* create the mux */
>> +       snprintf(clk_name, sizeof(clk_name), "%s#mux", dev_name(host->dev));
>> +       init.name = clk_name;
>> +       init.ops = &clk_mux_ops;
>> +       init.flags = CLK_IS_BASIC;
>> +       init.parent_names = mux_parent_names;
>> +       init.num_parents = mux_parent_count;
>> +
>> +       host->mux.reg = host->regs + SD_EMMC_CLOCK;
>> +       host->mux.shift = CLK_SRC_SHIFT;
>> +       host->mux.mask = CLK_SRC_MASK;
>> +       host->mux.flags = 0;
>> +       host->mux.table = NULL;
>> +       host->mux.hw.init = &init;
>> +
>> +       host->mux_clk = devm_clk_register(host->dev, &host->mux.hw);
>
> I think it would make sense to add a comment here to clarify why you
> register a clock like this.
>
> I assume it's beacuse you benefit from the clock framwork about
> acheiving the best/closest reqeust clockrate, as it take into account
> muxes/parent clocks as well!?

Correct.  This IP block has an internal mux and divider, so I want to
take advanate of the clock framework features to model those clocks.

>> +       if (WARN_ON(PTR_ERR_OR_ZERO(host->mux_clk)))
>> +               return PTR_ERR(host->mux_clk);
>> +
>> +       /* create the divider */
>> +       snprintf(clk_name, sizeof(clk_name), "%s#div", dev_name(host->dev));
>> +       init.name = devm_kstrdup(host->dev, clk_name, GFP_KERNEL);
>> +       init.ops = &clk_divider_ops;
>> +       init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
>> +       clk_div_parents[0] = __clk_get_name(host->mux_clk);
>> +       init.parent_names = clk_div_parents;
>> +       init.num_parents = ARRAY_SIZE(clk_div_parents);
>> +
>> +       host->cfg_div.reg = host->regs + SD_EMMC_CLOCK;
>> +       host->cfg_div.shift = CLK_DIV_SHIFT;
>> +       host->cfg_div.width = CLK_DIV_WIDTH;
>> +       host->cfg_div.hw.init = &init;
>> +       host->cfg_div.flags = CLK_DIVIDER_ONE_BASED |
>> +               CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ALLOW_ZERO;
>> +
>> +       host->cfg_div_clk = devm_clk_register(host->dev, &host->cfg_div.hw);
>
> Ditto.
>
>> +       if (WARN_ON(PTR_ERR_OR_ZERO(host->cfg_div_clk)))
>> +               return PTR_ERR(host->cfg_div_clk);
>> +
>> +       /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
>> +       clk_reg = 0;
>> +       clk_reg |= CLK_PHASE_180 << CLK_PHASE_SHIFT;
>> +       clk_reg |= CLK_SRC_XTAL << CLK_SRC_SHIFT;
>> +       clk_reg |= CLK_DIV_MAX << CLK_DIV_SHIFT;
>> +       clk_reg &= ~CLK_ALWAYS_ON;
>> +       reg_write(host, SD_EMMC_CLOCK, clk_reg);
>> +
>> +       clk_prepare_enable(host->cfg_div_clk);
>> +
>> +       /* Ensure clock starts in "auto" mode, not "always on" */
>> +       cfg = reg_read(host, SD_EMMC_CFG);
>> +       cfg &= ~CFG_CLK_ALWAYS_ON;
>> +       cfg |= CFG_AUTO_CLK;
>> +       reg_write(host, SD_EMMC_CFG, cfg);
>> +
>> +       meson_mmc_clk_set(host, f_min);
>> +
>> +       return ret;
>> +}
>> +
>> +static void meson_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>> +{
>> +       struct meson_host *host = mmc_priv(mmc);
>> +       u32 bus_width;
>> +       u32 val, orig;
>> +
>> +       /*
>> +        * GPIO regulator, only controls switching between 1v8 and
>> +        * 3v3, doesn't support MMC_POWER_OFF, MMC_POWER_ON.
>> +        */
>
> I don't follow. Shouldn't you call regulator_enable|disable() for the
> above regulator? Why not?
>
>> +       switch (ios->power_mode) {
>> +       case MMC_POWER_UP:
>> +               if (!IS_ERR(mmc->supply.vmmc))
>> +                       mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd);
>
> The above comment talks about 1v8 and 3v3, which seems to me that you
> are changing the I/O voltage, but that's not what *vmmc* should be
> used for.
>
> If this is about I/O voltage, you shall instead use *vqmmc* and the
> corresponding mmc_regulator_set_vqmmc() to change the voltage level.
>
> This also leaves me to ask, then how do you control the power to the
> card? This is actually what the *vmmc* should be used for.

I'll respin this whole section based on your clarifications.  I was
confused about how the various regulators are meant to be used.  Thanks
for clarifying.

>
>> +       }
>> +
>> +       meson_mmc_clk_set(host, ios->clock);
>> +
>> +       /* Bus width */
>> +       val = reg_read(host, SD_EMMC_CFG);
>> +       switch (ios->bus_width) {
>> +       case MMC_BUS_WIDTH_1:
>> +               bus_width = CFG_BUS_WIDTH_1;
>> +               break;
>> +       case MMC_BUS_WIDTH_4:
>> +               bus_width = CFG_BUS_WIDTH_4;
>> +               break;
>> +       case MMC_BUS_WIDTH_8:
>> +               bus_width = CFG_BUS_WIDTH_8;
>> +               break;
>> +       default:
>> +               dev_err(host->dev, "Invalid ios->bus_width: %u.  Setting to 4.\n",
>> +                       ios->bus_width);
>> +               bus_width = CFG_BUS_WIDTH_4;
>> +               return;
>> +       }
>> +
>> +       val = reg_read(host, SD_EMMC_CFG);
>> +       orig = val;
>> +
>> +       val &= ~(CFG_BUS_WIDTH_MASK << CFG_BUS_WIDTH_SHIFT);
>> +       val |= bus_width << CFG_BUS_WIDTH_SHIFT;
>> +
>> +       val &= ~(CFG_BLK_LEN_MASK << CFG_BLK_LEN_SHIFT);
>> +       val |= ilog2(SD_EMMC_CFG_BLK_SIZE) << CFG_BLK_LEN_SHIFT;
>> +
>> +       val &= ~(CFG_RESP_TIMEOUT_MASK << CFG_RESP_TIMEOUT_SHIFT);
>> +       val |= ilog2(SD_EMMC_CFG_RESP_TIMEOUT) << CFG_RESP_TIMEOUT_SHIFT;
>> +
>> +       val &= ~(CFG_RC_CC_MASK << CFG_RC_CC_SHIFT);
>> +       val |= ilog2(SD_EMMC_CFG_CMD_GAP) << CFG_RC_CC_SHIFT;
>> +
>> +       reg_write(host, SD_EMMC_CFG, val);
>> +
>> +       if (val != orig)
>> +               dev_dbg(host->dev, "%s: SD_EMMC_CFG: 0x%08x -> 0x%08x\n",
>> +                       __func__, orig, val);
>> +}
>> +
>
> [...]
>
>> +
>> +static int meson_mmc_probe(struct platform_device *pdev)
>> +{
>> +       struct device_node *np = pdev->dev.of_node;
>> +       struct resource *res;
>> +       struct meson_host *host;
>> +       struct mmc_host *mmc;
>> +       int ret;
>> +
>> +       mmc = mmc_alloc_host(sizeof(struct meson_host), &pdev->dev);
>> +       if (!mmc)
>> +               return -ENOMEM;
>> +       host = mmc_priv(mmc);
>> +       host->mmc = mmc;
>> +       host->dev = &pdev->dev;
>> +       dev_set_drvdata(&pdev->dev, host);
>> +
>> +       spin_lock_init(&host->lock);
>> +
>> +       host->core_clk = devm_clk_get(&pdev->dev, "core");
>> +       if (IS_ERR(host->core_clk)) {
>> +               ret = PTR_ERR(host->core_clk);
>> +               if (ret == -EPROBE_DEFER)
>> +                       dev_dbg(&pdev->dev,
>> +                               "Missing core clock.  EPROBE_DEFER\n");
>> +               else
>> +                       dev_err(&pdev->dev,
>> +                               "Unable to get core clk (ret=%d).\n", ret);
>
> I think these prints are unessarry, they should be printed by other
> frameworks already, right!?

OK, I'll drop these.

>> +               goto free_host;
>> +       }
>> +
>> +       /* Voltage supply */
>> +       mmc_of_parse_voltage(np, &host->ocr_mask);
>
> This looks odd.
>
> Usally we get "ocr_avail" when asking the vmmc regulator about its
> supported voltage range, via calling the below
> mmc_regulator_get_supply() API. Can you explain why thay isn't work
> for you?

You're right, that's working fine.  I'll drop this.

>> +       ret = mmc_regulator_get_supply(mmc);
>> +       if (ret == -EPROBE_DEFER) {
>> +               dev_dbg(&pdev->dev, "Missing regulator: EPROBE_DEFER");
>
> The print shouldn't be needed, please remove.

OK.

>> +               goto free_host;
>> +       }
>> +
>> +       if (!mmc->ocr_avail)
>> +               mmc->ocr_avail = host->ocr_mask;
>
> mmc->ocr_avail needs to be assigned to a valid value. This fallback
> doesn't cover that as host->ocr_mask may very well be zero.

Dropped this as it's now handled elsewhere.

>> +
>> +       ret = mmc_of_parse(mmc);
>> +       if (ret) {
>> +               dev_warn(&pdev->dev, "error parsing DT: %d\n", ret);
>> +               goto free_host;
>> +       }
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       if (!res) {
>> +               ret = -ENODEV;
>> +               goto free_host;
>> +       }
>> +       host->regs = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(host->regs)) {
>> +               ret = PTR_ERR(host->regs);
>> +               goto free_host;
>> +       }
>> +
>> +       host->irq = platform_get_irq(pdev, 0);
>> +       if (host->irq == 0) {
>> +               dev_err(&pdev->dev, "failed to get interrupt resource.\n");
>> +               ret = -EINVAL;
>> +               goto free_host;
>> +       }
>> +
>> +       ret = devm_request_threaded_irq(&pdev->dev, host->irq,
>> +                                       meson_mmc_irq, meson_mmc_irq_thread,
>> +                                       IRQF_SHARED, DRIVER_NAME, host);
>> +       if (ret)
>> +               goto free_host;
>> +
>> +       /* data bounce buffer */
>> +       host->bounce_buf_size = SZ_512K;
>> +       host->bounce_buf =
>> +               dma_alloc_coherent(host->dev, host->bounce_buf_size,
>> +                                  &host->bounce_dma_addr, GFP_KERNEL);
>> +       if (host->bounce_buf == NULL) {
>> +               dev_err(host->dev, "Unable to map allocate DMA bounce buffer.\n");
>> +               ret = -ENOMEM;
>> +               goto free_host;
>> +       }
>> +
>> +       clk_prepare_enable(host->core_clk);
>> +
>> +       ret = meson_mmc_clk_init(host);
>> +       if (ret)
>> +               goto free_host;
>> +
>> +       /* Stop execution */
>> +       reg_write(host, SD_EMMC_START, 0);
>> +
>> +       /* clear, ack, enable all interrupts */
>> +       reg_write(host, SD_EMMC_IRQ_EN, 0);
>> +       reg_write(host, SD_EMMC_STATUS, IRQ_EN_MASK);
>> +
>> +       mmc->ops = &meson_mmc_ops;
>> +       mmc_add_host(mmc);
>> +
>> +       return 0;
>> +
>> +free_host:
>> +       dev_dbg(host->dev, "Failed to probe: ret=%d\n", ret);
>> +       if (host->core_clk)
>> +               clk_disable_unprepare(host->core_clk);
>> +       mmc_free_host(mmc);
>> +       return ret;
>> +}
>> +
>
> [...]
>
>> +
>> +static const struct of_device_id meson_mmc_of_match[] = {
>> +       {
>> +               .compatible = "amlogic,meson-gxbb-mmc",
>
> You need to fold in a DT documentation patch, in this series as to
> describe all the required/optional resourses for the device. That of
> course also includes the compatible string.

That was all part of PATCH 1/2 of this series, or was there something
else missing from that patch?

Thanks for the review,

Kevin