[permalink] [raw]

On Wed, Jun 13, 2018 at 01:12:45PM +0200, Linus Walleij wrote:
> On Wed, Jun 13, 2018 at 1:06 PM, Linus Walleij <[email protected]> wrote:
> > On Thu, Jun 7, 2018 at 8:12 PM, Craig Tatlor <[email protected]> wrote:
> >
> >> Adds a function to interpolate against two points,
> >> this is carried arount as a helper function by tons of drivers.
> >>
> >> Signed-off-by: Craig Tatlor <[email protected]>
> >
> > The linear formula seems to fit the most natural form of linear
> > interpolation.
> >
> > I bet some John Carmack algorithm type people will soon
> > start to optimize this... :D
>
> For your entertainment here is some sunday reading:
> https://fgiesen.wordpress.com/2012/08/15/linear-interpolation-past-present-and-future/
>
> Yours,
> Linus Walleij

Thanks, interesting read.

2018-06-13 16:08:15

by Craig Tatlor

[permalink] [raw]

Hi,

First of all thanks for the patch and big sorry for the long delay
in reviewing this. I did not find enough time to do it properly
until now :(

On Thu, Jun 14, 2018 at 04:14:15PM +0100, Craig Tatlor wrote:
> This patch adds a driver for the BMS (Battery Monitoring System)
> block of the PM8941 PMIC, it uses a lookup table defined in the
> device tree to generate a capacity from the BMS supplied OCV, it
> then amends the coulomb counter to that to increase the accuracy
> of the estimated capacity.
>
> Signed-off-by: Craig Tatlor <[email protected]>
> Reviewed-by: Linus Walleij <[email protected]>
> ---
>
> * Changes from v5:
> Uses select for REGMAP_SPMI.
>
> * Changes from v4:
> Cleaned up percentage interpolation function,
> uses new fixp interpolation helper,
> added some more error cases,
> uses devm_power_supply_register(),
> uses a DIV_ROUND_CLOSEST for division and
> uses micro(volts / amp hours) instead of
> milli (volts / amp hours).
>
> drivers/power/supply/Kconfig | 9 +
> drivers/power/supply/Makefile | 1 +
> drivers/power/supply/qcom_bms.c | 487 ++++++++++++++++++++++++++++++++
> 3 files changed, 497 insertions(+)
> create mode 100644 drivers/power/supply/qcom_bms.c
>
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 428b426842f4..75f2f375f992 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -82,6 +82,15 @@ config BATTERY_ACT8945A
> Say Y here to enable support for power supply provided by
> Active-semi ActivePath ACT8945A charger.
>
> +config BATTERY_BMS
> + tristate "Qualcomm Battery Monitoring System driver"
> + depends on MFD_SPMI_PMIC || COMPILE_TEST
> + depends on OF
> + select REGMAP_SPMI
> + help
> + Say Y to include support for the Battery Monitoring hardware
> + found in some Qualcomm PM series PMICs.
> +
> config BATTERY_CPCAP
> tristate "Motorola CPCAP PMIC battery driver"
> depends on MFD_CPCAP && IIO
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index e83aa843bcc6..04204174b047 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_BATTERY_88PM860X) += 88pm860x_battery.o
> obj-$(CONFIG_BATTERY_ACT8945A) += act8945a_charger.o
> obj-$(CONFIG_BATTERY_AXP20X) += axp20x_battery.o
> obj-$(CONFIG_CHARGER_AXP20X) += axp20x_ac_power.o
> +obj-$(CONFIG_BATTERY_BMS) += qcom_bms.o
> obj-$(CONFIG_BATTERY_CPCAP) += cpcap-battery.o
> obj-$(CONFIG_BATTERY_DS2760) += ds2760_battery.o
> obj-$(CONFIG_BATTERY_DS2780) += ds2780_battery.o
> diff --git a/drivers/power/supply/qcom_bms.c b/drivers/power/supply/qcom_bms.c
> new file mode 100644
> index 000000000000..718fd745c0f7
> --- /dev/null
> +++ b/drivers/power/supply/qcom_bms.c
> @@ -0,0 +1,487 @@
> +// SPDX-License-Identifier: GPL
> +
> +/*
> + * Qualcomm Battery Monitoring System driver
> + *
> + * Copyright (C) 2018 Craig Tatlor <[email protected]>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/fixp-arith.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/slab.h>
> +#include <linux/bitops.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/regmap.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/iio/consumer.h>
> +
> +#define REG_BMS_OCV_FOR_SOC_DATA0 0x90
> +#define REG_BMS_SHDW_CC_DATA0 0xA8
> +#define REG_BMS_CC_DATA_CTL 0x42
> +#define REG_BMS_CC_CLEAR_CTL 0x4
> +
> +#define BMS_HOLD_OREG_DATA BIT(0)
> +#define BMS_CLEAR_SHDW_CC BIT(6)
> +
> +#define BMS_CC_READING_RESOLUTION_N 542535
> +#define BMS_CC_READING_RESOLUTION_D 10000
> +#define BMS_CC_READING_TICKS 56
> +#define BMS_SLEEP_CLK_HZ 32764
> +
> +#define SECONDS_PER_HOUR 3600
> +#define TEMPERATURE_COLS 5
> +#define MAX_CAPACITY_ROWS 50
> +
> +/* lookup table for ocv -> capacity conversion */
> +struct bms_ocv_lut {
> + int rows;
> + s8 temp_legend[TEMPERATURE_COLS];
> + u8 capacity_legend[MAX_CAPACITY_ROWS];
> + u32 lut[MAX_CAPACITY_ROWS][TEMPERATURE_COLS];
> +};
> +
> +/* lookup table for battery temperature -> fcc conversion */
> +struct bms_fcc_lut {
> + s8 temp_legend[TEMPERATURE_COLS];
> + u32 lut[TEMPERATURE_COLS];
> +};
> +
> +struct bms_device_info {
> + struct device *dev;
> + struct regmap *regmap;
> + struct bms_ocv_lut ocv_lut;
> + struct power_supply_desc bat_desc;
> + struct bms_fcc_lut fcc_lut;
> + struct iio_channel *adc;
> + struct mutex bms_output_lock;
> + u32 base_addr;
> +
> + int ocv_thr_irq;
> + u32 ocv;
> +};
> +
> +static bool between(int left, int right, int val)
> +{
> + if (left <= val && val <= right)
> + return true;
> +
> + if (left >= val && val >= right)
> + return true;
> +
> + return false;
> +}
> +
> +static int interpolate_capacity(int temp, u32 ocv,
> + struct bms_ocv_lut *ocv_lut)
> +{
> + int pcj_minus_one = 0, pcj = 0, i2 = 0, i3 = 0, i, j;
> +
> + for (j = 0; j < TEMPERATURE_COLS; j++)
> + if (temp <= ocv_lut->temp_legend[j])
> + break;
> +
> + if (ocv >= ocv_lut->lut[0][j])
> + return ocv_lut->capacity_legend[0];
> +
> + if (ocv <= ocv_lut->lut[ocv_lut->rows-1][j-1])
> + return ocv_lut->capacity_legend[ocv_lut->rows-1];
> +
> + for (i = 0; i < ocv_lut->rows-1; i++) {
> + if (between(ocv_lut->lut[i][j],
> + ocv_lut->lut[i+1][j], ocv))
> + i2 = i;
> +
> + if (between(ocv_lut->lut[i][j-1],
> + ocv_lut->lut[i+1][j-1], ocv))
> + i3 = i;
> + }
> +
> + /* interpolate two capacities */
> + pcj = fixp_linear_interpolate(ocv_lut->lut[i2][j],
> + ocv_lut->capacity_legend[i2],
> + ocv_lut->lut[i2+1][j],
> + ocv_lut->capacity_legend[i2+1],
> + ocv);
> +
> + pcj_minus_one = fixp_linear_interpolate(ocv_lut->lut[i3][j-1],
> + ocv_lut->capacity_legend[i3],
> + ocv_lut->lut[i3+1][j-1],
> + ocv_lut->capacity_legend[i3+1],
> + ocv);
> +
> + /* interpolate them with the battery temperature */
> + return fixp_linear_interpolate(ocv_lut->temp_legend[j-1],
> + pcj_minus_one,
> + ocv_lut->temp_legend[j],
> + pcj,
> + temp);
> +}
> +
> +static int interpolate_fcc(int temp, struct bms_fcc_lut *fcc_lut)
> +{
> + int i;
> +
> + for (i = 0; i < TEMPERATURE_COLS; i++)
> + if (temp <= fcc_lut->temp_legend[i])
> + break;
> +
> + return fixp_linear_interpolate(fcc_lut->temp_legend[i-1],
> + fcc_lut->lut[i-1],
> + fcc_lut->temp_legend[i],
> + fcc_lut->lut[i],
> + temp);
> +}
> +
> +static int bms_lock_output_data(struct bms_device_info *di)
> +{
> + int ret;
> +
> + ret = regmap_update_bits(di->regmap, di->base_addr +
> + REG_BMS_CC_DATA_CTL,
> + BMS_HOLD_OREG_DATA, BMS_HOLD_OREG_DATA);
> + if (ret) {
> + dev_err(di->dev, "failed to lock bms output: %d", ret);
> + return ret;
> + }
> +
> + /*
> + * Sleep for at least 100 microseconds here to make sure
> + * there has been at least three cycles of the sleep clock
> + * so that the registers are correctly locked.
> + */
> + usleep_range(100, 1000);
> +
> + return 0;
> +}
> +
> +static int bms_unlock_output_data(struct bms_device_info *di)
> +{
> + int ret;
> +
> + ret = regmap_update_bits(di->regmap, di->base_addr +
> + REG_BMS_CC_DATA_CTL,
> + BMS_HOLD_OREG_DATA, 0);
> + if (ret) {
> + dev_err(di->dev, "failed to unlock bms output: %d", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int bms_read_ocv(struct bms_device_info *di, u32 *ocv)
> +{
> + int ret;
> + u16 read_ocv;
> +
> + mutex_lock(&di->bms_output_lock);
> +
> + ret = bms_lock_output_data(di);
> + if (ret)
> + goto err_lock;
> +
> + ret = regmap_bulk_read(di->regmap, di->base_addr+
> + REG_BMS_OCV_FOR_SOC_DATA0, &read_ocv, 2);
> + if (ret) {
> + dev_err(di->dev, "open circuit voltage read failed: %d", ret);
> + goto err_read;
> + }
> +
> + dev_dbg(di->dev, "read open circuit voltage of: %d mv", read_ocv);
> +
> +

one empty line is enough.

> + *ocv = read_ocv * 1000;
> +
> +err_read:
> + bms_unlock_output_data(di);
> +
> +err_lock:
> + mutex_unlock(&di->bms_output_lock);
> +
> + return ret;
> +}
> +
> +static int bms_read_cc(struct bms_device_info *di, s64 *cc_uah)
> +{
> + int ret;
> + s64 cc_raw_s36, cc_raw, cc_uv, cc_pvh;
> +
> + mutex_lock(&di->bms_output_lock);
> +
> + ret = bms_lock_output_data(di);
> + if (ret)
> + goto err_lock;
> +
> + ret = regmap_bulk_read(di->regmap, di->base_addr +
> + REG_BMS_SHDW_CC_DATA0,
> + &cc_raw_s36, 5);
> + if (ret) {
> + dev_err(di->dev, "coulomb counter read failed: %d", ret);
> + goto err_read;
> + }
> +
> + ret = bms_unlock_output_data(di);
> + if (ret)
> + goto err_lock;
> +
> + mutex_unlock(&di->bms_output_lock);
> +
> + cc_raw = sign_extend32(cc_raw_s36, 28);
> +
> + /* convert raw to uv */
> + cc_uv = div_s64(cc_raw * BMS_CC_READING_RESOLUTION_N,
> + BMS_CC_READING_RESOLUTION_D);
> +
> + /* convert uv to pvh */
> + cc_pvh = div_s64(cc_uv * BMS_CC_READING_TICKS * 100000,
> + BMS_SLEEP_CLK_HZ * SECONDS_PER_HOUR);
> +
> + /* divide by impedance */
> + *cc_uah = div_s64(cc_pvh, 10000);
> +
> + dev_dbg(di->dev, "read coulomb counter value of: %lld uah", *cc_uah);
> +
> + return 0;
> +
> +err_read:
> + bms_unlock_output_data(di);
> +
> +err_lock:
> + mutex_unlock(&di->bms_output_lock);
> +
> + return ret;
> +}
> +
> +static void bms_reset_cc(struct bms_device_info *di)
> +{
> + int ret;
> +
> + mutex_lock(&di->bms_output_lock);
> +
> + ret = regmap_update_bits(di->regmap, di->base_addr +
> + REG_BMS_CC_CLEAR_CTL,
> + BMS_CLEAR_SHDW_CC,
> + BMS_CLEAR_SHDW_CC);
> + if (ret) {
> + dev_err(di->dev, "coulomb counter reset failed: %d", ret);
> + goto err_lock;
> + }
> +
> + /* wait at least three sleep cycles for cc to reset */
> + usleep_range(100, 1000);
> +
> + ret = regmap_update_bits(di->regmap, di->base_addr +
> + REG_BMS_CC_CLEAR_CTL,
> + BMS_CLEAR_SHDW_CC, 0);
> + if (ret)
> + dev_err(di->dev, "coulomb counter re-enable failed: %d", ret);
> +
> +err_lock:
> + mutex_unlock(&di->bms_output_lock);
> +}
> +
> +static int bms_calculate_capacity(struct bms_device_info *di, int *capacity)
> +{
> + unsigned long fcc;
> + int ret, temp, ocv_capacity, temp_degc;
> + s64 cc = 0;
> +
> + ret = iio_read_channel_raw(di->adc, &temp);
> + if (ret < 0) {
> + dev_err(di->dev, "failed to read temperature: %d", ret);
> + return ret;
> + }
> +
> + temp_degc = DIV_ROUND_CLOSEST(temp, 1000);
> +
> + ret = bms_read_cc(di, &cc);
> + if (ret < 0) {
> + dev_err(di->dev, "failed to read coulomb counter: %d", ret);
> + return ret;
> + }
> +
> + /* interpolate capacity from open circuit voltage */
> + ocv_capacity = interpolate_capacity(temp_degc, di->ocv,
> + &di->ocv_lut);
> +
> + /* interpolate the full charge capacity from temperature */
> + fcc = interpolate_fcc(temp_degc, &di->fcc_lut);
> +
> + /* append coloumb counter to capacity */
> + *capacity = DIV_ROUND_CLOSEST(fcc * ocv_capacity, 100);
> + *capacity = div_s64((*capacity - cc) * 100, fcc);
> +
> + return 0;
> +}
> +
> +
> +

one empty line is enough.

> +/*
> + * Return power_supply property
> + */
> +static int bms_get_property(struct power_supply *psy,
> + enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + struct bms_device_info *di = power_supply_get_drvdata(psy);
> + int ret;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_CAPACITY:
> + ret = bms_calculate_capacity(di, &val->intval);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + if (val->intval == INT_MAX || val->intval == INT_MIN)
> + ret = -EINVAL;
> +
> + return ret;
> +}
> +
> +static enum power_supply_property bms_props[] = {
> + POWER_SUPPLY_PROP_CAPACITY,
> +};

Why does the driver not expose the following battery information?

temperature info via POWER_SUPPLY_PROP_TEMP
ocv info via POWER_SUPPLY_PROP_VOLTAGE_OCV
CC info via POWER_SUPPLY_PROP_CHARGE_COUNTER
max current via POWER_SUPPLY_PROP_CURRENT_MAX

> +static irqreturn_t bms_ocv_thr_irq_handler(int irq, void *dev_id)
> +{
> + struct bms_device_info *di = dev_id;
> +
> + if (bms_read_ocv(di, &di->ocv) < 0)
> + return IRQ_HANDLED;
> +
> + bms_reset_cc(di);
> + return IRQ_HANDLED;
> +}

You want to call power_supply_changed() here to notify userspace.

> +static int bms_probe(struct platform_device *pdev)
> +{
> + struct power_supply_config psy_cfg = {};
> + struct bms_device_info *di;
> + struct power_supply *bat;
> + int ret;
> +
> + di = devm_kzalloc(&pdev->dev, sizeof(*di), GFP_KERNEL);
> + if (!di)
> + return -ENOMEM;
> +
> + di->dev = &pdev->dev;
> +
> + di->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> + if (!di->regmap) {
> + dev_err(di->dev, "Unable to get regmap");
> + return -EINVAL;
> + }
> +
> + di->adc = devm_iio_channel_get(&pdev->dev, "temp");
> + if (IS_ERR(di->adc))
> + return PTR_ERR(di->adc);
> +
> + ret = of_property_read_u32(di->dev->of_node, "reg", &di->base_addr);
> + if (ret < 0)
> + return ret;
> +
> + ret = of_property_read_u8_array(di->dev->of_node,
> + "qcom,ocv-temp-legend-celsius",
> + (u8 *)di->ocv_lut.temp_legend,
> + TEMPERATURE_COLS);
> + if (ret < 0) {
> + dev_err(di->dev, "no open circuit voltage temperature legend found");
> + return ret;
> + }
> +
> + di->ocv_lut.rows = of_property_read_variable_u8_array(di->dev->of_node,
> + "qcom,ocv-capacity-legend",
> + di->ocv_lut.capacity_legend, 0,
> + MAX_CAPACITY_ROWS);
> + if (di->ocv_lut.rows < 0) {
> + dev_err(di->dev, "no open circuit voltage capacity legend found");
> + return ret;
> + }
> +
> + ret = of_property_read_variable_u32_array(di->dev->of_node,
> + "qcom,ocv-lut-microvolt",
> + (u32 *)di->ocv_lut.lut,
> + TEMPERATURE_COLS,
> + TEMPERATURE_COLS *
> + MAX_CAPACITY_ROWS);
> + if (ret < 0) {
> + dev_err(di->dev, "no open circuit voltage lut array found");
> + return ret;
> + }
> +
> + ret = of_property_read_u8_array(di->dev->of_node,
> + "qcom,fcc-temp-legend-celsius",
> + (u8 *)di->fcc_lut.temp_legend,
> + TEMPERATURE_COLS);
> + if (ret < 0) {
> + dev_err(di->dev, "no full charge capacity temperature legend found");
> + return ret;
> + }
> +
> + ret = of_property_read_u32_array(di->dev->of_node,
> + "qcom,fcc-lut-microamp-hours",
> + di->fcc_lut.lut,
> + TEMPERATURE_COLS);
> + if (ret < 0) {
> + dev_err(di->dev, "no full charge capacity lut array found");
> + return ret;
> + }
> +
> + ret = bms_read_ocv(di, &di->ocv);
> + if (ret < 0) {
> + dev_err(di->dev, "failed to read initial open circuit voltage: %d",
> + ret);
> + return ret;
> + }
> +
> + mutex_init(&di->bms_output_lock);
> +
> + di->ocv_thr_irq = platform_get_irq_byname(pdev, "ocv_thr");
> +
> + ret = devm_request_threaded_irq(di->dev, di->ocv_thr_irq, NULL,
> + bms_ocv_thr_irq_handler,
> + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> + pdev->name, di);
> + if (ret < 0) {
> + dev_err(di->dev, "failed to request handler for open circuit voltage threshold IRQ");
> + return ret;
> + }
> +
> +

one empty line is enough.

> + di->bat_desc.name = "bms";
> + di->bat_desc.type = POWER_SUPPLY_TYPE_BATTERY;
> + di->bat_desc.properties = bms_props;
> + di->bat_desc.num_properties = ARRAY_SIZE(bms_props);
> + di->bat_desc.get_property = bms_get_property;
> +
> + psy_cfg.drv_data = di;

psy_cfg.of_node = pdev->dev.of_node;

> + bat = devm_power_supply_register(di->dev, &di->bat_desc, &psy_cfg);
> +
> + return PTR_ERR_OR_ZERO(bat);
> +}
> +
> +static const struct of_device_id bms_of_match[] = {
> + {.compatible = "qcom,pm8941-bms", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, bms_of_match);
> +
> +static struct platform_driver bms_driver = {
> + .probe = bms_probe,
> + .driver = {
> + .name = "qcom-bms",
> + .of_match_table = of_match_ptr(bms_of_match),
> + },
> +};
> +module_platform_driver(bms_driver);
> +
> +MODULE_AUTHOR("Craig Tatlor <[email protected]>");
> +MODULE_DESCRIPTION("Qualcomm BMS driver");
> +MODULE_LICENSE("GPL");

Apart from the things above I would like to see the information
about the battery cell itself being moved to 'struct
power_supply_battery_info', see also my comment in the DT bindings.

-- Sebastian

Attachments:

(No filename) (18.22 kB)
signature.asc (849.00 B)
Download all attachments

2018-09-16 20:06:01

[Dropped a couple of people from CC, added Baolin]

Hi Craig, Baolin and Rob,

On Thu, Sep 20, 2018 at 03:32:29PM +0100, Craig wrote:
> On 16 September 2018 13:10:45 BST, Sebastian Reichel <[email protected]> wrote:
> >Sorry for my long delay in reviewing this. I like the binding,
> >but the "qcom," specific properties should become common properties
> >in
> >
> >Documentation/devicetree/bindings/power/supply/battery.txt
> >and referenced via monitored-battery.

> Thanks for the review, what bindings for ocv would you prefer? The
> spreadtrum ones or mine?

Most importantly I want to see only one generic binding supporting
both use cases. As far as I can see there are two major differences:

1. Qcom uses legend properties and SC27XX embedds this into data
2. Qcom supports temperature based mapping

The second point is easy: Not having temperature information can
be a subset of the data with temperature info. The main thing to
discuss are the legend properties. I suppose we have these
proposals:

Proposal A (from Qcom BMS binding):

ocv-capacity-legend = /bits/ 8 <100 95 90 85 80 75 70 65 60 55 50 45 ...>;
ocv-temp-legend-celsius = /bits/ 8 <(-10) 0 25 50 65>;
ocv-lut-microvolt = <43050000 43050000 43030000 42990000

Proposal B (from SC27XX binding):

ocv-cap-table = <4185 100>, <4113 95>, <4066 90>, <4022 85> ...;

I prefer the second binding (with mV -> uV), but I think it becomes
messy when temperature is added. What do you think about the
following proposal (derived from pinctrl style):

Proposal C:

ocv-capacity-table-temperatures = <(-10) 0 10>;
ocv-capacity-table-0 = <4185000 100>, <4113000 95>, <4066000 90>, ...;
ocv-capacity-table-1 = <4200000 100>, <4185000 95>, <4113000 90>, ...;
ocv-capacity-table-2 = <4250000 100>, <4200000 95>, <4185000 90>, ...;

-- Sebastian

Attachments:

(No filename) (1.82 kB)
signature.asc (849.00 B)
Download all attachments

2018-09-20 19:14:31

by Craig Tatlor

[permalink] [raw]

Subject: Re: [PATCH v7 3/4] dt-bindings: power: supply: qcom_bms: Add bindings

On 20 September 2018 17:58:47 BST, Sebastian Reichel <[email protected]> wrote:
>[Dropped a couple of people from CC, added Baolin]
>
>Hi Craig, Baolin and Rob,
>
>On Thu, Sep 20, 2018 at 03:32:29PM +0100, Craig wrote:
>> On 16 September 2018 13:10:45 BST, Sebastian Reichel
><[email protected]> wrote:
>> >Sorry for my long delay in reviewing this. I like the binding,
>> >but the "qcom," specific properties should become common properties
>> >in
>> >
>> >Documentation/devicetree/bindings/power/supply/battery.txt
>> >and referenced via monitored-battery.
>
>> Thanks for the review, what bindings for ocv would you prefer? The
>> spreadtrum ones or mine?
>
>Most importantly I want to see only one generic binding supporting
>both use cases. As far as I can see there are two major differences:
>
>1. Qcom uses legend properties and SC27XX embedds this into data
>2. Qcom supports temperature based mapping
>
>The second point is easy: Not having temperature information can
>be a subset of the data with temperature info. The main thing to
>discuss are the legend properties. I suppose we have these
>proposals:
>
>Proposal A (from Qcom BMS binding):
>
>ocv-capacity-legend = /bits/ 8 <100 95 90 85 80 75 70 65 60 55 50 45
>...>;
>ocv-temp-legend-celsius = /bits/ 8 <(-10) 0 25 50 65>;
>ocv-lut-microvolt = <43050000 43050000 43030000 42990000
>
>Proposal B (from SC27XX binding):
>
>ocv-cap-table = <4185 100>, <4113 95>, <4066 90>, <4022 85> ...;
>
>I prefer the second binding (with mV -> uV), but I think it becomes
>messy when temperature is added. What do you think about the
>following proposal (derived from pinctrl style):
>
>Proposal C:
>
>ocv-capacity-table-temperatures = <(-10) 0 10>;
>ocv-capacity-table-0 = <4185000 100>, <4113000 95>, <4066000 90>, ...;
>ocv-capacity-table-1 = <4200000 100>, <4185000 95>, <4113000 90>, ...;
>ocv-capacity-table-2 = <4250000 100>, <4200000 95>, <4185000 90>, ...;
>
>-- Sebastian

C looks good to me however I do kinda think it should be millivolts as I don't think any hardware reads in microvolts and the zeroes make it look quite ugly

2018-09-20 20:09:15

by Baolin Wang

[permalink] [raw]

Subject: Re: [PATCH v7 3/4] dt-bindings: power: supply: qcom_bms: Add bindings

Hi Sebastian,

On 21 September 2018 at 00:58, Sebastian Reichel
<[email protected]> wrote:
> [Dropped a couple of people from CC, added Baolin]
>
> Hi Craig, Baolin and Rob,
>
> On Thu, Sep 20, 2018 at 03:32:29PM +0100, Craig wrote:
>> On 16 September 2018 13:10:45 BST, Sebastian Reichel <[email protected]> wrote:
>> >Sorry for my long delay in reviewing this. I like the binding,
>> >but the "qcom," specific properties should become common properties
>> >in
>> >
>> >Documentation/devicetree/bindings/power/supply/battery.txt
>> >and referenced via monitored-battery.
>
>> Thanks for the review, what bindings for ocv would you prefer? The
>> spreadtrum ones or mine?
>
> Most importantly I want to see only one generic binding supporting
> both use cases. As far as I can see there are two major differences:
>
> 1. Qcom uses legend properties and SC27XX embedds this into data
> 2. Qcom supports temperature based mapping
>
> The second point is easy: Not having temperature information can
> be a subset of the data with temperature info. The main thing to
> discuss are the legend properties. I suppose we have these
> proposals:
>
> Proposal A (from Qcom BMS binding):
>
> ocv-capacity-legend = /bits/ 8 <100 95 90 85 80 75 70 65 60 55 50 45 ...>;
> ocv-temp-legend-celsius = /bits/ 8 <(-10) 0 25 50 65>;
> ocv-lut-microvolt = <43050000 43050000 43030000 42990000
>
> Proposal B (from SC27XX binding):
>
> ocv-cap-table = <4185 100>, <4113 95>, <4066 90>, <4022 85> ...;
>
> I prefer the second binding (with mV -> uV), but I think it becomes
> messy when temperature is added. What do you think about the
> following proposal (derived from pinctrl style):
>
> Proposal C:
>
> ocv-capacity-table-temperatures = <(-10) 0 10>;
> ocv-capacity-table-0 = <4185000 100>, <4113000 95>, <4066000 90>, ...;
> ocv-capacity-table-1 = <4200000 100>, <4185000 95>, <4113000 90>, ...;
> ocv-capacity-table-2 = <4250000 100>, <4200000 95>, <4185000 90>, ...;

For SC27XX, we have no temperatures consideration, but I think
Proposal C can be compatible with our case.

--
Baolin Wang
Best Regards

2018-09-21 01:41:35

by Sebastian Reichel

[permalink] [raw]

Subject: Re: [PATCH v7 3/4] dt-bindings: power: supply: qcom_bms: Add bindings

Hi,

On Fri, Sep 21, 2018 at 04:08:28AM +0800, Baolin Wang wrote:
> Hi Sebastian,
>
> On 21 September 2018 at 00:58, Sebastian Reichel
> <[email protected]> wrote:
> > [Dropped a couple of people from CC, added Baolin]
> >
> > Hi Craig, Baolin and Rob,
> >
> > On Thu, Sep 20, 2018 at 03:32:29PM +0100, Craig wrote:
> >> On 16 September 2018 13:10:45 BST, Sebastian Reichel <[email protected]> wrote:
> >> >Sorry for my long delay in reviewing this. I like the binding,
> >> >but the "qcom," specific properties should become common properties
> >> >in
> >> >
> >> >Documentation/devicetree/bindings/power/supply/battery.txt
> >> >and referenced via monitored-battery.
> >
> >> Thanks for the review, what bindings for ocv would you prefer? The
> >> spreadtrum ones or mine?
> >
> > Most importantly I want to see only one generic binding supporting
> > both use cases. As far as I can see there are two major differences:
> >
> > 1. Qcom uses legend properties and SC27XX embedds this into data
> > 2. Qcom supports temperature based mapping
> >
> > The second point is easy: Not having temperature information can
> > be a subset of the data with temperature info. The main thing to
> > discuss are the legend properties. I suppose we have these
> > proposals:
> >
> > Proposal A (from Qcom BMS binding):
> >
> > ocv-capacity-legend = /bits/ 8 <100 95 90 85 80 75 70 65 60 55 50 45 ...>;
> > ocv-temp-legend-celsius = /bits/ 8 <(-10) 0 25 50 65>;
> > ocv-lut-microvolt = <43050000 43050000 43030000 42990000
> >
> > Proposal B (from SC27XX binding):
> >
> > ocv-cap-table = <4185 100>, <4113 95>, <4066 90>, <4022 85> ...;
> >
> > I prefer the second binding (with mV -> uV), but I think it becomes
> > messy when temperature is added. What do you think about the
> > following proposal (derived from pinctrl style):
> >
> > Proposal C:
> >
> > ocv-capacity-table-temperatures = <(-10) 0 10>;
> > ocv-capacity-table-0 = <4185000 100>, <4113000 95>, <4066000 90>, ...;
> > ocv-capacity-table-1 = <4200000 100>, <4185000 95>, <4113000 90>, ...;
> > ocv-capacity-table-2 = <4250000 100>, <4200000 95>, <4185000 90>, ...;
>
> For SC27XX, we have no temperatures consideration, but I think
> Proposal C can be compatible with our case.

Yes. I think for SC27XX proposal C could be used like this:

ocv-capacity-table-temperatures = <20>; /* room temperature */
ocv-capacity-table-0 = <4185000 100>, <4113000 95>, <4066000 90>, ...;

With only one curve defined it would be used for all temperatures.

-- Sebastian

Attachments:

(No filename) (2.55 kB)
signature.asc (849.00 B)
Download all attachments

2018-09-21 01:43:00

Quoting Craig Tatlor (2018-09-25 10:35:58)
> From: Taniya Das <[email protected]>
>
> Add support for the global clock controller found on SDM660
> based devices. This should allow most non-multimedia device
> drivers to probe and control their clocks.
> Based on CAF implementation.
>
> Signed-off-by: Taniya Das <[email protected]>
> [craig: rename parents to fit upstream, and other cleanups]
> Signed-off-by: Craig Tatlor <[email protected]>
> ---

Applied to clk-next

But there was some noise:

drivers/clk/qcom/gcc-sdm660.c:2304:10: warning: Initializer entry defined twice
drivers/clk/qcom/gcc-sdm660.c:2306:10: also defined here

and I didn't like seeing gcc_660, so I applied this fixup on top:

diff --git a/drivers/clk/qcom/gcc-sdm660.c b/drivers/clk/qcom/gcc-sdm660.c
index 1722a9b181e2..9efb55092ebf 100644
--- a/drivers/clk/qcom/gcc-sdm660.c
+++ b/drivers/clk/qcom/gcc-sdm660.c
@@ -2253,12 +2253,12 @@ static struct gdsc pcie_0_gdsc = {
};

static struct clk_hw *gcc_sdm660_hws[] = {
- [GCC_XO] = &xo.hw,
- [GCC_GPLL0_EARLY_DIV] = &gpll0_early_div.hw,
- [GCC_GPLL1_EARLY_DIV] = &gpll1_early_div.hw,
+ &xo.hw,
+ &gpll0_early_div.hw,
+ &gpll1_early_div.hw,
};

-static struct clk_regmap *gcc_660_clocks[] = {
+static struct clk_regmap *gcc_sdm660_clocks[] = {
[BLSP1_QUP1_I2C_APPS_CLK_SRC] = &blsp1_qup1_i2c_apps_clk_src.clkr,
[BLSP1_QUP1_SPI_APPS_CLK_SRC] = &blsp1_qup1_spi_apps_clk_src.clkr,
[BLSP1_QUP2_I2C_APPS_CLK_SRC] = &blsp1_qup2_i2c_apps_clk_src.clkr,
@@ -2365,9 +2365,8 @@ static struct clk_regmap *gcc_660_clocks[] = {
[GPLL0_EARLY] = &gpll0_early.clkr,
[GPLL1] = &gpll1.clkr,
[GPLL1_EARLY] = &gpll1_early.clkr,
- [GPLL1] = &gpll1.clkr,
- [GPLL4_EARLY] = &gpll4_early.clkr,
[GPLL4] = &gpll4.clkr,
+ [GPLL4_EARLY] = &gpll4_early.clkr,
[HMSS_GPLL0_CLK_SRC] = &hmss_gpll0_clk_src.clkr,
[HMSS_GPLL4_CLK_SRC] = &hmss_gpll4_clk_src.clkr,
[HMSS_RBCPR_CLK_SRC] = &hmss_rbcpr_clk_src.clkr,
@@ -2387,13 +2386,13 @@ static struct clk_regmap *gcc_660_clocks[] = {
[USB3_PHY_AUX_CLK_SRC] = &usb3_phy_aux_clk_src.clkr,
};

-static struct gdsc *gcc_660_gdscs[] = {
+static struct gdsc *gcc_sdm660_gdscs[] = {
[UFS_GDSC] = &ufs_gdsc,
[USB_30_GDSC] = &usb_30_gdsc,
[PCIE_0_GDSC] = &pcie_0_gdsc,
};

-static const struct qcom_reset_map gcc_660_resets[] = {
+static const struct qcom_reset_map gcc_sdm660_resets[] = {
[GCC_QUSB2PHY_PRIM_BCR] = { 0x12000 },
[GCC_QUSB2PHY_SEC_BCR] = { 0x12004 },
[GCC_UFS_BCR] = { 0x75000 },
@@ -2405,7 +2404,7 @@ static const struct qcom_reset_map gcc_660_resets[] = {
[GCC_USB_PHY_CFG_AHB2PHY_BCR] = { 0x6a000 },
};

-static const struct regmap_config gcc_660_regmap_config = {
+static const struct regmap_config gcc_sdm660_regmap_config = {
.reg_bits = 32,
.reg_stride = 4,
.val_bits = 32,
@@ -2413,29 +2412,29 @@ static const struct regmap_config gcc_660_regmap_config = {
.fast_io = true,
};

-static const struct qcom_cc_desc gcc_660_desc = {
- .config = &gcc_660_regmap_config,
- .clks = gcc_660_clocks,
- .num_clks = ARRAY_SIZE(gcc_660_clocks),
- .resets = gcc_660_resets,
- .num_resets = ARRAY_SIZE(gcc_660_resets),
- .gdscs = gcc_660_gdscs,
- .num_gdscs = ARRAY_SIZE(gcc_660_gdscs),
+static const struct qcom_cc_desc gcc_sdm660_desc = {
+ .config = &gcc_sdm660_regmap_config,
+ .clks = gcc_sdm660_clocks,
+ .num_clks = ARRAY_SIZE(gcc_sdm660_clocks),
+ .resets = gcc_sdm660_resets,
+ .num_resets = ARRAY_SIZE(gcc_sdm660_resets),
+ .gdscs = gcc_sdm660_gdscs,
+ .num_gdscs = ARRAY_SIZE(gcc_sdm660_gdscs),
};

-static const struct of_device_id gcc_660_match_table[] = {
+static const struct of_device_id gcc_sdm660_match_table[] = {
{ .compatible = "qcom,gcc-sdm630" },
{ .compatible = "qcom,gcc-sdm660" },
{ }
};
-MODULE_DEVICE_TABLE(of, gcc_660_match_table);
+MODULE_DEVICE_TABLE(of, gcc_sdm660_match_table);

-static int gcc_660_probe(struct platform_device *pdev)
+static int gcc_sdm660_probe(struct platform_device *pdev)
{
int i, ret = 0;
struct regmap *regmap;

- regmap = qcom_cc_map(pdev, &gcc_660_desc);
+ regmap = qcom_cc_map(pdev, &gcc_sdm660_desc);
if (IS_ERR(regmap))
return PTR_ERR(regmap);

@@ -2454,27 +2453,27 @@ static int gcc_660_probe(struct platform_device *pdev)
return ret;
}

- return qcom_cc_really_probe(pdev, &gcc_660_desc, regmap);
+ return qcom_cc_really_probe(pdev, &gcc_sdm660_desc, regmap);
}

-static struct platform_driver gcc_660_driver = {
- .probe = gcc_660_probe,
+static struct platform_driver gcc_sdm660_driver = {
+ .probe = gcc_sdm660_probe,
.driver = {
.name = "gcc-sdm660",
- .of_match_table = gcc_660_match_table,
+ .of_match_table = gcc_sdm660_match_table,
},
};

-static int __init gcc_660_init(void)
+static int __init gcc_sdm660_init(void)
{
- return platform_driver_register(&gcc_660_driver);
+ return platform_driver_register(&gcc_sdm660_driver);
}
-core_initcall_sync(gcc_660_init);
+core_initcall_sync(gcc_sdm660_init);

-static void __exit gcc_660_exit(void)
+static void __exit gcc_sdm660_exit(void)
{
- platform_driver_unregister(&gcc_660_driver);
+ platform_driver_unregister(&gcc_sdm660_driver);
}
-module_exit(gcc_660_exit);
+module_exit(gcc_sdm660_exit);

MODULE_DESCRIPTION("QCOM GCC sdm660 Driver");
diff --git a/include/dt-bindings/clock/qcom,gcc-sdm660.h b/include/dt-bindings/clock/qcom,gcc-sdm660.h
index 76aabcc5e7e3..468302282913 100644
--- a/include/dt-bindings/clock/qcom,gcc-sdm660.h
+++ b/include/dt-bindings/clock/qcom,gcc-sdm660.h
@@ -7,140 +7,137 @@
#ifndef _DT_BINDINGS_CLK_MSM_GCC_660_H
#define _DT_BINDINGS_CLK_MSM_GCC_660_H

-#define GCC_XO 0
-#define GCC_GPLL0_EARLY_DIV 1
-#define GCC_GPLL1_EARLY_DIV 2
-#define BLSP1_QUP1_I2C_APPS_CLK_SRC 3
-#define BLSP1_QUP1_SPI_APPS_CLK_SRC 4
-#define BLSP1_QUP2_I2C_APPS_CLK_SRC 5
-#define BLSP1_QUP2_SPI_APPS_CLK_SRC 6
-#define BLSP1_QUP3_I2C_APPS_CLK_SRC 7
-#define BLSP1_QUP3_SPI_APPS_CLK_SRC 8
-#define BLSP1_QUP4_I2C_APPS_CLK_SRC 9
-#define BLSP1_QUP4_SPI_APPS_CLK_SRC 10
-#define BLSP1_UART1_APPS_CLK_SRC 11
-#define BLSP1_UART2_APPS_CLK_SRC 12
-#define BLSP2_QUP1_I2C_APPS_CLK_SRC 13
-#define BLSP2_QUP1_SPI_APPS_CLK_SRC 14
-#define BLSP2_QUP2_I2C_APPS_CLK_SRC 15
-#define BLSP2_QUP2_SPI_APPS_CLK_SRC 16
-#define BLSP2_QUP3_I2C_APPS_CLK_SRC 17
-#define BLSP2_QUP3_SPI_APPS_CLK_SRC 18
-#define BLSP2_QUP4_I2C_APPS_CLK_SRC 19
-#define BLSP2_QUP4_SPI_APPS_CLK_SRC 20
-#define BLSP2_UART1_APPS_CLK_SRC 21
-#define BLSP2_UART2_APPS_CLK_SRC 22
-#define GCC_AGGRE2_UFS_AXI_CLK 23
-#define GCC_AGGRE2_USB3_AXI_CLK 24
-#define GCC_BIMC_GFX_CLK 25
-#define GCC_BIMC_HMSS_AXI_CLK 26
-#define GCC_BIMC_MSS_Q6_AXI_CLK 27
-#define GCC_BLSP1_AHB_CLK 28
-#define GCC_BLSP1_QUP1_I2C_APPS_CLK 29
-#define GCC_BLSP1_QUP1_SPI_APPS_CLK 30
-#define GCC_BLSP1_QUP2_I2C_APPS_CLK 31
-#define GCC_BLSP1_QUP2_SPI_APPS_CLK 32
-#define GCC_BLSP1_QUP3_I2C_APPS_CLK 33
-#define GCC_BLSP1_QUP3_SPI_APPS_CLK 34
-#define GCC_BLSP1_QUP4_I2C_APPS_CLK 35
-#define GCC_BLSP1_QUP4_SPI_APPS_CLK 36
-#define GCC_BLSP1_UART1_APPS_CLK 37
-#define GCC_BLSP1_UART2_APPS_CLK 38
-#define GCC_BLSP2_AHB_CLK 39
-#define GCC_BLSP2_QUP1_I2C_APPS_CLK 40
-#define GCC_BLSP2_QUP1_SPI_APPS_CLK 41
-#define GCC_BLSP2_QUP2_I2C_APPS_CLK 42
-#define GCC_BLSP2_QUP2_SPI_APPS_CLK 43
-#define GCC_BLSP2_QUP3_I2C_APPS_CLK 44
-#define GCC_BLSP2_QUP3_SPI_APPS_CLK 46
-#define GCC_BLSP2_QUP4_I2C_APPS_CLK 45
-#define GCC_BLSP2_QUP4_SPI_APPS_CLK 46
-#define GCC_BLSP2_UART1_APPS_CLK 47
-#define GCC_BLSP2_UART2_APPS_CLK 48
-#define GCC_BOOT_ROM_AHB_CLK 49
-#define GCC_CFG_NOC_USB2_AXI_CLK 50
-#define GCC_CFG_NOC_USB3_AXI_CLK 51
-#define GCC_DCC_AHB_CLK 52
-#define GCC_GP1_CLK 53
-#define GCC_GP2_CLK 54
-#define GCC_GP3_CLK 55
-#define GCC_GPU_BIMC_GFX_CLK 56
-#define GCC_GPU_CFG_AHB_CLK 57
-#define GCC_GPU_GPLL0_CLK 58
-#define GCC_GPU_GPLL0_DIV_CLK 59
-#define GCC_HMSS_DVM_BUS_CLK 60
-#define GCC_HMSS_RBCPR_CLK 61
-#define GCC_MMSS_GPLL0_CLK 62
-#define GCC_MMSS_GPLL0_DIV_CLK 63
-#define GCC_MMSS_NOC_CFG_AHB_CLK 64
-#define GCC_MMSS_SYS_NOC_AXI_CLK 65
-#define GCC_MSS_CFG_AHB_CLK 66
-#define GCC_MSS_GPLL0_DIV_CLK 67
-#define GCC_MSS_MNOC_BIMC_AXI_CLK 68
-#define GCC_MSS_Q6_BIMC_AXI_CLK 69
-#define GCC_MSS_SNOC_AXI_CLK 70
-#define GCC_PDM2_CLK 71
-#define GCC_PDM_AHB_CLK 72
-#define GCC_PRNG_AHB_CLK 73
-#define GCC_QSPI_AHB_CLK 74
-#define GCC_QSPI_SER_CLK 75
-#define GCC_SDCC1_AHB_CLK 76
-#define GCC_SDCC1_APPS_CLK 77
-#define GCC_SDCC1_ICE_CORE_CLK 78
-#define GCC_SDCC2_AHB_CLK 79
-#define GCC_SDCC2_APPS_CLK 80
-#define GCC_UFS_AHB_CLK 81
-#define GCC_UFS_AXI_CLK 82
-#define GCC_UFS_CLKREF_CLK 83
-#define GCC_UFS_ICE_CORE_CLK 84
-#define GCC_UFS_PHY_AUX_CLK 85
-#define GCC_UFS_RX_SYMBOL_0_CLK 86
-#define GCC_UFS_RX_SYMBOL_1_CLK 87
-#define GCC_UFS_TX_SYMBOL_0_CLK 88
-#define GCC_UFS_UNIPRO_CORE_CLK 89
-#define GCC_USB20_MASTER_CLK 90
-#define GCC_USB20_MOCK_UTMI_CLK 91
-#define GCC_USB20_SLEEP_CLK 92
-#define GCC_USB30_MASTER_CLK 93
-#define GCC_USB30_MOCK_UTMI_CLK 94
-#define GCC_USB30_SLEEP_CLK 95
-#define GCC_USB3_CLKREF_CLK 96
-#define GCC_USB3_PHY_AUX_CLK 97
-#define GCC_USB3_PHY_PIPE_CLK 98
-#define GCC_USB_PHY_CFG_AHB2PHY_CLK 99
-#define GP1_CLK_SRC 100
-#define GP2_CLK_SRC 101
-#define GP3_CLK_SRC 102
-#define GPLL0 103
-#define GPLL0_EARLY 104
-#define GPLL1 105
-#define GPLL1_EARLY 106
-#define GPLL4 107
-#define GPLL4_EARLY 108
-#define HMSS_GPLL0_CLK_SRC 109
-#define HMSS_GPLL4_CLK_SRC 110
-#define HMSS_RBCPR_CLK_SRC 111
-#define PDM2_CLK_SRC 112
-#define QSPI_SER_CLK_SRC 113
-#define SDCC1_APPS_CLK_SRC 114
-#define SDCC1_ICE_CORE_CLK_SRC 115
-#define SDCC2_APPS_CLK_SRC 116
-#define UFS_AXI_CLK_SRC 117
-#define UFS_ICE_CORE_CLK_SRC 118
-#define UFS_PHY_AUX_CLK_SRC 119
-#define UFS_UNIPRO_CORE_CLK_SRC 120
-#define USB20_MASTER_CLK_SRC 121
-#define USB20_MOCK_UTMI_CLK_SRC 122
-#define USB30_MASTER_CLK_SRC 123
-#define USB30_MOCK_UTMI_CLK_SRC 124
-#define USB3_PHY_AUX_CLK_SRC 125
-#define GPLL0_OUT_MSSCC 126
-#define GCC_UFS_AXI_HW_CTL_CLK 127
-#define GCC_UFS_ICE_CORE_HW_CTL_CLK 128
-#define GCC_UFS_PHY_AUX_HW_CTL_CLK 129
-#define GCC_UFS_UNIPRO_CORE_HW_CTL_CLK 130
-#define GCC_RX0_USB2_CLKREF_CLK 131
-#define GCC_RX1_USB2_CLKREF_CLK 132
+#define BLSP1_QUP1_I2C_APPS_CLK_SRC 0
+#define BLSP1_QUP1_SPI_APPS_CLK_SRC 1
+#define BLSP1_QUP2_I2C_APPS_CLK_SRC 2
+#define BLSP1_QUP2_SPI_APPS_CLK_SRC 3
+#define BLSP1_QUP3_I2C_APPS_CLK_SRC 4
+#define BLSP1_QUP3_SPI_APPS_CLK_SRC 5
+#define BLSP1_QUP4_I2C_APPS_CLK_SRC 6
+#define BLSP1_QUP4_SPI_APPS_CLK_SRC 7
+#define BLSP1_UART1_APPS_CLK_SRC 8
+#define BLSP1_UART2_APPS_CLK_SRC 9
+#define BLSP2_QUP1_I2C_APPS_CLK_SRC 10
+#define BLSP2_QUP1_SPI_APPS_CLK_SRC 11
+#define BLSP2_QUP2_I2C_APPS_CLK_SRC 12
+#define BLSP2_QUP2_SPI_APPS_CLK_SRC 13
+#define BLSP2_QUP3_I2C_APPS_CLK_SRC 14
+#define BLSP2_QUP3_SPI_APPS_CLK_SRC 15
+#define BLSP2_QUP4_I2C_APPS_CLK_SRC 16
+#define BLSP2_QUP4_SPI_APPS_CLK_SRC 17
+#define BLSP2_UART1_APPS_CLK_SRC 18
+#define BLSP2_UART2_APPS_CLK_SRC 19
+#define GCC_AGGRE2_UFS_AXI_CLK 20
+#define GCC_AGGRE2_USB3_AXI_CLK 21
+#define GCC_BIMC_GFX_CLK 22
+#define GCC_BIMC_HMSS_AXI_CLK 23
+#define GCC_BIMC_MSS_Q6_AXI_CLK 24
+#define GCC_BLSP1_AHB_CLK 25
+#define GCC_BLSP1_QUP1_I2C_APPS_CLK 26
+#define GCC_BLSP1_QUP1_SPI_APPS_CLK 27
+#define GCC_BLSP1_QUP2_I2C_APPS_CLK 28
+#define GCC_BLSP1_QUP2_SPI_APPS_CLK 29
+#define GCC_BLSP1_QUP3_I2C_APPS_CLK 30
+#define GCC_BLSP1_QUP3_SPI_APPS_CLK 31
+#define GCC_BLSP1_QUP4_I2C_APPS_CLK 32
+#define GCC_BLSP1_QUP4_SPI_APPS_CLK 33
+#define GCC_BLSP1_UART1_APPS_CLK 34
+#define GCC_BLSP1_UART2_APPS_CLK 35
+#define GCC_BLSP2_AHB_CLK 36
+#define GCC_BLSP2_QUP1_I2C_APPS_CLK 37
+#define GCC_BLSP2_QUP1_SPI_APPS_CLK 38
+#define GCC_BLSP2_QUP2_I2C_APPS_CLK 39
+#define GCC_BLSP2_QUP2_SPI_APPS_CLK 40
+#define GCC_BLSP2_QUP3_I2C_APPS_CLK 41
+#define GCC_BLSP2_QUP3_SPI_APPS_CLK 42
+#define GCC_BLSP2_QUP4_I2C_APPS_CLK 43
+#define GCC_BLSP2_QUP4_SPI_APPS_CLK 44
+#define GCC_BLSP2_UART1_APPS_CLK 45
+#define GCC_BLSP2_UART2_APPS_CLK 46
+#define GCC_BOOT_ROM_AHB_CLK 47
+#define GCC_CFG_NOC_USB2_AXI_CLK 48
+#define GCC_CFG_NOC_USB3_AXI_CLK 49
+#define GCC_DCC_AHB_CLK 50
+#define GCC_GP1_CLK 51
+#define GCC_GP2_CLK 52
+#define GCC_GP3_CLK 53
+#define GCC_GPU_BIMC_GFX_CLK 54
+#define GCC_GPU_CFG_AHB_CLK 55
+#define GCC_GPU_GPLL0_CLK 56
+#define GCC_GPU_GPLL0_DIV_CLK 57
+#define GCC_HMSS_DVM_BUS_CLK 58
+#define GCC_HMSS_RBCPR_CLK 59
+#define GCC_MMSS_GPLL0_CLK 60
+#define GCC_MMSS_GPLL0_DIV_CLK 61
+#define GCC_MMSS_NOC_CFG_AHB_CLK 62
+#define GCC_MMSS_SYS_NOC_AXI_CLK 63
+#define GCC_MSS_CFG_AHB_CLK 64
+#define GCC_MSS_GPLL0_DIV_CLK 65
+#define GCC_MSS_MNOC_BIMC_AXI_CLK 66
+#define GCC_MSS_Q6_BIMC_AXI_CLK 67
+#define GCC_MSS_SNOC_AXI_CLK 68
+#define GCC_PDM2_CLK 69
+#define GCC_PDM_AHB_CLK 70
+#define GCC_PRNG_AHB_CLK 71
+#define GCC_QSPI_AHB_CLK 72
+#define GCC_QSPI_SER_CLK 73
+#define GCC_SDCC1_AHB_CLK 74
+#define GCC_SDCC1_APPS_CLK 75
+#define GCC_SDCC1_ICE_CORE_CLK 76
+#define GCC_SDCC2_AHB_CLK 77
+#define GCC_SDCC2_APPS_CLK 78
+#define GCC_UFS_AHB_CLK 79
+#define GCC_UFS_AXI_CLK 80
+#define GCC_UFS_CLKREF_CLK 81
+#define GCC_UFS_ICE_CORE_CLK 82
+#define GCC_UFS_PHY_AUX_CLK 83
+#define GCC_UFS_RX_SYMBOL_0_CLK 84
+#define GCC_UFS_RX_SYMBOL_1_CLK 85
+#define GCC_UFS_TX_SYMBOL_0_CLK 86
+#define GCC_UFS_UNIPRO_CORE_CLK 87
+#define GCC_USB20_MASTER_CLK 88
+#define GCC_USB20_MOCK_UTMI_CLK 89
+#define GCC_USB20_SLEEP_CLK 90
+#define GCC_USB30_MASTER_CLK 91
+#define GCC_USB30_MOCK_UTMI_CLK 92
+#define GCC_USB30_SLEEP_CLK 93
+#define GCC_USB3_CLKREF_CLK 94
+#define GCC_USB3_PHY_AUX_CLK 95
+#define GCC_USB3_PHY_PIPE_CLK 96
+#define GCC_USB_PHY_CFG_AHB2PHY_CLK 97
+#define GP1_CLK_SRC 98
+#define GP2_CLK_SRC 99
+#define GP3_CLK_SRC 100
+#define GPLL0 101
+#define GPLL0_EARLY 102
+#define GPLL1 103
+#define GPLL1_EARLY 104
+#define GPLL4 105
+#define GPLL4_EARLY 106
+#define HMSS_GPLL0_CLK_SRC 107
+#define HMSS_GPLL4_CLK_SRC 108
+#define HMSS_RBCPR_CLK_SRC 109
+#define PDM2_CLK_SRC 110
+#define QSPI_SER_CLK_SRC 111
+#define SDCC1_APPS_CLK_SRC 112
+#define SDCC1_ICE_CORE_CLK_SRC 113
+#define SDCC2_APPS_CLK_SRC 114
+#define UFS_AXI_CLK_SRC 115
+#define UFS_ICE_CORE_CLK_SRC 116
+#define UFS_PHY_AUX_CLK_SRC 117
+#define UFS_UNIPRO_CORE_CLK_SRC 118
+#define USB20_MASTER_CLK_SRC 119
+#define USB20_MOCK_UTMI_CLK_SRC 120
+#define USB30_MASTER_CLK_SRC 121
+#define USB30_MOCK_UTMI_CLK_SRC 122
+#define USB3_PHY_AUX_CLK_SRC 123
+#define GPLL0_OUT_MSSCC 124
+#define GCC_UFS_AXI_HW_CTL_CLK 125
+#define GCC_UFS_ICE_CORE_HW_CTL_CLK 126
+#define GCC_UFS_PHY_AUX_HW_CTL_CLK 127
+#define GCC_UFS_UNIPRO_CORE_HW_CTL_CLK 128
+#define GCC_RX0_USB2_CLKREF_CLK 129
+#define GCC_RX1_USB2_CLKREF_CLK 130

#define PCIE_0_GDSC 0
#define UFS_GDSC 1