| Message ID | 20241025103621.4780-3-wahrenst@gmx.net |
|---|---|
| State | New |
| Headers | show |
| Series | ARM: bcm2835: Implement initial S2Idle for Raspberry Pi | expand |
Am 25.10.24 um 12:36 schrieb Stefan Wahren: > bcm2835-dma provides the service to others, so it should > suspend late and resume early. > > Signed-off-by: Stefan Wahren <wahrenst@gmx.net> Since there wasn't any feedback for this patch, i want to send a gentle ping ... Regards > --- > drivers/dma/bcm2835-dma.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c > index e1b92b4d7b05..647dda9f3376 100644 > --- a/drivers/dma/bcm2835-dma.c > +++ b/drivers/dma/bcm2835-dma.c > @@ -875,6 +875,35 @@ static struct dma_chan *bcm2835_dma_xlate(struct of_phandle_args *spec, > return chan; > } > > +static int bcm2835_dma_suspend_late(struct device *dev) > +{ > + struct bcm2835_dmadev *od = dev_get_drvdata(dev); > + struct bcm2835_chan *c, *next; > + > + list_for_each_entry_safe(c, next, &od->ddev.channels, > + vc.chan.device_node) { > + void __iomem *chan_base = c->chan_base; > + > + if (readl(chan_base + BCM2835_DMA_ADDR)) { > + dev_warn(dev, "Suspend is prevented by chan %d\n", > + c->ch); > + return -EBUSY; > + } > + } > + > + return 0; > +} > + > +static int bcm2835_dma_resume_early(struct device *dev) > +{ > + return 0; > +} > + > +static const struct dev_pm_ops bcm2835_dma_pm_ops = { > + SET_LATE_SYSTEM_SLEEP_PM_OPS(bcm2835_dma_suspend_late, > + bcm2835_dma_resume_early) > +}; > + > static int bcm2835_dma_probe(struct platform_device *pdev) > { > struct bcm2835_dmadev *od; > @@ -1033,6 +1062,7 @@ static struct platform_driver bcm2835_dma_driver = { > .driver = { > .name = "bcm2835-dma", > .of_match_table = of_match_ptr(bcm2835_dma_of_match), > + .pm = pm_ptr(&bcm2835_dma_pm_ops), > }, > }; > > -- > 2.34.1 >
On 25-10-24, 12:36, Stefan Wahren wrote: > bcm2835-dma provides the service to others, so it should > suspend late and resume early. > > Signed-off-by: Stefan Wahren <wahrenst@gmx.net> > --- > drivers/dma/bcm2835-dma.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c > index e1b92b4d7b05..647dda9f3376 100644 > --- a/drivers/dma/bcm2835-dma.c > +++ b/drivers/dma/bcm2835-dma.c > @@ -875,6 +875,35 @@ static struct dma_chan *bcm2835_dma_xlate(struct of_phandle_args *spec, > return chan; > } > > +static int bcm2835_dma_suspend_late(struct device *dev) > +{ > + struct bcm2835_dmadev *od = dev_get_drvdata(dev); > + struct bcm2835_chan *c, *next; > + > + list_for_each_entry_safe(c, next, &od->ddev.channels, > + vc.chan.device_node) { > + void __iomem *chan_base = c->chan_base; > + > + if (readl(chan_base + BCM2835_DMA_ADDR)) { > + dev_warn(dev, "Suspend is prevented by chan %d\n", > + c->ch); > + return -EBUSY; > + } Can you help understand how this helps by logging... we are not adding anything except checking this and resume is NOP as well! > + } > + > + return 0; > +} > + > +static int bcm2835_dma_resume_early(struct device *dev) > +{ > + return 0; > +} > + > +static const struct dev_pm_ops bcm2835_dma_pm_ops = { > + SET_LATE_SYSTEM_SLEEP_PM_OPS(bcm2835_dma_suspend_late, > + bcm2835_dma_resume_early) > +}; > + > static int bcm2835_dma_probe(struct platform_device *pdev) > { > struct bcm2835_dmadev *od; > @@ -1033,6 +1062,7 @@ static struct platform_driver bcm2835_dma_driver = { > .driver = { > .name = "bcm2835-dma", > .of_match_table = of_match_ptr(bcm2835_dma_of_match), > + .pm = pm_ptr(&bcm2835_dma_pm_ops), > }, > }; > > -- > 2.34.1
Hi Vinod, Am 02.12.24 um 17:52 schrieb Vinod Koul: > On 25-10-24, 12:36, Stefan Wahren wrote: >> bcm2835-dma provides the service to others, so it should >> suspend late and resume early. >> >> Signed-off-by: Stefan Wahren <wahrenst@gmx.net> >> --- >> drivers/dma/bcm2835-dma.c | 30 ++++++++++++++++++++++++++++++ >> 1 file changed, 30 insertions(+) >> >> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c >> index e1b92b4d7b05..647dda9f3376 100644 >> --- a/drivers/dma/bcm2835-dma.c >> +++ b/drivers/dma/bcm2835-dma.c >> @@ -875,6 +875,35 @@ static struct dma_chan *bcm2835_dma_xlate(struct of_phandle_args *spec, >> return chan; >> } >> >> +static int bcm2835_dma_suspend_late(struct device *dev) >> +{ >> + struct bcm2835_dmadev *od = dev_get_drvdata(dev); >> + struct bcm2835_chan *c, *next; >> + >> + list_for_each_entry_safe(c, next, &od->ddev.channels, >> + vc.chan.device_node) { >> + void __iomem *chan_base = c->chan_base; >> + >> + if (readl(chan_base + BCM2835_DMA_ADDR)) { >> + dev_warn(dev, "Suspend is prevented by chan %d\n", >> + c->ch); >> + return -EBUSY; >> + } > Can you help understand how this helps by logging... we are not adding > anything except checking this and resume is NOP as well! My intention of this patch is just to make sure, that no DMA transfer is in progress during late_suspend. So i followed the implementation of fsldma.c Additionally i added this warning mostly to know if this ever occurs. But i wasn't able to trigger. Should i drop the warning and make resume callback = NULL? Best regards
On 02-12-24, 19:51, Stefan Wahren wrote: > Hi Vinod, > > Am 02.12.24 um 17:52 schrieb Vinod Koul: > > On 25-10-24, 12:36, Stefan Wahren wrote: > > > bcm2835-dma provides the service to others, so it should > > > suspend late and resume early. > > > > > > Signed-off-by: Stefan Wahren <wahrenst@gmx.net> > > > --- > > > drivers/dma/bcm2835-dma.c | 30 ++++++++++++++++++++++++++++++ > > > 1 file changed, 30 insertions(+) > > > > > > diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c > > > index e1b92b4d7b05..647dda9f3376 100644 > > > --- a/drivers/dma/bcm2835-dma.c > > > +++ b/drivers/dma/bcm2835-dma.c > > > @@ -875,6 +875,35 @@ static struct dma_chan *bcm2835_dma_xlate(struct of_phandle_args *spec, > > > return chan; > > > } > > > > > > +static int bcm2835_dma_suspend_late(struct device *dev) > > > +{ > > > + struct bcm2835_dmadev *od = dev_get_drvdata(dev); > > > + struct bcm2835_chan *c, *next; > > > + > > > + list_for_each_entry_safe(c, next, &od->ddev.channels, > > > + vc.chan.device_node) { > > > + void __iomem *chan_base = c->chan_base; > > > + > > > + if (readl(chan_base + BCM2835_DMA_ADDR)) { > > > + dev_warn(dev, "Suspend is prevented by chan %d\n", > > > + c->ch); > > > + return -EBUSY; > > > + } > > Can you help understand how this helps by logging... we are not adding > > anything except checking this and resume is NOP as well! > My intention of this patch is just to make sure, that no DMA transfer is > in progress during late_suspend. So i followed the implementation of > fsldma.c > > Additionally i added this warning mostly to know if this ever occurs. > But i wasn't able to trigger. Okay in the case I dont think it is a abd idea. But the patch description should mention that add a warning while suspending if channels are active or something like that. Patch title and description should mention this.. > > Should i drop the warning and make resume callback = NULL? Yes that would be better as well, no point in having dummy code
diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c index e1b92b4d7b05..647dda9f3376 100644 --- a/drivers/dma/bcm2835-dma.c +++ b/drivers/dma/bcm2835-dma.c @@ -875,6 +875,35 @@ static struct dma_chan *bcm2835_dma_xlate(struct of_phandle_args *spec, return chan; } +static int bcm2835_dma_suspend_late(struct device *dev) +{ + struct bcm2835_dmadev *od = dev_get_drvdata(dev); + struct bcm2835_chan *c, *next; + + list_for_each_entry_safe(c, next, &od->ddev.channels, + vc.chan.device_node) { + void __iomem *chan_base = c->chan_base; + + if (readl(chan_base + BCM2835_DMA_ADDR)) { + dev_warn(dev, "Suspend is prevented by chan %d\n", + c->ch); + return -EBUSY; + } + } + + return 0; +} + +static int bcm2835_dma_resume_early(struct device *dev) +{ + return 0; +} + +static const struct dev_pm_ops bcm2835_dma_pm_ops = { + SET_LATE_SYSTEM_SLEEP_PM_OPS(bcm2835_dma_suspend_late, + bcm2835_dma_resume_early) +}; + static int bcm2835_dma_probe(struct platform_device *pdev) { struct bcm2835_dmadev *od; @@ -1033,6 +1062,7 @@ static struct platform_driver bcm2835_dma_driver = { .driver = { .name = "bcm2835-dma", .of_match_table = of_match_ptr(bcm2835_dma_of_match), + .pm = pm_ptr(&bcm2835_dma_pm_ops), }, };
bcm2835-dma provides the service to others, so it should suspend late and resume early. Signed-off-by: Stefan Wahren <wahrenst@gmx.net> --- drivers/dma/bcm2835-dma.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) -- 2.34.1