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

[Frank Rowand]

On 09/08/16 12:09, Frank Rowand wrote:
> On 09/08/16 08:58, Tony Lindgren wrote:
>> * Rob Herring <robh+dt@xxxxxxxxxx> [160908 06:38]:
>>> On Wed, Aug 31, 2016 at 4:41 PM, Tony Lindgren <tony@xxxxxxxxxxx> wrote:
>>>> * Frank Rowand <frowand.list@xxxxxxxxx> [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.

I should have been more explicit in that statement.

If the hardware device does not have "wires" routed to a connector or
bus then it is still the same device.  Thus the compatible should be
the same.

The difference is the way the device is used in the SOC or board.

>>
>> Regards,
>>
>> Tony
>>
> 
>