spi: dw: fixes, and manages resources migration

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

Quoted 2 lines · Baruch Siach

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.

drivers/spi/spi-dw-mmio.c static int dw_spi_mmio_probe(struct platform_device *pdev) · line 58

Quoted 3 lines

drivers/spi/spi-dw-mmio.c +2 -1

-	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.

drivers/spi/spi-dw.c int dw_spi_add_host(struct dw_spi *dws) · line 787

Quoted 5 lines

drivers/spi/spi-dw.c +1 -2

-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?

Quoted 6 lines

drivers/spi/spi-dw.c int dw_spi_add_host(struct dw_spi *dws) · line 787

Quoted 2 lines

drivers/spi/spi-dw.c +0 -1

 	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?

This is required for common clock support.

drivers/spi/spi-dw.c static int dw_spi_setup(struct spi_device *spi) · line 679

Quoted 10 lines · Baruch Siach

drivers/spi/spi-dw.c +5 -0

@@ -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.

drivers/spi/spi-dw.c +0 -1

index c3354e8..4e01019 100644
@@ -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;

drivers/spi/spi-dw.h +0 -0

index 1ccfc18..cb52cfe 100644

Quoted 24 lines · Baruch Siach

drivers/spi/spi-dw.c +0 -1

index c3354e8..4e01019 100644
@@ -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;

drivers/spi/spi-dw.h +0 -1

index 1ccfc18..cb52cfe 100644
@@ -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);
 

Quoted 1 line

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)!

Hi Gerhard,

Quoted 25 lines · Gerhard Sittig

Quoted 24 lines · Baruch Siach

drivers/spi/spi-dw.c +0 -1

index c3354e8..4e01019 100644
@@ -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;

drivers/spi/spi-dw.h +0 -1

index 1ccfc18..cb52cfe 100644
@@ -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);
 

Quoted 8 lines

Quoted 1 line

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.

Quoted 6 lines

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?

Quoted 2 lines

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.

Quoted 35 lines · Baruch Siach

Quoted 25 lines · Gerhard Sittig

Quoted 24 lines · Baruch Siach

drivers/spi/spi-dw.c +0 -1

index c3354e8..4e01019 100644
@@ -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;

drivers/spi/spi-dw.h +0 -1

index 1ccfc18..cb52cfe 100644
@@ -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);
 

Quoted 8 lines

Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 9 lines

Quoted 6 lines

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.

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.

Quoted 10 lines

Quoted 7 lines

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...

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.

Quoted 2 lines · Feng Tang

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.

Quoted 2 lines · Feng Tang

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.

Cc Feng Tang <[email protected]>

Signed-off-by Baruch Siach <[email protected]>

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.

Quoted 3 lines · Baruch Siach

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.

[ ... 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.

Quoted 5 lines · Baruch Siach

[ ... 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. [ ... ]

Quoted 2 lines · Baruch Siach

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. :)

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.