From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C27F1C433FE for ; Fri, 20 May 2022 07:00:40 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id D31BD83BBF; Fri, 20 May 2022 09:00:37 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=denx.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=denx.de; s=phobos-20191101; t=1653030038; bh=b77ncuRp/bOo9TOwwfqs3ojuiUM424cKQ7TdyNXpNhE=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=o49Tj4E+dQi0VIYJQspvfx2shHHQRLIYimF2bDIHiyQ1l9ekP8A5pySNy18yl2GmF 7Si+xXh8S79W3EswTtX7AeVGxdDQ9a4+L5eaPzbvINxTxFe2YFMtPgq0pKlE81py9Q qDYXRYVDA7DRckn6aFNhXXARxnXlPz0o4JPyS+fppxy6pbzBxpwMk4hu+q5OVIRHAz MqQ/UW/Jjwm38M7wymFhIpA1F5UmnXKRXMGRKHcq9kG5NHIreobS9eU0OU59H1RexS wokAQIto/VdN0Fu5RRE+QcW5emQL/7bHgB+tfbXoJvBISPdDkxuOwwxgl0Oks4LVAL xdaKT+93jbWcw== Received: from [IPV6:2001:a61:60db:7a01:2a3d:378a:daeb:e028] (unknown [IPv6:2001:a61:60db:7a01:2a3d:378a:daeb:e028]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: sbabic@denx.de) by phobos.denx.de (Postfix) with ESMTPSA id 58BAA81111; Fri, 20 May 2022 09:00:34 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=denx.de; s=phobos-20191101; t=1653030035; bh=b77ncuRp/bOo9TOwwfqs3ojuiUM424cKQ7TdyNXpNhE=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=oQe8te3rwXJVyReAw6bRytXdE51JQSAflT9C775Z37646aKWIr7SwsfP6ON5jj1rq wEU+8gdG7EVQXBsHNN/4zzlXT6xmjbW66xvtRAMaJdiQUu5aPms6mc+pPVfAUA9PbQ bN1WnDYbIUMUXWYBPO0AnL5WzyI+Hsdq40Z2LKPnKWHgb1JUup05kkFY62ON7QgctR 299zphL3st8wIglWxjfCt7LHF0OK6TVqEsAz+gwxVdw3sv3A/T6FYWs7evl9oWZncr skvlxjZu9ylr7GxWoMALgpgLNDuhOtxDEPzmUp0D1WF3hv0EkYZw8iiXDpbYD2hzID 9ZwBK37qzBG6A== Message-ID: <054f7a33-67dd-f366-5fa1-8d415afc9345@denx.de> Date: Fri, 20 May 2022 09:00:39 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1 Subject: Re: [PATCH v2 2/2] pmic: pca9450: Add regulator driver Content-Language: en-US To: Tim Harvey , Marek Vasut Cc: u-boot , Fabio Estevam , Peng Fan , Stefano Babic References: <20220424214047.51975-1-marex@denx.de> <20220424214047.51975-2-marex@denx.de> From: Stefano Babic In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean Hi Tim, Marek, On 20.05.22 00:24, Tim Harvey wrote: > On Sun, Apr 24, 2022 at 2:41 PM Marek Vasut wrote: >> >> Add PCA9450 regulator driver. This is complementary driver for the BUCKn >> and LDOn regulators provided by the PCA9450 PMIC driver. Currently the >> driver permits reading the settngs and configuring the BUCKn and LDOn >> regulators. >> >> Reviewed-by: Fabio Estevam >> Signed-off-by: Marek Vasut >> Cc: Fabio Estevam >> Cc: Peng Fan >> Cc: Stefano Babic >> --- >> V2: Add RB by Fabio >> --- >> drivers/power/pmic/pca9450.c | 6 +- >> drivers/power/regulator/Kconfig | 15 ++ >> drivers/power/regulator/Makefile | 1 + >> drivers/power/regulator/pca9450.c | 333 ++++++++++++++++++++++++++++++ >> include/power/pca9450.h | 11 + >> 5 files changed, 363 insertions(+), 3 deletions(-) >> create mode 100644 drivers/power/regulator/pca9450.c >> >> diff --git a/drivers/power/pmic/pca9450.c b/drivers/power/pmic/pca9450.c >> index 26c876c9c45..116ac49a8db 100644 >> --- a/drivers/power/pmic/pca9450.c >> +++ b/drivers/power/pmic/pca9450.c >> @@ -83,9 +83,9 @@ static struct dm_pmic_ops pca9450_ops = { >> }; >> >> static const struct udevice_id pca9450_ids[] = { >> - { .compatible = "nxp,pca9450a", .data = 0x25, }, >> - { .compatible = "nxp,pca9450b", .data = 0x25, }, >> - { .compatible = "nxp,pca9450c", .data = 0x25, }, >> + { .compatible = "nxp,pca9450a", .data = NXP_CHIP_TYPE_PCA9450A, }, >> + { .compatible = "nxp,pca9450b", .data = NXP_CHIP_TYPE_PCA9450BC, }, >> + { .compatible = "nxp,pca9450c", .data = NXP_CHIP_TYPE_PCA9450BC, }, >> { } >> }; >> >> diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig >> index cd253b95f2f..d486bad6bdc 100644 >> --- a/drivers/power/regulator/Kconfig >> +++ b/drivers/power/regulator/Kconfig >> @@ -60,6 +60,21 @@ config SPL_DM_REGULATOR_BD71837 >> This config enables implementation of driver-model regulator uclass >> features for regulators on ROHM BD71837 and BD71847 in SPL. >> >> +config DM_REGULATOR_PCA9450 >> + bool "Enable Driver Model for NXP PCA9450 regulators" >> + depends on DM_REGULATOR && DM_PMIC_PCA9450 >> + help >> + This config enables implementation of driver-model regulator uclass >> + features for regulators on NXP PCA9450 PMICs. PCA9450 contains 6 bucks >> + and 5 LDOS. The driver implements get/set api for value and enable. >> + >> +config SPL_DM_REGULATOR_PCA9450 >> + bool "Enable Driver Model for NXP PCA9450 regulators in SPL" >> + depends on DM_REGULATOR_PCA9450 >> + help >> + This config enables implementation of driver-model regulator uclass >> + features for regulators on ROHM PCA9450 in SPL. >> + >> config DM_REGULATOR_DA9063 >> bool "Enable Driver Model for REGULATOR DA9063" >> depends on DM_REGULATOR && DM_PMIC_DA9063 >> diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile >> index 4efb32a3228..d2d17f7aed0 100644 >> --- a/drivers/power/regulator/Makefile >> +++ b/drivers/power/regulator/Makefile >> @@ -11,6 +11,7 @@ obj-$(CONFIG_$(SPL_)DM_REGULATOR_DA9063) += da9063.o >> obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o >> obj-$(CONFIG_$(SPL_)DM_PMIC_PFUZE100) += pfuze100.o >> obj-$(CONFIG_$(SPL_)DM_REGULATOR_BD71837) += bd71837.o >> +obj-$(CONFIG_$(SPL_)DM_REGULATOR_PCA9450) += pca9450.o >> obj-$(CONFIG_$(SPL_)REGULATOR_PWM) += pwm_regulator.o >> obj-$(CONFIG_$(SPL_)DM_REGULATOR_FAN53555) += fan53555.o >> obj-$(CONFIG_$(SPL_)DM_REGULATOR_COMMON) += regulator_common.o >> diff --git a/drivers/power/regulator/pca9450.c b/drivers/power/regulator/pca9450.c >> new file mode 100644 >> index 00000000000..4847c9f90f0 >> --- /dev/null >> +++ b/drivers/power/regulator/pca9450.c >> @@ -0,0 +1,333 @@ >> +// SPDX-License-Identifier: GPL-2.0-or-later >> +/* >> + * NXP PCA9450 regulator driver >> + * Copyright (C) 2022 Marek Vasut >> + * >> + * Largely based on: >> + * ROHM BD71837 regulator driver >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#define HW_STATE_CONTROL 0 >> +#define DEBUG >> + >> +/** >> + * struct pca9450_vrange - describe linear range of voltages >> + * >> + * @min_volt: smallest voltage in range >> + * @step: how much voltage changes at each selector step >> + * @min_sel: smallest selector in the range >> + * @max_sel: maximum selector in the range >> + */ >> +struct pca9450_vrange { >> + unsigned int min_volt; >> + unsigned int step; >> + u8 min_sel; >> + u8 max_sel; >> +}; >> + >> +/** >> + * struct pca9450_plat - describe regulator control registers >> + * >> + * @name: name of the regulator. Used for matching the dt-entry >> + * @enable_reg: register address used to enable/disable regulator >> + * @enablemask: register mask used to enable/disable regulator >> + * @volt_reg: register address used to configure regulator voltage >> + * @volt_mask: register mask used to configure regulator voltage >> + * @ranges: pointer to ranges of regulator voltages and matching register >> + * values >> + * @numranges: number of voltage ranges pointed by ranges >> + * @dvs: whether the voltage can be changed when regulator is enabled >> + */ >> +struct pca9450_plat { >> + const char *name; >> + u8 enable_reg; >> + u8 enablemask; >> + u8 volt_reg; >> + u8 volt_mask; >> + struct pca9450_vrange *ranges; >> + unsigned int numranges; >> + bool dvs; >> +}; >> + >> +#define PCA_RANGE(_min, _vstep, _sel_low, _sel_hi) \ >> +{ \ >> + .min_volt = (_min), .step = (_vstep), \ >> + .min_sel = (_sel_low), .max_sel = (_sel_hi), \ >> +} >> + >> +#define PCA_DATA(_name, enreg, enmask, vreg, vmask, _range, _dvs) \ >> +{ \ >> + .name = (_name), .enable_reg = (enreg), .enablemask = (enmask), \ >> + .volt_reg = (vreg), .volt_mask = (vmask), .ranges = (_range), \ >> + .numranges = ARRAY_SIZE(_range), .dvs = (_dvs), \ >> +} >> + >> +static struct pca9450_vrange pca9450_buck123_vranges[] = { >> + PCA_RANGE(600000, 12500, 0, 0x7f), >> +}; >> + >> +static struct pca9450_vrange pca9450_buck456_vranges[] = { >> + PCA_RANGE(600000, 25000, 0, 0x70), >> + PCA_RANGE(3400000, 0, 0x71, 0x7f), >> +}; >> + >> +static struct pca9450_vrange pca9450_ldo1_vranges[] = { >> + PCA_RANGE(1600000, 100000, 0x0, 0x3), >> + PCA_RANGE(3000000, 100000, 0x4, 0x7), >> +}; >> + >> +static struct pca9450_vrange pca9450_ldo2_vranges[] = { >> + PCA_RANGE(800000, 50000, 0x0, 0x7), >> +}; >> + >> +static struct pca9450_vrange pca9450_ldo34_vranges[] = { >> + PCA_RANGE(800000, 100000, 0x0, 0x19), >> + PCA_RANGE(3300000, 0, 0x1a, 0x1f), >> +}; >> + >> +static struct pca9450_vrange pca9450_ldo5_vranges[] = { >> + PCA_RANGE(1800000, 100000, 0x0, 0xf), >> +}; >> + >> +/* >> + * We use enable mask 'HW_STATE_CONTROL' to indicate that this regulator >> + * must not be enabled or disabled by SW. The typical use-case for PCA9450 >> + * is powering NXP i.MX8. In this use-case we (for now) only allow control >> + * for BUCK4, BUCK5, BUCK6 which are not boot critical. >> + */ >> +static struct pca9450_plat pca9450_reg_data[] = { >> + /* Bucks 1-3 which support dynamic voltage scaling */ >> + PCA_DATA("BUCK1", PCA9450_BUCK1CTRL, HW_STATE_CONTROL, >> + PCA9450_BUCK1OUT_DVS0, DVS_BUCK_RUN_MASK, >> + pca9450_buck123_vranges, true), >> + PCA_DATA("BUCK2", PCA9450_BUCK2CTRL, HW_STATE_CONTROL, >> + PCA9450_BUCK2OUT_DVS0, DVS_BUCK_RUN_MASK, >> + pca9450_buck123_vranges, true), >> + PCA_DATA("BUCK3", PCA9450_BUCK3CTRL, HW_STATE_CONTROL, >> + PCA9450_BUCK3OUT_DVS0, DVS_BUCK_RUN_MASK, >> + pca9450_buck123_vranges, true), >> + /* Bucks 4-6 which do not support dynamic voltage scaling */ >> + PCA_DATA("BUCK4", PCA9450_BUCK4CTRL, HW_STATE_CONTROL, >> + PCA9450_BUCK4OUT, DVS_BUCK_RUN_MASK, >> + pca9450_buck456_vranges, false), >> + PCA_DATA("BUCK5", PCA9450_BUCK5CTRL, HW_STATE_CONTROL, >> + PCA9450_BUCK5OUT, DVS_BUCK_RUN_MASK, >> + pca9450_buck456_vranges, false), >> + PCA_DATA("BUCK6", PCA9450_BUCK6CTRL, HW_STATE_CONTROL, >> + PCA9450_BUCK6OUT, DVS_BUCK_RUN_MASK, >> + pca9450_buck456_vranges, false), >> + /* LDOs */ >> + PCA_DATA("LDO1", PCA9450_LDO1CTRL, HW_STATE_CONTROL, >> + PCA9450_LDO1CTRL, PCA9450_LDO12_MASK, >> + pca9450_ldo1_vranges, false), >> + PCA_DATA("LDO2", PCA9450_LDO2CTRL, HW_STATE_CONTROL, >> + PCA9450_LDO2CTRL, PCA9450_LDO12_MASK, >> + pca9450_ldo2_vranges, false), >> + PCA_DATA("LDO3", PCA9450_LDO3CTRL, HW_STATE_CONTROL, >> + PCA9450_LDO3CTRL, PCA9450_LDO34_MASK, >> + pca9450_ldo34_vranges, false), >> + PCA_DATA("LDO4", PCA9450_LDO4CTRL, HW_STATE_CONTROL, >> + PCA9450_LDO4CTRL, PCA9450_LDO34_MASK, >> + pca9450_ldo34_vranges, false), >> + PCA_DATA("LDO5", PCA9450_LDO5CTRL_H, HW_STATE_CONTROL, >> + PCA9450_LDO5CTRL_H, PCA9450_LDO5_MASK, >> + pca9450_ldo5_vranges, false), >> +}; >> + >> +static int vrange_find_value(struct pca9450_vrange *r, unsigned int sel, >> + unsigned int *val) >> +{ >> + if (!val || sel < r->min_sel || sel > r->max_sel) >> + return -EINVAL; >> + >> + *val = r->min_volt + r->step * (sel - r->min_sel); >> + return 0; >> +} >> + >> +static int vrange_find_selector(struct pca9450_vrange *r, int val, >> + unsigned int *sel) >> +{ >> + int ret = -EINVAL; >> + int num_vals = r->max_sel - r->min_sel + 1; >> + >> + if (val >= r->min_volt && >> + val <= r->min_volt + r->step * (num_vals - 1)) { >> + if (r->step) { >> + *sel = r->min_sel + ((val - r->min_volt) / r->step); >> + ret = 0; >> + } else { >> + *sel = r->min_sel; >> + ret = 0; >> + } >> + } >> + return ret; >> +} >> + >> +static int pca9450_get_enable(struct udevice *dev) >> +{ >> + struct pca9450_plat *plat = dev_get_plat(dev); >> + int val; >> + >> + /* >> + * boot critical regulators on pca9450 must not be controlled by sw >> + * due to the 'feature' which leaves power rails down if pca9450 is >> + * reseted to snvs state. hence we can't get the state here. >> + * >> + * if we are alive it means we probably are on run state and >> + * if the regulator can't be controlled we can assume it is >> + * enabled. >> + */ >> + if (plat->enablemask == HW_STATE_CONTROL) >> + return 1; >> + >> + val = pmic_reg_read(dev->parent, plat->enable_reg); >> + if (val < 0) >> + return val; >> + >> + return (val & plat->enablemask); >> +} >> + >> +static int pca9450_set_enable(struct udevice *dev, bool enable) >> +{ >> + int val = 0; >> + struct pca9450_plat *plat = dev_get_plat(dev); >> + >> + /* >> + * boot critical regulators on pca9450 must not be controlled by sw >> + * due to the 'feature' which leaves power rails down if pca9450 is >> + * reseted to snvs state. Hence we can't set the state here. >> + */ >> + if (plat->enablemask == HW_STATE_CONTROL) >> + return enable ? 0 : -EINVAL; >> + >> + if (enable) >> + val = plat->enablemask; >> + >> + return pmic_clrsetbits(dev->parent, plat->enable_reg, plat->enablemask, >> + val); >> +} >> + >> +static int pca9450_get_value(struct udevice *dev) >> +{ >> + struct pca9450_plat *plat = dev_get_plat(dev); >> + unsigned int reg, tmp; >> + int i, ret; >> + >> + ret = pmic_reg_read(dev->parent, plat->volt_reg); >> + if (ret < 0) >> + return ret; >> + >> + reg = ret; >> + reg &= plat->volt_mask; >> + >> + for (i = 0; i < plat->numranges; i++) { >> + struct pca9450_vrange *r = &plat->ranges[i]; >> + >> + if (!vrange_find_value(r, reg, &tmp)) >> + return tmp; >> + } >> + >> + pr_err("Unknown voltage value read from pmic\n"); >> + >> + return -EINVAL; >> +} >> + >> +static int pca9450_set_value(struct udevice *dev, int uvolt) >> +{ >> + struct pca9450_plat *plat = dev_get_plat(dev); >> + unsigned int sel; >> + int i, found = 0; >> + >> + /* >> + * An under/overshooting may occur if voltage is changed for other >> + * regulators but buck 1,2,3 or 4 when regulator is enabled. Prevent >> + * change to protect the HW >> + */ >> + if (!plat->dvs) >> + if (pca9450_get_enable(dev)) { >> + /* If the value is already set, skip the warning. */ >> + if (pca9450_get_value(dev) == uvolt) >> + return 0; >> + pr_err("Only DVS bucks can be changed when enabled\n"); >> + return -EINVAL; >> + } >> + >> + for (i = 0; i < plat->numranges; i++) { >> + struct pca9450_vrange *r = &plat->ranges[i]; >> + >> + found = !vrange_find_selector(r, uvolt, &sel); >> + if (found) { >> + unsigned int tmp; >> + >> + /* >> + * We require exactly the requested value to be >> + * supported - this can be changed later if needed >> + */ >> + found = !vrange_find_value(r, sel, &tmp); >> + if (found && tmp == uvolt) >> + break; >> + found = 0; >> + } >> + } >> + >> + if (!found) >> + return -EINVAL; >> + >> + return pmic_clrsetbits(dev->parent, plat->volt_reg, >> + plat->volt_mask, sel); >> +} >> + >> +static int pca9450_regulator_probe(struct udevice *dev) >> +{ >> + struct pca9450_plat *plat = dev_get_plat(dev); >> + int i, type; >> + >> + type = dev_get_driver_data(dev_get_parent(dev)); >> + >> + if (type != NXP_CHIP_TYPE_PCA9450A && type != NXP_CHIP_TYPE_PCA9450BC) { >> + debug("Unknown PMIC type\n"); >> + return -EINVAL; >> + } >> + >> + for (i = 0; i < ARRAY_SIZE(pca9450_reg_data); i++) { >> + if (strcmp(dev->name, pca9450_reg_data[i].name)) >> + continue; >> + >> + /* PCA9450B/PCA9450C uses BUCK1 and BUCK3 in dual-phase */ >> + if (type == NXP_CHIP_TYPE_PCA9450BC && >> + !strcmp(pca9450_reg_data[i].name, "BUCK3")) { >> + continue; >> + } >> + >> + *plat = pca9450_reg_data[i]; >> + >> + return 0; >> + } >> + >> + pr_err("Unknown regulator '%s'\n", dev->name); >> + >> + return -ENOENT; >> +} >> + >> +static const struct dm_regulator_ops pca9450_regulator_ops = { >> + .get_value = pca9450_get_value, >> + .set_value = pca9450_set_value, >> + .get_enable = pca9450_get_enable, >> + .set_enable = pca9450_set_enable, >> +}; >> + >> +U_BOOT_DRIVER(pca9450_regulator) = { >> + .name = PCA9450_REGULATOR_DRIVER, >> + .id = UCLASS_REGULATOR, >> + .ops = &pca9450_regulator_ops, >> + .probe = pca9450_regulator_probe, >> + .plat_auto = sizeof(struct pca9450_plat), >> +}; >> diff --git a/include/power/pca9450.h b/include/power/pca9450.h >> index 27703bb1f91..b714fc3477d 100644 >> --- a/include/power/pca9450.h >> +++ b/include/power/pca9450.h >> @@ -56,4 +56,15 @@ enum { >> >> int power_pca9450_init(unsigned char bus, unsigned char addr); >> >> +enum { >> + NXP_CHIP_TYPE_PCA9450A = 0, >> + NXP_CHIP_TYPE_PCA9450BC, >> + NXP_CHIP_TYPE_AMOUNT >> +}; >> + >> +#define DVS_BUCK_RUN_MASK 0x7f > > Marek, > > Looks like DVS_BUCK_RUN_MASK is a bad choice of names and should be > something like PCA9450_DVS_BUCK_RUN_MASK. > You have anticipated me - DVS_BUCK_RUN_MASK is redefined and breaks CI. https://source.denx.de/u-boot/custodians/u-boot-imx/-/jobs/436881 > It's defined differently in include/power/bd71837.h (where it should > also likely be changed to something that doesn't collide) > $ git grep DVS_BUCK_RUN_MASK include/ > include/power/bd71837.h:#define DVS_BUCK_RUN_MASK 0x3f > include/power/pca9450.h:#define DVS_BUCK_RUN_MASK 0x7f > > Granted, board/gateworks/venice/spl.c is the only file I see that > includes both of those. > > Is this merged yet? No, I set in patchworks that a new version is needed. Regards, Stefano > > Best Regards, > > Tim -- ===================================================================== DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de =====================================================================