| Message ID | 64681bf903104c8a02f118294e616e2a12a5ebe4.1533638405.git.baolin.wang@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] dt-bindings: spi: Add Spreadtrum SPI controller documentation | expand |
On Tue, Aug 07, 2018 at 06:43:38PM +0800, Baolin Wang wrote: > From: Lanqing Liu <lanqing.liu@spreadtrum.com> > > This patch adds the SPI controller driver for Spreadtrum SC9860 platform. This all looks pretty clean, a few comments below but nothing too major: > +static void sprd_spi_chipselect(struct spi_device *sdev, bool cs) > +{ > + struct spi_controller *sctlr = sdev->controller; > + struct sprd_spi *ss = spi_controller_get_devdata(sctlr); > + u32 val; > + int ret; > + > + ret = pm_runtime_get_sync(ss->dev); > + if (ret < 0) { > + dev_err(ss->dev, "failed to resume SPI controller\n"); > + return; > + } Something else further up the stack should probably have runtime PM enabled already - we should only be changing chip select as part of a bigger operation. If you use the core auto_runtime_pm feature this will definitely happen. > + bits_per_word = bits_per_word > 16 ? round_up(bits_per_word, 16) : > + round_up(bits_per_word, 8); Please > + switch (bits_per_word) { > + case 8: > + case 16: > + case 32: It'd be nice to have a default case, the core should check for you but it's nice to have defensive programming here. > +static int sprd_spi_transfer_one(struct spi_controller *sctlr, > + struct spi_device *sdev, > + struct spi_transfer *t) > +{ > + struct sprd_spi *ss = spi_controller_get_devdata(sctlr); > + int ret; > + > + ret = pm_runtime_get_sync(ss->dev); > + if (ret < 0) { > + dev_err(ss->dev, "failed to resume SPI controller\n"); > + goto rpm_err; > + } Same thing with runtime PM here - the core can do this for you. > +static int sprd_spi_setup(struct spi_device *spi_dev) > +{ > + struct spi_controller *sctlr = spi_dev->controller; > + struct sprd_spi *ss = spi_controller_get_devdata(sctlr); > + int ret; > + > + ret = pm_runtime_get_sync(ss->dev); > + if (ret < 0) { > + dev_err(ss->dev, "failed to resume SPI controller\n"); > + return ret; > + } > + > + ss->hw_mode = spi_dev->mode; > + sprd_spi_init_hw(ss); This can be called for one chip select while another is in progress so it's not good to actually configure the hardware here unless the configuration is in a chip select specific set of registers. Instead you should defer to when the transfer is being set up. > +static int sprd_spi_clk_init(struct platform_device *pdev, struct sprd_spi *ss) > +{ > + struct clk *clk_spi, *clk_parent; > + > + clk_spi = devm_clk_get(&pdev->dev, "spi"); > + if (IS_ERR(clk_spi)) { > + dev_warn(&pdev->dev, "can't get the spi clock\n"); > + clk_spi = NULL; > + } I suspect some of these clocks are essential and you should probably return an error if you can't get them (especially if probe deferral becomes a factor). > + if (!clk_set_parent(clk_spi, clk_parent)) > + ss->src_clk = clk_get_rate(clk_spi); > + else > + ss->src_clk = SPRD_SPI_DEFAULT_SOURCE; Are upstream DTs going to be missing the clock setup needed here?
Hi Trent, On 8 August 2018 at 01:10, Trent Piepho <tpiepho@impinj.com> wrote: > On Tue, 2018-08-07 at 18:43 +0800, Baolin Wang wrote: >> >> +static u32 sprd_spi_transfer_max_timeout(struct sprd_spi *ss, >> + struct spi_transfer *t) >> +{ >> + /* >> + * The time spent on transmission of the full FIFO data is the maximum >> + * SPI transmission time. >> + */ >> + u32 size = t->bits_per_word * SPRD_SPI_FIFO_SIZE; >> + u32 bit_time_us = SPRD_SPI_HZ / t->speed_hz + 1; >> + u32 total_time_us = size * bit_time_us; >> + /* >> + * There is an interval between data and the data in our SPI hardware, >> + * so the total transmission time need add the interval time. >> + * >> + * The inteval calculation formula: >> + * interval time (source clock cycles) = interval * 4 + 10. >> + */ >> + u32 interval_cycle = SPRD_SPI_FIFO_SIZE * ((ss->interval << 2) + 10); >> + u32 interval_time_us = interval_cycle * SPRD_SPI_HZ / ss->src_clk + 1; > > If interval is greater than 31, this will overflow. Usually we will not set such a big value for interval, but this is a risk like you said. So we will check and limit the interval value to make sure the formula will not overflow. > > Also SPRD_SPI_HZ is not the speed anything runs at, as one might think > from the name. It's the number of microseconds in a second. The is > already a Linux macro for that, USEC_PER_SEC, that you should use > instead. Right, will use USEC_PER_SEC instead. > >> + >> + return total_time_us + interval_time_us; >> +} >> + > > >> +static void sprd_spi_set_speed(struct sprd_spi *ss, u32 speed_hz) >> +{ >> + /* >> + * From SPI datasheet, the prescale calculation formula: >> + * prescale = SPI source clock / (2 * SPI_freq) - 1; >> + */ >> + u32 clk_div = ss->src_clk / (speed_hz << 1) - 1; > > You should probably round up here. The convention is to use the > closest speed less than equal to the requested speed, but since this is > a divider, rounding it down will select the next highest speed. Per the datasheet, we do not need round up/down the speed. Since our hardware can handle the clock calculated by the above formula if the requested speed is in the normal region (less than ->max_speed_hz). >> + >> + writel_relaxed(clk_div, ss->base + SPRD_SPI_CLKD); >> +} >> + > >> + >> +static int sprd_spi_probe(struct platform_device *pdev) >> +{ >> + struct device_node *np = pdev->dev.of_node; >> + struct spi_controller *sctlr; >> + struct resource *res; >> + struct sprd_spi *ss; >> + int ret; >> + >> + pdev->id = of_alias_get_id(pdev->dev.of_node, "spi"); >> + sctlr = spi_alloc_master(&pdev->dev, sizeof(*ss)); >> + if (!sctlr) >> + return -ENOMEM; >> + >> + ss = spi_controller_get_devdata(sctlr); >> + if (of_property_read_u32(np, "sprd,spi-interval", &ss->interval)) >> + ss->interval = SPRD_SPI_ITVL_NUM; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + ss->base = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(ss->base)) { >> + ret = PTR_ERR(ss->base); >> + goto free_controller; >> + } >> + >> + ss->dev = &pdev->dev; >> + sctlr->dev.of_node = pdev->dev.of_node; >> + sctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE | SPI_TX_DUAL; >> + sctlr->bus_num = pdev->id; >> + sctlr->setup = sprd_spi_setup; >> + sctlr->set_cs = sprd_spi_chipselect; >> + sctlr->transfer_one = sprd_spi_transfer_one; >> + sctlr->max_speed_hz = (ss->src_clk / 2) < SPRD_SPI_MAX_SPEED_HZ ? >> + ss->src_clk / 2 : SPRD_SPI_MAX_SPEED_HZ; > > You can write this as: > sctlr->max_speed_hz = min(ss->src_clk / 2, SPRD_SPI_MAX_SPEED_HZ); Right. Thanks for your comments. -- Baolin Wang Best Regards -- 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 8 August 2018 at 17:31, Mark Brown <broonie@kernel.org> wrote: > On Wed, Aug 08, 2018 at 10:45:33AM +0800, Baolin Wang wrote: >> On 7 August 2018 at 22:24, Mark Brown <broonie@kernel.org> wrote: >> > On Tue, Aug 07, 2018 at 06:43:38PM +0800, Baolin Wang wrote: > >> >> + bits_per_word = bits_per_word > 16 ? round_up(bits_per_word, 16) : >> >> + round_up(bits_per_word, 8); > >> > Please > >> Sorry I did not get your points here, could you elaborate on? > > Sorry, missed the actual comment there - use normal if statements rather > than the ternery operator, it's easier to read. Got it. Thanks. -- Baolin Wang Best Regards -- 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 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? -- Baolin Wang Best Regards -- 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
T24gV2VkLCAyMDE4LTA4LTA4IGF0IDExOjU0ICswMTAwLCBNYXJrIEJyb3duIHdyb3RlOg0KPiBP biBXZWQsIEF1ZyAwOCwgMjAxOCBhdCAwNjozNToyOFBNICswODAwLCBCYW9saW4gV2FuZyB3cm90 ZToNCj4gPiBPbiA4IEF1Z3VzdCAyMDE4IGF0IDE3OjUwLCBNYXJrIEJyb3duIDxicm9vbmllQGtl cm5lbC5vcmc+IHdyb3RlOg0KPiA+ID4gUmlnaHQsIEkgZG9uJ3QgdGhpbmsgd2UgYWRkZWQgdGhp cyB5ZXQgKGlmIHdlIGRpZCBJIGNhbid0IHNlZQ0KPiA+ID4gaXQpLiAgSSdkDQo+ID4gPiBhZGQg YSBuZXcgZmllbGQgdG8gc3BpX3RyYW5zZmVyIGZvciB0aGlzLCB0aGVuIG90aGVyIGNvbnRyb2xs ZXJzDQo+ID4gPiB3aXRoDQo+ID4gPiB0aGUgc2FtZSBzdXBwb3J0IGNhbiBpbXBsZW1lbnQgaXQg YXMgd2VsbCBhbmQgZHJpdmVycyBjYW4gc3RhcnQNCj4gPiA+IHVzaW5nDQo+ID4gPiBpdCB0b28u DQo+ID4gT0suIFNvIEkgd2lsbCBuYW1lIHRoZSBuZXcgZmlsZWQgYXMgJ3dvcmRfZGVsYXknLCBp cyBpdCBPSyBmb3IgeW91Pw0KPiANCj4gU291bmRzIGdvb2QsIHllcy4NCg0KU2hvdWxkIGl0IGJl IGluIMK1cyBsaWtlIHRoZSBleGlzdGluZyBpdGVyLXRyYW5zZmVyIGRlbGF5PyAgSSB0aGluaw0K cGVyaGFwcyB1bml0cyBvZiBjeWNsZXMgb2YgdGhlIFNQSSBjbG9jayBtYWtlIG1vcmUgc2Vuc2U/ -- 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 Wed, 2018-08-08 at 11:19 +0800, Baolin Wang wrote: > On 8 August 2018 at 01:10, Trent Piepho <tpiepho@impinj.com> wrote: > > On Tue, 2018-08-07 at 18:43 +0800, Baolin Wang wrote: > > > > > > +static u32 sprd_spi_transfer_max_timeout(struct sprd_spi *ss, > > > + struct spi_transfer *t) > > > +{ > > > + /* > > > + * The time spent on transmission of the full FIFO data is the maximum > > > + * SPI transmission time. > > > + */ > > > + u32 size = t->bits_per_word * SPRD_SPI_FIFO_SIZE; > > > + u32 bit_time_us = SPRD_SPI_HZ / t->speed_hz + 1; There's another flaw here in that the transfer speed, t->speed_hz, might not be exactly what is used, due to limitations of the clock divider. It would be better to find the actual SPI clock used, then use that in the calculations. > > > + u32 total_time_us = size * bit_time_us; > > > + /* > > > + * There is an interval between data and the data in our SPI hardware, > > > + * so the total transmission time need add the interval time. > > > + * > > > + * The inteval calculation formula: > > > + * interval time (source clock cycles) = interval * 4 + 10. > > > + */ > > > + u32 interval_cycle = SPRD_SPI_FIFO_SIZE * ((ss->interval << 2) + 10); > > > + u32 interval_time_us = interval_cycle * SPRD_SPI_HZ / ss->src_clk + 1; > > > > If interval is greater than 31, this will overflow. > > Usually we will not set such a big value for interval, but this is a > risk like you said. So we will check and limit the interval value to > make sure the formula will not overflow. > Better would be to limit the inter word delay to whatever maximum value your hardware supports, and then write code that can calculate that without error. > > > +static void sprd_spi_set_speed(struct sprd_spi *ss, u32 speed_hz) > > > +{ > > > + /* > > > + * From SPI datasheet, the prescale calculation formula: > > > + * prescale = SPI source clock / (2 * SPI_freq) - 1; > > > + */ > > > + u32 clk_div = ss->src_clk / (speed_hz << 1) - 1; > > > > You should probably round up here. The convention is to use the > > closest speed less than equal to the requested speed, but since this is > > a divider, rounding it down will select the next highest speed. > > Per the datasheet, we do not need round up/down the speed. Since our > hardware can handle the clock calculated by the above formula if the > requested speed is in the normal region (less than ->max_speed_hz). That is not what I mean. Let me explain differently. An integer divider like this can not produce any frequency exactly. Consider if src_clk is 10 MHz. A clk_div value of 0 produces a 5 MHz SPI clock. A clk_div value of 1 produces a 2.5 MHz SPI clock. What if the transfer requests a SPI clock of 3 MHz? Your math above will produce a SPI clock of 5 MHz, faster than requested. This is not the convention in Linux SPI masters. You should instead of have chosen a clk_div value of 1 to get a SPI clock of 2.5 MHz, the closest clock possible that is not greater than the requested value. To do this, round up. clk_div = DIV_ROUND_UP(ss->src_clk, speed_hz * 2) - 1; >
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. -- Baolin Wang Best Regards -- 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 Trent, On 9 August 2018 at 03:08, Trent Piepho <tpiepho@impinj.com> wrote: > On Wed, 2018-08-08 at 11:19 +0800, Baolin Wang wrote: >> On 8 August 2018 at 01:10, Trent Piepho <tpiepho@impinj.com> wrote: >> > On Tue, 2018-08-07 at 18:43 +0800, Baolin Wang wrote: >> > > >> > > +static u32 sprd_spi_transfer_max_timeout(struct sprd_spi *ss, >> > > + struct spi_transfer *t) >> > > +{ >> > > + /* >> > > + * The time spent on transmission of the full FIFO data is the maximum >> > > + * SPI transmission time. >> > > + */ >> > > + u32 size = t->bits_per_word * SPRD_SPI_FIFO_SIZE; >> > > + u32 bit_time_us = SPRD_SPI_HZ / t->speed_hz + 1; > > There's another flaw here in that the transfer speed, t->speed_hz, > might not be exactly what is used, due to limitations of the clock > divider. It would be better to find the actual SPI clock used, then > use that in the calculations. Right, will use the real speed to calculate the transfer time. > >> > > + u32 total_time_us = size * bit_time_us; >> > > + /* >> > > + * There is an interval between data and the data in our SPI hardware, >> > > + * so the total transmission time need add the interval time. >> > > + * >> > > + * The inteval calculation formula: >> > > + * interval time (source clock cycles) = interval * 4 + 10. >> > > + */ >> > > + u32 interval_cycle = SPRD_SPI_FIFO_SIZE * ((ss->interval << 2) + 10); >> > > + u32 interval_time_us = interval_cycle * SPRD_SPI_HZ / ss->src_clk + 1; >> > >> > If interval is greater than 31, this will overflow. >> >> Usually we will not set such a big value for interval, but this is a >> risk like you said. So we will check and limit the interval value to >> make sure the formula will not overflow. >> > > Better would be to limit the inter word delay to whatever maximum value > your hardware supports, and then write code that can calculate that > without error. Yes, will define the maximum word delay values to avoid overflow. > >> > > +static void sprd_spi_set_speed(struct sprd_spi *ss, u32 speed_hz) >> > > +{ >> > > + /* >> > > + * From SPI datasheet, the prescale calculation formula: >> > > + * prescale = SPI source clock / (2 * SPI_freq) - 1; >> > > + */ >> > > + u32 clk_div = ss->src_clk / (speed_hz << 1) - 1; >> > >> > You should probably round up here. The convention is to use the >> > closest speed less than equal to the requested speed, but since this is >> > a divider, rounding it down will select the next highest speed. >> >> Per the datasheet, we do not need round up/down the speed. Since our >> hardware can handle the clock calculated by the above formula if the >> requested speed is in the normal region (less than ->max_speed_hz). > > That is not what I mean. Let me explain differently. > > An integer divider like this can not produce any frequency exactly. > Consider if src_clk is 10 MHz. A clk_div value of 0 produces a 5 MHz > SPI clock. A clk_div value of 1 produces a 2.5 MHz SPI clock. > > What if the transfer requests a SPI clock of 3 MHz? > > Your math above will produce a SPI clock of 5 MHz, faster than > requested. This is not the convention in Linux SPI masters. You > should instead of have chosen a clk_div value of 1 to get a SPI clock > of 2.5 MHz, the closest clock possible that is not greater than the > requested value. > > To do this, round up. > > clk_div = DIV_ROUND_UP(ss->src_clk, speed_hz * 2) - 1; Thanks for your patient explanation. After talking with Lanqing who is the author of the SPI driver, we think you are definitely correct and will fix in next version according to your suggestion. Thanks a lot. -- Baolin Wang Best Regards -- 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 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: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; }; -- Baolin Wang Best Regards
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>; +};