Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1322107 > unrolled thread

Re: [PATCH v2] ata: add AMD Seattle platform driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] ata: add AMD Seattle platform driver Arnd Bergmann <arnd@arndb.de> - 2016-01-29 22:30 +0100
    Re: [PATCH v2] ata: add AMD Seattle platform driver One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-01-29 22:40 +0100
    Re: [PATCH v2] ata: add AMD Seattle platform driver Arnd Bergmann <arnd@arndb.de> - 2016-02-01 21:20 +0100
      Re: [PATCH v2] ata: add AMD Seattle platform driver Arnd Bergmann <arnd@arndb.de> - 2016-02-02 15:10 +0100
        Re: [PATCH v2] ata: add AMD Seattle platform driver Arnd Bergmann <arnd@arndb.de> - 2016-02-05 16:00 +0100

#1322107 — Re: [PATCH v2] ata: add AMD Seattle platform driver

On Tuesday 26 January 2016 10:56:20 Brijesh Singh wrote:
> 
> On 01/26/2016 06:17 AM, Arnd Bergmann wrote:
> > 
> > I think it needs more work: The changelog describes it as a normal
> > driver, but based on the previous discussion, this is just a hack
> > to work around broken BIOS versions that can no longer be fixed in
> > the field, and there has not been a decision what the proper
> > representation should be in ACPI.
> > 
> I am not sure if we should label this driver as a hack to workaround the
> broken BIOS. Unfortunately SoC did not implemented the enclosure management
> per spec. Its not BIOS issue.

The BIOS issue is how the hardware workaround is represented. The
idea of ACPI is to hide this kind of glitch from the operating system
instead of forcing it to have a driver for each hardware variant.

> > The patch also fails to address the devicetree based case, even though
> > we did come to a conclusion that the current behavior is a regression
> > (compared to what we had in drivers/ide/) and that there is a relatively
> > simple fix to do it right.
> > 
> I did looked at your recommendation for extending libahci to use ledtrig_ide_activity()
> but as I pointed out in previous discussion this function is missing several key features
> from EM (enclosure management) pov. e.g missing the slot number, missing the locate and fault led.
> In case of EM, each port will have at least three leds (activity, locate and fault). 
> Since these LED's are part of EM hence we need to ensure that tools like ledmon and ledctl (which uses libahci sysfs) works well.

I'd assume this can be easily extended, we just need to come up with
a naming scheme for the LEDs so we can identify them in DT.

> The main question is, what is recommended approach to override libachi enclosure managements
> transfer led messages function? A platform driver or something else.
> 
> Tejun and/or Hans do you have any recommendation ?

I think for the DT case, a platform driver that registers itself to the LED
subsystem is the best way. You probably still want to put the hardware
register into a "syscon" device that contains the entire set of registers
around it (presumably more hacks for other hardware features), and then
just reference the register using a regmap from the LED driver. The generic
AHCI driver can then get extended with supports for the LED subsystem, to
look for specific LEDs by name based on lot number and type of LED.

Those can be present on any machine with a generic AHCI implementation,
and can be easily implemented using the GPIO-LED driver on machines that
don't have a special purpose register for them.

For the ACPI case, I still think that an AML call from the AHCI driver
is the most logical solution. You mentioned that you believe that calling
into the AML interpreter up to 100 times per second is a noticeable
overhead, but I doubt that and would like to see actual number backing
that up. Note that most of the time, the status of the LEDs won't even
change, so the driver does not have to call into the AML while I/O
is in progress, or while it is stopped, only for the transition or in
case of locate and fault events that should be extremely rare.

	Arnd

[toc] | [next] | [standalone]

#1322115

> For the ACPI case, I still think that an AML call from the AHCI driver
> is the most logical solution. You mentioned that you believe that calling
> into the AML interpreter up to 100 times per second is a noticeable
> overhead, but I doubt that and would like to see actual number backing
> that up. Note that most of the time, the status of the LEDs won't even
> change, so the driver does not have to call into the AML while I/O
> is in progress, or while it is stopped, only for the transition or in
> case of locate and fault events that should be extremely rare.

It can also be delayed - you don't need to adjust the LED at any great
rate - it's for a human not a machine to read.

Alan

[toc] | [prev] | [next] | [standalone]

#1323453

On Monday 01 February 2016 12:56:06 Brijesh Singh wrote:
> On 01/29/2016 03:22 PM, Arnd Bergmann wrote:
> > 
> > For the ACPI case, I still think that an AML call from the AHCI driver
> > is the most logical solution. You mentioned that you believe that calling
> > into the AML interpreter up to 100 times per second is a noticeable
> > overhead, but I doubt that and would like to see actual number backing
> > that up. Note that most of the time, the status of the LEDs won't even
> > change, so the driver does not have to call into the AML while I/O
> > is in progress, or while it is stopped, only for the transition or in
> > case of locate and fault events that should be extremely rare.
> >
> During disk activity ahci_sw_activity_blink() is called based on timer expiration (~100ms).
> I just enabled the function profiler for 'dd if=/dev/zero of=/root/tmp bs=1M count=4096' and see the output below. The function was called 37 times in 2.5s
>  
> #echo 1 > function_profile_enabled
> 
> #dd if=/dev/zero of=/root/tempfile bs=1M count=4096
> 4096+0 records in
> 4096+0 records out
> 4294967296 bytes (4.3 GB) copied, 2.57334 s, 1.7 GB/s
> 
> #echo 0 > function_profile_enabled
> 
> #cat trace_stat/*
> Function                               Hit    Time            Avg             s^2
> --------                               ---    ----            ---             ---
> seattle_transmit_led_message            37    25.088 us       0.678 us        0.050 us    
> 
> I am not debating on your AML call recommendation, it sounds like a good idea however BIOS is already released hence its bit late to add AML methods for this. I am seeking guidance on what can be done in the given situation. I thought platform driver is one option to get this feature enabled in kernel.

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).

	Arnd

[toc] | [prev] | [next] | [standalone]

#1324109

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.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1327822

On Tuesday 02 February 2016 12:37:58 Brijesh Singh wrote:
> 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 don't have access to the Windows source code. Is this in the
architecture-independent part of their kernel, or only done on ARM64?
How do they decide what the second memory range is for?

If this is now a de-facto extension to the PCI_CLASS_STORAGE_SATA_AHCI binding,
it should probably be put into the next version of the AHCI spec, and then
there is no problem using it.


	Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web