[v2,1/1] i2c: designware: Fix wrong setting for {ss,fs,hs}_{h,l}cnt registers

From: Adrian Huang <ahuang12@lenovo.com>

When disabling CONFIG_X86_AMD_PLATFORM_DEVICE option, the driver
'drivers/acpi/acpi_apd.c' won't be compiled. This leads to a situation
where BMC (Baseboard Management Controller) cannot retrieve the memory
temperature via the i2c interface after i2c DW driver is loaded. Note
that BMC can retrieve the memory temperature before booting into OS.

[Debugging Detail]
  1. dev->pclk and dev->clk are NULL when calling devm_clk_get_optional()
     in dw_i2c_plat_probe().

  2. The callings of i2c_dw_scl_hcnt() in i2c_dw_set_timings_master()
     return 65528 (-8 in integer format) or 65533 (-3 in integer format).
     The following log shows SS's HCNT/LCNT:

       i2c_designware AMDI0010:01: Standard Mode HCNT:LCNT = 65533:65535

  3. The callings of i2c_dw_scl_lcnt() in i2c_dw_set_timings_master()
     return 65535 (-1 in integer format). The following log shows SS's
     HCNT/LCNT:

       i2c_designware AMDI0010:01: Fast Mode HCNT:LCNT = 65533:65535

  4. i2c_dw_init_master() configures the register IC_SS_SCL_HCNT with
     the value 65533. However, the DW i2c databook mentioned the value
     cannot be higher than 65525. Quote from the DW i2c databook:

       NOTE: This register must not be programmed to a value higher than
             65525, because DW_apb_i2c uses a 16-bit counter to flag an
             I2C bus idle condition when this counter reaches a value of
             IC_SS_SCL_HCNT + 10.

  5. Since ss_hcnt, ss_lcnt, fs_hcnt, and fs_lcnt are the invalid
     values, we should not write the corresponding registers.

Fix the issue by reading dev->{ss,fs,hs}_hcnt and dev->{ss,fs,hs}_lcnt
from HW registers if ic_clk is not set.

Link: https://lore.kernel.org/linux-i2c/8295cbe1-a7c5-4a35-a189-5d0bff51ede6@linux.intel.com/
Suggested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Adrian Huang <ahuang12@lenovo.com>
Reported-by: Dong Wang <wangdong28@lenovo.com>
Tested-by: Dong Wang <wangdong28@lenovo.com>

---
 drivers/i2c/busses/i2c-designware-common.c | 27 ++++++++++++++++--
 drivers/i2c/busses/i2c-designware-core.h   |  6 ++--
 drivers/i2c/busses/i2c-designware-master.c | 32 ++++++++++++++++------
 3 files changed, 53 insertions(+), 12 deletions(-)

On 8/2/24 4:01 PM, Adrian Huang wrote:
> From: Adrian Huang <ahuang12@lenovo.com>
> 
> When disabling CONFIG_X86_AMD_PLATFORM_DEVICE option, the driver
> 'drivers/acpi/acpi_apd.c' won't be compiled. This leads to a situation
> where BMC (Baseboard Management Controller) cannot retrieve the memory
> temperature via the i2c interface after i2c DW driver is loaded. Note
> that BMC can retrieve the memory temperature before booting into OS.
> 
> [Debugging Detail]
>    1. dev->pclk and dev->clk are NULL when calling devm_clk_get_optional()
>       in dw_i2c_plat_probe().
> 
>    2. The callings of i2c_dw_scl_hcnt() in i2c_dw_set_timings_master()
>       return 65528 (-8 in integer format) or 65533 (-3 in integer format).
>       The following log shows SS's HCNT/LCNT:
> 
>         i2c_designware AMDI0010:01: Standard Mode HCNT:LCNT = 65533:65535
> 
>    3. The callings of i2c_dw_scl_lcnt() in i2c_dw_set_timings_master()
>       return 65535 (-1 in integer format). The following log shows SS's
>       HCNT/LCNT:
> 
>         i2c_designware AMDI0010:01: Fast Mode HCNT:LCNT = 65533:65535
> 
>    4. i2c_dw_init_master() configures the register IC_SS_SCL_HCNT with
>       the value 65533. However, the DW i2c databook mentioned the value
>       cannot be higher than 65525. Quote from the DW i2c databook:
> 
>         NOTE: This register must not be programmed to a value higher than
>               65525, because DW_apb_i2c uses a 16-bit counter to flag an
>               I2C bus idle condition when this counter reaches a value of
>               IC_SS_SCL_HCNT + 10.
> 
>    5. Since ss_hcnt, ss_lcnt, fs_hcnt, and fs_lcnt are the invalid
>       values, we should not write the corresponding registers.
> 
> Fix the issue by reading dev->{ss,fs,hs}_hcnt and dev->{ss,fs,hs}_lcnt
> from HW registers if ic_clk is not set.
> 
> Link: https://lore.kernel.org/linux-i2c/8295cbe1-a7c5-4a35-a189-5d0bff51ede6@linux.intel.com/
> Suggested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Signed-off-by: Adrian Huang <ahuang12@lenovo.com>
> Reported-by: Dong Wang <wangdong28@lenovo.com>
> Tested-by: Dong Wang <wangdong28@lenovo.com>
> 
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Hi Adrian,

