spi: xilinx: Add DT support for selecting transfer word width
diff mbox series

Message ID 20191022090007.15147-1-alvaro.gamez@hazent.com
State Superseded
Headers show
Series
  • spi: xilinx: Add DT support for selecting transfer word width
Related show

Commit Message

Alvaro Gamez Machado Oct. 22, 2019, 9 a.m. UTC
Signed-off-by: Alvaro Gamez Machado <alvaro.gamez@hazent.com>
---
 drivers/spi/spi-xilinx.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Mark Brown Oct. 22, 2019, 10:20 a.m. UTC | #1
On Tue, Oct 22, 2019 at 11:00:07AM +0200, Alvaro Gamez Machado wrote:

>  		of_property_read_u32(pdev->dev.of_node, "xlnx,num-ss-bits",
>  					  &num_cs);
> +		ret = of_property_read_u32(pdev->dev.of_node,
> +					   "xlnx,num-transfer-bits",
> +					   &bits_per_word);
> +		if (ret)
> +			bits_per_word = 8;

Any new DT property needs documentation adding but in any case this
shouldn't be set in DT, it should be set by the client driver - it's
going to be a fixed property of the device, not something that varies
per system.
Alvaro Gamez Machado Oct. 22, 2019, 10:29 a.m. UTC | #2
On Tue, Oct 22, 2019 at 11:20:07AM +0100, Mark Brown wrote:
> On Tue, Oct 22, 2019 at 11:00:07AM +0200, Alvaro Gamez Machado wrote:
> 
> >  		of_property_read_u32(pdev->dev.of_node, "xlnx,num-ss-bits",
> >  					  &num_cs);
> > +		ret = of_property_read_u32(pdev->dev.of_node,
> > +					   "xlnx,num-transfer-bits",
> > +					   &bits_per_word);
> > +		if (ret)
> > +			bits_per_word = 8;
> 
> Any new DT property needs documentation adding but in any case this

Oh, ok. If this patch is of any interest, then I should provide changes to
the appropriate file under Documentation/, is that right?

I was preparing and testing a second patch to add "spi-bits-per-word"
property to children of the SPI driver, similar to the spi-max-frequency
property and others alike, to fully support AXI Quad SPI core with different
word widths. This also modifies the DT, so I guess it'd be better to send a
single patch that comprises all these changes alongside the DT
documentation. Would that be correct procedure?

> shouldn't be set in DT, it should be set by the client driver - it's
> going to be a fixed property of the device, not something that varies
> per system.

But this is in fact something that changes per system. When instantiating an
AXI Quad SPI core in a HDL design that's exactly one of the options you can
provide. In fact, in the board I'm working with right now, I'm instantiating
two SPI cores, one of them with 8 bits per word but the other one requires
32 bits per word, as the devices it's going to talk to have this
requirement.

Thanks,
Mark Brown Oct. 22, 2019, 11:26 a.m. UTC | #3
On Tue, Oct 22, 2019 at 12:29:01PM +0200, Alvaro Gamez Machado wrote:
> On Tue, Oct 22, 2019 at 11:20:07AM +0100, Mark Brown wrote:

> > Any new DT property needs documentation adding but in any case this

> Oh, ok. If this patch is of any interest, then I should provide changes to
> the appropriate file under Documentation/, is that right?

Yes.

> I was preparing and testing a second patch to add "spi-bits-per-word"
> property to children of the SPI driver, similar to the spi-max-frequency
> property and others alike, to fully support AXI Quad SPI core with different
> word widths. This also modifies the DT, so I guess it'd be better to send a
> single patch that comprises all these changes alongside the DT
> documentation. Would that be correct procedure?

Usually the DT changes should be a separate patch to the code changes.

> > shouldn't be set in DT, it should be set by the client driver - it's
> > going to be a fixed property of the device, not something that varies
> > per system.

> But this is in fact something that changes per system. When instantiating an
> AXI Quad SPI core in a HDL design that's exactly one of the options you can
> provide. In fact, in the board I'm working with right now, I'm instantiating
> two SPI cores, one of them with 8 bits per word but the other one requires
> 32 bits per word, as the devices it's going to talk to have this
> requirement.

This is still something that should be configured by the client driver,
if you send data with a different word size to that the client intends
it'll just get corrupted.
Alvaro Gamez Machado Oct. 22, 2019, 12:06 p.m. UTC | #4
On Tue, Oct 22, 2019 at 12:26:00PM +0100, Mark Brown wrote:
> On Tue, Oct 22, 2019 at 12:29:01PM +0200, Alvaro Gamez Machado wrote:
> > On Tue, Oct 22, 2019 at 11:20:07AM +0100, Mark Brown wrote:
> 
> > > Any new DT property needs documentation adding but in any case this
> 
> > Oh, ok. If this patch is of any interest, then I should provide changes to
> > the appropriate file under Documentation/, is that right?
> 
> Yes.
> 
> > I was preparing and testing a second patch to add "spi-bits-per-word"
> > property to children of the SPI driver, similar to the spi-max-frequency
> > property and others alike, to fully support AXI Quad SPI core with different
> > word widths. This also modifies the DT, so I guess it'd be better to send a
> > single patch that comprises all these changes alongside the DT
> > documentation. Would that be correct procedure?
> 
> Usually the DT changes should be a separate patch to the code changes.

Understood, thank you!
 
> > > shouldn't be set in DT, it should be set by the client driver - it's
> > > going to be a fixed property of the device, not something that varies
> > > per system.
> 
> > But this is in fact something that changes per system. When instantiating an
> > AXI Quad SPI core in a HDL design that's exactly one of the options you can
> > provide. In fact, in the board I'm working with right now, I'm instantiating
> > two SPI cores, one of them with 8 bits per word but the other one requires
> > 32 bits per word, as the devices it's going to talk to have this
> > requirement.
> 
> This is still something that should be configured by the client driver,
> if you send data with a different word size to that the client intends
> it'll just get corrupted.


The problem is that Xilinx's AXI Quad SPI core doesn't allow this. When
instantiating the core you must choose *the* transfer width, not the
*maximum* transfer width. So in my example above, no matter how I configure
my IP core, linux'll believe that its datawidth is 8. I could override it
hardcoding a 32 in spi-xilinx.c, but then what would happen with my other
IP core that needs a datawidth of 8? Client code cannot configure IP core
with a different datawidth because it simply does not allow it.
Geert Uytterhoeven Oct. 22, 2019, 1:03 p.m. UTC | #5
Hi Alvaro,

On Tue, Oct 22, 2019 at 12:38 PM Alvaro Gamez Machado
<alvaro.gamez@hazent.com> wrote:
> On Tue, Oct 22, 2019 at 11:20:07AM +0100, Mark Brown wrote:
> > On Tue, Oct 22, 2019 at 11:00:07AM +0200, Alvaro Gamez Machado wrote:
> >
> > >             of_property_read_u32(pdev->dev.of_node, "xlnx,num-ss-bits",
> > >                                       &num_cs);
> > > +           ret = of_property_read_u32(pdev->dev.of_node,
> > > +                                      "xlnx,num-transfer-bits",
> > > +                                      &bits_per_word);
> > > +           if (ret)
> > > +                   bits_per_word = 8;
> >
> > Any new DT property needs documentation adding but in any case this
>
> Oh, ok. If this patch is of any interest, then I should provide changes to
> the appropriate file under Documentation/, is that right?
>
> I was preparing and testing a second patch to add "spi-bits-per-word"
> property to children of the SPI driver, similar to the spi-max-frequency
> property and others alike, to fully support AXI Quad SPI core with different
> word widths. This also modifies the DT, so I guess it'd be better to send a
> single patch that comprises all these changes alongside the DT
> documentation. Would that be correct procedure?

Is this "spi-bits-per-word" a property of the SPI slave device?
If yes, it may be better to hardcode it in the SPI slave device driver,
as it is fixed for that type of SPI slave.

If not, perhaps I'm misunderstanding the purpose.

> > shouldn't be set in DT, it should be set by the client driver - it's
> > going to be a fixed property of the device, not something that varies
> > per system.
>
> But this is in fact something that changes per system. When instantiating an
> AXI Quad SPI core in a HDL design that's exactly one of the options you can
> provide. In fact, in the board I'm working with right now, I'm instantiating
> two SPI cores, one of them with 8 bits per word but the other one requires
> 32 bits per word, as the devices it's going to talk to have this
> requirement.

So you're instantiating two variants of the same "xlnx,axi-quad-spi-1.00.a"
controller, with different options?
While you could add an "xlnx,foo" DT property for that, an alternative could
be to introduce a new compatible value.
It all depends on how many options there are when instantiating such a
controller.

Gr{oetje,eeting}s,

                        Geert
Alvaro Gamez Machado Oct. 22, 2019, 2:01 p.m. UTC | #6
Hi Geert!

On Tue, Oct 22, 2019 at 03:03:18PM +0200, Geert Uytterhoeven wrote:
> Hi Alvaro,
> 
> On Tue, Oct 22, 2019 at 12:38 PM Alvaro Gamez Machado
> <alvaro.gamez@hazent.com> wrote:
> > On Tue, Oct 22, 2019 at 11:20:07AM +0100, Mark Brown wrote:
> > > On Tue, Oct 22, 2019 at 11:00:07AM +0200, Alvaro Gamez Machado wrote:
> > >
> > > >             of_property_read_u32(pdev->dev.of_node, "xlnx,num-ss-bits",
> > > >                                       &num_cs);
> > > > +           ret = of_property_read_u32(pdev->dev.of_node,
> > > > +                                      "xlnx,num-transfer-bits",
> > > > +                                      &bits_per_word);
> > > > +           if (ret)
> > > > +                   bits_per_word = 8;
> > >
> > > Any new DT property needs documentation adding but in any case this
> >
> > Oh, ok. If this patch is of any interest, then I should provide changes to
> > the appropriate file under Documentation/, is that right?
> >
> > I was preparing and testing a second patch to add "spi-bits-per-word"
> > property to children of the SPI driver, similar to the spi-max-frequency
> > property and others alike, to fully support AXI Quad SPI core with different
> > word widths. This also modifies the DT, so I guess it'd be better to send a
> > single patch that comprises all these changes alongside the DT
> > documentation. Would that be correct procedure?
> 
> Is this "spi-bits-per-word" a property of the SPI slave device?
> If yes, it may be better to hardcode it in the SPI slave device driver,
> as it is fixed for that type of SPI slave.
> 
> If not, perhaps I'm misunderstanding the purpose.

Ok, this is a bit confusing and my patch didn't really explain a lot. I'll
try to explain myself and not make a lot of mistakes.

Most, if not all SPI slaves, have a fixed word width, which is typically 8
bits, but there exist some devices for which this value is 16, 32 or who
knows what.

On the other hand, master SPI devices may have the possibility to use word
widths of 8, 16, 32 or whatever, and some of them may be able to switch its
word width on demand.

The specific case of AXI Quad SPI core is that you choose this value upon
instantiation and you cannot modify it via software. This means that you
could instantiate an 8 bit IP core to talk to a 32 wordwidth slave, and
you'd get no useful communication, that's the harwdare designer
responsibility: both devices must match in its wordwidth.

More over, it seems that linux needs to know this information to build the
messages it needs to send to the SPI master.

For something this simple, I believe the best approach is a DT property, as
I've proposed in my not-very-clearly-explained patch. So, this means a DT
property for the SPI master, although a different compatible string could be
used as you suggested. See below for my comments on this idea.

Any way, afer this, assuming we have a way of setting this master driver
with the correct wordwidth, and given that there may be SPI masters that are
able to switch it on demand, it seems feasible to connect two or more slaves
with different wordwidth to the same SPI master.

It's true that this value could be hardcoded in each slave device driver
code, but currently the SPI infrastructure sets this value to 8 by
default. What's wrong with this? The problem lies on userspace drivers.

For SPI slaves that do not have an in kernel code, we can add a compatible
string to spidev_dt_ids, which will provide a /dev/spidevX.Y node which can
be used from userspace, from the set of properties that we write on the DT.

Using this spidev node one would expect to be able to set this slave in
particular to a different datawidth using SPI user interface, with ioctl 
SPI_IOC_WR_BITS_PER_WORD, basically the same as what we'd do in the kernel.

What happens, however, is that on boot a wordwidth of 8 is assigned by
default, and so there occurs this discrepancy between what the master is
capable of and what the slave requires. The patch I sent before makes the
master know it can work with something different to 8, in my case that's 32.
But when trying to initialize /dev/spidev devices, the following happens:

xilinx_spi 44a10000.spi: can't setup spi1.0, status -22
spi_master spi1: spi_device register error /amba_pl/spi@44a10000/spidev@0
spi_master spi1: Failed to create SPI device for /amba_pl/spi@44a10000/spidev@0

So now there's no way for me to open /dev/spidev1.0 and change its
bits-per-word parameter, because /dev/spidev1.0 does not exist and it's
trying to link an 8 bit wordwidth slave with a 32 bit wordwidth master.

So, in fact here below is the full patch that will serve my purpose of

a) Configuring my up-to-now unconfigurable SPI master driver to support
   16 or 32 wordwidth.
b) Allow its slaves to be visible from userspace without the need of a
   kernel driver

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 92ea22fedb33..46bb103ef30e 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -380,7 +380,7 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 	struct xilinx_spi *xspi;
 	struct xspi_platform_data *pdata;
 	struct resource *res;
-	int ret, num_cs = 0, bits_per_word = 8;
+	int ret, num_cs = 0, bits_per_word;
 	struct spi_master *master;
 	u32 tmp;
 	u8 i;
@@ -392,6 +392,11 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 	} else {
 		of_property_read_u32(pdev->dev.of_node, "xlnx,num-ss-bits",
 					  &num_cs);
+		ret = of_property_read_u32(pdev->dev.of_node,
+					   "xlnx,num-transfer-bits",
+					   &bits_per_word);
+		if (ret)
+			bits_per_word = 8;
 	}
 
 	if (!num_cs) {
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index f9502dbbb5c1..06424b7b0d73 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1792,6 +1792,10 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 	}
 	spi->max_speed_hz = value;
 
+	/* Bits per word */
+	if (!of_property_read_u32(nc, "spi-bits-per-word", &value))
+		spi->bits_per_word = value;
+
 	return 0;
 }
 



> 
> > > shouldn't be set in DT, it should be set by the client driver - it's
> > > going to be a fixed property of the device, not something that varies
> > > per system.
> >
> > But this is in fact something that changes per system. When instantiating an
> > AXI Quad SPI core in a HDL design that's exactly one of the options you can
> > provide. In fact, in the board I'm working with right now, I'm instantiating
> > two SPI cores, one of them with 8 bits per word but the other one requires
> > 32 bits per word, as the devices it's going to talk to have this
> > requirement.
> 
> So you're instantiating two variants of the same "xlnx,axi-quad-spi-1.00.a"
> controller, with different options?
> While you could add an "xlnx,foo" DT property for that, an alternative could
> be to introduce a new compatible value.
> It all depends on how many options there are when instantiating such a
> controller.
> 

Regardless of which SPI slave will be connected to the SPI master, there's
the need to indicate in the DT how is this master configured. Now, you could
definitely add another compatible value, but then I'd need to introduce a
compatible value for 8 bits, another one for 16 and a last one for 32. I
don't think this is easier nor powerful enough, and I think the use of the
DT property matches very well with other drivers I've read, such as
axi-ethernet and others (most of them from the Xilinx IP core set), but I
surely can be in the wrong about this, I'm pretty noob on kernel code as you
can see.

I hope this provides more insight on what my intent is, thanks for your
patience!
Geert Uytterhoeven Oct. 22, 2019, 2:26 p.m. UTC | #7
Hi Alvaro,

On Tue, Oct 22, 2019 at 4:01 PM Alvaro Gamez Machado
<alvaro.gamez@hazent.com> wrote:
> On Tue, Oct 22, 2019 at 03:03:18PM +0200, Geert Uytterhoeven wrote:
> > On Tue, Oct 22, 2019 at 12:38 PM Alvaro Gamez Machado
> > <alvaro.gamez@hazent.com> wrote:
> > > On Tue, Oct 22, 2019 at 11:20:07AM +0100, Mark Brown wrote:
> > > > On Tue, Oct 22, 2019 at 11:00:07AM +0200, Alvaro Gamez Machado wrote:
> > > >
> > > > >             of_property_read_u32(pdev->dev.of_node, "xlnx,num-ss-bits",
> > > > >                                       &num_cs);
> > > > > +           ret = of_property_read_u32(pdev->dev.of_node,
> > > > > +                                      "xlnx,num-transfer-bits",
> > > > > +                                      &bits_per_word);
> > > > > +           if (ret)
> > > > > +                   bits_per_word = 8;
> > > >
> > > > Any new DT property needs documentation adding but in any case this
> > >
> > > Oh, ok. If this patch is of any interest, then I should provide changes to
> > > the appropriate file under Documentation/, is that right?
> > >
> > > I was preparing and testing a second patch to add "spi-bits-per-word"
> > > property to children of the SPI driver, similar to the spi-max-frequency
> > > property and others alike, to fully support AXI Quad SPI core with different
> > > word widths. This also modifies the DT, so I guess it'd be better to send a
> > > single patch that comprises all these changes alongside the DT
> > > documentation. Would that be correct procedure?
> >
> > Is this "spi-bits-per-word" a property of the SPI slave device?
> > If yes, it may be better to hardcode it in the SPI slave device driver,
> > as it is fixed for that type of SPI slave.
> >
> > If not, perhaps I'm misunderstanding the purpose.
>
> Ok, this is a bit confusing and my patch didn't really explain a lot. I'll
> try to explain myself and not make a lot of mistakes.
>
> Most, if not all SPI slaves, have a fixed word width, which is typically 8
> bits, but there exist some devices for which this value is 16, 32 or who
> knows what.
>
> On the other hand, master SPI devices may have the possibility to use word
> widths of 8, 16, 32 or whatever, and some of them may be able to switch its
> word width on demand.

OK.

> The specific case of AXI Quad SPI core is that you choose this value upon
> instantiation and you cannot modify it via software. This means that you
> could instantiate an 8 bit IP core to talk to a 32 wordwidth slave, and
> you'd get no useful communication, that's the harwdare designer
> responsibility: both devices must match in its wordwidth.
>
> More over, it seems that linux needs to know this information to build the
> messages it needs to send to the SPI master.

You also have to configure this in spi_controller.bits_per_word_mask.
Typical values are:
    - SPI_BPW_RANGE_MASK(8, 32),
    - SPI_BPW_MASK(8) | SPI_BPW_MASK(16) | SPI_BPW_MASK(32),
    - SPI_BPW_MASK(8).

So you have to use SPI_BPW_MASK(8)[*] resp. SPI_BPW_MASK(32).

[*] This could actually be relaxed to 8/16/24/32, as the last three can be
    emulated using 8.

> For something this simple, I believe the best approach is a DT property, as
> I've proposed in my not-very-clearly-explained patch. So, this means a DT
> property for the SPI master, although a different compatible string could be
> used as you suggested. See below for my comments on this idea.

OK.

> Any way, afer this, assuming we have a way of setting this master driver
> with the correct wordwidth, and given that there may be SPI masters that are
> able to switch it on demand, it seems feasible to connect two or more slaves
> with different wordwidth to the same SPI master.

Sure, but only if the master supports all bits_per_word values needed
by the slaves.

> It's true that this value could be hardcoded in each slave device driver
> code, but currently the SPI infrastructure sets this value to 8 by

Slave drivers for slaves using a different value should fill in a different
value.

