[PATCH 08/12] mpc5121: Added I2C support.

[PATCH 08/12] mpc5121: Added I2C support.

[Wolfgang Denk]

From: Piotr Ziecik <kosmo-nYOzD4b6Jr9Wk0Htik3J/w@public.gmane.org>

- Enabled I2C interrupts on MPC5121.
- Updated Kconfig for i2c-mpc driver.

Signed-off-by: Piotr Ziecik <kosmo-nYOzD4b6Jr9Wk0Htik3J/w@public.gmane.org>
Signed-off-by: Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org>
Cc: <linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Cc: John Rigby <jcrigby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/powerpc/platforms/512x/mpc5121_ads.c    |    2 ++
 arch/powerpc/platforms/512x/mpc512x.h        |    1 +
 arch/powerpc/platforms/512x/mpc512x_shared.c |   24 ++++++++++++++++++++++++
 drivers/i2c/busses/Kconfig                   |    9 +++++----
 4 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/512x/mpc5121_ads.c b/arch/powerpc/platforms/512x/mpc5121_ads.c
index 441abc4..a8976b4 100644
--- a/arch/powerpc/platforms/512x/mpc5121_ads.c
+++ b/arch/powerpc/platforms/512x/mpc5121_ads.c
@@ -42,6 +42,8 @@ static void __init mpc5121_ads_setup_arch(void)
 	for_each_compatible_node(np, "pci", "fsl,mpc5121-pci")
 		mpc83xx_add_bridge(np);
 #endif
+
+	mpc512x_init_i2c();
 }
 
 static void __init mpc5121_ads_init_IRQ(void)
diff --git a/arch/powerpc/platforms/512x/mpc512x.h b/arch/powerpc/platforms/512x/mpc512x.h
index 9c03693..f4db8a7 100644
--- a/arch/powerpc/platforms/512x/mpc512x.h
+++ b/arch/powerpc/platforms/512x/mpc512x.h
@@ -13,5 +13,6 @@
 #define __MPC512X_H__
 extern unsigned long mpc512x_find_ips_freq(struct device_node *node);
 extern void __init mpc512x_init_IRQ(void);
+extern void __init mpc512x_init_i2c(void);
 void __init mpc512x_declare_of_platform_devices(void);
 #endif				/* __MPC512X_H__ */
diff --git a/arch/powerpc/platforms/512x/mpc512x_shared.c b/arch/powerpc/platforms/512x/mpc512x_shared.c
index 7135d89..b776e45 100644
--- a/arch/powerpc/platforms/512x/mpc512x_shared.c
+++ b/arch/powerpc/platforms/512x/mpc512x_shared.c
@@ -65,6 +65,30 @@ void __init mpc512x_init_IRQ(void)
 	ipic_set_default_priority();
 }
 
+void __init mpc512x_init_i2c(void)
+{
+	struct device_node *np;
+	void __iomem *i2cctl;
+
+	/* Enable I2C interrupts */
+	np = of_find_compatible_node(NULL, NULL, "fsl,mpc5121-i2c-ctrl");
+	if (np) {
+		i2cctl = of_iomap(np, 0);
+		if (i2cctl) {
+			/*
+			 * Set interrupt enable bits:
+			 *  - I2C-0: bit 24,
+			 *  - I2C-1: bit 26,
+			 *  - I2C-2: bit 28.
+			 */
+			out_be32(i2cctl, 0x15000000);
+			iounmap(i2cctl);
+		}
+
+		of_node_put(np);
+	}
+}
+
 /*
  * Nodes to do bus probe on, soc and localbus
  */
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a48c8ae..57ed637 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -391,13 +391,14 @@ config I2C_IXP2000
 	  instead.
 
 config I2C_MPC
-	tristate "MPC107/824x/85xx/52xx/86xx"
+	tristate "MPC107/824x/85xx/512x/52xx/86xx"
 	depends on PPC32
 	help
 	  If you say yes to this option, support will be included for the
-	  built-in I2C interface on the MPC107/Tsi107/MPC8240/MPC8245 and
-	  MPC85xx/MPC8641 family processors. The driver may also work on 52xx
-	  family processors, though interrupts are known not to work.
+	  built-in I2C interface on the MPC107/Tsi107/MPC8240/MPC8245,
+	  MPC85xx/MPC8641 and MPC512x family processors. The driver may
+	  also work on 52xx family processors, though interrupts are known
+	  not to work.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-mpc.
-- 
1.6.0.6

Re: [PATCH 08/12] mpc5121: Added I2C support.

[Grant Likely]

On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org> wrote:
> From: Piotr Ziecik <kosmo-nYOzD4b6Jr9Wk0Htik3J/w@public.gmane.org>
>
> - Enabled I2C interrupts on MPC5121.
> - Updated Kconfig for i2c-mpc driver.

I think this workaround belongs in the driver itself.

g.

>
> Signed-off-by: Piotr Ziecik <kosmo-nYOzD4b6Jr9Wk0Htik3J/w@public.gmane.org>
> Signed-off-by: Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org>
> Cc: <linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> Cc: John Rigby <jcrigby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  arch/powerpc/platforms/512x/mpc5121_ads.c    |    2 ++
>  arch/powerpc/platforms/512x/mpc512x.h        |    1 +
>  arch/powerpc/platforms/512x/mpc512x_shared.c |   24 ++++++++++++++++++++++++
>  drivers/i2c/busses/Kconfig                   |    9 +++++----
>  4 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/platforms/512x/mpc5121_ads.c b/arch/powerpc/platforms/512x/mpc5121_ads.c
> index 441abc4..a8976b4 100644
> --- a/arch/powerpc/platforms/512x/mpc5121_ads.c
> +++ b/arch/powerpc/platforms/512x/mpc5121_ads.c
> @@ -42,6 +42,8 @@ static void __init mpc5121_ads_setup_arch(void)
>        for_each_compatible_node(np, "pci", "fsl,mpc5121-pci")
>                mpc83xx_add_bridge(np);
>  #endif
> +
> +       mpc512x_init_i2c();
>  }
>
>  static void __init mpc5121_ads_init_IRQ(void)
> diff --git a/arch/powerpc/platforms/512x/mpc512x.h b/arch/powerpc/platforms/512x/mpc512x.h
> index 9c03693..f4db8a7 100644
> --- a/arch/powerpc/platforms/512x/mpc512x.h
> +++ b/arch/powerpc/platforms/512x/mpc512x.h
> @@ -13,5 +13,6 @@
>  #define __MPC512X_H__
>  extern unsigned long mpc512x_find_ips_freq(struct device_node *node);
>  extern void __init mpc512x_init_IRQ(void);
> +extern void __init mpc512x_init_i2c(void);
>  void __init mpc512x_declare_of_platform_devices(void);
>  #endif                         /* __MPC512X_H__ */
> diff --git a/arch/powerpc/platforms/512x/mpc512x_shared.c b/arch/powerpc/platforms/512x/mpc512x_shared.c
> index 7135d89..b776e45 100644
> --- a/arch/powerpc/platforms/512x/mpc512x_shared.c
> +++ b/arch/powerpc/platforms/512x/mpc512x_shared.c
> @@ -65,6 +65,30 @@ void __init mpc512x_init_IRQ(void)
>        ipic_set_default_priority();
>  }
>
> +void __init mpc512x_init_i2c(void)
> +{
> +       struct device_node *np;
> +       void __iomem *i2cctl;
> +
> +       /* Enable I2C interrupts */
> +       np = of_find_compatible_node(NULL, NULL, "fsl,mpc5121-i2c-ctrl");
> +       if (np) {
> +               i2cctl = of_iomap(np, 0);
> +               if (i2cctl) {
> +                       /*
> +                        * Set interrupt enable bits:
> +                        *  - I2C-0: bit 24,
> +                        *  - I2C-1: bit 26,
> +                        *  - I2C-2: bit 28.
> +                        */
> +                       out_be32(i2cctl, 0x15000000);
> +                       iounmap(i2cctl);
> +               }
> +
> +               of_node_put(np);
> +       }
> +}
> +
>  /*
>  * Nodes to do bus probe on, soc and localbus
>  */
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index a48c8ae..57ed637 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -391,13 +391,14 @@ config I2C_IXP2000
>          instead.
>
>  config I2C_MPC
> -       tristate "MPC107/824x/85xx/52xx/86xx"
> +       tristate "MPC107/824x/85xx/512x/52xx/86xx"
>        depends on PPC32
>        help
>          If you say yes to this option, support will be included for the
> -         built-in I2C interface on the MPC107/Tsi107/MPC8240/MPC8245 and
> -         MPC85xx/MPC8641 family processors. The driver may also work on 52xx
> -         family processors, though interrupts are known not to work.
> +         built-in I2C interface on the MPC107/Tsi107/MPC8240/MPC8245,
> +         MPC85xx/MPC8641 and MPC512x family processors. The driver may
> +         also work on 52xx family processors, though interrupts are known
> +         not to work.
>
>          This driver can also be built as a module.  If so, the module
>          will be called i2c-mpc.
> --
> 1.6.0.6
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 08/12] mpc5121: Added I2C support.

[Wolfgang Denk]

Dear Grant Likely,

In message <fa686aa40905061401k319313c5q89fd3e245c30808f-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> you wrote:
> On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org> wrote:
> > From: Piotr Ziecik <kosmo-nYOzD4b6Jr9Wk0Htik3J/w@public.gmane.org>
> >
> > - Enabled I2C interrupts on MPC5121.
> > - Updated Kconfig for i2c-mpc driver.
> 
> I think this workaround belongs in the driver itself.

Sorry, I don't get it. Which workaround? What exactly should I change?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd-ynQEQJNshbs@public.gmane.org
Time is an illusion perpetrated by the manufacturers of space.

Re: [PATCH 08/12] mpc5121: Added I2C support.

[Grant Likely]

On Wed, May 6, 2009 at 4:19 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Grant Likely,
>
> In message <fa686aa40905061401k319313c5q89fd3e245c30808f@mail.gmail.com> you wrote:
>> On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd@denx.de> wrote:
>> > From: Piotr Ziecik <kosmo@semihalf.com>
>> >
>> > - Enabled I2C interrupts on MPC5121.
>> > - Updated Kconfig for i2c-mpc driver.
>>
>> I think this workaround belongs in the driver itself.
>
> Sorry, I don't get it. Which workaround? What exactly should I change?

Sorry, I misread the patch.  Never mind.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 08/12] mpc5121: Added I2C support.

[Grant Likely]

On Wed, May 6, 2009 at 4:51 PM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> On Wed, May 6, 2009 at 4:19 PM, Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org> wrote:
>> Dear Grant Likely,
>>
>> In message <fa686aa40905061401k319313c5q89fd3e245c30808f-JsoAwUIsXotQFR93xxRIaA@public.gmane.orgcom> you wrote:
>>> On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org> wrote:
>>> > From: Piotr Ziecik <kosmo-nYOzD4b6Jr9Wk0Htik3J/w@public.gmane.org>
>>> >
>>> > - Enabled I2C interrupts on MPC5121.
>>> > - Updated Kconfig for i2c-mpc driver.
>>>
>>> I think this workaround belongs in the driver itself.
>>
>> Sorry, I don't get it. Which workaround? What exactly should I change?
>
> Sorry, I misread the patch.  Never mind.

Actually, on 3rd reading, I think my first impression was correct
(even if I was wrong about it being a workaround).  This code in
mpc512x_init_i2c() is only relevant for i2c busses (it isn't shared
with any other drivers).  Therefore, it belongs with the i2c bus
itself.  It does not belong in platform code.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 08/12] mpc5121: Added I2C support.

[Wolfgang Grandegger]

