All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Cousson, Benoit" <b-cousson@ti.com>
To: Andy Green <andy@warmcat.com>
Cc: "linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>,
	"patches@linaro.org" <patches@linaro.org>,
	Andy Green <andy.green@linaro.org>
Subject: Re: [PATCH 2/4] OMAP3 I2C document why cpu type and not peripheral unit ID used to probe
Date: Thu, 3 Mar 2011 22:12:33 +0100	[thread overview]
Message-ID: <4D700441.50206@ti.com> (raw)
In-Reply-To: <20110303135030.30648.14923.stgit@otae.warmcat.com>

On 3/3/2011 2:50 PM, Andy Green wrote:
> Describe why we can't simply probe the peripheral unit ID
> to make the decision about what register map to use
>
> Cc: patches@linaro.org
> Signed-off-by: Andy Green<andy.green@linaro.org>
> ---
>
>   drivers/i2c/busses/i2c-omap.c |   11 +++++++++++
>   1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index b605ff3..d6500ec 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -1032,6 +1032,17 @@ omap_i2c_probe(struct platform_device *pdev)
>   	else
>   		dev->reg_shift = 2;
>
> +	dev->regs = (u8 *)reg_map;
> +
> +	/*
> +	 * this is a bit tricky, implementation on 4430 has the active
> +	 * part of its ID register moved to +4 instead of +0 as
> +	 * previously.  So, we can't probe just using the ID register
> +	 * Complicating matters the older implementation using the
> +	 * simpler register set on 3530 also reports its revision as
> +	 * 0x40, same as the 4430 newer implementation.
> +	 */

The issue is that this revision field is not really documented in OMAP4 
TRM, so you should not rely on it. Moreover, as you already noticed, the 
revision number is not even accurate. OMAP3 and 4 are using a different 
programming model but this is not reflected in this field.

Since that issue is quite common in many OMAP IPs, we introduced a SW 
field in hwmod_class that should reflect the change in IP programming 
model. For the moment it is a simple integer that we increment each time 
there is change in a programming model that will impact the driver.

You can check how it was done for the timer for example:
https://patchwork.kernel.org/patch/587211/
Then you need to copy that info to a pdata field in order to allow the 
driver to access it. Check omap_timer_init in:
https://patchwork.kernel.org/patch/587241/

> +
>   	if (cpu_is_omap44xx())
>   		dev->regs = (u8 *) omap4_reg_map;

OK, this is not part of your patch, but any call to cpu_is are forbidden 
from the driver. As soon as you have the revision field from hwmod you 
can get rid of all that code.

Regards,
Benoit

WARNING: multiple messages have this Message-ID (diff)
From: b-cousson@ti.com (Cousson, Benoit)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 2/4] OMAP3 I2C document why cpu type and not peripheral unit ID used to probe
Date: Thu, 3 Mar 2011 22:12:33 +0100	[thread overview]
Message-ID: <4D700441.50206@ti.com> (raw)
In-Reply-To: <20110303135030.30648.14923.stgit@otae.warmcat.com>

On 3/3/2011 2:50 PM, Andy Green wrote:
> Describe why we can't simply probe the peripheral unit ID
> to make the decision about what register map to use
>
> Cc: patches at linaro.org
> Signed-off-by: Andy Green<andy.green@linaro.org>
> ---
>
>   drivers/i2c/busses/i2c-omap.c |   11 +++++++++++
>   1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index b605ff3..d6500ec 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -1032,6 +1032,17 @@ omap_i2c_probe(struct platform_device *pdev)
>   	else
>   		dev->reg_shift = 2;
>
> +	dev->regs = (u8 *)reg_map;
> +
> +	/*
> +	 * this is a bit tricky, implementation on 4430 has the active
> +	 * part of its ID register moved to +4 instead of +0 as
> +	 * previously.  So, we can't probe just using the ID register
> +	 * Complicating matters the older implementation using the
> +	 * simpler register set on 3530 also reports its revision as
> +	 * 0x40, same as the 4430 newer implementation.
> +	 */

The issue is that this revision field is not really documented in OMAP4 
TRM, so you should not rely on it. Moreover, as you already noticed, the 
revision number is not even accurate. OMAP3 and 4 are using a different 
programming model but this is not reflected in this field.

Since that issue is quite common in many OMAP IPs, we introduced a SW 
field in hwmod_class that should reflect the change in IP programming 
model. For the moment it is a simple integer that we increment each time 
there is change in a programming model that will impact the driver.

You can check how it was done for the timer for example:
https://patchwork.kernel.org/patch/587211/
Then you need to copy that info to a pdata field in order to allow the 
driver to access it. Check omap_timer_init in:
https://patchwork.kernel.org/patch/587241/

> +
>   	if (cpu_is_omap44xx())
>   		dev->regs = (u8 *) omap4_reg_map;

OK, this is not part of your patch, but any call to cpu_is are forbidden 
from the driver. As soon as you have the revision field from hwmod you 
can get rid of all that code.

Regards,
Benoit

  reply	other threads:[~2011-03-03 21:12 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-03-03 13:50 [PATCH 0/4] OMAP 3 and 4 i2c fixes Andy Green
2011-03-03 13:50 ` Andy Green
2011-03-03 13:50 ` [PATCH 1/4] OMAP3 and 4 hwmod I2C units only allow 16 bit access Andy Green
2011-03-03 13:50   ` Andy Green
2011-03-03 17:42   ` Cousson, Benoit
2011-03-03 17:42     ` Cousson, Benoit
2011-03-03 17:56     ` Andy Green
2011-03-03 17:56       ` Andy Green
2011-03-03 20:40       ` Cousson, Benoit
2011-03-03 20:40         ` Cousson, Benoit
2011-03-04  8:33         ` Andy Green
2011-03-04  8:33           ` Andy Green
2011-03-04 10:05           ` Cousson, Benoit
2011-03-04 10:05             ` Cousson, Benoit
2011-03-04 15:20   ` Cousson, Benoit
2011-03-04 15:20     ` Cousson, Benoit
2011-03-03 13:50 ` [PATCH 2/4] OMAP3 I2C document why cpu type and not peripheral unit ID used to probe Andy Green
2011-03-03 13:50   ` Andy Green
2011-03-03 21:12   ` Cousson, Benoit [this message]
2011-03-03 21:12     ` Cousson, Benoit
2011-03-04  8:25     ` Andy Green
2011-03-04  8:25       ` Andy Green
2011-03-03 13:50 ` [PATCH 3/4] OMAP3 and 4 i2c mark extended reg enums as extended only Andy Green
2011-03-03 13:50   ` Andy Green
2011-03-03 21:33   ` Cousson, Benoit
2011-03-03 21:33     ` Cousson, Benoit
2011-03-04  8:32     ` Andy Green
2011-03-04  8:32       ` Andy Green
2011-03-04 10:05       ` Cousson, Benoit
2011-03-04 10:05         ` Cousson, Benoit
2011-03-03 13:50 ` [PATCH 4/4] OMAP3 and 4 I2C use cpu type consistently for new register availability Andy Green
2011-03-03 13:50   ` Andy Green
2011-03-03 21:45   ` Cousson, Benoit
2011-03-03 21:45     ` Cousson, Benoit
2011-03-03 21:55 ` [PATCH 0/4] OMAP 3 and 4 i2c fixes Cousson, Benoit
2011-03-03 21:55   ` Cousson, Benoit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4D700441.50206@ti.com \
    --to=b-cousson@ti.com \
    --cc=andy.green@linaro.org \
    --cc=andy@warmcat.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=patches@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.