| Message ID | 20250604-s2mpg1x-regulators-v1-7-6038740f49ae@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Samsung S2MPG10 regulator and S2MPG11 PMIC drivers | expand |
On Wed, 04 Jun 2025, André Draszik wrote: > Bucks can conceivably be used as supplies for LDOs, which means we need > to instantiate them separately from each other so that the supply- > consumer links can be resolved successfully at probe time. > > By doing so, the kernel will defer and retry instantiating the LDOs > once BUCKs have been created while without this change, it can be > impossible to mark BUCKs as LDO supplies. This becomes particularly > an issue with the upcoming support for the S2MPG11 PMIC, where > typically certain S2MP10/11 buck rails supply certain S2MP11/10 LDO > rails. > > The platform_device's ::id field is used to inform the regulator driver > which type of regulators (buck or ldo) to instantiate. I'm confused. There is nothing that differentiates the two, so why do you need to? > Signed-off-by: André Draszik <andre.draszik@linaro.org> > --- > drivers/mfd/sec-common.c | 4 +++- > include/linux/mfd/samsung/s2mpg10.h | 5 +++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/mfd/sec-common.c b/drivers/mfd/sec-common.c > index 42d55e70e34c8d7cd68cddaecc88017e259365b4..8a1694c6ed8708397a51ebd4a49c22387d7e3495 100644 > --- a/drivers/mfd/sec-common.c > +++ b/drivers/mfd/sec-common.c > @@ -14,6 +14,7 @@ > #include <linux/mfd/core.h> > #include <linux/mfd/samsung/core.h> > #include <linux/mfd/samsung/irq.h> > +#include <linux/mfd/samsung/s2mpg10.h> > #include <linux/mfd/samsung/s2mps11.h> > #include <linux/mfd/samsung/s2mps13.h> > #include <linux/module.h> > @@ -35,7 +36,8 @@ static const struct mfd_cell s2dos05_devs[] = { > > static const struct mfd_cell s2mpg10_devs[] = { > MFD_CELL_NAME("s2mpg10-meter"), > - MFD_CELL_NAME("s2mpg10-regulator"), > + MFD_CELL_BASIC("s2mpg10-regulator", NULL, NULL, 0, S2MPG10_REGULATOR_CELL_ID_BUCKS), > + MFD_CELL_BASIC("s2mpg10-regulator", NULL, NULL, 0, S2MPG10_REGULATOR_CELL_ID_LDOS), > MFD_CELL_NAME("s2mpg10-rtc"), > MFD_CELL_OF("s2mpg10-clk", NULL, NULL, 0, 0, "samsung,s2mpg10-clk"), > MFD_CELL_OF("s2mpg10-gpio", NULL, NULL, 0, 0, "samsung,s2mpg10-gpio"), > diff --git a/include/linux/mfd/samsung/s2mpg10.h b/include/linux/mfd/samsung/s2mpg10.h > index 9f5919b89a3c286bf1cd6b3ef0e74bc993bff01a..3e8bc65078472518c5e77f8bd199ee403eda18ea 100644 > --- a/include/linux/mfd/samsung/s2mpg10.h > +++ b/include/linux/mfd/samsung/s2mpg10.h > @@ -8,6 +8,11 @@ > #ifndef __LINUX_MFD_S2MPG10_H > #define __LINUX_MFD_S2MPG10_H > > +enum s2mpg10_regulator_mfd_cell_id { > + S2MPG10_REGULATOR_CELL_ID_BUCKS = 1, > + S2MPG10_REGULATOR_CELL_ID_LDOS = 2, > +}; > + > /* Common registers (type 0x000) */ > enum s2mpg10_common_reg { > S2MPG10_COMMON_CHIPID, > > -- > 2.49.0.1204.g71687c7c1d-goog >
Hi Lee, Thanks for your review! On Fri, 2025-06-13 at 15:19 +0100, Lee Jones wrote: > On Wed, 04 Jun 2025, André Draszik wrote: > > > Bucks can conceivably be used as supplies for LDOs, which means we need > > to instantiate them separately from each other so that the supply- > > consumer links can be resolved successfully at probe time. > > > > By doing so, the kernel will defer and retry instantiating the LDOs > > once BUCKs have been created while without this change, it can be > > impossible to mark BUCKs as LDO supplies. This becomes particularly > > an issue with the upcoming support for the S2MPG11 PMIC, where > > typically certain S2MP10/11 buck rails supply certain S2MP11/10 LDO > > rails. > > > > The platform_device's ::id field is used to inform the regulator driver > > which type of regulators (buck or ldo) to instantiate. > > I'm confused. > > There is nothing that differentiates the two, so why do you need to? On gs101, we have two PMICs, s2mpg10 and s2mpg11. Several s2mpg10 LDOs are consumers of various s2mpg10 bucks & s2mpg10 bucks, and several s2mpg11 LDOs are also supplied by various s2mpg10 bucks & s2mpg11 bucks. So we have a circular dependency here. LDOs of one PMIC depend on bucks of the other. If all s2mpg10 rails are handled by the same instance of the s2mpg10 regulator driver, probe will defer (and ultimately fail), because the supplies to the LDOs can not be resolved during probe. The same goes for s2mpg11. The result is that neither driver can probe successfully (unless you're _extremely_ lucky due to parallel probing, but we can not rely on that, of course). By splitting LDO and buck rails into separate instances, this circular dependency is gone, the buck-instance of each respective driver can probe, which then allows the LDO instance of the other driver to probe. Does that answer the question, or did I misunderstand it? Cheers, Andre'
On Fri, 2025-06-13 at 15:49 +0100, André Draszik wrote: > Hi Lee, > > Thanks for your review! > > On Fri, 2025-06-13 at 15:19 +0100, Lee Jones wrote: > > On Wed, 04 Jun 2025, André Draszik wrote: > > > > > Bucks can conceivably be used as supplies for LDOs, which means we need > > > to instantiate them separately from each other so that the supply- > > > consumer links can be resolved successfully at probe time. > > > > > > By doing so, the kernel will defer and retry instantiating the LDOs > > > once BUCKs have been created while without this change, it can be > > > impossible to mark BUCKs as LDO supplies. This becomes particularly > > > an issue with the upcoming support for the S2MPG11 PMIC, where > > > typically certain S2MP10/11 buck rails supply certain S2MP11/10 LDO > > > rails. > > > > > > The platform_device's ::id field is used to inform the regulator driver > > > which type of regulators (buck or ldo) to instantiate. > > > > I'm confused. > > > > There is nothing that differentiates the two, so why do you need to? > > On gs101, we have two PMICs, s2mpg10 and s2mpg11. Several s2mpg10 LDOs > are consumers of various s2mpg10 bucks & s2mpg10 bucks, and several Small typo, should read: ... Several s2mpg10 LDOs are consumers of various s2mpg10 bucks & s2mpg11 bucks... > s2mpg11 LDOs are also supplied by various s2mpg10 bucks & s2mpg11 bucks. > > So we have a circular dependency here. LDOs of one PMIC depend on bucks > of the other. > > If all s2mpg10 rails are handled by the same instance of the s2mpg10 > regulator driver, probe will defer (and ultimately fail), because the > supplies to the LDOs can not be resolved during probe. > > The same goes for s2mpg11. > > The result is that neither driver can probe successfully (unless you're > _extremely_ lucky due to parallel probing, but we can not rely on that, > of course). > > By splitting LDO and buck rails into separate instances, this circular > dependency is gone, the buck-instance of each respective driver can probe, > which then allows the LDO instance of the other driver to probe. > > Does that answer the question, or did I misunderstand it? > > > Cheers, > Andre'
diff --git a/drivers/mfd/sec-common.c b/drivers/mfd/sec-common.c index 42d55e70e34c8d7cd68cddaecc88017e259365b4..8a1694c6ed8708397a51ebd4a49c22387d7e3495 100644 --- a/drivers/mfd/sec-common.c +++ b/drivers/mfd/sec-common.c @@ -14,6 +14,7 @@ #include <linux/mfd/core.h> #include <linux/mfd/samsung/core.h> #include <linux/mfd/samsung/irq.h> +#include <linux/mfd/samsung/s2mpg10.h> #include <linux/mfd/samsung/s2mps11.h> #include <linux/mfd/samsung/s2mps13.h> #include <linux/module.h> @@ -35,7 +36,8 @@ static const struct mfd_cell s2dos05_devs[] = { static const struct mfd_cell s2mpg10_devs[] = { MFD_CELL_NAME("s2mpg10-meter"), - MFD_CELL_NAME("s2mpg10-regulator"), + MFD_CELL_BASIC("s2mpg10-regulator", NULL, NULL, 0, S2MPG10_REGULATOR_CELL_ID_BUCKS), + MFD_CELL_BASIC("s2mpg10-regulator", NULL, NULL, 0, S2MPG10_REGULATOR_CELL_ID_LDOS), MFD_CELL_NAME("s2mpg10-rtc"), MFD_CELL_OF("s2mpg10-clk", NULL, NULL, 0, 0, "samsung,s2mpg10-clk"), MFD_CELL_OF("s2mpg10-gpio", NULL, NULL, 0, 0, "samsung,s2mpg10-gpio"), diff --git a/include/linux/mfd/samsung/s2mpg10.h b/include/linux/mfd/samsung/s2mpg10.h index 9f5919b89a3c286bf1cd6b3ef0e74bc993bff01a..3e8bc65078472518c5e77f8bd199ee403eda18ea 100644 --- a/include/linux/mfd/samsung/s2mpg10.h +++ b/include/linux/mfd/samsung/s2mpg10.h @@ -8,6 +8,11 @@ #ifndef __LINUX_MFD_S2MPG10_H #define __LINUX_MFD_S2MPG10_H +enum s2mpg10_regulator_mfd_cell_id { + S2MPG10_REGULATOR_CELL_ID_BUCKS = 1, + S2MPG10_REGULATOR_CELL_ID_LDOS = 2, +}; + /* Common registers (type 0x000) */ enum s2mpg10_common_reg { S2MPG10_COMMON_CHIPID,
Bucks can conceivably be used as supplies for LDOs, which means we need to instantiate them separately from each other so that the supply- consumer links can be resolved successfully at probe time. By doing so, the kernel will defer and retry instantiating the LDOs once BUCKs have been created while without this change, it can be impossible to mark BUCKs as LDO supplies. This becomes particularly an issue with the upcoming support for the S2MPG11 PMIC, where typically certain S2MP10/11 buck rails supply certain S2MP11/10 LDO rails. The platform_device's ::id field is used to inform the regulator driver which type of regulators (buck or ldo) to instantiate. Signed-off-by: André Draszik <andre.draszik@linaro.org> --- drivers/mfd/sec-common.c | 4 +++- include/linux/mfd/samsung/s2mpg10.h | 5 +++++ 2 files changed, 8 insertions(+), 1 deletion(-)