* [RFC PATCH 0/2] Hello @ 2019-07-30 17:30 Philippe Schenker 2019-07-30 17:30 ` [RFC PATCH 1/2] Regulator: Core: Add clock-enable to fixed-regulator Philippe Schenker 2019-07-30 17:30 ` [RFC PATCH 2/2] ARM: dts: imx6ull: Add phy-supply to fec Philippe Schenker 0 siblings, 2 replies; 11+ messages in thread From: Philippe Schenker @ 2019-07-30 17:30 UTC (permalink / raw) To: Liam Girdwood, Mark Brown, linux-kernel; +Cc: Philippe Schenker From: Philippe Schenker <philippe.schenker@toradex.com> On our Colibri iMX6ULL board there is a circuit for switching the power supply of the ethernet PHY with the 50MHz RMII clock. This works quite fine but has one big problem. It is quite slow when switching the supply, so Linux has to wait there. I think this switch is at best represented as a fixed-regulator. In that way I can use "startup-delay-us" to represent slow switching regulator. But there's no current possibility to enable fixed-regulator with a clock. In this RFC I send a patch after we would be able to add a clock to a fixed-regulator in devicetree and then add the "startup-delay-us" which would solve all my problems relatively elegant. This works also the other way, if the PHY now needs power the clock is not powered off because regulator depends on it. Because this would need to change code in regulator core I like to ask for your oppinion first, or if anyone has another idea how I could solve that problem. Thanks in advance for your feedback! Philippe Philippe Schenker (2): Regulator: Core: Add clock-enable to fixed-regulator ARM: dts: imx6ull: Add phy-supply to fec arch/arm/boot/dts/imx6ull-colibri.dtsi | 12 ++++++++++++ drivers/regulator/core.c | 15 +++++++++++++++ drivers/regulator/fixed.c | 6 ++++++ include/linux/regulator/driver.h | 3 +++ include/linux/regulator/fixed.h | 1 + 5 files changed, 37 insertions(+) -- 2.22.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC PATCH 1/2] Regulator: Core: Add clock-enable to fixed-regulator 2019-07-30 17:30 [RFC PATCH 0/2] Hello Philippe Schenker @ 2019-07-30 17:30 ` Philippe Schenker 2019-07-30 18:10 ` Mark Brown 2019-07-30 17:30 ` [RFC PATCH 2/2] ARM: dts: imx6ull: Add phy-supply to fec Philippe Schenker 1 sibling, 1 reply; 11+ messages in thread From: Philippe Schenker @ 2019-07-30 17:30 UTC (permalink / raw) To: Liam Girdwood, Mark Brown, linux-kernel; +Cc: Philippe Schenker From: Philippe Schenker <philippe.schenker@toradex.com> This adds the possibility to enable a fixed-regulator with a clock. Signed-off-by: <philippe.schenker@toradex.com> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> --- drivers/regulator/core.c | 15 +++++++++++++++ drivers/regulator/fixed.c | 6 ++++++ include/linux/regulator/driver.h | 3 +++ include/linux/regulator/fixed.h | 1 + 4 files changed, 25 insertions(+) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index e0c0cf462004..f16f6c147858 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -26,6 +26,7 @@ #include <linux/regulator/driver.h> #include <linux/regulator/machine.h> #include <linux/module.h> +#include <linux/clk.h> #define CREATE_TRACE_POINTS #include <trace/events/regulator.h> @@ -2385,6 +2386,11 @@ static int _regulator_do_enable(struct regulator_dev *rdev) ret = rdev->desc->ops->enable(rdev); if (ret < 0) return ret; + } else if (rdev->ena_clk) { + ret = clk_prepare_enable(rdev->ena_clk); + if (ret) + return ret; + rdev->ena_clk_state++; } else { return -EINVAL; } @@ -2565,6 +2571,9 @@ static int _regulator_do_disable(struct regulator_dev *rdev) ret = rdev->desc->ops->disable(rdev); if (ret != 0) return ret; + } else if (rdev->ena_clk) { + clk_disable_unprepare(rdev->ena_clk); + rdev->ena_clk_state--; } /* cares about last_off_jiffy only if off_on_delay is required by @@ -2796,6 +2805,9 @@ static int _regulator_is_enabled(struct regulator_dev *rdev) if (rdev->ena_pin) return rdev->ena_gpio_state; + if (rdev->ena_clk) + return (rdev->ena_clk_state > 0) ? 1 : 0; + /* If we don't know then assume that the regulator is always on */ if (!rdev->desc->ops->is_enabled) return 1; @@ -5098,6 +5110,9 @@ regulator_register(const struct regulator_desc *regulator_desc, dangling_of_gpiod = false; } + if (cfg->ena_clk) + rdev->ena_clk = cfg->ena_clk; + /* register with sysfs */ rdev->dev.class = ®ulator_class; rdev->dev.parent = dev; diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c index 999547dde99d..0093c26cda3c 100644 --- a/drivers/regulator/fixed.c +++ b/drivers/regulator/fixed.c @@ -25,6 +25,7 @@ #include <linux/of.h> #include <linux/regulator/of_regulator.h> #include <linux/regulator/machine.h> +#include <linux/clk.h> struct fixed_voltage_data { struct regulator_desc desc; @@ -78,6 +79,10 @@ of_get_fixed_voltage_config(struct device *dev, if (of_find_property(np, "vin-supply", NULL)) config->input_supply = "vin"; + config->ena_clk = devm_clk_get(dev, NULL); + if (IS_ERR(config->ena_clk)) + config->ena_clk = NULL; + return config; } @@ -172,6 +177,7 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev) cfg.init_data = config->init_data; cfg.driver_data = drvdata; cfg.of_node = pdev->dev.of_node; + cfg.ena_clk = config->ena_clk; drvdata->dev = devm_regulator_register(&pdev->dev, &drvdata->desc, &cfg); diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h index 9a911bb5fb61..335ac1272bbd 100644 --- a/include/linux/regulator/driver.h +++ b/include/linux/regulator/driver.h @@ -414,6 +414,7 @@ struct regulator_config { void *driver_data; struct device_node *of_node; struct regmap *regmap; + struct clk *ena_clk; struct gpio_desc *ena_gpiod; }; @@ -477,6 +478,8 @@ struct regulator_dev { struct regulator_enable_gpio *ena_pin; unsigned int ena_gpio_state:1; + struct clk *ena_clk; + unsigned int ena_clk_state; unsigned int is_switch:1; diff --git a/include/linux/regulator/fixed.h b/include/linux/regulator/fixed.h index d44ce5f18a56..c291e1130381 100644 --- a/include/linux/regulator/fixed.h +++ b/include/linux/regulator/fixed.h @@ -38,6 +38,7 @@ struct fixed_voltage_config { unsigned startup_delay; unsigned enabled_at_boot:1; struct regulator_init_data *init_data; + struct clk *ena_clk; }; struct regulator_consumer_supply; -- 2.22.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] Regulator: Core: Add clock-enable to fixed-regulator 2019-07-30 17:30 ` [RFC PATCH 1/2] Regulator: Core: Add clock-enable to fixed-regulator Philippe Schenker @ 2019-07-30 18:10 ` Mark Brown 2019-07-30 21:00 ` Philippe Schenker 0 siblings, 1 reply; 11+ messages in thread From: Mark Brown @ 2019-07-30 18:10 UTC (permalink / raw) To: Philippe Schenker; +Cc: Liam Girdwood, linux-kernel, Philippe Schenker [-- Attachment #1: Type: text/plain, Size: 1157 bytes --] On Tue, Jul 30, 2019 at 07:30:04PM +0200, Philippe Schenker wrote: > From: Philippe Schenker <philippe.schenker@toradex.com> > > This adds the possibility to enable a fixed-regulator with a clock. Why? What does the hardware which makes this make sense look like? Your cover letter didn't explain at all clearly, it just said that there's a circuit that is connected to a clock which somehow switches something but it's not clear. It's certainly not clear that this should be in the core, the circuit doesn't sound like a good idea at all. > Signed-off-by: <philippe.schenker@toradex.com> > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> This needs a cleanup. > > /* cares about last_off_jiffy only if off_on_delay is required by > @@ -2796,6 +2805,9 @@ static int _regulator_is_enabled(struct regulator_dev *rdev) > if (rdev->ena_pin) > return rdev->ena_gpio_state; > > + if (rdev->ena_clk) > + return (rdev->ena_clk_state > 0) ? 1 : 0; > + Please write normal conditional statements, this isn't helping legibility. Though in this case the ternery operator is totally redundant anyway... [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] Regulator: Core: Add clock-enable to fixed-regulator 2019-07-30 18:10 ` Mark Brown @ 2019-07-30 21:00 ` Philippe Schenker 2019-07-31 21:23 ` Mark Brown 0 siblings, 1 reply; 11+ messages in thread From: Philippe Schenker @ 2019-07-30 21:00 UTC (permalink / raw) To: broonie; +Cc: lgirdwood, linux-kernel On Tue, 2019-07-30 at 19:10 +0100, Mark Brown wrote: > On Tue, Jul 30, 2019 at 07:30:04PM +0200, Philippe Schenker wrote: > > From: Philippe Schenker <philippe.schenker@toradex.com> > > > > This adds the possibility to enable a fixed-regulator with a clock. > > Why? What does the hardware which makes this make sense look like? Tomorrow I can provide some schematics if needed. But its just a simple switch that is switched by a clock (on when clock is on and off when clock is off). This clock is the RGMII 50MHz clock for the ethernet PHY. That switch switches power rail of a KSZ8041 ethernet PHY. So the power rail of the KSZ8041 PHY is switched by its own clock. > Your cover letter didn't explain at all clearly, it just said that > there's a circuit that is connected to a clock which somehow switches > something but it's not clear. It's certainly not clear that this > should > be in the core, the circuit doesn't sound like a good idea at all. Sorry if I didn't explain it clear enough. I hope the hardware part is clear now from the explanation above. Otherwise let me know I will provide further explanations/schematics. To your other questions, I will split those for better understanding: Why is a regulator even needed? - On power up of the PHY there is a huge time I have to wait for voltage rail to settle. In the range of 100ms. - Because there is a switch in the circuit I abstract it with a regulator-fixed in devicetree to make use of the startup-delay. - This regulator/switch is enabled with a clock. So to be able to use the startup delay I need an enable-by-clock on regulator-fixed. Why do I think this should be in core? - Normally this task is done with gpio that is already in regulator- core. - Because that is already there I added the functionality for enabled- by-clock-functionality. - I thought of creating a new regulator-clock driver but that would hold a lot of code duplication from regulator-fixed. Why is this a good Idea at all? - Well I'm here for the software part and should just support our hardware. If that is a good Idea at all I don't know, for sure it is not a solution that is from some school-book. But I tried it and measured it out and it seems to work pretty fine. - The reason behind all of that is limited GPIO availability from the iMX6ULL. > > > Signed-off-by: <philippe.schenker@toradex.com> > > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> > > This needs a cleanup. Of course, sorry I didn't saw that beforehand. Some mess created with cherry-picking... > > > > > /* cares about last_off_jiffy only if off_on_delay is required > > by > > @@ -2796,6 +2805,9 @@ static int _regulator_is_enabled(struct > > regulator_dev *rdev) > > if (rdev->ena_pin) > > return rdev->ena_gpio_state; > > > > + if (rdev->ena_clk) > > + return (rdev->ena_clk_state > 0) ? 1 : 0; > > + > > Please write normal conditional statements, this isn't helping > legibility. Though in this case the ternery operator is totally > redundant anyway... Yeah now that I look at it you're right. I have in mind that I copied that from somewhere to get the same coding style. I developed that in an old kernel so could be that it's from there. Anyway, this is just a concept for now and would need some more thinking... With this patch I want to put off a discussion, how we can support our hardware in mainline Linux. This is my first proposal for that. Philippe ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] Regulator: Core: Add clock-enable to fixed-regulator 2019-07-30 21:00 ` Philippe Schenker @ 2019-07-31 21:23 ` Mark Brown 2019-08-05 11:07 ` Philippe Schenker 0 siblings, 1 reply; 11+ messages in thread From: Mark Brown @ 2019-07-31 21:23 UTC (permalink / raw) To: Philippe Schenker; +Cc: lgirdwood, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2312 bytes --] On Tue, Jul 30, 2019 at 09:00:01PM +0000, Philippe Schenker wrote: > On Tue, 2019-07-30 at 19:10 +0100, Mark Brown wrote: > > On Tue, Jul 30, 2019 at 07:30:04PM +0200, Philippe Schenker wrote: > > > This adds the possibility to enable a fixed-regulator with a clock. > > Why? What does the hardware which makes this make sense look like? > Tomorrow I can provide some schematics if needed. But its just a simple > switch that is switched by a clock (on when clock is on and off when > clock is off). This clock is the RGMII 50MHz clock for the ethernet > PHY. So it's not switching with the clock, the circuit somehow keeps the switch latched? > Why is a regulator even needed? > - On power up of the PHY there is a huge time I have to wait for > voltage rail to settle. In the range of 100ms. > - Because there is a switch in the circuit I abstract it with a > regulator-fixed in devicetree to make use of the startup-delay. > - This regulator/switch is enabled with a clock. So to be able to use > the startup delay I need an enable-by-clock on regulator-fixed. It does feel like it might be simpler to just handle this as a quirk in the PHY or ethernet driver, this feels like an awful lot of work to add a sleep on what's probably only going to ever be one system. > Why do I think this should be in core? > - Normally this task is done with gpio that is already in regulator- > core. > - Because that is already there I added the functionality for enabled- > by-clock-functionality. > - I thought of creating a new regulator-clock driver but that would > hold a lot of code duplication from regulator-fixed. Hopefully not a *lot* of duplication. The GPIOs are handled in the core because they're really common and used by many regulator devices, the same will I hope not be true for clocks. > Why is this a good Idea at all? > - Well I'm here for the software part and should just support our > hardware. If that is a good Idea at all I don't know, for sure it is > not a solution that is from some school-book. But I tried it and > measured it out and it seems to work pretty fine. > - The reason behind all of that is limited GPIO availability from the > iMX6ULL. I guess my question here is what the trip through the regulator API buys us - it's a bit of a sledgehammer to crack a nut thing. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] Regulator: Core: Add clock-enable to fixed-regulator 2019-07-31 21:23 ` Mark Brown @ 2019-08-05 11:07 ` Philippe Schenker 2019-08-05 16:37 ` Mark Brown 0 siblings, 1 reply; 11+ messages in thread From: Philippe Schenker @ 2019-08-05 11:07 UTC (permalink / raw) To: broonie; +Cc: lgirdwood, linux-kernel On Wed, 2019-07-31 at 22:23 +0100, Mark Brown wrote: > On Tue, Jul 30, 2019 at 09:00:01PM +0000, Philippe Schenker wrote: > > On Tue, 2019-07-30 at 19:10 +0100, Mark Brown wrote: > > > On Tue, Jul 30, 2019 at 07:30:04PM +0200, Philippe Schenker wrote: > > > > This adds the possibility to enable a fixed-regulator with a clock. > > > Why? What does the hardware which makes this make sense look like? > > Tomorrow I can provide some schematics if needed. But its just a simple > > switch that is switched by a clock (on when clock is on and off when > > clock is off). This clock is the RGMII 50MHz clock for the ethernet > > PHY. > > So it's not switching with the clock, the circuit somehow keeps the > switch latched? No, it doesn't keep it latched. To make things clear here a status table: +----------+-----------+ | Clock | Switch | +----------+-----------+ | enabled | +3.3V on | +----------+-----------+ | disabled | +3.3V off | +--------- +-----------+ I tried to do schematics in ASCII, for documentation purpose on ML: +V3.3 >-----------------------+-------+--┐ +-----------> +3.3V_ETH | | ^ | switched | ---┴-- supply to PHY | |¯¯¯¯¯ +-+ | p-FET R | | | | | | +-+ | | | n-FET +-------+ ||--┘ | RMII_CLK_50MHz | | ||<-┐ | ----------------| |---+___|| | ----- | | | | ----- C | | C | +-+ | | R | | | | | | | | +-+ | | | | | | | | +++ +++ +++ GND GND GND If it shouldn't be clear here a picture: https://share.toradex.com/v9d7a97mvq1ks62 > > > Why is a regulator even needed? > > - On power up of the PHY there is a huge time I have to wait for > > voltage rail to settle. In the range of 100ms. > > - Because there is a switch in the circuit I abstract it with a > > regulator-fixed in devicetree to make use of the startup-delay. > > - This regulator/switch is enabled with a clock. So to be able to use > > the startup delay I need an enable-by-clock on regulator-fixed. > > It does feel like it might be simpler to just handle this as a quirk in > the PHY or ethernet driver, this feels like an awful lot of work to > add a sleep on what's probably only going to ever be one system. I thought of that too, but the problem with that approach is that I can't reflect the actual switching behavior. What would happen is if you turnethernet off with 'ip link set eth0 down', the clock would stop and therefore no more supply voltage to the PHY. But the ethernet driverwould in that case let the regulator enabled preventing, switching off the clock. Anyway I feel that to solve this with a quirk would be a little hackish, plus I'd anyway need to mess around with the Ethernet/PHY drivers. So why not solve it properly with a regulator that supports clocks? > > > Why do I think this should be in core? > > - Normally this task is done with gpio that is already in regulator- > > core. > > - Because that is already there I added the functionality for enabled- > > by-clock-functionality. > > - I thought of creating a new regulator-clock driver but that would > > hold a lot of code duplication from regulator-fixed. > > Hopefully not a *lot* of duplication. The GPIOs are handled in the core > because they're really common and used by many regulator devices, the > same will I hope not be true for clocks. I agree that they are commonly and widely used. To add support for clocks in regulator-core was really easy to do as I did it the same way as it is done with gpio's. If I don't need to touch regulator-core I don't want to. But as I said it was really easy for me to integrate it in there in a way without even understanding the whole regulator API. If it makes more sense to do it in a new file like clock-regulator.c and creating a new compatible that is what I'm trying to find out here. I'd be happy to write also a new clock-regulator driver for that purpose. > > > Why is this a good Idea at all? > > - Well I'm here for the software part and should just support our > > hardware. If that is a good Idea at all I don't know, for sure it is > > not a solution that is from some school-book. But I tried it and > > measured it out and it seems to work pretty fine. > > - The reason behind all of that is limited GPIO availability from the > > iMX6ULL. > > I guess my question here is what the trip through the regulator API buys > us - it's a bit of a sledgehammer to crack a nut thing. In my opinion this is not only about to solve my problem with startup-delay. I think that this is really a behavior that can be generic. That's also why I'm asking here how we want to solve that so not only I solve my little problem with a board quirk but in a broader view for possible future usage by others. It is possible that a regulator needs a clock. That exactly is, what we have on our board and works better than expected (at least by myself :-)). ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] Regulator: Core: Add clock-enable to fixed-regulator 2019-08-05 11:07 ` Philippe Schenker @ 2019-08-05 16:37 ` Mark Brown 2019-08-06 12:57 ` Philippe Schenker 0 siblings, 1 reply; 11+ messages in thread From: Mark Brown @ 2019-08-05 16:37 UTC (permalink / raw) To: Philippe Schenker; +Cc: lgirdwood, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3292 bytes --] On Mon, Aug 05, 2019 at 11:07:58AM +0000, Philippe Schenker wrote: > On Wed, 2019-07-31 at 22:23 +0100, Mark Brown wrote: Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to. > > So it's not switching with the clock, the circuit somehow keeps the > > switch latched? > No, it doesn't keep it latched. To make things clear here a status table: So the capacitor on the input of the p-FET is keeping the switch on? When I say it's not switching with the clock I mean it's not constantly bouncing on and off at whatever rate the clock is going at. > > It does feel like it might be simpler to just handle this as a quirk in > > the PHY or ethernet driver, this feels like an awful lot of work to > > add a sleep on what's probably only going to ever be one system. > I thought of that too, but the problem with that approach is that I > can't reflect the actual switching behavior. What would happen is if > you turnethernet off with 'ip link set eth0 down', the clock would > stop and therefore no more supply voltage to the PHY. But the ethernet > driverwould in that case let the regulator enabled preventing, > switching off the clock. You could include that in your quirk? > Anyway I feel that to solve this with a quirk would be a little > hackish, plus I'd anyway need to mess around with the Ethernet/PHY > drivers. So why not solve it properly with a regulator that supports > clocks? I think you are going to end up with a hack no matter what. > > Hopefully not a *lot* of duplication. The GPIOs are handled in the core > > because they're really common and used by many regulator devices, the > > same will I hope not be true for clocks. > I agree that they are commonly and widely used. To add support for clocks in > regulator-core was really easy to do as I did it the same way as it is done with > gpio's. If I don't need to touch regulator-core I don't want to. But as I said > it was really easy for me to integrate it in there in a way without even > understanding the whole regulator API. > If it makes more sense to do it in a new file like clock-regulator.c and > creating a new compatible that is what I'm trying to find out here. I'd be happy > to write also a new clock-regulator driver for that purpose. It would be better if it wasn't in the core, that keeps everything partitioned off nicely. > > I guess my question here is what the trip through the regulator API buys > > us - it's a bit of a sledgehammer to crack a nut thing. > In my opinion this is not only about to solve my problem with startup-delay. I > think that this is really a behavior that can be generic. That's also why I'm > asking here how we want to solve that so not only I solve my little problem with > a board quirk but in a broader view for possible future usage by others. > It is possible that a regulator needs a clock. That exactly is, what we have on > our board and works better than expected (at least by myself :-)). The majority of regulators that need clocks are PWM devices which is a whole other thing that we already support. This is a highly unusual hardware design, we don't have the regmap stuff in the core and that's a lot more common. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] Regulator: Core: Add clock-enable to fixed-regulator 2019-08-05 16:37 ` Mark Brown @ 2019-08-06 12:57 ` Philippe Schenker 2019-08-06 18:26 ` Mark Brown 0 siblings, 1 reply; 11+ messages in thread From: Philippe Schenker @ 2019-08-06 12:57 UTC (permalink / raw) To: broonie; +Cc: lgirdwood, linux-kernel On Mon, 2019-08-05 at 17:37 +0100, Mark Brown wrote: > On Mon, Aug 05, 2019 at 11:07:58AM +0000, Philippe Schenker wrote: > > On Wed, 2019-07-31 at 22:23 +0100, Mark Brown wrote: > > Please fix your mail client to word wrap within paragraphs at something > substantially less than 80 columns. Doing this makes your messages much > easier to read and reply to. That was a mistake, sorry. It actually is and was set to 80. Not sure what happened there. So it's not switching with the clock, the circuit somehow keeps the > > > switch latched? > > No, it doesn't keep it latched. To make things clear here a status table: > > So the capacitor on the input of the p-FET is keeping the switch on? > When I say it's not switching with the clock I mean it's not constantly > bouncing on and off at whatever rate the clock is going at. Ah, that's what you mean. Yes, the capacitor gets slowly charged with the resistor but nearly instantly discharged with the n-FET. So this capacitor is used as a Low-Pass filter to get the p-FET to be constantly switched. It is not bouncing on and off with the clock but rather it is switched constantly. > > > > It does feel like it might be simpler to just handle this as a quirk in > > > the PHY or ethernet driver, this feels like an awful lot of work to > > > add a sleep on what's probably only going to ever be one system. > > I thought of that too, but the problem with that approach is that I > > can't reflect the actual switching behavior. What would happen is if > > you turnethernet off with 'ip link set eth0 down', the clock would > > stop and therefore no more supply voltage to the PHY. But the ethernet > > driverwould in that case let the regulator enabled preventing, > > switching off the clock. > > You could include that in your quirk? Yes, this could be done as a quirk in the ethernet driver. But there the clock gets enabled/disabled in different places. So I would have to sort out where I need it and where not and basically go through the whole driver. Plus it gets intensive to maintain that solution. > > > Anyway I feel that to solve this with a quirk would be a little > > hackish, plus I'd anyway need to mess around with the Ethernet/PHY > > drivers. So why not solve it properly with a regulator that supports > > clocks? > > I think you are going to end up with a hack no matter what. That's exactly what I'm trying to prevent. To introduce a fixed regulator that can have a clock is not a hack for me. That the hardware solution is a hack is debatable yes, but why should I not try to solve it properly in software? > > > > Hopefully not a *lot* of duplication. The GPIOs are handled in the core > > > because they're really common and used by many regulator devices, the > > > same will I hope not be true for clocks. > > I agree that they are commonly and widely used. To add support for clocks in > > regulator-core was really easy to do as I did it the same way as it is done > > with > > gpio's. If I don't need to touch regulator-core I don't want to. But as I > > said > > it was really easy for me to integrate it in there in a way without even > > understanding the whole regulator API. > > If it makes more sense to do it in a new file like clock-regulator.c and > > creating a new compatible that is what I'm trying to find out here. I'd be > > happy > > to write also a new clock-regulator driver for that purpose. > > It would be better if it wasn't in the core, that keeps everything > partitioned off nicely. I agree that our hardware design is somewhat special and is not widely used. But I still think that this is valuable as a generic function and could possibly be of benefit to someone else. > > > > I guess my question here is what the trip through the regulator API buys > > > us - it's a bit of a sledgehammer to crack a nut thing. > > In my opinion this is not only about to solve my problem with startup-delay. > > I > > think that this is really a behavior that can be generic. That's also why > > I'm > > asking here how we want to solve that so not only I solve my little problem > > with > > a board quirk but in a broader view for possible future usage by others. > > It is possible that a regulator needs a clock. That exactly is, what we have > > on > > our board and works better than expected (at least by myself :-)). > > The majority of regulators that need clocks are PWM devices which is a > whole other thing that we already support. This is a highly unusual > hardware design, we don't have the regmap stuff in the core and that's a > lot more common. I agree with you, because of our unusual hardware design that this shouldn't be in core. But I do not agree on solving that with quirks. In the end I just want to represent our hardware in software. Would you agree to create a new clock-regulator.c driver? Or would it make more sense to extend fixed.c to support clocks-enable without touching core? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] Regulator: Core: Add clock-enable to fixed-regulator 2019-08-06 12:57 ` Philippe Schenker @ 2019-08-06 18:26 ` Mark Brown 2019-08-07 6:46 ` Philippe Schenker 0 siblings, 1 reply; 11+ messages in thread From: Mark Brown @ 2019-08-06 18:26 UTC (permalink / raw) To: Philippe Schenker; +Cc: lgirdwood, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1580 bytes --] On Tue, Aug 06, 2019 at 12:57:32PM +0000, Philippe Schenker wrote: > On Mon, 2019-08-05 at 17:37 +0100, Mark Brown wrote: > > So the capacitor on the input of the p-FET is keeping the switch on? > > When I say it's not switching with the clock I mean it's not constantly > > bouncing on and off at whatever rate the clock is going at. > Ah, that's what you mean. Yes, the capacitor gets slowly charged with > the > resistor but nearly instantly discharged with the n-FET. So this > capacitor > is used as a Low-Pass filter to get the p-FET to be constantly switched. > It is not bouncing on and off with the clock but rather it is switched > constantly. Good, I guess this might be part of why it's got this poor ramp time. > > I think you are going to end up with a hack no matter what. > That's exactly what I'm trying to prevent. To introduce a fixed > regulator that can have a clock is not a hack for me. > That the hardware solution is a hack is debatable yes, but why should I > not try to solve it properly in software? A lot of this discussion is around the definition of terms like "hack" and "proper". > In the end I just want to represent our hardware in software. Would you > agree to create a new clock-regulator.c driver? > Or would it make more sense to extend fixed.c to support clocks-enable > without touching core? At least a separate compatible makes sense, I'd have to see the code to be clear if a completely separate driver makes sense but it'll need separate ops at least. There'd definitely be a lot of overlap though so it's worth looking at. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] Regulator: Core: Add clock-enable to fixed-regulator 2019-08-06 18:26 ` Mark Brown @ 2019-08-07 6:46 ` Philippe Schenker 0 siblings, 0 replies; 11+ messages in thread From: Philippe Schenker @ 2019-08-07 6:46 UTC (permalink / raw) To: broonie; +Cc: lgirdwood, linux-kernel On Tue, 2019-08-06 at 19:26 +0100, Mark Brown wrote: > On Tue, Aug 06, 2019 at 12:57:32PM +0000, Philippe Schenker wrote: > > On Mon, 2019-08-05 at 17:37 +0100, Mark Brown wrote: > > > So the capacitor on the input of the p-FET is keeping the switch > > > on? > > > When I say it's not switching with the clock I mean it's not > > > constantly > > > bouncing on and off at whatever rate the clock is going at. > > Ah, that's what you mean. Yes, the capacitor gets slowly charged > > with > > the > > resistor but nearly instantly discharged with the n-FET. So this > > capacitor > > is used as a Low-Pass filter to get the p-FET to be constantly > > switched. > > It is not bouncing on and off with the clock but rather it is > > switched > > constantly. > > Good, I guess this might be part of why it's got this poor ramp time. Yes, I think so too. > > > > I think you are going to end up with a hack no matter what. > > That's exactly what I'm trying to prevent. To introduce a fixed > > regulator that can have a clock is not a hack for me. > > That the hardware solution is a hack is debatable yes, but why > > should I > > not try to solve it properly in software? > > A lot of this discussion is around the definition of terms like "hack" > and "proper". > > > In the end I just want to represent our hardware in software. Would > > you > > agree to create a new clock-regulator.c driver? > > Or would it make more sense to extend fixed.c to support clocks- > > enable > > without touching core? > > At least a separate compatible makes sense, I'd have to see the code > to > be clear if a completely separate driver makes sense but it'll need > separate ops at least. There'd definitely be a lot of overlap though > so > it's worth looking at. Okay, thanks for discussion! I will try to make something that will fit in mainline kernel and I will learn more about the regulator subsystem in general so I can make a solution that fits. But I'll need some time to do that. I will for sure link to that discussion when I send the patch. Philippe ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC PATCH 2/2] ARM: dts: imx6ull: Add phy-supply to fec 2019-07-30 17:30 [RFC PATCH 0/2] Hello Philippe Schenker 2019-07-30 17:30 ` [RFC PATCH 1/2] Regulator: Core: Add clock-enable to fixed-regulator Philippe Schenker @ 2019-07-30 17:30 ` Philippe Schenker 1 sibling, 0 replies; 11+ messages in thread From: Philippe Schenker @ 2019-07-30 17:30 UTC (permalink / raw) To: Liam Girdwood, Mark Brown, linux-kernel; +Cc: Philippe Schenker From: Philippe Schenker <philippe.schenker@toradex.com> This adds the proper phy-supply to the fec. This supply is actually switched by a clock that is now properly stated. This adds the advantage to add a delay for that particular regulator that is needed. Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> --- arch/arm/boot/dts/imx6ull-colibri.dtsi | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/arch/arm/boot/dts/imx6ull-colibri.dtsi b/arch/arm/boot/dts/imx6ull-colibri.dtsi index 9ad1da159768..a15f0807e9b0 100644 --- a/arch/arm/boot/dts/imx6ull-colibri.dtsi +++ b/arch/arm/boot/dts/imx6ull-colibri.dtsi @@ -47,6 +47,17 @@ states = <1800000 0x1 3300000 0x0>; vin-supply = <®_module_3v3>; }; + + reg_eth_phy: regulator-eth-phy { + compatible = "regulator-fixed"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + regulator-name = "eth_phy"; + regulator-type = "voltage"; + vin-supply = <®_module_3v3>; + clocks = <&clks IMX6UL_CLK_ENET2_REF_125M>; + startup-delay-us = <150000>; + }; }; &adc1 { @@ -66,6 +77,7 @@ pinctrl-0 = <&pinctrl_enet2>; phy-mode = "rmii"; phy-handle = <ðphy1>; + phy-supply = <®_eth_phy>; status = "okay"; mdio { -- 2.22.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-08-07 6:46 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-30 17:30 [RFC PATCH 0/2] Hello Philippe Schenker 2019-07-30 17:30 ` [RFC PATCH 1/2] Regulator: Core: Add clock-enable to fixed-regulator Philippe Schenker 2019-07-30 18:10 ` Mark Brown 2019-07-30 21:00 ` Philippe Schenker 2019-07-31 21:23 ` Mark Brown 2019-08-05 11:07 ` Philippe Schenker 2019-08-05 16:37 ` Mark Brown 2019-08-06 12:57 ` Philippe Schenker 2019-08-06 18:26 ` Mark Brown 2019-08-07 6:46 ` Philippe Schenker 2019-07-30 17:30 ` [RFC PATCH 2/2] ARM: dts: imx6ull: Add phy-supply to fec Philippe Schenker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).