mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH] fix i.MX51 babbage ethernet
@ 2014-05-08  7:31 Sascha Hauer
  2014-05-08  7:31 ` [PATCH 1/5] mfd: mc13xxx: Allow to set callback for mc13xxx Sascha Hauer
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Sascha Hauer @ 2014-05-08  7:31 UTC (permalink / raw)
  To: barebox

This once again bends the init order to fit another board. This series
works over a number of shortcomings in barebox and devicetree support.
The problem here is that the Babbage ethernet phy needs vgen3 of the PMIC
enabled.

- We currently do not have proper regulator support for the mc13xxx,
  so we manipulate the PMIC registers directly from the board file. This
  has to happen before the ethernet phy is initialized. Once again
  we fiddle with the initcall order to make that sure. It can't be
  very long until we need some dependency mechanism for devices.
- Even if that is fixed the ethernet phy currently can't be described
  in the devicetree so we can't properly attach a regulator to it.

The following fixes ethernet support for the Babbage board which seems
to be broken for quite some time, at least when barebox is used as a
first stage loader.

Sascha

----------------------------------------------------------------
Sascha Hauer (5):
      mfd: mc13xxx: Allow to set callback for mc13xxx
      spi: i.MX: Move to coredevice_initcall
      mfd: mc13xxx: move to coredevice_initcall
      ARM: i.MX51 babbage: use mc13xxx_register_callback to initialize PMIC
      ARM: dts: i.MX51 babbage: overwrite upstream FEC iomux settings

 arch/arm/boards/freescale-mx51-babbage/board.c | 27 +++++++-------
 arch/arm/dts/imx51-babbage.dts                 | 36 +++++++++++++++++++
 drivers/mfd/mc13xxx.c                          | 50 ++++++++++++++++++++------
 drivers/net/fec_imx.h                          |  1 +
 drivers/spi/imx_spi.c                          |  2 +-
 include/mfd/mc13xxx.h                          |  6 ++++
 6 files changed, 95 insertions(+), 27 deletions(-)

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/5] mfd: mc13xxx: Allow to set callback for mc13xxx
  2014-05-08  7:31 [PATCH] fix i.MX51 babbage ethernet Sascha Hauer
@ 2014-05-08  7:31 ` Sascha Hauer
  2014-05-18 23:14   ` Marc Reilly
  2014-05-08  7:31 ` [PATCH 2/5] spi: i.MX: Move to coredevice_initcall Sascha Hauer
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Sascha Hauer @ 2014-05-08  7:31 UTC (permalink / raw)
  To: barebox

Some boards have to initialize the PMIC before other devices can
be initialized. This requires three levels of initcalls: one level
in which the PMIC is probed, one in which the board can call mc13xxx_get()
and the third one to initialize the PMIC dependent devices.

Allow to register a callback which is called once the PMIC is initialized.
This way mc13xxx_get() is no longer necessary and the number of required
initcalls levels is reduced to two.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mfd/mc13xxx.c | 18 ++++++++++++++++++
 drivers/net/fec_imx.h |  1 +
 include/mfd/mc13xxx.h |  6 ++++++
 3 files changed, 25 insertions(+)

diff --git a/drivers/mfd/mc13xxx.c b/drivers/mfd/mc13xxx.c
index bfbd328..9842c56 100644
--- a/drivers/mfd/mc13xxx.c
+++ b/drivers/mfd/mc13xxx.c
@@ -60,6 +60,21 @@ int mc13xxx_revision(struct mc13xxx *mc13xxx)
 }
 EXPORT_SYMBOL(mc13xxx_revision);
 
+void(*mc13xxx_callback)(struct mc13xxx *mc13xxx);
+
+int mc13xxx_register_callback(void(*callback)(struct mc13xxx *mc13xxx))
+{
+	if (mc13xxx_callback)
+		return -EBUSY;
+
+	mc13xxx_callback = callback;
+
+	if (mc_dev)
+		mc13xxx_callback(mc_dev);
+
+	return 0;
+}
+
 #ifdef CONFIG_SPI
 static int spi_rw(struct spi_device *spi, void * buf, size_t len)
 {
@@ -350,6 +365,9 @@ static int __init mc13xxx_probe(struct device_d *dev)
 	mc_dev->revision = rev;
 	devfs_create(&mc_dev->cdev);
 
+	if (mc13xxx_callback)
+		mc13xxx_callback(mc_dev);
+
 	return 0;
 }
 
