Hi,
This series is a cleanup and improvement of the MT6358 regulator driver.
Various discrepancies were found while preparing to upstream MT8186
device trees, which utilize the MT6366 PMIC, that is also covered by
this driver.
Patches 1~8 should go through the regulator tree, and patch 9 through
the soc tree. This series (patches 7 and 8) depends on "regulator: Use
bitfield values for range selectors" [1] I sent out earlier.
This series can be seen as three parts:
Part 1 - Fixing bogus regulators (patches 1~4 and 9)
There are some regulators listed in the bindings and driver that have no
corresponding pin on the actual hardware. MediaTek says these are a
hardware construct for shared control of the same regulator in the
VCN33 case and an alternative control scheme for low power suspend.
In the VCN33 case, there's only one actual regulator, so we merge the
two and rename them to match the hardware pin. No existing devices use
these AFAICT, so this should be safe to change.
In the *_SSHUB case, the two extra regulators refer to alternative
configuration registers of the same regulators. They are intended for
the SoC's low power mode companion processor to use, not the main
processor or OS. It should be left to the implementation to choose
which set of registers to actually control.
Part 2 - Code cleanup (patches 5 and 6)
Various tables in the regulator driver were not constant, even though
they are just lookup tables. With some reworking of the code, they are
made constant.
Also, some regulators that have a single linear range were using linear
range helpers. This is more complicated than just declaring the range
and step directly in the description. This is simplified to use the
latter approach.
Part 3 - Output voltage fine tuning support (patches 7 and 8)
Many of the LDOs on these PMIC support an extra level of output voltage
fine tuning. Most default to no offset, but a couple have a non-zero
offset by default. Previously this was unaccounted for in the driver and
device tree constraints. On the outputs with non-zero offset, this ends
up becoming a discrepancy between the device tree and actual hardware.
These two patches adds support for this second level of tuning, modeled
as bunch of linear ranges. While it's unlikely we need this level of
control, it's nice to be able to read back the accurate hardware
settings.
Please have a look. After this series is done I'll send out patches for
the MT6366 PMIC, which is what started this. That will also include
updated YAML bindings for MT6366. I think we can merge MT6358 bindings
into them afterwards.
Thanks
ChenYu
[1] https://lore.kernel.org/linux-arm-kernel/[email protected]/
Chen-Yu Tsai (9):
regulator: dt-bindings: mt6358: Merge ldo_vcn33_* regulators
regulator: dt-bindings: mt6358: Drop *_sshub regulators
regulator: mt6358: Merge VCN33_* regulators
regulator: mt6358: Drop *_SSHUB regulators
regulator: mt6358: Const-ify mt6358_regulator_info data structures
regulator: mt6358: Use linear voltage helpers for single range
regulators
regulator: mt6358: Add output voltage fine tuning to fixed regulators
regulator: mt6358: Add output voltage fine tuning to variable LDOs
arm64: dts: mediatek: mt6358: Merge ldo_vcn33_* regulators
.../bindings/regulator/mt6358-regulator.txt | 34 +-
arch/arm64/boot/dts/mediatek/mt6358.dtsi | 11 +-
drivers/regulator/mt6358-regulator.c | 499 ++++++++----------
include/linux/regulator/mt6358-regulator.h | 10 +-
4 files changed, 234 insertions(+), 320 deletions(-)
--
2.41.0.162.gfafddb0af9-goog
The ldo_vcn33_bt and ldo_vcn33_wifi regulators are actually the same
regulator, having the same voltage setting and output pin. There are
simply two enable bits that are ORed together to enable the regulator.
Having two regulators representing the same output pin is misleading
from a design matching standpoint, and also error-prone in driver
implementations.
Merge the two as ldo_vcn33. Neither vcn33 regulators are referenced
in upstream device trees. As far as hardware designs go, none of the
Chromebooks using MT8183 w/ MT6358 use this output.
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
.../bindings/regulator/mt6358-regulator.txt | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/Documentation/devicetree/bindings/regulator/mt6358-regulator.txt b/Documentation/devicetree/bindings/regulator/mt6358-regulator.txt
index 7034cdca54e0..b22b956d1cbe 100644
--- a/Documentation/devicetree/bindings/regulator/mt6358-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/mt6358-regulator.txt
@@ -15,8 +15,7 @@ LDO:
ldo_vcamd, ldo_vcn18, ldo_vfe28, ldo_vsram_proc11, ldo_vcn28, ldo_vsram_others,
ldo_vsram_others_sshub, ldo_vsram_gpu, ldo_vxo22, ldo_vefuse, ldo_vaux18,
ldo_vmch, ldo_vbif28, ldo_vsram_proc12, ldo_vcama1, ldo_vemc, ldo_vio28, ldo_va12,
- ldo_vrf18, ldo_vcn33_bt, ldo_vcn33_wifi, ldo_vcama2, ldo_vmc, ldo_vldo28, ldo_vaud28,
- ldo_vsim2
+ ldo_vrf18, ldo_vcn33, ldo_vcama2, ldo_vmc, ldo_vldo28, ldo_vaud28, ldo_vsim2
Example:
@@ -305,15 +304,8 @@ Example:
regulator-enable-ramp-delay = <120>;
};
- mt6358_vcn33_bt_reg: ldo_vcn33_bt {
- regulator-name = "vcn33_bt";
- regulator-min-microvolt = <3300000>;
- regulator-max-microvolt = <3500000>;
- regulator-enable-ramp-delay = <270>;
- };
-
- mt6358_vcn33_wifi_reg: ldo_vcn33_wifi {
- regulator-name = "vcn33_wifi";
+ mt6358_vcn33_reg: ldo_vcn33 {
+ regulator-name = "vcn33";
regulator-min-microvolt = <3300000>;
regulator-max-microvolt = <3500000>;
regulator-enable-ramp-delay = <270>;
--
2.41.0.162.gfafddb0af9-goog
The *_sshub regulators are actually alternate configuration interfaces
for their non *_sshub counterparts. They are not separate regulator
outputs. These registers are intended for the companion processor to
use to configure the power rails while the main processor is sleeping.
They are not intended for the main operating system to use.
Since they are not real outputs they shouldn't be modeled separately.
Remove them. Luckily no device tree actually uses them.
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
.../bindings/regulator/mt6358-regulator.txt | 22 +++++--------------
1 file changed, 5 insertions(+), 17 deletions(-)
diff --git a/Documentation/devicetree/bindings/regulator/mt6358-regulator.txt b/Documentation/devicetree/bindings/regulator/mt6358-regulator.txt
index b22b956d1cbe..b6384306db5c 100644
--- a/Documentation/devicetree/bindings/regulator/mt6358-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/mt6358-regulator.txt
@@ -8,14 +8,14 @@ Documentation/devicetree/bindings/regulator/regulator.txt.
The valid names for regulators are::
BUCK:
- buck_vdram1, buck_vcore, buck_vcore_sshub, buck_vpa, buck_vproc11,
- buck_vproc12, buck_vgpu, buck_vs2, buck_vmodem, buck_vs1
+ buck_vdram1, buck_vcore, buck_vpa, buck_vproc11, buck_vproc12, buck_vgpu,
+ buck_vs2, buck_vmodem, buck_vs1
LDO:
ldo_vdram2, ldo_vsim1, ldo_vibr, ldo_vrf12, ldo_vio18, ldo_vusb, ldo_vcamio,
ldo_vcamd, ldo_vcn18, ldo_vfe28, ldo_vsram_proc11, ldo_vcn28, ldo_vsram_others,
- ldo_vsram_others_sshub, ldo_vsram_gpu, ldo_vxo22, ldo_vefuse, ldo_vaux18,
- ldo_vmch, ldo_vbif28, ldo_vsram_proc12, ldo_vcama1, ldo_vemc, ldo_vio28, ldo_va12,
- ldo_vrf18, ldo_vcn33, ldo_vcama2, ldo_vmc, ldo_vldo28, ldo_vaud28, ldo_vsim2
+ ldo_vsram_gpu, ldo_vxo22, ldo_vefuse, ldo_vaux18, ldo_vmch, ldo_vbif28,
+ ldo_vsram_proc12, ldo_vcama1, ldo_vemc, ldo_vio28, ldo_va12, ldo_vrf18,
+ ldo_vcn33, ldo_vcama2, ldo_vmc, ldo_vldo28, ldo_vaud28, ldo_vsim2
Example:
@@ -346,17 +346,5 @@ Example:
regulator-max-microvolt = <3100000>;
regulator-enable-ramp-delay = <540>;
};
-
- mt6358_vcore_sshub_reg: buck_vcore_sshub {
- regulator-name = "vcore_sshub";
- regulator-min-microvolt = <500000>;
- regulator-max-microvolt = <1293750>;
- };
-
- mt6358_vsram_others_sshub_reg: ldo_vsram_others_sshub {
- regulator-name = "vsram_others_sshub";
- regulator-min-microvolt = <500000>;
- regulator-max-microvolt = <1293750>;
- };
};
};
--
2.41.0.162.gfafddb0af9-goog
The VCN33_BT and VCN33_WIFI regulators are actually the same regulator,
having the same voltage setting and output pin. There are simply two
enable bits that are ORed together to enable the regulator.
Having two regulators representing the same output pin is misleading
from a design matching standpoint, and also error-prone in driver
implementations. If consumers try to set different voltages on either
regulator, the one set later would override the one set before. There
are ways around this, such as chaining them together and having the
downstream one act as a switch. But given there's only one output pin,
such a workaround doesn't match reality.
Remove the VCN33_WIFI regulator. During the probe phase, have the driver
sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix
so that the regulator name matches the pin name in the datasheet.
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/regulator/mt6358-regulator.c | 65 +++++++++++++++++-----
include/linux/regulator/mt6358-regulator.h | 6 +-
2 files changed, 52 insertions(+), 19 deletions(-)
diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
index c9e16bd092f6..faf6b0757019 100644
--- a/drivers/regulator/mt6358-regulator.c
+++ b/drivers/regulator/mt6358-regulator.c
@@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = {
2800000, 2900000, 3000000,
};
-static const unsigned int vcn33_bt_wifi_voltages[] = {
+static const unsigned int vcn33_voltages[] = {
3300000, 3400000, 3500000,
};
@@ -321,7 +321,7 @@ static const u32 vcama_idx[] = {
0, 7, 9, 10, 11, 12,
};
-static const u32 vcn33_bt_wifi_idx[] = {
+static const u32 vcn33_idx[] = {
1, 2, 3,
};
@@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
- MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
- vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
- 0, MT6358_VCN33_ANA_CON0, 0x300),
- MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
- vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
- 0, MT6358_VCN33_ANA_CON0, 0x300),
+ MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
+ MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
MT6358_LDO("ldo_vcama2", VCAMA2, vcama_voltages, vcama_idx,
MT6358_LDO_VCAMA2_CON0, 0, MT6358_VCAMA2_ANA_CON0, 0xf00),
MT6358_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
@@ -662,12 +658,8 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
MT6358_LDO_VMCH_CON0, 0, MT6358_VMCH_ANA_CON0, 0x700),
MT6366_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
- MT6366_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
- vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
- 0, MT6358_VCN33_ANA_CON0, 0x300),
- MT6366_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
- vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
- 0, MT6358_VCN33_ANA_CON0, 0x300),
+ MT6366_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
+ MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
MT6366_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
MT6358_LDO_VMC_CON0, 0, MT6358_VMC_ANA_CON0, 0xf00),
MT6366_LDO("ldo_vsim2", VSIM2, vsim_voltages, vsim_idx,
@@ -690,13 +682,56 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
MT6358_LDO_VSRAM_CON1, 0x7f),
};
+static int mt6358_sync_vcn33_setting(struct device *dev)
+{
+ struct mt6397_chip *mt6397 = dev_get_drvdata(dev->parent);
+ unsigned int val;
+ int ret;
+
+ /*
+ * VCN33_WIFI and VCN33_BT are two separate enable bits for the same
+ * regulator. They share the same voltage setting and output pin.
+ * Instead of having two potentially conflicting regulators, just have
+ * one VCN33 regulator. Sync the two enable bits and only use one in
+ * the regulator device.
+ */
+ ret = regmap_read(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, &val);
+ if (ret) {
+ dev_err(dev, "Failed to read VCN33_WIFI setting\n");
+ return ret;
+ }
+
+ if (!(val & BIT(0)))
+ return 0;
+
+ /* Sync VCN33_WIFI enable status to VCN33_BT */
+ ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_0, BIT(0), BIT(0));
+ if (ret) {
+ dev_err(dev, "Failed to sync VCN33_WIFI setting to VCN33_BT\n");
+ return ret;
+ }
+
+ /* Disable VCN33_WIFI */
+ ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, BIT(0), 0);
+ if (ret) {
+ dev_err(dev, "Failed to disable VCN33_BT\n");
+ return ret;
+ }
+
+ return 0;
+}
+
static int mt6358_regulator_probe(struct platform_device *pdev)
{
struct mt6397_chip *mt6397 = dev_get_drvdata(pdev->dev.parent);
struct regulator_config config = {};
struct regulator_dev *rdev;
struct mt6358_regulator_info *mt6358_info;
- int i, max_regulator;
+ int i, max_regulator, ret;
+
+ ret = mt6358_sync_vcn33_setting(&pdev->dev);
+ if (ret)
+ return ret;
if (mt6397->chip_id == MT6366_CHIP_ID) {
max_regulator = MT6366_MAX_REGULATOR;
diff --git a/include/linux/regulator/mt6358-regulator.h b/include/linux/regulator/mt6358-regulator.h
index bdcf83cd719e..a4307cd9edd6 100644
--- a/include/linux/regulator/mt6358-regulator.h
+++ b/include/linux/regulator/mt6358-regulator.h
@@ -41,8 +41,7 @@ enum {
MT6358_ID_VIO28,
MT6358_ID_VA12,
MT6358_ID_VRF18,
- MT6358_ID_VCN33_BT,
- MT6358_ID_VCN33_WIFI,
+ MT6358_ID_VCN33,
MT6358_ID_VCAMA2,
MT6358_ID_VMC,
MT6358_ID_VLDO28,
@@ -85,8 +84,7 @@ enum {
MT6366_ID_VIO28,
MT6366_ID_VA12,
MT6366_ID_VRF18,
- MT6366_ID_VCN33_BT,
- MT6366_ID_VCN33_WIFI,
+ MT6366_ID_VCN33,
MT6366_ID_VMC,
MT6366_ID_VAUD28,
MT6366_ID_VSIM2,
--
2.41.0.162.gfafddb0af9-goog
In the MT6358 regulator driver, each regulator is described by a
|struct regulator_desc| wrapped by a |struct mt6358_regulator_info|.
The latter was tied to the regulator device using the config's
driver_data field, which meant that the variables could not be constant.
Since each regulator device has a pointer to its regulator_desc, and
mt6358_regulator_info wraps that, the driver could use container_of()
to retrieve it instead.
Switch to using container_of(), drop tha driver_data setting, and
const-ify all the regulator descriptions.
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/regulator/mt6358-regulator.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
index 946a251a8b3a..2851ef53ac63 100644
--- a/drivers/regulator/mt6358-regulator.c
+++ b/drivers/regulator/mt6358-regulator.c
@@ -34,6 +34,8 @@ struct mt6358_regulator_info {
u32 modeset_mask;
};
+#define to_regulator_info(x) container_of((x), struct mt6358_regulator_info, desc)
+
#define MT6358_BUCK(match, vreg, min, max, step, \
volt_ranges, vosel_mask, _da_vsel_reg, _da_vsel_mask, \
_modeset_reg, _modeset_shift) \
@@ -342,9 +344,9 @@ static unsigned int mt6358_map_mode(unsigned int mode)
static int mt6358_set_voltage_sel(struct regulator_dev *rdev,
unsigned int selector)
{
+ const struct mt6358_regulator_info *info = to_regulator_info(rdev->desc);
int idx, ret;
const u32 *pvol;
- struct mt6358_regulator_info *info = rdev_get_drvdata(rdev);
pvol = info->index_table;
@@ -358,9 +360,9 @@ static int mt6358_set_voltage_sel(struct regulator_dev *rdev,
static int mt6358_get_voltage_sel(struct regulator_dev *rdev)
{
+ const struct mt6358_regulator_info *info = to_regulator_info(rdev->desc);
int idx, ret;
u32 selector;
- struct mt6358_regulator_info *info = rdev_get_drvdata(rdev);
const u32 *pvol;
ret = regmap_read(rdev->regmap, info->desc.vsel_reg, &selector);
@@ -384,8 +386,8 @@ static int mt6358_get_voltage_sel(struct regulator_dev *rdev)
static int mt6358_get_buck_voltage_sel(struct regulator_dev *rdev)
{
+ const struct mt6358_regulator_info *info = to_regulator_info(rdev->desc);
int ret, regval;
- struct mt6358_regulator_info *info = rdev_get_drvdata(rdev);
ret = regmap_read(rdev->regmap, info->da_vsel_reg, ®val);
if (ret != 0) {
@@ -402,9 +404,9 @@ static int mt6358_get_buck_voltage_sel(struct regulator_dev *rdev)
static int mt6358_get_status(struct regulator_dev *rdev)
{
+ const struct mt6358_regulator_info *info = to_regulator_info(rdev->desc);
int ret;
u32 regval;
- struct mt6358_regulator_info *info = rdev_get_drvdata(rdev);
ret = regmap_read(rdev->regmap, info->status_reg, ®val);
if (ret != 0) {
@@ -418,7 +420,7 @@ static int mt6358_get_status(struct regulator_dev *rdev)
static int mt6358_regulator_set_mode(struct regulator_dev *rdev,
unsigned int mode)
{
- struct mt6358_regulator_info *info = rdev_get_drvdata(rdev);
+ const struct mt6358_regulator_info *info = to_regulator_info(rdev->desc);
int val;
switch (mode) {
@@ -443,7 +445,7 @@ static int mt6358_regulator_set_mode(struct regulator_dev *rdev,
static unsigned int mt6358_regulator_get_mode(struct regulator_dev *rdev)
{
- struct mt6358_regulator_info *info = rdev_get_drvdata(rdev);
+ const struct mt6358_regulator_info *info = to_regulator_info(rdev->desc);
int ret, regval;
ret = regmap_read(rdev->regmap, info->modeset_reg, ®val);
@@ -498,7 +500,7 @@ static const struct regulator_ops mt6358_volt_fixed_ops = {
};
/* The array is indexed by id(MT6358_ID_XXX) */
-static struct mt6358_regulator_info mt6358_regulators[] = {
+static const struct mt6358_regulator_info mt6358_regulators[] = {
MT6358_BUCK("buck_vdram1", VDRAM1, 500000, 2087500, 12500,
buck_volt_range2, 0x7f, MT6358_BUCK_VDRAM1_DBG0, 0x7f,
MT6358_VDRAM1_ANA_CON0, 8),
@@ -589,7 +591,7 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
};
/* The array is indexed by id(MT6366_ID_XXX) */
-static struct mt6358_regulator_info mt6366_regulators[] = {
+static const struct mt6358_regulator_info mt6366_regulators[] = {
MT6366_BUCK("buck_vdram1", VDRAM1, 500000, 2087500, 12500,
buck_volt_range2, 0x7f, MT6358_BUCK_VDRAM1_DBG0, 0x7f,
MT6358_VDRAM1_ANA_CON0, 8),
@@ -712,7 +714,7 @@ static int mt6358_regulator_probe(struct platform_device *pdev)
struct mt6397_chip *mt6397 = dev_get_drvdata(pdev->dev.parent);
struct regulator_config config = {};
struct regulator_dev *rdev;
- struct mt6358_regulator_info *mt6358_info;
+ const struct mt6358_regulator_info *mt6358_info;
int i, max_regulator, ret;
ret = mt6358_sync_vcn33_setting(&pdev->dev);
@@ -729,7 +731,6 @@ static int mt6358_regulator_probe(struct platform_device *pdev)
for (i = 0; i < max_regulator; i++) {
config.dev = &pdev->dev;
- config.driver_data = &mt6358_info[i];
config.regmap = mt6397->regmap;
rdev = devm_regulator_register(&pdev->dev,
--
2.41.0.162.gfafddb0af9-goog
Some of the LDO regulators in the MT6358/MT6366 have sparsely populated
voltage tables, supported by custom get/set operators. While it works,
it requires more code and an extra field to store the lookup table.
These LDOs also have fine voltage calibration settings that can slightly
boost the output voltage from 0 mV to 100 mV, in 10 mV increments.
These combined could be modeled as a pickable set of linear ranges. The
coarse voltage setting is modeled as the range selector, while each
range has 11 selectors, starting from the range's base voltage, up to
+100 mV, in 10mV increments.
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/regulator/mt6358-regulator.c | 275 +++++++++++----------------
1 file changed, 115 insertions(+), 160 deletions(-)
diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
index 26060909cf90..0b186b66ae29 100644
--- a/drivers/regulator/mt6358-regulator.c
+++ b/drivers/regulator/mt6358-regulator.c
@@ -26,8 +26,6 @@ struct mt6358_regulator_info {
struct regulator_desc desc;
u32 status_reg;
u32 qi;
- const u32 *index_table;
- unsigned int n_table;
u32 da_vsel_reg;
u32 da_vsel_mask;
u32 modeset_reg;
@@ -64,9 +62,7 @@ struct mt6358_regulator_info {
.modeset_mask = BIT(_modeset_shift), \
}
-#define MT6358_LDO(match, vreg, ldo_volt_table, \
- ldo_index_table, enreg, enbit, vosel, \
- vosel_mask) \
+#define MT6358_LDO(match, vreg, volt_ranges, enreg, enbit, vosel, vosel_mask) \
[MT6358_ID_##vreg] = { \
.desc = { \
.name = #vreg, \
@@ -75,17 +71,19 @@ struct mt6358_regulator_info {
.type = REGULATOR_VOLTAGE, \
.id = MT6358_ID_##vreg, \
.owner = THIS_MODULE, \
- .n_voltages = ARRAY_SIZE(ldo_volt_table), \
- .volt_table = ldo_volt_table, \
- .vsel_reg = vosel, \
- .vsel_mask = vosel_mask, \
+ .n_voltages = ARRAY_SIZE(volt_ranges##_ranges) * 11, \
+ .linear_ranges = volt_ranges##_ranges, \
+ .linear_range_selectors = volt_ranges##_selectors, \
+ .n_linear_ranges = ARRAY_SIZE(volt_ranges##_ranges), \
+ .vsel_range_reg = vosel, \
+ .vsel_range_mask = vosel_mask, \
+ .vsel_reg = MT6358_##vreg##_ANA_CON0, \
+ .vsel_mask = GENMASK(3, 0), \
.enable_reg = enreg, \
.enable_mask = BIT(enbit), \
}, \
.status_reg = MT6358_LDO_##vreg##_CON1, \
.qi = BIT(15), \
- .index_table = ldo_index_table, \
- .n_table = ARRAY_SIZE(ldo_index_table), \
}
#define MT6358_LDO1(match, vreg, min, max, step, \
@@ -163,9 +161,7 @@ struct mt6358_regulator_info {
.modeset_mask = BIT(_modeset_shift), \
}
-#define MT6366_LDO(match, vreg, ldo_volt_table, \
- ldo_index_table, enreg, enbit, vosel, \
- vosel_mask) \
+#define MT6366_LDO(match, vreg, volt_ranges, enreg, enbit, vosel, vosel_mask) \
[MT6366_ID_##vreg] = { \
.desc = { \
.name = #vreg, \
@@ -174,17 +170,19 @@ struct mt6358_regulator_info {
.type = REGULATOR_VOLTAGE, \
.id = MT6366_ID_##vreg, \
.owner = THIS_MODULE, \
- .n_voltages = ARRAY_SIZE(ldo_volt_table), \
- .volt_table = ldo_volt_table, \
- .vsel_reg = vosel, \
- .vsel_mask = vosel_mask, \
+ .n_voltages = ARRAY_SIZE(volt_ranges##_ranges) * 11, \
+ .linear_ranges = volt_ranges##_ranges, \
+ .linear_range_selectors = volt_ranges##_selectors, \
+ .n_linear_ranges = ARRAY_SIZE(volt_ranges##_ranges), \
+ .vsel_range_reg = vosel, \
+ .vsel_range_mask = vosel_mask, \
+ .vsel_reg = MT6358_##vreg##_ANA_CON0, \
+ .vsel_mask = GENMASK(3, 0), \
.enable_reg = enreg, \
.enable_mask = BIT(enbit), \
}, \
.status_reg = MT6358_LDO_##vreg##_CON1, \
.qi = BIT(15), \
- .index_table = ldo_index_table, \
- .n_table = ARRAY_SIZE(ldo_index_table), \
}
#define MT6366_LDO1(match, vreg, min, max, step, \
@@ -235,95 +233,95 @@ struct mt6358_regulator_info {
}
-static const unsigned int vdram2_voltages[] = {
- 600000, 1800000,
-};
-
-static const unsigned int vsim_voltages[] = {
- 1700000, 1800000, 2700000, 3000000, 3100000,
-};
-
-static const unsigned int vibr_voltages[] = {
- 1200000, 1300000, 1500000, 1800000,
- 2000000, 2800000, 3000000, 3300000,
+/* VDRAM2 voltage selector not shown in datasheet */
+static const unsigned int vdram2_selectors[] = { 0, 12 };
+static const struct linear_range vdram2_ranges[] = {
+ REGULATOR_LINEAR_RANGE(600000, 0, 10, 10000),
+ REGULATOR_LINEAR_RANGE(1800000, 11, 21, 10000),
};
-static const unsigned int vusb_voltages[] = {
- 3000000, 3100000,
+static const unsigned int vsim_selectors[] = { 3, 4, 8, 11, 12 };
+static const struct linear_range vsim_ranges[] = {
+ REGULATOR_LINEAR_RANGE(1700000, 0, 10, 10000),
+ REGULATOR_LINEAR_RANGE(1800000, 11, 21, 10000),
+ REGULATOR_LINEAR_RANGE(2700000, 22, 32, 10000),
+ REGULATOR_LINEAR_RANGE(3000000, 33, 43, 10000),
+ REGULATOR_LINEAR_RANGE(3100000, 44, 54, 10000),
};
-static const unsigned int vcamd_voltages[] = {
- 900000, 1000000, 1100000, 1200000,
- 1300000, 1500000, 1800000,
+static const unsigned int vibr_selectors[] = { 0, 1, 2, 4, 5, 9, 11, 13 };
+static const struct linear_range vibr_ranges[] = {
+ REGULATOR_LINEAR_RANGE(1200000, 0, 10, 10000),
+ REGULATOR_LINEAR_RANGE(1300000, 11, 21, 10000),
+ REGULATOR_LINEAR_RANGE(1500000, 22, 32, 10000),
+ REGULATOR_LINEAR_RANGE(1800000, 33, 43, 10000),
+ REGULATOR_LINEAR_RANGE(2000000, 44, 54, 10000),
+ REGULATOR_LINEAR_RANGE(2800000, 55, 65, 10000),
+ REGULATOR_LINEAR_RANGE(3000000, 66, 76, 10000),
+ REGULATOR_LINEAR_RANGE(3300000, 77, 87, 10000),
};
-static const unsigned int vefuse_voltages[] = {
- 1700000, 1800000, 1900000,
+/* VUSB voltage selector not shown in datasheet */
+static const unsigned int vusb_selectors[] = { 3, 4 };
+static const struct linear_range vusb_ranges[] = {
+ REGULATOR_LINEAR_RANGE(3000000, 0, 10, 10000),
+ REGULATOR_LINEAR_RANGE(3100000, 11, 21, 10000),
};
-static const unsigned int vmch_vemc_voltages[] = {
- 2900000, 3000000, 3300000,
+static const unsigned int vcamd_selectors[] = { 3, 4, 5, 6, 7, 9, 12 };
+static const struct linear_range vcamd_ranges[] = {
+ REGULATOR_LINEAR_RANGE(900000, 0, 10, 10000),
+ REGULATOR_LINEAR_RANGE(1000000, 11, 21, 10000),
+ REGULATOR_LINEAR_RANGE(1100000, 22, 32, 10000),
+ REGULATOR_LINEAR_RANGE(1200000, 33, 43, 10000),
+ REGULATOR_LINEAR_RANGE(1300000, 44, 54, 10000),
+ REGULATOR_LINEAR_RANGE(1500000, 55, 65, 10000),
+ REGULATOR_LINEAR_RANGE(1800000, 66, 76, 10000),
};
-static const unsigned int vcama_voltages[] = {
- 1800000, 2500000, 2700000,
- 2800000, 2900000, 3000000,
+static const unsigned int vefuse_selectors[] = { 11, 12, 13 };
+static const struct linear_range vefuse_ranges[] = {
+ REGULATOR_LINEAR_RANGE(1700000, 0, 10, 10000),
+ REGULATOR_LINEAR_RANGE(1800000, 11, 21, 10000),
+ REGULATOR_LINEAR_RANGE(1900000, 22, 32, 10000),
};
-static const unsigned int vcn33_voltages[] = {
- 3300000, 3400000, 3500000,
+static const unsigned int vmch_vemc_selectors[] = { 2, 3, 5 };
+static const struct linear_range vmch_vemc_ranges[] = {
+ REGULATOR_LINEAR_RANGE(2900000, 0, 10, 10000),
+ REGULATOR_LINEAR_RANGE(3000000, 11, 21, 10000),
+ REGULATOR_LINEAR_RANGE(3300000, 22, 32, 10000),
};
-static const unsigned int vmc_voltages[] = {
- 1800000, 2900000, 3000000, 3300000,
+static const unsigned int vcama_selectors[] = { 0, 7, 9, 10, 11, 12 };
+static const struct linear_range vcama_ranges[] = {
+ REGULATOR_LINEAR_RANGE(1800000, 0, 10, 10000),
+ REGULATOR_LINEAR_RANGE(2500000, 11, 21, 10000),
+ REGULATOR_LINEAR_RANGE(2700000, 22, 32, 10000),
+ REGULATOR_LINEAR_RANGE(2800000, 33, 43, 10000),
+ REGULATOR_LINEAR_RANGE(2900000, 44, 54, 10000),
+ REGULATOR_LINEAR_RANGE(3000000, 55, 65, 10000),
};
-static const unsigned int vldo28_voltages[] = {
- 2800000, 3000000,
+static const unsigned int vcn33_selectors[] = { 1, 2, 3 };
+static const struct linear_range vcn33_ranges[] = {
+ REGULATOR_LINEAR_RANGE(3300000, 0, 10, 10000),
+ REGULATOR_LINEAR_RANGE(3400000, 11, 21, 10000),
+ REGULATOR_LINEAR_RANGE(3500000, 22, 32, 10000),
};
-static const u32 vdram2_idx[] = {
- 0, 12,
+static const unsigned int vmc_selectors[] = { 4, 10, 11, 13 };
+static const struct linear_range vmc_ranges[] = {
+ REGULATOR_LINEAR_RANGE(1800000, 0, 10, 10000),
+ REGULATOR_LINEAR_RANGE(2900000, 11, 21, 10000),
+ REGULATOR_LINEAR_RANGE(3000000, 22, 32, 10000),
+ REGULATOR_LINEAR_RANGE(3300000, 33, 43, 10000),
};
-static const u32 vsim_idx[] = {
- 3, 4, 8, 11, 12,
-};
-
-static const u32 vibr_idx[] = {
- 0, 1, 2, 4, 5, 9, 11, 13,
-};
-
-static const u32 vusb_idx[] = {
- 3, 4,
-};
-
-static const u32 vcamd_idx[] = {
- 3, 4, 5, 6, 7, 9, 12,
-};
-
-static const u32 vefuse_idx[] = {
- 11, 12, 13,
-};
-
-static const u32 vmch_vemc_idx[] = {
- 2, 3, 5,
-};
-
-static const u32 vcama_idx[] = {
- 0, 7, 9, 10, 11, 12,
-};
-
-static const u32 vcn33_idx[] = {
- 1, 2, 3,
-};
-
-static const u32 vmc_idx[] = {
- 4, 10, 11, 13,
-};
-
-static const u32 vldo28_idx[] = {
- 1, 3,
+static const unsigned int vldo28_selectors[] = { 1, 3 };
+static const struct linear_range vldo28_ranges[] = {
+ REGULATOR_LINEAR_RANGE(2800000, 0, 10, 10000),
+ REGULATOR_LINEAR_RANGE(3000000, 11, 21, 10000),
};
static unsigned int mt6358_map_mode(unsigned int mode)
@@ -332,49 +330,6 @@ static unsigned int mt6358_map_mode(unsigned int mode)
REGULATOR_MODE_NORMAL : REGULATOR_MODE_FAST;
}
-static int mt6358_set_voltage_sel(struct regulator_dev *rdev,
- unsigned int selector)
-{
- const struct mt6358_regulator_info *info = to_regulator_info(rdev->desc);
- int idx, ret;
- const u32 *pvol;
-
- pvol = info->index_table;
-
- idx = pvol[selector];
- idx <<= ffs(info->desc.vsel_mask) - 1;
- ret = regmap_update_bits(rdev->regmap, info->desc.vsel_reg,
- info->desc.vsel_mask, idx);
-
- return ret;
-}
-
-static int mt6358_get_voltage_sel(struct regulator_dev *rdev)
-{
- const struct mt6358_regulator_info *info = to_regulator_info(rdev->desc);
- int idx, ret;
- u32 selector;
- const u32 *pvol;
-
- ret = regmap_read(rdev->regmap, info->desc.vsel_reg, &selector);
- if (ret != 0) {
- dev_info(&rdev->dev,
- "Failed to get mt6358 %s vsel reg: %d\n",
- info->desc.name, ret);
- return ret;
- }
-
- selector = (selector & info->desc.vsel_mask) >>
- (ffs(info->desc.vsel_mask) - 1);
- pvol = info->index_table;
- for (idx = 0; idx < info->desc.n_voltages; idx++) {
- if (pvol[idx] == selector)
- return idx;
- }
-
- return -EINVAL;
-}
-
static int mt6358_get_buck_voltage_sel(struct regulator_dev *rdev)
{
const struct mt6358_regulator_info *info = to_regulator_info(rdev->desc);
@@ -471,10 +426,10 @@ static const struct regulator_ops mt6358_volt_range_ops = {
};
static const struct regulator_ops mt6358_volt_table_ops = {
- .list_voltage = regulator_list_voltage_table,
- .map_voltage = regulator_map_voltage_iterate,
- .set_voltage_sel = mt6358_set_voltage_sel,
- .get_voltage_sel = mt6358_get_voltage_sel,
+ .list_voltage = regulator_list_voltage_pickable_linear_range,
+ .map_voltage = regulator_map_voltage_pickable_linear_range,
+ .set_voltage_sel = regulator_set_voltage_sel_pickable_regmap,
+ .get_voltage_sel = regulator_get_voltage_sel_pickable_regmap,
.set_voltage_time_sel = regulator_set_voltage_time_sel,
.enable = regulator_enable_regmap,
.disable = regulator_disable_regmap,
@@ -534,34 +489,34 @@ static const struct mt6358_regulator_info mt6358_regulators[] = {
MT6358_REG_FIXED("ldo_vrf18", VRF18, MT6358_LDO_VRF18_CON0, 0, 1800000),
MT6358_REG_FIXED("ldo_vaud28", VAUD28,
MT6358_LDO_VAUD28_CON0, 0, 2800000),
- MT6358_LDO("ldo_vdram2", VDRAM2, vdram2_voltages, vdram2_idx,
+ MT6358_LDO("ldo_vdram2", VDRAM2, vdram2,
MT6358_LDO_VDRAM2_CON0, 0, MT6358_LDO_VDRAM2_ELR0, 0xf),
- MT6358_LDO("ldo_vsim1", VSIM1, vsim_voltages, vsim_idx,
+ MT6358_LDO("ldo_vsim1", VSIM1, vsim,
MT6358_LDO_VSIM1_CON0, 0, MT6358_VSIM1_ANA_CON0, 0xf00),
- MT6358_LDO("ldo_vibr", VIBR, vibr_voltages, vibr_idx,
+ MT6358_LDO("ldo_vibr", VIBR, vibr,
MT6358_LDO_VIBR_CON0, 0, MT6358_VIBR_ANA_CON0, 0xf00),
- MT6358_LDO("ldo_vusb", VUSB, vusb_voltages, vusb_idx,
+ MT6358_LDO("ldo_vusb", VUSB, vusb,
MT6358_LDO_VUSB_CON0_0, 0, MT6358_VUSB_ANA_CON0, 0x700),
- MT6358_LDO("ldo_vcamd", VCAMD, vcamd_voltages, vcamd_idx,
+ MT6358_LDO("ldo_vcamd", VCAMD, vcamd,
MT6358_LDO_VCAMD_CON0, 0, MT6358_VCAMD_ANA_CON0, 0xf00),
- MT6358_LDO("ldo_vefuse", VEFUSE, vefuse_voltages, vefuse_idx,
+ MT6358_LDO("ldo_vefuse", VEFUSE, vefuse,
MT6358_LDO_VEFUSE_CON0, 0, MT6358_VEFUSE_ANA_CON0, 0xf00),
- MT6358_LDO("ldo_vmch", VMCH, vmch_vemc_voltages, vmch_vemc_idx,
+ MT6358_LDO("ldo_vmch", VMCH, vmch_vemc,
MT6358_LDO_VMCH_CON0, 0, MT6358_VMCH_ANA_CON0, 0x700),
- MT6358_LDO("ldo_vcama1", VCAMA1, vcama_voltages, vcama_idx,
+ MT6358_LDO("ldo_vcama1", VCAMA1, vcama,
MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
- MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
+ MT6358_LDO("ldo_vemc", VEMC, vmch_vemc,
MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
- MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
+ MT6358_LDO("ldo_vcn33", VCN33, vcn33,
MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
- MT6358_LDO("ldo_vcama2", VCAMA2, vcama_voltages, vcama_idx,
+ MT6358_LDO("ldo_vcama2", VCAMA2, vcama,
MT6358_LDO_VCAMA2_CON0, 0, MT6358_VCAMA2_ANA_CON0, 0xf00),
- MT6358_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
+ MT6358_LDO("ldo_vmc", VMC, vmc,
MT6358_LDO_VMC_CON0, 0, MT6358_VMC_ANA_CON0, 0xf00),
- MT6358_LDO("ldo_vldo28", VLDO28, vldo28_voltages, vldo28_idx,
+ MT6358_LDO("ldo_vldo28", VLDO28, vldo28,
MT6358_LDO_VLDO28_CON0_0, 0,
MT6358_VLDO28_ANA_CON0, 0x300),
- MT6358_LDO("ldo_vsim2", VSIM2, vsim_voltages, vsim_idx,
+ MT6358_LDO("ldo_vsim2", VSIM2, vsim,
MT6358_LDO_VSIM2_CON0, 0, MT6358_VSIM2_ANA_CON0, 0xf00),
MT6358_LDO1("ldo_vsram_proc11", VSRAM_PROC11, 500000, 1293750, 6250,
MT6358_LDO_VSRAM_PROC11_DBG0, 0x7f00, MT6358_LDO_VSRAM_CON0, 0x7f),
@@ -610,25 +565,25 @@ static const struct mt6358_regulator_info mt6366_regulators[] = {
MT6366_REG_FIXED("ldo_vrf18", VRF18, MT6358_LDO_VRF18_CON0, 0, 1800000),
MT6366_REG_FIXED("ldo_vaud28", VAUD28,
MT6358_LDO_VAUD28_CON0, 0, 2800000),
- MT6366_LDO("ldo_vdram2", VDRAM2, vdram2_voltages, vdram2_idx,
+ MT6366_LDO("ldo_vdram2", VDRAM2, vdram2,
MT6358_LDO_VDRAM2_CON0, 0, MT6358_LDO_VDRAM2_ELR0, 0x10),
- MT6366_LDO("ldo_vsim1", VSIM1, vsim_voltages, vsim_idx,
+ MT6366_LDO("ldo_vsim1", VSIM1, vsim,
MT6358_LDO_VSIM1_CON0, 0, MT6358_VSIM1_ANA_CON0, 0xf00),
- MT6366_LDO("ldo_vibr", VIBR, vibr_voltages, vibr_idx,
+ MT6366_LDO("ldo_vibr", VIBR, vibr,
MT6358_LDO_VIBR_CON0, 0, MT6358_VIBR_ANA_CON0, 0xf00),
- MT6366_LDO("ldo_vusb", VUSB, vusb_voltages, vusb_idx,
+ MT6366_LDO("ldo_vusb", VUSB, vusb,
MT6358_LDO_VUSB_CON0_0, 0, MT6358_VUSB_ANA_CON0, 0x700),
- MT6366_LDO("ldo_vefuse", VEFUSE, vefuse_voltages, vefuse_idx,
+ MT6366_LDO("ldo_vefuse", VEFUSE, vefuse,
MT6358_LDO_VEFUSE_CON0, 0, MT6358_VEFUSE_ANA_CON0, 0xf00),
- MT6366_LDO("ldo_vmch", VMCH, vmch_vemc_voltages, vmch_vemc_idx,
+ MT6366_LDO("ldo_vmch", VMCH, vmch_vemc,
MT6358_LDO_VMCH_CON0, 0, MT6358_VMCH_ANA_CON0, 0x700),
- MT6366_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
+ MT6366_LDO("ldo_vemc", VEMC, vmch_vemc,
MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
- MT6366_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
+ MT6366_LDO("ldo_vcn33", VCN33, vcn33,
MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
- MT6366_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
+ MT6366_LDO("ldo_vmc", VMC, vmc,
MT6358_LDO_VMC_CON0, 0, MT6358_VMC_ANA_CON0, 0xf00),
- MT6366_LDO("ldo_vsim2", VSIM2, vsim_voltages, vsim_idx,
+ MT6366_LDO("ldo_vsim2", VSIM2, vsim,
MT6358_LDO_VSIM2_CON0, 0, MT6358_VSIM2_ANA_CON0, 0xf00),
MT6366_LDO1("ldo_vsram_proc11", VSRAM_PROC11, 500000, 1293750, 6250,
MT6358_LDO_VSRAM_PROC11_DBG0, 0x7f00, MT6358_LDO_VSRAM_CON0, 0x7f),
--
2.41.0.162.gfafddb0af9-goog
The "fixed" LDO regulators found on the MT6358 and MT6366 PMICs have
either no voltage selection register, or only one valid setting.
However these do have a fine voltage calibration setting that can
slightly boost the output voltage from 0 mV to 100 mV, in 10 mV
increments.
Add support for this by changing these into linear range regulators.
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/regulator/mt6358-regulator.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
index 31a16fb28ecd..26060909cf90 100644
--- a/drivers/regulator/mt6358-regulator.c
+++ b/drivers/regulator/mt6358-regulator.c
@@ -123,10 +123,13 @@ struct mt6358_regulator_info {
.type = REGULATOR_VOLTAGE, \
.id = MT6358_ID_##vreg, \
.owner = THIS_MODULE, \
- .n_voltages = 1, \
+ .n_voltages = 11, \
+ .vsel_reg = MT6358_##vreg##_ANA_CON0, \
+ .vsel_mask = GENMASK(3, 0), \
.enable_reg = enreg, \
.enable_mask = BIT(enbit), \
.min_uV = volt, \
+ .uV_step = 10000, \
}, \
.status_reg = MT6358_LDO_##vreg##_CON1, \
.qi = BIT(15), \
@@ -219,10 +222,13 @@ struct mt6358_regulator_info {
.type = REGULATOR_VOLTAGE, \
.id = MT6366_ID_##vreg, \
.owner = THIS_MODULE, \
- .n_voltages = 1, \
+ .n_voltages = 11, \
+ .vsel_reg = MT6358_##vreg##_ANA_CON0, \
+ .vsel_mask = GENMASK(3, 0), \
.enable_reg = enreg, \
.enable_mask = BIT(enbit), \
.min_uV = volt, \
+ .uV_step = 10000, \
}, \
.status_reg = MT6358_LDO_##vreg##_CON1, \
.qi = BIT(15), \
@@ -476,8 +482,13 @@ static const struct regulator_ops mt6358_volt_table_ops = {
.get_status = mt6358_get_status,
};
+/* "Fixed" LDOs with output voltage calibration +0 ~ +10 mV */
static const struct regulator_ops mt6358_volt_fixed_ops = {
.list_voltage = regulator_list_voltage_linear,
+ .map_voltage = regulator_map_voltage_linear,
+ .set_voltage_sel = regulator_set_voltage_sel_regmap,
+ .get_voltage_sel = mt6358_get_buck_voltage_sel,
+ .set_voltage_time_sel = regulator_set_voltage_time_sel,
.enable = regulator_enable_regmap,
.disable = regulator_disable_regmap,
.is_enabled = regulator_is_enabled_regmap,
--
2.41.0.162.gfafddb0af9-goog
The ldo_vcn33_bt and ldo_vcn33_wifi regulators are actually the same
regulator, having the same voltage setting and output pin. There are
simply two enable bits that are ORed together to enable the regulator.
Having two regulators representing the same output pin is misleading
from a design matching standpoint, and also error-prone in driver
implementations.
Now that the bindings have these two merged, merge them in the device
tree as well. Neither vcn33 regulators are referenced in upstream
device trees. As far as hardware designs go, none of the Chromebooks
using MT8183 w/ MT6358 use this output.
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
arch/arm64/boot/dts/mediatek/mt6358.dtsi | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/arch/arm64/boot/dts/mediatek/mt6358.dtsi b/arch/arm64/boot/dts/mediatek/mt6358.dtsi
index b605313bed99..186898f9384b 100644
--- a/arch/arm64/boot/dts/mediatek/mt6358.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt6358.dtsi
@@ -304,15 +304,8 @@ mt6358_vrf18_reg: ldo_vrf18 {
regulator-enable-ramp-delay = <120>;
};
- mt6358_vcn33_bt_reg: ldo_vcn33_bt {
- regulator-name = "vcn33_bt";
- regulator-min-microvolt = <3300000>;
- regulator-max-microvolt = <3500000>;
- regulator-enable-ramp-delay = <270>;
- };
-
- mt6358_vcn33_wifi_reg: ldo_vcn33_wifi {
- regulator-name = "vcn33_wifi";
+ mt6358_vcn33_reg: ldo_vcn33 {
+ regulator-name = "vcn33";
regulator-min-microvolt = <3300000>;
regulator-max-microvolt = <3500000>;
regulator-enable-ramp-delay = <270>;
--
2.41.0.162.gfafddb0af9-goog
Some of the regulators on the MT6358/MT6366 PMICs have just one linear
voltage range. These are the bulk regulators and VSRAM_* LDOs. Currently
they are modeled with one linear range, but also have their minimum,
maximum, and step voltage described.
Convert them to the linear voltage helpers. These helpers are a bit
simpler, and we can also drop the linear range definitions. Also reflow
the touched lines now that they are shorter.
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/regulator/mt6358-regulator.c | 121 +++++++++------------------
1 file changed, 40 insertions(+), 81 deletions(-)
diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
index 2851ef53ac63..31a16fb28ecd 100644
--- a/drivers/regulator/mt6358-regulator.c
+++ b/drivers/regulator/mt6358-regulator.c
@@ -37,7 +37,7 @@ struct mt6358_regulator_info {
#define to_regulator_info(x) container_of((x), struct mt6358_regulator_info, desc)
#define MT6358_BUCK(match, vreg, min, max, step, \
- volt_ranges, vosel_mask, _da_vsel_reg, _da_vsel_mask, \
+ vosel_mask, _da_vsel_reg, _da_vsel_mask, \
_modeset_reg, _modeset_shift) \
[MT6358_ID_##vreg] = { \
.desc = { \
@@ -48,8 +48,8 @@ struct mt6358_regulator_info {
.id = MT6358_ID_##vreg, \
.owner = THIS_MODULE, \
.n_voltages = ((max) - (min)) / (step) + 1, \
- .linear_ranges = volt_ranges, \
- .n_linear_ranges = ARRAY_SIZE(volt_ranges), \
+ .min_uV = (min), \
+ .uV_step = (step), \
.vsel_reg = MT6358_BUCK_##vreg##_ELR0, \
.vsel_mask = vosel_mask, \
.enable_reg = MT6358_BUCK_##vreg##_CON0, \
@@ -89,7 +89,7 @@ struct mt6358_regulator_info {
}
#define MT6358_LDO1(match, vreg, min, max, step, \
- volt_ranges, _da_vsel_reg, _da_vsel_mask, \
+ _da_vsel_reg, _da_vsel_mask, \
vosel, vosel_mask) \
[MT6358_ID_##vreg] = { \
.desc = { \
@@ -100,8 +100,8 @@ struct mt6358_regulator_info {
.id = MT6358_ID_##vreg, \
.owner = THIS_MODULE, \
.n_voltages = ((max) - (min)) / (step) + 1, \
- .linear_ranges = volt_ranges, \
- .n_linear_ranges = ARRAY_SIZE(volt_ranges), \
+ .min_uV = (min), \
+ .uV_step = (step), \
.vsel_reg = vosel, \
.vsel_mask = vosel_mask, \
.enable_reg = MT6358_LDO_##vreg##_CON0, \
@@ -133,7 +133,7 @@ struct mt6358_regulator_info {
}
#define MT6366_BUCK(match, vreg, min, max, step, \
- volt_ranges, vosel_mask, _da_vsel_reg, _da_vsel_mask, \
+ vosel_mask, _da_vsel_reg, _da_vsel_mask, \
_modeset_reg, _modeset_shift) \
[MT6366_ID_##vreg] = { \
.desc = { \
@@ -144,8 +144,8 @@ struct mt6358_regulator_info {
.id = MT6366_ID_##vreg, \
.owner = THIS_MODULE, \
.n_voltages = ((max) - (min)) / (step) + 1, \
- .linear_ranges = volt_ranges, \
- .n_linear_ranges = ARRAY_SIZE(volt_ranges), \
+ .min_uV = (min), \
+ .uV_step = (step), \
.vsel_reg = MT6358_BUCK_##vreg##_ELR0, \
.vsel_mask = vosel_mask, \
.enable_reg = MT6358_BUCK_##vreg##_CON0, \
@@ -185,7 +185,7 @@ struct mt6358_regulator_info {
}
#define MT6366_LDO1(match, vreg, min, max, step, \
- volt_ranges, _da_vsel_reg, _da_vsel_mask, \
+ _da_vsel_reg, _da_vsel_mask, \
vosel, vosel_mask) \
[MT6366_ID_##vreg] = { \
.desc = { \
@@ -196,8 +196,8 @@ struct mt6358_regulator_info {
.id = MT6366_ID_##vreg, \
.owner = THIS_MODULE, \
.n_voltages = ((max) - (min)) / (step) + 1, \
- .linear_ranges = volt_ranges, \
- .n_linear_ranges = ARRAY_SIZE(volt_ranges), \
+ .min_uV = (min), \
+ .uV_step = (step), \
.vsel_reg = vosel, \
.vsel_mask = vosel_mask, \
.enable_reg = MT6358_LDO_##vreg##_CON0, \
@@ -228,21 +228,6 @@ struct mt6358_regulator_info {
.qi = BIT(15), \
}
-static const struct linear_range buck_volt_range1[] = {
- REGULATOR_LINEAR_RANGE(500000, 0, 0x7f, 6250),
-};
-
-static const struct linear_range buck_volt_range2[] = {
- REGULATOR_LINEAR_RANGE(500000, 0, 0x7f, 12500),
-};
-
-static const struct linear_range buck_volt_range3[] = {
- REGULATOR_LINEAR_RANGE(500000, 0, 0x3f, 50000),
-};
-
-static const struct linear_range buck_volt_range4[] = {
- REGULATOR_LINEAR_RANGE(1000000, 0, 0x7f, 12500),
-};
static const unsigned int vdram2_voltages[] = {
600000, 1800000,
@@ -466,8 +451,8 @@ static unsigned int mt6358_regulator_get_mode(struct regulator_dev *rdev)
}
static const struct regulator_ops mt6358_volt_range_ops = {
- .list_voltage = regulator_list_voltage_linear_range,
- .map_voltage = regulator_map_voltage_linear_range,
+ .list_voltage = regulator_list_voltage_linear,
+ .map_voltage = regulator_map_voltage_linear,
.set_voltage_sel = regulator_set_voltage_sel_regmap,
.get_voltage_sel = mt6358_get_buck_voltage_sel,
.set_voltage_time_sel = regulator_set_voltage_time_sel,
@@ -502,32 +487,23 @@ static const struct regulator_ops mt6358_volt_fixed_ops = {
/* The array is indexed by id(MT6358_ID_XXX) */
static const struct mt6358_regulator_info mt6358_regulators[] = {
MT6358_BUCK("buck_vdram1", VDRAM1, 500000, 2087500, 12500,
- buck_volt_range2, 0x7f, MT6358_BUCK_VDRAM1_DBG0, 0x7f,
- MT6358_VDRAM1_ANA_CON0, 8),
+ 0x7f, MT6358_BUCK_VDRAM1_DBG0, 0x7f, MT6358_VDRAM1_ANA_CON0, 8),
MT6358_BUCK("buck_vcore", VCORE, 500000, 1293750, 6250,
- buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_DBG0, 0x7f,
- MT6358_VCORE_VGPU_ANA_CON0, 1),
+ 0x7f, MT6358_BUCK_VCORE_DBG0, 0x7f, MT6358_VCORE_VGPU_ANA_CON0, 1),
MT6358_BUCK("buck_vpa", VPA, 500000, 3650000, 50000,
- buck_volt_range3, 0x3f, MT6358_BUCK_VPA_DBG0, 0x3f,
- MT6358_VPA_ANA_CON0, 3),
+ 0x3f, MT6358_BUCK_VPA_DBG0, 0x3f, MT6358_VPA_ANA_CON0, 3),
MT6358_BUCK("buck_vproc11", VPROC11, 500000, 1293750, 6250,
- buck_volt_range1, 0x7f, MT6358_BUCK_VPROC11_DBG0, 0x7f,
- MT6358_VPROC_ANA_CON0, 1),
+ 0x7f, MT6358_BUCK_VPROC11_DBG0, 0x7f, MT6358_VPROC_ANA_CON0, 1),
MT6358_BUCK("buck_vproc12", VPROC12, 500000, 1293750, 6250,
- buck_volt_range1, 0x7f, MT6358_BUCK_VPROC12_DBG0, 0x7f,
- MT6358_VPROC_ANA_CON0, 2),
+ 0x7f, MT6358_BUCK_VPROC12_DBG0, 0x7f, MT6358_VPROC_ANA_CON0, 2),
MT6358_BUCK("buck_vgpu", VGPU, 500000, 1293750, 6250,
- buck_volt_range1, 0x7f, MT6358_BUCK_VGPU_ELR0, 0x7f,
- MT6358_VCORE_VGPU_ANA_CON0, 2),
+ 0x7f, MT6358_BUCK_VGPU_ELR0, 0x7f, MT6358_VCORE_VGPU_ANA_CON0, 2),
MT6358_BUCK("buck_vs2", VS2, 500000, 2087500, 12500,
- buck_volt_range2, 0x7f, MT6358_BUCK_VS2_DBG0, 0x7f,
- MT6358_VS2_ANA_CON0, 8),
+ 0x7f, MT6358_BUCK_VS2_DBG0, 0x7f, MT6358_VS2_ANA_CON0, 8),
MT6358_BUCK("buck_vmodem", VMODEM, 500000, 1293750, 6250,
- buck_volt_range1, 0x7f, MT6358_BUCK_VMODEM_DBG0, 0x7f,
- MT6358_VMODEM_ANA_CON0, 8),
+ 0x7f, MT6358_BUCK_VMODEM_DBG0, 0x7f, MT6358_VMODEM_ANA_CON0, 8),
MT6358_BUCK("buck_vs1", VS1, 1000000, 2587500, 12500,
- buck_volt_range4, 0x7f, MT6358_BUCK_VS1_DBG0, 0x7f,
- MT6358_VS1_ANA_CON0, 8),
+ 0x7f, MT6358_BUCK_VS1_DBG0, 0x7f, MT6358_VS1_ANA_CON0, 8),
MT6358_REG_FIXED("ldo_vrf12", VRF12,
MT6358_LDO_VRF12_CON0, 0, 1200000),
MT6358_REG_FIXED("ldo_vio18", VIO18,
@@ -577,48 +553,35 @@ static const struct mt6358_regulator_info mt6358_regulators[] = {
MT6358_LDO("ldo_vsim2", VSIM2, vsim_voltages, vsim_idx,
MT6358_LDO_VSIM2_CON0, 0, MT6358_VSIM2_ANA_CON0, 0xf00),
MT6358_LDO1("ldo_vsram_proc11", VSRAM_PROC11, 500000, 1293750, 6250,
- buck_volt_range1, MT6358_LDO_VSRAM_PROC11_DBG0, 0x7f00,
- MT6358_LDO_VSRAM_CON0, 0x7f),
+ MT6358_LDO_VSRAM_PROC11_DBG0, 0x7f00, MT6358_LDO_VSRAM_CON0, 0x7f),
MT6358_LDO1("ldo_vsram_others", VSRAM_OTHERS, 500000, 1293750, 6250,
- buck_volt_range1, MT6358_LDO_VSRAM_OTHERS_DBG0, 0x7f00,
- MT6358_LDO_VSRAM_CON2, 0x7f),
+ MT6358_LDO_VSRAM_OTHERS_DBG0, 0x7f00, MT6358_LDO_VSRAM_CON2, 0x7f),
MT6358_LDO1("ldo_vsram_gpu", VSRAM_GPU, 500000, 1293750, 6250,
- buck_volt_range1, MT6358_LDO_VSRAM_GPU_DBG0, 0x7f00,
- MT6358_LDO_VSRAM_CON3, 0x7f),
+ MT6358_LDO_VSRAM_GPU_DBG0, 0x7f00, MT6358_LDO_VSRAM_CON3, 0x7f),
MT6358_LDO1("ldo_vsram_proc12", VSRAM_PROC12, 500000, 1293750, 6250,
- buck_volt_range1, MT6358_LDO_VSRAM_PROC12_DBG0, 0x7f00,
- MT6358_LDO_VSRAM_CON1, 0x7f),
+ MT6358_LDO_VSRAM_PROC12_DBG0, 0x7f00, MT6358_LDO_VSRAM_CON1, 0x7f),
};
/* The array is indexed by id(MT6366_ID_XXX) */
static const struct mt6358_regulator_info mt6366_regulators[] = {
MT6366_BUCK("buck_vdram1", VDRAM1, 500000, 2087500, 12500,
- buck_volt_range2, 0x7f, MT6358_BUCK_VDRAM1_DBG0, 0x7f,
- MT6358_VDRAM1_ANA_CON0, 8),
+ 0x7f, MT6358_BUCK_VDRAM1_DBG0, 0x7f, MT6358_VDRAM1_ANA_CON0, 8),
MT6366_BUCK("buck_vcore", VCORE, 500000, 1293750, 6250,
- buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_DBG0, 0x7f,
- MT6358_VCORE_VGPU_ANA_CON0, 1),
+ 0x7f, MT6358_BUCK_VCORE_DBG0, 0x7f, MT6358_VCORE_VGPU_ANA_CON0, 1),
MT6366_BUCK("buck_vpa", VPA, 500000, 3650000, 50000,
- buck_volt_range3, 0x3f, MT6358_BUCK_VPA_DBG0, 0x3f,
- MT6358_VPA_ANA_CON0, 3),
+ 0x3f, MT6358_BUCK_VPA_DBG0, 0x3f, MT6358_VPA_ANA_CON0, 3),
MT6366_BUCK("buck_vproc11", VPROC11, 500000, 1293750, 6250,
- buck_volt_range1, 0x7f, MT6358_BUCK_VPROC11_DBG0, 0x7f,
- MT6358_VPROC_ANA_CON0, 1),
+ 0x7f, MT6358_BUCK_VPROC11_DBG0, 0x7f, MT6358_VPROC_ANA_CON0, 1),
MT6366_BUCK("buck_vproc12", VPROC12, 500000, 1293750, 6250,
- buck_volt_range1, 0x7f, MT6358_BUCK_VPROC12_DBG0, 0x7f,
- MT6358_VPROC_ANA_CON0, 2),
+ 0x7f, MT6358_BUCK_VPROC12_DBG0, 0x7f, MT6358_VPROC_ANA_CON0, 2),
MT6366_BUCK("buck_vgpu", VGPU, 500000, 1293750, 6250,
- buck_volt_range1, 0x7f, MT6358_BUCK_VGPU_ELR0, 0x7f,
- MT6358_VCORE_VGPU_ANA_CON0, 2),
+ 0x7f, MT6358_BUCK_VGPU_ELR0, 0x7f, MT6358_VCORE_VGPU_ANA_CON0, 2),
MT6366_BUCK("buck_vs2", VS2, 500000, 2087500, 12500,
- buck_volt_range2, 0x7f, MT6358_BUCK_VS2_DBG0, 0x7f,
- MT6358_VS2_ANA_CON0, 8),
+ 0x7f, MT6358_BUCK_VS2_DBG0, 0x7f, MT6358_VS2_ANA_CON0, 8),
MT6366_BUCK("buck_vmodem", VMODEM, 500000, 1293750, 6250,
- buck_volt_range1, 0x7f, MT6358_BUCK_VMODEM_DBG0, 0x7f,
- MT6358_VMODEM_ANA_CON0, 8),
+ 0x7f, MT6358_BUCK_VMODEM_DBG0, 0x7f, MT6358_VMODEM_ANA_CON0, 8),
MT6366_BUCK("buck_vs1", VS1, 1000000, 2587500, 12500,
- buck_volt_range4, 0x7f, MT6358_BUCK_VS1_DBG0, 0x7f,
- MT6358_VS1_ANA_CON0, 8),
+ 0x7f, MT6358_BUCK_VS1_DBG0, 0x7f, MT6358_VS1_ANA_CON0, 8),
MT6366_REG_FIXED("ldo_vrf12", VRF12,
MT6358_LDO_VRF12_CON0, 0, 1200000),
MT6366_REG_FIXED("ldo_vio18", VIO18,
@@ -657,17 +620,13 @@ static const struct mt6358_regulator_info mt6366_regulators[] = {
MT6366_LDO("ldo_vsim2", VSIM2, vsim_voltages, vsim_idx,
MT6358_LDO_VSIM2_CON0, 0, MT6358_VSIM2_ANA_CON0, 0xf00),
MT6366_LDO1("ldo_vsram_proc11", VSRAM_PROC11, 500000, 1293750, 6250,
- buck_volt_range1, MT6358_LDO_VSRAM_PROC11_DBG0, 0x7f00,
- MT6358_LDO_VSRAM_CON0, 0x7f),
+ MT6358_LDO_VSRAM_PROC11_DBG0, 0x7f00, MT6358_LDO_VSRAM_CON0, 0x7f),
MT6366_LDO1("ldo_vsram_others", VSRAM_OTHERS, 500000, 1293750, 6250,
- buck_volt_range1, MT6358_LDO_VSRAM_OTHERS_DBG0, 0x7f00,
- MT6358_LDO_VSRAM_CON2, 0x7f),
+ MT6358_LDO_VSRAM_OTHERS_DBG0, 0x7f00, MT6358_LDO_VSRAM_CON2, 0x7f),
MT6366_LDO1("ldo_vsram_gpu", VSRAM_GPU, 500000, 1293750, 6250,
- buck_volt_range1, MT6358_LDO_VSRAM_GPU_DBG0, 0x7f00,
- MT6358_LDO_VSRAM_CON3, 0x7f),
+ MT6358_LDO_VSRAM_GPU_DBG0, 0x7f00, MT6358_LDO_VSRAM_CON3, 0x7f),
MT6366_LDO1("ldo_vsram_proc12", VSRAM_PROC12, 500000, 1293750, 6250,
- buck_volt_range1, MT6358_LDO_VSRAM_PROC12_DBG0, 0x7f00,
- MT6358_LDO_VSRAM_CON1, 0x7f),
+ MT6358_LDO_VSRAM_PROC12_DBG0, 0x7f00, MT6358_LDO_VSRAM_CON1, 0x7f),
};
static int mt6358_sync_vcn33_setting(struct device *dev)
--
2.41.0.162.gfafddb0af9-goog
The *_SSHUB regulators are actually alternate configuration interfaces
for their non *_SSHUB counterparts. They are not separate regulator
outputs. These registers are intended for the companion processor to
use to configure the power rails while the main processor is sleeping.
They are not intended for the main operating system to use.
Since they are not real outputs they shouldn't be modeled separately.
Remove them. Luckily no device tree actually uses them.
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/regulator/mt6358-regulator.c | 14 --------------
include/linux/regulator/mt6358-regulator.h | 4 ----
2 files changed, 18 deletions(-)
diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
index faf6b0757019..946a251a8b3a 100644
--- a/drivers/regulator/mt6358-regulator.c
+++ b/drivers/regulator/mt6358-regulator.c
@@ -505,9 +505,6 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
MT6358_BUCK("buck_vcore", VCORE, 500000, 1293750, 6250,
buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_DBG0, 0x7f,
MT6358_VCORE_VGPU_ANA_CON0, 1),
- MT6358_BUCK("buck_vcore_sshub", VCORE_SSHUB, 500000, 1293750, 6250,
- buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_SSHUB_ELR0, 0x7f,
- MT6358_VCORE_VGPU_ANA_CON0, 1),
MT6358_BUCK("buck_vpa", VPA, 500000, 3650000, 50000,
buck_volt_range3, 0x3f, MT6358_BUCK_VPA_DBG0, 0x3f,
MT6358_VPA_ANA_CON0, 3),
@@ -583,10 +580,6 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
MT6358_LDO1("ldo_vsram_others", VSRAM_OTHERS, 500000, 1293750, 6250,
buck_volt_range1, MT6358_LDO_VSRAM_OTHERS_DBG0, 0x7f00,
MT6358_LDO_VSRAM_CON2, 0x7f),
- MT6358_LDO1("ldo_vsram_others_sshub", VSRAM_OTHERS_SSHUB, 500000,
- 1293750, 6250, buck_volt_range1,
- MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f,
- MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f),
MT6358_LDO1("ldo_vsram_gpu", VSRAM_GPU, 500000, 1293750, 6250,
buck_volt_range1, MT6358_LDO_VSRAM_GPU_DBG0, 0x7f00,
MT6358_LDO_VSRAM_CON3, 0x7f),
@@ -603,9 +596,6 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
MT6366_BUCK("buck_vcore", VCORE, 500000, 1293750, 6250,
buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_DBG0, 0x7f,
MT6358_VCORE_VGPU_ANA_CON0, 1),
- MT6366_BUCK("buck_vcore_sshub", VCORE_SSHUB, 500000, 1293750, 6250,
- buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_SSHUB_ELR0, 0x7f,
- MT6358_VCORE_VGPU_ANA_CON0, 1),
MT6366_BUCK("buck_vpa", VPA, 500000, 3650000, 50000,
buck_volt_range3, 0x3f, MT6358_BUCK_VPA_DBG0, 0x3f,
MT6358_VPA_ANA_CON0, 3),
@@ -670,10 +660,6 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
MT6366_LDO1("ldo_vsram_others", VSRAM_OTHERS, 500000, 1293750, 6250,
buck_volt_range1, MT6358_LDO_VSRAM_OTHERS_DBG0, 0x7f00,
MT6358_LDO_VSRAM_CON2, 0x7f),
- MT6366_LDO1("ldo_vsram_others_sshub", VSRAM_OTHERS_SSHUB, 500000,
- 1293750, 6250, buck_volt_range1,
- MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f,
- MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f),
MT6366_LDO1("ldo_vsram_gpu", VSRAM_GPU, 500000, 1293750, 6250,
buck_volt_range1, MT6358_LDO_VSRAM_GPU_DBG0, 0x7f00,
MT6358_LDO_VSRAM_CON3, 0x7f),
diff --git a/include/linux/regulator/mt6358-regulator.h b/include/linux/regulator/mt6358-regulator.h
index a4307cd9edd6..c71a6a9fce7a 100644
--- a/include/linux/regulator/mt6358-regulator.h
+++ b/include/linux/regulator/mt6358-regulator.h
@@ -47,8 +47,6 @@ enum {
MT6358_ID_VLDO28,
MT6358_ID_VAUD28,
MT6358_ID_VSIM2,
- MT6358_ID_VCORE_SSHUB,
- MT6358_ID_VSRAM_OTHERS_SSHUB,
MT6358_ID_RG_MAX,
};
@@ -88,8 +86,6 @@ enum {
MT6366_ID_VMC,
MT6366_ID_VAUD28,
MT6366_ID_VSIM2,
- MT6366_ID_VCORE_SSHUB,
- MT6366_ID_VSRAM_OTHERS_SSHUB,
MT6366_ID_RG_MAX,
};
--
2.41.0.162.gfafddb0af9-goog
Il 09/06/23 10:30, Chen-Yu Tsai ha scritto:
> The *_SSHUB regulators are actually alternate configuration interfaces
> for their non *_SSHUB counterparts. They are not separate regulator
> outputs. These registers are intended for the companion processor to
> use to configure the power rails while the main processor is sleeping.
> They are not intended for the main operating system to use.
>
> Since they are not real outputs they shouldn't be modeled separately.
> Remove them. Luckily no device tree actually uses them.
>
I'm not sure that MT6358/6366 are used only on Chromebook SoCs, and that this SSHUB
mechanism (companion processor) is the same across all firmwares.
I'd like someone from MediaTek to confirm that this is valid for both Chromebook
and Smartphone firmwares.
Regards,
Angelo
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> ---
> drivers/regulator/mt6358-regulator.c | 14 --------------
> include/linux/regulator/mt6358-regulator.h | 4 ----
> 2 files changed, 18 deletions(-)
>
> diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> index faf6b0757019..946a251a8b3a 100644
> --- a/drivers/regulator/mt6358-regulator.c
> +++ b/drivers/regulator/mt6358-regulator.c
> @@ -505,9 +505,6 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> MT6358_BUCK("buck_vcore", VCORE, 500000, 1293750, 6250,
> buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_DBG0, 0x7f,
> MT6358_VCORE_VGPU_ANA_CON0, 1),
> - MT6358_BUCK("buck_vcore_sshub", VCORE_SSHUB, 500000, 1293750, 6250,
> - buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_SSHUB_ELR0, 0x7f,
> - MT6358_VCORE_VGPU_ANA_CON0, 1),
> MT6358_BUCK("buck_vpa", VPA, 500000, 3650000, 50000,
> buck_volt_range3, 0x3f, MT6358_BUCK_VPA_DBG0, 0x3f,
> MT6358_VPA_ANA_CON0, 3),
> @@ -583,10 +580,6 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> MT6358_LDO1("ldo_vsram_others", VSRAM_OTHERS, 500000, 1293750, 6250,
> buck_volt_range1, MT6358_LDO_VSRAM_OTHERS_DBG0, 0x7f00,
> MT6358_LDO_VSRAM_CON2, 0x7f),
> - MT6358_LDO1("ldo_vsram_others_sshub", VSRAM_OTHERS_SSHUB, 500000,
> - 1293750, 6250, buck_volt_range1,
> - MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f,
> - MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f),
> MT6358_LDO1("ldo_vsram_gpu", VSRAM_GPU, 500000, 1293750, 6250,
> buck_volt_range1, MT6358_LDO_VSRAM_GPU_DBG0, 0x7f00,
> MT6358_LDO_VSRAM_CON3, 0x7f),
> @@ -603,9 +596,6 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> MT6366_BUCK("buck_vcore", VCORE, 500000, 1293750, 6250,
> buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_DBG0, 0x7f,
> MT6358_VCORE_VGPU_ANA_CON0, 1),
> - MT6366_BUCK("buck_vcore_sshub", VCORE_SSHUB, 500000, 1293750, 6250,
> - buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_SSHUB_ELR0, 0x7f,
> - MT6358_VCORE_VGPU_ANA_CON0, 1),
> MT6366_BUCK("buck_vpa", VPA, 500000, 3650000, 50000,
> buck_volt_range3, 0x3f, MT6358_BUCK_VPA_DBG0, 0x3f,
> MT6358_VPA_ANA_CON0, 3),
> @@ -670,10 +660,6 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> MT6366_LDO1("ldo_vsram_others", VSRAM_OTHERS, 500000, 1293750, 6250,
> buck_volt_range1, MT6358_LDO_VSRAM_OTHERS_DBG0, 0x7f00,
> MT6358_LDO_VSRAM_CON2, 0x7f),
> - MT6366_LDO1("ldo_vsram_others_sshub", VSRAM_OTHERS_SSHUB, 500000,
> - 1293750, 6250, buck_volt_range1,
> - MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f,
> - MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f),
> MT6366_LDO1("ldo_vsram_gpu", VSRAM_GPU, 500000, 1293750, 6250,
> buck_volt_range1, MT6358_LDO_VSRAM_GPU_DBG0, 0x7f00,
> MT6358_LDO_VSRAM_CON3, 0x7f),
> diff --git a/include/linux/regulator/mt6358-regulator.h b/include/linux/regulator/mt6358-regulator.h
> index a4307cd9edd6..c71a6a9fce7a 100644
> --- a/include/linux/regulator/mt6358-regulator.h
> +++ b/include/linux/regulator/mt6358-regulator.h
> @@ -47,8 +47,6 @@ enum {
> MT6358_ID_VLDO28,
> MT6358_ID_VAUD28,
> MT6358_ID_VSIM2,
> - MT6358_ID_VCORE_SSHUB,
> - MT6358_ID_VSRAM_OTHERS_SSHUB,
> MT6358_ID_RG_MAX,
> };
>
> @@ -88,8 +86,6 @@ enum {
> MT6366_ID_VMC,
> MT6366_ID_VAUD28,
> MT6366_ID_VSIM2,
> - MT6366_ID_VCORE_SSHUB,
> - MT6366_ID_VSRAM_OTHERS_SSHUB,
> MT6366_ID_RG_MAX,
> };
>
Il 09/06/23 10:30, Chen-Yu Tsai ha scritto:
> The VCN33_BT and VCN33_WIFI regulators are actually the same regulator,
> having the same voltage setting and output pin. There are simply two
> enable bits that are ORed together to enable the regulator.
>
> Having two regulators representing the same output pin is misleading
> from a design matching standpoint, and also error-prone in driver
> implementations. If consumers try to set different voltages on either
> regulator, the one set later would override the one set before. There
> are ways around this, such as chaining them together and having the
> downstream one act as a switch. But given there's only one output pin,
> such a workaround doesn't match reality.
>
> Remove the VCN33_WIFI regulator. During the probe phase, have the driver
> sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix
> so that the regulator name matches the pin name in the datasheet.
>
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> ---
> drivers/regulator/mt6358-regulator.c | 65 +++++++++++++++++-----
> include/linux/regulator/mt6358-regulator.h | 6 +-
> 2 files changed, 52 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> index c9e16bd092f6..faf6b0757019 100644
> --- a/drivers/regulator/mt6358-regulator.c
> +++ b/drivers/regulator/mt6358-regulator.c
> @@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = {
> 2800000, 2900000, 3000000,
> };
>
> -static const unsigned int vcn33_bt_wifi_voltages[] = {
> +static const unsigned int vcn33_voltages[] = {
> 3300000, 3400000, 3500000,
> };
>
> @@ -321,7 +321,7 @@ static const u32 vcama_idx[] = {
> 0, 7, 9, 10, 11, 12,
> };
>
> -static const u32 vcn33_bt_wifi_idx[] = {
> +static const u32 vcn33_idx[] = {
> 1, 2, 3,
> };
>
> @@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
> MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> - MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> - MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> + MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> MT6358_LDO("ldo_vcama2", VCAMA2, vcama_voltages, vcama_idx,
> MT6358_LDO_VCAMA2_CON0, 0, MT6358_VCAMA2_ANA_CON0, 0xf00),
> MT6358_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
> @@ -662,12 +658,8 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> MT6358_LDO_VMCH_CON0, 0, MT6358_VMCH_ANA_CON0, 0x700),
> MT6366_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> - MT6366_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> - MT6366_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> + MT6366_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> MT6366_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
> MT6358_LDO_VMC_CON0, 0, MT6358_VMC_ANA_CON0, 0xf00),
> MT6366_LDO("ldo_vsim2", VSIM2, vsim_voltages, vsim_idx,
> @@ -690,13 +682,56 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> MT6358_LDO_VSRAM_CON1, 0x7f),
> };
>
> +static int mt6358_sync_vcn33_setting(struct device *dev)
> +{
> + struct mt6397_chip *mt6397 = dev_get_drvdata(dev->parent);
> + unsigned int val;
> + int ret;
> +
> + /*
> + * VCN33_WIFI and VCN33_BT are two separate enable bits for the same
> + * regulator. They share the same voltage setting and output pin.
> + * Instead of having two potentially conflicting regulators, just have
> + * one VCN33 regulator. Sync the two enable bits and only use one in
> + * the regulator device.
> + */
> + ret = regmap_read(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, &val);
> + if (ret) {
> + dev_err(dev, "Failed to read VCN33_WIFI setting\n");
> + return ret;
> + }
> +
> + if (!(val & BIT(0)))
> + return 0;
> +
> + /* Sync VCN33_WIFI enable status to VCN33_BT */
> + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_0, BIT(0), BIT(0));
> + if (ret) {
> + dev_err(dev, "Failed to sync VCN33_WIFI setting to VCN33_BT\n");
> + return ret;
> + }
> +
> + /* Disable VCN33_WIFI */
> + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, BIT(0), 0);
> + if (ret) {
> + dev_err(dev, "Failed to disable VCN33_BT\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int mt6358_regulator_probe(struct platform_device *pdev)
> {
> struct mt6397_chip *mt6397 = dev_get_drvdata(pdev->dev.parent);
> struct regulator_config config = {};
> struct regulator_dev *rdev;
> struct mt6358_regulator_info *mt6358_info;
> - int i, max_regulator;
> + int i, max_regulator, ret;
> +
> + ret = mt6358_sync_vcn33_setting(&pdev->dev);
> + if (ret)
> + return ret;
I'd put this after the chip_id check, and I would also add a safety check for
that...
switch (mt6397->chip_id) {
case MT6366_CHIP_ID:
max_regulator = MT6366_MAX_REGULATOR;
mt6358_info = mt6366_regulators;
break;
case MT6358_CHIP_ID:
max_regulator = MT6358_MAX_REGULATOR;
mt6358_info = mt6358_regulators;
break;
default:
return -EINVAL;
}
ret = mt6358_sync_vcn33_setting(....)
...but I agree with your point here about this being a strange design and
also with your way of fixing the driver.
Regards,
Angelo
Il 09/06/23 10:30, Chen-Yu Tsai ha scritto:
> Some of the LDO regulators in the MT6358/MT6366 have sparsely populated
> voltage tables, supported by custom get/set operators. While it works,
> it requires more code and an extra field to store the lookup table.
> These LDOs also have fine voltage calibration settings that can slightly
> boost the output voltage from 0 mV to 100 mV, in 10 mV increments.
>
> These combined could be modeled as a pickable set of linear ranges. The
> coarse voltage setting is modeled as the range selector, while each
> range has 11 selectors, starting from the range's base voltage, up to
> +100 mV, in 10mV increments.
>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Il 09/06/23 10:30, Chen-Yu Tsai ha scritto:
> The "fixed" LDO regulators found on the MT6358 and MT6366 PMICs have
> either no voltage selection register, or only one valid setting.
> However these do have a fine voltage calibration setting that can
> slightly boost the output voltage from 0 mV to 100 mV, in 10 mV
> increments.
>
> Add support for this by changing these into linear range regulators.
>
> Signed-off-by: Chen-Yu Tsai <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
On 09/06/2023 10:29, Chen-Yu Tsai wrote:
> The *_sshub regulators are actually alternate configuration interfaces
> for their non *_sshub counterparts. They are not separate regulator
> outputs. These registers are intended for the companion processor to
> use to configure the power rails while the main processor is sleeping.
> They are not intended for the main operating system to use.
>
> Since they are not real outputs they shouldn't be modeled separately.
> Remove them. Luckily no device tree actually uses them.
>
> Signed-off-by: Chen-Yu Tsai <[email protected]>
Reviewed-by: Matthias Brugger <[email protected]>
> ---
> .../bindings/regulator/mt6358-regulator.txt | 22 +++++--------------
> 1 file changed, 5 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/regulator/mt6358-regulator.txt b/Documentation/devicetree/bindings/regulator/mt6358-regulator.txt
> index b22b956d1cbe..b6384306db5c 100644
> --- a/Documentation/devicetree/bindings/regulator/mt6358-regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/mt6358-regulator.txt
> @@ -8,14 +8,14 @@ Documentation/devicetree/bindings/regulator/regulator.txt.
>
> The valid names for regulators are::
> BUCK:
> - buck_vdram1, buck_vcore, buck_vcore_sshub, buck_vpa, buck_vproc11,
> - buck_vproc12, buck_vgpu, buck_vs2, buck_vmodem, buck_vs1
> + buck_vdram1, buck_vcore, buck_vpa, buck_vproc11, buck_vproc12, buck_vgpu,
> + buck_vs2, buck_vmodem, buck_vs1
> LDO:
> ldo_vdram2, ldo_vsim1, ldo_vibr, ldo_vrf12, ldo_vio18, ldo_vusb, ldo_vcamio,
> ldo_vcamd, ldo_vcn18, ldo_vfe28, ldo_vsram_proc11, ldo_vcn28, ldo_vsram_others,
> - ldo_vsram_others_sshub, ldo_vsram_gpu, ldo_vxo22, ldo_vefuse, ldo_vaux18,
> - ldo_vmch, ldo_vbif28, ldo_vsram_proc12, ldo_vcama1, ldo_vemc, ldo_vio28, ldo_va12,
> - ldo_vrf18, ldo_vcn33, ldo_vcama2, ldo_vmc, ldo_vldo28, ldo_vaud28, ldo_vsim2
> + ldo_vsram_gpu, ldo_vxo22, ldo_vefuse, ldo_vaux18, ldo_vmch, ldo_vbif28,
> + ldo_vsram_proc12, ldo_vcama1, ldo_vemc, ldo_vio28, ldo_va12, ldo_vrf18,
> + ldo_vcn33, ldo_vcama2, ldo_vmc, ldo_vldo28, ldo_vaud28, ldo_vsim2
>
> Example:
>
> @@ -346,17 +346,5 @@ Example:
> regulator-max-microvolt = <3100000>;
> regulator-enable-ramp-delay = <540>;
> };
> -
> - mt6358_vcore_sshub_reg: buck_vcore_sshub {
> - regulator-name = "vcore_sshub";
> - regulator-min-microvolt = <500000>;
> - regulator-max-microvolt = <1293750>;
> - };
> -
> - mt6358_vsram_others_sshub_reg: ldo_vsram_others_sshub {
> - regulator-name = "vsram_others_sshub";
> - regulator-min-microvolt = <500000>;
> - regulator-max-microvolt = <1293750>;
> - };
> };
> };
On 09/06/2023 10:29, Chen-Yu Tsai wrote:
> The ldo_vcn33_bt and ldo_vcn33_wifi regulators are actually the same
> regulator, having the same voltage setting and output pin. There are
> simply two enable bits that are ORed together to enable the regulator.
>
> Having two regulators representing the same output pin is misleading
> from a design matching standpoint, and also error-prone in driver
> implementations.
>
> Merge the two as ldo_vcn33. Neither vcn33 regulators are referenced
> in upstream device trees. As far as hardware designs go, none of the
> Chromebooks using MT8183 w/ MT6358 use this output.
>
> Signed-off-by: Chen-Yu Tsai <[email protected]>
Reviewed-by: Matthias Brugger <[email protected]>
> ---
> .../bindings/regulator/mt6358-regulator.txt | 14 +++-----------
> 1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/regulator/mt6358-regulator.txt b/Documentation/devicetree/bindings/regulator/mt6358-regulator.txt
> index 7034cdca54e0..b22b956d1cbe 100644
> --- a/Documentation/devicetree/bindings/regulator/mt6358-regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/mt6358-regulator.txt
> @@ -15,8 +15,7 @@ LDO:
> ldo_vcamd, ldo_vcn18, ldo_vfe28, ldo_vsram_proc11, ldo_vcn28, ldo_vsram_others,
> ldo_vsram_others_sshub, ldo_vsram_gpu, ldo_vxo22, ldo_vefuse, ldo_vaux18,
> ldo_vmch, ldo_vbif28, ldo_vsram_proc12, ldo_vcama1, ldo_vemc, ldo_vio28, ldo_va12,
> - ldo_vrf18, ldo_vcn33_bt, ldo_vcn33_wifi, ldo_vcama2, ldo_vmc, ldo_vldo28, ldo_vaud28,
> - ldo_vsim2
> + ldo_vrf18, ldo_vcn33, ldo_vcama2, ldo_vmc, ldo_vldo28, ldo_vaud28, ldo_vsim2
>
> Example:
>
> @@ -305,15 +304,8 @@ Example:
> regulator-enable-ramp-delay = <120>;
> };
>
> - mt6358_vcn33_bt_reg: ldo_vcn33_bt {
> - regulator-name = "vcn33_bt";
> - regulator-min-microvolt = <3300000>;
> - regulator-max-microvolt = <3500000>;
> - regulator-enable-ramp-delay = <270>;
> - };
> -
> - mt6358_vcn33_wifi_reg: ldo_vcn33_wifi {
> - regulator-name = "vcn33_wifi";
> + mt6358_vcn33_reg: ldo_vcn33 {
> + regulator-name = "vcn33";
> regulator-min-microvolt = <3300000>;
> regulator-max-microvolt = <3500000>;
> regulator-enable-ramp-delay = <270>;
On 09/06/2023 10:30, Chen-Yu Tsai wrote:
> The *_SSHUB regulators are actually alternate configuration interfaces
> for their non *_SSHUB counterparts. They are not separate regulator
> outputs. These registers are intended for the companion processor to
> use to configure the power rails while the main processor is sleeping.
> They are not intended for the main operating system to use.
>
> Since they are not real outputs they shouldn't be modeled separately.
> Remove them. Luckily no device tree actually uses them.
>
> Signed-off-by: Chen-Yu Tsai <[email protected]>
Reviewed-by: Matthias Brugger <[email protected]>
> ---
> drivers/regulator/mt6358-regulator.c | 14 --------------
> include/linux/regulator/mt6358-regulator.h | 4 ----
> 2 files changed, 18 deletions(-)
>
> diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> index faf6b0757019..946a251a8b3a 100644
> --- a/drivers/regulator/mt6358-regulator.c
> +++ b/drivers/regulator/mt6358-regulator.c
> @@ -505,9 +505,6 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> MT6358_BUCK("buck_vcore", VCORE, 500000, 1293750, 6250,
> buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_DBG0, 0x7f,
> MT6358_VCORE_VGPU_ANA_CON0, 1),
> - MT6358_BUCK("buck_vcore_sshub", VCORE_SSHUB, 500000, 1293750, 6250,
> - buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_SSHUB_ELR0, 0x7f,
> - MT6358_VCORE_VGPU_ANA_CON0, 1),
> MT6358_BUCK("buck_vpa", VPA, 500000, 3650000, 50000,
> buck_volt_range3, 0x3f, MT6358_BUCK_VPA_DBG0, 0x3f,
> MT6358_VPA_ANA_CON0, 3),
> @@ -583,10 +580,6 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> MT6358_LDO1("ldo_vsram_others", VSRAM_OTHERS, 500000, 1293750, 6250,
> buck_volt_range1, MT6358_LDO_VSRAM_OTHERS_DBG0, 0x7f00,
> MT6358_LDO_VSRAM_CON2, 0x7f),
> - MT6358_LDO1("ldo_vsram_others_sshub", VSRAM_OTHERS_SSHUB, 500000,
> - 1293750, 6250, buck_volt_range1,
> - MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f,
> - MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f),
> MT6358_LDO1("ldo_vsram_gpu", VSRAM_GPU, 500000, 1293750, 6250,
> buck_volt_range1, MT6358_LDO_VSRAM_GPU_DBG0, 0x7f00,
> MT6358_LDO_VSRAM_CON3, 0x7f),
> @@ -603,9 +596,6 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> MT6366_BUCK("buck_vcore", VCORE, 500000, 1293750, 6250,
> buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_DBG0, 0x7f,
> MT6358_VCORE_VGPU_ANA_CON0, 1),
> - MT6366_BUCK("buck_vcore_sshub", VCORE_SSHUB, 500000, 1293750, 6250,
> - buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_SSHUB_ELR0, 0x7f,
> - MT6358_VCORE_VGPU_ANA_CON0, 1),
> MT6366_BUCK("buck_vpa", VPA, 500000, 3650000, 50000,
> buck_volt_range3, 0x3f, MT6358_BUCK_VPA_DBG0, 0x3f,
> MT6358_VPA_ANA_CON0, 3),
> @@ -670,10 +660,6 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> MT6366_LDO1("ldo_vsram_others", VSRAM_OTHERS, 500000, 1293750, 6250,
> buck_volt_range1, MT6358_LDO_VSRAM_OTHERS_DBG0, 0x7f00,
> MT6358_LDO_VSRAM_CON2, 0x7f),
> - MT6366_LDO1("ldo_vsram_others_sshub", VSRAM_OTHERS_SSHUB, 500000,
> - 1293750, 6250, buck_volt_range1,
> - MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f,
> - MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f),
> MT6366_LDO1("ldo_vsram_gpu", VSRAM_GPU, 500000, 1293750, 6250,
> buck_volt_range1, MT6358_LDO_VSRAM_GPU_DBG0, 0x7f00,
> MT6358_LDO_VSRAM_CON3, 0x7f),
> diff --git a/include/linux/regulator/mt6358-regulator.h b/include/linux/regulator/mt6358-regulator.h
> index a4307cd9edd6..c71a6a9fce7a 100644
> --- a/include/linux/regulator/mt6358-regulator.h
> +++ b/include/linux/regulator/mt6358-regulator.h
> @@ -47,8 +47,6 @@ enum {
> MT6358_ID_VLDO28,
> MT6358_ID_VAUD28,
> MT6358_ID_VSIM2,
> - MT6358_ID_VCORE_SSHUB,
> - MT6358_ID_VSRAM_OTHERS_SSHUB,
> MT6358_ID_RG_MAX,
> };
>
> @@ -88,8 +86,6 @@ enum {
> MT6366_ID_VMC,
> MT6366_ID_VAUD28,
> MT6366_ID_VSIM2,
> - MT6366_ID_VCORE_SSHUB,
> - MT6366_ID_VSRAM_OTHERS_SSHUB,
> MT6366_ID_RG_MAX,
> };
>
On 09/06/2023 10:29, Chen-Yu Tsai wrote:
> The ldo_vcn33_bt and ldo_vcn33_wifi regulators are actually the same
> regulator, having the same voltage setting and output pin. There are
> simply two enable bits that are ORed together to enable the regulator.
>
> Having two regulators representing the same output pin is misleading
> from a design matching standpoint, and also error-prone in driver
> implementations.
>
> Merge the two as ldo_vcn33. Neither vcn33 regulators are referenced
> in upstream device trees. As far as hardware designs go, none of the
> Chromebooks using MT8183 w/ MT6358 use this output.
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Fri, Jun 09, 2023 at 04:30:00PM +0800, Chen-Yu Tsai wrote:
> The VCN33_BT and VCN33_WIFI regulators are actually the same regulator,
> having the same voltage setting and output pin. There are simply two
> enable bits that are ORed together to enable the regulator.
>
> Having two regulators representing the same output pin is misleading
> from a design matching standpoint, and also error-prone in driver
> implementations. If consumers try to set different voltages on either
> regulator, the one set later would override the one set before. There
> are ways around this, such as chaining them together and having the
> downstream one act as a switch. But given there's only one output pin,
> such a workaround doesn't match reality.
>
> Remove the VCN33_WIFI regulator. During the probe phase, have the driver
> sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix
> so that the regulator name matches the pin name in the datasheet.
>
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> ---
> drivers/regulator/mt6358-regulator.c | 65 +++++++++++++++++-----
> include/linux/regulator/mt6358-regulator.h | 6 +-
> 2 files changed, 52 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> index c9e16bd092f6..faf6b0757019 100644
> --- a/drivers/regulator/mt6358-regulator.c
> +++ b/drivers/regulator/mt6358-regulator.c
> @@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = {
> 2800000, 2900000, 3000000,
> };
>
> -static const unsigned int vcn33_bt_wifi_voltages[] = {
> +static const unsigned int vcn33_voltages[] = {
> 3300000, 3400000, 3500000,
> };
>
> @@ -321,7 +321,7 @@ static const u32 vcama_idx[] = {
> 0, 7, 9, 10, 11, 12,
> };
>
> -static const u32 vcn33_bt_wifi_idx[] = {
> +static const u32 vcn33_idx[] = {
> 1, 2, 3,
> };
>
> @@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
> MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> - MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> - MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> + MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
Excuse me if I am being daft here, but could you explain how this change
is compatible with existing devicetrees?
Thanks,
Conor.
On 09/06/2023 10:29, Chen-Yu Tsai wrote:
> The *_sshub regulators are actually alternate configuration interfaces
> for their non *_sshub counterparts. They are not separate regulator
> outputs. These registers are intended for the companion processor to
> use to configure the power rails while the main processor is sleeping.
> They are not intended for the main operating system to use.
>
> Since they are not real outputs they shouldn't be modeled separately.
> Remove them. Luckily no device tree actually uses them.
>
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Fri, Jun 09, 2023 at 04:56:05PM +0100, Conor Dooley wrote:
> On Fri, Jun 09, 2023 at 04:30:00PM +0800, Chen-Yu Tsai wrote:
> > The VCN33_BT and VCN33_WIFI regulators are actually the same regulator,
> > having the same voltage setting and output pin. There are simply two
> > enable bits that are ORed together to enable the regulator.
> >
> > Having two regulators representing the same output pin is misleading
> > from a design matching standpoint, and also error-prone in driver
> > implementations. If consumers try to set different voltages on either
> > regulator, the one set later would override the one set before. There
> > are ways around this, such as chaining them together and having the
> > downstream one act as a switch. But given there's only one output pin,
> > such a workaround doesn't match reality.
> >
> > Remove the VCN33_WIFI regulator. During the probe phase, have the driver
> > sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix
> > so that the regulator name matches the pin name in the datasheet.
> >
> > Signed-off-by: Chen-Yu Tsai <[email protected]>
> > ---
> > drivers/regulator/mt6358-regulator.c | 65 +++++++++++++++++-----
> > include/linux/regulator/mt6358-regulator.h | 6 +-
> > 2 files changed, 52 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> > index c9e16bd092f6..faf6b0757019 100644
> > --- a/drivers/regulator/mt6358-regulator.c
> > +++ b/drivers/regulator/mt6358-regulator.c
> > @@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = {
> > 2800000, 2900000, 3000000,
> > };
> >
> > -static const unsigned int vcn33_bt_wifi_voltages[] = {
> > +static const unsigned int vcn33_voltages[] = {
> > 3300000, 3400000, 3500000,
> > };
> >
> > @@ -321,7 +321,7 @@ static const u32 vcama_idx[] = {
> > 0, 7, 9, 10, 11, 12,
> > };
> >
> > -static const u32 vcn33_bt_wifi_idx[] = {
> > +static const u32 vcn33_idx[] = {
> > 1, 2, 3,
> > };
> >
> > @@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> > MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
> > MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> > MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> > - MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > - MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > + MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> > + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
>
> Excuse me if I am being daft here, but could you explain how this change
> is compatible with existing devicetrees?
Ah, I see in the binding commit there's a "Luckily no device tree actually
uses them." Does that just cover the kernel, or does it consider other
operating systems/bootloaders?
Cheers,
Conor.
On Fri, Jun 9, 2023 at 4:58 PM AngeloGioacchino Del Regno
<[email protected]> wrote:
>
> Il 09/06/23 10:30, Chen-Yu Tsai ha scritto:
> > The VCN33_BT and VCN33_WIFI regulators are actually the same regulator,
> > having the same voltage setting and output pin. There are simply two
> > enable bits that are ORed together to enable the regulator.
> >
> > Having two regulators representing the same output pin is misleading
> > from a design matching standpoint, and also error-prone in driver
> > implementations. If consumers try to set different voltages on either
> > regulator, the one set later would override the one set before. There
> > are ways around this, such as chaining them together and having the
> > downstream one act as a switch. But given there's only one output pin,
> > such a workaround doesn't match reality.
> >
> > Remove the VCN33_WIFI regulator. During the probe phase, have the driver
> > sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix
> > so that the regulator name matches the pin name in the datasheet.
> >
> > Signed-off-by: Chen-Yu Tsai <[email protected]>
> > ---
> > drivers/regulator/mt6358-regulator.c | 65 +++++++++++++++++-----
> > include/linux/regulator/mt6358-regulator.h | 6 +-
> > 2 files changed, 52 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> > index c9e16bd092f6..faf6b0757019 100644
> > --- a/drivers/regulator/mt6358-regulator.c
> > +++ b/drivers/regulator/mt6358-regulator.c
> > @@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = {
> > 2800000, 2900000, 3000000,
> > };
> >
> > -static const unsigned int vcn33_bt_wifi_voltages[] = {
> > +static const unsigned int vcn33_voltages[] = {
> > 3300000, 3400000, 3500000,
> > };
> >
> > @@ -321,7 +321,7 @@ static const u32 vcama_idx[] = {
> > 0, 7, 9, 10, 11, 12,
> > };
> >
> > -static const u32 vcn33_bt_wifi_idx[] = {
> > +static const u32 vcn33_idx[] = {
> > 1, 2, 3,
> > };
> >
> > @@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> > MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
> > MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> > MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> > - MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > - MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > + MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> > + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> > MT6358_LDO("ldo_vcama2", VCAMA2, vcama_voltages, vcama_idx,
> > MT6358_LDO_VCAMA2_CON0, 0, MT6358_VCAMA2_ANA_CON0, 0xf00),
> > MT6358_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
> > @@ -662,12 +658,8 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> > MT6358_LDO_VMCH_CON0, 0, MT6358_VMCH_ANA_CON0, 0x700),
> > MT6366_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> > MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> > - MT6366_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > - MT6366_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > + MT6366_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> > + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> > MT6366_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
> > MT6358_LDO_VMC_CON0, 0, MT6358_VMC_ANA_CON0, 0xf00),
> > MT6366_LDO("ldo_vsim2", VSIM2, vsim_voltages, vsim_idx,
> > @@ -690,13 +682,56 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> > MT6358_LDO_VSRAM_CON1, 0x7f),
> > };
> >
> > +static int mt6358_sync_vcn33_setting(struct device *dev)
> > +{
> > + struct mt6397_chip *mt6397 = dev_get_drvdata(dev->parent);
> > + unsigned int val;
> > + int ret;
> > +
> > + /*
> > + * VCN33_WIFI and VCN33_BT are two separate enable bits for the same
> > + * regulator. They share the same voltage setting and output pin.
> > + * Instead of having two potentially conflicting regulators, just have
> > + * one VCN33 regulator. Sync the two enable bits and only use one in
> > + * the regulator device.
> > + */
> > + ret = regmap_read(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, &val);
> > + if (ret) {
> > + dev_err(dev, "Failed to read VCN33_WIFI setting\n");
> > + return ret;
> > + }
> > +
> > + if (!(val & BIT(0)))
> > + return 0;
> > +
> > + /* Sync VCN33_WIFI enable status to VCN33_BT */
> > + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_0, BIT(0), BIT(0));
> > + if (ret) {
> > + dev_err(dev, "Failed to sync VCN33_WIFI setting to VCN33_BT\n");
> > + return ret;
> > + }
> > +
> > + /* Disable VCN33_WIFI */
> > + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, BIT(0), 0);
> > + if (ret) {
> > + dev_err(dev, "Failed to disable VCN33_BT\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int mt6358_regulator_probe(struct platform_device *pdev)
> > {
> > struct mt6397_chip *mt6397 = dev_get_drvdata(pdev->dev.parent);
> > struct regulator_config config = {};
> > struct regulator_dev *rdev;
> > struct mt6358_regulator_info *mt6358_info;
> > - int i, max_regulator;
> > + int i, max_regulator, ret;
> > +
> > + ret = mt6358_sync_vcn33_setting(&pdev->dev);
> > + if (ret)
> > + return ret;
>
> I'd put this after the chip_id check, and I would also add a safety check for
> that...
>
> switch (mt6397->chip_id) {
> case MT6366_CHIP_ID:
> max_regulator = MT6366_MAX_REGULATOR;
> mt6358_info = mt6366_regulators;
> break;
> case MT6358_CHIP_ID:
> max_regulator = MT6358_MAX_REGULATOR;
> mt6358_info = mt6358_regulators;
> break;
> default:
> return -EINVAL;
> }
>
> ret = mt6358_sync_vcn33_setting(....)
Sounds good. We wouldn't want to be poking random bits in some other PMIC.
> ...but I agree with your point here about this being a strange design and
> also with your way of fixing the driver.
What I heard was that they support separate Bluetooth and WiFi drivers that
don't have a common reference counting framework for their regulator
supplies using this scheme. Maybe they are doing the power sequencing in
some small firmware.
ChenYu
On Sat, Jun 10, 2023 at 11:28 PM Conor Dooley <[email protected]> wrote:
>
> On Fri, Jun 09, 2023 at 04:56:05PM +0100, Conor Dooley wrote:
> > On Fri, Jun 09, 2023 at 04:30:00PM +0800, Chen-Yu Tsai wrote:
> > > The VCN33_BT and VCN33_WIFI regulators are actually the same regulator,
> > > having the same voltage setting and output pin. There are simply two
> > > enable bits that are ORed together to enable the regulator.
> > >
> > > Having two regulators representing the same output pin is misleading
> > > from a design matching standpoint, and also error-prone in driver
> > > implementations. If consumers try to set different voltages on either
> > > regulator, the one set later would override the one set before. There
> > > are ways around this, such as chaining them together and having the
> > > downstream one act as a switch. But given there's only one output pin,
> > > such a workaround doesn't match reality.
> > >
> > > Remove the VCN33_WIFI regulator. During the probe phase, have the driver
> > > sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix
> > > so that the regulator name matches the pin name in the datasheet.
> > >
> > > Signed-off-by: Chen-Yu Tsai <[email protected]>
> > > ---
> > > drivers/regulator/mt6358-regulator.c | 65 +++++++++++++++++-----
> > > include/linux/regulator/mt6358-regulator.h | 6 +-
> > > 2 files changed, 52 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> > > index c9e16bd092f6..faf6b0757019 100644
> > > --- a/drivers/regulator/mt6358-regulator.c
> > > +++ b/drivers/regulator/mt6358-regulator.c
> > > @@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = {
> > > 2800000, 2900000, 3000000,
> > > };
> > >
> > > -static const unsigned int vcn33_bt_wifi_voltages[] = {
> > > +static const unsigned int vcn33_voltages[] = {
> > > 3300000, 3400000, 3500000,
> > > };
> > >
> > > @@ -321,7 +321,7 @@ static const u32 vcama_idx[] = {
> > > 0, 7, 9, 10, 11, 12,
> > > };
> > >
> > > -static const u32 vcn33_bt_wifi_idx[] = {
> > > +static const u32 vcn33_idx[] = {
> > > 1, 2, 3,
> > > };
> > >
> > > @@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> > > MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
> > > MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> > > MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> > > - MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> > > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> > > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > > - MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> > > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> > > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > > + MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> > > + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> >
> > Excuse me if I am being daft here, but could you explain how this change
> > is compatible with existing devicetrees?
>
> Ah, I see in the binding commit there's a "Luckily no device tree actually
> uses them." Does that just cover the kernel, or does it consider other
> operating systems/bootloaders?
That comment covers the upstream kernel and the downstream ChromeOS kernel
specifically. The bootloader that ChromeOS uses (coreboot) doesn't use
device trees. I don't know what MediaTek uses for their phones though.
AFAIK MediaTek only supports the Linux kernel, be it for Android or ChromeOS.
There's not a large community around it, unlike some of the other ARM SoCs.
I did find an old v4.4 Android kernel [1] for the MediaTek Helio P60
(MT6771) that is also paired with MT6358. There are no device tree
references to the VCN33 regulator either. Only the definition exists
in the mt6358.dtsi file, much like what we have upstream.
As far as the regulator driver goes, if it can't find a matching regulator
node, it's the same as if the node doesn't exist, and therefore the given
constraints are not ingested. If no constraints are ingested that can
turn it on, and no consumer references to enable it either, we can say
that the regulator is effectively unused.
ChenYu
[1] https://github.com/nokia-dev/android_kernel_nokia_mt6771
On Fri, Jun 9, 2023 at 5:03 PM AngeloGioacchino Del Regno
<[email protected]> wrote:
>
> Il 09/06/23 10:30, Chen-Yu Tsai ha scritto:
> > The *_SSHUB regulators are actually alternate configuration interfaces
> > for their non *_SSHUB counterparts. They are not separate regulator
> > outputs. These registers are intended for the companion processor to
> > use to configure the power rails while the main processor is sleeping.
> > They are not intended for the main operating system to use.
> >
> > Since they are not real outputs they shouldn't be modeled separately.
> > Remove them. Luckily no device tree actually uses them.
> >
>
> I'm not sure that MT6358/6366 are used only on Chromebook SoCs, and that this SSHUB
> mechanism (companion processor) is the same across all firmwares.
AFAICT from Internet sources there's also the MT6771 and MT6781, which
are used on some phones.
But what part are you concerned about? The upstream regulator driver does
not actually have any code to switch to/from normal operation and SSHUB
mode.
In a downstream kernel I found that the SSHUB mode is only used if SCP is
doing DVFS [1]. In that same kernel, the regulator driver [2] doesn't even
list the *_SSHUB versions. So if SCP DVFS is used, the regulator driver
has no idea what's going on, and can't interfere either, which I think is
actually a good thing. Only one side should have complete control of one
output.
ChenYu
[1] https://github.com/nokia-dev/android_kernel_nokia_mt6771/blob/android-9.0/drivers/misc/mediatek/scp/mt6771/scp_dvfs.c
[2] https://github.com/nokia-dev/android_kernel_nokia_mt6771/blob/android-9.0/drivers/misc/mediatek/pmic/mt6358/v1/regulator_codegen.c
> I'd like someone from MediaTek to confirm that this is valid for both Chromebook
> and Smartphone firmwares.
>
> Regards,
> Angelo
>
> > Signed-off-by: Chen-Yu Tsai <[email protected]>
> > ---
> > drivers/regulator/mt6358-regulator.c | 14 --------------
> > include/linux/regulator/mt6358-regulator.h | 4 ----
> > 2 files changed, 18 deletions(-)
> >
> > diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> > index faf6b0757019..946a251a8b3a 100644
> > --- a/drivers/regulator/mt6358-regulator.c
> > +++ b/drivers/regulator/mt6358-regulator.c
> > @@ -505,9 +505,6 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> > MT6358_BUCK("buck_vcore", VCORE, 500000, 1293750, 6250,
> > buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_DBG0, 0x7f,
> > MT6358_VCORE_VGPU_ANA_CON0, 1),
> > - MT6358_BUCK("buck_vcore_sshub", VCORE_SSHUB, 500000, 1293750, 6250,
> > - buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_SSHUB_ELR0, 0x7f,
> > - MT6358_VCORE_VGPU_ANA_CON0, 1),
> > MT6358_BUCK("buck_vpa", VPA, 500000, 3650000, 50000,
> > buck_volt_range3, 0x3f, MT6358_BUCK_VPA_DBG0, 0x3f,
> > MT6358_VPA_ANA_CON0, 3),
> > @@ -583,10 +580,6 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> > MT6358_LDO1("ldo_vsram_others", VSRAM_OTHERS, 500000, 1293750, 6250,
> > buck_volt_range1, MT6358_LDO_VSRAM_OTHERS_DBG0, 0x7f00,
> > MT6358_LDO_VSRAM_CON2, 0x7f),
> > - MT6358_LDO1("ldo_vsram_others_sshub", VSRAM_OTHERS_SSHUB, 500000,
> > - 1293750, 6250, buck_volt_range1,
> > - MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f,
> > - MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f),
> > MT6358_LDO1("ldo_vsram_gpu", VSRAM_GPU, 500000, 1293750, 6250,
> > buck_volt_range1, MT6358_LDO_VSRAM_GPU_DBG0, 0x7f00,
> > MT6358_LDO_VSRAM_CON3, 0x7f),
> > @@ -603,9 +596,6 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> > MT6366_BUCK("buck_vcore", VCORE, 500000, 1293750, 6250,
> > buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_DBG0, 0x7f,
> > MT6358_VCORE_VGPU_ANA_CON0, 1),
> > - MT6366_BUCK("buck_vcore_sshub", VCORE_SSHUB, 500000, 1293750, 6250,
> > - buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_SSHUB_ELR0, 0x7f,
> > - MT6358_VCORE_VGPU_ANA_CON0, 1),
> > MT6366_BUCK("buck_vpa", VPA, 500000, 3650000, 50000,
> > buck_volt_range3, 0x3f, MT6358_BUCK_VPA_DBG0, 0x3f,
> > MT6358_VPA_ANA_CON0, 3),
> > @@ -670,10 +660,6 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> > MT6366_LDO1("ldo_vsram_others", VSRAM_OTHERS, 500000, 1293750, 6250,
> > buck_volt_range1, MT6358_LDO_VSRAM_OTHERS_DBG0, 0x7f00,
> > MT6358_LDO_VSRAM_CON2, 0x7f),
> > - MT6366_LDO1("ldo_vsram_others_sshub", VSRAM_OTHERS_SSHUB, 500000,
> > - 1293750, 6250, buck_volt_range1,
> > - MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f,
> > - MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f),
> > MT6366_LDO1("ldo_vsram_gpu", VSRAM_GPU, 500000, 1293750, 6250,
> > buck_volt_range1, MT6358_LDO_VSRAM_GPU_DBG0, 0x7f00,
> > MT6358_LDO_VSRAM_CON3, 0x7f),
> > diff --git a/include/linux/regulator/mt6358-regulator.h b/include/linux/regulator/mt6358-regulator.h
> > index a4307cd9edd6..c71a6a9fce7a 100644
> > --- a/include/linux/regulator/mt6358-regulator.h
> > +++ b/include/linux/regulator/mt6358-regulator.h
> > @@ -47,8 +47,6 @@ enum {
> > MT6358_ID_VLDO28,
> > MT6358_ID_VAUD28,
> > MT6358_ID_VSIM2,
> > - MT6358_ID_VCORE_SSHUB,
> > - MT6358_ID_VSRAM_OTHERS_SSHUB,
> > MT6358_ID_RG_MAX,
> > };
> >
> > @@ -88,8 +86,6 @@ enum {
> > MT6366_ID_VMC,
> > MT6366_ID_VAUD28,
> > MT6366_ID_VSIM2,
> > - MT6366_ID_VCORE_SSHUB,
> > - MT6366_ID_VSRAM_OTHERS_SSHUB,
> > MT6366_ID_RG_MAX,
> > };
> >
>
On Fri, Jun 9, 2023 at 4:30 PM Chen-Yu Tsai <[email protected]> wrote:
>
> The VCN33_BT and VCN33_WIFI regulators are actually the same regulator,
> having the same voltage setting and output pin. There are simply two
> enable bits that are ORed together to enable the regulator.
>
> Having two regulators representing the same output pin is misleading
> from a design matching standpoint, and also error-prone in driver
> implementations. If consumers try to set different voltages on either
> regulator, the one set later would override the one set before. There
> are ways around this, such as chaining them together and having the
> downstream one act as a switch. But given there's only one output pin,
> such a workaround doesn't match reality.
>
> Remove the VCN33_WIFI regulator. During the probe phase, have the driver
> sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix
> so that the regulator name matches the pin name in the datasheet.
>
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> ---
> drivers/regulator/mt6358-regulator.c | 65 +++++++++++++++++-----
> include/linux/regulator/mt6358-regulator.h | 6 +-
> 2 files changed, 52 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> index c9e16bd092f6..faf6b0757019 100644
> --- a/drivers/regulator/mt6358-regulator.c
> +++ b/drivers/regulator/mt6358-regulator.c
> @@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = {
> 2800000, 2900000, 3000000,
> };
>
> -static const unsigned int vcn33_bt_wifi_voltages[] = {
> +static const unsigned int vcn33_voltages[] = {
> 3300000, 3400000, 3500000,
> };
>
> @@ -321,7 +321,7 @@ static const u32 vcama_idx[] = {
> 0, 7, 9, 10, 11, 12,
> };
>
> -static const u32 vcn33_bt_wifi_idx[] = {
> +static const u32 vcn33_idx[] = {
> 1, 2, 3,
> };
>
> @@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
> MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> - MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> - MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> + MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> MT6358_LDO("ldo_vcama2", VCAMA2, vcama_voltages, vcama_idx,
> MT6358_LDO_VCAMA2_CON0, 0, MT6358_VCAMA2_ANA_CON0, 0xf00),
> MT6358_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
> @@ -662,12 +658,8 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> MT6358_LDO_VMCH_CON0, 0, MT6358_VMCH_ANA_CON0, 0x700),
> MT6366_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> - MT6366_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> - MT6366_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> - 0, MT6358_VCN33_ANA_CON0, 0x300),
> + MT6366_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> MT6366_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
> MT6358_LDO_VMC_CON0, 0, MT6358_VMC_ANA_CON0, 0xf00),
> MT6366_LDO("ldo_vsim2", VSIM2, vsim_voltages, vsim_idx,
> @@ -690,13 +682,56 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> MT6358_LDO_VSRAM_CON1, 0x7f),
> };
>
> +static int mt6358_sync_vcn33_setting(struct device *dev)
> +{
> + struct mt6397_chip *mt6397 = dev_get_drvdata(dev->parent);
> + unsigned int val;
> + int ret;
> +
> + /*
> + * VCN33_WIFI and VCN33_BT are two separate enable bits for the same
> + * regulator. They share the same voltage setting and output pin.
> + * Instead of having two potentially conflicting regulators, just have
> + * one VCN33 regulator. Sync the two enable bits and only use one in
> + * the regulator device.
> + */
> + ret = regmap_read(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, &val);
> + if (ret) {
> + dev_err(dev, "Failed to read VCN33_WIFI setting\n");
> + return ret;
> + }
> +
> + if (!(val & BIT(0)))
> + return 0;
> +
> + /* Sync VCN33_WIFI enable status to VCN33_BT */
> + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_0, BIT(0), BIT(0));
> + if (ret) {
> + dev_err(dev, "Failed to sync VCN33_WIFI setting to VCN33_BT\n");
> + return ret;
> + }
> +
> + /* Disable VCN33_WIFI */
> + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, BIT(0), 0);
> + if (ret) {
> + dev_err(dev, "Failed to disable VCN33_BT\n");
I think it should be "VCN33_WIFI" in the error message?
Regards,
Fei
On Mon, Jun 12, 2023 at 12:19:01PM +0800, Chen-Yu Tsai wrote:
> On Sat, Jun 10, 2023 at 11:28 PM Conor Dooley <[email protected]> wrote:
> > Ah, I see in the binding commit there's a "Luckily no device tree actually
> > uses them." Does that just cover the kernel, or does it consider other
> > operating systems/bootloaders?
>
> That comment covers the upstream kernel and the downstream ChromeOS kernel
> specifically. The bootloader that ChromeOS uses (coreboot) doesn't use
> device trees. I don't know what MediaTek uses for their phones though.
>
> AFAIK MediaTek only supports the Linux kernel, be it for Android or ChromeOS.
> There's not a large community around it, unlike some of the other ARM SoCs.
>
> I did find an old v4.4 Android kernel [1] for the MediaTek Helio P60
> (MT6771) that is also paired with MT6358. There are no device tree
> references to the VCN33 regulator either. Only the definition exists
> in the mt6358.dtsi file, much like what we have upstream.
>
> As far as the regulator driver goes, if it can't find a matching regulator
> node, it's the same as if the node doesn't exist, and therefore the given
> constraints are not ingested. If no constraints are ingested that can
> turn it on, and no consumer references to enable it either, we can say
> that the regulator is effectively unused.
Okay, that sounds reasonable. Seems like you've done your research,
so thanks for that!
On Mon, Jun 12, 2023 at 12:45:21PM +0800, Chen-Yu Tsai wrote:
> list the *_SSHUB versions. So if SCP DVFS is used, the regulator driver
> has no idea what's going on, and can't interfere either, which I think is
> actually a good thing. Only one side should have complete control of one
> output.
Very much so - there's a SCMI regulator interface for communicating with
SCPs where that's required (eg, for things like SD cards).
Il 12/06/23 06:45, Chen-Yu Tsai ha scritto:
> On Fri, Jun 9, 2023 at 5:03 PM AngeloGioacchino Del Regno
> <[email protected]> wrote:
>>
>> Il 09/06/23 10:30, Chen-Yu Tsai ha scritto:
>>> The *_SSHUB regulators are actually alternate configuration interfaces
>>> for their non *_SSHUB counterparts. They are not separate regulator
>>> outputs. These registers are intended for the companion processor to
>>> use to configure the power rails while the main processor is sleeping.
>>> They are not intended for the main operating system to use.
>>>
>>> Since they are not real outputs they shouldn't be modeled separately.
>>> Remove them. Luckily no device tree actually uses them.
>>>
>>
>> I'm not sure that MT6358/6366 are used only on Chromebook SoCs, and that this SSHUB
>> mechanism (companion processor) is the same across all firmwares.
>
> AFAICT from Internet sources there's also the MT6771 and MT6781, which
> are used on some phones.
>
> But what part are you concerned about? The upstream regulator driver does
> not actually have any code to switch to/from normal operation and SSHUB
> mode.
>
> In a downstream kernel I found that the SSHUB mode is only used if SCP is
> doing DVFS [1]. In that same kernel, the regulator driver [2] doesn't even
> list the *_SSHUB versions. So if SCP DVFS is used, the regulator driver
> has no idea what's going on, and can't interfere either, which I think is
> actually a good thing. Only one side should have complete control of one
> output.
>
Ok, I'm sold! :-P
Jokes apart, thanks for clarifying. At this point, I agree with you in that this
is safe to do, so:
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
P.S.: Sorry for the late reply and thank you for the links to that old
downstream kernel.
Cheers,
Angelo
> ChenYu
>
> [1] https://github.com/nokia-dev/android_kernel_nokia_mt6771/blob/android-9.0/drivers/misc/mediatek/scp/mt6771/scp_dvfs.c
> [2] https://github.com/nokia-dev/android_kernel_nokia_mt6771/blob/android-9.0/drivers/misc/mediatek/pmic/mt6358/v1/regulator_codegen.c
>
>> I'd like someone from MediaTek to confirm that this is valid for both Chromebook
>> and Smartphone firmwares.
>>
>> Regards,
>> Angelo
>>
>>> Signed-off-by: Chen-Yu Tsai <[email protected]>
>>> ---
>>> drivers/regulator/mt6358-regulator.c | 14 --------------
>>> include/linux/regulator/mt6358-regulator.h | 4 ----
>>> 2 files changed, 18 deletions(-)
>>>
>>> diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
>>> index faf6b0757019..946a251a8b3a 100644
>>> --- a/drivers/regulator/mt6358-regulator.c
>>> +++ b/drivers/regulator/mt6358-regulator.c
>>> @@ -505,9 +505,6 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
>>> MT6358_BUCK("buck_vcore", VCORE, 500000, 1293750, 6250,
>>> buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_DBG0, 0x7f,
>>> MT6358_VCORE_VGPU_ANA_CON0, 1),
>>> - MT6358_BUCK("buck_vcore_sshub", VCORE_SSHUB, 500000, 1293750, 6250,
>>> - buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_SSHUB_ELR0, 0x7f,
>>> - MT6358_VCORE_VGPU_ANA_CON0, 1),
>>> MT6358_BUCK("buck_vpa", VPA, 500000, 3650000, 50000,
>>> buck_volt_range3, 0x3f, MT6358_BUCK_VPA_DBG0, 0x3f,
>>> MT6358_VPA_ANA_CON0, 3),
>>> @@ -583,10 +580,6 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
>>> MT6358_LDO1("ldo_vsram_others", VSRAM_OTHERS, 500000, 1293750, 6250,
>>> buck_volt_range1, MT6358_LDO_VSRAM_OTHERS_DBG0, 0x7f00,
>>> MT6358_LDO_VSRAM_CON2, 0x7f),
>>> - MT6358_LDO1("ldo_vsram_others_sshub", VSRAM_OTHERS_SSHUB, 500000,
>>> - 1293750, 6250, buck_volt_range1,
>>> - MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f,
>>> - MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f),
>>> MT6358_LDO1("ldo_vsram_gpu", VSRAM_GPU, 500000, 1293750, 6250,
>>> buck_volt_range1, MT6358_LDO_VSRAM_GPU_DBG0, 0x7f00,
>>> MT6358_LDO_VSRAM_CON3, 0x7f),
>>> @@ -603,9 +596,6 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
>>> MT6366_BUCK("buck_vcore", VCORE, 500000, 1293750, 6250,
>>> buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_DBG0, 0x7f,
>>> MT6358_VCORE_VGPU_ANA_CON0, 1),
>>> - MT6366_BUCK("buck_vcore_sshub", VCORE_SSHUB, 500000, 1293750, 6250,
>>> - buck_volt_range1, 0x7f, MT6358_BUCK_VCORE_SSHUB_ELR0, 0x7f,
>>> - MT6358_VCORE_VGPU_ANA_CON0, 1),
>>> MT6366_BUCK("buck_vpa", VPA, 500000, 3650000, 50000,
>>> buck_volt_range3, 0x3f, MT6358_BUCK_VPA_DBG0, 0x3f,
>>> MT6358_VPA_ANA_CON0, 3),
>>> @@ -670,10 +660,6 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
>>> MT6366_LDO1("ldo_vsram_others", VSRAM_OTHERS, 500000, 1293750, 6250,
>>> buck_volt_range1, MT6358_LDO_VSRAM_OTHERS_DBG0, 0x7f00,
>>> MT6358_LDO_VSRAM_CON2, 0x7f),
>>> - MT6366_LDO1("ldo_vsram_others_sshub", VSRAM_OTHERS_SSHUB, 500000,
>>> - 1293750, 6250, buck_volt_range1,
>>> - MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f,
>>> - MT6358_LDO_VSRAM_OTHERS_SSHUB_CON1, 0x7f),
>>> MT6366_LDO1("ldo_vsram_gpu", VSRAM_GPU, 500000, 1293750, 6250,
>>> buck_volt_range1, MT6358_LDO_VSRAM_GPU_DBG0, 0x7f00,
>>> MT6358_LDO_VSRAM_CON3, 0x7f),
>>> diff --git a/include/linux/regulator/mt6358-regulator.h b/include/linux/regulator/mt6358-regulator.h
>>> index a4307cd9edd6..c71a6a9fce7a 100644
>>> --- a/include/linux/regulator/mt6358-regulator.h
>>> +++ b/include/linux/regulator/mt6358-regulator.h
>>> @@ -47,8 +47,6 @@ enum {
>>> MT6358_ID_VLDO28,
>>> MT6358_ID_VAUD28,
>>> MT6358_ID_VSIM2,
>>> - MT6358_ID_VCORE_SSHUB,
>>> - MT6358_ID_VSRAM_OTHERS_SSHUB,
>>> MT6358_ID_RG_MAX,
>>> };
>>>
>>> @@ -88,8 +86,6 @@ enum {
>>> MT6366_ID_VMC,
>>> MT6366_ID_VAUD28,
>>> MT6366_ID_VSIM2,
>>> - MT6366_ID_VCORE_SSHUB,
>>> - MT6366_ID_VSRAM_OTHERS_SSHUB,
>>> MT6366_ID_RG_MAX,
>>> };
>>>
>>
On Fri, Jun 09, 2023 at 04:30:04PM +0800, Chen-Yu Tsai wrote:
> The "fixed" LDO regulators found on the MT6358 and MT6366 PMICs have
> either no voltage selection register, or only one valid setting.
> However these do have a fine voltage calibration setting that can
> slightly boost the output voltage from 0 mV to 100 mV, in 10 mV
> increments.
This and the followup patch break the build on both arm64 and x86_64:
/build/stage/linux/drivers/regulator/mt6358-regulator.c:127:29: error: ‘MT6358_VFE28_ANA_CON0’ undeclared here (not in a function); did you mean ‘MT6358_VIO28_ANA_CON0’?
127 | .vsel_reg = MT6358_##vreg##_ANA_CON0, \
| ^~~~~~~
/build/stage/linux/drivers/regulator/mt6358-regulator.c:525:9: note: in expansion of macro ‘MT6358_REG_FIXED’
525 | MT6358_REG_FIXED("ldo_vfe28", VFE28, MT6358_LDO_VFE28_CON0, 0, 2800000),
| ^~~~~~~~~~~~~~~~
/build/stage/linux/drivers/regulator/mt6358-regulator.c:127:29: error: ‘MT6358_VCN28_ANA_CON0’ undeclared here (not in a function); did you mean ‘MT6358_VCN18_ANA_CON0’?
127 | .vsel_reg = MT6358_##vreg##_ANA_CON0, \
| ^~~~~~~
/build/stage/linux/drivers/regulator/mt6358-regulator.c:526:9: note: in expansion of macro ‘MT6358_REG_FIXED’
526 | MT6358_REG_FIXED("ldo_vcn28", VCN28, MT6358_LDO_VCN28_CON0, 0, 2800000),
| ^~~~~~~~~~~~~~~~
/build/stage/linux/drivers/regulator/mt6358-regulator.c:127:29: error: ‘MT6358_VXO22_ANA_CON0’ undeclared here (not in a function); did you mean ‘MT6358_VIO28_ANA_CON0’?
127 | .vsel_reg = MT6358_##vreg##_ANA_CON0, \
| ^~~~~~~
/build/stage/linux/drivers/regulator/mt6358-regulator.c:527:9: note: in expansion of macro ‘MT6358_REG_FIXED’
527 | MT6358_REG_FIXED("ldo_vxo22", VXO22, MT6358_LDO_VXO22_CON0, 0, 2200000),
| ^~~~~~~~~~~~~~~~
/build/stage/linux/drivers/regulator/mt6358-regulator.c:127:29: error: ‘MT6358_VAUX18_ANA_CON0’ undeclared here (not in a function); did you mean ‘MT6358_VRF18_ANA_CON0’?
127 | .vsel_reg = MT6358_##vreg##_ANA_CON0, \
| ^~~~~~~
/build/stage/linux/drivers/regulator/mt6358-regulator.c:528:9: note: in expansion of macro ‘MT6358_REG_FIXED’
528 | MT6358_REG_FIXED("ldo_vaux18", VAUX18,
| ^~~~~~~~~~~~~~~~
/build/stage/linux/drivers/regulator/mt6358-regulator.c:127:29: error: ‘MT6358_VBIF28_ANA_CON0’ undeclared here (not in a function); did you mean ‘MT6358_VIO28_ANA_CON0’?
127 | .vsel_reg = MT6358_##vreg##_ANA_CON0, \
| ^~~~~~~
/build/stage/linux/drivers/regulator/mt6358-regulator.c:530:9: note: in expansion of macro ‘MT6358_REG_FIXED’
530 | MT6358_REG_FIXED("ldo_vbif28", VBIF28,
| ^~~~~~~~~~~~~~~~
/build/stage/linux/drivers/regulator/mt6358-regulator.c:127:29: error: ‘MT6358_VAUD28_ANA_CON0’ undeclared here (not in a function); did you mean ‘MT6358_VA12_ANA_CON0’?
127 | .vsel_reg = MT6358_##vreg##_ANA_CON0, \
| ^~~~~~~
/build/stage/linux/drivers/regulator/mt6358-regulator.c:535:9: note: in expansion of macro ‘MT6358_REG_FIXED’
535 | MT6358_REG_FIXED("ldo_vaud28", VAUD28,
| ^~~~~~~~~~~~~~~~
On Fri, 09 Jun 2023 16:29:57 +0800, Chen-Yu Tsai wrote:
> This series is a cleanup and improvement of the MT6358 regulator driver.
> Various discrepancies were found while preparing to upstream MT8186
> device trees, which utilize the MT6366 PMIC, that is also covered by
> this driver.
>
> Patches 1~8 should go through the regulator tree, and patch 9 through
> the soc tree. This series (patches 7 and 8) depends on "regulator: Use
> bitfield values for range selectors" [1] I sent out earlier.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
Thanks!
[1/9] regulator: dt-bindings: mt6358: Merge ldo_vcn33_* regulators
commit: a74d4c577c60b27fc57ea734ef8275921ae8dcb2
[2/9] regulator: dt-bindings: mt6358: Drop *_sshub regulators
commit: 82f305b18eb0505444eab8ac86bfa134b67cb38e
[3/9] regulator: mt6358: Merge VCN33_* regulators
commit: 65bae54e08c109ddbbf121bb00058cf3b3fb7b8e
[4/9] regulator: mt6358: Drop *_SSHUB regulators
commit: 04ba665248ed91576d326041108e5fc2ec2254eb
[5/9] regulator: mt6358: Const-ify mt6358_regulator_info data structures
commit: 1ff35e66cae53f7090a671afddaee45d4ccd9396
[6/9] regulator: mt6358: Use linear voltage helpers for single range regulators
commit: ea861df772fd8cca715d43f62fe13c09c975f7a2
[7/9] regulator: mt6358: Add output voltage fine tuning to fixed regulators
(no commit info)
[8/9] regulator: mt6358: Add output voltage fine tuning to variable LDOs
(no commit info)
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
On Thu, Jun 15, 2023 at 12:15 AM Mark Brown <[email protected]> wrote:
>
> On Fri, Jun 09, 2023 at 04:30:04PM +0800, Chen-Yu Tsai wrote:
> > The "fixed" LDO regulators found on the MT6358 and MT6366 PMICs have
> > either no voltage selection register, or only one valid setting.
> > However these do have a fine voltage calibration setting that can
> > slightly boost the output voltage from 0 mV to 100 mV, in 10 mV
> > increments.
>
> This and the followup patch break the build on both arm64 and x86_64:
>
> /build/stage/linux/drivers/regulator/mt6358-regulator.c:127:29: error: ‘MT6358_VFE28_ANA_CON0’ undeclared here (not in a function); did you mean ‘MT6358_VIO28_ANA_CON0’?
> 127 | .vsel_reg = MT6358_##vreg##_ANA_CON0, \
> | ^~~~~~~
Argh, I sequenced the patches in my tree incorrectly. I see you already
merged the first six patches. I'll send a new version including a header
change that this patch needs, and other fixups that reviewers suggested.
ChenYu
> /build/stage/linux/drivers/regulator/mt6358-regulator.c:525:9: note: in expansion of macro ‘MT6358_REG_FIXED’
> 525 | MT6358_REG_FIXED("ldo_vfe28", VFE28, MT6358_LDO_VFE28_CON0, 0, 2800000),
> | ^~~~~~~~~~~~~~~~
> /build/stage/linux/drivers/regulator/mt6358-regulator.c:127:29: error: ‘MT6358_VCN28_ANA_CON0’ undeclared here (not in a function); did you mean ‘MT6358_VCN18_ANA_CON0’?
> 127 | .vsel_reg = MT6358_##vreg##_ANA_CON0, \
> | ^~~~~~~
> /build/stage/linux/drivers/regulator/mt6358-regulator.c:526:9: note: in expansion of macro ‘MT6358_REG_FIXED’
> 526 | MT6358_REG_FIXED("ldo_vcn28", VCN28, MT6358_LDO_VCN28_CON0, 0, 2800000),
> | ^~~~~~~~~~~~~~~~
> /build/stage/linux/drivers/regulator/mt6358-regulator.c:127:29: error: ‘MT6358_VXO22_ANA_CON0’ undeclared here (not in a function); did you mean ‘MT6358_VIO28_ANA_CON0’?
> 127 | .vsel_reg = MT6358_##vreg##_ANA_CON0, \
> | ^~~~~~~
> /build/stage/linux/drivers/regulator/mt6358-regulator.c:527:9: note: in expansion of macro ‘MT6358_REG_FIXED’
> 527 | MT6358_REG_FIXED("ldo_vxo22", VXO22, MT6358_LDO_VXO22_CON0, 0, 2200000),
> | ^~~~~~~~~~~~~~~~
> /build/stage/linux/drivers/regulator/mt6358-regulator.c:127:29: error: ‘MT6358_VAUX18_ANA_CON0’ undeclared here (not in a function); did you mean ‘MT6358_VRF18_ANA_CON0’?
> 127 | .vsel_reg = MT6358_##vreg##_ANA_CON0, \
> | ^~~~~~~
> /build/stage/linux/drivers/regulator/mt6358-regulator.c:528:9: note: in expansion of macro ‘MT6358_REG_FIXED’
> 528 | MT6358_REG_FIXED("ldo_vaux18", VAUX18,
> | ^~~~~~~~~~~~~~~~
> /build/stage/linux/drivers/regulator/mt6358-regulator.c:127:29: error: ‘MT6358_VBIF28_ANA_CON0’ undeclared here (not in a function); did you mean ‘MT6358_VIO28_ANA_CON0’?
> 127 | .vsel_reg = MT6358_##vreg##_ANA_CON0, \
> | ^~~~~~~
> /build/stage/linux/drivers/regulator/mt6358-regulator.c:530:9: note: in expansion of macro ‘MT6358_REG_FIXED’
> 530 | MT6358_REG_FIXED("ldo_vbif28", VBIF28,
> | ^~~~~~~~~~~~~~~~
> /build/stage/linux/drivers/regulator/mt6358-regulator.c:127:29: error: ‘MT6358_VAUD28_ANA_CON0’ undeclared here (not in a function); did you mean ‘MT6358_VA12_ANA_CON0’?
> 127 | .vsel_reg = MT6358_##vreg##_ANA_CON0, \
> | ^~~~~~~
> /build/stage/linux/drivers/regulator/mt6358-regulator.c:535:9: note: in expansion of macro ‘MT6358_REG_FIXED’
> 535 | MT6358_REG_FIXED("ldo_vaud28", VAUD28,
> | ^~~~~~~~~~~~~~~~
On Mon, Jun 12, 2023 at 6:57 PM Fei Shao <[email protected]> wrote:
>
> On Fri, Jun 9, 2023 at 4:30 PM Chen-Yu Tsai <[email protected]> wrote:
> >
> > The VCN33_BT and VCN33_WIFI regulators are actually the same regulator,
> > having the same voltage setting and output pin. There are simply two
> > enable bits that are ORed together to enable the regulator.
> >
> > Having two regulators representing the same output pin is misleading
> > from a design matching standpoint, and also error-prone in driver
> > implementations. If consumers try to set different voltages on either
> > regulator, the one set later would override the one set before. There
> > are ways around this, such as chaining them together and having the
> > downstream one act as a switch. But given there's only one output pin,
> > such a workaround doesn't match reality.
> >
> > Remove the VCN33_WIFI regulator. During the probe phase, have the driver
> > sync the enable status of VCN33_WIFI to VCN33_BT. Also drop the suffix
> > so that the regulator name matches the pin name in the datasheet.
> >
> > Signed-off-by: Chen-Yu Tsai <[email protected]>
> > ---
> > drivers/regulator/mt6358-regulator.c | 65 +++++++++++++++++-----
> > include/linux/regulator/mt6358-regulator.h | 6 +-
> > 2 files changed, 52 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
> > index c9e16bd092f6..faf6b0757019 100644
> > --- a/drivers/regulator/mt6358-regulator.c
> > +++ b/drivers/regulator/mt6358-regulator.c
> > @@ -277,7 +277,7 @@ static const unsigned int vcama_voltages[] = {
> > 2800000, 2900000, 3000000,
> > };
> >
> > -static const unsigned int vcn33_bt_wifi_voltages[] = {
> > +static const unsigned int vcn33_voltages[] = {
> > 3300000, 3400000, 3500000,
> > };
> >
> > @@ -321,7 +321,7 @@ static const u32 vcama_idx[] = {
> > 0, 7, 9, 10, 11, 12,
> > };
> >
> > -static const u32 vcn33_bt_wifi_idx[] = {
> > +static const u32 vcn33_idx[] = {
> > 1, 2, 3,
> > };
> >
> > @@ -566,12 +566,8 @@ static struct mt6358_regulator_info mt6358_regulators[] = {
> > MT6358_LDO_VCAMA1_CON0, 0, MT6358_VCAMA1_ANA_CON0, 0xf00),
> > MT6358_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> > MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> > - MT6358_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > - MT6358_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > + MT6358_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> > + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> > MT6358_LDO("ldo_vcama2", VCAMA2, vcama_voltages, vcama_idx,
> > MT6358_LDO_VCAMA2_CON0, 0, MT6358_VCAMA2_ANA_CON0, 0xf00),
> > MT6358_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
> > @@ -662,12 +658,8 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> > MT6358_LDO_VMCH_CON0, 0, MT6358_VMCH_ANA_CON0, 0x700),
> > MT6366_LDO("ldo_vemc", VEMC, vmch_vemc_voltages, vmch_vemc_idx,
> > MT6358_LDO_VEMC_CON0, 0, MT6358_VEMC_ANA_CON0, 0x700),
> > - MT6366_LDO("ldo_vcn33_bt", VCN33_BT, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_0,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > - MT6366_LDO("ldo_vcn33_wifi", VCN33_WIFI, vcn33_bt_wifi_voltages,
> > - vcn33_bt_wifi_idx, MT6358_LDO_VCN33_CON0_1,
> > - 0, MT6358_VCN33_ANA_CON0, 0x300),
> > + MT6366_LDO("ldo_vcn33", VCN33, vcn33_voltages, vcn33_idx,
> > + MT6358_LDO_VCN33_CON0_0, 0, MT6358_VCN33_ANA_CON0, 0x300),
> > MT6366_LDO("ldo_vmc", VMC, vmc_voltages, vmc_idx,
> > MT6358_LDO_VMC_CON0, 0, MT6358_VMC_ANA_CON0, 0xf00),
> > MT6366_LDO("ldo_vsim2", VSIM2, vsim_voltages, vsim_idx,
> > @@ -690,13 +682,56 @@ static struct mt6358_regulator_info mt6366_regulators[] = {
> > MT6358_LDO_VSRAM_CON1, 0x7f),
> > };
> >
> > +static int mt6358_sync_vcn33_setting(struct device *dev)
> > +{
> > + struct mt6397_chip *mt6397 = dev_get_drvdata(dev->parent);
> > + unsigned int val;
> > + int ret;
> > +
> > + /*
> > + * VCN33_WIFI and VCN33_BT are two separate enable bits for the same
> > + * regulator. They share the same voltage setting and output pin.
> > + * Instead of having two potentially conflicting regulators, just have
> > + * one VCN33 regulator. Sync the two enable bits and only use one in
> > + * the regulator device.
> > + */
> > + ret = regmap_read(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, &val);
> > + if (ret) {
> > + dev_err(dev, "Failed to read VCN33_WIFI setting\n");
> > + return ret;
> > + }
> > +
> > + if (!(val & BIT(0)))
> > + return 0;
> > +
> > + /* Sync VCN33_WIFI enable status to VCN33_BT */
> > + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_0, BIT(0), BIT(0));
> > + if (ret) {
> > + dev_err(dev, "Failed to sync VCN33_WIFI setting to VCN33_BT\n");
> > + return ret;
> > + }
> > +
> > + /* Disable VCN33_WIFI */
> > + ret = regmap_update_bits(mt6397->regmap, MT6358_LDO_VCN33_CON0_1, BIT(0), 0);
> > + if (ret) {
> > + dev_err(dev, "Failed to disable VCN33_BT\n");
>
> I think it should be "VCN33_WIFI" in the error message?
Mark already merged the patch. I send followup fixes for this and the
call sequence.
ChenYu