[PATCH linux dev-4.10] drivers/misc: (aspeed-lpc-snoop): Add ast2400 to compat

[PATCH linux dev-4.10] drivers/misc: (aspeed-lpc-snoop): Add ast2400 to compat

[Patrick Venture]

This driver can be used on the aspeed ast2400.

Tested: ast2400 on quanta-q71l

Signed-off-by: Patrick Venture <venture@google.com>
---
 drivers/misc/aspeed-lpc-snoop.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/misc/aspeed-lpc-snoop.c b/drivers/misc/aspeed-lpc-snoop.c
index 593905565b74..0647cff6280a 100644
--- a/drivers/misc/aspeed-lpc-snoop.c
+++ b/drivers/misc/aspeed-lpc-snoop.c
@@ -241,6 +241,7 @@ static int aspeed_lpc_snoop_remove(struct platform_device *pdev)
 
 static const struct of_device_id aspeed_lpc_snoop_match[] = {
 	{ .compatible = "aspeed,ast2500-lpc-snoop" },
+	{ .compatible = "aspeed,ast2400-lpc-snoop" },
 	{ },
 };
 
-- 
2.13.2.725.g09c95d1e9-goog

Re: [PATCH linux dev-4.10] drivers/misc: (aspeed-lpc-snoop): Add ast2400 to compat

[Greg KH]

On Tue, Jul 04, 2017 at 08:45:03AM -0700, Patrick Venture wrote:
> This driver can be used on the aspeed ast2400.
> 
> Tested: ast2400 on quanta-q71l
> 
> Signed-off-by: Patrick Venture <venture@google.com>
> ---
>  drivers/misc/aspeed-lpc-snoop.c | 1 +
>  1 file changed, 1 insertion(+)

How can we go back in time and apply something to the 4.10 kernel tree?

confused,

greg k-h

Re: [PATCH linux dev-4.10] drivers/misc: (aspeed-lpc-snoop): Add ast2400 to compat

[Patrick Venture]

On Tue, Jul 4, 2017 at 8:50 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Jul 04, 2017 at 08:45:03AM -0700, Patrick Venture wrote:
>> This driver can be used on the aspeed ast2400.
>>
>> Tested: ast2400 on quanta-q71l
>>
>> Signed-off-by: Patrick Venture <venture@google.com>
>> ---
>>  drivers/misc/aspeed-lpc-snoop.c | 1 +
>>  1 file changed, 1 insertion(+)
>
> How can we go back in time and apply something to the 4.10 kernel tree?
>
> confused,
>
> greg k-h

Ooops. Bad subject line. I originally submitted this to a downstream
4.10.  Let me fix that.

Re: [PATCH linux dev-4.10] drivers/misc: (aspeed-lpc-snoop): Add ast2400 to compat

[Rob Lippert]

I checked the datasheets when I wrote this and ast2400 does not have
the (undocumented) HICRB register bits 14,15 that enables the BMC to
actually respond to the snoop'ed address.

Without setting that bit in the ast2500 the transactions to that I/O
port would timeout on the host side (although the BMC snoop logic
would still see it and log it).
Probably not an issue for x86 systems that don't have any LPC I/O
error handling anyways but LPC timeouts causes issues with POWER
systems since it sets a hardware FIR bit which can cause boot
failures.

-Rob

On Tue, Jul 4, 2017 at 8:45 AM, Patrick Venture <venture@google.com> wrote:
> This driver can be used on the aspeed ast2400.
>
> Tested: ast2400 on quanta-q71l
>
> Signed-off-by: Patrick Venture <venture@google.com>
> ---
>  drivers/misc/aspeed-lpc-snoop.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/misc/aspeed-lpc-snoop.c b/drivers/misc/aspeed-lpc-snoop.c
> index 593905565b74..0647cff6280a 100644
> --- a/drivers/misc/aspeed-lpc-snoop.c
> +++ b/drivers/misc/aspeed-lpc-snoop.c
> @@ -241,6 +241,7 @@ static int aspeed_lpc_snoop_remove(struct platform_device *pdev)
>
>  static const struct of_device_id aspeed_lpc_snoop_match[] = {
>         { .compatible = "aspeed,ast2500-lpc-snoop" },
> +       { .compatible = "aspeed,ast2400-lpc-snoop" },
>         { },
>  };
>
> --
> 2.13.2.725.g09c95d1e9-goog
>

Re: [PATCH linux dev-4.10] drivers/misc: (aspeed-lpc-snoop): Add ast2400 to compat

