| Message ID | 20230926210818.197356-2-fabrizio.castro.jz@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add RZ/V2M CSI slave support | expand |
Hi Fabrizio, On Tue, Sep 26, 2023 at 11:08 PM Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote: > The CSI IP found inside the Renesas RZ/V2M SoC can also work > in SPI slave mode. > When working in slave mode, the IP can make use of the SS > (Slave Select) pin, with "low" as default active level. > The active level of SS can be changed to "high" upon configuration. > This patch adds two new properties, one to make use of the > SS pin when in slave mode, and one to make the SS pin active high. > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> Thanks for your patch! > --- a/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml > +++ b/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml > @@ -39,6 +39,17 @@ properties: > power-domains: > maxItems: 1 > > + renesas,csi-ss: > + type: boolean > + description: > + Use CSI Slave Selection (SS) pin to enable transmission and reception when > + in slave mode. Can't this be done in a more generic way? I had expected that the existing SPI_NO_CS flag can be set using a property in the "slave" subnode, but apparently there is no "spi-no-cs" property defined yet. > + > + renesas,csi-ss-high: > + type: boolean > + description: > + The SS pin is active high (by default the SS pin is active low). Can't you use the "spi-cs-high" property in the "slave" subnode instead? > + > required: > - compatible > - reg Gr{oetje,eeting}s, Geert
On Tue, Sep 26, 2023 at 10:08:17PM +0100, Fabrizio Castro wrote: > The CSI IP found inside the Renesas RZ/V2M SoC can also work > in SPI slave mode. > When working in slave mode, the IP can make use of the SS > (Slave Select) pin, with "low" as default active level. > The active level of SS can be changed to "high" upon configuration. > This patch adds two new properties, one to make use of the > SS pin when in slave mode, and one to make the SS pin active high. Please avoid the use of outdated terminology like this, prefer "device mode" or similar. > + renesas,csi-ss: > + type: boolean > + description: > + Use CSI Slave Selection (SS) pin to enable transmission and reception when > + in slave mode. When would this ever not be true when in device mode?
On Wed, Sep 27, 2023 at 09:59:05AM +0200, Geert Uytterhoeven wrote: > On Tue, Sep 26, 2023 at 11:08 PM Fabrizio Castro > > + type: boolean > > + description: > > + Use CSI Slave Selection (SS) pin to enable transmission and reception when > > + in slave mode. > Can't this be done in a more generic way? I had expected that the > existing SPI_NO_CS flag can be set using a property in the "slave" subnode, > but apparently there is no "spi-no-cs" property defined yet. The description is clearly saying there is a chip select, _NO_CS seems entirely inappropriate. It's not specified in the device tree because when there's no chip select for a device it's a fundamental property of how the device is controlled and we don't need any information beyond the compatible.
Hi Mark, On Wed, Sep 27, 2023 at 11:00 AM Mark Brown <broonie@kernel.org> wrote: > On Wed, Sep 27, 2023 at 09:59:05AM +0200, Geert Uytterhoeven wrote: > > On Tue, Sep 26, 2023 at 11:08 PM Fabrizio Castro > > > + type: boolean > > > + description: > > > + Use CSI Slave Selection (SS) pin to enable transmission and reception when > > > + in slave mode. > > > Can't this be done in a more generic way? I had expected that the > > existing SPI_NO_CS flag can be set using a property in the "slave" subnode, > > but apparently there is no "spi-no-cs" property defined yet. > > The description is clearly saying there is a chip select, _NO_CS seems > entirely inappropriate. It's not specified in the device tree because > when there's no chip select for a device it's a fundamental property of > how the device is controlled and we don't need any information beyond > the compatible. In host mode, it indeed doesn't matter, as you can have only a single device connected with SPI_NO_CS. In device mode, the device needs to know if it must monitor the chip select line or not. In hindsight, I should have kept the question I had written initially, but deleted after having read the documentation for the corresponding RZ/V2M register bits: What does it mean if this is false? That there is no chip select? So "spi-no-cs" would be the inverse of "renesas,csi-ss". Gr{oetje,eeting}s, Geert
On Wed, Sep 27, 2023 at 11:10:58AM +0200, Geert Uytterhoeven wrote: > On Wed, Sep 27, 2023 at 11:00 AM Mark Brown <broonie@kernel.org> wrote: > > The description is clearly saying there is a chip select, _NO_CS seems > > entirely inappropriate. It's not specified in the device tree because > > when there's no chip select for a device it's a fundamental property of > > how the device is controlled and we don't need any information beyond > > the compatible. > In host mode, it indeed doesn't matter, as you can have only a single > device connected with SPI_NO_CS. > In device mode, the device needs to know if it must monitor the chip > select line or not. > In hindsight, I should have kept the question I had written initially, > but deleted after having read the documentation for the corresponding > RZ/V2M register bits: > What does it mean if this is false? That there is no chip select? > So "spi-no-cs" would be the inverse of "renesas,csi-ss". I see. Is there any control over what the chip select is when there is one, in which case we could just look to see if there's one specified? I'm a bit nervous about a generic property that maps onto _NO_CS since it's likely that people will start using that in device bindings when they shouldn't.
Hi Geert, Thanks for your reply! > From: Geert Uytterhoeven <geert@linux-m68k.org> > Subject: Re: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related > properties > > Hi Fabrizio, > > On Tue, Sep 26, 2023 at 11:08 PM Fabrizio Castro > <fabrizio.castro.jz@renesas.com> wrote: > > The CSI IP found inside the Renesas RZ/V2M SoC can also work > > in SPI slave mode. > > When working in slave mode, the IP can make use of the SS > > (Slave Select) pin, with "low" as default active level. > > The active level of SS can be changed to "high" upon configuration. > > This patch adds two new properties, one to make use of the > > SS pin when in slave mode, and one to make the SS pin active high. > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > Thanks for your patch! > > > --- a/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml > > +++ b/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml > > @@ -39,6 +39,17 @@ properties: > > power-domains: > > maxItems: 1 > > > > + renesas,csi-ss: > > + type: boolean > > + description: > > + Use CSI Slave Selection (SS) pin to enable transmission and > reception when > > + in slave mode. > > Can't this be done in a more generic way? I had expected that the > existing SPI_NO_CS flag can be set using a property in the "slave" > subnode, > but apparently there is no "spi-no-cs" property defined yet. I couldn't find a generic way to address that in a generic way. > > > + > > + renesas,csi-ss-high: > > + type: boolean > > + description: > > + The SS pin is active high (by default the SS pin is active > low). > > Can't you use the "spi-cs-high" property in the "slave" subnode > instead? I could. I didn't go for it for the first version of this series after reading the automatic handling of SPI_CS_HIGH (which for example doesn't get automatically added to the mode for targets using a GPIO as CS). Even though nothing prevents you from explicitly adding SPI_CS_HIGH to the mode and using "spi-cs-high" in the device tree, I thought that could be confusing, but maybe it's not? Thanks, Fab > > > + > > required: > > - compatible > > - reg > > 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
Hi Mark, Thanks for your reply! > From: Mark Brown <broonie@kernel.org> > Subject: Re: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related > properties > > On Tue, Sep 26, 2023 at 10:08:17PM +0100, Fabrizio Castro wrote: > > The CSI IP found inside the Renesas RZ/V2M SoC can also work > > in SPI slave mode. > > When working in slave mode, the IP can make use of the SS > > (Slave Select) pin, with "low" as default active level. > > The active level of SS can be changed to "high" upon configuration. > > This patch adds two new properties, one to make use of the > > SS pin when in slave mode, and one to make the SS pin active high. > > Please avoid the use of outdated terminology like this, prefer "device > mode" or similar. Okay, I'll resend with a more modern terminology ("host" and "target"). > > > + renesas,csi-ss: > > + type: boolean > > + description: > > + Use CSI Slave Selection (SS) pin to enable transmission and > reception when > > + in slave mode. > > When would this ever not be true when in device mode? This IP can do without the SS pin to start operations. In fact, the SS pin is disabled by default (I know...), it needs to be explicitly enabled, and when enabled the pin is active low by default. Cheers, Fab
Hi Geert, Thanks for your reply! > From: Geert Uytterhoeven <geert@linux-m68k.org> > Subject: Re: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related > properties > > Hi Mark, > > On Wed, Sep 27, 2023 at 11:00 AM Mark Brown <broonie@kernel.org> wrote: > > On Wed, Sep 27, 2023 at 09:59:05AM +0200, Geert Uytterhoeven wrote: > > > On Tue, Sep 26, 2023 at 11:08 PM Fabrizio Castro > > > > + type: boolean > > > > + description: > > > > + Use CSI Slave Selection (SS) pin to enable transmission > and reception when > > > > + in slave mode. > > > > > Can't this be done in a more generic way? I had expected that the > > > existing SPI_NO_CS flag can be set using a property in the "slave" > subnode, > > > but apparently there is no "spi-no-cs" property defined yet. > > > > The description is clearly saying there is a chip select, _NO_CS > seems > > entirely inappropriate. It's not specified in the device tree > because > > when there's no chip select for a device it's a fundamental property > of > > how the device is controlled and we don't need any information > beyond > > the compatible. > > In host mode, it indeed doesn't matter, as you can have only a single > device connected with SPI_NO_CS. > In device mode, the device needs to know if it must monitor the chip > select line or not. > > In hindsight, I should have kept the question I had written initially, > but deleted after having read the documentation for the corresponding > RZ/V2M register bits: > > What does it mean if this is false? That there is no chip select? Yes, that's what it means. The IP would just use the clock line to shift data in and out. Clearly, that could lead to "misunderstandings" as changing the active level of the clock line on the host side may lead to extra clock cycles interpreted by the slave. > > So "spi-no-cs" would be the inverse of "renesas,csi-ss". Yes, exactly. Cheers, Fab > > 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
Hi Mark, On Wed, Sep 27, 2023 at 11:21 AM Mark Brown <broonie@kernel.org> wrote: > On Wed, Sep 27, 2023 at 11:10:58AM +0200, Geert Uytterhoeven wrote: > > On Wed, Sep 27, 2023 at 11:00 AM Mark Brown <broonie@kernel.org> wrote: > > > > The description is clearly saying there is a chip select, _NO_CS seems > > > entirely inappropriate. It's not specified in the device tree because > > > when there's no chip select for a device it's a fundamental property of > > > how the device is controlled and we don't need any information beyond > > > the compatible. > > > In host mode, it indeed doesn't matter, as you can have only a single > > device connected with SPI_NO_CS. > > In device mode, the device needs to know if it must monitor the chip > > select line or not. > > > In hindsight, I should have kept the question I had written initially, > > but deleted after having read the documentation for the corresponding > > RZ/V2M register bits: > > > What does it mean if this is false? That there is no chip select? > > > So "spi-no-cs" would be the inverse of "renesas,csi-ss". > > I see. Is there any control over what the chip select is when there is > one, in which case we could just look to see if there's one specified? On RZ/V2M there isn't, as there is only a single hardware chip select. On MSIOF, there are 3 hardware chip selects, but apparently only the primary one can be used in target mode. I have to admit I never thought about this before (commit cf9e4784f3bde3e4 ("spi: sh-msiof: Add slave mode support") also predates commit 9cce882bedd2768d ("spi: sh-msiof: Extend support to 3 native chip selects")). Hence the SPI target DT bindings use a single "slave" subnode, without a unit address, thus assuming no explicit (or a default) chip select configuration. Gr{oetje,eeting}s, Geert
On Wed, Sep 27, 2023 at 11:44:17AM +0200, Geert Uytterhoeven wrote: > On Wed, Sep 27, 2023 at 11:21 AM Mark Brown <broonie@kernel.org> wrote: > > I see. Is there any control over what the chip select is when there is > > one, in which case we could just look to see if there's one specified? > On RZ/V2M there isn't, as there is only a single hardware chip select. > On MSIOF, there are 3 hardware chip selects, but apparently only the > primary one can be used in target mode. OK, it sounds like we do need a property then. Like I say I'd rather not have one that just works for _NO_CS in order to avoid confusion for people writing SPI device drivers, either something in the generic target binding or a device specific one.
Hi Mark, Thanks for your reply! > From: Mark Brown <broonie@kernel.org> > Subject: Re: [PATCH 1/2] spi: renesas,rzv2m-csi: Add SPI Slave related > properties > > On Wed, Sep 27, 2023 at 11:44:17AM +0200, Geert Uytterhoeven wrote: > > On Wed, Sep 27, 2023 at 11:21 AM Mark Brown <broonie@kernel.org> > wrote: > > > > I see. Is there any control over what the chip select is when > there is > > > one, in which case we could just look to see if there's one > specified? > > > On RZ/V2M there isn't, as there is only a single hardware chip > select. > > > On MSIOF, there are 3 hardware chip selects, but apparently only the > > primary one can be used in target mode. > > OK, it sounds like we do need a property then. Like I say I'd rather > not have one that just works for _NO_CS in order to avoid confusion > for > people writing SPI device drivers, either something in the generic > target binding or a device specific one. Shall I invert the logic then? What I mean is I could drop property "renesas,csi-ss" and add property "renesas,csi-no-ss" instead, therefore without "renesas,csi-no-ss" pin SS will be used, with "renesas,csi-no-ss" pin SS won't be used. What do you think? Also, I could drop "renesas,csi-ss-high" and use "spi-cs-high" instead? Geert, any thoughts on the above? Thanks, Fab
On Wed, Sep 27, 2023 at 10:18:57AM +0000, Fabrizio Castro wrote: > > From: Mark Brown <broonie@kernel.org> > > OK, it sounds like we do need a property then. Like I say I'd rather > > not have one that just works for _NO_CS in order to avoid confusion > > for > > people writing SPI device drivers, either something in the generic > > target binding or a device specific one. > Shall I invert the logic then? What I mean is I could drop property > "renesas,csi-ss" and add property "renesas,csi-no-ss" instead, therefore > without "renesas,csi-no-ss" pin SS will be used, with "renesas,csi-no-ss" > pin SS won't be used. > What do you think? That sounds fine for me, I guess we could add a further property if some new IP allows multiple options for the chip select in target mode. > Also, I could drop "renesas,csi-ss-high" and use "spi-cs-high" instead? I think that's OK but I looked less at that bit.
diff --git a/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml b/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml index e59183e53690..c3d8ad6525bb 100644 --- a/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml +++ b/Documentation/devicetree/bindings/spi/renesas,rzv2m-csi.yaml @@ -39,6 +39,17 @@ properties: power-domains: maxItems: 1 + renesas,csi-ss: + type: boolean + description: + Use CSI Slave Selection (SS) pin to enable transmission and reception when + in slave mode. + + renesas,csi-ss-high: + type: boolean + description: + The SS pin is active high (by default the SS pin is active low). + required: - compatible - reg @@ -50,6 +61,10 @@ required: - '#address-cells' - '#size-cells' +dependencies: + renesas,csi-ss: [ spi-slave ] + renesas,csi-ss-high: [ 'renesas,csi-ss' ] + unevaluatedProperties: false examples:
The CSI IP found inside the Renesas RZ/V2M SoC can also work in SPI slave mode. When working in slave mode, the IP can make use of the SS (Slave Select) pin, with "low" as default active level. The active level of SS can be changed to "high" upon configuration. This patch adds two new properties, one to make use of the SS pin when in slave mode, and one to make the SS pin active high. Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> --- .../bindings/spi/renesas,rzv2m-csi.yaml | 15 +++++++++++++++ 1 file changed, 15 insertions(+)