* [PATCH 1/2] dmaengine: sprd: delete enable opreation in probe
@ 2023-11-02 12:16 Kaiwei Liu
2023-11-24 13:11 ` Vinod Koul
0 siblings, 1 reply; 6+ messages in thread
From: Kaiwei Liu @ 2023-11-02 12:16 UTC (permalink / raw)
To: Vinod Koul, Orson Zhai, Baolin Wang, Chunyan Zhang
Cc: dmaengine, linux-kernel, kaiwei liu, Wenming Wu
From: "kaiwei.liu" <kaiwei.liu@unisoc.com>
In the probe of dma, it will allocate device memory and do some
initalization settings. All operations are only at the software
level and don't need the DMA hardware power on. It doesn't need
to resume the device and set the device active as well. here
delete unnecessary operation.
Signed-off-by: kaiwei.liu <kaiwei.liu@unisoc.com>
---
drivers/dma/sprd-dma.c | 19 ++-----------------
1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 08fcf1ec368c..8ab5a9082fc5 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -1203,21 +1203,11 @@ static int sprd_dma_probe(struct platform_device *pdev)
}
platform_set_drvdata(pdev, sdev);
- ret = sprd_dma_enable(sdev);
- if (ret)
- return ret;
-
- pm_runtime_set_active(&pdev->dev);
- pm_runtime_enable(&pdev->dev);
-
- ret = pm_runtime_get_sync(&pdev->dev);
- if (ret < 0)
- goto err_rpm;
ret = dma_async_device_register(&sdev->dma_dev);
if (ret < 0) {
dev_err(&pdev->dev, "register dma device failed:%d\n", ret);
- goto err_register;
+ return ret;
}
sprd_dma_info.dma_cap = sdev->dma_dev.cap_mask;
@@ -1226,16 +1216,11 @@ static int sprd_dma_probe(struct platform_device *pdev)
if (ret)
goto err_of_register;
- pm_runtime_put(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
return 0;
err_of_register:
dma_async_device_unregister(&sdev->dma_dev);
-err_register:
- pm_runtime_put_noidle(&pdev->dev);
- pm_runtime_disable(&pdev->dev);
-err_rpm:
- sprd_dma_disable(sdev);
return ret;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] dmaengine: sprd: delete enable opreation in probe
2023-11-02 12:16 [PATCH 1/2] dmaengine: sprd: delete enable opreation in probe Kaiwei Liu
@ 2023-11-24 13:11 ` Vinod Koul
2023-12-06 9:32 ` liu kaiwei
0 siblings, 1 reply; 6+ messages in thread
From: Vinod Koul @ 2023-11-24 13:11 UTC (permalink / raw)
To: Kaiwei Liu
Cc: Orson Zhai, Baolin Wang, Chunyan Zhang, dmaengine, linux-kernel,
kaiwei liu, Wenming Wu
On 02-11-23, 20:16, Kaiwei Liu wrote:
> From: "kaiwei.liu" <kaiwei.liu@unisoc.com>
Typo is subject line
>
> In the probe of dma, it will allocate device memory and do some
> initalization settings. All operations are only at the software
> level and don't need the DMA hardware power on. It doesn't need
> to resume the device and set the device active as well. here
> delete unnecessary operation.
Don't you need to read or write to the device? Without enable that wont
work right?
Lastly patches appear disjoint, pls thread them properly
>
> Signed-off-by: kaiwei.liu <kaiwei.liu@unisoc.com>
> ---
> drivers/dma/sprd-dma.c | 19 ++-----------------
> 1 file changed, 2 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 08fcf1ec368c..8ab5a9082fc5 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -1203,21 +1203,11 @@ static int sprd_dma_probe(struct platform_device *pdev)
> }
>
> platform_set_drvdata(pdev, sdev);
> - ret = sprd_dma_enable(sdev);
> - if (ret)
> - return ret;
> -
> - pm_runtime_set_active(&pdev->dev);
> - pm_runtime_enable(&pdev->dev);
> -
> - ret = pm_runtime_get_sync(&pdev->dev);
> - if (ret < 0)
> - goto err_rpm;
>
> ret = dma_async_device_register(&sdev->dma_dev);
> if (ret < 0) {
> dev_err(&pdev->dev, "register dma device failed:%d\n", ret);
> - goto err_register;
> + return ret;
> }
>
> sprd_dma_info.dma_cap = sdev->dma_dev.cap_mask;
> @@ -1226,16 +1216,11 @@ static int sprd_dma_probe(struct platform_device *pdev)
> if (ret)
> goto err_of_register;
>
> - pm_runtime_put(&pdev->dev);
> + pm_runtime_enable(&pdev->dev);
> return 0;
>
> err_of_register:
> dma_async_device_unregister(&sdev->dma_dev);
> -err_register:
> - pm_runtime_put_noidle(&pdev->dev);
> - pm_runtime_disable(&pdev->dev);
> -err_rpm:
> - sprd_dma_disable(sdev);
> return ret;
> }
>
> --
> 2.17.1
--
~Vinod
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] dmaengine: sprd: delete enable opreation in probe
2023-11-24 13:11 ` Vinod Koul
@ 2023-12-06 9:32 ` liu kaiwei
2023-12-11 11:40 ` Vinod Koul
0 siblings, 1 reply; 6+ messages in thread
From: liu kaiwei @ 2023-12-06 9:32 UTC (permalink / raw)
To: Vinod Koul
Cc: Kaiwei Liu, Orson Zhai, Baolin Wang, Chunyan Zhang, dmaengine,
linux-kernel, Wenming Wu
On Fri, Nov 24, 2023 at 9:11 PM Vinod Koul <vkoul@kernel.org> wrote:
>
> On 02-11-23, 20:16, Kaiwei Liu wrote:
> > From: "kaiwei.liu" <kaiwei.liu@unisoc.com>
>
> Typo is subject line
>
> >
> > In the probe of dma, it will allocate device memory and do some
> > initalization settings. All operations are only at the software
> > level and don't need the DMA hardware power on. It doesn't need
> > to resume the device and set the device active as well. here
> > delete unnecessary operation.
>
> Don't you need to read or write to the device? Without enable that wont
> work right?
>
Yes, it doesn't need to read or write to the device in the probe of DMA.
We will enable the DMA when allocating the DMA channel.
> Lastly patches appear disjoint, pls thread them properly
>
> >
> > Signed-off-by: kaiwei.liu <kaiwei.liu@unisoc.com>
> > ---
> > drivers/dma/sprd-dma.c | 19 ++-----------------
> > 1 file changed, 2 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > index 08fcf1ec368c..8ab5a9082fc5 100644
> > --- a/drivers/dma/sprd-dma.c
> > +++ b/drivers/dma/sprd-dma.c
> > @@ -1203,21 +1203,11 @@ static int sprd_dma_probe(struct platform_device *pdev)
> > }
> >
> > platform_set_drvdata(pdev, sdev);
> > - ret = sprd_dma_enable(sdev);
> > - if (ret)
> > - return ret;
> > -
> > - pm_runtime_set_active(&pdev->dev);
> > - pm_runtime_enable(&pdev->dev);
> > -
> > - ret = pm_runtime_get_sync(&pdev->dev);
> > - if (ret < 0)
> > - goto err_rpm;
> >
> > ret = dma_async_device_register(&sdev->dma_dev);
> > if (ret < 0) {
> > dev_err(&pdev->dev, "register dma device failed:%d\n", ret);
> > - goto err_register;
> > + return ret;
> > }
> >
> > sprd_dma_info.dma_cap = sdev->dma_dev.cap_mask;
> > @@ -1226,16 +1216,11 @@ static int sprd_dma_probe(struct platform_device *pdev)
> > if (ret)
> > goto err_of_register;
> >
> > - pm_runtime_put(&pdev->dev);
> > + pm_runtime_enable(&pdev->dev);
> > return 0;
> >
> > err_of_register:
> > dma_async_device_unregister(&sdev->dma_dev);
> > -err_register:
> > - pm_runtime_put_noidle(&pdev->dev);
> > - pm_runtime_disable(&pdev->dev);
> > -err_rpm:
> > - sprd_dma_disable(sdev);
> > return ret;
> > }
> >
> > --
> > 2.17.1
>
> --
> ~Vinod
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] dmaengine: sprd: delete enable opreation in probe
2023-12-06 9:32 ` liu kaiwei
@ 2023-12-11 11:40 ` Vinod Koul
2023-12-19 5:21 ` liu kaiwei
0 siblings, 1 reply; 6+ messages in thread
From: Vinod Koul @ 2023-12-11 11:40 UTC (permalink / raw)
To: liu kaiwei
Cc: Kaiwei Liu, Orson Zhai, Baolin Wang, Chunyan Zhang, dmaengine,
linux-kernel, Wenming Wu
On 06-12-23, 17:32, liu kaiwei wrote:
> On Fri, Nov 24, 2023 at 9:11 PM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 02-11-23, 20:16, Kaiwei Liu wrote:
> > > From: "kaiwei.liu" <kaiwei.liu@unisoc.com>
> >
> > Typo is subject line
> >
> > >
> > > In the probe of dma, it will allocate device memory and do some
> > > initalization settings. All operations are only at the software
> > > level and don't need the DMA hardware power on. It doesn't need
> > > to resume the device and set the device active as well. here
> > > delete unnecessary operation.
> >
> > Don't you need to read or write to the device? Without enable that wont
> > work right?
> >
>
> Yes, it doesn't need to read or write to the device in the probe of DMA.
> We will enable the DMA when allocating the DMA channel.
So you will probe even if device is not present! I think it makes sense
to access device registers in probe!
--
~Vinod
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] dmaengine: sprd: delete enable opreation in probe
2023-12-11 11:40 ` Vinod Koul
@ 2023-12-19 5:21 ` liu kaiwei
2023-12-19 7:00 ` Baolin Wang
0 siblings, 1 reply; 6+ messages in thread
From: liu kaiwei @ 2023-12-19 5:21 UTC (permalink / raw)
To: Vinod Koul
Cc: Kaiwei Liu, Orson Zhai, Baolin Wang, Chunyan Zhang, dmaengine,
linux-kernel, Wenming Wu
On Mon, Dec 11, 2023 at 7:41 PM Vinod Koul <vkoul@kernel.org> wrote:
>
> On 06-12-23, 17:32, liu kaiwei wrote:
> > On Fri, Nov 24, 2023 at 9:11 PM Vinod Koul <vkoul@kernel.org> wrote:
> > >
> > > On 02-11-23, 20:16, Kaiwei Liu wrote:
> > > > From: "kaiwei.liu" <kaiwei.liu@unisoc.com>
> > >
> > > Typo is subject line
> > >
> > > >
> > > > In the probe of dma, it will allocate device memory and do some
> > > > initalization settings. All operations are only at the software
> > > > level and don't need the DMA hardware power on. It doesn't need
> > > > to resume the device and set the device active as well. here
> > > > delete unnecessary operation.
> > >
> > > Don't you need to read or write to the device? Without enable that wont
> > > work right?
> > >
> >
> > Yes, it doesn't need to read or write to the device in the probe of DMA.
> > We will enable the DMA when allocating the DMA channel.
>
> So you will probe even if device is not present! I think it makes sense
> to access device registers in probe!
There is another reason why we delete enable/disable and not to access
device in probe. The current driver is applicable to two DMA devices
in different
power domain. For some scenes, one of the domain is power off and when you
probe, enable the dma with the domain power off may cause crash.
For example, one case is for audio co-processor and DMA serves for it,
DMA's power domain is off during initialization since audio is not used
at that time, so we cannot read/write DMA's register for this kind of cases.
@Baolin Wang
Hi baolin，what's your opinion?
> --
> ~Vinod
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] dmaengine: sprd: delete enable opreation in probe
2023-12-19 5:21 ` liu kaiwei
@ 2023-12-19 7:00 ` Baolin Wang
0 siblings, 0 replies; 6+ messages in thread
From: Baolin Wang @ 2023-12-19 7:00 UTC (permalink / raw)
To: liu kaiwei, Vinod Koul
Cc: Kaiwei Liu, Orson Zhai, Chunyan Zhang, dmaengine, linux-kernel,
Wenming Wu
On 12/19/2023 1:21 PM, liu kaiwei wrote:
> On Mon, Dec 11, 2023 at 7:41 PM Vinod Koul <vkoul@kernel.org> wrote:
>>
>> On 06-12-23, 17:32, liu kaiwei wrote:
>>> On Fri, Nov 24, 2023 at 9:11 PM Vinod Koul <vkoul@kernel.org> wrote:
>>>>
>>>> On 02-11-23, 20:16, Kaiwei Liu wrote:
>>>>> From: "kaiwei.liu" <kaiwei.liu@unisoc.com>
>>>>
>>>> Typo is subject line
>>>>
>>>>>
>>>>> In the probe of dma, it will allocate device memory and do some
>>>>> initalization settings. All operations are only at the software
>>>>> level and don't need the DMA hardware power on. It doesn't need
>>>>> to resume the device and set the device active as well. here
>>>>> delete unnecessary operation.
>>>>
>>>> Don't you need to read or write to the device? Without enable that wont
>>>> work right?
>>>>
>>>
>>> Yes, it doesn't need to read or write to the device in the probe of DMA.
>>> We will enable the DMA when allocating the DMA channel.
>>
>> So you will probe even if device is not present! I think it makes sense
>> to access device registers in probe!
>
> There is another reason why we delete enable/disable and not to access
> device in probe. The current driver is applicable to two DMA devices
> in different
> power domain. For some scenes, one of the domain is power off and when you
> probe, enable the dma with the domain power off may cause crash.
>
> For example, one case is for audio co-processor and DMA serves for it,
> DMA's power domain is off during initialization since audio is not used
> at that time, so we cannot read/write DMA's register for this kind of cases.
>
> @Baolin Wang
> Hi baolin，what's your opinion?
Please add your explanation into the commit message.
Moreover, I think Vinod's concern is reasonable, so you can not move the
pm_runtime_enable() after registering the devices, which means users can
access the device without powering on.
To solve your problem, I think you can move the pm_runtime_enable()
before dma_async_device_register(), then if users want to use DMA in
probe stage, the dma_chan_get()--->sprd_dma_alloc_chan_resources() will
help to power on it.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-19 7:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-02 12:16 [PATCH 1/2] dmaengine: sprd: delete enable opreation in probe Kaiwei Liu
2023-11-24 13:11 ` Vinod Koul
2023-12-06 9:32 ` liu kaiwei
2023-12-11 11:40 ` Vinod Koul
2023-12-19 5:21 ` liu kaiwei
2023-12-19 7:00 ` Baolin Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox