| Message ID | 20190120023150.17138-3-angus@akkea.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma: imx-sdma: add the sdma engine to the imx8mq | expand |
On Sun, Jan 20, 2019 at 4:32 AM Angus Ainslie (Purism) <angus@akkea.ca> wrote: > > On i.mx8 mscale B0 chip, AHB/SDMA clock ratio 2:1 can't be supportted, > since SDMA clock ratio has to be increased to 250Mhz, AHB can't reach > to 500Mhz, so use 1:1 instead. > > based on NXP commit MLK-16841-1 Hi Angus, Thanks for doing this! I'm not sure specifying the MLK here helps. I think it would be better to somehow add the original Signed-off-by and mention that the commit was pulled from NXP linux-imx tree. > > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca> > --- > .../devicetree/bindings/dma/fsl-imx-sdma.txt | 1 + > drivers/dma/imx-sdma.c | 20 +++++++++++++++---- > 2 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt > index 3c9a57a8443b..17544c1820b7 100644 > --- a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt > +++ b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt > @@ -67,6 +67,7 @@ Optional properties: > reg is the GPR register offset. > shift is the bit position inside the GPR register. > val is the value of the bit (0 or 1). > +- fsl,ratio-1-1: AHB/SDMA core clock ration 1:1, 2:1 without this. > > Examples: > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c > index 0b3a67ff8e82..65dada21d3c1 100644 > --- a/drivers/dma/imx-sdma.c > +++ b/drivers/dma/imx-sdma.c > @@ -440,6 +440,8 @@ struct sdma_engine { > unsigned int irq; > dma_addr_t bd0_phys; > struct sdma_buffer_descriptor *bd0; > + /* clock ratio for AHB:SDMA core. 1:1 is 1, 2:1 is 0*/ > + bool clk_ratio; > }; > > static int sdma_config_write(struct dma_chan *chan, > @@ -662,8 +664,14 @@ static int sdma_run_channel0(struct sdma_engine *sdma) > dev_err(sdma->dev, "Timeout waiting for CH0 ready\n"); > > /* Set bits of CONFIG register with dynamic context switching */ > - if (readl(sdma->regs + SDMA_H_CONFIG) == 0) > - writel_relaxed(SDMA_H_CONFIG_CSM, sdma->regs + SDMA_H_CONFIG); > + if (readl(sdma->regs + SDMA_H_CONFIG) == 0) { > + if (sdma->clk_ratio) > + reg = SDMA_H_CONFIG_CSM | SDMA_H_CONFIG_ACR; > + else > + reg = SDMA_H_CONFIG_CSM; > + > + writel_relaxed(reg, sdma->regs + SDMA_H_CONFIG); > + } > > return ret; > } > @@ -1880,8 +1888,10 @@ static int sdma_init(struct sdma_engine *sdma) > writel_relaxed(0x4050, sdma->regs + SDMA_CHN0ADDR); > > /* Set bits of CONFIG register but with static context switching */ > - /* FIXME: Check whether to set ACR bit depending on clock ratios */ > - writel_relaxed(0, sdma->regs + SDMA_H_CONFIG); > + if (sdma->clk_ratio) > + writel_relaxed(SDMA_H_CONFIG_ACR, sdma->regs + SDMA_H_CONFIG); > + else > + writel_relaxed(0, sdma->regs + SDMA_H_CONFIG); > > writel_relaxed(ccb_phys, sdma->regs + SDMA_H_C0PTR); > > @@ -1975,6 +1985,8 @@ static int sdma_probe(struct platform_device *pdev) > if (!sdma) > return -ENOMEM; > > + sdma->clk_ratio = of_property_read_bool(np, "fsl,ratio-1-1"); > + > spin_lock_init(&sdma->channel_0_lock); > > sdma->dev = &pdev->dev; > -- > 2.17.1 >
Hi Daniel, On 2019-01-20 02:58, Daniel Baluta wrote: > On Sun, Jan 20, 2019 at 4:32 AM Angus Ainslie (Purism) <angus@akkea.ca> > wrote: >> >> On i.mx8 mscale B0 chip, AHB/SDMA clock ratio 2:1 can't be supportted, >> since SDMA clock ratio has to be increased to 250Mhz, AHB can't reach >> to 500Mhz, so use 1:1 instead. >> >> based on NXP commit MLK-16841-1 > > Hi Angus, > > Thanks for doing this! > > I'm not sure specifying the MLK here helps. I think it would be better > to somehow add the original Signed-off-by and mention that the commit > was pulled from NXP linux-imx tree. If it was exactly the same patch I would have but as I added the lines in sdma_run_channel0 I didn't think it would be right to put signed off by "Robin Gong <yibin.gong@nxp.com>" as the code isn't what he signed off on. >> >> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca> >> --- >> .../devicetree/bindings/dma/fsl-imx-sdma.txt | 1 + >> drivers/dma/imx-sdma.c | 20 >> +++++++++++++++---- >> 2 files changed, 17 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt >> b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt >> index 3c9a57a8443b..17544c1820b7 100644 >> --- a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt >> +++ b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt >> @@ -67,6 +67,7 @@ Optional properties: >> reg is the GPR register offset. >> shift is the bit position inside the GPR register. >> val is the value of the bit (0 or 1). >> +- fsl,ratio-1-1: AHB/SDMA core clock ration 1:1, 2:1 without this. >> >> Examples: >> >> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c >> index 0b3a67ff8e82..65dada21d3c1 100644 >> --- a/drivers/dma/imx-sdma.c >> +++ b/drivers/dma/imx-sdma.c >> @@ -440,6 +440,8 @@ struct sdma_engine { >> unsigned int irq; >> dma_addr_t bd0_phys; >> struct sdma_buffer_descriptor *bd0; >> + /* clock ratio for AHB:SDMA core. 1:1 is 1, 2:1 is 0*/ >> + bool clk_ratio; >> }; >> >> static int sdma_config_write(struct dma_chan *chan, >> @@ -662,8 +664,14 @@ static int sdma_run_channel0(struct sdma_engine >> *sdma) >> dev_err(sdma->dev, "Timeout waiting for CH0 ready\n"); >> >> /* Set bits of CONFIG register with dynamic context switching >> */ >> - if (readl(sdma->regs + SDMA_H_CONFIG) == 0) >> - writel_relaxed(SDMA_H_CONFIG_CSM, sdma->regs + >> SDMA_H_CONFIG); >> + if (readl(sdma->regs + SDMA_H_CONFIG) == 0) { >> + if (sdma->clk_ratio) >> + reg = SDMA_H_CONFIG_CSM | SDMA_H_CONFIG_ACR; >> + else >> + reg = SDMA_H_CONFIG_CSM; >> + >> + writel_relaxed(reg, sdma->regs + SDMA_H_CONFIG); >> + } >> This is the code that I added out of an over abundance of prudence. Angus >> return ret; >> } >> @@ -1880,8 +1888,10 @@ static int sdma_init(struct sdma_engine *sdma) >> writel_relaxed(0x4050, sdma->regs + SDMA_CHN0ADDR); >> >> /* Set bits of CONFIG register but with static context >> switching */ >> - /* FIXME: Check whether to set ACR bit depending on clock >> ratios */ >> - writel_relaxed(0, sdma->regs + SDMA_H_CONFIG); >> + if (sdma->clk_ratio) >> + writel_relaxed(SDMA_H_CONFIG_ACR, sdma->regs + >> SDMA_H_CONFIG); >> + else >> + writel_relaxed(0, sdma->regs + SDMA_H_CONFIG); >> >> writel_relaxed(ccb_phys, sdma->regs + SDMA_H_C0PTR); >> >> @@ -1975,6 +1985,8 @@ static int sdma_probe(struct platform_device >> *pdev) >> if (!sdma) >> return -ENOMEM; >> >> + sdma->clk_ratio = of_property_read_bool(np, "fsl,ratio-1-1"); >> + >> spin_lock_init(&sdma->channel_0_lock); >> >> sdma->dev = &pdev->dev; >> -- >> 2.17.1 >>
On Sun, Jan 20, 2019 at 4:38 PM Angus Ainslie <angus@akkea.ca> wrote: > > Hi Daniel, > > On 2019-01-20 02:58, Daniel Baluta wrote: > > On Sun, Jan 20, 2019 at 4:32 AM Angus Ainslie (Purism) <angus@akkea.ca> > > wrote: > >> > >> On i.mx8 mscale B0 chip, AHB/SDMA clock ratio 2:1 can't be supportted, > >> since SDMA clock ratio has to be increased to 250Mhz, AHB can't reach > >> to 500Mhz, so use 1:1 instead. > >> > >> based on NXP commit MLK-16841-1 > > > > Hi Angus, > > > > Thanks for doing this! > > > > I'm not sure specifying the MLK here helps. I think it would be better > > to somehow add the original Signed-off-by and mention that the commit > > was pulled from NXP linux-imx tree. > > If it was exactly the same patch I would have but as I added the lines > in > sdma_run_channel0 I didn't think it would be right to put signed off by > "Robin Gong <yibin.gong@nxp.com>" as the code isn't what he signed off > on. I see your point. I often encounter this scenario when sending patches. I think the best solution would be to mention in the commit message the author of the initial patch which you based your work on.
Hi Angus, Am Samstag, den 19.01.2019, 19:31 -0700 schrieb Angus Ainslie (Purism): > On i.mx8 mscale B0 chip, AHB/SDMA clock ratio 2:1 can't be supportted, > since SDMA clock ratio has to be increased to 250Mhz, AHB can't reach > to 500Mhz, so use 1:1 instead. > > based on NXP commit MLK-16841-1 > > > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca> > --- > .../devicetree/bindings/dma/fsl-imx-sdma.txt | 1 + > drivers/dma/imx-sdma.c | 20 +++++++++++++++---- > 2 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt > index 3c9a57a8443b..17544c1820b7 100644 > --- a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt > +++ b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt > @@ -67,6 +67,7 @@ Optional properties: > reg is the GPR register offset. > shift is the bit position inside the GPR register. > val is the value of the bit (0 or 1). > +- fsl,ratio-1-1: AHB/SDMA core clock ration 1:1, 2:1 without this. Why does this need a separate DT property? Shouldn't the driver be able to infer this from the clock rates going into the SDMA hardware? Regards, Lucas > Examples: > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c > index 0b3a67ff8e82..65dada21d3c1 100644 > --- a/drivers/dma/imx-sdma.c > +++ b/drivers/dma/imx-sdma.c > @@ -440,6 +440,8 @@ struct sdma_engine { > > > unsigned int irq; > > > dma_addr_t bd0_phys; > > > struct sdma_buffer_descriptor *bd0; > > + /* clock ratio for AHB:SDMA core. 1:1 is 1, 2:1 is 0*/ > > > + bool clk_ratio; > }; > > static int sdma_config_write(struct dma_chan *chan, > @@ -662,8 +664,14 @@ static int sdma_run_channel0(struct sdma_engine *sdma) > > dev_err(sdma->dev, "Timeout waiting for CH0 ready\n"); > > > /* Set bits of CONFIG register with dynamic context switching */ > > - if (readl(sdma->regs + SDMA_H_CONFIG) == 0) > > - writel_relaxed(SDMA_H_CONFIG_CSM, sdma->regs + SDMA_H_CONFIG); > > + if (readl(sdma->regs + SDMA_H_CONFIG) == 0) { > > + if (sdma->clk_ratio) > > + reg = SDMA_H_CONFIG_CSM | SDMA_H_CONFIG_ACR; > > + else > > + reg = SDMA_H_CONFIG_CSM; > + > > + writel_relaxed(reg, sdma->regs + SDMA_H_CONFIG); > > + } > > > return ret; > } > @@ -1880,8 +1888,10 @@ static int sdma_init(struct sdma_engine *sdma) > > writel_relaxed(0x4050, sdma->regs + SDMA_CHN0ADDR); > > > /* Set bits of CONFIG register but with static context switching */ > > - /* FIXME: Check whether to set ACR bit depending on clock ratios */ > > - writel_relaxed(0, sdma->regs + SDMA_H_CONFIG); > > + if (sdma->clk_ratio) > > + writel_relaxed(SDMA_H_CONFIG_ACR, sdma->regs + SDMA_H_CONFIG); > > + else > > + writel_relaxed(0, sdma->regs + SDMA_H_CONFIG); > > > writel_relaxed(ccb_phys, sdma->regs + SDMA_H_C0PTR); > > @@ -1975,6 +1985,8 @@ static int sdma_probe(struct platform_device *pdev) > > if (!sdma) > > return -ENOMEM; > > > + sdma->clk_ratio = of_property_read_bool(np, "fsl,ratio-1-1"); > + > > spin_lock_init(&sdma->channel_0_lock); > > > sdma->dev = &pdev->dev;
Hi Lucas, On 2019-01-21 02:17, Lucas Stach wrote: > Hi Angus, > > Am Samstag, den 19.01.2019, 19:31 -0700 schrieb Angus Ainslie (Purism): >> On i.mx8 mscale B0 chip, AHB/SDMA clock ratio 2:1 can't be supportted, >> since SDMA clock ratio has to be increased to 250Mhz, AHB can't reach >> to 500Mhz, so use 1:1 instead. >> >> based on NXP commit MLK-16841-1 >> >> > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca> >> --- >> .../devicetree/bindings/dma/fsl-imx-sdma.txt | 1 + >> drivers/dma/imx-sdma.c | 20 >> +++++++++++++++---- >> 2 files changed, 17 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt >> b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt >> index 3c9a57a8443b..17544c1820b7 100644 >> --- a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt >> +++ b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt >> @@ -67,6 +67,7 @@ Optional properties: >> reg is the GPR register offset. >> shift is the bit position inside the GPR register. >> val is the value of the bit (0 or 1). >> +- fsl,ratio-1-1: AHB/SDMA core clock ration 1:1, 2:1 without this. > > Why does this need a separate DT property? Shouldn't the driver be able > to infer this from the clock rates going into the SDMA hardware? > That's probably a better way to do it then setting the property can't get missed. I'll address it in v3. Angus > Regards, > Lucas > >> Examples: >> >> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c >> index 0b3a67ff8e82..65dada21d3c1 100644 >> --- a/drivers/dma/imx-sdma.c >> +++ b/drivers/dma/imx-sdma.c >> @@ -440,6 +440,8 @@ struct sdma_engine { >> > > unsigned int irq; >> > > dma_addr_t bd0_phys; >> > > struct sdma_buffer_descriptor *bd0; >> > + /* clock ratio for AHB:SDMA core. 1:1 is 1, 2:1 is 0*/ >> > > + bool clk_ratio; >> }; >> >> static int sdma_config_write(struct dma_chan *chan, >> @@ -662,8 +664,14 @@ static int sdma_run_channel0(struct sdma_engine >> *sdma) >> > dev_err(sdma->dev, "Timeout waiting for CH0 ready\n"); >> >> > /* Set bits of CONFIG register with dynamic context switching */ >> > - if (readl(sdma->regs + SDMA_H_CONFIG) == 0) >> > - writel_relaxed(SDMA_H_CONFIG_CSM, sdma->regs + SDMA_H_CONFIG); >> > + if (readl(sdma->regs + SDMA_H_CONFIG) == 0) { >> > + if (sdma->clk_ratio) >> > + reg = SDMA_H_CONFIG_CSM | SDMA_H_CONFIG_ACR; >> > + else >> > + reg = SDMA_H_CONFIG_CSM; >> + >> > + writel_relaxed(reg, sdma->regs + SDMA_H_CONFIG); >> > + } >> >> > return ret; >> } >> @@ -1880,8 +1888,10 @@ static int sdma_init(struct sdma_engine *sdma) >> > writel_relaxed(0x4050, sdma->regs + SDMA_CHN0ADDR); >> >> > /* Set bits of CONFIG register but with static context switching */ >> > - /* FIXME: Check whether to set ACR bit depending on clock ratios */ >> > - writel_relaxed(0, sdma->regs + SDMA_H_CONFIG); >> > + if (sdma->clk_ratio) >> > + writel_relaxed(SDMA_H_CONFIG_ACR, sdma->regs + SDMA_H_CONFIG); >> > + else >> > + writel_relaxed(0, sdma->regs + SDMA_H_CONFIG); >> >> > writel_relaxed(ccb_phys, sdma->regs + SDMA_H_C0PTR); >> >> @@ -1975,6 +1985,8 @@ static int sdma_probe(struct platform_device >> *pdev) >> > if (!sdma) >> > return -ENOMEM; >> >> > + sdma->clk_ratio = of_property_read_bool(np, "fsl,ratio-1-1"); >> + >> > spin_lock_init(&sdma->channel_0_lock); >> >> > sdma->dev = &pdev->dev;
diff --git a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt index 3c9a57a8443b..17544c1820b7 100644 --- a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt +++ b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt @@ -67,6 +67,7 @@ Optional properties: reg is the GPR register offset. shift is the bit position inside the GPR register. val is the value of the bit (0 or 1). +- fsl,ratio-1-1: AHB/SDMA core clock ration 1:1, 2:1 without this. Examples: diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index 0b3a67ff8e82..65dada21d3c1 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -440,6 +440,8 @@ struct sdma_engine { unsigned int irq; dma_addr_t bd0_phys; struct sdma_buffer_descriptor *bd0; + /* clock ratio for AHB:SDMA core. 1:1 is 1, 2:1 is 0*/ + bool clk_ratio; }; static int sdma_config_write(struct dma_chan *chan, @@ -662,8 +664,14 @@ static int sdma_run_channel0(struct sdma_engine *sdma) dev_err(sdma->dev, "Timeout waiting for CH0 ready\n"); /* Set bits of CONFIG register with dynamic context switching */ - if (readl(sdma->regs + SDMA_H_CONFIG) == 0) - writel_relaxed(SDMA_H_CONFIG_CSM, sdma->regs + SDMA_H_CONFIG); + if (readl(sdma->regs + SDMA_H_CONFIG) == 0) { + if (sdma->clk_ratio) + reg = SDMA_H_CONFIG_CSM | SDMA_H_CONFIG_ACR; + else + reg = SDMA_H_CONFIG_CSM; + + writel_relaxed(reg, sdma->regs + SDMA_H_CONFIG); + } return ret; } @@ -1880,8 +1888,10 @@ static int sdma_init(struct sdma_engine *sdma) writel_relaxed(0x4050, sdma->regs + SDMA_CHN0ADDR); /* Set bits of CONFIG register but with static context switching */ - /* FIXME: Check whether to set ACR bit depending on clock ratios */ - writel_relaxed(0, sdma->regs + SDMA_H_CONFIG); + if (sdma->clk_ratio) + writel_relaxed(SDMA_H_CONFIG_ACR, sdma->regs + SDMA_H_CONFIG); + else + writel_relaxed(0, sdma->regs + SDMA_H_CONFIG); writel_relaxed(ccb_phys, sdma->regs + SDMA_H_C0PTR); @@ -1975,6 +1985,8 @@ static int sdma_probe(struct platform_device *pdev) if (!sdma) return -ENOMEM; + sdma->clk_ratio = of_property_read_bool(np, "fsl,ratio-1-1"); + spin_lock_init(&sdma->channel_0_lock); sdma->dev = &pdev->dev;
On i.mx8 mscale B0 chip, AHB/SDMA clock ratio 2:1 can't be supportted, since SDMA clock ratio has to be increased to 250Mhz, AHB can't reach to 500Mhz, so use 1:1 instead. based on NXP commit MLK-16841-1 Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca> --- .../devicetree/bindings/dma/fsl-imx-sdma.txt | 1 + drivers/dma/imx-sdma.c | 20 +++++++++++++++---- 2 files changed, 17 insertions(+), 4 deletions(-)