> Link: https://lore.kernel.org/linux-i2c/8295cbe1-a7c5-4a35-a189-5d0bff51ede6@linux.intel.com/
> Suggested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Signed-off-by: Adrian Huang <ahuang12@lenovo.com>
> Reported-by: Dong Wang <wangdong28@lenovo.com>
> Tested-by: Dong Wang <wangdong28@lenovo.com>

At first I thought that we need a Fixes tag, but on a second
thought I judged this more as a behavioural fix. Please, let me
konw if you think we want to have the Fixes tag here.

Meantime, I'm going to re-arrange the tag section. It's common to
sort the tags in a chronological order:

Reported-by: (it first gets reported)
Suggested-by: (then someone suggested the fix)
Signed-off-by: (then someone implemented the fix)
Tested-by: (finally someone tested it)
Link: (as reference)

I will also appreciate that the Reviewed-by and Tested-by and
Acked-by happen transparently and openly in the mailing list
rather behind the curtains.

Said that, thanks for your patch, I merged it into i2c/i2c-host.

Andi

Hi Andi,

On Tue, Aug 6, 2024 at 1:27 AM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Adrian,
>
> > Link: https://lore.kernel.org/linux-i2c/8295cbe1-a7c5-4a35-a189-5d0bff51ede6@linux.intel.com/
> > Suggested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > Signed-off-by: Adrian Huang <ahuang12@lenovo.com>
> > Reported-by: Dong Wang <wangdong28@lenovo.com>
> > Tested-by: Dong Wang <wangdong28@lenovo.com>
>
> At first I thought that we need a Fixes tag, but on a second
> thought I judged this more as a behavioural fix. Please, let me
> konw if you think we want to have the Fixes tag here.

Agree with your second thought, so no need to add the Fixes tag.

> Meantime, I'm going to re-arrange the tag section. It's common to
> sort the tags in a chronological order:
>
> Reported-by: (it first gets reported)
> Suggested-by: (then someone suggested the fix)
> Signed-off-by: (then someone implemented the fix)
> Tested-by: (finally someone tested it)
> Link: (as reference)
>
> I will also appreciate that the Reviewed-by and Tested-by and
> Acked-by happen transparently and openly in the mailing list
> rather behind the curtains.

I'm sorry for the inconvenience. Wil pay more attention in the future.

> Said that, thanks for your patch, I merged it into i2c/i2c-host.

Thank you.

-- Adrian

On Mon, Aug 05, 2024 at 07:26:55PM +0200, Andi Shyti wrote:
> Hi Adrian,
> 
> > Link: https://lore.kernel.org/linux-i2c/8295cbe1-a7c5-4a35-a189-5d0bff51ede6@linux.intel.com/
> > Suggested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > Signed-off-by: Adrian Huang <ahuang12@lenovo.com>
> > Reported-by: Dong Wang <wangdong28@lenovo.com>
> > Tested-by: Dong Wang <wangdong28@lenovo.com>
> 
> At first I thought that we need a Fixes tag, but on a second
> thought I judged this more as a behavioural fix. Please, let me
> konw if you think we want to have the Fixes tag here.
> 
> Meantime, I'm going to re-arrange the tag section. It's common to
> sort the tags in a chronological order:
> 
> Reported-by: (it first gets reported)
> Suggested-by: (then someone suggested the fix)
> Signed-off-by: (then someone implemented the fix)
> Tested-by: (finally someone tested it)
> Link: (as reference)

If you use `b4`, you may configure it to do this automatically and keep nice
with contributors :-)