From: Ahmad Fatoum The power supplies powering the IC may not necessarily be enabled by the time the driver probes. The binding describes the power rails, so enable them at probe with devm_regulator_bulk_get_enable(), before the reset line is requested and driven, so the chip is powered before its pins are driven. Boards that do not describe these supplies fall back to dummy regulators (with a "supply not found" warning) and keep working as before. A board that describes supplies but no reset line still needs to wait for the chip to boot before the first register access, so apply the existing start delay in that case too. Signed-off-by: Alvin Šipraga Signed-off-by: Ahmad Fatoum Co-developed-by: Oleksij Rempel Signed-off-by: Oleksij Rempel --- Anticipated reviewer questions: Q: Why devm_regulator_bulk_get_enable() rather than a per-supply loop over devm_regulator_get_enable_optional()? A: regulator_get_optional() must only be used for supplies that may be physically absent; these rails are always physically present, so the normal (non-optional) get is the correct API. Boards that do not describe the rails fall back to dummy regulators - a dummy-supply message, not a functional change or a regression; many systems boot with such messages. Hiding the message with a per-supply _optional loop would be a buggy use of the API. Q: On unbind or probe failure the devres unwind disables the rails after the reset GPIO/control have been released, leaving the chip unpowered with its reset pin still driven by the SoC - back-powering. Should a devm_add_action_or_reset() assert reset before the rails drop? A: Not in this patch. Asserting reset on teardown reopens the decision of commit 4f580e9aced1 ("net: dsa: realtek: do not assert reset on remove"), which intentionally stopped doing it, and would have to be gated so rtl8366rb - the chip that commit was about - is not changed. It is also only a partial mitigation: the SoC keeps driving the MDIO/RGMII pins regardless of the reset state. And these rails are usually fixed/always-on, so disabling them is a no-op; the window only exists on a design that genuinely gates them, where power-down sequencing is a board-level concern. Teardown sequencing is out of scope here and, if needed, belongs in a separate, dedicated change. Q: var->num_supplies is a per-variant constant (6 for every rtl8365mb), so the start delay also fires on boards that describe no rails and fall back to dummy regulators, where nothing was really powered. A: Intentional. After enabling the rails - real or dummy - the driver has no way to know the chip is ready, so it waits REALTEK_HW_START_DELAY before the first register access. The delay is harmless and only closes a race; a variant that declares no supplies (num_supplies == 0) is unaffected. Q: Why only rtl8365mb_variant, not rtl8366rb? A: The rail list is taken from the RTL8365MB datasheet. rtl8366rb has a different set and no in-tree board describes them, so it declares none (num_supplies stays 0) and its probe/teardown are unchanged. The companion dt-bindings patch likewise restricts the *-supply properties to the realtek,rtl8365mb compatible. Changes since v1: - Enable the rails with devm_regulator_bulk_get_enable() instead of a per-supply loop over devm_regulator_get_enable_optional(); the optional API is only for physically-absent supplies (Mark Brown), and undescribed rails fall back to dummy regulators, which is expected rather than a regression. - Describe the rails as a counted array (num_supplies) rather than a NULL-terminated list, matching the bulk API. --- drivers/net/dsa/realtek/realtek.h | 3 +++ drivers/net/dsa/realtek/rtl8365mb_main.c | 6 ++++++ drivers/net/dsa/realtek/rtl83xx.c | 17 ++++++++++++++++- 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h index 6e0148cee8d8..75a127545e5d 100644 --- a/drivers/net/dsa/realtek/realtek.h +++ b/drivers/net/dsa/realtek/realtek.h @@ -166,6 +166,9 @@ struct realtek_variant { u8 cmd_read; u8 cmd_write; size_t chip_data_sz; + /* Regulator supplies to enable at probe, or NULL */ + const char *const *supplies; + int num_supplies; }; /* RTL8366 library helpers */ diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c index 728231d8f94c..4c305756116c 100644 --- a/drivers/net/dsa/realtek/rtl8365mb_main.c +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c @@ -3334,6 +3334,10 @@ static const struct realtek_ops rtl8365mb_ops = { .phy_write = rtl8365mb_phy_write, }; +static const char *const rtl8365mb_supplies[] = { + "avddh", "avddl", "dvddio", "dvddio1", "dvddl", "pllvddl", +}; + const struct realtek_variant rtl8365mb_variant = { .ds_ops = &rtl8365mb_switch_ops, .ops = &rtl8365mb_ops, @@ -3342,6 +3346,8 @@ const struct realtek_variant rtl8365mb_variant = { .cmd_read = 0xb9, .cmd_write = 0xb8, .chip_data_sz = sizeof(struct rtl8365mb), + .supplies = rtl8365mb_supplies, + .num_supplies = ARRAY_SIZE(rtl8365mb_supplies), }; static const struct of_device_id rtl8365mb_of_match[] = { diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c index 35df809a5951..0ae9e311ef87 100644 --- a/drivers/net/dsa/realtek/rtl83xx.c +++ b/drivers/net/dsa/realtek/rtl83xx.c @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -195,7 +196,16 @@ rtl83xx_probe(struct device *dev, priv->leds_disabled = of_property_read_bool(dev->of_node, "realtek,disable-leds"); - /* TODO: if power is software controlled, set up any regulators here */ + /* Enable the supplies before the reset line is requested and driven, + * so the chip is powered before its pins are driven. + */ + if (var->num_supplies) { + ret = devm_regulator_bulk_get_enable(dev, var->num_supplies, + var->supplies); + if (ret) + return dev_err_ptr_probe(dev, ret, "failed to enable supplies\n"); + } + priv->reset_ctl = devm_reset_control_get_optional(dev, NULL); if (IS_ERR(priv->reset_ctl)) return dev_err_cast_probe(dev, priv->reset_ctl, @@ -216,6 +226,11 @@ rtl83xx_probe(struct device *dev, rtl83xx_reset_deassert(priv); msleep(REALTEK_HW_START_DELAY); dev_dbg(dev, "deasserted RESET\n"); + } else if (var->num_supplies) { + /* Powered but no reset line: still wait for the chip to boot + * before the first register access. + */ + msleep(REALTEK_HW_START_DELAY); } return priv; -- 2.47.3