Regression: bd698d24b1b57: i2c: designware: Get selected speed mode sda-hold-time via ACPI

Regression: bd698d24b1b57: i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Lorenzo Pieralisi]

Hi guys,

as a heads-up, with today mainline (commit 2868b2513aa7) I get the
following splat on AMD Seattle, reverting the $SUBJECT commit "solves"
the problem.

My I2C knowledge is a bit limited but I am not sure I understand why
we should be reading eg ss_hcnt/ss_lcnt depending on the dev->clk_freq
but then i2c_dw_init() _always_ requires those values to be set for
a given device. Again, I have no insights into I2C inner workings
so apologies for the silly assumption/question.

Please have a look into this, thanks.

Lorenzo

[    1.160597] ------------[ cut here ]------------
[    1.165207] WARNING: CPU: 0 PID: 1 at drivers/i2c/busses/i2c-designware-core.c:293 i2c_dw_clk_rate+0x20/0x30
[    1.175019] Modules linked in:
[    1.178065] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.11.0-11415-g98a1892 #17
[    1.185360] Hardware name: AMD Seattle/Seattle, BIOS 18:16:37 May  1 2017
[    1.192134] task: ffff8003ed8e0000 task.stack: ffff8003ed8e8000
[    1.198041] PC is at i2c_dw_clk_rate+0x20/0x30
[    1.202472] LR is at i2c_dw_init+0x104/0x3c0
[    1.206729] pc : [<ffff00000874bfc0>] lr : [<ffff00000874c8a4>] pstate: 60000045
[    1.214110] sp : ffff8003ed8ebb90
[    1.217412] x29: ffff8003ed8ebb90 x28: 0000000000000000 
[    1.222714] x27: ffff000008df5fd0 x26: ffff000008f1b000 
[    1.228014] x25: ffff000008d00454 x24: 000000000000012c 
[    1.233314] x23: 00000000000707ae x22: 000000000000012c 
[    1.238614] x21: ffff8003ed4c78b8 x20: 0000000000000000 
[    1.243915] x19: ffff8003ed4c7818 x18: 0000000000000000 
[    1.249214] x17: 00000000000017ff x16: 0000000000000003 
[    1.254515] x15: 0088000000000000 x14: 00000000452b2be8 
[    1.259815] x13: 0000000000000000 x12: 0000000000000018 
[    1.265114] x11: 0000000000000000 x10: 0101010101010101 
[    1.270415] x9 : 0000000000000000 x8 : ffff8003ed6e5900 
[    1.275714] x7 : 0000000000000000 x6 : 000000000000003f 
[    1.281014] x5 : 0000000000000008 x4 : 0000000000000000 
[    1.286314] x3 : ffff00000807806c x2 : 0000000000000000 
[    1.291613] x1 : 0000000000000000 x0 : ffff8003ed4c7818 
[    1.296915] ---[ end trace 11d80b7b3eea6126 ]---
[    1.301519] Call trace:
[    1.303953] Exception stack(0xffff8003ed8eb9c0 to 0xffff8003ed8ebaf0)
[    1.310381] b9c0: ffff8003ed4c7818 0001000000000000 ffff8003ed8ebb90 ffff00000874bfc0
[    1.318197] b9e0: ffff8003ed8eba40 ffff00000847468c 000000000000000f 0000000000000001
[    1.326013] ba00: ffff8003ef40dfc8 ffff8003ed6cc050 ffff000008f63000 ffff000008f63f66
[    1.333829] ba20: ffff8003ed8eba70 ffff00000845428c ffff8003ed595000 0000000000000000
[    1.341645] ba40: ffff8003ed8ebaa0 ffff000008481bd8 ffff8003ed8ebaa0 ffff00000845428c
[    1.349461] ba60: ffff8003ed4c7818 0000000000000000 0000000000000000 ffff00000807806c
[    1.357276] ba80: 0000000000000000 0000000000000008 000000000000003f 0000000000000000
[    1.365092] baa0: ffff8003ed6e5900 0000000000000000 0101010101010101 0000000000000000
[    1.372908] bac0: 0000000000000018 0000000000000000 00000000452b2be8 0088000000000000
[    1.380723] bae0: 0000000000000003 00000000000017ff
[    1.385588] [<ffff00000874bfc0>] i2c_dw_clk_rate+0x20/0x30
[    1.391061] [<ffff00000874cb98>] i2c_dw_probe+0x38/0x238
[    1.396360] [<ffff00000874d67c>] dw_i2c_plat_probe+0x2b4/0x558
[    1.402182] [<ffff0000085546c0>] platform_drv_probe+0x58/0xc0
[    1.407916] [<ffff000008552824>] driver_probe_device+0x21c/0x2c0
[    1.413909] [<ffff000008552974>] __driver_attach+0xac/0xb0
[    1.419382] [<ffff00000855099c>] bus_for_each_dev+0x64/0xa0
[    1.424941] [<ffff000008552138>] driver_attach+0x20/0x28
[    1.430241] [<ffff000008551c88>] bus_add_driver+0x110/0x230
[    1.435800] [<ffff0000085532f8>] driver_register+0x60/0xf8
[    1.441272] [<ffff0000085545f8>] __platform_driver_register+0x40/0x48
[    1.447702] [<ffff000008d44b98>] dw_i2c_init_driver+0x18/0x20
[    1.453437] [<ffff0000080830f8>] do_one_initcall+0x38/0x120
[    1.458997] [<ffff000008d00cf0>] kernel_init_freeable+0x198/0x238
[    1.465078] [<ffff000008906f38>] kernel_init+0x10/0x100
[    1.470289] [<ffff000008082ec0>] ret_from_fork+0x10/0x50
[    1.475601] Call trace:
[    1.478035] Exception stack(0xffff8003ed8eb9c0 to 0xffff8003ed8ebaf0)
[    1.484462] b9c0: ffff8003ed4c7818 0001000000000000 ffff8003ed8ebb90 ffff00000874bfc0
[    1.492278] b9e0: ffff8003ed8eba40 ffff00000847468c 000000000000000f 0000000000000001
[    1.500094] ba00: ffff8003ef40dfc8 ffff8003ed6cc050 ffff000008f63000 ffff000008f63f66
[    1.507910] ba20: ffff8003ed8eba70 ffff00000845428c ffff8003ed595000 0000000000000000
[    1.515725] ba40: ffff8003ed8ebaa0 ffff000008481bd8 ffff8003ed8ebaa0 ffff00000845428c
[    1.523541] ba60: ffff8003ed4c7818 0000000000000000 0000000000000000 ffff00000807806c
[    1.531357] ba80: 0000000000000000 0000000000000008 000000000000003f 0000000000000000
[    1.539173] baa0: ffff8003ed6e5900 0000000000000000 0101010101010101 0000000000000000
[    1.546988] bac0: 0000000000000018 0000000000000000 00000000452b2be8 0088000000000000
[    1.554804] bae0: 0000000000000003 00000000000017ff
[    1.559669] [<ffff00000874bfc0>] i2c_dw_clk_rate+0x20/0x30
[    1.565141] [<ffff00000874cb98>] i2c_dw_probe+0x38/0x238
[    1.570440] [<ffff00000874d67c>] dw_i2c_plat_probe+0x2b4/0x558
[    1.576260] [<ffff0000085546c0>] platform_drv_probe+0x58/0xc0
[    1.581993] [<ffff000008552824>] driver_probe_device+0x21c/0x2c0
[    1.587986] [<ffff000008552974>] __driver_attach+0xac/0xb0
[    1.593459] [<ffff00000855099c>] bus_for_each_dev+0x64/0xa0
[    1.599018] [<ffff000008552138>] driver_attach+0x20/0x28
[    1.604316] [<ffff000008551c88>] bus_add_driver+0x110/0x230
[    1.609875] [<ffff0000085532f8>] driver_register+0x60/0xf8
[    1.615348] [<ffff0000085545f8>] __platform_driver_register+0x40/0x48
[    1.621775] [<ffff000008d44b98>] dw_i2c_init_driver+0x18/0x20
[    1.627508] [<ffff0000080830f8>] do_one_initcall+0x38/0x120
[    1.633067] [<ffff000008d00cf0>] kernel_init_freeable+0x198/0x238
[    1.639147] [<ffff000008906f38>] kernel_init+0x10/0x100
[    1.644359] [<ffff000008082ec0>] ret_from_fork+0x10/0x50
[    1.649913] Call trace:
[    1.652349] Exception stack(0xffff8003ed8eb9c0 to 0xffff8003ed8ebaf0)
[    1.658776] b9c0: ffff8003ed728018 0001000000000000 ffff8003ed8ebb90 ffff00000874bfc0
[    1.666593] b9e0: ffff7e000fb5b880 ffff8003ef403c80 ffff8003ed8ebb00 ffff0000081d7f1c
[    1.674409] ba00: ffff7e000fb5b880 ffff8003ed6e2f80 ffff8003ed8ebb20 ffff0000081d7f1c
[    1.682225] ba20: ffff7e000fb5b880 ffff8003ed6e2a00 ffff8003ed8e0000 000000000008d9c0
[    1.690041] ba40: ffff8003ef403c80 ffff8003ed8e0000 ffff000008476ee4 ffff000008f1b000
[    1.697857] ba60: ffff8003ed728018 0000000000000000 0000000000000000 ffff00000807a06c
[    1.705673] ba80: 0000000000000000 0000000000000008 000000000000003f 0000000000000000
[    1.713489] baa0: ffff8003ed6e5080 0000000000000000 0101010101010101 0000000000000000
[    1.721305] bac0: 0000000000000018 0000000000000000 000000006255f330 0088000000000000
[    1.729120] bae0: 0000000000000003 00000000000017ff
[    1.733985] [<ffff00000874bfc0>] i2c_dw_clk_rate+0x20/0x30
[    1.739458] [<ffff00000874cb98>] i2c_dw_probe+0x38/0x238
[    1.744757] [<ffff00000874d67c>] dw_i2c_plat_probe+0x2b4/0x558
[    1.750577] [<ffff0000085546c0>] platform_drv_probe+0x58/0xc0
[    1.756310] [<ffff000008552824>] driver_probe_device+0x21c/0x2c0
[    1.762303] [<ffff000008552974>] __driver_attach+0xac/0xb0
[    1.767775] [<ffff00000855099c>] bus_for_each_dev+0x64/0xa0
[    1.773334] [<ffff000008552138>] driver_attach+0x20/0x28
[    1.778633] [<ffff000008551c88>] bus_add_driver+0x110/0x230
[    1.784192] [<ffff0000085532f8>] driver_register+0x60/0xf8
[    1.789665] [<ffff0000085545f8>] __platform_driver_register+0x40/0x48
[    1.796092] [<ffff000008d44b98>] dw_i2c_init_driver+0x18/0x20
[    1.801825] [<ffff0000080830f8>] do_one_initcall+0x38/0x120
[    1.807384] [<ffff000008d00cf0>] kernel_init_freeable+0x198/0x238
[    1.813463] [<ffff000008906f38>] kernel_init+0x10/0x100
[    1.818675] [<ffff000008082ec0>] ret_from_fork+0x10/0x50
[    1.823986] Call trace:
[    1.826420] Exception stack(0xffff8003ed8eb9c0 to 0xffff8003ed8ebaf0)
[    1.832848] b9c0: ffff8003ed728018 0001000000000000 ffff8003ed8ebb90 ffff00000874bfc0
[    1.840664] b9e0: ffff7e000fb5b880 ffff8003ef403c80 ffff8003ed8ebb00 ffff0000081d7f1c
[    1.848480] ba00: ffff7e000fb5b880 ffff8003ed6e2f80 ffff8003ed8ebb20 ffff0000081d7f1c
[    1.856295] ba20: ffff7e000fb5b880 ffff8003ed6e2a00 ffff8003ed8e0000 000000000008d9c0
[    1.864111] ba40: ffff8003ef403c80 ffff8003ed8e0000 ffff000008476ee4 ffff000008f1b000
[    1.871927] ba60: ffff8003ed728018 0000000000000000 0000000000000000 ffff00000807a06c
[    1.879743] ba80: 0000000000000000 0000000000000008 000000000000003f 0000000000000000
[    1.887559] baa0: ffff8003ed6e5080 0000000000000000 0101010101010101 0000000000000000
[    1.895374] bac0: 0000000000000018 0000000000000000 000000006255f330 0088000000000000
[    1.903190] bae0: 0000000000000003 00000000000017ff
[    1.908055] [<ffff00000874bfc0>] i2c_dw_clk_rate+0x20/0x30
[    1.913527] [<ffff00000874cb98>] i2c_dw_probe+0x38/0x238
[    1.918826] [<ffff00000874d67c>] dw_i2c_plat_probe+0x2b4/0x558
[    1.924646] [<ffff0000085546c0>] platform_drv_probe+0x58/0xc0
[    1.930379] [<ffff000008552824>] driver_probe_device+0x21c/0x2c0
[    1.936372] [<ffff000008552974>] __driver_attach+0xac/0xb0
[    1.941844] [<ffff00000855099c>] bus_for_each_dev+0x64/0xa0
[    1.947403] [<ffff000008552138>] driver_attach+0x20/0x28
[    1.952702] [<ffff000008551c88>] bus_add_driver+0x110/0x230
[    1.958261] [<ffff0000085532f8>] driver_register+0x60/0xf8
[    1.963734] [<ffff0000085545f8>] __platform_driver_register+0x40/0x48
[    1.970161] [<ffff000008d44b98>] dw_i2c_init_driver+0x18/0x20
[    1.975894] [<ffff0000080830f8>] do_one_initcall+0x38/0x120
[    1.981453] [<ffff000008d00cf0>] kernel_init_freeable+0x198/0x238
[    1.987533] [<ffff000008906f38>] kernel_init+0x10/0x100
[    1.992745] [<ffff000008082ec0>] ret_from_fork+0x10/0x50

Re: Regression: bd698d24b1b57: i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Andy Shevchenko]

On Tue, 2017-05-09 at 15:07 +0100, Lorenzo Pieralisi wrote:
> Hi guys,
> 
> as a heads-up, with today mainline (commit 2868b2513aa7) I get the
> following splat on AMD Seattle, reverting the $SUBJECT commit "solves"
> the problem.
> 
> My I2C knowledge is a bit limited but I am not sure I understand why
> we should be reading eg ss_hcnt/ss_lcnt depending on the dev->clk_freq
> but then i2c_dw_init() _always_ requires those values to be set for
> a given device. Again, I have no insights into I2C inner workings
> so apologies for the silly assumption/question.
> 
> Please have a look into this, thanks.

Since there is no clock defined you got a warning.

It means either ID is not added to drivers/acpi/acpi_apd.c or
platform has wrong values and thus 
dw_i2c_no_acpi_params should be expanded.

I have no such platform, so I can't tell which one is the right fix.
But it's not a revert by my opinion.

> [    1.160597] ------------[ cut here ]------------
> [    1.165207] WARNING: CPU: 0 PID: 1 at drivers/i2c/busses/i2c-
> designware-core.c:293 i2c_dw_clk_rate+0x20/0x30
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: Regression: bd698d24b1b57: i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Lorenzo Pieralisi]

[+ Ken, Jeff]

On Tue, May 09, 2017 at 05:54:11PM +0300, Andy Shevchenko wrote:
> On Tue, 2017-05-09 at 15:07 +0100, Lorenzo Pieralisi wrote:
> > Hi guys,
> > 
> > as a heads-up, with today mainline (commit 2868b2513aa7) I get the
> > following splat on AMD Seattle, reverting the $SUBJECT commit "solves"
> > the problem.
> > 
> > My I2C knowledge is a bit limited but I am not sure I understand why
> > we should be reading eg ss_hcnt/ss_lcnt depending on the dev->clk_freq
> > but then i2c_dw_init() _always_ requires those values to be set for
> > a given device. Again, I have no insights into I2C inner workings
> > so apologies for the silly assumption/question.
> > 
> > Please have a look into this, thanks.
> 
> Since there is no clock defined you got a warning.

IIUC there has never been a clock defined for this platform, that's
the problem. The warning appeared because the commit in $SUBJECT
prevents reading the ss_hcnt and ss_lcnt values from ACPI methods
that are there in ACPI tables (SSCN and FMCN), because it carries
out the SSCN FMCN look-up depending on the dev->clk_freq value.

Before $SUBJECT commit the values were read unconditionally from SSCN
and FMCN ACPI methods IIUC, again, I am no I2C expert so it is more
a question than anything else.