diff --git a/drivers/net/fec_imx.h b/drivers/net/fec_imx.h
index 0921b52..a77c1dc 100644
--- a/drivers/net/fec_imx.h
+++ b/drivers/net/fec_imx.h
@@ -146,6 +146,7 @@ struct fec_priv {
 	void (*phy_init)(struct phy_device *dev);
 	struct clk *clk;
 	enum fec_type type;
+	int phy_reset_gpio;
 };
 
 static inline int fec_is_imx27(struct fec_priv *priv)
diff --git a/include/mfd/mc13xxx.h b/include/mfd/mc13xxx.h
index 1946b1a..cf5c42d 100644
--- a/include/mfd/mc13xxx.h
+++ b/include/mfd/mc13xxx.h
@@ -171,6 +171,7 @@ extern int mc13xxx_revision(struct mc13xxx *mc13xxx);
 extern int mc13xxx_reg_read(struct mc13xxx *mc13xxx, u8 reg, u32 *val);
 extern int mc13xxx_reg_write(struct mc13xxx *mc13xxx, u8 reg, u32 val);
 extern int mc13xxx_set_bits(struct mc13xxx *mc13xxx, u8 reg, u32 mask, u32 val);
+int mc13xxx_register_callback(void(*callback)(struct mc13xxx *mc13xxx));
 #else
 static inline struct mc13xxx *mc13xxx_get(void)
 {
@@ -196,6 +197,11 @@ static inline int mc13xxx_set_bits(struct mc13xxx *mc13xxx, u8 reg, u32 mask, u3
 {
 	return -ENODEV;
 }
+
+static inline int mc13xxx_register_callback(void(*callback)(struct mc13xxx *mc13xxx))
+{
+	return -ENODEV;
+}
 #endif
 
 #endif /* __MFD_MC13XXX_H */
-- 
2.0.0.rc0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/5] spi: i.MX: Move to coredevice_initcall
  2014-05-08  7:31 [PATCH] fix i.MX51 babbage ethernet Sascha Hauer
  2014-05-08  7:31 ` [PATCH 1/5] mfd: mc13xxx: Allow to set callback for mc13xxx Sascha Hauer
@ 2014-05-08  7:31 ` Sascha Hauer
  2014-05-11 18:49   ` Alexander Shiyan
  2014-05-08  7:31 ` [PATCH 3/5] mfd: mc13xxx: move " Sascha Hauer
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Sascha Hauer @ 2014-05-08  7:31 UTC (permalink / raw)
  To: barebox

SPI is often used by other devices, so make sure it's initialized
early.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/spi/imx_spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/imx_spi.c b/drivers/spi/imx_spi.c
index e07cf1a..3146339 100644
--- a/drivers/spi/imx_spi.c
+++ b/drivers/spi/imx_spi.c
@@ -586,4 +586,4 @@ static struct driver_d imx_spi_driver = {
 	.of_compatible = DRV_OF_COMPAT(imx_spi_dt_ids),
 	.id_table = imx_spi_ids,
 };
-device_platform_driver(imx_spi_driver);
+coredevice_platform_driver(imx_spi_driver);
-- 
2.0.0.rc0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 3/5] mfd: mc13xxx: move to coredevice_initcall
  2014-05-08  7:31 [PATCH] fix i.MX51 babbage ethernet Sascha Hauer
  2014-05-08  7:31 ` [PATCH 1/5] mfd: mc13xxx: Allow to set callback for mc13xxx Sascha Hauer
  2014-05-08  7:31 ` [PATCH 2/5] spi: i.MX: Move to coredevice_initcall Sascha Hauer