> default. What's wrong with this? The problem lies on userspace drivers.
>
> For SPI slaves that do not have an in kernel code, we can add a compatible
> string to spidev_dt_ids, which will provide a /dev/spidevX.Y node which can
> be used from userspace, from the set of properties that we write on the DT.
>
> Using this spidev node one would expect to be able to set this slave in
> particular to a different datawidth using SPI user interface, with ioctl
> SPI_IOC_WR_BITS_PER_WORD, basically the same as what we'd do in the kernel.
>
> What happens, however, is that on boot a wordwidth of 8 is assigned by
> default, and so there occurs this discrepancy between what the master is
> capable of and what the slave requires. The patch I sent before makes the
> master know it can work with something different to 8, in my case that's 32.
> But when trying to initialize /dev/spidev devices, the following happens:
>
> xilinx_spi 44a10000.spi: can't setup spi1.0, status -22
> spi_master spi1: spi_device register error /amba_pl/spi@44a10000/spidev@0
> spi_master spi1: Failed to create SPI device for /amba_pl/spi@44a10000/spidev@0
>
> So now there's no way for me to open /dev/spidev1.0 and change its
> bits-per-word parameter, because /dev/spidev1.0 does not exist and it's
> trying to link an 8 bit wordwidth slave with a 32 bit wordwidth master.

OK, so that's a bug in the SPI core, or in spidev.

> So, in fact here below is the full patch that will serve my purpose of
>
> a) Configuring my up-to-now unconfigurable SPI master driver to support
>    16 or 32 wordwidth.

OK.

> b) Allow its slaves to be visible from userspace without the need of a
>    kernel driver
>
> diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
> index 92ea22fedb33..46bb103ef30e 100644
> --- a/drivers/spi/spi-xilinx.c
> +++ b/drivers/spi/spi-xilinx.c
> @@ -380,7 +380,7 @@ static int xilinx_spi_probe(struct platform_device *pdev)
>         struct xilinx_spi *xspi;
>         struct xspi_platform_data *pdata;
>         struct resource *res;
> -       int ret, num_cs = 0, bits_per_word = 8;
> +       int ret, num_cs = 0, bits_per_word;
>         struct spi_master *master;
>         u32 tmp;
>         u8 i;
> @@ -392,6 +392,11 @@ static int xilinx_spi_probe(struct platform_device *pdev)
>         } else {
>                 of_property_read_u32(pdev->dev.of_node, "xlnx,num-ss-bits",
>                                           &num_cs);
> +               ret = of_property_read_u32(pdev->dev.of_node,
> +                                          "xlnx,num-transfer-bits",
> +                                          &bits_per_word);
> +               if (ret)
> +                       bits_per_word = 8;
>         }
>
>         if (!num_cs) {

Looks good to me.

> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index f9502dbbb5c1..06424b7b0d73 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1792,6 +1792,10 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>         }
>         spi->max_speed_hz = value;
>
> +       /* Bits per word */
> +       if (!of_property_read_u32(nc, "spi-bits-per-word", &value))
> +               spi->bits_per_word = value;
> +
>         return 0;
>  }

Probably this should be fixed in spidev instead, to use a value that
conforms to spi_controller.bits_per_word_mask.
That would remove the need for specifying this in DT.
Once the spidev is instantiated, userspace can still change it using
SPI_IOC_WR_BITS_PER_WORD.

> > > > shouldn't be set in DT, it should be set by the client driver - it's
> > > > going to be a fixed property of the device, not something that varies
> > > > per system.
> > >
> > > But this is in fact something that changes per system. When instantiating an
> > > AXI Quad SPI core in a HDL design that's exactly one of the options you can
> > > provide. In fact, in the board I'm working with right now, I'm instantiating
> > > two SPI cores, one of them with 8 bits per word but the other one requires
> > > 32 bits per word, as the devices it's going to talk to have this
> > > requirement.
> >
> > So you're instantiating two variants of the same "xlnx,axi-quad-spi-1.00.a"
> > controller, with different options?
> > While you could add an "xlnx,foo" DT property for that, an alternative could
> > be to introduce a new compatible value.
> > It all depends on how many options there are when instantiating such a
> > controller.
> >
>
> Regardless of which SPI slave will be connected to the SPI master, there's
> the need to indicate in the DT how is this master configured. Now, you could
> definitely add another compatible value, but then I'd need to introduce a
> compatible value for 8 bits, another one for 16 and a last one for 32. I
> don't think this is easier nor powerful enough, and I think the use of the
> DT property matches very well with other drivers I've read, such as
> axi-ethernet and others (most of them from the Xilinx IP core set), but I
> surely can be in the wrong about this, I'm pretty noob on kernel code as you
> can see.

Yes, using a DT property for this is fine.  It just needs to be documented
in Documentation/devicetree/bindings/spi/spi-xilinx.txt (or its yaml
counterpart).

Gr{oetje,eeting}s,

                        Geert
