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

Groups > linux.kernel > #1303925 > unrolled thread

Re: [PATCH] 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] ata: add AMD Seattle platform driver Arnd Bergmann <arnd@arndb.de> - 2016-01-07 22:30 +0100
    Re: [PATCH] ata: add AMD Seattle platform driver Rob Herring <robh@kernel.org> - 2016-01-08 00:00 +0100
    Re: [PATCH] ata: add AMD Seattle platform driver Arnd Bergmann <arnd@arndb.de> - 2016-01-08 00:50 +0100
      Re: [PATCH] ata: add AMD Seattle platform driver Arnd Bergmann <arnd@arndb.de> - 2016-01-08 09:50 +0100
        Re: [PATCH] ata: add AMD Seattle platform driver Arnd Bergmann <arnd@arndb.de> - 2016-01-08 23:50 +0100
          Re: [PATCH] ata: add AMD Seattle platform driver Arnd Bergmann <arnd@arndb.de> - 2016-01-12 15:30 +0100
            Re: [PATCH] ata: add AMD Seattle platform driver Arnd Bergmann <arnd@arndb.de> - 2016-01-13 21:50 +0100
        Re: [PATCH] ata: add AMD Seattle platform driver Mark Langsdorf <mlangsdo@redhat.com> - 2016-01-11 16:40 +0100

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