@ 2014-05-08  7:31 ` Sascha Hauer
  2014-05-08  7:31 ` [PATCH 4/5] ARM: i.MX51 babbage: use mc13xxx_register_callback to initialize PMIC Sascha Hauer
  2014-05-08  7:31 ` [PATCH 5/5] ARM: dts: i.MX51 babbage: overwrite upstream FEC iomux settings Sascha Hauer
  4 siblings, 0 replies; 12+ messages in thread
From: Sascha Hauer @ 2014-05-08  7:31 UTC (permalink / raw)
  To: barebox

The PMIC is often a dependency for other devices, so make sure
it's initialized early. While at it, merge the spi/i2c registration
into a single initcall and use IS_ENABLED instead of ifdefs.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mfd/mc13xxx.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/mfd/mc13xxx.c b/drivers/mfd/mc13xxx.c
index 9842c56..9e86fc3 100644
--- a/drivers/mfd/mc13xxx.c
+++ b/drivers/mfd/mc13xxx.c
@@ -397,7 +397,6 @@ static __maybe_unused struct of_device_id mc13xxx_dt_ids[] = {
 	{ }
 };
 
-#ifdef CONFIG_I2C
 static struct driver_d mc13xxx_i2c_driver = {
 	.name		= "mc13xxx-i2c",
 	.probe		= mc13xxx_probe,
@@ -405,19 +404,30 @@ static struct driver_d mc13xxx_i2c_driver = {
 	.of_compatible	= DRV_OF_COMPAT(mc13xxx_dt_ids),
 };
 
-static int __init mc13xxx_i2c_init(void)
-{
-	return i2c_driver_register(&mc13xxx_i2c_driver);
-}
-device_initcall(mc13xxx_i2c_init);
-#endif
-
-#ifdef CONFIG_SPI
 static struct driver_d mc13xxx_spi_driver = {
 	.name		= "mc13xxx-spi",
 	.probe		= mc13xxx_probe,
 	.id_table	= mc13xxx_ids,
 	.of_compatible	= DRV_OF_COMPAT(mc13xxx_dt_ids),
 };
-device_spi_driver(mc13xxx_spi_driver);
-#endif
+
+static int __init mc13xxx_init(void)
+{
+	int err_spi = 0, err_i2c = 0;
+
+	if (IS_ENABLED(CONFIG_I2C))
+		err_spi = i2c_driver_register(&mc13xxx_i2c_driver);
+
+	if (IS_ENABLED(CONFIG_SPI))
+		err_i2c = spi_driver_register(&mc13xxx_spi_driver);
+
+	if (err_spi)
+		return err_spi;
+
+	if (err_i2c)
+		return err_i2c;
+
+	return 0;
+
+}
+coredevice_initcall(mc13xxx_init);
-- 
2.0.0.rc0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 4/5] ARM: i.MX51 babbage: use mc13xxx_register_callback to initialize PMIC
  2014-05-08  7:31 [PATCH] fix i.MX51 babbage ethernet Sascha Hauer
                   ` (2 preceding siblings ...)
  2014-05-08  7:31 ` [PATCH 3/5] mfd: mc13xxx: move " Sascha Hauer
@ 2014-05-08  7:31 ` Sascha Hauer
  2014-05-08  7:31 ` [PATCH 5/5] ARM: dts: i.MX51 babbage: overwrite upstream FEC iomux settings Sascha Hauer
  4 siblings, 0 replies; 12+ messages in thread
From: Sascha Hauer @ 2014-05-08  7:31 UTC (permalink / raw)
  To: barebox

This makes sure the PMIC is initialized once it's available. Move the
initcall to coredevice_initcall to make sure we initialize the PMIC
before the FEC driver is initialized. The ethernet phy needs vgen3
enabled in the PMIC initialization.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/boards/freescale-mx51-babbage/board.c | 27 ++++++++++++--------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/arch/arm/boards/freescale-mx51-babbage/board.c b/arch/arm/boards/freescale-mx51-babbage/board.c
index bfe5338..11c901c 100644
--- a/arch/arm/boards/freescale-mx51-babbage/board.c
+++ b/arch/arm/boards/freescale-mx51-babbage/board.c
@@ -14,6 +14,8 @@
  *
  */
 
+#define pr_fmt(fmt) "babbage: " fmt
+
 #include <common.h>
 #include <init.h>
 #include <environment.h>
@@ -44,17 +46,10 @@
 
 #define MX51_CCM_CACRR 0x10
 
-static void babbage_power_init(void)
+static void babbage_power_init(struct mc13xxx *mc13xxx)
 {
-	struct mc13xxx *mc13xxx;
 	u32 val;
 
-	mc13xxx = mc13xxx_get();
-	if (!mc13xxx) {
-		printf("could not get PMIC\n");
-		return;
-	}
-
 	/* Write needed to Power Gate 2 register */
 	mc13xxx_reg_read(mc13xxx, MC13892_REG_POWER_MISC, &val);
 	val &= ~0x10000;
@@ -149,21 +144,23 @@ static void babbage_power_init(void)
 	mc13xxx_reg_write(mc13xxx, MC13892_REG_MODE_1, val);
 
 	udelay(200);
+
+	pr_info("initialized PMIC\n");
+
+	console_flush();
+	imx51_init_lowlevel(800);
+	clock_notifier_call_chain();
 }
 
 extern char flash_header_imx51_babbage_start[];
 extern char flash_header_imx51_babbage_end[];
 
-static int imx51_babbage_late_init(void)
+static int imx51_babbage_init(void)
 {
 	if (!of_machine_is_compatible("fsl,imx51-babbage"))
 		return 0;
 
-	babbage_power_init();
-
-	console_flush();
-	imx51_init_lowlevel(800);
-	clock_notifier_call_chain();
+	mc13xxx_register_callback(babbage_power_init);
 
 	armlinux_set_architecture(MACH_TYPE_MX51_BABBAGE);
 
@@ -173,4 +170,4 @@ static int imx51_babbage_late_init(void)
 
 	return 0;
 }
-late_initcall(imx51_babbage_late_init);
+coredevice_initcall(imx51_babbage_init);
-- 
2.0.0.rc0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 5/5] ARM: dts: i.MX51 babbage: overwrite upstream FEC iomux settings
  2014-05-08  7:31 [PATCH] fix i.MX51 babbage ethernet Sascha Hauer
                   ` (3 preceding siblings ...)
  2014-05-08  7:31 ` [PATCH 4/5] ARM: i.MX51 babbage: use mc13xxx_register_callback to initialize PMIC Sascha Hauer
@ 2014-05-08  7:31 ` Sascha Hauer
  4 siblings, 0 replies; 12+ messages in thread
