[PATCH 1/2] i2c: designware: Rework probe() to get clock a bit later

[PATCH 1/2] i2c: designware: Rework probe() to get clock a bit later

[Mika Westerberg]

In order to be able to create missing clock for AMD (and in future possibly
others) we move getting clock for the device a bit later. Also make ACPI/DT
configuration in the same place depending on from where the device was
enumerated from.

Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 54 ++++++++++++++---------------
 1 file changed, 26 insertions(+), 28 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index bc8773333155..7087b6ee97e2 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -81,9 +81,6 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
 	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
 	bool fs_mode = dev->master_cfg & DW_IC_CON_SPEED_FAST;
 
-	if (!ACPI_HANDLE(&pdev->dev))
-		return -ENODEV;
-
 	dev->adapter.nr = -1;
 	dev->tx_fifo_depth = 32;
 	dev->rx_fifo_depth = 32;
@@ -123,6 +120,7 @@ static int dw_i2c_probe(struct platform_device *pdev)
 	struct i2c_adapter *adap;
 	struct resource *mem;
 	int irq, r;
+	u32 ht = 0;
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
@@ -145,6 +143,29 @@ static int dw_i2c_probe(struct platform_device *pdev)
 	dev->irq = irq;
 	platform_set_drvdata(pdev, dev);
 
+	dev->functionality =
+		I2C_FUNC_I2C |
+		I2C_FUNC_10BIT_ADDR |
+		I2C_FUNC_SMBUS_BYTE |
+		I2C_FUNC_SMBUS_BYTE_DATA |
+		I2C_FUNC_SMBUS_WORD_DATA |
+		I2C_FUNC_SMBUS_I2C_BLOCK;
+	dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
+		DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
+
+	if (ACPI_COMPANION(&pdev->dev)) {
+		dw_i2c_acpi_configure(pdev);
+	} else if (pdev->dev.of_node) {
+		of_property_read_u32(pdev->dev.of_node,
+				     "i2c-sda-hold-time-ns", &ht);
+		of_property_read_u32(pdev->dev.of_node,
+				     "i2c-sda-falling-time-ns",
+				     &dev->sda_falling_time);
+		of_property_read_u32(pdev->dev.of_node,
+				     "i2c-scl-falling-time-ns",
+				     &dev->scl_falling_time);
+	}
+
 	dev->clk = devm_clk_get(&pdev->dev, NULL);
 	dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
 
@@ -152,36 +173,13 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(dev->clk);
 	clk_prepare_enable(dev->clk);
 