On Thursday 07 January 2016 14:53:22 Brijesh Singh wrote:
> AMD Seattle SATA controller mostly conforms to AHCI interface with some
> special register to control SGPIO interface. In the case of an AHCI
> controller, the SGPIO feature is ideally implemented using the
> "Enclosure Management" register of the AHCI controller, but those
> registeres are not implemented in the Seattle SoC. Instead SoC
> (Rev B0 onwards) provides a 32-bit SGPIO control register which should
> be programmed to control the activity, locate and fault LEDs.
> 
> The driver is based on ahci_platform driver.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> CC: robh+dt@kernel.org
> CC: pawel.moll@arm.com
> CC: mark.rutland@arm.com
> CC: ijc+devicetree@hellion.org.uk
> CC: galak@codeaurora.org
> CC: tj@kernel.org
> CC: devicetree@vger.kernel.org
> CC: linux-ide@vger.kernel.org
> ---
>  .../devicetree/bindings/ata/sata-seattle.txt       |  34 ++++
>  drivers/ata/Kconfig                                |   8 +
>  drivers/ata/Makefile                               |   1 +
>  drivers/ata/ahci_seattle.c                         | 226 +++++++++++++++++++++
>  4 files changed, 269 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ata/sata-seattle.txt
>  create mode 100644 drivers/ata/ahci_seattle.c
> 
> diff --git a/Documentation/devicetree/bindings/ata/sata-seattle.txt b/Documentation/devicetree/bindings/ata/sata-seattle.txt
> new file mode 100644
> index 0000000..5ad46b7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/sata-seattle.txt
> @@ -0,0 +1,34 @@
> +* AHCI SATA Controller
> +
> +SATA nodes are defined to describe on-chip Serial ATA controllers.
> +The AMD Seattle SATA controller mostly conforms to the AHCI interface
> +with some special SGPIO register to contro activity LED interfaces.
> +
> +In the case of an AHCI controller, the SGPIO feature is ideally implemented
> +using the "Enclosure Management" register of the AHCI controller, but
> +those registeres are not implemented in the Seattle SoC.
> +
> +Each SATA controller should have its own node.
> +
> +
> +Required properties:
> +- compatible        : compatible string should be "amd,seattle-ahci".
> +- interrupts        : <interrupt mapping for SATA IRQ>
> +- reg               : <registers mapping>
> +
> +Optional properties:
> +- dma-coherent      : Present if dma operations are coherent
> +- clocks            : a list of phandle + clock specifier pairs
> +- target-supply     : regulator for SATA target power
> +- phys              : reference to the SATA PHY node
> +- phy-names         : must be "sata-phy"
> +
> +Examples:
> +        sata0@e0300000 {
> +		compatible = "amd,seattle-ahci";
> +		reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;

Looking at the register values, I doubt that the SGPIO is actually part of the
sata device. More likely, you are pointing in the middle of an actual
GPIO controller.

If so, please implement a GPIO driver for that device, and use the gpio-leds
driver to drive the LEDs. IIRC there is already a generic way to communicate
with the LEDs interface from libata, if not you can implement that in order
to keep the special case out of the platform driver.

> +		interrupts = <0x0 0x163 0x4>;
> +		clocks = <0x2>

This is not a valid property.

> +/* SGPIO Control Register definition
> + *
> + * Bit		Type		Description
> + * 31		RW		OD7.2 (activity)
> + * 30		RW		OD7.1 (locate)
> + * 29		RW		OD7.0 (fault)
> + * 28...8	RW		OD6.2...OD0.0 (3bits per port, 1 bit per LED)
> + * 7		RO		SGPIO feature flag
> + * 6:4		RO		Reserved
> + * 3:0		RO		Number of ports (0 means no port supported)
> + */

The 'reg' property in your example is only 8 bits wide, the above lists
32 bits.

	Arnd

[toc] | [next] | [standalone]

#1303974

On Thu, Jan 07, 2016 at 10:25:38PM +0100, Arnd Bergmann wrote:
> On Thursday 07 January 2016 14:53:22 Brijesh Singh wrote:
> > AMD Seattle SATA controller mostly conforms to AHCI interface with some
> > special register to control SGPIO interface. In the case of an AHCI
> > controller, the SGPIO feature is ideally implemented using the
> > "Enclosure Management" register of the AHCI controller, but those
> > registeres are not implemented in the Seattle SoC. Instead SoC
> > (Rev B0 onwards) provides a 32-bit SGPIO control register which should
> > be programmed to control the activity, locate and fault LEDs.

> > +Examples:
> > +        sata0@e0300000 {
> > +		compatible = "amd,seattle-ahci";
> > +		reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
> 
> Looking at the register values, I doubt that the SGPIO is actually part of the
> sata device. More likely, you are pointing in the middle of an actual
> GPIO controller.

SGPIO is really a poor name as it has little to do with GPIO. It's a 
serial protocol to set LED states. Still, I agree this should probably 
be a separate node with perhaps a phandle to syscon.
 
> If so, please implement a GPIO driver for that device, and use the gpio-leds
> driver to drive the LEDs. IIRC there is already a generic way to communicate
> with the LEDs interface from libata, if not you can implement that in order
> to keep the special case out of the platform driver.

There is kernel support for activity LEDs, but the others you want to 
control with ledmon/ledctl utilities rather than LED subsystem. Those 
utilities use an enclosure management sysfs file IIRC. There's no 
kernel support for SGPIO outside of the AHCI enclosure management 
register (at least there wasn't 2 years ago when I last looked). 

Rob

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

#1304023

On Thursday 07 January 2016 16:24:22 Brijesh Singh wrote:
> >> +
> >> +Examples:
> >> +        sata0@e0300000 {
> >> +            compatible = "amd,seattle-ahci";
> >> +            reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
> > 
> > Looking at the register values, I doubt that the SGPIO is actually part of the
> > sata device. More likely, you are pointing in the middle of an actual
> > GPIO controller.
> > 
> 
> That address is SGPIO control register for SATA. The current hardware implementation to control activity LED is not ideal.

Of course its a control register "for" SATA, what I meant is that it's
not part "of" the SATA IP block, which is hopefully a standard AHCI
compliant part as required by SBSA.

> A57 does not have access to GPIO's connected to backplane controller
> instead SoC has exposed two SGPIO control registers (LSIOC_SGPIO_CONTROL0:
> 0xE000_0078 and LSIOC_SGPIO_CONTROL1: 0xE000_007C) to A57. All we 
> need to do is to program these registers based on the disk activity.
> The firmware running on A5 reads the values and generate proper SGPIO
> timing and toggles the LEDs etc.

It still sounds like SGPIO is not part of the AHCI standard spec, but
rather a subset of a device called LSIOC.

> These registers are defined in SATA0/1 DSDT resource template and also
> documented in SoC BKDG. I just noticed that BKDG has wrong register
> definition so will ask documentation folks to fix that.
> 
> This driver is using SGPIO LED control similar to sata_highbank [1]
> except bit bang GPIO (which is done by firmware).
> 
> [1] http://lxr.free-electrons.com/source/drivers/ata/sata_highbank.c#L140

This one is rather different: there is a single device that combines
registers for AHCI, the PHY attached to it and the LED. This is not
SBSA compliant of course, and it requires having a special driver.

What you have instead looks like a regular AHCI implementation that
should just work with the standard driver as long as you describe how
it gets its LEDs.

	Arnd

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

#1304276

On Thursday 07 January 2016 19:46:08 Brijesh Singh wrote:
> Hi,
> 
> On 01/07/2016 05:42 PM, Arnd Bergmann wrote:
> > On Thursday 07 January 2016 16:24:22 Brijesh Singh wrote:
> >>>> +
> >>>> +Examples:
> >>>> +        sata0@e0300000 {
> >>>> +            compatible = "amd,seattle-ahci";
> >>>> +            reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
> >>>
> >>> Looking at the register values, I doubt that the SGPIO is actually part of the
> >>> sata device. More likely, you are pointing in the middle of an actual
> >>> GPIO controller.
> >>>
> >>
> >> That address is SGPIO control register for SATA. The current hardware implementation to control activity LED is not ideal.
> >
> > Of course its a control register "for" SATA, what I meant is that it's
> > not part "of" the SATA IP block, which is hopefully a standard AHCI
> > compliant part as required by SBSA.
> >
> Yes, its not part of SATA IP block. We just need a method of pass  SGPIO 
> control register address to driver.
> 
> Should I consider adding a property "sgpio-ctrl" to pass the register 
> address ?
> 
> e.g
> 
> sata0@e0300000 {
>    compatible = "amd,seattle-ahci";
>    reg = <0 0xe0300000 0 0x800>;
>    amd,sgpio-ctrl = <0xe0000078>;
>    interrupts = <0 355 4>;
>    clocks = <&sataclk_333mhz>;
>    dma-coherent;
> };

We generally don't refer to register locations with properties other than
'reg', so that approach would be worse. What I'd suggest you do is to
have the sgpio registers in a separate device node, and use the LED
binding to access it, see
 
Documentation/devicetree/bindings/leds/common.txt

It seems that none of the drivers/ata/ drivers use the leds interface
today, but that can be added to libata-*.c whenever the appropriate
properties are there.

> > This one is rather different: there is a single device that combines
> > registers for AHCI, the PHY attached to it and the LED. This is not
> > SBSA compliant of course, and it requires having a special driver.
> >
> > What you have instead looks like a regular AHCI implementation that
> > should just work with the standard driver as long as you describe how
> > it gets its LEDs.
> >
> Yes, its regular AHCI implementation and works well with ahci_platform 
> driver. In standard ahci_platform driver activity LEDs are blinked 
> through enclosure management interface. Given the current hardware 
> limitation it seems like creating a new driver would be cleaner. I am 
> open to suggestion.

I'd say the code in drivers/ata should be kept completely generic, referring
only to the include/linux/leds.h interfaces and properties added to
Documentation/devicetree/bindings/ata/ahci-platform.txt (if any).

For the driver that actually owns the register, it depends a bit on how
the hardware is structured, and you'd need to look at the datasheet (or
talk to a hardware designer) for that.

I suspect we should have a node for the entire block of registers
around the SGPIO, presumably something like

	syscon@0xe0000000 {
		compatible = "amd,$SOC_ID"-lsioc", "syscon";
		reg = <0xe0000000 0x1000>; /* find the actual length in the datasheet */
	};

That way, any driver that ends up needing a register from this block
can use the syscon interface to get hold of a mapping, and/or you can
have a high-level driver to expose other functionalities. It's probably
best to start doing it either entirely using syscon references from
other drivers, or using no syscon references, and putting everything into
a driver for the register set, but as I said it depends a lot on what
else is in there.

Can you send me a register list of the 0xe0000000 segment for reference?


	Arnd

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

#1305046

On Friday 08 January 2016 16:21:50 Brijesh Singh wrote:
> >> Should I consider adding a property "sgpio-ctrl" to pass the register 
> >> address ?
> >>
> >> e.g
> >>
> >> sata0@e0300000 {
> >>    compatible = "amd,seattle-ahci";
> >>    reg = <0 0xe0300000 0 0x800>;
> >>    amd,sgpio-ctrl = <0xe0000078>;
> >>    interrupts = <0 355 4>;
> >>    clocks = <&sataclk_333mhz>;
> >>    dma-coherent;
> >> };
> > 
> > We generally don't refer to register locations with properties other than
> > 'reg', so that approach would be worse. What I'd suggest you do is to
> > have the sgpio registers in a separate device node, and use the LED
> > binding to access it, see
> >  
> > Documentation/devicetree/bindings/leds/common.txt
> > 
> > It seems that none of the drivers/ata/ drivers use the leds interface
> > today, but that can be added to libata-*.c whenever the appropriate
> > properties are there.
> >
> 
> libata-*.c implements the "Enclosure management" style led messages but also has hooks
> to register a custom led control callback. Since Seattle platform does not support
> the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY 
> to indicate that we can still handle the led messages by our registered callback. I see
> that sata_highbank driver is doing something similar.

But if the LEDs are the only thing that is special, I think it makes
more sense to extend the generic driver. This is similar to how the
ahci driver knows how to deal with external PHY, clk, regulator etc
resources. All of those are not part of the AHCI spec, but are common
enough that we want to have them done in a single driver.

We really should not need a special driver just for handling LEDs
when we already deal with more complex stuff, and we have a proper
abstraction for LEDs in the kernel.

> > 
> > That way, any driver that ends up needing a register from this block
> > can use the syscon interface to get hold of a mapping, and/or you can
> > have a high-level driver to expose other functionalities. It's probably
> > best to start doing it either entirely using syscon references from
> > other drivers, or using no syscon references, and putting everything into
> > a driver for the register set, but as I said it depends a lot on what
> > else is in there.
> > 
> > Can you send me a register list of the 0xe0000000 segment for reference?
> >
> Thanks for pointing syncon, are you thinking something like this ?
> 
>                 sgpio0: syscon@e0000078 {
>                         compatible = "amd, seattle-sgpio", syscon";
>                         reg = <0x0 0xe0000078 0x0 0x1>;
>                 };

A syscon is defined as (roughly) a group of otherwise unrelated registers
that happen to be part of the same physical register area because the SoC
designer couldn't find a proper abstraction for them. When you define
a register area with a single byte in it, that is not a syscon.

> 
>                 sata@e0300000 {
>                         compatible = "amd,seattle-ahci";
>                         reg = <0x0 0xe0300000 0x0 0xf0000>;
>                         interrupts = <0x0 0x163 0x4>;
>                         amd,sgpio-ctrl = <&sgpio0>;
>                         dma-coherent;
>                 };
> 


The sata node should list "generic-ahci" so we can bind the normal
driver. You can leave the "amd,seattle-ahci" in addition in case we
ever need to know the difference to work around a bug, but it's really
not needed for the LED.

> SGPIO0 (0xe0000078) and SGPIO1 (0xe000007C) does not belong to any system control register set.
> They are meant to be used by SATA driver only and no other driver should every touch it. It almost feels
> its just extension of the IP block but it just happened to be way out of the IP block range.
> Other register near to 0xe000_0078 and 0xe000_007C should not be mapped by OS.

That is quite normal, a lot of chips have register blocks where one
register is meant for one device only. E.g. the clock controller may have
one register for controlling the clocks of the AHCI device. In the old
days, we would have hacks like what you did to turn on the clocks by poking
the register from the SATA driver, but now we abstract those things using
generic subsystems.

I think you either want a special led controller device node that
refers to the syscon device and exports the LEDs so they can be
accessed by the AHCI device, or do it in the device that contains
the registers.

> But this syscon approach also brings another problem. Currently my code is pretty simple for mapping this regs
> 
> +       plat_data->sgpio_ctrl = devm_ioremap_resource(dev,
> +                             platform_get_resource(pdev, IORESOURCE_MEM, 1));
> +       if (IS_ERR(plat_data->sgpio_ctrl))
> +               return &ahci_port_info;
> +
> 
> The above code works on ACPI and DT cases. But if we go with syncon approach
> then we need to handle DT and ACPI differently. Because sycon will provide
> struct regmap instead of struct resources and also make reading/writing a 
> bit different. I was trying to minimize the DT vs ACPI changes in the driver
> (other than binding) hence I think defining two ranges in sata controller
> reg property was much cleaner and it aligns with DSDT.

Isn't this the thing that ACPI based firmware would handle using AML anyway?
I don't think the server people would be too happy to add a new driver
each time a SATA controller does the LEDs slightly differently, and this
is really the kind of platform specific hack that AML is meant for.

	Arnd

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

#1307476

On Monday 11 January 2016 12:56:02 Brijesh Singh wrote:
> Hi Arnd,
> 
> Thanks for all your valuable feedbacks.
> 
> On 01/08/2016 04:47 PM, Arnd Bergmann wrote:
> >>
> >> libata-*.c implements the "Enclosure management" style led messages but also has hooks
> >> to register a custom led control callback. Since Seattle platform does not support
> >> the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY 
> >> to indicate that we can still handle the led messages by our registered callback. I see
> >> that sata_highbank driver is doing something similar.
> > 
> > But if the LEDs are the only thing that is special, I think it makes
> > more sense to extend the generic driver. This is similar to how the
> > ahci driver knows how to deal with external PHY, clk, regulator etc
> > resources. All of those are not part of the AHCI spec, but are common
> > enough that we want to have them done in a single driver.
> > 
> > We really should not need a special driver just for handling LEDs
> > when we already deal with more complex stuff, and we have a proper
> > abstraction for LEDs in the kernel.
> > 
> 
> I took a quick look at LED class and saw ledtrig-ide-disk.c (ledtrig_ide_activity). It seems this approach was used
> in deprecate ide-disk driver for blinking the activity led. Are you thinking that we implement something similar
> in libahci to control the LED blinking? 

Yes. I'm not overly familiar with the LED framework, but it seems to me that
something along those lines is the right idea. Note that the driver predates
devicetree and it only handles a single LED, so you probably need to do
something a bit more sophisticated to handle each LED separately and connect
it do the right disk through DT.

We actually seem to have quite a number of platforms relying on this:

$ git grep -w "ide-disk"
Documentation/devicetree/bindings/leds/common.txt:     "ide-disk" - LED indicates disk activity
Documentation/devicetree/bindings/leds/leds-gpio.txt:           linux,default-trigger = "ide-disk";
arch/arm/boot/dts/am57xx-beagle-x15.dts:                        linux,default-trigger = "ide-disk";
arch/arm/boot/dts/kirkwood-ns2lite.dts:                 linux,default-trigger = "ide-disk";
arch/arm/boot/dts/kirkwood-topkick.dts:                 linux,default-trigger = "ide-disk";
arch/arm/mach-davinci/board-dm644x-evm.c:               .default_trigger = "ide-disk", },
arch/arm/mach-omap1/board-osk.c:                        .default_trigger = "ide-disk", },
arch/arm/mach-pxa/spitz.c:              .default_trigger        = "ide-disk",
arch/mips/txx9/generic/setup.c:         "ide-disk",
arch/mips/txx9/rbtx4939/setup.c:                "ide-disk",
arch/powerpc/boot/dts/mpc8315erdb.dts:                  linux,default-trigger = "ide-disk";
arch/powerpc/boot/dts/mpc8377_rdb.dts:                  linux,default-trigger = "ide-disk";
arch/powerpc/boot/dts/mpc8378_rdb.dts:                  linux,default-trigger = "ide-disk";
arch/powerpc/boot/dts/mpc8379_rdb.dts:                  linux,default-trigger = "ide-disk";
arch/unicore32/kernel/gpio.c:           .default_trigger = "ide-disk", },
drivers/leds/leds-hp6xx.c:      .default_trigger        = "ide-disk",
drivers/leds/trigger/Makefile:obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK)       += ledtrig-ide-disk.o
drivers/leds/trigger/ledtrig-camera.c: * based on ledtrig-ide-disk.c
drivers/leds/trigger/ledtrig-ide-disk.c:        led_trigger_register_simple("ide-disk", &ledtrig_ide);
drivers/macintosh/Kconfig:        and the ide-disk LED trigger and configure appropriately through
drivers/macintosh/via-pmu-led.c:        .default_trigger = "ide-disk",

