[PATCH 0/4] spi: dw: fixes, and manages resources migration

[PATCH 0/4] spi: dw: fixes, and manages resources migration

[Baruch Siach]

This series migration the spi-dw driver and its mmio wrapper to manges 
resources, and adds a few small fixes. I have a few more patches in the queue 
for adding device tree support, gpio chip selects, and generic spi queue, but 
I'm waiting with time until I get a chance to test them properly.

Baruch Siach (4):
  spi: dw: use managed resources
  spi: dw-mmio: prepare the clock before enabling
  spi: dw: fix memory leak on error path
  spi: dw: drop unused struct dw_spi field

 drivers/spi/spi-dw-mmio.c | 67 ++++++++++++++++-------------------------------
 drivers/spi/spi-dw-pci.c  |  2 +-
 drivers/spi/spi-dw.c      | 28 +++++++++-----------
 drivers/spi/spi-dw.h      |  3 +--
 4 files changed, 38 insertions(+), 62 deletions(-)

-- 
1.8.5.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] spi: dw: use managed resources

[Baruch Siach]

Migrate mmio code and core driver to managed resources to reduce boilerplate
error handling code. Also, handle clk_enable() failure while at it.

This commit doesn't migrate the PCI specific code to managed resources, because
I can't test that code.

Cc: Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Jean-Hugues Deschenes <jean-hugues.deschenes-YGVykHU+fedBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
---
 drivers/spi/spi-dw-mmio.c | 63 ++++++++++++++++-------------------------------
 drivers/spi/spi-dw-pci.c  |  2 +-
 drivers/spi/spi-dw.c      | 18 ++++----------
 drivers/spi/spi-dw.h      |  2 +-
 4 files changed, 28 insertions(+), 57 deletions(-)

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index 168c620..a85ff37 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -33,11 +33,10 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 	struct resource *mem, *ioarea;
 	int ret;
 
-	dwsmmio = kzalloc(sizeof(struct dw_spi_mmio), GFP_KERNEL);
-	if (!dwsmmio) {
-		ret = -ENOMEM;
-		goto err_end;
-	}
+	dwsmmio = devm_kzalloc(&pdev->dev, sizeof(struct dw_spi_mmio),
+			GFP_KERNEL);
+	if (!dwsmmio)
+		return -ENOMEM;
 
 	dws = &dwsmmio->dws;
 
@@ -45,80 +44,60 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!mem) {
 		dev_err(&pdev->dev, "no mem resource?\n");
-		ret = -EINVAL;
-		goto err_kfree;
+		return -EINVAL;
 	}
 
-	ioarea = request_mem_region(mem->start, resource_size(mem),
-			pdev->name);
+	ioarea = devm_request_mem_region(&pdev->dev, mem->start,
+			resource_size(mem), pdev->name);
 	if (!ioarea) {
 		dev_err(&pdev->dev, "SPI region already claimed\n");
-		ret = -EBUSY;
-		goto err_kfree;
+		return -EBUSY;
 	}
 
-	dws->regs = ioremap_nocache(mem->start, resource_size(mem));
+	dws->regs = devm_ioremap_nocache(&pdev->dev, mem->start,
+			resource_size(mem));
 	if (!dws->regs) {
 		dev_err(&pdev->dev, "SPI region already mapped\n");
-		ret = -ENOMEM;
-		goto err_release_reg;
+		return -ENOMEM;
 	}
 
 	dws->irq = platform_get_irq(pdev, 0);
 	if (dws->irq < 0) {
 		dev_err(&pdev->dev, "no irq resource?\n");
-		ret = dws->irq; /* -ENXIO */
-		goto err_unmap;
+		return dws->irq; /* -ENXIO */
 	}
 
-	dwsmmio->clk = clk_get(&pdev->dev, NULL);
-	if (IS_ERR(dwsmmio->clk)) {
-		ret = PTR_ERR(dwsmmio->clk);
-		goto err_unmap;
-	}
-	clk_enable(dwsmmio->clk);
+	dwsmmio->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dwsmmio->clk))
+		return PTR_ERR(dwsmmio->clk);
+	ret = clk_enable(dwsmmio->clk);
+	if (ret)
+		return ret;
 
 	dws->parent_dev = &pdev->dev;
 	dws->bus_num = 0;
 	dws->num_cs = 4;
 	dws->max_freq = clk_get_rate(dwsmmio->clk);
 
-	ret = dw_spi_add_host(dws);
+	ret = dw_spi_add_host(&pdev->dev, dws);
 	if (ret)
-		goto err_clk;
+		goto out;
 
 	platform_set_drvdata(pdev, dwsmmio);
 	return 0;
 
-err_clk:
+out:
 	clk_disable(dwsmmio->clk);
-	clk_put(dwsmmio->clk);
-	dwsmmio->clk = NULL;
-err_unmap:
-	iounmap(dws->regs);
-err_release_reg:
-	release_mem_region(mem->start, resource_size(mem));
-err_kfree:
-	kfree(dwsmmio);
-err_end:
 	return ret;
 }
 
 static int dw_spi_mmio_remove(struct platform_device *pdev)
 {
 	struct dw_spi_mmio *dwsmmio = platform_get_drvdata(pdev);
-	struct resource *mem;
 
 	clk_disable(dwsmmio->clk);
-	clk_put(dwsmmio->clk);
-	dwsmmio->clk = NULL;
-
 	dw_spi_remove_host(&dwsmmio->dws);
-	iounmap(dwsmmio->dws.regs);
-	kfree(dwsmmio);
 
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	release_mem_region(mem->start, resource_size(mem));
 	return 0;
 }
 
