| Message ID | 20170220153512.6563-1-adrian.fiergolski@cern.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 20, 2017 at 04:35:12PM +0100, Adrian Fiergolski wrote: > If an SPI controller doesn't support 8 bit transfers > (master->bits_per_word_mask), it will be never registered (tested with > spidev): > of_register_spi_device calls spi_add_device which calls spi_setup. The last > takes as an argument spi_device struct, which, in case of spidev, has > bits_per_word set to 0. Thus, the spi_setup function will set it to default > 8. Further, the same function will call __spi_validate_bits_per_word which > will fail the whole registration for controllers not supporting 8 bit > transfers (i.e. xilinx-spi). This doens't make much sense, if a device driver requires a given number of bits per word forcing a different value via device tree is not going to result in the driver actually working. If you want to use a device with a controller with restricted bits per word support you need to change the device driver to support the limited capabilities of the controller.
Hi Mark, I think, we misunderstand. Any line of spidev (or device driver in general) is called, if master, setting its bit_per_word_mask, doesn't cover 8 bits per word. Thus, in such a case, any device driver has a possibility to be registered and handle the "limited capabilities of the controller". IMHO, the problem is in the spi core itself. I will try to be more clear. In my case, xilinx_spi_probe function (of spi-xilinx controller) sets bits_per_word_mask of spi_master struct only to 16 bits support. Later, xilinx_spi_probe calls of_register_spi_devices, which calls of_register_spi_devices. The last one allocates an empty spi_device struct and configures different options of the spi_device according to a device tree. bits_per_word are not covered here (why?), thus it is left 0 (value after allocation), which, by convention, means 8 bits support. At the end, the same function (of_register_spi_device) calls spi_add_device which finally calls spi_setup. The last call, according to convention, changes bits_per_word to 8 and calls __spi_validate_bits_per_word which fails, as master doesn't support 8 bit transmission. This fails registration sequence of a device driver. As you see, the device driver doesn't have possibility to modify bits_per_word during the registration process, thus it can't provide support for such limited controllers. Regards, Adrian On 21.02.2017 at 20:08, Mark Brown wrote: > On Mon, Feb 20, 2017 at 04:35:12PM +0100, Adrian Fiergolski wrote: >> If an SPI controller doesn't support 8 bit transfers >> (master->bits_per_word_mask), it will be never registered (tested with >> spidev): >> of_register_spi_device calls spi_add_device which calls spi_setup. The last >> takes as an argument spi_device struct, which, in case of spidev, has >> bits_per_word set to 0. Thus, the spi_setup function will set it to default >> 8. Further, the same function will call __spi_validate_bits_per_word which >> will fail the whole registration for controllers not supporting 8 bit >> transfers (i.e. xilinx-spi). > This doens't make much sense, if a device driver requires a given number > of bits per word forcing a different value via device tree is not going > to result in the driver actually working. If you want to use a device > with a controller with restricted bits per word support you need to > change the device driver to support the limited capabilities of the > controller. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 13, 2017 at 06:25:53PM +0100, Adrian Fiergolski wrote: > Hi Mark, Please don't top post, reply in line with needed context. This allows readers to readily follow the flow of conversation and understand what you are talking about and also helps ensure that everything in the discussion is being addressed. > In my case, xilinx_spi_probe function (of spi-xilinx controller) sets > bits_per_word_mask of spi_master struct only to 16 bits support. Later, > xilinx_spi_probe calls of_register_spi_devices, which calls > of_register_spi_devices. The last one allocates an empty spi_device > struct and configures different options of the spi_device according to a > device tree. bits_per_word are not covered here (why?), thus it is left > 0 (value after allocation), which, by convention, means 8 bits support. > At the end, the same function (of_register_spi_device) calls > spi_add_device which finally calls spi_setup. The last call, according > to convention, changes bits_per_word to 8 and calls > __spi_validate_bits_per_word which fails, as master doesn't support 8 > bit transmission. This fails registration sequence of a device driver. > As you see, the device driver doesn't have possibility to modify > bits_per_word during the registration process, thus it can't provide > support for such limited controllers. I can't see any way in which it follows from the above that it's a good idea to try to override bits per word settings in the device tree, that just wastes user time and is an abstraction failure. We need better handling of defaults done purely in the kernel.
On 13.03.2017 at 18:55, Mark Brown wrote: > >> In my case, xilinx_spi_probe function (of spi-xilinx controller) sets >> bits_per_word_mask of spi_master struct only to 16 bits support. Later, >> xilinx_spi_probe calls of_register_spi_devices, which calls >> of_register_spi_devices. The last one allocates an empty spi_device >> struct and configures different options of the spi_device according to a >> device tree. bits_per_word are not covered here (why?), thus it is left >> 0 (value after allocation), which, by convention, means 8 bits support. >> At the end, the same function (of_register_spi_device) calls >> spi_add_device which finally calls spi_setup. The last call, according >> to convention, changes bits_per_word to 8 and calls >> __spi_validate_bits_per_word which fails, as master doesn't support 8 >> bit transmission. This fails registration sequence of a device driver. >> As you see, the device driver doesn't have possibility to modify >> bits_per_word during the registration process, thus it can't provide >> support for such limited controllers. > I can't see any way in which it follows from the above that it's a good > idea to try to override bits per word settings in the device tree, that > just wastes user time and is an abstraction failure. We need better > handling of defaults done purely in the kernel. If enforcing by device tree specific for a given device driver SPI_CPHA, SPIC_CPOL, SPI_CS_HIGH, max_speed_hz, etc. if fine form the abstraction point of view, why it doesn't apply to bits_per_word ? -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Adrian, On Mon, Mar 13, 2017 at 7:12 PM, Adrian Fiergolski <Adrian.Fiergolski@cern.ch> wrote: > On 13.03.2017 at 18:55, Mark Brown wrote: >>> In my case, xilinx_spi_probe function (of spi-xilinx controller) sets >>> bits_per_word_mask of spi_master struct only to 16 bits support. Later, >>> xilinx_spi_probe calls of_register_spi_devices, which calls >>> of_register_spi_devices. The last one allocates an empty spi_device >>> struct and configures different options of the spi_device according to a >>> device tree. bits_per_word are not covered here (why?), thus it is left >>> 0 (value after allocation), which, by convention, means 8 bits support. >>> At the end, the same function (of_register_spi_device) calls >>> spi_add_device which finally calls spi_setup. The last call, according >>> to convention, changes bits_per_word to 8 and calls >>> __spi_validate_bits_per_word which fails, as master doesn't support 8 >>> bit transmission. This fails registration sequence of a device driver. >>> As you see, the device driver doesn't have possibility to modify >>> bits_per_word during the registration process, thus it can't provide >>> support for such limited controllers. >> I can't see any way in which it follows from the above that it's a good >> idea to try to override bits per word settings in the device tree, that >> just wastes user time and is an abstraction failure. We need better >> handling of defaults done purely in the kernel. > If enforcing by device tree specific for a given device driver SPI_CPHA, > SPIC_CPOL, SPI_CS_HIGH, max_speed_hz, etc. if fine form the abstraction > point of view, why it doesn't apply to bits_per_word ? Because unlike polarity, phase, and speed, bits_per_word is a property of the communication protocol. E.g. you can talk to the same EEPROM using different polarities, phase, or speed, but bits_per_word is fixed. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On 13.03.2017 at 20:57, Geert Uytterhoeven wrote: > On Mon, Mar 13, 2017 at 7:12 PM, Adrian Fiergolski > <Adrian.Fiergolski@cern.ch> wrote: >> On 13.03.2017 at 18:55, Mark Brown wrote: >>>> In my case, xilinx_spi_probe function (of spi-xilinx controller) sets >>>> bits_per_word_mask of spi_master struct only to 16 bits support. Later, >>>> xilinx_spi_probe calls of_register_spi_devices, which calls >>>> of_register_spi_devices. The last one allocates an empty spi_device >>>> struct and configures different options of the spi_device according to a >>>> device tree. bits_per_word are not covered here (why?), thus it is left >>>> 0 (value after allocation), which, by convention, means 8 bits support. >>>> At the end, the same function (of_register_spi_device) calls >>>> spi_add_device which finally calls spi_setup. The last call, according >>>> to convention, changes bits_per_word to 8 and calls >>>> __spi_validate_bits_per_word which fails, as master doesn't support 8 >>>> bit transmission. This fails registration sequence of a device driver. >>>> As you see, the device driver doesn't have possibility to modify >>>> bits_per_word during the registration process, thus it can't provide >>>> support for such limited controllers. >>> I can't see any way in which it follows from the above that it's a good >>> idea to try to override bits per word settings in the device tree, that >>> just wastes user time and is an abstraction failure. We need better >>> handling of defaults done purely in the kernel. >> If enforcing by device tree specific for a given device driver SPI_CPHA, >> SPIC_CPOL, SPI_CS_HIGH, max_speed_hz, etc. if fine form the abstraction >> point of view, why it doesn't apply to bits_per_word ? > Because unlike polarity, phase, and speed, bits_per_word is a property > of the communication protocol. > > E.g. you can talk to the same EEPROM using different polarities, phase, or > speed, but bits_per_word is fixed. In this case, currently, what is the proper way to handle SPI controllers (spi-xilinx) without 8-bit transmission support ? -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 13, 2017 at 09:26:04PM +0100, Adrian Fiergolski wrote: > On 13.03.2017 at 20:57, Geert Uytterhoeven wrote: > > On Mon, Mar 13, 2017 at 7:12 PM, Adrian Fiergolski d > >>> I can't see any way in which it follows from the above that it's a good > >>> idea to try to override bits per word settings in the device tree, that > >>> just wastes user time and is an abstraction failure. We need better > >>> handling of defaults done purely in the kernel. > >> If enforcing by device tree specific for a given device driver SPI_CPHA, > >> SPIC_CPOL, SPI_CS_HIGH, max_speed_hz, etc. if fine form the abstraction > >> point of view, why it doesn't apply to bits_per_word ? > > Because unlike polarity, phase, and speed, bits_per_word is a property > > of the communication protocol. > > E.g. you can talk to the same EEPROM using different polarities, phase, or > > speed, but bits_per_word is fixed. > In this case, currently, what is the proper way to handle SPI > controllers (spi-xilinx) without 8-bit transmission support ? As I said above we should fix the handling of defaults such that it is possible to instantiate a 16 bit using device on a 16 bit supporting controller; there should be no need to have anything about device tree in this.
diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt index 4b1d6e7..8401741 100644 --- a/Documentation/devicetree/bindings/spi/spi-bus.txt +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt @@ -43,26 +43,28 @@ cs3 : &gpio1 2 0 SPI slave nodes must be children of the SPI master node and can contain the following properties. -- reg - (required) chip select address of device. -- compatible - (required) name of SPI device following generic names - recommended practice. +- reg - (required) chip select address of device. +- compatible - (required) name of SPI device following generic names + recommended practice. - spi-max-frequency - (required) Maximum SPI clocking speed of device in Hz. -- spi-cpol - (optional) Empty property indicating device requires - inverse clock polarity (CPOL) mode. -- spi-cpha - (optional) Empty property indicating device requires - shifted clock phase (CPHA) mode. -- spi-cs-high - (optional) Empty property indicating device requires - chip select active high. -- spi-3wire - (optional) Empty property indicating device requires - 3-wire mode. -- spi-lsb-first - (optional) Empty property indicating device requires - LSB first mode. -- spi-tx-bus-width - (optional) The bus width (number of data wires) that is - used for MOSI. Defaults to 1 if not present. -- spi-rx-bus-width - (optional) The bus width (number of data wires) that is - used for MISO. Defaults to 1 if not present. -- spi-rx-delay-us - (optional) Microsecond delay after a read transfer. -- spi-tx-delay-us - (optional) Microsecond delay after a write transfer. +- spi-cpol - (optional) Empty property indicating device requires + inverse clock polarity (CPOL) mode. +- spi-cpha - (optional) Empty property indicating device requires + shifted clock phase (CPHA) mode. +- spi-cs-high - (optional) Empty property indicating device requires + chip select active high. +- spi-3wire - (optional) Empty property indicating device requires + 3-wire mode. +- spi-lsb-first - (optional) Empty property indicating device requires + LSB first mode. +- spi-tx-bus-width - (optional) The bus width (number of data wires) that is + used for MOSI. Defaults to 1 if not present. +- spi-rx-bus-width - (optional) The bus width (number of data wires) that is + used for MISO. Defaults to 1 if not present. +- spi-rx-delay-us - (optional) Microsecond delay after a read transfer. +- spi-tx-delay-us - (optional) Microsecond delay after a write transfer. +- spi-bits-per-word - (optional) Word size for a data transfer. Defaults is 8 + if not present. Some SPI controllers and devices support Dual and Quad SPI transfer mode. It allows data in the SPI system to be transferred using 2 wires (DUAL) or 4 diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 656dd3e..57fe058 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1594,6 +1594,17 @@ of_register_spi_device(struct spi_master *master, struct device_node *nc) } spi->max_speed_hz = value; + /* Device bits-per-word */ + if (!of_property_read_u32(nc, "spi-bits-per-word", &value)) { + if (value > 32) + dev_warn(&master->dev, + "bits-per-word %d not supported\n", + value); + else + spi->bits_per_word = value; + } + + /* Store a pointer to the node in the device structure */ of_node_get(nc); spi->dev.of_node = nc; @@ -1673,6 +1684,13 @@ static int acpi_spi_add_resource(struct acpi_resource *ares, void *data) spi->max_speed_hz = sb->connection_speed; + if (sb->data_bit_length > 32) + dev_warn(&master->dev, + "bits-per-word %d not supported\n", + sb->data_bit_length); + else + spi->bits_per_word = sb->data_bit_length; + if (sb->clock_phase == ACPI_SPI_SECOND_PHASE) spi->mode |= SPI_CPHA; if (sb->clock_polarity == ACPI_SPI_START_HIGH)
If an SPI controller doesn't support 8 bit transfers (master->bits_per_word_mask), it will be never registered (tested with spidev): of_register_spi_device calls spi_add_device which calls spi_setup. The last takes as an argument spi_device struct, which, in case of spidev, has bits_per_word set to 0. Thus, the spi_setup function will set it to default 8. Further, the same function will call __spi_validate_bits_per_word which will fail the whole registration for controllers not supporting 8 bit transfers (i.e. xilinx-spi). Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch> --- Documentation/devicetree/bindings/spi/spi-bus.txt | 40 ++++++++++++----------- drivers/spi/spi.c | 18 ++++++++++ 2 files changed, 39 insertions(+), 19 deletions(-)