I suspect this used to work in the past, but got broken when people moved
away from drivers/ide to drivers/ata, and nobody properly debugged the problem.

If you fix this right, the LEDs on all those platforms should light up again.

> Looking at current libahci gives me feeling that LED's are considered as
>  part of AHCI enclosure management implementation and hence LED triggers
> are not exposed outside the library. 
> If I am missing something then please correct me.

Yes, this sounds correct.
  
> > A syscon is defined as (roughly) a group of otherwise unrelated registers
> > that happen to be part of the same physical register area because the SoC
> > designer couldn't find a proper abstraction for them. When you define
> > a register area with a single byte in it, that is not a syscon.
> > 
> >>
> >>                 sata@e0300000 {
> >>                         compatible = "amd,seattle-ahci";
> >>                         reg = <0x0 0xe0300000 0x0 0xf0000>;
> >>                         interrupts = <0x0 0x163 0x4>;
> >>                         amd,sgpio-ctrl = <&sgpio0>;
> >>                         dma-coherent;
> >>                 };
> >>
> > 
> > 
> > The sata node should list "generic-ahci" so we can bind the normal
> > driver. You can leave the "amd,seattle-ahci" in addition in case we
> > ever need to know the difference to work around a bug, but it's really
> > not needed for the LED.
> > 
> >> SGPIO0 (0xe0000078) and SGPIO1 (0xe000007C) does not belong to any system control register set.
> >> They are meant to be used by SATA driver only and no other driver should every touch it. It almost feels
> >> its just extension of the IP block but it just happened to be way out of the IP block range.
> >> Other register near to 0xe000_0078 and 0xe000_007C should not be mapped by OS.
> > 
> > That is quite normal, a lot of chips have register blocks where one
> > register is meant for one device only. E.g. the clock controller may have
> > one register for controlling the clocks of the AHCI device. In the old
> > days, we would have hacks like what you did to turn on the clocks by poking
> > the register from the SATA driver, but now we abstract those things using
> > generic subsystems.
> > 
> > I think you either want a special led controller device node that
> > refers to the syscon device and exports the LEDs so they can be
> > accessed by the AHCI device, or do it in the device that contains
> > the registers.
> > 
> >> But this syscon approach also brings another problem. Currently my code is pretty simple for mapping this regs
> >>
> >> +       plat_data->sgpio_ctrl = devm_ioremap_resource(dev,
> >> +                             platform_get_resource(pdev, IORESOURCE_MEM, 1));
> >> +       if (IS_ERR(plat_data->sgpio_ctrl))
> >> +               return &ahci_port_info;
> >> +
> >>
> >> The above code works on ACPI and DT cases. But if we go with syncon approach
> >> then we need to handle DT and ACPI differently. Because sycon will provide
> >> struct regmap instead of struct resources and also make reading/writing a 
> >> bit different. I was trying to minimize the DT vs ACPI changes in the driver
> >> (other than binding) hence I think defining two ranges in sata controller
> >> reg property was much cleaner and it aligns with DSDT.
> > 
> > Isn't this the thing that ACPI based firmware would handle using AML anyway?
> > I don't think the server people would be too happy to add a new driver
> > each time a SATA controller does the LEDs slightly differently, and this
> > is really the kind of platform specific hack that AML is meant for.
> > 
> Sorry I am not able understand your comment, Could you please explain me what you mean by AML is meant for this kind of platform specific hack ?
>