diff --git a/drivers/spi/spi-dw-pci.c b/drivers/spi/spi-dw-pci.c
index 66fa995..1c00597 100644
--- a/drivers/spi/spi-dw-pci.c
+++ b/drivers/spi/spi-dw-pci.c
@@ -86,7 +86,7 @@ static int spi_pci_probe(struct pci_dev *pdev,
 			goto err_unmap;
 	}
 
-	ret = dw_spi_add_host(dws);
+	ret = dw_spi_add_host(&pdev->dev, dws);
 	if (ret)
 		goto err_unmap;
 
diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index b897c4ad..e37dfc1 100644
--- a/drivers/spi/spi-dw.c
+++ b/drivers/spi/spi-dw.c
@@ -776,7 +776,7 @@ static void spi_hw_init(struct dw_spi *dws)
 	}
 }
 
-int dw_spi_add_host(struct dw_spi *dws)
+int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
 {
 	struct spi_master *master;
 	int ret;
@@ -784,10 +784,8 @@ int dw_spi_add_host(struct dw_spi *dws)
 	BUG_ON(dws == NULL);
 
 	master = spi_alloc_master(dws->parent_dev, 0);
-	if (!master) {
-		ret = -ENOMEM;
-		goto exit;
-	}
+	if (!master)
+		return -ENOMEM;
 
 	dws->master = master;
 	dws->type = SSI_MOTO_SPI;
@@ -797,7 +795,7 @@ int dw_spi_add_host(struct dw_spi *dws)
 	snprintf(dws->name, sizeof(dws->name), "dw_spi%d",
 			dws->bus_num);
 
-	ret = request_irq(dws->irq, dw_spi_irq, IRQF_SHARED,
+	ret = devm_request_irq(dev, dws->irq, dw_spi_irq, IRQF_SHARED,
 			dws->name, dws);
 	if (ret < 0) {
 		dev_err(&master->dev, "can not get IRQ\n");
@@ -836,7 +834,7 @@ int dw_spi_add_host(struct dw_spi *dws)
 	}
 
 	spi_master_set_devdata(master, dws);
-	ret = spi_register_master(master);
+	ret = devm_spi_register_master(dev, master);
 	if (ret) {
 		dev_err(&master->dev, "problem registering spi master\n");
 		goto err_queue_alloc;
@@ -851,10 +849,8 @@ err_queue_alloc:
 		dws->dma_ops->dma_exit(dws);
 err_diable_hw:
 	spi_enable_chip(dws, 0);
-	free_irq(dws->irq, dws);
 err_free_master:
 	spi_master_put(master);
-exit:
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dw_spi_add_host);
@@ -878,10 +874,6 @@ void dw_spi_remove_host(struct dw_spi *dws)
 	spi_enable_chip(dws, 0);
 	/* Disable clk */
 	spi_set_clk(dws, 0);
-	free_irq(dws->irq, dws);
-
-	/* Disconnect from the SPI framework */
-	spi_unregister_master(dws->master);
 }
 EXPORT_SYMBOL_GPL(dw_spi_remove_host);
 
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 9c57c07..1ccfc18 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -231,7 +231,7 @@ struct dw_spi_chip {
 	void (*cs_control)(u32 command);
 };
 
-extern int dw_spi_add_host(struct dw_spi *dws);
+extern int dw_spi_add_host(struct device *dev, struct dw_spi *dws);
 extern void dw_spi_remove_host(struct dw_spi *dws);
 extern int dw_spi_suspend_host(struct dw_spi *dws);
 extern int dw_spi_resume_host(struct dw_spi *dws);
-- 
1.8.5.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] spi: dw-mmio: prepare the clock before enabling

[Baruch Siach]

This is required for common clock support.

Cc: Jean-Hugues Deschenes <jean-hugues.deschenes-YGVykHU+fedBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
---
 drivers/spi/spi-dw-mmio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index a85ff37..073c14e 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -70,7 +70,7 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 	dwsmmio->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(dwsmmio->clk))
 		return PTR_ERR(dwsmmio->clk);
-	ret = clk_enable(dwsmmio->clk);
+	ret = clk_prepare_enable(dwsmmio->clk);
 	if (ret)
 		return ret;
 
@@ -87,7 +87,7 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 	return 0;
 
 out:
-	clk_disable(dwsmmio->clk);
+	clk_disable_unprepare(dwsmmio->clk);
 	return ret;
 }
 
@@ -95,7 +95,7 @@ static int dw_spi_mmio_remove(struct platform_device *pdev)
 {
 	struct dw_spi_mmio *dwsmmio = platform_get_drvdata(pdev);
 
-	clk_disable(dwsmmio->clk);
+	clk_disable_unprepare(dwsmmio->clk);
 	dw_spi_remove_host(&dwsmmio->dws);
 
 	return 0;
-- 
1.8.5.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] spi: dw: fix memory leak on error path

[Baruch Siach]

Cc: Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
---
 drivers/spi/spi-dw.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index e37dfc1..c3354e8 100644
--- a/drivers/spi/spi-dw.c
+++ b/drivers/spi/spi-dw.c
@@ -616,6 +616,7 @@ static int dw_spi_setup(struct spi_device *spi)
 {
 	struct dw_spi_chip *chip_info = NULL;
 	struct chip_data *chip;
+	int ret;
 
 	/* Only alloc on first setup */
 	chip = spi_get_ctldata(spi);
@@ -656,7 +657,8 @@ static int dw_spi_setup(struct spi_device *spi)
 
 	if (!spi->max_speed_hz) {
 		dev_err(&spi->dev, "No max speed HZ parameter\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_kfree;
 	}
 	chip->speed_hz = spi->max_speed_hz;
 
@@ -669,6 +671,11 @@ static int dw_spi_setup(struct spi_device *spi)
 
 	spi_set_ctldata(spi, chip);
 	return 0;
+
+err_kfree:
+	kfree(chip);
+
+	return ret;
 }
 
 static void dw_spi_cleanup(struct spi_device *spi)
-- 
1.8.5.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] spi: dw: drop unused struct dw_spi field

