From mboxrd@z Thu Jan 1 00:00:00 1970 From: Brijesh Singh Subject: Re: [PATCH] ata: add AMD Seattle platform driver Date: Thu, 7 Jan 2016 19:46:08 -0600 Message-ID: <568F14E0.4060107@amd.com> References: <1452200002-31590-1-git-send-email-brijesh.singh@amd.com> <4652600.rUhSEOUPkl@wuerfel> <568EE596.3060005@amd.com> <4983521.tEaWggKCCv@wuerfel> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4983521.tEaWggKCCv@wuerfel> Sender: linux-kernel-owner@vger.kernel.org To: Arnd Bergmann 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 List-Id: linux-ide@vger.kernel.org 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 > From mboxrd@z Thu Jan 1 00:00:00 1970 From: brijesh.singh@amd.com (Brijesh Singh) Date: Thu, 7 Jan 2016 19:46:08 -0600 Subject: [PATCH] ata: add AMD Seattle platform driver In-Reply-To: <4983521.tEaWggKCCv@wuerfel> References: <1452200002-31590-1-git-send-email-brijesh.singh@amd.com> <4652600.rUhSEOUPkl@wuerfel> <568EE596.3060005@amd.com> <4983521.tEaWggKCCv@wuerfel> Message-ID: <568F14E0.4060107@amd.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi, On 01/07/2016 05:42 PM, Arnd Bergmann wrote: > On Thursday 07 January 2016 16:24:22 Brijesh Singh wrote: >>>> + >>>> +Examples: >>>> + sata0 at 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 at 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 > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753734AbcAHBq7 (ORCPT ); Thu, 7 Jan 2016 20:46:59 -0500 Received: from mail-bn1on0065.outbound.protection.outlook.com ([157.56.110.65]:56064 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752209AbcAHBq5 (ORCPT ); Thu, 7 Jan 2016 20:46:57 -0500 X-Greylist: delayed 16458 seconds by postgrey-1.27 at vger.kernel.org; Thu, 07 Jan 2016 20:46:56 EST Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=brijesh.singh@amd.com; Subject: Re: [PATCH] ata: add AMD Seattle platform driver To: Arnd Bergmann References: <1452200002-31590-1-git-send-email-brijesh.singh@amd.com> <4652600.rUhSEOUPkl@wuerfel> <568EE596.3060005@amd.com> <4983521.tEaWggKCCv@wuerfel> CC: , , , , , , , , , From: Brijesh Singh Message-ID: <568F14E0.4060107@amd.com> Date: Thu, 7 Jan 2016 19:46:08 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <4983521.tEaWggKCCv@wuerfel> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [70.112.153.56] X-ClientProxiedBy: BY1PR15CA0042.namprd15.prod.outlook.com (25.162.17.180) To SN1PR12MB0671.namprd12.prod.outlook.com (25.163.208.29) X-Microsoft-Exchange-Diagnostics: 1;SN1PR12MB0671;2:HcPvqrcPMqmI60Q5OrnOWwffkOqig17tYKZtdkIN0NC2xlr0weos5yD/ndpFmFQjEj8Zp/v2xppvQP0CUauliHXCA5WajCp/73GAgoWfbExkJQ8SNMFp5W8MoZ8Cg3dhNGqdOT+vrfntgKTmQkn8Rw==;3:YNAK2ef/fQJhNrt+VaaOibgoYDEvGqxdiQicm00/Y12o9YB9/nbN/YfS6QldvLXkcmNcBhPaCfPZxXa6/l+ZH+ULKv8fWYN8CTyDMKbCSHd6Y5IiSrh5GMT42xxla934;25:uV5MSZ8kZ0SLh/GoqI18X2vVf6c62u+olzUwss7JBwTzE7aI1bQqFsE1xVJ27G0uNT/GGclnpySmjPhCF+S/nWdcnEACMoDDwjlwhlpzFZsqj4dugMz5+fC+mjyIr+Iy3HUxgsYb4pOTSmJkeTLtDEiqfYrGbCR2DO3v1DBNRFv2aePd4GRXPSEV0qYLz/B0zNDep9B2FF9+/VMBbhLtzWCvbjE6P3tq1wsxa7HPgoanlTF9V2u+k6T2e/GMIzDq X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:SN1PR12MB0671; X-MS-Office365-Filtering-Correlation-Id: 5ccb94e2-a105-4cf7-c877-08d317cd958d X-Microsoft-Exchange-Diagnostics: 1;SN1PR12MB0671;20:T4KqKcoB2MeXkjTAm134ybKy13uKC434z86e37Wt+VgId4jVBiTx3zQ8+rwP5HaNu2w7bK3W4+8+iwsbcfDpOuYygFOG+RH0oiP+Go5Torv1H0QFpL18feN8amSMnAXk6EsrTU72nlu+cftaIr449hHtFb2Z5bHxcji5FW8pyzN2MlE1bCGfsz2c5EGpXxyD8KDBj2DUECiXTeeU6knUvM1sXkaCSj+DK7lO5aZQQLcVhgBL4ZEp0GJF30mgckz7mDsfkTWM+cV3zmEhC2Qvt5v93RI2BWZUliHVFzXk7zA9eH/o0GoAhFhNJWIkzojfj/toBxosCHE4qQ4pC5oPDHkiQ2CLFkmiIcrH0W98todPcWc0aR9HoKVZAfyuruKypZLC9ZU6FQQX0yjfHg0H0j0cS96IzXIV/Xky3CW+cFt+etoOgL4vRU9PP2YO0KdbyfotnR0s2tDF5ZGZKlaLdHj3c02v/8ma9NrqK0d8NAUwSGSuTkscZWfUaswyQJnX;4:QWC84jauTIsR3KkFUSEDhgAcpp6Xz2i0BhVNldbz23jViXPNnZGBruX/djMQcapPAzZRNle9kPPPMzKCoyjx0rptWny8jgX5R5KgGeO7L7HIKLwIWtgbP6JpB6iDxWBnRAZkHwnNgeWtgijAHfDcFIWaNyLGmsYMCnfAKryPbWLD3jWX3FKwUMVZmTksPD3hxNo2MtkyqUeBOHs8MYEYGLDxRqRSYdw2o0k+jLhqIARD+RalSA9q9EJ1Ij2LgqPDLm7LVQSF6T6F5+U/groMSj97Je4+ncOVWmgbtqGchzBUgqpcdRCHsylehz47u+LirepUqMBLI/wksQ5gNSbxfCawW2BTcCn0/23Q4X8SmPmb8NP724gL39xV4vsi9IPY X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(5005006)(520078)(8121501046)(10201501046)(3002001);SRVR:SN1PR12MB0671;BCL:0;PCL:0;RULEID:;SRVR:SN1PR12MB0671; X-Forefront-PRVS: 0815F8251E X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(6049001)(6009001)(377454003)(479174004)(199003)(24454002)(189002)(69224002)(83506001)(105586002)(110136002)(4326007)(33656002)(19580395003)(106356001)(64126003)(93886004)(87976001)(65816999)(87266999)(189998001)(23746002)(4001350100001)(15975445007)(40100003)(2950100001)(92566002)(50986999)(5001960100002)(54356999)(86362001)(5890100001)(3846002)(50466002)(42186005)(77096005)(230700001)(59896002)(47776003)(81156007)(76176999)(97736004)(117156001)(122386002)(65956001)(66066001)(6116002)(5008740100001)(1096002)(19580405001)(2906002)(80316001)(586003)(101416001)(5004730100002)(36756003)(65806001);DIR:OUT;SFP:1101;SCL:1;SRVR:SN1PR12MB0671;H:[192.168.1.30];FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?Windows-1252?Q?1;SN1PR12MB0671;23:mwWBGfnnA8B/Ex7FHd2lJAQIo/Z8Dj6n0koBc?= =?Windows-1252?Q?LfOu4wo4je4Y+ahBxOcY0kRQMEQRe1PFJo9KUgEWZzh0ndV+u+W8feB6?= =?Windows-1252?Q?3L54yNmnscvfWkmy7QX2piW29Ic+8MEjMPo59oaIkLvPOassB7D1TjA/?= =?Windows-1252?Q?t4KTym4Xflw0sJfBmfQEGZTQ8qNihgRpvpll1/0vspPG6Rgy3c/NY4BO?= =?Windows-1252?Q?Qq1tm+DIx1+wMQzC+ytrTdR79jNyjDMYMWFqpw+mXHMlOjPnKK0CzhDJ?= =?Windows-1252?Q?KiCH+jAarWjUlJxnIF8vMZc/MzLcSu7tL6R+qyQNIQpHSJVLIGW+NAk3?= =?Windows-1252?Q?R23G94amVM0L6CZ1MLVaAVcI0KLVW6nvmt+ajVsCAEiChzg7t7yTwVlL?= =?Windows-1252?Q?92sESuTbgTRbKhJPWQgr6u8hk2NUuQgUDWhpOKdgkB5AMEWxfzqsj7IM?= =?Windows-1252?Q?pXFv697wPhW//XbYGLTtafRtbSLalQd0lNUEh53Zfh+ZxtiuhOcb3gIL?= =?Windows-1252?Q?TddCu0Siv7C4UnF0r0EyXIncu2T7zBwVwz8zoupasIk/Tf/9xbL+HK0L?= =?Windows-1252?Q?IpVoz7OSyt69cBHMrLvKsMY00B/+O+2/iSqJrmydLH8NplSRjelK7U8j?= =?Windows-1252?Q?g14lfKFaMNhirnfaWpaq/ffP+VRlU3rMa4+ghWM2L6l7HYBTCjdGlmO3?= =?Windows-1252?Q?mMVtQN6alGIQKi2x7rQD/8ZUuU03GgiMCexAqwaAduI0VkeDa9TElf48?= =?Windows-1252?Q?S8OyVfcLn5N4N2qKHKKJieEdpOR0WhLACZQTL6Qrw+Q8b1Gge4QGXdcc?= =?Windows-1252?Q?iaM8+1O1klaIKAyLW75j+biH+hUFcyk5087xAU1+GTG72DsNW0a4J6NJ?= =?Windows-1252?Q?NrAiaxw99+PMAZ49qPu6+t/7JTB1WYvaPYoAh97V9HDV+v4WPF7rbQde?= =?Windows-1252?Q?KvoaF/G2gbD1XciQ7CtqxSOMle3h47s/8ThMdaXk6ekkbla/Mpw1y0SK?= =?Windows-1252?Q?KBB3wgstX/x3ZgFaWeysgOlBb6faMN0MNNfSwZ1yaUgzgSEWEJTTkHG7?= =?Windows-1252?Q?PjrSe4xBuvAI39Z7KiDyL9Eb9eHJnelyjmxYcSTZrK0Yzqi8sslRT/YG?= =?Windows-1252?Q?w6zsV6hvj2emdRrQFjGfg3yxWK7WcxsW8yUlwmVYJG2YmyJeoonNL0o1?= =?Windows-1252?Q?z/F7j+0vrRormBh6BY6MIHGvV0dFvyu3uFbS3i9QjAqkgYOO1E99RIGx?= =?Windows-1252?Q?8/ZOe64kOEh7ANmIkLGQvyI6NMj/XXYyG7VzJBVaM0eOmm+fVa87VrLc?= =?Windows-1252?Q?vOwk0PmCbLCGSCSSCYz28Mussctaa67bBdwsbkHoKkTlQVgz6fRx1v3C?= =?Windows-1252?Q?FiGfaIHgEhGJvzpOv1JpKJVSE4NTHXiL3fUKzuShb8OX2zSeebnzZZEG?= =?Windows-1252?Q?0jb1gZQWmycSyKmDNoBBvLMFVO1ac27hhB/+VafzAlVBlvcjJdOb3E42?= =?Windows-1252?Q?H4CDhPmW93Ucis1FDuioBlv7/5Ci6mGowM9h1waTqGhdUB6odsM2/99X?= =?Windows-1252?Q?n/3YXSg4qbxoXs=3D?= X-Microsoft-Exchange-Diagnostics: 1;SN1PR12MB0671;5:tJt3fOEeJA9v0zGmczQzIgB8wH5DvjAyFgJqENmT/AggkhPFXY828KtDIWOxFjfnyv2fe+bDV4ijx7ctOpUH+cC7kNbu6JPsQWd60P8OshsIkqsEj7d1zX0Nm8n5rLwvlSjGmBEKOM/3Sb0G1id9tg==;24:7R0Byk7hFwb45tSZUVmHvXHHLFUJjK3mlJ9UAaH/mYr0anOyLPUVtc/ZJkU+XlQNbu87QAMWxkcSbaTFn1hfLACk+mzQNUMniB7Zo7IE+N0=;20:RSZRl6y7ELte7knGORMwvNj0gz0g1m+7XHxZJRvuD9Vy/Q2b8SV0DnFbMsFKN0+63hfG07aURj6Bv79QSYRD/zltr0mYJth9kMwVuUKbu6uGJf+52t25RJRBuM9G8oZI728IIeNQ/Ex58X2/ObXDD3+uUAKykBHLBBH6R9/JnVMWzf84RhafJaoaL6NzNRv/zz6+2JADO37MH13GXVdLhVI2lhtdZF0zUNbhxxI8xFcScjGBR6HZLieeIogam9b6 SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Jan 2016 01:46:52.0921 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1PR12MB0671 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >