The Renesas RZ/N1 SPI Controller is based on the Synopsys DW SSI. This
series enables power mode in the driver so the clock domain will enable
the bus clock, adds the compatible string and updates the associated bindings
documentation.
Phil Edworthy (3):
dt: spi: Add Renesas RZ/N1 binding documentation
spi: dw: Add basic runtime PM support
spi: dw: Add compatible string for Renesas RZ/N1 SPI Controller
Documentation/devicetree/bindings/spi/renesas,rzn1-spi.txt | 11 +++++++++++
drivers/spi/spi-dw-mmio.c | 1 +
drivers/spi/spi-dw.c | 4 ++++
3 files changed, 16 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/renesas,rzn1-spi.txt
--
2.7.4
From: Phil Edworthy <[email protected]>
The Renesas RZ/N1 SPI Controller is based on the Synopsys DW SSI, but has
additional registers for software CS control and DMA. This patch does not
address the changes required for DMA support, it simply adds the compatible
string. The CS registers are not needed as Linux can use gpios for the CS
signals.
Signed-off-by: Gareth Williams <[email protected]>
Signed-off-by: Phil Edworthy <[email protected]>
---
drivers/spi/spi-dw-mmio.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index edb3cf6..3640b01 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -225,6 +225,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
{ .compatible = "mscc,ocelot-spi", .data = dw_spi_mscc_ocelot_init},
{ .compatible = "mscc,jaguar2-spi", .data = dw_spi_mscc_jaguar2_init},
{ .compatible = "amazon,alpine-dw-apb-ssi", .data = dw_spi_alpine_init},
+ { .compatible = "renesas,rzn1-spi", },
{ /* end of table */}
};
MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
--
2.7.4
From: Phil Edworthy <[email protected]>
The Renesas RZ/N1 SPI Controller is based on the Synopsys DW SSI, but has
additional registers for software CS control and DMA. This patch does not
address the changes required for DMA support, it simply adds the compatible
string. The CS functionality is not very useful and also not needed as
Linux can use gpios for the CS signals.
Add a compatible string to handle any unforeseen issues that may arise, and
pave the way for DMA support.
Signed-off-by: Gareth Williams <[email protected]>
Signed-off-by: Phil Edworthy <[email protected]>
---
Note: All the other manufacturers detail their compatible strings in
snps,dw-apb-ssi.txt. I think it makes sense for rzn1 to be in it's own file
due to the changes made to the peripheral for DMA support.
---
Documentation/devicetree/bindings/spi/renesas,rzn1-spi.txt | 11 +++++++++++
1 file changed, 11 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/renesas,rzn1-spi.txt
diff --git a/Documentation/devicetree/bindings/spi/renesas,rzn1-spi.txt b/Documentation/devicetree/bindings/spi/renesas,rzn1-spi.txt
new file mode 100644
index 0000000..fb1a672
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/renesas,rzn1-spi.txt
@@ -0,0 +1,11 @@
+Renesas RZ/N1 SPI Controller
+
+This controller is based on the Synopsys DW Synchronous Serial Interface and
+inherits all properties defined in snps,dw-apb-ssi.txt except for the
+compatible property.
+
+Required properties:
+- compatible : The device specific string followed by the generic RZ/N1 string.
+ Therefore it must be one of:
+ "renesas,r9a06g032-spi", "renesas,rzn1-spi"
+ "renesas,r9a06g033-spi", "renesas,rzn1-spi"
--
2.7.4
From: Phil Edworthy <[email protected]>
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 <[email protected]>
Signed-off-by: Gareth Williams <[email protected]>
---
drivers/spi/spi-dw.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index 9a49e07..593bbb0 100644
--- 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);
+
/* Basic HW init */
spi_hw_init(dev, dws);
--
2.7.4
On Fri, Sep 13, 2019 at 3:14 PM Gareth Williams
<[email protected]> wrote:
>
> From: Phil Edworthy <[email protected]>
>
> The Renesas RZ/N1 SPI Controller is based on the Synopsys DW SSI, but has
> additional registers for software CS control and DMA. This patch does not
> address the changes required for DMA support, it simply adds the compatible
> string. The CS registers are not needed as Linux can use gpios for the CS
> signals.
> + { .compatible = "renesas,rzn1-spi", },
Can't you simple use in DT something like
compatible = "renesas,rzn1-spi", "snps,dw-apb-ssi"
?
--
With Best Regards,
Andy Shevchenko
On Sun, Sep 15, 2019 at 02:00:33PM +0300, Andy Shevchenko wrote:
> On Fri, Sep 13, 2019 at 3:14 PM Gareth Williams
> > The Renesas RZ/N1 SPI Controller is based on the Synopsys DW SSI, but has
> > additional registers for software CS control and DMA. This patch does not
> > address the changes required for DMA support, it simply adds the compatible
> > string. The CS registers are not needed as Linux can use gpios for the CS
> > signals.
> > + { .compatible = "renesas,rzn1-spi", },
> Can't you simple use in DT something like
> compatible = "renesas,rzn1-spi", "snps,dw-apb-ssi"
> ?
Yes, you can and should do that but it's still nice to list the
compatibles explicitly in the driver in case someone leaves out the
fallback compatible for whatever reason - if both the driver and the DT
list things then there's a bit more robustness.
Hi Gareth,
On Fri, Sep 13, 2019 at 2:13 PM Gareth Williams
<[email protected]> wrote:
> From: Phil Edworthy <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Gareth Williams <[email protected]>
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?
> +
> /* 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 assume this will be called from drivers/spi/spi-dw-mmio.c, which already
enables the clock explicitly all the timer?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
Hi Geert,
I appreciate the feedback.
> On Mon, Sep 16, 2019 at 15:36 PM Geert Uytterhoeven
> <[email protected]> wrote:
> Hi Gareth,
>
> On Fri, Sep 13, 2019 at 2:13 PM Gareth Williams
> <[email protected]> wrote:
> > From: Phil Edworthy <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Gareth Williams <[email protected]>
>
> 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@linux-
> 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
Hi Geert,
> On Mon, Sep 17, 2019 at 07:36 PM Geert Uytterhoeven
> <[email protected]> wrote:
>
> Hi Gareth,
>
> On Mon, Sep 16, 2019 at 6:14 PM Gareth Williams
> <[email protected]> wrote:
> > > On Mon, Sep 16, 2019 at 15:36 PM Geert Uytterhoeven
> > > <[email protected]> wrote:
> > > On Fri, Sep 13, 2019 at 2:13 PM Gareth Williams
> > > <[email protected]> wrote:
> > > > From: Phil Edworthy <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > Signed-off-by: Gareth Williams <[email protected]>
> > >
> > > 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.
>
> > > 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.
>
> IC, that's useful sideband information.
>
> "pclk" is indeed an optional clock.
> "ssi_clk" must be first.
>
> However, to make use of the clock domain code, you still have to list "pclk"
> in DT, but use a different name, to avoid spi-dw-mmio.c enabling it all the
> time? Or do you plan to modify spi-dw-mmio.c for that?
> In the former case, you should document that in your bindings, which
> currently build on top of snps,dw-apb-ssi.txt, thus include "pclk".
We are intending to do the former, so I will include a binding update in V2 that
notes to rename "pclk" in the case a clock domain is in use. Thanks for pointing
this out.
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> 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
Hi Gareth,
On Mon, Sep 16, 2019 at 6:14 PM Gareth Williams
<[email protected]> wrote:
> > On Mon, Sep 16, 2019 at 15:36 PM Geert Uytterhoeven
> > <[email protected]> wrote:
> > On Fri, Sep 13, 2019 at 2:13 PM Gareth Williams
> > <[email protected]> wrote:
> > > From: Phil Edworthy <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Gareth Williams <[email protected]>
> >
> > 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.
> > 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.
IC, that's useful sideband information.
"pclk" is indeed an optional clock.
"ssi_clk" must be first.
However, to make use of the clock domain code, you still have to list "pclk"
in DT, but use a different name, to avoid spi-dw-mmio.c enabling it all the
time? Or do you plan to modify spi-dw-mmio.c for that?
In the former case, you should document that in your bindings, which
currently build on top of snps,dw-apb-ssi.txt, thus include "pclk".
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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