From: Sascha Hauer @ 2014-05-08  7:31 UTC (permalink / raw)
  To: barebox

As of v3.15-rc4 these contain NO_PAD_CTRL settings which are not
suitable for an initial setup. Overwrite upstream settings until
these are fixed.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/dts/imx51-babbage.dts | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/arch/arm/dts/imx51-babbage.dts b/arch/arm/dts/imx51-babbage.dts
index 82b25d3..e258586 100644
--- a/arch/arm/dts/imx51-babbage.dts
+++ b/arch/arm/dts/imx51-babbage.dts
@@ -36,3 +36,39 @@
 &iim {
 	barebox,provide-mac-address = <&fec 1 9>;
 };
+
+&iomuxc {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_hog>;
+
+	imx51-babbage {
+		pinctrl_fec: fecgrp {
+			/*
+			 * Overwrite upstream FEC iomux settings since these currently
+			 * have NO_PAD_CTRL instead of real settings. Remove this once
+			 * this is fixed upstream.
+			 */
+			fsl,pins = <
+				MX51_PAD_EIM_EB2__FEC_MDIO		0x000001f5
+				MX51_PAD_EIM_EB3__FEC_RDATA1		0x00000085
+				MX51_PAD_EIM_CS2__FEC_RDATA2		0x00000085
+				MX51_PAD_EIM_CS3__FEC_RDATA3		0x00000085
+				MX51_PAD_EIM_CS4__FEC_RX_ER		0x00000180
+				MX51_PAD_EIM_CS5__FEC_CRS		0x00000180
+				MX51_PAD_NANDF_RB2__FEC_COL		0x00000180
+				MX51_PAD_NANDF_RB3__FEC_RX_CLK		0x00000180
+				MX51_PAD_NANDF_D9__FEC_RDATA0		0x00002180
+				MX51_PAD_NANDF_D8__FEC_TDATA0		0x00002004
+				MX51_PAD_NANDF_CS2__FEC_TX_ER		0x00002004
+				MX51_PAD_NANDF_CS3__FEC_MDC		0x00002004
+				MX51_PAD_NANDF_CS4__FEC_TDATA1		0x00002004
+				MX51_PAD_NANDF_CS5__FEC_TDATA2		0x00002004
+				MX51_PAD_NANDF_CS6__FEC_TDATA3		0x00002004
+				MX51_PAD_NANDF_CS7__FEC_TX_EN		0x00002004
+				MX51_PAD_NANDF_RDY_INT__FEC_TX_CLK	0x00002180
+				MX51_PAD_NANDF_D11__FEC_RX_DV		0x000020a4
+				MX51_PAD_EIM_A20__GPIO2_14		0x00000085 /* Phy Reset */
+			>;
+		};
+	};
+};
-- 
2.0.0.rc0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/5] spi: i.MX: Move to coredevice_initcall
  2014-05-08  7:31 ` [PATCH 2/5] spi: i.MX: Move to coredevice_initcall Sascha Hauer
@ 2014-05-11 18:49   ` Alexander Shiyan
  2014-05-12 10:38     ` Sascha Hauer
  0 siblings, 1 reply; 12+ messages in thread
From: Alexander Shiyan @ 2014-05-11 18:49 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox

Thu,  8 May 2014 09:31:36 +0200 от Sascha Hauer <s.hauer@pengutronix.de>:
> SPI is often used by other devices, so make sure it's initialized
> early.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/spi/imx_spi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/imx_spi.c b/drivers/spi/imx_spi.c
> index e07cf1a..3146339 100644
> --- a/drivers/spi/imx_spi.c
> +++ b/drivers/spi/imx_spi.c
> @@ -586,4 +586,4 @@ static struct driver_d imx_spi_driver = {
>  	.of_compatible = DRV_OF_COMPAT(imx_spi_dt_ids),
>  	.id_table = imx_spi_ids,
>  };
> -device_platform_driver(imx_spi_driver);
> +coredevice_platform_driver(imx_spi_driver);
> -- 

Maybe we should put all drivers in a single runlevel and implement
handle of EPROBE_DEFER?

---

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/5] spi: i.MX: Move to coredevice_initcall
  2014-05-11 18:49   ` Alexander Shiyan
