* [PATCH] Allowing Xilinx's AXI Quad widths different than 8 bits on userspace @ 2019-10-24 11:07 Alvaro Gamez Machado 2019-10-24 11:07 ` [PATCH] spi: xilinx: add description of new property xlnx,num-transfer-bits Alvaro Gamez Machado ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Alvaro Gamez Machado @ 2019-10-24 11:07 UTC (permalink / raw) To: Michal Simek, Mark Brown, Shubhrajyoti Datta, linux-spi, Rob Herring, devicetree Cc: Alvaro Gamez Machado Hi, We had a couple of days ago a nice discussion [1] about a patch that I sent that was in need of clarification. I've taken into consideration all the conversation and I believe this small series will manage my ultimate goal (being able to use from user space a 32 bits wordwidth SPI slave with Xilinx's AXI IP core) while being explained enough and through the proper procedure. I assume there may still need to be some discussion to go on with this, but I thought it'd be clearer if we all had the code upfront in its current state. First patch documents the new device tree property, while the second one implements it. The third patch, that could be applied on its own regardless of the first two, solves a bug that appears only in combination of spidev usage and a master SPI device that does not support 8 bits as datawidth. [1] I have not been able to find a link to the archives of linux-spi, sorry Thanks, Alvaro Gamez Machado (3): spi: xilinx: add description of new property xlnx,num-transfer-bits spi: xilinx: Add DT support for selecting transfer word width spi: set bits_per_word based on controller's bits_per_word_mask Documentation/devicetree/bindings/spi/spi-xilinx.txt | 4 +++- drivers/spi/spi-xilinx.c | 7 ++++++- drivers/spi/spi.c | 2 ++ 3 files changed, 11 insertions(+), 2 deletions(-) -- 2.23.0 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] spi: xilinx: add description of new property xlnx,num-transfer-bits 2019-10-24 11:07 [PATCH] Allowing Xilinx's AXI Quad widths different than 8 bits on userspace Alvaro Gamez Machado @ 2019-10-24 11:07 ` Alvaro Gamez Machado 2019-10-24 11:57 ` Mark Brown 2019-10-24 11:57 ` Applied "spi: xilinx: add description of new property xlnx,num-transfer-bits" to the spi tree Mark Brown 2019-10-24 11:07 ` [PATCH] spi: xilinx: Add DT support for selecting transfer word width Alvaro Gamez Machado 2019-10-24 11:07 ` [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask Alvaro Gamez Machado 2 siblings, 2 replies; 17+ messages in thread From: Alvaro Gamez Machado @ 2019-10-24 11:07 UTC (permalink / raw) To: Michal Simek, Mark Brown, Shubhrajyoti Datta, linux-spi, Rob Herring, devicetree Cc: Alvaro Gamez Machado This property is used to set the number of bits per transfer (bits_per_word). Xilinx' IP core allows either 8, 16 or 32, and is non changeable on runtime, only when instantiating the core. Signed-off-by: Alvaro Gamez Machado <alvaro.gamez@hazent.com> --- Documentation/devicetree/bindings/spi/spi-xilinx.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/spi/spi-xilinx.txt b/Documentation/devicetree/bindings/spi/spi-xilinx.txt index dc924a5f71db..5f4ed3e5c994 100644 --- a/Documentation/devicetree/bindings/spi/spi-xilinx.txt +++ b/Documentation/devicetree/bindings/spi/spi-xilinx.txt @@ -8,7 +8,8 @@ Required properties: number. Optional properties: -- xlnx,num-ss-bits : Number of chip selects used. +- xlnx,num-ss-bits : Number of chip selects used. +- xlnx,num-transfer-bits : Number of bits per transfer. This will be 8 if not specified Example: axi_quad_spi@41e00000 { @@ -17,5 +18,6 @@ Example: interrupts = <0 31 1>; reg = <0x41e00000 0x10000>; xlnx,num-ss-bits = <0x1>; + xlnx,num-transfer-bits = <32>; }; -- 2.23.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] spi: xilinx: add description of new property xlnx,num-transfer-bits 2019-10-24 11:07 ` [PATCH] spi: xilinx: add description of new property xlnx,num-transfer-bits Alvaro Gamez Machado @ 2019-10-24 11:57 ` Mark Brown 2019-10-24 11:57 ` Applied "spi: xilinx: add description of new property xlnx,num-transfer-bits" to the spi tree Mark Brown 1 sibling, 0 replies; 17+ messages in thread From: Mark Brown @ 2019-10-24 11:57 UTC (permalink / raw) To: Alvaro Gamez Machado Cc: Michal Simek, Shubhrajyoti Datta, linux-spi, Rob Herring, devicetree [-- Attachment #1: Type: text/plain, Size: 477 bytes --] On Thu, Oct 24, 2019 at 01:07:55PM +0200, Alvaro Gamez Machado wrote: > This property is used to set the number of bits per transfer (bits_per_word). > > Xilinx' IP core allows either 8, 16 or 32, and is non changeable on runtime, > only when instantiating the core. When sending a patch series you should number the patches within the series - if you use git format-patch to generate the series it'll do that for you. Instead of [PATCH] it should say [PATCH x/y]. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Applied "spi: xilinx: add description of new property xlnx,num-transfer-bits" to the spi tree 2019-10-24 11:07 ` [PATCH] spi: xilinx: add description of new property xlnx,num-transfer-bits Alvaro Gamez Machado 2019-10-24 11:57 ` Mark Brown @ 2019-10-24 11:57 ` Mark Brown 1 sibling, 0 replies; 17+ messages in thread From: Mark Brown @ 2019-10-24 11:57 UTC (permalink / raw) To: Alvaro Gamez Machado Cc: devicetree, linux-spi, Mark Brown, Michal Simek, Rob Herring, Shubhrajyoti Datta The patch spi: xilinx: add description of new property xlnx,num-transfer-bits has been applied to the spi tree at https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-5.5 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark From e3354b17b4ac10ad2c23e244444ab38927a69ee9 Mon Sep 17 00:00:00 2001 From: Alvaro Gamez Machado <alvaro.gamez@hazent.com> Date: Thu, 24 Oct 2019 13:07:55 +0200 Subject: [PATCH] spi: xilinx: add description of new property xlnx,num-transfer-bits This property is used to set the number of bits per transfer (bits_per_word). Xilinx' IP core allows either 8, 16 or 32, and is non changeable on runtime, only when instantiating the core. Signed-off-by: Alvaro Gamez Machado <alvaro.gamez@hazent.com> Link: https://lore.kernel.org/r/20191024110757.25820-2-alvaro.gamez@hazent.com Signed-off-by: Mark Brown <broonie@kernel.org> --- Documentation/devicetree/bindings/spi/spi-xilinx.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/spi/spi-xilinx.txt b/Documentation/devicetree/bindings/spi/spi-xilinx.txt index dc924a5f71db..5f4ed3e5c994 100644 --- a/Documentation/devicetree/bindings/spi/spi-xilinx.txt +++ b/Documentation/devicetree/bindings/spi/spi-xilinx.txt @@ -8,7 +8,8 @@ Required properties: number. Optional properties: -- xlnx,num-ss-bits : Number of chip selects used. +- xlnx,num-ss-bits : Number of chip selects used. +- xlnx,num-transfer-bits : Number of bits per transfer. This will be 8 if not specified Example: axi_quad_spi@41e00000 { @@ -17,5 +18,6 @@ Example: interrupts = <0 31 1>; reg = <0x41e00000 0x10000>; xlnx,num-ss-bits = <0x1>; + xlnx,num-transfer-bits = <32>; }; -- 2.20.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH] spi: xilinx: Add DT support for selecting transfer word width 2019-10-24 11:07 [PATCH] Allowing Xilinx's AXI Quad widths different than 8 bits on userspace Alvaro Gamez Machado 2019-10-24 11:07 ` [PATCH] spi: xilinx: add description of new property xlnx,num-transfer-bits Alvaro Gamez Machado @ 2019-10-24 11:07 ` Alvaro Gamez Machado 2019-10-24 11:57 ` Applied "spi: xilinx: Add DT support for selecting transfer word width" to the spi tree Mark Brown 2019-10-24 11:07 ` [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask Alvaro Gamez Machado 2 siblings, 1 reply; 17+ messages in thread From: Alvaro Gamez Machado @ 2019-10-24 11:07 UTC (permalink / raw) To: Michal Simek, Mark Brown, Shubhrajyoti Datta, linux-spi, Rob Herring, devicetree Cc: Alvaro Gamez Machado This core supports either 8, 16 or 32 bits as word width. This value is only settable on instantiation, and thus we need to support any of them by means of the device tree. Signed-off-by: Alvaro Gamez Machado <alvaro.gamez@hazent.com> --- drivers/spi/spi-xilinx.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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) { -- 2.23.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Applied "spi: xilinx: Add DT support for selecting transfer word width" to the spi tree 2019-10-24 11:07 ` [PATCH] spi: xilinx: Add DT support for selecting transfer word width Alvaro Gamez Machado @ 2019-10-24 11:57 ` Mark Brown 0 siblings, 0 replies; 17+ messages in thread From: Mark Brown @ 2019-10-24 11:57 UTC (permalink / raw) To: Alvaro Gamez Machado Cc: devicetree, linux-spi, Mark Brown, Michal Simek, Rob Herring, Shubhrajyoti Datta The patch spi: xilinx: Add DT support for selecting transfer word width has been applied to the spi tree at https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-5.5 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark From e58f7d15e6beb255b3907054a0536db77c979a31 Mon Sep 17 00:00:00 2001 From: Alvaro Gamez Machado <alvaro.gamez@hazent.com> Date: Thu, 24 Oct 2019 13:07:56 +0200 Subject: [PATCH] spi: xilinx: Add DT support for selecting transfer word width This core supports either 8, 16 or 32 bits as word width. This value is only settable on instantiation, and thus we need to support any of them by means of the device tree. Signed-off-by: Alvaro Gamez Machado <alvaro.gamez@hazent.com> Link: https://lore.kernel.org/r/20191024110757.25820-3-alvaro.gamez@hazent.com Signed-off-by: Mark Brown <broonie@kernel.org> --- drivers/spi/spi-xilinx.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c index d5f9d5fbb3e8..8dd2bb99cb4d 100644 --- a/drivers/spi/spi-xilinx.c +++ b/drivers/spi/spi-xilinx.c @@ -391,7 +391,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; @@ -403,6 +403,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) { -- 2.20.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask 2019-10-24 11:07 [PATCH] Allowing Xilinx's AXI Quad widths different than 8 bits on userspace Alvaro Gamez Machado 2019-10-24 11:07 ` [PATCH] spi: xilinx: add description of new property xlnx,num-transfer-bits Alvaro Gamez Machado 2019-10-24 11:07 ` [PATCH] spi: xilinx: Add DT support for selecting transfer word width Alvaro Gamez Machado @ 2019-10-24 11:07 ` Alvaro Gamez Machado 2019-10-24 11:13 ` Mark Brown 2 siblings, 1 reply; 17+ messages in thread From: Alvaro Gamez Machado @ 2019-10-24 11:07 UTC (permalink / raw) To: Michal Simek, Mark Brown, Shubhrajyoti Datta, linux-spi, Rob Herring, devicetree Cc: Alvaro Gamez Machado By leaving this value unset, a default value of 8 was being set later on. If it happens that the SPI master driver doesn't support this value of 8, there will be an initial inconsistency between the SPI master and the device itself. This isn't a problem for most devices because kernel drivers associated to any device set the correct value themselves, but for device drivers that do not change this value (such as spidev that provides a userspace accesible device, which doesn't and can't know the value it has to use), an error is raised: 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 When this happens, the expected /dev/spidevX.Y device isn't created, and thus the user can't use the SPI_IOC_WR_BITS_PER_WORD ioctl to set the desired value. This change sets the initial value of bits_per to the smallest word that the controller allows, so of_spi_parse_dt sets a value that is usable by the controller. Signed-off-by: Alvaro Gamez Machado <alvaro.gamez@hazent.com> --- drivers/spi/spi.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index f9502dbbb5c1..794e20e54237 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; } -- 2.23.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask 2019-10-24 11:07 ` [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask Alvaro Gamez Machado @ 2019-10-24 11:13 ` Mark Brown 2019-10-24 12:54 ` Alvaro Gamez Machado 0 siblings, 1 reply; 17+ messages in thread From: Mark Brown @ 2019-10-24 11:13 UTC (permalink / raw) To: Alvaro Gamez Machado Cc: Michal Simek, Shubhrajyoti Datta, linux-spi, Rob Herring, devicetree [-- Attachment #1: Type: text/plain, Size: 683 bytes --] On Thu, Oct 24, 2019 at 01:07:57PM +0200, Alvaro Gamez Machado wrote: > By leaving this value unset, a default value of 8 was being set later on. > > If it happens that the SPI master driver doesn't support this value of 8, > there will be an initial inconsistency between the SPI master and the device > itself. This isn't a problem for most devices because kernel drivers This will break things, client devices are working on the basis that the default transfer width is 8 bits. As I've repeatedly said if we have different parts of the system with different ideas about the word size we're going to end up with data corruption. Please take this feedback on board. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask 2019-10-24 11:13 ` Mark Brown @ 2019-10-24 12:54 ` Alvaro Gamez Machado 2019-10-24 13:11 ` Mark Brown 0 siblings, 1 reply; 17+ messages in thread From: Alvaro Gamez Machado @ 2019-10-24 12:54 UTC (permalink / raw) To: Mark Brown Cc: Michal Simek, Shubhrajyoti Datta, linux-spi, Rob Herring, devicetree On Thu, Oct 24, 2019 at 12:13:00PM +0100, Mark Brown wrote: > On Thu, Oct 24, 2019 at 01:07:57PM +0200, Alvaro Gamez Machado wrote: > > By leaving this value unset, a default value of 8 was being set later on. > > > > If it happens that the SPI master driver doesn't support this value of 8, > > there will be an initial inconsistency between the SPI master and the device > > itself. This isn't a problem for most devices because kernel drivers > > This will break things, client devices are working on the basis that the > default transfer width is 8 bits. As I've repeatedly said if we have > different parts of the system with different ideas about the word size > we're going to end up with data corruption. Please take this feedback > on board. Oh, ok. I didn't understand this cleary from previous mails, now I see what you mean. I think then the only way this would be feasible is to check if 8 bits is an acceptable number for the master and, if it isn't, apply the lowest available data width. I believe this cannot break anything, as it leaves 8 as the default unless the master can't work with that number, in which case it really doesn't matter what client device wants because the hardware can't provide it. Thanks! diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 794e20e54237..4e26ac79e133 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -3079,8 +3079,12 @@ int spi_setup(struct spi_device *spi) return -EINVAL; } - if (!spi->bits_per_word) - spi->bits_per_word = 8; + if (!spi->bits_per_word) { + if (spi->controller->bits_per_word_mask & SPI_BPW_MASK(8)) + spi->bits_per_word = 8; + else + spi->bits_per_word = ffs(spi->controller->bits_per_word_mask); + } status = __spi_validate_bits_per_word(spi->controller, spi->bits_per_word); -- Alvaro G. M. ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask 2019-10-24 12:54 ` Alvaro Gamez Machado @ 2019-10-24 13:11 ` Mark Brown 2019-10-24 13:18 ` Alvaro Gamez Machado 0 siblings, 1 reply; 17+ messages in thread From: Mark Brown @ 2019-10-24 13:11 UTC (permalink / raw) To: Alvaro Gamez Machado Cc: Michal Simek, Shubhrajyoti Datta, linux-spi, Rob Herring, devicetree [-- Attachment #1: Type: text/plain, Size: 770 bytes --] On Thu, Oct 24, 2019 at 02:54:37PM +0200, Alvaro Gamez Machado wrote: > I think then the only way this would be feasible is to check if 8 bits is an > acceptable number for the master and, if it isn't, apply the lowest > available data width. I believe this cannot break anything, as it leaves 8 > as the default unless the master can't work with that number, in which case > it really doesn't matter what client device wants because the hardware can't > provide it. No, that still leaves the slave driver thinking it's sending 8 bits when really it's sending something else - the default is just 8 bits, if the controller can't do it then the transfer can't happen and there's an error. It's not a good idea to carry on if we're likely to introduce data corruption. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask 2019-10-24 13:11 ` Mark Brown @ 2019-10-24 13:18 ` Alvaro Gamez Machado 2019-10-24 13:41 ` Mark Brown 0 siblings, 1 reply; 17+ messages in thread From: Alvaro Gamez Machado @ 2019-10-24 13:18 UTC (permalink / raw) To: Mark Brown Cc: Michal Simek, Shubhrajyoti Datta, linux-spi, Rob Herring, devicetree On Thu, Oct 24, 2019 at 02:11:29PM +0100, Mark Brown wrote: > On Thu, Oct 24, 2019 at 02:54:37PM +0200, Alvaro Gamez Machado wrote: > > > I think then the only way this would be feasible is to check if 8 bits is an > > acceptable number for the master and, if it isn't, apply the lowest > > available data width. I believe this cannot break anything, as it leaves 8 > > as the default unless the master can't work with that number, in which case > > it really doesn't matter what client device wants because the hardware can't > > provide it. > > No, that still leaves the slave driver thinking it's sending 8 bits when > really it's sending something else - the default is just 8 bits, if the > controller can't do it then the transfer can't happen and there's an > error. It's not a good idea to carry on if we're likely to introduce > data corruption. Well, yes. But I don't think that's a software issue but a hardware one. If you have a board that has a SPI master that cannot talk to an 8 bits device and you expect to communicate with anything that accepts 8 bits you're not going to be able to. Either the kernel raises an error or it shuts up and tries its best. I understand the first option is better, but I also think that's not a software issue, that hardware simply cannot work as is regardless of what we do in software. The hardware devices simply can't talk to each other. -- Alvaro G. M. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask 2019-10-24 13:18 ` Alvaro Gamez Machado @ 2019-10-24 13:41 ` Mark Brown 2019-10-24 14:07 ` Alvaro Gamez Machado 0 siblings, 1 reply; 17+ messages in thread From: Mark Brown @ 2019-10-24 13:41 UTC (permalink / raw) To: Alvaro Gamez Machado Cc: Michal Simek, Shubhrajyoti Datta, linux-spi, Rob Herring, devicetree [-- Attachment #1: Type: text/plain, Size: 1634 bytes --] On Thu, Oct 24, 2019 at 03:18:57PM +0200, Alvaro Gamez Machado wrote: > On Thu, Oct 24, 2019 at 02:11:29PM +0100, Mark Brown wrote: > > No, that still leaves the slave driver thinking it's sending 8 bits when > > really it's sending something else - the default is just 8 bits, if the > > controller can't do it then the transfer can't happen and there's an > > error. It's not a good idea to carry on if we're likely to introduce > > data corruption. > Well, yes. But I don't think that's a software issue but a hardware one. > If you have a board that has a SPI master that cannot talk to an 8 bits > device and you expect to communicate with anything that accepts 8 bits > you're not going to be able to. Either the kernel raises an error or it > shuts up and tries its best. I understand the first option is better, but I > also think that's not a software issue, that hardware simply cannot work as > is regardless of what we do in software. The hardware devices simply can't > talk to each other. Sure, but then it's going to be easier to diagnose problems if the software says that it's identified a data format problem than it is to try to figure out the results of data corruption. There is also the possibility that if the formats the hardware needs to use can be made to align through rewriting software can persuade things to interoperate (eg, if all the transfers are multiples of 32 bits then a device can probably work with a controller that only supports 32 bit words even if the device expects a byte stream) but that requires explicit code rather than just silently accepting the data and hoping for the best. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask 2019-10-24 13:41 ` Mark Brown @ 2019-10-24 14:07 ` Alvaro Gamez Machado 2019-10-24 17:40 ` Mark Brown 0 siblings, 1 reply; 17+ messages in thread From: Alvaro Gamez Machado @ 2019-10-24 14:07 UTC (permalink / raw) To: Mark Brown Cc: Michal Simek, Shubhrajyoti Datta, linux-spi, Rob Herring, devicetree On Thu, Oct 24, 2019 at 02:41:16PM +0100, Mark Brown wrote: > On Thu, Oct 24, 2019 at 03:18:57PM +0200, Alvaro Gamez Machado wrote: > > On Thu, Oct 24, 2019 at 02:11:29PM +0100, Mark Brown wrote: > > > > No, that still leaves the slave driver thinking it's sending 8 bits when > > > really it's sending something else - the default is just 8 bits, if the > > > controller can't do it then the transfer can't happen and there's an > > > error. It's not a good idea to carry on if we're likely to introduce > > > data corruption. > > > Well, yes. But I don't think that's a software issue but a hardware one. > > > If you have a board that has a SPI master that cannot talk to an 8 bits > > device and you expect to communicate with anything that accepts 8 bits > > you're not going to be able to. Either the kernel raises an error or it > > shuts up and tries its best. I understand the first option is better, but I > > also think that's not a software issue, that hardware simply cannot work as > > is regardless of what we do in software. The hardware devices simply can't > > talk to each other. > > Sure, but then it's going to be easier to diagnose problems if the > software says that it's identified a data format problem than it is to > try to figure out the results of data corruption. There is also the > possibility that if the formats the hardware needs to use can be made to > align through rewriting software can persuade things to interoperate > (eg, if all the transfers are multiples of 32 bits then a device can > probably work with a controller that only supports 32 bit words even if > the device expects a byte stream) but that requires explicit code rather > than just silently accepting the data and hoping for the best. I guess there could be some workarounds to help in that situation. But I see that as an hypothetical occurrence whereas I have with me a physical board that needs 32 bits in both master and slave that I want to access using spidev and cannot. Lots of I's in that sentence, I know :) Anyhow, if this is not acceptable, the only other alternative I see right now is adding a new DT property, as in my emails yesterday. 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; } What do you think about this? This requires the user to explicitly select a different value rather than the default, so it can't break anything and would allow with the diagnostic of such broken hardware. I still think I like more changing the default to something the master is able to do. Otherwise we're going to keep trying to send 8 bits using a master that simply cannot do that, but this solution would work fine as well. Regards, -- Alvaro G. M. ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask 2019-10-24 14:07 ` Alvaro Gamez Machado @ 2019-10-24 17:40 ` Mark Brown 2019-10-25 6:39 ` Alvaro Gamez Machado 0 siblings, 1 reply; 17+ messages in thread From: Mark Brown @ 2019-10-24 17:40 UTC (permalink / raw) To: Alvaro Gamez Machado Cc: Michal Simek, Shubhrajyoti Datta, linux-spi, Rob Herring, devicetree [-- Attachment #1: Type: text/plain, Size: 506 bytes --] On Thu, Oct 24, 2019 at 04:07:32PM +0200, Alvaro Gamez Machado wrote: > I guess there could be some workarounds to help in that situation. But I see > that as an hypothetical occurrence whereas I have with me a physical board > that needs 32 bits in both master and slave that I want to access using > spidev and cannot. Lots of I's in that sentence, I know :) If you want to access this using spidev then add support for changing the word size to spidev and use that as I think Geert already suggested. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask 2019-10-24 17:40 ` Mark Brown @ 2019-10-25 6:39 ` Alvaro Gamez Machado 2019-10-25 11:56 ` Mark Brown 0 siblings, 1 reply; 17+ messages in thread From: Alvaro Gamez Machado @ 2019-10-25 6:39 UTC (permalink / raw) To: Mark Brown Cc: Michal Simek, Shubhrajyoti Datta, linux-spi, Rob Herring, devicetree On Thu, Oct 24, 2019 at 06:40:33PM +0100, Mark Brown wrote: > On Thu, Oct 24, 2019 at 04:07:32PM +0200, Alvaro Gamez Machado wrote: > > > I guess there could be some workarounds to help in that situation. But I see > > that as an hypothetical occurrence whereas I have with me a physical board > > that needs 32 bits in both master and slave that I want to access using > > spidev and cannot. Lots of I's in that sentence, I know :) > > If you want to access this using spidev then add support for changing > the word size to spidev and use that as I think Geert already suggested. I've been trying to do so for a couple hours and I've reached a conclusion. I've been too focused on my personal use case (too many I's indeed) and thought that the problem was in fact in spidev as Geert indicated, but now I think it isn't, so I must present my excuses for mistakenly drive the conversation in that direction. Geert also thought this could be an SPI core bug, and he was right on that, I think. In fact, not a single line of spidev is being executed when the error message is printed: xilinx_spi 44a00000.spi: at 0x44A00000 mapped to 0x(ptrval), irq=3 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 This does not come from spidev but directly from spi. What is happening is that when SPI slaves are defined via DT, their bits_per_word value is always unset (as 0), which turns later on in a default value of 8. Inside spi_setup function immediately after setting this default value to 8, __spi_validate_bits_per_word is called, that checks whether bits_per_word for the slave matches the controller available bitwidths: if (!spi->bits_per_word) spi->bits_per_word = 8; status = __spi_validate_bits_per_word(spi->controller, spi->bits_per_word); if (status) return status; This means that it doesn't really matter which is the driver that is going to claim the specific SPI slave. It may be spidev as in my use case, or it may really be any other driver. But its probe() function is never going to be called because the error is not raised inside the driver, but immediately after forcibly setting the default value to 8 in spi.c I can't modify spidev because spidev doesn't even know this is happening. I was completely wrong in my blaming of spidev, but now I'm reassured that my previous patches were going to core of the issue, regardless of my mistaken initial diagnostic. Thanks, -- Alvaro G. M. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask 2019-10-25 6:39 ` Alvaro Gamez Machado @ 2019-10-25 11:56 ` Mark Brown 2019-10-28 9:43 ` Alvaro Gamez Machado 0 siblings, 1 reply; 17+ messages in thread From: Mark Brown @ 2019-10-25 11:56 UTC (permalink / raw) To: Alvaro Gamez Machado Cc: Michal Simek, Shubhrajyoti Datta, linux-spi, Rob Herring, devicetree [-- Attachment #1: Type: text/plain, Size: 805 bytes --] On Fri, Oct 25, 2019 at 08:39:48AM +0200, Alvaro Gamez Machado wrote: > to claim the specific SPI slave. It may be spidev as in my use case, or it > may really be any other driver. But its probe() function is never going to > be called because the error is not raised inside the driver, but immediately > after forcibly setting the default value to 8 in spi.c Then you need to extend the validation the core is doing here to skip this parameter when registering the device and only enforce it after a driver is bound, we don't have a driver at the time we initially register the device so we can't enforce this. > I can't modify spidev because spidev doesn't even know this is happening. You are, at some point, going to need to set your spidev to 32 bits per word (spidev does already support this). [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask 2019-10-25 11:56 ` Mark Brown @ 2019-10-28 9:43 ` Alvaro Gamez Machado 0 siblings, 0 replies; 17+ messages in thread From: Alvaro Gamez Machado @ 2019-10-28 9:43 UTC (permalink / raw) To: Mark Brown Cc: Michal Simek, Shubhrajyoti Datta, linux-spi, Rob Herring, devicetree On Fri, Oct 25, 2019 at 12:56:55PM +0100, Mark Brown wrote: > On Fri, Oct 25, 2019 at 08:39:48AM +0200, Alvaro Gamez Machado wrote: > > > to claim the specific SPI slave. It may be spidev as in my use case, or it > > may really be any other driver. But its probe() function is never going to > > be called because the error is not raised inside the driver, but immediately > > after forcibly setting the default value to 8 in spi.c > > Then you need to extend the validation the core is doing here to > skip this parameter when registering the device and only enforce > it after a driver is bound, we don't have a driver at the time we > initially register the device so we can't enforce this. Ok, so I can remove the call to __spi_validate_bits_per_word() from spi_setup(), and instead place it on spi_drv_probe after calling spi_drv_probe() and detach the device if it returns with an error. The behaviour would be basically the same as it is now, but the slave driver will have an opportunity to choose a different transfer width if it wants to and can match the capabilities of the master. > > > I can't modify spidev because spidev doesn't even know this is happening. > > You are, at some point, going to need to set your spidev to 32 > bits per word (spidev does already support this). If I do the former, spidev will then need to load the initial bits per word value from DT before returning from probe(), otherwise it will end up detached when the check above is performed. Should I go for this? Thanks -- Alvaro G. M. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-10-28 9:43 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-24 11:07 [PATCH] Allowing Xilinx's AXI Quad widths different than 8 bits on userspace Alvaro Gamez Machado 2019-10-24 11:07 ` [PATCH] spi: xilinx: add description of new property xlnx,num-transfer-bits Alvaro Gamez Machado 2019-10-24 11:57 ` Mark Brown 2019-10-24 11:57 ` Applied "spi: xilinx: add description of new property xlnx,num-transfer-bits" to the spi tree Mark Brown 2019-10-24 11:07 ` [PATCH] spi: xilinx: Add DT support for selecting transfer word width Alvaro Gamez Machado 2019-10-24 11:57 ` Applied "spi: xilinx: Add DT support for selecting transfer word width" to the spi tree Mark Brown 2019-10-24 11:07 ` [PATCH] spi: set bits_per_word based on controller's bits_per_word_mask Alvaro Gamez Machado 2019-10-24 11:13 ` Mark Brown 2019-10-24 12:54 ` Alvaro Gamez Machado 2019-10-24 13:11 ` Mark Brown 2019-10-24 13:18 ` Alvaro Gamez Machado 2019-10-24 13:41 ` Mark Brown 2019-10-24 14:07 ` Alvaro Gamez Machado 2019-10-24 17:40 ` Mark Brown 2019-10-25 6:39 ` Alvaro Gamez Machado 2019-10-25 11:56 ` Mark Brown 2019-10-28 9:43 ` Alvaro Gamez Machado
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.