[1/3] dt-bindings: hwmon: Add IBM OCC bindings

These bindings describe the POWER processor On Chip Controller accessed
from a service processor or baseboard management controller (BMC).

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 .../bindings/hwmon/ibm,occ-hmwon.yaml         | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ibm,occ-hmwon.yaml

On Tue, 02 Aug 2022 14:46:54 -0500, Eddie James wrote:
> These bindings describe the POWER processor On Chip Controller accessed
> from a service processor or baseboard management controller (BMC).
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  .../bindings/hwmon/ibm,occ-hmwon.yaml         | 40 +++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ibm,occ-hmwon.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/hwmon/ibm,occ-hmwon.yaml: $id: relative path/filename doesn't match actual path or filename
	expected: http://devicetree.org/schemas/hwmon/ibm,occ-hmwon.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

On 02/08/2022 21:46, Eddie James wrote:
> These bindings describe the POWER processor On Chip Controller accessed
> from a service processor or baseboard management controller (BMC).
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  .../bindings/hwmon/ibm,occ-hmwon.yaml         | 40 +++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ibm,occ-hmwon.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ibm,occ-hmwon.yaml b/Documentation/devicetree/bindings/hwmon/ibm,occ-hmwon.yaml
> new file mode 100644
> index 000000000000..8f8c3b8d7129
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ibm,occ-hmwon.yaml
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/ibm,occ-hwmon.yaml#

typo here

Does not look like you tested the bindings. Please run `make
dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: IBM On-Chip Controller (OCC) accessed from a service processor
> +
> +maintainers:
> +  - Eddie James <eajames@linux.ibm.com>
> +
> +description: |
> +  This binding describes a POWER processor On-Chip Controller (OCC)

s/This binding describes a//
But instead describe the hardware. What is the OCC?

> +  accessed from a service processor or baseboard management controller
> +  (BMC).
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ibm,p9-occ-hwmon
> +      - ibm,p10-occ-hwmon
> +
> +  ibm,inactive-on-init:
> +    description: This property describes whether or not the OCC should
> +      be marked as active during device initialization. The alternative
> +      is for user space to mark the device active based on higher level
> +      communications between the BMC and the host processor.

I find the combination property name with this description confusing. It
sounds like init of OCC and somehow it should be inactive? I assume if
you initialize device, it is active. Or maybe the "init" is of something
else? What is more, non-negation is easier to understand, so rather
"ibm,active-on-boot" (or something like that).

> +    type: boolean
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    occ-hmwon {

just "hwmon"

> +        compatible = "ibm,p9-occ-hwmon";
> +        ibm,inactive-on-init;
> +    };


Best regards,
Krzysztof

On 8/3/22 01:55, Krzysztof Kozlowski wrote:
> On 02/08/2022 21:46, Eddie James wrote:
>> These bindings describe the POWER processor On Chip Controller accessed
>> from a service processor or baseboard management controller (BMC).
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>>   .../bindings/hwmon/ibm,occ-hmwon.yaml         | 40 +++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/ibm,occ-hmwon.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/ibm,occ-hmwon.yaml b/Documentation/devicetree/bindings/hwmon/ibm,occ-hmwon.yaml
>> new file mode 100644
>> index 000000000000..8f8c3b8d7129
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/ibm,occ-hmwon.yaml
>> @@ -0,0 +1,40 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/hwmon/ibm,occ-hwmon.yaml#
> typo here
>
> Does not look like you tested the bindings. Please run `make
> dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).


I actually did but it didn't catch that somehow. I had to use a somewhat 
hacked together python/pip on my system so perhaps that's to blame.


>
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: IBM On-Chip Controller (OCC) accessed from a service processor
>> +
>> +maintainers:
>> +  - Eddie James <eajames@linux.ibm.com>
>> +
>> +description: |
>> +  This binding describes a POWER processor On-Chip Controller (OCC)
> s/This binding describes a//
> But instead describe the hardware. What is the OCC?


OK I'll fix that. It's a management engine for system power and thermals.


