| Message ID | 1472216945-11818-10-git-send-email-jonathanh@nvidia.com |
|---|---|
| State | Accepted |
| Headers | show |
On 08/26/2016 07:09 AM, Jon Hunter wrote: > On Tegra124/132 the pins for I2C6 are shared with the Display Port AUX > (DPAUX) channel and on Tegra210 the pins for I2C4 and I2C6 are shared > with DPAUX1 and DPAUX0, respectively. The multiplexing of the pins is > handled by a register in the DPAUX and so the Tegra DPAUX driver has > been updated to register a pinctrl device for managing these pins. > > The pins for these particular I2C devices are bound to the I2C device > prior to probing. However, these I2C devices are in a different power > partition to the DPAUX devices that own the pins. Hence, it is desirable > to place the pins in the 'idle' state and allow the DPAUX power > partition to switch off, when these I2C devices is not in use. > Therefore, add calls to place the I2C pins in the 'default' and 'idle' > states when the I2C device is runtime resumed and suspended, > respectively. > > Please note that the pinctrl functions that set the state of the pins > check to see if the devices has pins associated and will return zero > if they do not. Therefore, it is safe to call these pinctrl functions > even for I2C devices that do not have any pins associated. I think this should be handled by drivers/i2c/muxes/i2c-mux-pinctrl.c instead? -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26/08/16 16:55, Stephen Warren wrote: > On 08/26/2016 07:09 AM, Jon Hunter wrote: >> On Tegra124/132 the pins for I2C6 are shared with the Display Port AUX >> (DPAUX) channel and on Tegra210 the pins for I2C4 and I2C6 are shared >> with DPAUX1 and DPAUX0, respectively. The multiplexing of the pins is >> handled by a register in the DPAUX and so the Tegra DPAUX driver has >> been updated to register a pinctrl device for managing these pins. >> >> The pins for these particular I2C devices are bound to the I2C device >> prior to probing. However, these I2C devices are in a different power >> partition to the DPAUX devices that own the pins. Hence, it is desirable >> to place the pins in the 'idle' state and allow the DPAUX power >> partition to switch off, when these I2C devices is not in use. >> Therefore, add calls to place the I2C pins in the 'default' and 'idle' >> states when the I2C device is runtime resumed and suspended, >> respectively. >> >> Please note that the pinctrl functions that set the state of the pins >> check to see if the devices has pins associated and will return zero >> if they do not. Therefore, it is safe to call these pinctrl functions >> even for I2C devices that do not have any pins associated. > > I think this should be handled by drivers/i2c/muxes/i2c-mux-pinctrl.c > instead? I remember having a look at i2c-mux some time back, but we did not have requirement to share the pins dynamically at runtime between the DPAUX and I2C devices. The pins are just configured at probe time for either the DPAUX or I2C device and then with this change when we are not active we can power down the pins. However, the pins are always bound to either the DPAUX or I2C. Cheers Jon
On 08/26/2016 10:38 AM, Jon Hunter wrote: > > On 26/08/16 16:55, Stephen Warren wrote: >> On 08/26/2016 07:09 AM, Jon Hunter wrote: >>> On Tegra124/132 the pins for I2C6 are shared with the Display Port AUX >>> (DPAUX) channel and on Tegra210 the pins for I2C4 and I2C6 are shared >>> with DPAUX1 and DPAUX0, respectively. The multiplexing of the pins is >>> handled by a register in the DPAUX and so the Tegra DPAUX driver has >>> been updated to register a pinctrl device for managing these pins. >>> >>> The pins for these particular I2C devices are bound to the I2C device >>> prior to probing. However, these I2C devices are in a different power >>> partition to the DPAUX devices that own the pins. Hence, it is desirable >>> to place the pins in the 'idle' state and allow the DPAUX power >>> partition to switch off, when these I2C devices is not in use. >>> Therefore, add calls to place the I2C pins in the 'default' and 'idle' >>> states when the I2C device is runtime resumed and suspended, >>> respectively. >>> >>> Please note that the pinctrl functions that set the state of the pins >>> check to see if the devices has pins associated and will return zero >>> if they do not. Therefore, it is safe to call these pinctrl functions >>> even for I2C devices that do not have any pins associated. >> >> I think this should be handled by drivers/i2c/muxes/i2c-mux-pinctrl.c >> instead? > > I remember having a look at i2c-mux some time back, but we did not have > requirement to share the pins dynamically at runtime between the DPAUX > and I2C devices. > > The pins are just configured at probe time for either the DPAUX or I2C > device and then with this change when we are not active we can power > down the pins. However, the pins are always bound to either the DPAUX or > I2C. Oh, so this isn't about 1 controller accessing 2 different physical buses at runtime by re-routing the SoC pinmux (which is the use-case i2c-mux-pinctrl handles), but simply about power-management. If the I2C controller didn't need to set a different pinctrl state during idle, I would say that all the pin muxing should be set up at system boot time, just like every other part of the pinmux is. In that case, the fact that the pins could be routed to 1 of 2 I2C controllers (DPAUX0 vs. I2C4) is irrelevant to the patch; the routing is a static thing anyway. Thinking some more, i2c-mux-pinctrl actually could handle this case, since it does define an idle pinmux state IIRC, or at least could be trivially extended to do so. That said, doing this directly in the I2C driver does seem better in this case, since the use-case is power about the power advantages for a single bus, rather than anything to do with multiple buses. Hence I'm OK with the concept of this patch. I didn't review the code though. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 26, 2016 at 3:09 PM, Jon Hunter <jonathanh@nvidia.com> wrote: > On Tegra124/132 the pins for I2C6 are shared with the Display Port AUX > (DPAUX) channel and on Tegra210 the pins for I2C4 and I2C6 are shared > with DPAUX1 and DPAUX0, respectively. The multiplexing of the pins is > handled by a register in the DPAUX and so the Tegra DPAUX driver has > been updated to register a pinctrl device for managing these pins. > > The pins for these particular I2C devices are bound to the I2C device > prior to probing. However, these I2C devices are in a different power > partition to the DPAUX devices that own the pins. Hence, it is desirable > to place the pins in the 'idle' state and allow the DPAUX power > partition to switch off, when these I2C devices is not in use. > Therefore, add calls to place the I2C pins in the 'default' and 'idle' > states when the I2C device is runtime resumed and suspended, > respectively. > > Please note that the pinctrl functions that set the state of the pins > check to see if the devices has pins associated and will return zero > if they do not. Therefore, it is safe to call these pinctrl functions > even for I2C devices that do not have any pins associated. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > Acked-by: Laxman Dewangan <ldewangan@nvidia.com> This is exactly how I imagined these states being used, so obviously: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 05e34dc29d5a..d86a993b75d6 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -28,6 +28,7 @@ #include <linux/of_device.h> #include <linux/module.h> #include <linux/reset.h> +#include <linux/pinctrl/consumer.h> #include <linux/pm_runtime.h> #include <asm/unaligned.h> @@ -407,6 +408,10 @@ static int tegra_i2c_runtime_resume(struct device *dev) struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); int ret; + ret = pinctrl_pm_select_default_state(i2c_dev->dev); + if (ret) + return ret; + if (!i2c_dev->hw->has_single_clk_source) { ret = clk_enable(i2c_dev->fast_clk); if (ret < 0) { @@ -435,7 +440,7 @@ static int tegra_i2c_runtime_suspend(struct device *dev) if (!i2c_dev->hw->has_single_clk_source) clk_disable(i2c_dev->fast_clk); - return 0; + return pinctrl_pm_select_idle_state(i2c_dev->dev); } static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)