RFC: Support pwm in phosphor-hwmon as fan target

RFC: Support pwm in phosphor-hwmon as fan target

[Lei YU]

The current phosphor-hwmon uses fanX_target for fan speed control. It expects
the fan driver to have `fanX_target` sysfs attribute, e.g. Witherspoon's
max31875 driver.
For the fans with `fanX_target`, phosphor-hwmon creates "Target" property on
the fan sensor Dbus object;
Then phosphor-fan-presence/control sets the "Target" property to do the
thermal control.

Other systems like Romulus do not have this attribute, and it uses `pwmX` to
control the fan speed instead. This is not supported in phosphor-hwmon yet.

To support such case, the proposal is to let phosphor-hwmon to create "Target"
property for the fans controlled by pwmX as well, via hwmon config file.

E.g. for Romulus, fan9, fan11, fan13 are used, where fan9 and fan13 are
controlled by pwm1, and fan11 is controlled by pwm2, then the config looks
like:
```
TARGET_fan9 = "pwm1"
TARGET_fan11 = "pwm2"
```

* When phosphor-hwmon sees TARGET_fanX, it creates "Target" property for this
fan, and use the related pwmX to control the fan speed;
* For the fan control yaml configs, define the relted fan zone and control
logic, where the fan
* phosphor-fan-presence/control uses the same "Target" property to set the
fan speed target, only that the value is range from 0~255.

The phosphor-hwmon changes are submitted at
https://gerrit.openbmc-project.xyz/#/c/8353/

Thanks

--
BRs,
Lei YU

Re: RFC: Support pwm in phosphor-hwmon as fan target

[Patrick Venture]

On Wed, Dec 20, 2017 at 6:09 AM, Lei YU <mine260309@gmail.com> wrote:
> The current phosphor-hwmon uses fanX_target for fan speed control. It expects
> the fan driver to have `fanX_target` sysfs attribute, e.g. Witherspoon's
> max31875 driver.
> For the fans with `fanX_target`, phosphor-hwmon creates "Target" property on
> the fan sensor Dbus object;
> Then phosphor-fan-presence/control sets the "Target" property to do the
> thermal control.
>
> Other systems like Romulus do not have this attribute, and it uses `pwmX` to
> control the fan speed instead. This is not supported in phosphor-hwmon yet.
>
> To support such case, the proposal is to let phosphor-hwmon to create "Target"
> property for the fans controlled by pwmX as well, via hwmon config file.
>
> E.g. for Romulus, fan9, fan11, fan13 are used, where fan9 and fan13 are
> controlled by pwm1, and fan11 is controlled by pwm2, then the config looks
> like:
> ```
> TARGET_fan9 = "pwm1"
> TARGET_fan11 = "pwm2"
> ```

So we have a similar situation however, added a new interface to the
sensor.value for FanPwm control, and then just add that target, but we
were doing name matching identically to the fanspeed targets --
instead of a configuration file setting up the matches.  However, it's
a pretty trivial difference and easy to implement! :D  So, I like
this.

>
> * When phosphor-hwmon sees TARGET_fanX, it creates "Target" property for this
> fan, and use the related pwmX to control the fan speed;
> * For the fan control yaml configs, define the relted fan zone and control
> logic, where the fan
> * phosphor-fan-presence/control uses the same "Target" property to set the
> fan speed target, only that the value is range from 0~255.
>
> The phosphor-hwmon changes are submitted at
> https://gerrit.openbmc-project.xyz/#/c/8353/

I reviewed your changes, and I'd actually like to merge our downstream
patch and yours -- since I think the better way is to not overload
fanspeed as something that can receive RPM or PWM, but rather have the
different interfaces.  This is an important change because then you're
not trying to interpret the value, or if someone wanted to add
fanspeed and control via some other mechanism, they could - - warning,
i haven't had caffeine yet so that might just be rambling.

>
> Thanks
>
> --
> BRs,
> Lei YU

I'm going to submit a couple patches that enable fanpwm partially to
show another approach that I think should be merged with this
approach. :D

Re: RFC: Support pwm in phosphor-hwmon as fan target

[Lei YU]

On Wed, Dec 20, 2017 at 11:49 PM, Patrick Venture <venture@google.com> wrote:
> On Wed, Dec 20, 2017 at 6:09 AM, Lei YU <mine260309@gmail.com> wrote:
>> The current phosphor-hwmon uses fanX_target for fan speed control. It expects
>> the fan driver to have `fanX_target` sysfs attribute, e.g. Witherspoon's
>> max31875 driver.
>> For the fans with `fanX_target`, phosphor-hwmon creates "Target" property on
>> the fan sensor Dbus object;
>> Then phosphor-fan-presence/control sets the "Target" property to do the
>> thermal control.
>>
>> Other systems like Romulus do not have this attribute, and it uses `pwmX` to
>> control the fan speed instead. This is not supported in phosphor-hwmon yet.
>>
>> To support such case, the proposal is to let phosphor-hwmon to create "Target"
>> property for the fans controlled by pwmX as well, via hwmon config file.
>>
>> E.g. for Romulus, fan9, fan11, fan13 are used, where fan9 and fan13 are
>> controlled by pwm1, and fan11 is controlled by pwm2, then the config looks
>> like:
>> ```
>> TARGET_fan9 = "pwm1"
>> TARGET_fan11 = "pwm2"
>> ```
>
> So we have a similar situation however, added a new interface to the
> sensor.value for FanPwm control, and then just add that target, but we
> were doing name matching identically to the fanspeed targets --
> instead of a configuration file setting up the matches.  However, it's
> a pretty trivial difference and easy to implement! :D  So, I like
> this.