dev->clk_freq is set to 400000 by default because FW does not the
contain (ie never contained) "clock-frequency" property and
acpi_speed can't be ascertained through I2C resources, that's how
I read what's happening.

> It means either ID is not added to drivers/acpi/acpi_apd.c or
> platform has wrong values and thus 
> dw_i2c_no_acpi_params should be expanded.
> 
> I have no such platform, so I can't tell which one is the right fix.
> But it's not a revert by my opinion.

It was working before the commit in $SUBJECT was applied, AMD chaps
may be able to chime in hopefully to clarify, whether that's a revert
or not we shall see but with a given FW the kernel behaviour changed,
it may affect other platforms too if their FW was not upgraded
accordingly that's why I reported it.

Thanks !
Lorenzo

> 
> > [    1.160597] ------------[ cut here ]------------
> > [    1.165207] WARNING: CPU: 0 PID: 1 at drivers/i2c/busses/i2c-
> > designware-core.c:293 i2c_dw_clk_rate+0x20/0x30
> > 
> 
> -- 
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy

Re: Regression: bd698d24b1b57: i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Andy Shevchenko]

On Tue, 2017-05-09 at 16:52 +0100, Lorenzo Pieralisi wrote:
> [+ Ken, Jeff]
> 
> On Tue, May 09, 2017 at 05:54:11PM +0300, Andy Shevchenko wrote:
> > On Tue, 2017-05-09 at 15:07 +0100, Lorenzo Pieralisi wrote:
> > > Hi guys,
> > > 
> > > as a heads-up, with today mainline (commit 2868b2513aa7) I get the
> > > following splat on AMD Seattle, reverting the $SUBJECT commit
> > > "solves"
> > > the problem.
> > > 
> > > My I2C knowledge is a bit limited but I am not sure I understand
> > > why
> > > we should be reading eg ss_hcnt/ss_lcnt depending on the dev-
> > > >clk_freq
> > > but then i2c_dw_init() _always_ requires those values to be set
> > > for
> > > a given device. Again, I have no insights into I2C inner workings
> > > so apologies for the silly assumption/question.
> > > 
> > > Please have a look into this, thanks.
> > 
> > Since there is no clock defined you got a warning.
> 
> IIUC there has never been a clock defined for this platform, that's
> the problem. The warning appeared because the commit in $SUBJECT
> prevents reading the ss_hcnt and ss_lcnt values from ACPI methods
> that are there in ACPI tables (SSCN and FMCN), because it carries
> out the SSCN FMCN look-up depending on the dev->clk_freq value.
> 
> Before $SUBJECT commit the values were read unconditionally from SSCN
> and FMCN ACPI methods IIUC, again, I am no I2C expert so it is more
> a question than anything else.
> 
> dev->clk_freq is set to 400000 by default because FW does not the
> contain (ie never contained) "clock-frequency" property and
> acpi_speed can't be ascertained through I2C resources, that's how
> I read what's happening.
> 
> > It means either ID is not added to drivers/acpi/acpi_apd.c or

