| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGXv+5GbcyEEBaEhB1QY5uZ-CnKpvRgOYhqv==d72KXkVSSB3g@mail.gmail.com>
Date: Tue, 8 Jul 2025 12:57:41 +0800
From: Chen-Yu Tsai <wenst@...omium.org>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: linux-mediatek@...ts.infradead.org, lee@...nel.org, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, matthias.bgg@...il.com,
lgirdwood@...il.com, broonie@...nel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
kernel@...labora.com
Subject: Re: [PATCH v2 4/8] regulator: Add support for MediaTek MT6363 SPMI
PMIC Regulators
Hi,
On Mon, Jul 7, 2025 at 10:29 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com> wrote:
>
> Add a driver for the regulators found on the MT6363 PMIC, fully
> controlled by SPMI interface.
> This PMIC regulates voltage with an input range of 2.6-5.0V, and
> features 10 buck converters and 26 LDOs.
>
> Link: https://lore.kernel.org/r/20250624073548.29732-5-angelogioacchino.delregno@collabora.com
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
> ---
> drivers/regulator/Kconfig | 9 +
> drivers/regulator/Makefile | 1 +
> drivers/regulator/mt6363-regulator.c | 1076 ++++++++++++++++++++
> include/linux/regulator/mt6363-regulator.h | 326 ++++++
> 4 files changed, 1412 insertions(+)
> create mode 100644 drivers/regulator/mt6363-regulator.c
> create mode 100644 include/linux/regulator/mt6363-regulator.h
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 81f2acd0f960..d770e51f7ad1 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -936,6 +936,15 @@ config REGULATOR_MT6360
> 2-channel buck with Thermal Shutdown and Overload Protection
> 6-channel High PSRR and Low Dropout LDO.
>
> +config REGULATOR_MT6363
> + tristate "MT6363 SPMI PMIC regulator driver"
> + depends on SPMI || COMPILE_TEST
> + help
> + Say Y here to enable support for regulators found in the MediaTek
> + MT6363 SPMI PMIC.
> + This driver supports the control of different power rails of device
> + through regulator interface.
> +
> config REGULATOR_MT6370
> tristate "MT6370 SubPMIC Regulator"
> depends on MFD_MT6370
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 46c0e75f6107..41eaeac5547d 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -111,6 +111,7 @@ obj-$(CONFIG_REGULATOR_MT6357) += mt6357-regulator.o
> obj-$(CONFIG_REGULATOR_MT6358) += mt6358-regulator.o
> obj-$(CONFIG_REGULATOR_MT6359) += mt6359-regulator.o
> obj-$(CONFIG_REGULATOR_MT6360) += mt6360-regulator.o
> +obj-$(CONFIG_REGULATOR_MT6363) += mt6363-regulator.o
> obj-$(CONFIG_REGULATOR_MT6370) += mt6370-regulator.o
> obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o
> obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o
> diff --git a/drivers/regulator/mt6363-regulator.c b/drivers/regulator/mt6363-regulator.c
> new file mode 100644
> index 000000000000..ea7d4e4f38cb
> --- /dev/null
> +++ b/drivers/regulator/mt6363-regulator.c
> @@ -0,0 +1,1076 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (c) 2024 MediaTek Inc.
> +// Copyright (c) 2025 Collabora Ltd
> +// AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
> +
> +#include <linux/delay.h>
> +#include <linux/devm-helpers.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/regulator/mt6363-regulator.h>
> +#include <linux/regulator/of_regulator.h>
> +
> +#define MT6363_REGULATOR_MODE_NORMAL 0
> +#define MT6363_REGULATOR_MODE_FCCM 1
> +#define MT6363_REGULATOR_MODE_LP 2
> +#define MT6363_REGULATOR_MODE_ULP 3
> +
> +#define EN_SET_OFFSET 0x1
> +#define EN_CLR_OFFSET 0x2
> +#define OP_CFG_OFFSET 0x5
> +
> +#define NORMAL_OP_CFG 0x10
> +#define NORMAL_OP_EN 0x800000
> +
> +#define OC_IRQ_ENABLE_DELAY_MS 10
> +
> +/* Unlock keys for TMA and BUCK_TOP */
> +#define MT6363_TMA_UNLOCK_VALUE 0x9c9c
> +#define MT6363_BUCK_TOP_UNLOCK_VALUE 0x5543
> +
> +#define MT6363_RG_BUCK_EFUSE_RSV1 0x1447
> +#define MT6363_RG_BUCK_EFUSE_RSV1_MASK GENMASK(7, 4)
> +
> +enum {
> + MT6363_ID_VS2,
> + MT6363_ID_VBUCK1,
> + MT6363_ID_VBUCK2,
> + MT6363_ID_VBUCK3,
> + MT6363_ID_VBUCK4,
> + MT6363_ID_VBUCK5,
> + MT6363_ID_VBUCK6,
> + MT6363_ID_VBUCK7,
> + MT6363_ID_VS1,
> + MT6363_ID_VS3,
This ordering is a bit weird. Is there any particular reason for this?
Order of the registers perhaps?
> + MT6363_ID_VBUCK1_SSHUB,
> + MT6363_ID_VBUCK2_SSHUB,
> + MT6363_ID_VBUCK4_SSHUB,
> + MT6363_ID_VSRAM_DIGRF,
> + MT6363_ID_VSRAM_MDFE,
> + MT6363_ID_VSRAM_MODEM,
> + MT6363_ID_VSRAM_CPUB,
> + MT6363_ID_VSRAM_CPUM,
> + MT6363_ID_VSRAM_CPUL,
> + MT6363_ID_VSRAM_APU,
> + MT6363_ID_VEMC,
> + MT6363_ID_VCN13,
> + MT6363_ID_VTREF18,
> + MT6363_ID_VAUX18,
> + MT6363_ID_VCN15,
> + MT6363_ID_VUFS18,
> + MT6363_ID_VIO18,
> + MT6363_ID_VM18,
> + MT6363_ID_VA15,
> + MT6363_ID_VRF18,
> + MT6363_ID_VRFIO18,
> + MT6363_ID_VIO075,
> + MT6363_ID_VUFS12,
> + MT6363_ID_VA12_1,
> + MT6363_ID_VA12_2,
> + MT6363_ID_VRF12,
> + MT6363_ID_VRF13,
> + MT6363_ID_VRF09,
> + MT6363_ID_ISINK_LOAD
> +};
> +
[...]
> +#define MT6363_LDO_LINEAR_OPS(match, vreg, vops, min, max, step, \
> + en_reg, en_bit, vs_reg, vs_mask, \
> + lp_reg, lp_bit) \
> +[MT6363_ID_##vreg] = { \
> + .desc = { \
> + .name = match, \
> + .of_match = of_match_ptr(match), \
> + .ops = &vops, \
> + .type = REGULATOR_VOLTAGE, \
> + .id = MT6363_ID_##vreg, \
> + .owner = THIS_MODULE, \
> + .n_voltages = (max - min) / step + 1, \
> + .min_uV = min, \
> + .uV_step = step, \
> + .enable_reg = en_reg, \
> + .enable_mask = BIT(en_bit), \
> + .vsel_reg = vs_reg, \
> + .vsel_mask = vs_mask, \
> + .of_map_mode = mt6363_map_mode, \
> + }, \
> + .lp_mode_reg = lp_reg, \
> + .lp_mode_mask = BIT(lp_bit), \
> + .hw_lp_mode_reg = MT6363_LDO_##vreg##_HW_LP_MODE, \
> + .hw_lp_mode_mask = 0x4, \
> +}
Some of the LDOs have an extra "calibration" setting that can tweak
the output in 0.01V intervals. Could we support that. At least for
VRF12, VRF13, VRF18, the default is an extra 0.04V. It would be
misleading for the DT to specify 1.8V while the regulator is actually
outputting 1.84V.
You could look at the MT6358 driver to see how I implemented it.
[...]
> +static irqreturn_t mt6363_oc_isr(int irq, void *data)
> +{
> + struct regulator_dev *rdev = (struct regulator_dev *)data;
> + struct mt6363_regulator_info *info = rdev_get_drvdata(rdev);
> +
> + disable_irq_nosync(info->irq);
> +
> + if (regulator_is_enabled_regmap(rdev))
> + regulator_notifier_call_chain(rdev, REGULATOR_EVENT_OVER_CURRENT, NULL);
> +
> + schedule_delayed_work(&info->oc_work, msecs_to_jiffies(OC_IRQ_ENABLE_DELAY_MS));
Is this needed for debouncing?
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int mt6363_set_ocp(struct regulator_dev *rdev, int lim, int severity, bool enable)
> +{
> + struct mt6363_regulator_info *info = rdev_get_drvdata(rdev);
> +
> + /* MT6363 supports only enabling protection and does not support limits */
> + if (lim || severity != REGULATOR_SEVERITY_PROT || !enable)
> + return -EINVAL;
> +
> + /* If there is no OCP interrupt, there's nothing to set */
> + if (info->irq <= 0)
> + return -EINVAL;
-EOPNOTSUPP instead? One gives an error while the other gives a warning.
> +
> + return devm_request_threaded_irq(&rdev->dev, info->irq, NULL,
> + mt6363_oc_isr, IRQF_ONESHOT,
> + info->desc.name, rdev);
> +}
> +
[...]
> +/* The array is indexed by id(MT6363_ID_XXX) */
> +static struct mt6363_regulator_info mt6363_regulators[] = {
> + MT6363_BUCK("buck-vs2", VS2, 0, 1600000, 12500,
> + MT6363_RG_BUCK0_EN_ADDR,
> + MT6363_RG_BUCK_VS2_EN_BIT,
> + MT6363_RG_BUCK_VS2_VOSEL_ADDR,
> + MT6363_RG_BUCK_VS2_VOSEL_MASK,
> + MT6363_RG_BUCK0_LP_ADDR,
> + MT6363_RG_BUCK_VS2_LP_BIT,
> + MT6363_RG_BUCK0_FCCM_ADDR,
> + MT6363_RG_VS2_FCCM_BIT),
All this doesn't need to be spelled out. Perhaps you could use some
preprocessor magic to assemble all the register names so that the
entry could be simplified to just
MT6363_BUCK("buck-vs2", VS2, 0, 1600000, 12500,
MT6363_RG_BUCK0_FCCM_ADDR),
[...]
> + MT6363_SSHUB("buck-sshub1", VBUCK1_SSHUB, 0, 1193750, 6250,
> + MT6363_RG_BUCK_VBUCK1_SSHUB_EN_ADDR,
> + MT6363_RG_BUCK_VBUCK1_SSHUB_VOSEL_ADDR,
> + MT6363_RG_BUCK_VBUCK1_SSHUB_VOSEL_MASK),
> + MT6363_SSHUB("buck-sshub2", VBUCK2_SSHUB, 0, 1193750, 6250,
> + MT6363_RG_BUCK_VBUCK2_SSHUB_EN_ADDR,
> + MT6363_RG_BUCK_VBUCK2_SSHUB_VOSEL_ADDR,
> + MT6363_RG_BUCK_VBUCK2_SSHUB_VOSEL_MASK),
> + MT6363_SSHUB("buck-sshub4", VBUCK4_SSHUB, 0, 1193750, 6250,
> + MT6363_RG_BUCK_VBUCK4_SSHUB_EN_ADDR,
> + MT6363_RG_BUCK_VBUCK4_SSHUB_VOSEL_ADDR,
> + MT6363_RG_BUCK_VBUCK4_SSHUB_VOSEL_MASK),
As mentioned in my reply to the binding patch, these aren't real regulators.
> + MT6363_LDO_L_SC("ldo-vsram-digrf", VSRAM_DIGRF, 400000, 1193750, 6250,
> + MT6363_RG_BUCK1_EN_ADDR,
> + MT6363_RG_LDO_VSRAM_DIGRF_EN_BIT,
> + MT6363_RG_LDO_VSRAM_DIGRF_VOSEL_ADDR,
> + MT6363_RG_LDO_VSRAM_DIGRF_VOSEL_MASK,
> + MT6363_RG_BUCK1_LP_ADDR,
> + MT6363_RG_LDO_VSRAM_DIGRF_LP_BIT),
Same here, these could be compressed smaller with some preprocessor magic.
[...]
ChenYu
Powered by blists - more mailing lists