| Message ID | 20180116190236.14558-1-srinivas.kandagatla@linaro.org |
|---|---|
| Headers | show |
| Series | dmaengine: qcom: bam_dma: fixes for remotely controlled bam | expand |
On 1/16/2018 12:02 PM, srinivas.kandagatla@linaro.org wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > When BAM is remotely controlled it does not sound correct to control > its clk on Linux side. Make it optional, so that its not madatory > for remote controlled BAM instances. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > drivers/dma/qcom/bam_dma.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c > index 03c4eb3fd314..78e488e8f96d 100644 > --- a/drivers/dma/qcom/bam_dma.c > +++ b/drivers/dma/qcom/bam_dma.c > @@ -1180,13 +1180,14 @@ static int bam_dma_probe(struct platform_device *pdev) > "qcom,controlled-remotely"); > > bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk"); > - if (IS_ERR(bdev->bamclk)) > - return PTR_ERR(bdev->bamclk); > - > - ret = clk_prepare_enable(bdev->bamclk); > - if (ret) { > - dev_err(bdev->dev, "failed to prepare/enable clock\n"); > - return ret; > + if (IS_ERR(bdev->bamclk)) { > + bdev->bamclk = NULL; > + } else { > + ret = clk_prepare_enable(bdev->bamclk); > + if (ret) { > + dev_err(bdev->dev, "failed to prepare/enable clock\n"); > + return ret; > + } I believe you can also keep pm_runtime disabled if BAM is remotely controlled. Thanks Sagar > } > > ret = bam_init(bdev); -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16/01/18 19:38, Sagar Dharia wrote: > > On 1/16/2018 12:02 PM, srinivas.kandagatla@linaro.org wrote: >> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> >> When BAM is remotely controlled it does not sound correct to control >> its clk on Linux side. Make it optional, so that its not madatory >> for remote controlled BAM instances. >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> --- >> drivers/dma/qcom/bam_dma.c | 15 ++++++++------- >> 1 file changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c >> index 03c4eb3fd314..78e488e8f96d 100644 >> --- a/drivers/dma/qcom/bam_dma.c >> +++ b/drivers/dma/qcom/bam_dma.c >> @@ -1180,13 +1180,14 @@ static int bam_dma_probe(struct >> platform_device *pdev) >> "qcom,controlled-remotely"); >> bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk"); >> - if (IS_ERR(bdev->bamclk)) >> - return PTR_ERR(bdev->bamclk); >> - >> - ret = clk_prepare_enable(bdev->bamclk); >> - if (ret) { >> - dev_err(bdev->dev, "failed to prepare/enable clock\n"); >> - return ret; >> + if (IS_ERR(bdev->bamclk)) { >> + bdev->bamclk = NULL; >> + } else { >> + ret = clk_prepare_enable(bdev->bamclk); >> + if (ret) { >> + dev_err(bdev->dev, "failed to prepare/enable clock\n"); >> + return ret; >> + } > I believe you can also keep pm_runtime disabled if BAM is remotely > controlled. Yes, that's another topic which should be fixed too. I will add that patch in next version. thanks, srini > > Thanks > Sagar >> } >> ret = bam_init(bdev); > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 16, 2018 at 07:02:32PM +0000, srinivas.kandagatla@linaro.org wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > Hi Andy, > > I did hit few issues while trying out SLIMBus BAM on DB820c, this BAM instance > is remotely controlled and powered up after ADSP is booted using QMI commands. What do you mean by "remotely controlled" in this series? > > Firstly some of the master registers are written even when the BAM is remotely > controlled, and secondly reading registers when bam is not ready yet. > > These 4 patches address these issues, there are few more issues like doing PM > in simillar usecase, these will be addressed soon. > > Thanks, > Srini > > Srinivas Kandagatla (4): > dmaengine: qcom: bam_dma: make bam clk optional > dmaengine: qcom: bam_dma: add num-channels binding for remotely > controlled > dmaengine: qcom: bam_dma: do not write to global regs in remote mode > dmaengine: qcom: bam_dma: Add num-ees dt binding for remotely > controlled > > .../devicetree/bindings/dma/qcom_bam_dma.txt | 4 ++ > drivers/dma/qcom/bam_dma.c | 56 +++++++++++++++------- > 2 files changed, 43 insertions(+), 17 deletions(-) > > -- > 2.15.1 >
On 17/01/18 10:18, Vinod Koul wrote: > On Tue, Jan 16, 2018 at 07:02:32PM +0000, srinivas.kandagatla@linaro.org wrote: >> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> >> Hi Andy, >> >> I did hit few issues while trying out SLIMBus BAM on DB820c, this BAM instance >> is remotely controlled and powered up after ADSP is booted using QMI commands. > > What do you mean by "remotely controlled" in this series? DMA controller is controlled by the remote processor, which is a DSP in this case. Linux side is in remote mode in this setup, this can setup transfer descriptors and start/stop a transfer. But all the initialization/powerup part will be done in the remote processor. --srini > >> >> Firstly some of the master registers are written even when the BAM is remotely >> controlled, and secondly reading registers when bam is not ready yet. >> >> These 4 patches address these issues, there are few more issues like doing PM >> in simillar usecase, these will be addressed soon. >> >> Thanks, >> Srini >> >> Srinivas Kandagatla (4): >> dmaengine: qcom: bam_dma: make bam clk optional >> dmaengine: qcom: bam_dma: add num-channels binding for remotely >> controlled >> dmaengine: qcom: bam_dma: do not write to global regs in remote mode >> dmaengine: qcom: bam_dma: Add num-ees dt binding for remotely >> controlled >> >> .../devicetree/bindings/dma/qcom_bam_dma.txt | 4 ++ >> drivers/dma/qcom/bam_dma.c | 56 +++++++++++++++------- >> 2 files changed, 43 insertions(+), 17 deletions(-) >> >> -- >> 2.15.1 >> > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 17, 2018 at 10:55:34AM +0000, Srinivas Kandagatla wrote: > > > On 17/01/18 10:18, Vinod Koul wrote: > >On Tue, Jan 16, 2018 at 07:02:32PM +0000, srinivas.kandagatla@linaro.org wrote: > >>From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > >> > >>Hi Andy, > >> > >>I did hit few issues while trying out SLIMBus BAM on DB820c, this BAM instance > >>is remotely controlled and powered up after ADSP is booted using QMI commands. > > > >What do you mean by "remotely controlled" in this series? > > DMA controller is controlled by the remote processor, which is a DSP in this > case. > Linux side is in remote mode in this setup, this can setup transfer > descriptors and start/stop a transfer. But all the initialization/powerup > part will be done in the remote processor. Yeah that was my guess too. So in this case does linux see these controllers/channels, i would presume no...
On Tue, Jan 16, 2018 at 07:02:33PM +0000, srinivas.kandagatla@linaro.org wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > When BAM is remotely controlled it does not sound correct to control > its clk on Linux side. Make it optional, so that its not madatory s/madatory/mandatory > for remote controlled BAM instances. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > drivers/dma/qcom/bam_dma.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c > index 03c4eb3fd314..78e488e8f96d 100644 > --- a/drivers/dma/qcom/bam_dma.c > +++ b/drivers/dma/qcom/bam_dma.c > @@ -1180,13 +1180,14 @@ static int bam_dma_probe(struct platform_device *pdev) > "qcom,controlled-remotely"); > > bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk"); but you still do clk_get unconditionally? > - if (IS_ERR(bdev->bamclk)) > - return PTR_ERR(bdev->bamclk); > - > - ret = clk_prepare_enable(bdev->bamclk); > - if (ret) { > - dev_err(bdev->dev, "failed to prepare/enable clock\n"); > - return ret; > + if (IS_ERR(bdev->bamclk)) { > + bdev->bamclk = NULL; > + } else { > + ret = clk_prepare_enable(bdev->bamclk); > + if (ret) { > + dev_err(bdev->dev, "failed to prepare/enable clock\n"); > + return ret; > + } wouldn't it be better to set that an instance is remote controlled and thus not at all visible to Linux? > } > > ret = bam_init(bdev); > -- > 2.15.1 > > -- > To unsubscribe from this list: send the line "unsubscribe dmaengine" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19/01/18 05:52, Vinod Koul wrote: > On Tue, Jan 16, 2018 at 07:02:33PM +0000, srinivas.kandagatla@linaro.org wrote: >> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> >> When BAM is remotely controlled it does not sound correct to control >> its clk on Linux side. Make it optional, so that its not madatory > > s/madatory/mandatory > Yep, >> for remote controlled BAM instances. >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> --- >> drivers/dma/qcom/bam_dma.c | 15 ++++++++------- >> 1 file changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c >> index 03c4eb3fd314..78e488e8f96d 100644 >> --- a/drivers/dma/qcom/bam_dma.c >> +++ b/drivers/dma/qcom/bam_dma.c >> @@ -1180,13 +1180,14 @@ static int bam_dma_probe(struct platform_device *pdev) >> "qcom,controlled-remotely"); >> >> bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk"); > > but you still do clk_get unconditionally? Only reason to do this way is to not break existing users in the mainline. remotely controlled BAM is already supported in upstream driver, there are users of this who pass clk from device tree, If I make this conditional then subsequent reads to the BAM registers for those instances might crash the system. This sounds wrong to control clk from linux for the dma controller which is remotely controlled. These users should be transitioned to new bindings once the new bindings endup in the mainline. > >> - if (IS_ERR(bdev->bamclk)) >> - return PTR_ERR(bdev->bamclk); >> - >> - ret = clk_prepare_enable(bdev->bamclk); >> - if (ret) { >> - dev_err(bdev->dev, "failed to prepare/enable clock\n"); >> - return ret; >> + if (IS_ERR(bdev->bamclk)) { >> + bdev->bamclk = NULL; >> + } else { >> + ret = clk_prepare_enable(bdev->bamclk); >> + if (ret) { >> + dev_err(bdev->dev, "failed to prepare/enable clock\n"); >> + return ret; >> + } > > wouldn't it be better to set that an instance is remote controlled and thus > not at all visible to Linux? We already have a flag "controlled_remotely" for that in the driver. thanks, srini > >> } >> >> ret = bam_init(bdev); >> -- >> 2.15.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe dmaengine" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 22, 2018 at 09:55:01AM +0000, Srinivas Kandagatla wrote: > >>@@ -1180,13 +1180,14 @@ static int bam_dma_probe(struct platform_device *pdev) > >> "qcom,controlled-remotely"); > >> bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk"); > > > >but you still do clk_get unconditionally? > > Only reason to do this way is to not break existing users in the mainline. > > remotely controlled BAM is already supported in upstream driver, there are > users of this who pass clk from device tree, If I make this conditional then > subsequent reads to the BAM registers for those instances might crash the > system. But these instances are remote controlled, so if we stop representing them in Linux, why would we read them?
On 23/01/18 09:19, Vinod Koul wrote: > On Mon, Jan 22, 2018 at 09:55:01AM +0000, Srinivas Kandagatla wrote: > >>>> @@ -1180,13 +1180,14 @@ static int bam_dma_probe(struct platform_device *pdev) >>>> "qcom,controlled-remotely"); >>>> bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk"); >>> >>> but you still do clk_get unconditionally? >> >> Only reason to do this way is to not break existing users in the mainline. >> >> remotely controlled BAM is already supported in upstream driver, there are >> users of this who pass clk from device tree, If I make this conditional then >> subsequent reads to the BAM registers for those instances might crash the >> system. > > But these instances are remote controlled, so if we stop representing them > in Linux, why would we read them? Plan is that we would transition those users once we get these bindings/changes in. Currently I don't have access to any of those devices so I made the changes safe, such that it does not break devices on mainline. --srini > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Hi Andy, I did hit few issues while trying out SLIMBus BAM on DB820c, this BAM instance is remotely controlled and powered up after ADSP is booted using QMI commands. Firstly some of the master registers are written even when the BAM is remotely controlled, and secondly reading registers when bam is not ready yet. These 4 patches address these issues, there are few more issues like doing PM in simillar usecase, these will be addressed soon. Thanks, Srini Srinivas Kandagatla (4): dmaengine: qcom: bam_dma: make bam clk optional dmaengine: qcom: bam_dma: add num-channels binding for remotely controlled dmaengine: qcom: bam_dma: do not write to global regs in remote mode dmaengine: qcom: bam_dma: Add num-ees dt binding for remotely controlled .../devicetree/bindings/dma/qcom_bam_dma.txt | 4 ++ drivers/dma/qcom/bam_dma.c | 56 +++++++++++++++------- 2 files changed, 43 insertions(+), 17 deletions(-)