All of lore.kernel.org
 help / color / mirror / Atom feed
From: Brijesh Singh <brijesh.singh@amd.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: brijesh.singh@amd.com, Tejun Heo <tj@kernel.org>,
	linux-kernel@vger.kernel.org, hdegoede@redhat.com,
	linux-ide@vger.kernel.org, Graeme Gregory <graeme@xora.org.uk>
Subject: Re: [PATCH v2] ata: add AMD Seattle platform driver
Date: Tue, 2 Feb 2016 12:37:58 -0600	[thread overview]
Message-ID: <56B0F786.9010504@amd.com> (raw)
In-Reply-To: <4743985.XZ7D12RurV@wuerfel>

Hi,

On 02/02/2016 08:08 AM, Arnd Bergmann wrote:
> On Monday 01 February 2016 16:15:59 Brijesh Singh wrote:
>>>
>>> This is where we really need the ACPI maintainers to explain the
>>> general policy for dealing with firmware updates.
>>>
>>> I would assume that adding the feature in a later firmware version
>>> is a compatible change, and the feature is non-essential (the
>>> device will work fine with the generic SATA driver, except
>>> the LEDs don't blink), so it's not a big deal, it's just what
>>> you get for having the firmware shipped before the driver is
>>> reviewed (don't do that).
>>>
>>
>> Agreed, the driver should have been reviewed earlier. And now changes in firmware will also require
>> them changing other OSes drivers.
> 
> Can you explain that? I would expect the addition of some AML methods
> to be a compatible change.
> 

current DSDT entry looks like this:

Device (SATA0)
{
.....

 Name(_CRS, ResourceTemplate()
 {
   Memory32Fixed(ReadWrite, 0xE03000000, 0x000010000)  /* SATA block address */
   Interrupt(ResourceConsumer, Level, ActiveHigh Exclusive,,,) { 387}
   Memory32Fixed(ReadWrite, 0xE00000078, 1)  /* SGPIO register */
}
  
......
}

Windows driver folks were okay to look at second resource field to map the SGPIO register and program the
registers to blink the LEDs. I think as per ACPI spec, its legal to pass more than one block in resource
template and since AML method is not mandatory for non standard enclosure management hence its entirely
possible that some BIOS vendors may not implement it at all. But if they implement and decide
to expose either AML method or register map but not both then Windows driver may break.

I believe most of BIOS vendors are using above DSDT block for SATA and by implementing the platform driver
we could enable this feature right away in Linux OS. We do prefer to use generic driver however given the current situation platform driver seems a reasonable choice. I wish SoC designer had implemented the full 
enclosure management per SATA spec.


> 	Arnd
> 

WARNING: multiple messages have this Message-ID (diff)
From: Brijesh Singh <brijesh.singh@amd.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: <brijesh.singh@amd.com>, Tejun Heo <tj@kernel.org>,
	<linux-kernel@vger.kernel.org>, <hdegoede@redhat.com>,
	<linux-ide@vger.kernel.org>, Graeme Gregory <graeme@xora.org.uk>
Subject: Re: [PATCH v2] ata: add AMD Seattle platform driver
Date: Tue, 2 Feb 2016 12:37:58 -0600	[thread overview]
Message-ID: <56B0F786.9010504@amd.com> (raw)
In-Reply-To: <4743985.XZ7D12RurV@wuerfel>

Hi,

On 02/02/2016 08:08 AM, Arnd Bergmann wrote:
> On Monday 01 February 2016 16:15:59 Brijesh Singh wrote:
>>>
>>> This is where we really need the ACPI maintainers to explain the
>>> general policy for dealing with firmware updates.
>>>
>>> I would assume that adding the feature in a later firmware version
>>> is a compatible change, and the feature is non-essential (the
>>> device will work fine with the generic SATA driver, except
>>> the LEDs don't blink), so it's not a big deal, it's just what
>>> you get for having the firmware shipped before the driver is
>>> reviewed (don't do that).
>>>
>>
>> Agreed, the driver should have been reviewed earlier. And now changes in firmware will also require
>> them changing other OSes drivers.
> 
> Can you explain that? I would expect the addition of some AML methods
> to be a compatible change.
> 

current DSDT entry looks like this:

Device (SATA0)
{
.....

 Name(_CRS, ResourceTemplate()
 {
   Memory32Fixed(ReadWrite, 0xE03000000, 0x000010000)  /* SATA block address */
   Interrupt(ResourceConsumer, Level, ActiveHigh Exclusive,,,) { 387}
   Memory32Fixed(ReadWrite, 0xE00000078, 1)  /* SGPIO register */
}
  
......
}

Windows driver folks were okay to look at second resource field to map the SGPIO register and program the
registers to blink the LEDs. I think as per ACPI spec, its legal to pass more than one block in resource
template and since AML method is not mandatory for non standard enclosure management hence its entirely
possible that some BIOS vendors may not implement it at all. But if they implement and decide
to expose either AML method or register map but not both then Windows driver may break.

I believe most of BIOS vendors are using above DSDT block for SATA and by implementing the platform driver
we could enable this feature right away in Linux OS. We do prefer to use generic driver however given the current situation platform driver seems a reasonable choice. I wish SoC designer had implemented the full 
enclosure management per SATA spec.


> 	Arnd
> 

  reply	other threads:[~2016-02-02 18:40 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-01-14 16:31 [PATCH v2] ata: add AMD Seattle platform driver Brijesh Singh
2016-01-14 16:31 ` Brijesh Singh
2016-01-20 21:24 ` Brijesh Singh
2016-01-20 21:24   ` Brijesh Singh
2016-01-25 20:43 ` Tejun Heo
2016-01-26  9:36   ` Hans de Goede
2016-03-16 20:12     ` Brijesh Singh
2016-03-16 20:12       ` Brijesh Singh
2016-01-26 12:17   ` Arnd Bergmann
2016-01-26 16:56     ` Brijesh Singh
2016-01-26 16:56       ` Brijesh Singh
2016-01-29 21:22       ` Arnd Bergmann
2016-01-29 21:31         ` One Thousand Gnomes
2016-02-01 18:56         ` Brijesh Singh
2016-02-01 18:56           ` Brijesh Singh
2016-02-01 20:14           ` Arnd Bergmann
2016-02-01 22:15             ` Brijesh Singh
2016-02-01 22:15               ` Brijesh Singh
2016-02-02 14:08               ` Arnd Bergmann
2016-02-02 18:37                 ` Brijesh Singh [this message]
2016-02-02 18:37                   ` Brijesh Singh
2016-02-05 14:50                   ` Arnd Bergmann
2016-02-05 17:23                     ` Brijesh Singh
2016-02-05 17:23                       ` Brijesh Singh
2016-02-08 18:12                       ` Brijesh Singh
2016-02-08 18:12                         ` Brijesh Singh
2016-03-16 21:07             ` Tejun Heo
2016-03-17 17:36               ` Arnd Bergmann
2016-03-18 18:36                 ` Brijesh Singh
2016-03-18 18:36                   ` Brijesh Singh
2016-03-18 20:19                   ` Tejun Heo
2016-04-14  9:08                   ` Matthias Brugger
2016-04-14 22:14                     ` Brijesh Singh
2016-04-14 22:14                       ` Brijesh Singh
2016-04-13 19:15 ` Tejun Heo

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=56B0F786.9010504@amd.com \
    --to=brijesh.singh@amd.com \
    --cc=arnd@arndb.de \
    --cc=graeme@xora.org.uk \
    --cc=hdegoede@redhat.com \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tj@kernel.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.