| Message ID | 20220307224750.18055-1-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/6] dmaengine: dw-edma: fix dw_edma_probe() can't be call globally | expand |
Hello Frank On Mon, Mar 07, 2022 at 04:47:45PM -0600, Frank Li wrote: > "struct dw_edma_chip" contains an internal structure "struct dw_edma" that is > used by the eDMA core internally. This structure should not be touched > by the eDMA controller drivers themselves. But currently, the eDMA > controller drivers like "dw-edma-pci" allocates and populates this > internal structure then passes it on to eDMA core. The eDMA core further > populates the structure and uses it. This is wrong! > > Hence, move all the "struct dw_edma" specifics from controller drivers > to the eDMA core. Thanks for the patchset. Alas it has just drawn my attention on v3 stage, otherwise I would have given to you my thoughts stright away on v1. Anyway first of all a cover letter would be very much appropriate to have a general notion about all the changes introduced in the set. Secondly I've just been working on adding the eDMA platform support myself, so you have been just about a week ahead of me submitting my changes. My work contains some of the modifications done by you (but have some additional fixes too) so I'll need to rebase it on top of your patchset when it's finished. Anyway am I understand correctly, that you've also been working on the DW PCIe driver alteration so one would properly initialize the eDMA-chip data structure? If so have you sent the patchset already? Could you give me a link and add me to Cc in the emailing thread? (That's where the cover letter with all the info and related patchsets would be very helpful.) Thirdly regarding this patch. Your modification is mainly correct, but I would suggest to change the concept. Instead of needlessly converting the code to using the dw_edma_chip structure pointer within the DW eDMA driver, it would be much easier in modification and more logical to keep using the struct dw_edma pointer. Especially seeing dw_edma structure is going to be a pure private data. So to speak what I would suggest is to have the next pointers setup: include/linux/dma/edma.h: struct dw_edma_chip { /* drop struct dw_edma *dw; */ }; drivers/dma/dw-edma/dw-edma-core.h: struct dw_edma { const struct dw_edma_chip *chip; }; struct dw_edma_chan { struct dw_edma *dw; }; Thus we'll have a cleaner concept here: struct dw_edma_chip - DW eDMA controller descriptor, defined and initialized in the client platform drivers. Can be static and const in general (which in your approach is impossible). struct dw_edma - pure DW eDMA driver private data. It will be used the within the local driver only and won't be seen outside. Thus you won't need an almost a half of the modifications below, would provide a cleaner interface, would support the const static DW eDMA chip info objects. More comments regarding the suggested approach and some additional notes are below. > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > > Resend added dmaengine@vger.kernel.org > > Change from v2 to v3 > - none > Change from v1 to v2 > - rework commit message > - remove duplicate field in struct dw_edma > > drivers/dma/dw-edma/dw-edma-core.c | 91 +++++----- > drivers/dma/dw-edma/dw-edma-core.h | 30 +--- > drivers/dma/dw-edma/dw-edma-v0-core.c | 206 ++++++++++++----------- > drivers/dma/dw-edma/dw-edma-v0-core.h | 8 +- > drivers/dma/dw-edma/dw-edma-v0-debugfs.c | 37 ++-- > include/linux/dma/edma.h | 47 +++++- > 6 files changed, 230 insertions(+), 189 deletions(-) > > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c > index 53289927dd0d6..0cb66434f9e14 100644 > --- a/drivers/dma/dw-edma/dw-edma-core.c > +++ b/drivers/dma/dw-edma/dw-edma-core.c > @@ -65,7 +65,7 @@ static struct dw_edma_burst *dw_edma_alloc_burst(struct dw_edma_chunk *chunk) > static struct dw_edma_chunk *dw_edma_alloc_chunk(struct dw_edma_desc *desc) > { > struct dw_edma_chan *chan = desc->chan; > - struct dw_edma *dw = chan->chip->dw; > + struct dw_edma_chip *chip = chan->chip; If you do as I suggest you won't need to modify this part. > struct dw_edma_chunk *chunk; > > chunk = kzalloc(sizeof(*chunk), GFP_NOWAIT); > @@ -82,11 +82,11 @@ static struct dw_edma_chunk *dw_edma_alloc_chunk(struct dw_edma_desc *desc) > */ > chunk->cb = !(desc->chunks_alloc % 2); > if (chan->dir == EDMA_DIR_WRITE) { > - chunk->ll_region.paddr = dw->ll_region_wr[chan->id].paddr; > - chunk->ll_region.vaddr = dw->ll_region_wr[chan->id].vaddr; > + chunk->ll_region.paddr = chip->ll_region_wr[chan->id].paddr; > + chunk->ll_region.vaddr = chip->ll_region_wr[chan->id].vaddr; > } else { > - chunk->ll_region.paddr = dw->ll_region_rd[chan->id].paddr; > - chunk->ll_region.vaddr = dw->ll_region_rd[chan->id].vaddr; > + chunk->ll_region.paddr = chip->ll_region_rd[chan->id].paddr; > + chunk->ll_region.vaddr = chip->ll_region_rd[chan->id].vaddr; > } Here you would just need to change the pointers "sandwich" to "dw->chip->...". > > if (desc->chunk) { > @@ -601,7 +601,8 @@ static void dw_edma_abort_interrupt(struct dw_edma_chan *chan) > static irqreturn_t dw_edma_interrupt(int irq, void *data, bool write) > { > struct dw_edma_irq *dw_irq = data; > - struct dw_edma *dw = dw_irq->dw; > + struct dw_edma_chip *chip = dw_irq->chip; > + struct dw_edma *dw = chip->dw; This also would have been unneeded, since the method relies on the data from the dw_edma structure. > unsigned long total, pos, val; > unsigned long off; > u32 mask; > @@ -616,7 +617,7 @@ static irqreturn_t dw_edma_interrupt(int irq, void *data, bool write) > mask = dw_irq->rd_mask; > } > > - val = dw_edma_v0_core_status_done_int(dw, write ? > + val = dw_edma_v0_core_status_done_int(chip, write ? > EDMA_DIR_WRITE : > EDMA_DIR_READ); > val &= mask; > @@ -626,7 +627,7 @@ static irqreturn_t dw_edma_interrupt(int irq, void *data, bool write) > dw_edma_done_interrupt(chan); > } > > - val = dw_edma_v0_core_status_abort_int(dw, write ? > + val = dw_edma_v0_core_status_abort_int(chip, write ? > EDMA_DIR_WRITE : > EDMA_DIR_READ); This won't be needed. > val &= mask; > @@ -718,7 +719,7 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write, > } > > INIT_LIST_HEAD(&dma->channels); > - for (j = 0; (alloc || dw->nr_irqs == 1) && j < cnt; j++, i++) { > + for (j = 0; (alloc || chip->nr_irqs == 1) && j < cnt; j++, i++) { > chan = &dw->chan[i]; > > dt_region = devm_kzalloc(dev, sizeof(*dt_region), GFP_KERNEL); > @@ -735,15 +736,15 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write, > chan->status = EDMA_ST_IDLE; > > if (write) > - chan->ll_max = (dw->ll_region_wr[j].sz / EDMA_LL_SZ); > + chan->ll_max = (chip->ll_region_wr[j].sz / EDMA_LL_SZ); > else > - chan->ll_max = (dw->ll_region_rd[j].sz / EDMA_LL_SZ); > + chan->ll_max = (chip->ll_region_rd[j].sz / EDMA_LL_SZ); > chan->ll_max -= 1; > > dev_vdbg(dev, "L. List:\tChannel %s[%u] max_cnt=%u\n", > write ? "write" : "read", j, chan->ll_max); > > - if (dw->nr_irqs == 1) > + if (chip->nr_irqs == 1) This would have been changed to using the "dw->chip->..." pattern. > pos = 0; > else > pos = off_alloc + (j % alloc); > @@ -755,7 +756,7 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write, > else > irq->rd_mask |= BIT(j); > > - irq->dw = dw; > + irq->chip = chip; Won't be needed. > memcpy(&chan->msi, &irq->msi, sizeof(chan->msi)); > > dev_vdbg(dev, "MSI:\t\tChannel %s[%u] addr=0x%.8x%.8x, data=0x%.8x\n", > @@ -767,13 +768,13 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write, > vchan_init(&chan->vc, dma); > > if (write) { > - dt_region->paddr = dw->dt_region_wr[j].paddr; > - dt_region->vaddr = dw->dt_region_wr[j].vaddr; > - dt_region->sz = dw->dt_region_wr[j].sz; > + dt_region->paddr = chip->dt_region_wr[j].paddr; > + dt_region->vaddr = chip->dt_region_wr[j].vaddr; > + dt_region->sz = chip->dt_region_wr[j].sz; > } else { > - dt_region->paddr = dw->dt_region_rd[j].paddr; > - dt_region->vaddr = dw->dt_region_rd[j].vaddr; > - dt_region->sz = dw->dt_region_rd[j].sz; > + dt_region->paddr = chip->dt_region_rd[j].paddr; > + dt_region->vaddr = chip->dt_region_rd[j].vaddr; > + dt_region->sz = chip->dt_region_rd[j].sz; Just replace with "dw->chip->..." > } > > dw_edma_v0_core_device_config(chan); > @@ -840,16 +841,16 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, > > ch_cnt = dw->wr_ch_cnt + dw->rd_ch_cnt; > > - if (dw->nr_irqs < 1) > + if (chip->nr_irqs < 1) > return -EINVAL; > > - if (dw->nr_irqs == 1) { > + if (chip->nr_irqs == 1) { > /* Common IRQ shared among all channels */ > - irq = dw->ops->irq_vector(dev, 0); > + irq = chip->ops->irq_vector(dev, 0); > err = request_irq(irq, dw_edma_interrupt_common, > IRQF_SHARED, dw->name, &dw->irq[0]); > if (err) { > - dw->nr_irqs = 0; > + chip->nr_irqs = 0; > return err; > } > > @@ -857,7 +858,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, > get_cached_msi_msg(irq, &dw->irq[0].msi); > } else { > /* Distribute IRQs equally among all channels */ > - int tmp = dw->nr_irqs; > + int tmp = chip->nr_irqs; > > while (tmp && (*wr_alloc + *rd_alloc) < ch_cnt) { > dw_edma_dec_irq_alloc(&tmp, wr_alloc, dw->wr_ch_cnt); > @@ -868,7 +869,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, > dw_edma_add_irq_mask(&rd_mask, *rd_alloc, dw->rd_ch_cnt); > > for (i = 0; i < (*wr_alloc + *rd_alloc); i++) { > - irq = dw->ops->irq_vector(dev, i); > + irq = chip->ops->irq_vector(dev, i); > err = request_irq(irq, > i < *wr_alloc ? > dw_edma_interrupt_write : > @@ -876,7 +877,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, > IRQF_SHARED, dw->name, > &dw->irq[i]); > if (err) { > - dw->nr_irqs = i; > + chip->nr_irqs = i; > return err; > } > > @@ -884,7 +885,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, > get_cached_msi_msg(irq, &dw->irq[i].msi); > } > > - dw->nr_irqs = i; > + chip->nr_irqs = i; ditto > } > > return err; > @@ -905,18 +906,24 @@ int dw_edma_probe(struct dw_edma_chip *chip) > if (!dev) > return -EINVAL; > > - dw = chip->dw; > - if (!dw || !dw->irq || !dw->ops || !dw->ops->irq_vector) > + dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL); > + if (!dw) > + return -ENOMEM; > + > + chip->dw = dw; Convert this to "dw->chip = chip". > + > + if (!chip->nr_irqs || !chip->ops) > return -EINVAL; Move this to be performed before the pointer initialization, since it's pointless to initialize anything if invalid data is passed. You can join it in with the "if (!dev)" statement. > > raw_spin_lock_init(&dw->lock); > > - dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt, > - dw_edma_v0_core_ch_count(dw, EDMA_DIR_WRITE)); > + > + dw->wr_ch_cnt = min_t(u16, chip->ll_wr_cnt, > + dw_edma_v0_core_ch_count(chip, EDMA_DIR_WRITE)); > dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt, EDMA_MAX_WR_CH); > > - dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt, > - dw_edma_v0_core_ch_count(dw, EDMA_DIR_READ)); > + dw->rd_ch_cnt = min_t(u16, chip->ll_rd_cnt, Regarding the field naming please see my comment to the dw_edma_chip structure. > + dw_edma_v0_core_ch_count(chip, EDMA_DIR_READ)); > dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt, EDMA_MAX_RD_CH); > > if (!dw->wr_ch_cnt && !dw->rd_ch_cnt) > @@ -934,7 +941,11 @@ int dw_edma_probe(struct dw_edma_chip *chip) > snprintf(dw->name, sizeof(dw->name), "dw-edma-core:%d", chip->id); > > /* Disable eDMA, only to establish the ideal initial conditions */ > - dw_edma_v0_core_off(dw); > + dw_edma_v0_core_off(chip); > + > + dw->irq = devm_kcalloc(dev, chip->nr_irqs, sizeof(*dw->irq), GFP_KERNEL); > + if (!dw->irq) > + return -ENOMEM; ditto > > /* Request IRQs */ > err = dw_edma_irq_request(chip, &wr_alloc, &rd_alloc); > @@ -960,10 +971,10 @@ int dw_edma_probe(struct dw_edma_chip *chip) > return 0; > > err_irq_free: > - for (i = (dw->nr_irqs - 1); i >= 0; i--) > - free_irq(dw->ops->irq_vector(dev, i), &dw->irq[i]); > + for (i = (chip->nr_irqs - 1); i >= 0; i--) > + free_irq(chip->ops->irq_vector(dev, i), &dw->irq[i]); > > - dw->nr_irqs = 0; > + chip->nr_irqs = 0; dw->chip->... > > return err; > } > @@ -977,11 +988,11 @@ int dw_edma_remove(struct dw_edma_chip *chip) > int i; > > /* Disable eDMA */ > - dw_edma_v0_core_off(dw); > + dw_edma_v0_core_off(chip); Won't need this. > > /* Free irqs */ > - for (i = (dw->nr_irqs - 1); i >= 0; i--) > - free_irq(dw->ops->irq_vector(dev, i), &dw->irq[i]); > + for (i = (chip->nr_irqs - 1); i >= 0; i--) > + free_irq(chip->ops->irq_vector(dev, i), &dw->irq[i]); Use "dw->chip->..." > > /* Power management */ > pm_runtime_disable(dev); > diff --git a/drivers/dma/dw-edma/dw-edma-core.h b/drivers/dma/dw-edma/dw-edma-core.h > index 60316d408c3e0..885f6719c9462 100644 > --- a/drivers/dma/dw-edma/dw-edma-core.h > +++ b/drivers/dma/dw-edma/dw-edma-core.h > @@ -15,20 +15,12 @@ > #include "../virt-dma.h" > > #define EDMA_LL_SZ 24 > -#define EDMA_MAX_WR_CH 8 > -#define EDMA_MAX_RD_CH 8 > > enum dw_edma_dir { > EDMA_DIR_WRITE = 0, > EDMA_DIR_READ > }; > > -enum dw_edma_map_format { > - EDMA_MF_EDMA_LEGACY = 0x0, > - EDMA_MF_EDMA_UNROLL = 0x1, > - EDMA_MF_HDMA_COMPAT = 0x5 > -}; > - > enum dw_edma_request { > EDMA_REQ_NONE = 0, > EDMA_REQ_STOP, > @@ -57,12 +49,6 @@ struct dw_edma_burst { > u32 sz; > }; > > -struct dw_edma_region { > - phys_addr_t paddr; > - void __iomem *vaddr; > - size_t sz; > -}; > - > struct dw_edma_chunk { > struct list_head list; > struct dw_edma_chan *chan; > @@ -106,11 +92,7 @@ struct dw_edma_irq { > struct msi_msg msi; > u32 wr_mask; > u32 rd_mask; > - struct dw_edma *dw; > -}; > - > -struct dw_edma_core_ops { > - int (*irq_vector)(struct device *dev, unsigned int nr); > + struct dw_edma_chip *chip; > }; > > struct dw_edma { > @@ -122,19 +104,9 @@ struct dw_edma { > struct dma_device rd_edma; > u16 rd_ch_cnt; > > - struct dw_edma_region rg_region; /* Registers */ AFAICS you replaced this with void __iomem *reg_base further in the dw_edma_chip structure. Even though I do agree with this modification, it's conceptually right, but the alteration is unrelated to the patch topic. So please unpin it to a dedicated patch placed before this one in the series. > - struct dw_edma_region ll_region_wr[EDMA_MAX_WR_CH]; > - struct dw_edma_region ll_region_rd[EDMA_MAX_RD_CH]; > - struct dw_edma_region dt_region_wr[EDMA_MAX_WR_CH]; > - struct dw_edma_region dt_region_rd[EDMA_MAX_RD_CH]; > - > struct dw_edma_irq *irq; > - int nr_irqs; > - > - enum dw_edma_map_format mf; > > struct dw_edma_chan *chan; > - const struct dw_edma_core_ops *ops; > > raw_spinlock_t lock; /* Only for legacy */ > #ifdef CONFIG_DEBUG_FS > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c > index 329fc2e57b703..6e2f83e31a03a 100644 > --- a/drivers/dma/dw-edma/dw-edma-v0-core.c > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c > @@ -23,92 +23,94 @@ enum dw_edma_control { > DW_EDMA_V0_LLE = BIT(9), > }; > > -static inline struct dw_edma_v0_regs __iomem *__dw_regs(struct dw_edma *dw) > +static inline struct dw_edma_v0_regs __iomem *__dw_regs(struct dw_edma_chip *chip) > { > - return dw->rg_region.vaddr; > + return chip->reg_base; > } Just use "dw->chip->reg_base" here and you won't need to introduce the most of the modifications below, since all of the them is connected with the __dw_regs() function prototype change. > > -#define SET_32(dw, name, value) \ > - writel(value, &(__dw_regs(dw)->name)) > +#define SET_32(chip, name, value) \ > + writel(value, &(__dw_regs(chip)->name)) > > -#define GET_32(dw, name) \ > - readl(&(__dw_regs(dw)->name)) > +#define GET_32(chip, name) \ > + readl(&(__dw_regs(chip)->name)) > > -#define SET_RW_32(dw, dir, name, value) \ > +#define SET_RW_32(chip, dir, name, value) \ > do { \ > if ((dir) == EDMA_DIR_WRITE) \ > - SET_32(dw, wr_##name, value); \ > + SET_32(chip, wr_##name, value); \ > else \ > - SET_32(dw, rd_##name, value); \ > + SET_32(chip, rd_##name, value); \ > } while (0) > > -#define GET_RW_32(dw, dir, name) \ > +#define GET_RW_32(chip, dir, name) \ > ((dir) == EDMA_DIR_WRITE \ > - ? GET_32(dw, wr_##name) \ > - : GET_32(dw, rd_##name)) > + ? GET_32(chip, wr_##name) \ > + : GET_32(chip, rd_##name)) > > -#define SET_BOTH_32(dw, name, value) \ > +#define SET_BOTH_32(chip, name, value) \ > do { \ > - SET_32(dw, wr_##name, value); \ > - SET_32(dw, rd_##name, value); \ > + SET_32(chip, wr_##name, value); \ > + SET_32(chip, rd_##name, value); \ > } while (0) > > #ifdef CONFIG_64BIT > > -#define SET_64(dw, name, value) \ > - writeq(value, &(__dw_regs(dw)->name)) > +#define SET_64(chip, name, value) \ > + writeq(value, &(__dw_regs(chip)->name)) > > -#define GET_64(dw, name) \ > - readq(&(__dw_regs(dw)->name)) > +#define GET_64(chip, name) \ > + readq(&(__dw_regs(chip)->name)) > > -#define SET_RW_64(dw, dir, name, value) \ > +#define SET_RW_64(chip, dir, name, value) \ > do { \ > if ((dir) == EDMA_DIR_WRITE) \ > - SET_64(dw, wr_##name, value); \ > + SET_64(chip, wr_##name, value); \ > else \ > - SET_64(dw, rd_##name, value); \ > + SET_64(chip, rd_##name, value); \ > } while (0) > > -#define GET_RW_64(dw, dir, name) \ > +#define GET_RW_64(chip, dir, name) \ > ((dir) == EDMA_DIR_WRITE \ > - ? GET_64(dw, wr_##name) \ > - : GET_64(dw, rd_##name)) > + ? GET_64(chip, wr_##name) \ > + : GET_64(chip, rd_##name)) > > -#define SET_BOTH_64(dw, name, value) \ > +#define SET_BOTH_64(chip, name, value) \ > do { \ > - SET_64(dw, wr_##name, value); \ > - SET_64(dw, rd_##name, value); \ > + SET_64(chip, wr_##name, value); \ > + SET_64(chip, rd_##name, value); \ > } while (0) > > #endif /* CONFIG_64BIT */ > > -#define SET_COMPAT(dw, name, value) \ > - writel(value, &(__dw_regs(dw)->type.unroll.name)) > +#define SET_COMPAT(chip, name, value) \ > + writel(value, &(__dw_regs(chip)->type.unroll.name)) > > -#define SET_RW_COMPAT(dw, dir, name, value) \ > +#define SET_RW_COMPAT(chip, dir, name, value) \ > do { \ > if ((dir) == EDMA_DIR_WRITE) \ > - SET_COMPAT(dw, wr_##name, value); \ > + SET_COMPAT(chip, wr_##name, value); \ > else \ > - SET_COMPAT(dw, rd_##name, value); \ > + SET_COMPAT(chip, rd_##name, value); \ > } while (0) > > static inline struct dw_edma_v0_ch_regs __iomem * > -__dw_ch_regs(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch) > +__dw_ch_regs(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch) > { > - if (dw->mf == EDMA_MF_EDMA_LEGACY) > - return &(__dw_regs(dw)->type.legacy.ch); > + if (chip->mf == EDMA_MF_EDMA_LEGACY) > + return &(__dw_regs(chip)->type.legacy.ch); > > if (dir == EDMA_DIR_WRITE) > - return &__dw_regs(dw)->type.unroll.ch[ch].wr; > + return &__dw_regs(chip)->type.unroll.ch[ch].wr; > > - return &__dw_regs(dw)->type.unroll.ch[ch].rd; > + return &__dw_regs(chip)->type.unroll.ch[ch].rd; > } > > -static inline void writel_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > +static inline void writel_ch(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch, > u32 value, void __iomem *addr) > { > - if (dw->mf == EDMA_MF_EDMA_LEGACY) { > + struct dw_edma *dw = chip->dw; > + > + if (chip->mf == EDMA_MF_EDMA_LEGACY) { > u32 viewport_sel; > unsigned long flags; > > @@ -119,7 +121,7 @@ static inline void writel_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > viewport_sel |= BIT(31); > > writel(viewport_sel, > - &(__dw_regs(dw)->type.legacy.viewport_sel)); > + &(__dw_regs(chip)->type.legacy.viewport_sel)); > writel(value, addr); > > raw_spin_unlock_irqrestore(&dw->lock, flags); > @@ -128,12 +130,13 @@ static inline void writel_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > } > } > > -static inline u32 readl_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > +static inline u32 readl_ch(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch, > const void __iomem *addr) > { > + struct dw_edma *dw = chip->dw; > u32 value; > > - if (dw->mf == EDMA_MF_EDMA_LEGACY) { > + if (chip->mf == EDMA_MF_EDMA_LEGACY) { > u32 viewport_sel; > unsigned long flags; > > @@ -144,7 +147,7 @@ static inline u32 readl_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > viewport_sel |= BIT(31); > > writel(viewport_sel, > - &(__dw_regs(dw)->type.legacy.viewport_sel)); > + &(__dw_regs(chip)->type.legacy.viewport_sel)); > value = readl(addr); > > raw_spin_unlock_irqrestore(&dw->lock, flags); > @@ -166,10 +169,12 @@ static inline u32 readl_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > #ifdef CONFIG_64BIT > > -static inline void writeq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > +static inline void writeq_ch(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch, > u64 value, void __iomem *addr) > { > - if (dw->mf == EDMA_MF_EDMA_LEGACY) { > + struct dw_edma *dw = chip->dw; > + > + if (chip->mf == EDMA_MF_EDMA_LEGACY) { > u32 viewport_sel; > unsigned long flags; > > @@ -180,7 +185,7 @@ static inline void writeq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > viewport_sel |= BIT(31); > > writel(viewport_sel, > - &(__dw_regs(dw)->type.legacy.viewport_sel)); > + &(__dw_regs(chip)->type.legacy.viewport_sel)); > writeq(value, addr); > > raw_spin_unlock_irqrestore(&dw->lock, flags); > @@ -189,12 +194,13 @@ static inline void writeq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > } > } > > -static inline u64 readq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > +static inline u64 readq_ch(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch, > const void __iomem *addr) > { > + struct dw_edma *dw = chip->dw; > u32 value; > > - if (dw->mf == EDMA_MF_EDMA_LEGACY) { > + if (chip->mf == EDMA_MF_EDMA_LEGACY) { > u32 viewport_sel; > unsigned long flags; > > @@ -205,7 +211,7 @@ static inline u64 readq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > viewport_sel |= BIT(31); > > writel(viewport_sel, > - &(__dw_regs(dw)->type.legacy.viewport_sel)); > + &(__dw_regs(chip)->type.legacy.viewport_sel)); > value = readq(addr); > > raw_spin_unlock_irqrestore(&dw->lock, flags); > @@ -228,25 +234,25 @@ static inline u64 readq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > #endif /* CONFIG_64BIT */ > > /* eDMA management callbacks */ > -void dw_edma_v0_core_off(struct dw_edma *dw) > +void dw_edma_v0_core_off(struct dw_edma_chip *chip) > { > - SET_BOTH_32(dw, int_mask, > + SET_BOTH_32(chip, int_mask, > EDMA_V0_DONE_INT_MASK | EDMA_V0_ABORT_INT_MASK); > - SET_BOTH_32(dw, int_clear, > + SET_BOTH_32(chip, int_clear, > EDMA_V0_DONE_INT_MASK | EDMA_V0_ABORT_INT_MASK); > - SET_BOTH_32(dw, engine_en, 0); > + SET_BOTH_32(chip, engine_en, 0); > } > > -u16 dw_edma_v0_core_ch_count(struct dw_edma *dw, enum dw_edma_dir dir) > +u16 dw_edma_v0_core_ch_count(struct dw_edma_chip *chip, enum dw_edma_dir dir) > { > u32 num_ch; > > if (dir == EDMA_DIR_WRITE) > num_ch = FIELD_GET(EDMA_V0_WRITE_CH_COUNT_MASK, > - GET_32(dw, ctrl)); > + GET_32(chip, ctrl)); > else > num_ch = FIELD_GET(EDMA_V0_READ_CH_COUNT_MASK, > - GET_32(dw, ctrl)); > + GET_32(chip, ctrl)); > > if (num_ch > EDMA_V0_MAX_NR_CH) > num_ch = EDMA_V0_MAX_NR_CH; > @@ -256,11 +262,11 @@ u16 dw_edma_v0_core_ch_count(struct dw_edma *dw, enum dw_edma_dir dir) So can omit most of the changes from the comment above and up to this comment and use "dw->chip->..." where it's required. > > enum dma_status dw_edma_v0_core_ch_status(struct dw_edma_chan *chan) > { > - struct dw_edma *dw = chan->chip->dw; > + struct dw_edma_chip *chip = chan->chip; Just use "dw = chan->dw" here or directly use "chan->dw" in the method. > u32 tmp; > > tmp = FIELD_GET(EDMA_V0_CH_STATUS_MASK, > - GET_CH_32(dw, chan->dir, chan->id, ch_control1)); > + GET_CH_32(chip, chan->dir, chan->id, ch_control1)); > > if (tmp == 1) > return DMA_IN_PROGRESS; > @@ -272,30 +278,30 @@ enum dma_status dw_edma_v0_core_ch_status(struct dw_edma_chan *chan) > > void dw_edma_v0_core_clear_done_int(struct dw_edma_chan *chan) > { > - struct dw_edma *dw = chan->chip->dw; > + struct dw_edma_chip *chip = chan->chip; ditto > > - SET_RW_32(dw, chan->dir, int_clear, > + SET_RW_32(chip, chan->dir, int_clear, > FIELD_PREP(EDMA_V0_DONE_INT_MASK, BIT(chan->id))); > } > > void dw_edma_v0_core_clear_abort_int(struct dw_edma_chan *chan) > { > - struct dw_edma *dw = chan->chip->dw; > + struct dw_edma_chip *chip = chan->chip; ditto > > - SET_RW_32(dw, chan->dir, int_clear, > + SET_RW_32(chip, chan->dir, int_clear, > FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id))); > } > > -u32 dw_edma_v0_core_status_done_int(struct dw_edma *dw, enum dw_edma_dir dir) > +u32 dw_edma_v0_core_status_done_int(struct dw_edma_chip *chip, enum dw_edma_dir dir) > { > return FIELD_GET(EDMA_V0_DONE_INT_MASK, > - GET_RW_32(dw, dir, int_status)); > + GET_RW_32(chip, dir, int_status)); > } > > -u32 dw_edma_v0_core_status_abort_int(struct dw_edma *dw, enum dw_edma_dir dir) > +u32 dw_edma_v0_core_status_abort_int(struct dw_edma_chip *chip, enum dw_edma_dir dir) > { > return FIELD_GET(EDMA_V0_ABORT_INT_MASK, > - GET_RW_32(dw, dir, int_status)); > + GET_RW_32(chip, dir, int_status)); Won't be needed > } > > static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk) > @@ -357,109 +363,109 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk) > void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first) > { > struct dw_edma_chan *chan = chunk->chan; > - struct dw_edma *dw = chan->chip->dw; > + struct dw_edma_chip *chip = chan->chip; Just replace with "dw = chan->dw" or directly use "chan->dw" in the method. Thus you can omit the alterations below. > u32 tmp; > > dw_edma_v0_core_write_chunk(chunk); > > if (first) { > /* Enable engine */ > - SET_RW_32(dw, chan->dir, engine_en, BIT(0)); > - if (dw->mf == EDMA_MF_HDMA_COMPAT) { > + SET_RW_32(chip, chan->dir, engine_en, BIT(0)); > + if (chip->mf == EDMA_MF_HDMA_COMPAT) { > switch (chan->id) { > case 0: > - SET_RW_COMPAT(dw, chan->dir, ch0_pwr_en, > + SET_RW_COMPAT(chip, chan->dir, ch0_pwr_en, > BIT(0)); > break; > case 1: > - SET_RW_COMPAT(dw, chan->dir, ch1_pwr_en, > + SET_RW_COMPAT(chip, chan->dir, ch1_pwr_en, > BIT(0)); > break; > case 2: > - SET_RW_COMPAT(dw, chan->dir, ch2_pwr_en, > + SET_RW_COMPAT(chip, chan->dir, ch2_pwr_en, > BIT(0)); > break; > case 3: > - SET_RW_COMPAT(dw, chan->dir, ch3_pwr_en, > + SET_RW_COMPAT(chip, chan->dir, ch3_pwr_en, > BIT(0)); > break; > case 4: > - SET_RW_COMPAT(dw, chan->dir, ch4_pwr_en, > + SET_RW_COMPAT(chip, chan->dir, ch4_pwr_en, > BIT(0)); > break; > case 5: > - SET_RW_COMPAT(dw, chan->dir, ch5_pwr_en, > + SET_RW_COMPAT(chip, chan->dir, ch5_pwr_en, > BIT(0)); > break; > case 6: > - SET_RW_COMPAT(dw, chan->dir, ch6_pwr_en, > + SET_RW_COMPAT(chip, chan->dir, ch6_pwr_en, > BIT(0)); > break; > case 7: > - SET_RW_COMPAT(dw, chan->dir, ch7_pwr_en, > + SET_RW_COMPAT(chip, chan->dir, ch7_pwr_en, > BIT(0)); > break; > } > } > /* Interrupt unmask - done, abort */ > - tmp = GET_RW_32(dw, chan->dir, int_mask); > + tmp = GET_RW_32(chip, chan->dir, int_mask); > tmp &= ~FIELD_PREP(EDMA_V0_DONE_INT_MASK, BIT(chan->id)); > tmp &= ~FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id)); > - SET_RW_32(dw, chan->dir, int_mask, tmp); > + SET_RW_32(chip, chan->dir, int_mask, tmp); > /* Linked list error */ > - tmp = GET_RW_32(dw, chan->dir, linked_list_err_en); > + tmp = GET_RW_32(chip, chan->dir, linked_list_err_en); > tmp |= FIELD_PREP(EDMA_V0_LINKED_LIST_ERR_MASK, BIT(chan->id)); > - SET_RW_32(dw, chan->dir, linked_list_err_en, tmp); > + SET_RW_32(chip, chan->dir, linked_list_err_en, tmp); > /* Channel control */ > - SET_CH_32(dw, chan->dir, chan->id, ch_control1, > + SET_CH_32(chip, chan->dir, chan->id, ch_control1, > (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE)); > /* Linked list */ > #ifdef CONFIG_64BIT > - SET_CH_64(dw, chan->dir, chan->id, llp.reg, > + SET_CH_64(chip, chan->dir, chan->id, llp.reg, > chunk->ll_region.paddr); > #else /* CONFIG_64BIT */ > - SET_CH_32(dw, chan->dir, chan->id, llp.lsb, > + SET_CH_32(chip, chan->dir, chan->id, llp.lsb, > lower_32_bits(chunk->ll_region.paddr)); > - SET_CH_32(dw, chan->dir, chan->id, llp.msb, > + SET_CH_32(chip, chan->dir, chan->id, llp.msb, > upper_32_bits(chunk->ll_region.paddr)); > #endif /* CONFIG_64BIT */ > } > /* Doorbell */ > - SET_RW_32(dw, chan->dir, doorbell, > + SET_RW_32(chip, chan->dir, doorbell, > FIELD_PREP(EDMA_V0_DOORBELL_CH_MASK, chan->id)); > } You can drop the changes from the previous comment up to this one. > > int dw_edma_v0_core_device_config(struct dw_edma_chan *chan) > { > - struct dw_edma *dw = chan->chip->dw; > + struct dw_edma_chip *chip = chan->chip; Use "chan->dw" here. > u32 tmp = 0; > > /* MSI done addr - low, high */ > - SET_RW_32(dw, chan->dir, done_imwr.lsb, chan->msi.address_lo); > - SET_RW_32(dw, chan->dir, done_imwr.msb, chan->msi.address_hi); > + SET_RW_32(chip, chan->dir, done_imwr.lsb, chan->msi.address_lo); > + SET_RW_32(chip, chan->dir, done_imwr.msb, chan->msi.address_hi); > /* MSI abort addr - low, high */ > - SET_RW_32(dw, chan->dir, abort_imwr.lsb, chan->msi.address_lo); > - SET_RW_32(dw, chan->dir, abort_imwr.msb, chan->msi.address_hi); > + SET_RW_32(chip, chan->dir, abort_imwr.lsb, chan->msi.address_lo); > + SET_RW_32(chip, chan->dir, abort_imwr.msb, chan->msi.address_hi); > /* MSI data - low, high */ > switch (chan->id) { > case 0: > case 1: > - tmp = GET_RW_32(dw, chan->dir, ch01_imwr_data); > + tmp = GET_RW_32(chip, chan->dir, ch01_imwr_data); > break; > > case 2: > case 3: > - tmp = GET_RW_32(dw, chan->dir, ch23_imwr_data); > + tmp = GET_RW_32(chip, chan->dir, ch23_imwr_data); > break; > > case 4: > case 5: > - tmp = GET_RW_32(dw, chan->dir, ch45_imwr_data); > + tmp = GET_RW_32(chip, chan->dir, ch45_imwr_data); > break; > > case 6: > case 7: > - tmp = GET_RW_32(dw, chan->dir, ch67_imwr_data); > + tmp = GET_RW_32(chip, chan->dir, ch67_imwr_data); > break; > } > > @@ -478,22 +484,22 @@ int dw_edma_v0_core_device_config(struct dw_edma_chan *chan) > switch (chan->id) { > case 0: > case 1: > - SET_RW_32(dw, chan->dir, ch01_imwr_data, tmp); > + SET_RW_32(chip, chan->dir, ch01_imwr_data, tmp); > break; > > case 2: > case 3: > - SET_RW_32(dw, chan->dir, ch23_imwr_data, tmp); > + SET_RW_32(chip, chan->dir, ch23_imwr_data, tmp); > break; > > case 4: > case 5: > - SET_RW_32(dw, chan->dir, ch45_imwr_data, tmp); > + SET_RW_32(chip, chan->dir, ch45_imwr_data, tmp); > break; > > case 6: > case 7: > - SET_RW_32(dw, chan->dir, ch67_imwr_data, tmp); > + SET_RW_32(chip, chan->dir, ch67_imwr_data, tmp); > break; The changes above won't be needed if you keep using the dw pointer here as I suggest. > } > > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.h b/drivers/dma/dw-edma/dw-edma-v0-core.h > index 2afa626b8300c..01a29c74c0c43 100644 > --- a/drivers/dma/dw-edma/dw-edma-v0-core.h > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.h > @@ -12,13 +12,13 @@ > #include <linux/dma/edma.h> > > /* eDMA management callbacks */ > -void dw_edma_v0_core_off(struct dw_edma *chan); > -u16 dw_edma_v0_core_ch_count(struct dw_edma *chan, enum dw_edma_dir dir); > +void dw_edma_v0_core_off(struct dw_edma_chip *chip); > +u16 dw_edma_v0_core_ch_count(struct dw_edma_chip *chip, enum dw_edma_dir dir); > enum dma_status dw_edma_v0_core_ch_status(struct dw_edma_chan *chan); > void dw_edma_v0_core_clear_done_int(struct dw_edma_chan *chan); > void dw_edma_v0_core_clear_abort_int(struct dw_edma_chan *chan); > -u32 dw_edma_v0_core_status_done_int(struct dw_edma *chan, enum dw_edma_dir dir); > -u32 dw_edma_v0_core_status_abort_int(struct dw_edma *chan, enum dw_edma_dir dir); > +u32 dw_edma_v0_core_status_done_int(struct dw_edma_chip *chip, enum dw_edma_dir dir); > +u32 dw_edma_v0_core_status_abort_int(struct dw_edma_chip *chip, enum dw_edma_dir dir); This modification won't be needed. > void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first); > int dw_edma_v0_core_device_config(struct dw_edma_chan *chan); > /* eDMA debug fs callbacks */ > diff --git a/drivers/dma/dw-edma/dw-edma-v0-debugfs.c b/drivers/dma/dw-edma/dw-edma-v0-debugfs.c > index 4b3bcffd15ef1..5819a64aceb0f 100644 > --- a/drivers/dma/dw-edma/dw-edma-v0-debugfs.c > +++ b/drivers/dma/dw-edma/dw-edma-v0-debugfs.c > @@ -38,7 +38,7 @@ > #define CHANNEL_STR "channel" > #define REGISTERS_STR "registers" > > -static struct dw_edma *dw; > +static struct dw_edma_chip *chip; Hmmm, why on Earth is this static and global?.. What if we have more than one host/EP controller with eDMA?.. Nice. I'll think on fixing this if you don't want to bother... > static struct dw_edma_v0_regs __iomem *regs; > > static struct { > @@ -53,8 +53,10 @@ struct debugfs_entries { > > static int dw_edma_debugfs_u32_get(void *data, u64 *val) > { > + struct dw_edma *dw = chip->dw; > + > void __iomem *reg = (void __force __iomem *)data; > - if (dw->mf == EDMA_MF_EDMA_LEGACY && > + if (chip->mf == EDMA_MF_EDMA_LEGACY && Use "dw->chip-> ..." here > reg >= (void __iomem *)®s->type.legacy.ch) { > void __iomem *ptr = ®s->type.legacy.ch; > u32 viewport_sel = 0; > @@ -127,6 +129,8 @@ static void dw_edma_debugfs_regs_ch(struct dw_edma_v0_ch_regs __iomem *regs, > > static void dw_edma_debugfs_regs_wr(struct dentry *dir) > { > + struct dw_edma *dw = chip->dw; > + > const struct debugfs_entries debugfs_regs[] = { > /* eDMA global registers */ > WR_REGISTER(engine_en), > @@ -173,7 +177,7 @@ static void dw_edma_debugfs_regs_wr(struct dentry *dir) > nr_entries = ARRAY_SIZE(debugfs_regs); > dw_edma_debugfs_create_x32(debugfs_regs, nr_entries, regs_dir); > > - if (dw->mf == EDMA_MF_HDMA_COMPAT) { > + if (chip->mf == EDMA_MF_HDMA_COMPAT) { ditto > nr_entries = ARRAY_SIZE(debugfs_unroll_regs); > dw_edma_debugfs_create_x32(debugfs_unroll_regs, nr_entries, > regs_dir); > @@ -195,6 +199,8 @@ static void dw_edma_debugfs_regs_wr(struct dentry *dir) > > static void dw_edma_debugfs_regs_rd(struct dentry *dir) > { > + struct dw_edma *dw = chip->dw; > + > const struct debugfs_entries debugfs_regs[] = { > /* eDMA global registers */ > RD_REGISTER(engine_en), > @@ -242,7 +248,7 @@ static void dw_edma_debugfs_regs_rd(struct dentry *dir) > nr_entries = ARRAY_SIZE(debugfs_regs); > dw_edma_debugfs_create_x32(debugfs_regs, nr_entries, regs_dir); > > - if (dw->mf == EDMA_MF_HDMA_COMPAT) { > + if (chip->mf == EDMA_MF_HDMA_COMPAT) { ditto > nr_entries = ARRAY_SIZE(debugfs_unroll_regs); > dw_edma_debugfs_create_x32(debugfs_unroll_regs, nr_entries, > regs_dir); > @@ -264,6 +270,7 @@ static void dw_edma_debugfs_regs_rd(struct dentry *dir) > > static void dw_edma_debugfs_regs(void) > { > + struct dw_edma *dw = chip->dw; > const struct debugfs_entries debugfs_regs[] = { > REGISTER(ctrl_data_arb_prior), > REGISTER(ctrl), > @@ -282,13 +289,15 @@ static void dw_edma_debugfs_regs(void) > dw_edma_debugfs_regs_rd(regs_dir); > } > > -void dw_edma_v0_debugfs_on(struct dw_edma_chip *chip) > +void dw_edma_v0_debugfs_on(struct dw_edma_chip *p) > { > - dw = chip->dw; > - if (!dw) > + struct dw_edma *dw; > + chip = p; > + if (!chip) > return; > > - regs = dw->rg_region.vaddr; > + dw = chip->dw; > + regs = chip->reg_base; As I said this is unrelated change. Please unpin to another patch. > if (!regs) > return; > > @@ -296,19 +305,19 @@ void dw_edma_v0_debugfs_on(struct dw_edma_chip *chip) > if (!dw->debugfs) > return; > > - debugfs_create_u32("mf", 0444, dw->debugfs, &dw->mf); > + debugfs_create_u32("mf", 0444, dw->debugfs, &chip->mf); "dw->chip->..." > debugfs_create_u16("wr_ch_cnt", 0444, dw->debugfs, &dw->wr_ch_cnt); > debugfs_create_u16("rd_ch_cnt", 0444, dw->debugfs, &dw->rd_ch_cnt); > > dw_edma_debugfs_regs(); > } > > -void dw_edma_v0_debugfs_off(struct dw_edma_chip *chip) > +void dw_edma_v0_debugfs_off(struct dw_edma_chip *p) > { > - dw = chip->dw; > - if (!dw) > + chip = p; > + if (!chip) > return; > > - debugfs_remove_recursive(dw->debugfs); > - dw->debugfs = NULL; > + debugfs_remove_recursive(chip->dw->debugfs); This won't be needed. > + chip->dw->debugfs = NULL; > } > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h > index cab6e18773dad..fcfbc0f47f83d 100644 > --- a/include/linux/dma/edma.h > +++ b/include/linux/dma/edma.h > @@ -12,19 +12,62 @@ > #include <linux/device.h> > #include <linux/dmaengine.h> > > +#define EDMA_MAX_WR_CH 8 > +#define EDMA_MAX_RD_CH 8 > + > struct dw_edma; This could be dropped. > > +struct dw_edma_region { > + phys_addr_t paddr; > + void __iomem *vaddr; > + size_t sz; > +}; > + > +struct dw_edma_core_ops { > + int (*irq_vector)(struct device *dev, unsigned int nr); > +}; > + > +enum dw_edma_map_format { > + EDMA_MF_EDMA_LEGACY = 0x0, > + EDMA_MF_EDMA_UNROLL = 0x1, > + EDMA_MF_HDMA_COMPAT = 0x5 > +}; > + > /** > * struct dw_edma_chip - representation of DesignWare eDMA controller hardware > * @dev: struct device of the eDMA controller > * @id: instance ID > - * @irq: irq line > + * @nr_irqs: total dma irq number Indeed, dw_edma_chip->irq field has been unused anyway... > + * reg64bit if support 64bit write to register Do you have this field in the structure below? I don't see it there. Drop this line then. > + * @ops DMA channel to IRQ number mapping > + * @wr_ch_cnt DMA write channel number > + * @rd_ch_cnt DMA read channel number > + * @rg_region DMA register region You changed this to reg_base in the structure below, but the doc has been left with the old name and field type. Please unpin the modification to a dedicated patch as I suggested before. > + * @ll_region_wr DMA descriptor link list memory for write channel > + * @ll_region_rd DMA descriptor link list memory for read channel > + * @mf DMA register map format > * @dw: struct dw_edma that is filed by dw_edma_probe() > */ > struct dw_edma_chip { > struct device *dev; > int id; > - int irq; > + int nr_irqs; > + const struct dw_edma_core_ops *ops; > + > + void __iomem *reg_base; > + > + u16 ll_wr_cnt; > + u16 ll_rd_cnt; Why did you name these fields with "ll_" prefix? These are the number of read/write channels. Moreover the structure doc above have them named as "wr_ch_cnt" and "rd_ch_cnt". So if you want to change the fields names please add an addition patch and justify why it's needed. > + /* link list address */ > + struct dw_edma_region ll_region_wr[EDMA_MAX_WR_CH]; > + struct dw_edma_region ll_region_rd[EDMA_MAX_RD_CH]; > + > + /* data region */ > + struct dw_edma_region dt_region_wr[EDMA_MAX_WR_CH]; > + struct dw_edma_region dt_region_rd[EDMA_MAX_RD_CH]; > + > + enum dw_edma_map_format mf; > + > struct dw_edma *dw; Finally this could be dropped. Thus the dw_edma_chip structure will be just the chip info data. -Sergey > }; > > -- > 2.24.0.rc1 >
On Wed, Mar 9, 2022 at 7:39 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > Hello Frank > > On Mon, Mar 07, 2022 at 04:47:45PM -0600, Frank Li wrote: > > "struct dw_edma_chip" contains an internal structure "struct dw_edma" that is > > used by the eDMA core internally. This structure should not be touched > > by the eDMA controller drivers themselves. But currently, the eDMA > > controller drivers like "dw-edma-pci" allocates and populates this > > internal structure then passes it on to eDMA core. The eDMA core further > > populates the structure and uses it. This is wrong! > > > > Hence, move all the "struct dw_edma" specifics from controller drivers > > to the eDMA core. > > Thanks for the patchset. Alas it has just drawn my attention on v3 > stage, otherwise I would have given to you my thoughts stright away on > v1. Anyway first of all a cover letter would be very much appropriate > to have a general notion about all the changes introduced in the set. > > Secondly I've just been working on adding the eDMA platform support > myself, so you have been just about a week ahead of me submitting my > changes. My work contains some of the modifications done by you (but > have some additional fixes too) so I'll need to rebase it on top of > your patchset when it's finished. Anyway am I understand correctly, > that you've also been working on the DW PCIe driver alteration so one > would properly initialize the eDMA-chip data structure? If so have you > sent the patchset already? Could you give me a link and add me to Cc > in the emailing thread? (That's where the cover letter with all the > info and related patchsets would be very helpful.) > > Thirdly regarding this patch. Your modification is mainly correct, but > I would suggest to change the concept. Instead of needlessly converting > the code to using the dw_edma_chip structure pointer within the DW eDMA > driver, it would be much easier in modification and more logical to > keep using the struct dw_edma pointer. Especially seeing dw_edma > structure is going to be a pure private data. So to speak what I would > suggest is to have the next pointers setup: > > include/linux/dma/edma.h: > struct dw_edma_chip { > /* drop struct dw_edma *dw; */ > }; The key problem at dw_edma_remove(struct dw_edma_chip *chip) It needs dw information to do clean up work. > > drivers/dma/dw-edma/dw-edma-core.h: > struct dw_edma { > const struct dw_edma_chip *chip; > }; > > struct dw_edma_chan { > struct dw_edma *dw; > }; > > Thus we'll have a cleaner concept here: > struct dw_edma_chip - DW eDMA controller descriptor, defined and > initialized in the client platform drivers. Can be static and const in > general (which in your approach is impossible). > struct dw_edma - pure DW eDMA driver private data. It will be used > the within the local driver only and won't be seen outside. > > Thus you won't need an almost a half of the modifications below, > would provide a cleaner interface, would support the const > static DW eDMA chip info objects. > > More comments regarding the suggested approach and some additional > notes are below. > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > > > Resend added dmaengine@vger.kernel.org > > > > Change from v2 to v3 > > - none > > Change from v1 to v2 > > - rework commit message > > - remove duplicate field in struct dw_edma > > > > drivers/dma/dw-edma/dw-edma-core.c | 91 +++++----- > > drivers/dma/dw-edma/dw-edma-core.h | 30 +--- > > drivers/dma/dw-edma/dw-edma-v0-core.c | 206 ++++++++++++----------- > > drivers/dma/dw-edma/dw-edma-v0-core.h | 8 +- > > drivers/dma/dw-edma/dw-edma-v0-debugfs.c | 37 ++-- > > include/linux/dma/edma.h | 47 +++++- > > 6 files changed, 230 insertions(+), 189 deletions(-) > > > > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c > > index 53289927dd0d6..0cb66434f9e14 100644 > > --- a/drivers/dma/dw-edma/dw-edma-core.c > > +++ b/drivers/dma/dw-edma/dw-edma-core.c > > @@ -65,7 +65,7 @@ static struct dw_edma_burst *dw_edma_alloc_burst(struct dw_edma_chunk *chunk) > > static struct dw_edma_chunk *dw_edma_alloc_chunk(struct dw_edma_desc *desc) > > { > > struct dw_edma_chan *chan = desc->chan; > > > - struct dw_edma *dw = chan->chip->dw; > > + struct dw_edma_chip *chip = chan->chip; > > If you do as I suggest you won't need to modify this part. > > > struct dw_edma_chunk *chunk; > > > > chunk = kzalloc(sizeof(*chunk), GFP_NOWAIT); > > @@ -82,11 +82,11 @@ static struct dw_edma_chunk *dw_edma_alloc_chunk(struct dw_edma_desc *desc) > > */ > > chunk->cb = !(desc->chunks_alloc % 2); > > if (chan->dir == EDMA_DIR_WRITE) { > > > - chunk->ll_region.paddr = dw->ll_region_wr[chan->id].paddr; > > - chunk->ll_region.vaddr = dw->ll_region_wr[chan->id].vaddr; > > + chunk->ll_region.paddr = chip->ll_region_wr[chan->id].paddr; > > + chunk->ll_region.vaddr = chip->ll_region_wr[chan->id].vaddr; > > } else { > > - chunk->ll_region.paddr = dw->ll_region_rd[chan->id].paddr; > > - chunk->ll_region.vaddr = dw->ll_region_rd[chan->id].vaddr; > > + chunk->ll_region.paddr = chip->ll_region_rd[chan->id].paddr; > > + chunk->ll_region.vaddr = chip->ll_region_rd[chan->id].vaddr; > > } > > Here you would just need to change the pointers "sandwich" to > "dw->chip->...". > > > > > if (desc->chunk) { > > @@ -601,7 +601,8 @@ static void dw_edma_abort_interrupt(struct dw_edma_chan *chan) > > static irqreturn_t dw_edma_interrupt(int irq, void *data, bool write) > > { > > struct dw_edma_irq *dw_irq = data; > > > - struct dw_edma *dw = dw_irq->dw; > > + struct dw_edma_chip *chip = dw_irq->chip; > > + struct dw_edma *dw = chip->dw; > > This also would have been unneeded, since the method relies on the data > from the dw_edma structure. > > > unsigned long total, pos, val; > > unsigned long off; > > u32 mask; > > @@ -616,7 +617,7 @@ static irqreturn_t dw_edma_interrupt(int irq, void *data, bool write) > > mask = dw_irq->rd_mask; > > } > > > > > - val = dw_edma_v0_core_status_done_int(dw, write ? > > + val = dw_edma_v0_core_status_done_int(chip, write ? > > EDMA_DIR_WRITE : > > EDMA_DIR_READ); > > val &= mask; > > @@ -626,7 +627,7 @@ static irqreturn_t dw_edma_interrupt(int irq, void *data, bool write) > > dw_edma_done_interrupt(chan); > > } > > > > - val = dw_edma_v0_core_status_abort_int(dw, write ? > > + val = dw_edma_v0_core_status_abort_int(chip, write ? > > EDMA_DIR_WRITE : > > EDMA_DIR_READ); > > This won't be needed. > > > val &= mask; > > @@ -718,7 +719,7 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write, > > } > > > > INIT_LIST_HEAD(&dma->channels); > > > - for (j = 0; (alloc || dw->nr_irqs == 1) && j < cnt; j++, i++) { > > + for (j = 0; (alloc || chip->nr_irqs == 1) && j < cnt; j++, i++) { > > chan = &dw->chan[i]; > > > > dt_region = devm_kzalloc(dev, sizeof(*dt_region), GFP_KERNEL); > > @@ -735,15 +736,15 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write, > > chan->status = EDMA_ST_IDLE; > > > > if (write) > > - chan->ll_max = (dw->ll_region_wr[j].sz / EDMA_LL_SZ); > > + chan->ll_max = (chip->ll_region_wr[j].sz / EDMA_LL_SZ); > > else > > - chan->ll_max = (dw->ll_region_rd[j].sz / EDMA_LL_SZ); > > + chan->ll_max = (chip->ll_region_rd[j].sz / EDMA_LL_SZ); > > chan->ll_max -= 1; > > > > dev_vdbg(dev, "L. List:\tChannel %s[%u] max_cnt=%u\n", > > write ? "write" : "read", j, chan->ll_max); > > > > - if (dw->nr_irqs == 1) > > + if (chip->nr_irqs == 1) > > This would have been changed to using the "dw->chip->..." pattern. > > > pos = 0; > > else > > pos = off_alloc + (j % alloc); > > @@ -755,7 +756,7 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write, > > else > > irq->rd_mask |= BIT(j); > > > > > - irq->dw = dw; > > + irq->chip = chip; > > Won't be needed. > > > memcpy(&chan->msi, &irq->msi, sizeof(chan->msi)); > > > > dev_vdbg(dev, "MSI:\t\tChannel %s[%u] addr=0x%.8x%.8x, data=0x%.8x\n", > > @@ -767,13 +768,13 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write, > > vchan_init(&chan->vc, dma); > > > > if (write) { > > > - dt_region->paddr = dw->dt_region_wr[j].paddr; > > - dt_region->vaddr = dw->dt_region_wr[j].vaddr; > > - dt_region->sz = dw->dt_region_wr[j].sz; > > + dt_region->paddr = chip->dt_region_wr[j].paddr; > > + dt_region->vaddr = chip->dt_region_wr[j].vaddr; > > + dt_region->sz = chip->dt_region_wr[j].sz; > > } else { > > - dt_region->paddr = dw->dt_region_rd[j].paddr; > > - dt_region->vaddr = dw->dt_region_rd[j].vaddr; > > - dt_region->sz = dw->dt_region_rd[j].sz; > > + dt_region->paddr = chip->dt_region_rd[j].paddr; > > + dt_region->vaddr = chip->dt_region_rd[j].vaddr; > > + dt_region->sz = chip->dt_region_rd[j].sz; > > Just replace with "dw->chip->..." > > > } > > > > dw_edma_v0_core_device_config(chan); > > @@ -840,16 +841,16 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, > > > > ch_cnt = dw->wr_ch_cnt + dw->rd_ch_cnt; > > > > > - if (dw->nr_irqs < 1) > > + if (chip->nr_irqs < 1) > > return -EINVAL; > > > > - if (dw->nr_irqs == 1) { > > + if (chip->nr_irqs == 1) { > > /* Common IRQ shared among all channels */ > > - irq = dw->ops->irq_vector(dev, 0); > > + irq = chip->ops->irq_vector(dev, 0); > > err = request_irq(irq, dw_edma_interrupt_common, > > IRQF_SHARED, dw->name, &dw->irq[0]); > > if (err) { > > - dw->nr_irqs = 0; > > + chip->nr_irqs = 0; > > return err; > > } > > > > @@ -857,7 +858,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, > > get_cached_msi_msg(irq, &dw->irq[0].msi); > > } else { > > /* Distribute IRQs equally among all channels */ > > - int tmp = dw->nr_irqs; > > + int tmp = chip->nr_irqs; > > > > while (tmp && (*wr_alloc + *rd_alloc) < ch_cnt) { > > dw_edma_dec_irq_alloc(&tmp, wr_alloc, dw->wr_ch_cnt); > > @@ -868,7 +869,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, > > dw_edma_add_irq_mask(&rd_mask, *rd_alloc, dw->rd_ch_cnt); > > > > for (i = 0; i < (*wr_alloc + *rd_alloc); i++) { > > - irq = dw->ops->irq_vector(dev, i); > > + irq = chip->ops->irq_vector(dev, i); > > err = request_irq(irq, > > i < *wr_alloc ? > > dw_edma_interrupt_write : > > @@ -876,7 +877,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, > > IRQF_SHARED, dw->name, > > &dw->irq[i]); > > if (err) { > > - dw->nr_irqs = i; > > + chip->nr_irqs = i; > > return err; > > } > > > > @@ -884,7 +885,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, > > get_cached_msi_msg(irq, &dw->irq[i].msi); > > } > > > > - dw->nr_irqs = i; > > + chip->nr_irqs = i; > > ditto > > > } > > > > return err; > > @@ -905,18 +906,24 @@ int dw_edma_probe(struct dw_edma_chip *chip) > > if (!dev) > > return -EINVAL; > > > > - dw = chip->dw; > > - if (!dw || !dw->irq || !dw->ops || !dw->ops->irq_vector) > > + dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL); > > + if (!dw) > > + return -ENOMEM; > > + > > > + chip->dw = dw; > > Convert this to "dw->chip = chip". > > > + > > + if (!chip->nr_irqs || !chip->ops) > > return -EINVAL; > > Move this to be performed before the pointer initialization, since > it's pointless to initialize anything if invalid data is passed. > You can join it in with the "if (!dev)" statement. > > > > > raw_spin_lock_init(&dw->lock); > > > > - dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt, > > - dw_edma_v0_core_ch_count(dw, EDMA_DIR_WRITE)); > > + > > > + dw->wr_ch_cnt = min_t(u16, chip->ll_wr_cnt, > > + dw_edma_v0_core_ch_count(chip, EDMA_DIR_WRITE)); > > dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt, EDMA_MAX_WR_CH); > > > > - dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt, > > - dw_edma_v0_core_ch_count(dw, EDMA_DIR_READ)); > > + dw->rd_ch_cnt = min_t(u16, chip->ll_rd_cnt, > > Regarding the field naming please see my comment to the dw_edma_chip > structure. > > > + dw_edma_v0_core_ch_count(chip, EDMA_DIR_READ)); > > dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt, EDMA_MAX_RD_CH); > > > > if (!dw->wr_ch_cnt && !dw->rd_ch_cnt) > > @@ -934,7 +941,11 @@ int dw_edma_probe(struct dw_edma_chip *chip) > > snprintf(dw->name, sizeof(dw->name), "dw-edma-core:%d", chip->id); > > > > /* Disable eDMA, only to establish the ideal initial conditions */ > > > - dw_edma_v0_core_off(dw); > > + dw_edma_v0_core_off(chip); > > + > > + dw->irq = devm_kcalloc(dev, chip->nr_irqs, sizeof(*dw->irq), GFP_KERNEL); > > + if (!dw->irq) > > + return -ENOMEM; > > ditto > > > > > /* Request IRQs */ > > err = dw_edma_irq_request(chip, &wr_alloc, &rd_alloc); > > @@ -960,10 +971,10 @@ int dw_edma_probe(struct dw_edma_chip *chip) > > return 0; > > > > err_irq_free: > > > - for (i = (dw->nr_irqs - 1); i >= 0; i--) > > - free_irq(dw->ops->irq_vector(dev, i), &dw->irq[i]); > > + for (i = (chip->nr_irqs - 1); i >= 0; i--) > > + free_irq(chip->ops->irq_vector(dev, i), &dw->irq[i]); > > > > - dw->nr_irqs = 0; > > + chip->nr_irqs = 0; > > dw->chip->... > > > > > return err; > > } > > @@ -977,11 +988,11 @@ int dw_edma_remove(struct dw_edma_chip *chip) > > int i; > > > > /* Disable eDMA */ > > > - dw_edma_v0_core_off(dw); > > + dw_edma_v0_core_off(chip); > > Won't need this. > > > > > /* Free irqs */ > > - for (i = (dw->nr_irqs - 1); i >= 0; i--) > > - free_irq(dw->ops->irq_vector(dev, i), &dw->irq[i]); > > + for (i = (chip->nr_irqs - 1); i >= 0; i--) > > + free_irq(chip->ops->irq_vector(dev, i), &dw->irq[i]); > > Use "dw->chip->..." > > > > > /* Power management */ > > pm_runtime_disable(dev); > > diff --git a/drivers/dma/dw-edma/dw-edma-core.h b/drivers/dma/dw-edma/dw-edma-core.h > > index 60316d408c3e0..885f6719c9462 100644 > > --- a/drivers/dma/dw-edma/dw-edma-core.h > > +++ b/drivers/dma/dw-edma/dw-edma-core.h > > @@ -15,20 +15,12 @@ > > #include "../virt-dma.h" > > > > #define EDMA_LL_SZ 24 > > -#define EDMA_MAX_WR_CH 8 > > -#define EDMA_MAX_RD_CH 8 > > > > enum dw_edma_dir { > > EDMA_DIR_WRITE = 0, > > EDMA_DIR_READ > > }; > > > > -enum dw_edma_map_format { > > - EDMA_MF_EDMA_LEGACY = 0x0, > > - EDMA_MF_EDMA_UNROLL = 0x1, > > - EDMA_MF_HDMA_COMPAT = 0x5 > > -}; > > - > > enum dw_edma_request { > > EDMA_REQ_NONE = 0, > > EDMA_REQ_STOP, > > @@ -57,12 +49,6 @@ struct dw_edma_burst { > > u32 sz; > > }; > > > > -struct dw_edma_region { > > - phys_addr_t paddr; > > - void __iomem *vaddr; > > - size_t sz; > > -}; > > - > > struct dw_edma_chunk { > > struct list_head list; > > struct dw_edma_chan *chan; > > @@ -106,11 +92,7 @@ struct dw_edma_irq { > > struct msi_msg msi; > > u32 wr_mask; > > u32 rd_mask; > > - struct dw_edma *dw; > > -}; > > - > > -struct dw_edma_core_ops { > > - int (*irq_vector)(struct device *dev, unsigned int nr); > > + struct dw_edma_chip *chip; > > }; > > > > struct dw_edma { > > @@ -122,19 +104,9 @@ struct dw_edma { > > struct dma_device rd_edma; > > u16 rd_ch_cnt; > > > > > - struct dw_edma_region rg_region; /* Registers */ > > AFAICS you replaced this with void __iomem *reg_base further in the > dw_edma_chip structure. Even though I do agree with this modification, > it's conceptually right, but the alteration is unrelated to the patch > topic. So please unpin it to a dedicated patch placed before this one > in the series. > > > - struct dw_edma_region ll_region_wr[EDMA_MAX_WR_CH]; > > - struct dw_edma_region ll_region_rd[EDMA_MAX_RD_CH]; > > - struct dw_edma_region dt_region_wr[EDMA_MAX_WR_CH]; > > - struct dw_edma_region dt_region_rd[EDMA_MAX_RD_CH]; > > - > > struct dw_edma_irq *irq; > > - int nr_irqs; > > - > > - enum dw_edma_map_format mf; > > > > struct dw_edma_chan *chan; > > - const struct dw_edma_core_ops *ops; > > > > raw_spinlock_t lock; /* Only for legacy */ > > #ifdef CONFIG_DEBUG_FS > > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c > > index 329fc2e57b703..6e2f83e31a03a 100644 > > --- a/drivers/dma/dw-edma/dw-edma-v0-core.c > > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c > > @@ -23,92 +23,94 @@ enum dw_edma_control { > > DW_EDMA_V0_LLE = BIT(9), > > }; > > > > > -static inline struct dw_edma_v0_regs __iomem *__dw_regs(struct dw_edma *dw) > > +static inline struct dw_edma_v0_regs __iomem *__dw_regs(struct dw_edma_chip *chip) > > { > > - return dw->rg_region.vaddr; > > + return chip->reg_base; > > } > > Just use "dw->chip->reg_base" here and you won't need to introduce the > most of the modifications below, since all of the them is connected > with the __dw_regs() function prototype change. > > > > > -#define SET_32(dw, name, value) \ > > - writel(value, &(__dw_regs(dw)->name)) > > +#define SET_32(chip, name, value) \ > > + writel(value, &(__dw_regs(chip)->name)) > > > > -#define GET_32(dw, name) \ > > - readl(&(__dw_regs(dw)->name)) > > +#define GET_32(chip, name) \ > > + readl(&(__dw_regs(chip)->name)) > > > > -#define SET_RW_32(dw, dir, name, value) \ > > +#define SET_RW_32(chip, dir, name, value) \ > > do { \ > > if ((dir) == EDMA_DIR_WRITE) \ > > - SET_32(dw, wr_##name, value); \ > > + SET_32(chip, wr_##name, value); \ > > else \ > > - SET_32(dw, rd_##name, value); \ > > + SET_32(chip, rd_##name, value); \ > > } while (0) > > > > -#define GET_RW_32(dw, dir, name) \ > > +#define GET_RW_32(chip, dir, name) \ > > ((dir) == EDMA_DIR_WRITE \ > > - ? GET_32(dw, wr_##name) \ > > - : GET_32(dw, rd_##name)) > > + ? GET_32(chip, wr_##name) \ > > + : GET_32(chip, rd_##name)) > > > > -#define SET_BOTH_32(dw, name, value) \ > > +#define SET_BOTH_32(chip, name, value) \ > > do { \ > > - SET_32(dw, wr_##name, value); \ > > - SET_32(dw, rd_##name, value); \ > > + SET_32(chip, wr_##name, value); \ > > + SET_32(chip, rd_##name, value); \ > > } while (0) > > > > #ifdef CONFIG_64BIT > > > > -#define SET_64(dw, name, value) \ > > - writeq(value, &(__dw_regs(dw)->name)) > > +#define SET_64(chip, name, value) \ > > + writeq(value, &(__dw_regs(chip)->name)) > > > > -#define GET_64(dw, name) \ > > - readq(&(__dw_regs(dw)->name)) > > +#define GET_64(chip, name) \ > > + readq(&(__dw_regs(chip)->name)) > > > > -#define SET_RW_64(dw, dir, name, value) \ > > +#define SET_RW_64(chip, dir, name, value) \ > > do { \ > > if ((dir) == EDMA_DIR_WRITE) \ > > - SET_64(dw, wr_##name, value); \ > > + SET_64(chip, wr_##name, value); \ > > else \ > > - SET_64(dw, rd_##name, value); \ > > + SET_64(chip, rd_##name, value); \ > > } while (0) > > > > -#define GET_RW_64(dw, dir, name) \ > > +#define GET_RW_64(chip, dir, name) \ > > ((dir) == EDMA_DIR_WRITE \ > > - ? GET_64(dw, wr_##name) \ > > - : GET_64(dw, rd_##name)) > > + ? GET_64(chip, wr_##name) \ > > + : GET_64(chip, rd_##name)) > > > > -#define SET_BOTH_64(dw, name, value) \ > > +#define SET_BOTH_64(chip, name, value) \ > > do { \ > > - SET_64(dw, wr_##name, value); \ > > - SET_64(dw, rd_##name, value); \ > > + SET_64(chip, wr_##name, value); \ > > + SET_64(chip, rd_##name, value); \ > > } while (0) > > > > #endif /* CONFIG_64BIT */ > > > > -#define SET_COMPAT(dw, name, value) \ > > - writel(value, &(__dw_regs(dw)->type.unroll.name)) > > +#define SET_COMPAT(chip, name, value) \ > > + writel(value, &(__dw_regs(chip)->type.unroll.name)) > > > > -#define SET_RW_COMPAT(dw, dir, name, value) \ > > +#define SET_RW_COMPAT(chip, dir, name, value) \ > > do { \ > > if ((dir) == EDMA_DIR_WRITE) \ > > - SET_COMPAT(dw, wr_##name, value); \ > > + SET_COMPAT(chip, wr_##name, value); \ > > else \ > > - SET_COMPAT(dw, rd_##name, value); \ > > + SET_COMPAT(chip, rd_##name, value); \ > > } while (0) > > > > static inline struct dw_edma_v0_ch_regs __iomem * > > -__dw_ch_regs(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch) > > +__dw_ch_regs(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch) > > { > > - if (dw->mf == EDMA_MF_EDMA_LEGACY) > > - return &(__dw_regs(dw)->type.legacy.ch); > > + if (chip->mf == EDMA_MF_EDMA_LEGACY) > > + return &(__dw_regs(chip)->type.legacy.ch); > > > > if (dir == EDMA_DIR_WRITE) > > - return &__dw_regs(dw)->type.unroll.ch[ch].wr; > > + return &__dw_regs(chip)->type.unroll.ch[ch].wr; > > > > - return &__dw_regs(dw)->type.unroll.ch[ch].rd; > > + return &__dw_regs(chip)->type.unroll.ch[ch].rd; > > } > > > > -static inline void writel_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > +static inline void writel_ch(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch, > > u32 value, void __iomem *addr) > > { > > - if (dw->mf == EDMA_MF_EDMA_LEGACY) { > > + struct dw_edma *dw = chip->dw; > > + > > + if (chip->mf == EDMA_MF_EDMA_LEGACY) { > > u32 viewport_sel; > > unsigned long flags; > > > > @@ -119,7 +121,7 @@ static inline void writel_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > viewport_sel |= BIT(31); > > > > writel(viewport_sel, > > - &(__dw_regs(dw)->type.legacy.viewport_sel)); > > + &(__dw_regs(chip)->type.legacy.viewport_sel)); > > writel(value, addr); > > > > raw_spin_unlock_irqrestore(&dw->lock, flags); > > @@ -128,12 +130,13 @@ static inline void writel_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > } > > } > > > > -static inline u32 readl_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > +static inline u32 readl_ch(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch, > > const void __iomem *addr) > > { > > + struct dw_edma *dw = chip->dw; > > u32 value; > > > > - if (dw->mf == EDMA_MF_EDMA_LEGACY) { > > + if (chip->mf == EDMA_MF_EDMA_LEGACY) { > > u32 viewport_sel; > > unsigned long flags; > > > > @@ -144,7 +147,7 @@ static inline u32 readl_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > viewport_sel |= BIT(31); > > > > writel(viewport_sel, > > - &(__dw_regs(dw)->type.legacy.viewport_sel)); > > + &(__dw_regs(chip)->type.legacy.viewport_sel)); > > value = readl(addr); > > > > raw_spin_unlock_irqrestore(&dw->lock, flags); > > @@ -166,10 +169,12 @@ static inline u32 readl_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > > > #ifdef CONFIG_64BIT > > > > -static inline void writeq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > +static inline void writeq_ch(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch, > > u64 value, void __iomem *addr) > > { > > - if (dw->mf == EDMA_MF_EDMA_LEGACY) { > > + struct dw_edma *dw = chip->dw; > > + > > + if (chip->mf == EDMA_MF_EDMA_LEGACY) { > > u32 viewport_sel; > > unsigned long flags; > > > > @@ -180,7 +185,7 @@ static inline void writeq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > viewport_sel |= BIT(31); > > > > writel(viewport_sel, > > - &(__dw_regs(dw)->type.legacy.viewport_sel)); > > + &(__dw_regs(chip)->type.legacy.viewport_sel)); > > writeq(value, addr); > > > > raw_spin_unlock_irqrestore(&dw->lock, flags); > > @@ -189,12 +194,13 @@ static inline void writeq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > } > > } > > > > -static inline u64 readq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > +static inline u64 readq_ch(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch, > > const void __iomem *addr) > > { > > + struct dw_edma *dw = chip->dw; > > u32 value; > > > > - if (dw->mf == EDMA_MF_EDMA_LEGACY) { > > + if (chip->mf == EDMA_MF_EDMA_LEGACY) { > > u32 viewport_sel; > > unsigned long flags; > > > > @@ -205,7 +211,7 @@ static inline u64 readq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > viewport_sel |= BIT(31); > > > > writel(viewport_sel, > > - &(__dw_regs(dw)->type.legacy.viewport_sel)); > > + &(__dw_regs(chip)->type.legacy.viewport_sel)); > > value = readq(addr); > > > > raw_spin_unlock_irqrestore(&dw->lock, flags); > > @@ -228,25 +234,25 @@ static inline u64 readq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > #endif /* CONFIG_64BIT */ > > > > /* eDMA management callbacks */ > > -void dw_edma_v0_core_off(struct dw_edma *dw) > > +void dw_edma_v0_core_off(struct dw_edma_chip *chip) > > { > > - SET_BOTH_32(dw, int_mask, > > + SET_BOTH_32(chip, int_mask, > > EDMA_V0_DONE_INT_MASK | EDMA_V0_ABORT_INT_MASK); > > - SET_BOTH_32(dw, int_clear, > > + SET_BOTH_32(chip, int_clear, > > EDMA_V0_DONE_INT_MASK | EDMA_V0_ABORT_INT_MASK); > > - SET_BOTH_32(dw, engine_en, 0); > > + SET_BOTH_32(chip, engine_en, 0); > > } > > > > -u16 dw_edma_v0_core_ch_count(struct dw_edma *dw, enum dw_edma_dir dir) > > +u16 dw_edma_v0_core_ch_count(struct dw_edma_chip *chip, enum dw_edma_dir dir) > > { > > u32 num_ch; > > > > if (dir == EDMA_DIR_WRITE) > > num_ch = FIELD_GET(EDMA_V0_WRITE_CH_COUNT_MASK, > > - GET_32(dw, ctrl)); > > + GET_32(chip, ctrl)); > > else > > num_ch = FIELD_GET(EDMA_V0_READ_CH_COUNT_MASK, > > - GET_32(dw, ctrl)); > > + GET_32(chip, ctrl)); > > > > if (num_ch > EDMA_V0_MAX_NR_CH) > > num_ch = EDMA_V0_MAX_NR_CH; > > @@ -256,11 +262,11 @@ u16 dw_edma_v0_core_ch_count(struct dw_edma *dw, enum dw_edma_dir dir) > > So can omit most of the changes from the comment above and up to this > comment and use "dw->chip->..." where it's required. > > > > > enum dma_status dw_edma_v0_core_ch_status(struct dw_edma_chan *chan) > > { > > > - struct dw_edma *dw = chan->chip->dw; > > + struct dw_edma_chip *chip = chan->chip; > > Just use "dw = chan->dw" here or directly use "chan->dw" in the > method. > > > u32 tmp; > > > > tmp = FIELD_GET(EDMA_V0_CH_STATUS_MASK, > > - GET_CH_32(dw, chan->dir, chan->id, ch_control1)); > > + GET_CH_32(chip, chan->dir, chan->id, ch_control1)); > > > > if (tmp == 1) > > return DMA_IN_PROGRESS; > > @@ -272,30 +278,30 @@ enum dma_status dw_edma_v0_core_ch_status(struct dw_edma_chan *chan) > > > > void dw_edma_v0_core_clear_done_int(struct dw_edma_chan *chan) > > { > > > - struct dw_edma *dw = chan->chip->dw; > > + struct dw_edma_chip *chip = chan->chip; > > ditto > > > > > - SET_RW_32(dw, chan->dir, int_clear, > > + SET_RW_32(chip, chan->dir, int_clear, > > FIELD_PREP(EDMA_V0_DONE_INT_MASK, BIT(chan->id))); > > } > > > > void dw_edma_v0_core_clear_abort_int(struct dw_edma_chan *chan) > > { > > > - struct dw_edma *dw = chan->chip->dw; > > + struct dw_edma_chip *chip = chan->chip; > > ditto > > > > > > - SET_RW_32(dw, chan->dir, int_clear, > > + SET_RW_32(chip, chan->dir, int_clear, > > FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id))); > > } > > > > -u32 dw_edma_v0_core_status_done_int(struct dw_edma *dw, enum dw_edma_dir dir) > > +u32 dw_edma_v0_core_status_done_int(struct dw_edma_chip *chip, enum dw_edma_dir dir) > > { > > return FIELD_GET(EDMA_V0_DONE_INT_MASK, > > - GET_RW_32(dw, dir, int_status)); > > + GET_RW_32(chip, dir, int_status)); > > } > > > > -u32 dw_edma_v0_core_status_abort_int(struct dw_edma *dw, enum dw_edma_dir dir) > > +u32 dw_edma_v0_core_status_abort_int(struct dw_edma_chip *chip, enum dw_edma_dir dir) > > { > > return FIELD_GET(EDMA_V0_ABORT_INT_MASK, > > - GET_RW_32(dw, dir, int_status)); > > + GET_RW_32(chip, dir, int_status)); > > Won't be needed > > > } > > > > static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk) > > @@ -357,109 +363,109 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk) > > void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first) > > { > > struct dw_edma_chan *chan = chunk->chan; > > > - struct dw_edma *dw = chan->chip->dw; > > + struct dw_edma_chip *chip = chan->chip; > > Just replace with "dw = chan->dw" or directly use "chan->dw" in the > method. Thus you can omit the alterations below. > > > u32 tmp; > > > > dw_edma_v0_core_write_chunk(chunk); > > > > if (first) { > > /* Enable engine */ > > - SET_RW_32(dw, chan->dir, engine_en, BIT(0)); > > - if (dw->mf == EDMA_MF_HDMA_COMPAT) { > > + SET_RW_32(chip, chan->dir, engine_en, BIT(0)); > > + if (chip->mf == EDMA_MF_HDMA_COMPAT) { > > switch (chan->id) { > > case 0: > > - SET_RW_COMPAT(dw, chan->dir, ch0_pwr_en, > > + SET_RW_COMPAT(chip, chan->dir, ch0_pwr_en, > > BIT(0)); > > break; > > case 1: > > - SET_RW_COMPAT(dw, chan->dir, ch1_pwr_en, > > + SET_RW_COMPAT(chip, chan->dir, ch1_pwr_en, > > BIT(0)); > > break; > > case 2: > > - SET_RW_COMPAT(dw, chan->dir, ch2_pwr_en, > > + SET_RW_COMPAT(chip, chan->dir, ch2_pwr_en, > > BIT(0)); > > break; > > case 3: > > - SET_RW_COMPAT(dw, chan->dir, ch3_pwr_en, > > + SET_RW_COMPAT(chip, chan->dir, ch3_pwr_en, > > BIT(0)); > > break; > > case 4: > > - SET_RW_COMPAT(dw, chan->dir, ch4_pwr_en, > > + SET_RW_COMPAT(chip, chan->dir, ch4_pwr_en, > > BIT(0)); > > break; > > case 5: > > - SET_RW_COMPAT(dw, chan->dir, ch5_pwr_en, > > + SET_RW_COMPAT(chip, chan->dir, ch5_pwr_en, > > BIT(0)); > > break; > > case 6: > > - SET_RW_COMPAT(dw, chan->dir, ch6_pwr_en, > > + SET_RW_COMPAT(chip, chan->dir, ch6_pwr_en, > > BIT(0)); > > break; > > case 7: > > - SET_RW_COMPAT(dw, chan->dir, ch7_pwr_en, > > + SET_RW_COMPAT(chip, chan->dir, ch7_pwr_en, > > BIT(0)); > > break; > > } > > } > > /* Interrupt unmask - done, abort */ > > - tmp = GET_RW_32(dw, chan->dir, int_mask); > > + tmp = GET_RW_32(chip, chan->dir, int_mask); > > tmp &= ~FIELD_PREP(EDMA_V0_DONE_INT_MASK, BIT(chan->id)); > > tmp &= ~FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id)); > > - SET_RW_32(dw, chan->dir, int_mask, tmp); > > + SET_RW_32(chip, chan->dir, int_mask, tmp); > > /* Linked list error */ > > - tmp = GET_RW_32(dw, chan->dir, linked_list_err_en); > > + tmp = GET_RW_32(chip, chan->dir, linked_list_err_en); > > tmp |= FIELD_PREP(EDMA_V0_LINKED_LIST_ERR_MASK, BIT(chan->id)); > > - SET_RW_32(dw, chan->dir, linked_list_err_en, tmp); > > + SET_RW_32(chip, chan->dir, linked_list_err_en, tmp); > > /* Channel control */ > > - SET_CH_32(dw, chan->dir, chan->id, ch_control1, > > + SET_CH_32(chip, chan->dir, chan->id, ch_control1, > > (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE)); > > /* Linked list */ > > #ifdef CONFIG_64BIT > > - SET_CH_64(dw, chan->dir, chan->id, llp.reg, > > + SET_CH_64(chip, chan->dir, chan->id, llp.reg, > > chunk->ll_region.paddr); > > #else /* CONFIG_64BIT */ > > - SET_CH_32(dw, chan->dir, chan->id, llp.lsb, > > + SET_CH_32(chip, chan->dir, chan->id, llp.lsb, > > lower_32_bits(chunk->ll_region.paddr)); > > - SET_CH_32(dw, chan->dir, chan->id, llp.msb, > > + SET_CH_32(chip, chan->dir, chan->id, llp.msb, > > upper_32_bits(chunk->ll_region.paddr)); > > #endif /* CONFIG_64BIT */ > > } > > /* Doorbell */ > > - SET_RW_32(dw, chan->dir, doorbell, > > + SET_RW_32(chip, chan->dir, doorbell, > > FIELD_PREP(EDMA_V0_DOORBELL_CH_MASK, chan->id)); > > } > > You can drop the changes from the previous comment up to this one. > > > > > int dw_edma_v0_core_device_config(struct dw_edma_chan *chan) > > { > > > - struct dw_edma *dw = chan->chip->dw; > > + struct dw_edma_chip *chip = chan->chip; > > Use "chan->dw" here. > > > u32 tmp = 0; > > > > /* MSI done addr - low, high */ > > - SET_RW_32(dw, chan->dir, done_imwr.lsb, chan->msi.address_lo); > > - SET_RW_32(dw, chan->dir, done_imwr.msb, chan->msi.address_hi); > > + SET_RW_32(chip, chan->dir, done_imwr.lsb, chan->msi.address_lo); > > + SET_RW_32(chip, chan->dir, done_imwr.msb, chan->msi.address_hi); > > /* MSI abort addr - low, high */ > > - SET_RW_32(dw, chan->dir, abort_imwr.lsb, chan->msi.address_lo); > > - SET_RW_32(dw, chan->dir, abort_imwr.msb, chan->msi.address_hi); > > + SET_RW_32(chip, chan->dir, abort_imwr.lsb, chan->msi.address_lo); > > + SET_RW_32(chip, chan->dir, abort_imwr.msb, chan->msi.address_hi); > > /* MSI data - low, high */ > > switch (chan->id) { > > case 0: > > case 1: > > - tmp = GET_RW_32(dw, chan->dir, ch01_imwr_data); > > + tmp = GET_RW_32(chip, chan->dir, ch01_imwr_data); > > break; > > > > case 2: > > case 3: > > - tmp = GET_RW_32(dw, chan->dir, ch23_imwr_data); > > + tmp = GET_RW_32(chip, chan->dir, ch23_imwr_data); > > break; > > > > case 4: > > case 5: > > - tmp = GET_RW_32(dw, chan->dir, ch45_imwr_data); > > + tmp = GET_RW_32(chip, chan->dir, ch45_imwr_data); > > break; > > > > case 6: > > case 7: > > - tmp = GET_RW_32(dw, chan->dir, ch67_imwr_data); > > + tmp = GET_RW_32(chip, chan->dir, ch67_imwr_data); > > break; > > } > > > > @@ -478,22 +484,22 @@ int dw_edma_v0_core_device_config(struct dw_edma_chan *chan) > > switch (chan->id) { > > case 0: > > case 1: > > - SET_RW_32(dw, chan->dir, ch01_imwr_data, tmp); > > + SET_RW_32(chip, chan->dir, ch01_imwr_data, tmp); > > break; > > > > case 2: > > case 3: > > - SET_RW_32(dw, chan->dir, ch23_imwr_data, tmp); > > + SET_RW_32(chip, chan->dir, ch23_imwr_data, tmp); > > break; > > > > case 4: > > case 5: > > - SET_RW_32(dw, chan->dir, ch45_imwr_data, tmp); > > + SET_RW_32(chip, chan->dir, ch45_imwr_data, tmp); > > break; > > > > case 6: > > case 7: > > - SET_RW_32(dw, chan->dir, ch67_imwr_data, tmp); > > + SET_RW_32(chip, chan->dir, ch67_imwr_data, tmp); > > break; > > The changes above won't be needed if you keep using the dw pointer > here as I suggest. > > > } > > > > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.h b/drivers/dma/dw-edma/dw-edma-v0-core.h > > index 2afa626b8300c..01a29c74c0c43 100644 > > --- a/drivers/dma/dw-edma/dw-edma-v0-core.h > > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.h > > @@ -12,13 +12,13 @@ > > #include <linux/dma/edma.h> > > > > /* eDMA management callbacks */ > > > -void dw_edma_v0_core_off(struct dw_edma *chan); > > -u16 dw_edma_v0_core_ch_count(struct dw_edma *chan, enum dw_edma_dir dir); > > +void dw_edma_v0_core_off(struct dw_edma_chip *chip); > > +u16 dw_edma_v0_core_ch_count(struct dw_edma_chip *chip, enum dw_edma_dir dir); > > enum dma_status dw_edma_v0_core_ch_status(struct dw_edma_chan *chan); > > void dw_edma_v0_core_clear_done_int(struct dw_edma_chan *chan); > > void dw_edma_v0_core_clear_abort_int(struct dw_edma_chan *chan); > > -u32 dw_edma_v0_core_status_done_int(struct dw_edma *chan, enum dw_edma_dir dir); > > -u32 dw_edma_v0_core_status_abort_int(struct dw_edma *chan, enum dw_edma_dir dir); > > +u32 dw_edma_v0_core_status_done_int(struct dw_edma_chip *chip, enum dw_edma_dir dir); > > +u32 dw_edma_v0_core_status_abort_int(struct dw_edma_chip *chip, enum dw_edma_dir dir); > > This modification won't be needed. > > > void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first); > > int dw_edma_v0_core_device_config(struct dw_edma_chan *chan); > > /* eDMA debug fs callbacks */ > > diff --git a/drivers/dma/dw-edma/dw-edma-v0-debugfs.c b/drivers/dma/dw-edma/dw-edma-v0-debugfs.c > > index 4b3bcffd15ef1..5819a64aceb0f 100644 > > --- a/drivers/dma/dw-edma/dw-edma-v0-debugfs.c > > +++ b/drivers/dma/dw-edma/dw-edma-v0-debugfs.c > > @@ -38,7 +38,7 @@ > > #define CHANNEL_STR "channel" > > #define REGISTERS_STR "registers" > > > > > -static struct dw_edma *dw; > > +static struct dw_edma_chip *chip; > > Hmmm, why on Earth is this static and global?.. What if we have more > than one host/EP controller with eDMA?.. Nice. I'll think on fixing > this if you don't want to bother... > > > static struct dw_edma_v0_regs __iomem *regs; > > > > static struct { > > @@ -53,8 +53,10 @@ struct debugfs_entries { > > > > static int dw_edma_debugfs_u32_get(void *data, u64 *val) > > { > > + struct dw_edma *dw = chip->dw; > > + > > void __iomem *reg = (void __force __iomem *)data; > > > - if (dw->mf == EDMA_MF_EDMA_LEGACY && > > + if (chip->mf == EDMA_MF_EDMA_LEGACY && > > Use "dw->chip-> ..." here > > > reg >= (void __iomem *)®s->type.legacy.ch) { > > void __iomem *ptr = ®s->type.legacy.ch; > > u32 viewport_sel = 0; > > @@ -127,6 +129,8 @@ static void dw_edma_debugfs_regs_ch(struct dw_edma_v0_ch_regs __iomem *regs, > > > > static void dw_edma_debugfs_regs_wr(struct dentry *dir) > > { > > + struct dw_edma *dw = chip->dw; > > + > > const struct debugfs_entries debugfs_regs[] = { > > /* eDMA global registers */ > > WR_REGISTER(engine_en), > > @@ -173,7 +177,7 @@ static void dw_edma_debugfs_regs_wr(struct dentry *dir) > > nr_entries = ARRAY_SIZE(debugfs_regs); > > dw_edma_debugfs_create_x32(debugfs_regs, nr_entries, regs_dir); > > > > > - if (dw->mf == EDMA_MF_HDMA_COMPAT) { > > + if (chip->mf == EDMA_MF_HDMA_COMPAT) { > > ditto > > > nr_entries = ARRAY_SIZE(debugfs_unroll_regs); > > dw_edma_debugfs_create_x32(debugfs_unroll_regs, nr_entries, > > regs_dir); > > @@ -195,6 +199,8 @@ static void dw_edma_debugfs_regs_wr(struct dentry *dir) > > > > static void dw_edma_debugfs_regs_rd(struct dentry *dir) > > { > > + struct dw_edma *dw = chip->dw; > > + > > const struct debugfs_entries debugfs_regs[] = { > > /* eDMA global registers */ > > RD_REGISTER(engine_en), > > @@ -242,7 +248,7 @@ static void dw_edma_debugfs_regs_rd(struct dentry *dir) > > nr_entries = ARRAY_SIZE(debugfs_regs); > > dw_edma_debugfs_create_x32(debugfs_regs, nr_entries, regs_dir); > > > > > - if (dw->mf == EDMA_MF_HDMA_COMPAT) { > > + if (chip->mf == EDMA_MF_HDMA_COMPAT) { > > ditto > > > nr_entries = ARRAY_SIZE(debugfs_unroll_regs); > > dw_edma_debugfs_create_x32(debugfs_unroll_regs, nr_entries, > > regs_dir); > > @@ -264,6 +270,7 @@ static void dw_edma_debugfs_regs_rd(struct dentry *dir) > > > > static void dw_edma_debugfs_regs(void) > > { > > + struct dw_edma *dw = chip->dw; > > const struct debugfs_entries debugfs_regs[] = { > > REGISTER(ctrl_data_arb_prior), > > REGISTER(ctrl), > > @@ -282,13 +289,15 @@ static void dw_edma_debugfs_regs(void) > > dw_edma_debugfs_regs_rd(regs_dir); > > } > > > > > -void dw_edma_v0_debugfs_on(struct dw_edma_chip *chip) > > +void dw_edma_v0_debugfs_on(struct dw_edma_chip *p) > > { > > - dw = chip->dw; > > - if (!dw) > > + struct dw_edma *dw; > > + chip = p; > > + if (!chip) > > return; > > > > - regs = dw->rg_region.vaddr; > > + dw = chip->dw; > > + regs = chip->reg_base; > > As I said this is unrelated change. Please unpin to another patch. > > > if (!regs) > > return; > > > > @@ -296,19 +305,19 @@ void dw_edma_v0_debugfs_on(struct dw_edma_chip *chip) > > if (!dw->debugfs) > > return; > > > > > - debugfs_create_u32("mf", 0444, dw->debugfs, &dw->mf); > > + debugfs_create_u32("mf", 0444, dw->debugfs, &chip->mf); > > "dw->chip->..." > > > debugfs_create_u16("wr_ch_cnt", 0444, dw->debugfs, &dw->wr_ch_cnt); > > debugfs_create_u16("rd_ch_cnt", 0444, dw->debugfs, &dw->rd_ch_cnt); > > > > dw_edma_debugfs_regs(); > > } > > > > -void dw_edma_v0_debugfs_off(struct dw_edma_chip *chip) > > +void dw_edma_v0_debugfs_off(struct dw_edma_chip *p) > > { > > - dw = chip->dw; > > - if (!dw) > > + chip = p; > > + if (!chip) > > return; > > > > - debugfs_remove_recursive(dw->debugfs); > > - dw->debugfs = NULL; > > + debugfs_remove_recursive(chip->dw->debugfs); > > This won't be needed. > > > + chip->dw->debugfs = NULL; > > } > > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h > > index cab6e18773dad..fcfbc0f47f83d 100644 > > --- a/include/linux/dma/edma.h > > +++ b/include/linux/dma/edma.h > > @@ -12,19 +12,62 @@ > > #include <linux/device.h> > > #include <linux/dmaengine.h> > > > > +#define EDMA_MAX_WR_CH 8 > > +#define EDMA_MAX_RD_CH 8 > > + > > > struct dw_edma; > > This could be dropped. > > > > > +struct dw_edma_region { > > + phys_addr_t paddr; > > + void __iomem *vaddr; > > + size_t sz; > > +}; > > + > > +struct dw_edma_core_ops { > > + int (*irq_vector)(struct device *dev, unsigned int nr); > > +}; > > + > > +enum dw_edma_map_format { > > + EDMA_MF_EDMA_LEGACY = 0x0, > > + EDMA_MF_EDMA_UNROLL = 0x1, > > + EDMA_MF_HDMA_COMPAT = 0x5 > > +}; > > + > > /** > > * struct dw_edma_chip - representation of DesignWare eDMA controller hardware > > * @dev: struct device of the eDMA controller > > * @id: instance ID > > > - * @irq: irq line > > + * @nr_irqs: total dma irq number > > Indeed, dw_edma_chip->irq field has been unused anyway... > > > + * reg64bit if support 64bit write to register > > Do you have this field in the structure below? I don't see it there. > Drop this line then. > > > + * @ops DMA channel to IRQ number mapping > > + * @wr_ch_cnt DMA write channel number > > + * @rd_ch_cnt DMA read channel number > > > + * @rg_region DMA register region > > You changed this to reg_base in the structure below, but the doc has > been left with the old name and field type. Please unpin the modification > to a dedicated patch as I suggested before. > > > + * @ll_region_wr DMA descriptor link list memory for write channel > > + * @ll_region_rd DMA descriptor link list memory for read channel > > + * @mf DMA register map format > > * @dw: struct dw_edma that is filed by dw_edma_probe() > > */ > > struct dw_edma_chip { > > struct device *dev; > > int id; > > - int irq; > > + int nr_irqs; > > + const struct dw_edma_core_ops *ops; > > + > > + void __iomem *reg_base; > > + > > + u16 ll_wr_cnt; > > + u16 ll_rd_cnt; > > Why did you name these fields with "ll_" prefix? These are the number of > read/write channels. Moreover the structure doc above have them named as > "wr_ch_cnt" and "rd_ch_cnt". So if you want to change the fields names > please add an addition patch and justify why it's needed. > > > + /* link list address */ > > + struct dw_edma_region ll_region_wr[EDMA_MAX_WR_CH]; > > + struct dw_edma_region ll_region_rd[EDMA_MAX_RD_CH]; > > + > > + /* data region */ > > + struct dw_edma_region dt_region_wr[EDMA_MAX_WR_CH]; > > + struct dw_edma_region dt_region_rd[EDMA_MAX_RD_CH]; > > + > > + enum dw_edma_map_format mf; > > + > > > struct dw_edma *dw; > > Finally this could be dropped. Thus the dw_edma_chip structure will > be just the chip info data. > > -Sergey > > > }; > > > > -- > > 2.24.0.rc1 > >
On Wed, Mar 09, 2022 at 10:37:51AM -0600, Zhi Li wrote: > On Wed, Mar 9, 2022 at 7:39 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > Hello Frank > > > > On Mon, Mar 07, 2022 at 04:47:45PM -0600, Frank Li wrote: > > > "struct dw_edma_chip" contains an internal structure "struct dw_edma" that is > > > used by the eDMA core internally. This structure should not be touched > > > by the eDMA controller drivers themselves. But currently, the eDMA > > > controller drivers like "dw-edma-pci" allocates and populates this > > > internal structure then passes it on to eDMA core. The eDMA core further > > > populates the structure and uses it. This is wrong! > > > > > > Hence, move all the "struct dw_edma" specifics from controller drivers > > > to the eDMA core. > > > > Thanks for the patchset. Alas it has just drawn my attention on v3 > > stage, otherwise I would have given to you my thoughts stright away on > > v1. Anyway first of all a cover letter would be very much appropriate > > to have a general notion about all the changes introduced in the set. > > > > Secondly I've just been working on adding the eDMA platform support > > myself, so you have been just about a week ahead of me submitting my > > changes. My work contains some of the modifications done by you (but > > have some additional fixes too) so I'll need to rebase it on top of > > your patchset when it's finished. Anyway am I understand correctly, > > that you've also been working on the DW PCIe driver alteration so one > > would properly initialize the eDMA-chip data structure? If so have you > > sent the patchset already? Could you give me a link and add me to Cc > > in the emailing thread? (That's where the cover letter with all the > > info and related patchsets would be very helpful.) > > > > Thirdly regarding this patch. Your modification is mainly correct, but > > I would suggest to change the concept. Instead of needlessly converting > > the code to using the dw_edma_chip structure pointer within the DW eDMA > > driver, it would be much easier in modification and more logical to > > keep using the struct dw_edma pointer. Especially seeing dw_edma > > structure is going to be a pure private data. So to speak what I would > > suggest is to have the next pointers setup: > > > > include/linux/dma/edma.h: > > struct dw_edma_chip { > > /* drop struct dw_edma *dw; */ > > }; > > The key problem at dw_edma_remove(struct dw_edma_chip *chip) > It needs dw information to do clean up work. Not a problem. Just leave the structure declaration in the DW eDMA global header file: include/linux/dma/edma.h: + struct dw_edma; and return the eDMA-descriptor from the probe method: struct dw_edma *dw_edma_probe(struct dw_edma_chip *chip); Then the probe-method callee will be able to clean all the eDMA driver data up by passing the pointer to the eDMA data to the remove method: void dw_edma_remove(struct dw_edma *); -Sergey > > > > > drivers/dma/dw-edma/dw-edma-core.h: > > struct dw_edma { > > const struct dw_edma_chip *chip; > > }; > > > > struct dw_edma_chan { > > struct dw_edma *dw; > > }; > > > > Thus we'll have a cleaner concept here: > > struct dw_edma_chip - DW eDMA controller descriptor, defined and > > initialized in the client platform drivers. Can be static and const in > > general (which in your approach is impossible). > > struct dw_edma - pure DW eDMA driver private data. It will be used > > the within the local driver only and won't be seen outside. > > > > Thus you won't need an almost a half of the modifications below, > > would provide a cleaner interface, would support the const > > static DW eDMA chip info objects. > > > > More comments regarding the suggested approach and some additional > > notes are below. > > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > > --- > > > > > > Resend added dmaengine@vger.kernel.org > > > > > > Change from v2 to v3 > > > - none > > > Change from v1 to v2 > > > - rework commit message > > > - remove duplicate field in struct dw_edma > > > > > > drivers/dma/dw-edma/dw-edma-core.c | 91 +++++----- > > > drivers/dma/dw-edma/dw-edma-core.h | 30 +--- > > > drivers/dma/dw-edma/dw-edma-v0-core.c | 206 ++++++++++++----------- > > > drivers/dma/dw-edma/dw-edma-v0-core.h | 8 +- > > > drivers/dma/dw-edma/dw-edma-v0-debugfs.c | 37 ++-- > > > include/linux/dma/edma.h | 47 +++++- > > > 6 files changed, 230 insertions(+), 189 deletions(-) > > > > > > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c > > > index 53289927dd0d6..0cb66434f9e14 100644 > > > --- a/drivers/dma/dw-edma/dw-edma-core.c > > > +++ b/drivers/dma/dw-edma/dw-edma-core.c > > > @@ -65,7 +65,7 @@ static struct dw_edma_burst *dw_edma_alloc_burst(struct dw_edma_chunk *chunk) > > > static struct dw_edma_chunk *dw_edma_alloc_chunk(struct dw_edma_desc *desc) > > > { > > > struct dw_edma_chan *chan = desc->chan; > > > > > - struct dw_edma *dw = chan->chip->dw; > > > + struct dw_edma_chip *chip = chan->chip; > > > > If you do as I suggest you won't need to modify this part. > > > > > struct dw_edma_chunk *chunk; > > > > > > chunk = kzalloc(sizeof(*chunk), GFP_NOWAIT); > > > @@ -82,11 +82,11 @@ static struct dw_edma_chunk *dw_edma_alloc_chunk(struct dw_edma_desc *desc) > > > */ > > > chunk->cb = !(desc->chunks_alloc % 2); > > > if (chan->dir == EDMA_DIR_WRITE) { > > > > > - chunk->ll_region.paddr = dw->ll_region_wr[chan->id].paddr; > > > - chunk->ll_region.vaddr = dw->ll_region_wr[chan->id].vaddr; > > > + chunk->ll_region.paddr = chip->ll_region_wr[chan->id].paddr; > > > + chunk->ll_region.vaddr = chip->ll_region_wr[chan->id].vaddr; > > > } else { > > > - chunk->ll_region.paddr = dw->ll_region_rd[chan->id].paddr; > > > - chunk->ll_region.vaddr = dw->ll_region_rd[chan->id].vaddr; > > > + chunk->ll_region.paddr = chip->ll_region_rd[chan->id].paddr; > > > + chunk->ll_region.vaddr = chip->ll_region_rd[chan->id].vaddr; > > > } > > > > Here you would just need to change the pointers "sandwich" to > > "dw->chip->...". > > > > > > > > if (desc->chunk) { > > > @@ -601,7 +601,8 @@ static void dw_edma_abort_interrupt(struct dw_edma_chan *chan) > > > static irqreturn_t dw_edma_interrupt(int irq, void *data, bool write) > > > { > > > struct dw_edma_irq *dw_irq = data; > > > > > - struct dw_edma *dw = dw_irq->dw; > > > + struct dw_edma_chip *chip = dw_irq->chip; > > > + struct dw_edma *dw = chip->dw; > > > > This also would have been unneeded, since the method relies on the data > > from the dw_edma structure. > > > > > unsigned long total, pos, val; > > > unsigned long off; > > > u32 mask; > > > @@ -616,7 +617,7 @@ static irqreturn_t dw_edma_interrupt(int irq, void *data, bool write) > > > mask = dw_irq->rd_mask; > > > } > > > > > > > > - val = dw_edma_v0_core_status_done_int(dw, write ? > > > + val = dw_edma_v0_core_status_done_int(chip, write ? > > > EDMA_DIR_WRITE : > > > EDMA_DIR_READ); > > > val &= mask; > > > @@ -626,7 +627,7 @@ static irqreturn_t dw_edma_interrupt(int irq, void *data, bool write) > > > dw_edma_done_interrupt(chan); > > > } > > > > > > - val = dw_edma_v0_core_status_abort_int(dw, write ? > > > + val = dw_edma_v0_core_status_abort_int(chip, write ? > > > EDMA_DIR_WRITE : > > > EDMA_DIR_READ); > > > > This won't be needed. > > > > > val &= mask; > > > @@ -718,7 +719,7 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write, > > > } > > > > > > INIT_LIST_HEAD(&dma->channels); > > > > > - for (j = 0; (alloc || dw->nr_irqs == 1) && j < cnt; j++, i++) { > > > + for (j = 0; (alloc || chip->nr_irqs == 1) && j < cnt; j++, i++) { > > > chan = &dw->chan[i]; > > > > > > dt_region = devm_kzalloc(dev, sizeof(*dt_region), GFP_KERNEL); > > > @@ -735,15 +736,15 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write, > > > chan->status = EDMA_ST_IDLE; > > > > > > if (write) > > > - chan->ll_max = (dw->ll_region_wr[j].sz / EDMA_LL_SZ); > > > + chan->ll_max = (chip->ll_region_wr[j].sz / EDMA_LL_SZ); > > > else > > > - chan->ll_max = (dw->ll_region_rd[j].sz / EDMA_LL_SZ); > > > + chan->ll_max = (chip->ll_region_rd[j].sz / EDMA_LL_SZ); > > > chan->ll_max -= 1; > > > > > > dev_vdbg(dev, "L. List:\tChannel %s[%u] max_cnt=%u\n", > > > write ? "write" : "read", j, chan->ll_max); > > > > > > - if (dw->nr_irqs == 1) > > > + if (chip->nr_irqs == 1) > > > > This would have been changed to using the "dw->chip->..." pattern. > > > > > pos = 0; > > > else > > > pos = off_alloc + (j % alloc); > > > @@ -755,7 +756,7 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write, > > > else > > > irq->rd_mask |= BIT(j); > > > > > > > > - irq->dw = dw; > > > + irq->chip = chip; > > > > Won't be needed. > > > > > memcpy(&chan->msi, &irq->msi, sizeof(chan->msi)); > > > > > > dev_vdbg(dev, "MSI:\t\tChannel %s[%u] addr=0x%.8x%.8x, data=0x%.8x\n", > > > @@ -767,13 +768,13 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write, > > > vchan_init(&chan->vc, dma); > > > > > > if (write) { > > > > > - dt_region->paddr = dw->dt_region_wr[j].paddr; > > > - dt_region->vaddr = dw->dt_region_wr[j].vaddr; > > > - dt_region->sz = dw->dt_region_wr[j].sz; > > > + dt_region->paddr = chip->dt_region_wr[j].paddr; > > > + dt_region->vaddr = chip->dt_region_wr[j].vaddr; > > > + dt_region->sz = chip->dt_region_wr[j].sz; > > > } else { > > > - dt_region->paddr = dw->dt_region_rd[j].paddr; > > > - dt_region->vaddr = dw->dt_region_rd[j].vaddr; > > > - dt_region->sz = dw->dt_region_rd[j].sz; > > > + dt_region->paddr = chip->dt_region_rd[j].paddr; > > > + dt_region->vaddr = chip->dt_region_rd[j].vaddr; > > > + dt_region->sz = chip->dt_region_rd[j].sz; > > > > Just replace with "dw->chip->..." > > > > > } > > > > > > dw_edma_v0_core_device_config(chan); > > > @@ -840,16 +841,16 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, > > > > > > ch_cnt = dw->wr_ch_cnt + dw->rd_ch_cnt; > > > > > > > > - if (dw->nr_irqs < 1) > > > + if (chip->nr_irqs < 1) > > > return -EINVAL; > > > > > > - if (dw->nr_irqs == 1) { > > > + if (chip->nr_irqs == 1) { > > > /* Common IRQ shared among all channels */ > > > - irq = dw->ops->irq_vector(dev, 0); > > > + irq = chip->ops->irq_vector(dev, 0); > > > err = request_irq(irq, dw_edma_interrupt_common, > > > IRQF_SHARED, dw->name, &dw->irq[0]); > > > if (err) { > > > - dw->nr_irqs = 0; > > > + chip->nr_irqs = 0; > > > return err; > > > } > > > > > > @@ -857,7 +858,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, > > > get_cached_msi_msg(irq, &dw->irq[0].msi); > > > } else { > > > /* Distribute IRQs equally among all channels */ > > > - int tmp = dw->nr_irqs; > > > + int tmp = chip->nr_irqs; > > > > > > while (tmp && (*wr_alloc + *rd_alloc) < ch_cnt) { > > > dw_edma_dec_irq_alloc(&tmp, wr_alloc, dw->wr_ch_cnt); > > > @@ -868,7 +869,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, > > > dw_edma_add_irq_mask(&rd_mask, *rd_alloc, dw->rd_ch_cnt); > > > > > > for (i = 0; i < (*wr_alloc + *rd_alloc); i++) { > > > - irq = dw->ops->irq_vector(dev, i); > > > + irq = chip->ops->irq_vector(dev, i); > > > err = request_irq(irq, > > > i < *wr_alloc ? > > > dw_edma_interrupt_write : > > > @@ -876,7 +877,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, > > > IRQF_SHARED, dw->name, > > > &dw->irq[i]); > > > if (err) { > > > - dw->nr_irqs = i; > > > + chip->nr_irqs = i; > > > return err; > > > } > > > > > > @@ -884,7 +885,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, > > > get_cached_msi_msg(irq, &dw->irq[i].msi); > > > } > > > > > > - dw->nr_irqs = i; > > > + chip->nr_irqs = i; > > > > ditto > > > > > } > > > > > > return err; > > > @@ -905,18 +906,24 @@ int dw_edma_probe(struct dw_edma_chip *chip) > > > if (!dev) > > > return -EINVAL; > > > > > > - dw = chip->dw; > > > - if (!dw || !dw->irq || !dw->ops || !dw->ops->irq_vector) > > > + dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL); > > > + if (!dw) > > > + return -ENOMEM; > > > + > > > > > + chip->dw = dw; > > > > Convert this to "dw->chip = chip". > > > > > + > > > + if (!chip->nr_irqs || !chip->ops) > > > return -EINVAL; > > > > Move this to be performed before the pointer initialization, since > > it's pointless to initialize anything if invalid data is passed. > > You can join it in with the "if (!dev)" statement. > > > > > > > > raw_spin_lock_init(&dw->lock); > > > > > > - dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt, > > > - dw_edma_v0_core_ch_count(dw, EDMA_DIR_WRITE)); > > > + > > > > > + dw->wr_ch_cnt = min_t(u16, chip->ll_wr_cnt, > > > + dw_edma_v0_core_ch_count(chip, EDMA_DIR_WRITE)); > > > dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt, EDMA_MAX_WR_CH); > > > > > > - dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt, > > > - dw_edma_v0_core_ch_count(dw, EDMA_DIR_READ)); > > > + dw->rd_ch_cnt = min_t(u16, chip->ll_rd_cnt, > > > > Regarding the field naming please see my comment to the dw_edma_chip > > structure. > > > > > + dw_edma_v0_core_ch_count(chip, EDMA_DIR_READ)); > > > dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt, EDMA_MAX_RD_CH); > > > > > > if (!dw->wr_ch_cnt && !dw->rd_ch_cnt) > > > @@ -934,7 +941,11 @@ int dw_edma_probe(struct dw_edma_chip *chip) > > > snprintf(dw->name, sizeof(dw->name), "dw-edma-core:%d", chip->id); > > > > > > /* Disable eDMA, only to establish the ideal initial conditions */ > > > > > - dw_edma_v0_core_off(dw); > > > + dw_edma_v0_core_off(chip); > > > + > > > + dw->irq = devm_kcalloc(dev, chip->nr_irqs, sizeof(*dw->irq), GFP_KERNEL); > > > + if (!dw->irq) > > > + return -ENOMEM; > > > > ditto > > > > > > > > /* Request IRQs */ > > > err = dw_edma_irq_request(chip, &wr_alloc, &rd_alloc); > > > @@ -960,10 +971,10 @@ int dw_edma_probe(struct dw_edma_chip *chip) > > > return 0; > > > > > > err_irq_free: > > > > > - for (i = (dw->nr_irqs - 1); i >= 0; i--) > > > - free_irq(dw->ops->irq_vector(dev, i), &dw->irq[i]); > > > + for (i = (chip->nr_irqs - 1); i >= 0; i--) > > > + free_irq(chip->ops->irq_vector(dev, i), &dw->irq[i]); > > > > > > - dw->nr_irqs = 0; > > > + chip->nr_irqs = 0; > > > > dw->chip->... > > > > > > > > return err; > > > } > > > @@ -977,11 +988,11 @@ int dw_edma_remove(struct dw_edma_chip *chip) > > > int i; > > > > > > /* Disable eDMA */ > > > > > - dw_edma_v0_core_off(dw); > > > + dw_edma_v0_core_off(chip); > > > > Won't need this. > > > > > > > > /* Free irqs */ > > > - for (i = (dw->nr_irqs - 1); i >= 0; i--) > > > - free_irq(dw->ops->irq_vector(dev, i), &dw->irq[i]); > > > + for (i = (chip->nr_irqs - 1); i >= 0; i--) > > > + free_irq(chip->ops->irq_vector(dev, i), &dw->irq[i]); > > > > Use "dw->chip->..." > > > > > > > > /* Power management */ > > > pm_runtime_disable(dev); > > > diff --git a/drivers/dma/dw-edma/dw-edma-core.h b/drivers/dma/dw-edma/dw-edma-core.h > > > index 60316d408c3e0..885f6719c9462 100644 > > > --- a/drivers/dma/dw-edma/dw-edma-core.h > > > +++ b/drivers/dma/dw-edma/dw-edma-core.h > > > @@ -15,20 +15,12 @@ > > > #include "../virt-dma.h" > > > > > > #define EDMA_LL_SZ 24 > > > -#define EDMA_MAX_WR_CH 8 > > > -#define EDMA_MAX_RD_CH 8 > > > > > > enum dw_edma_dir { > > > EDMA_DIR_WRITE = 0, > > > EDMA_DIR_READ > > > }; > > > > > > -enum dw_edma_map_format { > > > - EDMA_MF_EDMA_LEGACY = 0x0, > > > - EDMA_MF_EDMA_UNROLL = 0x1, > > > - EDMA_MF_HDMA_COMPAT = 0x5 > > > -}; > > > - > > > enum dw_edma_request { > > > EDMA_REQ_NONE = 0, > > > EDMA_REQ_STOP, > > > @@ -57,12 +49,6 @@ struct dw_edma_burst { > > > u32 sz; > > > }; > > > > > > -struct dw_edma_region { > > > - phys_addr_t paddr; > > > - void __iomem *vaddr; > > > - size_t sz; > > > -}; > > > - > > > struct dw_edma_chunk { > > > struct list_head list; > > > struct dw_edma_chan *chan; > > > @@ -106,11 +92,7 @@ struct dw_edma_irq { > > > struct msi_msg msi; > > > u32 wr_mask; > > > u32 rd_mask; > > > - struct dw_edma *dw; > > > -}; > > > - > > > -struct dw_edma_core_ops { > > > - int (*irq_vector)(struct device *dev, unsigned int nr); > > > + struct dw_edma_chip *chip; > > > }; > > > > > > struct dw_edma { > > > @@ -122,19 +104,9 @@ struct dw_edma { > > > struct dma_device rd_edma; > > > u16 rd_ch_cnt; > > > > > > > > - struct dw_edma_region rg_region; /* Registers */ > > > > AFAICS you replaced this with void __iomem *reg_base further in the > > dw_edma_chip structure. Even though I do agree with this modification, > > it's conceptually right, but the alteration is unrelated to the patch > > topic. So please unpin it to a dedicated patch placed before this one > > in the series. > > > > > - struct dw_edma_region ll_region_wr[EDMA_MAX_WR_CH]; > > > - struct dw_edma_region ll_region_rd[EDMA_MAX_RD_CH]; > > > - struct dw_edma_region dt_region_wr[EDMA_MAX_WR_CH]; > > > - struct dw_edma_region dt_region_rd[EDMA_MAX_RD_CH]; > > > - > > > struct dw_edma_irq *irq; > > > - int nr_irqs; > > > - > > > - enum dw_edma_map_format mf; > > > > > > struct dw_edma_chan *chan; > > > - const struct dw_edma_core_ops *ops; > > > > > > raw_spinlock_t lock; /* Only for legacy */ > > > #ifdef CONFIG_DEBUG_FS > > > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c > > > index 329fc2e57b703..6e2f83e31a03a 100644 > > > --- a/drivers/dma/dw-edma/dw-edma-v0-core.c > > > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c > > > @@ -23,92 +23,94 @@ enum dw_edma_control { > > > DW_EDMA_V0_LLE = BIT(9), > > > }; > > > > > > > > -static inline struct dw_edma_v0_regs __iomem *__dw_regs(struct dw_edma *dw) > > > +static inline struct dw_edma_v0_regs __iomem *__dw_regs(struct dw_edma_chip *chip) > > > { > > > - return dw->rg_region.vaddr; > > > + return chip->reg_base; > > > } > > > > Just use "dw->chip->reg_base" here and you won't need to introduce the > > most of the modifications below, since all of the them is connected > > with the __dw_regs() function prototype change. > > > > > > > > -#define SET_32(dw, name, value) \ > > > - writel(value, &(__dw_regs(dw)->name)) > > > +#define SET_32(chip, name, value) \ > > > + writel(value, &(__dw_regs(chip)->name)) > > > > > > -#define GET_32(dw, name) \ > > > - readl(&(__dw_regs(dw)->name)) > > > +#define GET_32(chip, name) \ > > > + readl(&(__dw_regs(chip)->name)) > > > > > > -#define SET_RW_32(dw, dir, name, value) \ > > > +#define SET_RW_32(chip, dir, name, value) \ > > > do { \ > > > if ((dir) == EDMA_DIR_WRITE) \ > > > - SET_32(dw, wr_##name, value); \ > > > + SET_32(chip, wr_##name, value); \ > > > else \ > > > - SET_32(dw, rd_##name, value); \ > > > + SET_32(chip, rd_##name, value); \ > > > } while (0) > > > > > > -#define GET_RW_32(dw, dir, name) \ > > > +#define GET_RW_32(chip, dir, name) \ > > > ((dir) == EDMA_DIR_WRITE \ > > > - ? GET_32(dw, wr_##name) \ > > > - : GET_32(dw, rd_##name)) > > > + ? GET_32(chip, wr_##name) \ > > > + : GET_32(chip, rd_##name)) > > > > > > -#define SET_BOTH_32(dw, name, value) \ > > > +#define SET_BOTH_32(chip, name, value) \ > > > do { \ > > > - SET_32(dw, wr_##name, value); \ > > > - SET_32(dw, rd_##name, value); \ > > > + SET_32(chip, wr_##name, value); \ > > > + SET_32(chip, rd_##name, value); \ > > > } while (0) > > > > > > #ifdef CONFIG_64BIT > > > > > > -#define SET_64(dw, name, value) \ > > > - writeq(value, &(__dw_regs(dw)->name)) > > > +#define SET_64(chip, name, value) \ > > > + writeq(value, &(__dw_regs(chip)->name)) > > > > > > -#define GET_64(dw, name) \ > > > - readq(&(__dw_regs(dw)->name)) > > > +#define GET_64(chip, name) \ > > > + readq(&(__dw_regs(chip)->name)) > > > > > > -#define SET_RW_64(dw, dir, name, value) \ > > > +#define SET_RW_64(chip, dir, name, value) \ > > > do { \ > > > if ((dir) == EDMA_DIR_WRITE) \ > > > - SET_64(dw, wr_##name, value); \ > > > + SET_64(chip, wr_##name, value); \ > > > else \ > > > - SET_64(dw, rd_##name, value); \ > > > + SET_64(chip, rd_##name, value); \ > > > } while (0) > > > > > > -#define GET_RW_64(dw, dir, name) \ > > > +#define GET_RW_64(chip, dir, name) \ > > > ((dir) == EDMA_DIR_WRITE \ > > > - ? GET_64(dw, wr_##name) \ > > > - : GET_64(dw, rd_##name)) > > > + ? GET_64(chip, wr_##name) \ > > > + : GET_64(chip, rd_##name)) > > > > > > -#define SET_BOTH_64(dw, name, value) \ > > > +#define SET_BOTH_64(chip, name, value) \ > > > do { \ > > > - SET_64(dw, wr_##name, value); \ > > > - SET_64(dw, rd_##name, value); \ > > > + SET_64(chip, wr_##name, value); \ > > > + SET_64(chip, rd_##name, value); \ > > > } while (0) > > > > > > #endif /* CONFIG_64BIT */ > > > > > > -#define SET_COMPAT(dw, name, value) \ > > > - writel(value, &(__dw_regs(dw)->type.unroll.name)) > > > +#define SET_COMPAT(chip, name, value) \ > > > + writel(value, &(__dw_regs(chip)->type.unroll.name)) > > > > > > -#define SET_RW_COMPAT(dw, dir, name, value) \ > > > +#define SET_RW_COMPAT(chip, dir, name, value) \ > > > do { \ > > > if ((dir) == EDMA_DIR_WRITE) \ > > > - SET_COMPAT(dw, wr_##name, value); \ > > > + SET_COMPAT(chip, wr_##name, value); \ > > > else \ > > > - SET_COMPAT(dw, rd_##name, value); \ > > > + SET_COMPAT(chip, rd_##name, value); \ > > > } while (0) > > > > > > static inline struct dw_edma_v0_ch_regs __iomem * > > > -__dw_ch_regs(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch) > > > +__dw_ch_regs(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch) > > > { > > > - if (dw->mf == EDMA_MF_EDMA_LEGACY) > > > - return &(__dw_regs(dw)->type.legacy.ch); > > > + if (chip->mf == EDMA_MF_EDMA_LEGACY) > > > + return &(__dw_regs(chip)->type.legacy.ch); > > > > > > if (dir == EDMA_DIR_WRITE) > > > - return &__dw_regs(dw)->type.unroll.ch[ch].wr; > > > + return &__dw_regs(chip)->type.unroll.ch[ch].wr; > > > > > > - return &__dw_regs(dw)->type.unroll.ch[ch].rd; > > > + return &__dw_regs(chip)->type.unroll.ch[ch].rd; > > > } > > > > > > -static inline void writel_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > > +static inline void writel_ch(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch, > > > u32 value, void __iomem *addr) > > > { > > > - if (dw->mf == EDMA_MF_EDMA_LEGACY) { > > > + struct dw_edma *dw = chip->dw; > > > + > > > + if (chip->mf == EDMA_MF_EDMA_LEGACY) { > > > u32 viewport_sel; > > > unsigned long flags; > > > > > > @@ -119,7 +121,7 @@ static inline void writel_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > > viewport_sel |= BIT(31); > > > > > > writel(viewport_sel, > > > - &(__dw_regs(dw)->type.legacy.viewport_sel)); > > > + &(__dw_regs(chip)->type.legacy.viewport_sel)); > > > writel(value, addr); > > > > > > raw_spin_unlock_irqrestore(&dw->lock, flags); > > > @@ -128,12 +130,13 @@ static inline void writel_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > > } > > > } > > > > > > -static inline u32 readl_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > > +static inline u32 readl_ch(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch, > > > const void __iomem *addr) > > > { > > > + struct dw_edma *dw = chip->dw; > > > u32 value; > > > > > > - if (dw->mf == EDMA_MF_EDMA_LEGACY) { > > > + if (chip->mf == EDMA_MF_EDMA_LEGACY) { > > > u32 viewport_sel; > > > unsigned long flags; > > > > > > @@ -144,7 +147,7 @@ static inline u32 readl_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > > viewport_sel |= BIT(31); > > > > > > writel(viewport_sel, > > > - &(__dw_regs(dw)->type.legacy.viewport_sel)); > > > + &(__dw_regs(chip)->type.legacy.viewport_sel)); > > > value = readl(addr); > > > > > > raw_spin_unlock_irqrestore(&dw->lock, flags); > > > @@ -166,10 +169,12 @@ static inline u32 readl_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > > > > > #ifdef CONFIG_64BIT > > > > > > -static inline void writeq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > > +static inline void writeq_ch(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch, > > > u64 value, void __iomem *addr) > > > { > > > - if (dw->mf == EDMA_MF_EDMA_LEGACY) { > > > + struct dw_edma *dw = chip->dw; > > > + > > > + if (chip->mf == EDMA_MF_EDMA_LEGACY) { > > > u32 viewport_sel; > > > unsigned long flags; > > > > > > @@ -180,7 +185,7 @@ static inline void writeq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > > viewport_sel |= BIT(31); > > > > > > writel(viewport_sel, > > > - &(__dw_regs(dw)->type.legacy.viewport_sel)); > > > + &(__dw_regs(chip)->type.legacy.viewport_sel)); > > > writeq(value, addr); > > > > > > raw_spin_unlock_irqrestore(&dw->lock, flags); > > > @@ -189,12 +194,13 @@ static inline void writeq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > > } > > > } > > > > > > -static inline u64 readq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > > +static inline u64 readq_ch(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch, > > > const void __iomem *addr) > > > { > > > + struct dw_edma *dw = chip->dw; > > > u32 value; > > > > > > - if (dw->mf == EDMA_MF_EDMA_LEGACY) { > > > + if (chip->mf == EDMA_MF_EDMA_LEGACY) { > > > u32 viewport_sel; > > > unsigned long flags; > > > > > > @@ -205,7 +211,7 @@ static inline u64 readq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > > viewport_sel |= BIT(31); > > > > > > writel(viewport_sel, > > > - &(__dw_regs(dw)->type.legacy.viewport_sel)); > > > + &(__dw_regs(chip)->type.legacy.viewport_sel)); > > > value = readq(addr); > > > > > > raw_spin_unlock_irqrestore(&dw->lock, flags); > > > @@ -228,25 +234,25 @@ static inline u64 readq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, > > > #endif /* CONFIG_64BIT */ > > > > > > /* eDMA management callbacks */ > > > -void dw_edma_v0_core_off(struct dw_edma *dw) > > > +void dw_edma_v0_core_off(struct dw_edma_chip *chip) > > > { > > > - SET_BOTH_32(dw, int_mask, > > > + SET_BOTH_32(chip, int_mask, > > > EDMA_V0_DONE_INT_MASK | EDMA_V0_ABORT_INT_MASK); > > > - SET_BOTH_32(dw, int_clear, > > > + SET_BOTH_32(chip, int_clear, > > > EDMA_V0_DONE_INT_MASK | EDMA_V0_ABORT_INT_MASK); > > > - SET_BOTH_32(dw, engine_en, 0); > > > + SET_BOTH_32(chip, engine_en, 0); > > > } > > > > > > -u16 dw_edma_v0_core_ch_count(struct dw_edma *dw, enum dw_edma_dir dir) > > > +u16 dw_edma_v0_core_ch_count(struct dw_edma_chip *chip, enum dw_edma_dir dir) > > > { > > > u32 num_ch; > > > > > > if (dir == EDMA_DIR_WRITE) > > > num_ch = FIELD_GET(EDMA_V0_WRITE_CH_COUNT_MASK, > > > - GET_32(dw, ctrl)); > > > + GET_32(chip, ctrl)); > > > else > > > num_ch = FIELD_GET(EDMA_V0_READ_CH_COUNT_MASK, > > > - GET_32(dw, ctrl)); > > > + GET_32(chip, ctrl)); > > > > > > if (num_ch > EDMA_V0_MAX_NR_CH) > > > num_ch = EDMA_V0_MAX_NR_CH; > > > @@ -256,11 +262,11 @@ u16 dw_edma_v0_core_ch_count(struct dw_edma *dw, enum dw_edma_dir dir) > > > > So can omit most of the changes from the comment above and up to this > > comment and use "dw->chip->..." where it's required. > > > > > > > > enum dma_status dw_edma_v0_core_ch_status(struct dw_edma_chan *chan) > > > { > > > > > - struct dw_edma *dw = chan->chip->dw; > > > + struct dw_edma_chip *chip = chan->chip; > > > > Just use "dw = chan->dw" here or directly use "chan->dw" in the > > method. > > > > > u32 tmp; > > > > > > tmp = FIELD_GET(EDMA_V0_CH_STATUS_MASK, > > > - GET_CH_32(dw, chan->dir, chan->id, ch_control1)); > > > + GET_CH_32(chip, chan->dir, chan->id, ch_control1)); > > > > > > if (tmp == 1) > > > return DMA_IN_PROGRESS; > > > @@ -272,30 +278,30 @@ enum dma_status dw_edma_v0_core_ch_status(struct dw_edma_chan *chan) > > > > > > void dw_edma_v0_core_clear_done_int(struct dw_edma_chan *chan) > > > { > > > > > - struct dw_edma *dw = chan->chip->dw; > > > + struct dw_edma_chip *chip = chan->chip; > > > > ditto > > > > > > > > - SET_RW_32(dw, chan->dir, int_clear, > > > + SET_RW_32(chip, chan->dir, int_clear, > > > FIELD_PREP(EDMA_V0_DONE_INT_MASK, BIT(chan->id))); > > > } > > > > > > void dw_edma_v0_core_clear_abort_int(struct dw_edma_chan *chan) > > > { > > > > > - struct dw_edma *dw = chan->chip->dw; > > > + struct dw_edma_chip *chip = chan->chip; > > > > ditto > > > > > > > > > > - SET_RW_32(dw, chan->dir, int_clear, > > > + SET_RW_32(chip, chan->dir, int_clear, > > > FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id))); > > > } > > > > > > -u32 dw_edma_v0_core_status_done_int(struct dw_edma *dw, enum dw_edma_dir dir) > > > +u32 dw_edma_v0_core_status_done_int(struct dw_edma_chip *chip, enum dw_edma_dir dir) > > > { > > > return FIELD_GET(EDMA_V0_DONE_INT_MASK, > > > - GET_RW_32(dw, dir, int_status)); > > > + GET_RW_32(chip, dir, int_status)); > > > } > > > > > > -u32 dw_edma_v0_core_status_abort_int(struct dw_edma *dw, enum dw_edma_dir dir) > > > +u32 dw_edma_v0_core_status_abort_int(struct dw_edma_chip *chip, enum dw_edma_dir dir) > > > { > > > return FIELD_GET(EDMA_V0_ABORT_INT_MASK, > > > - GET_RW_32(dw, dir, int_status)); > > > + GET_RW_32(chip, dir, int_status)); > > > > Won't be needed > > > > > } > > > > > > static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk) > > > @@ -357,109 +363,109 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk) > > > void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first) > > > { > > > struct dw_edma_chan *chan = chunk->chan; > > > > > - struct dw_edma *dw = chan->chip->dw; > > > + struct dw_edma_chip *chip = chan->chip; > > > > Just replace with "dw = chan->dw" or directly use "chan->dw" in the > > method. Thus you can omit the alterations below. > > > > > u32 tmp; > > > > > > dw_edma_v0_core_write_chunk(chunk); > > > > > > if (first) { > > > /* Enable engine */ > > > - SET_RW_32(dw, chan->dir, engine_en, BIT(0)); > > > - if (dw->mf == EDMA_MF_HDMA_COMPAT) { > > > + SET_RW_32(chip, chan->dir, engine_en, BIT(0)); > > > + if (chip->mf == EDMA_MF_HDMA_COMPAT) { > > > switch (chan->id) { > > > case 0: > > > - SET_RW_COMPAT(dw, chan->dir, ch0_pwr_en, > > > + SET_RW_COMPAT(chip, chan->dir, ch0_pwr_en, > > > BIT(0)); > > > break; > > > case 1: > > > - SET_RW_COMPAT(dw, chan->dir, ch1_pwr_en, > > > + SET_RW_COMPAT(chip, chan->dir, ch1_pwr_en, > > > BIT(0)); > > > break; > > > case 2: > > > - SET_RW_COMPAT(dw, chan->dir, ch2_pwr_en, > > > + SET_RW_COMPAT(chip, chan->dir, ch2_pwr_en, > > > BIT(0)); > > > break; > > > case 3: > > > - SET_RW_COMPAT(dw, chan->dir, ch3_pwr_en, > > > + SET_RW_COMPAT(chip, chan->dir, ch3_pwr_en, > > > BIT(0)); > > > break; > > > case 4: > > > - SET_RW_COMPAT(dw, chan->dir, ch4_pwr_en, > > > + SET_RW_COMPAT(chip, chan->dir, ch4_pwr_en, > > > BIT(0)); > > > break; > > > case 5: > > > - SET_RW_COMPAT(dw, chan->dir, ch5_pwr_en, > > > + SET_RW_COMPAT(chip, chan->dir, ch5_pwr_en, > > > BIT(0)); > > > break; > > > case 6: > > > - SET_RW_COMPAT(dw, chan->dir, ch6_pwr_en, > > > + SET_RW_COMPAT(chip, chan->dir, ch6_pwr_en, > > > BIT(0)); > > > break; > > > case 7: > > > - SET_RW_COMPAT(dw, chan->dir, ch7_pwr_en, > > > + SET_RW_COMPAT(chip, chan->dir, ch7_pwr_en, > > > BIT(0)); > > > break; > > > } > > > } > > > /* Interrupt unmask - done, abort */ > > > - tmp = GET_RW_32(dw, chan->dir, int_mask); > > > + tmp = GET_RW_32(chip, chan->dir, int_mask); > > > tmp &= ~FIELD_PREP(EDMA_V0_DONE_INT_MASK, BIT(chan->id)); > > > tmp &= ~FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id)); > > > - SET_RW_32(dw, chan->dir, int_mask, tmp); > > > + SET_RW_32(chip, chan->dir, int_mask, tmp); > > > /* Linked list error */ > > > - tmp = GET_RW_32(dw, chan->dir, linked_list_err_en); > > > + tmp = GET_RW_32(chip, chan->dir, linked_list_err_en); > > > tmp |= FIELD_PREP(EDMA_V0_LINKED_LIST_ERR_MASK, BIT(chan->id)); > > > - SET_RW_32(dw, chan->dir, linked_list_err_en, tmp); > > > + SET_RW_32(chip, chan->dir, linked_list_err_en, tmp); > > > /* Channel control */ > > > - SET_CH_32(dw, chan->dir, chan->id, ch_control1, > > > + SET_CH_32(chip, chan->dir, chan->id, ch_control1, > > > (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE)); > > > /* Linked list */ > > > #ifdef CONFIG_64BIT > > > - SET_CH_64(dw, chan->dir, chan->id, llp.reg, > > > + SET_CH_64(chip, chan->dir, chan->id, llp.reg, > > > chunk->ll_region.paddr); > > > #else /* CONFIG_64BIT */ > > > - SET_CH_32(dw, chan->dir, chan->id, llp.lsb, > > > + SET_CH_32(chip, chan->dir, chan->id, llp.lsb, > > > lower_32_bits(chunk->ll_region.paddr)); > > > - SET_CH_32(dw, chan->dir, chan->id, llp.msb, > > > + SET_CH_32(chip, chan->dir, chan->id, llp.msb, > > > upper_32_bits(chunk->ll_region.paddr)); > > > #endif /* CONFIG_64BIT */ > > > } > > > /* Doorbell */ > > > - SET_RW_32(dw, chan->dir, doorbell, > > > + SET_RW_32(chip, chan->dir, doorbell, > > > FIELD_PREP(EDMA_V0_DOORBELL_CH_MASK, chan->id)); > > > } > > > > You can drop the changes from the previous comment up to this one. > > > > > > > > int dw_edma_v0_core_device_config(struct dw_edma_chan *chan) > > > { > > > > > - struct dw_edma *dw = chan->chip->dw; > > > + struct dw_edma_chip *chip = chan->chip; > > > > Use "chan->dw" here. > > > > > u32 tmp = 0; > > > > > > /* MSI done addr - low, high */ > > > - SET_RW_32(dw, chan->dir, done_imwr.lsb, chan->msi.address_lo); > > > - SET_RW_32(dw, chan->dir, done_imwr.msb, chan->msi.address_hi); > > > + SET_RW_32(chip, chan->dir, done_imwr.lsb, chan->msi.address_lo); > > > + SET_RW_32(chip, chan->dir, done_imwr.msb, chan->msi.address_hi); > > > /* MSI abort addr - low, high */ > > > - SET_RW_32(dw, chan->dir, abort_imwr.lsb, chan->msi.address_lo); > > > - SET_RW_32(dw, chan->dir, abort_imwr.msb, chan->msi.address_hi); > > > + SET_RW_32(chip, chan->dir, abort_imwr.lsb, chan->msi.address_lo); > > > + SET_RW_32(chip, chan->dir, abort_imwr.msb, chan->msi.address_hi); > > > /* MSI data - low, high */ > > > switch (chan->id) { > > > case 0: > > > case 1: > > > - tmp = GET_RW_32(dw, chan->dir, ch01_imwr_data); > > > + tmp = GET_RW_32(chip, chan->dir, ch01_imwr_data); > > > break; > > > > > > case 2: > > > case 3: > > > - tmp = GET_RW_32(dw, chan->dir, ch23_imwr_data); > > > + tmp = GET_RW_32(chip, chan->dir, ch23_imwr_data); > > > break; > > > > > > case 4: > > > case 5: > > > - tmp = GET_RW_32(dw, chan->dir, ch45_imwr_data); > > > + tmp = GET_RW_32(chip, chan->dir, ch45_imwr_data); > > > break; > > > > > > case 6: > > > case 7: > > > - tmp = GET_RW_32(dw, chan->dir, ch67_imwr_data); > > > + tmp = GET_RW_32(chip, chan->dir, ch67_imwr_data); > > > break; > > > } > > > > > > @@ -478,22 +484,22 @@ int dw_edma_v0_core_device_config(struct dw_edma_chan *chan) > > > switch (chan->id) { > > > case 0: > > > case 1: > > > - SET_RW_32(dw, chan->dir, ch01_imwr_data, tmp); > > > + SET_RW_32(chip, chan->dir, ch01_imwr_data, tmp); > > > break; > > > > > > case 2: > > > case 3: > > > - SET_RW_32(dw, chan->dir, ch23_imwr_data, tmp); > > > + SET_RW_32(chip, chan->dir, ch23_imwr_data, tmp); > > > break; > > > > > > case 4: > > > case 5: > > > - SET_RW_32(dw, chan->dir, ch45_imwr_data, tmp); > > > + SET_RW_32(chip, chan->dir, ch45_imwr_data, tmp); > > > break; > > > > > > case 6: > > > case 7: > > > - SET_RW_32(dw, chan->dir, ch67_imwr_data, tmp); > > > + SET_RW_32(chip, chan->dir, ch67_imwr_data, tmp); > > > break; > > > > The changes above won't be needed if you keep using the dw pointer > > here as I suggest. > > > > > } > > > > > > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.h b/drivers/dma/dw-edma/dw-edma-v0-core.h > > > index 2afa626b8300c..01a29c74c0c43 100644 > > > --- a/drivers/dma/dw-edma/dw-edma-v0-core.h > > > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.h > > > @@ -12,13 +12,13 @@ > > > #include <linux/dma/edma.h> > > > > > > /* eDMA management callbacks */ > > > > > -void dw_edma_v0_core_off(struct dw_edma *chan); > > > -u16 dw_edma_v0_core_ch_count(struct dw_edma *chan, enum dw_edma_dir dir); > > > +void dw_edma_v0_core_off(struct dw_edma_chip *chip); > > > +u16 dw_edma_v0_core_ch_count(struct dw_edma_chip *chip, enum dw_edma_dir dir); > > > enum dma_status dw_edma_v0_core_ch_status(struct dw_edma_chan *chan); > > > void dw_edma_v0_core_clear_done_int(struct dw_edma_chan *chan); > > > void dw_edma_v0_core_clear_abort_int(struct dw_edma_chan *chan); > > > -u32 dw_edma_v0_core_status_done_int(struct dw_edma *chan, enum dw_edma_dir dir); > > > -u32 dw_edma_v0_core_status_abort_int(struct dw_edma *chan, enum dw_edma_dir dir); > > > +u32 dw_edma_v0_core_status_done_int(struct dw_edma_chip *chip, enum dw_edma_dir dir); > > > +u32 dw_edma_v0_core_status_abort_int(struct dw_edma_chip *chip, enum dw_edma_dir dir); > > > > This modification won't be needed. > > > > > void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first); > > > int dw_edma_v0_core_device_config(struct dw_edma_chan *chan); > > > /* eDMA debug fs callbacks */ > > > diff --git a/drivers/dma/dw-edma/dw-edma-v0-debugfs.c b/drivers/dma/dw-edma/dw-edma-v0-debugfs.c > > > index 4b3bcffd15ef1..5819a64aceb0f 100644 > > > --- a/drivers/dma/dw-edma/dw-edma-v0-debugfs.c > > > +++ b/drivers/dma/dw-edma/dw-edma-v0-debugfs.c > > > @@ -38,7 +38,7 @@ > > > #define CHANNEL_STR "channel" > > > #define REGISTERS_STR "registers" > > > > > > > > -static struct dw_edma *dw; > > > +static struct dw_edma_chip *chip; > > > > Hmmm, why on Earth is this static and global?.. What if we have more > > than one host/EP controller with eDMA?.. Nice. I'll think on fixing > > this if you don't want to bother... > > > > > static struct dw_edma_v0_regs __iomem *regs; > > > > > > static struct { > > > @@ -53,8 +53,10 @@ struct debugfs_entries { > > > > > > static int dw_edma_debugfs_u32_get(void *data, u64 *val) > > > { > > > + struct dw_edma *dw = chip->dw; > > > + > > > void __iomem *reg = (void __force __iomem *)data; > > > > > - if (dw->mf == EDMA_MF_EDMA_LEGACY && > > > + if (chip->mf == EDMA_MF_EDMA_LEGACY && > > > > Use "dw->chip-> ..." here > > > > > reg >= (void __iomem *)®s->type.legacy.ch) { > > > void __iomem *ptr = ®s->type.legacy.ch; > > > u32 viewport_sel = 0; > > > @@ -127,6 +129,8 @@ static void dw_edma_debugfs_regs_ch(struct dw_edma_v0_ch_regs __iomem *regs, > > > > > > static void dw_edma_debugfs_regs_wr(struct dentry *dir) > > > { > > > + struct dw_edma *dw = chip->dw; > > > + > > > const struct debugfs_entries debugfs_regs[] = { > > > /* eDMA global registers */ > > > WR_REGISTER(engine_en), > > > @@ -173,7 +177,7 @@ static void dw_edma_debugfs_regs_wr(struct dentry *dir) > > > nr_entries = ARRAY_SIZE(debugfs_regs); > > > dw_edma_debugfs_create_x32(debugfs_regs, nr_entries, regs_dir); > > > > > > > > - if (dw->mf == EDMA_MF_HDMA_COMPAT) { > > > + if (chip->mf == EDMA_MF_HDMA_COMPAT) { > > > > ditto > > > > > nr_entries = ARRAY_SIZE(debugfs_unroll_regs); > > > dw_edma_debugfs_create_x32(debugfs_unroll_regs, nr_entries, > > > regs_dir); > > > @@ -195,6 +199,8 @@ static void dw_edma_debugfs_regs_wr(struct dentry *dir) > > > > > > static void dw_edma_debugfs_regs_rd(struct dentry *dir) > > > { > > > + struct dw_edma *dw = chip->dw; > > > + > > > const struct debugfs_entries debugfs_regs[] = { > > > /* eDMA global registers */ > > > RD_REGISTER(engine_en), > > > @@ -242,7 +248,7 @@ static void dw_edma_debugfs_regs_rd(struct dentry *dir) > > > nr_entries = ARRAY_SIZE(debugfs_regs); > > > dw_edma_debugfs_create_x32(debugfs_regs, nr_entries, regs_dir); > > > > > > > > - if (dw->mf == EDMA_MF_HDMA_COMPAT) { > > > + if (chip->mf == EDMA_MF_HDMA_COMPAT) { > > > > ditto > > > > > nr_entries = ARRAY_SIZE(debugfs_unroll_regs); > > > dw_edma_debugfs_create_x32(debugfs_unroll_regs, nr_entries, > > > regs_dir); > > > @@ -264,6 +270,7 @@ static void dw_edma_debugfs_regs_rd(struct dentry *dir) > > > > > > static void dw_edma_debugfs_regs(void) > > > { > > > + struct dw_edma *dw = chip->dw; > > > const struct debugfs_entries debugfs_regs[] = { > > > REGISTER(ctrl_data_arb_prior), > > > REGISTER(ctrl), > > > @@ -282,13 +289,15 @@ static void dw_edma_debugfs_regs(void) > > > dw_edma_debugfs_regs_rd(regs_dir); > > > } > > > > > > > > -void dw_edma_v0_debugfs_on(struct dw_edma_chip *chip) > > > +void dw_edma_v0_debugfs_on(struct dw_edma_chip *p) > > > { > > > - dw = chip->dw; > > > - if (!dw) > > > + struct dw_edma *dw; > > > + chip = p; > > > + if (!chip) > > > return; > > > > > > - regs = dw->rg_region.vaddr; > > > + dw = chip->dw; > > > + regs = chip->reg_base; > > > > As I said this is unrelated change. Please unpin to another patch. > > > > > if (!regs) > > > return; > > > > > > @@ -296,19 +305,19 @@ void dw_edma_v0_debugfs_on(struct dw_edma_chip *chip) > > > if (!dw->debugfs) > > > return; > > > > > > > > - debugfs_create_u32("mf", 0444, dw->debugfs, &dw->mf); > > > + debugfs_create_u32("mf", 0444, dw->debugfs, &chip->mf); > > > > "dw->chip->..." > > > > > debugfs_create_u16("wr_ch_cnt", 0444, dw->debugfs, &dw->wr_ch_cnt); > > > debugfs_create_u16("rd_ch_cnt", 0444, dw->debugfs, &dw->rd_ch_cnt); > > > > > > dw_edma_debugfs_regs(); > > > } > > > > > > -void dw_edma_v0_debugfs_off(struct dw_edma_chip *chip) > > > +void dw_edma_v0_debugfs_off(struct dw_edma_chip *p) > > > { > > > - dw = chip->dw; > > > - if (!dw) > > > + chip = p; > > > + if (!chip) > > > return; > > > > > > - debugfs_remove_recursive(dw->debugfs); > > > - dw->debugfs = NULL; > > > + debugfs_remove_recursive(chip->dw->debugfs); > > > > This won't be needed. > > > > > + chip->dw->debugfs = NULL; > > > } > > > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h > > > index cab6e18773dad..fcfbc0f47f83d 100644 > > > --- a/include/linux/dma/edma.h > > > +++ b/include/linux/dma/edma.h > > > @@ -12,19 +12,62 @@ > > > #include <linux/device.h> > > > #include <linux/dmaengine.h> > > > > > > +#define EDMA_MAX_WR_CH 8 > > > +#define EDMA_MAX_RD_CH 8 > > > + > > > > > struct dw_edma; > > > > This could be dropped. > > > > > > > > +struct dw_edma_region { > > > + phys_addr_t paddr; > > > + void __iomem *vaddr; > > > + size_t sz; > > > +}; > > > + > > > +struct dw_edma_core_ops { > > > + int (*irq_vector)(struct device *dev, unsigned int nr); > > > +}; > > > + > > > +enum dw_edma_map_format { > > > + EDMA_MF_EDMA_LEGACY = 0x0, > > > + EDMA_MF_EDMA_UNROLL = 0x1, > > > + EDMA_MF_HDMA_COMPAT = 0x5 > > > +}; > > > + > > > /** > > > * struct dw_edma_chip - representation of DesignWare eDMA controller hardware > > > * @dev: struct device of the eDMA controller > > > * @id: instance ID > > > > > - * @irq: irq line > > > + * @nr_irqs: total dma irq number > > > > Indeed, dw_edma_chip->irq field has been unused anyway... > > > > > + * reg64bit if support 64bit write to register > > > > Do you have this field in the structure below? I don't see it there. > > Drop this line then. > > > > > + * @ops DMA channel to IRQ number mapping > > > + * @wr_ch_cnt DMA write channel number > > > + * @rd_ch_cnt DMA read channel number > > > > > + * @rg_region DMA register region > > > > You changed this to reg_base in the structure below, but the doc has > > been left with the old name and field type. Please unpin the modification > > to a dedicated patch as I suggested before. > > > > > + * @ll_region_wr DMA descriptor link list memory for write channel > > > + * @ll_region_rd DMA descriptor link list memory for read channel > > > + * @mf DMA register map format > > > * @dw: struct dw_edma that is filed by dw_edma_probe() > > > */ > > > struct dw_edma_chip { > > > struct device *dev; > > > int id; > > > - int irq; > > > + int nr_irqs; > > > + const struct dw_edma_core_ops *ops; > > > + > > > + void __iomem *reg_base; > > > + > > > + u16 ll_wr_cnt; > > > + u16 ll_rd_cnt; > > > > Why did you name these fields with "ll_" prefix? These are the number of > > read/write channels. Moreover the structure doc above have them named as > > "wr_ch_cnt" and "rd_ch_cnt". So if you want to change the fields names > > please add an addition patch and justify why it's needed. > > > > > + /* link list address */ > > > + struct dw_edma_region ll_region_wr[EDMA_MAX_WR_CH]; > > > + struct dw_edma_region ll_region_rd[EDMA_MAX_RD_CH]; > > > + > > > + /* data region */ > > > + struct dw_edma_region dt_region_wr[EDMA_MAX_WR_CH]; > > > + struct dw_edma_region dt_region_rd[EDMA_MAX_RD_CH]; > > > + > > > + enum dw_edma_map_format mf; > > > + > > > > > struct dw_edma *dw; > > > > Finally this could be dropped. Thus the dw_edma_chip structure will > > be just the chip info data. > > > > -Sergey > > > > > }; > > > > > > -- > > > 2.24.0.rc1 > > >
Hey, On Wed, Mar 09, 2022 at 04:39:40PM +0300, Serge Semin wrote: > Hello Frank > > On Mon, Mar 07, 2022 at 04:47:45PM -0600, Frank Li wrote: > > "struct dw_edma_chip" contains an internal structure "struct dw_edma" that is > > used by the eDMA core internally. This structure should not be touched > > by the eDMA controller drivers themselves. But currently, the eDMA > > controller drivers like "dw-edma-pci" allocates and populates this > > internal structure then passes it on to eDMA core. The eDMA core further > > populates the structure and uses it. This is wrong! > > > > Hence, move all the "struct dw_edma" specifics from controller drivers > > to the eDMA core. > > Thanks for the patchset. Alas it has just drawn my attention on v3 > stage, otherwise I would have given to you my thoughts stright away on > v1. Anyway first of all a cover letter would be very much appropriate > to have a general notion about all the changes introduced in the set. > +1 for cover letter. > Secondly I've just been working on adding the eDMA platform support > myself, so you have been just about a week ahead of me submitting my > changes. Welcome to the ship :) We (me and my colleague) were also working on eDMA support for Qcom platform, so jumped in. > My work contains some of the modifications done by you (but > have some additional fixes too) so I'll need to rebase it on top of > your patchset when it's finished. Anyway am I understand correctly, > that you've also been working on the DW PCIe driver alteration so one > would properly initialize the eDMA-chip data structure? If so have you > sent the patchset already? Could you give me a link and add me to Cc > in the emailing thread? (That's where the cover letter with all the > info and related patchsets would be very helpful.) > https://lore.kernel.org/linux-pci/20220309120149.GB134091@thinkpad/T/#m979eb506c73ab3cfca2e7a43635ecdaec18d8097 But this patch got dropped in v3 as the ep support for imx driver has not landed yet. > Thirdly regarding this patch. Your modification is mainly correct, but > I would suggest to change the concept. Instead of needlessly converting > the code to using the dw_edma_chip structure pointer within the DW eDMA > driver, it would be much easier in modification and more logical to > keep using the struct dw_edma pointer. Especially seeing dw_edma > structure is going to be a pure private data. So to speak what I would > suggest is to have the next pointers setup: > I'm afraid that this will not work for all cases (unless I miss something). As Zhi Li pointed out, there are places where only chip pointer will be passed and we'd need to extract the private data (dw_edma) from it. Tbh I also considered your idea but because of the above mentioned issue and also referring to other implementations like gpiochip, I settled with Frank's idea of copying the fields. Thanks, Mani
On Wed, Mar 09, 2022 at 11:42:33PM +0530, Manivannan Sadhasivam wrote: > Hey, > > On Wed, Mar 09, 2022 at 04:39:40PM +0300, Serge Semin wrote: > > Hello Frank > > > > On Mon, Mar 07, 2022 at 04:47:45PM -0600, Frank Li wrote: > > > "struct dw_edma_chip" contains an internal structure "struct dw_edma" that is > > > used by the eDMA core internally. This structure should not be touched > > > by the eDMA controller drivers themselves. But currently, the eDMA > > > controller drivers like "dw-edma-pci" allocates and populates this > > > internal structure then passes it on to eDMA core. The eDMA core further > > > populates the structure and uses it. This is wrong! > > > > > > Hence, move all the "struct dw_edma" specifics from controller drivers > > > to the eDMA core. > > > > Thanks for the patchset. Alas it has just drawn my attention on v3 > > stage, otherwise I would have given to you my thoughts stright away on > > v1. Anyway first of all a cover letter would be very much appropriate > > to have a general notion about all the changes introduced in the set. > > > > +1 for cover letter. > > > Secondly I've just been working on adding the eDMA platform support > > myself, so you have been just about a week ahead of me submitting my > > changes. > > Welcome to the ship :) We (me and my colleague) were also working on eDMA > support for Qcom platform, so jumped in. > > > My work contains some of the modifications done by you (but > > have some additional fixes too) so I'll need to rebase it on top of > > your patchset when it's finished. Anyway am I understand correctly, > > that you've also been working on the DW PCIe driver alteration so one > > would properly initialize the eDMA-chip data structure? If so have you > > sent the patchset already? Could you give me a link and add me to Cc > > in the emailing thread? (That's where the cover letter with all the > > info and related patchsets would be very helpful.) > > > > https://lore.kernel.org/linux-pci/20220309120149.GB134091@thinkpad/T/#m979eb506c73ab3cfca2e7a43635ecdaec18d8097 > > But this patch got dropped in v3 as the ep support for imx driver has not landed > yet. I see. Ok then. > > > Thirdly regarding this patch. Your modification is mainly correct, but > > I would suggest to change the concept. Instead of needlessly converting > > the code to using the dw_edma_chip structure pointer within the DW eDMA > > driver, it would be much easier in modification and more logical to > > keep using the struct dw_edma pointer. Especially seeing dw_edma > > structure is going to be a pure private data. So to speak what I would > > suggest is to have the next pointers setup: > > > > I'm afraid that this will not work for all cases (unless I miss something). As > Zhi Li pointed out, there are places where only chip pointer will be passed and > we'd need to extract the private data (dw_edma) from it. > > Tbh I also considered your idea but because of the above mentioned issue and > also referring to other implementations like gpiochip, I settled with Frank's > idea of copying the fields. What places are these? I see the only obstacle is the dw_edma_remove() method. But it's easily fixable. Except that, everything else is more or less straightforward (just a few methods need to have prototypes converted to accepting dw_edma instead dw_edma_chip). In order to make the code design more coherent, we need to split up private data and device/platform info. As I see it dw_edma_chip is nothing but a chip info data. The eDMA driver is supposed to mainly use and pass it's private data, not the platform info. It will greatly improve the code readability and maintainability. Such approach will also prevent a temptation of adding new private data fields into the dw_edma_chip structure since reaching the pointer to dw_edma will be much easier that getting the dw_edma_chip data. In this case dw_edma_chip will be something like i2c_board_info in i2c. Ideally dw_edma_chip could be a temporarily defined device info, which memory after the dw_edma_probe() method invocation could be freed. But in order to implement that we'd need a bit more modifications introduced. Last but not least the approach suggested by me is easier to implement, thus having easier review, easier backporting and causing less potential bugs. -Sergey > > Thanks, > Mani
On Wed, Mar 09, 2022 at 10:01:23PM +0300, Serge Semin wrote: [...] > > I'm afraid that this will not work for all cases (unless I miss something). As > > Zhi Li pointed out, there are places where only chip pointer will be passed and > > we'd need to extract the private data (dw_edma) from it. > > > > Tbh I also considered your idea but because of the above mentioned issue and > > also referring to other implementations like gpiochip, I settled with Frank's > > idea of copying the fields. > > What places are these? I see the only obstacle is the dw_edma_remove() > method. But it's easily fixable. Yeah, right. I overlooked that part. > Except that, everything else is more > or less straightforward (just a few methods need to have prototypes > converted to accepting dw_edma instead dw_edma_chip). > > In order to make the code design more coherent, we need to split up > private data and device/platform info. As I see it dw_edma_chip is > nothing but a chip info data. The eDMA driver is supposed to mainly > use and pass it's private data, not the platform info. It will greatly > improve the code readability and maintainability. Such approach will > also prevent a temptation of adding new private data fields into the > dw_edma_chip structure since reaching the pointer to dw_edma will be > much easier that getting the dw_edma_chip data. In this case > dw_edma_chip will be something like i2c_board_info in i2c. > > Ideally dw_edma_chip could be a temporarily defined device info, which > memory after the dw_edma_probe() method invocation could be freed. But > in order to implement that we'd need a bit more modifications > introduced. > While at it, we should also consider adding an ops structure for passing the callbacks from controller drivers. Currently the eDMA driver has the callbacks defined in v0-core.c but it is used directly instead of as a callback. This should anyway needs to be fixed when another version of the IP get's added. Thanks, Mani
On Thu, Mar 10, 2022 at 11:52:42AM +0530, Manivannan Sadhasivam wrote: > On Wed, Mar 09, 2022 at 10:01:23PM +0300, Serge Semin wrote: > > [...] > > > > I'm afraid that this will not work for all cases (unless I miss something). As > > > Zhi Li pointed out, there are places where only chip pointer will be passed and > > > we'd need to extract the private data (dw_edma) from it. > > > > > > Tbh I also considered your idea but because of the above mentioned issue and > > > also referring to other implementations like gpiochip, I settled with Frank's > > > idea of copying the fields. > > > > What places are these? I see the only obstacle is the dw_edma_remove() > > method. But it's easily fixable. > > Yeah, right. I overlooked that part. > > > Except that, everything else is more > > or less straightforward (just a few methods need to have prototypes > > converted to accepting dw_edma instead dw_edma_chip). > > > > In order to make the code design more coherent, we need to split up > > private data and device/platform info. As I see it dw_edma_chip is > > nothing but a chip info data. The eDMA driver is supposed to mainly > > use and pass it's private data, not the platform info. It will greatly > > improve the code readability and maintainability. Such approach will > > also prevent a temptation of adding new private data fields into the > > dw_edma_chip structure since reaching the pointer to dw_edma will be > > much easier that getting the dw_edma_chip data. In this case > > dw_edma_chip will be something like i2c_board_info in i2c. > > > > Ideally dw_edma_chip could be a temporarily defined device info, which > > memory after the dw_edma_probe() method invocation could be freed. But > > in order to implement that we'd need a bit more modifications > > introduced. > > > > While at it, we should also consider adding an ops structure for passing the > callbacks from controller drivers. Currently the eDMA driver has the callbacks > defined in v0-core.c but it is used directly instead of as a callback. Are you saying about DBI/Native IOs? If so seems reasonable. Though in my case it isn't required.) The only problem was a dword-aligned access, which has been created in the DW eDMA driver by default. -Sergey > > This should anyway needs to be fixed when another version of the IP get's added. > > Thanks, > Mani
On Thu, Mar 10, 2022 at 11:41:12AM +0300, Serge Semin wrote: > On Thu, Mar 10, 2022 at 11:52:42AM +0530, Manivannan Sadhasivam wrote: > > On Wed, Mar 09, 2022 at 10:01:23PM +0300, Serge Semin wrote: > > > > [...] > > > > > > I'm afraid that this will not work for all cases (unless I miss something). As > > > > Zhi Li pointed out, there are places where only chip pointer will be passed and > > > > we'd need to extract the private data (dw_edma) from it. > > > > > > > > Tbh I also considered your idea but because of the above mentioned issue and > > > > also referring to other implementations like gpiochip, I settled with Frank's > > > > idea of copying the fields. > > > > > > What places are these? I see the only obstacle is the dw_edma_remove() > > > method. But it's easily fixable. > > > > Yeah, right. I overlooked that part. > > > > > Except that, everything else is more > > > or less straightforward (just a few methods need to have prototypes > > > converted to accepting dw_edma instead dw_edma_chip). > > > > > > In order to make the code design more coherent, we need to split up > > > private data and device/platform info. As I see it dw_edma_chip is > > > nothing but a chip info data. The eDMA driver is supposed to mainly > > > use and pass it's private data, not the platform info. It will greatly > > > improve the code readability and maintainability. Such approach will > > > also prevent a temptation of adding new private data fields into the > > > dw_edma_chip structure since reaching the pointer to dw_edma will be > > > much easier that getting the dw_edma_chip data. In this case > > > dw_edma_chip will be something like i2c_board_info in i2c. > > > > > > Ideally dw_edma_chip could be a temporarily defined device info, which > > > memory after the dw_edma_probe() method invocation could be freed. But > > > in order to implement that we'd need a bit more modifications > > > introduced. > > > > > > > > While at it, we should also consider adding an ops structure for passing the > > callbacks from controller drivers. Currently the eDMA driver has the callbacks > > defined in v0-core.c but it is used directly instead of as a callback. > > Are you saying about DBI/Native IOs? If so seems reasonable. Though in > my case it isn't required.) The only problem was a dword-aligned access, > which has been created in the DW eDMA driver by default. > It is not causing any problem but it doesn't look correct to me. Btw, do you have a patch for DWORD access? If so, please share. We are also facing the problem and like to see how to are handling it. Thanks, Mani > -Sergey > > > > > This should anyway needs to be fixed when another version of the IP get's added. > > > > Thanks, > > Mani
On Thu, Mar 10, 2022 at 02:26:32PM +0530, Manivannan Sadhasivam wrote: > On Thu, Mar 10, 2022 at 11:41:12AM +0300, Serge Semin wrote: > > On Thu, Mar 10, 2022 at 11:52:42AM +0530, Manivannan Sadhasivam wrote: > > > On Wed, Mar 09, 2022 at 10:01:23PM +0300, Serge Semin wrote: > > > > > > [...] > > > > > > > > I'm afraid that this will not work for all cases (unless I miss something). As > > > > > Zhi Li pointed out, there are places where only chip pointer will be passed and > > > > > we'd need to extract the private data (dw_edma) from it. > > > > > > > > > > Tbh I also considered your idea but because of the above mentioned issue and > > > > > also referring to other implementations like gpiochip, I settled with Frank's > > > > > idea of copying the fields. > > > > > > > > What places are these? I see the only obstacle is the dw_edma_remove() > > > > method. But it's easily fixable. > > > > > > Yeah, right. I overlooked that part. > > > > > > > Except that, everything else is more > > > > or less straightforward (just a few methods need to have prototypes > > > > converted to accepting dw_edma instead dw_edma_chip). > > > > > > > > In order to make the code design more coherent, we need to split up > > > > private data and device/platform info. As I see it dw_edma_chip is > > > > nothing but a chip info data. The eDMA driver is supposed to mainly > > > > use and pass it's private data, not the platform info. It will greatly > > > > improve the code readability and maintainability. Such approach will > > > > also prevent a temptation of adding new private data fields into the > > > > dw_edma_chip structure since reaching the pointer to dw_edma will be > > > > much easier that getting the dw_edma_chip data. In this case > > > > dw_edma_chip will be something like i2c_board_info in i2c. > > > > > > > > Ideally dw_edma_chip could be a temporarily defined device info, which > > > > memory after the dw_edma_probe() method invocation could be freed. But > > > > in order to implement that we'd need a bit more modifications > > > > introduced. > > > > > > > > > > > > While at it, we should also consider adding an ops structure for passing the > > > callbacks from controller drivers. Currently the eDMA driver has the callbacks > > > defined in v0-core.c but it is used directly instead of as a callback. > > > > Are you saying about DBI/Native IOs? If so seems reasonable. Though in > > my case it isn't required.) The only problem was a dword-aligned access, > > which has been created in the DW eDMA driver by default. > > > > It is not causing any problem but it doesn't look correct to me. > > Btw, do you have a patch for DWORD access? If so, please share. We are also > facing the problem and like to see how to are handling it. There are two types of accessors DW PCIe Host/EP driver defines: CFG-space and DBI interface. Both of them can be implemented either with default IO methods, or redefined by the particular platform. In addition to that the CFG-space IOs are split up into two types: host/EP own CFG-space and others (peripheral devices) CFG-spaces. In my case the dword-aligned access is supposed to be performed only for the DBI-interface and host CFG-space (which basically turns to be the same registers space). The driver has a bug in this matter, which I fixed with a patch attached to this email. After that patch is applied the only thing you'll need is to redefine the dw_pcie_ops.{read_dbi,write_dbi,write_dbi2} callbacks with the platform specific ones, which would make sure IOs are dword aligned. Like this: static int bt1_pcie_read_mmio(void __iomem *addr, int size, u32 *val) { unsigned int ofs = (uintptr_t)addr & 0x3; if (!IS_ALIGNED((uintptr_t)addr, size)) return PCIBIOS_BAD_REGISTER_NUMBER; *val = readl(addr - ofs) >> ofs * BITS_PER_BYTE; if (size == 4) { return PCIBIOS_SUCCESSFUL; } else if (size == 2) { *val &= 0xffff; return PCIBIOS_SUCCESSFUL; } else if (size == 1) { *val &= 0xff; return PCIBIOS_SUCCESSFUL; } return PCIBIOS_BAD_REGISTER_NUMBER; } static int bt1_pcie_write_mmio(void __iomem *addr, int size, u32 val) { unsigned int ofs = (uintptr_t)addr & 0x3; u32 tmp, mask; if (!IS_ALIGNED((uintptr_t)addr, size)) return PCIBIOS_BAD_REGISTER_NUMBER; if (size == 4) { writel(val, addr); return PCIBIOS_SUCCESSFUL; } else if (size == 2 || size == 1) { mask = GENMASK(size * BITS_PER_BYTE - 1, 0); tmp = readl(addr - ofs) & ~(mask << ofs * BITS_PER_BYTE); tmp |= (val & mask) << ofs * BITS_PER_BYTE; writel(tmp, addr - ofs); return PCIBIOS_SUCCESSFUL; } return PCIBIOS_BAD_REGISTER_NUMBER; } Regarding the peripheral devices CFG-space IOs. I didn't fix it there since in my case there were no problems with iATU outbound windows. DBI slave and iATU slave interfaces works differently. The latter in my case permits non-dword-aligned IOs. If you are experiencing problems with all the PCIe memory accesses (not only DBI-space/host CFG-space, but the whole iATU outbound IOs), then there won't be easy solution for that other than somehow fixing the read{b,w,l(})/write{b,w,l}() platform-specific internals. In a way so they would differentiate PCIe and normal MMIO IOs and make sure all of them are properly aligned. Alternatively you can fix each particular peripheral device driver. It isn't that good option though. Regarding DW eDMA driver. It doesn't have the problem in this matter, since it is designed to call readl/writel methods only for the controller setups. They are dword-aligned. Regarding attached patch and my work. I'll send it out after Frank finishes his eDMA patchset review, since other than the attached patch I've got some more useful fixes, which need to be rebased on top of the Frank' work. -Sergey > > Thanks, > Mani > > > -Sergey > > > > > > > > This should anyway needs to be fixed when another version of the IP get's added. > > > > > > Thanks, > > > Mani
diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c index 53289927dd0d6..0cb66434f9e14 100644 --- a/drivers/dma/dw-edma/dw-edma-core.c +++ b/drivers/dma/dw-edma/dw-edma-core.c @@ -65,7 +65,7 @@ static struct dw_edma_burst *dw_edma_alloc_burst(struct dw_edma_chunk *chunk) static struct dw_edma_chunk *dw_edma_alloc_chunk(struct dw_edma_desc *desc) { struct dw_edma_chan *chan = desc->chan; - struct dw_edma *dw = chan->chip->dw; + struct dw_edma_chip *chip = chan->chip; struct dw_edma_chunk *chunk; chunk = kzalloc(sizeof(*chunk), GFP_NOWAIT); @@ -82,11 +82,11 @@ static struct dw_edma_chunk *dw_edma_alloc_chunk(struct dw_edma_desc *desc) */ chunk->cb = !(desc->chunks_alloc % 2); if (chan->dir == EDMA_DIR_WRITE) { - chunk->ll_region.paddr = dw->ll_region_wr[chan->id].paddr; - chunk->ll_region.vaddr = dw->ll_region_wr[chan->id].vaddr; + chunk->ll_region.paddr = chip->ll_region_wr[chan->id].paddr; + chunk->ll_region.vaddr = chip->ll_region_wr[chan->id].vaddr; } else { - chunk->ll_region.paddr = dw->ll_region_rd[chan->id].paddr; - chunk->ll_region.vaddr = dw->ll_region_rd[chan->id].vaddr; + chunk->ll_region.paddr = chip->ll_region_rd[chan->id].paddr; + chunk->ll_region.vaddr = chip->ll_region_rd[chan->id].vaddr; } if (desc->chunk) { @@ -601,7 +601,8 @@ static void dw_edma_abort_interrupt(struct dw_edma_chan *chan) static irqreturn_t dw_edma_interrupt(int irq, void *data, bool write) { struct dw_edma_irq *dw_irq = data; - struct dw_edma *dw = dw_irq->dw; + struct dw_edma_chip *chip = dw_irq->chip; + struct dw_edma *dw = chip->dw; unsigned long total, pos, val; unsigned long off; u32 mask; @@ -616,7 +617,7 @@ static irqreturn_t dw_edma_interrupt(int irq, void *data, bool write) mask = dw_irq->rd_mask; } - val = dw_edma_v0_core_status_done_int(dw, write ? + val = dw_edma_v0_core_status_done_int(chip, write ? EDMA_DIR_WRITE : EDMA_DIR_READ); val &= mask; @@ -626,7 +627,7 @@ static irqreturn_t dw_edma_interrupt(int irq, void *data, bool write) dw_edma_done_interrupt(chan); } - val = dw_edma_v0_core_status_abort_int(dw, write ? + val = dw_edma_v0_core_status_abort_int(chip, write ? EDMA_DIR_WRITE : EDMA_DIR_READ); val &= mask; @@ -718,7 +719,7 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write, } INIT_LIST_HEAD(&dma->channels); - for (j = 0; (alloc || dw->nr_irqs == 1) && j < cnt; j++, i++) { + for (j = 0; (alloc || chip->nr_irqs == 1) && j < cnt; j++, i++) { chan = &dw->chan[i]; dt_region = devm_kzalloc(dev, sizeof(*dt_region), GFP_KERNEL); @@ -735,15 +736,15 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write, chan->status = EDMA_ST_IDLE; if (write) - chan->ll_max = (dw->ll_region_wr[j].sz / EDMA_LL_SZ); + chan->ll_max = (chip->ll_region_wr[j].sz / EDMA_LL_SZ); else - chan->ll_max = (dw->ll_region_rd[j].sz / EDMA_LL_SZ); + chan->ll_max = (chip->ll_region_rd[j].sz / EDMA_LL_SZ); chan->ll_max -= 1; dev_vdbg(dev, "L. List:\tChannel %s[%u] max_cnt=%u\n", write ? "write" : "read", j, chan->ll_max); - if (dw->nr_irqs == 1) + if (chip->nr_irqs == 1) pos = 0; else pos = off_alloc + (j % alloc); @@ -755,7 +756,7 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write, else irq->rd_mask |= BIT(j); - irq->dw = dw; + irq->chip = chip; memcpy(&chan->msi, &irq->msi, sizeof(chan->msi)); dev_vdbg(dev, "MSI:\t\tChannel %s[%u] addr=0x%.8x%.8x, data=0x%.8x\n", @@ -767,13 +768,13 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write, vchan_init(&chan->vc, dma); if (write) { - dt_region->paddr = dw->dt_region_wr[j].paddr; - dt_region->vaddr = dw->dt_region_wr[j].vaddr; - dt_region->sz = dw->dt_region_wr[j].sz; + dt_region->paddr = chip->dt_region_wr[j].paddr; + dt_region->vaddr = chip->dt_region_wr[j].vaddr; + dt_region->sz = chip->dt_region_wr[j].sz; } else { - dt_region->paddr = dw->dt_region_rd[j].paddr; - dt_region->vaddr = dw->dt_region_rd[j].vaddr; - dt_region->sz = dw->dt_region_rd[j].sz; + dt_region->paddr = chip->dt_region_rd[j].paddr; + dt_region->vaddr = chip->dt_region_rd[j].vaddr; + dt_region->sz = chip->dt_region_rd[j].sz; } dw_edma_v0_core_device_config(chan); @@ -840,16 +841,16 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, ch_cnt = dw->wr_ch_cnt + dw->rd_ch_cnt; - if (dw->nr_irqs < 1) + if (chip->nr_irqs < 1) return -EINVAL; - if (dw->nr_irqs == 1) { + if (chip->nr_irqs == 1) { /* Common IRQ shared among all channels */ - irq = dw->ops->irq_vector(dev, 0); + irq = chip->ops->irq_vector(dev, 0); err = request_irq(irq, dw_edma_interrupt_common, IRQF_SHARED, dw->name, &dw->irq[0]); if (err) { - dw->nr_irqs = 0; + chip->nr_irqs = 0; return err; } @@ -857,7 +858,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, get_cached_msi_msg(irq, &dw->irq[0].msi); } else { /* Distribute IRQs equally among all channels */ - int tmp = dw->nr_irqs; + int tmp = chip->nr_irqs; while (tmp && (*wr_alloc + *rd_alloc) < ch_cnt) { dw_edma_dec_irq_alloc(&tmp, wr_alloc, dw->wr_ch_cnt); @@ -868,7 +869,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, dw_edma_add_irq_mask(&rd_mask, *rd_alloc, dw->rd_ch_cnt); for (i = 0; i < (*wr_alloc + *rd_alloc); i++) { - irq = dw->ops->irq_vector(dev, i); + irq = chip->ops->irq_vector(dev, i); err = request_irq(irq, i < *wr_alloc ? dw_edma_interrupt_write : @@ -876,7 +877,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, IRQF_SHARED, dw->name, &dw->irq[i]); if (err) { - dw->nr_irqs = i; + chip->nr_irqs = i; return err; } @@ -884,7 +885,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip, get_cached_msi_msg(irq, &dw->irq[i].msi); } - dw->nr_irqs = i; + chip->nr_irqs = i; } return err; @@ -905,18 +906,24 @@ int dw_edma_probe(struct dw_edma_chip *chip) if (!dev) return -EINVAL; - dw = chip->dw; - if (!dw || !dw->irq || !dw->ops || !dw->ops->irq_vector) + dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL); + if (!dw) + return -ENOMEM; + + chip->dw = dw; + + if (!chip->nr_irqs || !chip->ops) return -EINVAL; raw_spin_lock_init(&dw->lock); - dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt, - dw_edma_v0_core_ch_count(dw, EDMA_DIR_WRITE)); + + dw->wr_ch_cnt = min_t(u16, chip->ll_wr_cnt, + dw_edma_v0_core_ch_count(chip, EDMA_DIR_WRITE)); dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt, EDMA_MAX_WR_CH); - dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt, - dw_edma_v0_core_ch_count(dw, EDMA_DIR_READ)); + dw->rd_ch_cnt = min_t(u16, chip->ll_rd_cnt, + dw_edma_v0_core_ch_count(chip, EDMA_DIR_READ)); dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt, EDMA_MAX_RD_CH); if (!dw->wr_ch_cnt && !dw->rd_ch_cnt) @@ -934,7 +941,11 @@ int dw_edma_probe(struct dw_edma_chip *chip) snprintf(dw->name, sizeof(dw->name), "dw-edma-core:%d", chip->id); /* Disable eDMA, only to establish the ideal initial conditions */ - dw_edma_v0_core_off(dw); + dw_edma_v0_core_off(chip); + + dw->irq = devm_kcalloc(dev, chip->nr_irqs, sizeof(*dw->irq), GFP_KERNEL); + if (!dw->irq) + return -ENOMEM; /* Request IRQs */ err = dw_edma_irq_request(chip, &wr_alloc, &rd_alloc); @@ -960,10 +971,10 @@ int dw_edma_probe(struct dw_edma_chip *chip) return 0; err_irq_free: - for (i = (dw->nr_irqs - 1); i >= 0; i--) - free_irq(dw->ops->irq_vector(dev, i), &dw->irq[i]); + for (i = (chip->nr_irqs - 1); i >= 0; i--) + free_irq(chip->ops->irq_vector(dev, i), &dw->irq[i]); - dw->nr_irqs = 0; + chip->nr_irqs = 0; return err; } @@ -977,11 +988,11 @@ int dw_edma_remove(struct dw_edma_chip *chip) int i; /* Disable eDMA */ - dw_edma_v0_core_off(dw); + dw_edma_v0_core_off(chip); /* Free irqs */ - for (i = (dw->nr_irqs - 1); i >= 0; i--) - free_irq(dw->ops->irq_vector(dev, i), &dw->irq[i]); + for (i = (chip->nr_irqs - 1); i >= 0; i--) + free_irq(chip->ops->irq_vector(dev, i), &dw->irq[i]); /* Power management */ pm_runtime_disable(dev); diff --git a/drivers/dma/dw-edma/dw-edma-core.h b/drivers/dma/dw-edma/dw-edma-core.h index 60316d408c3e0..885f6719c9462 100644 --- a/drivers/dma/dw-edma/dw-edma-core.h +++ b/drivers/dma/dw-edma/dw-edma-core.h @@ -15,20 +15,12 @@ #include "../virt-dma.h" #define EDMA_LL_SZ 24 -#define EDMA_MAX_WR_CH 8 -#define EDMA_MAX_RD_CH 8 enum dw_edma_dir { EDMA_DIR_WRITE = 0, EDMA_DIR_READ }; -enum dw_edma_map_format { - EDMA_MF_EDMA_LEGACY = 0x0, - EDMA_MF_EDMA_UNROLL = 0x1, - EDMA_MF_HDMA_COMPAT = 0x5 -}; - enum dw_edma_request { EDMA_REQ_NONE = 0, EDMA_REQ_STOP, @@ -57,12 +49,6 @@ struct dw_edma_burst { u32 sz; }; -struct dw_edma_region { - phys_addr_t paddr; - void __iomem *vaddr; - size_t sz; -}; - struct dw_edma_chunk { struct list_head list; struct dw_edma_chan *chan; @@ -106,11 +92,7 @@ struct dw_edma_irq { struct msi_msg msi; u32 wr_mask; u32 rd_mask; - struct dw_edma *dw; -}; - -struct dw_edma_core_ops { - int (*irq_vector)(struct device *dev, unsigned int nr); + struct dw_edma_chip *chip; }; struct dw_edma { @@ -122,19 +104,9 @@ struct dw_edma { struct dma_device rd_edma; u16 rd_ch_cnt; - struct dw_edma_region rg_region; /* Registers */ - struct dw_edma_region ll_region_wr[EDMA_MAX_WR_CH]; - struct dw_edma_region ll_region_rd[EDMA_MAX_RD_CH]; - struct dw_edma_region dt_region_wr[EDMA_MAX_WR_CH]; - struct dw_edma_region dt_region_rd[EDMA_MAX_RD_CH]; - struct dw_edma_irq *irq; - int nr_irqs; - - enum dw_edma_map_format mf; struct dw_edma_chan *chan; - const struct dw_edma_core_ops *ops; raw_spinlock_t lock; /* Only for legacy */ #ifdef CONFIG_DEBUG_FS diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c index 329fc2e57b703..6e2f83e31a03a 100644 --- a/drivers/dma/dw-edma/dw-edma-v0-core.c +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c @@ -23,92 +23,94 @@ enum dw_edma_control { DW_EDMA_V0_LLE = BIT(9), }; -static inline struct dw_edma_v0_regs __iomem *__dw_regs(struct dw_edma *dw) +static inline struct dw_edma_v0_regs __iomem *__dw_regs(struct dw_edma_chip *chip) { - return dw->rg_region.vaddr; + return chip->reg_base; } -#define SET_32(dw, name, value) \ - writel(value, &(__dw_regs(dw)->name)) +#define SET_32(chip, name, value) \ + writel(value, &(__dw_regs(chip)->name)) -#define GET_32(dw, name) \ - readl(&(__dw_regs(dw)->name)) +#define GET_32(chip, name) \ + readl(&(__dw_regs(chip)->name)) -#define SET_RW_32(dw, dir, name, value) \ +#define SET_RW_32(chip, dir, name, value) \ do { \ if ((dir) == EDMA_DIR_WRITE) \ - SET_32(dw, wr_##name, value); \ + SET_32(chip, wr_##name, value); \ else \ - SET_32(dw, rd_##name, value); \ + SET_32(chip, rd_##name, value); \ } while (0) -#define GET_RW_32(dw, dir, name) \ +#define GET_RW_32(chip, dir, name) \ ((dir) == EDMA_DIR_WRITE \ - ? GET_32(dw, wr_##name) \ - : GET_32(dw, rd_##name)) + ? GET_32(chip, wr_##name) \ + : GET_32(chip, rd_##name)) -#define SET_BOTH_32(dw, name, value) \ +#define SET_BOTH_32(chip, name, value) \ do { \ - SET_32(dw, wr_##name, value); \ - SET_32(dw, rd_##name, value); \ + SET_32(chip, wr_##name, value); \ + SET_32(chip, rd_##name, value); \ } while (0) #ifdef CONFIG_64BIT -#define SET_64(dw, name, value) \ - writeq(value, &(__dw_regs(dw)->name)) +#define SET_64(chip, name, value) \ + writeq(value, &(__dw_regs(chip)->name)) -#define GET_64(dw, name) \ - readq(&(__dw_regs(dw)->name)) +#define GET_64(chip, name) \ + readq(&(__dw_regs(chip)->name)) -#define SET_RW_64(dw, dir, name, value) \ +#define SET_RW_64(chip, dir, name, value) \ do { \ if ((dir) == EDMA_DIR_WRITE) \ - SET_64(dw, wr_##name, value); \ + SET_64(chip, wr_##name, value); \ else \ - SET_64(dw, rd_##name, value); \ + SET_64(chip, rd_##name, value); \ } while (0) -#define GET_RW_64(dw, dir, name) \ +#define GET_RW_64(chip, dir, name) \ ((dir) == EDMA_DIR_WRITE \ - ? GET_64(dw, wr_##name) \ - : GET_64(dw, rd_##name)) + ? GET_64(chip, wr_##name) \ + : GET_64(chip, rd_##name)) -#define SET_BOTH_64(dw, name, value) \ +#define SET_BOTH_64(chip, name, value) \ do { \ - SET_64(dw, wr_##name, value); \ - SET_64(dw, rd_##name, value); \ + SET_64(chip, wr_##name, value); \ + SET_64(chip, rd_##name, value); \ } while (0) #endif /* CONFIG_64BIT */ -#define SET_COMPAT(dw, name, value) \ - writel(value, &(__dw_regs(dw)->type.unroll.name)) +#define SET_COMPAT(chip, name, value) \ + writel(value, &(__dw_regs(chip)->type.unroll.name)) -#define SET_RW_COMPAT(dw, dir, name, value) \ +#define SET_RW_COMPAT(chip, dir, name, value) \ do { \ if ((dir) == EDMA_DIR_WRITE) \ - SET_COMPAT(dw, wr_##name, value); \ + SET_COMPAT(chip, wr_##name, value); \ else \ - SET_COMPAT(dw, rd_##name, value); \ + SET_COMPAT(chip, rd_##name, value); \ } while (0) static inline struct dw_edma_v0_ch_regs __iomem * -__dw_ch_regs(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch) +__dw_ch_regs(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch) { - if (dw->mf == EDMA_MF_EDMA_LEGACY) - return &(__dw_regs(dw)->type.legacy.ch); + if (chip->mf == EDMA_MF_EDMA_LEGACY) + return &(__dw_regs(chip)->type.legacy.ch); if (dir == EDMA_DIR_WRITE) - return &__dw_regs(dw)->type.unroll.ch[ch].wr; + return &__dw_regs(chip)->type.unroll.ch[ch].wr; - return &__dw_regs(dw)->type.unroll.ch[ch].rd; + return &__dw_regs(chip)->type.unroll.ch[ch].rd; } -static inline void writel_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, +static inline void writel_ch(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch, u32 value, void __iomem *addr) { - if (dw->mf == EDMA_MF_EDMA_LEGACY) { + struct dw_edma *dw = chip->dw; + + if (chip->mf == EDMA_MF_EDMA_LEGACY) { u32 viewport_sel; unsigned long flags; @@ -119,7 +121,7 @@ static inline void writel_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, viewport_sel |= BIT(31); writel(viewport_sel, - &(__dw_regs(dw)->type.legacy.viewport_sel)); + &(__dw_regs(chip)->type.legacy.viewport_sel)); writel(value, addr); raw_spin_unlock_irqrestore(&dw->lock, flags); @@ -128,12 +130,13 @@ static inline void writel_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, } } -static inline u32 readl_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, +static inline u32 readl_ch(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch, const void __iomem *addr) { + struct dw_edma *dw = chip->dw; u32 value; - if (dw->mf == EDMA_MF_EDMA_LEGACY) { + if (chip->mf == EDMA_MF_EDMA_LEGACY) { u32 viewport_sel; unsigned long flags; @@ -144,7 +147,7 @@ static inline u32 readl_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, viewport_sel |= BIT(31); writel(viewport_sel, - &(__dw_regs(dw)->type.legacy.viewport_sel)); + &(__dw_regs(chip)->type.legacy.viewport_sel)); value = readl(addr); raw_spin_unlock_irqrestore(&dw->lock, flags); @@ -166,10 +169,12 @@ static inline u32 readl_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, #ifdef CONFIG_64BIT -static inline void writeq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, +static inline void writeq_ch(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch, u64 value, void __iomem *addr) { - if (dw->mf == EDMA_MF_EDMA_LEGACY) { + struct dw_edma *dw = chip->dw; + + if (chip->mf == EDMA_MF_EDMA_LEGACY) { u32 viewport_sel; unsigned long flags; @@ -180,7 +185,7 @@ static inline void writeq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, viewport_sel |= BIT(31); writel(viewport_sel, - &(__dw_regs(dw)->type.legacy.viewport_sel)); + &(__dw_regs(chip)->type.legacy.viewport_sel)); writeq(value, addr); raw_spin_unlock_irqrestore(&dw->lock, flags); @@ -189,12 +194,13 @@ static inline void writeq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, } } -static inline u64 readq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, +static inline u64 readq_ch(struct dw_edma_chip *chip, enum dw_edma_dir dir, u16 ch, const void __iomem *addr) { + struct dw_edma *dw = chip->dw; u32 value; - if (dw->mf == EDMA_MF_EDMA_LEGACY) { + if (chip->mf == EDMA_MF_EDMA_LEGACY) { u32 viewport_sel; unsigned long flags; @@ -205,7 +211,7 @@ static inline u64 readq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, viewport_sel |= BIT(31); writel(viewport_sel, - &(__dw_regs(dw)->type.legacy.viewport_sel)); + &(__dw_regs(chip)->type.legacy.viewport_sel)); value = readq(addr); raw_spin_unlock_irqrestore(&dw->lock, flags); @@ -228,25 +234,25 @@ static inline u64 readq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, #endif /* CONFIG_64BIT */ /* eDMA management callbacks */ -void dw_edma_v0_core_off(struct dw_edma *dw) +void dw_edma_v0_core_off(struct dw_edma_chip *chip) { - SET_BOTH_32(dw, int_mask, + SET_BOTH_32(chip, int_mask, EDMA_V0_DONE_INT_MASK | EDMA_V0_ABORT_INT_MASK); - SET_BOTH_32(dw, int_clear, + SET_BOTH_32(chip, int_clear, EDMA_V0_DONE_INT_MASK | EDMA_V0_ABORT_INT_MASK); - SET_BOTH_32(dw, engine_en, 0); + SET_BOTH_32(chip, engine_en, 0); } -u16 dw_edma_v0_core_ch_count(struct dw_edma *dw, enum dw_edma_dir dir) +u16 dw_edma_v0_core_ch_count(struct dw_edma_chip *chip, enum dw_edma_dir dir) { u32 num_ch; if (dir == EDMA_DIR_WRITE) num_ch = FIELD_GET(EDMA_V0_WRITE_CH_COUNT_MASK, - GET_32(dw, ctrl)); + GET_32(chip, ctrl)); else num_ch = FIELD_GET(EDMA_V0_READ_CH_COUNT_MASK, - GET_32(dw, ctrl)); + GET_32(chip, ctrl)); if (num_ch > EDMA_V0_MAX_NR_CH) num_ch = EDMA_V0_MAX_NR_CH; @@ -256,11 +262,11 @@ u16 dw_edma_v0_core_ch_count(struct dw_edma *dw, enum dw_edma_dir dir) enum dma_status dw_edma_v0_core_ch_status(struct dw_edma_chan *chan) { - struct dw_edma *dw = chan->chip->dw; + struct dw_edma_chip *chip = chan->chip; u32 tmp; tmp = FIELD_GET(EDMA_V0_CH_STATUS_MASK, - GET_CH_32(dw, chan->dir, chan->id, ch_control1)); + GET_CH_32(chip, chan->dir, chan->id, ch_control1)); if (tmp == 1) return DMA_IN_PROGRESS; @@ -272,30 +278,30 @@ enum dma_status dw_edma_v0_core_ch_status(struct dw_edma_chan *chan) void dw_edma_v0_core_clear_done_int(struct dw_edma_chan *chan) { - struct dw_edma *dw = chan->chip->dw; + struct dw_edma_chip *chip = chan->chip; - SET_RW_32(dw, chan->dir, int_clear, + SET_RW_32(chip, chan->dir, int_clear, FIELD_PREP(EDMA_V0_DONE_INT_MASK, BIT(chan->id))); } void dw_edma_v0_core_clear_abort_int(struct dw_edma_chan *chan) { - struct dw_edma *dw = chan->chip->dw; + struct dw_edma_chip *chip = chan->chip; - SET_RW_32(dw, chan->dir, int_clear, + SET_RW_32(chip, chan->dir, int_clear, FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id))); } -u32 dw_edma_v0_core_status_done_int(struct dw_edma *dw, enum dw_edma_dir dir) +u32 dw_edma_v0_core_status_done_int(struct dw_edma_chip *chip, enum dw_edma_dir dir) { return FIELD_GET(EDMA_V0_DONE_INT_MASK, - GET_RW_32(dw, dir, int_status)); + GET_RW_32(chip, dir, int_status)); } -u32 dw_edma_v0_core_status_abort_int(struct dw_edma *dw, enum dw_edma_dir dir) +u32 dw_edma_v0_core_status_abort_int(struct dw_edma_chip *chip, enum dw_edma_dir dir) { return FIELD_GET(EDMA_V0_ABORT_INT_MASK, - GET_RW_32(dw, dir, int_status)); + GET_RW_32(chip, dir, int_status)); } static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk) @@ -357,109 +363,109 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk) void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first) { struct dw_edma_chan *chan = chunk->chan; - struct dw_edma *dw = chan->chip->dw; + struct dw_edma_chip *chip = chan->chip; u32 tmp; dw_edma_v0_core_write_chunk(chunk); if (first) { /* Enable engine */ - SET_RW_32(dw, chan->dir, engine_en, BIT(0)); - if (dw->mf == EDMA_MF_HDMA_COMPAT) { + SET_RW_32(chip, chan->dir, engine_en, BIT(0)); + if (chip->mf == EDMA_MF_HDMA_COMPAT) { switch (chan->id) { case 0: - SET_RW_COMPAT(dw, chan->dir, ch0_pwr_en, + SET_RW_COMPAT(chip, chan->dir, ch0_pwr_en, BIT(0)); break; case 1: - SET_RW_COMPAT(dw, chan->dir, ch1_pwr_en, + SET_RW_COMPAT(chip, chan->dir, ch1_pwr_en, BIT(0)); break; case 2: - SET_RW_COMPAT(dw, chan->dir, ch2_pwr_en, + SET_RW_COMPAT(chip, chan->dir, ch2_pwr_en, BIT(0)); break; case 3: - SET_RW_COMPAT(dw, chan->dir, ch3_pwr_en, + SET_RW_COMPAT(chip, chan->dir, ch3_pwr_en, BIT(0)); break; case 4: - SET_RW_COMPAT(dw, chan->dir, ch4_pwr_en, + SET_RW_COMPAT(chip, chan->dir, ch4_pwr_en, BIT(0)); break; case 5: - SET_RW_COMPAT(dw, chan->dir, ch5_pwr_en, + SET_RW_COMPAT(chip, chan->dir, ch5_pwr_en, BIT(0)); break; case 6: - SET_RW_COMPAT(dw, chan->dir, ch6_pwr_en, + SET_RW_COMPAT(chip, chan->dir, ch6_pwr_en, BIT(0)); break; case 7: - SET_RW_COMPAT(dw, chan->dir, ch7_pwr_en, + SET_RW_COMPAT(chip, chan->dir, ch7_pwr_en, BIT(0)); break; } } /* Interrupt unmask - done, abort */ - tmp = GET_RW_32(dw, chan->dir, int_mask); + tmp = GET_RW_32(chip, chan->dir, int_mask); tmp &= ~FIELD_PREP(EDMA_V0_DONE_INT_MASK, BIT(chan->id)); tmp &= ~FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id)); - SET_RW_32(dw, chan->dir, int_mask, tmp); + SET_RW_32(chip, chan->dir, int_mask, tmp); /* Linked list error */ - tmp = GET_RW_32(dw, chan->dir, linked_list_err_en); + tmp = GET_RW_32(chip, chan->dir, linked_list_err_en); tmp |= FIELD_PREP(EDMA_V0_LINKED_LIST_ERR_MASK, BIT(chan->id)); - SET_RW_32(dw, chan->dir, linked_list_err_en, tmp); + SET_RW_32(chip, chan->dir, linked_list_err_en, tmp); /* Channel control */ - SET_CH_32(dw, chan->dir, chan->id, ch_control1, + SET_CH_32(chip, chan->dir, chan->id, ch_control1, (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE)); /* Linked list */ #ifdef CONFIG_64BIT - SET_CH_64(dw, chan->dir, chan->id, llp.reg, + SET_CH_64(chip, chan->dir, chan->id, llp.reg, chunk->ll_region.paddr); #else /* CONFIG_64BIT */ - SET_CH_32(dw, chan->dir, chan->id, llp.lsb, + SET_CH_32(chip, chan->dir, chan->id, llp.lsb, lower_32_bits(chunk->ll_region.paddr)); - SET_CH_32(dw, chan->dir, chan->id, llp.msb, + SET_CH_32(chip, chan->dir, chan->id, llp.msb, upper_32_bits(chunk->ll_region.paddr)); #endif /* CONFIG_64BIT */ } /* Doorbell */ - SET_RW_32(dw, chan->dir, doorbell, + SET_RW_32(chip, chan->dir, doorbell, FIELD_PREP(EDMA_V0_DOORBELL_CH_MASK, chan->id)); } int dw_edma_v0_core_device_config(struct dw_edma_chan *chan) { - struct dw_edma *dw = chan->chip->dw; + struct dw_edma_chip *chip = chan->chip; u32 tmp = 0; /* MSI done addr - low, high */ - SET_RW_32(dw, chan->dir, done_imwr.lsb, chan->msi.address_lo); - SET_RW_32(dw, chan->dir, done_imwr.msb, chan->msi.address_hi); + SET_RW_32(chip, chan->dir, done_imwr.lsb, chan->msi.address_lo); + SET_RW_32(chip, chan->dir, done_imwr.msb, chan->msi.address_hi); /* MSI abort addr - low, high */ - SET_RW_32(dw, chan->dir, abort_imwr.lsb, chan->msi.address_lo); - SET_RW_32(dw, chan->dir, abort_imwr.msb, chan->msi.address_hi); + SET_RW_32(chip, chan->dir, abort_imwr.lsb, chan->msi.address_lo); + SET_RW_32(chip, chan->dir, abort_imwr.msb, chan->msi.address_hi); /* MSI data - low, high */ switch (chan->id) { case 0: case 1: - tmp = GET_RW_32(dw, chan->dir, ch01_imwr_data); + tmp = GET_RW_32(chip, chan->dir, ch01_imwr_data); break; case 2: case 3: - tmp = GET_RW_32(dw, chan->dir, ch23_imwr_data); + tmp = GET_RW_32(chip, chan->dir, ch23_imwr_data); break; case 4: case 5: - tmp = GET_RW_32(dw, chan->dir, ch45_imwr_data); + tmp = GET_RW_32(chip, chan->dir, ch45_imwr_data); break; case 6: case 7: - tmp = GET_RW_32(dw, chan->dir, ch67_imwr_data); + tmp = GET_RW_32(chip, chan->dir, ch67_imwr_data); break; } @@ -478,22 +484,22 @@ int dw_edma_v0_core_device_config(struct dw_edma_chan *chan) switch (chan->id) { case 0: case 1: - SET_RW_32(dw, chan->dir, ch01_imwr_data, tmp); + SET_RW_32(chip, chan->dir, ch01_imwr_data, tmp); break; case 2: case 3: - SET_RW_32(dw, chan->dir, ch23_imwr_data, tmp); + SET_RW_32(chip, chan->dir, ch23_imwr_data, tmp); break; case 4: case 5: - SET_RW_32(dw, chan->dir, ch45_imwr_data, tmp); + SET_RW_32(chip, chan->dir, ch45_imwr_data, tmp); break; case 6: case 7: - SET_RW_32(dw, chan->dir, ch67_imwr_data, tmp); + SET_RW_32(chip, chan->dir, ch67_imwr_data, tmp); break; } diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.h b/drivers/dma/dw-edma/dw-edma-v0-core.h index 2afa626b8300c..01a29c74c0c43 100644 --- a/drivers/dma/dw-edma/dw-edma-v0-core.h +++ b/drivers/dma/dw-edma/dw-edma-v0-core.h @@ -12,13 +12,13 @@ #include <linux/dma/edma.h> /* eDMA management callbacks */ -void dw_edma_v0_core_off(struct dw_edma *chan); -u16 dw_edma_v0_core_ch_count(struct dw_edma *chan, enum dw_edma_dir dir); +void dw_edma_v0_core_off(struct dw_edma_chip *chip); +u16 dw_edma_v0_core_ch_count(struct dw_edma_chip *chip, enum dw_edma_dir dir); enum dma_status dw_edma_v0_core_ch_status(struct dw_edma_chan *chan); void dw_edma_v0_core_clear_done_int(struct dw_edma_chan *chan); void dw_edma_v0_core_clear_abort_int(struct dw_edma_chan *chan); -u32 dw_edma_v0_core_status_done_int(struct dw_edma *chan, enum dw_edma_dir dir); -u32 dw_edma_v0_core_status_abort_int(struct dw_edma *chan, enum dw_edma_dir dir); +u32 dw_edma_v0_core_status_done_int(struct dw_edma_chip *chip, enum dw_edma_dir dir); +u32 dw_edma_v0_core_status_abort_int(struct dw_edma_chip *chip, enum dw_edma_dir dir); void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first); int dw_edma_v0_core_device_config(struct dw_edma_chan *chan); /* eDMA debug fs callbacks */ diff --git a/drivers/dma/dw-edma/dw-edma-v0-debugfs.c b/drivers/dma/dw-edma/dw-edma-v0-debugfs.c index 4b3bcffd15ef1..5819a64aceb0f 100644 --- a/drivers/dma/dw-edma/dw-edma-v0-debugfs.c +++ b/drivers/dma/dw-edma/dw-edma-v0-debugfs.c @@ -38,7 +38,7 @@ #define CHANNEL_STR "channel" #define REGISTERS_STR "registers" -static struct dw_edma *dw; +static struct dw_edma_chip *chip; static struct dw_edma_v0_regs __iomem *regs; static struct { @@ -53,8 +53,10 @@ struct debugfs_entries { static int dw_edma_debugfs_u32_get(void *data, u64 *val) { + struct dw_edma *dw = chip->dw; + void __iomem *reg = (void __force __iomem *)data; - if (dw->mf == EDMA_MF_EDMA_LEGACY && + if (chip->mf == EDMA_MF_EDMA_LEGACY && reg >= (void __iomem *)®s->type.legacy.ch) { void __iomem *ptr = ®s->type.legacy.ch; u32 viewport_sel = 0; @@ -127,6 +129,8 @@ static void dw_edma_debugfs_regs_ch(struct dw_edma_v0_ch_regs __iomem *regs, static void dw_edma_debugfs_regs_wr(struct dentry *dir) { + struct dw_edma *dw = chip->dw; + const struct debugfs_entries debugfs_regs[] = { /* eDMA global registers */ WR_REGISTER(engine_en), @@ -173,7 +177,7 @@ static void dw_edma_debugfs_regs_wr(struct dentry *dir) nr_entries = ARRAY_SIZE(debugfs_regs); dw_edma_debugfs_create_x32(debugfs_regs, nr_entries, regs_dir); - if (dw->mf == EDMA_MF_HDMA_COMPAT) { + if (chip->mf == EDMA_MF_HDMA_COMPAT) { nr_entries = ARRAY_SIZE(debugfs_unroll_regs); dw_edma_debugfs_create_x32(debugfs_unroll_regs, nr_entries, regs_dir); @@ -195,6 +199,8 @@ static void dw_edma_debugfs_regs_wr(struct dentry *dir) static void dw_edma_debugfs_regs_rd(struct dentry *dir) { + struct dw_edma *dw = chip->dw; + const struct debugfs_entries debugfs_regs[] = { /* eDMA global registers */ RD_REGISTER(engine_en), @@ -242,7 +248,7 @@ static void dw_edma_debugfs_regs_rd(struct dentry *dir) nr_entries = ARRAY_SIZE(debugfs_regs); dw_edma_debugfs_create_x32(debugfs_regs, nr_entries, regs_dir); - if (dw->mf == EDMA_MF_HDMA_COMPAT) { + if (chip->mf == EDMA_MF_HDMA_COMPAT) { nr_entries = ARRAY_SIZE(debugfs_unroll_regs); dw_edma_debugfs_create_x32(debugfs_unroll_regs, nr_entries, regs_dir); @@ -264,6 +270,7 @@ static void dw_edma_debugfs_regs_rd(struct dentry *dir) static void dw_edma_debugfs_regs(void) { + struct dw_edma *dw = chip->dw; const struct debugfs_entries debugfs_regs[] = { REGISTER(ctrl_data_arb_prior), REGISTER(ctrl), @@ -282,13 +289,15 @@ static void dw_edma_debugfs_regs(void) dw_edma_debugfs_regs_rd(regs_dir); } -void dw_edma_v0_debugfs_on(struct dw_edma_chip *chip) +void dw_edma_v0_debugfs_on(struct dw_edma_chip *p) { - dw = chip->dw; - if (!dw) + struct dw_edma *dw; + chip = p; + if (!chip) return; - regs = dw->rg_region.vaddr; + dw = chip->dw; + regs = chip->reg_base; if (!regs) return; @@ -296,19 +305,19 @@ void dw_edma_v0_debugfs_on(struct dw_edma_chip *chip) if (!dw->debugfs) return; - debugfs_create_u32("mf", 0444, dw->debugfs, &dw->mf); + debugfs_create_u32("mf", 0444, dw->debugfs, &chip->mf); debugfs_create_u16("wr_ch_cnt", 0444, dw->debugfs, &dw->wr_ch_cnt); debugfs_create_u16("rd_ch_cnt", 0444, dw->debugfs, &dw->rd_ch_cnt); dw_edma_debugfs_regs(); } -void dw_edma_v0_debugfs_off(struct dw_edma_chip *chip) +void dw_edma_v0_debugfs_off(struct dw_edma_chip *p) { - dw = chip->dw; - if (!dw) + chip = p; + if (!chip) return; - debugfs_remove_recursive(dw->debugfs); - dw->debugfs = NULL; + debugfs_remove_recursive(chip->dw->debugfs); + chip->dw->debugfs = NULL; } diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h index cab6e18773dad..fcfbc0f47f83d 100644 --- a/include/linux/dma/edma.h +++ b/include/linux/dma/edma.h @@ -12,19 +12,62 @@ #include <linux/device.h> #include <linux/dmaengine.h> +#define EDMA_MAX_WR_CH 8 +#define EDMA_MAX_RD_CH 8 + struct dw_edma; +struct dw_edma_region { + phys_addr_t paddr; + void __iomem *vaddr; + size_t sz; +}; + +struct dw_edma_core_ops { + int (*irq_vector)(struct device *dev, unsigned int nr); +}; + +enum dw_edma_map_format { + EDMA_MF_EDMA_LEGACY = 0x0, + EDMA_MF_EDMA_UNROLL = 0x1, + EDMA_MF_HDMA_COMPAT = 0x5 +}; + /** * struct dw_edma_chip - representation of DesignWare eDMA controller hardware * @dev: struct device of the eDMA controller * @id: instance ID - * @irq: irq line + * @nr_irqs: total dma irq number + * reg64bit if support 64bit write to register + * @ops DMA channel to IRQ number mapping + * @wr_ch_cnt DMA write channel number + * @rd_ch_cnt DMA read channel number + * @rg_region DMA register region + * @ll_region_wr DMA descriptor link list memory for write channel + * @ll_region_rd DMA descriptor link list memory for read channel + * @mf DMA register map format * @dw: struct dw_edma that is filed by dw_edma_probe() */ struct dw_edma_chip { struct device *dev; int id; - int irq; + int nr_irqs; + const struct dw_edma_core_ops *ops; + + void __iomem *reg_base; + + u16 ll_wr_cnt; + u16 ll_rd_cnt; + /* link list address */ + struct dw_edma_region ll_region_wr[EDMA_MAX_WR_CH]; + struct dw_edma_region ll_region_rd[EDMA_MAX_RD_CH]; + + /* data region */ + struct dw_edma_region dt_region_wr[EDMA_MAX_WR_CH]; + struct dw_edma_region dt_region_rd[EDMA_MAX_RD_CH]; + + enum dw_edma_map_format mf; + struct dw_edma *dw; };
"struct dw_edma_chip" contains an internal structure "struct dw_edma" that is used by the eDMA core internally. This structure should not be touched by the eDMA controller drivers themselves. But currently, the eDMA controller drivers like "dw-edma-pci" allocates and populates this internal structure then passes it on to eDMA core. The eDMA core further populates the structure and uses it. This is wrong! Hence, move all the "struct dw_edma" specifics from controller drivers to the eDMA core. Signed-off-by: Frank Li <Frank.Li@nxp.com> --- Resend added dmaengine@vger.kernel.org Change from v2 to v3 - none Change from v1 to v2 - rework commit message - remove duplicate field in struct dw_edma drivers/dma/dw-edma/dw-edma-core.c | 91 +++++----- drivers/dma/dw-edma/dw-edma-core.h | 30 +--- drivers/dma/dw-edma/dw-edma-v0-core.c | 206 ++++++++++++----------- drivers/dma/dw-edma/dw-edma-v0-core.h | 8 +- drivers/dma/dw-edma/dw-edma-v0-debugfs.c | 37 ++-- include/linux/dma/edma.h | 47 +++++- 6 files changed, 230 insertions(+), 189 deletions(-)