Message-ID: <CAPDyKFqW7-JiNZTpCEUv+=Hf_rLh4x89h+i7_9UUwmxRERg_5A@mail.gmail.com>
Date:   Fri, 17 Mar 2017 12:24:57 +0100
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Jan Glauber <jglauber@...ium.com>
Cc:     "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        David Daney <ddaney@...iumnetworks.com>,
        "Steven J . Hill" <Steven.Hill@...ium.com>,
        David Daney <david.daney@...ium.com>
Subject: Re: [PATCH v12 2/9] mmc: cavium: Add core MMC driver for Cavium SOCs

On 10 March 2017 at 14:25, Jan Glauber <jglauber@...ium.com> wrote:
> This core driver will be used by a MIPS platform driver
> or by an ARM64 PCI driver. The core driver implements the
> mmc_host_ops and slot probe & remove functions.
> Callbacks are provided to allow platform specific interrupt
> enable and bus locking.
>
> The host controller supports:
> - up to 4 slots that can contain sd-cards or eMMC chips
> - 1, 4 and 8 bit bus width
> - SDR and DDR
> - transfers up to 52 Mhz (might be less when multiple slots are used)
> - DMA read/write
> - multi-block read/write (but not stream mode)
>
> Voltage is limited to 3.3v and shared for all slots (vmmc and vmmcq).
>
> A global lock for all MMC devices is required because the host
> controller is shared.
>
> Signed-off-by: Jan Glauber <jglauber@...ium.com>
> Signed-off-by: David Daney <david.daney@...ium.com>
> Signed-off-by: Steven J. Hill <steven.hill@...ium.com>
> ---
>  drivers/mmc/host/cavium-mmc.c | 988 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/mmc/host/cavium-mmc.h | 178 ++++++++
>  2 files changed, 1166 insertions(+)
>  create mode 100644 drivers/mmc/host/cavium-mmc.c
>  create mode 100644 drivers/mmc/host/cavium-mmc.h
>
> diff --git a/drivers/mmc/host/cavium-mmc.c b/drivers/mmc/host/cavium-mmc.c
> new file mode 100644
> index 0000000..11fdcfb
> --- /dev/null
> +++ b/drivers/mmc/host/cavium-mmc.c
> @@ -0,0 +1,988 @@
> +/*
> + * Shared part of driver for MMC/SDHC controller on Cavium OCTEON and
> + * ThunderX SOCs.
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * Copyright (C) 2012-2017 Cavium Inc.
> + * Authors:
> + *   David Daney <david.daney@...ium.com>
> + *   Peter Swain <pswain@...ium.com>
> + *   Steven J. Hill <steven.hill@...ium.com>
> + *   Jan Glauber <jglauber@...ium.com>
> + */
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/dma-direction.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/slot-gpio.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/scatterlist.h>
> +#include <linux/time.h>
> +
> +#include "cavium-mmc.h"
> +
> +const char *cvm_mmc_irq_names[] = {
> +       "MMC Buffer",
> +       "MMC Command",
> +       "MMC DMA",
> +       "MMC Command Error",
> +       "MMC DMA Error",
> +       "MMC Switch",
> +       "MMC Switch Error",
> +       "MMC DMA int Fifo",
> +       "MMC DMA int",
> +};

Debug-leftover?

[...]

> +
> +static void cvm_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> +       struct cvm_mmc_slot *slot = mmc_priv(mmc);
> +       struct cvm_mmc_host *host = slot->host;
> +       int clk_period = 0, power_class = 10, bus_width = 0;
> +       u64 clock, emm_switch;
> +
> +       host->acquire_bus(host);
> +       cvm_mmc_switch_to(slot);
> +
> +       /* Set the power state */
> +       switch (ios->power_mode) {
> +       case MMC_POWER_ON:
> +               break;
> +
> +       case MMC_POWER_OFF:
> +               cvm_mmc_reset_bus(slot);
> +
> +               if (host->global_pwr_gpiod)
> +                       gpiod_set_value_cansleep(host->global_pwr_gpiod, 0);

If I have understood the changelog correctly this GPIO is shared for
all slots, right?

In such case, this doesn't work. You need to centralize the management
of this GPIO pin (to enable reference counting), as otherwise one slot
can decide to power off its card while another still uses their card
and expecting the power to be on.

Another option would be to model it as GPIO regulator (using a device
tree overlay as we discussed earlier), then you get the reference
counting for free - and easily get ocr_avail mask from the mmc core's
regulator API. :-)

Moreover, I didn't find this GPIO being documented as a DT binding, it
should and it should also be marked as deprecated.

> +               else
> +                       mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
> +               break;
> +
> +       case MMC_POWER_UP:
> +               if (host->global_pwr_gpiod)
> +                       gpiod_set_value_cansleep(host->global_pwr_gpiod, 1);

Dittto.

> +               else
> +                       mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd);
> +               break;
> +       }
> +

[...]

> +
> +static int cvm_mmc_of_parse(struct device *dev, struct cvm_mmc_slot *slot)
> +{
> +       u32 id, cmd_skew = 0, dat_skew = 0, bus_width = 0, f_max = 0;
> +       struct device_node *node = dev->of_node;
> +       struct mmc_host *mmc = slot->mmc;
> +       u64 clock_period;
> +       int ret;
> +
> +       ret = of_property_read_u32(node, "reg", &id);
> +       if (ret) {
> +               dev_err(dev, "Missing or invalid reg property on %s\n",
> +                       of_node_full_name(node));
> +               return ret;
> +       }
> +
> +       if (id >= CAVIUM_MAX_MMC || slot->host->slot[id]) {
> +               dev_err(dev, "Invalid reg property on %s\n",
> +                       of_node_full_name(node));
> +               return -EINVAL;
> +       }
> +
> +       /* Deprecated Cavium bindings for old firmware */
> +       of_property_read_u32(node, "cavium,bus-max-width", &bus_width);
> +       of_property_read_u32(node, "spi-max-frequency", &f_max);
> +       if (slot->host->global_pwr_gpiod) {
> +               /* Get a sane OCR mask for other parts of the MMC subsytem. */
> +               ret = mmc_of_parse_voltage(node, &mmc->ocr_avail);

I noticed your comment to Arnd for the cover-letter. So I assume you
will remove this and instead assign mmc->ocr_avail a default value in
cases when you don't have a vmmc regulator to find it from.

> +               if (ret < 0)
> +                       return ret;
> +       }
> +
> +       /* Cavium-specific DT properties */
> +       of_property_read_u32(node, "cavium,cmd-clk-skew", &cmd_skew);
> +       of_property_read_u32(node, "cavium,dat-clk-skew", &dat_skew);
> +
> +       if (!slot->host->global_pwr_gpiod) {
> +               mmc->supply.vmmc = devm_regulator_get(dev, "vmmc");
> +               if (IS_ERR(mmc->supply.vmmc))
> +                       return PTR_ERR(mmc->supply.vmmc);
> +
> +               ret = mmc_regulator_get_ocrmask(mmc->supply.vmmc);
> +               if (ret > 0)
> +                       mmc->ocr_avail = ret;
> +       }
> +
> +       /* Common MMC bindings */
> +       ret = mmc_of_parse(mmc);
> +       if (ret)
> +               return ret;
> +
> +       /* Set bus width */
> +       if (!bus_width) {
> +               if (mmc->caps & MMC_CAP_8_BIT_DATA)
> +                       bus_width = 8;
> +               else if (mmc->caps & MMC_CAP_4_BIT_DATA)
> +                       bus_width = 4;
> +               else
> +                       bus_width = 1;
> +       }
> +
> +       switch (bus_width) {
> +       case 8:
> +               slot->bus_width = 2;

Why do you need to store this in slot struct? The information is
already available in the mmc host.

> +               slot->mmc->caps = MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA;

This is wrong, as you will clear all the other mmc caps potentially
assigned by mmc_of_parse() above.

Moreover, you can use mmc->caps instead of slot->mmc->caps.

> +               break;
> +       case 4:
> +               slot->bus_width = 1;
> +               slot->mmc->caps = MMC_CAP_4_BIT_DATA;
> +               break;
> +       case 1:
> +               slot->bus_width = 0;
> +               break;
> +       }

I would rather make the deprecated bindings to take the lowest
precedence and besides, this bus_width setup looks messy. How about
something like this instead:

mmc_of_parse();

if (!(mmc->caps & (MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA))) {
    of_property_read_u32(node, "cavium,bus-max-width", &bus_width);
    if (bus_width == 8)
          mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA;
    else if (bus_width == 4)
          mmc->caps |= MMC_CAP_4_BIT_DATA;
}

> +
> +       /* Set maximum and minimum frequency */
> +       if (f_max)
> +               mmc->f_max = f_max;

Again, let's make sure the deprecated bindings takes lower precedence.
Thus if mmc->f_max has a value let's use that and if not, then parse
the deprecated DT binding and use that value instead.

> +       if (!mmc->f_max || mmc->f_max > 52000000)
> +               mmc->f_max = 52000000;
> +       mmc->f_min = 400000;
> +
> +       /* Sampling register settings, period in picoseconds */
> +       clock_period = 1000000000000ull / slot->host->sys_freq;
> +       slot->cmd_cnt = (cmd_skew + clock_period / 2) / clock_period;
> +       slot->dat_cnt = (dat_skew + clock_period / 2) / clock_period;
> +
> +       return id;
> +}

[...]

> diff --git a/drivers/mmc/host/cavium-mmc.h b/drivers/mmc/host/cavium-mmc.h
> new file mode 100644
> index 0000000..c3843448
> --- /dev/null
> +++ b/drivers/mmc/host/cavium-mmc.h

[...]

 +
> +/* Protoypes */
> +irqreturn_t cvm_mmc_interrupt(int irq, void *dev_id);
> +int cvm_mmc_of_slot_probe(struct device *dev, struct cvm_mmc_host *host);
> +int cvm_mmc_of_slot_remove(struct cvm_mmc_slot *slot);
> +extern const char *cvm_mmc_irq_names[];

Debug leftover?

Kind regards
Uffe

Powered by blists - more mailing lists