>
>> +  accessed from a service processor or baseboard management controller
>> +  (BMC).
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ibm,p9-occ-hwmon
>> +      - ibm,p10-occ-hwmon
>> +
>> +  ibm,inactive-on-init:
>> +    description: This property describes whether or not the OCC should
>> +      be marked as active during device initialization. The alternative
>> +      is for user space to mark the device active based on higher level
>> +      communications between the BMC and the host processor.
> I find the combination property name with this description confusing. It
> sounds like init of OCC and somehow it should be inactive? I assume if
> you initialize device, it is active. Or maybe the "init" is of something
> else? What is more, non-negation is easier to understand, so rather
> "ibm,active-on-boot" (or something like that).


Well, the host processor initializes the OCC during it's boot, but this 
document is describing a binding to be used by a service processor 
talking to the OCC. So the OCC may be in any state. The init meant 
driver init, but I will simply the description and change the property 
to be more explicit: ibm,no-poll-on-init since that is what is actually 
happening. Similar to the FSI binding for no-scan-on-init.


>
>> +    type: boolean
>> +
>> +required:
>> +  - compatible
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    occ-hmwon {
> just "hwmon"
>
>> +        compatible = "ibm,p9-occ-hwmon";
>> +        ibm,inactive-on-init;
>> +    };


Thanks for the review!

Eddie


>
> Best regards,
> Krzysztof

On 09/08/2022 22:42, Eddie James wrote:
>>> +  ibm,inactive-on-init:
>>> +    description: This property describes whether or not the OCC should
>>> +      be marked as active during device initialization. The alternative
>>> +      is for user space to mark the device active based on higher level
>>> +      communications between the BMC and the host processor.
>> I find the combination property name with this description confusing. It
>> sounds like init of OCC and somehow it should be inactive? I assume if
>> you initialize device, it is active. Or maybe the "init" is of something
>> else? What is more, non-negation is easier to understand, so rather
>> "ibm,active-on-boot" (or something like that).
> 
> 
> Well, the host processor initializes the OCC during it's boot, but this 
> document is describing a binding to be used by a service processor 
> talking to the OCC. So the OCC may be in any state. The init meant 
> driver init, but I will simply the description and change the property 
> to be more explicit: ibm,no-poll-on-init since that is what is actually 
> happening. Similar to the FSI binding for no-scan-on-init.

no-scan-on-init is not a good example. It wasn't even reviewed by Rob
(looking at commit). In both cases you describe driver behavior which is
not appropriate for bindings. Instead you should describe the hardware
characteristics/feature/bug/state which require skipping the initialization.


Best regards,
Krzysztof

On 8/10/22 02:43, Krzysztof Kozlowski wrote:
> On 09/08/2022 22:42, Eddie James wrote:
>>>> +  ibm,inactive-on-init:
>>>> +    description: This property describes whether or not the OCC should
>>>> +      be marked as active during device initialization. The alternative
>>>> +      is for user space to mark the device active based on higher level
>>>> +      communications between the BMC and the host processor.
>>> I find the combination property name with this description confusing. It
>>> sounds like init of OCC and somehow it should be inactive? I assume if
>>> you initialize device, it is active. Or maybe the "init" is of something
>>> else? What is more, non-negation is easier to understand, so rather
>>> "ibm,active-on-boot" (or something like that).
>>
>> Well, the host processor initializes the OCC during it's boot, but this
>> document is describing a binding to be used by a service processor
>> talking to the OCC. So the OCC may be in any state. The init meant
>> driver init, but I will simply the description and change the property
>> to be more explicit: ibm,no-poll-on-init since that is what is actually
>> happening. Similar to the FSI binding for no-scan-on-init.
> no-scan-on-init is not a good example. It wasn't even reviewed by Rob
> (looking at commit). In both cases you describe driver behavior which is
> not appropriate for bindings. Instead you should describe the hardware
> characteristics/feature/bug/state which require skipping the initialization.


Yep... there is no such hardware thing. The driver should never poll the 
OCC during driver initialization (since host state isn't known), but it 
did for the first couple of years of the drivers existence because we 
didn't catch that it could cause problems. I submitted a patch to change 
the driver behavior, but it does change the user space interface, so the 
argument is that the new behavior should be optional.

I suppose one correct way to control that would be a module parameter, 
but that really doesn't work well on embedded-like systems like ours.

Thanks very much for your feedback Krzysztof. Joel, any suggestions here?

Eddie


>
>
> Best regards,
> Krzysztof