Grant Likely wrote:
> On Wed, May 6, 2009 at 4:51 PM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>> On Wed, May 6, 2009 at 4:19 PM, Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org> wrote:
>>> Dear Grant Likely,
>>>
>>> In message <fa686aa40905061401k319313c5q89fd3e245c30808f-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> you wrote:
>>>> On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org> wrote:
>>>>> From: Piotr Ziecik <kosmo-nYOzD4b6Jr9Wk0Htik3J/w@public.gmane.org>
>>>>>
>>>>> - Enabled I2C interrupts on MPC5121.
>>>>> - Updated Kconfig for i2c-mpc driver.
>>>> I think this workaround belongs in the driver itself.
>>> Sorry, I don't get it. Which workaround? What exactly should I change?
>> Sorry, I misread the patch.  Never mind.
> 
> Actually, on 3rd reading, I think my first impression was correct
> (even if I was wrong about it being a workaround).  This code in
> mpc512x_init_i2c() is only relevant for i2c busses (it isn't shared
> with any other drivers).  Therefore, it belongs with the i2c bus
> itself.  It does not belong in platform code.

Right. Furthermore, the i2c-mpc.c should be extened to support bus speed
setting for the MPC512x, which has been merged recently (see commit id
f2bd5efe).

Wolfgang.

Re: [PATCH 08/12] mpc5121: Added I2C support.

[John Rigby]

[-- Attachment #1.1: Type: text/plain, Size: 1341 bytes --]

Ok, the interrupt enabling should happen in the driver.  Should it key off
compatible or should a new property be added like the existing 5200 clocking
property?

On Wed, May 6, 2009 at 8:41 PM, Grant Likely <grant.likely@secretlab.ca>wrote:

> On Wed, May 6, 2009 at 4:51 PM, Grant Likely <grant.likely@secretlab.ca>
> wrote:
> > On Wed, May 6, 2009 at 4:19 PM, Wolfgang Denk <wd@denx.de> wrote:
> >> Dear Grant Likely,
> >>
> >> In message <fa686aa40905061401k319313c5q89fd3e245c30808f@mail.gmail.com>
> you wrote:
> >>> On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd@denx.de> wrote:
> >>> > From: Piotr Ziecik <kosmo@semihalf.com>
> >>> >
> >>> > - Enabled I2C interrupts on MPC5121.
> >>> > - Updated Kconfig for i2c-mpc driver.
> >>>
> >>> I think this workaround belongs in the driver itself.
> >>
> >> Sorry, I don't get it. Which workaround? What exactly should I change?
> >
> > Sorry, I misread the patch.  Never mind.
>
> Actually, on 3rd reading, I think my first impression was correct
> (even if I was wrong about it being a workaround).  This code in
> mpc512x_init_i2c() is only relevant for i2c busses (it isn't shared
> with any other drivers).  Therefore, it belongs with the i2c bus
> itself.  It does not belong in platform code.
>
> Cheers,
> g.
>
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
>

[-- Attachment #1.2: Type: text/html, Size: 2143 bytes --]

[-- Attachment #2: Type: text/plain, Size: 146 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 08/12] mpc5121: Added I2C support.

[Grant Likely]

On Thu, May 7, 2009 at 8:12 PM, John Rigby <jcrigby@gmail.com> wrote:
> Ok, the interrupt enabling should happen in the driver.  Should it key off
> compatible or should a new property be added like the existing 5200 clocking
> property?

key off compatible.  And the 5200 clocking property has been depreciated.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 08/12] mpc5121: Added I2C support.

[Piotr Zięcik]

> Right. Furthermore, the i2c-mpc.c should be extened to support bus speed
> setting for the MPC512x, which has been merged recently (see commit id
> f2bd5efe).

I have simple question about bus speed setting support. Existing 
implementation uses default safe speed if there is no 'clock-frequency' 
property in i2c node. Comments in code suggest that this behaviour is left 
for backward compatibility only. Should I make the 'clock-frequency'
property mandatory for a new type of I2C controller (MPC5121) ?

-- 
Best Regards.
Piotr Zięcik

Re: [PATCH 08/12] mpc5121: Added I2C support.

[Grant Likely]

2009/5/18 Piotr Zięcik <kosmo@semihalf.com>:
>> Right. Furthermore, the i2c-mpc.c should be extened to support bus speed
>> setting for the MPC512x, which has been merged recently (see commit id
>> f2bd5efe).
>
> I have simple question about bus speed setting support. Existing
> implementation uses default safe speed if there is no 'clock-frequency'
> property in i2c node. Comments in code suggest that this behaviour is left
> for backward compatibility only. Should I make the 'clock-frequency'
> property mandatory for a new type of I2C controller (MPC5121) ?

yes.  At least in documentation, but I wouldn't do extra work to
disable that behaviour for 5121 boards.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 08/12] mpc5121: Added I2C support.

[Wolfgang Grandegger]

Piotr Zięcik wrote:
>> Right. Furthermore, the i2c-mpc.c should be extened to support bus speed
>> setting for the MPC512x, which has been merged recently (see commit id
>> f2bd5efe).
> 
> I have simple question about bus speed setting support. Existing 
> implementation uses default safe speed if there is no 'clock-frequency' 
> property in i2c node. Comments in code suggest that this behaviour is left 
> for backward compatibility only. Should I make the 'clock-frequency'
> property mandatory for a new type of I2C controller (MPC5121) ?

I don't think so, for the same backward compatibility reason as for the
other boards. But the DTS file might be changed to use clock-frequency.
BTW, when I wrote the code I already had the MPC512x in mind. IIRC, the
FDR table and the function scanning it for the MPC52xx should work for
the MPC512x as well. It's mainly a matter of using the proper function
to get the source clock frequency and fiddling with multiple-arch support.

Wolfgang.

Re: [PATCH 08/12] mpc5121: Added I2C support.

[Piotr Zięcik]

Monday 18 May 2009 16:29:04 Wolfgang Grandegger napisał(a):
> > I have simple question about bus speed setting support. Existing
> > implementation uses default safe speed if there is no 'clock-frequency'
> > property in i2c node. Comments in code suggest that this behaviour is
> > left for backward compatibility only. Should I make the 'clock-frequency'
> > property mandatory for a new type of I2C controller (MPC5121) ?
>
> I don't think so, for the same backward compatibility reason as for the
> other boards. But the DTS file might be changed to use clock-frequency.

In my opinion implementing "backward compatilility" for MPC5121 is not good
idea. MPC5121 I2C support is completly new thing in mainline. Simply, there is 
no DTS with which I have to be compatible. Adding backward compatibility
with nothing may be confusing.

-- 
Best Regards,
Piotr Zięcik

Re: [PATCH 08/12] mpc5121: Added I2C support.

[Wolfgang Grandegger]

Piotr Zięcik wrote:
> Monday 18 May 2009 16:29:04 Wolfgang Grandegger napisał(a):
>>> I have simple question about bus speed setting support. Existing
>>> implementation uses default safe speed if there is no 'clock-frequency'
>>> property in i2c node. Comments in code suggest that this behaviour is
>>> left for backward compatibility only. Should I make the 'clock-frequency'
>>> property mandatory for a new type of I2C controller (MPC5121) ?
>> I don't think so, for the same backward compatibility reason as for the
>> other boards. But the DTS file might be changed to use clock-frequency.
> 
> In my opinion implementing "backward compatilility" for MPC5121 is not good
> idea. MPC5121 I2C support is completly new thing in mainline. Simply, there is 
> no DTS with which I have to be compatible. Adding backward compatibility
> with nothing may be confusing.

There is a port for the  MPC5121 in mainline since 2.6.25 including DTS
file:

http://lxr.linux.no/linux+v2.6.25/arch/powerpc/boot/dts/mpc5121ads.dts

If it was really usable or even used is another question. But it's fine
for me be more restrictive, e.g. print a warning when save values are
selected.

Wolfgang.