-	if (pdev->dev.of_node) {
-		u32 ht = 0;
+	if (!dev->sda_hold_time && ht) {
 		u32 ic_clk = dev->get_clk_rate_khz(dev);
-
-		of_property_read_u32(pdev->dev.of_node,
-					"i2c-sda-hold-time-ns", &ht);
 		dev->sda_hold_time = div_u64((u64)ic_clk * ht + 500000,
 					     1000000);
-
-		of_property_read_u32(pdev->dev.of_node,
-				     "i2c-sda-falling-time-ns",
-				     &dev->sda_falling_time);
-		of_property_read_u32(pdev->dev.of_node,
-				     "i2c-scl-falling-time-ns",
-				     &dev->scl_falling_time);
 	}
 
-	dev->functionality =
-		I2C_FUNC_I2C |
-		I2C_FUNC_10BIT_ADDR |
-		I2C_FUNC_SMBUS_BYTE |
-		I2C_FUNC_SMBUS_BYTE_DATA |
-		I2C_FUNC_SMBUS_WORD_DATA |
-		I2C_FUNC_SMBUS_I2C_BLOCK;
-	dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
-		DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
-
-	/* Try first if we can configure the device from ACPI */
-	r = dw_i2c_acpi_configure(pdev);
-	if (r) {
+	if (!dev->tx_fifo_depth) {
 		u32 param1 = i2c_dw_read_comp_param(dev);
 
 		dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
-- 
2.1.0

[PATCH 2/2] i2c: designware: Add support for AMD I2C controller

[Mika Westerberg]

From: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Add support for AMD version of the DW I2C host controller. The device is
enumerated from ACPI namespace with ACPI ID AMD0010. Because the core
driver needs an input source clock, and this is not an Intel LPSS device
where clocks are provided through drivers/acpi/acpi_lpss.c, we register the
clock ourselves if the clock rate is given in ->driver_data.

Signed-off-by: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/Kconfig                  |  1 +
 drivers/i2c/busses/i2c-designware-platdrv.c | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 2ac87fa3058d..9384498ef3c1 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -422,6 +422,7 @@ config I2C_DESIGNWARE_CORE
 
 config I2C_DESIGNWARE_PLATFORM
 	tristate "Synopsys DesignWare Platform"
+	depends on COMMON_CLK
 	select I2C_DESIGNWARE_CORE
 	help
 	  If you say yes to this option, support will be included for the
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 7087b6ee97e2..ecd0215d93c2 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -30,6 +30,7 @@
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/errno.h>
 #include <linux/sched.h>
 #include <linux/err.h>
@@ -80,6 +81,7 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
 {
 	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
 	bool fs_mode = dev->master_cfg & DW_IC_CON_SPEED_FAST;
+	const struct acpi_device_id *id;
 
 	dev->adapter.nr = -1;
 	dev->tx_fifo_depth = 32;
@@ -94,9 +96,29 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
 	dw_i2c_acpi_params(pdev, "FMCN", &dev->fs_hcnt, &dev->fs_lcnt,
 			   fs_mode ? &dev->sda_hold_time : NULL);
 
+	/*
+	 * Provide a way for Designware I2C host controllers that are not
+	 * based on Intel LPSS to specify their input clock frequency via
+	 * ->driver_data.
+	 */
+	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
+	if (id && id->driver_data)
+		clk_register_fixed_rate(&pdev->dev, dev_name(&pdev->dev), NULL,
+					CLK_IS_ROOT, id->driver_data);
+
 	return 0;
 }
 
+static void dw_i2c_acpi_unconfigure(struct platform_device *pdev)
+{
+	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
+	const struct acpi_device_id *id;
+
+	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
+	if (id && id->driver_data)
+		clk_unregister(dev->clk);
+}
+
 static const struct acpi_device_id dw_i2c_acpi_match[] = {
 	{ "INT33C2", 0 },
 	{ "INT33C3", 0 },
@@ -104,6 +126,7 @@ static const struct acpi_device_id dw_i2c_acpi_match[] = {
 	{ "INT3433", 0 },
 	{ "80860F41", 0 },
 	{ "808622C1", 0 },
+	{ "AMD0010", 133 * 1000 * 1000 },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, dw_i2c_acpi_match);
@@ -112,6 +135,7 @@ static inline int dw_i2c_acpi_configure(struct platform_device *pdev)
 {
 	return -ENODEV;
 }
+static inline void dw_i2c_acpi_unconfigure(struct platform_device *pdev) { }
 #endif
 
 static int dw_i2c_probe(struct platform_device *pdev)
@@ -235,6 +259,9 @@ static int dw_i2c_remove(struct platform_device *pdev)
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
+	if (ACPI_COMPANION(&pdev->dev))
+		dw_i2c_acpi_unconfigure(pdev);
+
 	return 0;
 }
 
-- 
2.1.0

Re: [PATCH 1/2] i2c: designware: Rework probe() to get clock a bit later

[carl peng]

On Thu, Sep 18, 2014 at 5:26 PM, Mika Westerberg
<mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> In order to be able to create missing clock for AMD (and in future possibly
> others) we move getting clock for the device a bit later. Also make ACPI/DT
> configuration in the same place depending on from where the device was
> enumerated from.
>
> Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Tested-by: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 54 ++++++++++++++---------------
>  1 file changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index bc8773333155..7087b6ee97e2 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -81,9 +81,6 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
>         struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
>         bool fs_mode = dev->master_cfg & DW_IC_CON_SPEED_FAST;
>
> -       if (!ACPI_HANDLE(&pdev->dev))
> -               return -ENODEV;
> -
>         dev->adapter.nr = -1;
>         dev->tx_fifo_depth = 32;
>         dev->rx_fifo_depth = 32;
> @@ -123,6 +120,7 @@ static int dw_i2c_probe(struct platform_device *pdev)
>         struct i2c_adapter *adap;
>         struct resource *mem;
>         int irq, r;
> +       u32 ht = 0;
>
>         irq = platform_get_irq(pdev, 0);
>         if (irq < 0) {
> @@ -145,6 +143,29 @@ static int dw_i2c_probe(struct platform_device *pdev)
>         dev->irq = irq;
>         platform_set_drvdata(pdev, dev);
>
> +       dev->functionality =
> +               I2C_FUNC_I2C |
> +               I2C_FUNC_10BIT_ADDR |
> +               I2C_FUNC_SMBUS_BYTE |
> +               I2C_FUNC_SMBUS_BYTE_DATA |
> +               I2C_FUNC_SMBUS_WORD_DATA |
> +               I2C_FUNC_SMBUS_I2C_BLOCK;
> +       dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
> +               DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
> +
> +       if (ACPI_COMPANION(&pdev->dev)) {
> +               dw_i2c_acpi_configure(pdev);
> +       } else if (pdev->dev.of_node) {
> +               of_property_read_u32(pdev->dev.of_node,
> +                                    "i2c-sda-hold-time-ns", &ht);
> +               of_property_read_u32(pdev->dev.of_node,
> +                                    "i2c-sda-falling-time-ns",
> +                                    &dev->sda_falling_time);
> +               of_property_read_u32(pdev->dev.of_node,
> +                                    "i2c-scl-falling-time-ns",
> +                                    &dev->scl_falling_time);
> +       }
> +
>         dev->clk = devm_clk_get(&pdev->dev, NULL);
>         dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
>
> @@ -152,36 +173,13 @@ static int dw_i2c_probe(struct platform_device *pdev)
>                 return PTR_ERR(dev->clk);
>         clk_prepare_enable(dev->clk);
>
> -       if (pdev->dev.of_node) {
> -               u32 ht = 0;
> +       if (!dev->sda_hold_time && ht) {
>                 u32 ic_clk = dev->get_clk_rate_khz(dev);
> -
> -               of_property_read_u32(pdev->dev.of_node,
> -                                       "i2c-sda-hold-time-ns", &ht);
>                 dev->sda_hold_time = div_u64((u64)ic_clk * ht + 500000,
>                                              1000000);
> -
> -               of_property_read_u32(pdev->dev.of_node,
> -                                    "i2c-sda-falling-time-ns",
> -                                    &dev->sda_falling_time);
> -               of_property_read_u32(pdev->dev.of_node,
> -                                    "i2c-scl-falling-time-ns",
> -                                    &dev->scl_falling_time);
>         }
>
> -       dev->functionality =
> -               I2C_FUNC_I2C |
> -               I2C_FUNC_10BIT_ADDR |
> -               I2C_FUNC_SMBUS_BYTE |
> -               I2C_FUNC_SMBUS_BYTE_DATA |
> -               I2C_FUNC_SMBUS_WORD_DATA |
> -               I2C_FUNC_SMBUS_I2C_BLOCK;
> -       dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
> -               DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
> -
> -       /* Try first if we can configure the device from ACPI */
> -       r = dw_i2c_acpi_configure(pdev);
> -       if (r) {
> +       if (!dev->tx_fifo_depth) {
>                 u32 param1 = i2c_dw_read_comp_param(dev);
>
>                 dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
> --
> 2.1.0
>

Re: [PATCH 1/2] i2c: designware: Rework probe() to get clock a bit later

[Mika Westerberg]

On Fri, Sep 19, 2014 at 03:08:58PM +0800, carl peng wrote:
> On Thu, Sep 18, 2014 at 5:26 PM, Mika Westerberg
> <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> > In order to be able to create missing clock for AMD (and in future possibly
> > others) we move getting clock for the device a bit later. Also make ACPI/DT
> > configuration in the same place depending on from where the device was
> > enumerated from.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> 
> Tested-by: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Thanks!

Re: [PATCH 1/2] i2c: designware: Rework probe() to get clock a bit later

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 448 bytes --]

On Thu, Sep 18, 2014 at 12:26:06PM +0300, Mika Westerberg wrote:
> In order to be able to create missing clock for AMD (and in future possibly
> others) we move getting clock for the device a bit later. Also make ACPI/DT
> configuration in the same place depending on from where the device was
> enumerated from.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Applied to for-next, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]

On Thu, Sep 18, 2014 at 12:26:07PM +0300, Mika Westerberg wrote:
> From: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Add support for AMD version of the DW I2C host controller. The device is
> enumerated from ACPI namespace with ACPI ID AMD0010. Because the core
> driver needs an input source clock, and this is not an Intel LPSS device
> where clocks are provided through drivers/acpi/acpi_lpss.c, we register the
> clock ourselves if the clock rate is given in ->driver_data.
> 
> Signed-off-by: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---

Applied to for-next, still wondering...

>  drivers/i2c/busses/Kconfig                  |  1 +
>  drivers/i2c/busses/i2c-designware-platdrv.c | 27 +++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 2ac87fa3058d..9384498ef3c1 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -422,6 +422,7 @@ config I2C_DESIGNWARE_CORE
>  
>  config I2C_DESIGNWARE_PLATFORM
>  	tristate "Synopsys DesignWare Platform"
> +	depends on COMMON_CLK

... do all previous users have COMMON_CLK?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller

[Mika Westerberg]

On Sat, Sep 20, 2014 at 11:36:34AM +0200, Wolfram Sang wrote:
> On Thu, Sep 18, 2014 at 12:26:07PM +0300, Mika Westerberg wrote:
> > From: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > 
> > Add support for AMD version of the DW I2C host controller. The device is
> > enumerated from ACPI namespace with ACPI ID AMD0010. Because the core
> > driver needs an input source clock, and this is not an Intel LPSS device
> > where clocks are provided through drivers/acpi/acpi_lpss.c, we register the
> > clock ourselves if the clock rate is given in ->driver_data.
> > 
> > Signed-off-by: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > ---
> 
> Applied to for-next, still wondering...

Thanks!

> 
> >  drivers/i2c/busses/Kconfig                  |  1 +
> >  drivers/i2c/busses/i2c-designware-platdrv.c | 27 +++++++++++++++++++++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 2ac87fa3058d..9384498ef3c1 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -422,6 +422,7 @@ config I2C_DESIGNWARE_CORE
> >  
> >  config I2C_DESIGNWARE_PLATFORM
> >  	tristate "Synopsys DesignWare Platform"
> > +	depends on COMMON_CLK
> 
> ... do all previous users have COMMON_CLK?

The driver is being used on x86, ARM and ARC it seems. For x86 and ARM
we pretty much have COMMON_CLK nowadays but I'm not sure about ARC.
That's why I have copied Christian Ruppert.

Christian,

Do you see problems on your side if we depend on COMMON_CLK?

Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller

[Christian Ruppert]

Dear Mika,

On Mon, Sep 22, 2014 at 12:12:07PM +0300, Mika Westerberg wrote:
> On Sat, Sep 20, 2014 at 11:36:34AM +0200, Wolfram Sang wrote:
> > On Thu, Sep 18, 2014 at 12:26:07PM +0300, Mika Westerberg wrote:
> > > From: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > 
> > > Add support for AMD version of the DW I2C host controller. The device is
> > > enumerated from ACPI namespace with ACPI ID AMD0010. Because the core
> > > driver needs an input source clock, and this is not an Intel LPSS device
> > > where clocks are provided through drivers/acpi/acpi_lpss.c, we register the
> > > clock ourselves if the clock rate is given in ->driver_data.
> > > 
> > > Signed-off-by: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > > ---
> > 
> > Applied to for-next, still wondering...
> 
> Thanks!
> 
> > 
> > >  drivers/i2c/busses/Kconfig                  |  1 +
> > >  drivers/i2c/busses/i2c-designware-platdrv.c | 27 +++++++++++++++++++++++++++
> > >  2 files changed, 28 insertions(+)
> > > 
> > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > > index 2ac87fa3058d..9384498ef3c1 100644
> > > --- a/drivers/i2c/busses/Kconfig
> > > +++ b/drivers/i2c/busses/Kconfig
> > > @@ -422,6 +422,7 @@ config I2C_DESIGNWARE_CORE
> > >  
> > >  config I2C_DESIGNWARE_PLATFORM
> > >  	tristate "Synopsys DesignWare Platform"
> > > +	depends on COMMON_CLK
> > 
> > ... do all previous users have COMMON_CLK?
> 
> The driver is being used on x86, ARM and ARC it seems. For x86 and ARM
> we pretty much have COMMON_CLK nowadays but I'm not sure about ARC.
> That's why I have copied Christian Ruppert.
> 
> Christian,
> 
> Do you see problems on your side if we depend on COMMON_CLK?

COMMON_CLK is not selected by the ARC architecture in general. However,
we do select COMMON_CLK in the TB10x platform which uses the designware
I2C driver so this new dependency is no problem for us.

Vineet,

Do you see any issues with this on other existing ARC platforms, e.g.
arcfpga?

Greetings,
  Christian

Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller

[Vineet Gupta]

On Monday 22 September 2014 05:59 PM, Christian Ruppert wrote:
> Dear Mika,
>
> On Mon, Sep 22, 2014 at 12:12:07PM +0300, Mika Westerberg wrote:
>> On Sat, Sep 20, 2014 at 11:36:34AM +0200, Wolfram Sang wrote:
>>> On Thu, Sep 18, 2014 at 12:26:07PM +0300, Mika Westerberg wrote:
>>>> From: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>
>>>> Add support for AMD version of the DW I2C host controller. The device is
>>>> enumerated from ACPI namespace with ACPI ID AMD0010. Because the core
>>>> driver needs an input source clock, and this is not an Intel LPSS device
>>>> where clocks are provided through drivers/acpi/acpi_lpss.c, we register the
>>>> clock ourselves if the clock rate is given in ->driver_data.
>>>>
>>>> Signed-off-by: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>>> ---
>>> Applied to for-next, still wondering...
>> Thanks!
>>
>>>>  drivers/i2c/busses/Kconfig                  |  1 +
>>>>  drivers/i2c/busses/i2c-designware-platdrv.c | 27 +++++++++++++++++++++++++++
>>>>  2 files changed, 28 insertions(+)
>>>>
>>>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>>>> index 2ac87fa3058d..9384498ef3c1 100644
>>>> --- a/drivers/i2c/busses/Kconfig
>>>> +++ b/drivers/i2c/busses/Kconfig
>>>> @@ -422,6 +422,7 @@ config I2C_DESIGNWARE_CORE
>>>>  
>>>>  config I2C_DESIGNWARE_PLATFORM
>>>>  	tristate "Synopsys DesignWare Platform"
>>>> +	depends on COMMON_CLK
>>> ... do all previous users have COMMON_CLK?
>> The driver is being used on x86, ARM and ARC it seems. For x86 and ARM
>> we pretty much have COMMON_CLK nowadays but I'm not sure about ARC.
>> That's why I have copied Christian Ruppert.
>>
>> Christian,
>>
>> Do you see problems on your side if we depend on COMMON_CLK?
> COMMON_CLK is not selected by the ARC architecture in general. However,
> we do select COMMON_CLK in the TB10x platform which uses the designware
> I2C driver so this new dependency is no problem for us.
>
> Vineet,
>
> Do you see any issues with this on other existing ARC platforms, e.g.
> arcfpga?

So what needs to be done, COMMON_CLK needs to be defined in arch/arc/Kconfig ? And
if so why ?
OTOH, if we do have to, I don't see the issue with it  - we just didn't need it
for the legacy platform.

-Vineet

Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller

[Mika Westerberg]

On Mon, Sep 22, 2014 at 01:48:57PM +0000, Vineet Gupta wrote:
> On Monday 22 September 2014 05:59 PM, Christian Ruppert wrote:
> > Dear Mika,
> >
> > On Mon, Sep 22, 2014 at 12:12:07PM +0300, Mika Westerberg wrote:
> >> On Sat, Sep 20, 2014 at 11:36:34AM +0200, Wolfram Sang wrote:
> >>> On Thu, Sep 18, 2014 at 12:26:07PM +0300, Mika Westerberg wrote:
> >>>> From: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>>>
> >>>> Add support for AMD version of the DW I2C host controller. The device is
> >>>> enumerated from ACPI namespace with ACPI ID AMD0010. Because the core
> >>>> driver needs an input source clock, and this is not an Intel LPSS device
> >>>> where clocks are provided through drivers/acpi/acpi_lpss.c, we register the
> >>>> clock ourselves if the clock rate is given in ->driver_data.
> >>>>
> >>>> Signed-off-by: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>>> Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> >>>> ---
> >>> Applied to for-next, still wondering...
> >> Thanks!
> >>
> >>>>  drivers/i2c/busses/Kconfig                  |  1 +
> >>>>  drivers/i2c/busses/i2c-designware-platdrv.c | 27 +++++++++++++++++++++++++++
> >>>>  2 files changed, 28 insertions(+)
> >>>>
> >>>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> >>>> index 2ac87fa3058d..9384498ef3c1 100644
> >>>> --- a/drivers/i2c/busses/Kconfig
> >>>> +++ b/drivers/i2c/busses/Kconfig
> >>>> @@ -422,6 +422,7 @@ config I2C_DESIGNWARE_CORE
> >>>>  
> >>>>  config I2C_DESIGNWARE_PLATFORM
> >>>>  	tristate "Synopsys DesignWare Platform"
> >>>> +	depends on COMMON_CLK
> >>> ... do all previous users have COMMON_CLK?
> >> The driver is being used on x86, ARM and ARC it seems. For x86 and ARM
> >> we pretty much have COMMON_CLK nowadays but I'm not sure about ARC.
> >> That's why I have copied Christian Ruppert.
> >>
> >> Christian,
> >>
> >> Do you see problems on your side if we depend on COMMON_CLK?
> > COMMON_CLK is not selected by the ARC architecture in general. However,
> > we do select COMMON_CLK in the TB10x platform which uses the designware
> > I2C driver so this new dependency is no problem for us.
> >
> > Vineet,
> >
> > Do you see any issues with this on other existing ARC platforms, e.g.
> > arcfpga?
> 
> So what needs to be done, COMMON_CLK needs to be defined in arch/arc/Kconfig ? And
> if so why ?

Without COMMON_CLK, you are not able to select I2C_DESIGNWARE_PLATFORM
anymore. So if something on ARC depends on this driver then we either
need the COMMON_CLK there or figure out alternative way to fix Carl's
problem.

> OTOH, if we do have to, I don't see the issue with it  - we just didn't need it
> for the legacy platform.

Is the legacy platform using this driver? Grepping "i2c_designware"
returns zero board files so at least that should be covered.

Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller

[Vineet Gupta]

On Monday 22 September 2014 07:30 PM, Mika Westerberg wrote:
>>> COMMON_CLK is not selected by the ARC architecture in general. However,
>>> > > we do select COMMON_CLK in the TB10x platform which uses the designware
>>> > > I2C driver so this new dependency is no problem for us.
>>> > >
>>> > > Vineet,
>>> > >
>>> > > Do you see any issues with this on other existing ARC platforms, e.g.
>>> > > arcfpga?
>> > 
>> > So what needs to be done, COMMON_CLK needs to be defined in arch/arc/Kconfig ? And
>> > if so why ?
> Without COMMON_CLK, you are not able to select I2C_DESIGNWARE_PLATFORM
> anymore. So if something on ARC depends on this driver then we either
> need the COMMON_CLK there or figure out alternative way to fix Carl's
> problem.

I have not seen the orig patch, but it seems COMMON_CLK is already being selected
by TB10x, do we still need it in arch/arcKconfig, for all ARC platforms ?

>> > OTOH, if we do have to, I don't see the issue with it  - we just didn't need it
>> > for the legacy platform.
> Is the legacy platform using this driver? Grepping "i2c_designware"
> returns zero board files so at least that should be covered.
>

Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller

[Mika Westerberg]

On Mon, Sep 22, 2014 at 02:16:25PM +0000, Vineet Gupta wrote:
> On Monday 22 September 2014 07:30 PM, Mika Westerberg wrote:
> >>> COMMON_CLK is not selected by the ARC architecture in general. However,
> >>> > > we do select COMMON_CLK in the TB10x platform which uses the designware
> >>> > > I2C driver so this new dependency is no problem for us.
> >>> > >
> >>> > > Vineet,
> >>> > >
> >>> > > Do you see any issues with this on other existing ARC platforms, e.g.
> >>> > > arcfpga?
> >> > 
> >> > So what needs to be done, COMMON_CLK needs to be defined in arch/arc/Kconfig ? And
> >> > if so why ?
> > Without COMMON_CLK, you are not able to select I2C_DESIGNWARE_PLATFORM
> > anymore. So if something on ARC depends on this driver then we either
> > need the COMMON_CLK there or figure out alternative way to fix Carl's
> > problem.
> 
> I have not seen the orig patch, but it seems COMMON_CLK is already being selected
> by TB10x, do we still need it in arch/arcKconfig, for all ARC platforms ?

Only if the driver is being used by something else than TB10x on ARC. So
that we don't cause regressions to existing users.

Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller

[Christian Ruppert]

On Mon, Sep 22, 2014 at 02:16:25PM +0000, Vineet Gupta wrote:
> On Monday 22 September 2014 07:30 PM, Mika Westerberg wrote:
> >>> COMMON_CLK is not selected by the ARC architecture in general. However,
> >>> > > we do select COMMON_CLK in the TB10x platform which uses the designware
> >>> > > I2C driver so this new dependency is no problem for us.
> >>> > >
> >>> > > Vineet,
> >>> > >
> >>> > > Do you see any issues with this on other existing ARC platforms, e.g.
> >>> > > arcfpga?
> >> > 
> >> > So what needs to be done, COMMON_CLK needs to be defined in arch/arc/Kconfig ? And
> >> > if so why ?
> > Without COMMON_CLK, you are not able to select I2C_DESIGNWARE_PLATFORM
> > anymore. So if something on ARC depends on this driver then we either
> > need the COMMON_CLK there or figure out alternative way to fix Carl's
> > problem.
> 
> I have not seen the orig patch, but it seems COMMON_CLK is already being selected
> by TB10x, do we still need it in arch/arcKconfig, for all ARC platforms ?

Sorry for the confusion, I should have given you some context. Mika
has checked that designware i2c is used by some ARC platforms but he
didn't say which ones. The orig patch makes COMMON_CLK a requirement for
designware i2c.

I checked that we're fine for TB10x (we need COMMON_CLK anyway) but
since you weren't in copy I just wanted to make sure that none of the
other ARC platforms (which don't seem to select COMMON_CLK) use
designware i2c and thus run into trouble.

If there is no designware i2c in any of your platforms, simply ignore
my message. From my point of view there is no need to move "select
COMMON_CLK" up to the ARC architecture level.

Greetings,
  Christian

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller

[Mika Westerberg]

On Mon, Sep 22, 2014 at 07:22:52PM +0200, Christian Ruppert wrote:
> On Mon, Sep 22, 2014 at 02:16:25PM +0000, Vineet Gupta wrote:
> > On Monday 22 September 2014 07:30 PM, Mika Westerberg wrote:
> > >>> COMMON_CLK is not selected by the ARC architecture in general. However,
> > >>> > > we do select COMMON_CLK in the TB10x platform which uses the designware
> > >>> > > I2C driver so this new dependency is no problem for us.
> > >>> > >
> > >>> > > Vineet,
> > >>> > >
> > >>> > > Do you see any issues with this on other existing ARC platforms, e.g.
> > >>> > > arcfpga?
> > >> > 
> > >> > So what needs to be done, COMMON_CLK needs to be defined in arch/arc/Kconfig ? And
> > >> > if so why ?
> > > Without COMMON_CLK, you are not able to select I2C_DESIGNWARE_PLATFORM
> > > anymore. So if something on ARC depends on this driver then we either
> > > need the COMMON_CLK there or figure out alternative way to fix Carl's
> > > problem.
> > 
> > I have not seen the orig patch, but it seems COMMON_CLK is already being selected
> > by TB10x, do we still need it in arch/arcKconfig, for all ARC platforms ?
> 
> Sorry for the confusion, I should have given you some context. Mika
> has checked that designware i2c is used by some ARC platforms but he
> didn't say which ones. The orig patch makes COMMON_CLK a requirement for
> designware i2c.

Yeah, sorry about not mentioning the affected platforms. I've included
them now in this email.

> I checked that we're fine for TB10x (we need COMMON_CLK anyway) but
> since you weren't in copy I just wanted to make sure that none of the
> other ARC platforms (which don't seem to select COMMON_CLK) use
> designware i2c and thus run into trouble.
> 
> If there is no designware i2c in any of your platforms, simply ignore
> my message. From my point of view there is no need to move "select
> COMMON_CLK" up to the ARC architecture level.

I grepped for snps,i2c-designware from the kernel source tree and this
is what I found:

Only ARC that is using the driver:

 arch/arc/boot/dts/abilis_tb10x.dtsi -> OK

Two ARMs that pass clock to the driver:

 arch/arm/boot/dts/berlin2q.dtsi -> OK
 arch/arm/boot/dts/socfpga.dts -> OK

Following files have the designware driver listed but it seems to be
disabled and does not get any clocks:

 arch/arm/boot/dts/nspire-cx.dts

 arch/arm/boot/dts/spear1310.dtsi
 arch/arm/boot/dts/spear320.dtsi
 arch/arm/boot/dts/spear3xx.dtsi
 arch/arm/boot/dts/spear600.dtsi

I think we are good to go with the first three but not sure about the
last five. Adding Viresh Kumar and Daniel Tang to the thread.

Viresh, Daniel,

The thread is here http://patchwork.ozlabs.org/patch/390695/.

In summary, we are adding COMMON_CLK dependency to the
i2c-designware-platdrv.c in order to get clocks to one AMD DW based host
controller and wanted to check that we don't break the existing users
(nspire and SPEAR).

Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller

[Vineet Gupta]

Hi Chrisitian,

On Monday 22 September 2014 10:53 PM, Christian Ruppert wrote:
> If there is no designware i2c in any of your platforms, simply ignore
> my message. From my point of view there is no need to move "select
> COMMON_CLK" up to the ARC architecture level.

I still ended up moving COMMON_CLK out of tb10x to arch/arc - due to consolidation
of .init_machine() callbacks - from platform to arc core.

Give linux-next a spin (perhaps later today as I just pushed a build error in that
area).

You'll like the almost empty platform file :-)

-Vineet

Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller

[Viresh Kumar]

On Tue, Sep 23, 2014 at 3:05 AM, Mika Westerberg
<mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>  arch/arm/boot/dts/spear1310.dtsi
>  arch/arm/boot/dts/spear320.dtsi
>  arch/arm/boot/dts/spear3xx.dtsi
>  arch/arm/boot/dts/spear600.dtsi
>
> I think we are good to go with the first three but not sure about the
> last five. Adding Viresh Kumar and Daniel Tang to the thread.
>
> Viresh, Daniel,
>
> The thread is here http://patchwork.ozlabs.org/patch/390695/.
>
> In summary, we are adding COMMON_CLK dependency to the
> i2c-designware-platdrv.c in order to get clocks to one AMD DW based host
> controller and wanted to check that we don't break the existing users
> (nspire and SPEAR).

A dependency on COMMON_CLK is fine for SPEAr .

Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2403 bytes --]

On Mon, Sep 22, 2014 at 12:12:07PM +0300, Mika Westerberg wrote:
> On Sat, Sep 20, 2014 at 11:36:34AM +0200, Wolfram Sang wrote:
> > On Thu, Sep 18, 2014 at 12:26:07PM +0300, Mika Westerberg wrote:
> > > From: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > 
> > > Add support for AMD version of the DW I2C host controller. The device is
> > > enumerated from ACPI namespace with ACPI ID AMD0010. Because the core
> > > driver needs an input source clock, and this is not an Intel LPSS device
> > > where clocks are provided through drivers/acpi/acpi_lpss.c, we register the
> > > clock ourselves if the clock rate is given in ->driver_data.
> > > 
> > > Signed-off-by: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > > ---
> > 
> > Applied to for-next, still wondering...
> 
> Thanks!

I reconsidered and these two patches are not in i2c/for-next because of
two reasons:

1) They do not apply cleanly on top of other i2c-designware changes I
applied (check i2c/for-next I just pushed out). The conflicts might not
be hard, but they were not trivial enough for me to do them inbetween
other things. I'd be very happy about a repost on top of i2c/for-next.

> 
> > 
> > >  drivers/i2c/busses/Kconfig                  |  1 +
> > >  drivers/i2c/busses/i2c-designware-platdrv.c | 27 +++++++++++++++++++++++++++
> > >  2 files changed, 28 insertions(+)
> > > 
> > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > > index 2ac87fa3058d..9384498ef3c1 100644
> > > --- a/drivers/i2c/busses/Kconfig
> > > +++ b/drivers/i2c/busses/Kconfig
> > > @@ -422,6 +422,7 @@ config I2C_DESIGNWARE_CORE
> > >  
> > >  config I2C_DESIGNWARE_PLATFORM
> > >  	tristate "Synopsys DesignWare Platform"
> > > +	depends on COMMON_CLK
> > 
> > ... do all previous users have COMMON_CLK?
> 
> The driver is being used on x86, ARM and ARC it seems. For x86 and ARM
> we pretty much have COMMON_CLK nowadays but I'm not sure about ARC.

2) "pretty much have" is not so convincing to me. That is such a generic
core, it probably has enough out-of-tree users as well. And with all
these ACPI regressions from this cycle, I am very cautious right now.
Brainstorming: What about "depends on (ACPI && COMMON_CLK) || !ACPI"?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller

[Mika Westerberg]

On Mon, Sep 29, 2014 at 11:24:50PM +0200, Wolfram Sang wrote:
> On Mon, Sep 22, 2014 at 12:12:07PM +0300, Mika Westerberg wrote:
> > On Sat, Sep 20, 2014 at 11:36:34AM +0200, Wolfram Sang wrote:
> > > On Thu, Sep 18, 2014 at 12:26:07PM +0300, Mika Westerberg wrote:
> > > > From: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > 
> > > > Add support for AMD version of the DW I2C host controller. The device is
> > > > enumerated from ACPI namespace with ACPI ID AMD0010. Because the core
> > > > driver needs an input source clock, and this is not an Intel LPSS device
> > > > where clocks are provided through drivers/acpi/acpi_lpss.c, we register the
> > > > clock ourselves if the clock rate is given in ->driver_data.
> > > > 
> > > > Signed-off-by: Carl Peng <carlpeng008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > > > ---
> > > 
> > > Applied to for-next, still wondering...
> > 
> > Thanks!
> 
> I reconsidered and these two patches are not in i2c/for-next because of
> two reasons:
> 
> 1) They do not apply cleanly on top of other i2c-designware changes I
> applied (check i2c/for-next I just pushed out). The conflicts might not
> be hard, but they were not trivial enough for me to do them inbetween
> other things. I'd be very happy about a repost on top of i2c/for-next.

OK, I'll do that.

> > > 
> > > >  drivers/i2c/busses/Kconfig                  |  1 +
> > > >  drivers/i2c/busses/i2c-designware-platdrv.c | 27 +++++++++++++++++++++++++++
> > > >  2 files changed, 28 insertions(+)
> > > > 
> > > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > > > index 2ac87fa3058d..9384498ef3c1 100644
> > > > --- a/drivers/i2c/busses/Kconfig
> > > > +++ b/drivers/i2c/busses/Kconfig
> > > > @@ -422,6 +422,7 @@ config I2C_DESIGNWARE_CORE
> > > >  
> > > >  config I2C_DESIGNWARE_PLATFORM
> > > >  	tristate "Synopsys DesignWare Platform"
> > > > +	depends on COMMON_CLK
> > > 
> > > ... do all previous users have COMMON_CLK?
> > 
> > The driver is being used on x86, ARM and ARC it seems. For x86 and ARM
> > we pretty much have COMMON_CLK nowadays but I'm not sure about ARC.
> 
> 2) "pretty much have" is not so convincing to me. That is such a generic
> core, it probably has enough out-of-tree users as well.

I wasn't aware that we care about out-of-tree users.

> And with all these ACPI regressions from this cycle, I am very
> cautious right now.  Brainstorming: What about "depends on (ACPI &&
> COMMON_CLK) || !ACPI"?

Understood. I'll try the above.

Thanks.

Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 902 bytes --]


> > 1) They do not apply cleanly on top of other i2c-designware changes I
> > applied (check i2c/for-next I just pushed out). The conflicts might not
> > be hard, but they were not trivial enough for me to do them inbetween
> > other things. I'd be very happy about a repost on top of i2c/for-next.
> 
> OK, I'll do that.

Great, much appreciated!

> > 2) "pretty much have" is not so convincing to me. That is such a generic
> > core, it probably has enough out-of-tree users as well.
> 
> I wasn't aware that we care about out-of-tree users.

This is one example coming from a feeling that we should at least try to
make this change having less impact.

> > And with all these ACPI regressions from this cycle, I am very
> > cautious right now.  Brainstorming: What about "depends on (ACPI &&
> > COMMON_CLK) || !ACPI"?
> 
> Understood. I'll try the above.

Cool, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller

[Christian Ruppert]

Hi Vineet,

On Fri, Sep 26, 2014 at 09:20:20AM +0530, Vineet Gupta wrote:
> Hi Chrisitian,
> 
> On Monday 22 September 2014 10:53 PM, Christian Ruppert wrote:
> > If there is no designware i2c in any of your platforms, simply ignore
> > my message. From my point of view there is no need to move "select
> > COMMON_CLK" up to the ARC architecture level.
> 
> I still ended up moving COMMON_CLK out of tb10x to arch/arc - due to consolidation
> of .init_machine() callbacks - from platform to arc core.
> 
> Give linux-next a spin (perhaps later today as I just pushed a build error in that
> area).
> 
> You'll like the almost empty platform file :-)

This looks very nice indeed. Support for "standard" platforms is simple
and elegant. The fact that most platforms won't need any specific
support code at all (apart from drivers) goes well with the idea of
device tree.  However, the kernel-doc comment for init_machine in
mach_desc.h is now slightly confusing (still mentioning device tree).

With this patch there remains only a single detail we need to manage
through platform-specific code: the reset handler. Today we still
provide a patch for the machine_restart function in reset.c to our
customers so that rebooting from the command line works. Do you have any
plans/ideas to fix this one as well?

Greetings,
  Christian

arc platform code updates (was Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller)

[Vineet Gupta]

+CC Guenter Roeck

On Tuesday 07 October 2014 03:00 PM, Christian Ruppert wrote:
> Hi Vineet,
>
> On Fri, Sep 26, 2014 at 09:20:20AM +0530, Vineet Gupta wrote:
>> Hi Chrisitian,
>>
>> On Monday 22 September 2014 10:53 PM, Christian Ruppert wrote:
>>> If there is no designware i2c in any of your platforms, simply ignore
>>> my message. From my point of view there is no need to move "select
>>> COMMON_CLK" up to the ARC architecture level.
>> I still ended up moving COMMON_CLK out of tb10x to arch/arc - due to consolidation
>> of .init_machine() callbacks - from platform to arc core.
>>
>> Give linux-next a spin (perhaps later today as I just pushed a build error in that
>> area).
>>
>> You'll like the almost empty platform file :-)
> This looks very nice indeed. Support for "standard" platforms is simple
> and elegant. The fact that most platforms won't need any specific
> support code at all (apart from drivers) goes well with the idea of
> device tree.  However, the kernel-doc comment for init_machine in
> mach_desc.h is now slightly confusing (still mentioning device tree).

A platform of future can still call of_platform_populate() etc to reparse the
stuff for say it's platform devices !
So I would think it is still relevant !

> With this patch there remains only a single detail we need to manage
> through platform-specific code: the reset handler. Today we still
> provide a patch for the machine_restart function in reset.c to our
> customers so that rebooting from the command line works. Do you have any
> plans/ideas to fix this one as well?

Patches are welcome ;-)

ATM, I dont have a specific use-case for my current platforms, so can't write the
code - you can propose a patch and then we can work out what's best in general for
all ARC platforms. BTW there's a series in flight on related topic from Guenter so
please take a look at that too for big picture !

http://www.spinics.net/linux/lists/kernel/msg1840650.html

Cheers,
-Vineet

>
> Greetings,
>   Christian
>

Re: arc platform code updates (was Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller)

[Christian Ruppert]

On Tue, Oct 07, 2014 at 12:35:29PM +0000, Vineet Gupta wrote:
> +CC Guenter Roeck
> [...]
> > However, the kernel-doc comment for init_machine in
> > mach_desc.h is now slightly confusing (still mentioning device tree).
> 
> A platform of future can still call of_platform_populate() etc to reparse the
> stuff for say it's platform devices !
> So I would think it is still relevant !

OK.

> > With this patch there remains only a single detail we need to manage
> > through platform-specific code: the reset handler. Today we still
> > provide a patch for the machine_restart function in reset.c to our
> > customers so that rebooting from the command line works. Do you have any
> > plans/ideas to fix this one as well?
> 
> Patches are welcome ;-)
> 
> ATM, I dont have a specific use-case for my current platforms, so can't write the
> code - you can propose a patch and then we can work out what's best in general for
> all ARC platforms. BTW there's a series in flight on related topic from Guenter so
> please take a look at that too for big picture !
> 
> http://www.spinics.net/linux/lists/kernel/msg1840650.html

