| Message ID | 64681bf903104c8a02f118294e616e2a12a5ebe4.1533638405.git.baolin.wang@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] dt-bindings: spi: Add Spreadtrum SPI controller documentation | expand |
On Tue, Aug 07, 2018 at 06:43:37PM +0800, Baolin Wang wrote: > +Optional properties: > +- sprd,spi-interval: Specify the intervals of two SPI frames, which can be > + converted to the delay clock cycles = interval number * 4 + 10. What's a frame here, and does it overlap with any of the existing delay configuration we have? In general it's better for this stuff to be configured at runtime by the device rather than at DT time by the controller since that way if the device needs the delays we always do them if we can and if they are only needed some of the time (eg, for only one device on the bus or for only some operations) then we don't take the performance hit when we don't need to.
Hi Mark, On 7 August 2018 at 21:41, Mark Brown <broonie@kernel.org> wrote: > On Tue, Aug 07, 2018 at 06:43:37PM +0800, Baolin Wang wrote: > >> +Optional properties: >> +- sprd,spi-interval: Specify the intervals of two SPI frames, which can be >> + converted to the delay clock cycles = interval number * 4 + 10. > > What's a frame here, and does it overlap with any of the existing delay > configuration we have? In general it's better for this stuff to be > configured at runtime by the device rather than at DT time by the > controller since that way if the device needs the delays we always do > them if we can and if they are only needed some of the time (eg, for > only one device on the bus or for only some operations) then we don't > take the performance hit when we don't need to. Sorry for confusing. Let me try to explain it explicitly. We can set the word size (bits_per_word) for each transmission, for our SPI controller, after every word size transmission, we need one interval time (hardware automatically) to make sure the slave has enough time to receive the whole data. Yes, I agree we should configure it at runtime by the device, but we did not find one member to use in 'struct spi_transfer', we just find one similar 'delay_usecs' member in 'struct spi_transfer' but not same. We can use 'delay_usecs' to set our hardware interval value, but we should clean it when transfer is done, since we do not need to delay after the transfer in spi_transfer_one _message(). Or can we add one new member maybe named 'word_interval' to indicate the interval time between word size transmission? What do you prefer or other better solution? Thanks.
On Wed, Aug 08, 2018 at 10:26:42AM +0800, Baolin Wang wrote: > Sorry for confusing. Let me try to explain it explicitly. > We can set the word size (bits_per_word) for each transmission, for > our SPI controller, after every word size transmission, we need one > interval time (hardware automatically) to make sure the slave has > enough time to receive the whole data. OK, so it's an inter word delay. Some other controllers definitely have the same feature. > Yes, I agree we should configure it at runtime by the device, but we > did not find one member to use in 'struct spi_transfer', we just find > one similar 'delay_usecs' member in 'struct spi_transfer' but not > same. We can use 'delay_usecs' to set our hardware interval value, > but we should clean it when transfer is done, since we do not need to > delay after the transfer in spi_transfer_one _message(). Or can we add > one new member maybe named 'word_interval' to indicate the interval > time between word size transmission? Right, I don't think we added this yet (if we did I can't see it). I'd add a new field to spi_transfer for this, then other controllers with the same support can implement it as well and drivers can start using it too.
On 8 August 2018 at 17:50, Mark Brown <broonie@kernel.org> wrote: > On Wed, Aug 08, 2018 at 10:26:42AM +0800, Baolin Wang wrote: > >> Sorry for confusing. Let me try to explain it explicitly. >> We can set the word size (bits_per_word) for each transmission, for >> our SPI controller, after every word size transmission, we need one >> interval time (hardware automatically) to make sure the slave has >> enough time to receive the whole data. > > OK, so it's an inter word delay. Some other controllers definitely have > the same feature. > >> Yes, I agree we should configure it at runtime by the device, but we >> did not find one member to use in 'struct spi_transfer', we just find >> one similar 'delay_usecs' member in 'struct spi_transfer' but not >> same. We can use 'delay_usecs' to set our hardware interval value, >> but we should clean it when transfer is done, since we do not need to >> delay after the transfer in spi_transfer_one _message(). Or can we add >> one new member maybe named 'word_interval' to indicate the interval >> time between word size transmission? > > Right, I don't think we added this yet (if we did I can't see it). I'd > add a new field to spi_transfer for this, then other controllers with > the same support can implement it as well and drivers can start using > it too. OK. So I will name the new filed as 'word_delay', is it OK for you?
On Wed, Aug 08, 2018 at 06:35:28PM +0800, Baolin Wang wrote: > On 8 August 2018 at 17:50, Mark Brown <broonie@kernel.org> wrote: > > Right, I don't think we added this yet (if we did I can't see it). I'd > > add a new field to spi_transfer for this, then other controllers with > > the same support can implement it as well and drivers can start using > > it too. > OK. So I will name the new filed as 'word_delay', is it OK for you? Sounds good, yes.
On 8 August 2018 at 18:54, Mark Brown <broonie@kernel.org> wrote: > On Wed, Aug 08, 2018 at 06:35:28PM +0800, Baolin Wang wrote: >> On 8 August 2018 at 17:50, Mark Brown <broonie@kernel.org> wrote: > >> > Right, I don't think we added this yet (if we did I can't see it). I'd >> > add a new field to spi_transfer for this, then other controllers with >> > the same support can implement it as well and drivers can start using >> > it too. > >> OK. So I will name the new filed as 'word_delay', is it OK for you? > > Sounds good, yes. OK. Thanks.
Hi Trent, On 9 August 2018 at 02:57, Trent Piepho <tpiepho@impinj.com> wrote: > On Wed, 2018-08-08 at 11:54 +0100, Mark Brown wrote: >> On Wed, Aug 08, 2018 at 06:35:28PM +0800, Baolin Wang wrote: >> > On 8 August 2018 at 17:50, Mark Brown <broonie@kernel.org> wrote: >> > > Right, I don't think we added this yet (if we did I can't see >> > > it). I'd >> > > add a new field to spi_transfer for this, then other controllers >> > > with >> > > the same support can implement it as well and drivers can start >> > > using >> > > it too. >> > OK. So I will name the new filed as 'word_delay', is it OK for you? >> >> Sounds good, yes. > > Should it be in µs like the existing iter-transfer delay? I think > perhaps units of cycles of the SPI clock make more sense? Since some SPI controllers just want some interval values (neither µs unit nor cycles unit ) set into hardware, and the hardware will convert to the correct delay time automatically. So I did not force 'word_delay' unit as µs or cycle, and just let the slave devices decide the unit which depends on the SPI hardware requirement.
On Tue, Aug 07, 2018 at 06:43:37PM +0800, Baolin Wang wrote: > From: Lanqing Liu <lanqing.liu@spreadtrum.com> > > This patch adds the binding documentation for Spreadtrum SPI > controller device. > > Signed-off-by: Lanqing Liu <lanqing.liu@spreadtrum.com> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > --- > Documentation/devicetree/bindings/spi/spi-sprd.txt | 31 ++++++++++++++++++++ > 1 file changed, 31 insertions(+) > create mode 100644 Documentation/devicetree/bindings/spi/spi-sprd.txt > > diff --git a/Documentation/devicetree/bindings/spi/spi-sprd.txt b/Documentation/devicetree/bindings/spi/spi-sprd.txt > new file mode 100644 > index 0000000..06ff746 > --- /dev/null > +++ b/Documentation/devicetree/bindings/spi/spi-sprd.txt > @@ -0,0 +1,31 @@ > +Spreadtrum SPI Controller > + > +Required properties: > +- compatible: Should be "sprd,sc9860-spi". > +- reg: Offset and length of SPI controller register space. > +- interrupts: Should contain SPI interrupt. > +- clock-names: Should contain following entries: > + "spi" for SPI clock, > + "source" for SPI source (parent) clock, Do the h/w block actually get this clock or the driver needs it? In the latter case, it should not be in DT. > + "enable" for SPI module enable clock. > +- clocks: List of clock input name strings sorted in the same order > + as the clock-names property. > +- #address-cells: The number of cells required to define a chip select > + address on the SPI bus. Should be set to 1. > +- #size-cells: Should be set to 0. > + > +Optional properties: > +- sprd,spi-interval: Specify the intervals of two SPI frames, which can be > + converted to the delay clock cycles = interval number * 4 + 10. There are read and write delay properties you can use. > + > +Example: > +spi0: spi@70a00000{ > + compatible = "sprd,sc9860-spi"; > + reg = <0 0x70a00000 0 0x1000>; > + interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; > + clock-names = "spi", "source","enable"; > + clocks = <&clk_spi0>, <&ext_26m>, <&clk_ap_apb_gates 5>; > + sprd,spi-interval = <9>; > + #address-cells = <1>; > + #size-cells = <0>; > +}; > -- > 1.7.9.5 >
On Thu, Aug 09, 2018 at 11:03:11AM +0800, Baolin Wang wrote: > Hi Trent, > > On 9 August 2018 at 02:57, Trent Piepho <tpiepho@impinj.com> wrote: > > On Wed, 2018-08-08 at 11:54 +0100, Mark Brown wrote: > >> On Wed, Aug 08, 2018 at 06:35:28PM +0800, Baolin Wang wrote: > >> > On 8 August 2018 at 17:50, Mark Brown <broonie@kernel.org> wrote: > >> > > Right, I don't think we added this yet (if we did I can't see > >> > > it). I'd > >> > > add a new field to spi_transfer for this, then other controllers > >> > > with > >> > > the same support can implement it as well and drivers can start > >> > > using > >> > > it too. > >> > OK. So I will name the new filed as 'word_delay', is it OK for you? > >> > >> Sounds good, yes. > > > > Should it be in µs like the existing iter-transfer delay? I think > > perhaps units of cycles of the SPI clock make more sense? > > Since some SPI controllers just want some interval values (neither µs > unit nor cycles unit ) set into hardware, and the hardware will > convert to the correct delay time automatically. So I did not force > 'word_delay' unit as µs or cycle, and just let the slave devices > decide the unit which depends on the SPI hardware requirement. This needs to be defined units in DT, not decided by each controller. The controller capabilities just affect what are valid values (or what the resolution is). Rob
Hi Rob, On 15 August 2018 at 04:21, Rob Herring <robh@kernel.org> wrote: > On Tue, Aug 07, 2018 at 06:43:37PM +0800, Baolin Wang wrote: >> From: Lanqing Liu <lanqing.liu@spreadtrum.com> >> >> This patch adds the binding documentation for Spreadtrum SPI >> controller device. >> >> Signed-off-by: Lanqing Liu <lanqing.liu@spreadtrum.com> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >> --- >> Documentation/devicetree/bindings/spi/spi-sprd.txt | 31 ++++++++++++++++++++ >> 1 file changed, 31 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/spi/spi-sprd.txt >> >> diff --git a/Documentation/devicetree/bindings/spi/spi-sprd.txt b/Documentation/devicetree/bindings/spi/spi-sprd.txt >> new file mode 100644 >> index 0000000..06ff746 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/spi/spi-sprd.txt >> @@ -0,0 +1,31 @@ >> +Spreadtrum SPI Controller >> + >> +Required properties: >> +- compatible: Should be "sprd,sc9860-spi". >> +- reg: Offset and length of SPI controller register space. >> +- interrupts: Should contain SPI interrupt. >> +- clock-names: Should contain following entries: >> + "spi" for SPI clock, >> + "source" for SPI source (parent) clock, > > Do the h/w block actually get this clock or the driver needs it? In the > latter case, it should not be in DT. Yes, we will get the actual SPI source clock form the "source" clock property. > >> + "enable" for SPI module enable clock. >> +- clocks: List of clock input name strings sorted in the same order >> + as the clock-names property. >> +- #address-cells: The number of cells required to define a chip select >> + address on the SPI bus. Should be set to 1. >> +- #size-cells: Should be set to 0. >> + >> +Optional properties: >> +- sprd,spi-interval: Specify the intervals of two SPI frames, which can be >> + converted to the delay clock cycles = interval number * 4 + 10. > > There are read and write delay properties you can use. Right. We've agreed introducing another field for spi_transfer to represent this and will remove 'sprd,spi-interval' from DT.
Hi Rob, On 15 August 2018 at 04:27, Rob Herring <robh@kernel.org> wrote: > On Thu, Aug 09, 2018 at 11:03:11AM +0800, Baolin Wang wrote: >> Hi Trent, >> >> On 9 August 2018 at 02:57, Trent Piepho <tpiepho@impinj.com> wrote: >> > On Wed, 2018-08-08 at 11:54 +0100, Mark Brown wrote: >> >> On Wed, Aug 08, 2018 at 06:35:28PM +0800, Baolin Wang wrote: >> >> > On 8 August 2018 at 17:50, Mark Brown <broonie@kernel.org> wrote: >> >> > > Right, I don't think we added this yet (if we did I can't see >> >> > > it). I'd >> >> > > add a new field to spi_transfer for this, then other controllers >> >> > > with >> >> > > the same support can implement it as well and drivers can start >> >> > > using >> >> > > it too. >> >> > OK. So I will name the new filed as 'word_delay', is it OK for you? >> >> >> >> Sounds good, yes. >> > >> > Should it be in µs like the existing iter-transfer delay? I think >> > perhaps units of cycles of the SPI clock make more sense? >> >> Since some SPI controllers just want some interval values (neither µs >> unit nor cycles unit ) set into hardware, and the hardware will >> convert to the correct delay time automatically. So I did not force >> 'word_delay' unit as µs or cycle, and just let the slave devices >> decide the unit which depends on the SPI hardware requirement. > > This needs to be defined units in DT, not decided by each controller. Do you mean we should introduce one standard property (maybe named as 'word_delay_unit') to define the word_delay unit? If we really need to specify the unit of word_delay, I think we can add comments for spi_tansfer to specify the unit, which will be better. diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index a64235e..7a72c0a 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -711,6 +711,8 @@ extern void spi_res_release(struct spi_controller *ctlr, * @delay_usecs: microseconds to delay after this transfer before * (optionally) changing the chipselect status, then starting * the next transfer or completing this @spi_message. + * @word_delay: clock cycles to inter word delay after each word size (set by bits_per_word) + * transmission. * @transfer_list: transfers are sequenced through @spi_message.transfers * @tx_sg: Scatterlist for transmit, currently not for client use * @rx_sg: Scatterlist for receive, currently not for client use @@ -793,6 +795,7 @@ struct spi_transfer { u8 bits_per_word; u16 delay_usecs; u32 speed_hz; + u16 word_delay; struct list_head transfer_list; };
diff --git a/Documentation/devicetree/bindings/spi/spi-sprd.txt b/Documentation/devicetree/bindings/spi/spi-sprd.txt new file mode 100644 index 0000000..06ff746 --- /dev/null +++ b/Documentation/devicetree/bindings/spi/spi-sprd.txt @@ -0,0 +1,31 @@ +Spreadtrum SPI Controller + +Required properties: +- compatible: Should be "sprd,sc9860-spi". +- reg: Offset and length of SPI controller register space. +- interrupts: Should contain SPI interrupt. +- clock-names: Should contain following entries: + "spi" for SPI clock, + "source" for SPI source (parent) clock, + "enable" for SPI module enable clock. +- clocks: List of clock input name strings sorted in the same order + as the clock-names property. +- #address-cells: The number of cells required to define a chip select + address on the SPI bus. Should be set to 1. +- #size-cells: Should be set to 0. + +Optional properties: +- sprd,spi-interval: Specify the intervals of two SPI frames, which can be + converted to the delay clock cycles = interval number * 4 + 10. + +Example: +spi0: spi@70a00000{ + compatible = "sprd,sc9860-spi"; + reg = <0 0x70a00000 0 0x1000>; + interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; + clock-names = "spi", "source","enable"; + clocks = <&clk_spi0>, <&ext_26m>, <&clk_ap_apb_gates 5>; + sprd,spi-interval = <9>; + #address-cells = <1>; + #size-cells = <0>; +};