Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966531AbcDLWhO (ORCPT ); Tue, 12 Apr 2016 18:37:14 -0400 Received: from mail-pf0-f171.google.com ([209.85.192.171]:32829 "EHLO mail-pf0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966404AbcDLWhM (ORCPT ); Tue, 12 Apr 2016 18:37:12 -0400 Message-ID: <570D7895.7000606@gmail.com> Date: Tue, 12 Apr 2016 15:37:09 -0700 From: Frank Rowand Reply-To: frowand.list@gmail.com User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20130801 Thunderbird/17.0.8 MIME-Version: 1.0 To: Rob Herring CC: Tony Lindgren , Grant Likely , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , linux-omap , Nishanth Menon , Tero Kristo , Tom Rini Subject: Re: [PATCH] of: Add generic handling for hardware incomplete fail state References: <1460486275-12256-1-git-send-email-tony@atomide.com> <570D56FE.2070408@gmail.com> <20160412203431.GU5995@atomide.com> <570D6B7A.3050203@gmail.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4806 Lines: 124 On 4/12/2016 3:20 PM, Rob Herring wrote: > On Tue, Apr 12, 2016 at 4:41 PM, Frank Rowand wrote: >> On 4/12/2016 1:34 PM, Tony Lindgren wrote: >>> Hi, >>> >>> * Frank Rowand [160412 13:15]: >>>> Hi Tony, >>>> >>>> I agree with the need for some way of handling the incomplete >>>> hardware issue. I like the idea of having a uniform method for >>>> all nodes. >>>> >>>> I am stumbling over what the status property is supposed to convey >>>> and what the "fail-hw-incomplete" is meant to convey. >>>> >>>> The status property is meant to convey the current state of the >>>> node. >>>> >>>> "fail-hw-incomplete" is meant to describe the node implementation, >>>> saying that some portions of hardware that the driver expects to >>>> be present do not exist. If I understood your explanation at ELC >>>> correctly, an examples of this could be that a uart cell is not >>>> routed to transmit and receive data pins or the interrupt line >>>> from the cell is not routed to an interrupt controller. So the >>>> node is not useful, but it makes sense to be able to power manage >>>> the node, turning off power so that it is not wasting power. >>> >>> Yes cases like that are common. >>> >>>> It seems to me that the info that needs to be conveyed is a >>>> description of the hardware, stating: >>>> - some portions or features of the node are not present and/or >>>> are not usable >>>> - power management of the node is possible >>>> >>>> Status of "fail-sss" is meant to indicate an error was detected in >>>> the device, and that the error might (or might not) be repairable. >>>> >>>> So the difference I see is state vs hardware description. > > The question to ask is are we indicating the "operational status of a > device"? If yes, that is the definition of status and using it would > be appropriate. > > IMO, I think we are. I see the reasoning. I could go either way, but I lean toward thinking of it as hardware description. >>> OK thanks for the clarification. I don't see why "fail-hw-incomplete" >>> could not be set dynamically during the probe in some cases based >>> on the SoC revision detection for example. So from that point of >>> view using status with the "fail-sss" logic would make more sense. >> >> If the probe detects that the device should only be power managed >> based on the SoC revision, then it would simply be one more >> test added at the top of probe. The patch would change from: >> >> if (of_device_is_incomplete(pdev->dev.of_node)) { >> >> to: >> >> if (of_device_is_incomplete(pdev->dev.of_node) || socrev == XXX) { > > I think Tony meant the bootloader or platform code would do this and > tweak the DT. We don't have much of a standard API for revision > checking, so drivers don't check SoC revisions generally. OK, that makes more sense to me. >> That code would be the same whether the property involved was >> status or something else. >> >>> >>>> I would prefer to come up with a new boolean property (with a >>>> standard name that any node binding could choose to implement) >>>> that says something like "only power management is available for >>>> this node, do not attempt to use any other feature of the node". >>> >>> Heh that's going to be a long property name :) How about >>> unusable-incomplete-idle-only :) >> >> Or even pm-only. Maybe I got a little carried away with my >> verbosity. :) > > I don't think we should define it so narrowly. I think DT just > indicates the device is in a non-usable state (somewhere between ok > and disabled) and the driver knows what to do with that information. My concern is that "non-usable" state is really vague. I would prefer that the message (however it is communicated) tells the driver either what is usable or what is unusable. >>>> With that change, the bulk of your patch looks good, with >>>> minor changes: >>>> >>>> __of_device_is_available() would not need to change. >>>> >>>> __of_device_is_incomplete() would change to check the new >>>> boolean property. (And I would suggest renaming it to >>>> something that conveys it is ok to power manage the >>>> device, but do not do anything else to the device.) >>> >>> I'm fine with property too, but the runtime probe fail state >>> changes worry me a bit with that one. >> >> I don't understand what the concern is. The change I suggested >> would use exactly the same code for probe as the example patch >> you provided, but just with a slight name change for the function. >> >> >>> I think Rob also preferred to use the status though while we >>> chatted at ELC? >> >> That is the impression I got too. We'll have to see if I can >> convince him otherwise. > > I did, but I'm not wed to it. I think it depends on the question above. > > Rob >