| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1530827112.6642.2@smtp.crapouillou.net>
Date: Thu, 05 Jul 2018 23:45:12 +0200
From: Paul Cercueil <paul@...pouillou.net>
To: PrasannaKumar Muralidharan <prasannatsmkumar@...il.com>
Cc: Vinod Koul <vkoul@...nel.org>, Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Ralf Baechle <ralf@...ux-mips.org>,
Paul Burton <paul.burton@...s.com>,
James Hogan <jhogan@...nel.org>,
Zubair Lutfullah Kakakhel <Zubair.Kakakhel@...tec.com>,
Mathieu Malaterre <malat@...ian.org>,
Daniel Silsby <dansilsby@...il.com>, dmaengine@...r.kernel.org,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>,
open list <linux-kernel@...r.kernel.org>,
Linux-MIPS <linux-mips@...ux-mips.org>
Subject: Re: [PATCH 02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers
> Paul,
>
> On 3 July 2018 at 18:02, Paul Cercueil <paul@...pouillou.net> wrote:
>> The register area of the JZ4780 DMA core can be split into different
>> sections for different purposes:
>>
>> * one set of registers is used to perform actions at the DMA core
>> level,
>> that will generally affect all channels;
>>
>> * one set of registers per DMA channel, to perform actions at the
>> DMA
>> channel level, that will only affect the channel in question.
>>
>> The problem rises when trying to support new versions of the JZ47xx
>> Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one
>> with six DMA channels, and the register sets are interleaved:
>> <DMA0 chan regs> <DMA1 chan regs> <DMA0 ctrl regs> <DMA1 ctrl regs>
>>
>> By using one memory resource for the channel-specific registers and
>> one memory resource for the core-specific registers, we can support
>> the JZ4770, by initializing the driver once per DMA core with
>> different
>> addresses.
>
> As per my understanding device tree should be modified only when
> hardware changes. This looks the other way around. It must be possible
> to achieve what you are trying to do in this patch without changing
> the device tree.
I would agree that devicetree has an ABI that we shouldn't break if
possible.
However DTS support for all the Ingenic SoCs/boards is far from being
complete, and more importantly, all Ingenic-based boards compile the DTS
file within the kernel; so breaking the ABI is not (yet) a problem, and
we should push the big changes right now while it's still possible.
>> Signed-off-by: Paul Cercueil <paul@...pouillou.net>
>> ---
>> .../devicetree/bindings/dma/jz4780-dma.txt | 6 +-
>> drivers/dma/dma-jz4780.c | 106
>> +++++++++++-------
>> 2 files changed, 69 insertions(+), 43 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
>> b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
>> index f25feee62b15..f9b1864f5b77 100644
>> --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
>> +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
>> @@ -3,7 +3,8 @@
>> Required properties:
>>
>> - compatible: Should be "ingenic,jz4780-dma"
>> -- reg: Should contain the DMA controller registers location and
>> length.
>> +- reg: Should contain the DMA channel registers location and
>> length, followed
>> + by the DMA controller registers location and length.
>> - interrupts: Should contain the interrupt specifier of the DMA
>> controller.
>> - interrupt-parent: Should be the phandle of the interrupt
>> controller that
>> - clocks: Should contain a clock specifier for the JZ4780 PDMA
>> clock.
>> @@ -22,7 +23,8 @@ Example:
>>
>> dma: dma@...20000 {
>> compatible = "ingenic,jz4780-dma";
>> - reg = <0x13420000 0x10000>;
>> + reg = <0x13420000 0x400
>> + 0x13421000 0x40>;
>>
>> interrupt-parent = <&intc>;
>> interrupts = <10>;
>> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
>> index b40f491f0367..4d234caf5d62 100644
>> --- a/drivers/dma/dma-jz4780.c
>> +++ b/drivers/dma/dma-jz4780.c
>> @@ -25,26 +25,26 @@
>> #include "virt-dma.h"
>>
>> /* Global registers. */
>> -#define JZ_DMA_REG_DMAC 0x1000
>> -#define JZ_DMA_REG_DIRQP 0x1004
>> -#define JZ_DMA_REG_DDR 0x1008
>> -#define JZ_DMA_REG_DDRS 0x100c
>> -#define JZ_DMA_REG_DMACP 0x101c
>> -#define JZ_DMA_REG_DSIRQP 0x1020
>> -#define JZ_DMA_REG_DSIRQM 0x1024
>> -#define JZ_DMA_REG_DCIRQP 0x1028
>> -#define JZ_DMA_REG_DCIRQM 0x102c
>> +#define JZ_DMA_REG_DMAC 0x00
>> +#define JZ_DMA_REG_DIRQP 0x04
>> +#define JZ_DMA_REG_DDR 0x08
>> +#define JZ_DMA_REG_DDRS 0x0c
>> +#define JZ_DMA_REG_DMACP 0x1c
>> +#define JZ_DMA_REG_DSIRQP 0x20
>> +#define JZ_DMA_REG_DSIRQM 0x24
>> +#define JZ_DMA_REG_DCIRQP 0x28
>> +#define JZ_DMA_REG_DCIRQM 0x2c
>>
>> /* Per-channel registers. */
>> #define JZ_DMA_REG_CHAN(n) (n * 0x20)
>> -#define JZ_DMA_REG_DSA(n) (0x00 + JZ_DMA_REG_CHAN(n))
>> -#define JZ_DMA_REG_DTA(n) (0x04 + JZ_DMA_REG_CHAN(n))
>> -#define JZ_DMA_REG_DTC(n) (0x08 + JZ_DMA_REG_CHAN(n))
>> -#define JZ_DMA_REG_DRT(n) (0x0c + JZ_DMA_REG_CHAN(n))
>> -#define JZ_DMA_REG_DCS(n) (0x10 + JZ_DMA_REG_CHAN(n))
>> -#define JZ_DMA_REG_DCM(n) (0x14 + JZ_DMA_REG_CHAN(n))
>> -#define JZ_DMA_REG_DDA(n) (0x18 + JZ_DMA_REG_CHAN(n))
>> -#define JZ_DMA_REG_DSD(n) (0x1c + JZ_DMA_REG_CHAN(n))
>> +#define JZ_DMA_REG_DSA 0x00
>> +#define JZ_DMA_REG_DTA 0x04
>> +#define JZ_DMA_REG_DTC 0x08
>> +#define JZ_DMA_REG_DRT 0x0c
>> +#define JZ_DMA_REG_DCS 0x10
>> +#define JZ_DMA_REG_DCM 0x14
>> +#define JZ_DMA_REG_DDA 0x18
>> +#define JZ_DMA_REG_DSD 0x1c
>>
>> #define JZ_DMA_DMAC_DMAE BIT(0)
>> #define JZ_DMA_DMAC_AR BIT(2)
>> @@ -140,7 +140,8 @@ enum jz_version {
>>
>> struct jz4780_dma_dev {
>> struct dma_device dma_device;
>> - void __iomem *base;
>> + void __iomem *chn_base;
>> + void __iomem *ctrl_base;
>> struct clk *clk;
>> unsigned int irq;
>> unsigned int nb_channels;
>> @@ -174,16 +175,28 @@ static inline struct jz4780_dma_dev
>> *jz4780_dma_chan_parent(
>> dma_device);
>> }
>>
>> -static inline uint32_t jz4780_dma_readl(struct jz4780_dma_dev
>> *jzdma,
>> +static inline uint32_t jz4780_dma_chn_readl(struct jz4780_dma_dev
>> *jzdma,
>> + unsigned int chn, unsigned int reg)
>> +{
>> + return readl(jzdma->chn_base + reg + JZ_DMA_REG_CHAN(chn));
>> +}
>> +
>> +static inline void jz4780_dma_chn_writel(struct jz4780_dma_dev
>> *jzdma,
>> + unsigned int chn, unsigned int reg, uint32_t val)
>> +{
>> + writel(val, jzdma->chn_base + reg + JZ_DMA_REG_CHAN(chn));
>> +}
>> +
>> +static inline uint32_t jz4780_dma_ctrl_readl(struct jz4780_dma_dev
>> *jzdma,
>> unsigned int reg)
>> {
>> - return readl(jzdma->base + reg);
>> + return readl(jzdma->ctrl_base + reg);
>> }
>>
>> -static inline void jz4780_dma_writel(struct jz4780_dma_dev *jzdma,
>> +static inline void jz4780_dma_ctrl_writel(struct jz4780_dma_dev
>> *jzdma,
>> unsigned int reg, uint32_t val)
>> {
>> - writel(val, jzdma->base + reg);
>> + writel(val, jzdma->ctrl_base + reg);
>> }
>>
>> static struct jz4780_dma_desc *jz4780_dma_desc_alloc(
>> @@ -478,17 +491,18 @@ static void jz4780_dma_begin(struct
>> jz4780_dma_chan *jzchan)
>> }
>>
>> /* Use 8-word descriptors. */
>> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id),
>> JZ_DMA_DCS_DES8);
>> + jz4780_dma_chn_writel(jzdma, jzchan->id,
>> + JZ_DMA_REG_DCS, JZ_DMA_DCS_DES8);
>>
>> /* Write descriptor address and initiate descriptor fetch.
>> */
>> desc_phys = jzchan->desc->desc_phys +
>> (jzchan->curr_hwdesc *
>> sizeof(*jzchan->desc->desc));
>> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DDA(jzchan->id),
>> desc_phys);
>> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DDRS, BIT(jzchan->id));
>> + jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DDA,
>> desc_phys);
>> + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DDRS,
>> BIT(jzchan->id));
>>
>> /* Enable the channel. */
>> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id),
>> - JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE);
>> + jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS,
>> + JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE);
>> }
>>
>> static void jz4780_dma_issue_pending(struct dma_chan *chan)
>> @@ -514,7 +528,7 @@ static int jz4780_dma_terminate_all(struct
>> dma_chan *chan)
>> spin_lock_irqsave(&jzchan->vchan.lock, flags);
>>
>> /* Clear the DMA status and stop the transfer. */
>> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), 0);
>> + jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0);
>> if (jzchan->desc) {
>> vchan_terminate_vdesc(&jzchan->desc->vdesc);
>> jzchan->desc = NULL;
>> @@ -563,8 +577,8 @@ static size_t jz4780_dma_desc_residue(struct
>> jz4780_dma_chan *jzchan,
>> residue += desc->desc[i].dtc <<
>> jzchan->transfer_shift;
>>
>> if (next_sg != 0) {
>> - count = jz4780_dma_readl(jzdma,
>> -
>> JZ_DMA_REG_DTC(jzchan->id));
>> + count = jz4780_dma_chn_readl(jzdma, jzchan->id,
>> + JZ_DMA_REG_DTC);
>> residue += count << jzchan->transfer_shift;
>> }
>>
>> @@ -611,8 +625,8 @@ static void jz4780_dma_chan_irq(struct
>> jz4780_dma_dev *jzdma,
>>
>> spin_lock(&jzchan->vchan.lock);
>>
>> - dcs = jz4780_dma_readl(jzdma, JZ_DMA_REG_DCS(jzchan->id));
>> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), 0);
>> + dcs = jz4780_dma_chn_readl(jzdma, jzchan->id,
>> JZ_DMA_REG_DCS);
>> + jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0);
>>
>> if (dcs & JZ_DMA_DCS_AR) {
>> dev_warn(&jzchan->vchan.chan.dev->device,
>> @@ -651,7 +665,7 @@ static irqreturn_t jz4780_dma_irq_handler(int
>> irq, void *data)
>> uint32_t pending, dmac;
>> int i;
>>
>> - pending = jz4780_dma_readl(jzdma, JZ_DMA_REG_DIRQP);
>> + pending = jz4780_dma_ctrl_readl(jzdma, JZ_DMA_REG_DIRQP);
>>
>> for (i = 0; i < jzdma->nb_channels; i++) {
>> if (!(pending & (1<<i)))
>> @@ -661,12 +675,12 @@ static irqreturn_t jz4780_dma_irq_handler(int
>> irq, void *data)
>> }
>>
>> /* Clear halt and address error status of all channels. */
>> - dmac = jz4780_dma_readl(jzdma, JZ_DMA_REG_DMAC);
>> + dmac = jz4780_dma_ctrl_readl(jzdma, JZ_DMA_REG_DMAC);
>> dmac &= ~(JZ_DMA_DMAC_HLT | JZ_DMA_DMAC_AR);
>> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DMAC, dmac);
>> + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC, dmac);
>>
>> /* Clear interrupt pending status. */
>> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DIRQP, 0);
>> + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DIRQP, 0);
>>
>> return IRQ_HANDLED;
>> }
>> @@ -804,9 +818,19 @@ static int jz4780_dma_probe(struct
>> platform_device *pdev)
>> return -EINVAL;
>> }
>>
>> - jzdma->base = devm_ioremap_resource(dev, res);
>> - if (IS_ERR(jzdma->base))
>> - return PTR_ERR(jzdma->base);
>> + jzdma->chn_base = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(jzdma->chn_base))
>> + return PTR_ERR(jzdma->chn_base);
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> + if (!res) {
>> + dev_err(dev, "failed to get I/O memory\n");
>> + return -EINVAL;
>> + }
>> +
>> + jzdma->ctrl_base = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(jzdma->ctrl_base))
>> + return PTR_ERR(jzdma->ctrl_base);
>>
>> ret = platform_get_irq(pdev, 0);
>> if (ret < 0) {
>> @@ -864,9 +888,9 @@ static int jz4780_dma_probe(struct
>> platform_device *pdev)
>> * Also set the FMSC bit - it increases MSC performance, so
>> it makes
>> * little sense not to enable it.
>> */
>> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DMAC,
>> + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC,
>> JZ_DMA_DMAC_DMAE | JZ_DMA_DMAC_FMSC);
>> - jz4780_dma_writel(jzdma, JZ_DMA_REG_DMACP, 0);
>> + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMACP, 0);
>>
>> INIT_LIST_HEAD(&dd->channels);
>>
>> --
>> 2.18.0
>>
>>
>
> Regards,
> PrasannaKumar
Powered by blists - more mailing lists