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: Chris Ball <redacted>
Cc: Detlev Zundel <redacted>
CC: Dong Aisheng <redacted>
Cc: Grant Likely <redacted>
Cc: Linux ARM kernel <redacted>
Cc: Rob Herring <redacted>
CC: Shawn Guo <redacted>
Cc: Stefano Babic <redacted>
Cc: Wolfgang Denk <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: Chris Ball <redacted>
Cc: Detlev Zundel <redacted>
CC: Dong Aisheng <redacted>
Cc: Grant Likely <redacted>
Cc: Linux ARM kernel <redacted>
Cc: Rob Herring <redacted>
CC: Shawn Guo <redacted>
Cc: Stefano Babic <redacted>
Cc: Wolfgang Denk <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: Chris Ball <redacted>
Cc: Detlev Zundel <redacted>
CC: Dong Aisheng <redacted>
Cc: Grant Likely <redacted>
Cc: Linux ARM kernel <redacted>
Cc: Rob Herring <redacted>
CC: Shawn Guo <redacted>
Cc: Stefano Babic <redacted>
Cc: Wolfgang Denk <redacted>
---
include/linux/spi/mxs-spi.h | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
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: Chris Ball <redacted>
Cc: Detlev Zundel <redacted>
CC: Dong Aisheng <redacted>
Cc: Fabio Estevam <redacted>
Cc: Grant Likely <redacted>
Cc: Linux ARM kernel <redacted>
Cc: Rob Herring <redacted>
CC: Shawn Guo <redacted>
Cc: Stefano Babic <redacted>
Cc: Wolfgang Denk <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: Chris Ball <redacted>
Cc: Detlev Zundel <redacted>
CC: Dong Aisheng <redacted>
Cc: Fabio Estevam <redacted>
Cc: Grant Likely <redacted>
Cc: Linux ARM kernel <redacted>
Cc: Rob Herring <redacted>
CC: Shawn Guo <redacted>
Cc: Stefano Babic <redacted>
Cc: Wolfgang Denk <redacted>
---
drivers/clk/mxs/Makefile | 2 +-
drivers/clk/mxs/clk-ssp.c | 61 +++++++++++++++++++++++++++++++++++++++++++
drivers/mmc/host/mxs-mmc.c | 39 +--------------------------
include/linux/spi/mxs-spi.h | 2 ++
4 files changed, 65 insertions(+), 39 deletions(-)
create mode 100644 drivers/clk/mxs/clk-ssp.c
@@ -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: Chris Ball <redacted>
Cc: Detlev Zundel <redacted>
CC: Dong Aisheng <redacted>
Cc: Grant Likely <redacted>
Cc: Linux ARM kernel <redacted>
Cc: Rob Herring <redacted>
CC: Shawn Guo <redacted>
Cc: Stefano Babic <redacted>
Cc: Wolfgang Denk <redacted>
---
drivers/spi/Kconfig | 7 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-mxs.c | 427 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 435 insertions(+)
create mode 100644 drivers/spi/spi-mxs.c
V2: Fix my patch version management
Select STMP_DEVICE (thanks Shawn for pointing this out)
@@ -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);
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Chris Ball <redacted>
Cc: Detlev Zundel <redacted>
CC: Dong Aisheng <redacted>
Cc: Fabio Estevam <redacted>
Cc: Grant Likely <redacted>
Cc: Linux ARM kernel <redacted>
Cc: Rob Herring <redacted>
CC: Shawn Guo <redacted>
Cc: Stefano Babic <redacted>
Cc: Wolfgang Denk <redacted>
---
drivers/spi/spi-mxs.c | 230 +++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 215 insertions(+), 15 deletions(-)
V2: Only get DMA resource of we don't do DT configuration (based on observation
by Fabio, thanks!)
@@ -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)
@@ -276,18 +389,48 @@ static int mxs_spi_transfer_one(struct spi_master *host, struct spi_message *m)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)
@@ -317,15 +475,18 @@ static int __devinit mxs_spi_probe(struct platform_device *pdev)structspi_master*host;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);
@@ -340,10 +501,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;+}host=spi_alloc_master(&pdev->dev,sizeof(*spi));if(!host)
@@ -363,8 +540,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_host_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_host_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>;+};
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]
[...]
Bump? Can these be applied now (probably without 10/10 patch)?
Best regards,
Marek Vasut
From: Attila Kinali <hidden> Date: 2012-07-16 07:57:41
Moin,
I just wanted to try out this patchset as i have a use for proper spi
support on i.mx23. But this patch (4/10) fails to apply at line 635
On Fri, 6 Jul 2012 08:17:23 +0200
Marek Vasut [off-list ref] wrote:
@@ -635,6 +640,7 @@ static int mxs_mmc_probe(struct platform_device *pdev)dma_cap_mask_tmask;structregulator*reg_vmmc;enumof_gpio_flagsflags;+structmxs_ssp*ssp;iores=platform_get_resource(pdev,IORESOURCE_MEM,0);dmares=platform_get_resource(pdev,IORESOURCE_DMA,0);
The function variables do not match.
Also a 3way merge didnt work as the comit on which this patchset was
based on wasnt found. I tried linux, linux-next, and the imx repos
from pengutronix and Shawn, to no avail.
Is there any other repo around that i'm not aware of?
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
The function variables do not match.
Also a 3way merge didnt work as the comit on which this patchset was
based on wasnt found. I tried linux, linux-next, and the imx repos
from pengutronix and Shawn, to no avail.
Is there any other repo around that i'm not aware of?
Attila Kinali
Well I have it rebased on top of current -next, but ... will some of the SPI
maintainers possibly apply it to their -next tree any soon? Or why are these
patches stuck as they are without much review ?
It seems Shawn is OK with the latest version, so can these be applied? I'll post
the rebased version again if necessary.
Best regards,
Marek Vasut
From: Attila Kinali <hidden> Date: 2012-07-16 11:32:44
Moin Marek,
On Mon, 16 Jul 2012 12:59:14 +0200
Marek Vasut [off-list ref] wrote:
Well I have it rebased on top of current -next, but ... will some of the SPI
maintainers possibly apply it to their -next tree any soon? Or why are these
patches stuck as they are without much review ?
Do you have somewhere a public git repo with the patches?
I'd like to give them a try.
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
On Fri, Jul 06, 2012 at 06:17:25AM -0000, Marek Vasut wrote:
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: Chris Ball <redacted>
Cc: Detlev Zundel <redacted>
CC: Dong Aisheng <redacted>
Cc: Grant Likely <redacted>
Cc: Linux ARM kernel <redacted>
Cc: Rob Herring <redacted>
CC: Shawn Guo <redacted>
Cc: Stefano Babic <redacted>
Cc: Wolfgang Denk <redacted>
---
drivers/spi/Kconfig | 7 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-mxs.c | 427 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 435 insertions(+)
create mode 100644 drivers/spi/spi-mxs.c
V2: Fix my patch version management
Select STMP_DEVICE (thanks Shawn for pointing this out)
Hi,
I have one question about this patch.
[ ... ]
quoted hunk
index 0000000..3c0b1ac
--- /dev/null+++ b/drivers/spi/spi-mxs.c
[ ... ]
+
+static int __devinit mxs_spi_probe(struct platform_device *pdev)
+{
Is the kfree() here and in the probe function really necessary ?
Couple of reasons for asking: No other SPI master driver calls it in the remove
function (unless I missed it), most drivers don't call it in the probe
function error path, and if I call it in the remove function in a SPI master
driver I am working on, and load/unload the module several times in a row, I get
a nasty kernel crash.
Thanks,
Guenter
Is the kfree() here and in the probe function really necessary ?
It certainly would seem that way.
Couple of reasons for asking: No other SPI master driver calls it in the
remove function (unless I missed it), most drivers don't call it in the
probe function error path, and if I call it in the remove function in a
SPI master driver I am working on, and load/unload the module several
times in a row, I get a nasty kernel crash.
It seems the spi_master class takes care of that kfree() in
spi.c:spi_master_release() . Good catch, thanks!
Is the kfree() here and in the probe function really necessary ?
It certainly would seem that way.
quoted
Couple of reasons for asking: No other SPI master driver calls it in the
remove function (unless I missed it), most drivers don't call it in the
probe function error path, and if I call it in the remove function in a
SPI master driver I am working on, and load/unload the module several
times in a row, I get a nasty kernel crash.
It seems the spi_master class takes care of that kfree() in
spi.c:spi_master_release() . Good catch, thanks!
Given that, and assuming that spi_master_put() results in the call to
spi_master_release() for both the error case in the probe function and for
the release function, I take it that the kfree() is not needed at all,
and that the documentation for spi_alloc_master() is wrong. Does that sound
reasonable ?
Thanks,
Guenter
On Wed, Aug 01, 2012 at 11:53:36AM +0800, Shawn Guo wrote:
On Wed, Aug 01, 2012 at 04:31:04AM +0200, Marek Vasut wrote:
quoted
quoted
Couple of reasons for asking: No other SPI master driver calls it in the
remove function (unless I missed it), most drivers don't call it in the
probe function error path, and if I call it in the remove function in a
SPI master driver I am working on, and load/unload the module several
times in a row, I get a nasty kernel crash.
It seems the spi_master class takes care of that kfree() in
spi.c:spi_master_release() . Good catch, thanks!
I do not hardware setup to confirm that right away. When
spi_master_release will be called exactly? The time that
spi_master_put gets called? I'm trying to understand if the kfree
is not needed only in remove function, or both probe and remove.
I think the call to spi_master_put() triggers the call to spi_master_release().
If so, kfree() would not be needed at all, and the documentation is wrong.
Thanks,
Guenter
On Tue, Jul 31, 2012 at 01:53:00PM -0700, Guenter Roeck wrote:
quoted
+ spi_master_put(host);
+ kfree(host);
+
Is the kfree() here and in the probe function really necessary ?
The following is how the kerneldoc of spi_alloc_master says.
* The caller is responsible for assigning the bus number and initializing
* the master's methods before calling spi_register_master(); and (after errors
* adding the device) calling spi_master_put() and kfree() to prevent a memory
* leak.
Couple of reasons for asking: No other SPI master driver calls it in the remove
function (unless I missed it), most drivers don't call it in the probe
function error path, and if I call it in the remove function in a SPI master
driver I am working on, and load/unload the module several times in a row, I get
a nasty kernel crash.
So sounds like either code or the kerneldoc needs patching?
Regards,
Shawn
On Wed, Aug 01, 2012 at 04:31:04AM +0200, Marek Vasut wrote:
quoted
Couple of reasons for asking: No other SPI master driver calls it in the
remove function (unless I missed it), most drivers don't call it in the
probe function error path, and if I call it in the remove function in a
SPI master driver I am working on, and load/unload the module several
times in a row, I get a nasty kernel crash.
It seems the spi_master class takes care of that kfree() in
spi.c:spi_master_release() . Good catch, thanks!
I do not hardware setup to confirm that right away. When
spi_master_release will be called exactly? The time that
spi_master_put gets called? I'm trying to understand if the kfree
is not needed only in remove function, or both probe and remove.
Regards,
Shawn
On Wed, Aug 01, 2012 at 04:31:04AM +0200, Marek Vasut wrote:
quoted
quoted
Couple of reasons for asking: No other SPI master driver calls it in
the remove function (unless I missed it), most drivers don't call it
in the probe function error path, and if I call it in the remove
function in a SPI master driver I am working on, and load/unload the
module several times in a row, I get a nasty kernel crash.
It seems the spi_master class takes care of that kfree() in
spi.c:spi_master_release() . Good catch, thanks!
I do not hardware setup to confirm that right away. When
spi_master_release will be called exactly? The time that
spi_master_put gets called? I'm trying to understand if the kfree
is not needed only in remove function, or both probe and remove.
I checked, it's called in both cases ... (if .probe() crashes, release() is
called, so kfree() is unneeded)
On Tue, Jul 31, 2012 at 08:35:59PM -0700, Guenter Roeck wrote:
I think the call to spi_master_put() triggers the call to spi_master_release().
If so, kfree() would not be needed at all, and the documentation is wrong.
Also those drivers calling kfree in probe.
Regards,
Shawn
On Tue, Jul 31, 2012 at 08:35:59PM -0700, Guenter Roeck wrote:
quoted
I think the call to spi_master_put() triggers the call to
spi_master_release(). If so, kfree() would not be needed at all, and the
documentation is wrong.
On Wed, Aug 01, 2012 at 07:00:54AM +0200, Marek Vasut wrote:
Dear Shawn Guo,
quoted
On Tue, Jul 31, 2012 at 08:35:59PM -0700, Guenter Roeck wrote:
quoted
I think the call to spi_master_put() triggers the call to
spi_master_release(). If so, kfree() would not be needed at all, and the
documentation is wrong.
Also those drivers calling kfree in probe.
Looks like that to me ...
Doesn't seem to be far spread, fortunately. Only spi-davinci.c, spi-imx.c, and
spi-omap2-mcspi.c as far as I can see, plus the misleading comment in spi.c.
Anyone up for writing some patches ? If not, I'll do it.
Thanks,
Guenter
On Wed, Aug 01, 2012 at 01:58:56PM +0800, Shawn Guo wrote:
On Tue, Jul 31, 2012 at 10:29:47PM -0700, Guenter Roeck wrote:
quoted
Anyone up for writing some patches ? If not, I'll do it.
Go ahead.
Ok, will do. It isn't that simple, actually, since at least some of the drivers
also call spi_master_get(), and thus need two calls to spi_master_put() (or a
call to spi_master_put and a call to kfree).
Guenter
On Wed, Aug 1, 2012 at 10:59 AM, Guenter Roeck [off-list ref] wrote:
On Wed, Aug 01, 2012 at 07:00:54AM +0200, Marek Vasut wrote:
quoted
Dear Shawn Guo,
quoted
On Tue, Jul 31, 2012 at 08:35:59PM -0700, Guenter Roeck wrote:
quoted
I think the call to spi_master_put() triggers the call to
spi_master_release(). If so, kfree() would not be needed at all, and the
documentation is wrong.
Also those drivers calling kfree in probe.
Looks like that to me ...
Doesn't seem to be far spread, fortunately. Only spi-davinci.c, spi-imx.c, and
spi-omap2-mcspi.c
I have a omapsdp I could patch spi-omap2-mcspi.c file thanks for the catch.
as far as I can see, plus the misleading comment in spi.c.
Anyone up for writing some patches ? If not, I'll do it.
Thanks,
Guenter
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
spi-devel-general mailing list
spi-devel-general at lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
On Tue, Jul 31, 2012 at 10:42:28PM -0700, Guenter Roeck wrote:
quoted
On Wed, Aug 01, 2012 at 01:58:56PM +0800, Shawn Guo wrote:
quoted
On Tue, Jul 31, 2012 at 10:29:47PM -0700, Guenter Roeck wrote:
quoted
Anyone up for writing some patches ? If not, I'll do it.
Go ahead.
Ok, will do. It isn't that simple, actually, since at least some of the
drivers also call spi_master_get(), and thus need two calls to
spi_master_put() (or a call to spi_master_put and a call to kfree).
Hmm, are you saying that there must be a spi_master_put call matching
spi_alloc_master? I think we only need to have spi_master_get and
spi_master_put matched.
Naw, spi_master_get() does refcounting, spi_alloc_master() doesnt. You don't
need to match spi_alloc_master() with spi_master_put()
On Tue, Jul 31, 2012 at 10:42:28PM -0700, Guenter Roeck wrote:
On Wed, Aug 01, 2012 at 01:58:56PM +0800, Shawn Guo wrote:
quoted
On Tue, Jul 31, 2012 at 10:29:47PM -0700, Guenter Roeck wrote:
quoted
Anyone up for writing some patches ? If not, I'll do it.
Go ahead.
Ok, will do. It isn't that simple, actually, since at least some of the drivers
also call spi_master_get(), and thus need two calls to spi_master_put() (or a
call to spi_master_put and a call to kfree).
Hmm, are you saying that there must be a spi_master_put call matching
spi_alloc_master? I think we only need to have spi_master_get and
spi_master_put matched.
Regards,
Shawn
On Wed, Aug 01, 2012 at 02:28:40PM +0800, Shawn Guo wrote:
On Tue, Jul 31, 2012 at 10:42:28PM -0700, Guenter Roeck wrote:
quoted
On Wed, Aug 01, 2012 at 01:58:56PM +0800, Shawn Guo wrote:
quoted
On Tue, Jul 31, 2012 at 10:29:47PM -0700, Guenter Roeck wrote:
quoted
Anyone up for writing some patches ? If not, I'll do it.
Go ahead.
Ok, will do. It isn't that simple, actually, since at least some of the drivers
also call spi_master_get(), and thus need two calls to spi_master_put() (or a
call to spi_master_put and a call to kfree).
Hmm, are you saying that there must be a spi_master_put call matching
spi_alloc_master? I think we only need to have spi_master_get and
spi_master_put matched.
Yes, I think that may be so. Of course, I may be wrong, but ultimately that is
what almost all drivers do in the probe error path. Some of the drivers do it in
the remove path as well, though many don't. I suspect that all drivers using
spi_alloc_master() which do not call spi_master_put() in the remove function may
have a memory leak.
Someone who knows the spi infrastructure better than I should have a closer
look, though. The question is really quite simple: For example, in spi-atmel.c,
how is the allocated master freed in the _remove function ? If it doesn't need
the call to spi_master_put(), why do, for example, spi-stmp.c or spi-mpc52xx.c
call it ?
On the other side, I must admit I am getting more and more confused after
looking into the code. For example, the probe function error path in spi-mpc52xx.c
accesses the master's devdata after the call to spi_master_put(). If
spi_master_put() frees the memory as we think it does, the code would access
freed memory. The same happens in the remove path. And spi_master_put() is not
always called, meaning there is either a memory leak or I am completely confused.
Thanks,
Guenter
On Wed, Aug 01, 2012 at 11:16:15AM +0530, Shubhrajyoti Datta wrote:
On Wed, Aug 1, 2012 at 10:59 AM, Guenter Roeck [off-list ref] wrote:
quoted
On Wed, Aug 01, 2012 at 07:00:54AM +0200, Marek Vasut wrote:
quoted
Dear Shawn Guo,
quoted
On Tue, Jul 31, 2012 at 08:35:59PM -0700, Guenter Roeck wrote:
quoted
I think the call to spi_master_put() triggers the call to
spi_master_release(). If so, kfree() would not be needed at all, and the
documentation is wrong.
Also those drivers calling kfree in probe.
Looks like that to me ...
Doesn't seem to be far spread, fortunately. Only spi-davinci.c, spi-imx.c, and
spi-omap2-mcspi.c
I have a omapsdp I could patch spi-omap2-mcspi.c file thanks for the catch.
For that it would be good to determine if there is a memory leak when removing
the driver (I don't see where the memory allocated with spi_alloc_master is
removed).
Thanks,
Guenter
On Wed, Aug 01, 2012 at 08:10:37AM +0200, Marek Vasut wrote:
Dear Shawn Guo,
quoted
On Tue, Jul 31, 2012 at 10:42:28PM -0700, Guenter Roeck wrote:
quoted
On Wed, Aug 01, 2012 at 01:58:56PM +0800, Shawn Guo wrote:
quoted
On Tue, Jul 31, 2012 at 10:29:47PM -0700, Guenter Roeck wrote:
quoted
Anyone up for writing some patches ? If not, I'll do it.
Go ahead.
Ok, will do. It isn't that simple, actually, since at least some of the
drivers also call spi_master_get(), and thus need two calls to
spi_master_put() (or a call to spi_master_put and a call to kfree).
Hmm, are you saying that there must be a spi_master_put call matching
spi_alloc_master? I think we only need to have spi_master_get and
spi_master_put matched.
Naw, spi_master_get() does refcounting, spi_alloc_master() doesnt. You don't
need to match spi_alloc_master() with spi_master_put()
I must be missing something. Why do almost all spi drivers call it in the error
path, even if there is no call to spi_master_get ?
Thanks,
Guenter
On Wed, Aug 01, 2012 at 02:28:40PM +0800, Shawn Guo wrote:
quoted
On Tue, Jul 31, 2012 at 10:42:28PM -0700, Guenter Roeck wrote:
quoted
On Wed, Aug 01, 2012 at 01:58:56PM +0800, Shawn Guo wrote:
quoted
On Tue, Jul 31, 2012 at 10:29:47PM -0700, Guenter Roeck wrote:
quoted
Anyone up for writing some patches ? If not, I'll do it.
Go ahead.
Ok, will do. It isn't that simple, actually, since at least some of the
drivers also call spi_master_get(), and thus need two calls to
spi_master_put() (or a call to spi_master_put and a call to kfree).
Hmm, are you saying that there must be a spi_master_put call matching
spi_alloc_master? I think we only need to have spi_master_get and
spi_master_put matched.
Yes, I think that may be so. Of course, I may be wrong, but ultimately that
is what almost all drivers do in the probe error path. Some of the drivers
do it in the remove path as well, though many don't. I suspect that all
drivers using spi_alloc_master() which do not call spi_master_put() in the
remove function may have a memory leak.
CCing Mark.
Someone who knows the spi infrastructure better than I should have a closer
look, though. The question is really quite simple: For example, in
spi-atmel.c, how is the allocated master freed in the _remove function ?
If it doesn't need the call to spi_master_put(), why do, for example,
spi-stmp.c or spi-mpc52xx.c call it ?
On the other side, I must admit I am getting more and more confused after
looking into the code. For example, the probe function error path in
spi-mpc52xx.c accesses the master's devdata after the call to
spi_master_put(). If spi_master_put() frees the memory as we think it
does, the code would access freed memory. The same happens in the remove
path. And spi_master_put() is not always called, meaning there is either a
memory leak or I am completely confused.
I'll poke through the stuff later if you won't get your answers (later == around
tomorrow)
From: Marek Vasut <hidden> Date: 2012-08-01 06:41:34
Dear Guenter Roeck,
On Wed, Aug 01, 2012 at 11:16:15AM +0530, Shubhrajyoti Datta wrote:
quoted
On Wed, Aug 1, 2012 at 10:59 AM, Guenter Roeck [off-list ref] wrote:
quoted
On Wed, Aug 01, 2012 at 07:00:54AM +0200, Marek Vasut wrote:
quoted
Dear Shawn Guo,
quoted
On Tue, Jul 31, 2012 at 08:35:59PM -0700, Guenter Roeck wrote:
quoted
I think the call to spi_master_put() triggers the call to
spi_master_release(). If so, kfree() would not be needed at all,
and the documentation is wrong.
Also those drivers calling kfree in probe.
Looks like that to me ...
Doesn't seem to be far spread, fortunately. Only spi-davinci.c,
spi-imx.c, and spi-omap2-mcspi.c
I have a omapsdp I could patch spi-omap2-mcspi.c file thanks for the
catch.
For that it would be good to determine if there is a memory leak when
removing the driver (I don't see where the memory allocated with
spi_alloc_master is removed).
When the refcounting for the device reaches 0, it's deallocated. (Aka _put long
enough and it'll disappear)
From: Marek Vasut <hidden> Date: 2012-08-01 06:45:19
Dear Guenter Roeck,
On Wed, Aug 01, 2012 at 08:10:37AM +0200, Marek Vasut wrote:
quoted
Dear Shawn Guo,
quoted
On Tue, Jul 31, 2012 at 10:42:28PM -0700, Guenter Roeck wrote:
quoted
On Wed, Aug 01, 2012 at 01:58:56PM +0800, Shawn Guo wrote:
quoted
On Tue, Jul 31, 2012 at 10:29:47PM -0700, Guenter Roeck wrote:
quoted
Anyone up for writing some patches ? If not, I'll do it.
Go ahead.
Ok, will do. It isn't that simple, actually, since at least some of
the drivers also call spi_master_get(), and thus need two calls to
spi_master_put() (or a call to spi_master_put and a call to kfree).
Hmm, are you saying that there must be a spi_master_put call matching
spi_alloc_master? I think we only need to have spi_master_get and
spi_master_put matched.
Naw, spi_master_get() does refcounting, spi_alloc_master() doesnt. You
don't need to match spi_alloc_master() with spi_master_put()
I must be missing something. Why do almost all spi drivers call it in the
error path, even if there is no call to spi_master_get ?
To push the refcounting to 0, to deallocate the device, I'd say ...
Best regards,
Marek Vasut
On Wed, Aug 01, 2012 at 08:45:19AM +0200, Marek Vasut wrote:
Dear Guenter Roeck,
quoted
On Wed, Aug 01, 2012 at 08:10:37AM +0200, Marek Vasut wrote:
quoted
Dear Shawn Guo,
quoted
On Tue, Jul 31, 2012 at 10:42:28PM -0700, Guenter Roeck wrote:
quoted
On Wed, Aug 01, 2012 at 01:58:56PM +0800, Shawn Guo wrote:
quoted
On Tue, Jul 31, 2012 at 10:29:47PM -0700, Guenter Roeck wrote:
quoted
Anyone up for writing some patches ? If not, I'll do it.
Go ahead.
Ok, will do. It isn't that simple, actually, since at least some of
the drivers also call spi_master_get(), and thus need two calls to
spi_master_put() (or a call to spi_master_put and a call to kfree).
Hmm, are you saying that there must be a spi_master_put call matching
spi_alloc_master? I think we only need to have spi_master_get and
spi_master_put matched.
Naw, spi_master_get() does refcounting, spi_alloc_master() doesnt. You
don't need to match spi_alloc_master() with spi_master_put()
I must be missing something. Why do almost all spi drivers call it in the
error path, even if there is no call to spi_master_get ?
To push the refcounting to 0, to deallocate the device, I'd say ...
Guess we are in violent agreement. The sequence would then either be
master = spi_alloc_device();
...
spi_master_put(master);
or
master = spi_alloc_device();
...
kfree(master);
which makes sense to me. Question still is why most drivers neither call kfree()
nor spi_master_put() in the remove function.
Thanks,
Guenter
On Wed, Aug 01, 2012 at 08:45:19AM +0200, Marek Vasut wrote:
quoted
quoted
I must be missing something. Why do almost all spi drivers call it in the
error path, even if there is no call to spi_master_get ?
To push the refcounting to 0, to deallocate the device, I'd say ...
It's not going to work if spi_master_put is called without
spi_master_get being called before that.
spi_alloc_master() calls device_initialize() which resuires a
device_put() (called from spi_master_put()) to free the device.
Thus each call of either spi_alloc_master() or spi_master_get() must
be paired with an spi_master_put() call to free the resources.
The kfree() is taken care of by the spi_master_release() function that
is called once the last reference to the underlying struct device has
been released. Thus kfree() must not be called after
spi_alloc_master().
Lothar Wa?mann
--
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________
On Tue, Jul 31, 2012 at 11:56:50PM -0700, Guenter Roeck wrote:
Guess we are in violent agreement. The sequence would then either be
master = spi_alloc_device();
The discussion is around spi_alloc_master rather than spi_alloc_device,
isn't it?
Regards,
Shawn
...
spi_master_put(master);
or
master = spi_alloc_device();
...
kfree(master);
which makes sense to me. Question still is why most drivers neither call kfree()
nor spi_master_put() in the remove function.
On Wed, Aug 01, 2012 at 03:50:12PM +0800, Shawn Guo wrote:
On Tue, Jul 31, 2012 at 11:56:50PM -0700, Guenter Roeck wrote:
quoted
Guess we are in violent agreement. The sequence would then either be
master = spi_alloc_device();
The discussion is around spi_alloc_master rather than spi_alloc_device,
isn't it?
Yes, sorry. Too late at night, too tired :(.
Guenter
Regards,
Shawn
quoted
...
spi_master_put(master);
or
master = spi_alloc_device();
...
kfree(master);
which makes sense to me. Question still is why most drivers neither call kfree()
nor spi_master_put() in the remove function.