From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750900AbbIZFAD (ORCPT ); Sat, 26 Sep 2015 01:00:03 -0400 Received: from mail-by2on0094.outbound.protection.outlook.com ([207.46.100.94]:46560 "EHLO na01-by2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750777AbbIZFAA (ORCPT ); Sat, 26 Sep 2015 01:00:00 -0400 Authentication-Results: spf=pass (sender IP is 149.199.60.83) smtp.mailfrom=xilinx.com; vger.kernel.org; dkim=none (message not signed) header.d=none;vger.kernel.org; dmarc=bestguesspass action=none header.from=xilinx.com; Date: Fri, 25 Sep 2015 21:59:53 -0700 From: =?utf-8?B?U8O2cmVu?= Brinkmann To: Shawn Lin CC: Michal Simek , Ulf Hansson , , , Subject: Re: [PATCH v2 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan Message-ID: <20150926045953.GB19692@xsjsorenbubuntu> References: <1442212161-23234-1-git-send-email-shawn.lin@rock-chips.com> <20150914150751.GI19678@xsjsorenbubuntu> <55F76F60.7060506@rock-chips.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <55F76F60.7060506@rock-chips.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-RCIS-Action: ALLOW X-TM-AS-Product-Ver: IMSS-7.1.0.1224-8.0.0.1202-21836.005 X-TM-AS-User-Approved-Sender: Yes;Yes X-EOPAttributedMessage: 0 X-Microsoft-Exchange-Diagnostics: 1;BL2FFO11FD012;1:XqcnTBDxQQOREjoTNHkuOw1yYTk/ZuVRVn/nbxXMidRz34p+BJnai+W70BL5QWvh3jBqgo0TW9t/FBT3jJ8a4VgDUB2xyfBEIdAWW3vSnvnHe8RcCUc3cNkW0TjY0402GrSfB2pToAp3TL4a8mS35l0VtWp+mGM7DNqVizS70uWiXgnXrHP7Xi5TvEYLSiZMK62rUsLSOQrBFrzaO/YJV9nmyeO18/EUs1egtWEZP54RRxCHlpINAyonWKz4M/vsg6YFT0ceyt6u2hxjLT3ws6+onigIctdLZA6u5ptmGZK8jDjOixNwpO/HXztDxlPUFT4s+87lmP2g2wjfITYATY4QbbcxBc+aOIly4lAJ2N25djk2xrjML0tasPJh8z9Z X-Forefront-Antispam-Report: CIP:149.199.60.83;CTRY:US;IPV:NLI;EFV:NLI;SFV:NSPM;SFS:(10009020)(6009001)(2980300002)(438002)(52314003)(377424004)(24454002)(164054003)(199003)(479174004)(189002)(52044002)(69224002)(33656002)(54356999)(77096005)(57986006)(76176999)(19580395003)(2950100001)(5001860100001)(19580405001)(4001350100001)(46102003)(81156007)(5001830100001)(50986999)(83506001)(36386004)(6806005)(64706001)(47776003)(92566002)(77156002)(230783001)(4001540100001)(189998001)(62966003)(63266004)(23676002)(85202003)(106466001)(110136002)(50466002)(86362001)(5007970100001)(87936001)(85182001)(5001960100002)(33716001)(11100500001)(76506005)(107986001);DIR:OUT;SFP:1101;SCL:1;SRVR:BL2FFO11HUB022;H:xsj-pvapsmtpgw01;FPR:;SPF:Pass;PTR:unknown-60-83.xilinx.com;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: 1;BL2FFO11HUB022;2:B5Hw6Ow3+zgXAiD0poLClbKefadCoMnOcq1TgbEzulIe+hVLZR5enJVreIAnKoSvkJLGQHpvuA4FphnChDxFoToQabRObf6v4IV+o01q/iRP/XDylJ/ZwdIJT5YpWhFSPU/zyjs4L+LPntdAFpMkD0lElBGGRu+WWSB5mhK0Noc=;3:te2g2xD7eOBe4KXtVuJDPQ32i+2HIRmrUMP/dBNXLPICtCOsjNgPNUTIflcSSqnhcevrZuGG7jywmf408bcGo01xs25+d1etrbL1CDu1qmwJ+tG7TlecRBTCcfEKT1GuvPWoC+1koaaSHSLCJlg6W/gpmVmJJ8zMv1Xl6d3KISFSx+lBBfGCR5iZfiAsEUKnrQP/UdAKHwNO7f+5mfRrkHJZir0Epp7Jv/gtu71sfCtJ2aHRwBcqBDoK6yFvWDvwF8MgtyqR1ahA8IDLJMN8pg==;25:7t/sYJPlEjIEUONmV0AUx4rXV4uVd92256xXNUHK9tWCtDN66LRjXNV3kwF3QxszY3F9tW8VQDCBDSTv16OjE0mO8jZfcem+5WKKDhYM3ms33xsvYnoEnpluA7r+l75KJJmyfpkZTK3wZxzCeU9A8rAZT0/NnNE0DAa0Rw7oM6cSjJV6KLKMxdbzoqA9N4eRQm3VVGF2i05K1Yl0K18+/iQNFajwPD3nt4+wq054BjPRW/sxIzGSc8knaUElUa7S02z6Vh95KAKO/SvBYPhABA== X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(8251501001);SRVR:BL2FFO11HUB022; X-Microsoft-Exchange-Diagnostics: 1;BL2FFO11HUB022;20:QTFetgStJMiV3vKzfl17gtELOIA8PIMqxfdHpCnGxyhEiMEr1eBupcvHjdG1qT2ZBs3Y1dsXbp6MmXbK29nubVM5eav8ZeS5F6eB8oG+y1nNZ6x/Wa1/jVRMfR+7QpAnx3s3hgWfBUntmI/6zwZ/ajMsfKuHWWd0iQ7PyPFnZpNvRY1MkRh01J8i4DcMD+G1WpfEjUQVaSFhlyQh5qQBlt3RMImKAqtHQRhbm7wbRj7vR5D5iAsVTfRA13oTxqYiiNB2Yqap4TbFTFlfmK8YiZpEvhSBGSv6n47vl5ABALKm2Miudc8NY9TzlX/UFUT67nVTB1Fsjbw6JgaimYbVY28OqofYPRIsx06GSv3qv4JcOofapb0LKIZqinoueKy+MD19KkJPUoHczQjcKuafZhTs5fq7CP711Tld/jTvB5ftogHz4aO9PTsgsqArQAs7Op5RpA4D9TmFTnR69snq0uR2c+ZTS5pLgpaVJ451vCHbDxSROoY8QWF17k2cwYTV;4:Zp8WtR9D6pju13LfhBixJIM4XIzoey/eZLNW2ZpRh92YUc/M68VTWXEgEwk9dGpkMnH3jx7WruwQz1VhIEr9a8M9ObYoyuf/JTGrctIpi6HbmejgHkfRcncfAkbMeNGyfSm9+KFOd5K3NMXTwtxnJPfminnvCuLIVYKE6gII73wbK5GA5sMTI37Iygkwq0RcA4pT29lbCJyg+prtRgKA5DQ7ZrFOtitfmje+Tv6ySS/zAy/aH9T5SHDU3wxUdX1bK68HnH7aDEwa2Eyy+RUhNwT8aLVj8jwV579eqAIRGhVyrobZRHSasNMr51E/dsguCSDJmF2RvuzJZ+WO4Xq1YW5oZLoRNrXK36VI7EGhIUo= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(520078)(8121501046)(5005006)(3002001);SRVR:BL2FFO11HUB022;BCL:0;PCL:0;RULEID:;SRVR:BL2FFO11HUB022; X-Forefront-PRVS: 071156160B X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtCTDJGRk8xMUhVQjAyMjsyMzpVUW83MFEzcVJaZlFFZVlHOEhvWEFzU0xC?= =?utf-8?B?Mm5FWkNSZEJpMTl4YTlUQmVjSlEreTdldmtwQkc5WUN1KzVNbUtQcHJIV0ZO?= =?utf-8?B?QVMvcDdJQjNiVnkrV3pnSkZCOGg1Q3RoSmZRRTB0R2dKeVdlcWxzR3VMeXBF?= =?utf-8?B?VFlkYnVSOUVXVklJRUNsTjRaRGNKdnpJQVgzZkZpMFdIT2luSXYxV1c0cGhy?= =?utf-8?B?SlJnNmFmS3BuWFJDSzVXRXRyNENSbFBMdUtnKys2eXB4ZlF3d2FjY2dsYzVr?= =?utf-8?B?REhsQ0kvcThCMm1MTXNhd21oRXhqODJGYXhDMWVtMXVzOXpxLzR4cVE0VEJi?= =?utf-8?B?RXBCUnY3NWZGNlRwaWJKN1dacU5VQ2pFR2ZvdlFDbndiVUZIWnZwZllGQlIz?= =?utf-8?B?ZXBNakthalFwaHlTWlhXK1dlYVJOZGNCa3hRbEx6bEcrclBZVzhSVVB0eUtD?= =?utf-8?B?V09temNFUlY5TFd1K0gyQ053cmNvV1BwUTQ1UGhmUGE5NVNlS29vbnpNN2lk?= =?utf-8?B?ZUdhNDM3YkxuQUNoTkpaTnYrazFHTmcwb3p1MzNhWmZINmQxanZHU2JOdDY4?= =?utf-8?B?SWVXYTEzQkRRMExnZFZVU2tXbGs3WUIvb0xEYzNETnZKQkdkR0lNZG9jek56?= =?utf-8?B?V0hWcDhRRFcxWmgreGVkc0c5WHNWQ2hPZnhRbFJwREoyVWZTSG1Qd2NkTU9x?= =?utf-8?B?SUVhV0FsNGpxaVhXd2l5Q0RxbnZHRzZIelZHSkNiM0pGeDFKUU51b1doOUJG?= =?utf-8?B?VVAxSDh0UGtLQ2NyZW5SRW1XbWkvN2Zmbm9PRHR5NSsrREN6RDFNcXFFZXdJ?= =?utf-8?B?VkYybVRReUZqc01SYXE3eTlIRWI1VThTcFUyOGxjRyt0RWNCdjA2K3JQMnNU?= =?utf-8?B?c3FUYnJGUW9tUUQrQllaVnB2L3hSVHEvRktsRThvNHNQNksvZ2JyT0R4S3V4?= =?utf-8?B?MVVPZEFDWGo3Z1pXTThWRldoaHd6NW1LT2NDVXl5dWVzbjI3blUrOEFDWnk3?= =?utf-8?B?b29GZllidE1mVzFhRk00a0MydmhoNlpnN0o2c3FNeG9hVncrZlpyY0pLenBF?= =?utf-8?B?NXpCM2hTTXJiSXJsYnptWVRic3BTUllTREdFenNTdDJmaC9YSzJ2RUVQbXVM?= =?utf-8?B?NVM2Zkw3bmNvcTVnUTZXeDlTU1puWnJPdVdmb3RGNWh4Mm9oRjE3MFkxeng4?= =?utf-8?B?Tnd6Q1A1RHJES1Z3VkRWTkdPR0l3aUczZEZSNEliVmxVM0sxTG5BTlgreG9n?= =?utf-8?B?N0x6Q1RjZ0gvdmhtS3oyMTYycGd5elBBWStkYWFSeituRkRQcitkRlVyaFhI?= =?utf-8?B?Y1M5cXpUTWJZR01meFI0bHZNY0xCSmw4YUM1U0wrTGU3QzlMNkJXUGt4dDlz?= =?utf-8?B?SmpHYTBQL2owU3h5QzJscWswL01ERlpGc0NnNCtnMzgrZVRiMDhlcWNwaWRK?= =?utf-8?B?bTEzWkJPNTExMEk5T2thWHNwam4yRmpURHFvZWlvWHhGWkR2enlpRVMrWmRS?= =?utf-8?B?eE1GdklwTVdXL0QxbGJuZXp1K0Y5Q1NoSHdjV3FjL0xFU3RjN1dhVmdRUVFK?= =?utf-8?B?NEgwWHBLQzJmVndxeExYdk12V01tRUNDeG5Pc1lWRGRTQUd1ZEZibzl3endk?= =?utf-8?B?NGFOei9iYWxTcXlQRENNbVNFVFJxc1p3dW5kbVAwd0U4SVNLaEVJbVdIK2JK?= =?utf-8?B?UGowdWszRXhKWWNzclNEUEVYYnZHdlptSG82bVBBYXp0RXdqQnN6U3QrNlF6?= =?utf-8?B?MVJUL2tmaW1CNDBtenVoZHlma0FybXE0Zm5VYlFjVS82bDdnRit6cGpSaExY?= =?utf-8?B?YmMydkVDQjU4MnNQYjdDVU1KYzJLV3U0RENuR0p3dDJPUVRnPT0=?= X-Microsoft-Exchange-Diagnostics: 1;BL2FFO11HUB022;5:wYLZVfSjAlQb5U9LZXLLmX31bsh25FbWFjv8V7JFXEz7QJru0NffCdRyd5zjvqe1JDsldOvqMkm9Kj5C9oAHw2LsFhUEl4+oNG/z15UHThHWMPSm2A28EBzP09pFKh1qISQeR8XhKPedMiBr/gdGDA==;24:8fPYGCLGDCpKoqce1qqF/3w7Fvh1g508gAl9YytOQOJh3Al2+uwyETwOjLGZBhGGTZ9SwpWcXV7BL1nu6n5Q4PubGw7IdcD8WwJ7gVrfAxk= SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: xilinx.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Sep 2015 04:59:57.2941 (UTC) X-MS-Exchange-CrossTenant-Id: 657af505-d5df-48d0-8300-c31994686c5c X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=657af505-d5df-48d0-8300-c31994686c5c;Ip=[149.199.60.83];Helo=[xsj-pvapsmtpgw01] X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL2FFO11HUB022 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2015-09-15 at 09:07AM +0800, Shawn Lin wrote: > On 2015/9/14 23:07, Sören Brinkmann wrote: > >Hi Shawn, > > > >overall, it looks good to me. I have some questions though. > > > >On Mon, 2015-09-14 at 02:29PM +0800, Shawn Lin wrote: > > [...] > > >>+err_phy_exit: > >>+ phy_init(phy); > > > >Just to confirm, are these actions in the error path correct? E.g. > >if the power_off() call fails, is it safe to call power_on()? Isn't > >the phy still powered on? (this would apply to other error paths too) > > > > Cool question! > > While writing this, I had read generic phy stuffs deliberately to find a > solution for a case: how to deal with ping-pong fails? In another word, if > power_off call fails, then we should call power_on, but unfortunately it > fails again then we call power_off... so endless nested err handling... No > answer yet. > > So, I assumed two cases happened when power_off call fails: > (1) *real power_off* is done, but some other stuffs in the calling path > fail, so phy is really power_off in theory. We need to power_on it again, > but if it fail, we don't know PHY is on or off since we don't know power_on > fails for what? *real power on* ? some other stuffs? > > (2) *real power_off* isn't completed, so indeed it's *still* in power_on > state. The reason we never need to check the return value of power_on cross > the err handling is that whether power_on call successfully or not, it's > always make phy in power_on state. > > Now, let's think about case(1). > After reading dozens of sample codes(such as USB, UFS, MBUS) that adopt > generic phy framework for PHY management, real thing should be like that: > they NEVER deal with case(1). > > It's a trick of sub-phy drivers themself. power_on/off calling path return > err for two case: > <1> phy_runtime callback fails. It's after *real power on/off*, so > definitely *real power on/off* is conpleted. That is the case(2) I mentioned > above. > <2> sub-phy drivers return err for phy->ops->power_off(phy); Look > into all the sub-phy drivers twice, we find that they always return success > for phy->ops->power_off hook. Why? Because all of them > write registers to enable/disable something, always consider things going > well. Actually if we write value into a register, we have to think that it's > functional. > > Anyway, back to this patch. > Indeed we also write value into arasan phy's register to do > phy_power_on/off/init/exit to make things work. Right, we return success > state for all of these them just as all the other sub-phy drivers do. > > Feel free to let me know if I make mistakes or misunderstanding above. > > >>+ return ret; > >>+} > > [...] > > >>+ } > >>+ } > > > >I assume you looked at options for having the error paths in a > >consolidated location? I guess this may be the nicest solution since all > >of this is in this conditional block? > > > > yep, otherwise we should add some *if* statements to check sdhci_arasan->phy > cross the err handles. And I intent to strictly limit > the phy stuffs under the scope of arasan,sdhci-5.1 currently. > > >Feel free to add my > >Acked-by: Sören Brinkmann > > > > Thanks, Sören. :) Makes all sense to me. Thanks for all the details. Sören