From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754353AbcAHIrf (ORCPT ); Fri, 8 Jan 2016 03:47:35 -0500 Received: from mout.kundenserver.de ([212.227.17.13]:64780 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752070AbcAHIrd (ORCPT ); Fri, 8 Jan 2016 03:47:33 -0500 From: Arnd Bergmann To: Brijesh Singh Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, mark.rutland@arm.com, devicetree@vger.kernel.org, pawel.moll@arm.com, ijc+devicetree@hellion.org.uk, linux-ide@vger.kernel.org, robh+dt@kernel.org, galak@codeaurora.org, tj@kernel.org Subject: Re: [PATCH] ata: add AMD Seattle platform driver Date: Fri, 08 Jan 2016 09:46:50 +0100 Message-ID: <10869853.plxna0HzWE@wuerfel> User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; ) In-Reply-To: <568F14E0.4060107@amd.com> References: <1452200002-31590-1-git-send-email-brijesh.singh@amd.com> <4983521.tEaWggKCCv@wuerfel> <568F14E0.4060107@amd.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:zaREIZy1Pr6KzHO19MphRWk6CKd1nIVn92QpeANrcq5oS36XNgI 9MBqA+4/JH0e8/TB5WxsS4Zd4Rs7IwFfUfELOPc0+W61oPsp0V+fNHsBGYoPoRVNFw0s93C SVfhZWM3nxHNVvKc3JpuUHHiFWiEZIjoGz0Ykrg5FUojzvXJ2z0J1vuLL+aep2ZJThljzly JHhtsnruzrlghH/MyRISg== X-UI-Out-Filterresults: notjunk:1;V01:K0:sPVvs4duElo=:NQWC7jBaiq2vR1jV1V0f0B Uyj1OtIRfVQbhcX/g2F11mi6N4fSknwfWCYM4udmVO75ZZPl9o+2rdjyRa0I7eMPr0v/5Tyr9 K6ug3W+Ii9kXiIqEwXBFulVa8cGDx9U1gYPm7OQIVAiF4Q2KkRuOIpMNIZWBRG0T3y5vSAdzJ RjEkII/7crvoDgzmloZ4hfD/mx3P6oirla4X3ceGaorLQ90d91N222AmoeRY5Zf2TxcRRvhw/ FKW2dWlyDu1KId21st8KLsnQqgPXJwh/tGjYxkVIH970BeSb4ju1tcgMHsaHqyQpTL5z3sUp7 CxGkzvxFNLf/0S6XinReFzTQhYfv7Yr56O788RRolkcgwzNPvE9YiWXRVEiP7NGTqlRZXpum4 iZP8HkUnTBE8qHLWBvQF/PILdIODHm+/+16ZVHf+2MdUxWXnp1Lf2KqAJETvcbw0vKbwTjIZz zoe5em37UWiF5CLegeViCM/TMyqIjIJC88/mDIzZLzI/M3MbdBQKIZZgkCOEK5czIE55ISI1s /Q9XUvJbo0sAEaWtO+jsADdhRtgo4JOS6gVEeDw9fdYxcQC7Y4+ugTzYZOfffQCLC6ciVgIDH z5wWMbe4komomSe5dxb36wmSOJKW2B8xUuJsfm9WWZnIPXnOd6NKSWrK4yfB6mwJThzB/9qK0 bKKJ8/eQZAapWlyo7iELgmwH1bt/3Zb4xpKyE4Con67+gHr57w0wzwmbtB5IA58GWfjRQ233f pUFwf9pSIiOFw6ow Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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