[03/16] dt-bindings: spi: Add spi peripheral specific property

  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
  

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.