[Baruch Siach]

Cc: Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
---
 drivers/spi/spi-dw.c | 1 -
 drivers/spi/spi-dw.h | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index c3354e8..4e01019 100644
--- a/drivers/spi/spi-dw.c
+++ b/drivers/spi/spi-dw.c
@@ -427,7 +427,6 @@ static void pump_transfers(unsigned long data)
 	dws->tx_end = dws->tx + transfer->len;
 	dws->rx = transfer->rx_buf;
 	dws->rx_end = dws->rx + transfer->len;
-	dws->cs_change = transfer->cs_change;
 	dws->len = dws->cur_transfer->len;
 	if (chip != dws->prev_chip)
 		cs_change = 1;
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 1ccfc18..cb52cfe 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -135,7 +135,6 @@ struct dw_spi {
 	u8			n_bytes;	/* current is a 1/2 bytes op */
 	u8			max_bits_per_word;	/* maxim is 16b */
 	u32			dma_width;
-	int			cs_change;
 	irqreturn_t		(*transfer_handler)(struct dw_spi *dws);
 	void			(*cs_control)(u32 command);
 
-- 
1.8.5.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] spi: dw: drop unused struct dw_spi field

[Gerhard Sittig]

On Thu, Dec 26, 2013 at 09:00 +0200, Baruch Siach wrote:
> 
> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
> index c3354e8..4e01019 100644
> --- a/drivers/spi/spi-dw.c
> +++ b/drivers/spi/spi-dw.c
> @@ -427,7 +427,6 @@ static void pump_transfers(unsigned long data)
>  	dws->tx_end = dws->tx + transfer->len;
>  	dws->rx = transfer->rx_buf;
>  	dws->rx_end = dws->rx + transfer->len;
> -	dws->cs_change = transfer->cs_change;
>  	dws->len = dws->cur_transfer->len;
>  	if (chip != dws->prev_chip)
>  		cs_change = 1;
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index 1ccfc18..cb52cfe 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -135,7 +135,6 @@ struct dw_spi {
>  	u8			n_bytes;	/* current is a 1/2 bytes op */
>  	u8			max_bits_per_word;	/* maxim is 16b */
>  	u32			dma_width;
> -	int			cs_change;
>  	irqreturn_t		(*transfer_handler)(struct dw_spi *dws);
>  	void			(*cs_control)(u32 command);
>  
> -- 

This looks suspicious.  Are you (the driver) ignoring the
cs_change information which the caller (the code which emits the
SPI message transfer call) provides?  This appears to be a
deficiency in the SPI master's code then.  Callers should be able
to control CS behaviour between the partial transfers of an SPI
message.  It's part of the API.

While you are checking whether the SPI master obeys the caller's
CS change spec, can you check the optional delay between partial
transfers as well (I assume that both get handled in the same
spots of the code path)?  ISTR that these two aspects (and the
lack of GPIO backed chip selects given how the hardware CS line
acts) were real obstacles when trying to use this DW SPI master.

Thank you for looking at it (and for having GPIO CS lines in your
queue, which would unbreak the worst blocker)!


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] spi: dw: fixes, and manages resources migration

[Gerhard Sittig]

On Thu, Dec 26, 2013 at 09:00 +0200, Baruch Siach wrote:
> 
> I have a few more patches in the queue for adding device tree
> support, gpio chip selects, and generic spi queue, but I'm
> waiting with time until I get a chance to test them properly.

Just some notes on GPIO backed CS lines.  When I recently worked
with them (not in mainline, but in a "non-existing" :) tree which
is why I haven't upstreamed), I learned the following:

With cs-gpios you can have an arbitrary number of CS lines,
regardless of how many internal hardwired CS lines the controller
may have.  Make sure to test setups with high CS numbers as well.

Make sure to test with low CS numbers, too, because cs-gpios can
both extend the number of internally provided CS lines, as well
as "shadow" the internal CS lines.  Internal and external lines
can get mixed in arbitrary ways.

The SPI master driver's code appeared to assume that always in
internal CS is involved, without it the transfer of data was
stuck.  So I had to pick an internal CS trigger in dirty ways
despite of using a GPIO line for CS control, just to "unfreeze"
the data transmission.  You may see a similar issue.

To summarize you may want to consider the following DT spec

  ...
  cs-gpios = <&pio 0>, <0>, <0>, <0>, <&pio 1>;
  ...

and run your tests with SPI slaves on CS0, CS1, and CS4.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] spi: dw: drop unused struct dw_spi field

[Baruch Siach]

Hi Gerhard,

