Re: [PATCHv2] of: Add generic handling for ePAPR 1.1 fail-sss states

[Frank Rowand <frowand.list@gmail.com>]

On 09/08/16 08:58, Tony Lindgren wrote:
> * Rob Herring <robh+dt@kernel.org> [160908 06:38]:
>> On Wed, Aug 31, 2016 at 4:41 PM, Tony Lindgren <tony@atomide.com> wrote:
>>> * Frank Rowand <frowand.list@gmail.com> [160831 13:51]:
>>>> I am still opposed to using the status property for this purpose.
>>>>
>>>> The status property is intended to report an operational problem with
>>>> a device or a device that the kernel can cause to be operational (such
>>>> as a quiescent cpu being enabled).  It is the only property I am aware
>>>> of to report _state_.
>>
>> Yes, in theory a device can go from disabled to okay, but that's
>> generally never been supported. Linux takes the simple approach of
>> "disabled" means ignore it. I think we'll see that change with
>> overlays.
> 
> Yeah I think we have to assume that.
> 
>>>> It is unfortunate that Linux has adopted the practice of overloading status
>>>> to determine whether a piece of hardware exists or does not exist.  This
>>>> is extremely useful for the way we structure the .dts and .dtsi files but
>>>> should have used a new property name.  We are stuck with that choice of
>>>> using the status property for two purposes, first the state of a device,
>>>> and secondly the hardware description of existing or not existing.
>>
>> I don't agree. Generally, disabled means the h/w is there, but don't
>> use it. There may be some cases where the hardware doesn't exist for
>> the convenience of having a single dts, but that's the exception.
>>
>>>> Why not just create a new property that describes the hardware?
>>>> Perhaps something like:
>>>>
>>>>    incomplete = "pins_output", "buggy_dma";
>>>
>>> New property for incomplete works for me. Rob, got any comments here?
>>
>> Pins not muxed out or connected on the board has to be the #1 reason
>> for disabled status. I don't think we need or want another way to
>> express that.
> 
> Both status and and a separate property work for me.
> 
> If no other considerations, we should probably pick something with a
> a limited set of states to avoid it getting out of control and being
> misused for something weird like driver probe order..

I do not want to add another property that conveys state.  The property
that I was proposing does not convey state -- it describes the hardware.


> For example, just status = "fail" would be enough for the cases I've
> seen. That would still allow probe the device, then PM runtime idle
> it and bail out with -ENODEV.
> 
> For whatever warnings or errors the driver needs to show, the driver
> could probably figure it out. I don't know if we want to or need to
> pass any informational messages with the incomplete status or
> property :)
> 
>> We may have discussed this, but why can't the driver that checks fail
>> state just check whatever was used to set the device to fail in the
>> first place?
> 
> Well there may be no way to check if something is pinned out based on
> the hardware. The same SoC can be packaged with different pins. In that
> case only the die id or serial number for each produced chip is
> different, not the revision numbers. So for cases like that the dtb
> file or kernel cmdline is the only information available. And we can't
> assume pinctrl is required as it's perfectly fine to do that only in
> the bootloader for the static cases to save memory.
> 
> Just to consider other ways of doing it, we could use the compatible
> flag to tag devices that need to be just idled on probe, but that does
> not seem like generic solution to me.

Yuck.  Again overloading a property to convey multiple pieces of
information.


> 
> Regards,
> 
> Tony
>