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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 4 August 2016 at 01:18, Kevin Hilman <khilman@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxxxx>
> + *
> + * 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!?

[...]

> +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.

> +
> +       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!?

> +       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.

> +       }
> +
> +       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!?

> +               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?

> +       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.

> +               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.

> +
> +       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.

> +       },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, meson_mmc_of_match);

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux