From: Michael Riesch <michael.riesch@wolfvision.net>
To: Ahmad Fatoum <a.fatoum@pengutronix.de>, barebox@lists.infradead.org
Subject: Re: [PATCH v4 6/6] regulator: add Rockchip rk808 support
Date: Thu, 17 Nov 2022 08:25:59 +0100 [thread overview]
Message-ID: <52d826a4-4b41-fcc3-0482-44763d8833aa@wolfvision.net> (raw)
In-Reply-To: <20220724190006.2160802-6-a.fatoum@pengutronix.de>
Hi Ahmad,
I was tracking down some strange behavior and found something:
On 7/24/22 21:00, Ahmad Fatoum wrote:
> [...]
> +static struct rk_regulator_cfg rk809_reg[] = {
> + {{
> + /* .name = "DCDC_REG1", */
> + .supply_name = "vcc1",
> + .ops = &rk817_buck_ops_range,
> + .n_voltages = RK817_BUCK1_SEL_CNT + 1,
> + .linear_ranges = rk817_buck1_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(rk817_buck1_voltage_ranges),
> + .vsel_reg = RK817_BUCK1_ON_VSEL_REG,
> + .vsel_mask = RK817_BUCK_VSEL_MASK,
> + .enable_reg = RK817_POWER_EN_REG(0),
> + .enable_mask = ENABLE_MASK(RK817_ID_DCDC1),
> + .enable_val = ENABLE_MASK(RK817_ID_DCDC1),
> + .disable_val = DISABLE_VAL(RK817_ID_DCDC1),
> + }}, {{
> + /* .name = "DCDC_REG2", */
> + .supply_name = "vcc2",
> + .ops = &rk817_buck_ops_range,
> + .n_voltages = RK817_BUCK1_SEL_CNT + 1,
> + .linear_ranges = rk817_buck1_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(rk817_buck1_voltage_ranges),
> + .vsel_reg = RK817_BUCK2_ON_VSEL_REG,
> + .vsel_mask = RK817_BUCK_VSEL_MASK,
> + .enable_reg = RK817_POWER_EN_REG(0),
> + .enable_mask = ENABLE_MASK(RK817_ID_DCDC2),
> + .enable_val = ENABLE_MASK(RK817_ID_DCDC2),
> + .disable_val = DISABLE_VAL(RK817_ID_DCDC2),
> + }}, {{
> + /* .name = "DCDC_REG3", */
> + .supply_name = "vcc3",
> + .ops = &rk817_buck_ops_range,
> + .n_voltages = RK817_BUCK1_SEL_CNT + 1,
> + .linear_ranges = rk817_buck1_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(rk817_buck1_voltage_ranges),
> + .vsel_reg = RK817_BUCK3_ON_VSEL_REG,
> + .vsel_mask = RK817_BUCK_VSEL_MASK,
> + .enable_reg = RK817_POWER_EN_REG(0),
> + .enable_mask = ENABLE_MASK(RK817_ID_DCDC3),
> + .enable_val = ENABLE_MASK(RK817_ID_DCDC3),
> + .disable_val = DISABLE_VAL(RK817_ID_DCDC3),
> + }}, {{
> + /* .name = "DCDC_REG4", */
> + .supply_name = "vcc4",
> + .ops = &rk817_buck_ops_range,
> + .n_voltages = RK817_BUCK3_SEL_CNT + 1,
> + .linear_ranges = rk817_buck3_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(rk817_buck3_voltage_ranges),
> + .vsel_reg = RK817_BUCK4_ON_VSEL_REG,
> + .vsel_mask = RK817_BUCK_VSEL_MASK,
> + .enable_reg = RK817_POWER_EN_REG(0),
> + .enable_mask = ENABLE_MASK(RK817_ID_DCDC4),
> + .enable_val = ENABLE_MASK(RK817_ID_DCDC4),
> + .disable_val = DISABLE_VAL(RK817_ID_DCDC4),
> + }}, {{
> + /* .name = "DCDC_REG5", */
> + .supply_name = "vcc9",
> + .ops = &rk809_buck5_ops_range,
> + .n_voltages = RK809_BUCK5_SEL_CNT,
> + .linear_ranges = rk809_buck5_voltage_ranges,
> + .n_linear_ranges = ARRAY_SIZE(rk809_buck5_voltage_ranges),
> + .vsel_reg = RK809_BUCK5_CONFIG(0),
> + .vsel_mask = RK809_BUCK5_VSEL_MASK,
> + .enable_reg = RK817_POWER_EN_REG(3),
> + .enable_mask = ENABLE_MASK(1),
> + .enable_val = ENABLE_MASK(1),
> + .disable_val = DISABLE_VAL(1),
> + }},
The RK809 features an additional BUCK converter (DCDC_REG5), here on
index 4 of the array...
> + RK817_DESC(/* "LDO_REG1", */ "vcc5", 600, 3400, 25,
> + RK817_LDO_ON_VSEL_REG(0), RK817_LDO_VSEL_MASK,
> + RK817_POWER_EN_REG(1), ENABLE_MASK(0),
> + DISABLE_VAL(0), 400),
> + RK817_DESC(/* "LDO_REG2", */ "vcc5", 600, 3400, 25,
> + RK817_LDO_ON_VSEL_REG(1), RK817_LDO_VSEL_MASK,
> + RK817_POWER_EN_REG(1), ENABLE_MASK(1),
> + DISABLE_VAL(1), 400),
> + RK817_DESC(/* "LDO_REG3", */ "vcc5", 600, 3400, 25,
> + RK817_LDO_ON_VSEL_REG(2), RK817_LDO_VSEL_MASK,
> + RK817_POWER_EN_REG(1), ENABLE_MASK(2),
> + DISABLE_VAL(2), 400),
> + RK817_DESC(/* "LDO_REG4", */ "vcc6", 600, 3400, 25,
> + RK817_LDO_ON_VSEL_REG(3), RK817_LDO_VSEL_MASK,
> + RK817_POWER_EN_REG(1), ENABLE_MASK(3),
> + DISABLE_VAL(3), 400),
> + RK817_DESC(/* "LDO_REG5", */ "vcc6", 600, 3400, 25,
> + RK817_LDO_ON_VSEL_REG(4), RK817_LDO_VSEL_MASK,
> + RK817_POWER_EN_REG(2), ENABLE_MASK(0),
> + DISABLE_VAL(0), 400),
> + RK817_DESC(/* "LDO_REG6", */ "vcc6", 600, 3400, 25,
> + RK817_LDO_ON_VSEL_REG(5), RK817_LDO_VSEL_MASK,
> + RK817_POWER_EN_REG(2), ENABLE_MASK(1),
> + DISABLE_VAL(1), 400),
> + RK817_DESC(/* "LDO_REG7", */ "vcc7", 600, 3400, 25,
> + RK817_LDO_ON_VSEL_REG(6), RK817_LDO_VSEL_MASK,
> + RK817_POWER_EN_REG(2), ENABLE_MASK(2),
> + DISABLE_VAL(2), 400),
> + RK817_DESC(/* "LDO_REG8", */ "vcc7", 600, 3400, 25,
> + RK817_LDO_ON_VSEL_REG(7), RK817_LDO_VSEL_MASK,
> + RK817_POWER_EN_REG(2), ENABLE_MASK(3),
> + DISABLE_VAL(3), 400),
> + RK817_DESC(/* "LDO_REG9", */ "vcc7", 600, 3400, 25,
> + RK817_LDO_ON_VSEL_REG(8), RK817_LDO_VSEL_MASK,
> + RK817_POWER_EN_REG(3), ENABLE_MASK(0),
> + DISABLE_VAL(0), 400),
> + RK817_DESC_SWITCH(/* "SWITCH_REG1", */ "vcc9",
> + RK817_POWER_EN_REG(3), ENABLE_MASK(2), DISABLE_VAL(2)),
> + RK817_DESC_SWITCH(/* "SWITCH_REG2", */ "vcc8",
> + RK817_POWER_EN_REG(3), ENABLE_MASK(3), DISABLE_VAL(3)),
> +};
> +static_assert(ARRAY_SIZE(rk809_reg) == RK809_NUM_REGULATORS);
> [...]
> +static struct of_regulator_match rk809_reg_matches[] = {
> + MATCH(809, DCDC_REG1, DCDC1),
> + MATCH(809, DCDC_REG2, DCDC2),
> + MATCH(809, DCDC_REG3, DCDC3),
> + MATCH(809, DCDC_REG4, DCDC4),
> + MATCH(809, LDO_REG1, LDO1),
> + MATCH(809, LDO_REG2, LDO2),
> + MATCH(809, LDO_REG3, LDO3),
> + MATCH(809, LDO_REG4, LDO4),
> + MATCH(809, LDO_REG5, LDO5),
> + MATCH(809, LDO_REG6, LDO6),
> + MATCH(809, LDO_REG7, LDO7),
> + MATCH(809, LDO_REG8, LDO8),
> + MATCH(809, LDO_REG9, LDO9),
> + MATCH(809, DCDC_REG5, DCDC5),
Here, BUCK 5 is at index 13...
> + MATCH(809, SWITCH_REG1, SW1),
> + MATCH(809, SWITCH_REG2, SW2),
> +};
> +static_assert(ARRAY_SIZE(rk809_reg_matches) == RK809_NUM_REGULATORS);
> [...]
> +static int rk808_regulator_probe(struct device_d *dev)
> +{
> + struct rk808 *rk808 = dev->parent->priv;
> + struct rk_regulator_cfg *regulators;
> + struct of_regulator_match *matches;
> + int ret, i, nregulators;
> +
> + switch (rk808->variant) {
> + case RK805_ID:
> + regulators = rk805_reg;
> + matches = rk805_reg_matches;
> + nregulators = RK805_NUM_REGULATORS;
> + break;
> + case RK808_ID:
> + regulators = rk808_reg;
> + matches = rk808_reg_matches;
> + nregulators = RK809_NUM_REGULATORS;
> + break;
> + case RK809_ID:
> + regulators = rk809_reg;
> + matches = rk809_reg_matches;
> + nregulators = RK809_NUM_REGULATORS;
> + break;
> + case RK817_ID:
> + regulators = rk817_reg;
> + matches = rk817_reg_matches;
> + nregulators = RK817_NUM_REGULATORS;
> + break;
> + case RK818_ID:
> + regulators = rk818_reg;
> + matches = rk818_reg_matches;
> + nregulators = RK818_NUM_REGULATORS;
> + break;
> + default:
> + dev_err(dev, "unsupported RK8XX ID %lu\n", rk808->variant);
> + return -EINVAL;
> + }
> +
> + ret = rk808_regulator_dt_parse(&rk808->i2c->dev, matches, nregulators);
> + if (ret < 0)
> + return ret;
> +
> + /* Instantiate the regulators */
> + for (i = 0; i < nregulators; i++) {
> + ret = rk808_regulator_register(rk808, i, &matches[i],
> + ®ulators[i]);
... but here it a 1:1 mapping between matches and regulators is assumed.
In technical terms, board go brzzz.
I moved the DCDC_REG5 regulator description to index 13 for a quick fix.
If this is the desired solution, I can spin a patch. What do you think?
Best regards,
Michael
> [...]
next prev parent reply other threads:[~2022-11-17 7:30 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-24 19:00 [PATCH v4 1/6] mfd: implement mfd_add_devices Ahmad Fatoum
2022-07-24 19:00 ` [PATCH v4 2/6] regmap: implement regmap_init_i2c_smbus Ahmad Fatoum
2022-07-24 19:00 ` [PATCH v4 3/6] regulator: consult min_uv, max_uv for regulator_get_voltage Ahmad Fatoum
2022-07-24 19:00 ` [PATCH v4 4/6] regulator: recursively enable/disable regulator dependency tree Ahmad Fatoum
2022-07-24 19:00 ` [PATCH v4 5/6] regulator: fixed: request vin-supply as needed Ahmad Fatoum
2022-07-24 19:00 ` [PATCH v4 6/6] regulator: add Rockchip rk808 support Ahmad Fatoum
2022-11-17 7:25 ` Michael Riesch [this message]
2022-08-08 14:22 ` [PATCH v4 1/6] mfd: implement mfd_add_devices Sascha Hauer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52d826a4-4b41-fcc3-0482-44763d8833aa@wolfvision.net \
--to=michael.riesch@wolfvision.net \
--cc=a.fatoum@pengutronix.de \
--cc=barebox@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox