| Message ID | 20180703123214.23090-3-paul@crapouillou.net |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | dma-jz4780 improvements | expand |
Hi Paul, On Tue, Jul 03, 2018 at 02:32:02PM +0200, Paul Cercueil wrote: > @@ -804,9 +818,19 @@ static int jz4780_dma_probe(struct platform_device *pdev) > return -EINVAL; > } > > - jzdma->base = devm_ioremap_resource(dev, res); > - if (IS_ERR(jzdma->base)) > - return PTR_ERR(jzdma->base); > + jzdma->chn_base = devm_ioremap_resource(dev, res); > + if (IS_ERR(jzdma->chn_base)) > + return PTR_ERR(jzdma->chn_base); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (!res) { > + dev_err(dev, "failed to get I/O memory\n"); > + return -EINVAL; > + } > + > + jzdma->ctrl_base = devm_ioremap_resource(dev, res); > + if (IS_ERR(jzdma->ctrl_base)) > + return PTR_ERR(jzdma->ctrl_base); Could we have this failure case fall back to the existing behaviour where we only have a single resource covering all the registers? That would avoid breaking bisection between this patch & the one that updates the JZ4780 DT. For example: #define JZ4780_DMA_CTRL_OFFSET 0x1000 res = platform_get_resource(pdev, IORESOURCE_MEM, 1); if (res) { jzdma->ctrl_base = devm_ioremap_resource(dev, res); if (IS_ERR(jzdma->ctrl_base)) return PTR_ERR(jzdma->ctrl_base); } else { jzdma->ctrl_base = jzdma->chn_base + JZ4780_DMA_CTRL_OFFSET; } Then you could remove the fallback after patch 13, to end up with the same code you have now but without breaking bisection. Most correct might be to move patch 13 to right after this one, so that the JZ4780-specific fallback can be removed before adding support for any of the other SoCs. Thanks, Paul -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Paul, On 3 July 2018 at 18:02, Paul Cercueil <paul@crapouillou.net> wrote: > The register area of the JZ4780 DMA core can be split into different > sections for different purposes: > > * one set of registers is used to perform actions at the DMA core level, > that will generally affect all channels; > > * one set of registers per DMA channel, to perform actions at the DMA > channel level, that will only affect the channel in question. > > The problem rises when trying to support new versions of the JZ47xx > Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one > with six DMA channels, and the register sets are interleaved: > <DMA0 chan regs> <DMA1 chan regs> <DMA0 ctrl regs> <DMA1 ctrl regs> > > By using one memory resource for the channel-specific registers and > one memory resource for the core-specific registers, we can support > the JZ4770, by initializing the driver once per DMA core with different > addresses. As per my understanding device tree should be modified only when hardware changes. This looks the other way around. It must be possible to achieve what you are trying to do in this patch without changing the device tree. > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > .../devicetree/bindings/dma/jz4780-dma.txt | 6 +- > drivers/dma/dma-jz4780.c | 106 +++++++++++------- > 2 files changed, 69 insertions(+), 43 deletions(-) > > diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt b/Documentation/devicetree/bindings/dma/jz4780-dma.txt > index f25feee62b15..f9b1864f5b77 100644 > --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt > +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt > @@ -3,7 +3,8 @@ > Required properties: > > - compatible: Should be "ingenic,jz4780-dma" > -- reg: Should contain the DMA controller registers location and length. > +- reg: Should contain the DMA channel registers location and length, followed > + by the DMA controller registers location and length. > - interrupts: Should contain the interrupt specifier of the DMA controller. > - interrupt-parent: Should be the phandle of the interrupt controller that > - clocks: Should contain a clock specifier for the JZ4780 PDMA clock. > @@ -22,7 +23,8 @@ Example: > > dma: dma@13420000 { > compatible = "ingenic,jz4780-dma"; > - reg = <0x13420000 0x10000>; > + reg = <0x13420000 0x400 > + 0x13421000 0x40>; > > interrupt-parent = <&intc>; > interrupts = <10>; > diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c > index b40f491f0367..4d234caf5d62 100644 > --- a/drivers/dma/dma-jz4780.c > +++ b/drivers/dma/dma-jz4780.c > @@ -25,26 +25,26 @@ > #include "virt-dma.h" > > /* Global registers. */ > -#define JZ_DMA_REG_DMAC 0x1000 > -#define JZ_DMA_REG_DIRQP 0x1004 > -#define JZ_DMA_REG_DDR 0x1008 > -#define JZ_DMA_REG_DDRS 0x100c > -#define JZ_DMA_REG_DMACP 0x101c > -#define JZ_DMA_REG_DSIRQP 0x1020 > -#define JZ_DMA_REG_DSIRQM 0x1024 > -#define JZ_DMA_REG_DCIRQP 0x1028 > -#define JZ_DMA_REG_DCIRQM 0x102c > +#define JZ_DMA_REG_DMAC 0x00 > +#define JZ_DMA_REG_DIRQP 0x04 > +#define JZ_DMA_REG_DDR 0x08 > +#define JZ_DMA_REG_DDRS 0x0c > +#define JZ_DMA_REG_DMACP 0x1c > +#define JZ_DMA_REG_DSIRQP 0x20 > +#define JZ_DMA_REG_DSIRQM 0x24 > +#define JZ_DMA_REG_DCIRQP 0x28 > +#define JZ_DMA_REG_DCIRQM 0x2c > > /* Per-channel registers. */ > #define JZ_DMA_REG_CHAN(n) (n * 0x20) > -#define JZ_DMA_REG_DSA(n) (0x00 + JZ_DMA_REG_CHAN(n)) > -#define JZ_DMA_REG_DTA(n) (0x04 + JZ_DMA_REG_CHAN(n)) > -#define JZ_DMA_REG_DTC(n) (0x08 + JZ_DMA_REG_CHAN(n)) > -#define JZ_DMA_REG_DRT(n) (0x0c + JZ_DMA_REG_CHAN(n)) > -#define JZ_DMA_REG_DCS(n) (0x10 + JZ_DMA_REG_CHAN(n)) > -#define JZ_DMA_REG_DCM(n) (0x14 + JZ_DMA_REG_CHAN(n)) > -#define JZ_DMA_REG_DDA(n) (0x18 + JZ_DMA_REG_CHAN(n)) > -#define JZ_DMA_REG_DSD(n) (0x1c + JZ_DMA_REG_CHAN(n)) > +#define JZ_DMA_REG_DSA 0x00 > +#define JZ_DMA_REG_DTA 0x04 > +#define JZ_DMA_REG_DTC 0x08 > +#define JZ_DMA_REG_DRT 0x0c > +#define JZ_DMA_REG_DCS 0x10 > +#define JZ_DMA_REG_DCM 0x14 > +#define JZ_DMA_REG_DDA 0x18 > +#define JZ_DMA_REG_DSD 0x1c > > #define JZ_DMA_DMAC_DMAE BIT(0) > #define JZ_DMA_DMAC_AR BIT(2) > @@ -140,7 +140,8 @@ enum jz_version { > > struct jz4780_dma_dev { > struct dma_device dma_device; > - void __iomem *base; > + void __iomem *chn_base; > + void __iomem *ctrl_base; > struct clk *clk; > unsigned int irq; > unsigned int nb_channels; > @@ -174,16 +175,28 @@ static inline struct jz4780_dma_dev *jz4780_dma_chan_parent( > dma_device); > } > > -static inline uint32_t jz4780_dma_readl(struct jz4780_dma_dev *jzdma, > +static inline uint32_t jz4780_dma_chn_readl(struct jz4780_dma_dev *jzdma, > + unsigned int chn, unsigned int reg) > +{ > + return readl(jzdma->chn_base + reg + JZ_DMA_REG_CHAN(chn)); > +} > + > +static inline void jz4780_dma_chn_writel(struct jz4780_dma_dev *jzdma, > + unsigned int chn, unsigned int reg, uint32_t val) > +{ > + writel(val, jzdma->chn_base + reg + JZ_DMA_REG_CHAN(chn)); > +} > + > +static inline uint32_t jz4780_dma_ctrl_readl(struct jz4780_dma_dev *jzdma, > unsigned int reg) > { > - return readl(jzdma->base + reg); > + return readl(jzdma->ctrl_base + reg); > } > > -static inline void jz4780_dma_writel(struct jz4780_dma_dev *jzdma, > +static inline void jz4780_dma_ctrl_writel(struct jz4780_dma_dev *jzdma, > unsigned int reg, uint32_t val) > { > - writel(val, jzdma->base + reg); > + writel(val, jzdma->ctrl_base + reg); > } > > static struct jz4780_dma_desc *jz4780_dma_desc_alloc( > @@ -478,17 +491,18 @@ static void jz4780_dma_begin(struct jz4780_dma_chan *jzchan) > } > > /* Use 8-word descriptors. */ > - jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), JZ_DMA_DCS_DES8); > + jz4780_dma_chn_writel(jzdma, jzchan->id, > + JZ_DMA_REG_DCS, JZ_DMA_DCS_DES8); > > /* Write descriptor address and initiate descriptor fetch. */ > desc_phys = jzchan->desc->desc_phys + > (jzchan->curr_hwdesc * sizeof(*jzchan->desc->desc)); > - jz4780_dma_writel(jzdma, JZ_DMA_REG_DDA(jzchan->id), desc_phys); > - jz4780_dma_writel(jzdma, JZ_DMA_REG_DDRS, BIT(jzchan->id)); > + jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DDA, desc_phys); > + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DDRS, BIT(jzchan->id)); > > /* Enable the channel. */ > - jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), > - JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE); > + jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, > + JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE); > } > > static void jz4780_dma_issue_pending(struct dma_chan *chan) > @@ -514,7 +528,7 @@ static int jz4780_dma_terminate_all(struct dma_chan *chan) > spin_lock_irqsave(&jzchan->vchan.lock, flags); > > /* Clear the DMA status and stop the transfer. */ > - jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), 0); > + jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0); > if (jzchan->desc) { > vchan_terminate_vdesc(&jzchan->desc->vdesc); > jzchan->desc = NULL; > @@ -563,8 +577,8 @@ static size_t jz4780_dma_desc_residue(struct jz4780_dma_chan *jzchan, > residue += desc->desc[i].dtc << jzchan->transfer_shift; > > if (next_sg != 0) { > - count = jz4780_dma_readl(jzdma, > - JZ_DMA_REG_DTC(jzchan->id)); > + count = jz4780_dma_chn_readl(jzdma, jzchan->id, > + JZ_DMA_REG_DTC); > residue += count << jzchan->transfer_shift; > } > > @@ -611,8 +625,8 @@ static void jz4780_dma_chan_irq(struct jz4780_dma_dev *jzdma, > > spin_lock(&jzchan->vchan.lock); > > - dcs = jz4780_dma_readl(jzdma, JZ_DMA_REG_DCS(jzchan->id)); > - jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), 0); > + dcs = jz4780_dma_chn_readl(jzdma, jzchan->id, JZ_DMA_REG_DCS); > + jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0); > > if (dcs & JZ_DMA_DCS_AR) { > dev_warn(&jzchan->vchan.chan.dev->device, > @@ -651,7 +665,7 @@ static irqreturn_t jz4780_dma_irq_handler(int irq, void *data) > uint32_t pending, dmac; > int i; > > - pending = jz4780_dma_readl(jzdma, JZ_DMA_REG_DIRQP); > + pending = jz4780_dma_ctrl_readl(jzdma, JZ_DMA_REG_DIRQP); > > for (i = 0; i < jzdma->nb_channels; i++) { > if (!(pending & (1<<i))) > @@ -661,12 +675,12 @@ static irqreturn_t jz4780_dma_irq_handler(int irq, void *data) > } > > /* Clear halt and address error status of all channels. */ > - dmac = jz4780_dma_readl(jzdma, JZ_DMA_REG_DMAC); > + dmac = jz4780_dma_ctrl_readl(jzdma, JZ_DMA_REG_DMAC); > dmac &= ~(JZ_DMA_DMAC_HLT | JZ_DMA_DMAC_AR); > - jz4780_dma_writel(jzdma, JZ_DMA_REG_DMAC, dmac); > + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC, dmac); > > /* Clear interrupt pending status. */ > - jz4780_dma_writel(jzdma, JZ_DMA_REG_DIRQP, 0); > + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DIRQP, 0); > > return IRQ_HANDLED; > } > @@ -804,9 +818,19 @@ static int jz4780_dma_probe(struct platform_device *pdev) > return -EINVAL; > } > > - jzdma->base = devm_ioremap_resource(dev, res); > - if (IS_ERR(jzdma->base)) > - return PTR_ERR(jzdma->base); > + jzdma->chn_base = devm_ioremap_resource(dev, res); > + if (IS_ERR(jzdma->chn_base)) > + return PTR_ERR(jzdma->chn_base); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (!res) { > + dev_err(dev, "failed to get I/O memory\n"); > + return -EINVAL; > + } > + > + jzdma->ctrl_base = devm_ioremap_resource(dev, res); > + if (IS_ERR(jzdma->ctrl_base)) > + return PTR_ERR(jzdma->ctrl_base); > > ret = platform_get_irq(pdev, 0); > if (ret < 0) { > @@ -864,9 +888,9 @@ static int jz4780_dma_probe(struct platform_device *pdev) > * Also set the FMSC bit - it increases MSC performance, so it makes > * little sense not to enable it. > */ > - jz4780_dma_writel(jzdma, JZ_DMA_REG_DMAC, > + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC, > JZ_DMA_DMAC_DMAE | JZ_DMA_DMAC_FMSC); > - jz4780_dma_writel(jzdma, JZ_DMA_REG_DMACP, 0); > + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMACP, 0); > > INIT_LIST_HEAD(&dd->channels); > > -- > 2.18.0 > > Regards, PrasannaKumar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Paul, > Hi Paul, > > On Tue, Jul 03, 2018 at 02:32:02PM +0200, Paul Cercueil wrote: >> @@ -804,9 +818,19 @@ static int jz4780_dma_probe(struct >> platform_device *pdev) >> return -EINVAL; >> } >> >> - jzdma->base = devm_ioremap_resource(dev, res); >> - if (IS_ERR(jzdma->base)) >> - return PTR_ERR(jzdma->base); >> + jzdma->chn_base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(jzdma->chn_base)) >> + return PTR_ERR(jzdma->chn_base); >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); >> + if (!res) { >> + dev_err(dev, "failed to get I/O memory\n"); >> + return -EINVAL; >> + } >> + >> + jzdma->ctrl_base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(jzdma->ctrl_base)) >> + return PTR_ERR(jzdma->ctrl_base); > > Could we have this failure case fall back to the existing behaviour > where we only have a single resource covering all the registers? That > would avoid breaking bisection between this patch & the one that > updates > the JZ4780 DT. > > For example: > > #define JZ4780_DMA_CTRL_OFFSET 0x1000 > > res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > if (res) { > jzdma->ctrl_base = devm_ioremap_resource(dev, res); > if (IS_ERR(jzdma->ctrl_base)) > return PTR_ERR(jzdma->ctrl_base); > } else { > jzdma->ctrl_base = jzdma->chn_base + JZ4780_DMA_CTRL_OFFSET; > } > > Then you could remove the fallback after patch 13, to end up with the > same code you have now but without breaking bisection. > > Most correct might be to move patch 13 to right after this one, so > that > the JZ4780-specific fallback can be removed before adding support for > any of the other SoCs. Sure, I can do that for the V2. Thanks, -Paul -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Paul, > > On 3 July 2018 at 18:02, Paul Cercueil <paul@crapouillou.net> wrote: >> The register area of the JZ4780 DMA core can be split into different >> sections for different purposes: >> >> * one set of registers is used to perform actions at the DMA core >> level, >> that will generally affect all channels; >> >> * one set of registers per DMA channel, to perform actions at the >> DMA >> channel level, that will only affect the channel in question. >> >> The problem rises when trying to support new versions of the JZ47xx >> Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one >> with six DMA channels, and the register sets are interleaved: >> <DMA0 chan regs> <DMA1 chan regs> <DMA0 ctrl regs> <DMA1 ctrl regs> >> >> By using one memory resource for the channel-specific registers and >> one memory resource for the core-specific registers, we can support >> the JZ4770, by initializing the driver once per DMA core with >> different >> addresses. > > As per my understanding device tree should be modified only when > hardware changes. This looks the other way around. It must be possible > to achieve what you are trying to do in this patch without changing > the device tree. I would agree that devicetree has an ABI that we shouldn't break if possible. However DTS support for all the Ingenic SoCs/boards is far from being complete, and more importantly, all Ingenic-based boards compile the DTS file within the kernel; so breaking the ABI is not (yet) a problem, and we should push the big changes right now while it's still possible. >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> --- >> .../devicetree/bindings/dma/jz4780-dma.txt | 6 +- >> drivers/dma/dma-jz4780.c | 106 >> +++++++++++------- >> 2 files changed, 69 insertions(+), 43 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt >> b/Documentation/devicetree/bindings/dma/jz4780-dma.txt >> index f25feee62b15..f9b1864f5b77 100644 >> --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt >> +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt >> @@ -3,7 +3,8 @@ >> Required properties: >> >> - compatible: Should be "ingenic,jz4780-dma" >> -- reg: Should contain the DMA controller registers location and >> length. >> +- reg: Should contain the DMA channel registers location and >> length, followed >> + by the DMA controller registers location and length. >> - interrupts: Should contain the interrupt specifier of the DMA >> controller. >> - interrupt-parent: Should be the phandle of the interrupt >> controller that >> - clocks: Should contain a clock specifier for the JZ4780 PDMA >> clock. >> @@ -22,7 +23,8 @@ Example: >> >> dma: dma@13420000 { >> compatible = "ingenic,jz4780-dma"; >> - reg = <0x13420000 0x10000>; >> + reg = <0x13420000 0x400 >> + 0x13421000 0x40>; >> >> interrupt-parent = <&intc>; >> interrupts = <10>; >> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c >> index b40f491f0367..4d234caf5d62 100644 >> --- a/drivers/dma/dma-jz4780.c >> +++ b/drivers/dma/dma-jz4780.c >> @@ -25,26 +25,26 @@ >> #include "virt-dma.h" >> >> /* Global registers. */ >> -#define JZ_DMA_REG_DMAC 0x1000 >> -#define JZ_DMA_REG_DIRQP 0x1004 >> -#define JZ_DMA_REG_DDR 0x1008 >> -#define JZ_DMA_REG_DDRS 0x100c >> -#define JZ_DMA_REG_DMACP 0x101c >> -#define JZ_DMA_REG_DSIRQP 0x1020 >> -#define JZ_DMA_REG_DSIRQM 0x1024 >> -#define JZ_DMA_REG_DCIRQP 0x1028 >> -#define JZ_DMA_REG_DCIRQM 0x102c >> +#define JZ_DMA_REG_DMAC 0x00 >> +#define JZ_DMA_REG_DIRQP 0x04 >> +#define JZ_DMA_REG_DDR 0x08 >> +#define JZ_DMA_REG_DDRS 0x0c >> +#define JZ_DMA_REG_DMACP 0x1c >> +#define JZ_DMA_REG_DSIRQP 0x20 >> +#define JZ_DMA_REG_DSIRQM 0x24 >> +#define JZ_DMA_REG_DCIRQP 0x28 >> +#define JZ_DMA_REG_DCIRQM 0x2c >> >> /* Per-channel registers. */ >> #define JZ_DMA_REG_CHAN(n) (n * 0x20) >> -#define JZ_DMA_REG_DSA(n) (0x00 + JZ_DMA_REG_CHAN(n)) >> -#define JZ_DMA_REG_DTA(n) (0x04 + JZ_DMA_REG_CHAN(n)) >> -#define JZ_DMA_REG_DTC(n) (0x08 + JZ_DMA_REG_CHAN(n)) >> -#define JZ_DMA_REG_DRT(n) (0x0c + JZ_DMA_REG_CHAN(n)) >> -#define JZ_DMA_REG_DCS(n) (0x10 + JZ_DMA_REG_CHAN(n)) >> -#define JZ_DMA_REG_DCM(n) (0x14 + JZ_DMA_REG_CHAN(n)) >> -#define JZ_DMA_REG_DDA(n) (0x18 + JZ_DMA_REG_CHAN(n)) >> -#define JZ_DMA_REG_DSD(n) (0x1c + JZ_DMA_REG_CHAN(n)) >> +#define JZ_DMA_REG_DSA 0x00 >> +#define JZ_DMA_REG_DTA 0x04 >> +#define JZ_DMA_REG_DTC 0x08 >> +#define JZ_DMA_REG_DRT 0x0c >> +#define JZ_DMA_REG_DCS 0x10 >> +#define JZ_DMA_REG_DCM 0x14 >> +#define JZ_DMA_REG_DDA 0x18 >> +#define JZ_DMA_REG_DSD 0x1c >> >> #define JZ_DMA_DMAC_DMAE BIT(0) >> #define JZ_DMA_DMAC_AR BIT(2) >> @@ -140,7 +140,8 @@ enum jz_version { >> >> struct jz4780_dma_dev { >> struct dma_device dma_device; >> - void __iomem *base; >> + void __iomem *chn_base; >> + void __iomem *ctrl_base; >> struct clk *clk; >> unsigned int irq; >> unsigned int nb_channels; >> @@ -174,16 +175,28 @@ static inline struct jz4780_dma_dev >> *jz4780_dma_chan_parent( >> dma_device); >> } >> >> -static inline uint32_t jz4780_dma_readl(struct jz4780_dma_dev >> *jzdma, >> +static inline uint32_t jz4780_dma_chn_readl(struct jz4780_dma_dev >> *jzdma, >> + unsigned int chn, unsigned int reg) >> +{ >> + return readl(jzdma->chn_base + reg + JZ_DMA_REG_CHAN(chn)); >> +} >> + >> +static inline void jz4780_dma_chn_writel(struct jz4780_dma_dev >> *jzdma, >> + unsigned int chn, unsigned int reg, uint32_t val) >> +{ >> + writel(val, jzdma->chn_base + reg + JZ_DMA_REG_CHAN(chn)); >> +} >> + >> +static inline uint32_t jz4780_dma_ctrl_readl(struct jz4780_dma_dev >> *jzdma, >> unsigned int reg) >> { >> - return readl(jzdma->base + reg); >> + return readl(jzdma->ctrl_base + reg); >> } >> >> -static inline void jz4780_dma_writel(struct jz4780_dma_dev *jzdma, >> +static inline void jz4780_dma_ctrl_writel(struct jz4780_dma_dev >> *jzdma, >> unsigned int reg, uint32_t val) >> { >> - writel(val, jzdma->base + reg); >> + writel(val, jzdma->ctrl_base + reg); >> } >> >> static struct jz4780_dma_desc *jz4780_dma_desc_alloc( >> @@ -478,17 +491,18 @@ static void jz4780_dma_begin(struct >> jz4780_dma_chan *jzchan) >> } >> >> /* Use 8-word descriptors. */ >> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), >> JZ_DMA_DCS_DES8); >> + jz4780_dma_chn_writel(jzdma, jzchan->id, >> + JZ_DMA_REG_DCS, JZ_DMA_DCS_DES8); >> >> /* Write descriptor address and initiate descriptor fetch. >> */ >> desc_phys = jzchan->desc->desc_phys + >> (jzchan->curr_hwdesc * >> sizeof(*jzchan->desc->desc)); >> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DDA(jzchan->id), >> desc_phys); >> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DDRS, BIT(jzchan->id)); >> + jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DDA, >> desc_phys); >> + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DDRS, >> BIT(jzchan->id)); >> >> /* Enable the channel. */ >> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), >> - JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE); >> + jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, >> + JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE); >> } >> >> static void jz4780_dma_issue_pending(struct dma_chan *chan) >> @@ -514,7 +528,7 @@ static int jz4780_dma_terminate_all(struct >> dma_chan *chan) >> spin_lock_irqsave(&jzchan->vchan.lock, flags); >> >> /* Clear the DMA status and stop the transfer. */ >> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), 0); >> + jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0); >> if (jzchan->desc) { >> vchan_terminate_vdesc(&jzchan->desc->vdesc); >> jzchan->desc = NULL; >> @@ -563,8 +577,8 @@ static size_t jz4780_dma_desc_residue(struct >> jz4780_dma_chan *jzchan, >> residue += desc->desc[i].dtc << >> jzchan->transfer_shift; >> >> if (next_sg != 0) { >> - count = jz4780_dma_readl(jzdma, >> - >> JZ_DMA_REG_DTC(jzchan->id)); >> + count = jz4780_dma_chn_readl(jzdma, jzchan->id, >> + JZ_DMA_REG_DTC); >> residue += count << jzchan->transfer_shift; >> } >> >> @@ -611,8 +625,8 @@ static void jz4780_dma_chan_irq(struct >> jz4780_dma_dev *jzdma, >> >> spin_lock(&jzchan->vchan.lock); >> >> - dcs = jz4780_dma_readl(jzdma, JZ_DMA_REG_DCS(jzchan->id)); >> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), 0); >> + dcs = jz4780_dma_chn_readl(jzdma, jzchan->id, >> JZ_DMA_REG_DCS); >> + jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0); >> >> if (dcs & JZ_DMA_DCS_AR) { >> dev_warn(&jzchan->vchan.chan.dev->device, >> @@ -651,7 +665,7 @@ static irqreturn_t jz4780_dma_irq_handler(int >> irq, void *data) >> uint32_t pending, dmac; >> int i; >> >> - pending = jz4780_dma_readl(jzdma, JZ_DMA_REG_DIRQP); >> + pending = jz4780_dma_ctrl_readl(jzdma, JZ_DMA_REG_DIRQP); >> >> for (i = 0; i < jzdma->nb_channels; i++) { >> if (!(pending & (1<<i))) >> @@ -661,12 +675,12 @@ static irqreturn_t jz4780_dma_irq_handler(int >> irq, void *data) >> } >> >> /* Clear halt and address error status of all channels. */ >> - dmac = jz4780_dma_readl(jzdma, JZ_DMA_REG_DMAC); >> + dmac = jz4780_dma_ctrl_readl(jzdma, JZ_DMA_REG_DMAC); >> dmac &= ~(JZ_DMA_DMAC_HLT | JZ_DMA_DMAC_AR); >> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DMAC, dmac); >> + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC, dmac); >> >> /* Clear interrupt pending status. */ >> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DIRQP, 0); >> + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DIRQP, 0); >> >> return IRQ_HANDLED; >> } >> @@ -804,9 +818,19 @@ static int jz4780_dma_probe(struct >> platform_device *pdev) >> return -EINVAL; >> } >> >> - jzdma->base = devm_ioremap_resource(dev, res); >> - if (IS_ERR(jzdma->base)) >> - return PTR_ERR(jzdma->base); >> + jzdma->chn_base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(jzdma->chn_base)) >> + return PTR_ERR(jzdma->chn_base); >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); >> + if (!res) { >> + dev_err(dev, "failed to get I/O memory\n"); >> + return -EINVAL; >> + } >> + >> + jzdma->ctrl_base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(jzdma->ctrl_base)) >> + return PTR_ERR(jzdma->ctrl_base); >> >> ret = platform_get_irq(pdev, 0); >> if (ret < 0) { >> @@ -864,9 +888,9 @@ static int jz4780_dma_probe(struct >> platform_device *pdev) >> * Also set the FMSC bit - it increases MSC performance, so >> it makes >> * little sense not to enable it. >> */ >> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DMAC, >> + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC, >> JZ_DMA_DMAC_DMAE | JZ_DMA_DMAC_FMSC); >> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DMACP, 0); >> + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMACP, 0); >> >> INIT_LIST_HEAD(&dd->channels); >> >> -- >> 2.18.0 >> >> > > Regards, > PrasannaKumar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6 July 2018 at 03:15, Paul Cercueil <paul@crapouillou.net> wrote: > > >> Paul, >> >> On 3 July 2018 at 18:02, Paul Cercueil <paul@crapouillou.net> wrote: >>> >>> The register area of the JZ4780 DMA core can be split into different >>> sections for different purposes: >>> >>> * one set of registers is used to perform actions at the DMA core level, >>> that will generally affect all channels; >>> >>> * one set of registers per DMA channel, to perform actions at the DMA >>> channel level, that will only affect the channel in question. >>> >>> The problem rises when trying to support new versions of the JZ47xx >>> Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one >>> with six DMA channels, and the register sets are interleaved: >>> <DMA0 chan regs> <DMA1 chan regs> <DMA0 ctrl regs> <DMA1 ctrl regs> >>> >>> By using one memory resource for the channel-specific registers and >>> one memory resource for the core-specific registers, we can support >>> the JZ4770, by initializing the driver once per DMA core with different >>> addresses. >> >> >> As per my understanding device tree should be modified only when >> hardware changes. This looks the other way around. It must be possible >> to achieve what you are trying to do in this patch without changing >> the device tree. > > > I would agree that devicetree has an ABI that we shouldn't break if > possible. > > However DTS support for all the Ingenic SoCs/boards is far from being > complete, and more importantly, all Ingenic-based boards compile the DTS > file within the kernel; so breaking the ABI is not (yet) a problem, and > we should push the big changes right now while it's still possible. Completely agree with you in this. Let's wait and see what DT maintainer's view. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03-07-18, 14:32, Paul Cercueil wrote: > The register area of the JZ4780 DMA core can be split into different > sections for different purposes: > > * one set of registers is used to perform actions at the DMA core level, > that will generally affect all channels; > > * one set of registers per DMA channel, to perform actions at the DMA > channel level, that will only affect the channel in question. > > The problem rises when trying to support new versions of the JZ47xx > Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one > with six DMA channels, and the register sets are interleaved: > <DMA0 chan regs> <DMA1 chan regs> <DMA0 ctrl regs> <DMA1 ctrl regs> > > By using one memory resource for the channel-specific registers and > one memory resource for the core-specific registers, we can support > the JZ4770, by initializing the driver once per DMA core with different > addresses. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > .../devicetree/bindings/dma/jz4780-dma.txt | 6 +- Pls move to separate patch. > drivers/dma/dma-jz4780.c | 106 +++++++++++------- > 2 files changed, 69 insertions(+), 43 deletions(-) > > diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt b/Documentation/devicetree/bindings/dma/jz4780-dma.txt > index f25feee62b15..f9b1864f5b77 100644 > --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt > +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt > @@ -3,7 +3,8 @@ > Required properties: > > - compatible: Should be "ingenic,jz4780-dma" > -- reg: Should contain the DMA controller registers location and length. > +- reg: Should contain the DMA channel registers location and length, followed > + by the DMA controller registers location and length. > - interrupts: Should contain the interrupt specifier of the DMA controller. > - interrupt-parent: Should be the phandle of the interrupt controller that > - clocks: Should contain a clock specifier for the JZ4780 PDMA clock. > @@ -22,7 +23,8 @@ Example: > > dma: dma@13420000 { > compatible = "ingenic,jz4780-dma"; > - reg = <0x13420000 0x10000>; > + reg = <0x13420000 0x400 > + 0x13421000 0x40>; Second should be optional or we break platform which may not have updated DT.. > - jzdma->base = devm_ioremap_resource(dev, res); > - if (IS_ERR(jzdma->base)) > - return PTR_ERR(jzdma->base); > + jzdma->chn_base = devm_ioremap_resource(dev, res); > + if (IS_ERR(jzdma->chn_base)) > + return PTR_ERR(jzdma->chn_base); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (!res) { > + dev_err(dev, "failed to get I/O memory\n"); > + return -EINVAL; > + } okay and this breaks if you happen to get probed on older DT. I think DT is treated as ABI so you need to continue support older method while finding if DT has split resources
Le lun. 9 juil. 2018 à 19:03, Vinod <vkoul@kernel.org> a écrit : > On 03-07-18, 14:32, Paul Cercueil wrote: >> The register area of the JZ4780 DMA core can be split into different >> sections for different purposes: >> >> * one set of registers is used to perform actions at the DMA core >> level, >> that will generally affect all channels; >> >> * one set of registers per DMA channel, to perform actions at the >> DMA >> channel level, that will only affect the channel in question. >> >> The problem rises when trying to support new versions of the JZ47xx >> Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one >> with six DMA channels, and the register sets are interleaved: >> <DMA0 chan regs> <DMA1 chan regs> <DMA0 ctrl regs> <DMA1 ctrl regs> >> >> By using one memory resource for the channel-specific registers and >> one memory resource for the core-specific registers, we can support >> the JZ4770, by initializing the driver once per DMA core with >> different >> addresses. >> >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> --- >> .../devicetree/bindings/dma/jz4780-dma.txt | 6 +- > > Pls move to separate patch. OK. >> drivers/dma/dma-jz4780.c | 106 >> +++++++++++------- >> 2 files changed, 69 insertions(+), 43 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt >> b/Documentation/devicetree/bindings/dma/jz4780-dma.txt >> index f25feee62b15..f9b1864f5b77 100644 >> --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt >> +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt >> @@ -3,7 +3,8 @@ >> Required properties: >> >> - compatible: Should be "ingenic,jz4780-dma" >> -- reg: Should contain the DMA controller registers location and >> length. >> +- reg: Should contain the DMA channel registers location and >> length, followed >> + by the DMA controller registers location and length. >> - interrupts: Should contain the interrupt specifier of the DMA >> controller. >> - interrupt-parent: Should be the phandle of the interrupt >> controller that >> - clocks: Should contain a clock specifier for the JZ4780 PDMA >> clock. >> @@ -22,7 +23,8 @@ Example: >> >> dma: dma@13420000 { >> compatible = "ingenic,jz4780-dma"; >> - reg = <0x13420000 0x10000>; >> + reg = <0x13420000 0x400 >> + 0x13421000 0x40>; > > Second should be optional or we break platform which may not have > updated DT.. See comment below. >> - jzdma->base = devm_ioremap_resource(dev, res); >> - if (IS_ERR(jzdma->base)) >> - return PTR_ERR(jzdma->base); >> + jzdma->chn_base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(jzdma->chn_base)) >> + return PTR_ERR(jzdma->chn_base); >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); >> + if (!res) { >> + dev_err(dev, "failed to get I/O memory\n"); >> + return -EINVAL; >> + } > > okay and this breaks if you happen to get probed on older DT. I think > DT > is treated as ABI so you need to continue support older method while > finding if DT has split resources See my response to PrasannaKumar. All the Ingenic-based boards do compile the devicetree within the kernel, so I think it's still fine to add breaking changes. I'll wait on @Rob to give his point of view on this, though. (It's not something hard to change, but I'd like to know what's the policy in that case. I have other DT-breaking patches to submit) > -- > ~Vinod -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10-07-18, 17:36, Paul Cercueil wrote: > > > @@ -3,7 +3,8 @@ > > > Required properties: > > > > > > - compatible: Should be "ingenic,jz4780-dma" > > > -- reg: Should contain the DMA controller registers location and > > > length. > > > +- reg: Should contain the DMA channel registers location and > > > length, followed > > > + by the DMA controller registers location and length. > > > - interrupts: Should contain the interrupt specifier of the DMA > > > controller. > > > - interrupt-parent: Should be the phandle of the interrupt > > > controller that > > > - clocks: Should contain a clock specifier for the JZ4780 PDMA > > > clock. > > > @@ -22,7 +23,8 @@ Example: > > > > > > dma: dma@13420000 { > > > compatible = "ingenic,jz4780-dma"; > > > - reg = <0x13420000 0x10000>; > > > + reg = <0x13420000 0x400 > > > + 0x13421000 0x40>; > > > > Second should be optional or we break platform which may not have > > updated DT.. > > See comment below. > > > > - jzdma->base = devm_ioremap_resource(dev, res); > > > - if (IS_ERR(jzdma->base)) > > > - return PTR_ERR(jzdma->base); > > > + jzdma->chn_base = devm_ioremap_resource(dev, res); > > > + if (IS_ERR(jzdma->chn_base)) > > > + return PTR_ERR(jzdma->chn_base); > > > + > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > > + if (!res) { > > > + dev_err(dev, "failed to get I/O memory\n"); > > > + return -EINVAL; > > > + } > > > > okay and this breaks if you happen to get probed on older DT. I think DT > > is treated as ABI so you need to continue support older method while > > finding if DT has split resources > > See my response to PrasannaKumar. All the Ingenic-based boards do compile > the devicetree within the kernel, so I think it's still fine to add breaking > changes. I'll wait on @Rob to give his point of view on this, though. > > (It's not something hard to change, but I'd like to know what's the policy > in that case. I have other DT-breaking patches to submit) The policy is that DT is an ABI and should not break :) Who maintains Ingenic arch. MAINTAINERS doesn't tell me.
Le mer. 11 juil. 2018 à 14:16, Vinod <vkoul@kernel.org> a écrit : > On 10-07-18, 17:36, Paul Cercueil wrote: > >> > > @@ -3,7 +3,8 @@ >> > > Required properties: >> > > >> > > - compatible: Should be "ingenic,jz4780-dma" >> > > -- reg: Should contain the DMA controller registers location >> and >> > > length. >> > > +- reg: Should contain the DMA channel registers location and >> > > length, followed >> > > + by the DMA controller registers location and length. >> > > - interrupts: Should contain the interrupt specifier of the >> DMA >> > > controller. >> > > - interrupt-parent: Should be the phandle of the interrupt >> > > controller that >> > > - clocks: Should contain a clock specifier for the JZ4780 PDMA >> > > clock. >> > > @@ -22,7 +23,8 @@ Example: >> > > >> > > dma: dma@13420000 { >> > > compatible = "ingenic,jz4780-dma"; >> > > - reg = <0x13420000 0x10000>; >> > > + reg = <0x13420000 0x400 >> > > + 0x13421000 0x40>; >> > >> > Second should be optional or we break platform which may not have >> > updated DT.. >> >> See comment below. >> >> > > - jzdma->base = devm_ioremap_resource(dev, res); >> > > - if (IS_ERR(jzdma->base)) >> > > - return PTR_ERR(jzdma->base); >> > > + jzdma->chn_base = devm_ioremap_resource(dev, res); >> > > + if (IS_ERR(jzdma->chn_base)) >> > > + return PTR_ERR(jzdma->chn_base); >> > > + >> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); >> > > + if (!res) { >> > > + dev_err(dev, "failed to get I/O memory\n"); >> > > + return -EINVAL; >> > > + } >> > >> > okay and this breaks if you happen to get probed on older DT. I >> think DT >> > is treated as ABI so you need to continue support older method >> while >> > finding if DT has split resources >> >> See my response to PrasannaKumar. All the Ingenic-based boards do >> compile >> the devicetree within the kernel, so I think it's still fine to add >> breaking >> changes. I'll wait on @Rob to give his point of view on this, >> though. >> >> (It's not something hard to change, but I'd like to know what's the >> policy >> in that case. I have other DT-breaking patches to submit) > > The policy is that DT is an ABI and should not break :) > > Who maintains Ingenic arch. MAINTAINERS doesn't tell me. Unofficially that would be me :) Otherwise that would be the MIPS maintainers, Ralf and Paul (Burton). > -- > ~Vinod -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Vinod, On Wed, Jul 11, 2018 at 05:46:55PM +0530, Vinod wrote: > > > > - jzdma->base = devm_ioremap_resource(dev, res); > > > > - if (IS_ERR(jzdma->base)) > > > > - return PTR_ERR(jzdma->base); > > > > + jzdma->chn_base = devm_ioremap_resource(dev, res); > > > > + if (IS_ERR(jzdma->chn_base)) > > > > + return PTR_ERR(jzdma->chn_base); > > > > + > > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > > > + if (!res) { > > > > + dev_err(dev, "failed to get I/O memory\n"); > > > > + return -EINVAL; > > > > + } > > > > > > okay and this breaks if you happen to get probed on older DT. I think DT > > > is treated as ABI so you need to continue support older method while > > > finding if DT has split resources > > > > See my response to PrasannaKumar. All the Ingenic-based boards do compile > > the devicetree within the kernel, so I think it's still fine to add breaking > > changes. I'll wait on @Rob to give his point of view on this, though. > > > > (It's not something hard to change, but I'd like to know what's the policy > > in that case. I have other DT-breaking patches to submit) > > The policy is that DT is an ABI and should not break :) I think in general that's a good policy to have for compatibility, but if it's known for certain that the DT for all users of a driver is always built into the kernel then I don't see why we shouldn't feel free to change a binding. I agree with Paul that it'd be interesting to hear the DT binding maintainers take on this. Thanks, Paul -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 11, 2018 at 04:27:15PM -0700, Paul Burton wrote: > Hi Vinod, > > On Wed, Jul 11, 2018 at 05:46:55PM +0530, Vinod wrote: > > > > > - jzdma->base = devm_ioremap_resource(dev, res); > > > > > - if (IS_ERR(jzdma->base)) > > > > > - return PTR_ERR(jzdma->base); > > > > > + jzdma->chn_base = devm_ioremap_resource(dev, res); > > > > > + if (IS_ERR(jzdma->chn_base)) > > > > > + return PTR_ERR(jzdma->chn_base); > > > > > + > > > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > > > > + if (!res) { > > > > > + dev_err(dev, "failed to get I/O memory\n"); > > > > > + return -EINVAL; > > > > > + } > > > > > > > > okay and this breaks if you happen to get probed on older DT. I think DT > > > > is treated as ABI so you need to continue support older method while > > > > finding if DT has split resources > > > > > > See my response to PrasannaKumar. All the Ingenic-based boards do compile > > > the devicetree within the kernel, so I think it's still fine to add breaking > > > changes. I'll wait on @Rob to give his point of view on this, though. > > > > > > (It's not something hard to change, but I'd like to know what's the policy > > > in that case. I have other DT-breaking patches to submit) > > > > The policy is that DT is an ABI and should not break :) > > I think in general that's a good policy to have for compatibility, but > if it's known for certain that the DT for all users of a driver is > always built into the kernel then I don't see why we shouldn't feel free > to change a binding. I agree with Paul that it'd be interesting to hear > the DT binding maintainers take on this. If the platform maintainers (and their users) don't care, then I don't have an issue with the change. It should still be an exception and not just any change goes. The commit message should still highlight that compatibility is being broken and why. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt b/Documentation/devicetree/bindings/dma/jz4780-dma.txt index f25feee62b15..f9b1864f5b77 100644 --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt @@ -3,7 +3,8 @@ Required properties: - compatible: Should be "ingenic,jz4780-dma" -- reg: Should contain the DMA controller registers location and length. +- reg: Should contain the DMA channel registers location and length, followed + by the DMA controller registers location and length. - interrupts: Should contain the interrupt specifier of the DMA controller. - interrupt-parent: Should be the phandle of the interrupt controller that - clocks: Should contain a clock specifier for the JZ4780 PDMA clock. @@ -22,7 +23,8 @@ Example: dma: dma@13420000 { compatible = "ingenic,jz4780-dma"; - reg = <0x13420000 0x10000>; + reg = <0x13420000 0x400 + 0x13421000 0x40>; interrupt-parent = <&intc>; interrupts = <10>; diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c index b40f491f0367..4d234caf5d62 100644 --- a/drivers/dma/dma-jz4780.c +++ b/drivers/dma/dma-jz4780.c @@ -25,26 +25,26 @@ #include "virt-dma.h" /* Global registers. */ -#define JZ_DMA_REG_DMAC 0x1000 -#define JZ_DMA_REG_DIRQP 0x1004 -#define JZ_DMA_REG_DDR 0x1008 -#define JZ_DMA_REG_DDRS 0x100c -#define JZ_DMA_REG_DMACP 0x101c -#define JZ_DMA_REG_DSIRQP 0x1020 -#define JZ_DMA_REG_DSIRQM 0x1024 -#define JZ_DMA_REG_DCIRQP 0x1028 -#define JZ_DMA_REG_DCIRQM 0x102c +#define JZ_DMA_REG_DMAC 0x00 +#define JZ_DMA_REG_DIRQP 0x04 +#define JZ_DMA_REG_DDR 0x08 +#define JZ_DMA_REG_DDRS 0x0c +#define JZ_DMA_REG_DMACP 0x1c +#define JZ_DMA_REG_DSIRQP 0x20 +#define JZ_DMA_REG_DSIRQM 0x24 +#define JZ_DMA_REG_DCIRQP 0x28 +#define JZ_DMA_REG_DCIRQM 0x2c /* Per-channel registers. */ #define JZ_DMA_REG_CHAN(n) (n * 0x20) -#define JZ_DMA_REG_DSA(n) (0x00 + JZ_DMA_REG_CHAN(n)) -#define JZ_DMA_REG_DTA(n) (0x04 + JZ_DMA_REG_CHAN(n)) -#define JZ_DMA_REG_DTC(n) (0x08 + JZ_DMA_REG_CHAN(n)) -#define JZ_DMA_REG_DRT(n) (0x0c + JZ_DMA_REG_CHAN(n)) -#define JZ_DMA_REG_DCS(n) (0x10 + JZ_DMA_REG_CHAN(n)) -#define JZ_DMA_REG_DCM(n) (0x14 + JZ_DMA_REG_CHAN(n)) -#define JZ_DMA_REG_DDA(n) (0x18 + JZ_DMA_REG_CHAN(n)) -#define JZ_DMA_REG_DSD(n) (0x1c + JZ_DMA_REG_CHAN(n)) +#define JZ_DMA_REG_DSA 0x00 +#define JZ_DMA_REG_DTA 0x04 +#define JZ_DMA_REG_DTC 0x08 +#define JZ_DMA_REG_DRT 0x0c +#define JZ_DMA_REG_DCS 0x10 +#define JZ_DMA_REG_DCM 0x14 +#define JZ_DMA_REG_DDA 0x18 +#define JZ_DMA_REG_DSD 0x1c #define JZ_DMA_DMAC_DMAE BIT(0) #define JZ_DMA_DMAC_AR BIT(2) @@ -140,7 +140,8 @@ enum jz_version { struct jz4780_dma_dev { struct dma_device dma_device; - void __iomem *base; + void __iomem *chn_base; + void __iomem *ctrl_base; struct clk *clk; unsigned int irq; unsigned int nb_channels; @@ -174,16 +175,28 @@ static inline struct jz4780_dma_dev *jz4780_dma_chan_parent( dma_device); } -static inline uint32_t jz4780_dma_readl(struct jz4780_dma_dev *jzdma, +static inline uint32_t jz4780_dma_chn_readl(struct jz4780_dma_dev *jzdma, + unsigned int chn, unsigned int reg) +{ + return readl(jzdma->chn_base + reg + JZ_DMA_REG_CHAN(chn)); +} + +static inline void jz4780_dma_chn_writel(struct jz4780_dma_dev *jzdma, + unsigned int chn, unsigned int reg, uint32_t val) +{ + writel(val, jzdma->chn_base + reg + JZ_DMA_REG_CHAN(chn)); +} + +static inline uint32_t jz4780_dma_ctrl_readl(struct jz4780_dma_dev *jzdma, unsigned int reg) { - return readl(jzdma->base + reg); + return readl(jzdma->ctrl_base + reg); } -static inline void jz4780_dma_writel(struct jz4780_dma_dev *jzdma, +static inline void jz4780_dma_ctrl_writel(struct jz4780_dma_dev *jzdma, unsigned int reg, uint32_t val) { - writel(val, jzdma->base + reg); + writel(val, jzdma->ctrl_base + reg); } static struct jz4780_dma_desc *jz4780_dma_desc_alloc( @@ -478,17 +491,18 @@ static void jz4780_dma_begin(struct jz4780_dma_chan *jzchan) } /* Use 8-word descriptors. */ - jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), JZ_DMA_DCS_DES8); + jz4780_dma_chn_writel(jzdma, jzchan->id, + JZ_DMA_REG_DCS, JZ_DMA_DCS_DES8); /* Write descriptor address and initiate descriptor fetch. */ desc_phys = jzchan->desc->desc_phys + (jzchan->curr_hwdesc * sizeof(*jzchan->desc->desc)); - jz4780_dma_writel(jzdma, JZ_DMA_REG_DDA(jzchan->id), desc_phys); - jz4780_dma_writel(jzdma, JZ_DMA_REG_DDRS, BIT(jzchan->id)); + jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DDA, desc_phys); + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DDRS, BIT(jzchan->id)); /* Enable the channel. */ - jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), - JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE); + jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, + JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE); } static void jz4780_dma_issue_pending(struct dma_chan *chan) @@ -514,7 +528,7 @@ static int jz4780_dma_terminate_all(struct dma_chan *chan) spin_lock_irqsave(&jzchan->vchan.lock, flags); /* Clear the DMA status and stop the transfer. */ - jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), 0); + jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0); if (jzchan->desc) { vchan_terminate_vdesc(&jzchan->desc->vdesc); jzchan->desc = NULL; @@ -563,8 +577,8 @@ static size_t jz4780_dma_desc_residue(struct jz4780_dma_chan *jzchan, residue += desc->desc[i].dtc << jzchan->transfer_shift; if (next_sg != 0) { - count = jz4780_dma_readl(jzdma, - JZ_DMA_REG_DTC(jzchan->id)); + count = jz4780_dma_chn_readl(jzdma, jzchan->id, + JZ_DMA_REG_DTC); residue += count << jzchan->transfer_shift; } @@ -611,8 +625,8 @@ static void jz4780_dma_chan_irq(struct jz4780_dma_dev *jzdma, spin_lock(&jzchan->vchan.lock); - dcs = jz4780_dma_readl(jzdma, JZ_DMA_REG_DCS(jzchan->id)); - jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), 0); + dcs = jz4780_dma_chn_readl(jzdma, jzchan->id, JZ_DMA_REG_DCS); + jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0); if (dcs & JZ_DMA_DCS_AR) { dev_warn(&jzchan->vchan.chan.dev->device, @@ -651,7 +665,7 @@ static irqreturn_t jz4780_dma_irq_handler(int irq, void *data) uint32_t pending, dmac; int i; - pending = jz4780_dma_readl(jzdma, JZ_DMA_REG_DIRQP); + pending = jz4780_dma_ctrl_readl(jzdma, JZ_DMA_REG_DIRQP); for (i = 0; i < jzdma->nb_channels; i++) { if (!(pending & (1<<i))) @@ -661,12 +675,12 @@ static irqreturn_t jz4780_dma_irq_handler(int irq, void *data) } /* Clear halt and address error status of all channels. */ - dmac = jz4780_dma_readl(jzdma, JZ_DMA_REG_DMAC); + dmac = jz4780_dma_ctrl_readl(jzdma, JZ_DMA_REG_DMAC); dmac &= ~(JZ_DMA_DMAC_HLT | JZ_DMA_DMAC_AR); - jz4780_dma_writel(jzdma, JZ_DMA_REG_DMAC, dmac); + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC, dmac); /* Clear interrupt pending status. */ - jz4780_dma_writel(jzdma, JZ_DMA_REG_DIRQP, 0); + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DIRQP, 0); return IRQ_HANDLED; } @@ -804,9 +818,19 @@ static int jz4780_dma_probe(struct platform_device *pdev) return -EINVAL; } - jzdma->base = devm_ioremap_resource(dev, res); - if (IS_ERR(jzdma->base)) - return PTR_ERR(jzdma->base); + jzdma->chn_base = devm_ioremap_resource(dev, res); + if (IS_ERR(jzdma->chn_base)) + return PTR_ERR(jzdma->chn_base); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); + if (!res) { + dev_err(dev, "failed to get I/O memory\n"); + return -EINVAL; + } + + jzdma->ctrl_base = devm_ioremap_resource(dev, res); + if (IS_ERR(jzdma->ctrl_base)) + return PTR_ERR(jzdma->ctrl_base); ret = platform_get_irq(pdev, 0); if (ret < 0) { @@ -864,9 +888,9 @@ static int jz4780_dma_probe(struct platform_device *pdev) * Also set the FMSC bit - it increases MSC performance, so it makes * little sense not to enable it. */ - jz4780_dma_writel(jzdma, JZ_DMA_REG_DMAC, + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC, JZ_DMA_DMAC_DMAE | JZ_DMA_DMAC_FMSC); - jz4780_dma_writel(jzdma, JZ_DMA_REG_DMACP, 0); + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMACP, 0); INIT_LIST_HEAD(&dd->channels);
The register area of the JZ4780 DMA core can be split into different sections for different purposes: * one set of registers is used to perform actions at the DMA core level, that will generally affect all channels; * one set of registers per DMA channel, to perform actions at the DMA channel level, that will only affect the channel in question. The problem rises when trying to support new versions of the JZ47xx Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one with six DMA channels, and the register sets are interleaved: <DMA0 chan regs> <DMA1 chan regs> <DMA0 ctrl regs> <DMA1 ctrl regs> By using one memory resource for the channel-specific registers and one memory resource for the core-specific registers, we can support the JZ4770, by initializing the driver once per DMA core with different addresses. Signed-off-by: Paul Cercueil <paul@crapouillou.net> --- .../devicetree/bindings/dma/jz4780-dma.txt | 6 +- drivers/dma/dma-jz4780.c | 106 +++++++++++------- 2 files changed, 69 insertions(+), 43 deletions(-)