From: Yassine Oudjana <[email protected]>
Add DT schema for the switch-mode battery charger found on Qualcomm
PMICs such as PMI8994. Due to lack of documentation, some interrupt
descriptions might be inaccurate.
Signed-off-by: Yassine Oudjana <[email protected]>
---
.../bindings/power/supply/qcom,smbchg.yaml | 205 ++++++++++++++++++
MAINTAINERS | 8 +
2 files changed, 213 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
new file mode 100644
index 000000000000..d825a9c10b3e
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
@@ -0,0 +1,205 @@
+# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/supply/qcom,smbchg.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm PMIC Switch-Mode Battery Charger
+
+maintainers:
+ - Yassine Oudjana <[email protected]>
+ - Alejandro Tafalla <[email protected]>
+
+properties:
+ compatible:
+ enum:
+ - qcom,pmi8994-smbchg
+ - qcom,pmi8996-smbchg
+
+ reg:
+ maxItems: 1
+
+ monitored-battery:
+ description: |
+ phandle of battery characteristics node.
+ The charger uses the following properties:
+ - charge-term-current-microamp
+ - constant-charge-current-max-microamp
+ - voltage-max-design-microvolt
+ The constant-charge-current-max-microamp and voltage-max-design-microvolt
+ properties must be set.
+ See Documentation/devicetree/bindings/power/supply/battery.yaml
+
+ interrupts:
+ items:
+ - description: Charger error
+ - description: Charger inhibited
+ - description: Charger precharge safety timer timeout
+ - description: Charger charge safety timer timeout
+ - description: Charger pre to fast charging switch threshold reached
+ - description: Charger recharge threshold reached
+ - description: Charger taper threshold reached
+ - description: Charger charge termination threshold reached
+ - description: Battery hot
+ - description: Battery warm
+ - description: Battery cold
+ - description: Battery cool
+ - description: Battery overvoltage
+ - description: Battery low
+ - description: Battery missing
+ - description: Battery thermistor missing # unconfirmed
+ - description: USB input undervolt
+ - description: USB input overvolt
+ - description: USB input source detected
+ - description: OTG regulator failure
+ - description: OTG regulator overcurrent
+ - description: Automatic input current limiting done
+ - description: USB ID pin changed
+ - description: DC input undervolt
+ - description: DC input overvolt
+ - description: Power OK
+ - description: Temperature shutdown
+ - description: Watchdog timeout
+ - description: Flash failure
+ - description: OTST2 # unknown
+ - description: OTST3 # unknown
+
+ interrupt-names:
+ items:
+ - const: chg-error
+ - const: chg-inhibit
+ - const: chg-prechg-sft
+ - const: chg-complete-chg-sft
+ - const: chg-p2f-thr
+ - const: chg-rechg-thr
+ - const: chg-taper-thr
+ - const: chg-tcc-thr
+ - const: batt-hot
+ - const: batt-warm
+ - const: batt-cold
+ - const: batt-cool
+ - const: batt-ov
+ - const: batt-low
+ - const: batt-missing
+ - const: batt-term-missing
+ - const: usbin-uv
+ - const: usbin-ov
+ - const: usbin-src-det
+ - const: otg-fail
+ - const: otg-oc
+ - const: aicl-done
+ - const: usbid-change
+ - const: dcin-uv
+ - const: dcin-ov
+ - const: power-ok
+ - const: temp-shutdown
+ - const: wdog-timeout
+ - const: flash-fail
+ - const: otst2
+ - const: otst3
+
+ otg-vbus:
+ type: object
+
+ description:
+ OTG regulator subnode.
+
+required:
+ - compatible
+ - reg
+ - monitored-battery
+ - interrupts
+ - interrupt-names
+ - otg-vbus
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ battery: battery {
+ compatible = "simple-battery";
+
+ charge-full-design-microamp-hours = <4070000>;
+ charge-term-current-microamp = <100000>;
+ voltage-min-design-microvolt = <3400000>;
+ voltage-max-design-microvolt = <4400000>;
+ };
+
+ pmic {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ charger@1000 {
+ compatible = "qcom,pmi8996-smbchg";
+ reg = <0x1000>;
+
+ interrupts = <0x2 0x10 0x0 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x10 0x1 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x10 0x2 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x10 0x3 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x10 0x4 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x10 0x5 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x10 0x6 IRQ_TYPE_EDGE_RISING>,
+ <0x2 0x10 0x7 IRQ_TYPE_EDGE_RISING>,
+ <0x2 0x12 0x0 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x12 0x1 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x12 0x2 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x12 0x3 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x12 0x4 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x12 0x5 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x12 0x6 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x12 0x7 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x13 0x0 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x13 0x1 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x13 0x2 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x13 0x3 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x13 0x4 IRQ_TYPE_EDGE_RISING>,
+ <0x2 0x13 0x5 IRQ_TYPE_EDGE_RISING>,
+ <0x2 0x13 0x6 IRQ_TYPE_EDGE_FALLING>,
+ <0x2 0x14 0x0 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x14 0x1 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x16 0x0 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x16 0x1 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x16 0x2 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x16 0x3 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x16 0x4 IRQ_TYPE_EDGE_BOTH>,
+ <0x2 0x16 0x5 IRQ_TYPE_EDGE_BOTH>;
+ interrupt-names = "chg-error",
+ "chg-inhibit",
+ "chg-prechg-sft",
+ "chg-complete-chg-sft",
+ "chg-p2f-thr",
+ "chg-rechg-thr",
+ "chg-taper-thr",
+ "chg-tcc-thr",
+ "batt-hot",
+ "batt-warm",
+ "batt-cold",
+ "batt-cool",
+ "batt-ov",
+ "batt-low",
+ "batt-missing",
+ "batt-term-missing",
+ "usbin-uv",
+ "usbin-ov",
+ "usbin-src-det",
+ "otg-fail",
+ "otg-oc",
+ "aicl-done",
+ "usbid-change",
+ "dcin-uv",
+ "dcin-ov",
+ "power-ok",
+ "temp-shutdown",
+ "wdog-timeout",
+ "flash-fail",
+ "otst2",
+ "otst3";
+
+ monitored-battery = <&battery>;
+
+ otg-vbus { };
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 3796fd700727..7e9f5893c0eb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16951,6 +16951,14 @@ F: Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
F: drivers/net/ethernet/qualcomm/rmnet/
F: include/linux/if_rmnet.h
+QUALCOMM SMBCHG DRIVER
+M: Yassine Oudjana <[email protected]>
+M: Alejandro Tafalla <[email protected]>
+L: [email protected]
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
+
QUALCOMM TSENS THERMAL DRIVER
M: Amit Kucheria <[email protected]>
M: Thara Gopinath <[email protected]>
--
2.37.1
On 08/08/2022 10:34, Yassine Oudjana wrote:
> From: Yassine Oudjana <[email protected]>
>
> Add DT schema for the switch-mode battery charger found on Qualcomm
> PMICs such as PMI8994. Due to lack of documentation, some interrupt
> descriptions might be inaccurate.
>
> Signed-off-by: Yassine Oudjana <[email protected]>
> ---
> .../bindings/power/supply/qcom,smbchg.yaml | 205 ++++++++++++++++++
> MAINTAINERS | 8 +
> 2 files changed, 213 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
>
> diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
> new file mode 100644
> index 000000000000..d825a9c10b3e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
> @@ -0,0 +1,205 @@
> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/qcom,smbchg.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm PMIC Switch-Mode Battery Charger
> +
> +maintainers:
> + - Yassine Oudjana <[email protected]>
> + - Alejandro Tafalla <[email protected]>
> +
> +properties:
> + compatible:
> + enum:
> + - qcom,pmi8994-smbchg
> + - qcom,pmi8996-smbchg
> +
> + reg:
> + maxItems: 1
> +
> + monitored-battery:
> + description: |
> + phandle of battery characteristics node.
> + The charger uses the following properties:
> + - charge-term-current-microamp
> + - constant-charge-current-max-microamp
> + - voltage-max-design-microvolt
> + The constant-charge-current-max-microamp and voltage-max-design-microvolt
> + properties must be set.
> + See Documentation/devicetree/bindings/power/supply/battery.yaml
> +
> + interrupts:
> + items:
> + - description: Charger error
> + - description: Charger inhibited
> + - description: Charger precharge safety timer timeout
> + - description: Charger charge safety timer timeout
> + - description: Charger pre to fast charging switch threshold reached
> + - description: Charger recharge threshold reached
> + - description: Charger taper threshold reached
> + - description: Charger charge termination threshold reached
> + - description: Battery hot
> + - description: Battery warm
> + - description: Battery cold
> + - description: Battery cool
> + - description: Battery overvoltage
> + - description: Battery low
> + - description: Battery missing
> + - description: Battery thermistor missing # unconfirmed
> + - description: USB input undervolt
> + - description: USB input overvolt
> + - description: USB input source detected
> + - description: OTG regulator failure
> + - description: OTG regulator overcurrent
> + - description: Automatic input current limiting done
> + - description: USB ID pin changed
> + - description: DC input undervolt
> + - description: DC input overvolt
> + - description: Power OK
> + - description: Temperature shutdown
> + - description: Watchdog timeout
> + - description: Flash failure
> + - description: OTST2 # unknown
> + - description: OTST3 # unknown
It seems you listed register interrupts, not physical pins. This should
be interrupt lines.
Best regards,
Krzysztof
On Mon, 8 Aug 2022 11:42:34 +0300, Krzysztof Kozlowski <[email protected]> wrote:
> On 08/08/2022 10:34, Yassine Oudjana wrote:
> > From: Yassine Oudjana <[email protected]>
> >
> > Add DT schema for the switch-mode battery charger found on Qualcomm
> > PMICs such as PMI8994. Due to lack of documentation, some interrupt
> > descriptions might be inaccurate.
> >
> > Signed-off-by: Yassine Oudjana <[email protected]>
> > ---
> > .../bindings/power/supply/qcom,smbchg.yaml | 205 ++++++++++++++++++
> > MAINTAINERS | 8 +
> > 2 files changed, 213 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
> > new file mode 100644
> > index 000000000000..d825a9c10b3e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
> > @@ -0,0 +1,205 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/power/supply/qcom,smbchg.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm PMIC Switch-Mode Battery Charger
> > +
> > +maintainers:
> > + - Yassine Oudjana <[email protected]>
> > + - Alejandro Tafalla <[email protected]>
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - qcom,pmi8994-smbchg
> > + - qcom,pmi8996-smbchg
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + monitored-battery:
> > + description: |
> > + phandle of battery characteristics node.
> > + The charger uses the following properties:
> > + - charge-term-current-microamp
> > + - constant-charge-current-max-microamp
> > + - voltage-max-design-microvolt
> > + The constant-charge-current-max-microamp and voltage-max-design-microvolt
> > + properties must be set.
> > + See Documentation/devicetree/bindings/power/supply/battery.yaml
> > +
> > + interrupts:
> > + items:
> > + - description: Charger error
> > + - description: Charger inhibited
> > + - description: Charger precharge safety timer timeout
> > + - description: Charger charge safety timer timeout
> > + - description: Charger pre to fast charging switch threshold reached
> > + - description: Charger recharge threshold reached
> > + - description: Charger taper threshold reached
> > + - description: Charger charge termination threshold reached
> > + - description: Battery hot
> > + - description: Battery warm
> > + - description: Battery cold
> > + - description: Battery cool
> > + - description: Battery overvoltage
> > + - description: Battery low
> > + - description: Battery missing
> > + - description: Battery thermistor missing # unconfirmed
> > + - description: USB input undervolt
> > + - description: USB input overvolt
> > + - description: USB input source detected
> > + - description: OTG regulator failure
> > + - description: OTG regulator overcurrent
> > + - description: Automatic input current limiting done
> > + - description: USB ID pin changed
> > + - description: DC input undervolt
> > + - description: DC input overvolt
> > + - description: Power OK
> > + - description: Temperature shutdown
> > + - description: Watchdog timeout
> > + - description: Flash failure
> > + - description: OTST2 # unknown
> > + - description: OTST3 # unknown
>
> It seems you listed register interrupts, not physical pins. This should
> be interrupt lines.
I'm not sure what I'm supposed to do here. I couldn't find an interrupt-lines
property used anywhere so that's not what you meant, right?
On 20/11/2022 16:46, Yassine Oudjana wrote:
>>> + interrupts:
>>> + items:
>>> + - description: Charger error
>>> + - description: Charger inhibited
>>> + - description: Charger precharge safety timer timeout
>>> + - description: Charger charge safety timer timeout
>>> + - description: Charger pre to fast charging switch threshold reached
>>> + - description: Charger recharge threshold reached
>>> + - description: Charger taper threshold reached
>>> + - description: Charger charge termination threshold reached
>>> + - description: Battery hot
>>> + - description: Battery warm
>>> + - description: Battery cold
>>> + - description: Battery cool
>>> + - description: Battery overvoltage
>>> + - description: Battery low
>>> + - description: Battery missing
>>> + - description: Battery thermistor missing # unconfirmed
>>> + - description: USB input undervolt
>>> + - description: USB input overvolt
>>> + - description: USB input source detected
>>> + - description: OTG regulator failure
>>> + - description: OTG regulator overcurrent
>>> + - description: Automatic input current limiting done
>>> + - description: USB ID pin changed
>>> + - description: DC input undervolt
>>> + - description: DC input overvolt
>>> + - description: Power OK
>>> + - description: Temperature shutdown
>>> + - description: Watchdog timeout
>>> + - description: Flash failure
>>> + - description: OTST2 # unknown
>>> + - description: OTST3 # unknown
>>
>> It seems you listed register interrupts, not physical pins. This should
>> be interrupt lines.
>
> I'm not sure what I'm supposed to do here. I couldn't find an interrupt-lines
> property used anywhere so that's not what you meant, right?
Are these physical interrupt lines this device has, register offsets or
virtual interrupts (e.g. passed via irq_chip)? Definitely not the first
and rather offsets for qpnpint_irq_domain_translate. Devicetree is not
for describing register layout of devices. IOW, register layout does not
change on different boards, because the device is exactly the same, so
there is no point to put it into DTS.
Best regards,
Krzysztof
On Mon, Nov 21 2022 at 09:26:59 +01:00:00, Krzysztof Kozlowski
<[email protected]> wrote:
> On 20/11/2022 16:46, Yassine Oudjana wrote:
>>>> + interrupts:
>>>> + items:
>>>> + - description: Charger error
>>>> + - description: Charger inhibited
>>>> + - description: Charger precharge safety timer timeout
>>>> + - description: Charger charge safety timer timeout
>>>> + - description: Charger pre to fast charging switch
>>>> threshold reached
>>>> + - description: Charger recharge threshold reached
>>>> + - description: Charger taper threshold reached
>>>> + - description: Charger charge termination threshold reached
>>>> + - description: Battery hot
>>>> + - description: Battery warm
>>>> + - description: Battery cold
>>>> + - description: Battery cool
>>>> + - description: Battery overvoltage
>>>> + - description: Battery low
>>>> + - description: Battery missing
>>>> + - description: Battery thermistor missing # unconfirmed
>>>> + - description: USB input undervolt
>>>> + - description: USB input overvolt
>>>> + - description: USB input source detected
>>>> + - description: OTG regulator failure
>>>> + - description: OTG regulator overcurrent
>>>> + - description: Automatic input current limiting done
>>>> + - description: USB ID pin changed
>>>> + - description: DC input undervolt
>>>> + - description: DC input overvolt
>>>> + - description: Power OK
>>>> + - description: Temperature shutdown
>>>> + - description: Watchdog timeout
>>>> + - description: Flash failure
>>>> + - description: OTST2 # unknown
>>>> + - description: OTST3 # unknown
>>>
>>> It seems you listed register interrupts, not physical pins. This
>>> should
>>> be interrupt lines.
>>
>> I'm not sure what I'm supposed to do here. I couldn't find an
>> interrupt-lines
>> property used anywhere so that's not what you meant, right?
>
> Are these physical interrupt lines this device has, register offsets
> or
> virtual interrupts (e.g. passed via irq_chip)? Definitely not the
> first
> and rather offsets for qpnpint_irq_domain_translate. Devicetree is not
> for describing register layout of devices. IOW, register layout does
> not
> change on different boards, because the device is exactly the same, so
> there is no point to put it into DTS.
>
So how would I describe the interrupts then? Or if you are saying I
shouldn't have these interrupts in DT at all, how would I get them and
register handlers for them in the driver? the PMIC arbiter takes 4
interrupt cells, 3 of which are these offsets specifying the peripheral
and interrupt. All other PMIC peripherals currently described in DT
(examples being qcom,pm8916-wcd-analog-codec, qcom,pm8941-pwrkey and
qcom-wled) have their interrupts (if any) described this way, with the
only exceptions perhaps being the GPIO and MPP controllers which are
themselves interrupt controllers. Changing the way PMIC peripheral
interrupts are described would require changing PMIC arbiter bindings
and code which I believe is out of the scope of this patch series.
On 21/11/2022 11:36, Yassine Oudjana wrote:
>
> On Mon, Nov 21 2022 at 09:26:59 +01:00:00, Krzysztof Kozlowski
> <[email protected]> wrote:
>> On 20/11/2022 16:46, Yassine Oudjana wrote:
>>>>> + interrupts:
>>>>> + items:
>>>>> + - description: Charger error
>>>>> + - description: Charger inhibited
>>>>> + - description: Charger precharge safety timer timeout
>>>>> + - description: Charger charge safety timer timeout
>>>>> + - description: Charger pre to fast charging switch
>>>>> threshold reached
>>>>> + - description: Charger recharge threshold reached
>>>>> + - description: Charger taper threshold reached
>>>>> + - description: Charger charge termination threshold reached
>>>>> + - description: Battery hot
>>>>> + - description: Battery warm
>>>>> + - description: Battery cold
>>>>> + - description: Battery cool
>>>>> + - description: Battery overvoltage
>>>>> + - description: Battery low
>>>>> + - description: Battery missing
>>>>> + - description: Battery thermistor missing # unconfirmed
>>>>> + - description: USB input undervolt
>>>>> + - description: USB input overvolt
>>>>> + - description: USB input source detected
>>>>> + - description: OTG regulator failure
>>>>> + - description: OTG regulator overcurrent
>>>>> + - description: Automatic input current limiting done
>>>>> + - description: USB ID pin changed
>>>>> + - description: DC input undervolt
>>>>> + - description: DC input overvolt
>>>>> + - description: Power OK
>>>>> + - description: Temperature shutdown
>>>>> + - description: Watchdog timeout
>>>>> + - description: Flash failure
>>>>> + - description: OTST2 # unknown
>>>>> + - description: OTST3 # unknown
>>>>
>>>> It seems you listed register interrupts, not physical pins. This
>>>> should
>>>> be interrupt lines.
>>>
>>> I'm not sure what I'm supposed to do here. I couldn't find an
>>> interrupt-lines
>>> property used anywhere so that's not what you meant, right?
>>
>> Are these physical interrupt lines this device has, register offsets
>> or
>> virtual interrupts (e.g. passed via irq_chip)? Definitely not the
>> first
>> and rather offsets for qpnpint_irq_domain_translate. Devicetree is not
>> for describing register layout of devices. IOW, register layout does
>> not
>> change on different boards, because the device is exactly the same, so
>> there is no point to put it into DTS.
>>
>
> So how would I describe the interrupts then? Or if you are saying I
> shouldn't have these interrupts in DT at all, how would I get them and
> register handlers for them in the driver? the PMIC arbiter takes 4
> interrupt cells, 3 of which are these offsets specifying the peripheral
> and interrupt. All other PMIC peripherals currently described in DT
> (examples being qcom,pm8916-wcd-analog-codec, qcom,pm8941-pwrkey and
> qcom-wled) have their interrupts (if any) described this way, with the
> only exceptions perhaps being the GPIO and MPP controllers which are
> themselves interrupt controllers. Changing the way PMIC peripheral
> interrupts are described would require changing PMIC arbiter bindings
> and code which I believe is out of the scope of this patch series.
I don't think this would touch PMIC arbiter bindings, rather the PMIC
itself. Usually complex devices (like PMICs) have one few physical
interrupt lines and many registers related to some specific interrupts.
For example:
1. One IRQ line,
2. Register with bits for overvoltage, undervoltage, vharger error etc.
Now how the MFD child device accesses them. Since this is strictly
related to hardware programming model, it's not something you put to
Devicetree. Instead parent device (PMIC) registers IRQ chip for its one
interrupt line with several Linux (or virtual) interrupts. The children
then just get a virtual IRQ from the parent (PMIC) and setup a
handler(s) for them.
You will find some examples for this, just grep for regmap_irq_get_virq,
for the drivers using regmap_irq_chip (or irq_create_mapping for
non-regmaps).
Since it is *one* device - the PMIC and its child like charger - the
register layout is fixed thus I think these virtual (or Linux)
interrupts are fixed as well.
I don't know why Qualcomm PMIC for SPMI is done differently.
Best regards,
Krzysztof
Hi,
On Mon, 21 Nov 2022 at 19:07, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 21/11/2022 11:36, Yassine Oudjana wrote:
> >
> > On Mon, Nov 21 2022 at 09:26:59 +01:00:00, Krzysztof Kozlowski
> > <[email protected]> wrote:
> >> On 20/11/2022 16:46, Yassine Oudjana wrote:
> >>>>> + interrupts:
> >>>>> + items:
> >>>>> + - description: Charger error
> >>>>> + - description: Charger inhibited
> >>>>> + - description: Charger precharge safety timer timeout
> >>>>> + - description: Charger charge safety timer timeout
> >>>>> + - description: Charger pre to fast charging switch
> >>>>> threshold reached
> >>>>> + - description: Charger recharge threshold reached
> >>>>> + - description: Charger taper threshold reached
> >>>>> + - description: Charger charge termination threshold reached
> >>>>> + - description: Battery hot
> >>>>> + - description: Battery warm
> >>>>> + - description: Battery cold
> >>>>> + - description: Battery cool
> >>>>> + - description: Battery overvoltage
> >>>>> + - description: Battery low
> >>>>> + - description: Battery missing
> >>>>> + - description: Battery thermistor missing # unconfirmed
> >>>>> + - description: USB input undervolt
> >>>>> + - description: USB input overvolt
> >>>>> + - description: USB input source detected
> >>>>> + - description: OTG regulator failure
> >>>>> + - description: OTG regulator overcurrent
> >>>>> + - description: Automatic input current limiting done
> >>>>> + - description: USB ID pin changed
> >>>>> + - description: DC input undervolt
> >>>>> + - description: DC input overvolt
> >>>>> + - description: Power OK
> >>>>> + - description: Temperature shutdown
> >>>>> + - description: Watchdog timeout
> >>>>> + - description: Flash failure
> >>>>> + - description: OTST2 # unknown
> >>>>> + - description: OTST3 # unknown
> >>>>
> >>>> It seems you listed register interrupts, not physical pins. This
> >>>> should
> >>>> be interrupt lines.
> >>>
> >>> I'm not sure what I'm supposed to do here. I couldn't find an
> >>> interrupt-lines
> >>> property used anywhere so that's not what you meant, right?
> >>
> >> Are these physical interrupt lines this device has, register offsets
> >> or
> >> virtual interrupts (e.g. passed via irq_chip)? Definitely not the
> >> first
> >> and rather offsets for qpnpint_irq_domain_translate. Devicetree is not
> >> for describing register layout of devices. IOW, register layout does
> >> not
> >> change on different boards, because the device is exactly the same, so
> >> there is no point to put it into DTS.
> >>
> >
> > So how would I describe the interrupts then? Or if you are saying I
> > shouldn't have these interrupts in DT at all, how would I get them and
> > register handlers for them in the driver? the PMIC arbiter takes 4
> > interrupt cells, 3 of which are these offsets specifying the peripheral
> > and interrupt. All other PMIC peripherals currently described in DT
> > (examples being qcom,pm8916-wcd-analog-codec, qcom,pm8941-pwrkey and
> > qcom-wled) have their interrupts (if any) described this way, with the
> > only exceptions perhaps being the GPIO and MPP controllers which are
> > themselves interrupt controllers. Changing the way PMIC peripheral
> > interrupts are described would require changing PMIC arbiter bindings
> > and code which I believe is out of the scope of this patch series.
>
> I don't think this would touch PMIC arbiter bindings, rather the PMIC
> itself. Usually complex devices (like PMICs) have one few physical
> interrupt lines and many registers related to some specific interrupts.
> For example:
> 1. One IRQ line,
> 2. Register with bits for overvoltage, undervoltage, vharger error etc.
>
> Now how the MFD child device accesses them. Since this is strictly
> related to hardware programming model, it's not something you put to
> Devicetree. Instead parent device (PMIC) registers IRQ chip for its one
> interrupt line with several Linux (or virtual) interrupts. The children
> then just get a virtual IRQ from the parent (PMIC) and setup a
> handler(s) for them.
Unfortunately this is not how SPMI PMICs work (at least on the
Qualcomm platforms). Access to interrupt registers is handled via the
SPMI bus arbiter writes, not through the GPIO pin or typical spmi's
bus interface (in the other words, not through the PMIC's SPMI
regmap). I guess we can add an intermediate irq chip to automatically
handle the USID, etc. However I doubt that it will really bring a lot
in our case.
> You will find some examples for this, just grep for regmap_irq_get_virq,
> for the drivers using regmap_irq_chip (or irq_create_mapping for
> non-regmaps).
>
> Since it is *one* device - the PMIC and its child like charger - the
> register layout is fixed thus I think these virtual (or Linux)
> interrupts are fixed as well.
>
> I don't know why Qualcomm PMIC for SPMI is done differently.
>
> Best regards,
> Krzysztof
>
--
With best wishes
Dmitry
On 22/11/2022 14:30, Dmitry Baryshkov wrote:
> Hi,
>
> On Mon, 21 Nov 2022 at 19:07, Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 21/11/2022 11:36, Yassine Oudjana wrote:
>>>
>>> On Mon, Nov 21 2022 at 09:26:59 +01:00:00, Krzysztof Kozlowski
>>> <[email protected]> wrote:
>>>> On 20/11/2022 16:46, Yassine Oudjana wrote:
>>>>>>> + interrupts:
>>>>>>> + items:
>>>>>>> + - description: Charger error
>>>>>>> + - description: Charger inhibited
>>>>>>> + - description: Charger precharge safety timer timeout
>>>>>>> + - description: Charger charge safety timer timeout
>>>>>>> + - description: Charger pre to fast charging switch
>>>>>>> threshold reached
>>>>>>> + - description: Charger recharge threshold reached
>>>>>>> + - description: Charger taper threshold reached
>>>>>>> + - description: Charger charge termination threshold reached
>>>>>>> + - description: Battery hot
>>>>>>> + - description: Battery warm
>>>>>>> + - description: Battery cold
>>>>>>> + - description: Battery cool
>>>>>>> + - description: Battery overvoltage
>>>>>>> + - description: Battery low
>>>>>>> + - description: Battery missing
>>>>>>> + - description: Battery thermistor missing # unconfirmed
>>>>>>> + - description: USB input undervolt
>>>>>>> + - description: USB input overvolt
>>>>>>> + - description: USB input source detected
>>>>>>> + - description: OTG regulator failure
>>>>>>> + - description: OTG regulator overcurrent
>>>>>>> + - description: Automatic input current limiting done
>>>>>>> + - description: USB ID pin changed
>>>>>>> + - description: DC input undervolt
>>>>>>> + - description: DC input overvolt
>>>>>>> + - description: Power OK
>>>>>>> + - description: Temperature shutdown
>>>>>>> + - description: Watchdog timeout
>>>>>>> + - description: Flash failure
>>>>>>> + - description: OTST2 # unknown
>>>>>>> + - description: OTST3 # unknown
>>>>>>
>>>>>> It seems you listed register interrupts, not physical pins. This
>>>>>> should
>>>>>> be interrupt lines.
>>>>>
>>>>> I'm not sure what I'm supposed to do here. I couldn't find an
>>>>> interrupt-lines
>>>>> property used anywhere so that's not what you meant, right?
>>>>
>>>> Are these physical interrupt lines this device has, register offsets
>>>> or
>>>> virtual interrupts (e.g. passed via irq_chip)? Definitely not the
>>>> first
>>>> and rather offsets for qpnpint_irq_domain_translate. Devicetree is not
>>>> for describing register layout of devices. IOW, register layout does
>>>> not
>>>> change on different boards, because the device is exactly the same, so
>>>> there is no point to put it into DTS.
>>>>
>>>
>>> So how would I describe the interrupts then? Or if you are saying I
>>> shouldn't have these interrupts in DT at all, how would I get them and
>>> register handlers for them in the driver? the PMIC arbiter takes 4
>>> interrupt cells, 3 of which are these offsets specifying the peripheral
>>> and interrupt. All other PMIC peripherals currently described in DT
>>> (examples being qcom,pm8916-wcd-analog-codec, qcom,pm8941-pwrkey and
>>> qcom-wled) have their interrupts (if any) described this way, with the
>>> only exceptions perhaps being the GPIO and MPP controllers which are
>>> themselves interrupt controllers. Changing the way PMIC peripheral
>>> interrupts are described would require changing PMIC arbiter bindings
>>> and code which I believe is out of the scope of this patch series.
>>
>> I don't think this would touch PMIC arbiter bindings, rather the PMIC
>> itself. Usually complex devices (like PMICs) have one few physical
>> interrupt lines and many registers related to some specific interrupts.
>> For example:
>> 1. One IRQ line,
>> 2. Register with bits for overvoltage, undervoltage, vharger error etc.
>>
>> Now how the MFD child device accesses them. Since this is strictly
>> related to hardware programming model, it's not something you put to
>> Devicetree. Instead parent device (PMIC) registers IRQ chip for its one
>> interrupt line with several Linux (or virtual) interrupts. The children
>> then just get a virtual IRQ from the parent (PMIC) and setup a
>> handler(s) for them.
>
> Unfortunately this is not how SPMI PMICs work (at least on the
> Qualcomm platforms). Access to interrupt registers is handled via the
> SPMI bus arbiter writes, not through the GPIO pin or typical spmi's
> bus interface (in the other words, not through the PMIC's SPMI
> regmap).
I am not sure how this is related... Just because they do not use same
regmap/interface does not mean that child device should have register
bits as interrupt sources. Do you model I2C PMICs devices the same way?
No. They get the interrupts from the parent and how the parent handles
them (same or different regmap) is separate problem.
The charger node does not make SPMI bus as interrupt parent, so these
interrupts are going to the SPMI PMIC don't they? or is it mistake in
DTS - lack of interrupt-parent?
> I guess we can add an intermediate irq chip to automatically
> handle the USID, etc. However I doubt that it will really bring a lot
> in our case.
The charger node defines all interrupts with SID=2, which is also not
really correct. The parent device is SID=2. The child - does not matter.
DT is a tree for some reason...
Best regards,
Krzysztof
Hi,
On Mon, 28 Nov 2022 at 13:39, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 22/11/2022 14:30, Dmitry Baryshkov wrote:
> > Hi,
> >
> > On Mon, 21 Nov 2022 at 19:07, Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 21/11/2022 11:36, Yassine Oudjana wrote:
> >>>
> >>> On Mon, Nov 21 2022 at 09:26:59 +01:00:00, Krzysztof Kozlowski
> >>> <[email protected]> wrote:
> >>>> On 20/11/2022 16:46, Yassine Oudjana wrote:
> >>>>>>> + interrupts:
> >>>>>>> + items:
> >>>>>>> + - description: Charger error
> >>>>>>> + - description: Charger inhibited
> >>>>>>> + - description: Charger precharge safety timer timeout
> >>>>>>> + - description: Charger charge safety timer timeout
> >>>>>>> + - description: Charger pre to fast charging switch
> >>>>>>> threshold reached
> >>>>>>> + - description: Charger recharge threshold reached
> >>>>>>> + - description: Charger taper threshold reached
> >>>>>>> + - description: Charger charge termination threshold reached
> >>>>>>> + - description: Battery hot
> >>>>>>> + - description: Battery warm
> >>>>>>> + - description: Battery cold
> >>>>>>> + - description: Battery cool
> >>>>>>> + - description: Battery overvoltage
> >>>>>>> + - description: Battery low
> >>>>>>> + - description: Battery missing
> >>>>>>> + - description: Battery thermistor missing # unconfirmed
> >>>>>>> + - description: USB input undervolt
> >>>>>>> + - description: USB input overvolt
> >>>>>>> + - description: USB input source detected
> >>>>>>> + - description: OTG regulator failure
> >>>>>>> + - description: OTG regulator overcurrent
> >>>>>>> + - description: Automatic input current limiting done
> >>>>>>> + - description: USB ID pin changed
> >>>>>>> + - description: DC input undervolt
> >>>>>>> + - description: DC input overvolt
> >>>>>>> + - description: Power OK
> >>>>>>> + - description: Temperature shutdown
> >>>>>>> + - description: Watchdog timeout
> >>>>>>> + - description: Flash failure
> >>>>>>> + - description: OTST2 # unknown
> >>>>>>> + - description: OTST3 # unknown
> >>>>>>
> >>>>>> It seems you listed register interrupts, not physical pins. This
> >>>>>> should
> >>>>>> be interrupt lines.
> >>>>>
> >>>>> I'm not sure what I'm supposed to do here. I couldn't find an
> >>>>> interrupt-lines
> >>>>> property used anywhere so that's not what you meant, right?
> >>>>
> >>>> Are these physical interrupt lines this device has, register offsets
> >>>> or
> >>>> virtual interrupts (e.g. passed via irq_chip)? Definitely not the
> >>>> first
> >>>> and rather offsets for qpnpint_irq_domain_translate. Devicetree is not
> >>>> for describing register layout of devices. IOW, register layout does
> >>>> not
> >>>> change on different boards, because the device is exactly the same, so
> >>>> there is no point to put it into DTS.
> >>>>
> >>>
> >>> So how would I describe the interrupts then? Or if you are saying I
> >>> shouldn't have these interrupts in DT at all, how would I get them and
> >>> register handlers for them in the driver? the PMIC arbiter takes 4
> >>> interrupt cells, 3 of which are these offsets specifying the peripheral
> >>> and interrupt. All other PMIC peripherals currently described in DT
> >>> (examples being qcom,pm8916-wcd-analog-codec, qcom,pm8941-pwrkey and
> >>> qcom-wled) have their interrupts (if any) described this way, with the
> >>> only exceptions perhaps being the GPIO and MPP controllers which are
> >>> themselves interrupt controllers. Changing the way PMIC peripheral
> >>> interrupts are described would require changing PMIC arbiter bindings
> >>> and code which I believe is out of the scope of this patch series.
> >>
> >> I don't think this would touch PMIC arbiter bindings, rather the PMIC
> >> itself. Usually complex devices (like PMICs) have one few physical
> >> interrupt lines and many registers related to some specific interrupts.
> >> For example:
> >> 1. One IRQ line,
> >> 2. Register with bits for overvoltage, undervoltage, vharger error etc.
> >>
> >> Now how the MFD child device accesses them. Since this is strictly
> >> related to hardware programming model, it's not something you put to
> >> Devicetree. Instead parent device (PMIC) registers IRQ chip for its one
> >> interrupt line with several Linux (or virtual) interrupts. The children
> >> then just get a virtual IRQ from the parent (PMIC) and setup a
> >> handler(s) for them.
> >
> > Unfortunately this is not how SPMI PMICs work (at least on the
> > Qualcomm platforms). Access to interrupt registers is handled via the
> > SPMI bus arbiter writes, not through the GPIO pin or typical spmi's
> > bus interface (in the other words, not through the PMIC's SPMI
> > regmap).
>
> I am not sure how this is related... Just because they do not use same
> regmap/interface does not mean that child device should have register
> bits as interrupt sources. Do you model I2C PMICs devices the same way?
> No. They get the interrupts from the parent and how the parent handles
> them (same or different regmap) is separate problem.
For i2c PMICs it's typically not the case, since the interrupt is OOB.
It is an external GPIO pin, as you have described previously.
For Qcom SPMI PMIC the interrupts are really handled by the SPMI host
controller (arbieter). So the DT really represents the hardware.
>
> The charger node does not make SPMI bus as interrupt parent, so these
> interrupts are going to the SPMI PMIC don't they? or is it mistake in
> DTS - lack of interrupt-parent?
I thought that with the lack of the interrupt-parent, the OS will
traverse the tree up until it finds one. Is it so?
> > I guess we can add an intermediate irq chip to automatically
> > handle the USID, etc. However I doubt that it will really bring a lot
> > in our case.
>
> The charger node defines all interrupts with SID=2, which is also not
> really correct. The parent device is SID=2. The child - does not matter.
> DT is a tree for some reason...
Yes, I agree with you. The bindings do not look ideal here. I was
trying to point out that this is not a problem with the smbchg device
only. Other Qcom SPMI PMIC devices also use exactly the same approach
(except gpio and mpps).
So, we can either:
- Continue using the current approach
- Rework pmic driver to provide an intermediate IRQ domain. This would
require finding a way to ping SPMI ARB's IRQ registers from PMIC
controller (via callbacks, via extra interrupt translation, etc).
--
With best wishes
Dmitry
On 08/08/2022 09:34, Yassine Oudjana wrote:
> From: Yassine Oudjana <[email protected]>
>
> + See Documentation/devicetree/bindings/power/supply/battery.yaml
> +
> + interrupts:
> + items:
> + - description: Charger error
> + - description: Charger inhibited
> + - description: Charger precharge safety timer timeout
> + - description: Charger charge safety timer timeout
> + - description: Charger pre to fast charging switch threshold reached
> + - description: Charger recharge threshold reached
> + - description: Charger taper threshold reached
After discussing with Dmitry, it's current design of the PMIC (parent
device) so it's fine. We would need to make bigger refactoring of
drivers and bindings to change it to match other typical MFD PMICs. In
current state it's the only way to express device interrupts.
Two more comments below.
> + - description: Charger charge termination threshold reached
> + - description: Battery hot
> + - description: Battery warm
> + - description: Battery cold
> + - description: Battery cool
> + - description: Battery overvoltage
> + - description: Battery low
> + - description: Battery missing
> + - description: Battery thermistor missing # unconfirmed
> + - description: USB input undervolt
> + - description: USB input overvolt
> + - description: USB input source detected
> + - description: OTG regulator failure
> + - description: OTG regulator overcurrent
> + - description: Automatic input current limiting done
> + - description: USB ID pin changed
> + - description: DC input undervolt
> + - description: DC input overvolt
> + - description: Power OK
> + - description: Temperature shutdown
> + - description: Watchdog timeout
> + - description: Flash failure
> + - description: OTST2 # unknown
> + - description: OTST3 # unknown
> +
> + interrupt-names:
> + items:
> + - const: chg-error
> + - const: chg-inhibit
> + - const: chg-prechg-sft
> + - const: chg-complete-chg-sft
> + - const: chg-p2f-thr
> + - const: chg-rechg-thr
> + - const: chg-taper-thr
> + - const: chg-tcc-thr
> + - const: batt-hot
> + - const: batt-warm
> + - const: batt-cold
> + - const: batt-cool
> + - const: batt-ov
> + - const: batt-low
> + - const: batt-missing
> + - const: batt-term-missing
> + - const: usbin-uv
> + - const: usbin-ov
> + - const: usbin-src-det
> + - const: otg-fail
> + - const: otg-oc
> + - const: aicl-done
> + - const: usbid-change
> + - const: dcin-uv
> + - const: dcin-ov
> + - const: power-ok
> + - const: temp-shutdown
> + - const: wdog-timeout
> + - const: flash-fail
> + - const: otst2
> + - const: otst3
> +
> + otg-vbus:
> + type: object
I think I did not comment about this one - this looks like regulator.yaml.
> +
> + description:
> + OTG regulator subnode.
> +
> +required:
> + - compatible
> + - reg
> + - monitored-battery
> + - interrupts
> + - interrupt-names
> + - otg-vbus
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + battery: battery {
> + compatible = "simple-battery";
> +
> + charge-full-design-microamp-hours = <4070000>;
> + charge-term-current-microamp = <100000>;
> + voltage-min-design-microvolt = <3400000>;
> + voltage-max-design-microvolt = <4400000>;
> + };
> +
> + pmic {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + charger@1000 {
> + compatible = "qcom,pmi8996-smbchg";
> + reg = <0x1000>;
> +
> + interrupts = <0x2 0x10 0x0 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x10 0x1 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x10 0x2 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x10 0x3 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x10 0x4 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x10 0x5 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x10 0x6 IRQ_TYPE_EDGE_RISING>,
> + <0x2 0x10 0x7 IRQ_TYPE_EDGE_RISING>,
> + <0x2 0x12 0x0 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x12 0x1 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x12 0x2 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x12 0x3 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x12 0x4 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x12 0x5 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x12 0x6 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x12 0x7 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x13 0x0 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x13 0x1 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x13 0x2 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x13 0x3 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x13 0x4 IRQ_TYPE_EDGE_RISING>,
> + <0x2 0x13 0x5 IRQ_TYPE_EDGE_RISING>,
> + <0x2 0x13 0x6 IRQ_TYPE_EDGE_FALLING>,
> + <0x2 0x14 0x0 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x14 0x1 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x16 0x0 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x16 0x1 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x16 0x2 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x16 0x3 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x16 0x4 IRQ_TYPE_EDGE_BOTH>,
> + <0x2 0x16 0x5 IRQ_TYPE_EDGE_BOTH>;
> + interrupt-names = "chg-error",
> + "chg-inhibit",
> + "chg-prechg-sft",
> + "chg-complete-chg-sft",
> + "chg-p2f-thr",
> + "chg-rechg-thr",
> + "chg-taper-thr",
> + "chg-tcc-thr",
> + "batt-hot",
> + "batt-warm",
> + "batt-cold",
> + "batt-cool",
> + "batt-ov",
> + "batt-low",
> + "batt-missing",
> + "batt-term-missing",
> + "usbin-uv",
> + "usbin-ov",
> + "usbin-src-det",
> + "otg-fail",
> + "otg-oc",
> + "aicl-done",
> + "usbid-change",
> + "dcin-uv",
> + "dcin-ov",
> + "power-ok",
> + "temp-shutdown",
> + "wdog-timeout",
> + "flash-fail",
> + "otst2",
> + "otst3";
> +
> + monitored-battery = <&battery>;
> +
> + otg-vbus { };
Why empty?
Best regards,
Krzysztof