| Message ID | 20230106200809.330769-1-william.zhang@broadcom.com |
|---|---|
| Headers | show |
| Series | spi: bcm63xx-hsspi: driver and doc updates | expand |
On Fri, Jan 06, 2023 at 12:07:55PM -0800, William Zhang wrote: > brcm,no-clk-gate is a Broadcom Broadband HS SPI controller specific > property for certain SPI device such as Broadcom ISI voice daughtercard > to work properly. It disables the clock gating feature when the chip > select is deasserted for any device that wants to keep the clock > running. Why would this property be Broadcom specific? Other devices could in theory implement this. > +properties: > + brcm,no-clk-gate: > + $ref: /schemas/types.yaml#/definitions/flag > + description: > + Some SPI device such as Broadcom ISI based voice daughtercard requires SPI > + clock running even when chip select is deasserted. By default the > + controller turns off or gate the clock when cs is not active to save > + power. This flag tells the controller driver to keep the clock running > + when chip select is not active. This seems problematic with any host controlled chip select support...
Hi Mark, On 01/06/2023 01:14 PM, Mark Brown wrote: > On Fri, Jan 06, 2023 at 12:07:55PM -0800, William Zhang wrote: > >> brcm,no-clk-gate is a Broadcom Broadband HS SPI controller specific >> property for certain SPI device such as Broadcom ISI voice daughtercard >> to work properly. It disables the clock gating feature when the chip >> select is deasserted for any device that wants to keep the clock >> running. > > Why would this property be Broadcom specific? Other devices could in > theory implement this. > It does not need to be Broadcom specific if other SoC's SPI bus controller support such function. I am not aware of such case but certainly I am no expert on other chips. I can put it in the generic spi-peripheral-props.yaml if that is what you suggest. >> +properties: >> + brcm,no-clk-gate: >> + $ref: /schemas/types.yaml#/definitions/flag >> + description: >> + Some SPI device such as Broadcom ISI based voice daughtercard requires SPI >> + clock running even when chip select is deasserted. By default the >> + controller turns off or gate the clock when cs is not active to save >> + power. This flag tells the controller driver to keep the clock running >> + when chip select is not active. > > This seems problematic with any host controlled chip select support... > Yes those ISI chip based voice cards do need such strange requirement and will not work with other controller. That is one of the reason I put this as Broadcom specific option.
On 01/06/2023 01:47 PM, Mark Brown wrote: > On Fri, Jan 06, 2023 at 12:07:59PM -0800, William Zhang wrote: > >> Polling mode provides better throughput in general by avoiding the >> interrupt overhead as the maximum data size one interrupt can handle is >> only 512 bytes. > >> When interrupt is not defined in the HSSPI dts node, driver switches to >> polling mode for controller SPI message processing. Also add driver >> banner message when the driver is loaded successfully. > > This should not be something the user selects via the DT, if the polling > mode is better then the driver should just use it regardless of there > being an interrupt wired up. Generally there's some point at which the > benefits of polling become minimal (and the costs more impactful) but if > the DMA setup is as bad as it sounds then the driver should just ignore > the interrupt. > I agree but this is more for backward compatibility as the original driver uses interrupt to work with MIPS based SoC and I do not want to change that behavior. For newer devices I added, they work in polling mode by default with the dts I supplied. IMHO it is also good to have this fallback option to use interrupt in case user is sensitive to cpu usage.
On 01/06/2023 02:07 PM, Mark Brown wrote: > On Fri, Jan 06, 2023 at 12:08:06PM -0800, William Zhang wrote: >> In general the controller supports SPI dual mode operation but the >> particular SPI flash dual io read op switches from single mode in cmd >> phase to dual mode in address and data phase. This is not compatible >> with prepend operation where cmd and address are sent out through the >> prepend buffer and they must use same the number of io pins. >> >> This patch disables these SPI flash dual io read ops through the mem_ops >> supports_op interface when prepend mode is used. This makes sure the SPI >> flash driver selects the compatible read ops at run time. > > This suggests that your prepend mode is attempting to combine > incompatible transfers doesn't it? Presumably other devices could > generate similar access patterns... > As far as I can see, only spi mem op has such pattern but agree it is possible that other device can generate such usage. I can add a check for this condition(all the prepend data must use same io width) so error can be printed and user can switch to dummy cs workaround. Thank you for your feedbacks so far and have a nice weekend!
On 06/01/2023 21:07, William Zhang wrote: > This is the preparation for updates on the bcm63xx hsspi driver. Convert > the text based bindings to json-schema per new dts requirement. > > Signed-off-by: William Zhang <william.zhang@broadcom.com> > --- > > .../bindings/spi/brcm,bcm63xx-hsspi.yaml | 52 +++++++++++++++++++ > .../bindings/spi/spi-bcm63xx-hsspi.txt | 33 ------------ > 2 files changed, 52 insertions(+), 33 deletions(-) > create mode 100644 Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml > delete mode 100644 Documentation/devicetree/bindings/spi/spi-bcm63xx-hsspi.txt > > diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml > new file mode 100644 > index 000000000000..45f1417b1213 > --- /dev/null > +++ b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml > @@ -0,0 +1,52 @@ > +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/spi/brcm,bcm63xx-hsspi.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Broadcom BCM6328 High Speed SPI controller > + > +maintainers: > + - Jonas Gorski <jonas.gorski@gmail.com> > + Missing reference to spi-controller. > +properties: > + compatible: > + const: brcm,bcm6328-hsspi > + > + reg: > + maxItems: 1 > + > + clocks: > + items: > + - description: spi master reference clock > + - description: spi master pll clock > + > + clock-names: > + items: > + - const: hsspi > + - const: pll > + > + interrupts: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - interrupts > + > +unevaluatedProperties: false This is for cases when you have reference to other schema. Best regards, Krzysztof
On Fri, Jan 6, 2023 at 9:27 PM William Zhang <william.zhang@broadcom.com> wrote: > > Hi Mark, > > On 01/06/2023 01:14 PM, Mark Brown wrote: > > On Fri, Jan 06, 2023 at 12:07:55PM -0800, William Zhang wrote: > > > >> brcm,no-clk-gate is a Broadcom Broadband HS SPI controller specific > >> property for certain SPI device such as Broadcom ISI voice daughtercard > >> to work properly. It disables the clock gating feature when the chip > >> select is deasserted for any device that wants to keep the clock > >> running. > > > > Why would this property be Broadcom specific? Other devices could in > > theory implement this. > > > It does not need to be Broadcom specific if other SoC's SPI bus > controller support such function. I am not aware of such case but > certainly I am no expert on other chips. I can put it in the generic > spi-peripheral-props.yaml if that is what you suggest. > > >> +properties: > >> + brcm,no-clk-gate: > >> + $ref: /schemas/types.yaml#/definitions/flag > >> + description: > >> + Some SPI device such as Broadcom ISI based voice daughtercard requires SPI > >> + clock running even when chip select is deasserted. By default the > >> + controller turns off or gate the clock when cs is not active to save > >> + power. This flag tells the controller driver to keep the clock running > >> + when chip select is not active. > > > > This seems problematic with any host controlled chip select support... > > > Yes those ISI chip based voice cards do need such strange requirement > and will not work with other controller. That is one of the reason I > put this as Broadcom specific option. Keeping the clock on or not would affect all devices unless you have a per device clock you can gate, so making this a per device flag doesn't make sense. If this is a requirement of the slave device, then the device's compatible string can imply the need for this and its driver can tell the host controller in some way. Rob
On 01/07/2023 07:32 AM, Krzysztof Kozlowski wrote: > On 06/01/2023 21:07, William Zhang wrote: >> This is the preparation for updates on the bcm63xx hsspi driver. Convert >> the text based bindings to json-schema per new dts requirement. >> >> Signed-off-by: William Zhang <william.zhang@broadcom.com> >> --- >> >> .../bindings/spi/brcm,bcm63xx-hsspi.yaml | 52 +++++++++++++++++++ >> .../bindings/spi/spi-bcm63xx-hsspi.txt | 33 ------------ >> 2 files changed, 52 insertions(+), 33 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml >> delete mode 100644 Documentation/devicetree/bindings/spi/spi-bcm63xx-hsspi.txt >> >> diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml >> new file mode 100644 >> index 000000000000..45f1417b1213 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml >> @@ -0,0 +1,52 @@ >> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/spi/brcm,bcm63xx-hsspi.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Broadcom BCM6328 High Speed SPI controller >> + >> +maintainers: >> + - Jonas Gorski <jonas.gorski@gmail.com> >> + > > Missing reference to spi-controller. > This was word to word conversion from the text file. But I will update with this required reference. >> +properties: >> + compatible: >> + const: brcm,bcm6328-hsspi >> + >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + items: >> + - description: spi master reference clock >> + - description: spi master pll clock >> + >> + clock-names: >> + items: >> + - const: hsspi >> + - const: pll >> + >> + interrupts: >> + maxItems: 1 >> + >> +required: >> + - compatible >> + - reg >> + - clocks >> + - clock-names >> + - interrupts >> + >> +unevaluatedProperties: false > > This is for cases when you have reference to other schema. > Will drop here. But will add back in patch 1 which produces the final version of this file and need this property. > > Best regards, > Krzysztof >
Hi Rob, On 01/07/2023 07:38 AM, Rob Herring wrote: > On Fri, Jan 6, 2023 at 9:27 PM William Zhang <william.zhang@broadcom.com> wrote: >> >> Hi Mark, >> >> On 01/06/2023 01:14 PM, Mark Brown wrote: >>> On Fri, Jan 06, 2023 at 12:07:55PM -0800, William Zhang wrote: >>> >>>> brcm,no-clk-gate is a Broadcom Broadband HS SPI controller specific >>>> property for certain SPI device such as Broadcom ISI voice daughtercard >>>> to work properly. It disables the clock gating feature when the chip >>>> select is deasserted for any device that wants to keep the clock >>>> running. >>> >>> Why would this property be Broadcom specific? Other devices could in >>> theory implement this. >>> >> It does not need to be Broadcom specific if other SoC's SPI bus >> controller support such function. I am not aware of such case but >> certainly I am no expert on other chips. I can put it in the generic >> spi-peripheral-props.yaml if that is what you suggest. >> >>>> +properties: >>>> + brcm,no-clk-gate: >>>> + $ref: /schemas/types.yaml#/definitions/flag >>>> + description: >>>> + Some SPI device such as Broadcom ISI based voice daughtercard requires SPI >>>> + clock running even when chip select is deasserted. By default the >>>> + controller turns off or gate the clock when cs is not active to save >>>> + power. This flag tells the controller driver to keep the clock running >>>> + when chip select is not active. >>> >>> This seems problematic with any host controlled chip select support... >>> >> Yes those ISI chip based voice cards do need such strange requirement >> and will not work with other controller. That is one of the reason I >> put this as Broadcom specific option. > > Keeping the clock on or not would affect all devices unless you have a > per device clock you can gate, so making this a per device flag > doesn't make sense. > This applies only to each chip select. There is only one device under each chip select. So won't impact any other devices under other cs. > If this is a requirement of the slave device, then the device's > compatible string can imply the need for this and its driver can tell > the host controller in some way. That is true but spi host controller driver reads and parses these slave device flag directly. > > Rob >
On 09/01/2023 08:52, William Zhang wrote: > > > On 01/07/2023 07:32 AM, Krzysztof Kozlowski wrote: >> On 06/01/2023 21:07, William Zhang wrote: >>> This is the preparation for updates on the bcm63xx hsspi driver. Convert >>> the text based bindings to json-schema per new dts requirement. >>> >>> Signed-off-by: William Zhang <william.zhang@broadcom.com> >>> --- >>> >>> .../bindings/spi/brcm,bcm63xx-hsspi.yaml | 52 +++++++++++++++++++ >>> .../bindings/spi/spi-bcm63xx-hsspi.txt | 33 ------------ >>> 2 files changed, 52 insertions(+), 33 deletions(-) >>> create mode 100644 Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml >>> delete mode 100644 Documentation/devicetree/bindings/spi/spi-bcm63xx-hsspi.txt >>> >>> diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml >>> new file mode 100644 >>> index 000000000000..45f1417b1213 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml >>> @@ -0,0 +1,52 @@ >>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/spi/brcm,bcm63xx-hsspi.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Broadcom BCM6328 High Speed SPI controller >>> + >>> +maintainers: >>> + - Jonas Gorski <jonas.gorski@gmail.com> >>> + >> >> Missing reference to spi-controller. >> > This was word to word conversion from the text file. But I will update > with this required reference. > >>> +properties: >>> + compatible: >>> + const: brcm,bcm6328-hsspi >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + clocks: >>> + items: >>> + - description: spi master reference clock >>> + - description: spi master pll clock >>> + >>> + clock-names: >>> + items: >>> + - const: hsspi >>> + - const: pll >>> + >>> + interrupts: >>> + maxItems: 1 >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - clocks >>> + - clock-names >>> + - interrupts >>> + >>> +unevaluatedProperties: false >> >> This is for cases when you have reference to other schema. >> > Will drop here. But will add back in patch 1 which produces the final > version of this file and need this property. When you add reference to spi-controller, keep it here. This was wrong when the reference was missing. Best regards, Krzysztof
On Fri, Jan 06, 2023 at 07:35:59PM -0800, William Zhang wrote: > On 01/06/2023 01:47 PM, Mark Brown wrote: > > On Fri, Jan 06, 2023 at 12:07:59PM -0800, William Zhang wrote: > > > When interrupt is not defined in the HSSPI dts node, driver switches to > > > polling mode for controller SPI message processing. Also add driver > > > banner message when the driver is loaded successfully. > > This should not be something the user selects via the DT, if the polling > > mode is better then the driver should just use it regardless of there > > being an interrupt wired up. Generally there's some point at which the > > benefits of polling become minimal (and the costs more impactful) but if > > the DMA setup is as bad as it sounds then the driver should just ignore > > the interrupt. > I agree but this is more for backward compatibility as the original driver > uses interrupt to work with MIPS based SoC and I do not want to change that > behavior. For newer devices I added, they work in polling mode by default > with the dts I supplied. IMHO it is also good to have this fallback option > to use interrupt in case user is sensitive to cpu usage. You can put whatever logic is needed in the code - for something like this an architecture based define isn't ideal but is probably good enough if need be (though I'd not be surprised if it turned out that there was also some performance benefit for the MIPS systems too, at least for smaller transfers).
On Mon, Jan 09, 2023 at 12:06:13AM -0800, William Zhang wrote: > > On Fri, Jan 6, 2023 at 9:27 PM William Zhang <william.zhang@broadcom.com> wrote: > > Keeping the clock on or not would affect all devices unless you have a > > per device clock you can gate, so making this a per device flag > > doesn't make sense. > This applies only to each chip select. There is only one device under each > chip select. So won't impact any other devices under other cs. I don't understand how this would work - usually a SPI controller has a single set of clock, MOSI and MISO lines with the only per device thing being the chip select. If the clock line is used by all devices then it must be kept on for all of them if it's to be kept on for one of them.
On 01/09/2023 11:06 AM, Mark Brown wrote: > On Fri, Jan 06, 2023 at 07:35:59PM -0800, William Zhang wrote: >> On 01/06/2023 01:47 PM, Mark Brown wrote: >>> On Fri, Jan 06, 2023 at 12:07:59PM -0800, William Zhang wrote: > >>>> When interrupt is not defined in the HSSPI dts node, driver switches to >>>> polling mode for controller SPI message processing. Also add driver >>>> banner message when the driver is loaded successfully. > >>> This should not be something the user selects via the DT, if the polling >>> mode is better then the driver should just use it regardless of there >>> being an interrupt wired up. Generally there's some point at which the >>> benefits of polling become minimal (and the costs more impactful) but if >>> the DMA setup is as bad as it sounds then the driver should just ignore >>> the interrupt. > >> I agree but this is more for backward compatibility as the original driver >> uses interrupt to work with MIPS based SoC and I do not want to change that >> behavior. For newer devices I added, they work in polling mode by default >> with the dts I supplied. IMHO it is also good to have this fallback option >> to use interrupt in case user is sensitive to cpu usage. > > You can put whatever logic is needed in the code - for something like > this an architecture based define isn't ideal but is probably good > enough if need be (though I'd not be surprised if it turned out that > there was also some performance benefit for the MIPS systems too, at > least for smaller transfers). > I just don't know what other logic I can put in the driver to select interrupt or polling mode. Only the end user know if performance or cpu usage is more important to their application.
Hi Mark, On 01/09/2023 11:19 AM, Mark Brown wrote: > On Mon, Jan 09, 2023 at 12:06:13AM -0800, William Zhang wrote: >>> On Fri, Jan 6, 2023 at 9:27 PM William Zhang <william.zhang@broadcom.com> wrote: > >>> Keeping the clock on or not would affect all devices unless you have a >>> per device clock you can gate, so making this a per device flag >>> doesn't make sense. > >> This applies only to each chip select. There is only one device under each >> chip select. So won't impact any other devices under other cs. > > I don't understand how this would work - usually a SPI controller has a > single set of clock, MOSI and MISO lines with the only per device thing > being the chip select. If the clock line is used by all devices then it > must be kept on for all of them if it's to be kept on for one of them. > This setting is set per spi message for particular chip select of the device when starting the message through bcm63xx_hsspi_set_clk function and restore to default(clock gating) when message is done through bcm63xx_hsspi_restore_clk_gate.
On Mon, Jan 09, 2023 at 12:18:09PM -0800, William Zhang wrote: > This setting is set per spi message for particular chip select of the device > when starting the message through bcm63xx_hsspi_set_clk function and restore > to default(clock gating) when message is done through > bcm63xx_hsspi_restore_clk_gate. In that case I am extremely confused about what the feature is supposed to do. The description says: + brcm,no-clk-gate: + $ref: /schemas/types.yaml#/definitions/flag + description: + Some SPI device such as Broadcom ISI based voice daughtercard requires +SPI + clock running even when chip select is deasserted. By default the + controller turns off or gate the clock when cs is not active to save + power. This flag tells the controller driver to keep the clock running + when chip select is not active. which to me sounds like the clock should never be turned off and instead left running at all times. Switching back to clock gating after sending the message doesn't seem to correspond to the above at all, the message being done would normally also be the point at which chip select is deasserted.
On Mon, Jan 09, 2023 at 12:10:30PM -0800, William Zhang wrote: > On 01/09/2023 11:06 AM, Mark Brown wrote: > > You can put whatever logic is needed in the code - for something like > > this an architecture based define isn't ideal but is probably good > > enough if need be (though I'd not be surprised if it turned out that > > there was also some performance benefit for the MIPS systems too, at > > least for smaller transfers). > I just don't know what other logic I can put in the driver to select > interrupt or polling mode. Only the end user know if performance or cpu > usage is more important to their application. Usually you can take a reasonable guess as to what would be a good point to start switching, typically for short enough transfers the overhead of setting up DMA, waiting for interrupts and tearing things down is very much larger than the cost of just doing PIO - a bunch of other drivers have pick a number logic of some kind, often things like FIFO sizes are a good key for where to look. A lot of the time this is good enough, and it means that users have much better facilities for making tradeoffs if they have a range of transfer sizes available - it's not an either/or thing but based on some features of the individual message/transfer. It is true that for people with heavy SPI traffic or otherwise very demanding requirements for a specific system and software stack additional tuning might produce better results, exposing some sysfs knobs to allow tuning of parameters at runtime would be helpful for them and I'd certainly be happy to see that added.
On 01/10/2023 02:01 PM, Mark Brown wrote: > On Mon, Jan 09, 2023 at 12:18:09PM -0800, William Zhang wrote: > >> This setting is set per spi message for particular chip select of the device >> when starting the message through bcm63xx_hsspi_set_clk function and restore >> to default(clock gating) when message is done through >> bcm63xx_hsspi_restore_clk_gate. > > In that case I am extremely confused about what the feature is supposed > to do. The description says: > > + brcm,no-clk-gate: > + $ref: /schemas/types.yaml#/definitions/flag > + description: > + Some SPI device such as Broadcom ISI based voice daughtercard requires > +SPI > + clock running even when chip select is deasserted. By default the > + controller turns off or gate the clock when cs is not active to save > + power. This flag tells the controller driver to keep the clock running > + when chip select is not active. > > > which to me sounds like the clock should never be turned off and instead > left running at all times. Switching back to clock gating after sending > the message doesn't seem to correspond to the above at all, the message > being done would normally also be the point at which chip select is > deasserted. > This feature is used by our voice team and as far I can tell, it is used to keep clock running between the transfers within the same message. But now that we have prepend mode to combine to one transfer or dummy workaround to keep cs always active between transfers, this indeed does not seems right. I will have to talk to the voice team why this is still needed and get back here.
On 01/10/2023 02:49 PM, Mark Brown wrote: > On Mon, Jan 09, 2023 at 12:10:30PM -0800, William Zhang wrote: >> On 01/09/2023 11:06 AM, Mark Brown wrote: > >>> You can put whatever logic is needed in the code - for something like >>> this an architecture based define isn't ideal but is probably good >>> enough if need be (though I'd not be surprised if it turned out that >>> there was also some performance benefit for the MIPS systems too, at >>> least for smaller transfers). > >> I just don't know what other logic I can put in the driver to select >> interrupt or polling mode. Only the end user know if performance or cpu >> usage is more important to their application. > > Usually you can take a reasonable guess as to what would be a good point > to start switching, typically for short enough transfers the overhead of > setting up DMA, waiting for interrupts and tearing things down is very > much larger than the cost of just doing PIO - a bunch of other drivers > have pick a number logic of some kind, often things like FIFO sizes are > a good key for where to look. A lot of the time this is good enough, > and it means that users have much better facilities for making tradeoffs > if they have a range of transfer sizes available - it's not an either/or > thing but based on some features of the individual message/transfer. > > It is true that for people with heavy SPI traffic or otherwise very > demanding requirements for a specific system and software stack > additional tuning might produce better results, exposing some sysfs > knobs to allow tuning of parameters at runtime would be helpful for them > and I'd certainly be happy to see that added. > Thanks for the explanation. I saw the spi-uniphier.c and spi-bcm2835.c doing the trick you mentioned(thanks Kursad for pointing out). In our case, even the maximum fifo size usage(512bytes), the polling still have better performance than interrupt. The MTD test result included in this patch is based on maximum fifo usage. So there is no benefit to switch to interrupt based on transfer size. Does the spi framework has any requirement on how much time that the driver's transfer_one function can spend? I can see the polling function might take quite some time in busy loop if the clock is low, for example, at 100Hz(slowest clock this controller can go), it takes 512x8/100Hz ~= 41ms to complete. If this is something in concern, I can do the interrupt switch based on a time limit value if interrupt is available.
On Wed, Jan 11, 2023 at 12:13:43PM -0800, William Zhang wrote: > Thanks for the explanation. I saw the spi-uniphier.c and spi-bcm2835.c doing > the trick you mentioned(thanks Kursad for pointing out). In our case, even > the maximum fifo size usage(512bytes), the polling still have better > performance than interrupt. The MTD test result included in this patch is > based on maximum fifo usage. So there is no benefit to switch to interrupt > based on transfer size. > Does the spi framework has any requirement on how much time that the > driver's transfer_one function can spend? I can see the polling function > might take quite some time in busy loop if the clock is low, for example, at > 100Hz(slowest clock this controller can go), it takes 512x8/100Hz ~= 41ms to > complete. If this is something in concern, I can do the interrupt switch > based on a time limit value if interrupt is available. There's no fixed limit, some hardware just doesn't have DMA. Some doesn't even have interrupts which is even better. If there's always a clear benefit for using PIO then just do that, perhaps creating a sysfs hook to allow people to switch to DMA if they need it (rather than requiring people to update their DT, this is really a runtime performance tradeoff rather than a description of the hardware). If there's a point at which the performance is very similar then it's probably worth switching to DMA there to lower the CPU usage, but if it's always faster to use PIO then just defaulting to PIO seems like the best option.
On 01/11/2023 02:41 PM, Mark Brown wrote: > On Wed, Jan 11, 2023 at 12:13:43PM -0800, William Zhang wrote: > >> Thanks for the explanation. I saw the spi-uniphier.c and spi-bcm2835.c doing >> the trick you mentioned(thanks Kursad for pointing out). In our case, even >> the maximum fifo size usage(512bytes), the polling still have better >> performance than interrupt. The MTD test result included in this patch is >> based on maximum fifo usage. So there is no benefit to switch to interrupt >> based on transfer size. > >> Does the spi framework has any requirement on how much time that the >> driver's transfer_one function can spend? I can see the polling function >> might take quite some time in busy loop if the clock is low, for example, at >> 100Hz(slowest clock this controller can go), it takes 512x8/100Hz ~= 41ms to >> complete. If this is something in concern, I can do the interrupt switch >> based on a time limit value if interrupt is available. > > There's no fixed limit, some hardware just doesn't have DMA. Some > doesn't even have interrupts which is even better. If there's always a > clear benefit for using PIO then just do that, perhaps creating a sysfs > hook to allow people to switch to DMA if they need it (rather than > requiring people to update their DT, this is really a runtime > performance tradeoff rather than a description of the hardware). If > there's a point at which the performance is very similar then it's > probably worth switching to DMA there to lower the CPU usage, but if > it's always faster to use PIO then just defaulting to PIO seems like the > best option. > Sure. I will add the sysfs option. Interrupt node is now required in dts but by default it will still runs at polling mode until sysfs option is set to use interrupt.