| Message ID | 20230106200809.330769-4-william.zhang@broadcom.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | spi: bcm63xx-hsspi: driver and doc updates | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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 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 06/01/2023 21:07, 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. > +additionalProperties: true > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > index ead2cccf658f..f85d777c7b67 100644 > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > @@ -108,5 +108,6 @@ allOf: > - $ref: cdns,qspi-nor-peripheral-props.yaml# > - $ref: samsung,spi-peripheral-props.yaml# > - $ref: nvidia,tegra210-quad-peripheral-props.yaml# > + - $ref: brcm,bcm63xx-hsspi-peripheral-props.yaml# Don't break the order. 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 01/08/2023 06:52 AM, Krzysztof Kozlowski wrote: > On 06/01/2023 21:07, 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. > > >> +additionalProperties: true >> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml >> index ead2cccf658f..f85d777c7b67 100644 >> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml >> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml >> @@ -108,5 +108,6 @@ allOf: >> - $ref: cdns,qspi-nor-peripheral-props.yaml# >> - $ref: samsung,spi-peripheral-props.yaml# >> - $ref: nvidia,tegra210-quad-peripheral-props.yaml# >> + - $ref: brcm,bcm63xx-hsspi-peripheral-props.yaml# > > Don't break the order. > Will fix in v2 > Best regards, > Krzysztof >
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.
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 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.
diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi-peripheral-props.yaml new file mode 100644 index 000000000000..81884e2cc42d --- /dev/null +++ b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi-peripheral-props.yaml @@ -0,0 +1,27 @@ +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/spi/brcm,bcm63xx-hsspi-peripheral-props.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Peripheral-specific properties for Broadcom Broadband SoC HSSPI controller + +description: + See spi-peripheral-props.yaml for more info. + +maintainers: + - William Zhang <william.zhang@broadcom.com> + - Kursad Oney <kursad.oney@broadcom.com> + - Jonas Gorski <jonas.gorski@gmail.com> + +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. + +additionalProperties: true diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml index ead2cccf658f..f85d777c7b67 100644 --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml @@ -108,5 +108,6 @@ allOf: - $ref: cdns,qspi-nor-peripheral-props.yaml# - $ref: samsung,spi-peripheral-props.yaml# - $ref: nvidia,tegra210-quad-peripheral-props.yaml# + - $ref: brcm,bcm63xx-hsspi-peripheral-props.yaml# additionalProperties: true
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. Signed-off-by: William Zhang <william.zhang@broadcom.com> --- .../brcm,bcm63xx-hsspi-peripheral-props.yaml | 27 +++++++++++++++++++ .../bindings/spi/spi-peripheral-props.yaml | 1 + 2 files changed, 28 insertions(+) create mode 100644 Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi-peripheral-props.yaml