Alvaro Gamez Machado Oct. 22, 2019, 3:09 p.m. UTC | #8
On Tue, Oct 22, 2019 at 04:26:32PM +0200, Geert Uytterhoeven wrote:
> Hi Alvaro,
> 
> On Tue, Oct 22, 2019 at 4:01 PM Alvaro Gamez Machado
> <alvaro.gamez@hazent.com> wrote:
> > On Tue, Oct 22, 2019 at 03:03:18PM +0200, Geert Uytterhoeven wrote:
> > > On Tue, Oct 22, 2019 at 12:38 PM Alvaro Gamez Machado
> > > <alvaro.gamez@hazent.com> wrote:
> > > > On Tue, Oct 22, 2019 at 11:20:07AM +0100, Mark Brown wrote:
> > > > > On Tue, Oct 22, 2019 at 11:00:07AM +0200, Alvaro Gamez Machado wrote:
> > > > >
> > > > > >             of_property_read_u32(pdev->dev.of_node, "xlnx,num-ss-bits",
> > > > > >                                       &num_cs);
> > > > > > +           ret = of_property_read_u32(pdev->dev.of_node,
> > > > > > +                                      "xlnx,num-transfer-bits",
> > > > > > +                                      &bits_per_word);
> > > > > > +           if (ret)
> > > > > > +                   bits_per_word = 8;
> > > > >
> > > > > Any new DT property needs documentation adding but in any case this
> > > >
> > > > Oh, ok. If this patch is of any interest, then I should provide changes to
> > > > the appropriate file under Documentation/, is that right?
> > > >
> > > > I was preparing and testing a second patch to add "spi-bits-per-word"
> > > > property to children of the SPI driver, similar to the spi-max-frequency
> > > > property and others alike, to fully support AXI Quad SPI core with different
> > > > word widths. This also modifies the DT, so I guess it'd be better to send a
> > > > single patch that comprises all these changes alongside the DT
> > > > documentation. Would that be correct procedure?
> > >
> > > Is this "spi-bits-per-word" a property of the SPI slave device?
> > > If yes, it may be better to hardcode it in the SPI slave device driver,
> > > as it is fixed for that type of SPI slave.
> > >
> > > If not, perhaps I'm misunderstanding the purpose.
> >
> > Ok, this is a bit confusing and my patch didn't really explain a lot. I'll
> > try to explain myself and not make a lot of mistakes.
> >
> > Most, if not all SPI slaves, have a fixed word width, which is typically 8
> > bits, but there exist some devices for which this value is 16, 32 or who
> > knows what.
> >
> > On the other hand, master SPI devices may have the possibility to use word
> > widths of 8, 16, 32 or whatever, and some of them may be able to switch its
> > word width on demand.
> 
> OK.
> 
> > The specific case of AXI Quad SPI core is that you choose this value upon
> > instantiation and you cannot modify it via software. This means that you
> > could instantiate an 8 bit IP core to talk to a 32 wordwidth slave, and
> > you'd get no useful communication, that's the harwdare designer
> > responsibility: both devices must match in its wordwidth.
> >
> > More over, it seems that linux needs to know this information to build the
> > messages it needs to send to the SPI master.
> 
> You also have to configure this in spi_controller.bits_per_word_mask.
> Typical values are:
>     - SPI_BPW_RANGE_MASK(8, 32),
>     - SPI_BPW_MASK(8) | SPI_BPW_MASK(16) | SPI_BPW_MASK(32),
>     - SPI_BPW_MASK(8).
> 
> So you have to use SPI_BPW_MASK(8)[*] resp. SPI_BPW_MASK(32).
> 
> [*] This could actually be relaxed to 8/16/24/32, as the last three can be
>     emulated using 8.

Yes. My patch does not include any change on this because spi-xilinx already
takes into account that there's no range for it, and simply sets
master->bits_per_word_mask = SPI_BPW_MASK(bits_per_word);
so no problem here.
 
> > For something this simple, I believe the best approach is a DT property, as
> > I've proposed in my not-very-clearly-explained patch. So, this means a DT
> > property for the SPI master, although a different compatible string could be
> > used as you suggested. See below for my comments on this idea.
> 
> OK.
> 
> > Any way, afer this, assuming we have a way of setting this master driver
> > with the correct wordwidth, and given that there may be SPI masters that are
> > able to switch it on demand, it seems feasible to connect two or more slaves
> > with different wordwidth to the same SPI master.
> 
> Sure, but only if the master supports all bits_per_word values needed
> by the slaves.
> 
> > It's true that this value could be hardcoded in each slave device driver
> > code, but currently the SPI infrastructure sets this value to 8 by
> 
> Slave drivers for slaves using a different value should fill in a different
> value.
> 
> > default. What's wrong with this? The problem lies on userspace drivers.
> >
> > For SPI slaves that do not have an in kernel code, we can add a compatible
> > string to spidev_dt_ids, which will provide a /dev/spidevX.Y node which can
> > be used from userspace, from the set of properties that we write on the DT.
> >
> > Using this spidev node one would expect to be able to set this slave in
> > particular to a different datawidth using SPI user interface, with ioctl
> > SPI_IOC_WR_BITS_PER_WORD, basically the same as what we'd do in the kernel.
> >
> > What happens, however, is that on boot a wordwidth of 8 is assigned by
> > default, and so there occurs this discrepancy between what the master is
> > capable of and what the slave requires. The patch I sent before makes the
> > master know it can work with something different to 8, in my case that's 32.
> > But when trying to initialize /dev/spidev devices, the following happens:
> >
> > xilinx_spi 44a10000.spi: can't setup spi1.0, status -22
> > spi_master spi1: spi_device register error /amba_pl/spi@44a10000/spidev@0
> > spi_master spi1: Failed to create SPI device for /amba_pl/spi@44a10000/spidev@0
> >
> > So now there's no way for me to open /dev/spidev1.0 and change its
> > bits-per-word parameter, because /dev/spidev1.0 does not exist and it's
> > trying to link an 8 bit wordwidth slave with a 32 bit wordwidth master.
> 
> OK, so that's a bug in the SPI core, or in spidev.
> 
> > So, in fact here below is the full patch that will serve my purpose of
> >
> > a) Configuring my up-to-now unconfigurable SPI master driver to support
> >    16 or 32 wordwidth.
> 
> OK.
> 
> > b) Allow its slaves to be visible from userspace without the need of a
> >    kernel driver
> >
> > diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
> > index 92ea22fedb33..46bb103ef30e 100644
> > --- a/drivers/spi/spi-xilinx.c
> > +++ b/drivers/spi/spi-xilinx.c
> > @@ -380,7 +380,7 @@ static int xilinx_spi_probe(struct platform_device *pdev)
> >         struct xilinx_spi *xspi;
> >         struct xspi_platform_data *pdata;
> >         struct resource *res;
> > -       int ret, num_cs = 0, bits_per_word = 8;
> > +       int ret, num_cs = 0, bits_per_word;
> >         struct spi_master *master;
> >         u32 tmp;
> >         u8 i;
> > @@ -392,6 +392,11 @@ static int xilinx_spi_probe(struct platform_device *pdev)
> >         } else {
> >                 of_property_read_u32(pdev->dev.of_node, "xlnx,num-ss-bits",
> >                                           &num_cs);
> > +               ret = of_property_read_u32(pdev->dev.of_node,
> > +                                          "xlnx,num-transfer-bits",
> > +                                          &bits_per_word);
> > +               if (ret)
> > +                       bits_per_word = 8;
> >         }
> >
> >         if (!num_cs) {
> 
> Looks good to me.
> 
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index f9502dbbb5c1..06424b7b0d73 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -1792,6 +1792,10 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
> >         }
> >         spi->max_speed_hz = value;
> >
> > +       /* Bits per word */
> > +       if (!of_property_read_u32(nc, "spi-bits-per-word", &value))
> > +               spi->bits_per_word = value;
> > +
> >         return 0;
> >  }
> 
> Probably this should be fixed in spidev instead, to use a value that
> conforms to spi_controller.bits_per_word_mask.
> That would remove the need for specifying this in DT.
> Once the spidev is instantiated, userspace can still change it using
> SPI_IOC_WR_BITS_PER_WORD.

Ok, I see. The idea is not to introduce this new property but have a similar
line of code that checks ctlr->bits_per_word_mask and sets
spi->bits_per_word to a value that lies in this range. Am I allowed to
process this bitmask with ffs() or would that break the encapsulation
intended by SPI_BPW macros? Maybe something like this:

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index f9502dbbb5c1..3e09a9d43f2b 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1792,6 +1792,8 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 	}
 	spi->max_speed_hz = value;
 
+	spi->bits_per_word = ffs(ctlr->bits_per_word_mask);
+
 	return 0;
 }
 
This should be then a patch on itself because it's solving the user mode bug.



> > > > > shouldn't be set in DT, it should be set by the client driver - it's
> > > > > going to be a fixed property of the device, not something that varies
> > > > > per system.
> > > >
> > > > But this is in fact something that changes per system. When instantiating an
> > > > AXI Quad SPI core in a HDL design that's exactly one of the options you can
> > > > provide. In fact, in the board I'm working with right now, I'm instantiating
> > > > two SPI cores, one of them with 8 bits per word but the other one requires
> > > > 32 bits per word, as the devices it's going to talk to have this
> > > > requirement.
> > >
> > > So you're instantiating two variants of the same "xlnx,axi-quad-spi-1.00.a"
> > > controller, with different options?
> > > While you could add an "xlnx,foo" DT property for that, an alternative could
> > > be to introduce a new compatible value.
> > > It all depends on how many options there are when instantiating such a
> > > controller.
> > >
> >
> > Regardless of which SPI slave will be connected to the SPI master, there's
> > the need to indicate in the DT how is this master configured. Now, you could
> > definitely add another compatible value, but then I'd need to introduce a
> > compatible value for 8 bits, another one for 16 and a last one for 32. I
> > don't think this is easier nor powerful enough, and I think the use of the
> > DT property matches very well with other drivers I've read, such as
> > axi-ethernet and others (most of them from the Xilinx IP core set), but I
> > surely can be in the wrong about this, I'm pretty noob on kernel code as you
> > can see.
> 
> Yes, using a DT property for this is fine.  It just needs to be documented
> in Documentation/devicetree/bindings/spi/spi-xilinx.txt (or its yaml
> counterpart).