Just one question, did you look at all into above driver?
I suppose it missed ID and properties for the device.

> > platform has wrong values and thus 
> > dw_i2c_no_acpi_params should be expanded.
> > 
> > I have no such platform, so I can't tell which one is the right fix.
> > But it's not a revert by my opinion.
> 
> It was working before the commit in $SUBJECT was applied, AMD chaps
> may be able to chime in hopefully to clarify, whether that's a revert
> or not we shall see but with a given FW the kernel behaviour changed,
> it may affect other platforms too if their FW was not upgraded
> accordingly that's why I reported it.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: Regression: bd698d24b1b57: i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Lorenzo Pieralisi]

On Tue, May 09, 2017 at 07:20:48PM +0300, Andy Shevchenko wrote:
> On Tue, 2017-05-09 at 16:52 +0100, Lorenzo Pieralisi wrote:
> > [+ Ken, Jeff]
> > 
> > On Tue, May 09, 2017 at 05:54:11PM +0300, Andy Shevchenko wrote:
> > > On Tue, 2017-05-09 at 15:07 +0100, Lorenzo Pieralisi wrote:
> > > > Hi guys,
> > > > 
> > > > as a heads-up, with today mainline (commit 2868b2513aa7) I get the
> > > > following splat on AMD Seattle, reverting the $SUBJECT commit
> > > > "solves"
> > > > the problem.
> > > > 
> > > > My I2C knowledge is a bit limited but I am not sure I understand
> > > > why
> > > > we should be reading eg ss_hcnt/ss_lcnt depending on the dev-
> > > > >clk_freq
> > > > but then i2c_dw_init() _always_ requires those values to be set
> > > > for
> > > > a given device. Again, I have no insights into I2C inner workings
> > > > so apologies for the silly assumption/question.
> > > > 
> > > > Please have a look into this, thanks.
> > > 
> > > Since there is no clock defined you got a warning.
> > 
> > IIUC there has never been a clock defined for this platform, that's
> > the problem. The warning appeared because the commit in $SUBJECT
> > prevents reading the ss_hcnt and ss_lcnt values from ACPI methods
> > that are there in ACPI tables (SSCN and FMCN), because it carries
> > out the SSCN FMCN look-up depending on the dev->clk_freq value.
> > 
> > Before $SUBJECT commit the values were read unconditionally from SSCN
> > and FMCN ACPI methods IIUC, again, I am no I2C expert so it is more
> > a question than anything else.
> > 
> > dev->clk_freq is set to 400000 by default because FW does not the
> > contain (ie never contained) "clock-frequency" property and
> > acpi_speed can't be ascertained through I2C resources, that's how
> > I read what's happening.
> > 
> > > It means either ID is not added to drivers/acpi/acpi_apd.c or
> 
> Just one question, did you look at all into above driver?
> I suppose it missed ID and properties for the device.

I looked into it yes. I have also looked at ACPI tables and they
contain the SSCN and FMCN methods and AFAIK the set-up was working
fine before the commit in $SUBJECT - AMD guys - please correct me
if I am wrong, I found this thread which explains things a bit:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/395983.html

If we have to patch drivers/acpi/acpi_apd.c to restore the previous
behaviour that's fine by me but this is a regression nonetheless unless
someone explains to me why the current firmware set-up (with SSCN and
FMCN) is unreliable/deprecated, it is not clear to me at all and it
may affect other platforms too.

Thanks,
Lorenzo

Re: Regression: bd698d24b1b57: i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Jarkko Nikula]

On 05/10/2017 12:24 PM, Lorenzo Pieralisi wrote:
> On Tue, May 09, 2017 at 07:20:48PM +0300, Andy Shevchenko wrote:
>>>> It means either ID is not added to drivers/acpi/acpi_apd.c or
>>
>> Just one question, did you look at all into above driver?
>> I suppose it missed ID and properties for the device.
>
> I looked into it yes. I have also looked at ACPI tables and they
> contain the SSCN and FMCN methods and AFAIK the set-up was working
> fine before the commit in $SUBJECT - AMD guys - please correct me
> if I am wrong, I found this thread which explains things a bit:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/395983.html
>
> If we have to patch drivers/acpi/acpi_apd.c to restore the previous
> behaviour that's fine by me but this is a regression nonetheless unless
> someone explains to me why the current firmware set-up (with SSCN and
> FMCN) is unreliable/deprecated, it is not clear to me at all and it
> may affect other platforms too.
>
I guess best would be to fix the i2c_dw_init(). Maybe by moving timing 
parameters settings out to another function, do setting only for 
selected speed and calculate only once in case parameters are not set.

I don't think timing parameters for standard or fast speeds are not 
needed unless operating at those speeds but have to check that first. At 
least that's what we are doing for high speed.

-- 
Jarkko

Re: Regression: bd698d24b1b57: i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Ard Biesheuvel]

On 10 May 2017 at 14:55, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> On 05/10/2017 12:24 PM, Lorenzo Pieralisi wrote:
>>
>> On Tue, May 09, 2017 at 07:20:48PM +0300, Andy Shevchenko wrote:
>>>>>
>>>>> It means either ID is not added to drivers/acpi/acpi_apd.c or
>>>
>>>
>>> Just one question, did you look at all into above driver?
>>> I suppose it missed ID and properties for the device.
>>
>>
>> I looked into it yes. I have also looked at ACPI tables and they
>> contain the SSCN and FMCN methods and AFAIK the set-up was working
>> fine before the commit in $SUBJECT - AMD guys - please correct me
>> if I am wrong, I found this thread which explains things a bit:
>>
>>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/395983.html
>>
>> If we have to patch drivers/acpi/acpi_apd.c to restore the previous
>> behaviour that's fine by me but this is a regression nonetheless unless
>> someone explains to me why the current firmware set-up (with SSCN and
>> FMCN) is unreliable/deprecated, it is not clear to me at all and it
>> may affect other platforms too.
>>
> I guess best would be to fix the i2c_dw_init(). Maybe by moving timing
> parameters settings out to another function, do setting only for selected
> speed and calculate only once in case parameters are not set.
>
> I don't think timing parameters for standard or fast speeds are not needed
> unless operating at those speeds but have to check that first. At least
> that's what we are doing for high speed.
>