Actually, before sending my previous mail I looked at the current
implementation of the halt hook and didn't like it (otherwise I would
have proposed something in the lines). So this one is definitely a step
forward! I'm wondering about two things concerning reset, however:

1. Is the PM module the right place for a reset handler? On the one hand
   reset is functionally very similar to power off but on the other hand
   reset is technically not a power management functionality. If the PM
   module is not the right place, which would be the right place
   instead?

2. What would be the desired behaviour/semantics for a reset handler
   chain equivalent to the power off handler chain. I see two
   possibilities here:
   a) Implementation exactly like power off. Every handler is expected
      to reset the entire system and never returns.
   b) A more "modular" implementation where different subsystems are
      being reset sequentially (e.g. first reset peripherals through
      GPIO in "high priority" handlers and finally reset the core in the
      terminal "low priority" handler).

Greetings,
  Christian

Re: arc platform code updates (was Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller)

[Guenter Roeck]

On 10/10/2014 02:13 AM, Christian Ruppert wrote:
> On Tue, Oct 07, 2014 at 12:35:29PM +0000, Vineet Gupta wrote:
>> +CC Guenter Roeck
>> [...]
>>> However, the kernel-doc comment for init_machine in
>>> mach_desc.h is now slightly confusing (still mentioning device tree).
>>
>> A platform of future can still call of_platform_populate() etc to reparse the
>> stuff for say it's platform devices !
>> So I would think it is still relevant !
>
> OK.
>
>>> With this patch there remains only a single detail we need to manage
>>> through platform-specific code: the reset handler. Today we still
>>> provide a patch for the machine_restart function in reset.c to our
>>> customers so that rebooting from the command line works. Do you have any
>>> plans/ideas to fix this one as well?
>>
>> Patches are welcome ;-)
>>
>> ATM, I dont have a specific use-case for my current platforms, so can't write the
>> code - you can propose a patch and then we can work out what's best in general for
>> all ARC platforms. BTW there's a series in flight on related topic from Guenter so
>> please take a look at that too for big picture !
>>
>> http://www.spinics.net/linux/lists/kernel/msg1840650.html
>
> Actually, before sending my previous mail I looked at the current
> implementation of the halt hook and didn't like it (otherwise I would
> have proposed something in the lines). So this one is definitely a step
> forward! I'm wondering about two things concerning reset, however:
>
> 1. Is the PM module the right place for a reset handler? On the one hand
>     reset is functionally very similar to power off but on the other hand
>     reset is technically not a power management functionality. If the PM
>     module is not the right place, which would be the right place
>     instead?
>
> 2. What would be the desired behaviour/semantics for a reset handler
>     chain equivalent to the power off handler chain. I see two
>     possibilities here:
>     a) Implementation exactly like power off. Every handler is expected
>        to reset the entire system and never returns.
>     b) A more "modular" implementation where different subsystems are
>        being reset sequentially (e.g. first reset peripherals through
>        GPIO in "high priority" handlers and finally reset the core in the
>        terminal "low priority" handler).
>

Hi Christian,

The restart handler patch series will hopefully make it into 3.18.
It will not be in kernel/power/ but in kernel/reboot.c which seemed
to be more appropriate. The semantics is exactly the same as for the
poweroff handler. Actually, the poweroff handler code is derived
from the restart handler code. Implementation is like 2a).

The restart handler code is available here:

https://git.kernel.org/cgit/linux/kernel/git/groeck/linux-staging.git/log/?h=restart-handler

There is also a tag, restart-handler-for-v3.18, which you can use to merge
the series into your tree if needed.

Hope this helps,
Guenter