Ok, so to summarize things a bit:

a) My first patch is acceptable, but I have to document DT changes.
   Is it right to do it on the same commit and send them to these lists
   or shall I notify too the maintainers of bindings/spi/spi-xilinx.txt?

b) There's a bug that affects spidev and can be solved with a patch similar
   to the one above. I'll be testing that and submit it as well.

c) I'll try to reflect all this conversation in both commits


Best regards!
Mark Brown Oct. 22, 2019, 3:13 p.m. UTC | #9
On Tue, Oct 22, 2019 at 03:03:18PM +0200, Geert Uytterhoeven wrote:

> Is this "spi-bits-per-word" a property of the SPI slave device?
> If yes, it may be better to hardcode it in the SPI slave device driver,
> as it is fixed for that type of SPI slave.

> If not, perhaps I'm misunderstanding the purpose.

It most likely is.

> > AXI Quad SPI core in a HDL design that's exactly one of the options you can
> > provide. In fact, in the board I'm working with right now, I'm instantiating
> > two SPI cores, one of them with 8 bits per word but the other one requires
> > 32 bits per word, as the devices it's going to talk to have this
> > requirement.

> So you're instantiating two variants of the same "xlnx,axi-quad-spi-1.00.a"
> controller, with different options?
> While you could add an "xlnx,foo" DT property for that, an alternative could
> be to introduce a new compatible value.
> It all depends on how many options there are when instantiating such a
> controller.

...and the slaves will still need to set the word length they're trying
to use otherwise things might get corrupted.
Mark Brown Oct. 22, 2019, 3:20 p.m. UTC | #10
On Tue, Oct 22, 2019 at 02:06:11PM +0200, Alvaro Gamez Machado wrote:
> On Tue, Oct 22, 2019 at 12:26:00PM +0100, Mark Brown wrote:

> > This is still something that should be configured by the client driver,
> > if you send data with a different word size to that the client intends
> > it'll just get corrupted.

> The problem is that Xilinx's AXI Quad SPI core doesn't allow this. When
> instantiating the core you must choose *the* transfer width, not the
> *maximum* transfer width. So in my example above, no matter how I configure
> my IP core, linux'll believe that its datawidth is 8. I could override it
> hardcoding a 32 in spi-xilinx.c, but then what would happen with my other
> IP core that needs a datawidth of 8? Client code cannot configure IP core
> with a different datawidth because it simply does not allow it.

If the SPI controller can't cope with sending anything but 32 bits
that's fine, the slave still needs to know that it's supposed to be
handing the host controller data laid out in 32 bit words.  All the
components need to agree about how the data is supposed to be
interpreted.
Michal Simek Oct. 23, 2019, 5:37 a.m. UTC | #11
+Shubhrajyoti,

On 22. 10. 19 17:20, Mark Brown wrote:
> On Tue, Oct 22, 2019 at 02:06:11PM +0200, Alvaro Gamez Machado wrote:
>> On Tue, Oct 22, 2019 at 12:26:00PM +0100, Mark Brown wrote:
> 
>>> This is still something that should be configured by the client driver,
>>> if you send data with a different word size to that the client intends
>>> it'll just get corrupted.
> 
>> The problem is that Xilinx's AXI Quad SPI core doesn't allow this. When
>> instantiating the core you must choose *the* transfer width, not the
>> *maximum* transfer width. So in my example above, no matter how I configure
>> my IP core, linux'll believe that its datawidth is 8. I could override it
>> hardcoding a 32 in spi-xilinx.c, but then what would happen with my other
>> IP core that needs a datawidth of 8? Client code cannot configure IP core
>> with a different datawidth because it simply does not allow it.
> 
> If the SPI controller can't cope with sending anything but 32 bits
> that's fine, the slave still needs to know that it's supposed to be
> handing the host controller data laid out in 32 bit words.  All the
> components need to agree about how the data is supposed to be
> interpreted.

Shubhrajyoti: Can you please comment this thread?

Thanks,
Michal

Patch
diff mbox series

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 92ea22fedb33..46bb103ef30e 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -380,7 +380,7 @@  static int xilinx_spi_probe(struct platform_device *pdev)
 	struct xilinx_spi *xspi;
 	struct xspi_platform_data *pdata;
 	struct resource *res;
-	int ret, num_cs = 0, bits_per_word = 8;
+	int ret, num_cs = 0, bits_per_word;
 	struct spi_master *master;
 	u32 tmp;
 	u8 i;
@@ -392,6 +392,11 @@  static int xilinx_spi_probe(struct platform_device *pdev)
 	} else {
 		of_property_read_u32(pdev->dev.of_node, "xlnx,num-ss-bits",
 					  &num_cs);
+		ret = of_property_read_u32(pdev->dev.of_node,
+					   "xlnx,num-transfer-bits",
+					   &bits_per_word);
+		if (ret)
+			bits_per_word = 8;
 	}
 
 	if (!num_cs) {