I meant the sgpio register should not be exposed through a resource on
an ACPI based system but the access be hidden behind a call into an AML
method.

You basically extend the generic AHCI driver to understand three ways of
blinking the LEDS:

a) standard AHCI enclosure management
b) the Linux LED subsystem using whatever LED implementation the platform provides
c) calling into the ACPI interpreter to do platform specific hacks

	Arnd

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

#1308802

On Wednesday 13 January 2016 10:55:04 Brijesh Singh wrote:
> On 01/12/2016 08:24 AM, Arnd Bergmann wrote:

> > c) calling into the ACPI interpreter to do platform specific hacks
> 
> Thanks for explaining. Now I understood your comment on AML however we need to consider the following:
> 
> a) activity LED blinking routines are called very frequent (10 to 100ms based on emp->state) and executing AML method that often would introduces its own overhead.
> 
> b) all BIOS vendors need to implement a new methods in their DSDT and release a new BIOS.
> 
> c) other OS'es (mainly Windows) driver need to be updated.
> 
> 
> We don't know how many other SoC's have similar hacky implementation which can take advantage of extending generic
> AHCI driver to call into AML methods for LED blink. Given some of these causes I think having a platform driver is much cleaner. I can drop DT binding part from this driver and keep just ACPI binding.
> 
> Thoughts ?

I don't care what you do with ACPI, just make sure we can use the generic
driver without any platform specific hacks for the DT case.

For ACPI, please talk to the respective maintainers about whether they want
to have a custom driver for this case, or a proper abstraction in the generic
driver.

	Arnd

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

#1306418

On 01/08/2016 04:21 PM, Brijesh Singh wrote:
> Hi,

>> We generally don't refer to register locations with properties other than
>> 'reg', so that approach would be worse. What I'd suggest you do is to
>> have the sgpio registers in a separate device node, and use the LED
>> binding to access it, see
>>
>> Documentation/devicetree/bindings/leds/common.txt
>>
>> It seems that none of the drivers/ata/ drivers use the leds interface
>> today, but that can be added to libata-*.c whenever the appropriate
>> properties are there.
>>
>
> libata-*.c implements the "Enclosure management" style led messages but also has hooks
> to register a custom led control callback. Since Seattle platform does not support
> the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
> to indicate that we can still handle the led messages by our registered callback. I see
> that sata_highbank driver is doing something similar.

The sata_highbank driver is doing it wrong and shouldn't have been
accepted in its current condition. Enclosure management really should
be a separate device. Please don't use it as an example.

--Mark Langsdorf

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web