[RFC 0/4] ARM: S3C24XX: add dmaengine based dma-driver

[RFC 0/4] ARM: S3C24XX: add dmaengine based dma-driver

[Heiko Stübner]

This series tries to provide a basic dmaengine driver for the s3c24xx
SoCs.

The driver currently has some limitations, in that it does not support the
earlier s3c24xx socs, that cannot use every channel, but have special
channel requirements for specific slave-targets.

Another limitation is, that it currently does not support scatter-gather
lists with more than element, due to me not understanding sg at first.
While it does not hinder the usability, as all applicable Samsung drivers
currently use only 1-element-lists, I plan to fix this in the next revision.

In any case, I would be thankful for pointers to the obvious mistakes I'll
probably have made, due to this being my first travel into dmaengine-land.


Heiko Stuebner (4):
  ARM: S3C24XX: number the dma clocks
  dma: add dmaengine driver for Samsung s3c24xx SoCs
  ARM: S3C24XX: add platform-devices for new dma driver for s3c2412 and s3c2443
  ARM: SAMSUNG: set s3c24xx_dma_filter for s3c64xx-spi0 device

 arch/arm/mach-s3c24xx/clock-s3c2412.c     |    8 +-
 arch/arm/mach-s3c24xx/common-s3c2443.c    |   12 +-
 arch/arm/mach-s3c24xx/common.c            |  103 +++
 arch/arm/mach-s3c24xx/common.h            |    3 +
 arch/arm/plat-samsung/devs.c              |    5 +-
 drivers/dma/Kconfig                       |   12 +-
 drivers/dma/Makefile                      |    1 +
 drivers/dma/s3c24xx-dma.c                 | 1129 +++++++++++++++++++++++++++++
 include/linux/platform_data/dma-s3c24xx.h |   54 ++
 9 files changed, 1315 insertions(+), 12 deletions(-)
 create mode 100644 drivers/dma/s3c24xx-dma.c
 create mode 100644 include/linux/platform_data/dma-s3c24xx.h

-- 
1.7.2.3

[RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs

[Linus Walleij]

On Sat, May 11, 2013 at 1:31 PM, Heiko St?bner <heiko@sntech.de> wrote:

> Conceptually the s3c24xx-dma feels like a distant relative of the pl08x
> with numerous virtual channels being mapped to a lot less physical ones.
> The driver therefore borrows a lot from the amba-pl08x driver in this
> regard. Functionality-wise the driver gains a memcpy ability in addition
> to the slave_sg one.
>
> The driver currently only supports the "newer" SoCs which can use any
> physical channel for any dma slave. Support for the older SoCs where
> each channel only supports a subset of possible dma slaves will have to
> be added later.
>
> Tested on a s3c2416-based board, memcpy using the dmatest module and
> slave_sg partially using the spi-s3c64xx driver.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>

So have I understood correctly if I assume that *some* S3C
variants, i.e. this: arch/arm/mach-s3c64xx/dma.c
have a vanilla, unmodified, or just slightly modified
PL08x block, while this DMAC is something probably based on
the PL08x where some ASIC engineer has had a good time hacking
around in the VHDL code to meet some feature requirements.
Correct? Or plausible guess?

Exactly *how* far away from the pl08x hardware is it?

I guess you have already come to the conclusion that the
amba-pl08x.c driver cannot be augmented to handle this hardware
after some educated reading of that code?

But are really no parts reusable?

For example, if the LLIs have the same layout, could we split
out the LLI handling from amba-pl08x into a separate file and reuse
that?

The more you share with amba-pl08x the better for everyone,
I am positively sure. And please include Russell on the review
chain, he wrote the virtual channel abstraction.

If I was given the option amongst S3C work I would definatley
have augmented the amba-pl08x.c to handle the stuff in
arch/arm/mach-s3c64xx/dma.c *first* then started to work on
this driver, but I guess you might not be working on s3c64xx?

Yours,
Linus Walleij

[RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs

[Heiko Stübner]

Am Dienstag, 14. Mai 2013, 14:47:19 schrieb Linus Walleij:
> On Sat, May 11, 2013 at 1:31 PM, Heiko St?bner <heiko@sntech.de> wrote:
> > Conceptually the s3c24xx-dma feels like a distant relative of the pl08x
> > with numerous virtual channels being mapped to a lot less physical ones.
> > The driver therefore borrows a lot from the amba-pl08x driver in this
> > regard. Functionality-wise the driver gains a memcpy ability in addition
> > to the slave_sg one.
> > 
> > The driver currently only supports the "newer" SoCs which can use any
> > physical channel for any dma slave. Support for the older SoCs where
> > each channel only supports a subset of possible dma slaves will have to
> > be added later.
> > 
> > Tested on a s3c2416-based board, memcpy using the dmatest module and
> > slave_sg partially using the spi-s3c64xx driver.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> 
> So have I understood correctly if I assume that *some* S3C
> variants, i.e. this: arch/arm/mach-s3c64xx/dma.c
> have a vanilla, unmodified, or just slightly modified
> PL08x block, while this DMAC is something probably based on
> the PL08x where some ASIC engineer has had a good time hacking
> around in the VHDL code to meet some feature requirements.
> Correct? Or plausible guess?

You're guess is at good as mine :-) . The public s3c64xx (ARM11 based) 
documentation says that it is using s PL080 as dma controller while the 
s3c24xx (ARM9 based) SoCs have this one, which doesn't come with any label in 
the manuals.
Similar to the s3c64xx using a vic, while the s3c24xx uses something 
homegrown.

The relationship description was more based on the concepts used, i.e. the 
virtual channel concept and general handling of dma transfers feel somehow 
similar - as I said these are my first steps into this, so I still need to 
understand a lot.


> Exactly *how* far away from the pl08x hardware is it?
> 
> I guess you have already come to the conclusion that the
> amba-pl08x.c driver cannot be augmented to handle this hardware
> after some educated reading of that code?
> 
> But are really no parts reusable?
> 
> For example, if the LLIs have the same layout, could we split
> out the LLI handling from amba-pl08x into a separate file and reuse
> that?

The s3c24xx-dma doesn't have these. You simply put source and destination 
addresses on the bus, tell it the number of transfers and start it, while the 
pl080 seems to split these then into the LLIs, which seems to be some kind of 
hardware feature.

For the rest, the register-layout of the controller is completely different (I 
checked the PL080 manual), each channel has its own interrupt and the above 
mentioned missing LLIs.

Both drivers share somehow how they handle end of transfers, i.e. how they 
reuse the physical channel for a possible waiting virtual channel. But it 
didn't look like anything worth factoring out.


> The more you share with amba-pl08x the better for everyone,
> I am positively sure. And please include Russell on the review
> chain, he wrote the virtual channel abstraction.

I think the best feature is already factored out - the virtual channel 
handling, which was very nice to have.

I used the list from get_maintainer.pl, so sorry for forgetting Russel.


> If I was given the option amongst S3C work I would definatley
> have augmented the amba-pl08x.c to handle the stuff in
> arch/arm/mach-s3c64xx/dma.c *first* then started to work on
> this driver, but I guess you might not be working on s3c64xx?

correct, my pet-project is s3c2416-based [0], so I haven't seen any other 
Samsung stuff at all.


Heiko


[0] http://www.youtube.com/user/mmind81

[RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs

[Tomasz Figa]

Hi Linus, Heiko,

On Tuesday 14 of May 2013 14:47:19 Linus Walleij wrote:
> On Sat, May 11, 2013 at 1:31 PM, Heiko St?bner <heiko@sntech.de> wrote:
> > Conceptually the s3c24xx-dma feels like a distant relative of the pl08x
> > with numerous virtual channels being mapped to a lot less physical ones.
> > The driver therefore borrows a lot from the amba-pl08x driver in this
> > regard. Functionality-wise the driver gains a memcpy ability in addition
> > to the slave_sg one.
> > 
> > The driver currently only supports the "newer" SoCs which can use any
> > physical channel for any dma slave. Support for the older SoCs where
> > each channel only supports a subset of possible dma slaves will have to
> > be added later.
> > 
> > Tested on a s3c2416-based board, memcpy using the dmatest module and
> > slave_sg partially using the spi-s3c64xx driver.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> 
> So have I understood correctly if I assume that *some* S3C
> variants, i.e. this: arch/arm/mach-s3c64xx/dma.c
> have a vanilla, unmodified, or just slightly modified
> PL08x block, while this DMAC is something probably based on
> the PL08x where some ASIC engineer has had a good time hacking
> around in the VHDL code to meet some feature requirements.
> Correct? Or plausible guess?
> 
> Exactly *how* far away from the pl08x hardware is it?

AFAIK the DMAC of S3C24xx is completely different from PL08x. I think Heiko 
just meant that it uses similar concepts, like virtual channels.

> I guess you have already come to the conclusion that the
> amba-pl08x.c driver cannot be augmented to handle this hardware
> after some educated reading of that code?
> 
> But are really no parts reusable?
> 
> For example, if the LLIs have the same layout, could we split
> out the LLI handling from amba-pl08x into a separate file and reuse
> that?
> 
> The more you share with amba-pl08x the better for everyone,
> I am positively sure. And please include Russell on the review
> chain, he wrote the virtual channel abstraction.
> 
> If I was given the option amongst S3C work I would definatley
> have augmented the amba-pl08x.c to handle the stuff in
> arch/arm/mach-s3c64xx/dma.c *first* then started to work on
> this driver, but I guess you might not be working on s3c64xx?

Well, I still hope that someone will pick up the work on adapting PL08x driver 
to support PL080s of S3C64xx as well, but most likely it will end up with me 
doing it, as a part of DT support for S3C64xx.

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Kernel and System Framework

[RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs

[Linus Walleij]

On Tue, May 14, 2013 at 3:51 PM, Heiko St?bner <heiko@sntech.de> wrote:
> Am Dienstag, 14. Mai 2013, 14:47:19 schrieb Linus Walleij:
>> On Sat, May 11, 2013 at 1:31 PM, Heiko St?bner <heiko@sntech.de> wrote:
>> > Conceptually the s3c24xx-dma feels like a distant relative of the pl08x
>> > with numerous virtual channels being mapped to a lot less physical ones.
>> > The driver therefore borrows a lot from the amba-pl08x driver in this
>> > regard. Functionality-wise the driver gains a memcpy ability in addition
>> > to the slave_sg one.
>> >
>> > The driver currently only supports the "newer" SoCs which can use any
>> > physical channel for any dma slave. Support for the older SoCs where
>> > each channel only supports a subset of possible dma slaves will have to
>> > be added later.
>> >
>> > Tested on a s3c2416-based board, memcpy using the dmatest module and
>> > slave_sg partially using the spi-s3c64xx driver.
>> >
>> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>>
>> So have I understood correctly if I assume that *some* S3C
>> variants, i.e. this: arch/arm/mach-s3c64xx/dma.c
>> have a vanilla, unmodified, or just slightly modified
>> PL08x block, while this DMAC is something probably based on
>> the PL08x where some ASIC engineer has had a good time hacking
>> around in the VHDL code to meet some feature requirements.
>> Correct? Or plausible guess?
>
> You're guess is at good as mine :-) . The public s3c64xx (ARM11 based)
> documentation says that it is using s PL080 as dma controller while the
> s3c24xx (ARM9 based) SoCs have this one, which doesn't come with any label in
> the manuals.
> Similar to the s3c64xx using a vic, while the s3c24xx uses something
> homegrown.
>
> The relationship description was more based on the concepts used, i.e. the
> virtual channel concept and general handling of dma transfers feel somehow
> similar - as I said these are my first steps into this, so I still need to
> understand a lot.

OK then, a separate driver seems required, will look a bit closer at
the patch as such.

Yours,
Linus Walleij

[RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs

[Linus Walleij]

On Sat, May 11, 2013 at 1:31 PM, Heiko St?bner <heiko@sntech.de> wrote:

> This adds a new driver to support the s3c24xx dma using the dmaengine
> and make the old one in mach-s3c24xx obsolete in the long run.
>
> Conceptually the s3c24xx-dma feels like a distant relative of the pl08x
> with numerous virtual channels being mapped to a lot less physical ones.
> The driver therefore borrows a lot from the amba-pl08x driver in this
> regard. Functionality-wise the driver gains a memcpy ability in addition
> to the slave_sg one.
>
> The driver currently only supports the "newer" SoCs which can use any
> physical channel for any dma slave. Support for the older SoCs where
> each channel only supports a subset of possible dma slaves will have to
> be added later.
>
> Tested on a s3c2416-based board, memcpy using the dmatest module and
> slave_sg partially using the spi-s3c64xx driver.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
(...)
> +static u32 s3c24xx_dma_getbytes_chan(struct s3c24xx_dma_chan *s3cchan)
> +{
> +       struct s3c24xx_dma_phy *phy = s3cchan->phy;
> +       struct s3c24xx_txd *txd = s3cchan->at;
> +       u32 tc = readl(phy->base + DSTAT) & DSTAT_CURRTC_MASK;
> +
> +       switch (txd->dcon & DCON_DSZ_MASK) {
> +       case DCON_DSZ_BYTE:
> +               return tc;
> +       case DCON_DSZ_HALFWORD:
> +               return tc * 2;
> +       case DCON_DSZ_WORD:
> +               return tc * 4;
> +       default:
> +               break;
> +       }
> +
> +       BUG();

Isn't that a bit nasty. This macro should be used with care and we
should recover if possible. dev_err()?

> +/*
> + * Set the initial DMA register values.
> + * Put them into the hardware and start the transfer.
> + */
> +static void s3c24xx_dma_start_next_txd(struct s3c24xx_dma_chan *s3cchan)
> +{
> +       struct s3c24xx_dma_engine *s3cdma = s3cchan->host;
> +       struct s3c24xx_dma_phy *phy = s3cchan->phy;
> +       struct virt_dma_desc *vd = vchan_next_desc(&s3cchan->vc);
> +       struct s3c24xx_txd *txd = to_s3c24xx_txd(&vd->tx);
> +       u32 dcon = txd->dcon;
> +       u32 val;
> +
> +       list_del(&txd->vd.node);
> +
> +       s3cchan->at = txd;
> +
> +       /* Wait for channel inactive */
> +       while (s3c24xx_dma_phy_busy(phy))
> +               cpu_relax();
> +
> +       /* transfer-size and -count from len and width */
> +       switch (txd->width) {
> +       case 1:
> +               dcon |= DCON_DSZ_BYTE | txd->len;
> +               break;
> +       case 2:
> +               dcon |= DCON_DSZ_HALFWORD | (txd->len / 2);
> +               break;
> +       case 4:
> +               dcon |= DCON_DSZ_WORD | (txd->len / 4);
> +               break;
> +       }
> +
> +       if (s3cchan->slave) {
> +               if (s3cdma->sdata->has_reqsel) {
> +                       int reqsel = s3cdma->pdata->reqsel_map[s3cchan->id];
> +                       writel((reqsel << 1) | DMAREQSEL_HW,
> +                              phy->base + DMAREQSEL);
> +               } else {
> +                       dev_err(&s3cdma->pdev->dev, "non-reqsel unsupported\n");
> +                       return;
> +                       dcon |= DCON_HWTRIG;
> +               }
> +       } else {
> +               if (s3cdma->sdata->has_reqsel) {
> +                       writel(0, phy->base + DMAREQSEL);
> +               } else {
> +                       dev_err(&s3cdma->pdev->dev, "non-reqsel unsupported\n");
> +                       return;
> +               }
> +       }
> +
> +       writel(txd->src_addr, phy->base + DISRC);
> +       writel(txd->disrcc, phy->base + DISRCC);
> +       writel(txd->dst_addr, phy->base + DIDST);
> +       writel(txd->didstc, phy->base + DIDSTC);
> +
> +       writel(dcon, phy->base + DCON);
> +
> +       val = readl(phy->base + DMASKTRIG);
> +       val &= ~DMASKTRIG_STOP;
> +       val |= DMASKTRIG_ON;
> +       writel(val, phy->base + DMASKTRIG);
> +
> +       /* trigger the dma operation for memcpy transfers */
> +       if (!s3cchan->slave) {
> +               val = readl(phy->base + DMASKTRIG);
> +               val |= DMASKTRIG_SWTRIG;
> +               writel(val, phy->base + DMASKTRIG);
> +       }
> +}

Since you have a few readl() and writel() in potentially
time-critical code, consider using readl_relaxed() and
writel_relaxed().

> +static irqreturn_t s3c24xx_dma_irq(int irq, void *data)
> +{
> +       struct s3c24xx_dma_phy *phy = data;
> +       struct s3c24xx_dma_chan *s3cchan = phy->serving;
> +       struct s3c24xx_txd *txd;
> +
> +       dev_dbg(&phy->host->pdev->dev, "interrupt on channel %d\n", phy->id);
> +
> +       if (!s3cchan) {
> +               dev_err(&phy->host->pdev->dev, "interrupt on unused channel %d\n",
> +                       phy->id);
> +               return IRQ_NONE;
> +       }
> +
> +       spin_lock(&s3cchan->vc.lock);
> +       txd = s3cchan->at;
> +       if (txd) {
> +               s3cchan->at = NULL;
> +               vchan_cookie_complete(&txd->vd);
> +
> +               /*
> +                * And start the next descriptor (if any),
> +                * otherwise free this channel.
> +                */
> +               if (vchan_next_desc(&s3cchan->vc))
> +                       s3c24xx_dma_start_next_txd(s3cchan);
> +               else
> +                       s3c24xx_dma_phy_free(s3cchan);
> +       }
> +       spin_unlock(&s3cchan->vc.lock);
> +
> +       return IRQ_HANDLED;
> +}

OK so one IRQ per channel. Is there really no status
register to check or flag to clear on these IRQs?

Apart from these smallish things it looks good to me:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs

[Heiko Stübner]

Am Mittwoch, 15. Mai 2013, 20:53:32 schrieb Linus Walleij:
> On Sat, May 11, 2013 at 1:31 PM, Heiko St?bner <heiko@sntech.de> wrote:
> > This adds a new driver to support the s3c24xx dma using the dmaengine
> > and make the old one in mach-s3c24xx obsolete in the long run.
> > 
> > Conceptually the s3c24xx-dma feels like a distant relative of the pl08x
> > with numerous virtual channels being mapped to a lot less physical ones.
> > The driver therefore borrows a lot from the amba-pl08x driver in this
> > regard. Functionality-wise the driver gains a memcpy ability in addition
> > to the slave_sg one.
> > 
> > The driver currently only supports the "newer" SoCs which can use any
> > physical channel for any dma slave. Support for the older SoCs where
> > each channel only supports a subset of possible dma slaves will have to
> > be added later.
> > 
> > Tested on a s3c2416-based board, memcpy using the dmatest module and
> > slave_sg partially using the spi-s3c64xx driver.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> 
> (...)
> 
> > +static u32 s3c24xx_dma_getbytes_chan(struct s3c24xx_dma_chan *s3cchan)
> > +{
> > +       struct s3c24xx_dma_phy *phy = s3cchan->phy;
> > +       struct s3c24xx_txd *txd = s3cchan->at;
> > +       u32 tc = readl(phy->base + DSTAT) & DSTAT_CURRTC_MASK;
> > +
> > +       switch (txd->dcon & DCON_DSZ_MASK) {
> > +       case DCON_DSZ_BYTE:
> > +               return tc;
> > +       case DCON_DSZ_HALFWORD:
> > +               return tc * 2;
> > +       case DCON_DSZ_WORD:
> > +               return tc * 4;
> > +       default:
> > +               break;
> > +       }
> > +
> > +       BUG();
> 
> Isn't that a bit nasty. This macro should be used with care and we
> should recover if possible. dev_err()?

runtime_config already denies any settings not in the 1,2 or 4bytes range - 
the default-part should therefore never be reached. So if any other value 
magically appears in the register and triggers the default-part, something is 
seriously wrong. So my guess is, the BUG might be appropriate.

On the other hand the whole default+BUG part could also simply go away, for 
the same reasons.


> > +/*
> > + * Set the initial DMA register values.
> > + * Put them into the hardware and start the transfer.
> > + */
> > +static void s3c24xx_dma_start_next_txd(struct s3c24xx_dma_chan *s3cchan)
> > +{
> > +       struct s3c24xx_dma_engine *s3cdma = s3cchan->host;
> > +       struct s3c24xx_dma_phy *phy = s3cchan->phy;
> > +       struct virt_dma_desc *vd = vchan_next_desc(&s3cchan->vc);
> > +       struct s3c24xx_txd *txd = to_s3c24xx_txd(&vd->tx);
> > +       u32 dcon = txd->dcon;
> > +       u32 val;
> > +
> > +       list_del(&txd->vd.node);
> > +
> > +       s3cchan->at = txd;
> > +
> > +       /* Wait for channel inactive */
> > +       while (s3c24xx_dma_phy_busy(phy))
> > +               cpu_relax();
> > +
> > +       /* transfer-size and -count from len and width */
> > +       switch (txd->width) {
> > +       case 1:
> > +               dcon |= DCON_DSZ_BYTE | txd->len;
> > +               break;
> > +       case 2:
> > +               dcon |= DCON_DSZ_HALFWORD | (txd->len / 2);
> > +               break;
> > +       case 4:
> > +               dcon |= DCON_DSZ_WORD | (txd->len / 4);
> > +               break;
> > +       }
> > +
> > +       if (s3cchan->slave) {
> > +               if (s3cdma->sdata->has_reqsel) {
> > +                       int reqsel =
> > s3cdma->pdata->reqsel_map[s3cchan->id]; +                      
> > writel((reqsel << 1) | DMAREQSEL_HW,
> > +                              phy->base + DMAREQSEL);
> > +               } else {
> > +                       dev_err(&s3cdma->pdev->dev, "non-reqsel
> > unsupported\n"); +                       return;
> > +                       dcon |= DCON_HWTRIG;
> > +               }
> > +       } else {
> > +               if (s3cdma->sdata->has_reqsel) {
> > +                       writel(0, phy->base + DMAREQSEL);
> > +               } else {
> > +                       dev_err(&s3cdma->pdev->dev, "non-reqsel
> > unsupported\n"); +                       return;
> > +               }
> > +       }
> > +
> > +       writel(txd->src_addr, phy->base + DISRC);
> > +       writel(txd->disrcc, phy->base + DISRCC);
> > +       writel(txd->dst_addr, phy->base + DIDST);
> > +       writel(txd->didstc, phy->base + DIDSTC);
> > +
> > +       writel(dcon, phy->base + DCON);
> > +
> > +       val = readl(phy->base + DMASKTRIG);
> > +       val &= ~DMASKTRIG_STOP;
> > +       val |= DMASKTRIG_ON;
> > +       writel(val, phy->base + DMASKTRIG);
> > +
> > +       /* trigger the dma operation for memcpy transfers */
> > +       if (!s3cchan->slave) {
> > +               val = readl(phy->base + DMASKTRIG);
> > +               val |= DMASKTRIG_SWTRIG;
> > +               writel(val, phy->base + DMASKTRIG);
> > +       }
> > +}
> 
> Since you have a few readl() and writel() in potentially
> time-critical code, consider using readl_relaxed() and
> writel_relaxed().

You're right of course.

If I understand the writel semantics and the thread from you from 2011 [0] 
correctly, only the writel to DMASKTRIG mustn't be relaxed to make sure the 
settings registers are written to before, so like:

       writel_relaxed(txd->src_addr, phy->base + DISRC);
       writel_relaxed(txd->disrcc, phy->base + DISRCC);
       writel_relaxed(txd->dst_addr, phy->base + DIDST);
       writel_relaxed(txd->didstc, phy->base + DIDSTC);
       writel_relaxed(dcon, phy->base + DCON);

       val = readl_relaxed(phy->base + DMASKTRIG);
       val &= ~DMASKTRIG_STOP;
       val |= DMASKTRIG_ON;
       writel(val, phy->base + DMASKTRIG);


And note to self: check if the memcpy-speciality can move into the first 
DMASKTRIG write.

> > +static irqreturn_t s3c24xx_dma_irq(int irq, void *data)
> > +{
> > +       struct s3c24xx_dma_phy *phy = data;
> > +       struct s3c24xx_dma_chan *s3cchan = phy->serving;
> > +       struct s3c24xx_txd *txd;
> > +
> > +       dev_dbg(&phy->host->pdev->dev, "interrupt on channel %d\n",
> > phy->id); +
> > +       if (!s3cchan) {
> > +               dev_err(&phy->host->pdev->dev, "interrupt on unused
> > channel %d\n", +                       phy->id);
> > +               return IRQ_NONE;
> > +       }
> > +
> > +       spin_lock(&s3cchan->vc.lock);
> > +       txd = s3cchan->at;
> > +       if (txd) {
> > +               s3cchan->at = NULL;
> > +               vchan_cookie_complete(&txd->vd);
> > +
> > +               /*
> > +                * And start the next descriptor (if any),
> > +                * otherwise free this channel.
> > +                */
> > +               if (vchan_next_desc(&s3cchan->vc))
> > +                       s3c24xx_dma_start_next_txd(s3cchan);
> > +               else
> > +                       s3c24xx_dma_phy_free(s3cchan);
> > +       }
> > +       spin_unlock(&s3cchan->vc.lock);
> > +
> > +       return IRQ_HANDLED;
> > +}
> 
> OK so one IRQ per channel. Is there really no status
> register to check or flag to clear on these IRQs?

Nope there are none. The only status you get from the controller is busy/non-
busy and remaining transfers for the transaction - the DSTAT register in the 
code. And not listed in the code the current memory addresses (source and 
destination) in use. There are no other registers at all.

And the irq itself only triggers when all transfers of the transaction are 
done (transfer_count is 0).


> Apart from these smallish things it looks good to me:
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

really? Cool.
Part of me was expecting tomatoes or other vegetables to be thrown ;-) .


Thanks for looking thru the driver.
Heiko


[0] http://comments.gmane.org/gmane.linux.ports.arm.kernel/117626

[RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs

[Sylwester Nawrocki]

On 05/15/2013 10:31 PM, Heiko St?bner wrote:
>>> +       BUG();
>> >
>> >  Isn't that a bit nasty. This macro should be used with care and we
>> >  should recover if possible. dev_err()?
>
> runtime_config already denies any settings not in the 1,2 or 4bytes range -
> the default-part should therefore never be reached. So if any other value
> magically appears in the register and triggers the default-part, something is
> seriously wrong. So my guess is, the BUG might be appropriate.
>
> On the other hand the whole default+BUG part could also simply go away, for
> the same reasons.

IMHO BUG() is not needed at all. As Linus suggested dev_err() is such case
or WARN_ON() would be more appropriate. This has been discussed in the past
extensively, not sure if you are aware of the other Linus' opinion on
BUG()/BUG_ON() proliferation: https://lkml.org/lkml/2012/9/27/461

Regards,
Sylwester

[RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs

[Heiko Stübner]

Am Mittwoch, 15. Mai 2013, 23:20:08 schrieb Sylwester Nawrocki:
> On 05/15/2013 10:31 PM, Heiko St?bner wrote:
> >>> +       BUG();
> >>> 
> >> >  Isn't that a bit nasty. This macro should be used with care and we
> >> >  should recover if possible. dev_err()?
> > 
> > runtime_config already denies any settings not in the 1,2 or 4bytes range
> > - the default-part should therefore never be reached. So if any other
> > value magically appears in the register and triggers the default-part,
> > something is seriously wrong. So my guess is, the BUG might be
> > appropriate.
> > 
> > On the other hand the whole default+BUG part could also simply go away,
> > for the same reasons.
> 
> IMHO BUG() is not needed at all. As Linus suggested dev_err() is such case
> or WARN_ON() would be more appropriate. This has been discussed in the past
> extensively, not sure if you are aware of the other Linus' opinion on
> BUG()/BUG_ON() proliferation: https://lkml.org/lkml/2012/9/27/461

Very interesting read and I'll keep this in mind in the future. What about the 
other option ... i.e. simply getting rid of the whole "error handling", as the 
other code paths should already make sure that only valid values get written 
into the register.

Can the value change in the register somehow on its own without kernel 
intervention, or does this not happen?


Thanks
Heiko

[RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs

[Tomasz Figa]

On Wednesday 15 of May 2013 23:48:31 Heiko St?bner wrote:
> Am Mittwoch, 15. Mai 2013, 23:20:08 schrieb Sylwester Nawrocki:
> > On 05/15/2013 10:31 PM, Heiko St?bner wrote:
> > >>> +       BUG();
> > >>> 
> > >> >  Isn't that a bit nasty. This macro should be used with care and
> > >> >  we
> > >> >  should recover if possible. dev_err()?
> > > 
> > > runtime_config already denies any settings not in the 1,2 or 4bytes
> > > range - the default-part should therefore never be reached. So if
> > > any other value magically appears in the register and triggers the
> > > default-part, something is seriously wrong. So my guess is, the BUG
> > > might be appropriate.
> > > 
> > > On the other hand the whole default+BUG part could also simply go
> > > away,
> > > for the same reasons.
> > 
> > IMHO BUG() is not needed at all. As Linus suggested dev_err() is such
> > case or WARN_ON() would be more appropriate. This has been discussed
> > in the past extensively, not sure if you are aware of the other
> > Linus' opinion on BUG()/BUG_ON() proliferation:
> > https://lkml.org/lkml/2012/9/27/461
> Very interesting read and I'll keep this in mind in the future. What
> about the other option ... i.e. simply getting rid of the whole "error
> handling", as the other code paths should already make sure that only
> valid values get written into the register.
> 
> Can the value change in the register somehow on its own without kernel
> intervention, or does this not happen?

Hmm, it depends on hardware, I guess. Not sure how it works on this 
particular IP.

Still, the mentioned BUG() was about a value in a driver-filled struct, 
wasn't it?

/* Quoting the the code for reference */

> +static u32 s3c24xx_dma_getbytes_chan(struct s3c24xx_dma_chan *s3cchan)
> +{
> +       struct s3c24xx_dma_phy *phy = s3cchan->phy;
> +       struct s3c24xx_txd *txd = s3cchan->at;
> +       u32 tc = readl(phy->base + DSTAT) & DSTAT_CURRTC_MASK;
> +
> +       switch (txd->dcon & DCON_DSZ_MASK) {
> +       case DCON_DSZ_BYTE:
> +               return tc;
> +       case DCON_DSZ_HALFWORD:
> +               return tc * 2;
> +       case DCON_DSZ_WORD:
> +               return tc * 4;
> +       default:
> +               break;
> +       }
> +
> +       BUG();

(Btw. I don't see anything setting the DCON_DSZ bits in this field. Am I 
missing something?)

Best regards,
Tomasz

[RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs

[Heiko Stübner]

Am Donnerstag, 16. Mai 2013, 00:02:40 schrieb Tomasz Figa:
> On Wednesday 15 of May 2013 23:48:31 Heiko St?bner wrote:
> > Am Mittwoch, 15. Mai 2013, 23:20:08 schrieb Sylwester Nawrocki:
> > > On 05/15/2013 10:31 PM, Heiko St?bner wrote:
> > > >>> +       BUG();
> > > >>> 
> > > >> >  Isn't that a bit nasty. This macro should be used with care and
> > > >> >  we
> > > >> >  should recover if possible. dev_err()?
> > > > 
> > > > runtime_config already denies any settings not in the 1,2 or 4bytes
> > > > range - the default-part should therefore never be reached. So if
> > > > any other value magically appears in the register and triggers the
> > > > default-part, something is seriously wrong. So my guess is, the BUG
> > > > might be appropriate.
> > > > 
> > > > On the other hand the whole default+BUG part could also simply go
> > > > away,
> > > > for the same reasons.
> > > 
> > > IMHO BUG() is not needed at all. As Linus suggested dev_err() is such
> > > case or WARN_ON() would be more appropriate. This has been discussed
> > > in the past extensively, not sure if you are aware of the other
> > > Linus' opinion on BUG()/BUG_ON() proliferation:
> > > https://lkml.org/lkml/2012/9/27/461
> > 
> > Very interesting read and I'll keep this in mind in the future. What
> > about the other option ... i.e. simply getting rid of the whole "error
> > handling", as the other code paths should already make sure that only
> > valid values get written into the register.
> > 
> > Can the value change in the register somehow on its own without kernel
> > intervention, or does this not happen?
> 
> Hmm, it depends on hardware, I guess. Not sure how it works on this
> particular IP.
> 
> Still, the mentioned BUG() was about a value in a driver-filled struct,
> wasn't it?
> 
> /* Quoting the the code for reference */
> 
> > +static u32 s3c24xx_dma_getbytes_chan(struct s3c24xx_dma_chan *s3cchan)
> > +{
> > +       struct s3c24xx_dma_phy *phy = s3cchan->phy;
> > +       struct s3c24xx_txd *txd = s3cchan->at;
> > +       u32 tc = readl(phy->base + DSTAT) & DSTAT_CURRTC_MASK;
> > +
> > +       switch (txd->dcon & DCON_DSZ_MASK) {
> > +       case DCON_DSZ_BYTE:
> > +               return tc;
> > +       case DCON_DSZ_HALFWORD:
> > +               return tc * 2;
> > +       case DCON_DSZ_WORD:
> > +               return tc * 4;
> > +       default:
> > +               break;
> > +       }
> > +
> > +       BUG();
> 
> (Btw. I don't see anything setting the DCON_DSZ bits in this field. Am I
> missing something?)

this is for calculating the remaining bytes of the transaction. which is used 
in s3c24xx_dma_tx_status. 

And when looking at it again, I can't really fathom why I did it this way with 
decoding the DSZ from the dcon value of the s3c24xx_txd again instead of 
simply using the width value of the same struct .... 

So it can be much simpler as
	(...)
     u32 tc = readl(phy->base + DSTAT) & DSTAT_CURRTC_MASK;
	return tc * txd->width;

getting rid of this stuff alltogether


still puzzled how I came up with this strangeness in the first place
Heiko

[RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs

[Tomasz Figa]

On Thursday 16 of May 2013 00:45:03 Heiko St?bner wrote:
> Am Donnerstag, 16. Mai 2013, 00:02:40 schrieb Tomasz Figa:
> > On Wednesday 15 of May 2013 23:48:31 Heiko St?bner wrote:
> > > Am Mittwoch, 15. Mai 2013, 23:20:08 schrieb Sylwester Nawrocki:
> > > > On 05/15/2013 10:31 PM, Heiko St?bner wrote:
> > > > >>> +       BUG();
> > > > >>> 
> > > > >> >  Isn't that a bit nasty. This macro should be used with care
> > > > >> >  and
> > > > >> >  we
> > > > >> >  should recover if possible. dev_err()?
> > > > > 
> > > > > runtime_config already denies any settings not in the 1,2 or
> > > > > 4bytes
> > > > > range - the default-part should therefore never be reached. So
> > > > > if
> > > > > any other value magically appears in the register and triggers
> > > > > the
> > > > > default-part, something is seriously wrong. So my guess is, the
> > > > > BUG
> > > > > might be appropriate.
> > > > > 
> > > > > On the other hand the whole default+BUG part could also simply
> > > > > go
> > > > > away,
> > > > > for the same reasons.
> > > > 
> > > > IMHO BUG() is not needed at all. As Linus suggested dev_err() is
> > > > such
> > > > case or WARN_ON() would be more appropriate. This has been
> > > > discussed
> > > > in the past extensively, not sure if you are aware of the other
> > > > Linus' opinion on BUG()/BUG_ON() proliferation:
> > > > https://lkml.org/lkml/2012/9/27/461
> > > 
> > > Very interesting read and I'll keep this in mind in the future. What
> > > about the other option ... i.e. simply getting rid of the whole
> > > "error
> > > handling", as the other code paths should already make sure that
> > > only
> > > valid values get written into the register.
> > > 
> > > Can the value change in the register somehow on its own without
> > > kernel
> > > intervention, or does this not happen?
> > 
> > Hmm, it depends on hardware, I guess. Not sure how it works on this
> > particular IP.
> > 
> > Still, the mentioned BUG() was about a value in a driver-filled
> > struct,
> > wasn't it?
> > 
> > /* Quoting the the code for reference */
> > 
> > > +static u32 s3c24xx_dma_getbytes_chan(struct s3c24xx_dma_chan
> > > *s3cchan)
> > > +{
> > > +       struct s3c24xx_dma_phy *phy = s3cchan->phy;
> > > +       struct s3c24xx_txd *txd = s3cchan->at;
> > > +       u32 tc = readl(phy->base + DSTAT) & DSTAT_CURRTC_MASK;
> > > +
> > > +       switch (txd->dcon & DCON_DSZ_MASK) {
> > > +       case DCON_DSZ_BYTE:
> > > +               return tc;
> > > +       case DCON_DSZ_HALFWORD:
> > > +               return tc * 2;
> > > +       case DCON_DSZ_WORD:
> > > +               return tc * 4;
> > > +       default:
> > > +               break;
> > > +       }
> > > +
> > > +       BUG();
> > 
> > (Btw. I don't see anything setting the DCON_DSZ bits in this field. Am
> > I missing something?)
> 
> this is for calculating the remaining bytes of the transaction. which is
> used in s3c24xx_dma_tx_status.
> 
> And when looking at it again, I can't really fathom why I did it this
> way with decoding the DSZ from the dcon value of the s3c24xx_txd again
> instead of simply using the width value of the same struct ....
> 
> So it can be much simpler as
> 	(...)
>      u32 tc = readl(phy->base + DSTAT) & DSTAT_CURRTC_MASK;
> 	return tc * txd->width;
> 
> getting rid of this stuff alltogether
> 
> 
> still puzzled how I came up with this strangeness in the first place

Hehe, happens.

I'm still yet to review the whole series, but I'm failing to find enough 
time. Hopefully will get to do it soon.

Best regards,
Tomasz

[RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs

[Linus Walleij]

On Wed, May 15, 2013 at 10:31 PM, Heiko St?bner <heiko@sntech.de> wrote:

> If I understand the writel semantics and the thread from you from 2011 [0]
> correctly, only the writel to DMASKTRIG mustn't be relaxed to make sure the
> settings registers are written to before, so like:
>
>        writel_relaxed(txd->src_addr, phy->base + DISRC);
>        writel_relaxed(txd->disrcc, phy->base + DISRCC);
>        writel_relaxed(txd->dst_addr, phy->base + DIDST);
>        writel_relaxed(txd->didstc, phy->base + DIDSTC);
>        writel_relaxed(dcon, phy->base + DCON);
>
>        val = readl_relaxed(phy->base + DMASKTRIG);
>        val &= ~DMASKTRIG_STOP;
>        val |= DMASKTRIG_ON;
>        writel(val, phy->base + DMASKTRIG);

Yep. That will drain write buffers etc and make sure all outstanding writes hit
the hardware.

Yours,
Linus Walleij