diff mbox series

[v1,01/15] dt-bindings: add pwrseq device tree bindings

Message ID 20211006035407.1147909-2-dmitry.baryshkov@linaro.org (mailing list archive)
State New, archived
Headers show
Series create power sequencing subsystem | expand

Commit Message

Dmitry Baryshkov Oct. 6, 2021, 3:53 a.m. UTC
Add device tree bindings for the new power sequencer subsystem.
Consumers would reference pwrseq nodes using "foo-pwrseq" properties.
Providers would use '#pwrseq-cells' property to declare the amount of
cells in the pwrseq specifier.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../bindings/power/pwrseq/pwrseq.yaml         | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/pwrseq/pwrseq.yaml

Comments

Rob Herring Oct. 26, 2021, 12:53 p.m. UTC | #1
On Wed, Oct 06, 2021 at 06:53:53AM +0300, Dmitry Baryshkov wrote:
> Add device tree bindings for the new power sequencer subsystem.
> Consumers would reference pwrseq nodes using "foo-pwrseq" properties.
> Providers would use '#pwrseq-cells' property to declare the amount of
> cells in the pwrseq specifier.

Please use get_maintainers.pl.

This is not a pattern I want to encourage, so NAK on a common binding.

> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  .../bindings/power/pwrseq/pwrseq.yaml         | 32 +++++++++++++++++++
>  1 file changed, 32 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/pwrseq/pwrseq.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/pwrseq/pwrseq.yaml b/Documentation/devicetree/bindings/power/pwrseq/pwrseq.yaml
> new file mode 100644
> index 000000000000..4a8f6c0218bf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq.yaml
> @@ -0,0 +1,32 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/pwrseq/pwrseq.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Power Sequencer devices
> +
> +maintainers:
> +  - Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> +
> +properties:
> +  "#powerseq-cells":
> +    description:
> +      Number of cells in a pwrseq specifier.
> +
> +patternProperties:
> +  ".*-pwrseq$":
> +    description: Power sequencer supply phandle(s) for this node
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    qca_pwrseq: qca-pwrseq {
> +      #pwrseq-cells = <1>;
> +    };
> +
> +    bluetooth {
> +      bt-pwrseq = <&qca_pwrseq 1>;
> +    };
> +...
> -- 
> 2.33.0
> 
>
Dmitry Baryshkov Oct. 26, 2021, 2:42 p.m. UTC | #2
On 26/10/2021 15:53, Rob Herring wrote:
> On Wed, Oct 06, 2021 at 06:53:53AM +0300, Dmitry Baryshkov wrote:
>> Add device tree bindings for the new power sequencer subsystem.
>> Consumers would reference pwrseq nodes using "foo-pwrseq" properties.
>> Providers would use '#pwrseq-cells' property to declare the amount of
>> cells in the pwrseq specifier.
> 
> Please use get_maintainers.pl.
> 
> This is not a pattern I want to encourage, so NAK on a common binding.


Could you please spend a few more words, describing what is not 
encouraged? The whole foo-subsys/#subsys-cells structure?

Or just specifying the common binding?