[Patrick Venture]

On Wed, Jul 5, 2017 at 10:33 AM, Rob Lippert <roblip@gmail.com> wrote:
> I checked the datasheets when I wrote this and ast2400 does not have
> the (undocumented) HICRB register bits 14,15 that enables the BMC to
> actually respond to the snoop'ed address.

You're right, it is marked as "reserved" in the datasheet for the ast2400.

>
> Without setting that bit in the ast2500 the transactions to that I/O
> port would timeout on the host side (although the BMC snoop logic
> would still see it and log it).
> Probably not an issue for x86 systems that don't have any LPC I/O
> error handling anyways but LPC timeouts causes issues with POWER
> systems since it sets a hardware FIR bit which can cause boot
> failures.

Interesting.  I've been running experiments on x86 and I haven't seen
any errors, so that adds more credence to your point.  If a device
doesn't respond within X time, three times in a row, you get a triple
fault.  But, on x86, I don't think I've seen any mechanism with an
expectation that a port IO write will have a guaranteed response.

For the use-case I'm chasing, my goal being to snoop PoST codes from
the host, there is in the datasheet a post-code control register set,
but I haven't explored configuring them or whether someone has written
the fifo driver for them.

>
> -Rob
>
> On Tue, Jul 4, 2017 at 8:45 AM, Patrick Venture <venture@google.com> wrote:
>> This driver can be used on the aspeed ast2400.
>>
>> Tested: ast2400 on quanta-q71l
>>
>> Signed-off-by: Patrick Venture <venture@google.com>
>> ---
>>  drivers/misc/aspeed-lpc-snoop.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/misc/aspeed-lpc-snoop.c b/drivers/misc/aspeed-lpc-snoop.c
>> index 593905565b74..0647cff6280a 100644
>> --- a/drivers/misc/aspeed-lpc-snoop.c
>> +++ b/drivers/misc/aspeed-lpc-snoop.c
>> @@ -241,6 +241,7 @@ static int aspeed_lpc_snoop_remove(struct platform_device *pdev)
>>
>>  static const struct of_device_id aspeed_lpc_snoop_match[] = {
>>         { .compatible = "aspeed,ast2500-lpc-snoop" },
>> +       { .compatible = "aspeed,ast2400-lpc-snoop" },
>>         { },

An approach would be to ditch this change and instead refer to the
ast2500-lpc-snoop in my device-tree to avoid anyone non-x86 from
running this configuration and hitting issues.

>>  };
>>
>> --
>> 2.13.2.725.g09c95d1e9-goog
>>

Re: [PATCH linux dev-4.10] drivers/misc: (aspeed-lpc-snoop): Add ast2400 to compat

[Rob Lippert]

