On Wed, Nov 21, 2012 at 3:16 AM, chao bi <chao...@intel.com> wrote:
> This patch is to implement SSP SPI controller driver, which has been applied
> and
> validated on intel Moorestown & Medfield platform. The patch are originated by
> Ken Mills <ken.k.mi...@intel.com> and Sylvain Centelles
> <sylvain.centel...@intel.com>,
> and to be further developed by Channing <chao...@intel.com> and Chen Jun
> <jun.d.c...@intel.com> according to their integration & validation on
> Medfield platform.
>
> Signed-off-by: Ken Mills <ken.k.mi...@intel.com>
> Signed-off-by: Sylvain Centelles <sylvain.centel...@intel.com>
> Signed-off-by: channing <chao...@intel.com>
> Signed-off-by: Chen Jun <jun.d.c...@intel.com>
OK...
> +#ifdef DUMP_RX
So since this #define DUMP_RX is not part of this patch and not of the
kernel at large it's basically an #if 0 and all the code within such
defines should be deleted.
But I guess you have this undocumented feature that the developer
is supposed to hack the file and insert #define DUMP_RX to use it.
Idea: if you want to keep this use the kernel verbose debug system.
In drivers/spi/Kconfig we have:
config SPI_DEBUG
boolean "Debug support for SPI drivers"
depends on DEBUG_KERNEL
help
Say "yes" to enable debug messaging (like dev_dbg and pr_debug),
sysfs, and debugfs support in SPI controller and protocol drivers.
So insert something like:
config SPI_VERBOSE_DEBUG
boolean "Verbose debug support for SPI drivers"
depends on SPI_DEBUG
....
Modify Makefile to contain:
ccflags-$(CONFIG_SPI_VERBOSE_DEBUG) := -DVERBOSE_DEBUG
Then put the above withing #ifdef CONFIG_SPI_VERBOSE_DEBUG
Then you can use the dev_vdgb() and friends from <linux/device.h>.
Because I think that's what it is essentially: verbose debugging.
> +static void dump_trailer(const struct device *dev, char *buf, int len, int
> sz)
> +{
> + int tlen1 = (len < sz ? len : sz);
> + int tlen2 = ((len - sz) > sz) ? sz : (len - sz);
> + unsigned char *p;
> + static char msg[MAX_SPI_TRANSFER_SIZE];
> +
> + memset(msg, '\0', sizeof(msg));
> + p = buf;
> + while (p < buf + tlen1)
> + sprintf(msg, "%s%02x", msg, (unsigned int)*p++);
> +
> + if (tlen2 > 0) {
> + sprintf(msg, "%s .....", msg);
> + p = (buf+len) - tlen2;
> + while (p < buf + len)
> + sprintf(msg, "%s%02x", msg, (unsigned int)*p++);
> + }
> +
> + dev_info(dev, "DUMP: %p[0:%d ... %d:%d]:%s", buf, tlen1 - 1,
> + len-tlen2, len - 1, msg);
dev_vdbg().
(...)
> +static inline u32 is_tx_fifo_empty(struct ssp_driver_context *drv_context)
Change return type to bool if you're just returning 0 or 1.
> +{
> + u32 sssr;
> + sssr = read_SSSR(drv_context->ioaddr);
> + if ((sssr & SSSR_TFL_MASK) || (sssr & SSSR_TNF) == 0)
> + return 0;
> + else
> + return 1;
> +}
return false/true.
> +static inline u32 is_rx_fifo_empty(struct ssp_driver_context *drv_context)
> +{
> + return ((read_SSSR(drv_context->ioaddr) & SSSR_RNE) == 0);
> +}
Dito. Here it is even more obvious.
(...)
> +static void flush(struct ssp_driver_context *drv_context)
> +{
> + void *reg = drv_context->ioaddr;
> + u32 i = 0;
> +
> + /* If the transmit fifo is not empty, reset the interface. */
> + if (!is_tx_fifo_empty(drv_context)) {
> + dev_err(&drv_context->pdev->dev,
> + "TX FIFO not empty. Reset of SPI IF");
> + disable_interface(drv_context);
> + return;
> + }
> +
> + dev_dbg(&drv_context->pdev->dev, " SSSR=%x\r\n", read_SSSR(reg));
> + while (!is_rx_fifo_empty(drv_context) && (i < SPI_FIFO_SIZE + 1)) {
> + read_SSDR(reg);
> + i++;
> + }
> + WARN(i > 0, "%d words flush occured\n", i);
WARN really? Why not dev_warn()?
> +static int null_writer(struct ssp_driver_context *drv_context)
> +static int null_reader(struct ssp_driver_context *drv_context)
> +static int u8_writer(struct ssp_driver_context *drv_context)
> +static int u8_reader(struct ssp_driver_context *drv_context)
> +static int u16_writer(struct ssp_driver_context *drv_context)
> +static int u16_reader(struct ssp_driver_context *drv_context)
> +static int u32_writer(struct ssp_driver_context *drv_context)
> +static int u32_reader(struct ssp_driver_context *drv_context)
These seem to all be designed to return 0 or 1 and should then be
bool. It seems strange actually, you would expect that such a
function returns the number of bytes or words read/written.
> +static bool chan_filter(struct dma_chan *chan, void *param)
> +static void unmap_dma_buffers(struct ssp_driver_context *drv_context)
> +static void intel_mid_ssp_spi_dma_done(void *arg)
> +static void intel_mid_ssp_spi_dma_init(struct ssp_driver_context
> *drv_context)
> +static void intel_mid_ssp_spi_dma_exit(struct ssp_driver_context
> *drv_context)
> +static void dma_transfer(struct ssp_driver_context *drv_context)
> +static int map_dma_buffers(struct ssp_driver_context *drv_context)
DMA code looks correct but it'd be nice to get Dan Williams
or Vinod Koul to check it. It's sooo easy to make tiny mistakes
(though it seems this code has seen some testing indeed).
(...)
> +/**
> + * sram_to_ddr_cpy() - Copy data from Langwell SDRAM to DDR
> + * @drv_context: Pointer to the private driver context
> + */
> +static void sram_to_ddr_cpy(struct ssp_driver_context *drv_context)
> +{
> + u32 length = drv_context->len;
> +
> + if ((drv_context->quirks & QUIRKS_DMA_USE_NO_TRAIL)
> + && (drv_context->len > drv_context->rx_fifo_threshold *
> + drv_context->n_bytes))
> + length = TRUNCATE(drv_context->len,
> + drv_context->rx_fifo_threshold *
> drv_context->n_bytes);
TRUNCATE is a too generic name but I'll leave that comment for
the header file where it's defined.
It looks very strange.
Isn't this simply an arithmetic soup construction to say:
length = drv_context->len / (drv_context->rx_fifo_threshold *
drv_context->n_bytes);
Integer division redefined in unintelligible terms.
Please look over this. And that goes for the other instance of TRUNCATE()
as well.
> +
> + memcpy_fromio(drv_context->rx, drv_context->virt_addr_sram_rx,
> length);
> +}
memcpy_fromio() on some SRAM...
If the SRAM is just a RAM why do you need the _fromio() copying?
(Just curious.)
> +static void int_transfer_complete(struct ssp_driver_context *drv_context)
> +{
> + void *reg = drv_context->ioaddr;
> + struct spi_message *msg;
> + struct device *dev = &drv_context->pdev->dev;
> +
> + if (unlikely(drv_context->quirks & QUIRKS_USE_PM_QOS))
> + pm_qos_update_request(&drv_context->pm_qos_req,
> + PM_QOS_DEFAULT_VALUE);
It's weird that using PM QoS is treated as an unlikely oddity.
Should it not be the other way around?
> +
> + if (unlikely(drv_context->quirks & QUIRKS_SRAM_ADDITIONAL_CPY))
> + sram_to_ddr_cpy(drv_context);
> +
> + if (likely(drv_context->quirks & QUIRKS_DMA_USE_NO_TRAIL))
> + drain_trail(drv_context);
Uusally likely() / unlikely() micro-optimization is discouraged,
do you have specific performance numbers for using it so
much here?
> + else
> + /* Stop getting Time Outs */
> + write_SSTO(0, reg);
> +
> + drv_context->cur_msg->status = 0;
> + drv_context->cur_msg->actual_length = drv_context->len;
> +
> +#ifdef DUMP_RX
> + dump_trailer(dev, drv_context->rx, drv_context->len, 16);
> +#endif
Atleast avoid doing these inlined #ifdefs please.
Define a stub up-there where it's defined instead:
#if DUMP_RX
void dump_trailer()
{
...
}
#else
void static inline dump_trailer() {}
#endif
The kernel already relies on the compiler to remove such
code completely, so should be done here as well.
> + dev_dbg(dev, "End of transfer. SSSR:%08X\n", read_SSSR(reg));
> + msg = drv_context->cur_msg;
> + if (likely(msg->complete))
> + msg->complete(msg->context);
> +}
So this is duplicating the code in the bulk SPI code.
Please try to use the generic transfer queue, it's really nice.
> +static void int_transfer_complete_work(struct work_struct *work)
> +{
> + struct ssp_driver_context *drv_context = container_of(work,
> + struct ssp_driver_context, complete_work);
> +
> + int_transfer_complete(drv_context);
> +}
What does "int_" mean in the above function signature?
Repeatedly in the file says interrupt mode is not supported so it can't
be "interrupt". "internal"?
Actaully it seems this is dead code. The only reference in the
driver appears to be when an unused WQ us pointed at this
function.
I think you have some clean-up to do if you don't support interrupt
mode.
> +static void poll_transfer_complete(struct ssp_driver_context *drv_context)
> +{
> + struct spi_message *msg;
> +
> + /* Update total byte transfered return count actual bytes read */
> + drv_context->cur_msg->actual_length +=
> + drv_context->len - (drv_context->rx_end - drv_context->rx);
> +
> + drv_context->cur_msg->status = 0;
> +
> + msg = drv_context->cur_msg;
> + if (likely(msg->complete))
> + msg->complete(msg->context);
> +}
This is also reimplementing the message transfer queue in the core.
Use the generic message queue.
> +/**
> + * ssp_int() - Interrupt handler
> + * @irq
> + * @dev_id
> + *
> + * The SSP interrupt is not used for transfer which are handled by
> + * DMA or polling: only under/over run are catched to detect
> + * broken transfers.
> + */
> +static irqreturn_t ssp_int(int irq, void *dev_id)
> +{
> + struct ssp_driver_context *drv_context = dev_id;
> + void *reg = drv_context->ioaddr;
> + struct device *dev = &drv_context->pdev->dev;
> + u32 status = read_SSSR(reg);
> +
> + /* It should never be our interrupt since SSP will */
> + /* only trigs interrupt for under/over run. */
/*
* Squash to some nice multiline comment will you?
*/
> + if (likely(!(status & drv_context->mask_sr)))
> + return IRQ_NONE;
> +
> + if (status & SSSR_ROR || status & SSSR_TUR) {
> + dev_err(dev, "--- SPI ROR or TUR occurred : SSSR=%x\n",
> status);
> + WARN_ON(1);
> + if (status & SSSR_ROR)
> + dev_err(dev, "we have Overrun\n");
> + if (status & SSSR_TUR)
> + dev_err(dev, "we have Underrun\n");
> + }
> +
> + /* We can fall here when not using DMA mode */
fall? fail?
> + if (!drv_context->cur_msg) {
> + disable_interface(drv_context);
> + disable_triggers(drv_context);
> + }
So you only do something if you *don't* have any messages?
> + /* clear status register */
> + write_SSSR(drv_context->clear_sr, reg);
> + return IRQ_HANDLED;
> +}
I can't see how this error interrupt handler actually handles
errors. Has this been tested? It only seems you print an error
message and go on as usual.
You should tear down the ongoing transfer, set the msg->status
to some error code and/or retransmit or something should you not?
Atleast put in a TODO so that maintainers of this driver
know that there's something unhandled here.
(...)
> +/**
> + * start_bitbanging() - Clock synchronization by bit banging
> + * @drv_context: Pointer to private driver context
> + *
> + * This clock synchronization will be removed as soon as it is
> + * handled by the SCU.
> + */
> +static void start_bitbanging(struct ssp_driver_context *drv_context)
> +{
> + u32 sssr;
> + u32 count = 0;
> + u32 cr0;
> + void *i2c_reg = drv_context->I2C_ioaddr;
> + struct device *dev = &drv_context->pdev->dev;
> + void *reg = drv_context->ioaddr;
> + struct chip_data *chip = spi_get_ctldata(drv_context->cur_msg->spi);
> + cr0 = chip->cr0;
> +
> + dev_warn(dev, "In %s : Starting bit banging\n",\
> + __func__);
> + if (read_SSSR(reg) & SSP_NOT_SYNC)
> + dev_warn(dev, "SSP clock desynchronized.\n");
> + if (!(read_SSCR0(reg) & SSCR0_SSE))
> + dev_warn(dev, "in SSCR0, SSP disabled.\n");
> +
> + dev_dbg(dev, "SSP not ready, start CLK sync\n");
> +
> + write_SSCR0(cr0 & ~SSCR0_SSE, reg);
> + write_SSPSP(0x02010007, reg);
Aha 0x0201007.
Usually we define the bitfields, and actually you have:
<snip from header file>
+#define SSPSP_FSRT (1 << 25) /* Frame Sync Relative Timing */
+#define SSPSP_DMYSTOP(x) ((x) << 23) /* Dummy Stop */
+#define SSPSP_SFRMWDTH(x)((x) << 16) /* Serial Frame Width */
+#define SSPSP_SFRMDLY(x) ((x) << 9) /* Serial Frame Delay */
+#define SSPSP_DMYSTRT(x) ((x) << 7) /* Dummy Start */
+#define SSPSP_STRTDLY(x) ((x) << 4) /* Start Delay */
+#define SSPSP_ETDS (1 << 3) /* End of Transfer data State */
+#define SSPSP_SFRMP (1 << 2) /* Serial Frame Polarity */
+#define SSPSP_SCMODE(x) ((x) << 0) /* Serial Bit Rate Clock Mode */
<end>
Please use these bit specifiers to conjure the magic number instead.
(...)
> + write_I2CDATA(0x3, i2c_reg);
> + udelay(I2C_ACCESS_USDELAY);
> + write_I2CCTRL(0x01070034, i2c_reg);
> + udelay(I2C_ACCESS_USDELAY);
> + write_I2CDATA(0x00000099, i2c_reg);
> + udelay(I2C_ACCESS_USDELAY);
> + write_I2CCTRL(0x01070038, i2c_reg);
> + udelay(I2C_ACCESS_USDELAY);
Dito.
> + /* Bit bang the clock until CSS clears */
> + while ((sssr & 0x400000) && (count < MAX_BITBANGING_LOOP)) {
> + write_I2CDATA(0x2, i2c_reg);
> + udelay(I2C_ACCESS_USDELAY);
> + write_I2CCTRL(0x01070034, i2c_reg);
> + udelay(I2C_ACCESS_USDELAY);
> + write_I2CDATA(0x3, i2c_reg);
> + udelay(I2C_ACCESS_USDELAY);
> + write_I2CCTRL(0x01070034, i2c_reg);
> + udelay(I2C_ACCESS_USDELAY);
Dito.
> + if (count >= MAX_BITBANGING_LOOP)
> + dev_err(dev,
> + "ERROR in %s : infinite loop on bit banging.
> Aborting\n",
> + __func__);
> +
> + dev_dbg(dev, "---Bit bang count=%d\n", count);
> +
> + write_I2CDATA(0x0, i2c_reg);
> + udelay(I2C_ACCESS_USDELAY);
> + write_I2CCTRL(0x01070038, i2c_reg);
Dito.
> +static unsigned int ssp_get_clk_div(int speed)
> +{
> + return max(100000000 / speed, 4) - 1;
> +}
This was nice to see, good use of the max() operator!
(...)
> +static int transfer(struct spi_device *spi, struct spi_message *msg)
This is duplicating the core message transfer queue.
Refactor this code to use the new infrastructure.
(...)
> +static int intel_mid_ssp_spi_probe(struct pci_dev *pdev,
> + const struct pci_device_id *ent)
> +{
> + struct device *dev = &pdev->dev;
> + struct spi_master *master;
> + struct ssp_driver_context *drv_context = 0;
> + int status;
> + u32 iolen = 0;
> + u8 ssp_cfg;
> + int pos;
> + void __iomem *syscfg_ioaddr;
> + unsigned long syscfg;
> +
> + /* Check if the SSP we are probed for has been allocated */
> + /* to operate as SPI. This information is retreived from */
> + /* the field adid of the Vendor-Specific PCI capability */
> + /* which is used as a configuration register. */
/*
* Convert to multiline comment
*/
> + pos = pci_find_capability(pdev, PCI_CAP_ID_VNDR);
> + if (pos > 0) {
> + pci_read_config_byte(pdev,
> + pos + VNDR_CAPABILITY_ADID_OFFSET,
> + &ssp_cfg);
> + } else {
> + dev_info(dev, "No Vendor Specific PCI capability\n");
> + goto err_abort_probe;
> + }
> +
> + if (SSP_CFG_GET_MODE(ssp_cfg) != SSP_CFG_SPI_MODE_ID) {
> + dev_info(dev, "Unsupported SSP mode (%02xh)\n",
> + ssp_cfg);
> + goto err_abort_probe;
> + }
> +
> + dev_info(dev, "found PCI SSP controller(ID: %04xh:%04xh cfg:
> %02xh)\n",
> + pdev->vendor, pdev->device, ssp_cfg);
> +
> + status = pci_enable_device(pdev);
> + if (status)
> + return status;
> +
> + /* Allocate Slave with space for drv_context and null dma buffer */
> + master = spi_alloc_master(dev, sizeof(struct ssp_driver_context));
> +
> + if (!master) {
> + dev_err(dev, "cannot alloc spi_slave\n");
> + status = -ENOMEM;
> + goto err_free_0;
> + }
> +
> + drv_context = spi_master_get_devdata(master);
> + drv_context->master = master;
> +
> + drv_context->pdev = pdev;
> + drv_context->quirks = ent->driver_data;
> +
> + /* Set platform & configuration quirks */
> + if (drv_context->quirks & QUIRKS_PLATFORM_MRST) {
> + /* Apply bit banging workarround on MRST */
> + drv_context->quirks |= QUIRKS_BIT_BANGING;
> + /* MRST slave mode workarrounds */
> + if (SSP_CFG_IS_SPI_SLAVE(ssp_cfg))
> + drv_context->quirks |=
> + QUIRKS_USE_PM_QOS |
> + QUIRKS_SRAM_ADDITIONAL_CPY;
> + }
> + drv_context->quirks |= QUIRKS_DMA_USE_NO_TRAIL;
> + if (SSP_CFG_IS_SPI_SLAVE(ssp_cfg))
> + drv_context->quirks |= QUIRKS_SPI_SLAVE_CLOCK_MODE;
> +
> + master->mode_bits = SPI_CPOL | SPI_CPHA;
> + master->bus_num = SSP_CFG_GET_SPI_BUS_NB(ssp_cfg);
> + master->num_chipselect = 1;
> + master->cleanup = cleanup;
> + master->setup = setup;
> + master->transfer = transfer;
Use the new message queue mechanism.
> + drv_context->dma_wq = create_workqueue("intel_mid_ssp_spi");
> + INIT_WORK(&drv_context->complete_work, int_transfer_complete_work);
As noted this workqueue seems to be completely unused. Kill it.
> + if (drv_context->quirks & QUIRKS_BIT_BANGING) {
> + /* Bit banging on the clock is done through */
> + /* DFT which is available through I2C. */
> + /* get base address of I2C_Serbus registers */
> + drv_context->I2C_paddr = 0xff12b000;
What on earth is this?
Note the comment says "get base address", you're not getting it at
all, you're hardcoding it. Resources like this should be passed in from
the outside.
What will happen in the next platform when some ASIC engineer
decide to move this some pages ahead?
Don't you have some platform data/device tree/ACPI table or
whatever where this is supposed to be stored?
Looks like a criss-cross dependency to some I2C block, and
as such it deserves a big fat comment about the weirdness
going on here.
> + drv_context->I2C_ioaddr =
> + ioremap_nocache(drv_context->I2C_paddr, 0x10);
Like the size of that ioregion.
And use devm_ioremap_nocache() to utilize managed resources.
> + if (!drv_context->I2C_ioaddr) {
> + status = -ENOMEM;
> + goto err_free_3;
> + }
> + }
> +
> + /* Attach to IRQ */
> + drv_context->irq = pdev->irq;
> + status = request_irq(drv_context->irq, ssp_int, IRQF_SHARED,
> + "intel_mid_ssp_spi", drv_context);
Use managed resources throughout:
devm_request_irq() in this case.
> + if (status < 0) {
> + dev_err(&pdev->dev, "can not get IRQ\n");
> + goto err_free_4;
> + }
> +
> + if (drv_context->quirks & QUIRKS_PLATFORM_MDFL) {
> + /* get base address of DMA selector. */
> + syscfg = drv_context->paddr - SYSCFG;
> + syscfg_ioaddr = ioremap_nocache(syscfg, 0x10);
devm_ioremap_nocache()
> + if (!syscfg_ioaddr) {
> + status = -ENOMEM;
> + goto err_free_5;
> + }
> + iowrite32(ioread32(syscfg_ioaddr) | 2, syscfg_ioaddr);
> + }
> +
> + tasklet_init(&drv_context->poll_transfer, poll_transfer,
> + (unsigned long)drv_context);
I think this tasklet can be removed and you can have the SPI core
message queue drive the transfers. But prove me wrong.
> + /* Register with the SPI framework */
> + dev_info(dev, "register with SPI framework (bus spi%d)\n",
> + master->bus_num);
> +
> + status = spi_register_master(master);
> +
> + if (status != 0) {
> + dev_err(dev, "problem registering spi\n");
> + goto err_free_5;
> + }
> +
> + pci_set_drvdata(pdev, drv_context);
> +
> + /* Create the PM_QOS request */
> + if (drv_context->quirks & QUIRKS_USE_PM_QOS)
> + pm_qos_add_request(&drv_context->pm_qos_req,
> + PM_QOS_CPU_DMA_LATENCY,
> + PM_QOS_DEFAULT_VALUE);
> +
> + return status;
> +
> +err_free_5:
> + free_irq(drv_context->irq, drv_context);
> +err_free_4:
> + iounmap(drv_context->I2C_ioaddr);
> +err_free_3:
> + iounmap(drv_context->ioaddr);
These three go away with managed devm_* resources.
> +err_free_2:
> + pci_release_region(pdev, 0);
> +err_free_1:
> + spi_master_put(master);
> +err_free_0:
> + pci_disable_device(pdev);
> +
> + return status;
> +err_abort_probe:
> + dev_info(dev, "Abort probe for SSP %04xh:%04xh\n",
> + pdev->vendor, pdev->device);
> + return -ENODEV;
> +}
(...)
> +static void __devexit intel_mid_ssp_spi_remove(struct pci_dev *pdev)
> +{
> + struct ssp_driver_context *drv_context = pci_get_drvdata(pdev);
> +
> + if (!drv_context)
> + return;
> +
> + /* Release IRQ */
> + free_irq(drv_context->irq, drv_context);
> +
> + iounmap(drv_context->ioaddr);
> + if (drv_context->quirks & QUIRKS_BIT_BANGING)
> + iounmap(drv_context->I2C_ioaddr);
These also go away with devm_*
> +
> + /* disconnect from the SPI framework */
> + spi_unregister_master(drv_context->master);
> +
> + pci_set_drvdata(pdev, NULL);
> + pci_release_region(pdev, 0);
> + pci_disable_device(pdev);
> +
> + return;
> +}
> +
> +#ifdef CONFIG_PM
> +/**
> + * intel_mid_ssp_spi_suspend() - Driver suspend procedure
> + * @pdev: Pointer to the pci_dev struct
> + * @state: pm_message_t
> + */
> +static int intel_mid_ssp_spi_suspend(struct pci_dev *pdev, pm_message_t
> state)
> +{
> + struct ssp_driver_context *drv_context = pci_get_drvdata(pdev);
> + dev_dbg(&pdev->dev, "suspend\n");
> +
> + tasklet_disable(&drv_context->poll_transfer);
> +
> + return 0;
> +}
When using the central message queue you probably just call
spi_master_suspend()
spi_master_resume()
here and the framework takes care of the message queue.
(...)
> +++ b/include/linux/spi/spi-intel-mid-ssp.h
> @@ -0,0 +1,326 @@
> +/*
> + * Copyright (C) Intel 2009
> + * Ken Mills <ken.k.mi...@intel.com>
> + * Sylvain Centelles <sylvain.centel...@intel.com>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Cut these nice wallpapers... OK no big deal maybe.
(...)
For the following review comments, begin with adding:
#include <linux/bitops.h>
So you get the BIT() macro and some more.
> +#define SSP_NOT_SYNC 0x400000
Then you can write:
#define SSP_NOT_SYNC BIT(22)
Which tell us what this is actually about. A flag in bit 22.
> +#define MAX_SPI_TRANSFER_SIZE 8192
> +#define MAX_BITBANGING_LOOP 10000
> +#define SPI_FIFO_SIZE 16
> +
> +/* PM QoS define */
> +#define MIN_EXIT_LATENCY 20
State unit. milliseconds I think?
> +/* SSP assignement configuration from PCI config */
> +#define SSP_CFG_GET_MODE(ssp_cfg) ((ssp_cfg) & 0x07)
> +#define SSP_CFG_GET_SPI_BUS_NB(ssp_cfg) (((ssp_cfg) >> 3) & 0x07)
> +#define SSP_CFG_IS_SPI_SLAVE(ssp_cfg) ((ssp_cfg) & 0x40)
& BIT(6)
> +#define SSP_CFG_SPI_MODE_ID 1
> +/* adid field offset is 6 inside the vendor specific capability */
> +#define VNDR_CAPABILITY_ADID_OFFSET 6
> +
> +/* Driver's quirk flags */
> +/* This workarround bufferizes data in the audio fabric SDRAM from */
> +/* where the DMA transfers will operate. Should be enabled only for */
> +/* SPI slave mode. */
> +#define QUIRKS_SRAM_ADDITIONAL_CPY 1
BIT(0)
> +/* If set the trailing bytes won't be handled by the DMA. */
> +/* Trailing byte feature not fully available. */
> +#define QUIRKS_DMA_USE_NO_TRAIL 2
BIT(1)
> +/* If set, the driver will use PM_QOS to reduce the latency */
> +/* introduced by the deeper C-states which may produce over/under */
> +/* run issues. Must be used in slave mode. In master mode, the */
> +/* latency is not critical, but setting this workarround may */
> +/* improve the SPI throughput. */
> +#define QUIRKS_USE_PM_QOS 4
BIT(2)
> +/* This quirks is set on Moorestown */
> +#define QUIRKS_PLATFORM_MRST 8
BIT(3)
> +/* This quirks is set on Medfield */
> +#define QUIRKS_PLATFORM_MDFL 16
BIT(4)
> +/* If set, the driver will apply the bitbanging workarround needed */
> +/* to enable defective Langwell stepping A SSP. The defective SSP */
> +/* can be enabled only once, and should never be disabled. */
> +#define QUIRKS_BIT_BANGING 32
BIT(5)
> +/* If set, SPI is in slave clock mode */
> +#define QUIRKS_SPI_SLAVE_CLOCK_MODE 64
BIT(6)
> +/* Uncomment to get RX and TX short dumps after each transfer */
> +/* #define DUMP_RX 1 */
As mentioned in the main file, convert to a Kconfig verbose config option.
> +#define MAX_TRAILING_BYTE_RETRY 16
> +#define MAX_TRAILING_BYTE_LOOP 100
Max iterations?
> +#define DELAY_TO_GET_A_WORD 3
> +#define DFLT_TIMEOUT_VAL 500
milliseconds?
> +#define DEFINE_SSP_REG(reg, off) \
> +static inline u32 read_##reg(void *p) { return __raw_readl(p + (off)); } \
> +static inline void write_##reg(u32 v, void *p) { __raw_writel(v, p + (off));
> }
But the other code is using io-accessors, so what about you use
ioread32()/iowrite32() instead?
In any way readl_relaxed() and writel_relaxed() would be preferable
to this I think?
> +#define RX_DIRECTION 0
> +#define TX_DIRECTION 1
> +
> +#define I2C_ACCESS_USDELAY 10
> +
> +#define DFLT_BITS_PER_WORD 16
> +#define MIN_BITS_PER_WORD 4
> +#define MAX_BITS_PER_WORD 32
> +#define DFLT_FIFO_BURST_SIZE IMSS_FIFO_BURST_8
> +
> +#define TRUNCATE(x, a) ((x) & ~((a)-1))
Too generic name. And what it does is actually mask the (a) upper
bits so it's misleading too.
I'm confused over this macro, as per comments in the code, and
suspect it should be removed in favor of integer division.
If you have to keep this then atleast rewrite it using a
static inline.
> +DEFINE_SSP_REG(SSCR0, 0x00)
> +DEFINE_SSP_REG(SSCR1, 0x04)
> +DEFINE_SSP_REG(SSSR, 0x08)
> +DEFINE_SSP_REG(SSITR, 0x0c)
> +DEFINE_SSP_REG(SSDR, 0x10)
> +DEFINE_SSP_REG(SSTO, 0x28)
> +DEFINE_SSP_REG(SSPSP, 0x2c)
> +
> +DEFINE_SSP_REG(I2CCTRL, 0x00);
> +DEFINE_SSP_REG(I2CDATA, 0x04);
> +
> +DEFINE_SSP_REG(GPLR1, 0x04);
> +DEFINE_SSP_REG(GPDR1, 0x0c);
> +DEFINE_SSP_REG(GPSR1, 0x14);
> +DEFINE_SSP_REG(GPCR1, 0x1C);
> +DEFINE_SSP_REG(GAFR1_U, 0x44);
> +
> +#define SYSCFG 0x20bc0
Which means?
> +#define SRAM_BASE_ADDR 0xfffdc000
Should be passed as resource, se above reasoning for the
"I2C" base address. What happens on next ASIC spin when
the engineer move this base offset etc, don't you have any
system discovery?
> +#define SRAM_RX_ADDR SRAM_BASE_ADDR
> +#define SRAM_TX_ADDR (SRAM_BASE_ADDR + MAX_SPI_TRANSFER_SIZE)
> +
> +#define SSCR0_DSS (0x0000000f) /* Data Size Select (mask) */
> +#define SSCR0_DataSize(x) ((x) - 1) /* Data Size Select [4..16] */
No lowercase in macros at all please.
SSCR0_DATASIZE() is fine.
> +#define SSCR0_FRF (0x00000030) /* FRame Format (mask) */
> +#define SSCR0_Motorola (0x0 << 4) /* Motorola's SPI mode */
Dito.
And this is a very funny way to define the integer "0".
I understand the intent but...
> +#define SSCR0_ECS (1 << 6) /* External clock select */
BIT(6)
> +#define SSCR0_SSE (1 << 7) /* Synchronous Serial Port Enable */
BIT(7)
> +
> +#define SSCR0_SCR (0x000fff00) /* Serial Clock Rate (mask) */
> +#define SSCR0_SerClkDiv(x) (((x) - 1) << 8) /* Divisor [1..4096] */
Uppercase.
> +#define SSCR0_EDSS (1 << 20) /* Extended data size select */
> +#define SSCR0_NCS (1 << 21) /* Network clock select */
> +#define SSCR0_RIM (1 << 22) /* Receive FIFO overrrun int mask */
> +#define SSCR0_TUM (1 << 23) /* Transmit FIFO underrun int mask */
BIT(20), BIT(21) ...
> +#define SSCR0_FRDC (0x07000000) /* Frame rate divider control (mask) */
> +#define SSCR0_SlotsPerFrm(x) (((x) - 1) << 24) /* Time slots per frame */
Uppercase.
> +#define SSCR0_ADC (1 << 30) /* Audio clock select */
> +#define SSCR0_MOD (1 << 31) /* Mode (normal or network) */
BIT(30), BIT(31)
> +#define SSCR1_RIE (1 << 0) /* Receive FIFO Interrupt Enable */
> +#define SSCR1_TIE (1 << 1) /* Transmit FIFO Interrupt Enable */
> +#define SSCR1_LBM (1 << 2) /* Loop-Back Mode */
> +#define SSCR1_SPO (1 << 3) /* SSPSCLK polarity setting */
> +#define SSCR1_SPH (1 << 4) /* Motorola SPI SSPSCLK phase setting */
> +#define SSCR1_MWDS (1 << 5) /* Microwire Transmit Data Size */
> +#define SSCR1_TFT (0x000003c0) /* Transmit FIFO Threshold (mask) */
> +#define SSCR1_TxTresh(x) (((x) - 1) << 6) /* level [1..16] */
> +#define SSCR1_RFT (0x00003c00) /* Receive FIFO Threshold (mask) */
> +#define SSCR1_RxTresh(x) (((x) - 1) << 10) /* level [1..16] */
BIT(0), BIT(1)...
> +#define SSSR_TNF (1 << 2) /* Tx FIFO Not Full */
> +#define SSSR_RNE (1 << 3) /* Rx FIFO Not Empty */
> +#define SSSR_BSY (1 << 4) /* SSP Busy */
> +#define SSSR_TFS (1 << 5) /* Tx FIFO Service Request */
> +#define SSSR_RFS (1 << 6) /* Rx FIFO Service Request */
> +#define SSSR_ROR (1 << 7) /* Rx FIFO Overrun */
You know the drill.
> +#define SSSR_TFL_MASK (0x0F << 8) /* Tx FIFO level field mask
> */
> +
> +#define SSCR0_TIM (1 << 23) /* Transmit FIFO Under Run Int Mask
> */
> +#define SSCR0_RIM (1 << 22) /* Receive FIFO Over Run int Mask */
> +#define SSCR0_NCS (1 << 21) /* Network Clock Select */
> +#define SSCR0_EDSS (1 << 20) /* Extended Data Size Select */
> +
> +#define SSCR0_TISSP (1 << 4) /* TI Sync Serial Protocol */
> +#define SSCR0_PSP (3 << 4) /* PSP - Programmable Serial Protocol */
> +#define SSCR1_TTELP (1 << 31) /* TXD Tristate Enable Last Phase */
> +#define SSCR1_TTE (1 << 30) /* TXD Tristate Enable */
> +#define SSCR1_EBCEI (1 << 29) /* Enable Bit Count Error interrupt */
> +#define SSCR1_SCFR (1 << 28) /* Slave Clock free Running */
> +#define SSCR1_ECRA (1 << 27) /* Enable Clock Request A */
> +#define SSCR1_ECRB (1 << 26) /* Enable Clock request B */
> +#define SSCR1_SCLKDIR (1 << 25) /* Serial Bit Rate Clock Direction */
> +#define SSCR1_SFRMDIR (1 << 24) /* Frame Direction */
> +#define SSCR1_RWOT (1 << 23) /* Receive Without Transmit */
> +#define SSCR1_TRAIL (1 << 22) /* Trailing Byte */
> +#define SSCR1_TSRE (1 << 21) /* Transmit Service Request Enable */
> +#define SSCR1_RSRE (1 << 20) /* Receive Service Request Enable */
> +#define SSCR1_TINTE (1 << 19) /* Receiver Time-out Interrupt enable */
> +#define SSCR1_PINTE (1 << 18) /* Trailing Byte Interupt Enable */
> +#define SSCR1_STRF (1 << 15) /* Select FIFO or EFWR */
> +#define SSCR1_EFWR (1 << 14) /* Enable FIFO Write/Read */
> +#define SSCR1_IFS (1 << 16) /* Invert Frame Signal */
> +
> +#define SSSR_BCE (1 << 23) /* Bit Count Error */
> +#define SSSR_CSS (1 << 22) /* Clock Synchronisation Status */
> +#define SSSR_TUR (1 << 21) /* Transmit FIFO Under Run */
> +#define SSSR_EOC (1 << 20) /* End Of Chain */
> +#define SSSR_TINT (1 << 19) /* Receiver Time-out Interrupt */
> +#define SSSR_PINT (1 << 18) /* Peripheral Trailing Byte Interrupt */
Use BIT() macro throughout.
> +#define SSPSP_FSRT (1 << 25) /* Frame Sync Relative Timing */
> +#define SSPSP_DMYSTOP(x) ((x) << 23) /* Dummy Stop */
> +#define SSPSP_SFRMWDTH(x)((x) << 16) /* Serial Frame Width */
> +#define SSPSP_SFRMDLY(x) ((x) << 9) /* Serial Frame Delay */
> +#define SSPSP_DMYSTRT(x) ((x) << 7) /* Dummy Start */
> +#define SSPSP_STRTDLY(x) ((x) << 4) /* Start Delay */
> +#define SSPSP_ETDS (1 << 3) /* End of Transfer data State */
> +#define SSPSP_SFRMP (1 << 2) /* Serial Frame Polarity */
> +#define SSPSP_SCMODE(x) ((x) << 0) /* Serial Bit Rate Clock Mode */
(...)
> +/*
> + * For testing SSCR1 changes that require SSP restart, basically
> + * everything except the service and interrupt enables
> + */
> +
> +#define SSCR1_CHANGE_MASK (SSCR1_TTELP | SSCR1_TTE | SSCR1_SCFR \
> + | SSCR1_ECRA | SSCR1_ECRB | SSCR1_SCLKDIR \
> + | SSCR1_SFRMDIR | SSCR1_RWOT | SSCR1_TRAIL \
> + | SSCR1_IFS | SSCR1_STRF | SSCR1_EFWR \
> + | SSCR1_RFT | SSCR1_TFT | SSCR1_MWDS \
> + | SSCR1_SPH | SSCR1_SPO | SSCR1_LBM)
> +
> +struct callback_param {
> + void *drv_context;
> + u32 direction;
> +};
> +
Convert the inline documentation below to use kerneldoc.
> +struct ssp_driver_context {
> + /* Driver model hookup */
> + struct pci_dev *pdev;
> +
> + /* SPI framework hookup */
> + struct spi_master *master;
> +
> + /* SSP register addresses */
> + unsigned long paddr;
> + void *ioaddr;
> + int irq;
> +
> + /* I2C registers */
> + dma_addr_t I2C_paddr;
> + void *I2C_ioaddr;
Skip the caps.
i2c_paddr, i2c_ioaddr is fine.
But I think "paddr" is a bad name because it probably spells
out "physical address", "daddr" is more to the point, because
dma address is not necessarily == physical address.
> + /* SSP masks*/
> + u32 cr1_sig;
> + u32 cr1;
> + u32 clear_sr;
> + u32 mask_sr;
> +
> + /* PM_QOS request */
> + struct pm_qos_request pm_qos_req;
> +
> + struct tasklet_struct poll_transfer;
> +
> + spinlock_t lock;
> +
> + /* Current message transfer state info */
> + struct spi_message *cur_msg;
> + size_t len;
> + size_t len_dma_rx;
> + size_t len_dma_tx;
> + void *tx;
> + void *tx_end;
> + void *rx;
> + void *rx_end;
> + bool dma_initialized;
> + int dma_mapped;
> + dma_addr_t rx_dma;
> + dma_addr_t tx_dma;
> + u8 n_bytes;
> + int (*write)(struct ssp_driver_context *drv_context);
> + int (*read)(struct ssp_driver_context *drv_context);
> +
> + struct intel_mid_dma_slave dmas_tx;
> + struct intel_mid_dma_slave dmas_rx;
> + struct dma_chan *txchan;
> + struct dma_chan *rxchan;
> + struct workqueue_struct *dma_wq;
> + struct work_struct complete_work;
> +
> + u8 __iomem *virt_addr_sram_tx;
> + u8 __iomem *virt_addr_sram_rx;
> +
> + int txdma_done;
> + int rxdma_done;
> + struct callback_param tx_param;
> + struct callback_param rx_param;
With kerneldoc it's easier to tell what the usecase is for these
callbacks.
> + struct pci_dev *dmac1;
It seems that something like a pci_dev * should be used
to refer to the I2C and SRAM as well?
> +
> + unsigned long quirks;
> + u32 rx_fifo_threshold;
> +};
> +
> +struct chip_data {
> + u32 cr0;
> + u32 cr1;
> + u32 timeout;
> + u8 n_bytes;
> + u8 dma_enabled;
bool?
> + u8 bits_per_word;
> + u32 speed_hz;
Should that be u32? unsigned int seems more apropriate for a frequency.
> + int (*write)(struct ssp_driver_context *drv_context);
> + int (*read)(struct ssp_driver_context *drv_context);
> +};
kerneldoc me.
> +enum intel_mid_ssp_spi_fifo_burst {
> + IMSS_FIFO_BURST_1,
> + IMSS_FIFO_BURST_4,
> + IMSS_FIFO_BURST_8
> +};
> +
> +/* spi_board_info.controller_data for SPI slave devices,
> + * copied to spi_device.platform_data ... mostly for dma tuning
> + */
> +struct intel_mid_ssp_spi_chip {
> + enum intel_mid_ssp_spi_fifo_burst burst_size;
> + u32 timeout;
> + u8 enable_loopback;
> + u8 dma_enabled;
The last two entries looks like they should be bool.
> +};
kerneldoc.
> +#define SPI_DIB_NAME_LEN 16
> +#define SPI_DIB_SPEC_INFO_LEN 10
> +
> +struct spi_dib_header {
> + u32 signature;
> + u32 length;
> + u8 rev;
> + u8 checksum;
> + u8 dib[0];
> +} __packed;
Why is this packed?
Yours,
Linus Walleij
------------------------------------------------------------------------------
LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial
Remotely access PCs and mobile devices and provide instant support
Improve your efficiency, and focus on delivering more value-add services
Discover what IT Professionals Know. Rescue delivers
http://p.sf.net/sfu/logmein_12329d2d
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
