| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170307104922.GA19134@hardcore>
Date: Tue, 7 Mar 2017 11:49:22 +0100
From: Jan Glauber <jan.glauber@...iumnetworks.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
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 v11 2/9] mmc: cavium: Add core MMC driver for Cavium SOCs
On Fri, Mar 03, 2017 at 12:47:14PM +0100, Ulf Hansson wrote:
> On 6 February 2017 at 14:39, 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.
>
> 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.
> >
> > 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 | 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.
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?
> > + 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.
I'll remove the comment.
> > + * 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?
Did David's reponse answer your questions?
> > + *
> > + * - David Daney <ddaney@...ium.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.
OK, seems to work.
> > + 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.
OK.
> > +
> > + 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!?
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?
> > +
> > + 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.
OK, I'll simplify it.
> 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().
OK.
> > +{
> > + 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.
OK.
> > +
> > + /* 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.
May I ask why you dislike the bitfields? Or maybe it is easier when I
explain why I decided to keep them:
- 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.
> > + 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?
Yes, this is the minimum I need to share the cavium-mmc-core.
> > +extern const struct mmc_host_ops cvm_mmc_ops;
>
> Why do you need this?
Left-over from development, can be removed now.
> Kind regards
> Uffe
Thanks for the review!
Jan
Powered by blists - more mailing lists