| Message ID | 20180413161117.20274-3-radu.pirea@microchip.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Driver for AT91 USART in SPI mode | expand |
On 13/04/2018 19:11:16+0300, Radu Pirea wrote: > These are bindings for at91-usart IP in spi spi mode. There is no support for > internal chip select. Only kind of chip selects available are gpio chip > selects. > > Signed-off-by: Radu Pirea <radu.pirea@microchip.com> > --- > .../bindings/spi/microchip,at91-usart-spi.txt | 24 +++++++++++++++++++ > 1 file changed, 24 insertions(+) > create mode 100644 Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt > > diff --git a/Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt b/Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt > new file mode 100644 > index 000000000000..92d33ccdffae > --- /dev/null > +++ b/Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt > @@ -0,0 +1,24 @@ > +* Universal Synchronous Asynchronous Receiver/Transmitter (USART) in SPI mode > + > +Required properties: > +- #size-cells : Must be <0> > +- #address-cells : Must be <1> > +- compatible: Should be "microchip,at91sam9g45-usart-spi" or "microchip,sama5d2-usart-spi" > +- reg: Should contain registers location and length > +- interrupts: Should contain interrupt > +- clocks: phandles to input clocks. > +- clock-names: tuple listing input clock names. > + Required elements: "usart" > +- cs-gpios: chipselects (internal cs not supported) > + > +Example: > + spi0: spi@f001c000 { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "microchip,sama5d2-usart-spi", "microchip,at91sam9g45-usart-spi"; I'm pretty sure this will be considered configuration rather than hardware description. Why don't you do something like the flexcom mode selection? > + reg = <0xf001c000 0x100>; > + interrupts = <12 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&usart0_clk>; > + clock-names = "usart"; > + cs-gpios = <&pioB 3 0>; > + }; > -- > 2.17.0 >
On 13/04/2018 at 18:23, Alexandre Belloni wrote: > On 13/04/2018 19:11:16+0300, Radu Pirea wrote: >> These are bindings for at91-usart IP in spi spi mode. There is no support for >> internal chip select. Only kind of chip selects available are gpio chip >> selects. >> >> Signed-off-by: Radu Pirea <radu.pirea@microchip.com> >> --- >> .../bindings/spi/microchip,at91-usart-spi.txt | 24 +++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt >> >> diff --git a/Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt b/Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt >> new file mode 100644 >> index 000000000000..92d33ccdffae >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt >> @@ -0,0 +1,24 @@ >> +* Universal Synchronous Asynchronous Receiver/Transmitter (USART) in SPI mode >> + >> +Required properties: >> +- #size-cells : Must be <0> >> +- #address-cells : Must be <1> >> +- compatible: Should be "microchip,at91sam9g45-usart-spi" or "microchip,sama5d2-usart-spi" >> +- reg: Should contain registers location and length >> +- interrupts: Should contain interrupt >> +- clocks: phandles to input clocks. >> +- clock-names: tuple listing input clock names. >> + Required elements: "usart" >> +- cs-gpios: chipselects (internal cs not supported) >> + >> +Example: >> + spi0: spi@f001c000 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + compatible = "microchip,sama5d2-usart-spi", "microchip,at91sam9g45-usart-spi"; > > I'm pretty sure this will be considered configuration rather than > hardware description. Why don't you do something like the flexcom mode > selection? Because we are not in the same situation as having a glue layer that would select one of the already existing peripherals with associated drivers above. This layout of the hardware is completely different from the USART one and it seems to makes sense to address it with a different hardware description and so a different compatible string. Regards, Nicolas >> + reg = <0xf001c000 0x100>; >> + interrupts = <12 IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&usart0_clk>; >> + clock-names = "usart"; >> + cs-gpios = <&pioB 3 0>; >> + }; >> -- >> 2.17.0 >> >
On 13/04/2018 19:12:54+0200, Nicolas Ferre wrote: > > > diff --git a/Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt b/Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt > > > new file mode 100644 > > > index 000000000000..92d33ccdffae > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt > > > @@ -0,0 +1,24 @@ > > > +* Universal Synchronous Asynchronous Receiver/Transmitter (USART) in SPI mode > > > + > > > +Required properties: > > > +- #size-cells : Must be <0> > > > +- #address-cells : Must be <1> > > > +- compatible: Should be "microchip,at91sam9g45-usart-spi" or "microchip,sama5d2-usart-spi" > > > +- reg: Should contain registers location and length > > > +- interrupts: Should contain interrupt > > > +- clocks: phandles to input clocks. > > > +- clock-names: tuple listing input clock names. > > > + Required elements: "usart" > > > +- cs-gpios: chipselects (internal cs not supported) > > > + > > > +Example: > > > + spi0: spi@f001c000 { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + compatible = "microchip,sama5d2-usart-spi", "microchip,at91sam9g45-usart-spi"; > > > > I'm pretty sure this will be considered configuration rather than > > hardware description. Why don't you do something like the flexcom mode > > selection? > > Because we are not in the same situation as having a glue layer that would > select one of the already existing peripherals with associated drivers > above. > This layout of the hardware is completely different from the USART one and > it seems to makes sense to address it with a different hardware description > and so a different compatible string. > But then, you can end up with two drivers trying to use the same IP because nothing prevents you from writing a DT with both a usart and an spi node enabled for the same IP. request_mem_region() will not help here because then the working driver will depend on the probing order.
On Fri, Apr 13, 2018 at 08:12:51PM +0200, Alexandre Belloni wrote: > On 13/04/2018 19:12:54+0200, Nicolas Ferre wrote: > > This layout of the hardware is completely different from the USART one and > > it seems to makes sense to address it with a different hardware description > > and so a different compatible string. > But then, you can end up with two drivers trying to use the same IP > because nothing prevents you from writing a DT with both a usart and an > spi node enabled for the same IP. request_mem_region() will not help > here because then the working driver will depend on the probing order. We don't really have too much in the way of better ideas for how to handle this though. Take a look at how the PXA SSP stuff handles this, though that's not really doing too much different it at least layers a mechanism on top to avoid collisions.
On Tue, 2018-04-17 at 12:03 +0100, Mark Brown wrote: > On Fri, Apr 13, 2018 at 08:12:51PM +0200, Alexandre Belloni wrote: > > On 13/04/2018 19:12:54+0200, Nicolas Ferre wrote: > > > This layout of the hardware is completely different from the > > > USART one and > > > it seems to makes sense to address it with a different hardware > > > description > > > and so a different compatible string. > > But then, you can end up with two drivers trying to use the same IP > > because nothing prevents you from writing a DT with both a usart > > and an > > spi node enabled for the same IP. request_mem_region() will not > > help > > here because then the working driver will depend on the probing > > order. > > We don't really have too much in the way of better ideas for how to > handle this though. Take a look at how the PXA SSP stuff handles > this, > though that's not really doing too much different it at least layers > a > mechanism on top to avoid collisions. Hi Mark, Thank you for suggestions. I followed your advice and looked at PXA SSP driver. In my opinion it is a layer that avoids collsions and unfortunately complicates things a bit. My ideea is to keep the things as simple as possible. For example, I can enhance usart-serial and usart-spi drivers to print detailed messages if probe fails because one driver tries to request a memory region already used by another driver. What do you think? Is this approach a good way to move forward? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17/04/2018 12:03:58+0100, Mark Brown wrote: > On Fri, Apr 13, 2018 at 08:12:51PM +0200, Alexandre Belloni wrote: > > On 13/04/2018 19:12:54+0200, Nicolas Ferre wrote: > > > > This layout of the hardware is completely different from the USART one and > > > it seems to makes sense to address it with a different hardware description > > > and so a different compatible string. > > > But then, you can end up with two drivers trying to use the same IP > > because nothing prevents you from writing a DT with both a usart and an > > spi node enabled for the same IP. request_mem_region() will not help > > here because then the working driver will depend on the probing order. > > We don't really have too much in the way of better ideas for how to > handle this though. Take a look at how the PXA SSP stuff handles this, > though that's not really doing too much different it at least layers a > mechanism on top to avoid collisions. My suggestion was to add an MFD driver that would match the current compatible and either have an atmel,usart-mode property or maybe more risky, check whether there are children nodes. Based on that, the correct platform device can be added, either an usart or an spi master device can be registered.
On Thu, Apr 19, 2018 at 03:32:33PM +0200, Alexandre Belloni wrote: > My suggestion was to add an MFD driver that would match the current > compatible and either have an atmel,usart-mode property or maybe more > risky, check whether there are children nodes. Based on that, the > correct platform device can be added, either an usart or an spi master > device can be registered. Yeah, that's another approach which could work and is a bit easier from the DT point of view - the PXA stuff predated DT and was adapted into it.
On Thu, Apr 19, 2018 at 01:04:16PM +0300, Radu Pirea wrote: > Thank you for suggestions. I followed your advice and looked at PXA SSP > driver. In my opinion it is a layer that avoids collsions and > unfortunately complicates things a bit. My ideea is to keep the things > as simple as possible. For example, I can enhance usart-serial and > usart-spi drivers to print detailed messages if probe fails because one > driver tries to request a memory region already used by another driver. > What do you think? Is this approach a good way to move forward? Alexandre's suggestion using a MFD with a property to select the mode seems more solid TBH.
diff --git a/Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt b/Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt new file mode 100644 index 000000000000..92d33ccdffae --- /dev/null +++ b/Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt @@ -0,0 +1,24 @@ +* Universal Synchronous Asynchronous Receiver/Transmitter (USART) in SPI mode + +Required properties: +- #size-cells : Must be <0> +- #address-cells : Must be <1> +- compatible: Should be "microchip,at91sam9g45-usart-spi" or "microchip,sama5d2-usart-spi" +- reg: Should contain registers location and length +- interrupts: Should contain interrupt +- clocks: phandles to input clocks. +- clock-names: tuple listing input clock names. + Required elements: "usart" +- cs-gpios: chipselects (internal cs not supported) + +Example: + spi0: spi@f001c000 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "microchip,sama5d2-usart-spi", "microchip,at91sam9g45-usart-spi"; + reg = <0xf001c000 0x100>; + interrupts = <12 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&usart0_clk>; + clock-names = "usart"; + cs-gpios = <&pioB 3 0>; + };
These are bindings for at91-usart IP in spi spi mode. There is no support for internal chip select. Only kind of chip selects available are gpio chip selects. Signed-off-by: Radu Pirea <radu.pirea@microchip.com> --- .../bindings/spi/microchip,at91-usart-spi.txt | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt