On Tue, Jun 25, 2013 at 01:22:20PM +0200, Tomasz Figa wrote:
> On Tuesday 25 of June 2013 11:28:18 Mark Brown wrote:

> > On my non-DT s3c64xx system I get this when trying to use SPI for the
> > first time:

> Could you clarify first time? Does it work on further retries?

No, the system appears to lock up fairly solidly - I suspect poor error
handling in the SPI driver.

> > [ 1.739385] s3c64xx-spi s3c6410-spi.0: Failed to get RX DMA channel
> > [ 1.743437] spi_master spi0: failed to prepare transfer hardware

> > though I haven't yet tried your clock patches (there were some conficts
> > that I didn't get round to picking up yet).

> This shouldn't really matter, as long as the pl080 drivers probes
> correctly.

Indeed, I was wondering if it was failing to probe due to not getting
its clocks though since that's another area you've been actively working
on.

Attachments:

(No filename) (879.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-06-25 15:44:12

by Tomasz Figa

[permalink] [raw]

Subject: Re: [RFC PATCH v2 00/12] ARM: s3c64xx: Let amba-pl08x driver handle DMA

On Tuesday 25 of June 2013 16:38:43 Mark Brown wrote:
> On Tue, Jun 25, 2013 at 01:22:20PM +0200, Tomasz Figa wrote:
> > On Tuesday 25 of June 2013 11:28:18 Mark Brown wrote:
> > > On my non-DT s3c64xx system I get this when trying to use SPI for the
> >
> > > first time:
> > Could you clarify first time? Does it work on further retries?
>
> No, the system appears to lock up fairly solidly - I suspect poor error
> handling in the SPI driver.
>
> > > [ 1.739385] s3c64xx-spi s3c6410-spi.0: Failed to get RX DMA
> > > channel
> > > [ 1.743437] spi_master spi0: failed to prepare transfer hardware
> > >
> > > though I haven't yet tried your clock patches (there were some
> > > conficts
> > > that I didn't get round to picking up yet).
> >
> > This shouldn't really matter, as long as the pl080 drivers probes
> > correctly.
>
> Indeed, I was wondering if it was failing to probe due to not getting
> its clocks though since that's another area you've been actively working
> on.

Let me check if I can get something with SPI interface that I would be able
to connect to my board at home and do some testing.

Best regards,
Tomasz

2013-06-25 15:57:02

by Vinod Koul

[permalink] [raw]

Subject: Re: [RFC PATCH v2 01/12] dmaengine: PL08x: Refactor pl08x_getbytes_chan() to lower indentation

On Sat, Jun 22, 2013 at 10:42:33PM +0200, Tomasz Figa wrote:
> Further patch will introduce support for PL080S, which requires some
> things to be done conditionally, thus increasing indentation level of
> some functions even more.
>
> This patch reduces indentation level of pl08x_getbytes_chan() function
> by inverting several conditions and returning from function wherever
> possible.
>
> Signed-off-by: Tomasz Figa <[email protected]>
> ---
> drivers/dma/amba-pl08x.c | 53 ++++++++++++++++++++++++++----------------------
> 1 file changed, 29 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index 06fe45c..6a12392 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -469,47 +469,52 @@ static inline u32 get_bytes_in_cctl(u32 cctl)
> /* The channel should be paused when calling this */
> static u32 pl08x_getbytes_chan(struct pl08x_dma_chan *plchan)
> {
> + struct pl08x_lli *llis_va;
> struct pl08x_phy_chan *ch;
> + dma_addr_t llis_bus;
> struct pl08x_txd *txd;
> - size_t bytes = 0;
> + size_t bytes;
> + int index;
> + u32 clli;
>
> ch = plchan->phychan;
> txd = plchan->at;
>
> + if (!ch || !txd)
> + return 0;
shouldnt this be err return

> +
> /*
> * Follow the LLIs to get the number of remaining
> * bytes in the currently active transaction.
> */
> - if (ch && txd) {
> - u32 clli = readl(ch->base + PL080_CH_LLI) & ~PL080_LLI_LM_AHB2;
> + clli = readl(ch->base + PL080_CH_LLI) & ~PL080_LLI_LM_AHB2;
>
> - /* First get the remaining bytes in the active transfer */
> - bytes = get_bytes_in_cctl(readl(ch->base + PL080_CH_CONTROL));
> + /* First get the remaining bytes in the active transfer */
> + bytes = get_bytes_in_cctl(readl(ch->base + PL080_CH_CONTROL));
>
> - if (clli) {
> - struct pl08x_lli *llis_va = txd->llis_va;
> - dma_addr_t llis_bus = txd->llis_bus;
> - int index;
> + if (!clli)
> + return bytes;
>
> - BUG_ON(clli < llis_bus || clli >= llis_bus +
> + llis_va = txd->llis_va;
> + llis_bus = txd->llis_bus;
> +
> + BUG_ON(clli < llis_bus || clli >= llis_bus +
> sizeof(struct pl08x_lli) * MAX_NUM_TSFR_LLIS);
IMO BUG_ON is too much for this. Perhaps returning error and logging error would
be okay

--
~Vinod

2013-06-25 16:15:10

by Vinod Koul

[permalink] [raw]

Subject: Re: [RFC PATCH v2 02/12] dmaengine: PL08x: Add support for different offset of CONFIG register

On Sat, Jun 22, 2013 at 10:42:34PM +0200, Tomasz Figa wrote:
> Some variants of PL08x (namely PL080S, found in Samsung S3C64xx SoCs)
> have CONFIG register at different offset. This patch makes the driver
> use offset from vendor data struct.
>
> Signed-off-by: Tomasz Figa <[email protected]>
Acked-by: Vinod Koul <[email protected]>

--
~Vinod

2013-06-25 16:50:44

by Vinod Koul

[permalink] [raw]

Subject: Re: [RFC PATCH v2 05/12] dmaengine: PL08x: Add support for different maximum transfer size

On Sat, Jun 22, 2013 at 10:42:37PM +0200, Tomasz Figa wrote:
> PL080S has separate register to store transfer size in, allowing single
> transfer to be much larger than in standard PL080.
>
> This patch makes the amba-pl08x driver aware of this and removes writing
> transfer size to reserved bits of CH_CONTROL register on PL080S, which
> was not a problem witn transfer sizes fitting the original bitfield
> of PL080, but now would overwrite other fields.
>
> Signed-off-by: Tomasz Figa <[email protected]>
Acked by: Vinod Koul <[email protected]>

--
~Vinod

2013-06-25 18:34:52

by Russell King - ARM Linux

[permalink] [raw]

Subject: Re: [RFC PATCH v2 01/12] dmaengine: PL08x: Refactor pl08x_getbytes_chan() to lower indentation

On Tue, Jun 25, 2013 at 08:46:12PM +0530, Vinod Koul wrote:
> On Sat, Jun 22, 2013 at 10:42:33PM +0200, Tomasz Figa wrote:
> > - BUG_ON(clli < llis_bus || clli >= llis_bus +
> > + llis_va = txd->llis_va;
> > + llis_bus = txd->llis_bus;
> > +
> > + BUG_ON(clli < llis_bus || clli >= llis_bus +
> > sizeof(struct pl08x_lli) * MAX_NUM_TSFR_LLIS);
> IMO BUG_ON is too much for this. Perhaps returning error and logging error would
> be okay

That should be a separate patch, because that's an unrelated change
to this patch. As this part is just reformatting this part of the
code, it should contain no actual changes.

There's lots that this driver "should" do differently...