* [PATCH V4] i2c: designware: fix wrong Tx/Rx FIFO for ACPI
@ 2016-12-13 10:03 Tin Huynh
2016-12-13 11:25 ` Andy Shevchenko
0 siblings, 1 reply; 5+ messages in thread
From: Tin Huynh @ 2016-12-13 10:03 UTC (permalink / raw)
To: Jarkko Nikula, Andy Shevchenko, Mika Westerberg, Wolfram Sang
Cc: linux-i2c, linux-kernel, linux-acpi, Loc Ho, Thang Nguyen,
Phong Vo, patches, Tin Huynh
ACPI always sets Tx/Rx FIFO to 32. This configuration will
cause problem if the IP core supports a FIFO size of less than 32.
The driver should read the FIFO size from the IP and select the smaller
one of the two.
Signed-off-by: Tin Huynh <tnhuynh@apm.com>
---
drivers/i2c/busses/i2c-designware-platdrv.c | 27 ++++++++++++++++++++-------
1 files changed, 20 insertions(+), 7 deletions(-)
Change from V3:
-Use uppercase of FIFO instead of lowercase.
-Fix the problem when IP core return 0 of FIFO.
Change from V2:
-Add a helper function to set FIFO size.
Change from V1:
-Revert the default 32 for FIFO, read parameter from IP core
and pick the smaller one of the two.
-Correct the title to describe new approach.
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0b42a12..24032d6 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -150,6 +150,25 @@ static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
return 0;
}
+static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id)
+{
+ u32 param, tx_fifo_depth, rx_fifo_depth;
+
+ param = i2c_dw_read_comp_param(dev);
+ tx_fifo_depth = ((param >> 16) & 0xff) + 1;
+ rx_fifo_depth = ((param >> 8) & 0xff) + 1;
+ if (!dev->tx_fifo_depth) {
+ dev->tx_fifo_depth = tx_fifo_depth;
+ dev->rx_fifo_depth = rx_fifo_depth;
+ dev->adapter.nr = id;
+ } else if (tx_fifo_depth > 1) {
+ dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth,
+ tx_fifo_depth);
+ dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth,
+ rx_fifo_depth);
+ }
+}
+
static int dw_i2c_plat_probe(struct platform_device *pdev)
{
struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -246,13 +265,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
1000000);
}
- if (!dev->tx_fifo_depth) {
- u32 param1 = i2c_dw_read_comp_param(dev);
-
- dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
- dev->rx_fifo_depth = ((param1 >> 8) & 0xff) + 1;
- dev->adapter.nr = pdev->id;
- }
+ dw_i2c_set_fifo_size(dev, pdev->id);
adap = &dev->adapter;
adap->owner = THIS_MODULE;
--
1.7.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH V4] i2c: designware: fix wrong Tx/Rx FIFO for ACPI
2016-12-13 10:03 [PATCH V4] i2c: designware: fix wrong Tx/Rx FIFO for ACPI Tin Huynh
@ 2016-12-13 11:25 ` Andy Shevchenko
2016-12-13 13:35 ` Jarkko Nikula
2016-12-14 3:20 ` Tin Huynh
0 siblings, 2 replies; 5+ messages in thread
From: Andy Shevchenko @ 2016-12-13 11:25 UTC (permalink / raw)
To: Tin Huynh, Jarkko Nikula, Mika Westerberg, Wolfram Sang
Cc: linux-i2c, linux-kernel, linux-acpi, Loc Ho, Thang Nguyen,
Phong Vo, patches
On Tue, 2016-12-13 at 17:03 +0700, Tin Huynh wrote:
> ACPI always sets Tx/Rx FIFO to 32. This configuration will
> cause problem if the IP core supports a FIFO size of less than 32.
> The driver should read the FIFO size from the IP and select the
> smaller
> one of the two.
>
> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
>
One comment below, after addressing it
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/i2c/busses/i2c-designware-platdrv.c | 27
> ++++++++++++++++++++-------
> 1 files changed, 20 insertions(+), 7 deletions(-)
>
> Change from V3:
> -Use uppercase of FIFO instead of lowercase.
> -Fix the problem when IP core return 0 of FIFO.
>
> Change from V2:
> -Add a helper function to set FIFO size.
>
> Change from V1:
> -Revert the default 32 for FIFO, read parameter from IP core
> and pick the smaller one of the two.
> -Correct the title to describe new approach.
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 0b42a12..24032d6 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -150,6 +150,25 @@ static int i2c_dw_plat_prepare_clk(struct
> dw_i2c_dev *i_dev, bool prepare)
> return 0;
> }
>
> +static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id)
> +{
> + u32 param, tx_fifo_depth, rx_fifo_depth;
> +
>
/*
* Try to detect the FIFO depth if not
set by interface driver,
* the depth could be from 2 to 256 from
HW spec.
*/
> + param = i2c_dw_read_comp_param(dev);
> + tx_fifo_depth = ((param >> 16) & 0xff) + 1;
> + rx_fifo_depth = ((param >> 8) & 0xff) + 1;
> + if (!dev->tx_fifo_depth) {
> + dev->tx_fifo_depth = tx_fifo_depth;
> + dev->rx_fifo_depth = rx_fifo_depth;
> + dev->adapter.nr = id;
> + } else if (tx_fifo_depth > 1) {
This makes sense now, though I would add a comment here and use >= 2 to
reflect datasheet.
/*
* Choose minimum values between HW and interface
* driver provided.
*/
> + dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth,
> + tx_fifo_depth);
> + dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth,
> + rx_fifo_depth);
> + }
> +}
> +
> static int dw_i2c_plat_probe(struct platform_device *pdev)
> {
> struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev-
> >dev);
> @@ -246,13 +265,7 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
> 1000000);
> }
>
> - if (!dev->tx_fifo_depth) {
> - u32 param1 = i2c_dw_read_comp_param(dev);
> -
> - dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
> - dev->rx_fifo_depth = ((param1 >> 8) & 0xff) + 1;
> - dev->adapter.nr = pdev->id;
> - }
> + dw_i2c_set_fifo_size(dev, pdev->id);
>
> adap = &dev->adapter;
> adap->owner = THIS_MODULE;
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V4] i2c: designware: fix wrong Tx/Rx FIFO for ACPI
2016-12-13 11:25 ` Andy Shevchenko
@ 2016-12-13 13:35 ` Jarkko Nikula
2016-12-14 3:20 ` Tin Huynh
1 sibling, 0 replies; 5+ messages in thread
From: Jarkko Nikula @ 2016-12-13 13:35 UTC (permalink / raw)
To: Andy Shevchenko, Tin Huynh, Mika Westerberg, Wolfram Sang
Cc: linux-i2c, linux-kernel, linux-acpi, Loc Ho, Thang Nguyen,
Phong Vo, patches
On 12/13/2016 01:25 PM, Andy Shevchenko wrote:
> On Tue, 2016-12-13 at 17:03 +0700, Tin Huynh wrote:
>> ACPI always sets Tx/Rx FIFO to 32. This configuration will
>> cause problem if the IP core supports a FIFO size of less than 32.
>> The driver should read the FIFO size from the IP and select the
>> smaller
>> one of the two.
>>
>> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
>>
>
> One comment below, after addressing it
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
Feel free to add my
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V4] i2c: designware: fix wrong Tx/Rx FIFO for ACPI
2016-12-13 11:25 ` Andy Shevchenko
2016-12-13 13:35 ` Jarkko Nikula
@ 2016-12-14 3:20 ` Tin Huynh
2016-12-14 8:59 ` Jarkko Nikula
1 sibling, 1 reply; 5+ messages in thread
From: Tin Huynh @ 2016-12-14 3:20 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jarkko Nikula, Mika Westerberg, Wolfram Sang, linux-i2c,
linux-kernel, linux-acpi, Loc Ho, Thang Nguyen, Phong Vo,
patches
On Tue, Dec 13, 2016 at 6:25 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, 2016-12-13 at 17:03 +0700, Tin Huynh wrote:
>> ACPI always sets Tx/Rx FIFO to 32. This configuration will
>> cause problem if the IP core supports a FIFO size of less than 32.
>> The driver should read the FIFO size from the IP and select the
>> smaller
>> one of the two.
>>
>> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
>>
>
> One comment below, after addressing it
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
>> ---
>> drivers/i2c/busses/i2c-designware-platdrv.c | 27
>> ++++++++++++++++++++-------
>> 1 files changed, 20 insertions(+), 7 deletions(-)
>>
>> Change from V3:
>> -Use uppercase of FIFO instead of lowercase.
>> -Fix the problem when IP core return 0 of FIFO.
>>
>> Change from V2:
>> -Add a helper function to set FIFO size.
>>
>> Change from V1:
>> -Revert the default 32 for FIFO, read parameter from IP core
>> and pick the smaller one of the two.
>> -Correct the title to describe new approach.
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index 0b42a12..24032d6 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -150,6 +150,25 @@ static int i2c_dw_plat_prepare_clk(struct
>> dw_i2c_dev *i_dev, bool prepare)
>> return 0;
>> }
>>
>> +static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id)
>> +{
>> + u32 param, tx_fifo_depth, rx_fifo_depth;
>> +
>>
>
> /*
> * Try to detect the FIFO depth if not
> set by interface driver,
> * the depth could be from 2 to 256 from
> HW spec.
> */
>
I will add your comment to the driver.
>> + param = i2c_dw_read_comp_param(dev);
>> + tx_fifo_depth = ((param >> 16) & 0xff) + 1;
>> + rx_fifo_depth = ((param >> 8) & 0xff) + 1;
>> + if (!dev->tx_fifo_depth) {
>> + dev->tx_fifo_depth = tx_fifo_depth;
>> + dev->rx_fifo_depth = rx_fifo_depth;
>> + dev->adapter.nr = id;
>> + } else if (tx_fifo_depth > 1) {
>
> This makes sense now, though I would add a comment here and use >= 2 to
> reflect datasheet.
>
> /*
> * Choose minimum values between HW and interface
> * driver provided.
> */
>
I will implement as your comment. However , because adding 1 to the
value , can i use > 2 or >=3 ?
>> + dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth,
>> + tx_fifo_depth);
>> + dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth,
>> + rx_fifo_depth);
>> + }
>> +}
>> +
>> static int dw_i2c_plat_probe(struct platform_device *pdev)
>> {
>> struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev-
>> >dev);
>> @@ -246,13 +265,7 @@ static int dw_i2c_plat_probe(struct
>> platform_device *pdev)
>> 1000000);
>> }
>>
>> - if (!dev->tx_fifo_depth) {
>> - u32 param1 = i2c_dw_read_comp_param(dev);
>> -
>> - dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
>> - dev->rx_fifo_depth = ((param1 >> 8) & 0xff) + 1;
>> - dev->adapter.nr = pdev->id;
>> - }
>> + dw_i2c_set_fifo_size(dev, pdev->id);
>>
>> adap = &dev->adapter;
>> adap->owner = THIS_MODULE;
>
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy
Thank you and regards,
Tin
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V4] i2c: designware: fix wrong Tx/Rx FIFO for ACPI
2016-12-14 3:20 ` Tin Huynh
@ 2016-12-14 8:59 ` Jarkko Nikula
0 siblings, 0 replies; 5+ messages in thread
From: Jarkko Nikula @ 2016-12-14 8:59 UTC (permalink / raw)
To: Tin Huynh, Andy Shevchenko
Cc: Mika Westerberg, Wolfram Sang, linux-i2c, linux-kernel,
linux-acpi, Loc Ho, Thang Nguyen, Phong Vo, patches
On 12/14/2016 05:20 AM, Tin Huynh wrote:
> On Tue, Dec 13, 2016 at 6:25 PM, Andy Shevchenko
>>> + param = i2c_dw_read_comp_param(dev);
>>> + tx_fifo_depth = ((param >> 16) & 0xff) + 1;
>>> + rx_fifo_depth = ((param >> 8) & 0xff) + 1;
>>> + if (!dev->tx_fifo_depth) {
>>> + dev->tx_fifo_depth = tx_fifo_depth;
>>> + dev->rx_fifo_depth = rx_fifo_depth;
>>> + dev->adapter.nr = id;
>>> + } else if (tx_fifo_depth > 1) {
>>
>> This makes sense now, though I would add a comment here and use >= 2 to
>> reflect datasheet.
>>
>> /*
>> * Choose minimum values between HW and interface
>> * driver provided.
>> */
>>
> I will implement as your comment. However , because adding 1 to the
> value , can i use > 2 or >=3 ?
either > 1 or >= 2 since register value 0x01 from HW means FIFO depth 2
and register value 0x00 is reserved.
--
Jarkko
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-12-14 8:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-13 10:03 [PATCH V4] i2c: designware: fix wrong Tx/Rx FIFO for ACPI Tin Huynh
2016-12-13 11:25 ` Andy Shevchenko
2016-12-13 13:35 ` Jarkko Nikula
2016-12-14 3:20 ` Tin Huynh
2016-12-14 8:59 ` Jarkko Nikula
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).