> 
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   .../bindings/power/pwrseq/pwrseq.yaml         | 32 +++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/power/pwrseq/pwrseq.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/power/pwrseq/pwrseq.yaml b/Documentation/devicetree/bindings/power/pwrseq/pwrseq.yaml
>> new file mode 100644
>> index 000000000000..4a8f6c0218bf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq.yaml
>> @@ -0,0 +1,32 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/power/pwrseq/pwrseq.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Power Sequencer devices
>> +
>> +maintainers:
>> +  - Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> +
>> +properties:
>> +  "#powerseq-cells":
>> +    description:
>> +      Number of cells in a pwrseq specifier.
>> +
>> +patternProperties:
>> +  ".*-pwrseq$":
>> +    description: Power sequencer supply phandle(s) for this node
>> +
>> +additionalProperties: true
>> +
>> +examples:
>> +  - |
>> +    qca_pwrseq: qca-pwrseq {
>> +      #pwrseq-cells = <1>;
>> +    };
>> +
>> +    bluetooth {
>> +      bt-pwrseq = <&qca_pwrseq 1>;
>> +    };
>> +...
>> -- 
>> 2.33.0
>>
>>
Rob Herring Oct. 27, 2021, 9:53 p.m. UTC | #3
On Tue, Oct 26, 2021 at 9:42 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 26/10/2021 15:53, Rob Herring wrote:
> > On Wed, Oct 06, 2021 at 06:53:53AM +0300, Dmitry Baryshkov wrote:
> >> Add device tree bindings for the new power sequencer subsystem.
> >> Consumers would reference pwrseq nodes using "foo-pwrseq" properties.
> >> Providers would use '#pwrseq-cells' property to declare the amount of
> >> cells in the pwrseq specifier.
> >
> > Please use get_maintainers.pl.
> >
> > This is not a pattern I want to encourage, so NAK on a common binding.
>
>
> Could you please spend a few more words, describing what is not
> encouraged? The whole foo-subsys/#subsys-cells structure?

No, that's generally how common provider/consumer style bindings work.

> Or just specifying the common binding?

If we could do it again, I would not have mmc pwrseq binding. The
properties belong in the device's node. So don't generalize the mmc
pwrseq binding.

It's a kernel problem if the firmware says there's a device on a
'discoverable' bus and the kernel can't discover it. I know you have
the added complication of a device with 2 interfaces, but please,
let's solve one problem at a time.

Rob
Dmitry Baryshkov Nov. 2, 2021, 3:26 p.m. UTC | #4
On 28/10/2021 00:53, Rob Herring wrote:
> On Tue, Oct 26, 2021 at 9:42 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> On 26/10/2021 15:53, Rob Herring wrote:
>>> On Wed, Oct 06, 2021 at 06:53:53AM +0300, Dmitry Baryshkov wrote:
>>>> Add device tree bindings for the new power sequencer subsystem.
>>>> Consumers would reference pwrseq nodes using "foo-pwrseq" properties.
>>>> Providers would use '#pwrseq-cells' property to declare the amount of
>>>> cells in the pwrseq specifier.
>>>
>>> Please use get_maintainers.pl.
>>>
>>> This is not a pattern I want to encourage, so NAK on a common binding.
>>
>>
>> Could you please spend a few more words, describing what is not
>> encouraged? The whole foo-subsys/#subsys-cells structure?
> 
> No, that's generally how common provider/consumer style bindings work.
> 
>> Or just specifying the common binding?
> 
> If we could do it again, I would not have mmc pwrseq binding. The
> properties belong in the device's node. So don't generalize the mmc
> pwrseq binding.
> 
> It's a kernel problem if the firmware says there's a device on a
> 'discoverable' bus and the kernel can't discover it. I know you have
> the added complication of a device with 2 interfaces, but please,
> let's solve one problem at a time.

The PCI bus handling is a separate topic for now (as you have seen from 
the clearly WIP patches targeting just testing of qca6390's wifi part).

For me there are three parts of the device:
- power regulator / device embedded power domain.
- WiFi
- Bluetooth

With the power regulator being a complex and a bit nasty beast. It has 
several regulators beneath, which have to be powered up in a proper way.
Next platforms might bring additional requirements common to both WiFi 
and BT parts (like having additional clocks, etc). It is externally 
controlled (after providing power to it you have to tell, which part of 
the chip is required by pulling up the WiFi and/or BT enable GPIOs.

Having to duplicate this information in BT and WiFi cases results in 
non-aligned bindings (with WiFi and BT parts using different set of 
properties and different property names) and non-algined drivers (so the 
result of the powerup would depend on the order of drivers probing).

So far I still suppose that having a single separate entity controlling 
the powerup of such chips is the right thing to do.

I'd prefer to use the power-domain bindings (as the idea seems to be 
aligned here), but as the power-domain is used for the in-chip power 
domains, we had to invent the pwrseq name.
Srinivas Kandagatla March 10, 2023, 7:56 a.m. UTC | #5
On 02/11/2021 15:26, Dmitry Baryshkov wrote:
> On 28/10/2021 00:53, Rob Herring wrote:
>> On Tue, Oct 26, 2021 at 9:42 AM Dmitry Baryshkov
>> <dmitry.baryshkov@linaro.org> wrote:
>>>
>>> On 26/10/2021 15:53, Rob Herring wrote:
>>>> On Wed, Oct 06, 2021 at 06:53:53AM +0300, Dmitry Baryshkov wrote:
>>>>> Add device tree bindings for the new power sequencer subsystem.
>>>>> Consumers would reference pwrseq nodes using "foo-pwrseq" properties.
>>>>> Providers would use '#pwrseq-cells' property to declare the amount of
>>>>> cells in the pwrseq specifier.
>>>>
>>>> Please use get_maintainers.pl.
>>>>
>>>> This is not a pattern I want to encourage, so NAK on a common binding.
>>>
>>>
>>> Could you please spend a few more words, describing what is not
>>> encouraged? The whole foo-subsys/#subsys-cells structure?
>>
>> No, that's generally how common provider/consumer style bindings work.
>>
>>> Or just specifying the common binding?
>>
>> If we could do it again, I would not have mmc pwrseq binding. The
>> properties belong in the device's node. So don't generalize the mmc
>> pwrseq binding.
>>
>> It's a kernel problem if the firmware says there's a device on a
>> 'discoverable' bus and the kernel can't discover it. I know you have
>> the added complication of a device with 2 interfaces, but please,
>> let's solve one problem at a time.

Just to keep this topic updated with some pointers [1] to changes done 
to solve same problem in USB Hub. These patches 
(drivers/usb/misc/onboard_usb_hub*) have been merged since last year July.

It looks like we can take some inspiration from this to address PCIE Bus 
issue aswell.

Thanks to Neil to point this.

[1] 
https://lore.kernel.org/lkml/20220630193530.2608178-1-mka@chromium.org/T/


--srini
> 
> The PCI bus handling is a separate topic for now (as you have seen from 
> the clearly WIP patches targeting just testing of qca6390's wifi part).
> 
> For me there are three parts of the device:
> - power regulator / device embedded power domain.
> - WiFi
> - Bluetooth
> 
> With the power regulator being a complex and a bit nasty beast. It has 
> several regulators beneath, which have to be powered up in a proper way.
> Next platforms might bring additional requirements common to both WiFi 
> and BT parts (like having additional clocks, etc). It is externally 
> controlled (after providing power to it you have to tell, which part of 
> the chip is required by pulling up the WiFi and/or BT enable GPIOs.
> 
> Having to duplicate this information in BT and WiFi cases results in 
> non-aligned bindings (with WiFi and BT parts using different set of 
> properties and different property names) and non-algined drivers (so the 
> result of the powerup would depend on the order of drivers probing).
> 
> So far I still suppose that having a single separate entity controlling 
> the powerup of such chips is the right thing to do.
> 
> I'd prefer to use the power-domain bindings (as the idea seems to be 
> aligned here), but as the power-domain is used for the in-chip power 
> domains, we had to invent the pwrseq name.
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/power/pwrseq/pwrseq.yaml b/Documentation/devicetree/bindings/power/pwrseq/pwrseq.yaml
new file mode 100644
index 000000000000..4a8f6c0218bf
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq.yaml
@@ -0,0 +1,32 @@ 
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/pwrseq/pwrseq.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Power Sequencer devices
+
+maintainers:
+  - Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
+
+properties:
+  "#powerseq-cells":
+    description:
+      Number of cells in a pwrseq specifier.
+
+patternProperties:
+  ".*-pwrseq$":
+    description: Power sequencer supply phandle(s) for this node
+
+additionalProperties: true
+
+examples:
+  - |
+    qca_pwrseq: qca-pwrseq {
+      #pwrseq-cells = <1>;
+    };
+
+    bluetooth {
+      bt-pwrseq = <&qca_pwrseq 1>;
+    };
+...