@ 2014-05-12 10:38     ` Sascha Hauer
  2014-05-14 17:57       ` Alexander Shiyan
  0 siblings, 1 reply; 12+ messages in thread
From: Sascha Hauer @ 2014-05-12 10:38 UTC (permalink / raw)
  To: Alexander Shiyan; +Cc: barebox

On Sun, May 11, 2014 at 10:49:26PM +0400, Alexander Shiyan wrote:
> Thu,  8 May 2014 09:31:36 +0200 от Sascha Hauer <s.hauer@pengutronix.de>:
> > SPI is often used by other devices, so make sure it's initialized
> > early.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/spi/imx_spi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/spi/imx_spi.c b/drivers/spi/imx_spi.c
> > index e07cf1a..3146339 100644
> > --- a/drivers/spi/imx_spi.c
> > +++ b/drivers/spi/imx_spi.c
> > @@ -586,4 +586,4 @@ static struct driver_d imx_spi_driver = {
> >  	.of_compatible = DRV_OF_COMPAT(imx_spi_dt_ids),
> >  	.id_table = imx_spi_ids,
> >  };
> > -device_platform_driver(imx_spi_driver);
> > +coredevice_platform_driver(imx_spi_driver);
> > -- 
> 
> Maybe we should put all drivers in a single runlevel and implement
> handle of EPROBE_DEFER?

I think about EPROBE_DEFER for longer now. What has hold me back was
that we would have to release all resources before returning
EPROBE_DEFER. Most drivers don't have proper error pathes implemented
and I don't see much gain in fixing this. For a bootloader the error
release pathes are unnecessary overhead, because nobody will ever
use the same resources again (at least without EPROBE_DEFER).
My recent idea to work around that issue is this:

- put the drivers private data into dev->priv
- when returning an error from probe, instead of freeing the data
  keep it.
- when probing again re-use the data.

With this drivers could be written like:

	priv = dev->priv;
	if (!priv) {
		priv = xzalloc(sizeof(priv));
		dev->priv = priv;
	}

	if (!priv->clk) {
		clk = clk_get(dev, NULL);
		if (IS_ERR(clk))
			return PTR_ERR(clk);
		priv->clk = clk;
		clk_enable(priv->clk);
	}

	if (!priv->regulator) {
		regulator = regulator_get(dev, "vmmc");
		if (IS_ERR(regulator))
			return PTR_ERR(regulator);
		priv->regulator = regulator;
	}

This way drivers could allocate the missing resources as needed without
freeing them between different calls to probe().

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/5] spi: i.MX: Move to coredevice_initcall
  2014-05-12 10:38     ` Sascha Hauer
@ 2014-05-14 17:57       ` Alexander Shiyan
  2014-05-19  8:47         ` Sascha Hauer
  0 siblings, 1 reply; 12+ messages in thread
From: Alexander Shiyan @ 2014-05-14 17:57 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox

Mon, 12 May 2014 12:38:50 +0200 от Sascha Hauer <s.hauer@pengutronix.de>:
> On Sun, May 11, 2014 at 10:49:26PM +0400, Alexander Shiyan wrote:
> > Thu,  8 May 2014 09:31:36 +0200 от Sascha Hauer <s.hauer@pengutronix.de>:
> > > SPI is often used by other devices, so make sure it's initialized
> > > early.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > >  drivers/spi/imx_spi.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/spi/imx_spi.c b/drivers/spi/imx_spi.c
> > > index e07cf1a..3146339 100644
> > > --- a/drivers/spi/imx_spi.c
> > > +++ b/drivers/spi/imx_spi.c
> > > @@ -586,4 +586,4 @@ static struct driver_d imx_spi_driver = {
> > >  	.of_compatible = DRV_OF_COMPAT(imx_spi_dt_ids),
> > >  	.id_table = imx_spi_ids,
> > >  };
> > > -device_platform_driver(imx_spi_driver);
> > > +coredevice_platform_driver(imx_spi_driver);
> > > -- 
> > 
> > Maybe we should put all drivers in a single runlevel and implement
> > handle of EPROBE_DEFER?
> 
> I think about EPROBE_DEFER for longer now. What has hold me back was
> that we would have to release all resources before returning
> EPROBE_DEFER. Most drivers don't have proper error pathes implemented
> and I don't see much gain in fixing this. For a bootloader the error
> release pathes are unnecessary overhead, because nobody will ever
> use the same resources again (at least without EPROBE_DEFER).
> My recent idea to work around that issue is this:
> 
> - put the drivers private data into dev->priv
> - when returning an error from probe, instead of freeing the data
>   keep it.
> - when probing again re-use the data.
> 
> With this drivers could be written like:
> 
> 	priv = dev->priv;
> 	if (!priv) {
> 		priv = xzalloc(sizeof(priv));
> 		dev->priv = priv;
> 	}
> 
> 	if (!priv->clk) {
> 		clk = clk_get(dev, NULL);
> 		if (IS_ERR(clk))
> 			return PTR_ERR(clk);
> 		priv->clk = clk;
> 		clk_enable(priv->clk);
> 	}
> 
> 	if (!priv->regulator) {
> 		regulator = regulator_get(dev, "vmmc");
> 		if (IS_ERR(regulator))
> 			return PTR_ERR(regulator);
> 		priv->regulator = regulator;
> 	}
> 
> This way drivers could allocate the missing resources as needed without
> freeing them between different calls to probe().

The idea is good, but I think that it will not be so easy to do.
In addition, we obtain additional incompatibility with the kernel code.

---

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] mfd: mc13xxx: Allow to set callback for mc13xxx
  2014-05-08  7:31 ` [PATCH 1/5] mfd: mc13xxx: Allow to set callback for mc13xxx Sascha Hauer
@ 2014-05-18 23:14   ` Marc Reilly
  2014-05-19  6:40     ` Sascha Hauer
  0 siblings, 1 reply; 12+ messages in thread
From: Marc Reilly @ 2014-05-18 23:14 UTC (permalink / raw)
  To: barebox

Hi,

I'm a bit slow on this one, so sorry if I'm too late to the party. :)

Cheers,
Marc


On Thursday, May 08, 2014 09:31:35 AM Sascha Hauer wrote:
> Some boards have to initialize the PMIC before other devices can
> be initialized. This requires three levels of initcalls: one level
> in which the PMIC is probed, one in which the board can call mc13xxx_get()
> and the third one to initialize the PMIC dependent devices.
> 
> Allow to register a callback which is called once the PMIC is initialized.
> This way mc13xxx_get() is no longer necessary and the number of required
> initcalls levels is reduced to two.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mfd/mc13xxx.c | 18 ++++++++++++++++++
>  drivers/net/fec_imx.h |  1 +
>  include/mfd/mc13xxx.h |  6 ++++++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/drivers/mfd/mc13xxx.c b/drivers/mfd/mc13xxx.c
> index bfbd328..9842c56 100644
> --- a/drivers/mfd/mc13xxx.c
> +++ b/drivers/mfd/mc13xxx.c
> @@ -60,6 +60,21 @@ int mc13xxx_revision(struct mc13xxx *mc13xxx)
>  }
>  EXPORT_SYMBOL(mc13xxx_revision);
> 
> +void(*mc13xxx_callback)(struct mc13xxx *mc13xxx);


Should (can?) this be static?


> +
> +int mc13xxx_register_callback(void(*callback)(struct mc13xxx *mc13xxx))


What about mc13xxx_register_init_callback, or something which makes it more 
apparent when the callback is called.


> +{
> +	if (mc13xxx_callback)
> +		return -EBUSY;
> +
> +	mc13xxx_callback = callback;
> +
> +	if (mc_dev)
> +		mc13xxx_callback(mc_dev);
> +
> +	return 0;
> +}
> +
>  #ifdef CONFIG_SPI
>  static int spi_rw(struct spi_device *spi, void * buf, size_t len)
>  {
> @@ -350,6 +365,9 @@ static int __init mc13xxx_probe(struct device_d *dev)
>  	mc_dev->revision = rev;
>  	devfs_create(&mc_dev->cdev);
> 
> +	if (mc13xxx_callback)
> +		mc13xxx_callback(mc_dev);
> +
>  	return 0;
>  }
> 
> diff --git a/drivers/net/fec_imx.h b/drivers/net/fec_imx.h
> index 0921b52..a77c1dc 100644
> --- a/drivers/net/fec_imx.h
> +++ b/drivers/net/fec_imx.h
> @@ -146,6 +146,7 @@ struct fec_priv {
>  	void (*phy_init)(struct phy_device *dev);
>  	struct clk *clk;
>  	enum fec_type type;
> +	int phy_reset_gpio;


Did this sneak in?


>  };
> 
>  static inline int fec_is_imx27(struct fec_priv *priv)
> diff --git a/include/mfd/mc13xxx.h b/include/mfd/mc13xxx.h
> index 1946b1a..cf5c42d 100644
> --- a/include/mfd/mc13xxx.h
> +++ b/include/mfd/mc13xxx.h
> @@ -171,6 +171,7 @@ extern int mc13xxx_revision(struct mc13xxx *mc13xxx);
>  extern int mc13xxx_reg_read(struct mc13xxx *mc13xxx, u8 reg, u32 *val);
>  extern int mc13xxx_reg_write(struct mc13xxx *mc13xxx, u8 reg, u32 val);
>  extern int mc13xxx_set_bits(struct mc13xxx *mc13xxx, u8 reg, u32 mask, u32
> val); +int mc13xxx_register_callback(void(*callback)(struct mc13xxx
> *mc13xxx)); #else
>  static inline struct mc13xxx *mc13xxx_get(void)
>  {
> @@ -196,6 +197,11 @@ static inline int mc13xxx_set_bits(struct mc13xxx
> *mc13xxx, u8 reg, u32 mask, u3 {
>  	return -ENODEV;
>  }
> +
> +static inline int mc13xxx_register_callback(void(*callback)(struct mc13xxx
> *mc13xxx)) +{
> +	return -ENODEV;
> +}
>  #endif
> 
>  #endif /* __MFD_MC13XXX_H */


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] mfd: mc13xxx: Allow to set callback for mc13xxx
  2014-05-18 23:14   ` Marc Reilly
@ 2014-05-19  6:40     ` Sascha Hauer
  0 siblings, 0 replies; 12+ messages in thread
From: Sascha Hauer @ 2014-05-19  6:40 UTC (permalink / raw)
  To: Marc Reilly; +Cc: barebox

Hi MArc,

On Mon, May 19, 2014 at 09:14:19AM +1000, Marc Reilly wrote:
> Hi,
> 
> I'm a bit slow on this one, so sorry if I'm too late to the party. :)