On Thu, Dec 26, 2013 at 12:42:58PM +0100, Gerhard Sittig wrote:
> On Thu, Dec 26, 2013 at 09:00 +0200, Baruch Siach wrote:
> > diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
> > index c3354e8..4e01019 100644
> > --- a/drivers/spi/spi-dw.c
> > +++ b/drivers/spi/spi-dw.c
> > @@ -427,7 +427,6 @@ static void pump_transfers(unsigned long data)
> >  	dws->tx_end = dws->tx + transfer->len;
> >  	dws->rx = transfer->rx_buf;
> >  	dws->rx_end = dws->rx + transfer->len;
> > -	dws->cs_change = transfer->cs_change;
> >  	dws->len = dws->cur_transfer->len;
> >  	if (chip != dws->prev_chip)
> >  		cs_change = 1;
> > diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> > index 1ccfc18..cb52cfe 100644
> > --- a/drivers/spi/spi-dw.h
> > +++ b/drivers/spi/spi-dw.h
> > @@ -135,7 +135,6 @@ struct dw_spi {
> >  	u8			n_bytes;	/* current is a 1/2 bytes op */
> >  	u8			max_bits_per_word;	/* maxim is 16b */
> >  	u32			dma_width;
> > -	int			cs_change;
> >  	irqreturn_t		(*transfer_handler)(struct dw_spi *dws);
> >  	void			(*cs_control)(u32 command);
> >  
> > -- 
> 
> This looks suspicious.  Are you (the driver) ignoring the
> cs_change information which the caller (the code which emits the
> SPI message transfer call) provides?  This appears to be a
> deficiency in the SPI master's code then.  Callers should be able
> to control CS behaviour between the partial transfers of an SPI
> message.  It's part of the API.

The pump_transfers() local cs_change variable tracks chip select status 
between transfers. In practice this field is meaningless for this hardware 
without external chip select control, like GPIO.

> While you are checking whether the SPI master obeys the caller's
> CS change spec, can you check the optional delay between partial
> transfers as well (I assume that both get handled in the same
> spots of the code path)?  ISTR that these two aspects (and the
> lack of GPIO backed chip selects given how the hardware CS line
> acts) were real obstacles when trying to use this DW SPI master.

The code in pump_transfers() seems to honor delay_usecs. What was the problem 
you encountered?

> Thank you for looking at it (and for having GPIO CS lines in your
> queue, which would unbreak the worst blocker)!

I really hope I would be able to test these patches. There are some 
organizational changes taking place at the moment that might prevent this. If 
you are interested I'll send you the current queue.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] spi: dw: fixes, and manages resources migration

[Baruch Siach]

Hi Gerhard,

On Thu, Dec 26, 2013 at 12:55:07PM +0100, Gerhard Sittig wrote:
> On Thu, Dec 26, 2013 at 09:00 +0200, Baruch Siach wrote:
> > I have a few more patches in the queue for adding device tree
> > support, gpio chip selects, and generic spi queue, but I'm
> > waiting with time until I get a chance to test them properly.
> 
> Just some notes on GPIO backed CS lines.  When I recently worked
> with them (not in mainline, but in a "non-existing" :) tree which
> is why I haven't upstreamed), I learned the following:
> 
> With cs-gpios you can have an arbitrary number of CS lines,
> regardless of how many internal hardwired CS lines the controller
> may have.  Make sure to test setups with high CS numbers as well.
> 
> Make sure to test with low CS numbers, too, because cs-gpios can
> both extend the number of internally provided CS lines, as well
> as "shadow" the internal CS lines.  Internal and external lines
> can get mixed in arbitrary ways.
> 
> The SPI master driver's code appeared to assume that always in
> internal CS is involved, without it the transfer of data was
> stuck.  So I had to pick an internal CS trigger in dirty ways
> despite of using a GPIO line for CS control, just to "unfreeze"
> the data transmission.  You may see a similar issue.
> 
> To summarize you may want to consider the following DT spec
> 
>   ...
>   cs-gpios = <&pio 0>, <0>, <0>, <0>, <&pio 1>;
>   ...
> 
> and run your tests with SPI slaves on CS0, CS1, and CS4.

Thanks for the head up. I was considering adding a spi-dw specific "dummy-cs" 
property, to be used for cs-gpio numbers that are higher than num-cs.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] spi: dw: drop unused struct dw_spi field

[Gerhard Sittig]

On Thu, Dec 26, 2013 at 13:58 +0200, Baruch Siach wrote:
> 
> Hi Gerhard,
> 
> On Thu, Dec 26, 2013 at 12:42:58PM +0100, Gerhard Sittig wrote:
> > On Thu, Dec 26, 2013 at 09:00 +0200, Baruch Siach wrote:
> > > diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
> > > index c3354e8..4e01019 100644
> > > --- a/drivers/spi/spi-dw.c
> > > +++ b/drivers/spi/spi-dw.c
> > > @@ -427,7 +427,6 @@ static void pump_transfers(unsigned long data)
> > >  	dws->tx_end = dws->tx + transfer->len;
> > >  	dws->rx = transfer->rx_buf;
> > >  	dws->rx_end = dws->rx + transfer->len;
> > > -	dws->cs_change = transfer->cs_change;
> > >  	dws->len = dws->cur_transfer->len;
> > >  	if (chip != dws->prev_chip)
> > >  		cs_change = 1;
> > > diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> > > index 1ccfc18..cb52cfe 100644
> > > --- a/drivers/spi/spi-dw.h
> > > +++ b/drivers/spi/spi-dw.h
> > > @@ -135,7 +135,6 @@ struct dw_spi {
> > >  	u8			n_bytes;	/* current is a 1/2 bytes op */
> > >  	u8			max_bits_per_word;	/* maxim is 16b */
> > >  	u32			dma_width;
> > > -	int			cs_change;
> > >  	irqreturn_t		(*transfer_handler)(struct dw_spi *dws);
> > >  	void			(*cs_control)(u32 command);
> > >  
> > > -- 
> > 
> > This looks suspicious.  Are you (the driver) ignoring the
> > cs_change information which the caller (the code which emits the
> > SPI message transfer call) provides?  This appears to be a
> > deficiency in the SPI master's code then.  Callers should be able
> > to control CS behaviour between the partial transfers of an SPI
> > message.  It's part of the API.
> 
> The pump_transfers() local cs_change variable tracks chip select status 
> between transfers. In practice this field is meaningless for this hardware 
> without external chip select control, like GPIO.

Please re-check, or tell me if your code base is different from
mainline and I'm missing something.

Inspection of local code (v3.13-rc5-4-gf399199c01c7) suggests
that the local cs_change gets preset to 0 and enforced to 1 if
the "chip" (the SPI slave?) changes.  I can't see any other
condition that gets evaluated.  So the current driver does ignore
the caller's request.

Your change removes the dws->cs_change member and its assignment
from transfer->cs_change.  A ':grep cs_change drivers/spi/*dw*'
suggests that the CS change condition as specified by callers
completely gets ignored. :-O  This is a bug in my book.

Regardless of whether the internal hardware implementation of the
SPI controller's dedicated CS lines don't support software
control of the line (which may be the cause of the bug, and
causes trouble even for SPI messages which consist of a single
transfer), and can get fixed or not, is just an implementation
detail that's specific to this setup in combination with the
internal CS line.  But please don't ignore the caller's spec and
thus break the SPI master's use for all other setups as well.
Especially when you already have GPIO controlled CS lines on your
radar.  I still agree with others that the hardware CS line
behaviour should be considered broken (dramatically reduces the
usefulness of the controller), and only GPIO backed CS lines
actually unbreak this master since they do introduce software
control for the signal which previously was completely absent.

> > While you are checking whether the SPI master obeys the caller's
> > CS change spec, can you check the optional delay between partial
> > transfers as well (I assume that both get handled in the same
> > spots of the code path)?  ISTR that these two aspects (and the
> > lack of GPIO backed chip selects given how the hardware CS line
> > acts) were real obstacles when trying to use this DW SPI master.
> 
> The code in pump_transfers() seems to honor delay_usecs. What was the problem 
> you encountered?

Since you made me look at the code:  The delay_usecs spec appears
to only get obeyed _between_ transfers.  It gets ignored for the
last or only transfer of a message.  Which may be unexpected, and
differs from what other SPI controllers implement.  Can't tell
whether this breaks existing SPI slaves, but the delay probably
was introduced for some reason.  I guess deasserting CS while the
slave still wants it does have implications...

And I remember that the DW-SPI master code I've seen in the past
may not have been outright wrong, but somehow was organized in
unexpected ways.  IIRC it handled the end of a transfer and the
end of a message in separate code paths, duplicating quite some
code yet handling the last transfer in different ways.  I was
irritated.  But that must have been a different implementation, I
know for sure it wasn't mainline, or else I had done something
about it.

Please also note that switching to common queue support for the
SPI master (which you have in your queue as well) may in
bypassing fix the last transfer's being special for the DW-SPI
driver.  Then you should not care, it's all just transfers, no
matter how many of them form a message.


> > Thank you for looking at it (and for having GPIO CS lines in your
> > queue, which would unbreak the worst blocker)!
> 
> I really hope I would be able to test these patches. There are some 
> organizational changes taking place at the moment that might prevent this. If 
> you are interested I'll send you the current queue.

Don't worry.  No need to rush anything.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] spi: dw: fixes, and manages resources migration

[Gerhard Sittig]

On Thu, Dec 26, 2013 at 14:12 +0200, Baruch Siach wrote:
> 
> [ ... GPIO controlled CS lines for SPI ... ]
> 
> Thanks for the head up. I was considering adding a spi-dw specific "dummy-cs" 
> property, to be used for cs-gpio numbers that are higher than num-cs.

Look at it the other way for a moment.  These GPIO backed CS
lines aren't "dummies", having control over them and having more
than four of them is not at all exceptional or strange, it's the
actual motivation for cs-gpios in the first place.  And all of
these GPIO lines are "real" and useful.

Try to see the internal CS lines in the hardware of the SPI
controller as the exception instead. :) These are the ones that
are rather limited, both in their number and in their behaviour,
and are of questionable use.  These internal lines of
uncontrollable behaviour only get used in the absence of a GPIO
spec for the CS line.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] spi: dw: drop unused struct dw_spi field

[Baruch Siach]

Hi Gerhard,

On Thu, Dec 26, 2013 at 04:33:46PM +0100, Gerhard Sittig wrote:
> On Thu, Dec 26, 2013 at 13:58 +0200, Baruch Siach wrote:
> > On Thu, Dec 26, 2013 at 12:42:58PM +0100, Gerhard Sittig wrote:
> > > On Thu, Dec 26, 2013 at 09:00 +0200, Baruch Siach wrote:
> > > > diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
> > > > index c3354e8..4e01019 100644
> > > > --- a/drivers/spi/spi-dw.c
> > > > +++ b/drivers/spi/spi-dw.c
> > > > @@ -427,7 +427,6 @@ static void pump_transfers(unsigned long data)
> > > >  	dws->tx_end = dws->tx + transfer->len;
> > > >  	dws->rx = transfer->rx_buf;
> > > >  	dws->rx_end = dws->rx + transfer->len;
> > > > -	dws->cs_change = transfer->cs_change;
> > > >  	dws->len = dws->cur_transfer->len;
> > > >  	if (chip != dws->prev_chip)
> > > >  		cs_change = 1;
> > > > diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> > > > index 1ccfc18..cb52cfe 100644
> > > > --- a/drivers/spi/spi-dw.h
> > > > +++ b/drivers/spi/spi-dw.h
> > > > @@ -135,7 +135,6 @@ struct dw_spi {
> > > >  	u8			n_bytes;	/* current is a 1/2 bytes op */
> > > >  	u8			max_bits_per_word;	/* maxim is 16b */
> > > >  	u32			dma_width;
> > > > -	int			cs_change;
> > > >  	irqreturn_t		(*transfer_handler)(struct dw_spi *dws);
> > > >  	void			(*cs_control)(u32 command);
> > > >  
> > > > -- 
> > > 
> > > This looks suspicious.  Are you (the driver) ignoring the
> > > cs_change information which the caller (the code which emits the
> > > SPI message transfer call) provides?  This appears to be a
> > > deficiency in the SPI master's code then.  Callers should be able
> > > to control CS behaviour between the partial transfers of an SPI
> > > message.  It's part of the API.
> > 
> > The pump_transfers() local cs_change variable tracks chip select status 
> > between transfers. In practice this field is meaningless for this hardware 
> > without external chip select control, like GPIO.
> 
> Please re-check, or tell me if your code base is different from
> mainline and I'm missing something.
> 
> Inspection of local code (v3.13-rc5-4-gf399199c01c7) suggests
> that the local cs_change gets preset to 0 and enforced to 1 if
> the "chip" (the SPI slave?) changes.  I can't see any other
> condition that gets evaluated.  So the current driver does ignore
> the caller's request.

Right. I missed that.

> Your change removes the dws->cs_change member and its assignment
> from transfer->cs_change.  A ':grep cs_change drivers/spi/*dw*'
> suggests that the CS change condition as specified by callers
> completely gets ignored. :-O  This is a bug in my book.

Yes. But this patch doesn't change the situation.

> Regardless of whether the internal hardware implementation of the
> SPI controller's dedicated CS lines don't support software
> control of the line (which may be the cause of the bug, and
> causes trouble even for SPI messages which consist of a single
> transfer), and can get fixed or not, is just an implementation
> detail that's specific to this setup in combination with the
> internal CS line.  But please don't ignore the caller's spec and
> thus break the SPI master's use for all other setups as well.
> Especially when you already have GPIO controlled CS lines on your
> radar.  I still agree with others that the hardware CS line
> behaviour should be considered broken (dramatically reduces the
> usefulness of the controller), and only GPIO backed CS lines
> actually unbreak this master since they do introduce software
> control for the signal which previously was completely absent.

It seems that the cs_contorl callback is a driver specific hack that was meant 
to allow software control of the actual chip-select signal. However, there is 
no implementation of cs_control in current mainline code.

> > > While you are checking whether the SPI master obeys the caller's
> > > CS change spec, can you check the optional delay between partial
> > > transfers as well (I assume that both get handled in the same
> > > spots of the code path)?  ISTR that these two aspects (and the
> > > lack of GPIO backed chip selects given how the hardware CS line
> > > acts) were real obstacles when trying to use this DW SPI master.
> > 
> > The code in pump_transfers() seems to honor delay_usecs. What was the problem 
> > you encountered?
> 
> Since you made me look at the code:  The delay_usecs spec appears
> to only get obeyed _between_ transfers.  It gets ignored for the
> last or only transfer of a message.  Which may be unexpected, and
> differs from what other SPI controllers implement.  Can't tell
> whether this breaks existing SPI slaves, but the delay probably
> was introduced for some reason.  I guess deasserting CS while the
> slave still wants it does have implications...

Right. This is a bug.

> And I remember that the DW-SPI master code I've seen in the past
> may not have been outright wrong, but somehow was organized in
> unexpected ways.  IIRC it handled the end of a transfer and the
> end of a message in separate code paths, duplicating quite some
> code yet handling the last transfer in different ways.  I was
> irritated.  But that must have been a different implementation, I
> know for sure it wasn't mainline, or else I had done something
> about it.

Well, I once wrote a driver for this SPI master. I didn't get much response on 
the mailing list at the time, so I moved on, and this driver went in instead. 
I hope it's not my code that irritated you.

> Please also note that switching to common queue support for the
> SPI master (which you have in your queue as well) may in
> bypassing fix the last transfer's being special for the DW-SPI
> driver.  Then you should not care, it's all just transfers, no
> matter how many of them form a message.

Great. So this problem would solve itself if/when my common queue patch goes 
in.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] spi: dw: fixes, and manages resources migration

[Baruch Siach]

Hi Gerhard,

On Thu, Dec 26, 2013 at 04:42:50PM +0100, Gerhard Sittig wrote:
> On Thu, Dec 26, 2013 at 14:12 +0200, Baruch Siach wrote:
> > 
> > [ ... GPIO controlled CS lines for SPI ... ]
> > 
> > Thanks for the head up. I was considering adding a spi-dw specific "dummy-cs" 
> > property, to be used for cs-gpio numbers that are higher than num-cs.
> 
> Look at it the other way for a moment.  These GPIO backed CS
> lines aren't "dummies", having control over them and having more
> than four of them is not at all exceptional or strange, it's the
> actual motivation for cs-gpios in the first place.  And all of
> these GPIO lines are "real" and useful.
> 
> Try to see the internal CS lines in the hardware of the SPI
> controller as the exception instead. :) These are the ones that
> are rather limited, both in their number and in their behaviour,
> and are of questionable use.  These internal lines of
> uncontrollable behaviour only get used in the absence of a GPIO
> spec for the CS line.

That's what I meant. The "dummy-cs" should be an internal chip-select that 
does not control any slave signal. It is meant to be used only to trigger the 
start of transfer. This is necessary in particular when the number of GPIO 
chip-select lines is higher than the number of internal chip-selects. 
Otherwise, the high bits in the Slave Enable Register are ignored.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] spi: dw: fix memory leak on error path

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

On Thu, Dec 26, 2013 at 09:00:14AM +0200, Baruch Siach wrote:

> @@ -669,6 +671,11 @@ static int dw_spi_setup(struct spi_device *spi)
>  
>  	spi_set_ctldata(spi, chip);
>  	return 0;
> +
> +err_kfree:
> +	kfree(chip);
> +
> +	return ret;
>  }

A better fix would be to convert to devm_kzalloc() so there is no
possibility of paths that don't free (and move the ctldata set earlier
so we don't forget about it on creation).

Indeed this patch will introduce a bug - on the second call to this
function if we hit an error then the struct will be freed but ctldata
won't be cleared.  This will mean that if there is a further call then
ctldata will still point at the old struct and the function will try to
use it rather than allocating a new one.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/4] spi: dw: use managed resources

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1074 bytes --]

On Thu, Dec 26, 2013 at 09:00:12AM +0200, Baruch Siach wrote:

> This commit doesn't migrate the PCI specific code to managed resources, because
> I can't test that code.

Given that this is a simple mechanical transition I don't see that as a
big concern...  I'd be more worried about bugs being introduced due to
the order in which things get unwound than I would about errors from a
conversion to devm unless the unwinding code is already complex.

> -	dws->regs = ioremap_nocache(mem->start, resource_size(mem));
> +	dws->regs = devm_ioremap_nocache(&pdev->dev, mem->start,
> +			resource_size(mem));

You can just use devm_ioremap_resource() - it will check if the resource
is cacheable and map it nocache otherwise.

> -int dw_spi_add_host(struct dw_spi *dws)
> +int dw_spi_add_host(struct device *dev, struct dw_spi *dws)

>  	master = spi_alloc_master(dws->parent_dev, 0);
> -	if (!master) {

Why is the device we're passing in ever going to be different to
dws->parent_dev, or alternatively can we just remove dws->parent_dev
instead and keep the signature change?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/4] spi: dw: drop unused struct dw_spi field

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 227 bytes --]

On Thu, Dec 26, 2013 at 09:00:15AM +0200, Baruch Siach wrote:
> Cc: Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/4] spi: dw: fix memory leak on error path

[Baruch Siach]

Hi Mark,

On Mon, Dec 30, 2013 at 01:08:14PM +0000, Mark Brown wrote:
> On Thu, Dec 26, 2013 at 09:00:14AM +0200, Baruch Siach wrote:
> > @@ -669,6 +671,11 @@ static int dw_spi_setup(struct spi_device *spi)
> >  
> >  	spi_set_ctldata(spi, chip);
> >  	return 0;
> > +
> > +err_kfree:
> > +	kfree(chip);
> > +
> > +	return ret;
> >  }
> 
> A better fix would be to convert to devm_kzalloc() so there is no
> possibility of paths that don't free (and move the ctldata set earlier
> so we don't forget about it on creation).

Good idea. Will do.

> Indeed this patch will introduce a bug - on the second call to this
> function if we hit an error then the struct will be freed but ctldata
> won't be cleared.  This will mean that if there is a further call then
> ctldata will still point at the old struct and the function will try to
> use it rather than allocating a new one.

Thanks for reviewing.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] spi: dw-mmio: prepare the clock before enabling

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]

On Thu, Dec 26, 2013 at 09:00:13AM +0200, Baruch Siach wrote:
> This is required for common clock support.

This is fine but depends on the previous patch.  Consider moving the
clock management to runtime PM (the core has support for managing that
for normal transfers) so that it's only enabled when the IP is in use
which should save a little power.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/4] spi: dw: use managed resources

[Baruch Siach]

Hi Mark,

On Mon, Dec 30, 2013 at 01:24:20PM +0000, Mark Brown wrote:
> On Thu, Dec 26, 2013 at 09:00:12AM +0200, Baruch Siach wrote:
> > This commit doesn't migrate the PCI specific code to managed resources, 
> > because I can't test that code.
> 
> Given that this is a simple mechanical transition I don't see that as a
> big concern...  I'd be more worried about bugs being introduced due to
> the order in which things get unwound than I would about errors from a
> conversion to devm unless the unwinding code is already complex.

OK. Will do.

> > -	dws->regs = ioremap_nocache(mem->start, resource_size(mem));
> > +	dws->regs = devm_ioremap_nocache(&pdev->dev, mem->start,
> > +			resource_size(mem));
> 
> You can just use devm_ioremap_resource() - it will check if the resource
> is cacheable and map it nocache otherwise.

Will do.

> > -int dw_spi_add_host(struct dw_spi *dws)
> > +int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
> 
> >  	master = spi_alloc_master(dws->parent_dev, 0);
> > -	if (!master) {
> 
> Why is the device we're passing in ever going to be different to
> dws->parent_dev, or alternatively can we just remove dws->parent_dev
> instead and keep the signature change?

I'm not sure I follow. What's exactly the alternative to dws->parent_dev?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] spi: dw: use managed resources

[Baruch Siach]

Hi Mark,

On Mon, Dec 30, 2013 at 04:48:38PM +0200, Baruch Siach wrote:
> > >  	master = spi_alloc_master(dws->parent_dev, 0);
> > > -	if (!master) {
> > 
> > Why is the device we're passing in ever going to be different to
> > dws->parent_dev, or alternatively can we just remove dws->parent_dev
> > instead and keep the signature change?
> 
> I'm not sure I follow. What's exactly the alternative to dws->parent_dev?

OK. Got it now. Patches are on the way.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] spi: dw: fixes, and manages resources migration

[Gerhard Sittig]

On Thu, Dec 26, 2013 at 22:34 +0200, Baruch Siach wrote:
> 
> That's what I meant. The "dummy-cs" should be an internal
> chip-select that does not control any slave signal. [ ... ]

Ah, nevermind then, I simply misunderstood. :)  It's good to see
how much thought you already put into this topic.  Thank you
again for working on GPIO backed CS support for the DW-SPI
controller!

Regarding device tree bindings:  Make sure to keep "arbitrary
implementation details" out of the bindings, i.e. only put into
bindings what's related to the hardware, or what _each_ potential
user of this binding needs to do because of hardware features or
hardware limitations, regardless whether this piece of software
is an OS or not or which OS it may be.

So the introduction of another dummy-cs property might be
appropriate given the observed behaviour of the hardware.
Alternatively one might just pick an arbitrary internal CS number
at runtime (probably the highest internal number, i.e. the last
one), which should not be a problem as long as this behaviour is
documented -- GPIOs then can get used for this CS number and all
numbers above, so no functionality is missing.  Mind that this
workaround then is a feature of the Linux driver, and not the DT
binding.

I'm just saying all of this because any DT properties you
introduce not only need to get documented and reviewed, but even
more so because compatibility needs to be kept in the future and
changes are neither easy nor quick.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] spi: dw: fixes, and manages resources migration

[Baruch Siach]

Hi Gerhard,

On Tue, Dec 31, 2013 at 01:34:25PM +0100, Gerhard Sittig wrote:
> On Thu, Dec 26, 2013 at 22:34 +0200, Baruch Siach wrote:
> > That's what I meant. The "dummy-cs" should be an internal
> > chip-select that does not control any slave signal. [ ... ]
> 
> Ah, nevermind then, I simply misunderstood. :)

Thinking about it again I think that "dummy-cs" is not a good name. Maybe 
"disconnected-cs" would be better. This is the actual hardware level 
description of the chip-select, rather than software level use as dummy value.

[...]

> So the introduction of another dummy-cs property might be
> appropriate given the observed behaviour of the hardware.
> Alternatively one might just pick an arbitrary internal CS number
> at runtime (probably the highest internal number, i.e. the last
> one), which should not be a problem as long as this behaviour is
> documented -- GPIOs then can get used for this CS number and all
> numbers above, so no functionality is missing.  Mind that this
> workaround then is a feature of the Linux driver, and not the DT
> binding.

Choosing an arbitrary chip-select number is dangerous. You might damage the 
hardware by choosing wrong. I don't think that relying on documentation 
(where?) is reasonable in this case.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] spi: dw: drop unused struct dw_spi field

[Feng Tang]

Hi Gerhard,

On Thu, 26 Dec 2013 16:33:46 +0100
Gerhard Sittig <gsi-ynQEQJNshbs@public.gmane.org> wrote:

> On Thu, Dec 26, 2013 at 13:58 +0200, Baruch Siach wrote:
> 
> Please re-check, or tell me if your code base is different from
> mainline and I'm missing something.
> 
> Inspection of local code (v3.13-rc5-4-gf399199c01c7) suggests
> that the local cs_change gets preset to 0 and enforced to 1 if
> the "chip" (the SPI slave?) changes.  I can't see any other
> condition that gets evaluated.  So the current driver does ignore
> the caller's request.

For gpio-CS spi slave devices, I don't have much experience.

This dw-spi driver has worked with both low-speed uart and high-speed
 modem device with the internal CS register setting (no extern cs line)
on our platforms (Medfield/Cloverview platforms)

IIRC (I haven't touch this code for long time), some other developer 
contributed the "cs_control" callback to dw spi driver, and I guess it
was for the external CS line.

As myself have no HW to debug the external CS-line, any improvement in
this area for the driver will be highly appreciated.

> 
> > > While you are checking whether the SPI master obeys the caller's
> > > CS change spec, can you check the optional delay between partial
> > > transfers as well (I assume that both get handled in the same
> > > spots of the code path)?  ISTR that these two aspects (and the
> > > lack of GPIO backed chip selects given how the hardware CS line
> > > acts) were real obstacles when trying to use this DW SPI master.
> > 
> > The code in pump_transfers() seems to honor delay_usecs. What was the problem 
> > you encountered?
> 
> Since you made me look at the code:  The delay_usecs spec appears
> to only get obeyed _between_ transfers.  It gets ignored for the
> last or only transfer of a message.  Which may be unexpected, and
> differs from what other SPI controllers implement.  Can't tell
> whether this breaks existing SPI slaves, but the delay probably
> was introduced for some reason.  I guess deasserting CS while the
> slave still wants it does have implications...

Actually quiet several spi controller drivers handle the delay_usecs
this way, like the Blackfin, pl022, you can simply grep delay_usecs
 in drivers/spi/

I wrote this driver several years ago, and IIRC, the intention of
skipping the udelay for last xfer is trying to save some udelay time,
as switching to another spi_msg will surely take a while. I didn't
see problem with it used by low-speed uart spi device or
high speed modem on our platforms.

But if you do see problem with this behavior, we can change it and
also notify the other spi controllers who use the same method. 

Thanks,
Feng


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] spi: dw: drop unused struct dw_spi field

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 358 bytes --]

On Thu, Jan 02, 2014 at 12:21:33AM +0800, Feng Tang wrote:

> But if you do see problem with this behavior, we can change it and
> also notify the other spi controllers who use the same method. 

All the drivers should be being converted to use transfer_one() (and the
forthcoming stuff extending that) which will handle delay_usecs for them
anyway.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/4] spi: dw: drop unused struct dw_spi field

[Baruch Siach]

Hi Mark, Feng,

On Wed, Jan 01, 2014 at 05:32:49PM +0000, Mark Brown wrote:
> On Thu, Jan 02, 2014 at 12:21:33AM +0800, Feng Tang wrote:
> > But if you do see problem with this behavior, we can change it and
> > also notify the other spi controllers who use the same method. 
> 
> All the drivers should be being converted to use transfer_one() (and the
> forthcoming stuff extending that) which will handle delay_usecs for them
> anyway.

As I mentioned in the cover letter of this series, I have patches to convert 
this driver to the generic queue. If someone with access to hardware is 
willing to test, I'll post it. It is not clear yet whether I'll have access to 
hardware in the near future.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] spi: dw: drop unused struct dw_spi field

[Feng Tang]

On Wed, 1 Jan 2014 17:32:49 +0000
Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> On Thu, Jan 02, 2014 at 12:21:33AM +0800, Feng Tang wrote:
> 
> > But if you do see problem with this behavior, we can change it and
> > also notify the other spi controllers who use the same method. 
> 
> All the drivers should be being converted to use transfer_one() (and the
> forthcoming stuff extending that) which will handle delay_usecs for them
> anyway.

That's nice! These general thing should be taken care of by the spi core.

Thanks,
Feng
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html