Re: [PATCH] spi: tegra: add spi driver for SLINK controller
On 10/29/2012 10:18 AM, Laxman Dewangan wrote: > On Monday 29 October 2012 08:47 PM, Stephen Warren wrote: >> On 10/26/2012 12:49 PM, Laxman Dewangan wrote: Why not just always set SLINK_FIFO_ERROR; does it have to be set in the write only if the status was previously asserted? If that is true, how do you avoid a race condition where the bit gets set in SLINK_STATUS after you read it but before you write to clear it? >>> Status gets updated together. There is no steps of updating status. >> Sorry, I don't understand this answer. > > The status should be updated once by HW and so there is no race condition. > HW behavior is that if the tx or Rx error occurs, it updates the status, > generates interrupt and still continue transfer and later on, it > generates the ready. > In first isr, we read status, error status found and so in isr thread, > we reset controller to stop the transfer. > > So in good state, only ready bit will be set and hence writing 1 to > clear it. > In error state, clearing error first in ISR and in isr thread resetting > the controller to stop the controller engine. OK, I see why there's no race. It still seems simply to me if tegra_slink_clear_status() just always writes all the status bits, but I suppose it isn't a correctness issue. >>> Is there a way to support the reset of controller. We will need this >>> functionality. >> >> Why do we need to reset the controller at all; can't we simply program >> all the (few) configuration registers? Are there HW bugs that hang the >> controller and require a reset or something? > > HW generates error, then interrupt and still continue transfer and > later point of time it generates the transfer done. > We want to stop the transfer once error get detected. For this we need > to reset controller. > I did disabling rx and tx but still controller shows as busy. Oh dear. Well, I guess it'll have to be OK then; we'll just have to find a way of decoupling this API from the mach-tegra directory later:-( -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
On Monday 29 October 2012 08:47 PM, Stephen Warren wrote: On 10/26/2012 12:49 PM, Laxman Dewangan wrote: Why not just always set SLINK_FIFO_ERROR; does it have to be set in the write only if the status was previously asserted? If that is true, how do you avoid a race condition where the bit gets set in SLINK_STATUS after you read it but before you write to clear it? Status gets updated together. There is no steps of updating status. Sorry, I don't understand this answer. The status should be updated once by HW and so there is no race condition. HW behavior is that if the tx or Rx error occurs, it updates the status, generates interrupt and still continue transfer and later on, it generates the ready. In first isr, we read status, error status found and so in isr thread, we reset controller to stop the transfer. So in good state, only ready bit will be set and hence writing 1 to clear it. In error state, clearing error first in ISR and in isr thread resetting the controller to stop the controller engine. Is there a way to support the reset of controller. We will need this functionality. Why do we need to reset the controller at all; can't we simply program all the (few) configuration registers? Are there HW bugs that hang the controller and require a reset or something? HW generates error, then interrupt and still continue transfer and later point of time it generates the transfer done. We want to stop the transfer once error get detected. For this we need to reset controller. I did disabling rx and tx but still controller shows as busy. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
On 10/26/2012 12:49 PM, Laxman Dewangan wrote:
> Thanks Stephen for review.
> I have taken care of almost all feedback. Some of having my below comments.
>
> On Tuesday 23 October 2012 01:32 AM, Stephen Warren wrote:
>> On 10/18/2012 04:47 AM, Laxman Dewangan wrote:
>>> Tegra20/Tegra30 supports the spi interface through its SLINK
>>> controller. Add spi driver for SLINK controller.
>>> + val_write = SLINK_RDY;
>>> + val_write |= (val& SLINK_FIFO_ERROR);
>> Why not just always set SLINK_FIFO_ERROR; does it have to be set in the
>> write only if the status was previously asserted? If that is true, how
>> do you avoid a race condition where the bit gets set in SLINK_STATUS
>> after you read it but before you write to clear it?
>
> Status gets updated together. There is no steps of updating status.
Sorry, I don't understand this answer.
>>> + spin_lock_irqsave(>lock, flags);
>>> + if (err) {
>>> + dev_err(tspi->dev,
>>> + "DmaXfer: ERROR bit set 0x%x\n",
>>> tspi->status_reg);
>>> + dev_err(tspi->dev,
>>> + "DmaXfer 0x%08x:0x%08x:0x%08x\n",
>>> tspi->command_reg,
>>> + tspi->command2_reg,
>>> tspi->dma_control_reg);
>>> + tegra_periph_reset_assert(tspi->clk);
>>> + udelay(2);
>>> + tegra_periph_reset_deassert(tspi->clk);
>> Do we /have/ to reset the SPI block; can't we just disable it in the
>> control register, clear all status, and re-program it from scratch?
>>
>> If at all possible, I would like to avoid introducing any new use of
>> tegra_periph_reset_{,de}assert, since that API has no standard subsystem
>> equivalent (or if it does, isn't hooked into the standard subsystem
>> yet), and hence means this driver relies on a header file currently in
>> arch/arm/mach-tegra/include/mach, and we need to move or delete all such
>> headers in order to support single zImage.
>
> Is there a way to support the reset of controller. We will need this
> functionality.
Why do we need to reset the controller at all; can't we simply program
all the (few) configuration registers? Are there HW bugs that hang the
controller and require a reset or something?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
On 10/26/2012 12:49 PM, Laxman Dewangan wrote:
Thanks Stephen for review.
I have taken care of almost all feedback. Some of having my below comments.
On Tuesday 23 October 2012 01:32 AM, Stephen Warren wrote:
On 10/18/2012 04:47 AM, Laxman Dewangan wrote:
Tegra20/Tegra30 supports the spi interface through its SLINK
controller. Add spi driver for SLINK controller.
+ val_write = SLINK_RDY;
+ val_write |= (val SLINK_FIFO_ERROR);
Why not just always set SLINK_FIFO_ERROR; does it have to be set in the
write only if the status was previously asserted? If that is true, how
do you avoid a race condition where the bit gets set in SLINK_STATUS
after you read it but before you write to clear it?
Status gets updated together. There is no steps of updating status.
Sorry, I don't understand this answer.
+ spin_lock_irqsave(tspi-lock, flags);
+ if (err) {
+ dev_err(tspi-dev,
+ DmaXfer: ERROR bit set 0x%x\n,
tspi-status_reg);
+ dev_err(tspi-dev,
+ DmaXfer 0x%08x:0x%08x:0x%08x\n,
tspi-command_reg,
+ tspi-command2_reg,
tspi-dma_control_reg);
+ tegra_periph_reset_assert(tspi-clk);
+ udelay(2);
+ tegra_periph_reset_deassert(tspi-clk);
Do we /have/ to reset the SPI block; can't we just disable it in the
control register, clear all status, and re-program it from scratch?
If at all possible, I would like to avoid introducing any new use of
tegra_periph_reset_{,de}assert, since that API has no standard subsystem
equivalent (or if it does, isn't hooked into the standard subsystem
yet), and hence means this driver relies on a header file currently in
arch/arm/mach-tegra/include/mach, and we need to move or delete all such
headers in order to support single zImage.
Is there a way to support the reset of controller. We will need this
functionality.
Why do we need to reset the controller at all; can't we simply program
all the (few) configuration registers? Are there HW bugs that hang the
controller and require a reset or something?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
On Monday 29 October 2012 08:47 PM, Stephen Warren wrote: On 10/26/2012 12:49 PM, Laxman Dewangan wrote: Why not just always set SLINK_FIFO_ERROR; does it have to be set in the write only if the status was previously asserted? If that is true, how do you avoid a race condition where the bit gets set in SLINK_STATUS after you read it but before you write to clear it? Status gets updated together. There is no steps of updating status. Sorry, I don't understand this answer. The status should be updated once by HW and so there is no race condition. HW behavior is that if the tx or Rx error occurs, it updates the status, generates interrupt and still continue transfer and later on, it generates the ready. In first isr, we read status, error status found and so in isr thread, we reset controller to stop the transfer. So in good state, only ready bit will be set and hence writing 1 to clear it. In error state, clearing error first in ISR and in isr thread resetting the controller to stop the controller engine. Is there a way to support the reset of controller. We will need this functionality. Why do we need to reset the controller at all; can't we simply program all the (few) configuration registers? Are there HW bugs that hang the controller and require a reset or something? HW generates error, then interrupt and still continue transfer and later point of time it generates the transfer done. We want to stop the transfer once error get detected. For this we need to reset controller. I did disabling rx and tx but still controller shows as busy. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
On 10/29/2012 10:18 AM, Laxman Dewangan wrote: On Monday 29 October 2012 08:47 PM, Stephen Warren wrote: On 10/26/2012 12:49 PM, Laxman Dewangan wrote: Why not just always set SLINK_FIFO_ERROR; does it have to be set in the write only if the status was previously asserted? If that is true, how do you avoid a race condition where the bit gets set in SLINK_STATUS after you read it but before you write to clear it? Status gets updated together. There is no steps of updating status. Sorry, I don't understand this answer. The status should be updated once by HW and so there is no race condition. HW behavior is that if the tx or Rx error occurs, it updates the status, generates interrupt and still continue transfer and later on, it generates the ready. In first isr, we read status, error status found and so in isr thread, we reset controller to stop the transfer. So in good state, only ready bit will be set and hence writing 1 to clear it. In error state, clearing error first in ISR and in isr thread resetting the controller to stop the controller engine. OK, I see why there's no race. It still seems simply to me if tegra_slink_clear_status() just always writes all the status bits, but I suppose it isn't a correctness issue. Is there a way to support the reset of controller. We will need this functionality. Why do we need to reset the controller at all; can't we simply program all the (few) configuration registers? Are there HW bugs that hang the controller and require a reset or something? HW generates error, then interrupt and still continue transfer and later point of time it generates the transfer done. We want to stop the transfer once error get detected. For this we need to reset controller. I did disabling rx and tx but still controller shows as busy. Oh dear. Well, I guess it'll have to be OK then; we'll just have to find a way of decoupling this API from the mach-tegra directory later:-( -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
Thanks Stephen for review.
I have taken care of almost all feedback. Some of having my below comments.
On Tuesday 23 October 2012 01:32 AM, Stephen Warren wrote:
On 10/18/2012 04:47 AM, Laxman Dewangan wrote:
Tegra20/Tegra30 supports the spi interface through its SLINK
controller. Add spi driver for SLINK controller.
+static inline void tegra_slink_writel(struct tegra_slink_data *tspi,
+ unsigned long val, unsigned long reg)
+{
+ writel(val, tspi->base + reg);
+
+ /* Read back register to make sure that register writes completed */
+ if (reg != SLINK_TX_FIFO)
+ readl(tspi->base + SLINK_MAS_DATA);
Is that really necessary on every single write, or only for certain
specific operations? This seems rather heavy-weight.
We do write register very less (as we shadow the register locally), I
do not see any perf degradation here.
+ val_write = SLINK_RDY;
+ val_write |= (val& SLINK_FIFO_ERROR);
Why not just always set SLINK_FIFO_ERROR; does it have to be set in the
write only if the status was previously asserted? If that is true, how
do you avoid a race condition where the bit gets set in SLINK_STATUS
after you read it but before you write to clear it?
Status gets updated together. There is no steps of updating status.
+ if (bits_per_word == 8 || bits_per_word == 16) {
+ tspi->is_packed = 1;
+ tspi->words_per_32bit = 32/bits_per_word;
Spaces required around all operators. Does this pass checkpatch? No:
total: 1405 errors, 11 warnings, 1418 lines checked
I saw the issue by my checkpatch is not caching the issue. Let me
recheck my command.
+ bits_per_word = t->bits_per_word ? t->bits_per_word :
+ tspi->cur_spi->bits_per_word;
That logic is repeated a few times. Shouldn't there be a macro to avoid
cut/paste. Perhaps even in the SPI core rather than this driver.
I will post the change for moving the code to core later. I will keep
this for now.
+ struct tegra_slink_data *tspi, struct spi_transfer *t)
Are there no cases where we can simply DMA straight into the client
buffer? I suppose it would be limited to cache-line-aligned
cache-line-size-aligned buffers which is probably unlikely in general:-(
I think it is possible by checking some flags, I will explore and will
post the fix later.
+static int tegra_slink_unprepare_transfer(struct spi_master *master)
+{
+ struct tegra_slink_data *tspi = spi_master_get_devdata(master);
+
+ pm_runtime_put_sync(tspi->dev);
+ return 0;
+}
Does this driver actually implement any runtime PM ops? I certainly
don't see any in struct dev_pm_ops tegra_slink_dev_pm_ops. Related, why
do tegra_slink_clk_{en,dis}able exist; shouldn't those functions be the
implementation of the runtime PM ops? Related, why are
tegra_slink_clk_{en,dis}able called all over the place, rather than
relying on runtime PM API calls, or tegra_slink_{,un}prepare_transfer
having been called?
OK, I move the clock control in runtime callbacks.
So that all implies that we wait for the very first interrupt from the
SPI peripheral for a transfer, and then wait for the DMA controller to
complete all DMA (which would probably entail actually waiting for DMA
to drain the RX FIFO in the RX case). Does it make sense to simply
return from the ISR if not all conditions that we will wait on have
occurred, and so avoid blocking this ISR thread? I suppose since this
thread gets blocked rather than spinning, it's not a big deal though.
In this case, we can get rid of isr thread and move the wait/status
check to caller thread.
I need to do some more stress on this and if it is fine then will post
the patch later for this, not as part of this patch.
+ spin_lock_irqsave(>lock, flags);
+ if (err) {
+ dev_err(tspi->dev,
+ "DmaXfer: ERROR bit set 0x%x\n", tspi->status_reg);
+ dev_err(tspi->dev,
+ "DmaXfer 0x%08x:0x%08x:0x%08x\n", tspi->command_reg,
+ tspi->command2_reg, tspi->dma_control_reg);
+ tegra_periph_reset_assert(tspi->clk);
+ udelay(2);
+ tegra_periph_reset_deassert(tspi->clk);
Do we /have/ to reset the SPI block; can't we just disable it in the
control register, clear all status, and re-program it from scratch?
If at all possible, I would like to avoid introducing any new use of
tegra_periph_reset_{,de}assert, since that API has no standard subsystem
equivalent (or if it does, isn't hooked into the standard subsystem
yet), and hence means this driver relies on a header file currently in
arch/arm/mach-tegra/include/mach, and we need to move or delete all such
headers in order to support single zImage.
Is there a way to support the reset of controller. We will need this
functionality.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
Thanks Stephen for review.
I have taken care of almost all feedback. Some of having my below comments.
On Tuesday 23 October 2012 01:32 AM, Stephen Warren wrote:
On 10/18/2012 04:47 AM, Laxman Dewangan wrote:
Tegra20/Tegra30 supports the spi interface through its SLINK
controller. Add spi driver for SLINK controller.
+static inline void tegra_slink_writel(struct tegra_slink_data *tspi,
+ unsigned long val, unsigned long reg)
+{
+ writel(val, tspi-base + reg);
+
+ /* Read back register to make sure that register writes completed */
+ if (reg != SLINK_TX_FIFO)
+ readl(tspi-base + SLINK_MAS_DATA);
Is that really necessary on every single write, or only for certain
specific operations? This seems rather heavy-weight.
We do write register very less (as we shadow the register locally), I
do not see any perf degradation here.
+ val_write = SLINK_RDY;
+ val_write |= (val SLINK_FIFO_ERROR);
Why not just always set SLINK_FIFO_ERROR; does it have to be set in the
write only if the status was previously asserted? If that is true, how
do you avoid a race condition where the bit gets set in SLINK_STATUS
after you read it but before you write to clear it?
Status gets updated together. There is no steps of updating status.
+ if (bits_per_word == 8 || bits_per_word == 16) {
+ tspi-is_packed = 1;
+ tspi-words_per_32bit = 32/bits_per_word;
Spaces required around all operators. Does this pass checkpatch? No:
total: 1405 errors, 11 warnings, 1418 lines checked
I saw the issue by my checkpatch is not caching the issue. Let me
recheck my command.
+ bits_per_word = t-bits_per_word ? t-bits_per_word :
+ tspi-cur_spi-bits_per_word;
That logic is repeated a few times. Shouldn't there be a macro to avoid
cut/paste. Perhaps even in the SPI core rather than this driver.
I will post the change for moving the code to core later. I will keep
this for now.
+ struct tegra_slink_data *tspi, struct spi_transfer *t)
Are there no cases where we can simply DMA straight into the client
buffer? I suppose it would be limited to cache-line-aligned
cache-line-size-aligned buffers which is probably unlikely in general:-(
I think it is possible by checking some flags, I will explore and will
post the fix later.
+static int tegra_slink_unprepare_transfer(struct spi_master *master)
+{
+ struct tegra_slink_data *tspi = spi_master_get_devdata(master);
+
+ pm_runtime_put_sync(tspi-dev);
+ return 0;
+}
Does this driver actually implement any runtime PM ops? I certainly
don't see any in struct dev_pm_ops tegra_slink_dev_pm_ops. Related, why
do tegra_slink_clk_{en,dis}able exist; shouldn't those functions be the
implementation of the runtime PM ops? Related, why are
tegra_slink_clk_{en,dis}able called all over the place, rather than
relying on runtime PM API calls, or tegra_slink_{,un}prepare_transfer
having been called?
OK, I move the clock control in runtime callbacks.
So that all implies that we wait for the very first interrupt from the
SPI peripheral for a transfer, and then wait for the DMA controller to
complete all DMA (which would probably entail actually waiting for DMA
to drain the RX FIFO in the RX case). Does it make sense to simply
return from the ISR if not all conditions that we will wait on have
occurred, and so avoid blocking this ISR thread? I suppose since this
thread gets blocked rather than spinning, it's not a big deal though.
In this case, we can get rid of isr thread and move the wait/status
check to caller thread.
I need to do some more stress on this and if it is fine then will post
the patch later for this, not as part of this patch.
+ spin_lock_irqsave(tspi-lock, flags);
+ if (err) {
+ dev_err(tspi-dev,
+ DmaXfer: ERROR bit set 0x%x\n, tspi-status_reg);
+ dev_err(tspi-dev,
+ DmaXfer 0x%08x:0x%08x:0x%08x\n, tspi-command_reg,
+ tspi-command2_reg, tspi-dma_control_reg);
+ tegra_periph_reset_assert(tspi-clk);
+ udelay(2);
+ tegra_periph_reset_deassert(tspi-clk);
Do we /have/ to reset the SPI block; can't we just disable it in the
control register, clear all status, and re-program it from scratch?
If at all possible, I would like to avoid introducing any new use of
tegra_periph_reset_{,de}assert, since that API has no standard subsystem
equivalent (or if it does, isn't hooked into the standard subsystem
yet), and hence means this driver relies on a header file currently in
arch/arm/mach-tegra/include/mach, and we need to move or delete all such
headers in order to support single zImage.
Is there a way to support the reset of controller. We will need this
functionality.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
On Mon, Oct 22, 2012 at 02:02:47PM -0600, Stephen Warren wrote: > On 10/18/2012 04:47 AM, Laxman Dewangan wrote: > > + /* Read back register to make sure that register writes completed */ > > + if (reg != SLINK_TX_FIFO) > > + readl(tspi->base + SLINK_MAS_DATA); > Is that really necessary on every single write, or only for certain > specific operations? This seems rather heavy-weight. I saw that myself - I figured that since SPI is (relatively) slow itself the simplicity gained from doing the flush unconditionally was reasonable, further optimisation can always be done later. > > + val = tegra_slink_readl(tspi, SLINK_STATUS); > > + /* Write 1 to clear status register */ > > + val_write = SLINK_RDY; > > + val_write |= (val & SLINK_FIFO_ERROR); > Why not just always set SLINK_FIFO_ERROR; does it have to be set in the > write only if the status was previously asserted? If that is true, how > do you avoid a race condition where the bit gets set in SLINK_STATUS > after you read it but before you write to clear it? The whole point with this sort of interrupt scheme is usually that the interrupt is level triggered and will just continue to be asserted if some source within the interrupt isn't acked; the race is usually the other way around where you may ack a status you didn't see. > > +static unsigned int tegra_slink_read_rx_fifo_to_client_rxbuf( > > > + bits_per_word = t->bits_per_word ? t->bits_per_word : > > + tspi->cur_spi->bits_per_word; > That logic is repeated a few times. Shouldn't there be a macro to avoid > cut/paste. Perhaps even in the SPI core rather than this driver. Yes, should be in the SPI core as this pattern exists for all SPI drivers. > Doesn't this function need to parse all the other standardized > properties from spi-bus.txt, or does the SPI core handle that? No, it doesn't. But perhaps it should (see the thing above about the per-transfer max frequency too...). In general SPI hasn't done much about factoring code out of drivers until very recently. signature.asc Description: Digital signature
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
On Mon, Oct 22, 2012 at 02:02:47PM -0600, Stephen Warren wrote: On 10/18/2012 04:47 AM, Laxman Dewangan wrote: + /* Read back register to make sure that register writes completed */ + if (reg != SLINK_TX_FIFO) + readl(tspi-base + SLINK_MAS_DATA); Is that really necessary on every single write, or only for certain specific operations? This seems rather heavy-weight. I saw that myself - I figured that since SPI is (relatively) slow itself the simplicity gained from doing the flush unconditionally was reasonable, further optimisation can always be done later. + val = tegra_slink_readl(tspi, SLINK_STATUS); + /* Write 1 to clear status register */ + val_write = SLINK_RDY; + val_write |= (val SLINK_FIFO_ERROR); Why not just always set SLINK_FIFO_ERROR; does it have to be set in the write only if the status was previously asserted? If that is true, how do you avoid a race condition where the bit gets set in SLINK_STATUS after you read it but before you write to clear it? The whole point with this sort of interrupt scheme is usually that the interrupt is level triggered and will just continue to be asserted if some source within the interrupt isn't acked; the race is usually the other way around where you may ack a status you didn't see. +static unsigned int tegra_slink_read_rx_fifo_to_client_rxbuf( + bits_per_word = t-bits_per_word ? t-bits_per_word : + tspi-cur_spi-bits_per_word; That logic is repeated a few times. Shouldn't there be a macro to avoid cut/paste. Perhaps even in the SPI core rather than this driver. Yes, should be in the SPI core as this pattern exists for all SPI drivers. Doesn't this function need to parse all the other standardized properties from spi-bus.txt, or does the SPI core handle that? No, it doesn't. But perhaps it should (see the thing above about the per-transfer max frequency too...). In general SPI hasn't done much about factoring code out of drivers until very recently. signature.asc Description: Digital signature
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
On 10/18/2012 04:47 AM, Laxman Dewangan wrote:
> Tegra20/Tegra30 supports the spi interface through its SLINK
> controller. Add spi driver for SLINK controller.
> diff --git a/drivers/spi/spi-tegra20-slink.c b/drivers/spi/spi-tegra20-slink.c
> +static inline void tegra_slink_writel(struct tegra_slink_data *tspi,
> + unsigned long val, unsigned long reg)
> +{
> + writel(val, tspi->base + reg);
> +
> + /* Read back register to make sure that register writes completed */
> + if (reg != SLINK_TX_FIFO)
> + readl(tspi->base + SLINK_MAS_DATA);
Is that really necessary on every single write, or only for certain
specific operations? This seems rather heavy-weight.
> +static void tegra_slink_clear_status(struct tegra_slink_data *tspi)
> +{
> + unsigned long val;
> + unsigned long val_write = 0;
> +
> + val = tegra_slink_readl(tspi, SLINK_STATUS);
> +
> + /* Write 1 to clear status register */
> + val_write = SLINK_RDY;
> + val_write |= (val & SLINK_FIFO_ERROR);
Why not just always set SLINK_FIFO_ERROR; does it have to be set in the
write only if the status was previously asserted? If that is true, how
do you avoid a race condition where the bit gets set in SLINK_STATUS
after you read it but before you write to clear it?
> + tegra_slink_writel(tspi, val_write, SLINK_STATUS);
> +}
> +static unsigned tegra_slink_calculate_curr_xfer_param(
> + if (bits_per_word == 8 || bits_per_word == 16) {
> + tspi->is_packed = 1;
> + tspi->words_per_32bit = 32/bits_per_word;
Spaces required around all operators. Does this pass checkpatch? No:
total: 1405 errors, 11 warnings, 1418 lines checked
> +static unsigned tegra_slink_fill_tx_fifo_from_client_txbuf(
> + if (tspi->is_packed) {
> + nbytes = tspi->curr_dma_words * tspi->bytes_per_word;
> + max_n_32bit = (min(nbytes, tx_empty_count*4) - 1)/4 + 1;
> + for (count = 0; count < max_n_32bit; ++count) {
Very minor almost irrelevant nit: count++ would be much more typical here.
> + x = 0;
> + for (i = 0; (i < 4) && nbytes; i++, nbytes--)
> + x |= (*tx_buf++) << (i*8);
> + tegra_slink_writel(tspi, x, SLINK_TX_FIFO);
> + }
> + written_words = min(max_n_32bit * tspi->words_per_32bit,
> + tspi->curr_dma_words);
The calculations here seem a little over-complex. Why not calculate the
FIFO space in words above, and just set written words based on that.
Something like:
fifo_words_left = tx_empty_count * spi->words_per_32bit;
written_words = min(fifo_words_left, tspi->curr_dma_words);
nbytes = written_words * spi->bytes_per_word;
max_n_32bit = (nbytes + 3) / 4;
Most likely a similar comment applies to all the other similar functions
for filling FIFOs and buffers.
In fact, I suspect you can completely get rid of the if (is_packed)
statement here by appropriately parameterising the code using variables
in *spi...
> +static unsigned int tegra_slink_read_rx_fifo_to_client_rxbuf(
> + bits_per_word = t->bits_per_word ? t->bits_per_word :
> + tspi->cur_spi->bits_per_word;
That logic is repeated a few times. Shouldn't there be a macro to avoid
cut/paste. Perhaps even in the SPI core rather than this driver.
> + rx_mask = (1 << bits_per_word) - 1;
> + for (count = 0; count < rx_full_count; ++count) {
> + x = tegra_slink_readl(tspi, SLINK_RX_FIFO);
> + x &= rx_mask;
I don't think you need that mask; the loop below only extracts bytes
that actually exist in the FIFO right?
> + for (i = 0; (i < tspi->bytes_per_word); ++i)
> + *rx_buf++ = (x >> (i*8)) & 0xFF;
> +static void tegra_slink_copy_client_txbuf_to_spi_txbuf(
> + struct tegra_slink_data *tspi, struct spi_transfer *t)
Are there no cases where we can simply DMA straight into the client
buffer? I suppose it would be limited to cache-line-aligned
cache-line-size-aligned buffers which is probably unlikely in general:-(
> +static int tegra_slink_start_dma_based_transfer(
> + struct tegra_slink_data *tspi, struct spi_transfer *t)
> + /* Make sure that Rx and Tx fifo are empty */
> + status = tegra_slink_readl(tspi, SLINK_STATUS);
> + if ((status & SLINK_FIFO_EMPTY) != SLINK_FIFO_EMPTY)
> + dev_err(tspi->dev,
> + "Rx/Tx fifo are not empty status 0x%08lx\n", status);
Shouldn't this return an error, or drain the FIFO, or something?
> +static int tegra_slink_init_dma_param(struct tegra_slink_data *tspi,
> + if (dma_to_memory) {
> + dma_sconfig.src_addr = tspi->phys + SLINK_RX_FIFO;
> + dma_sconfig.dst_addr = tspi->phys + SLINK_RX_FIFO;
> + } else {
> + dma_sconfig.src_addr =
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
On Mon, Oct 22, 2012 at 01:28:26PM +0100, Mark Brown wrote:
> On Thu, Oct 18, 2012 at 04:17:13PM +0530, Laxman Dewangan wrote:
[...]
> > +static struct tegra_spi_platform_data *tegra_slink_parese_dt(
> > + struct platform_device *pdev)
> > +{
>
> There doens't seem to be any binding documentation; binding
> documentation is mandatory.
Also this should probably be renamed to tegra_slink_parse_dt().
Thierry
pgpi2aMwvDxYM.pgp
Description: PGP signature
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
On Thu, Oct 18, 2012 at 04:17:13PM +0530, Laxman Dewangan wrote:
> + udelay(1);
> + wmb();
> + }
> + tspi->dma_control_reg = val;
> + val |= SLINK_DMA_EN;
> + tegra_slink_writel(tspi, val, SLINK_DMA_CTL);
> + return 0;
> +}
> +static int tegra_slink_init_dma_param(struct tegra_slink_data *tspi,
> + bool dma_to_memory)
> +{
Blank line between functions.
> + tegra_slink_clk_disable(tspi);
> + pm_runtime_put_sync(tspi->dev);
> + return 0;
Throughout the code you're using pm_runtime_put_sync() - why does this
need to be _sync()? Can't we just use a regular put()? If we can't we
should document why.
> +static int tegra_slink_prepare_transfer(struct spi_master *master)
> +{
> + struct tegra_slink_data *tspi = spi_master_get_devdata(master);
> +
> + pm_runtime_get_sync(tspi->dev);
> + return 0;
Should really check the error code of the pm_runtime call here.
> +static struct tegra_spi_platform_data *tegra_slink_parese_dt(
> + struct platform_device *pdev)
> +{
There doens't seem to be any binding documentation; binding
documentation is mandatory.
> + prop = of_get_property(pdev->dev.of_node, "spi_max_frequency", NULL);
> + if (prop)
> + pdata->spi_max_frequency = be32_to_cpup(prop);
> + else
> + pdata->spi_max_frequency = 2500; /* 25MHz */
Why is the default not being picked in the general non-OF case?
> +const struct tegra_slink_chip_data tegra30_spi_cdata = {
> + .cs_hold_time = true,
> +};
> +
> +#ifdef CONFIG_OF
> +const struct tegra_slink_chip_data tegra20_spi_cdata = {
> + .cs_hold_time = false,
> +};
> +
> +static struct of_device_id tegra_slink_of_match[] __devinitconst = {
> + { .compatible = "nvidia,tegra20-slink", .data = _spi_cdata, },
> + { .compatible = "nvidia,tegra30-slink", .data = _spi_cdata, },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, tegra_slink_of_match);
> +#endif
The ifdefs here look misplaced?
> + spi_irq = platform_get_irq(pdev, 0);
> + if (unlikely(spi_irq < 0)) {
Putting optimisation hints like unlikely() here is pointless.
> + ret = devm_request_threaded_irq(>dev, tspi->irq,
> + tegra_slink_isr, tegra_slink_isr_thread, IRQF_ONESHOT,
> + dev_name(>dev), tspi);
> + if (ret < 0) {
> + dev_err(>dev, "Failed to register ISR for IRQ %d\n",
> + tspi->irq);
> + goto exit_free_master;
> + }
Any use of devm_ IRQ functions is suspicous... why is this safe against
an interrupt firing after the SPI device has started to deallocate?
> +#ifdef CONFIG_PM_SLEEP
> +static int tegra_slink_suspend(struct device *dev)
> +{
> + struct spi_master *master = dev_get_drvdata(dev);
> +
> + spi_master_suspend(master);
> + return 0;
Should use the return code of spi_master_suspend().
signature.asc
Description: Digital signature
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
On Thu, Oct 18, 2012 at 04:17:13PM +0530, Laxman Dewangan wrote:
+ udelay(1);
+ wmb();
+ }
+ tspi-dma_control_reg = val;
+ val |= SLINK_DMA_EN;
+ tegra_slink_writel(tspi, val, SLINK_DMA_CTL);
+ return 0;
+}
+static int tegra_slink_init_dma_param(struct tegra_slink_data *tspi,
+ bool dma_to_memory)
+{
Blank line between functions.
+ tegra_slink_clk_disable(tspi);
+ pm_runtime_put_sync(tspi-dev);
+ return 0;
Throughout the code you're using pm_runtime_put_sync() - why does this
need to be _sync()? Can't we just use a regular put()? If we can't we
should document why.
+static int tegra_slink_prepare_transfer(struct spi_master *master)
+{
+ struct tegra_slink_data *tspi = spi_master_get_devdata(master);
+
+ pm_runtime_get_sync(tspi-dev);
+ return 0;
Should really check the error code of the pm_runtime call here.
+static struct tegra_spi_platform_data *tegra_slink_parese_dt(
+ struct platform_device *pdev)
+{
There doens't seem to be any binding documentation; binding
documentation is mandatory.
+ prop = of_get_property(pdev-dev.of_node, spi_max_frequency, NULL);
+ if (prop)
+ pdata-spi_max_frequency = be32_to_cpup(prop);
+ else
+ pdata-spi_max_frequency = 2500; /* 25MHz */
Why is the default not being picked in the general non-OF case?
+const struct tegra_slink_chip_data tegra30_spi_cdata = {
+ .cs_hold_time = true,
+};
+
+#ifdef CONFIG_OF
+const struct tegra_slink_chip_data tegra20_spi_cdata = {
+ .cs_hold_time = false,
+};
+
+static struct of_device_id tegra_slink_of_match[] __devinitconst = {
+ { .compatible = nvidia,tegra20-slink, .data = tegra20_spi_cdata, },
+ { .compatible = nvidia,tegra30-slink, .data = tegra30_spi_cdata, },
+ {}
+};
+MODULE_DEVICE_TABLE(of, tegra_slink_of_match);
+#endif
The ifdefs here look misplaced?
+ spi_irq = platform_get_irq(pdev, 0);
+ if (unlikely(spi_irq 0)) {
Putting optimisation hints like unlikely() here is pointless.
+ ret = devm_request_threaded_irq(pdev-dev, tspi-irq,
+ tegra_slink_isr, tegra_slink_isr_thread, IRQF_ONESHOT,
+ dev_name(pdev-dev), tspi);
+ if (ret 0) {
+ dev_err(pdev-dev, Failed to register ISR for IRQ %d\n,
+ tspi-irq);
+ goto exit_free_master;
+ }
Any use of devm_ IRQ functions is suspicous... why is this safe against
an interrupt firing after the SPI device has started to deallocate?
+#ifdef CONFIG_PM_SLEEP
+static int tegra_slink_suspend(struct device *dev)
+{
+ struct spi_master *master = dev_get_drvdata(dev);
+
+ spi_master_suspend(master);
+ return 0;
Should use the return code of spi_master_suspend().
signature.asc
Description: Digital signature
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
On Mon, Oct 22, 2012 at 01:28:26PM +0100, Mark Brown wrote:
On Thu, Oct 18, 2012 at 04:17:13PM +0530, Laxman Dewangan wrote:
[...]
+static struct tegra_spi_platform_data *tegra_slink_parese_dt(
+ struct platform_device *pdev)
+{
There doens't seem to be any binding documentation; binding
documentation is mandatory.
Also this should probably be renamed to tegra_slink_parse_dt().
Thierry
pgpi2aMwvDxYM.pgp
Description: PGP signature
Re: [PATCH] spi: tegra: add spi driver for SLINK controller
On 10/18/2012 04:47 AM, Laxman Dewangan wrote:
Tegra20/Tegra30 supports the spi interface through its SLINK
controller. Add spi driver for SLINK controller.
diff --git a/drivers/spi/spi-tegra20-slink.c b/drivers/spi/spi-tegra20-slink.c
+static inline void tegra_slink_writel(struct tegra_slink_data *tspi,
+ unsigned long val, unsigned long reg)
+{
+ writel(val, tspi-base + reg);
+
+ /* Read back register to make sure that register writes completed */
+ if (reg != SLINK_TX_FIFO)
+ readl(tspi-base + SLINK_MAS_DATA);
Is that really necessary on every single write, or only for certain
specific operations? This seems rather heavy-weight.
+static void tegra_slink_clear_status(struct tegra_slink_data *tspi)
+{
+ unsigned long val;
+ unsigned long val_write = 0;
+
+ val = tegra_slink_readl(tspi, SLINK_STATUS);
+
+ /* Write 1 to clear status register */
+ val_write = SLINK_RDY;
+ val_write |= (val SLINK_FIFO_ERROR);
Why not just always set SLINK_FIFO_ERROR; does it have to be set in the
write only if the status was previously asserted? If that is true, how
do you avoid a race condition where the bit gets set in SLINK_STATUS
after you read it but before you write to clear it?
+ tegra_slink_writel(tspi, val_write, SLINK_STATUS);
+}
+static unsigned tegra_slink_calculate_curr_xfer_param(
+ if (bits_per_word == 8 || bits_per_word == 16) {
+ tspi-is_packed = 1;
+ tspi-words_per_32bit = 32/bits_per_word;
Spaces required around all operators. Does this pass checkpatch? No:
total: 1405 errors, 11 warnings, 1418 lines checked
+static unsigned tegra_slink_fill_tx_fifo_from_client_txbuf(
+ if (tspi-is_packed) {
+ nbytes = tspi-curr_dma_words * tspi-bytes_per_word;
+ max_n_32bit = (min(nbytes, tx_empty_count*4) - 1)/4 + 1;
+ for (count = 0; count max_n_32bit; ++count) {
Very minor almost irrelevant nit: count++ would be much more typical here.
+ x = 0;
+ for (i = 0; (i 4) nbytes; i++, nbytes--)
+ x |= (*tx_buf++) (i*8);
+ tegra_slink_writel(tspi, x, SLINK_TX_FIFO);
+ }
+ written_words = min(max_n_32bit * tspi-words_per_32bit,
+ tspi-curr_dma_words);
The calculations here seem a little over-complex. Why not calculate the
FIFO space in words above, and just set written words based on that.
Something like:
fifo_words_left = tx_empty_count * spi-words_per_32bit;
written_words = min(fifo_words_left, tspi-curr_dma_words);
nbytes = written_words * spi-bytes_per_word;
max_n_32bit = (nbytes + 3) / 4;
Most likely a similar comment applies to all the other similar functions
for filling FIFOs and buffers.
In fact, I suspect you can completely get rid of the if (is_packed)
statement here by appropriately parameterising the code using variables
in *spi...
+static unsigned int tegra_slink_read_rx_fifo_to_client_rxbuf(
+ bits_per_word = t-bits_per_word ? t-bits_per_word :
+ tspi-cur_spi-bits_per_word;
That logic is repeated a few times. Shouldn't there be a macro to avoid
cut/paste. Perhaps even in the SPI core rather than this driver.
+ rx_mask = (1 bits_per_word) - 1;
+ for (count = 0; count rx_full_count; ++count) {
+ x = tegra_slink_readl(tspi, SLINK_RX_FIFO);
+ x = rx_mask;
I don't think you need that mask; the loop below only extracts bytes
that actually exist in the FIFO right?
+ for (i = 0; (i tspi-bytes_per_word); ++i)
+ *rx_buf++ = (x (i*8)) 0xFF;
+static void tegra_slink_copy_client_txbuf_to_spi_txbuf(
+ struct tegra_slink_data *tspi, struct spi_transfer *t)
Are there no cases where we can simply DMA straight into the client
buffer? I suppose it would be limited to cache-line-aligned
cache-line-size-aligned buffers which is probably unlikely in general:-(
+static int tegra_slink_start_dma_based_transfer(
+ struct tegra_slink_data *tspi, struct spi_transfer *t)
+ /* Make sure that Rx and Tx fifo are empty */
+ status = tegra_slink_readl(tspi, SLINK_STATUS);
+ if ((status SLINK_FIFO_EMPTY) != SLINK_FIFO_EMPTY)
+ dev_err(tspi-dev,
+ Rx/Tx fifo are not empty status 0x%08lx\n, status);
Shouldn't this return an error, or drain the FIFO, or something?
+static int tegra_slink_init_dma_param(struct tegra_slink_data *tspi,
+ if (dma_to_memory) {
+ dma_sconfig.src_addr = tspi-phys + SLINK_RX_FIFO;
+ dma_sconfig.dst_addr = tspi-phys + SLINK_RX_FIFO;
+ } else {
+ dma_sconfig.src_addr = tspi-phys + SLINK_TX_FIFO;
+ dma_sconfig.dst_addr = tspi-phys + SLINK_TX_FIFO;
+ }