Don't worry, you're not too late.
> > 
> > +void(*mc13xxx_callback)(struct mc13xxx *mc13xxx);
> 
> 
> Should (can?) this be static?

Yes, it should. fixed.

> > +int mc13xxx_register_callback(void(*callback)(struct mc13xxx *mc13xxx))
> 
> 
> What about mc13xxx_register_init_callback, or something which makes it more 
> apparent when the callback is called.

Indeed that's a better name.

> > diff --git a/drivers/net/fec_imx.h b/drivers/net/fec_imx.h
> > index 0921b52..a77c1dc 100644
> > --- a/drivers/net/fec_imx.h
> > +++ b/drivers/net/fec_imx.h
> > @@ -146,6 +146,7 @@ struct fec_priv {
> >  	void (*phy_init)(struct phy_device *dev);
> >  	struct clk *clk;
> >  	enum fec_type type;
> > +	int phy_reset_gpio;
> 
> 
> Did this sneak in?

Oops. Fixed.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/5] spi: i.MX: Move to coredevice_initcall
  2014-05-14 17:57       ` Alexander Shiyan
@ 2014-05-19  8:47         ` Sascha Hauer
  0 siblings, 0 replies; 12+ messages in thread
From: Sascha Hauer @ 2014-05-19  8:47 UTC (permalink / raw)
  To: Alexander Shiyan; +Cc: barebox

On Wed, May 14, 2014 at 09:57:57PM +0400, Alexander Shiyan wrote:
> > 
> > - put the drivers private data into dev->priv
> > - when returning an error from probe, instead of freeing the data
> >   keep it.
> > - when probing again re-use the data.
> > 
> > With this drivers could be written like:
> > 
> > 	priv = dev->priv;
> > 	if (!priv) {
> > 		priv = xzalloc(sizeof(priv));
> > 		dev->priv = priv;
> > 	}
> > 
> > 	if (!priv->clk) {
> > 		clk = clk_get(dev, NULL);
> > 		if (IS_ERR(clk))
> > 			return PTR_ERR(clk);
> > 		priv->clk = clk;
> > 		clk_enable(priv->clk);
> > 	}
> > 
> > 	if (!priv->regulator) {
> > 		regulator = regulator_get(dev, "vmmc");
> > 		if (IS_ERR(regulator))
> > 			return PTR_ERR(regulator);
> > 		priv->regulator = regulator;
> > 	}
> > 
> > This way drivers could allocate the missing resources as needed without
> > freeing them between different calls to probe().
> 
> The idea is good, but I think that it will not be so easy to do.

Where do you expect problems?

> In addition, we obtain additional incompatibility with the kernel code.

I think this is not a problem in this case. The probe pathes of the
device drivers contain nearly the same elements, but are quite different
in the details anyway.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2014-05-19  8:48 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-08  7:31 [PATCH] fix i.MX51 babbage ethernet Sascha Hauer
2014-05-08  7:31 ` [PATCH 1/5] mfd: mc13xxx: Allow to set callback for mc13xxx Sascha Hauer
2014-05-18 23:14   ` Marc Reilly
2014-05-19  6:40     ` Sascha Hauer
2014-05-08  7:31 ` [PATCH 2/5] spi: i.MX: Move to coredevice_initcall Sascha Hauer
2014-05-11 18:49   ` Alexander Shiyan
2014-05-12 10:38     ` Sascha Hauer
2014-05-14 17:57       ` Alexander Shiyan
2014-05-19  8:47         ` Sascha Hauer
2014-05-08  7:31 ` [PATCH 3/5] mfd: mc13xxx: move " Sascha Hauer
2014-05-08  7:31 ` [PATCH 4/5] ARM: i.MX51 babbage: use mc13xxx_register_callback to initialize PMIC Sascha Hauer
2014-05-08  7:31 ` [PATCH 5/5] ARM: dts: i.MX51 babbage: overwrite upstream FEC iomux settings Sascha Hauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox