Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1591903 > unrolled thread
| Started by | Ulf Hansson <ulf.hansson@linaro.org> |
|---|---|
| First post | 2017-03-03 13:20 +0100 |
| Last post | 2017-03-08 11:00 +0100 |
| Articles | 2 — 1 participant |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v11 2/9] mmc: cavium: Add core MMC driver for Cavium SOCs Ulf Hansson <ulf.hansson@linaro.org> - 2017-03-03 13:20 +0100
Re: [PATCH v11 2/9] mmc: cavium: Add core MMC driver for Cavium SOCs Ulf Hansson <ulf.hansson@linaro.org> - 2017-03-08 11:00 +0100
| From | Ulf Hansson <ulf.hansson@linaro.org> |
|---|---|
| Date | 2017-03-03 13:20 +0100 |
| Subject | Re: [PATCH v11 2/9] mmc: cavium: Add core MMC driver for Cavium SOCs |
| Message-ID | <tgUlI-13r-11@gated-at.bofh.it> |
On 6 February 2017 at 14:39, Jan Glauber <jglauber@cavium.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.
What voltage? The I/O voltage or the voltage for the card?
VMMC or VMMCQ?
>
> A global lock for all MMC devices is required because the host
> controller is shared.
>
> Signed-off-by: Jan Glauber <jglauber@cavium.com>
> Signed-off-by: David Daney <david.daney@cavium.com>
> Signed-off-by: Steven J. Hill <steven.hill@cavium.com>
> ---
> drivers/mmc/host/cavium-mmc.c | 1029 +++++++++++++++++++++++++++++++++++++++++
> drivers/mmc/host/cavium-mmc.h | 303 ++++++++++++
> 2 files changed, 1332 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..40aee08
> --- /dev/null
> +++ b/drivers/mmc/host/cavium-mmc.c
[...]
> +
> +static bool bad_status(union mio_emm_rsp_sts *rsp_sts)
> +{
> + if (rsp_sts->s.rsp_bad_sts || rsp_sts->s.rsp_crc_err ||
> + rsp_sts->s.rsp_timeout || rsp_sts->s.blk_crc_err ||
> + rsp_sts->s.blk_timeout || rsp_sts->s.dbuf_err)
> + return true;
> +
> + return false;
> +}
> +
> +/* Try to clean up failed DMA. */
> +static void cleanup_dma(struct cvm_mmc_host *host,
> + union mio_emm_rsp_sts *rsp_sts)
> +{
> + union mio_emm_dma emm_dma;
> +
> + emm_dma.val = readq(host->base + MIO_EMM_DMA);
> + emm_dma.s.dma_val = 1;
> + emm_dma.s.dat_null = 1;
> + emm_dma.s.bus_id = rsp_sts->s.bus_id;
> + writeq(emm_dma.val, host->base + MIO_EMM_DMA);
> +}
> +
> +irqreturn_t cvm_mmc_interrupt(int irq, void *dev_id)
> +{
> + struct cvm_mmc_host *host = dev_id;
> + union mio_emm_rsp_sts rsp_sts;
> + union mio_emm_int emm_int;
> + struct mmc_request *req;
> + bool host_done;
> +
> + /* Clear interrupt bits (write 1 clears ). */
> + emm_int.val = readq(host->base + MIO_EMM_INT);
> + writeq(emm_int.val, host->base + MIO_EMM_INT);
> +
> + if (emm_int.s.switch_err)
> + check_switch_errors(host);
> +
> + req = host->current_req;
> + if (!req)
> + goto out;
> +
> + rsp_sts.val = readq(host->base + MIO_EMM_RSP_STS);
> + /*
> + * dma_val set means DMA is still in progress. Don't touch
> + * the request and wait for the interrupt indicating that
> + * the DMA is finished.
> + */
> + if (rsp_sts.s.dma_val && host->dma_active)
> + goto out;
> +
> + if (!host->dma_active && emm_int.s.buf_done && req->data) {
> + unsigned int type = (rsp_sts.val >> 7) & 3;
> +
> + if (type == 1)
> + do_read(host, req, rsp_sts.s.dbuf);
> + else if (type == 2)
> + do_write(req);
> + }
> +
> + host_done = emm_int.s.cmd_done || emm_int.s.dma_done ||
> + emm_int.s.cmd_err || emm_int.s.dma_err;
> +
> + if (!(host_done && req->done))
> + goto no_req_done;
> +
> + if (bad_status(&rsp_sts))
> + req->cmd->error = -EILSEQ;
I don't think you should treat all errors as -EILSEQ. Please assign a
proper error code, depending on the error.
> + else
> + req->cmd->error = 0;
> +
> + if (host->dma_active && req->data)
> + if (!finish_dma(host, req->data))
> + goto no_req_done;
> +
> + set_cmd_response(host, req, &rsp_sts);
> + if (emm_int.s.dma_err && rsp_sts.s.dma_pend)
> + cleanup_dma(host, &rsp_sts);
> +
> + host->current_req = NULL;
> + req->done(req);
> +
> +no_req_done:
> + if (host_done)
> + host->release_bus(host);
> +out:
> + return IRQ_RETVAL(emm_int.val != 0);
> +}
> +
> +/*
> + * Program DMA_CFG and if needed DMA_ADR.
> + * Returns 0 on error, DMA address otherwise.
> + */
> +static u64 prepare_dma_single(struct cvm_mmc_host *host, struct mmc_data *data)
> +{
> + union mio_emm_dma_cfg dma_cfg;
> + int count;
> + u64 addr;
> +
> + count = dma_map_sg(host->dev, data->sg, data->sg_len,
> + get_dma_dir(data));
> + if (!count)
> + return 0;
> +
> + dma_cfg.val = 0;
> + dma_cfg.s.en = 1;
> + dma_cfg.s.rw = (data->flags & MMC_DATA_WRITE) ? 1 : 0;
> +#ifdef __LITTLE_ENDIAN
> + dma_cfg.s.endian = 1;
> +#endif
> + dma_cfg.s.size = (sg_dma_len(&data->sg[0]) / 8) - 1;
> +
> + addr = sg_dma_address(&data->sg[0]);
> + dma_cfg.s.adr = addr;
> + writeq(dma_cfg.val, host->dma_base + MIO_EMM_DMA_CFG);
> +
> + pr_debug("[%s] sg_dma_len: %u total sg_elem: %d\n",
> + (dma_cfg.s.rw) ? "W" : "R", sg_dma_len(&data->sg[0]), count);
> + return addr;
> +}
> +
> +static u64 prepare_dma(struct cvm_mmc_host *host, struct mmc_data *data)
> +{
> + return prepare_dma_single(host, data);
> +}
> +
> +static void prepare_ext_dma(struct mmc_host *mmc, struct mmc_request *mrq,
> + union mio_emm_dma *emm_dma)
> +{
> + struct cvm_mmc_slot *slot = mmc_priv(mmc);
> +
> + /*
> + * Our MMC host hardware does not issue single commands,
> + * because that would require the driver and the MMC core
> + * to do work to determine the proper sequence of commands.
I don't get this. The allowed sequence of the commands is determined
by the SD/(e)MMC/SDIO spec and much of this knowledge is the
responsibility of the mmc core.
> + * Instead, our hardware is superior to most other MMC bus
No need to brag about your HW. Let's just describe how it works instead.
> + * hosts. The sequence of MMC commands required to execute
> + * a transfer are issued automatically by the bus hardware.
What does this really mean? Is this about HW support for better
dealing with data requests?
> + *
> + * - David Daney <ddaney@cavium.com>
> + */
> + emm_dma->val = 0;
> + emm_dma->s.bus_id = slot->bus_id;
> + emm_dma->s.dma_val = 1;
> + emm_dma->s.sector = (mrq->data->blksz == 512) ? 1 : 0;
> + emm_dma->s.rw = (mrq->data->flags & MMC_DATA_WRITE) ? 1 : 0;
> + emm_dma->s.block_cnt = mrq->data->blocks;
> + emm_dma->s.card_addr = mrq->cmd->arg;
> + if (mmc_card_mmc(mmc->card) || (mmc_card_sd(mmc->card) &&
> + (mmc->card->scr.cmds & SD_SCR_CMD23_SUPPORT)))
> + emm_dma->s.multi = 1;
> +
> + pr_debug("[%s] blocks: %u multi: %d\n", (emm_dma->s.rw) ? "W" : "R",
> + mrq->data->blocks, emm_dma->s.multi);
> +}
> +
[...]
> +
> +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, power_class = 10, bus_width = 0;
> + union mio_emm_switch emm_switch;
> + u64 clock;
> +
> + 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);
> + 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);
> + else
> + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd);
> + break;
> + }
> +
> + /* Set bus width */
> + switch (ios->bus_width) {
> + case MMC_BUS_WIDTH_8:
> + bus_width = 2;
> + break;
> + case MMC_BUS_WIDTH_4:
> + bus_width = 1;
> + break;
> + case MMC_BUS_WIDTH_1:
> + bus_width = 0;
> + break;
> + }
> +
> + slot->bus_width = bus_width;
> +
> + if (!ios->clock)
There are cases when the core change the clock rate to 0, and then it
expects the mmc host to gate the clock. It probably a good idea for
you to do that as well.
> + goto out;
> +
> + /* Change the clock frequency. */
> + clock = ios->clock;
> + if (clock > 52000000)
> + clock = 52000000;
> + slot->clock = clock;
> + clk_period = (host->sys_freq + clock - 1) / (2 * clock);
> +
> + emm_switch.val = 0;
> + emm_switch.s.hs_timing = (ios->timing == MMC_TIMING_MMC_HS);
> + emm_switch.s.bus_width = bus_width;
> + emm_switch.s.power_class = power_class;
> + emm_switch.s.clk_hi = clk_period;
> + emm_switch.s.clk_lo = clk_period;
> + emm_switch.s.bus_id = slot->bus_id;
> +
> + if (!switch_val_changed(slot, emm_switch.val))
> + goto out;
> +
> + set_wdog(slot, 0);
> + do_switch(host, emm_switch.val);
> + slot->cached_switch = emm_switch.val;
> +out:
> + host->release_bus(host);
> +}
[...]
> +
> +static int set_bus_width(struct device *dev, struct cvm_mmc_slot *slot, u32 id)
> +{
> + u32 bus_width;
> + int ret;
> +
> + /*
> + * The "cavium,bus-max-width" property is DEPRECATED and should
> + * not be used. We handle it here to support older firmware.
> + * Going forward, the standard "bus-width" property is used
> + * instead of the Cavium-specific property.
> + */
> + if (!(slot->mmc->caps & (MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA))) {
> + /* Try legacy "cavium,bus-max-width" property. */
> + ret = of_property_read_u32(dev->of_node, "cavium,bus-max-width",
> + &bus_width);
> + if (ret) {
> + /* No bus width specified, use default. */
> + bus_width = 8;
> + dev_info(dev, "Default width 8 used for slot %u\n", id);
> + }
> + } else {
> + /* Hosts capable of 8-bit transfers can also do 4 bits */
> + bus_width = (slot->mmc->caps & MMC_CAP_8_BIT_DATA) ? 8 : 4;
> + }
This looks a bit unnessarry complex.
I would instead suggest the following order of how to perform the OF
parsing. Bindings that get parsed later, overrides the earlier.
1. Parse deprecated bindings.
2. Parse cavium specific bindings.
3. Parse common mmc bindings.
4. Check some caps, to make sure those have valid values as to cover
cases when the OF parsing didn't find values.
The same comment applies for the other OF parsing functions below.
> +
> + switch (bus_width) {
> + case 8:
> + slot->bus_width = (MMC_BUS_WIDTH_8 - 1);
> + slot->mmc->caps = MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA;
> + break;
> + case 4:
> + slot->bus_width = (MMC_BUS_WIDTH_4 - 1);
> + slot->mmc->caps = MMC_CAP_4_BIT_DATA;
> + break;
> + case 1:
> + slot->bus_width = MMC_BUS_WIDTH_1;
> + break;
> + default:
> + dev_err(dev, "Invalid bus width for slot %u\n", id);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static void set_frequency(struct device *dev, struct mmc_host *mmc, u32 id)
> +{
> + int ret;
> +
> + /*
> + * The "spi-max-frequency" property is DEPRECATED and should
> + * not be used. We handle it here to support older firmware.
> + * Going forward, the standard "max-frequency" property is
> + * used instead of the Cavium-specific property.
> + */
> + if (mmc->f_max == 0) {
> + /* Try legacy "spi-max-frequency" property. */
> + ret = of_property_read_u32(dev->of_node, "spi-max-frequency",
> + &mmc->f_max);
> + if (ret) {
> + /* No frequency properties found, use default. */
> + mmc->f_max = 52000000;
> + dev_info(dev, "Default %u frequency used for slot %u\n",
> + mmc->f_max, id);
> + }
> + } else if (mmc->f_max > 52000000)
> + mmc->f_max = 52000000;
> +
> + /* Set minimum frequency */
> + mmc->f_min = 400000;
> +}
> +
> +static int set_voltage(struct device *dev, struct mmc_host *mmc,
> + struct cvm_mmc_host *host)
> +{
> + int ret;
> +
> + /*
> + * Legacy platform doesn't support regulator but enables power gpio
> + * directly during platform probe.
> + */
> + if (host->global_pwr_gpiod)
> + /* Get a sane OCR mask for other parts of the MMC subsytem. */
> + return mmc_of_parse_voltage(dev->of_node, &mmc->ocr_avail);
Does really the legacy platforms use the mmc voltage range DT bindings!?
I would rather see that you assign a default value to mmc->ocr_avail,
than using this binding.
> +
> + mmc->supply.vmmc = devm_regulator_get(dev, "vmmc");
> + if (IS_ERR(mmc->supply.vmmc)) {
> + ret = PTR_ERR(mmc->supply.vmmc);
> + } else {
> + ret = mmc_regulator_get_ocrmask(mmc->supply.vmmc);
> + if (ret > 0) {
> + mmc->ocr_avail = ret;
> + ret = 0;
> + }
> + }
This if-else-if is a bit messy.
Why not just return when you get an error instead. That should simply the code.
Maybe you can have look and try to clean up this in the hole file
where you think it would make an improvment.
> + return ret;
> +}
> +
> +int cvm_mmc_slot_probe(struct device *dev, struct cvm_mmc_host *host)
To reflect that OF is needed, perhaps rename function to
cvm_mmc_of_slot_probe().
> +{
> + struct device_node *node = dev->of_node;
> + u32 id, cmd_skew, dat_skew;
> + struct cvm_mmc_slot *slot;
> + struct mmc_host *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 || host->slot[id]) {
> + dev_err(dev, "Invalid reg property on %s\n",
> + of_node_full_name(node));
> + return -EINVAL;
> + }
> +
> + mmc = mmc_alloc_host(sizeof(struct cvm_mmc_slot), dev);
> + if (!mmc)
> + return -ENOMEM;
> +
> + slot = mmc_priv(mmc);
> + slot->mmc = mmc;
> + slot->host = host;
> +
> + ret = mmc_of_parse(mmc);
> + if (ret)
> + goto error;
> +
> + ret = set_bus_width(dev, slot, id);
> + if (ret)
> + goto error;
> +
> + set_frequency(dev, mmc, id);
> +
> + /* Octeon-specific DT properties. */
> + ret = of_property_read_u32(node, "cavium,cmd-clk-skew", &cmd_skew);
> + if (ret)
> + cmd_skew = 0;
> + ret = of_property_read_u32(node, "cavium,dat-clk-skew", &dat_skew);
> + if (ret)
> + dat_skew = 0;
> +
> + ret = set_voltage(dev, mmc, host);
> + if (ret < 0)
> + goto error;
The functions set_bus_width(), set_freqeuncy(), set_voltage() all
performs OF parsing and there are some parsing also being done above.
I would suggest you bundle all OF parsing into one function, perhaps
name it "cvm_mmc_of_parse()" or similar. That should make the code a
lot cleaner.
> +
> + /* Set up host parameters */
> + mmc->ops = &cvm_mmc_ops;
> +
> + mmc->caps |= MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
> + MMC_CAP_ERASE | MMC_CAP_CMD23 | MMC_CAP_POWER_OFF_CARD;
> +
> + mmc->max_segs = 1;
> +
> + /* DMA size field can address up to 8 MB */
> + mmc->max_seg_size = 8 * 1024 * 1024;
> + mmc->max_req_size = mmc->max_seg_size;
> + /* External DMA is in 512 byte blocks */
> + mmc->max_blk_size = 512;
> + /* DMA block count field is 15 bits */
> + mmc->max_blk_count = 32767;
> +
> + slot->clock = mmc->f_min;
> + slot->sclock = host->sys_freq;
> +
> + /* Period in picoseconds. */
> + clock_period = 1000000000000ull / slot->sclock;
> + slot->cmd_cnt = (cmd_skew + clock_period / 2) / clock_period;
> + slot->dat_cnt = (dat_skew + clock_period / 2) / clock_period;
> +
> + slot->bus_id = id;
> + slot->cached_rca = 1;
> +
> + host->acquire_bus(host);
> + host->slot[id] = slot;
> + cvm_mmc_switch_to(slot);
> + cvm_mmc_init_lowlevel(slot);
> + host->release_bus(host);
> +
> + ret = mmc_add_host(mmc);
> + if (ret) {
> + dev_err(dev, "mmc_add_host() returned %d\n", ret);
> + goto error;
> + }
> +
> + return 0;
> +
> +error:
> + slot->host->slot[id] = NULL;
> + mmc_free_host(slot->mmc);
> + return ret;
> +}
> +
> +int cvm_mmc_slot_remove(struct cvm_mmc_slot *slot)
> +{
> + mmc_remove_host(slot->mmc);
> + slot->host->slot[slot->bus_id] = NULL;
> + mmc_free_host(slot->mmc);
> + return 0;
> +}
> diff --git a/drivers/mmc/host/cavium-mmc.h b/drivers/mmc/host/cavium-mmc.h
> new file mode 100644
> index 0000000..27fb02b
> --- /dev/null
> +++ b/drivers/mmc/host/cavium-mmc.h
> @@ -0,0 +1,303 @@
> +/*
> + * Driver for MMC and SSD cards for 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-2016 Cavium Inc.
> + */
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/mmc/host.h>
> +#include <linux/of.h>
> +#include <linux/scatterlist.h>
> +#include <linux/semaphore.h>
> +
> +#define CAVIUM_MAX_MMC 4
> +
> +struct cvm_mmc_host {
> + struct device *dev;
> + void __iomem *base;
> + void __iomem *dma_base;
> + u64 emm_cfg;
> + int last_slot;
> + struct clk *clk;
> + int sys_freq;
> +
> + struct mmc_request *current_req;
> + struct sg_mapping_iter smi;
> + bool dma_active;
> +
> + struct gpio_desc *global_pwr_gpiod;
> +
> + struct cvm_mmc_slot *slot[CAVIUM_MAX_MMC];
> +
> + void (*acquire_bus)(struct cvm_mmc_host *);
> + void (*release_bus)(struct cvm_mmc_host *);
> + void (*int_enable)(struct cvm_mmc_host *, u64);
> +};
> +
> +struct cvm_mmc_slot {
> + struct mmc_host *mmc; /* slot-level mmc_core object */
> + struct cvm_mmc_host *host; /* common hw for all slots */
> +
> + u64 clock;
> + unsigned int sclock;
> +
> + u64 cached_switch;
> + u64 cached_rca;
> +
> + unsigned int cmd_cnt; /* sample delay */
> + unsigned int dat_cnt; /* sample delay */
> +
> + int bus_width;
> + int bus_id;
> +};
> +
> +struct cvm_mmc_cr_type {
> + u8 ctype;
> + u8 rtype;
> +};
> +
> +struct cvm_mmc_cr_mods {
> + u8 ctype_xor;
> + u8 rtype_xor;
> +};
> +
> +/* Bitfield definitions */
> +
> +union mio_emm_cmd {
> + u64 val;
> + struct mio_emm_cmd_s {
> +#ifdef __BIG_ENDIAN_BITFIELD
Huh. Sorry, but this is a big nack from me.
This isn't the common method for how we deal with endian issues in the
kernel. Please remove all use of the union types here and below. The
follow common patterns for how we deal with endian issues.
> + u64 :2;
> + u64 bus_id:2;
> + u64 cmd_val:1;
> + u64 :3;
> + u64 dbuf:1;
> + u64 offset:6;
> + u64 :6;
> + u64 ctype_xor:2;
> + u64 rtype_xor:3;
> + u64 cmd_idx:6;
> + u64 arg:32;
> +#else
> + u64 arg:32;
> + u64 cmd_idx:6;
> + u64 rtype_xor:3;
> + u64 ctype_xor:2;
> + u64 :6;
> + u64 offset:6;
> + u64 dbuf:1;
> + u64 :3;
> + u64 cmd_val:1;
> + u64 bus_id:2;
> + u64 :2;
> +#endif
> + } s;
> +};
> +
> +union mio_emm_dma {
> + u64 val;
> + struct mio_emm_dma_s {
> +#ifdef __BIG_ENDIAN_BITFIELD
> + u64 :2;
> + u64 bus_id:2;
> + u64 dma_val:1;
> + u64 sector:1;
> + u64 dat_null:1;
> + u64 thres:6;
> + u64 rel_wr:1;
> + u64 rw:1;
> + u64 multi:1;
> + u64 block_cnt:16;
> + u64 card_addr:32;
> +#else
> + u64 card_addr:32;
> + u64 block_cnt:16;
> + u64 multi:1;
> + u64 rw:1;
> + u64 rel_wr:1;
> + u64 thres:6;
> + u64 dat_null:1;
> + u64 sector:1;
> + u64 dma_val:1;
> + u64 bus_id:2;
> + u64 :2;
> +#endif
> + } s;
> +};
> +
> +union mio_emm_dma_cfg {
> + u64 val;
> + struct mio_emm_dma_cfg_s {
> +#ifdef __BIG_ENDIAN_BITFIELD
> + u64 en:1;
> + u64 rw:1;
> + u64 clr:1;
> + u64 :1;
> + u64 swap32:1;
> + u64 swap16:1;
> + u64 swap8:1;
> + u64 endian:1;
> + u64 size:20;
> + u64 adr:36;
> +#else
> + u64 adr:36;
> + u64 size:20;
> + u64 endian:1;
> + u64 swap8:1;
> + u64 swap16:1;
> + u64 swap32:1;
> + u64 :1;
> + u64 clr:1;
> + u64 rw:1;
> + u64 en:1;
> +#endif
> + } s;
> +};
> +
> +union mio_emm_int {
> + u64 val;
> + struct mio_emm_int_s {
> +#ifdef __BIG_ENDIAN_BITFIELD
> + u64 :57;
> + u64 switch_err:1;
> + u64 switch_done:1;
> + u64 dma_err:1;
> + u64 cmd_err:1;
> + u64 dma_done:1;
> + u64 cmd_done:1;
> + u64 buf_done:1;
> +#else
> + u64 buf_done:1;
> + u64 cmd_done:1;
> + u64 dma_done:1;
> + u64 cmd_err:1;
> + u64 dma_err:1;
> + u64 switch_done:1;
> + u64 switch_err:1;
> + u64 :57;
> +#endif
> + } s;
> +};
> +
> +union mio_emm_rsp_sts {
> + u64 val;
> + struct mio_emm_rsp_sts_s {
> +#ifdef __BIG_ENDIAN_BITFIELD
> + u64 :2;
> + u64 bus_id:2;
> + u64 cmd_val:1;
> + u64 switch_val:1;
> + u64 dma_val:1;
> + u64 dma_pend:1;
> + u64 :27;
> + u64 dbuf_err:1;
> + u64 :4;
> + u64 dbuf:1;
> + u64 blk_timeout:1;
> + u64 blk_crc_err:1;
> + u64 rsp_busybit:1;
> + u64 stp_timeout:1;
> + u64 stp_crc_err:1;
> + u64 stp_bad_sts:1;
> + u64 stp_val:1;
> + u64 rsp_timeout:1;
> + u64 rsp_crc_err:1;
> + u64 rsp_bad_sts:1;
> + u64 rsp_val:1;
> + u64 rsp_type:3;
> + u64 cmd_type:2;
> + u64 cmd_idx:6;
> + u64 cmd_done:1;
> +#else
> + u64 cmd_done:1;
> + u64 cmd_idx:6;
> + u64 cmd_type:2;
> + u64 rsp_type:3;
> + u64 rsp_val:1;
> + u64 rsp_bad_sts:1;
> + u64 rsp_crc_err:1;
> + u64 rsp_timeout:1;
> + u64 stp_val:1;
> + u64 stp_bad_sts:1;
> + u64 stp_crc_err:1;
> + u64 stp_timeout:1;
> + u64 rsp_busybit:1;
> + u64 blk_crc_err:1;
> + u64 blk_timeout:1;
> + u64 dbuf:1;
> + u64 :4;
> + u64 dbuf_err:1;
> + u64 :27;
> + u64 dma_pend:1;
> + u64 dma_val:1;
> + u64 switch_val:1;
> + u64 cmd_val:1;
> + u64 bus_id:2;
> + u64 :2;
> +#endif
> + } s;
> +};
> +
> +union mio_emm_sample {
> + u64 val;
> + struct mio_emm_sample_s {
> +#ifdef __BIG_ENDIAN_BITFIELD
> + u64 :38;
> + u64 cmd_cnt:10;
> + u64 :6;
> + u64 dat_cnt:10;
> +#else
> + u64 dat_cnt:10;
> + u64 :6;
> + u64 cmd_cnt:10;
> + u64 :38;
> +#endif
> + } s;
> +};
> +
> +union mio_emm_switch {
> + u64 val;
> + struct mio_emm_switch_s {
> +#ifdef __BIG_ENDIAN_BITFIELD
> + u64 :2;
> + u64 bus_id:2;
> + u64 switch_exe:1;
> + u64 switch_err0:1;
> + u64 switch_err1:1;
> + u64 switch_err2:1;
> + u64 :7;
> + u64 hs_timing:1;
> + u64 :5;
> + u64 bus_width:3;
> + u64 :4;
> + u64 power_class:4;
> + u64 clk_hi:16;
> + u64 clk_lo:16;
> +#else
> + u64 clk_lo:16;
> + u64 clk_hi:16;
> + u64 power_class:4;
> + u64 :4;
> + u64 bus_width:3;
> + u64 :5;
> + u64 hs_timing:1;
> + u64 :7;
> + u64 switch_err2:1;
> + u64 switch_err1:1;
> + u64 switch_err0:1;
> + u64 switch_exe:1;
> + u64 bus_id:2;
> + u64 :2;
> +#endif
> + } s;
> +};
> +
> +/* Protoypes */
> +irqreturn_t cvm_mmc_interrupt(int irq, void *dev_id);
> +int cvm_mmc_slot_probe(struct device *dev, struct cvm_mmc_host *host);
> +int cvm_mmc_slot_remove(struct cvm_mmc_slot *slot);
Why do you need this here? Are those intended as library functions for
the different cavium variant drivers?
> +extern const struct mmc_host_ops cvm_mmc_ops;
Why do you need this?
Kind regards
Uffe
[toc] | [next] | [standalone]
| From | Ulf Hansson <ulf.hansson@linaro.org> |
|---|---|
| Date | 2017-03-08 11:00 +0100 |
| Message-ID | <tiGxX-4My-11@gated-at.bofh.it> |
| In reply to | #1591903 |
[...]
>> > Voltage is limited to 3.3v and shared for all slots.
>>
>> What voltage? The I/O voltage or the voltage for the card?
>>
>> VMMC or VMMCQ?
>
> From my understanding both, VMMC and VMMCQ are fixed at 3.3v.
Okay, then make sure to explicitly state that here.
[...]
>> > + if (bad_status(&rsp_sts))
>> > + req->cmd->error = -EILSEQ;
>>
>> I don't think you should treat all errors as -EILSEQ. Please assign a
>> proper error code, depending on the error.
>
> Agreed, -ETIMEDOUT seems more appropriate for the timeouts. I'll go for
> -EIO for the dbuf_err (buffer space missing). What should I use for the
> CRC errors, -EILSEQ?
Yes, correct.
[...]
>> What does this really mean? Is this about HW support for better
>> dealing with data requests?
>
> Did David's reponse answer your questions?
Yes.
[...]
>> > + /*
>> > + * Legacy platform doesn't support regulator but enables power gpio
>> > + * directly during platform probe.
>> > + */
>> > + if (host->global_pwr_gpiod)
>> > + /* Get a sane OCR mask for other parts of the MMC subsytem. */
>> > + return mmc_of_parse_voltage(dev->of_node, &mmc->ocr_avail);
>>
>> Does really the legacy platforms use the mmc voltage range DT bindings!?
>
> The legacy DT's use (in the mmc slot nodes):
>
> voltage-ranges = <3300 3300>;
>
>> I would rather see that you assign a default value to mmc->ocr_avail,
>> than using this binding.
>
> The volatage seems to be identical for all legacy bindings I can find,
> so is it better to not parse it and use the 3.3 as default?
Yes, I think so.
[...]
>> > +union mio_emm_cmd {
>> > + u64 val;
>> > + struct mio_emm_cmd_s {
>> > +#ifdef __BIG_ENDIAN_BITFIELD
>>
>> Huh. Sorry, but this is a big nack from me.
>>
>> This isn't the common method for how we deal with endian issues in the
>> kernel. Please remove all use of the union types here and below. The
>> follow common patterns for how we deal with endian issues.
>
> May I ask why you dislike the bitfields? Or maybe it is easier when I
> explain why I decided to keep them:
My main concern is that is different compared to how we deal with
endian issues in the kernel.
I just don't like homebrewed hacks, but prefers sticking to defacto
standard methods.
>
> - One drawback of bitfields is poor performance on some architectures.
> That is not the case here, both MIPS64 and ARM64 have instructions
> capable of using bitfields without performance impact.
>
> - The used bitfield are all aligned to word size, usually the pattern in
> the driver is to readq / writeq the whole word (therefore the union
> val) and then set or read certain fields. That should avoid IMHO the
> unspecified behaviour the C standard mentions.
>
> - I prefer BIT_ULL and friends for single bits, but using macros for
> more then one bit is (again IMHO) much less readable then using
> bitfiels here. And all the endianess definitions are _only_ in the
> header file.
>
> Also, if I need to convert all of these I'll probably add some new bugs.
> What we have currently works fine on both MIPS and ARM64.
I understand that is will have an impact, however there are plenty of
good references in the kernel for how to do this.
[...]
Kind regards
Uffe
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web