Commit bd698d24b1b57 is broken and should be reverted: the splat on
AMD Seattle is the most visible symptom, but the logic change results
in DW_IC_SS_SCL_HCNT/DW_IC_SS_SCL_LCNT possibly being set to different
values than specified by the ACPI methods. The missing clock splat is
only a symptom of this: the driver tries to calculate values from the
clock frequency that should have been read from the DSDT.

For reference, this is the description we have on the platform in question:

            Method (SSCN, 0, NotSerialized)
            {
                Return (Package (0x03)
                {
                    0x0430,
                    0x04E1,
                    0x00
                })
            }

            Method (FMCN, 0, NotSerialized)
            {
                Return (Package (0x03)
                {
                    0x00DE,
                    0x018F,
                    0x00
                })
            }

Whether it hurts to program different standard mode values here is
irrelevant. It is simply wrong.

Re: Regression: bd698d24b1b57: i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Jarkko Nikula]

On 05/18/2017 03:24 PM, Ard Biesheuvel wrote:
> On 10 May 2017 at 14:55, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
>> On 05/10/2017 12:24 PM, Lorenzo Pieralisi wrote:
>>>
>>> On Tue, May 09, 2017 at 07:20:48PM +0300, Andy Shevchenko wrote:
>>>>>>
>>>>>> It means either ID is not added to drivers/acpi/acpi_apd.c or
>>>>
>>>>
>>>> Just one question, did you look at all into above driver?
>>>> I suppose it missed ID and properties for the device.
>>>
>>>
>>> I looked into it yes. I have also looked at ACPI tables and they
>>> contain the SSCN and FMCN methods and AFAIK the set-up was working
>>> fine before the commit in $SUBJECT - AMD guys - please correct me
>>> if I am wrong, I found this thread which explains things a bit:
>>>
>>>
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/395983.html
>>>
>>> If we have to patch drivers/acpi/acpi_apd.c to restore the previous
>>> behaviour that's fine by me but this is a regression nonetheless unless
>>> someone explains to me why the current firmware set-up (with SSCN and
>>> FMCN) is unreliable/deprecated, it is not clear to me at all and it
>>> may affect other platforms too.
>>>
>> I guess best would be to fix the i2c_dw_init(). Maybe by moving timing
>> parameters settings out to another function, do setting only for selected
>> speed and calculate only once in case parameters are not set.
>>
>> I don't think timing parameters for standard or fast speeds are not needed
>> unless operating at those speeds but have to check that first. At least
>> that's what we are doing for high speed.
>>
>
> Commit bd698d24b1b57 is broken and should be reverted: the splat on
> AMD Seattle is the most visible symptom, but the logic change results
> in DW_IC_SS_SCL_HCNT/DW_IC_SS_SCL_LCNT possibly being set to different
> values than specified by the ACPI methods. The missing clock splat is
> only a symptom of this: the driver tries to calculate values from the
> clock frequency that should have been read from the DSDT.
>
> For reference, this is the description we have on the platform in question:
>
>             Method (SSCN, 0, NotSerialized)
>             {
>                 Return (Package (0x03)
>                 {
>                     0x0430,
>                     0x04E1,
>                     0x00
>                 })
>             }
>
>             Method (FMCN, 0, NotSerialized)
>             {
>                 Return (Package (0x03)
>                 {
>                     0x00DE,
>                     0x018F,
>                     0x00
>                 })
>             }
>
> Whether it hurts to program different standard mode values here is
> irrelevant. It is simply wrong.
>
I have a fix for this. Could you try does it fix the issue for you?

http://www.spinics.net/lists/linux-i2c/msg29509.html

-- 
Jarkko

Re: Regression: bd698d24b1b57: i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Ard Biesheuvel]