Thanks for the comments!
I saw the FanPwm interface, adding a new interface for FanPwm is reasonable,
however, the current phosphor-fan-presence/control uses the interface
"xyz.openbmc_project.Control.FanSpeed" already.
If we have this new FanPwm interface, that part of code needs to be
updated as well.
Also I am not sure what happens if (in case) some fan object implements both
"FanSpeed" and "FanPwm" interface?

>
>>
>> * When phosphor-hwmon sees TARGET_fanX, it creates "Target" property for this
>> fan, and use the related pwmX to control the fan speed;
>> * For the fan control yaml configs, define the relted fan zone and control
>> logic, where the fan
>> * phosphor-fan-presence/control uses the same "Target" property to set the
>> fan speed target, only that the value is range from 0~255.
>>
>> The phosphor-hwmon changes are submitted at
>> https://gerrit.openbmc-project.xyz/#/c/8353/
>
> I reviewed your changes, and I'd actually like to merge our downstream
> patch and yours -- since I think the better way is to not overload
> fanspeed as something that can receive RPM or PWM, but rather have the
> different interfaces.  This is an important change because then you're
> not trying to interpret the value, or if someone wanted to add
> fanspeed and control via some other mechanism, they could - - warning,
> i haven't had caffeine yet so that might just be rambling.
>

Looking forward to your patches.
I wonder how do you config which fan to implement which FanPwm?

>>
>> Thanks
>>
>> --
>> BRs,
>> Lei YU
>
> I'm going to submit a couple patches that enable fanpwm partially to
> show another approach that I think should be merged with this
> approach. :D

Re: RFC: Support pwm in phosphor-hwmon as fan target

[Patrick Venture]

On Wed, Dec 20, 2017 at 6:43 PM, Lei YU <mine260309@gmail.com> wrote:
> On Wed, Dec 20, 2017 at 11:49 PM, Patrick Venture <venture@google.com> wrote:
>> On Wed, Dec 20, 2017 at 6:09 AM, Lei YU <mine260309@gmail.com> wrote:
>>> The current phosphor-hwmon uses fanX_target for fan speed control. It expects
>>> the fan driver to have `fanX_target` sysfs attribute, e.g. Witherspoon's
>>> max31875 driver.
>>> For the fans with `fanX_target`, phosphor-hwmon creates "Target" property on
>>> the fan sensor Dbus object;
>>> Then phosphor-fan-presence/control sets the "Target" property to do the
>>> thermal control.
>>>
>>> Other systems like Romulus do not have this attribute, and it uses `pwmX` to
>>> control the fan speed instead. This is not supported in phosphor-hwmon yet.
>>>
>>> To support such case, the proposal is to let phosphor-hwmon to create "Target"
>>> property for the fans controlled by pwmX as well, via hwmon config file.
>>>
>>> E.g. for Romulus, fan9, fan11, fan13 are used, where fan9 and fan13 are
>>> controlled by pwm1, and fan11 is controlled by pwm2, then the config looks
>>> like:
>>> ```
>>> TARGET_fan9 = "pwm1"
>>> TARGET_fan11 = "pwm2"
>>> ```
>>
>> So we have a similar situation however, added a new interface to the
>> sensor.value for FanPwm control, and then just add that target, but we
>> were doing name matching identically to the fanspeed targets --
>> instead of a configuration file setting up the matches.  However, it's
>> a pretty trivial difference and easy to implement! :D  So, I like
>> this.
>
> Thanks for the comments!
> I saw the FanPwm interface, adding a new interface for FanPwm is reasonable,
> however, the current phosphor-fan-presence/control uses the interface
> "xyz.openbmc_project.Control.FanSpeed" already.
> If we have this new FanPwm interface, that part of code needs to be
> updated as well.

I haven't looked at the control code for that in a while, but
presumably it's output is a value of either RPM or PWM -- which could
be something determined by what dbus interface is available.

> Also I am not sure what happens if (in case) some fan object implements both
> "FanSpeed" and "FanPwm" interface?

In that case, you end up with two control interfaces for the fan.
Which is fine in terms of dbus and support.

>
>>
>>>
>>> * When phosphor-hwmon sees TARGET_fanX, it creates "Target" property for this
>>> fan, and use the related pwmX to control the fan speed;
>>> * For the fan control yaml configs, define the relted fan zone and control
>>> logic, where the fan
>>> * phosphor-fan-presence/control uses the same "Target" property to set the
>>> fan speed target, only that the value is range from 0~255.
>>>
>>> The phosphor-hwmon changes are submitted at
>>> https://gerrit.openbmc-project.xyz/#/c/8353/
>>
>> I reviewed your changes, and I'd actually like to merge our downstream
>> patch and yours -- since I think the better way is to not overload
>> fanspeed as something that can receive RPM or PWM, but rather have the
>> different interfaces.  This is an important change because then you're
>> not trying to interpret the value, or if someone wanted to add
>> fanspeed and control via some other mechanism, they could - - warning,
>> i haven't had caffeine yet so that might just be rambling.
>>
>
> Looking forward to your patches.
> I wonder how do you config which fan to implement which FanPwm?

So we've been lucky, where we just do it the same way the RPM target
works, where it's matched by number.  However, that's why I want to
merge our patches because I like your approach for matching them.

>
>>>
>>> Thanks
>>>
>>> --
>>> BRs,
>>> Lei YU
>>
>> I'm going to submit a couple patches that enable fanpwm partially to
>> show another approach that I think should be merged with this
>> approach. :D