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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html