linux-kernel - RE: [PATCH 2/3] spi: dw: Add basic runtime PM support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <TY2PR01MB2924AA1B012C2D305EE5C9B7DF8C0@TY2PR01MB2924.jpnprd01.prod.outlook.com>
Date:   Mon, 16 Sep 2019 16:14:13 +0000
From:   Gareth Williams <gareth.williams.jx@...esas.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
CC:     Mark Brown <broonie@...nel.org>,
        Phil Edworthy <phil.edworthy@...esas.com>,
        linux-spi <linux-spi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 2/3] spi: dw: Add basic runtime PM support

Hi Geert,

I appreciate the feedback.

> On Mon, Sep 16, 2019 at 15:36 PM Geert Uytterhoeven
> <geert@...ux-m68k.org> wrote:
> Hi Gareth,
> 
> On Fri, Sep 13, 2019 at 2:13 PM Gareth Williams
> <gareth.williams.jx@...esas.com> wrote:
> > From: Phil Edworthy <phil.edworthy@...esas.com>
> >
> > Enable runtime PM so that the clock used to access the registers in
> > the peripheral is turned on using a clock domain.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@...esas.com>
> > Signed-off-by: Gareth Williams <gareth.williams.jx@...esas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/spi/spi-dw.c
> > +++ b/drivers/spi/spi-dw.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/module.h>
> >  #include <linux/highmem.h>
> >  #include <linux/delay.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/slab.h>
> >  #include <linux/spi/spi.h>
> >
> > @@ -497,6 +498,9 @@ int dw_spi_add_host(struct device *dev, struct
> dw_spi *dws)
> >         if (dws->set_cs)
> >                 master->set_cs = dws->set_cs;
> >
> > +       pm_runtime_enable(dev);
> > +       pm_runtime_get_sync(dev);
> 
> The second line keeps the device powered all the time.
> What about setting spi_controller.auto_runtime_pm = true, so the SPI code
> can manage its Runtime PM status?
That makes sense and works on target, I will change this for V2.

> 
> > +
> >         /* Basic HW init */
> >         spi_hw_init(dev, dws);
> >
> 
> What about the error path?
> Don't you need to disable Runtime PM again in dw_spi_remove_host()?
I will add a call to disable pm in dw_spi_remove_host() and the err path in
dw_spi_add_host for v2. 

> 
> I assume this will be called from drivers/spi/spi-dw-mmio.c, which already
> enables the clock explicitly all the timer?
Yes, spi-dw-mmio.c already enables the bus clock, however we want to use clock 
domain to enable the clock and not explicitly provide pclk in the dts. If there are 
no other uses of that pclk, we can remove that later on. 

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds


Kind Regards,

Gareth