On 18 May 2017 at 14:05, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> On 05/18/2017 03:24 PM, Ard Biesheuvel wrote:
>>
>> On 10 May 2017 at 14:55, Jarkko Nikula <jarkko.nikula@linux.intel.com>
>> wrote:
>>>
>>> On 05/10/2017 12:24 PM, Lorenzo Pieralisi wrote:
>>>>
>>>>
>>>> On Tue, May 09, 2017 at 07:20:48PM +0300, Andy Shevchenko wrote:
>>>>>>>
>>>>>>>
>>>>>>> It means either ID is not added to drivers/acpi/acpi_apd.c or
>>>>>
>>>>>
>>>>>
>>>>> Just one question, did you look at all into above driver?
>>>>> I suppose it missed ID and properties for the device.
>>>>
>>>>
>>>>
>>>> I looked into it yes. I have also looked at ACPI tables and they
>>>> contain the SSCN and FMCN methods and AFAIK the set-up was working
>>>> fine before the commit in $SUBJECT - AMD guys - please correct me
>>>> if I am wrong, I found this thread which explains things a bit:
>>>>
>>>>
>>>>
>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/395983.html
>>>>
>>>> If we have to patch drivers/acpi/acpi_apd.c to restore the previous
>>>> behaviour that's fine by me but this is a regression nonetheless unless
>>>> someone explains to me why the current firmware set-up (with SSCN and
>>>> FMCN) is unreliable/deprecated, it is not clear to me at all and it
>>>> may affect other platforms too.
>>>>
>>> I guess best would be to fix the i2c_dw_init(). Maybe by moving timing
>>> parameters settings out to another function, do setting only for selected
>>> speed and calculate only once in case parameters are not set.
>>>
>>> I don't think timing parameters for standard or fast speeds are not
>>> needed
>>> unless operating at those speeds but have to check that first. At least
>>> that's what we are doing for high speed.
>>>
>>
>> Commit bd698d24b1b57 is broken and should be reverted: the splat on
>> AMD Seattle is the most visible symptom, but the logic change results
>> in DW_IC_SS_SCL_HCNT/DW_IC_SS_SCL_LCNT possibly being set to different
>> values than specified by the ACPI methods. The missing clock splat is
>> only a symptom of this: the driver tries to calculate values from the
>> clock frequency that should have been read from the DSDT.
>>
>> For reference, this is the description we have on the platform in
>> question:
>>
>>             Method (SSCN, 0, NotSerialized)
>>             {
>>                 Return (Package (0x03)
>>                 {
>>                     0x0430,
>>                     0x04E1,
>>                     0x00
>>                 })
>>             }
>>
>>             Method (FMCN, 0, NotSerialized)
>>             {
>>                 Return (Package (0x03)
>>                 {
>>                     0x00DE,
>>                     0x018F,
>>                     0x00
>>                 })
>>             }
>>
>> Whether it hurts to program different standard mode values here is
>> irrelevant. It is simply wrong.
>>
> I have a fix for this. Could you try does it fix the issue for you?
>
> http://www.spinics.net/lists/linux-i2c/msg29509.html
>

Thanks. But the question is really whether we want to change the
driver so that the DW_IC_SS_SCL_HCNT/DW_IC_SS_SCL_LCNT registers are
no longer programmed at all when running at a higher speed. Can you
guarantee that this will not cause any change in behavior in all
instances of this hardware that are supported currently, no matter how
they are synthesized and described to the OS? The previous commit only
affected the platdrv variety AFAIR

Re: Regression: bd698d24b1b57: i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Jarkko Nikula]

On 05/18/2017 04:44 PM, Ard Biesheuvel wrote:
> On 18 May 2017 at 14:05, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
>> I have a fix for this. Could you try does it fix the issue for you?
>>
>> http://www.spinics.net/lists/linux-i2c/msg29509.html
>>
>
> Thanks. But the question is really whether we want to change the
> driver so that the DW_IC_SS_SCL_HCNT/DW_IC_SS_SCL_LCNT registers are
> no longer programmed at all when running at a higher speed. Can you
> guarantee that this will not cause any change in behavior in all
> instances of this hardware that are supported currently, no matter how
> they are synthesized and described to the OS? The previous commit only
> affected the platdrv variety AFAIR
>
Obviously I cannot guarantee are there any side effects as I don't know 
the IP. However I forgot one important point: high-speed transfers 
always starts in fast-mode so those timing parameters too are needed 
when bus is configured for high-speed. So best is to write all of them.

-- 
Jarkko