[PATCH v2 0/2] ARM: imx6ul-14x14-evk: Fix suspend over nfs by phy
Right now attempting doing suspend/resume while root is mounted over NFS hangs on imx6ul-14x14-evk. This is happening because ksz8081 phy fixups are lost on resume. Fix this by using equivalent devicetree properties instead of a phy fixup and handling those properties on resume in the micrel driver. In theory it might now be possible to remove the phy fixup from mach-imx6ul entirely but it is possible that this would break other imx6ul boards which use the same phy. The solution would be to patch their dts but it's not clear how to identify affected boards. This code is shared with imx6ull-14x14-evk but 6ull suspend needs an unrelated patch: https://lkml.org/lkml/2017/5/30/584 This is something of a corner case so there is no CC: stable. Changes since v1: https://lkml.org/lkml/2017/5/30/672 * Split a kszphy_config_reset function for stuff shared between config_init and resume. Calling config_init directly could be an option but on some HW variants it does extra stuff like parsing devicetree options. That would not be appropriate for resume code. Leonard Crestez (2): ARM: dts: imx6ul-14x14-evk: Add ksz8081 phy properties net: phy: micrel: Restore led_mode and clk_sel on resume arch/arm/boot/dts/imx6ul-14x14-evk.dts | 6 + drivers/net/phy/micrel.c | 42 ++ 2 files changed, 34 insertions(+), 14 deletions(-) -- 2.7.4
Re: [PATCH 2/2] net: phy: micrel: Restore led_mode and clk_sel on resume
On Tue, 2017-05-30 at 15:19 -0700, Florian Fainelli wrote: > On 05/30/2017 03:08 PM, Leonard Crestez wrote: > > On Tue, 2017-05-30 at 11:05 -0700, Florian Fainelli wrote: > > > Should not we actually call kszphy_config_init() in order to restore > > > broadcast and nand disable bits as well? > > > > I don't know. In my case the B_CAST_OFF bit doesn't seem to be lost and > > NAND_TREE_ON is already off by the time it gets to linux. > > > > The bit that get lost seem to disappear just as the phy is resumed. I > > added some prints and they look like this: > > > > PM: early resume of devices complete after 6.534 msecs > > begin resume > > 0x1F=0x8190 0x16=0x202 > > after genphy_resume 0x1F=0x8100 0x16=0x202 > > end > > resume 0x1F=0x8190 0x16=0x202 > > OK, so it actually would not hurt to call config_init() again here, right? No, but if only some registers are lost then why reconfigure others? In particular it seems that only MII_KSZPHY_CTRL_2 is lost but not MII_KSZPHY_OMSO. However a few extra phy reads don't really matter. Calling config_init is the path of least resistance. Another problem is that the ksz9031 driver uses kszphy_resume but has a completely different init function. The bits I am concerned about do not seem to exist in hardware but it does something completely unrelated with parsing OF properties. Should I split this into ksz8021_resume and ksz9031_resume? It seems likely that more of the kszphy drivers need suspend/resume code but nobody got around to testing them. > > > If not, I would be more comfortable if we did create a specific function > > > that takes care of setting the reference clock and LED mode. > > > > Ok, I can add a function called kszphy_config_reset() with a comment > > explaining it's for bits lost on reset/resume. > > > > Or perhaps a better option would be to just save/restore the entire > > 0x1F register? > > Register 0 through 15 are standardized, but those after that are not, > and PHYs with a gazillion of registers already need to have a specific > set of suspend/resume sequence due to their proprietary indirect > register scheme, so we cannot quite come up with a generic function that > would cache all 16 or 32 registers and flat out restore those. > > Similarly, having phy_resume() systematically calling into config_init() > is a bit of a stretch and can be pretty inefficient. I'm not suggesting changing this at the phy core level. Just that maybe kszphy_resume specifically could save/restore register 0x1F instead of going through config logic again (which would involve extra reads and writes). However it seems that this has complications, for example on some versions the leds are written to a different register. It's not really worth optimizing to such an extent.
Re: [PATCH 1/2] ARM: dts: imx6ul-14x14-evk: Add ksz8081 phy properties
On Tue, 2017-05-30 at 11:10 -0700, Florian Fainelli wrote: > On 05/30/2017 10:34 AM, Leonard Crestez wrote: > > Right now mach-imx6ul registers a fixup for the ksz8081 phy. The same > > register values can be set through the micrel phy driver by using dts > > properties. > > > > This seems preferable and allows cleanly fixing suspend/resume. > > > > Signed-off-by: Leonard Crestez > > Should you have a Fixes: tag for that? Sounds like something you'd want > to backport to stable trees as well, no? I don't know if suspend over nfs ever worked on this board in upstream so there is no commit that this patch "Fixes: ". It would make sense to CC stable.
Re: [PATCH 2/2] net: phy: micrel: Restore led_mode and clk_sel on resume
On Tue, 2017-05-30 at 11:05 -0700, Florian Fainelli wrote:
> On 05/30/2017 10:34 AM, Leonard Crestez wrote:
> > These bits seem to be lost after a suspend/resume cycle so just set them
> > again.
> >
> > This patch fixes ethernet suspend/resume on imx6ul-14x14-evk boards.
> >
> > Signed-off-by: Leonard Crestez
> > ---
> > drivers/net/phy/micrel.c | 15 +++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> > index 6a5fd18..c53ee17 100644
> > --- a/drivers/net/phy/micrel.c
> > +++ b/drivers/net/phy/micrel.c
> > @@ -700,6 +700,9 @@ static int kszphy_suspend(struct phy_device *phydev)
> >
> > static int kszphy_resume(struct phy_device *phydev)
> > {
> > + struct kszphy_priv *priv = phydev->priv;
> > + int ret;
> > +
> > genphy_resume(phydev);
> >
> > /* Enable PHY Interrupts */
> > @@ -709,6 +712,18 @@ static int kszphy_resume(struct phy_device *phydev)
> > phydev->drv->config_intr(phydev);
> > }
> >
> > + if (priv->rmii_ref_clk_sel) {
> > + ret = kszphy_rmii_clk_sel(phydev, priv->rmii_ref_clk_sel_val);
> > + if (ret) {
> > + phydev_err(phydev,
> > + "failed to set rmii reference clock\n");
> > + return ret;
> > + }
> > + }
> > +
> > + if (priv->led_mode >= 0)
> > + kszphy_setup_led(phydev, priv->type->led_mode_reg,
> > priv->led_mode);
>
> Should not we actually call kszphy_config_init() in order to restore
> broadcast and nand disable bits as well?
I don't know. In my case the B_CAST_OFF bit doesn't seem to be lost and
NAND_TREE_ON is already off by the time it gets to linux.
The bit that get lost seem to disappear just as the phy is resumed. I
added some prints and they look like this:
PM: early resume of devices complete after 6.534 msecs
begin resume
0x1F=0x8190 0x16=0x202
after genphy_resume 0x1F=0x8100 0x16=0x202
end
resume 0x1F=0x8190 0x16=0x202
> If not, I would be more comfortable if we did create a specific function
> that takes care of setting the reference clock and LED mode.
Ok, I can add a function called kszphy_config_reset() with a comment
explaining it's for bits lost on reset/resume.
Or perhaps a better option would be to just save/restore the entire
0x1F register?
[PATCH 1/2] ARM: dts: imx6ul-14x14-evk: Add ksz8081 phy properties
Right now mach-imx6ul registers a fixup for the ksz8081 phy. The same
register values can be set through the micrel phy driver by using dts
properties.
This seems preferable and allows cleanly fixing suspend/resume.
Signed-off-by: Leonard Crestez
---
arch/arm/boot/dts/imx6ul-14x14-evk.dts | 6 ++
1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/imx6ul-14x14-evk.dts
b/arch/arm/boot/dts/imx6ul-14x14-evk.dts
index f18e1f1..d2be8aa 100644
--- a/arch/arm/boot/dts/imx6ul-14x14-evk.dts
+++ b/arch/arm/boot/dts/imx6ul-14x14-evk.dts
@@ -120,10 +120,16 @@
ethphy0: ethernet-phy@2 {
reg = <2>;
+ micrel,led-mode = <1>;
+ clocks = <&clks IMX6UL_CLK_ENET_REF>;
+ clock-names = "rmii-ref";
};
ethphy1: ethernet-phy@1 {
reg = <1>;
+ micrel,led-mode = <1>;
+ clocks = <&clks IMX6UL_CLK_ENET2_REF>;
+ clock-names = "rmii-ref";
};
};
};
--
2.7.4
[PATCH 0/2] ARM: imx6ul-14x14-evk: Fix suspend over nfs by phy cleanup
Right now attempting doing suspend/resume while root is mounted over NFS hangs on imx6ul-14x14-evk. This is happening because ksz8081 phy fixups are lost on resume. Fix this by using equivalent devicetree properties instead of a phy fixup and handling those properties on resume in the micrel driver. Other solutions would be possible such as having a way to run phy fixups again on resume. This alternative is precisely targeted. In theory it might now be possible to remove the phy fixup from mach-imx6ul entirely but it is possible that this would break other imx6ul boards which use the same phy. The solution would be to patch their dts but it's not clear how to identify affected boards. This code is shared with imx6ull-14x14-evk but 6ull suspend needs an unrelated patch: https://lkml.org/lkml/2017/5/30/584 Leonard Crestez (2): ARM: dts: imx6ul-14x14-evk: Add ksz8081 phy properties net: phy: micrel: Restore led_mode and clk_sel on resume arch/arm/boot/dts/imx6ul-14x14-evk.dts | 6 ++ drivers/net/phy/micrel.c | 15 +++ 2 files changed, 21 insertions(+) -- 2.7.4
[PATCH 2/2] net: phy: micrel: Restore led_mode and clk_sel on resume
These bits seem to be lost after a suspend/resume cycle so just set them
again.
This patch fixes ethernet suspend/resume on imx6ul-14x14-evk boards.
Signed-off-by: Leonard Crestez
---
drivers/net/phy/micrel.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 6a5fd18..c53ee17 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -700,6 +700,9 @@ static int kszphy_suspend(struct phy_device *phydev)
static int kszphy_resume(struct phy_device *phydev)
{
+ struct kszphy_priv *priv = phydev->priv;
+ int ret;
+
genphy_resume(phydev);
/* Enable PHY Interrupts */
@@ -709,6 +712,18 @@ static int kszphy_resume(struct phy_device *phydev)
phydev->drv->config_intr(phydev);
}
+ if (priv->rmii_ref_clk_sel) {
+ ret = kszphy_rmii_clk_sel(phydev, priv->rmii_ref_clk_sel_val);
+ if (ret) {
+ phydev_err(phydev,
+ "failed to set rmii reference clock\n");
+ return ret;
+ }
+ }
+
+ if (priv->led_mode >= 0)
+ kszphy_setup_led(phydev, priv->type->led_mode_reg,
priv->led_mode);
+
return 0;
}
--
2.7.4
[PATCH] ARM: imx6ull: Make suspend/resume work like on 6ul
Suspend and resume on imx6ull is currenty not working because of some
missed checks where behavior should match imx6ul.
Signed-off-by: Leonard Crestez
---
arch/arm/mach-imx/mxc.h | 6 ++
arch/arm/mach-imx/pm-imx6.c | 6 --
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-imx/mxc.h b/arch/arm/mach-imx/mxc.h
index 34f2ff6..e00d626 100644
--- a/arch/arm/mach-imx/mxc.h
+++ b/arch/arm/mach-imx/mxc.h
@@ -39,6 +39,7 @@
#define MXC_CPU_IMX6SX 0x62
#define MXC_CPU_IMX6Q 0x63
#define MXC_CPU_IMX6UL 0x64
+#define MXC_CPU_IMX6ULL0x65
#define MXC_CPU_IMX7D 0x72
#define IMX_DDR_TYPE_LPDDR21
@@ -73,6 +74,11 @@ static inline bool cpu_is_imx6ul(void)
return __mxc_cpu_type == MXC_CPU_IMX6UL;
}
+static inline bool cpu_is_imx6ull(void)
+{
+ return __mxc_cpu_type == MXC_CPU_IMX6ULL;
+}
+
static inline bool cpu_is_imx6q(void)
{
return __mxc_cpu_type == MXC_CPU_IMX6Q;
diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
index e61b1d1..ecdf071 100644
--- a/arch/arm/mach-imx/pm-imx6.c
+++ b/arch/arm/mach-imx/pm-imx6.c
@@ -295,7 +295,8 @@ int imx6_set_lpm(enum mxc_cpu_pwr_mode mode)
val &= ~BM_CLPCR_SBYOS;
if (cpu_is_imx6sl())
val |= BM_CLPCR_BYPASS_PMIC_READY;
- if (cpu_is_imx6sl() || cpu_is_imx6sx() || cpu_is_imx6ul())
+ if (cpu_is_imx6sl() || cpu_is_imx6sx() || cpu_is_imx6ul() ||
+ cpu_is_imx6ull())
val |= BM_CLPCR_BYP_MMDC_CH0_LPM_HS;
else
val |= BM_CLPCR_BYP_MMDC_CH1_LPM_HS;
@@ -312,7 +313,8 @@ int imx6_set_lpm(enum mxc_cpu_pwr_mode mode)
val |= BM_CLPCR_SBYOS;
if (cpu_is_imx6sl() || cpu_is_imx6sx())
val |= BM_CLPCR_BYPASS_PMIC_READY;
- if (cpu_is_imx6sl() || cpu_is_imx6sx() || cpu_is_imx6ul())
+ if (cpu_is_imx6sl() || cpu_is_imx6sx() || cpu_is_imx6ul() ||
+ cpu_is_imx6ull())
val |= BM_CLPCR_BYP_MMDC_CH0_LPM_HS;
else
val |= BM_CLPCR_BYP_MMDC_CH1_LPM_HS;
--
2.7.4
[PATCH] cpufreq: imx6q: imx6ull should use the same flow as imx6ul
From: Octavian Purdila
This fixes an issue with imx6ull where setting the frequency to 528Mhz
would actually set the ARM clock to 324Mhz.
Signed-off-by: Octavian Purdila
Signed-off-by: Leonard Crestez
---
drivers/cpufreq/imx6q-cpufreq.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 9c13f09..b6edd3c 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -101,7 +101,8 @@ static int imx6q_set_target(struct cpufreq_policy *policy,
unsigned int index)
* - Reprogram pll1_sys_clk and reparent pll1_sw_clk back to it
* - Disable pll2_pfd2_396m_clk
*/
- if (of_machine_is_compatible("fsl,imx6ul")) {
+ if (of_machine_is_compatible("fsl,imx6ul") ||
+ of_machine_is_compatible("fsl,imx6ull")) {
/*
* When changing pll1_sw_clk's parent to pll1_sys_clk,
* CPU may run at higher than 528MHz, this will lead to
@@ -215,7 +216,8 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
goto put_clk;
}
- if (of_machine_is_compatible("fsl,imx6ul")) {
+ if (of_machine_is_compatible("fsl,imx6ul") ||
+ of_machine_is_compatible("fsl,imx6ull")) {
pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
if (IS_ERR(pll2_bus_clk) || IS_ERR(secondary_sel_clk)) {
--
2.7.4
Re: [PATCH] ARM: imx_v6_v7_defconfig: Explicitly restore CONFIG_DEBUG_FS
On Fri, 2017-05-26 at 08:42 -0700, Paul E. McKenney wrote:
> On Fri, May 26, 2017 at 02:26:06PM +0300, Leonard Crestez wrote:
> >
> > This option was removed by "make savedefconfig" in
> > commit c5054a98bce4 ("ARM: imx_v6_v7_defconfig: Select SMSC_PHY")
> >
> > This happened because CONFIG_DEBUG_FS was implicitly selected by
> > CONFIG_TREE_RCU_TRACE which defaulted to true because CONFIG_RCU_TRACE
> > was enabled by default by commit 961518259b3b ("rcu: Enable RCU
> > tracepoints by default to aid in debugging")
> >
> > Recently however CONFIG_RCU_TRACE was completely removed by
> > commit 6e74c237c410 ("rcu: Remove debugfs tracing")
> CONFIG_RCU_TRACE is still very much alive in its new home at
> kernel/rcu/Kconfig.debug:
Sorry, what was removed is the dependency on DEBUG_FS. In particular
this snippet:
diff --git a/init/Kconfig b/init/Kconfig
index 2aa14ff..3025383 100644
--
- a/init/Kconfig
+++ b/init/Kconfig
@@ -659,14 +659,6 @@ config
RCU_FAST_NO_HZ
Say N if you are unsure.
-config
TREE_RCU_TRACE
- def_bool RCU_TRACE && ( TREE_RCU || PREEMPT_RCU )
- select DEBUG_FS
- help
- This option provides
tracing for the TREE_RCU and
- PREEMPT_RCU implementations,
permitting Makefile to
- trivially select
kernel/rcutree_trace.c.
-
config RCU_BOOST
bool "Enable RCU
priority boosting"
depends on RT_MUTEXES && PREEMPT_RCU &&
RCU_EXPERT
> config RCU_TRACE
> bool "Enable tracing for RCU"
> depends on DEBUG_KERNEL
> default y if TREE_RCU
> select TRACE_CLOCK
> help
> This option enables additional tracepoints for ftrace-style
> event tracing.
>
> Say Y here if you want to enable RCU tracing
> Say N if you are unsure.
>
> That said, I need to make it default to "y" if PREEMPT_RCU as well as
> the current TREE_RCU. Would that help?
I don't think you can help unless you want to make RCU_TRACE depend on
DEBUG_FS for no reason. The proper fix is to have DEBUG_FS inside the
imx_v6_v7_defconfig.
My problem is just an unfortunate accident with default dependencies
and overzealous savedefconfig.
As far as I understand after recent changes RCU_TRACE is now only
useful with tracepoints. Shouldn't it depend on TRACEPOINTS in some
way? It's still possible to compile with RCU_TRACE on and TRACEPOINTS
off, is this intentional? It seems that when this happens you're just
relying on tracepoint macros to expand to nothing.
Fr example maybe RCU_TRACE should be "default y if (TREE_RCU &&
TRACEPOINTS); depends on TRACEPOINTS"?
In theory you could also "select TRACEPOINTS" but then you would end up
enabling TRACEPOINTS by default in all configurations that use TREE_RCU
but don't explicitly disable RCU_TRACE.
I'd say that if $SUBSYSTEM_DEBUG depends on $MAJOR_DEBUG_FEATURE it
should try to avoid selecting $MAJOR_DEBUG_FEATURE by default if it's
not otherwise enabled. As far as I can tell this is how RCU ended up
indirectly pulling in DEBUG_FS and later unexpectedly removed it.
Or I could be completely wrong, kconfig can be very confusing.
--
Regards,
Leonard
[PATCH] ARM: imx_v6_v7_defconfig: Explicitly restore CONFIG_DEBUG_FS
This option was removed by "make savedefconfig" in
commit c5054a98bce4 ("ARM: imx_v6_v7_defconfig: Select SMSC_PHY")
This happened because CONFIG_DEBUG_FS was implicitly selected by
CONFIG_TREE_RCU_TRACE which defaulted to true because CONFIG_RCU_TRACE
was enabled by default by commit 961518259b3b ("rcu: Enable RCU
tracepoints by default to aid in debugging")
Recently however CONFIG_RCU_TRACE was completely removed by
commit 6e74c237c410 ("rcu: Remove debugfs tracing")
The result is that imx_v6_v7_defconfig no longer includes DEBUG_FS on
linux-next since next-20170517. This is bad, DEBUG_FS is extremely
useful for kernel introspection and testing.
Signed-off-by: Leonard Crestez
---
Patch is against next-20170526. Applying it to shawnguo/imx/defconfig
and cycling via savedefconfig makes this diff go away.
Alternatively maybe DEBUG_FS itself could be made "default y if DEBUG_KERNEL"
or similar?
arch/arm/configs/imx_v6_v7_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/imx_v6_v7_defconfig
b/arch/arm/configs/imx_v6_v7_defconfig
index bf1e7e3..51ca814 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -381,6 +381,7 @@ CONFIG_NLS_ISO8859_1=y
CONFIG_NLS_ISO8859_15=m
CONFIG_NLS_UTF8=y
CONFIG_PRINTK_TIME=y
+CONFIG_DEBUG_FS=y
CONFIG_MAGIC_SYSRQ=y
# CONFIG_SCHED_DEBUG is not set
CONFIG_PROVE_LOCKING=y
--
2.7.4
Re: [PATCH 1/1] spi: imx: fix issue when tx_buf or rx_buf is NULL
On Thu, 2017-05-18 at 03:01 -0700, jiada_w...@mentor.com wrote: > From: Jiada Wang > > In case either transfer->tx_buf or transfer->rx_buf is NULL, > manipulation of buffer in spi_imx_u32_swap_u[8|16]() will cause > NULL pointer dereference crash. > > Add buffer check at very beginning of spi_imx_u32_swap_u[8|16](), > to avoid such crash. > > Signed-off-by: Jiada Wang > Reported-by: Leonard Crestez Tested-by: Leonard Crestez
Re: [PATCH linux-next v2 1/1] spi: imx: dynamic burst length adjust for PIO mode
On Wed, 2017-05-17 at 18:50 -0700, Jiada Wang wrote:
> Hello Leonard
>
> Thanks for the report, can you help to check if the following change
> address the issue?
>
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 782045f..19b30cf 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -288,6 +288,9 @@ static void spi_imx_u32_swap_u8(struct spi_transfer
> *transfer, u32 *buf)
> {
> int i;
>
> + if (!buf)
> + return;
> +
> for (i = 0; i < transfer->len / 4; i++)
> *(buf + i) = cpu_to_be32(*(buf + i));
> }
> @@ -296,6 +299,9 @@ static void spi_imx_u32_swap_u16(struct spi_transfer
> *transfer, u32 *buf)
> {
> int i;
>
> + if (!buf)
> + return;
> +
> for (i = 0; i < transfer->len / 4; i++) {
> u16 *temp = (u16 *)buf;
>
Yes, this does seem to fix it.
Re: [PATCH linux-next v2 1/1] spi: imx: dynamic burst length adjust for PIO mode
On Mon, 2017-05-01 at 03:31 -0700, jiada_w...@mentor.com wrote: > From: Jiada Wang > > previously burst length (BURST_LENGTH) is always set to equal > to bits_per_word, causes a 10us gap between each word in > transfer, which significantly affects performance. > > This patch uses 32 bits transfer to simulate lower bits transfer, > and adjusts burst length runtimely to use biggeest burst length > as possible to reduce the gaps in transfer for PIO mode. > > Signed-off-by: Jiada Wang > --- > drivers/spi/spi-imx.c | 157 > +++--- > 1 file changed, 149 insertions(+), 8 deletions(-) This patch made it's way to linux-next and broke boot on imx6dl- sabreauto and imx6sl-evk boards (but others work). The crashes look like this: [1.442384] spi_imx 2008000.ecspi: dma setup error -19, use pio [1.452930] Unable to handle kernel NULL pointer dereference at virtual address [1.461320] pgd = c0004000 [1.464078] [] *pgd= [1.467821] Internal error: Oops: 5 [#1] SMP ARM [1.472464] Modules linked in: [1.475558] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.12.0-rc1-1-g8d4a6ca #90 [1.483234] Hardware name: Freescale i.MX6 SoloLite (Device Tree) [1.489349] task: ef058000 task.stack: ef054000 [1.493926] PC is at spi_imx_transfer+0x2d8/0x364 [1.498659] LR is at spi_bitbang_transfer_one+0x80/0xa0 [1.503910] pc : []lr : []psr: 2013 [1.503910] sp : ef0558b0 ip : ef0558e0 fp : ef0558dc [1.515412] r10: ee938a98 r9 : ef340c28 r8 : ee938800 [1.520658] r7 : ee938800 r6 : ef340800 r5 : ef055aac r4 : ef340ce0 [1.527206] r3 : 0001 r2 : fffc r1 : r0 : ee938800 [1.533757] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none [1.540913] Control: 10c5387d Table: 8000404a DAC: 0051 [1.546680] Process swapper/0 (pid: 1, stack limit = 0xef054210) [1.552709] Stack: (0xef0558b0 to 0xef056000) [1.557093] 58a0: c05f5cfc c05f587c ef055aac [1.565301] 58c0: ef340800 ef340ce0 ee938800 ef340c28 ef055904 ef0558e0 c05f3440 c05f5f98 [1.573509] 58e0: ee938800 ef055aac ef340800 ef055a38 ef340c28 ef055944 ef055908 [1.581714] 5900: c05f21c0 c05f33cc ee938800 ef05592c ef055920 c0496c3c [1.589920] 5920: ef340800 ef055a38 ee938a98 ef055984 ef055948 [1.598126] 5940: c05f28c8 c05f2118 ef055a38 ef340800 ef05596c ef340a80 c016d518 ef055990 [1.606332] 5960: ee938800 ef055a38 ef340800 ef340c28 ee938a98 ef055a14 ef055988 [1.614537] 5980: c05f2bf0 c05f24e0 ef340ac4 6013 dead4ead [1.622743] 59a0: c1669b70 c0be1d7c ef0559b4 ef0559b4 [1.630948] 59c0: dead4ead c1669b70 c0be1d7c [1.639153] 59e0: ef0559b4 ef0559b4 ee938800 ee938800 ef055a38 ef055a38 0006 c1669b3c [1.647359] 5a00: ee938800 ef055b27 ef055a2c ef055a18 c05f2c34 c05f2a30 ef0c8bc1 ef0c8bc0 [1.655565] 5a20: ef055b14 ef055a30 c05f2d20 c05f2c10 ef055a54 ef055b4a ef055aa4 ef055ae0 [1.663770] 5a40: ee938800 c05f0640 ef055990 0007 0001 ff8d ef055a5c [1.671975] 5a60: ef055a5c ef055a68 ef055a68 ef0c8bc0 0001 [1.680179] 5a80: 0802 [1.688384] 5aa0: 01312d00 ef055ae0 ef055a38 ef0c8bc1 0006 [1.696588] 5ac0: 0810 01312d00 [1.704794] 5ae0: ef055a38 ef055aa4 ef058000 c05c298c ee938800 ee938a70 ee938800 ef341018 [1.712999] 5b00: ef7e6f0c c0a669a4 ef055b3c ef055b18 c05c29c4 c05f2c58 0006 [1.721206] 5b20: ef055b4c 9f055b30 c05c298c c0a669a4 ef055b74 ef055b40 c05d4970 c05c2998 [1.729412] 5b40: ef055b5c ef055b50 c016d518 c016d320 ef055b74 ef341018 c0a669a4 ee938a70 [1.737617] 5b60: ee938800 ef055bb4 ef055b78 c05d5634 c05d4954 ef341010 ef341050 [1.745822] 5b80: ef341018 ef341010 ee938a70 ef341018 [1.754027] 5ba0: ef055be4 ef055bb8 c05c2f88 c05d5300 c0e3ed44 [1.762232] 5bc0: ee938800 c0e3ed34 c0e3ed44 ef055c04 ef055be8 [1.770437] 5be0: c05f0070 c05c2e78 ee938800 c166767c c0e3ed44 ef055c2c ef055c08 [1.778643] 5c00: c053ed28 c05efff8 c0e3ed44 ef055c78 ee938800 0001 c1667638 [1.786848] 5c20: ef055c4c ef055c30 c053ef1c c053eacc ef055c78 c053ee7c 0001 [1.795055] 5c40: ef055c74 ef055c50 c053d050 c053ee88 ef0c0ed4 ee8adfd4 ee938800 ee938800 [1.803261] 5c60: ee938834 c0e403a4 ef055c9c ef055c78 c053e998 c053cff0 ee938800 0001 [1.811466] 5c80: ee938808 ee938800 c0e403a4 ef055cac ef055ca0 c053ef9c c05
[PATCH v2] drm/imx: imx-ldb: Accept drm_of_find_panel_or_bridge failure
Not having an endpoint bound in DT should not cause a failure here,
there are fallbacks. So explicitly accept a missing endpoint.
This behavior change was introduced by refactoring in drm_of parsing
code and it should not require dts changes.
In particular this fixes imx6qdl-sabreauto boards.
Link: https://lists.freedesktop.org/archives/dri-devel/2017-May/141233.html
Fixes: ebc944613567 ("drm: convert drivers to use drm_of_find_panel_or_bridge")
Signed-off-by: Leonard Crestez
---
This relies on drm_of_find_panel_or_bridge returning -ENODEV
specifically if no remote is found for the endpoint. This behavior can
be seen by looking at the code but is not otherwise obviously
guaranteed.
Perhaps this should be explicitly mentioned in that function's
documentation?
Changes since v1:
* Fix returning if ret is 0.
drivers/gpu/drm/imx/imx-ldb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 8fb801f..8b05ecb 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -673,7 +673,7 @@ static int imx_ldb_bind(struct device *dev, struct device
*master, void *data)
ret = drm_of_find_panel_or_bridge(child,
imx_ldb->lvds_mux ? 4 : 2, 0,
&channel->panel,
&channel->bridge);
- if (ret)
+ if (ret && ret != -ENODEV)
return ret;
/* panel ddc only if there is no bridge */
--
2.7.4
[PATCH] drm/imx: imx-ldb: Accept drm_of_find_panel_or_bridge failure
Not having an endpoint bound in DT should not cause a failure here,
there are fallbacks. So let's explicitly accept a missing endpoint.
This behavior change was introduced by refactoring in drm_of parsing
code and it should not require dts changes.
In particular this fixes imx6qdl-sabreauto boards.
Link: https://lists.freedesktop.org/archives/dri-devel/2017-May/141233.html
Fixes: ebc944613567 ("drm: convert drivers to use drm_of_find_panel_or_bridge")
Signed-off-by: Leonard Crestez
---
This relies on drm_of_find_panel_or_bridge returning -ENODEV
specifically if no remote is found for the endpoint. This behavior can
be seen by looking at the code but is not otherwise obviously
guaranteed.
Perhaps this should be explicitly mentioned in that function's
documentation?
drivers/gpu/drm/imx/imx-ldb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 8fb801f..4c8a521 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -673,7 +673,7 @@ static int imx_ldb_bind(struct device *dev, struct device
*master, void *data)
ret = drm_of_find_panel_or_bridge(child,
imx_ldb->lvds_mux ? 4 : 2, 0,
&channel->panel,
&channel->bridge);
- if (ret)
+ if (ret != -ENODEV)
return ret;
/* panel ddc only if there is no bridge */
--
2.7.4
Re: [PATCH v3 4/5] drm: convert drivers to use drm_of_find_panel_or_bridge
On Wed, Mar 22, 2017 at 5:01 PM, Philipp Zabel wrote: > On Wed, 2017-03-22 at 08:26 -0500, Rob Herring wrote: > > > > Similar to the previous commit, convert drivers open coding OF graph > > parsing to use drm_of_find_panel_or_bridge instead. > > > > This changes some error messages to debug messages (in the graph core). > > Graph connections are often "no connects" depending on the particular > > board, so we want to avoid spurious messages. Plus the kernel is not a > > DT validator. > > > > Signed-off-by: Rob Herring > > Reviewed-by: Archit Taneja > Tested-by: Philipp Zabel > (imx-ldb on i.MX6) It seems that this breaks on (at least) imx6qdl-sabreauto. The relevant section of the boot log looks like this: imx-drm display-subsystem: bound imx-ipuv3-crtc.2 (ops ipu_crtc_ops) imx-drm display-subsystem: bound imx-ipuv3-crtc.3 (ops ipu_crtc_ops) dwhdmi-imx 12.hdmi: Detected HDMI TX controller v1.31a with HDCP (DWC HDMI 3D TX PHY) dwhdmi-imx 12.hdmi: registered DesignWare HDMI I2C bus driver imx-drm display-subsystem: bound 12.hdmi (ops dw_hdmi_imx_ops) imx-drm display-subsystem: failed to bind 200.aips-bus:ldb (ops imx_ldb_ops): -19 imx-drm display-subsystem: master bind failed: -19 It seems that imx6qdl-sabreauto does not have any panel defined in dts. This used to be ignored when of_graph_get_endpoint_by_regs returned NULL but now drm_of_find_panel_or_bridge returns -ENODEV and this causes imx_ldb_bind to fail altogether. Defining a panel works (including showing stuff on a LVDS panel). Ignoring -ENODEV also fixes this: --- drivers/gpu/drm/imx/imx-ldb.c +++ drivers/gpu/drm/imx/imx-ldb.c @@ -673,7 +673,7 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data) ret = drm_of_find_panel_or_bridge(child, imx_ldb->lvds_mux ? 4 : 2, 0, &channel->panel, &channel->bridge); - if (ret) + if (ret != -ENODEV) return ret; /* panel ddc only if there is no bridge */ I don't know much about drm and it's not clear if failing to find a panel should be an error here or not and the hack above is likely the wrong way to handle it anyway. I was bisecting the fact that suspend now breaks on upstream. The fact that a probe error later breaks suspend is possibly an unrelated issue, right? -- Regards, Leonard
[PATCH v2] ARM: dts: imx6sx-sdb: Remove OPP override
The board file for imx6sx-sdb overrides cpufreq operating points to use
higher voltages. This is done because the board has a shared rail for
VDD_ARM_IN and VDD_SOC_IN and when using LDO bypass the shared voltage
needs to be a value suitable for both ARM and SOC.
This only applies to LDO bypass mode, a feature not present in upstream.
When LDOs are enabled the effect is to use higher voltages than necessary
for no good reason.
Setting these higher voltages can make some boards fail to boot with ugly
semi-random crashes reminiscent of memory corruption. These failures only
happen on board rev. C, rev. B is reported to still work.
Signed-off-by: Leonard Crestez
CC: sta...@vger.kernel.org
---
It is not known exactly why setting these higher voltages causes crashes.
Maybe this means that it's not appropriate to CC stable?
Removing this override is a correct change anyway because these OPP
overrides do not serve a purpose without ldo bypass.
Changes since v1:
- Adjusted commit message
- Link: https://lkml.org/lkml/2017/4/25/636
arch/arm/boot/dts/imx6sx-sdb.dts | 17 -
1 file changed, 17 deletions(-)
diff --git a/arch/arm/boot/dts/imx6sx-sdb.dts b/arch/arm/boot/dts/imx6sx-sdb.dts
index 5bb8fd5..d71da30 100644
--- a/arch/arm/boot/dts/imx6sx-sdb.dts
+++ b/arch/arm/boot/dts/imx6sx-sdb.dts
@@ -12,23 +12,6 @@
model = "Freescale i.MX6 SoloX SDB RevB Board";
};
-&cpu0 {
- operating-points = <
- /* kHzuV */
- 996000 125
- 792000 1175000
- 396000 1175000
- 198000 1175000
- >;
- fsl,soc-operating-points = <
- /* ARM kHz SOC uV */
- 996000 125
- 792000 1175000
- 396000 1175000
- 198000 1175000
- >;
-};
-
&i2c1 {
clock-frequency = <10>;
pinctrl-names = "default";
--
2.7.4
Re: [PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override
On Fri, 2017-05-05 at 09:18 +0800, Shawn Guo wrote: > On Thu, May 04, 2017 at 04:34:14PM +0200, Marek Vasut wrote: > > If you model the > > power distribution correctly, the OPP hackery can be removed. > The OPP hackery can be removed even without reg_arm/reg_soc modeling. > That's why we can do hackery dropping and reg_arm/reg_soc modeling in > separate patches. > > @Leonard, if someday we support 'LDO bypass' mode in upstream kernel, > the OPP hackery needs to be back in some way even with reg_arm/reg_soc > modeling in place, right? Or will we have a better way to ensure SW1A > rail can always feed a correct voltage directly to reg_arm®_soc? Maybe? Or maybe the cpufreq driver could detect this situation and handle it internally. In the vendor tree this the cpufreq driver has a special fsl,arm-soc-shared property for this anyway. I posted another RFC at upstreaming ldo-bypass recently but it did not handle this particular case of a shared input rail. It can be handled separately. See: https://lkml.org/lkml/2017/3/22/640 But getting that series to an acceptable state might take a long time. -- Regards, Leonard
Re: [PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override
On Wed, 2017-05-03 at 21:33 +0200, Marek Vasut wrote: > On 05/03/2017 07:58 PM, Leonard Crestez wrote: > > On Wed, 2017-05-03 at 17:59 +0200, Marek Vasut wrote: > > > On 05/03/2017 04:58 PM, Leonard Crestez wrote: > > > > On Wed, 2017-05-03 at 16:26 +0200, Marek Vasut wrote: > > > > > 2) It actually fixes a problem with the voltage rails such that the > > > > > DVFS > > > > > works without leaving the system in unstable or dead state. You do > > > > > need the second part of my patch if you drop the OPP hackery, > > > > > without > > > > > it the power framework cannot correctly configure the core > > > > > voltages, > > > > > so the patch from Leonard makes things worse. > > > > No, I think there is a misunderstanding here. The second part of your > > > > patch will cause cpufreq poking at LDOs to indirectly adjust the input > > > > from the PMIC to the minimum required (this is LDO target + > > > > min_dropout_uv). Without it by default VDD_ARM_SOC_IN will remain fixed > > > > as 1375mV from boot. > > > Who sets / guarantees that default value for ARM and SOC rails ? > > I think it's from the PMIC hardware itself (but maybe uboot plays with > > it). VDD_ARM_SOC_IN on this board is tied to SW1AB from MMPF0200: > > > > http://www.nxp.com/assets/documents/data/en/data-sheets/MMPF0200.pdf > > > > It seems reasonable to rely on such voltages set externally. > Isn't it an established rule that Linux should not depend on bootloader > settings ? Or did that change ? I don't actually know. Is there a hard and fast rule about this, even when it comes to voltages? In theory it is possible for a bootloader to set a low cpu frequency and low voltage and then have the chip fail when the cpufreq driver attempts to go higher. Setting vin-supply on reg_arm/reg_soc would fix that. > Well the regulator(s) cannot be correctly configured if the kernel > doesn't have the correct power distribution described in the DT . It depends on your definition of "correctness". It it certainly possible to get a functional system while only partially describing regulator relationships. I think there is a further misunderstanding here. I have a problem where imx6sx-sdb rev C boards crash on boot with upstream (but are reported to work fine with rev B). Removing the OPP overrides fixes this specific issue. I don't object to the second part of your patch, setting correct supply links is a good thing for various reasons. It is just not necessary for fixing the concrete crash mentioned above (and I tested this). It should probably go in a separate patch. It might seem a pedantic difference but it's good to accurately describe the effect of patches in commit messages. For example it might help somebody looking to backport various fixes. -- Regards, Leonard
Re: [PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override
On Wed, 2017-05-03 at 17:59 +0200, Marek Vasut wrote: > On 05/03/2017 04:58 PM, Leonard Crestez wrote: > > On Wed, 2017-05-03 at 16:26 +0200, Marek Vasut wrote: > > > 2) It actually fixes a problem with the voltage rails such that the DVFS > > > works without leaving the system in unstable or dead state. You do > > > need the second part of my patch if you drop the OPP hackery, without > > > it the power framework cannot correctly configure the core voltages, > > > so the patch from Leonard makes things worse. > > No, I think there is a misunderstanding here. The second part of your > > patch will cause cpufreq poking at LDOs to indirectly adjust the input > > from the PMIC to the minimum required (this is LDO target + > > min_dropout_uv). Without it by default VDD_ARM_SOC_IN will remain fixed > > as 1375mV from boot. > Who sets / guarantees that default value for ARM and SOC rails ? I think it's from the PMIC hardware itself (but maybe uboot plays with it). VDD_ARM_SOC_IN on this board is tied to SW1AB from MMPF0200: http://www.nxp.com/assets/documents/data/en/data-sheets/MMPF0200.pdf It seems reasonable to rely on such voltages set externally. > With the OPP override in place, there's at least the guarantee that both > rails will have the same voltage requirement. If you remove the OPP > override without modeling the actual regulator wiring, the guarantee is > gone. The imx6sx chip has internal LDO_ARM and LDO_SOC regulators which can generate separate voltages for arm/soc. The fact that these regulators share the same supply is only an issue when they are set in bypass mode. However the boot issues happen on REV C but apparently not REV B of the board. I don't have a good explanation for this so maybe I am missing something. -- Regards, Leonard
Re: [PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override
On Wed, 2017-05-03 at 16:26 +0200, Marek Vasut wrote: > On 05/03/2017 03:57 PM, Shawn Guo wrote: > > > > On Tue, Apr 25, 2017 at 07:28:06PM +0200, Marek Vasut wrote: > > > > > > On 04/25/2017 07:23 PM, Leonard Crestez wrote: > > > > > > > > Anyway, that version also sets the supply for reg_arm and reg_soc. It > > > > is not necessary for fixing the crash I'm seeing but is good because it > > > > will result in the minimum voltage on VDD_ARM_SOC_IN rather than a fix > > > > 1375mv. I tested Marek's patch and it works fine on my rev B board > > > > (which otherwise fails to boot upstream). > > > Oh that's nice , thanks ! I don't have SDB and I hacked it up after a > > > brief discussion with Fabio without even compile-testing it, thus RFC. > > > Glad to hear it works and thanks for testing it ! Can you add a formal > > > Tested-by please ? > > Hi Marek, > Hi Shawn, > > > Thanks for your patch. But I prefer Leonard's version because: 1) it > > has a better commit log; 2) it sticks to one-patch-does-one-thing > > policy. > 2) It actually fixes a problem with the voltage rails such that the DVFS > works without leaving the system in unstable or dead state. You do > need the second part of my patch if you drop the OPP hackery, without > it the power framework cannot correctly configure the core voltages, > so the patch from Leonard makes things worse. No, I think there is a misunderstanding here. The second part of your patch will cause cpufreq poking at LDOs to indirectly adjust the input from the PMIC to the minimum required (this is LDO target + min_dropout_uv). Without it by default VDD_ARM_SOC_IN will remain fixed as 1375mV from boot. That default value should be high enough for all cpufreq settings. Setting the LDO parent will cause this voltage to be dynamically reduced when possible (at low frequencies). This is not required for proper operation, it is just an optimization to do more of the regulation in the PMIC instead. It should work just fine without the second part. That OPP override exists for "LDO bypass" mode, a feature not present in upstream. In that mode the internal regulators are set to bypass mode and voltage is controlled directly from the PMIC. Since VDD_ARM and VDD_SOC have different minimum requirements but are joined on the board the OPP voltages are adjusted to max(VDD_ARM, VDD_SOC). If LDOs are enabled there is no good reason to do this. I don't care which patch goes in but the effect of the patch should be clarified. -- Regards, Leonard
[PATCH] ARM: imx_v6_v7_defconfig: Enable cpufreq governors
Enable more common cpufreq governors in imx defconfig because this is very useful for testing. In particular you can't use cpufreq-set -f $FREQ without explicitly defining CONFIG_CPU_FREQ_GOV_USERSPACE=y. Signed-off-by: Leonard Crestez --- It might make sense for all governors to be enabled by default from drivers/cpufreq/Kconfig and allow defconfigs to be shorter. Right now the descriptions for some of them includes a line that says "If in doubt, say Y" but the config options don't have actually have a default value defined and they effectively default to N. Cycling via savedefconfig on shawnguo/for-next also generates some reordering for some newly added options CONFIG_TOUCHSCREEN_MAX11801=y and CONFIG_HID_MULTITOUCH=y. Those were not included but it's strange that this happens. Maybe those options were inserted manually, or otherwise there is an annoying bug in kconfig? arch/arm/configs/imx_v6_v7_defconfig | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig index bb6fa56..bf1e7e3 100644 --- a/arch/arm/configs/imx_v6_v7_defconfig +++ b/arch/arm/configs/imx_v6_v7_defconfig @@ -55,6 +55,9 @@ CONFIG_CMDLINE="noinitrd console=ttymxc0,115200" CONFIG_KEXEC=y CONFIG_CPU_FREQ=y CONFIG_CPU_FREQ_DEFAULT_GOV_ONDEMAND=y +CONFIG_CPU_FREQ_GOV_POWERSAVE=y +CONFIG_CPU_FREQ_GOV_USERSPACE=y +CONFIG_CPU_FREQ_GOV_CONSERVATIVE=y CONFIG_ARM_IMX6Q_CPUFREQ=y CONFIG_CPU_IDLE=y CONFIG_VFP=y -- 2.7.4
Re: [PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override
On Tue, 2017-04-25 at 14:02 -0300, Fabio Estevam wrote:
> On Tue, Apr 25, 2017 at 2:02 PM, Fabio Estevam wrote:
> >
> > Hi Leonard,
> >
> > On Tue, Apr 25, 2017 at 1:57 PM, Leonard Crestez
> > wrote:
> > >
> > > The board file for imx6sx-dbg overrides cpufreq operating points to use
> > > higher voltages. This is done because the board has a shared rail for
> > > VDD_ARM_IN and VDD_SOC_IN and when using LDO bypass the shared voltage
> > > needs to be a value suitable for both ARM and SOC.
> > >
> > > This was introduced in:
> > >
> > > commit 54183bd7f766 ("ARM: imx6sx-sdb: add revb board and make it
> > > default")
> > >
> > > This only only applies to LDO bypass mode, a feature not present in
> > > upstream. When LDOs are enabled the effect is to use higher voltages than
> > > necesarry for no good reason.
> > >
> > > Setting these higher voltages can make some boards fail to boot with ugly
> > > semi-random crashes, reminiscent of memory corruption. These failures
> > > happen the first time the lowest idle state is used. Remove the OPP
> > > override in order to fix those crashes.
> > >
> > > Signed-off-by: Leonard Crestez
> > >
> > > ---
> > > It's not clear exactly why the crashes happen. Perhaps waking up from idle
> > > draws more power than is available? Removing this override is a correct
> > > change anyway so maybe there is no need to investigate deeper.
> > Marek just sent a similar one a few minutes ago:
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/503230.html
> Forgot to add Marek.
Wow, that was literally 15 minutes before my patch. In my defense I did
search the archives before starting to format the patch but it had not
arrived yet.
Anyway, that version also sets the supply for reg_arm and reg_soc. It
is not necessary for fixing the crash I'm seeing but is good because it
will result in the minimum voltage on VDD_ARM_SOC_IN rather than a fix
1375mv. I tested Marek's patch and it works fine on my rev B board
(which otherwise fails to boot upstream).
--
Regards,
Leonard
[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override
The board file for imx6sx-dbg overrides cpufreq operating points to use
higher voltages. This is done because the board has a shared rail for
VDD_ARM_IN and VDD_SOC_IN and when using LDO bypass the shared voltage
needs to be a value suitable for both ARM and SOC.
This was introduced in:
commit 54183bd7f766 ("ARM: imx6sx-sdb: add revb board and make it default")
This only only applies to LDO bypass mode, a feature not present in
upstream. When LDOs are enabled the effect is to use higher voltages than
necesarry for no good reason.
Setting these higher voltages can make some boards fail to boot with ugly
semi-random crashes, reminiscent of memory corruption. These failures
happen the first time the lowest idle state is used. Remove the OPP
override in order to fix those crashes.
Signed-off-by: Leonard Crestez
---
It's not clear exactly why the crashes happen. Perhaps waking up from idle
draws more power than is available? Removing this override is a correct
change anyway so maybe there is no need to investigate deeper.
arch/arm/boot/dts/imx6sx-sdb.dts | 17 -
1 file changed, 17 deletions(-)
diff --git a/arch/arm/boot/dts/imx6sx-sdb.dts b/arch/arm/boot/dts/imx6sx-sdb.dts
index 5bb8fd5..d71da30 100644
--- a/arch/arm/boot/dts/imx6sx-sdb.dts
+++ b/arch/arm/boot/dts/imx6sx-sdb.dts
@@ -12,23 +12,6 @@
model = "Freescale i.MX6 SoloX SDB RevB Board";
};
-&cpu0 {
- operating-points = <
- /* kHzuV */
- 996000 125
- 792000 1175000
- 396000 1175000
- 198000 1175000
- >;
- fsl,soc-operating-points = <
- /* ARM kHz SOC uV */
- 996000 125
- 792000 1175000
- 396000 1175000
- 198000 1175000
- >;
-};
-
&i2c1 {
clock-frequency = <10>;
pinctrl-names = "default";
--
2.7.4
Re: [PATCH v2 0/4] ARM: imx: Set LDO regulator supply
On Tue, 2017-04-04 at 20:04 +0300, Leonard Crestez wrote:
> Setting the LDO regulator parent is optional but beneficial. It will cause
> the PMIC output voltage to be dynamically set to the minimum input for the
> LDOs, this should be more efficient.
>
> This propagation was introduced by:
> commit fc42112c0eaa ("regulator: core: Propagate voltage changes to supply
> regulators")
>
> Changes since v1:
> * Drop patch 1 since it only avoids logging a warning and the gpc driver
> is going through more changes.
> * Initialize cpufreq->suspend_freq based on policy->max instead.
> * Remove reference to ldo-bypass from suspend_freq patch message.
Hello,
This is a gentle reminder that this was sent ~2 weeks ago and patches 1
and 2 are still waiting (with no objections). Patches 3/4 were applied.
--
Regards,
Leonard
Re: [RFC 4/8] regulator: core: Check enabling bypass respects constraints
On Fri, 2017-04-07 at 12:22 +0100, Mark Brown wrote: > On Fri, Apr 07, 2017 at 01:51:52PM +0300, Leonard Crestez wrote: > > It currently seems to work how I expect but from your statement it's > > not clear if it's entirely intentional. > The current behaviour of bypassed regulators is intentional. I did not mean to imply that there is something wrong with bypassed regulators. I just wanted more information about how regulators (non- bypassed) pick their voltage when consumers allow a range. After some more reading through the code it seems that the driver itself receives the range (either through set_voltage or map_voltage) and gets to make the choice. So it seems fine for my concerns, sorry to bother you. -- Regards, Leonard
Re: [RFC PATCH 3/3] cpufreq: imx6q: refine clk operations
On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
> +static int num_clks;
> +static struct clk_bulk_data clks[] = {
> + { .id = "arm" },
> + { .id = "pll1_sys" },
> + { .id = "step" },
> + { .id = "pll1_sw" },
> + { .id = "pll2_pfd2_396m" },
> + { .id = "pll2_bus" },
> + { .id = "secondary_sel" },
> +};
The .id is only required for initialization, it seems strange to keep
it around runtime data. It might be better for this API to work with an
array of clk* and separate array of names (or clk_bulk_init_data if we
need flags). Variable references would be shorter and it would allow
more data to be const.
> -put_clk:
> - if (!IS_ERR(arm_clk))
> - clk_put(arm_clk);
> - if (!IS_ERR(pll1_sys_clk))
> - clk_put(pll1_sys_clk);
> - if (!IS_ERR(pll1_sw_clk))
> - clk_put(pll1_sw_clk);
> - if (!IS_ERR(step_clk))
> - clk_put(step_clk);
> - if (!IS_ERR(pll2_pfd2_396m_clk))
> - clk_put(pll2_pfd2_396m_clk);
> - if (!IS_ERR(pll2_bus_clk))
> - clk_put(pll2_bus_clk);
> - if (!IS_ERR(secondary_sel_clk))
> - clk_put(secondary_sel_clk);
> +
> + clk_bulk_put(num_clks, clks);
> +put_node:
> of_node_put(np);
> +
> return ret;
> }
My subjective opinion is that a better way to clean this up would be to
have a single imx6q_cpufreq_clean function that takes all resources and
does stuff like:
if (!IS_ERR(clk)) clk_put(clk);
clk = NULL;
That function can be called from both _remove and failed _probe without
having to keep track of which resources have been allocated until then.
Just free and NULL all clocks/regulators and simplify control flow.
--
Regards,
Leonard
Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories
On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
> +/**
> + * clk_bulk_enable - ungate a bulk of clocks
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table being ungated
> + *
> + * clk_bulk_enable must not sleep
> + * Returns 0 on success, -EERROR otherwise.
> + */
> +int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
> +{
> + int ret;
> + int i;
> +
> + for (i = 0; i < num_clks; i++) {
> + ret = clk_enable(clks[i].clk);
> + if (ret) {
> + pr_err("Failed to enable clk '%s': %d\n",
> + clks[i].id, ret);
> + goto err;
> + }
> + }
> +
> + return 0;
> +
> +err:
> + while (--i >= 0)
> + clk_put(clks[i].clk);
Shouldn't this be clk_disable?
And you can probably use clk_bulk_disable(i, clks) instead
Re: [PATCH v2 2/4] cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend
On Tue, 2017-04-11 at 12:07 +0530, Viresh Kumar wrote: > On 04-04-17, 20:04, Leonard Crestez wrote: > > If the cpufreq driver tries to modify voltage/freq during suspend/resume > > it might need to control an external PMIC via I2C or SPI but those > > devices might be already suspended. This issue is likely to happen > > whenever the LDOs have their vin-supply set. > > > > To avoid this scenario we just increase cpufreq to the maximum before > > suspend. > > > > Signed-off-by: Leonard Crestez > > --- > > drivers/cpufreq/imx6q-cpufreq.c | 8 +++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > I have already Acked this earlier, why didn't you add it ? Because v2 is different based on comments from Lucas. It didn't seem appropriate to keep an old "Acked-by" in this case.
Re: [RFC 4/8] regulator: core: Check enabling bypass respects constraints
On Thu, 2017-04-06 at 19:52 +0100, Mark Brown wrote:
> On Tue, Mar 28, 2017 at 10:49:55PM +0300, Leonard Crestez wrote:
> > On Tue, 2017-03-28 at 17:47 +0100, Mark Brown wrote:
> > > To repeat what I said previously the whole point of bypassing is to not
> > > do regulation and generally the constraints in the unregulated idle case
> > > are substantially more relaxed. This would break use cases relying on
> > > the existing behaviour which wouldn't expect to affect the parent
> > > voltage at all, either stopping things working or making them less
> > > efficient by needlessly regulating the voltage down which defeats the
> > > main point of bypassing.
> > So what you want is to prevent voltage changes unless strictly
> > required, even lowering? What I want is to get the minimum voltage in
> > the SOC because that's where power is being consumed.
> So your end goal here is to bypass a regulator which is forced into your
> system design by being integrated into the SoC which isn't able to
> regulate down to a low enough voltage for your use case? I guess one
> question is if you need to use the regulator at all?
Both the PMIC and the LDOs can provide the required voltage range. LDO
bypass mode is a design tradeoff: the PMIC provides more efficient
conversion but with more fluctuations.
My question was about how the regulator framework handles constraints
and consumer voltage requests. What I want is for the output to be set
to the minimum value that meets all constraints. Maybe other users
would prefer regulators to keep their output as much as possible?
This is relevant even if LDOs are enabled, it's still preferable to
have PMIC output as low as possible because then less of the voltage
reduction is performed in the LDO.
It currently seems to work how I expect but from your statement it's
not clear if it's entirely intentional.
> > It's not at all obvious that in bypass mode the output constraints of a
> > regulator need not be respected by the core. I expected the opposite,
> > this is something that should be documented.
> SubmittingPatches... Bear in mind that most regulators are fixed
> voltage in a given system so bypass would be very difficult to use if it
> tried to pass the constraints upstream.
> > But if the bypassed regulator has a downstream consumer then it's
> > requirements should definitely still be met in bypass mode, right? I
> > could set my maximum voltage directly from cpufreq in that case.
> What we're interpreting bypass mode as meaning is "stop regulating".
> There will still be some limits but we're expecting them to be enforced
> in the extremes of the constraints in the parent regulators.
> > Or should a bypassed regulator ignore all other requests? One of the
> > behaviors that this patch series relies on is that calling set_voltage
> > on a bypassed regulator propagates this request to the supply and picks
> > the minimum voltage there. An alternative implementation would be to
> Yes, the expectation is that if anything is being changed it won't have
> any effect until regulation is reenabled but we're not particularly
> expecting much activity on bypassed regulators.
OK, so it is up to consumers to ensure the supply voltage is acceptable
before allowing bypass. If they want to do something clever they should
register as a consumer to the supply as well.
> >
> > call set_voltage directly on the supply regulator by changing the
> > "{arm,soc,pu}-supply" references in DT to point to the PMIC instead.
> > Would that be better?
> That's more what's expected here.
Ok, I will try that approach instead.
> > Both approaches work. Relying on propagation feels like it is the
> > "right way" to handle this, even if it's harder to get right and the
> > regulator core does not entirely support it. But it's possible that
> > this is based on a misunderstanding of what "bypass" is actually
> > supposed to do.
> Another option would be to add a regulator configuration which allowed
> the regulator to transparently go into bypass mode if the parent could
> do things directly, only enabling regulation if the parent couldn't
> support. That would mean you'd loose the supply cleanup function which
> is typically part of why there are LDOs in the system but it sounds like
> you're OK with that in at least your use case.
Dynamic enabling of bypass mode in the core seems very complex and not
terribly useful for my case. I pretty much want them always in bypass
or always enabled.
Re: [PATCH v3 33/37] mtd: nand: allocate aligned buffers if NAND_OWN_BUFFERS is unset
On Thu, Mar 30, 2017 at 11:15 AM, Masahiro Yamada
wrote:
>
> Some NAND controllers are using DMA engine requiring a specific
> buffer alignment. The core provides no guarantee on the nand_buffers
> pointers, which forces some drivers to allocate their own buffers
> and pass the NAND_OWN_BUFFERS flag.
>
> Rework the nand_buffers allocation logic to allocate each buffer
> independently. This should make most NAND controllers/DMA engine
> happy, and allow us to get rid of these custom buf allocation in
> NAND controller drivers.
>
> Signed-off-by: Masahiro Yamada
> @@ -4914,8 +4930,12 @@ void nand_cleanup(struct nand_chip *chip)
> > /* Free bad block table memory */
> kfree(chip->bbt);
> - if (!(chip->options & NAND_OWN_BUFFERS))
> + if (!(chip->options & NAND_OWN_BUFFERS)) {
> + kfree(chip->buffers->databuf);
> + kfree(chip->buffers->ecccode);
> + kfree(chip->buffers->ecccalc);
> kfree(chip->buffers);
> + }
It seems that chip->buffers might not be allocated at this point, for
example if nand_cleanup is called during a failed probe. You should
check if (chip->buffers != NULL) before freeing stuff inside it.
When attempting to run linux-next on various imx6qdl-sabreauto boards
they now panic on boot. This happens because they have nand chips in
devicetree which are not physically populated on the board. This
normally fails in nand_scan_ident but now crashes later in
nand_cleanup.
--
Regards,
Leonard
[PATCH v2 2/4] cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend
If the cpufreq driver tries to modify voltage/freq during suspend/resume
it might need to control an external PMIC via I2C or SPI but those
devices might be already suspended. This issue is likely to happen
whenever the LDOs have their vin-supply set.
To avoid this scenario we just increase cpufreq to the maximum before
suspend.
Signed-off-by: Leonard Crestez
---
drivers/cpufreq/imx6q-cpufreq.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index be90ee3..786122e 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -161,8 +161,13 @@ static int imx6q_set_target(struct cpufreq_policy *policy,
unsigned int index)
static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
{
+ int ret;
+
policy->clk = arm_clk;
- return cpufreq_generic_init(policy, freq_table, transition_latency);
+ ret = cpufreq_generic_init(policy, freq_table, transition_latency);
+ policy->suspend_freq = policy->max;
+
+ return ret;
}
static struct cpufreq_driver imx6q_cpufreq_driver = {
@@ -173,6 +178,7 @@ static struct cpufreq_driver imx6q_cpufreq_driver = {
.init = imx6q_cpufreq_init,
.name = "imx6q-cpufreq",
.attr = cpufreq_generic_attr,
+ .suspend = cpufreq_generic_suspend,
};
static int imx6q_cpufreq_probe(struct platform_device *pdev)
--
2.7.4
[PATCH v2 3/4] ARM: dts: imx6qdl-sabresd: Set LDO regulator supply
Setting the supply is optional but beneficial, it will cause PMIC
voltages to be dynamically changed with cpu frequency.
Signed-off-by: Leonard Crestez
Reviewed-by: Lucas Stach
---
arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 12
1 file changed, 12 insertions(+)
diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index 63bf95e..58055ce 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -548,6 +548,18 @@
status = "okay";
};
+®_arm {
+ vin-supply = <&sw1a_reg>;
+};
+
+®_pu {
+ vin-supply = <&sw1c_reg>;
+};
+
+®_soc {
+ vin-supply = <&sw1c_reg>;
+};
+
&snvs_poweroff {
status = "okay";
};
--
2.7.4
Re: [PATCH 3/5] cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend
On Tue, 2017-04-04 at 11:51 +0200, Lucas Stach wrote:
> Am Freitag, den 31.03.2017, 22:25 +0300 schrieb Leonard Crestez:
> >
> > If the cpufreq driver tries to modify voltage/freq during suspend/resume
> > it might need to control an external PMIC via I2C or SPI but those
> > devices might be already suspended.
> >
> > To avoid this scenario we just increase cpufreq to highest setpoint
> > before suspend. This issue can easily be triggered by ldo-bypass but in
> > theory any regulator set_voltage call can end up having to modify
> > external supply voltages.
> >
> > Signed-off-by: Leonard Crestez
> > Acked-by: Viresh Kumar
> > ---
> > drivers/cpufreq/imx6q-cpufreq.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/cpufreq/imx6q-cpufreq.c
> > b/drivers/cpufreq/imx6q-cpufreq.c
> > index be90ee3..e2c1fbf 100644
> > --- a/drivers/cpufreq/imx6q-cpufreq.c
> > +++ b/drivers/cpufreq/imx6q-cpufreq.c
> > @@ -162,6 +162,7 @@ static int imx6q_set_target(struct cpufreq_policy
> > *policy, unsigned int index)
> > static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
> > {
> > policy->clk = arm_clk;
> > + policy->suspend_freq = freq_table[soc_opp_count - 1].frequency;
> I think soc_opp_count includes all OPPs from the DT, some of which might
> be disabled based on the fuse settings of the SoC. So this should
> probably not switch to the highest OPP unconditionally, but rather
> switch to the highest _enabled_ OPP.
You're right, this does not appear to be correct. Looking at
soc_opp_count it should probably be a local variable in the code
initializing imx6_soc_volt. And the imx6_soc_volt array itself could
now be replaced with opp's support for multiple supply voltages.
I'll post v2.
[PATCH v2 4/4] ARM: dts: imx6qp-sabresd: Set reg_arm regulator supply
On imx6qp-sabresd LDO_ARM is connected to a different PMIC output than
the other imx6qdl-sabresd boards.
Setting cpu0 arm-supply to sw2_reg is wrong, this must have mistakenly
slipped out of the vendor tree where this is are used for LDO bypass.
Signed-off-by: Leonard Crestez
Reviewed-by: Lucas Stach
---
arch/arm/boot/dts/imx6qp-sabresd.dts | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/imx6qp-sabresd.dts
b/arch/arm/boot/dts/imx6qp-sabresd.dts
index b234580..a8a5004 100644
--- a/arch/arm/boot/dts/imx6qp-sabresd.dts
+++ b/arch/arm/boot/dts/imx6qp-sabresd.dts
@@ -50,8 +50,8 @@
compatible = "fsl,imx6qp-sabresd", "fsl,imx6qp";
};
-&cpu0 {
- arm-supply = <&sw2_reg>;
+®_arm {
+ vin-supply = <&sw2_reg>;
};
&iomuxc {
--
2.7.4
[PATCH v2 1/4] cpufreq: imx6q: Fix handling EPROBE_DEFER from regulator
From: Irina Tirdea
If there are any errors in getting the cpu0 regulators, the driver returns
-ENOENT. In case the regulators are not yet available, the devm_regulator_get
calls will return -EPROBE_DEFER, so that the driver can be probed later.
If we return -ENOENT, the driver will fail its initialization and will
not try to probe again (when the regulators become available).
Return the actual error received from regulator_get in probe. Print a
differentiated message in case we need to probe the device later and
in case we actually failed. Also add a message to inform when the
driver has been successfully registered.
Signed-off-by: Irina Tirdea
Signed-off-by: Leonard Crestez
Reviewed-by: Lucas Stach
Acked-by: Viresh Kumar
---
drivers/cpufreq/imx6q-cpufreq.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 7719b02..be90ee3 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -222,6 +222,13 @@ static int imx6q_cpufreq_probe(struct platform_device
*pdev)
arm_reg = regulator_get(cpu_dev, "arm");
pu_reg = regulator_get_optional(cpu_dev, "pu");
soc_reg = regulator_get(cpu_dev, "soc");
+ if (PTR_ERR(arm_reg) == -EPROBE_DEFER ||
+ PTR_ERR(soc_reg) == -EPROBE_DEFER ||
+ PTR_ERR(pu_reg) == -EPROBE_DEFER) {
+ ret = -EPROBE_DEFER;
+ dev_dbg(cpu_dev, "regulators not ready, defer\n");
+ goto put_reg;
+ }
if (IS_ERR(arm_reg) || IS_ERR(soc_reg)) {
dev_err(cpu_dev, "failed to get regulators\n");
ret = -ENOENT;
--
2.7.4
[PATCH v2 0/4] ARM: imx: Set LDO regulator supply
Setting the LDO regulator parent is optional but beneficial. It will cause
the PMIC output voltage to be dynamically set to the minimum input for the
LDOs, this should be more efficient.
This propagation was introduced by:
commit fc42112c0eaa ("regulator: core: Propagate voltage changes to supply
regulators")
Changes since v1:
* Drop patch 1 since it only avoids logging a warning and the gpc driver
is going through more changes.
* Initialize cpufreq->suspend_freq based on policy->max instead.
* Remove reference to ldo-bypass from suspend_freq patch message.
It's not clear if policy->cpuinfo.max would be preferable, at init time
they should be identical.
Link: https://lkml.org/lkml/2017/3/31/683
Irina Tirdea (1):
cpufreq: imx6q: Fix handling EPROBE_DEFER from regulator
Leonard Crestez (3):
cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend
ARM: dts: imx6qdl-sabresd: Set LDO regulator supply
ARM: dts: imx6qp-sabresd: Set reg_arm regulator supply
arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 12
arch/arm/boot/dts/imx6qp-sabresd.dts | 4 ++--
drivers/cpufreq/imx6q-cpufreq.c| 15 ++-
3 files changed, 28 insertions(+), 3 deletions(-)
--
2.7.4
Re: [PATCH 1/5] ARM: imx: gpc: Do not print error message for EPROBE_DEFER
On Tue, 2017-04-04 at 11:46 +0200, Lucas Stach wrote: > Am Freitag, den 31.03.2017, 22:25 +0300 schrieb Leonard Crestez: > > > > The pu regulator request will return -EPROBE_DEFER if it has a supply > > from PMIC and this supply is not yet registered. This does not represent > > an error since the driver will call probe again later, so only print a > > warning message in this case. > > > > Signed-off-by: Irina Tirdea > > Signed-off-by: Leonard Crestez > > --- > > arch/arm/mach-imx/gpc.c | 6 +- > The driver moved places, together with a large rewrite, to > drivers/soc/imx/. > > This issue isn't present in the new driver, so this patch can just be > dropped. Wait, I'm confused. In what tree did that happen? This patch is against linus's tree as of last week and there is nothing in drivers/soc/imx. I saw patches for a new gpcv2 driver but that seems to be only for imx7. This patch is for imx6. Will that driver also support imx6? Link: https://lkml.org/lkml/2017/3/28/834 -- Regards, Leonard
[PATCH 0/5] ARM: imx: Set LDO regulator supply
Setting the LDO regulator parent is optional but beneficial. It will cause
the PMIC output voltage to be dynamically set to the minimum input for the
LDOs, this should be more efficient.
This propagation was introduced by:
commit fc42112c0eaa ("regulator: core: Propagate voltage changes to supply
regulators")
In this case it is desirable that the parent regulator sets the minimum
voltage for it's consumers. It's not clear if this behavior from the
regulator core is entirely intentional. If it's not then perhaps it should
be made configurable through DT somehow?
This makes probe order more complicated, fix that in patches 1 and 2.
It can also break suspend because cpufreq voltage switches can end up
talking to a PMIC via I2C which is already suspended. This is fixed in
patch 3.
It's good to have these issues fixed upstream because they might affect
other complex configurations.
These changes are required for LDO bypass but they are also useful
standalone. Here's a link to the that other discussion:
https://lkml.org/lkml/2017/3/22/640
Irina Tirdea (1):
cpufreq: imx6q: Fix handling EPROBE_DEFER from regulator
Leonard Crestez (4):
ARM: imx: gpc: Do not print error message for EPROBE_DEFER
cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend
ARM: dts: imx6qdl-sabresd: Set LDO regulator supply
ARM: dts: imx6qp-sabresd: Set reg_arm regulator supply
arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 12
arch/arm/boot/dts/imx6qp-sabresd.dts | 4 ++--
arch/arm/mach-imx/gpc.c| 6 +-
drivers/cpufreq/imx6q-cpufreq.c| 9 +
4 files changed, 28 insertions(+), 3 deletions(-)
--
2.7.4
[PATCH 2/5] cpufreq: imx6q: Fix handling EPROBE_DEFER from regulator
From: Irina Tirdea
If there are any errors in getting the cpu0 regulators, the driver returns
-ENOENT. In case the regulators are not yet available, the devm_regulator_get
calls will return -EPROBE_DEFER, so that the driver can be probed later.
If we return -ENOENT, the driver will fail its initialization and will
not try to probe again (when the regulators become available).
Return the actual error received from regulator_get in probe. Print a
differentiated message in case we need to probe the device later and
in case we actually failed. Also add a message to inform when the
driver has been successfully registered.
Signed-off-by: Irina Tirdea
Signed-off-by: Leonard Crestez
Acked-by: Viresh Kumar
---
drivers/cpufreq/imx6q-cpufreq.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 7719b02..be90ee3 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -222,6 +222,13 @@ static int imx6q_cpufreq_probe(struct platform_device
*pdev)
arm_reg = regulator_get(cpu_dev, "arm");
pu_reg = regulator_get_optional(cpu_dev, "pu");
soc_reg = regulator_get(cpu_dev, "soc");
+ if (PTR_ERR(arm_reg) == -EPROBE_DEFER ||
+ PTR_ERR(soc_reg) == -EPROBE_DEFER ||
+ PTR_ERR(pu_reg) == -EPROBE_DEFER) {
+ ret = -EPROBE_DEFER;
+ dev_dbg(cpu_dev, "regulators not ready, defer\n");
+ goto put_reg;
+ }
if (IS_ERR(arm_reg) || IS_ERR(soc_reg)) {
dev_err(cpu_dev, "failed to get regulators\n");
ret = -ENOENT;
--
2.7.4
[PATCH 1/5] ARM: imx: gpc: Do not print error message for EPROBE_DEFER
The pu regulator request will return -EPROBE_DEFER if it has a supply
from PMIC and this supply is not yet registered. This does not represent
an error since the driver will call probe again later, so only print a
warning message in this case.
Signed-off-by: Irina Tirdea
Signed-off-by: Leonard Crestez
---
arch/arm/mach-imx/gpc.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
index 1dc2a34..ce64d11 100644
--- a/arch/arm/mach-imx/gpc.c
+++ b/arch/arm/mach-imx/gpc.c
@@ -466,7 +466,11 @@ static int imx_gpc_probe(struct platform_device *pdev)
pu_reg = NULL;
if (IS_ERR(pu_reg)) {
ret = PTR_ERR(pu_reg);
- dev_err(&pdev->dev, "failed to get pu regulator: %d\n", ret);
+ if (ret == -EPROBE_DEFER)
+ dev_dbg(&pdev->dev, "pu regulator not ready, retry\n");
+ else
+ dev_err(&pdev->dev, "failed to get pu regulator: %d\n",
+ ret);
return ret;
}
--
2.7.4
[PATCH 5/5] ARM: dts: imx6qp-sabresd: Set reg_arm regulator supply
On imx6qp-sabresd LDO_ARM is connected to a different PMIC output than
the other imx6qdl-sabresd boards.
Setting cpu0 arm-supply to sw2_reg is wrong, this must have mistakenly
slipped out of the vendor tree where this is are used for LDO bypass.
Signed-off-by: Leonard Crestez
---
arch/arm/boot/dts/imx6qp-sabresd.dts | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/imx6qp-sabresd.dts
b/arch/arm/boot/dts/imx6qp-sabresd.dts
index b234580..a8a5004 100644
--- a/arch/arm/boot/dts/imx6qp-sabresd.dts
+++ b/arch/arm/boot/dts/imx6qp-sabresd.dts
@@ -50,8 +50,8 @@
compatible = "fsl,imx6qp-sabresd", "fsl,imx6qp";
};
-&cpu0 {
- arm-supply = <&sw2_reg>;
+®_arm {
+ vin-supply = <&sw2_reg>;
};
&iomuxc {
--
2.7.4
[PATCH 3/5] cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend
If the cpufreq driver tries to modify voltage/freq during suspend/resume
it might need to control an external PMIC via I2C or SPI but those
devices might be already suspended.
To avoid this scenario we just increase cpufreq to highest setpoint
before suspend. This issue can easily be triggered by ldo-bypass but in
theory any regulator set_voltage call can end up having to modify
external supply voltages.
Signed-off-by: Leonard Crestez
Acked-by: Viresh Kumar
---
drivers/cpufreq/imx6q-cpufreq.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index be90ee3..e2c1fbf 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -162,6 +162,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy,
unsigned int index)
static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
{
policy->clk = arm_clk;
+ policy->suspend_freq = freq_table[soc_opp_count - 1].frequency;
return cpufreq_generic_init(policy, freq_table, transition_latency);
}
@@ -173,6 +174,7 @@ static struct cpufreq_driver imx6q_cpufreq_driver = {
.init = imx6q_cpufreq_init,
.name = "imx6q-cpufreq",
.attr = cpufreq_generic_attr,
+ .suspend = cpufreq_generic_suspend,
};
static int imx6q_cpufreq_probe(struct platform_device *pdev)
--
2.7.4
[PATCH 4/5] ARM: dts: imx6qdl-sabresd: Set LDO regulator supply
Setting the supply is optional but beneficial, it will cause PMIC
voltages to be dynamically changed with cpu frequency.
Signed-off-by: Leonard Crestez
---
arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 12
1 file changed, 12 insertions(+)
diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index 63bf95e..58055ce 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -548,6 +548,18 @@
status = "okay";
};
+®_arm {
+ vin-supply = <&sw1a_reg>;
+};
+
+®_pu {
+ vin-supply = <&sw1c_reg>;
+};
+
+®_soc {
+ vin-supply = <&sw1c_reg>;
+};
+
&snvs_poweroff {
status = "okay";
};
--
2.7.4
Re: linux-next: Tree for Mar 30
On Thu, 2017-03-30 at 16:55 +1100, Stephen Rothwell wrote:
> Hi all,
>
> Changes since 20170329:
>
> Undropped tree: xen-tip
>
> The vfs tree gained a conflict against Linus' tree.
>
> The drm tree gained conflicts against the drm-intel-fixes tree.
>
> The mailbox tree lost its build failure.
>
> The phy-next tree gained a build failure, so I used the version from
> next-20170329.
>
> The vhost tree gained a build failure, so I used the version from
> next-20170329.
>
> Non-merge commits (relative to Linus' tree): 5728
> 6122 files changed, 441716 insertions(+), 108564 deletions(-)
>
>
>
> I have created today's linux-next tree at
> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> (patches at http://www.kernel.org/pub/linux/kernel/next/ ). If you
> are tracking the linux-next tree using git, you should not use "git pull"
> to do so as that will try to merge the new linux-next release with the
> old one. You should use "git fetch" and checkout or reset to the new
> master.
>
> You can see which trees have been included by looking in the Next/Trees
> file in the source. There are also quilt-import.log and merge.log
> files in the Next directory. Between each merge, the tree was built
> with a ppc64_defconfig for powerpc and an allmodconfig (with
> CONFIG_BUILD_DOCSRC=n) for x86_64, a multi_v7_defconfig for arm and a
> native build of tools/perf. After the final fixups (if any), I do an
> x86_64 modules_install followed by builds for x86_64 allnoconfig,
> powerpc allnoconfig (32 and 64 bit), ppc44x_defconfig, allyesconfig
> and pseries_le_defconfig and i386, sparc and sparc64 defconfig.
>
> Below is a summary of the state of the merge.
>
> I am currently merging 255 trees (counting Linus' and 37 trees of bug
> fix patches pending for the current merge release).
>
> Stats about the size of the tree over time can be seen at
> http://neuling.org/linux-next-size.html .
>
> Status of my local build tests will be at
> http://kisskb.ellerman.id.au/linux-next . If maintainers want to give
> advice about cross compilers/configs that work, we are always open to add
> more builds.
It seems there is a build error for imx_v6_v7_defconfig.
Link: http://kisskb.ellerman.id.au/kisskb/buildresult/12980252/
Error:
drivers/built-in.o: In function `vblank_disable_and_save':
imx-ocotp.c:(.text+0xb45e8): undefined reference to `__bad_cmpxchg'
Makefile:986: recipe for target 'vmlinux' failed
This seems to be caused by something from drm:
commit 43dc7fe2b211 ("drm: Mark up accesses of vblank->enabled outside of its
spinlock")
It introduces a cmpxchg on a bool which is apparently not supported on
ARM v6 (it only supports 32-bit values). The error looks legit to me
(not caused by an infrastructure bug).
It seems the kbuild test robot also found a similar breakage on tilegx:
https://lists.freedesktop.org/archives/dri-devel/2017-March/137322.html
--
Regards,
Leonard
Re: [RFC 8/8] ARM: dts: imx6qdl-sabresd: Enable fsl,ldo-bypass
On Wed, 2017-03-22 at 18:13 +0100, Lucas Stach wrote: > Am Mittwoch, den 22.03.2017, 18:53 +0200 schrieb Leonard Crestez: > > This enables LDO bypass by default on the imx6qdl-sabresd boards. New > > dts files with -ldo suffix are added for users who want to run with LDOs > > enabled on these boards anyway. > Given that using LDO bypass affects the device lifetime negatively (see > AN4724), I think the default should still be to use LDO enabled mode and > have new DTs for people that desire to shorten the lifetime of the SoC > for a minimal drop in power consumption. This was based on what the Freescale BSP does, I don't particularly object to upstream having a different default. It would be nice for testing if this ldo-bypass path was enabled by default for some boards but it's not a very good reason. I will switch the default. -- Regards, Leonard
[PATCH v2] ARM: imx_v6_v7_defconfig: Select SMSC_PHY
The imx6sl-evk board has a LAN8720A ethernet phy supported by SMSC_PHY.
Add this driver to the default imx config since the device is present on
one of the evaluation boards.
This used to work mostly fine with the generic phy driver until
commit 0878fff1f42c ("net: phy: Do not perform software reset for
Generic PHY"). The fact that soft reset is no longer performed
apparently causes RX to sometimes failes which can cause netboot to
timeout on DHCP. This is eventually retried and it works after link
up/down but can takes 90 seconds to reach the login prompt.
This was generated with "make savedefconfig" and it includes a few
additional minor cleanups.
Signed-off-by: Leonard Crestez
Reviewed-by: Fabio Estevam
Acked-by: Florian Fainelli
Acked-by: Fugang Duan
---
I also tried to do some debugging in the fec driver and it apparently receives
corrupted packets when this happens. If I hack it to go into promiscuous mode
unconditionally it gets a whole bunch of rx errors (crc errors, length errors
and so on). So the phy config is probably wrong and is confusing the mac?
In theory it might be possible to make that driver "just work" with phy
settings from uboot but it's not clear that's worthwhile.
Link to v1: https://lkml.org/lkml/2017/3/22/416
Changes:
* Improve commit message to fix checkpatch.pl warnings.
arch/arm/configs/imx_v6_v7_defconfig | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/arm/configs/imx_v6_v7_defconfig
b/arch/arm/configs/imx_v6_v7_defconfig
index eaba3b1..e605389 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -143,6 +143,7 @@ CONFIG_SMSC911X=y
# CONFIG_NET_VENDOR_STMICRO is not set
CONFIG_AT803X_PHY=y
CONFIG_MICREL_PHY=y
+CONFIG_SMSC_PHY=y
CONFIG_USB_PEGASUS=m
CONFIG_USB_RTL8150=m
CONFIG_USB_RTL8152=m
@@ -152,7 +153,6 @@ CONFIG_BRCMFMAC=m
CONFIG_WL12XX=m
CONFIG_WLCORE_SDIO=m
# CONFIG_WILINK_PLATFORM_DATA is not set
-# CONFIG_INPUT_MOUSEDEV_PSAUX is not set
CONFIG_INPUT_EVDEV=y
CONFIG_INPUT_EVBUG=m
CONFIG_KEYBOARD_GPIO=y
@@ -376,7 +376,6 @@ CONFIG_NLS_ISO8859_1=y
CONFIG_NLS_ISO8859_15=m
CONFIG_NLS_UTF8=y
CONFIG_PRINTK_TIME=y
-CONFIG_DEBUG_FS=y
CONFIG_MAGIC_SYSRQ=y
# CONFIG_SCHED_DEBUG is not set
CONFIG_PROVE_LOCKING=y
--
2.7.4
Re: [RFC 3/8] cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend
On Thu, 2017-03-23 at 10:04 +0530, Viresh Kumar wrote: > On 22-03-17, 18:53, Leonard Crestez wrote: > > If the cpufreq driver tries to modify voltage/freq during suspend/resume > > it might need to control an external PMIC via I2C or SPI but those > > devices might be already suspended. > > > > To avoid this scenario we just increase cpufreq to highest setpoint > > before suspend. This issue can easily be triggered by ldo-bypass but in > > theory any regulator set_voltage call can end up having to modify > > external supply voltages. > > > > Signed-off-by: Leonard Crestez > > --- > > drivers/cpufreq/imx6q-cpufreq.c | 2 ++ > > 1 file changed, 2 insertions(+) > > Acked-by: Viresh Kumar The first couple of patches are obvious fixes despite being marked as RFC. It would be great if you could apply them to your tree separately from the rest of the series but I'm not sure what the process is here.
Re: [RFC 4/8] regulator: core: Check enabling bypass respects constraints
On Tue, 2017-03-28 at 17:47 +0100, Mark Brown wrote:
> On Tue, Mar 28, 2017 at 03:39:41PM +0300, Leonard Crestez wrote:
> > On Fri, 2017-03-24 at 12:52 +, 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.
Sorry, still messing around with Evolution and Exchange.
> > > to the supply. Usually bypass is used for low power retention modes
> > > with different settings to those used in normal operation that wouldn't
> > > be desired in normal operation, if we were going to have constraints for
> > > this I'd expect a separate set used during bypass.
> >
> > In this particular case it's not possible to set constraints on the parent
> > regulator so that both ldo-enable and ldo-bypass modes work. The maximum
> > allowed
> > voltage for ldo-bypass is lower than the minimum required to support the
> > chip at
> > max frequency wit ldo-enable.
>
> If things are really so sensitive that you can't bypass without lowering
> the voltage then it's hard to see how you can safely transition into and
> out of bypass mode.
The CPU frequency is set to the minimum value so that when bypass mode
is entered and voltage rises (because the dropout goes away) it is
still low enough. Transitioning out of bypass mode is not implemented
but you would presumably have to go to the minimum frequency again,
raise the voltage above what is required and the flip the switch.
> > I'm not sure I understand why you are against applying constraints to the
> > parent
> > when in bypass mode, it seems like the obvious thing to do if you want to
> > support flexible configuration. The check I introduced is probably not
> > enough to
> > cover all cases, for example it would still be possible to explicitly change
> > parent voltage afterwards.
>
> To repeat what I said previously the whole point of bypassing is to not
> do regulation and generally the constraints in the unregulated idle case
> are substantially more relaxed. This would break use cases relying on
> the existing behaviour which wouldn't expect to affect the parent
> voltage at all, either stopping things working or making them less
> efficient by needlessly regulating the voltage down which defeats the
> main point of bypassing.
So what you want is to prevent voltage changes unless strictly
required, even lowering? What I want is to get the minimum voltage in
the SOC because that's where power is being consumed.
It's not at all obvious that in bypass mode the output constraints of a
regulator need not be respected by the core. I expected the opposite,
this is something that should be documented.
But if the bypassed regulator has a downstream consumer then it's
requirements should definitely still be met in bypass mode, right? I
could set my maximum voltage directly from cpufreq in that case.
Or should a bypassed regulator ignore all other requests? One of the
behaviors that this patch series relies on is that calling set_voltage
on a bypassed regulator propagates this request to the supply and picks
the minimum voltage there. An alternative implementation would be to
call set_voltage directly on the supply regulator by changing the
"{arm,soc,pu}-supply" references in DT to point to the PMIC instead.
Would that be better?
Both approaches work. Relying on propagation feels like it is the
"right way" to handle this, even if it's harder to get right and the
regulator core does not entirely support it. But it's possible that
this is based on a misunderstanding of what "bypass" is actually
supposed to do.
--
Regards,
Leonard
Re: [RFC 6/8] regulator: core: Add regulator_is_bypass function
On Fri, 2017-03-24 at 12:55 +, Mark Brown wrote: > On Wed, Mar 22, 2017 at 06:53:08PM +0200, Leonard Crestez wrote: > > /** > > + * regulator_is_bypass - Determine if the regulator is in bypass mode > Bypass is a verb so this should be regulator_is_bypassed() Very well, I will change this.
Re: [RFC 4/8] regulator: core: Check enabling bypass respects constraints
On Fri, 2017-03-24 at 12:52 +, Mark Brown wrote:
> On Wed, Mar 22, 2017 at 06:53:06PM +0200, Leonard Crestez wrote:
>
> >
> > Enabling bypass mode makes a regulator passthrough the supply voltage
> > directly. It is possible that the supply voltage is set high enough that
> > it violates machine constraints so let's check for that.
> >
> > The supply voltage might be higher because of min_dropout_uV or maybe
> > there is just an unrelated consumer who requested a higher voltage.
> I would expect that if bypass is enabled then the constraints on the
> parent regulator would be set appropriately to support this, I wouldn't
> expect that we'd try to apply the operating constraints of the regulator
> to the supply. Usually bypass is used for low power retention modes
> with different settings to those used in normal operation that wouldn't
> be desired in normal operation, if we were going to have constraints for
> this I'd expect a separate set used during bypass.
In this particular case it's not possible to set constraints on the parent
regulator so that both ldo-enable and ldo-bypass modes work. The maximum allowed
voltage for ldo-bypass is lower than the minimum required to support the chip at
max frequency wit ldo-enable.
It would be possible to also change the constraint values on the PMIC together
with ldo-bypass in the .dts files but that seems awful.
I'm not sure I understand why you are against applying constraints to the parent
when in bypass mode, it seems like the obvious thing to do if you want to
support flexible configuration. The check I introduced is probably not enough to
cover all cases, for example it would still be possible to explicitly change
parent voltage afterwards.
A regulator_dev registers a consumer for the supply. Right now this is being
used to propagate minimum voltages upwards since commit fc42112c0eaa ("Propagate
voltage changes to supply regulators"). It seems to me like it would be
reasonable to also use it to propagate maximum voltage from constraints, right?
Instead of asking for [best_uV + min_dropout_uV, INT_MAX] it could instead ask
for [min_uV + bypass ? min_dropout_uV : 0, min(max_uV, constraints->max_uV)].
The _regulator_do_set_voltage call on the supply can deal with stuff like
mapping it to a selector, just like it does for regulator consumers.
If more elaborate constraints are required instead of this simple behavior it
could be handled by adding an interface for drivers to expose explicit dynamic
min/max constraints.
--
Regards,
Leonard
Re: [RFC 5/8] regulator: anatop: fix min dropout for bypass mode
On Fri, 2017-03-24 at 12:54 +, Mark Brown wrote: > On Wed, Mar 22, 2017 at 06:53:07PM +0200, Leonard Crestez wrote: > > + if (anatop_reg->bypass) > > + anatop_reg->rdesc.min_dropout_uV = 0; > > + else > > + anatop_reg->rdesc.min_dropout_uV = LDO_MIN_DROPOUT_UV; > No, this is completely broken - you can't expect to randomly change hthe > regulator description at runtime behind the back of the framework and > expect things to work. If there is a need to do this we need an > interface for getting the current value and a way to notify of changes. > > That said I would not expect the dropout voltage to be considered at > all when the regulator is bypassed, since the regulator is not > regulating it doesn't need any headroom. It's a more complex solution but this could be handled in the core instead. Basically the core would treat min_dropout_uV as zero if the regulator is currently in bypass mode. In theory a function could be added in regulator_ops to ask a regulator driver what requirements it has for its supply but this does not seem necessary.
Re: [PATCH] ARM: imx_v6_v7_defconfig: Select SMSC_PHY
On Fri, 2017-03-24 at 14:43 +0800, Dong Aisheng wrote: > Hi Leonard, > > On Wed, Mar 22, 2017 at 04:27:37PM +0200, Leonard Crestez wrote: > > > > The imx6sl-evk board has a LAN8720A ethernet phy supported by SMSC_PHY. > > Add this driver to the default imx config since the device is present on > > one of the evaluation boards. > Upstream currently no evaluation board support. > > This may leave to Shawn to decide whether apply it. Really? But the dts for this eval board is included in upstream. Shouldn't the default config try to work with the default imx*.dts files included? > > This used to work mostly fine with the generic phy driver but since > > commit 0878fff1f42c18e448ab5b8b4f6a3eb32365b5b6 that driver no longer > I guess you should get a checkpatch error here. Because I didn't reference that other commit right? Sorry about this. Should I submit again with an improved commit message? > > performs a soft reset on startup. This causes netboot to sometimes > > timeout on DHCP because RX doesn't work right. DHCP is eventually retried > > and > Probably another checkpatch warning. Why, line too long? > > This was generated with "make savedefconfig" and it includes a few > > additional minor cleanups. > > > It is a bit strange, after apply your patch, i still get a lot difference > as follows when savedefconfig: > > I don't know what's wrong. Toolchains difference? Maybe, kconfig acts strangely sometimes. It's not clear option ordering is guaranteed to be stable. I don't think it matters though. -- Regards, Leonard
Re: [RFC 7/8] cpufreq: imx6q: Initialize LDO bypass
On Wed, 2017-03-22 at 18:09 +0100, Lucas Stach wrote: > Am Mittwoch, den 22.03.2017, 18:53 +0200 schrieb Leonard Crestez: > > > > Several imx6* socs have three built-in regulators LDO_ARM LDO_SOC and > > LDO_PU used to control internal chip voltages. "ldo-bypass" mode refers > > to placing these regulators in bypass mode and controlling voltages from > > an external power management chip instead. This is intended to save > > power at the expense of an extra PMIC on the board. > > > > The voltages for these regulators are controlled from the imxq6 cpufreq > > driver so it makes sense to also control LDO bypass from here. The gpc > > driver also fetches a reference to LDO_PU and uses it to gate power to > > graphics blocks. > > > > The LDO regulator has a minimum dropout voltage of 125mv so enabling > > bypass mode will raise the effective voltage by that amount. We need set > > the minimum frequency first to avoid overvolting when enabling LDO > > bypass. > > > > The binding is an u32 fsl,ldo-bypass in the gpc node because that's how > > it was defined in the freescale vendor tree for a long time and > > compatibility is desirable. Otherwise it would be a bool. > > > > Some versions of u-boot shipped by freescale check this same property > > and set regulators in bypass mode before linux actually starts so we > > check for that scenario as well and finish early. > I've not looked at the patch at all, but this feels like the wrong > location to implement this. Using bypass mode or not should really be a > internal decision of the regulator driver, influenced by a DT property > to allow bypass mode. > > The regulator driver can also implement the correct sequencing of first > lowering external voltage to min + dropout, then going into bypass mode, > then lower the external voltage by the amount of the dropout. I don't > see why we need a frequency switch for this at all. Because minimum voltages are dictated by core frequency. At high frequency the (minimum voltage for frequency + dropout) is too high and would go beyond the maximum of 1300 mv when bypass is enabled. It doesn't actually instantly break, this is based on the "operating ranges" from this document: http://www.nxp.com/assets/documents/data/en/data-sheets/IMX6DQCEC.pdf > Implementing this in the consumers seems like the wrong spot. It doesn't belong in drivers for individual regulators either, it's a piece of board-level global configuration. -- Regards, Leonard
[RFC 3/8] cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend
If the cpufreq driver tries to modify voltage/freq during suspend/resume
it might need to control an external PMIC via I2C or SPI but those
devices might be already suspended.
To avoid this scenario we just increase cpufreq to highest setpoint
before suspend. This issue can easily be triggered by ldo-bypass but in
theory any regulator set_voltage call can end up having to modify
external supply voltages.
Signed-off-by: Leonard Crestez
---
drivers/cpufreq/imx6q-cpufreq.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index be90ee3..e2c1fbf 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -162,6 +162,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy,
unsigned int index)
static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
{
policy->clk = arm_clk;
+ policy->suspend_freq = freq_table[soc_opp_count - 1].frequency;
return cpufreq_generic_init(policy, freq_table, transition_latency);
}
@@ -173,6 +174,7 @@ static struct cpufreq_driver imx6q_cpufreq_driver = {
.init = imx6q_cpufreq_init,
.name = "imx6q-cpufreq",
.attr = cpufreq_generic_attr,
+ .suspend = cpufreq_generic_suspend,
};
static int imx6q_cpufreq_probe(struct platform_device *pdev)
--
2.7.4
[RFC 0/8] ARM: imx: Upstream fsl,ldo-bypass
Freescale imx socs have three built-in regulators LDO_ARM LDO_SOC and LDO_PU used to control internal chip voltages. "ldo-bypass" mode refers to placing these regulators in bypass mode and controlling voltages from an external power management chip instead. The intention is to save power (at the expense of an extra PMIC on the board). Looking through commit messages and emails it seems that there are other users of unusual power configurations for imx but there doesn't appear to be any board dts file using this mode. This is an attempt to upstream this feature based on how the freescale BSP does it. This series only enables it for the imx6qdl-sabresd boards for simplicity. Versions of u-boot shipped by freescale for imx partially handle this in the bootloader. His patch series tries to detect if the bootloader did this and takes advantage of it but it is not required. This series works fine with upstream u-boot. The binding is odd because it's shared with those versions of uboot. The LDO regulators have a minimum dropout of 125mv and this is removed when bypass mode is entered. This results in a brief increase in voltage. The bypass code sets the minimum frequency before enabling LDO bypass to avoid overvolting. Some new checks are added in the regulator core to check for this (patch 4) and the checks would trigger without lowering the frequency. Issues that need to be discussed: * I'm not happy with the regulator_allow_bypass API. It claims to "allow the regulator to go into bypass mode if all other consumers for the regulator also enable bypass mode" but it doesn't seem to properly handle the case when additional consumers show up after bypass mode is enabled. It's not even clear how it should be done, should regulator_get on a bypassed regulator disable bypass mode? There are only 2 other users of this and they don't seem to use multiple consumers. For imx the gpc driver is a secondary consumer for vddpu so this needs to be handled somehow. There is a hack in patch 7 which forces gpc to always probe after cpufreq and rely on it to perform the bypass procedure. Having both gpc and cpufreq call regulator_allow_bypass would sort of work but it would be ugly. If gpc probes first it would attempt bypass and get an error (because of the supply voltage check), bypass would later be performed successfully by cpufreq. I think it would be better to have an imperative regulator_set_bypass API. I think it might even be reasonable for it to completely replace the current regulator_allow_bypass API. It would also make the code a lot simpler, there would no longer be a need to check regulator_is_bypass after regulator_allow_bypass. * There is an anatop patch which dynamically modifies regulator_desc to zero-out "min_dropout_uV" in bypass mode, despite the fact that documentation claims that regulator_desc should be read-only. Perhaps the core should handle this or this should be replaced with a function in regulator_ops? Irina Tirdea (2): cpufreq: imx6q: Fix handling EPROBE_DEFER from regulator regulator: anatop: fix min dropout for bypass mode Leonard Crestez (6): ARM: imx: gpc: Do not print error message for EPROBE_DEFER cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend regulator: core: Check enabling bypass respects constraints regulator: core: Add regulator_is_bypass function cpufreq: imx6q: Initialize LDO bypass ARM: dts: imx6qdl-sabresd: Enable fsl,ldo-bypass .../devicetree/bindings/power/fsl,imx-gpc.txt | 4 + arch/arm/boot/dts/imx6dl-sabresd-ldo.dts | 13 ++ arch/arm/boot/dts/imx6q-sabresd-ldo.dts| 13 ++ arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 19 +++ arch/arm/boot/dts/imx6qdl.dtsi | 6 +- arch/arm/boot/dts/imx6qp-sabresd-ldo.dts | 13 ++ arch/arm/boot/dts/imx6qp-sabresd.dts | 4 +- arch/arm/mach-imx/gpc.c| 13 +- drivers/cpufreq/imx6q-cpufreq.c| 180 + drivers/regulator/anatop-regulator.c | 9 +- drivers/regulator/core.c | 78 - include/linux/regulator/consumer.h | 1 + 12 files changed, 344 insertions(+), 9 deletions(-) create mode 100644 arch/arm/boot/dts/imx6dl-sabresd-ldo.dts create mode 100644 arch/arm/boot/dts/imx6q-sabresd-ldo.dts create mode 100644 arch/arm/boot/dts/imx6qp-sabresd-ldo.dts -- 2.7.4
[RFC 2/8] cpufreq: imx6q: Fix handling EPROBE_DEFER from regulator
From: Irina Tirdea
If there are any errors in getting the cpu0 regulators, the driver returns
-ENOENT. In case the regulators are not yet available, the devm_regulator_get
calls will return -EPROBE_DEFER, so that the driver can be probed later.
If we return -ENOENT, the driver will fail its initialization and will
not try to probe again (when the regulators become available).
Return the actual error received from regulator_get in probe. Print a
differentiated message in case we need to probe the device later and
in case we actually failed. Also add a message to inform when the
driver has been successfully registered.
Signed-off-by: Irina Tirdea
Signed-off-by: Leonard Crestez
---
drivers/cpufreq/imx6q-cpufreq.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 7719b02..be90ee3 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -222,6 +222,13 @@ static int imx6q_cpufreq_probe(struct platform_device
*pdev)
arm_reg = regulator_get(cpu_dev, "arm");
pu_reg = regulator_get_optional(cpu_dev, "pu");
soc_reg = regulator_get(cpu_dev, "soc");
+ if (PTR_ERR(arm_reg) == -EPROBE_DEFER ||
+ PTR_ERR(soc_reg) == -EPROBE_DEFER ||
+ PTR_ERR(pu_reg) == -EPROBE_DEFER) {
+ ret = -EPROBE_DEFER;
+ dev_dbg(cpu_dev, "regulators not ready, defer\n");
+ goto put_reg;
+ }
if (IS_ERR(arm_reg) || IS_ERR(soc_reg)) {
dev_err(cpu_dev, "failed to get regulators\n");
ret = -ENOENT;
--
2.7.4
[RFC 4/8] regulator: core: Check enabling bypass respects constraints
Enabling bypass mode makes a regulator passthrough the supply voltage
directly. It is possible that the supply voltage is set high enough that
it violates machine constraints so let's check for that.
The supply voltage might be higher because of min_dropout_uV or maybe
there is just an unrelated consumer who requested a higher voltage.
Signed-off-by: Leonard Crestez
---
drivers/regulator/core.c | 52 ++--
1 file changed, 50 insertions(+), 2 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 53d4fc7..9d893aa 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3453,6 +3453,54 @@ int regulator_set_load(struct regulator *regulator, int
uA_load)
}
EXPORT_SYMBOL_GPL(regulator_set_load);
+static int _regulator_set_bypass(struct regulator *regulator, bool bypass)
+{
+ struct regulator_dev *rdev = regulator->rdev;
+ int output_voltage;
+ int supply_voltage;
+
+ if (bypass && !rdev->supply) {
+ rdev_err(rdev, "Refuse to set bypass on regulator with no
supply!\n");
+ return -EINVAL;
+ }
+
+ /* Check that enabling bypass won't break constraints */
+ if (bypass && _regulator_is_enabled(rdev)) {
+ output_voltage = _regulator_get_voltage(rdev);
+ if (output_voltage < 0) {
+ rdev_err(rdev, "Failed to get old output voltage before"
+ " enabling bypass: %d\n",
output_voltage);
+ return output_voltage;
+ }
+ supply_voltage = _regulator_get_voltage(rdev->supply->rdev);
+ if (supply_voltage < 0) {
+ rdev_err(rdev, "Failed to get supply voltage before"
+ " enabling bypass: %d\n",
supply_voltage);
+ return supply_voltage;
+ }
+ if (supply_voltage < rdev->constraints->min_uV ||
+ supply_voltage > rdev->constraints->max_uV) {
+ rdev_err(rdev, "Enabling bypass would change voltage"
+ " from %duV to %duV violating"
+ " constraint range %duV to %duV\n",
+ output_voltage,
+ supply_voltage,
+ rdev->constraints->min_uV,
+ rdev->constraints->max_uV);
+ return -EINVAL;
+ }
+ rdev_dbg(rdev, "Enabling bypass would change voltage"
+ " from %duV to %duV respecting"
+ " constraint range %duV to %duV\n",
+ output_voltage,
+ supply_voltage,
+ rdev->constraints->min_uV,
+ rdev->constraints->max_uV);
+ }
+
+ return rdev->desc->ops->set_bypass(rdev, bypass);
+}
+
/**
* regulator_allow_bypass - allow the regulator to go into bypass mode
*
@@ -3481,7 +3529,7 @@ int regulator_allow_bypass(struct regulator *regulator,
bool enable)
rdev->bypass_count++;
if (rdev->bypass_count == rdev->open_count) {
- ret = rdev->desc->ops->set_bypass(rdev, enable);
+ ret = _regulator_set_bypass(regulator, enable);
if (ret != 0)
rdev->bypass_count--;
}
@@ -3490,7 +3538,7 @@ int regulator_allow_bypass(struct regulator *regulator,
bool enable)
rdev->bypass_count--;
if (rdev->bypass_count != rdev->open_count) {
- ret = rdev->desc->ops->set_bypass(rdev, enable);
+ ret = _regulator_set_bypass(regulator, enable);
if (ret != 0)
rdev->bypass_count++;
}
--
2.7.4
[RFC 6/8] regulator: core: Add regulator_is_bypass function
This is a simple kernel API to query the bypass state of a regulator.
Signed-off-by: Leonard Crestez
---
drivers/regulator/core.c | 26 ++
include/linux/regulator/consumer.h | 1 +
2 files changed, 27 insertions(+)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 9d893aa..7d4f59e 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3554,6 +3554,32 @@ int regulator_allow_bypass(struct regulator *regulator,
bool enable)
EXPORT_SYMBOL_GPL(regulator_allow_bypass);
/**
+ * regulator_is_bypass - Determine if the regulator is in bypass mode
+ *
+ * @regulator: Regulator to examine
+ *
+ * @return 0 or 1 for true/false or errno on failure
+ *
+ * Returns zero on a regulator without bypass support.
+ */
+int regulator_is_bypass(struct regulator *regulator)
+{
+ struct regulator_dev *rdev = regulator->rdev;
+ bool bypass;
+ int ret = 0;
+
+ if (!rdev->desc->ops->get_bypass)
+ return 0;
+
+ mutex_lock(&rdev->mutex);
+ ret = rdev->desc->ops->get_bypass(rdev, &bypass);
+ mutex_unlock(&rdev->mutex);
+
+ return ret ? ret : !!bypass;
+}
+EXPORT_SYMBOL_GPL(regulator_is_bypass);
+
+/**
* regulator_register_notifier - register regulator event notifier
* @regulator: regulator source
* @nb: notifier block
diff --git a/include/linux/regulator/consumer.h
b/include/linux/regulator/consumer.h
index ea0fffa..ba78511 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -261,6 +261,7 @@ int regulator_get_error_flags(struct regulator *regulator,
int regulator_set_load(struct regulator *regulator, int load_uA);
int regulator_allow_bypass(struct regulator *regulator, bool allow);
+int regulator_is_bypass(struct regulator *regulator);
struct regmap *regulator_get_regmap(struct regulator *regulator);
int regulator_get_hardware_vsel_register(struct regulator *regulator,
--
2.7.4
[RFC 7/8] cpufreq: imx6q: Initialize LDO bypass
Several imx6* socs have three built-in regulators LDO_ARM LDO_SOC and
LDO_PU used to control internal chip voltages. "ldo-bypass" mode refers
to placing these regulators in bypass mode and controlling voltages from
an external power management chip instead. This is intended to save
power at the expense of an extra PMIC on the board.
The voltages for these regulators are controlled from the imxq6 cpufreq
driver so it makes sense to also control LDO bypass from here. The gpc
driver also fetches a reference to LDO_PU and uses it to gate power to
graphics blocks.
The LDO regulator has a minimum dropout voltage of 125mv so enabling
bypass mode will raise the effective voltage by that amount. We need set
the minimum frequency first to avoid overvolting when enabling LDO
bypass.
The binding is an u32 fsl,ldo-bypass in the gpc node because that's how
it was defined in the freescale vendor tree for a long time and
compatibility is desirable. Otherwise it would be a bool.
Some versions of u-boot shipped by freescale check this same property
and set regulators in bypass mode before linux actually starts so we
check for that scenario as well and finish early.
Signed-off-by: Leonard Crestez
---
.../devicetree/bindings/power/fsl,imx-gpc.txt | 4 +
arch/arm/mach-imx/gpc.c| 7 +
drivers/cpufreq/imx6q-cpufreq.c| 171 +
3 files changed, 182 insertions(+)
diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
b/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
index 65cc034..8a7584b 100644
--- a/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
+++ b/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
@@ -11,6 +11,10 @@ Required properties:
datasheet
- interrupts: Should contain GPC interrupt request 1
- pu-supply: Link to the LDO regulator powering the PU power domain
+- fsl,ldo-bypass: Should be 0 or 1 to enable LDO bypass mode (default 0).
+ This is performed in cooperation with cpufreq. Some versions of uboot will
+ also look at this property and set the arm and soc regulators in bypass mode
+ before linux.
- clocks: Clock phandles to devices in the PU power domain that need
to be enabled during domain power-up for reset propagation.
- #power-domain-cells: Should be 1, see below:
diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
index ce64d11..62a2555 100644
--- a/arch/arm/mach-imx/gpc.c
+++ b/arch/arm/mach-imx/gpc.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
#include "common.h"
#include "hardware.h"
@@ -461,6 +462,12 @@ static int imx_gpc_probe(struct platform_device *pdev)
if (!of_property_read_bool(pdev->dev.of_node, "#power-domain-cells"))
return 0;
+ /* wait for cpufreq to initialize before using pu_reg */
+ if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ) &&
cpufreq_get_current_driver() == NULL) {
+ dev_dbg(&pdev->dev, "cpufreq driver not ready, retry\n");
+ return -EPROBE_DEFER;
+ }
+
pu_reg = devm_regulator_get_optional(&pdev->dev, "pu");
if (PTR_ERR(pu_reg) == -ENODEV)
pu_reg = NULL;
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index e2c1fbf..a0c11ed 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -159,8 +159,179 @@ static int imx6q_set_target(struct cpufreq_policy
*policy, unsigned int index)
return 0;
}
+/*
+ * Enable ldo-bypass mode if configured and not already performed by u-boot
+ */
+static int imx6q_cpufreq_init_ldo_bypass(void)
+{
+ struct device_node *gpc_node;
+ unsigned long old_freq_hz;
+ int i, old_freq_index;
+ u32 bypass = 0;
+ int ret = 0, ret2;
+
+ /* Read ldo-bypass property */
+ gpc_node = of_find_compatible_node(NULL, NULL, "fsl,imx6q-gpc");
+ if (!gpc_node)
+ return 0;
+ ret = of_property_read_u32(gpc_node, "fsl,ldo-bypass", &bypass);
+ if (ret && ret != -EINVAL)
+ dev_warn(cpu_dev, "failed reading fsl,ldo-bypass property:
%d\n", ret);
+ if (!bypass)
+ goto out;
+
+ /*
+* Freescale u-boot handles ldo-bypass by setting
+* arm/soc in bypass and vddpu disabled.
+*
+* If this is the case we don't need any special freq lowering.
+*/
+ if (regulator_is_bypass(arm_reg) == 1 &&
+ regulator_is_bypass(soc_reg) == 1)
+ {
+ dev_info(cpu_dev, "vddarm and vddsoc already in bypass\n");
+ if (IS_ERR(pu_reg)) {
+ ret = 0;
+ goto out;
+ } else if (regulator_is_enabled(pu_reg) == 0) {
+ ret = regulator_
[RFC 1/8] ARM: imx: gpc: Do not print error message for EPROBE_DEFER
The pu regulator request will return -EPROBE_DEFER if it has a supply
from PMIC and this supply is not yet registered. This does not represent
an error since the driver will call probe again later, so only print a
warning message in this case.
Signed-off-by: Irina Tirdea
Signed-off-by: Leonard Crestez
---
arch/arm/mach-imx/gpc.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
index 1dc2a34..ce64d11 100644
--- a/arch/arm/mach-imx/gpc.c
+++ b/arch/arm/mach-imx/gpc.c
@@ -466,7 +466,11 @@ static int imx_gpc_probe(struct platform_device *pdev)
pu_reg = NULL;
if (IS_ERR(pu_reg)) {
ret = PTR_ERR(pu_reg);
- dev_err(&pdev->dev, "failed to get pu regulator: %d\n", ret);
+ if (ret == -EPROBE_DEFER)
+ dev_dbg(&pdev->dev, "pu regulator not ready, retry\n");
+ else
+ dev_err(&pdev->dev, "failed to get pu regulator: %d\n",
+ ret);
return ret;
}
--
2.7.4
[RFC 8/8] ARM: dts: imx6qdl-sabresd: Enable fsl,ldo-bypass
This enables LDO bypass by default on the imx6qdl-sabresd boards. New
dts files with -ldo suffix are added for users who want to run with LDOs
enabled on these boards anyway.
Signed-off-by: Leonard Crestez
---
arch/arm/boot/dts/imx6dl-sabresd-ldo.dts | 13 +
arch/arm/boot/dts/imx6q-sabresd-ldo.dts | 13 +
arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 19 +++
arch/arm/boot/dts/imx6qdl.dtsi | 6 +++---
arch/arm/boot/dts/imx6qp-sabresd-ldo.dts | 13 +
arch/arm/boot/dts/imx6qp-sabresd.dts | 4 ++--
6 files changed, 63 insertions(+), 5 deletions(-)
create mode 100644 arch/arm/boot/dts/imx6dl-sabresd-ldo.dts
create mode 100644 arch/arm/boot/dts/imx6q-sabresd-ldo.dts
create mode 100644 arch/arm/boot/dts/imx6qp-sabresd-ldo.dts
diff --git a/arch/arm/boot/dts/imx6dl-sabresd-ldo.dts
b/arch/arm/boot/dts/imx6dl-sabresd-ldo.dts
new file mode 100644
index 000..1b224d6
--- /dev/null
+++ b/arch/arm/boot/dts/imx6dl-sabresd-ldo.dts
@@ -0,0 +1,13 @@
+/*
+ * Copyright 2017 NXP
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include "imx6dl-sabresd.dts"
+
+&gpc {
+ fsl,ldo-bypass = <0>;
+};
diff --git a/arch/arm/boot/dts/imx6q-sabresd-ldo.dts
b/arch/arm/boot/dts/imx6q-sabresd-ldo.dts
new file mode 100644
index 000..4d2e8cc
--- /dev/null
+++ b/arch/arm/boot/dts/imx6q-sabresd-ldo.dts
@@ -0,0 +1,13 @@
+/*
+ * Copyright 2017 NXP
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include "imx6q-sabresd.dts"
+
+&gpc {
+ fsl,ldo-bypass = <0>;
+};
diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index 63bf95e..5a73d9d 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -182,6 +182,10 @@
status = "okay";
};
+&gpc {
+ fsl,ldo-bypass = <1>;
+};
+
&hdmi {
ddc-i2c-bus = <&i2c2>;
status = "okay";
@@ -548,6 +552,21 @@
status = "okay";
};
+®_arm {
+ vin-supply = <&sw1a_reg>;
+ regulator-allow-bypass;
+};
+
+®_pu {
+ vin-supply = <&sw1c_reg>;
+ regulator-allow-bypass;
+};
+
+®_soc {
+ vin-supply = <&sw1c_reg>;
+ regulator-allow-bypass;
+};
+
&snvs_poweroff {
status = "okay";
};
diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index 6d7bf64..54fe769 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -677,7 +677,7 @@
compatible = "fsl,anatop-regulator";
regulator-name = "vddarm";
regulator-min-microvolt = <725000>;
- regulator-max-microvolt = <145>;
+ regulator-max-microvolt = <130>;
regulator-always-on;
anatop-reg-offset = <0x140>;
anatop-vol-bit-shift = <0>;
@@ -694,7 +694,7 @@
compatible = "fsl,anatop-regulator";
regulator-name = "vddpu";
regulator-min-microvolt = <725000>;
- regulator-max-microvolt = <145>;
+ regulator-max-microvolt = <130>;
regulator-enable-ramp-delay = <150>;
anatop-reg-offset = <0x140>;
anatop-vol-bit-shift = <9>;
@@ -711,7 +711,7 @@
compatible = "fsl,anatop-regulator";
regulator-name = "vddsoc";
regulator-min-microvolt = <725000>;
- regulator-max-microvolt = <145>;
+ regulator-max-microvolt = <130>;
regulator-always-on;
anatop-reg-offset = <0x140>;
anatop-vol-bit-shift = <18>;
diff --git a/arch/arm/boot/dts/imx6qp-sabresd-ldo.dts
b/arch/arm/boot/dts/imx6qp-sabresd-ldo.dts
new file mode 100644
index 000..c659533
--- /dev/null
+++ b/arch/arm/
[RFC 5/8] regulator: anatop: fix min dropout for bypass mode
From: Irina Tirdea
In bypass mode, the anatop digital regulators do not have any minimum
dropout value (the input voltage is equal to the output voltage according
to documentation).
Having a min dropout value of 125mV will lead to an increased voltage
for PMIC supplies.
Only set minimum dropout value for ldo enabled mode.
Signed-off-by: Irina Tirdea
Signed-off-by: Leonard Crestez
---
drivers/regulator/anatop-regulator.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/regulator/anatop-regulator.c
b/drivers/regulator/anatop-regulator.c
index b041f27..64554e8 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -38,6 +38,8 @@
#define LDO_POWER_GATE 0x00
#define LDO_FET_FULL_ON0x1f
+#define LDO_MIN_DROPOUT_UV 125000
+
struct anatop_regulator {
const char *name;
u32 control_reg;
@@ -153,6 +155,10 @@ static int anatop_regmap_set_bypass(struct regulator_dev
*reg, bool enable)
sel = enable ? LDO_FET_FULL_ON : anatop_reg->sel;
anatop_reg->bypass = enable;
+ if (anatop_reg->bypass)
+ anatop_reg->rdesc.min_dropout_uV = 0;
+ else
+ anatop_reg->rdesc.min_dropout_uV = LDO_MIN_DROPOUT_UV;
return regulator_set_voltage_sel_regmap(reg, sel);
}
@@ -264,7 +270,7 @@ static int anatop_regulator_probe(struct platform_device
*pdev)
rdesc->vsel_reg = sreg->control_reg;
rdesc->vsel_mask = ((1 << sreg->vol_bit_width) - 1) <<
sreg->vol_bit_shift;
- rdesc->min_dropout_uV = 125000;
+ rdesc->min_dropout_uV = LDO_MIN_DROPOUT_UV;
config.dev = &pdev->dev;
config.init_data = initdata;
@@ -286,6 +292,7 @@ static int anatop_regulator_probe(struct platform_device
*pdev)
if (sreg->sel == LDO_FET_FULL_ON) {
sreg->sel = 0;
sreg->bypass = true;
+ rdesc->min_dropout_uV = 0;
}
/*
--
2.7.4
[PATCH] ARM: imx_v6_v7_defconfig: Select SMSC_PHY
The imx6sl-evk board has a LAN8720A ethernet phy supported by SMSC_PHY. Add this driver to the default imx config since the device is present on one of the evaluation boards. This used to work mostly fine with the generic phy driver but since commit 0878fff1f42c18e448ab5b8b4f6a3eb32365b5b6 that driver no longer performs a soft reset on startup. This causes netboot to sometimes timeout on DHCP because RX doesn't work right. DHCP is eventually retried and it works the second time but it takes 90+ seconds to get a login prompt. This was generated with "make savedefconfig" and it includes a few additional minor cleanups. Signed-off-by: Leonard Crestez --- I also tried to do some debugging in the fec driver and it apparently receives corrupted packets when this happens. If I hack it to go into promiscuous mode unconditionally it gets a whole bunch of rx errors (crc errors, length errors and so on). So the phy config is probably wrong and is confusing the mac? In theory it might be possible to make that driver "just work" with phy settings from uboot but it's not clear it's worthwhile. arch/arm/configs/imx_v6_v7_defconfig | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig index eaba3b1..e605389 100644 --- a/arch/arm/configs/imx_v6_v7_defconfig +++ b/arch/arm/configs/imx_v6_v7_defconfig @@ -143,6 +143,7 @@ CONFIG_SMSC911X=y # CONFIG_NET_VENDOR_STMICRO is not set CONFIG_AT803X_PHY=y CONFIG_MICREL_PHY=y +CONFIG_SMSC_PHY=y CONFIG_USB_PEGASUS=m CONFIG_USB_RTL8150=m CONFIG_USB_RTL8152=m @@ -152,7 +153,6 @@ CONFIG_BRCMFMAC=m CONFIG_WL12XX=m CONFIG_WLCORE_SDIO=m # CONFIG_WILINK_PLATFORM_DATA is not set -# CONFIG_INPUT_MOUSEDEV_PSAUX is not set CONFIG_INPUT_EVDEV=y CONFIG_INPUT_EVBUG=m CONFIG_KEYBOARD_GPIO=y @@ -376,7 +376,6 @@ CONFIG_NLS_ISO8859_1=y CONFIG_NLS_ISO8859_15=m CONFIG_NLS_UTF8=y CONFIG_PRINTK_TIME=y -CONFIG_DEBUG_FS=y CONFIG_MAGIC_SYSRQ=y # CONFIG_SCHED_DEBUG is not set CONFIG_PROVE_LOCKING=y -- 2.7.4
Re: [PATCH] clk: core: Copy connection id
On Tue, 2017-02-28 at 00:05 -0800, sb...@codeaurora.org wrote: > On 02/25, Leonard Crestez wrote: > > > > On Fri, 2017-02-24 at 12:44 -0800, Stephen Boyd wrote: > > > > > > On 02/20, Leonard Crestez wrote: > > > > > > > > Some drivers use sprintf to build clk connection id names but > > > > the > > > > clk > > > > core will save those strings and occasionally print them back. > > > > Duplicate > > > > the con_id strings instead of fixing all the users. > > > Good catch. What about dev_id though? That could also have the > > > same problem if some device is removed and we're still holding a > > > reference to the kobject's name. This is probably more rare than > > > what is happening here, but still seems possible that we might > > > trip over that later. > > A device should normally free the clks it uses before it is > > destroyed. > > This means that if dev_id is pointing to freed memory then the clk > > itself was probably leaked, right? > Sure. clk_get_sys() could be called and then we could have > something sprintf the dev_id there. A quick grep doesn't show any > place where that happens though so it seems safe right now. > > That said, it would be nice to clearly document that we don't > expect dev_id to be freed or changed during the lifetime of the > clk structure, but we do allow con_id to change. Perhaps the copy > shows that, but a comment would also be useful so we don't have > people wondering why dev_id isn't copied as well. This should be mentioned on the public documentation for clk_get_sys, clk_get and devm_clk_get, right? These seem to be the public entry points to the clk subsystem and users are expected to read their docs. Do you want me to resend the patch with these notes? I'm not comfortable adding to documentation when I don't fully understand the system myself. I only discovered this while looking into unrelated driver issues.
Re: [PATCH] clk: core: Copy connection id
On Fri, 2017-02-24 at 12:44 -0800, Stephen Boyd wrote: > On 02/20, Leonard Crestez wrote: > > Some drivers use sprintf to build clk connection id names but the > > clk > > core will save those strings and occasionally print them back. > > Duplicate > > the con_id strings instead of fixing all the users. > > Good catch. What about dev_id though? That could also have the > same problem if some device is removed and we're still holding a > reference to the kobject's name. This is probably more rare than > what is happening here, but still seems possible that we might > trip over that later. A device should normally free the clks it uses before it is destroyed. This means that if dev_id is pointing to freed memory then the clk itself was probably leaked, right? This is obvious misuse of the API, not like sprintf-ing a con_id in a complex driver. I don't really think it's worth copying strings for it. -- Regards, Leonard
[PATCH] cpufreq: imx6q: use devm to fetch resources
Replace the clk_get and regulator_get will devm variants to free the
resources automatically when probe failed or driver is removed.
The device we are probing is not cpu_dev but the cpufreq platform_device
(pdev->dev). In order for this to actually work correctly we assign to
the cpufreq device the of_node from cpu_dev.
Signed-off-by: Leonard Crestez
---
drivers/cpufreq/imx6q-cpufreq.c | 75 +
1 file changed, 24 insertions(+), 51 deletions(-)
Something similar was rejected in the past:
https://patchwork.kernel.org/patch/7099051/
New version assigns the of_node from the cpu device to the cpufreq device so
that devm_clk_get(pdev->dev, ...) works as expected. It is not clear if this is
allowed but I don't see why it wouldn't.
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index ef1fa81..374bc72 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -1,5 +1,6 @@
/*
* Copyright (C) 2013 Freescale Semiconductor, Inc.
+ * Copyright (C) 2017 NXP
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
@@ -198,35 +199,37 @@ static int imx6q_cpufreq_probe(struct platform_device
*pdev)
return -ENOENT;
}
- arm_clk = clk_get(cpu_dev, "arm");
- pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
- pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
- step_clk = clk_get(cpu_dev, "step");
- pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
+ pdev->dev.of_node = cpu_dev->of_node;
+
+ arm_clk = devm_clk_get(&pdev->dev, "arm");
+ pll1_sys_clk = devm_clk_get(&pdev->dev, "pll1_sys");
+ pll1_sw_clk = devm_clk_get(&pdev->dev, "pll1_sw");
+ step_clk = devm_clk_get(&pdev->dev, "step");
+ pll2_pfd2_396m_clk = devm_clk_get(&pdev->dev, "pll2_pfd2_396m");
if (IS_ERR(arm_clk) || IS_ERR(pll1_sys_clk) || IS_ERR(pll1_sw_clk) ||
IS_ERR(step_clk) || IS_ERR(pll2_pfd2_396m_clk)) {
dev_err(cpu_dev, "failed to get clocks\n");
ret = -ENOENT;
- goto put_clk;
+ goto put_node;
}
if (of_machine_is_compatible("fsl,imx6ul")) {
- pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
- secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
+ pll2_bus_clk = devm_clk_get(&pdev->dev, "pll2_bus");
+ secondary_sel_clk = devm_clk_get(&pdev->dev, "secondary_sel");
if (IS_ERR(pll2_bus_clk) || IS_ERR(secondary_sel_clk)) {
dev_err(cpu_dev, "failed to get clocks specific to
imx6ul\n");
ret = -ENOENT;
- goto put_clk;
+ goto put_node;
}
}
- arm_reg = regulator_get(cpu_dev, "arm");
- pu_reg = regulator_get_optional(cpu_dev, "pu");
- soc_reg = regulator_get(cpu_dev, "soc");
+ arm_reg = devm_regulator_get(&pdev->dev, "arm");
+ pu_reg = devm_regulator_get_optional(&pdev->dev, "pu");
+ soc_reg = devm_regulator_get(&pdev->dev, "soc");
if (IS_ERR(arm_reg) || IS_ERR(soc_reg)) {
dev_err(cpu_dev, "failed to get regulators\n");
ret = -ENOENT;
- goto put_reg;
+ goto put_node;
}
/*
@@ -239,7 +242,7 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
ret = dev_pm_opp_of_add_table(cpu_dev);
if (ret < 0) {
dev_err(cpu_dev, "failed to init OPP table: %d\n", ret);
- goto put_reg;
+ goto put_node;
}
/* Because we have added the OPPs here, we must free them */
@@ -256,11 +259,11 @@ static int imx6q_cpufreq_probe(struct platform_device
*pdev)
ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
if (ret) {
dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
- goto put_reg;
+ goto out_free_opp;
}
/* Make imx6_soc_volt array's size same as arm opp number */
- imx6_soc_volt = devm_kzalloc(cpu_dev, sizeof(*imx6_soc_volt) * num,
GFP_KERNEL);
+ imx6_soc_volt = devm_kzalloc(&pdev->dev, sizeof(*imx6_soc_volt) * num,
GFP_KERNEL);
if (imx6_soc_volt == NULL) {
ret = -ENOMEM;
goto free_freq_table;
@@ -339,7 +342,6 @@ static int imx6q_cpufreq_probe(struct platform_device
[PATCH] clk: core: Copy connection id
Some drivers use sprintf to build clk connection id names but the clk core will save those strings and occasionally print them back. Duplicate the con_id strings instead of fixing all the users. Signed-off-by: Leonard Crestez --- drivers/clk/clk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) Some examples of using sprintf for con_id include: drivers/mfd/omap-usb-host.c drivers/tty/serial/samsung.c sound/soc/fsl/fsl_asrc.c There are lots more. They are difficult to find and "fixing" them on the consumer side requires nasty code to keep track of the allocated clkname. diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 0fb39fe..67201f6 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2502,7 +2502,7 @@ struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id, clk->core = hw->core; clk->dev_id = dev_id; - clk->con_id = con_id; + clk->con_id = kstrdup_const(con_id, GFP_KERNEL); clk->max_rate = ULONG_MAX; clk_prepare_lock(); @@ -2518,6 +2518,7 @@ void __clk_free_clk(struct clk *clk) hlist_del(&clk->clks_node); clk_prepare_unlock(); + kfree_const(clk->con_id); kfree(clk); } -- 2.7.4
[PATCH] regulator: Fix regulator_summary for deviceless consumers
It is allowed to call regulator_get with a NULL dev argument (_regulator_get explicitly checks for it) but this causes an error later when printing /sys/kernel/debug/regulator_summary. Fix this by explicitly handling "deviceless" consumers in the debugfs code. This fixes errors like this: root@leonard-imx6:~# cat /sys/kernel/debug/regulator/regulator_summary Unable to handle kernel NULL pointer dereference at virtual address 015c pgd = a8bd4000 [015c] *pgd=a8c15831, *pte=, *ppte= Internal error: Oops: 17 [#2] PREEMPT SMP ARM Modules linked in: CPU: 0 PID: 673 Comm: cat Tainted: G D 4.9.5-01680-g3d4992e #18 Hardware name: Freescale i.MX6 SoloLite (Device Tree) task: a8e07700 task.stack: a8c2a000 PC is at regulator_summary_show_subtree+0x1b4/0x274 LR is at seq_puts+0x48/0x58 pc : [<8044eb90>]lr : [<8021f82c>]psr: 800e0013 sp : a8c2bdf0 ip : fp : 80c52720 r10: 80c21fc0 r9 : 0004 r8 : 80f253a4 r7 : a821601c r6 : a874d400 r5 : a8216000 r4 : a8b1c000 r3 : 80c21fc0 r2 : 0004 r1 : 80c52720 r0 : a8b1c000 Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user Control: 10c53c7d Table: a8bd404a DAC: 0055 Process cat (pid: 673, stack limit = 0xa8c2a210) Stack: (0xa8c2bdf0 to 0xa8c2c000) bde0: 001e 80c167f0 0001 0002 be00: 0004 001b 10624dd3 a80ca35c a8c2be64 be20: 8044ec50 a8190dac 0002 a8216060 a8b1c000 8044ec50 a8c2bf80 0001 be40: a8c2be98 0002 8044ec78 a8216060 804b942c 80c524f8 a8c2bf80 be60: 0001 a80ca35c a82161ac a8b1c000 a87cd240 a87cd240 8044dc6c be80: a8b1c000 8021efc4 a8bd5db8 76e73000 a8b1c030 bea0: a8c2bf80 80177644 a60017f8 8021ee2c a87cd240 76e73000 0002 a8c2bf80 bec0: 80a48d9c 0002 80356080 a8bd3bc0 0001 0800 a87cd240 bee0: 80356034 0002 a8c2bf80 0002 801fbf4c 76e72004 a8c2bfb0 bf00: 2710 76fca000 76faed14 801012b0 80114820 00022000 00022000 bf20: a8c2bf8c 0003 a8b488c0 76e72000 0022 0073 801e44a4 bf40: 00076e72 a87cd240 76e73000 0002 a8c2bf80 0002 801fcd08 bf60: 00022000 0003 a87cd240 a87cd240 76e73000 801fdb00 bf80: 0002 0002 76e73000 0003 80107724 bfa0: a8c2a000 80107580 0002 0002 0003 76e73000 0002 0002a3fc bfc0: 0002 0002 76e73000 0003 7fffe000 76ffe000 0002 bfe0: 7ea10b64 00014b14 76f52770 600e0010 0003 [<8044eb90>] (regulator_summary_show_subtree) from [<8044ec78>] (regulator_summary_show_roots+0x28/0x30) [<8044ec78>] (regulator_summary_show_roots) from [<804b942c>] (class_for_each_device+0x4c/0xb4) [<804b942c>] (class_for_each_device) from [<8044dc6c>] (regulator_summary_show+0x3c/0x48) [<8044dc6c>] (regulator_summary_show) from [<8021efc4>] (seq_read+0x198/0x4a0) [<8021efc4>] (seq_read) from [<80356080>] (full_proxy_read+0x4c/0x6c) [<80356080>] (full_proxy_read) from [<801fbf4c>] (__vfs_read+0x1c/0x10c) [<801fbf4c>] (__vfs_read) from [<801fcd08>] (vfs_read+0x8c/0x118) [<801fcd08>] (vfs_read) from [<801fdb00>] (SyS_read+0x3c/0x90) [<801fdb00>] (SyS_read) from [<80107580>] (ret_fast_syscall+0x0/0x34) Code: e59f80c4 e1a0100b e59d2018 e1a4 (e59ce15c) ---[ end trace eca2c2e6d835da26 ]--- Signed-off-by: Leonard Crestez --- drivers/regulator/core.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) Some quick grepping only finds a very small number of users for regulator_get(NULL, ...), it might make more sense to simply require always passing a device pointer. I found this because there was a patch in the freescale imx tree which did this and in our case it was easier to just fix the consumer. Here are the current obvious upstream users: $ git grep 'regulator_get(NULL' arch/arm/mach-davinci/da850.c: cvdd = regulator_get(NULL, "cvdd"); arch/arm/mach-pxa/em-x270.c:em_x270_usb_ldo = regulator_get(NULL, "vcc usb"); drivers/cpufreq/pxa2xx-cpufreq.c: vcc_core = regulator_get(NULL, "vcc_core"); drivers/cpufreq/s3c2416-cpufreq.c: s3c_freq->vddarm = regulator_get(NULL, "vddarm"); drivers/cpufreq/s3c64xx-cpufreq.c: vddarm = regulator_get(NULL, "vddarm"); drivers/cpufreq/s5pv210-cpufreq.c: arm_regulator = regulator_get(NULL, "vddarm"); drivers/cpufreq/s5pv210-cpufreq.c: int_regulator = regulator_get(NULL, "vddint"); diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 04baac9..6631954 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -4391,12 +4391,13 @@ static void regulator_summary_show_subtree(struct seq_file *s, seq_puts(
Re: [PATCH] of: alloc anywhere from memblock if range not specified
Hello,
I have some trouble with this patch.
It seems the intention is to allow CMA to be placed in highmem. If the
CMA area is larger than highmem and no alloc-ranges is specified (just a
size) it is possible to end up allocating a area that spans from
multiple zones. This later breaks checks in cma_activate_area and makes
most dma allocations fail.
Am I missing something or this a bug?
On Mon, Feb 22, 2016 at 3:45 PM, Vinayak Menon
wrote:
early_init_dt_alloc_reserved_memory_arch passes end as 0 to
__memblock_alloc_base, when limits are not specified. But
__memblock_alloc_base takes end value of 0 as MEMBLOCK_ALLOC_ACCESSIBLE
and limits the end to memblock.current_limit. This results in regions
never being placed in HIGHMEM area, for e.g. CMA.
Let __memblock_alloc_base allocate from anywhere in memory if limits are
not specified.
Acked-by: Marek Szyprowski
Signed-off-by: Vinayak Menon
---
drivers/of/of_reserved_mem.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 1a3556a..ed01c01 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -32,11 +32,13 @@ int __init __weak
early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
phys_addr_t align, phys_addr_t start, phys_addr_t end, bool nomap,
phys_addr_t *res_base)
{
+ phys_addr_t base;
/*
* We use __memblock_alloc_base() because memblock_alloc_base()
* panic()s on allocation failure.
*/
- phys_addr_t base = __memblock_alloc_base(size, align, end);
+ end = !end ? MEMBLOCK_ALLOC_ANYWHERE : end;
+ base = __memblock_alloc_base(size, align, end);
if (!base)
return -ENOMEM;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 2/2] ti-adc081c: Initial triggered buffer support
On 04/01/2016 11:34 AM, Peter Meerwald-Stadler wrote:
-static const struct iio_chan_spec adc081c_channel = {
- .type = IIO_VOLTAGE,
- .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-};
the patch would look cleaner/shorter if adc081c_channel won't get moved
around
It was not moved around, it is now defined by a macro. Buffer support
requires defining scan_type which contains a different number of bits.
The macros are now after iio_info adc081c_info instead of before, I can
move that around. I also noticed that I declared both a struct
adcxx1c_model and an unused struct adcxx1c_info. I will remove that.
The first patch in the series doesn't use any per-model data and just
stores the number of bits in driver_data. I can change the series to
introduce adcxx1c_model in the first patch.
+static irqreturn_t adc081c_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct adc081c *data = iio_priv(indio_dev);
+ s64 ts;
+ u16 buf[8];
comment: 2 bytes data + 6 bytes padding + 8 bytes timestamp
+ int ret;
+
+ /* Otherwise iio_push_to_buffers will corrupt the stack. */
+ if (indio_dev->scan_bytes > sizeof(buf)) {
+ dev_crit_once(&indio_dev->dev, "Bad iio_scan_bytes=%d > %d\n",
+ indio_dev->scan_bytes, (int)sizeof(buf));
rather than casting sizeof(buf), use the correct printf length modifier,
i.e. %z
not sure if this check is needed
I guess it's not needed. My first version defined the buffer incorrectly
and caused a messy crash. Calculating manual buffer alignments seems
very fragile.
It seems that C99 variable-length-arrays work fine, something like:
u16 buf[indio_dev->scan_bytes / 2];
Would that be acceptable? It compiles without warnings and there some
other places in the kernel where VLAs are used.
+ ret = i2c_smbus_read_word_swapped(data->i2c, REG_CONV_RES);
REG_CONV_RES should be called ADC081C_REG_CONV_RES, but that's a separate
issue
Yes, but that would be an entirely unrelated renaming.
+ ts = iio_get_time_ns();
why is the timestamp taken here?, seems strange
often this is done together with iio_push_to_buffers_with_timestamp()
I wanted to keep it as close to the read as possible. In this case it
doesn't matter.
- iio->channels = &adc081c_channel;
- iio->num_channels = 1;
+ iio->channels = model->channels;
+ iio->num_channels = 2;
the number of channels could go into the adcxx1c_info struct
But it's a constant, it does not vary between devices. I could make an
ADC081C_NUM_CHANNELS define.
--
Regards,
Leonard
Re: [RFC v2] percpu: Add a separate function to merge free areas
On 12/04/2014 07:57 PM, Tejun Heo wrote: > Hello, > > On Wed, Dec 03, 2014 at 12:33:59AM +0200, Leonard Crestez wrote: >> It seems that free_percpu performance is very bad when working with small >> objects. The easiest way to reproduce this is to allocate and then free a >> large >> number of percpu int counters in order. Small objects (reference counters >> and >> pointers) are common users of alloc_percpu and I think this should be fast. >> This particular issue can be encountered with very large number of net_device >> structs. > > Do you actually experience this with an actual workload? The thing is > allocation has the same quadratic complexity. If this is actually an > issue (which can definitely be the case), I'd much prefer implementing > a properly scalable area allocator than mucking with the current > implementation. Yes, we are actually experiencing issues with this. We create lots of virtual net_devices and routes, which means lots of percpu counters/pointers. In particular we are getting worse performance than in older kernels because the net_device refcnt is now a percpu counter. We could turn that back into a single integer but this would negate an upstream optimization. We are working on top of linux_3.10. We already pulled some allocation optimizations. At least for simple allocation patterns pcpu_alloc does not appear to be unreasonably slow. Having a "properly scalable" percpu allocator would be quite nice indeed. Regards, Leonard -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC v2] percpu: Add a separate function to merge free areas
Hello,
It seems that free_percpu performance is very bad when working with small
objects. The easiest way to reproduce this is to allocate and then free a large
number of percpu int counters in order. Small objects (reference counters and
pointers) are common users of alloc_percpu and I think this should be fast.
This particular issue can be encountered with very large number of net_device
structs.
The problem seems to be that pcpu_chunk keeps an array of all alocated areas.
At
free time pcpu_free_area will delete items from that array linearly, using
memmove. This has worst-case quadratic complexity in the number of areas per
chunk. This gets really bad if the areas are small and are allocated and freed
in order.
One way to fix this is to merge free areas in a separate function and handle
multiple frees at once. There is a patch below which does this, but
I'm not sure it's correct.
Instead of merging free areas on every free we only do it when 2/3 of
the slots in the map are used. This should hopefully amortize to linear
complexity instead of quadratic.
I've posted an earlier version of this to lkml earlier but got no response. That
was probably because I only posted to lkml. I now sent to linux-mm and the
people
reported by "get_maintainer.pl". Here's a link to the older post:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg777188.html
That version of the patch is incorrect. Never updating contig_hint on free means
that once a chunk's contig_hint reaches 0 (when completely allocated) that chunk
will never be considered for allocation again. It also doesn't amortize the
frees.
Entirely different solutions could be considered. For example it might make
sense to
make chunks smaller (they are about ~40K on the system I care about). Or maybe
even
write an entirely custom allocator for very small fixed-size percpu objects.
Signed-off-by: Crestez Dan Leonard
---
mm/percpu.c | 100 +++-
1 file changed, 78 insertions(+), 22 deletions(-)
diff --git a/mm/percpu.c b/mm/percpu.c
index 014bab6..9d85198 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -109,6 +109,7 @@ struct pcpu_chunk {
int map_used; /* # of map entries used before
the sentry */
int map_alloc; /* # of map entries allocated */
+ int map_free_count; /* # of map entries freed but
not merged */
int *map; /* allocation map */
struct work_struct map_extend_work;/* async ->map[] extension */
@@ -375,6 +376,69 @@ static void pcpu_chunk_relocate(struct pcpu_chunk *chunk,
int oslot)
}
/**
+ * percpu_merge_free_spaces - merge spaces in a chunk
+ * @chunk: chunk of interest
+ *
+ * This function should merge a continous region of free
+ * spaces into a single one.
+ *
+ * CONTEXT:
+ * pcpu_lock.
+ */
+static void pcpu_merge_free_spaces(struct pcpu_chunk *chunk)
+{
+ int start;
+ int i, j;
+
+ chunk->map_free_count = 0;
+
+ /* We copy from map[i] into map[j] while merging free spaces. */
+ i = j = chunk->first_free;
+ /* In case first_free points to something busy */
+ if (chunk->map[i] & 1)
+ goto eat_busy;
+
+eat_free:
+ /* Look for busy space
+* Last chunk is always busy, no need to check map_used
+*/
+ for (start = i; !(chunk->map[i] & 1); ++i);
+
+ /* Collapse */
+ if (j != start)
+ chunk->map[j] = chunk->map[start];
+ ++chunk->map_free_count;
+ ++j;
+
+ chunk->contig_hint = max(chunk->contig_hint,
+ (chunk->map[i] & ~1) - chunk->map[start]);
+
+eat_busy:
+ /* Look for free space */
+ for (start = i; i <= chunk->map_used && (chunk->map[i] & 1); ++i);
+
+ /* Move stuff if appropriate */
+ if (j != start)
+ memmove(chunk->map + j, chunk->map + start, (i - start) *
sizeof(*chunk->map));
+ j += i - start;
+
+ if (i > chunk->map_used)
+ goto end;
+ else
+ goto eat_free;
+
+end:
+ /* Done */
+ chunk->map_used = j - 1;
+}
+
+static inline void pcpu_check_merge_free_spaces(struct pcpu_chunk *chunk)
+{
+ if (chunk->map_free_count * 3 >= chunk->map_used * 2)
+ pcpu_merge_free_spaces(chunk);
+}
+
+/**
* pcpu_need_to_extend - determine whether chunk area map needs to be extended
* @chunk: chunk of interest
* @is_atomic: the allocation context
@@ -623,10 +687,12 @@ static int pcpu_alloc_area(struct pcpu_chunk *chunk, int
size, int align,
*++p = off += head;
++i;
max_contig = max(head, max_contig);
+ chunk->map_free_count++;
}
if (tail) {
p[1] = off + size;