On Wed, Jul 5, 2017 at 10:48 AM, Patrick Venture <venture@google.com> wrote:
> On Wed, Jul 5, 2017 at 10:33 AM, Rob Lippert <roblip@gmail.com> wrote:
>> I checked the datasheets when I wrote this and ast2400 does not have
>> the (undocumented) HICRB register bits 14,15 that enables the BMC to
>> actually respond to the snoop'ed address.
>
> You're right, it is marked as "reserved" in the datasheet for the ast2400.
>
>>
>> Without setting that bit in the ast2500 the transactions to that I/O
>> port would timeout on the host side (although the BMC snoop logic
>> would still see it and log it).
>> Probably not an issue for x86 systems that don't have any LPC I/O
>> error handling anyways but LPC timeouts causes issues with POWER
>> systems since it sets a hardware FIR bit which can cause boot
>> failures.
>
> Interesting.  I've been running experiments on x86 and I haven't seen
> any errors, so that adds more credence to your point.  If a device
> doesn't respond within X time, three times in a row, you get a triple
> fault.  But, on x86, I don't think I've seen any mechanism with an
> expectation that a port IO write will have a guaranteed response.
>
> For the use-case I'm chasing, my goal being to snoop PoST codes from
> the host, there is in the datasheet a post-code control register set,
> but I haven't explored configuring them or whether someone has written
> the fifo driver for them.
>
>>
>> -Rob
>>
>> On Tue, Jul 4, 2017 at 8:45 AM, Patrick Venture <venture@google.com> wrote:
>>> This driver can be used on the aspeed ast2400.
>>>
>>> Tested: ast2400 on quanta-q71l
>>>
>>> Signed-off-by: Patrick Venture <venture@google.com>
>>> ---
>>>  drivers/misc/aspeed-lpc-snoop.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/misc/aspeed-lpc-snoop.c b/drivers/misc/aspeed-lpc-snoop.c
>>> index 593905565b74..0647cff6280a 100644
>>> --- a/drivers/misc/aspeed-lpc-snoop.c
>>> +++ b/drivers/misc/aspeed-lpc-snoop.c
>>> @@ -241,6 +241,7 @@ static int aspeed_lpc_snoop_remove(struct platform_device *pdev)
>>>
>>>  static const struct of_device_id aspeed_lpc_snoop_match[] = {
>>>         { .compatible = "aspeed,ast2500-lpc-snoop" },
>>> +       { .compatible = "aspeed,ast2400-lpc-snoop" },
>>>         { },
>
> An approach would be to ditch this change and instead refer to the
> ast2500-lpc-snoop in my device-tree to avoid anyone non-x86 from
> running this configuration and hitting issues.

This change is probably fine since the driver does still work but you
should also guard the setting of the HICRB bits with #ifdef
MACH_ASPEED_G5 or similar to avoid setting reserved bits on the G4
hardware.

-Rob

Re: [PATCH linux dev-4.10] drivers/misc: (aspeed-lpc-snoop): Add ast2400 to compat

[Patrick Venture]

On Wed, Jul 5, 2017 at 10:52 AM, Rob Lippert <roblip@gmail.com> wrote:
> On Wed, Jul 5, 2017 at 10:48 AM, Patrick Venture <venture@google.com> wrote:
>> On Wed, Jul 5, 2017 at 10:33 AM, Rob Lippert <roblip@gmail.com> wrote:
>>> I checked the datasheets when I wrote this and ast2400 does not have
>>> the (undocumented) HICRB register bits 14,15 that enables the BMC to
>>> actually respond to the snoop'ed address.
>>
>> You're right, it is marked as "reserved" in the datasheet for the ast2400.
>>
>>>
>>> Without setting that bit in the ast2500 the transactions to that I/O
>>> port would timeout on the host side (although the BMC snoop logic
>>> would still see it and log it).
>>> Probably not an issue for x86 systems that don't have any LPC I/O
>>> error handling anyways but LPC timeouts causes issues with POWER
>>> systems since it sets a hardware FIR bit which can cause boot
>>> failures.
>>
>> Interesting.  I've been running experiments on x86 and I haven't seen
>> any errors, so that adds more credence to your point.  If a device
>> doesn't respond within X time, three times in a row, you get a triple
>> fault.  But, on x86, I don't think I've seen any mechanism with an
>> expectation that a port IO write will have a guaranteed response.
>>
>> For the use-case I'm chasing, my goal being to snoop PoST codes from
>> the host, there is in the datasheet a post-code control register set,
>> but I haven't explored configuring them or whether someone has written
>> the fifo driver for them.
>>
>>>
>>> -Rob
>>>
>>> On Tue, Jul 4, 2017 at 8:45 AM, Patrick Venture <venture@google.com> wrote:
>>>> This driver can be used on the aspeed ast2400.
>>>>
>>>> Tested: ast2400 on quanta-q71l
>>>>
>>>> Signed-off-by: Patrick Venture <venture@google.com>
>>>> ---
>>>>  drivers/misc/aspeed-lpc-snoop.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/misc/aspeed-lpc-snoop.c b/drivers/misc/aspeed-lpc-snoop.c
>>>> index 593905565b74..0647cff6280a 100644
>>>> --- a/drivers/misc/aspeed-lpc-snoop.c
>>>> +++ b/drivers/misc/aspeed-lpc-snoop.c
>>>> @@ -241,6 +241,7 @@ static int aspeed_lpc_snoop_remove(struct platform_device *pdev)
>>>>
>>>>  static const struct of_device_id aspeed_lpc_snoop_match[] = {
>>>>         { .compatible = "aspeed,ast2500-lpc-snoop" },
>>>> +       { .compatible = "aspeed,ast2400-lpc-snoop" },
>>>>         { },
>>
>> An approach would be to ditch this change and instead refer to the
>> ast2500-lpc-snoop in my device-tree to avoid anyone non-x86 from
>> running this configuration and hitting issues.
>
> This change is probably fine since the driver does still work but you
> should also guard the setting of the HICRB bits with #ifdef
> MACH_ASPEED_G5 or similar to avoid setting reserved bits on the G4
> hardware.

Will update patch forthwith.

>
> -Rob