| Message ID | 1452200002-31590-1-git-send-email-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
Hi, >> + >> +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. 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. 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 > 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 >
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
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
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; }; >> 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. > 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. Thanks for all your review feedbacks. > Arnd >
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
On Thu, 2016-01-07 at 14:53 -0600, 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. trivia: > diff --git a/drivers/ata/ahci_seattle.c b/drivers/ata/ahci_seattle.c [] > +static ssize_t seattle_transmit_led_message(struct ata_port *ap, u32 state, > + ssize_t size) > +{ > + struct ahci_host_priv *hpriv = ap->host->private_data; > + struct ahci_port_priv *pp = ap->private_data; > + struct seattle_plat_data *plat_data = hpriv->plat_data; > + unsigned long flags; > + int pmp; > + struct ahci_em_priv *emp; > + u32 val; > + > + /* get the slot number from the message */ > + pmp = (state & EM_MSG_LED_PMP_SLOT) >> 8; > + if (pmp < EM_MAX_SLOTS) > + emp = &pp->em_priv[pmp]; > + else > + return -EINVAL; This is generally done using a inverted test that returns immediately if (pmp >= EMP_MAX_SLOTS) return -EINVAL; emp = &pp->em_priv[pmp];
Hi, >> 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. >> 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? > Thanks for pointing syncon, are you thinking something like this ? sgpio0: syscon@e0000078 { compatible = "amd, seattle-sgpio", syscon"; reg = <0x0 0xe0000078 0x0 0x1>; }; sata@e0300000 { compatible = "amd,seattle-ahci"; reg = <0x0 0xe0300000 0x0 0xf0000>; interrupts = <0x0 0x163 0x4>; amd,sgpio-ctrl = <&sgpio0>; dma-coherent; }; 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. 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. Just for reference the SATA DSDT looks like this: Device (SATA0) { ..... Name(_CRS, ResourceTemplate() { Memory32Fixed(ReadWrite, 0xE03000000, 0x000010000) Interrupt(ResourceConsumer, Level, ActiveHigh Exclusive,,,) { 387} Memory32Fixed(ReadWrite, 0xE00000078, 1) } ...... } > > Arnd >
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
Hi, On 07-01-16 21:53, 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> This driver looks good from a libahci_platform usage pov. So with the 2 remarks from other reviewers fixed this patch is: Acked-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > 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>; > + interrupts = <0x0 0x163 0x4>; > + clocks = <0x2> > + dma-coherent; > + }; > + > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig > index 6aaa3f8..56f764d 100644 > --- a/drivers/ata/Kconfig > +++ b/drivers/ata/Kconfig > @@ -193,6 +193,14 @@ config SATA_FSL > > If unsure, say N. > > +config SATA_AHCI_SEATTLE > + tristate "AMD Seattle 6.0Gbps AHCI SATA host controller support" > + depends on ARCH_SEATTLE > + help > + This option enables support for AMD Seattle SATA host controller. > + > + If unsure, say N > + > config SATA_INIC162X > tristate "Initio 162x SATA support (Very Experimental)" > depends on PCI > diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile > index af45eff..9c0a96a 100644 > --- a/drivers/ata/Makefile > +++ b/drivers/ata/Makefile > @@ -4,6 +4,7 @@ obj-$(CONFIG_ATA) += libata.o > # non-SFF interface > obj-$(CONFIG_SATA_AHCI) += ahci.o libahci.o > obj-$(CONFIG_SATA_ACARD_AHCI) += acard-ahci.o libahci.o > +obj-$(CONFIG_SATA_AHCI_SEATTLE) += ahci_seattle.o libahci.o libahci_platform.o > obj-$(CONFIG_SATA_AHCI_PLATFORM) += ahci_platform.o libahci.o libahci_platform.o > obj-$(CONFIG_SATA_FSL) += sata_fsl.o > obj-$(CONFIG_SATA_INIC162X) += sata_inic162x.o > diff --git a/drivers/ata/ahci_seattle.c b/drivers/ata/ahci_seattle.c > new file mode 100644 > index 0000000..8c9ab67 > --- /dev/null > +++ b/drivers/ata/ahci_seattle.c > @@ -0,0 +1,226 @@ > +/* > + * AMD Seattle AHCI SATA driver > + * > + * Copyright (c) 2015, Advanced Micro Devices > + * Author: Brijesh Singh <brijesh.singh@amd.com> > + * > + * based on the AHCI SATA platform driver by Jeff Garzik and Anton Vorontsov > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/pm.h> > +#include <linux/device.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/libata.h> > +#include <linux/ahci_platform.h> > +#include <linux/acpi.h> > +#include <linux/pci_ids.h> > +#include "ahci.h" > + > +/* 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) > + */ > +#define ACTIVITY_BIT_POS(x) (8 + (3 * x)) > +#define LOCATE_BIT_POS(x) (ACTIVITY_BIT_POS(x) + 1) > +#define FAULT_BIT_POS(x) (LOCATE_BIT_POS(x) + 1) > + > +#define ACTIVITY_MASK 0x00010000 > +#define LOCATE_MASK 0x00080000 > +#define FAULT_MASK 0x00400000 > + > +#define DRV_NAME "ahci-seattle" > + > +static ssize_t seattle_transmit_led_message(struct ata_port *ap, u32 state, > + ssize_t size); > + > +struct seattle_plat_data { > + void __iomem *sgpio_ctrl; > +}; > + > +static struct ata_port_operations ahci_port_ops = { > + .inherits = &ahci_ops, > +}; > + > +static const struct ata_port_info ahci_port_info = { > + .flags = AHCI_FLAG_COMMON, > + .pio_mask = ATA_PIO4, > + .udma_mask = ATA_UDMA6, > + .port_ops = &ahci_port_ops, > +}; > + > +static struct ata_port_operations ahci_seattle_ops = { > + .inherits = &ahci_ops, > + .transmit_led_message = seattle_transmit_led_message, > +}; > + > +static const struct ata_port_info ahci_port_seattle_info = { > + .flags = AHCI_FLAG_COMMON | ATA_FLAG_EM | ATA_FLAG_SW_ACTIVITY, > + .link_flags = ATA_LFLAG_SW_ACTIVITY, > + .pio_mask = ATA_PIO4, > + .udma_mask = ATA_UDMA6, > + .port_ops = &ahci_seattle_ops, > +}; > + > +static struct scsi_host_template ahci_platform_sht = { > + AHCI_SHT(DRV_NAME), > +}; > + > +static ssize_t seattle_transmit_led_message(struct ata_port *ap, u32 state, > + ssize_t size) > +{ > + struct ahci_host_priv *hpriv = ap->host->private_data; > + struct ahci_port_priv *pp = ap->private_data; > + struct seattle_plat_data *plat_data = hpriv->plat_data; > + unsigned long flags; > + int pmp; > + struct ahci_em_priv *emp; > + u32 val; > + > + /* get the slot number from the message */ > + pmp = (state & EM_MSG_LED_PMP_SLOT) >> 8; > + if (pmp < EM_MAX_SLOTS) > + emp = &pp->em_priv[pmp]; > + else > + return -EINVAL; > + > + val = ioread32(plat_data->sgpio_ctrl); > + if (state & ACTIVITY_MASK) > + val |= 1 << ACTIVITY_BIT_POS((ap->port_no)); > + else > + val &= ~(1 << ACTIVITY_BIT_POS((ap->port_no))); > + > + if (state & LOCATE_MASK) > + val |= 1 << LOCATE_BIT_POS((ap->port_no)); > + else > + val &= ~(1 << LOCATE_BIT_POS((ap->port_no))); > + > + if (state & FAULT_MASK) > + val |= 1 << FAULT_BIT_POS((ap->port_no)); > + else > + val &= ~(1 << FAULT_BIT_POS((ap->port_no))); > + > + iowrite32(val, plat_data->sgpio_ctrl); > + > + spin_lock_irqsave(ap->lock, flags); > + > + /* save off new led state for port/slot */ > + emp->led_state = state; > + > + spin_unlock_irqrestore(ap->lock, flags); > + > + return size; > +} > + > +static const struct ata_port_info *ahci_seattle_get_port_info( > + struct platform_device *pdev, struct ahci_host_priv *hpriv) > +{ > + struct device *dev = &pdev->dev; > + struct seattle_plat_data *plat_data; > + u32 val; > + > + plat_data = devm_kzalloc(dev, sizeof(*plat_data), GFP_KERNEL); > + if (IS_ERR(plat_data)) > + return &ahci_port_info; > + > + 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; > + > + val = ioread32(plat_data->sgpio_ctrl); > + > + if (!(val & 0xf)) > + return &ahci_port_info; > + > + hpriv->em_loc = 0; > + hpriv->em_buf_sz = 4; > + hpriv->em_msg_type = EM_MSG_TYPE_LED; > + hpriv->plat_data = plat_data; > + > + dev_info(dev, "SGPIO LED control is enabled.\n"); > + return &ahci_port_seattle_info; > +} > + > +static int ahci_seattle_probe(struct platform_device *pdev) > +{ > + int rc; > + struct ahci_host_priv *hpriv; > + > + hpriv = ahci_platform_get_resources(pdev); > + if (IS_ERR(hpriv)) > + return PTR_ERR(hpriv); > + > + rc = ahci_platform_enable_resources(hpriv); > + if (rc) > + return rc; > + > + rc = ahci_platform_init_host(pdev, hpriv, > + ahci_seattle_get_port_info(pdev, hpriv), > + &ahci_platform_sht); > + if (rc) > + goto disable_resources; > + > + return 0; > +disable_resources: > + ahci_platform_disable_resources(hpriv); > + return rc; > +} > + > +static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_platform_suspend, > + ahci_platform_resume); > + > +#ifdef CONFIG_OF > +static const struct of_device_id ahci_of_match[] = { > + { .compatible = "amd,seattle-ahci", }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, ahci_of_match); > +#endif > + > +#ifdef CONFIG_ACPI > +static const struct acpi_device_id ahci_acpi_match[] = { > + { "AMDI0600", 0 }, > + {} > +}; > +MODULE_DEVICE_TABLE(acpi, ahci_acpi_match); > +#endif > + > +static struct platform_driver ahci_seattle_driver = { > + .probe = ahci_seattle_probe, > + .remove = ata_platform_remove_one, > + .driver = { > + .name = DRV_NAME, > +#ifdef CONFIG_OF > + .of_match_table = ahci_of_match, > +#endif > +#ifdef CONFIG_ACPI > + .acpi_match_table = ahci_acpi_match, > +#endif > + .pm = &ahci_pm_ops, > + }, > +}; > +module_platform_driver(ahci_seattle_driver); > + > +MODULE_DESCRIPTION("Seattle AHCI SATA platform driver"); > +MODULE_AUTHOR("Brijesh Singh <brijesh.singh@amd.com>"); > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("platform:" DRV_NAME); >
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
Hi, On 01/11/2016 09:33 AM, Mark Langsdorf wrote: > On 01/08/2016 04:21 PM, Brijesh Singh 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. > > 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. > Okay. Thanks for letting me know. Should I consider extending the generic driver to handle this Seattle platform specific feature or go with a new platform driver approach (i.e the one I am doing right now) ? -Brijesh
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? 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. > > 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 ? "SATA0" and "SATA1" DSDT Resource template contains two resources (#0 SATA block register and #1 SGPIO control register). Device (SATA0) { Name(_CRS, ResourceTemplate() { Memory32Fixed(ReadWrite, 0xE03000000, 0x000010000) Interrupt(ResourceConsumer, Level, ActiveHigh Exclusive,,,) { 387} Memory32Fixed(ReadWrite, 0xE00000078, 1) } } Device (SATA1) { Name(_CRS, ResourceTemplate() { Memory32Fixed(ReadWrite, 0xE0D100000, 0x000010000) Interrupt(ResourceConsumer, Level, ActiveHigh Exclusive,,,) { 393} Memory32Fixed(ReadWrite, 0xE0000007C, 1) } } The generic ahci platform driver calls ahci_platform_get_resources() to get the resources. ahci_platform_get_resources() maps the resource #0. Since the SGPIO control resource is part of SATA device resource hence I could simply ioremap resource #1 for both DT and ACPI cases. Since windows driver is making use of same DSDT hence its going to be tricky to modify the DSDT to create a new device entry for SATA LED. Before creating a new driver, I did attempt to extend the existing ahci_platform driver and was able to get it working but it was not looking good hence created a new driver. One of the issue was knowing when to map the SGPIO control register in ACPI case. ahci_platform driver does binding based on _CLS mask so I was not sure whether doing ioremap of resource#1 is safe and would not cause problem on non Seattle platform. If you want then i can send you patch for quick comment and see if that make sense instead of adding a new driver. > Arnd >
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
Hi, On 01/12/2016 08:24 AM, Arnd Bergmann wrote: >>> >>> 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 Yes I could look into exploring LED implementation for DT case. > 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 ? -Brijesh > > Arnd >
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
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>; + interrupts = <0x0 0x163 0x4>; + clocks = <0x2> + dma-coherent; + }; + diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig index 6aaa3f8..56f764d 100644 --- a/drivers/ata/Kconfig +++ b/drivers/ata/Kconfig @@ -193,6 +193,14 @@ config SATA_FSL If unsure, say N. +config SATA_AHCI_SEATTLE + tristate "AMD Seattle 6.0Gbps AHCI SATA host controller support" + depends on ARCH_SEATTLE + help + This option enables support for AMD Seattle SATA host controller. + + If unsure, say N + config SATA_INIC162X tristate "Initio 162x SATA support (Very Experimental)" depends on PCI diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile index af45eff..9c0a96a 100644 --- a/drivers/ata/Makefile +++ b/drivers/ata/Makefile @@ -4,6 +4,7 @@ obj-$(CONFIG_ATA) += libata.o # non-SFF interface obj-$(CONFIG_SATA_AHCI) += ahci.o libahci.o obj-$(CONFIG_SATA_ACARD_AHCI) += acard-ahci.o libahci.o +obj-$(CONFIG_SATA_AHCI_SEATTLE) += ahci_seattle.o libahci.o libahci_platform.o obj-$(CONFIG_SATA_AHCI_PLATFORM) += ahci_platform.o libahci.o libahci_platform.o obj-$(CONFIG_SATA_FSL) += sata_fsl.o obj-$(CONFIG_SATA_INIC162X) += sata_inic162x.o diff --git a/drivers/ata/ahci_seattle.c b/drivers/ata/ahci_seattle.c new file mode 100644 index 0000000..8c9ab67 --- /dev/null +++ b/drivers/ata/ahci_seattle.c @@ -0,0 +1,226 @@ +/* + * AMD Seattle AHCI SATA driver + * + * Copyright (c) 2015, Advanced Micro Devices + * Author: Brijesh Singh <brijesh.singh@amd.com> + * + * based on the AHCI SATA platform driver by Jeff Garzik and Anton Vorontsov + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/pm.h> +#include <linux/device.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/libata.h> +#include <linux/ahci_platform.h> +#include <linux/acpi.h> +#include <linux/pci_ids.h> +#include "ahci.h" + +/* 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) + */ +#define ACTIVITY_BIT_POS(x) (8 + (3 * x)) +#define LOCATE_BIT_POS(x) (ACTIVITY_BIT_POS(x) + 1) +#define FAULT_BIT_POS(x) (LOCATE_BIT_POS(x) + 1) + +#define ACTIVITY_MASK 0x00010000 +#define LOCATE_MASK 0x00080000 +#define FAULT_MASK 0x00400000 + +#define DRV_NAME "ahci-seattle" + +static ssize_t seattle_transmit_led_message(struct ata_port *ap, u32 state, + ssize_t size); + +struct seattle_plat_data { + void __iomem *sgpio_ctrl; +}; + +static struct ata_port_operations ahci_port_ops = { + .inherits = &ahci_ops, +}; + +static const struct ata_port_info ahci_port_info = { + .flags = AHCI_FLAG_COMMON, + .pio_mask = ATA_PIO4, + .udma_mask = ATA_UDMA6, + .port_ops = &ahci_port_ops, +}; + +static struct ata_port_operations ahci_seattle_ops = { + .inherits = &ahci_ops, + .transmit_led_message = seattle_transmit_led_message, +}; + +static const struct ata_port_info ahci_port_seattle_info = { + .flags = AHCI_FLAG_COMMON | ATA_FLAG_EM | ATA_FLAG_SW_ACTIVITY, + .link_flags = ATA_LFLAG_SW_ACTIVITY, + .pio_mask = ATA_PIO4, + .udma_mask = ATA_UDMA6, + .port_ops = &ahci_seattle_ops, +}; + +static struct scsi_host_template ahci_platform_sht = { + AHCI_SHT(DRV_NAME), +}; + +static ssize_t seattle_transmit_led_message(struct ata_port *ap, u32 state, + ssize_t size) +{ + struct ahci_host_priv *hpriv = ap->host->private_data; + struct ahci_port_priv *pp = ap->private_data; + struct seattle_plat_data *plat_data = hpriv->plat_data; + unsigned long flags; + int pmp; + struct ahci_em_priv *emp; + u32 val; + + /* get the slot number from the message */ + pmp = (state & EM_MSG_LED_PMP_SLOT) >> 8; + if (pmp < EM_MAX_SLOTS) + emp = &pp->em_priv[pmp]; + else + return -EINVAL; + + val = ioread32(plat_data->sgpio_ctrl); + if (state & ACTIVITY_MASK) + val |= 1 << ACTIVITY_BIT_POS((ap->port_no)); + else + val &= ~(1 << ACTIVITY_BIT_POS((ap->port_no))); + + if (state & LOCATE_MASK) + val |= 1 << LOCATE_BIT_POS((ap->port_no)); + else + val &= ~(1 << LOCATE_BIT_POS((ap->port_no))); + + if (state & FAULT_MASK) + val |= 1 << FAULT_BIT_POS((ap->port_no)); + else + val &= ~(1 << FAULT_BIT_POS((ap->port_no))); + + iowrite32(val, plat_data->sgpio_ctrl); + + spin_lock_irqsave(ap->lock, flags); + + /* save off new led state for port/slot */ + emp->led_state = state; + + spin_unlock_irqrestore(ap->lock, flags); + + return size; +} + +static const struct ata_port_info *ahci_seattle_get_port_info( + struct platform_device *pdev, struct ahci_host_priv *hpriv) +{ + struct device *dev = &pdev->dev; + struct seattle_plat_data *plat_data; + u32 val; + + plat_data = devm_kzalloc(dev, sizeof(*plat_data), GFP_KERNEL); + if (IS_ERR(plat_data)) + return &ahci_port_info; + + 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; + + val = ioread32(plat_data->sgpio_ctrl); + + if (!(val & 0xf)) + return &ahci_port_info; + + hpriv->em_loc = 0; + hpriv->em_buf_sz = 4; + hpriv->em_msg_type = EM_MSG_TYPE_LED; + hpriv->plat_data = plat_data; + + dev_info(dev, "SGPIO LED control is enabled.\n"); + return &ahci_port_seattle_info; +} + +static int ahci_seattle_probe(struct platform_device *pdev) +{ + int rc; + struct ahci_host_priv *hpriv; + + hpriv = ahci_platform_get_resources(pdev); + if (IS_ERR(hpriv)) + return PTR_ERR(hpriv); + + rc = ahci_platform_enable_resources(hpriv); + if (rc) + return rc; + + rc = ahci_platform_init_host(pdev, hpriv, + ahci_seattle_get_port_info(pdev, hpriv), + &ahci_platform_sht); + if (rc) + goto disable_resources; + + return 0; +disable_resources: + ahci_platform_disable_resources(hpriv); + return rc; +} + +static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_platform_suspend, + ahci_platform_resume); + +#ifdef CONFIG_OF +static const struct of_device_id ahci_of_match[] = { + { .compatible = "amd,seattle-ahci", }, + {} +}; +MODULE_DEVICE_TABLE(of, ahci_of_match); +#endif + +#ifdef CONFIG_ACPI +static const struct acpi_device_id ahci_acpi_match[] = { + { "AMDI0600", 0 }, + {} +}; +MODULE_DEVICE_TABLE(acpi, ahci_acpi_match); +#endif + +static struct platform_driver ahci_seattle_driver = { + .probe = ahci_seattle_probe, + .remove = ata_platform_remove_one, + .driver = { + .name = DRV_NAME, +#ifdef CONFIG_OF + .of_match_table = ahci_of_match, +#endif +#ifdef CONFIG_ACPI + .acpi_match_table = ahci_acpi_match, +#endif + .pm = &ahci_pm_ops, + }, +}; +module_platform_driver(ahci_seattle_driver); + +MODULE_DESCRIPTION("Seattle AHCI SATA platform driver"); +MODULE_AUTHOR("Brijesh Singh <brijesh.singh@amd.com>"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:" DRV_NAME);
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