[PATCH v2 2/2] MMC: meson: initial support for GXBB platforms
Kevin Hilman
khilman at baylibre.com
Tue Sep 13 08:53:46 PDT 2016
Ulf Hansson <ulf.hansson at linaro.org> writes:
> On 4 August 2016 at 01:18, Kevin Hilman <khilman at 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 at 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 at 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 at 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
More information about the linux-amlogic
mailing list