This patchset adds SPI master support for Freescale i.MX233/i.MX28.
This is rebased on top of next-20120713. Please consider applying,
bugs shall now be squashed during the previous two review rounds.
Marek Vasut (10):
mmc: spi: Move SSP register definitions into separate file
mmc: spi: Rename IMX2[38]_MMC to IMX2[38]_SSP
mmc: spi: Add necessary bits into mxs-spi.h
mmc: spi: Pull out parts shared between MMC and SPI
mmc: spi: Pull out the SSP clock configuration function
spi: Add SPI driver for mx233/mx28
mmc: spi: Pull out common DMA parts from MXS MMC
spi: Add DMA support into SPI driver
spi: Add SSP/SPI device tree documentation
ARM: mx28: Add SPI pinmux into imx28.dtsi
Documentation/devicetree/bindings/spi/mxs-spi.txt | 18 +
arch/arm/boot/dts/imx28.dtsi | 39 ++
drivers/clk/mxs/Makefile | 2 +-
drivers/clk/mxs/clk-ssp.c | 62 ++
drivers/mmc/host/mxs-mmc.c | 325 ++++-------
drivers/spi/Kconfig | 7 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-mxs.c | 626 +++++++++++++++++++++
include/linux/spi/mxs-spi.h | 150 +++++
9 files changed, 1011 insertions(+), 219 deletions(-)
create mode 100644 Documentation/devicetree/bindings/spi/mxs-spi.txt
create mode 100644 drivers/clk/mxs/clk-ssp.c
create mode 100644 drivers/spi/spi-mxs.c
create mode 100644 include/linux/spi/mxs-spi.h
V2: Fixes for MX233, to allow compiling as a module (thanks to Attila Kinali).
Rebase on top of next-20120723
Cc: Attila Kinali <redacted>
Cc: Chris Ball <redacted>
CC: Dong Aisheng <redacted>
Cc: Fabio Estevam <redacted>
Cc: Grant Likely <redacted>
Cc: Linux ARM kernel <redacted>
Cc: Mark Brown <redacted>
CC: Shawn Guo <redacted>
--
1.7.10.4
Move the definitions into separate file so separate SPI driver can be
implemented. The SSP controller in MXS can act both as a MMC host and
as a SPI host.
Based on previous attempt by:
Fabio Estevam [off-list ref]
Signed-off-by: Fabio Estevam <redacted>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Attila Kinali <redacted>
Cc: Chris Ball <redacted>
CC: Dong Aisheng <redacted>
Cc: Grant Likely <redacted>
Cc: Linux ARM kernel <redacted>
Cc: Mark Brown <redacted>
CC: Shawn Guo <redacted>
---
drivers/mmc/host/mxs-mmc.c | 87 ++--------------------------------
include/linux/spi/mxs-spi.h | 109 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 112 insertions(+), 84 deletions(-)
create mode 100644 include/linux/spi/mxs-spi.h
Since the SSP controller can act as both SPI and MMC host,
renaming the enum to properly reflect the naming seems
appropriate.
Based on previous attempt by:
Fabio Estevam [off-list ref]
Signed-off-by: Fabio Estevam <redacted>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Attila Kinali <redacted>
Cc: Chris Ball <redacted>
CC: Dong Aisheng <redacted>
Cc: Grant Likely <redacted>
Cc: Linux ARM kernel <redacted>
Cc: Mark Brown <redacted>
CC: Shawn Guo <redacted>
---
drivers/mmc/host/mxs-mmc.c | 18 +++++++++---------
include/linux/spi/mxs-spi.h | 8 ++++----
2 files changed, 13 insertions(+), 13 deletions(-)
Add missing register bits and registers into mxs-spi.h .
These will be used by the SPI driver.
Based on previous attempt by:
Fabio Estevam [off-list ref]
Signed-off-by: Fabio Estevam <redacted>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Attila Kinali <redacted>
Cc: Chris Ball <redacted>
CC: Dong Aisheng <redacted>
Cc: Grant Likely <redacted>
Cc: Linux ARM kernel <redacted>
Cc: Mark Brown <redacted>
CC: Shawn Guo <redacted>
---
include/linux/spi/mxs-spi.h | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
V2: Location of HW_SSP_DATA register differs on MX233 and MX28,
handle accordingly. (Thanks to Attila Kinali for finding this).
Abstract out the common part of private data shared between MMC
and SPI. These shall later allow to use common clock configuration
function.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Attila Kinali <redacted>
Cc: Chris Ball <redacted>
CC: Dong Aisheng <redacted>
Cc: Fabio Estevam <redacted>
Cc: Grant Likely <redacted>
Cc: Linux ARM kernel <redacted>
Cc: Mark Brown <redacted>
CC: Shawn Guo <redacted>
---
drivers/mmc/host/mxs-mmc.c | 107 ++++++++++++++++++++++++-------------------
include/linux/spi/mxs-spi.h | 8 ++++
2 files changed, 67 insertions(+), 48 deletions(-)
@@ -408,15 +411,15 @@ static void mxs_mmc_adtc(struct mxs_mmc_host *host)blocks=1;/* xfer count, block size and count need to be set differently */-if(ssp_is_old(host)){+if(ssp_is_old(ssp)){ctrl0|=BF_SSP(data_size,CTRL0_XFER_COUNT);cmd0|=BF_SSP(log2_blksz,CMD0_BLOCK_SIZE)|BF_SSP(blocks-1,CMD0_BLOCK_COUNT);}else{-writel(data_size,host->base+HW_SSP_XFER_SIZE);+writel(data_size,ssp->base+HW_SSP_XFER_SIZE);writel(BF_SSP(log2_blksz,BLOCK_SIZE_BLOCK_SIZE)|BF_SSP(blocks-1,BLOCK_SIZE_BLOCK_COUNT),-host->base+HW_SSP_BLOCK_SIZE);+ssp->base+HW_SSP_BLOCK_SIZE);}if((cmd->opcode==MMC_STOP_TRANSMISSION)||
@@ -431,11 +434,11 @@ static void mxs_mmc_adtc(struct mxs_mmc_host *host)}/* set the timeout count */-timeout=mxs_ns_to_ssp_ticks(host->clk_rate,data->timeout_ns);-val=readl(host->base+HW_SSP_TIMING(host));+timeout=mxs_ns_to_ssp_ticks(ssp->clk_rate,data->timeout_ns);+val=readl(ssp->base+HW_SSP_TIMING(ssp));val&=~(BM_SSP_TIMING_TIMEOUT);val|=BF_SSP(timeout,TIMING_TIMEOUT);-writel(val,host->base+HW_SSP_TIMING(host));+writel(val,ssp->base+HW_SSP_TIMING(ssp));/* pio */host->ssp_pio_words[0]=ctrl0;
Pull out the MMC clock configuration function and make it
into SSP clock configuration function, so it can be used by
the SPI driver too.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Attila Kinali <redacted>
Cc: Chris Ball <redacted>
CC: Dong Aisheng <redacted>
Cc: Fabio Estevam <redacted>
Cc: Grant Likely <redacted>
Cc: Linux ARM kernel <redacted>
Cc: Mark Brown <redacted>
CC: Shawn Guo <redacted>
---
drivers/clk/mxs/Makefile | 2 +-
drivers/clk/mxs/clk-ssp.c | 62 +++++++++++++++++++++++++++++++++++++++++++
drivers/mmc/host/mxs-mmc.c | 39 +--------------------------
include/linux/spi/mxs-spi.h | 2 ++
4 files changed, 66 insertions(+), 39 deletions(-)
create mode 100644 drivers/clk/mxs/clk-ssp.c
V2: Add EXPORT_SYMBOL_GPL(mxs_ssp_set_clk_rate); to allow compiling as a module.
(Thanks to Attila Kinali).
@@ -2,7 +2,7 @@# Makefile for mxs specific clk#-obj-y+=clk.oclk-pll.oclk-ref.oclk-div.oclk-frac.o+obj-y+=clk.oclk-pll.oclk-ref.oclk-div.oclk-frac.oclk-ssp.oobj-$(CONFIG_SOC_IMX23)+=clk-imx23.oobj-$(CONFIG_SOC_IMX28)+=clk-imx28.o
This is slightly reworked version of the SPI driver.
Support for DT has been added and it's been converted
to queued API.
Based on previous attempt by:
Fabio Estevam [off-list ref]
Signed-off-by: Fabio Estevam <redacted>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Attila Kinali <redacted>
Cc: Chris Ball <redacted>
CC: Dong Aisheng <redacted>
Cc: Grant Likely <redacted>
Cc: Linux ARM kernel <redacted>
Cc: Mark Brown <redacted>
CC: Shawn Guo <redacted>
---
drivers/spi/Kconfig | 7 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-mxs.c | 428 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 436 insertions(+)
create mode 100644 drivers/spi/spi-mxs.c
V2: Adjust the HW_SSP_DATA usage as it's now a parametrized macro
These parts will be used by the MXS SPI driver too.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Attila Kinali <redacted>
Cc: Chris Ball <redacted>
CC: Dong Aisheng <redacted>
Cc: Fabio Estevam <redacted>
Cc: Grant Likely <redacted>
Cc: Linux ARM kernel <redacted>
Cc: Mark Brown <redacted>
CC: Shawn Guo <redacted>
---
drivers/mmc/host/mxs-mmc.c | 88 +++++++++++++++++++++----------------------
drivers/spi/spi-mxs.c | 1 -
include/linux/spi/mxs-spi.h | 9 +++++
3 files changed, 51 insertions(+), 47 deletions(-)
@@ -632,7 +628,7 @@ static int mxs_mmc_probe(struct platform_device *pdev)*tousegenericDMAbindinglaterwhenthehelpersgetin.*/ret=of_property_read_u32(np,"fsl,ssp-dma-channel",-&host->dma_channel);+&ssp->dma_channel);if(ret){dev_err(mmc_dev(host->mmc),"failed to get dma channel\n");
@@ -640,7 +636,7 @@ static int mxs_mmc_probe(struct platform_device *pdev)}}else{ssp->devid=pdev->id_entry->driver_data;-host->dma_channel=dmares->start;+ssp->dma_channel=dmares->start;}host->mmc=mmc;
@@ -673,9 +669,9 @@ static int mxs_mmc_probe(struct platform_device *pdev)dma_cap_zero(mask);dma_cap_set(DMA_SLAVE,mask);-host->dma_data.chan_irq=irq_dma;-host->dmach=dma_request_channel(mask,mxs_mmc_dma_filter,host);-if(!host->dmach){+ssp->dma_data.chan_irq=irq_dma;+ssp->dmach=dma_request_channel(mask,mxs_mmc_dma_filter,host);+if(!ssp->dmach){dev_err(mmc_dev(host->mmc),"%s: failed to request dma\n",__func__);gotoout_clk_put;
@@ -714,7 +710,7 @@ static int mxs_mmc_probe(struct platform_device *pdev)mmc->max_blk_size=1<<0xf;mmc->max_blk_count=(ssp_is_old(ssp))?0xff:0xffffff;mmc->max_req_size=(ssp_is_old(ssp))?0xffff:0xffffffff;-mmc->max_seg_size=dma_get_max_seg_size(host->dmach->device->dev);+mmc->max_seg_size=dma_get_max_seg_size(ssp->dmach->device->dev);platform_set_drvdata(pdev,mmc);
@@ -734,8 +730,8 @@ static int mxs_mmc_probe(struct platform_device *pdev)return0;out_free_dma:-if(host->dmach)-dma_release_channel(host->dmach);+if(ssp->dmach)+dma_release_channel(ssp->dmach);out_clk_put:clk_disable_unprepare(ssp->clk);clk_put(ssp->clk);
@@ -754,8 +750,8 @@ static int mxs_mmc_remove(struct platform_device *pdev)platform_set_drvdata(pdev,NULL);-if(host->dmach)-dma_release_channel(host->dmach);+if(ssp->dmach)+dma_release_channel(ssp->dmach);clk_disable_unprepare(ssp->clk);clk_put(ssp->clk);
@@ -192,6 +196,115 @@ static int mxs_ssp_wait(struct mxs_spi *spi, int offset, int mask, bool set)return0;}+staticvoidmxs_ssp_dma_irq_callback(void*param)+{+structmxs_spi*spi=param;+complete(&spi->c);+}++staticirqreturn_tmxs_ssp_irq_handler(intirq,void*dev_id)+{+structmxs_ssp*ssp=dev_id;+dev_err(ssp->dev,"%s[%i] CTRL1=%08x STATUS=%08x\n",+__func__,__LINE__,+readl(ssp->base+HW_SSP_CTRL1(ssp)),+readl(ssp->base+HW_SSP_STATUS(ssp)));+returnIRQ_HANDLED;+}++staticintmxs_spi_txrx_dma(structmxs_spi*spi,intcs,+unsignedchar*buf,intlen,+int*first,int*last,intwrite)+{+structmxs_ssp*ssp=&spi->ssp;+structdma_async_tx_descriptor*desc;+structscatterlistsg[SG_NUM];+intsg_count;+uint32_tpio=BM_SSP_CTRL0_DATA_XFER|mxs_spi_cs_to_reg(cs);+intret;++if(len>SG_NUM*SG_MAXLEN){+dev_err(ssp->dev,"Data chunk too big for DMA\n");+return-EINVAL;+}++init_completion(&spi->c);++if(*first)+pio|=BM_SSP_CTRL0_LOCK_CS;+if(*last)+pio|=BM_SSP_CTRL0_IGNORE_CRC;+if(!write)+pio|=BM_SSP_CTRL0_READ;++if(ssp->devid==IMX23_SSP)+pio|=len;+else+writel(len,ssp->base+HW_SSP_XFER_SIZE);++/* Queue the PIO register write transfer. */+desc=dmaengine_prep_slave_sg(ssp->dmach,+(structscatterlist*)&pio,+1,DMA_TRANS_NONE,0);+if(!desc){+dev_err(ssp->dev,+"Failed to get PIO reg. write descriptor.\n");+return-EINVAL;+}++/* Queue the DMA data transfer. */+sg_init_table(sg,(len/SG_MAXLEN)+1);+sg_count=0;+while(len){+sg_set_buf(&sg[sg_count++],buf,min(len,SG_MAXLEN));+len-=min(len,SG_MAXLEN);+buf+=min(len,SG_MAXLEN);+}+dma_map_sg(ssp->dev,sg,sg_count,+write?DMA_TO_DEVICE:DMA_FROM_DEVICE);++desc=dmaengine_prep_slave_sg(ssp->dmach,sg,sg_count,+write?DMA_MEM_TO_DEV:DMA_DEV_TO_MEM,+DMA_PREP_INTERRUPT|DMA_CTRL_ACK);++if(!desc){+dev_err(ssp->dev,+"Failed to get DMA data write descriptor.\n");+ret=-EINVAL;+gotoerr;+}++/*+*Thelastdescriptormusthavethiscallback,+*tofinishtheDMAtransaction.+*/+desc->callback=mxs_ssp_dma_irq_callback;+desc->callback_param=spi;++/* Start the transfer. */+dmaengine_submit(desc);+dma_async_issue_pending(ssp->dmach);++ret=wait_for_completion_timeout(&spi->c,+msecs_to_jiffies(SSP_TIMEOUT));++if(!ret){+dev_err(ssp->dev,"DMA transfer timeout\n");+ret=-ETIMEDOUT;+gotoerr;+}++ret=0;++err:+for(--sg_count;sg_count>=0;sg_count--){+dma_unmap_sg(ssp->dev,&sg[sg_count],1,+write?DMA_TO_DEVICE:DMA_FROM_DEVICE);+}++returnret;+}+staticintmxs_spi_txrx_pio(structmxs_spi*spi,intcs,unsignedchar*buf,intlen,int*first,int*last,intwrite)
@@ -277,18 +390,48 @@ static int mxs_spi_transfer_one(struct spi_master *master,first=1;if(&t->transfer_list==m->transfers.prev)last=1;-if(t->rx_buf&&t->tx_buf){+if((t->rx_buf&&t->tx_buf)||(t->rx_dma&&t->tx_dma)){dev_err(ssp->dev,"Cannot send and receive simultaneously\n");return-EINVAL;}-if(t->tx_buf)-status=mxs_spi_txrx_pio(spi,cs,(void*)t->tx_buf,-t->len,&first,&last,1);-if(t->rx_buf)-status=mxs_spi_txrx_pio(spi,cs,t->rx_buf,-t->len,&first,&last,0);+/*+*SmallblockscanbetransferedviaPIO.+*Measuredbyempiricmeans:+*+*ddif=/dev/mtdblock0of=/dev/nullbs=1024kcount=1+*+*DMAonly:2.164808seconds,473.0KB/s+*Combined:1.676276seconds,610.9KB/s+*/+if(t->len<=256){+writel(BM_SSP_CTRL1_DMA_ENABLE,+ssp->base+HW_SSP_CTRL1(ssp)++STMP_OFFSET_REG_CLR);++if(t->tx_buf)+status=mxs_spi_txrx_pio(spi,cs,+(void*)t->tx_buf,+t->len,&first,&last,1);+if(t->rx_buf)+status=mxs_spi_txrx_pio(spi,cs,+t->rx_buf,t->len,+&first,&last,0);+}else{+writel(BM_SSP_CTRL1_DMA_ENABLE,+ssp->base+HW_SSP_CTRL1(ssp)++STMP_OFFSET_REG_SET);++if(t->tx_buf)+status=mxs_spi_txrx_dma(spi,cs,+(void*)t->tx_buf,t->len,+&first,&last,1);+if(t->rx_buf)+status=mxs_spi_txrx_dma(spi,cs,+t->rx_buf,t->len,+&first,&last,0);+}m->actual_length+=t->len;if(status)
@@ -303,6 +446,21 @@ static int mxs_spi_transfer_one(struct spi_master *master,returnstatus;}+staticboolmxs_ssp_dma_filter(structdma_chan*chan,void*param)+{+structmxs_ssp*ssp=param;++if(!mxs_dma_is_apbh(chan))+returnfalse;++if(chan->chan_id!=ssp->dma_channel)+returnfalse;++chan->private=&ssp->dma_data;++returntrue;+}+staticconststructof_device_idmxs_spi_dt_ids[]={{.compatible="fsl,imx23-spi",.data=(void*)IMX23_SSP,},{.compatible="fsl,imx28-spi",.data=(void*)IMX28_SSP,},
@@ -318,15 +476,18 @@ static int __devinit mxs_spi_probe(struct platform_device *pdev)structspi_master*master;structmxs_spi*spi;structmxs_ssp*ssp;-structresource*iores;+structresource*iores,*dmares;structpinctrl*pinctrl;structclk*clk;void__iomem*base;-intdevid;-intret=0;+intdevid,dma_channel;+intret=0,irq_err,irq_dma;+dma_cap_mask_tmask;iores=platform_get_resource(pdev,IORESOURCE_MEM,0);-if(!iores)+irq_err=platform_get_irq(pdev,0);+irq_dma=platform_get_irq(pdev,1);+if(!iores||irq_err<0||irq_dma<0)return-EINVAL;base=devm_request_and_ioremap(&pdev->dev,iores);
@@ -341,10 +502,26 @@ static int __devinit mxs_spi_probe(struct platform_device *pdev)if(IS_ERR(clk))returnPTR_ERR(clk);-if(np)+if(np){devid=(enummxs_ssp_id)of_id->data;-else+/*+*TODO:Thisisatemporarysolutionandshouldbechanged+*tousegenericDMAbindinglaterwhenthehelpersgetin.+*/+ret=of_property_read_u32(np,"fsl,ssp-dma-channel",+&dma_channel);+if(ret){+dev_err(&pdev->dev,+"Failed to get DMA channel\n");+return-EINVAL;+}+}else{+dmares=platform_get_resource(pdev,IORESOURCE_DMA,0);+if(!dmares)+return-EINVAL;devid=pdev->id_entry->driver_data;+dma_channel=dmares->start;+}master=spi_alloc_master(&pdev->dev,sizeof(*spi));if(!master)
@@ -364,8 +541,28 @@ static int __devinit mxs_spi_probe(struct platform_device *pdev)ssp->clk=clk;ssp->base=base;ssp->devid=devid;+ssp->dma_channel=dma_channel;++ret=devm_request_irq(&pdev->dev,irq_err,mxs_ssp_irq_handler,0,+DRIVER_NAME,ssp);+if(ret)+gotoout_master_free;++dma_cap_zero(mask);+dma_cap_set(DMA_SLAVE,mask);+ssp->dma_data.chan_irq=irq_dma;+ssp->dmach=dma_request_channel(mask,mxs_ssp_dma_filter,ssp);+if(!ssp->dmach){+dev_err(ssp->dev,"Failed to request DMA\n");+gotoout_master_free;+}+/*+*Crankuptheclockto120MHz,thiswillbefurtherdividedontoa+*properspeed.+*/clk_prepare_enable(ssp->clk);+clk_set_rate(ssp->clk,120*1000*1000);ssp->clk_rate=clk_get_rate(ssp->clk)/1000;stmp_reset_block(ssp->base);
@@ -0,0 +1,18 @@+* Freescale MX233/MX28 SSP/SPI++Required properties:+- compatible: Should be "fsl,<soc>-spi", where soc is "imx23" or "imx28"+- reg: Offset and length of the register set for the device+- interrupts: Should contain SSP interrupts (error irq first, dma irq second)+- fsl,ssp-dma-channel: APBX DMA channel for the SSP++Example:++ssp0: ssp at 80010000 {+ #address-cells = <1>;+ #size-cells = <0>;+ compatible = "fsl,imx28-spi";+ reg = <0x80010000 2000>;+ interrupts = <96 82>;+ fsl,ssp-dma-channel = <0>;+};
From: Attila Kinali <hidden> Date: 2012-07-24 09:25:27
On Mon, 23 Jul 2012 22:40:42 +0200
Marek Vasut [off-list ref] wrote:
This patchset adds SPI master support for Freescale i.MX233/i.MX28.
This is rebased on top of next-20120713. Please consider applying,
bugs shall now be squashed during the previous two review rounds.
The patchset is good and works as expectd
Tested-by: Attila Kinali <redacted>
Attila Kinali
--
The trouble with you, Shev, is you don't say anything until you've saved
up a whole truckload of damned heavy brick arguments and then you dump
them all out and never look at the bleeding body mangled beneath the heap
-- Tirin, The Dispossessed, U. Le Guin
@@ -0,0 +1,18 @@+* Freescale MX233/MX28 SSP/SPI++Required properties:+- compatible: Should be "fsl,<soc>-spi", where soc is "imx23" or "imx28"+- reg: Offset and length of the register set for the device+- interrupts: Should contain SSP interrupts (error irq first, dma irq second)+- fsl,ssp-dma-channel: APBX DMA channel for the SSP++Example:++ssp0: ssp at 80010000 {+ #address-cells = <1>;+ #size-cells = <0>;+ compatible = "fsl,imx28-spi";+ reg = <0x80010000 2000>;
@@ -0,0 +1,18 @@+* Freescale MX233/MX28 SSP/SPI++Required properties:+- compatible: Should be "fsl,<soc>-spi", where soc is "imx23" or "imx28"+- reg: Offset and length of the register set for the device+- interrupts: Should contain SSP interrupts (error irq first, dma irq
This is actually not the first place I was curious about in the imx28 DTS ...
yes, the size is 0x2000, but "2000" as is is used in multiple places across
imx28. Shawn?
On Tue, Jul 24, 2012 at 09:43:48PM +0200, Marek Vasut wrote:
This is actually not the first place I was curious about in the imx28 DTS ...
yes, the size is 0x2000, but "2000" as is is used in multiple places across
imx28. Shawn?
It seems that I overlooked the problem when it got introduced at the
first place. I will send a patch to fix it. Thanks for spotting it.
--
Regards,
Shawn
On Tue, Jul 24, 2012 at 09:43:48PM +0200, Marek Vasut wrote:
quoted
This is actually not the first place I was curious about in the imx28 DTS
... yes, the size is 0x2000, but "2000" as is is used in multiple places
across imx28. Shawn?
It seems that I overlooked the problem when it got introduced at the
first place. I will send a patch to fix it. Thanks for spotting it.
Ok good.
Mark, do you want me to send a V2 of only this particular patch or whole set (I
think that's overkill)?
Best regards,
Marek Vasut
+ if (dev->max_speed_hz)
+ hz = dev->max_speed_hz;
+ if (t && t->speed_hz)
+ hz = t->speed_hz;
+ if (hz == 0) {
+ dev_err(&dev->dev, "Cannot continue with zero clock\n");
+ return -EINVAL;
+ }
These two blocks use a different style (the first does the first assign
unconditionally, the second uses initialisation with declaration). I
prefer the first style but YMMV and it doesn't matter.
For the missing clock rate might it make sense to just use whatever the
clock happens to come out as?
From: Mark Brown <hidden> Date: 2012-08-01 20:34:48
On Mon, Jul 23, 2012 at 10:40:50PM +0200, Marek Vasut wrote:
+ /*
+ * Small blocks can be transfered via PIO.
+ * Measured by empiric means:
+ *
+ * dd if=/dev/mtdblock0 of=/dev/null bs=1024k count=1
+ *
+ * DMA only: 2.164808 seconds, 473.0KB/s
+ * Combined: 1.676276 seconds, 610.9KB/s
+ */
I've seen other devices end up using the FIFO length as the decision
point here which makes intuitive sense to me unless the FIFO is
extremely deep.
Not reviewed this properly yet but it looks OK from a first read
through.
On Mon, Jul 23, 2012 at 10:40:48PM +0200, Marek Vasut wrote:
quoted
This is slightly reworked version of the SPI driver.
Support for DT has been added and it's been converted
to queued API.
Looks reasonable overall.
quoted
+ bits_per_word = dev->bits_per_word;
+ if (t && t->bits_per_word)
+ bits_per_word = t->bits_per_word;
+
+ if (bits_per_word != 8) {
+ dev_err(&dev->dev, "%s, unsupported bits_per_word=%d\n",
+ __func__, bits_per_word);
+ return -EINVAL;
+ }
+ if (dev->max_speed_hz)
+ hz = dev->max_speed_hz;
+ if (t && t->speed_hz)
+ hz = t->speed_hz;
+ if (hz == 0) {
+ dev_err(&dev->dev, "Cannot continue with zero clock\n");
+ return -EINVAL;
+ }
These two blocks use a different style (the first does the first assign
unconditionally, the second uses initialisation with declaration). I
prefer the first style but YMMV and it doesn't matter.
For the missing clock rate might it make sense to just use whatever the
clock happens to come out as?
Hm, the max_speed_hz should be the cap for the transfer speed. I will change the
function to this:
hz = dev->max_speed_hz;
if (t && t->speed_hz)
hz = min(hz, t->speed_hz);
On Mon, Jul 23, 2012 at 10:40:50PM +0200, Marek Vasut wrote:
quoted
+ /*
+ * Small blocks can be transfered via PIO.
+ * Measured by empiric means:
+ *
+ * dd if=/dev/mtdblock0 of=/dev/null bs=1024k count=1
+ *
+ * DMA only: 2.164808 seconds, 473.0KB/s
+ * Combined: 1.676276 seconds, 610.9KB/s
+ */
I've seen other devices end up using the FIFO length as the decision
point here which makes intuitive sense to me unless the FIFO is
extremely deep.
Yep, that's all good. This device has not too deep FIFO, but the reason I chose
not to go that way is because the DMA allocations have some overhead too and
this values seemed to be the best.
Not reviewed this properly yet but it looks OK from a first read
through.
From: Mark Brown <hidden> Date: 2012-08-02 16:00:17
On Thu, Aug 02, 2012 at 04:58:38PM +0200, Marek Vasut wrote:
quoted
It'd be nice to only keep the clocks enabled while doing transfers but
again totally non-essential.
Hm, this is spread across mxs. Shawn, is there any plan for PM implementation
for MXS ?
Take a look at s3c64xx - I did something with runtime PM for this, it's
something that can reasonably be implemented in drivers and as far as I
can tell most framework implementations end up using runtime PM anyway.
On Thu, Aug 02, 2012 at 04:58:38PM +0200, Marek Vasut wrote:
quoted
quoted
It'd be nice to only keep the clocks enabled while doing transfers but
again totally non-essential.
Hm, this is spread across mxs. Shawn, is there any plan for PM
implementation for MXS ?
Take a look at s3c64xx - I did something with runtime PM for this, it's
something that can reasonably be implemented in drivers and as far as I
can tell most framework implementations end up using runtime PM anyway.
We don't even support runtime pm on mxs yet IIRC
Best regards,
Marek Vasut
It sounds really strange to manipulate WAIT_FOR_CMD and WAIT_FOR_IRQ
bits to adjust the chip select, and when reading the driver, it seemed
suspicious to me. After going through the datasheet, indeed those bits
are the appropriate one to select between the SS0, SS1 and SS2 chip
selects, but I find the code not really obvious. Would it be possible
to make it more obvious either by adding or comment or doing something
like:
/* Should be put in some header file */
#define BM_SSP_CTRL0_SPI_CS_BITS (20)
+static void mxs_spi_set_cs(struct mxs_spi *spi, unsigned cs)
+{
+ struct mxs_ssp *ssp = &spi->ssp;
+
+ writel(0x3 << BM_SSP_CTRL0_SPI_CS_BITS,
+ ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
+ writel(cs,
+ ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+}
Regards,
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
On Fri, Aug 3, 2012 at 10:38 AM, Thomas Petazzoni
[off-list ref] wrote:
It sounds really strange to manipulate WAIT_FOR_CMD and WAIT_FOR_IRQ
bits to adjust the chip select, and when reading the driver, it seemed
suspicious to me. After going through the datasheet, indeed those bits
are the appropriate one to select between the SS0, SS1 and SS2 chip
selects, but I find the code not really obvious. Would it be possible
to make it more obvious either by adding or comment or doing something
like:
/* Should be put in some header file */
#define BM_SSP_CTRL0_SPI_CS_BITS (20)
+static void mxs_spi_set_cs(struct mxs_spi *spi, unsigned cs)
+{
+ struct mxs_ssp *ssp = &spi->ssp;
+
+ writel(0x3 << BM_SSP_CTRL0_SPI_CS_BITS,
+ ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
+ writel(cs,
+ ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+}
On Fri, Aug 3, 2012 at 10:38 AM, Thomas Petazzoni
[off-list ref] wrote:
quoted
It sounds really strange to manipulate WAIT_FOR_CMD and WAIT_FOR_IRQ
bits to adjust the chip select, and when reading the driver, it seemed
suspicious to me. After going through the datasheet, indeed those bits
are the appropriate one to select between the SS0, SS1 and SS2 chip
selects, but I find the code not really obvious. Would it be possible
to make it more obvious either by adding or comment or doing something
like:
/* Should be put in some header file */
#define BM_SSP_CTRL0_SPI_CS_BITS (20)
+static void mxs_spi_set_cs(struct mxs_spi *spi, unsigned cs)
+{
+ struct mxs_ssp *ssp = &spi->ssp;
+
+ writel(0x3 << BM_SSP_CTRL0_SPI_CS_BITS,
+ ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
+ writel(cs,
+ ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+}
On Fri, Aug 3, 2012 at 10:38 AM, Thomas Petazzoni
[off-list ref] wrote:
quoted
It sounds really strange to manipulate WAIT_FOR_CMD and WAIT_FOR_IRQ
bits to adjust the chip select, and when reading the driver, it seemed
suspicious to me. After going through the datasheet, indeed those bits
are the appropriate one to select between the SS0, SS1 and SS2 chip
selects, but I find the code not really obvious. Would it be possible
to make it more obvious either by adding or comment or doing something
like:
/* Should be put in some header file */
#define BM_SSP_CTRL0_SPI_CS_BITS (20)
+static void mxs_spi_set_cs(struct mxs_spi *spi, unsigned cs)
+{
+ struct mxs_ssp *ssp = &spi->ssp;
+
+ writel(0x3 << BM_SSP_CTRL0_SPI_CS_BITS,
+ ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
+ writel(cs,
+ ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+}
I'll just add the following comment if it's ok with you:
/*
* i.MX28 Datasheet: 17.10.1: HW_SSP_CTRL0
*
* The bits BM_SSP_CTRL0_WAIT_FOR_CMD and BM_SSP_CTRL0_WAIT_FOR_IRQ
* in HW_SSP_CTRL0 register do have multiple usage, please refer to
* the datasheet for further details. In SPI mode, they are used to
* toggle the chip-select lines (nCS pins).
*/
I hope it'll suffice. Recycling bits in registers is really crazy practice and
I'd like to avoid these getting out of scope of this flaw's location.
From: Maxime Ripard <hidden> Date: 2012-08-03 14:30:39
If you don't do it, the bus number will be a bogus value, possibly used
for device file names in drivers like spidev.
Signed-off-by: Maxime Ripard <redacted>
---
drivers/spi/spi-mxs.c | 9 +++++++++
1 file changed, 9 insertions(+)
Ok, I didn't see this thread, obviously.
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com