Physical LED Design Proposal

Physical LED Design Proposal

[Jayashree D]

[-- Attachment #1: Type: text/plain, Size: 2363 bytes --]

Hi Team,

Problem Description :

In the existing phosphor-led-sysfs design, it exposes one service per LED.
Therefore, multiple services will be created for multiple GPIO pins
configured for LED. To abstract this method and also to create LEDs under a
single service, a new implementation is proposed.

Existing Implementation :

1. Physical Leds are defined in the device tree under "leds" section.
2. Corresponding GPIO pin are defined for the physical LEDs.
3. "udev rules" are used to monitor the physical LEDs.
4. Once the LED in initialized in device tree, udev event will be created
and it will trigger a systemd service for that LED.
5. Therefore, if multiple GPIO pins are configured for LEDs, then it will
create a multiple systemd services
(xyz.openbmc_project.led.controller@.service)
for phosphor-led-sysfs based on the LED name.

Example :

busctl tree xyz.openbmc_project.LED.Controller.led1
`-/xyz
  `-/xyz/openbmc_project
    `-/xyz/openbmc_project/led
      `-/xyz/openbmc_project/led/physical
        `-/xyz/openbmc_project/led/physical/led1

busctl tree xyz.openbmc_project.LED.Controller.led2
`-/xyz
  `-/xyz/openbmc_project
    `-/xyz/openbmc_project/led
      `-/xyz/openbmc_project/led/physical
        `-/xyz/openbmc_project/led/physical/led2



New Implementation :

1. Physical Leds are defined in the device tree under "leds" section.
2. Corresponding GPIO pin are defined for the physical LEDs.
3. "udev rules" are used to monitor the physical LEDs.
4. Once the udev event is initialized for the LED, it will store those LED
name using the script in udev instead of triggering systemd   service.
5. Phosphor-led-sysfs will have a single systemd service
(xyz.openbmc_project.led.controller.service) running by default at system
startup.
6. A dbus method call will be exposed from the service. udev will notify
the LEDs detected in the driver.

Example :

busctl tree xyz.openbmc_project.LED.Controller
`-/xyz
  `-/xyz/openbmc_project
    `-/xyz/openbmc_project/led
      `-/xyz/openbmc_project/led/physical
        `-/xyz/openbmc_project/led/physical/led1
        `-/xyz/openbmc_project/led/physical/led2


This was already discussed in the previous mail thread. Please refer to the
below link.
https://lists.ozlabs.org/pipermail/openbmc/2022-April/030272.html

Please provide your suggestions on this new proposal.


Thanks
Jayashree

[-- Attachment #2: Type: text/html, Size: 2824 bytes --]

Re: Physical LED Design Proposal

[Jayashree D]

[-- Attachment #1: Type: text/plain, Size: 2667 bytes --]

Hi Team,

Could you please provide your suggestions on the above design for LED.

Thanks,
Jayashree


On Fri, May 27, 2022 at 12:42 PM Jayashree D <srid.11486@gmail.com> wrote:

> Hi Team,
>
> Problem Description :
>
> In the existing phosphor-led-sysfs design, it exposes one service per LED.
> Therefore, multiple services will be created for multiple GPIO pins
> configured for LED. To abstract this method and also to create LEDs under a
> single service, a new implementation is proposed.
>
> Existing Implementation :
>
> 1. Physical Leds are defined in the device tree under "leds" section.
> 2. Corresponding GPIO pin are defined for the physical LEDs.
> 3. "udev rules" are used to monitor the physical LEDs.
> 4. Once the LED in initialized in device tree, udev event will be created
> and it will trigger a systemd service for that LED.
> 5. Therefore, if multiple GPIO pins are configured for LEDs, then it will
> create a multiple systemd services (xyz.openbmc_project.led.controller@.service)
> for phosphor-led-sysfs based on the LED name.
>
> Example :
>
> busctl tree xyz.openbmc_project.LED.Controller.led1
> `-/xyz
>   `-/xyz/openbmc_project
>     `-/xyz/openbmc_project/led
>       `-/xyz/openbmc_project/led/physical
>         `-/xyz/openbmc_project/led/physical/led1
>
> busctl tree xyz.openbmc_project.LED.Controller.led2
> `-/xyz
>   `-/xyz/openbmc_project
>     `-/xyz/openbmc_project/led
>       `-/xyz/openbmc_project/led/physical
>         `-/xyz/openbmc_project/led/physical/led2
>
>
>
> New Implementation :
>
> 1. Physical Leds are defined in the device tree under "leds" section.
> 2. Corresponding GPIO pin are defined for the physical LEDs.
> 3. "udev rules" are used to monitor the physical LEDs.
> 4. Once the udev event is initialized for the LED, it will store those LED
> name using the script in udev instead of triggering systemd   service.
> 5. Phosphor-led-sysfs will have a single systemd service
> (xyz.openbmc_project.led.controller.service) running by default at system
> startup.
> 6. A dbus method call will be exposed from the service. udev will notify
> the LEDs detected in the driver.
>
> Example :
>
> busctl tree xyz.openbmc_project.LED.Controller
> `-/xyz
>   `-/xyz/openbmc_project
>     `-/xyz/openbmc_project/led
>       `-/xyz/openbmc_project/led/physical
>         `-/xyz/openbmc_project/led/physical/led1
>         `-/xyz/openbmc_project/led/physical/led2
>
>
> This was already discussed in the previous mail thread. Please refer to
> the below link.
> https://lists.ozlabs.org/pipermail/openbmc/2022-April/030272.html
>
> Please provide your suggestions on this new proposal.
>
>
> Thanks
> Jayashree
>

[-- Attachment #2: Type: text/html, Size: 3385 bytes --]

Re: Physical LED Design Proposal

[Ed Tanous]

On Tue, Jun 7, 2022 at 12:07 AM Jayashree D <srid.11486@gmail.com> wrote:
>
> Hi Team,
>
> Could you please provide your suggestions on the above design for LED.
>
> Thanks,
> Jayashree
>
>
> On Fri, May 27, 2022 at 12:42 PM Jayashree D <srid.11486@gmail.com> wrote:
>>
>> Hi Team,
>>
>> Problem Description :
>>
>> In the existing phosphor-led-sysfs design, it exposes one service per LED. Therefore, multiple services will be created for multiple GPIO pins configured for LED. To abstract this method and also to create LEDs under a single service, a new implementation is proposed.

You've kind of jumped directly to a solution without spending any
details on why this design is necessary.  What are you trying to
achieve?  Why does the existing solution not work?  You allude to
multiple services being bad, but you don't really state why, or what's
preventing that from working.  This is a section labeled problem
description, it needs to describe the problem, ideally in much more
length than your solution itself.

>>
>> Existing Implementation :
>>
>> 1. Physical Leds are defined in the device tree under "leds" section.
>> 2. Corresponding GPIO pin are defined for the physical LEDs.
>> 3. "udev rules" are used to monitor the physical LEDs.
>> 4. Once the LED in initialized in device tree, udev event will be created and it will trigger a systemd service for that LED.
>> 5. Therefore, if multiple GPIO pins are configured for LEDs, then it will create a multiple systemd services (xyz.openbmc_project.led.controller@.service) for phosphor-led-sysfs based on the LED name.
>>
>> Example :
>>
>> busctl tree xyz.openbmc_project.LED.Controller.led1
>> `-/xyz
>>   `-/xyz/openbmc_project
>>     `-/xyz/openbmc_project/led
>>       `-/xyz/openbmc_project/led/physical
>>         `-/xyz/openbmc_project/led/physical/led1
>>
>> busctl tree xyz.openbmc_project.LED.Controller.led2
>> `-/xyz
>>   `-/xyz/openbmc_project
>>     `-/xyz/openbmc_project/led
>>       `-/xyz/openbmc_project/led/physical
>>         `-/xyz/openbmc_project/led/physical/led2
>>
>>
>>
>> New Implementation :
>>
>> 1. Physical Leds are defined in the device tree under "leds" section.
>> 2. Corresponding GPIO pin are defined for the physical LEDs.
>> 3. "udev rules" are used to monitor the physical LEDs.
>> 4. Once the udev event is initialized for the LED, it will store those LED name using the script in udev instead of triggering systemd   service.
>> 5. Phosphor-led-sysfs will have a single systemd service (xyz.openbmc_project.led.controller.service) running by default at system startup.
>> 6. A dbus method call will be exposed from the service. udev will notify the LEDs detected in the driver.
>>
>> Example :
>>
>> busctl tree xyz.openbmc_project.LED.Controller
>> `-/xyz
>>   `-/xyz/openbmc_project
>>     `-/xyz/openbmc_project/led
>>       `-/xyz/openbmc_project/led/physical
>>         `-/xyz/openbmc_project/led/physical/led1
>>         `-/xyz/openbmc_project/led/physical/led2
>>
>>
>> This was already discussed in the previous mail thread. Please refer to the below link.
>> https://lists.ozlabs.org/pipermail/openbmc/2022-April/030272.html
>>
>> Please provide your suggestions on this new proposal.
>>
>>
>> Thanks
>> Jayashree

Re: Physical LED Design Proposal

[Jayashree D]

[-- Attachment #1: Type: text/plain, Size: 4315 bytes --]

On Tue, Jun 7, 2022 at 9:41 PM Ed Tanous <edtanous@google.com> wrote:

> On Tue, Jun 7, 2022 at 12:07 AM Jayashree D <srid.11486@gmail.com> wrote:
> >
> > Hi Team,
> >
> > Could you please provide your suggestions on the above design for LED.
> >
> > Thanks,
> > Jayashree
> >
> >
> > On Fri, May 27, 2022 at 12:42 PM Jayashree D <srid.11486@gmail.com>
> wrote:
> >>
> >> Hi Team,
> >>
> >> Problem Description :
> >>
> >> In the existing phosphor-led-sysfs design, it exposes one service per
> LED. Therefore, multiple services will be created for multiple GPIO pins
> configured for LED. To abstract this method and also to create LEDs under a
> single service, a new implementation is proposed.
>
> You've kind of jumped directly to a solution without spending any
> details on why this design is necessary.  What are you trying to
> achieve?  Why does the existing solution not work?  You allude to
> multiple services being bad, but you don't really state why, or what's
> preventing that from working.  This is a section labeled problem
> description, it needs to describe the problem, ideally in much more
> length than your solution itself.
>
>  The Yosemite V2 Platform combines a Power LED and a System Identification
LED into a single bicolor blue-yellow LED per host.
A total of 4 × LEDs will be placed along the front edge of the board in a
grid.
The grid will be 2×rows of 2 × LEDs to match the layout of the card slots.

Based on each host status, blue or yellow led needs to be blink, OFF or ON.
Therefore, bi-color led needs to be paired as a group and exposed in the
userspace.

Based on the existing implementation in phopshor-led-sysfs, pairing groups
will be difficult, since it exposes one service per LED.

Therefore, refactoring the phosphor-led-sysfs repo to run under a single
service and pair a group of LED which represents each host.

>>
> >> Existing Implementation :
> >>
> >> 1. Physical Leds are defined in the device tree under "leds" section.
> >> 2. Corresponding GPIO pin are defined for the physical LEDs.
> >> 3. "udev rules" are used to monitor the physical LEDs.
> >> 4. Once the LED in initialized in device tree, udev event will be
> created and it will trigger a systemd service for that LED.
> >> 5. Therefore, if multiple GPIO pins are configured for LEDs, then it
> will create a multiple systemd services (xyz.openbmc_project.led.controller@.service)
> for phosphor-led-sysfs based on the LED name.
> >>
> >> Example :
> >>
> >> busctl tree xyz.openbmc_project.LED.Controller.led1
> >> `-/xyz
> >>   `-/xyz/openbmc_project
> >>     `-/xyz/openbmc_project/led
> >>       `-/xyz/openbmc_project/led/physical
> >>         `-/xyz/openbmc_project/led/physical/led1
> >>
> >> busctl tree xyz.openbmc_project.LED.Controller.led2
> >> `-/xyz
> >>   `-/xyz/openbmc_project
> >>     `-/xyz/openbmc_project/led
> >>       `-/xyz/openbmc_project/led/physical
> >>         `-/xyz/openbmc_project/led/physical/led2
> >>
> >>
> >>
> >> New Implementation :
> >>
> >> 1. Physical Leds are defined in the device tree under "leds" section.
> >> 2. Corresponding GPIO pin are defined for the physical LEDs.
> >> 3. "udev rules" are used to monitor the physical LEDs.
> >> 4. Once the udev event is initialized for the LED, it will store those
> LED name using the script in udev instead of triggering systemd   service.
> >> 5. Phosphor-led-sysfs will have a single systemd service
> (xyz.openbmc_project.led.controller.service) running by default at system
> startup.
> >> 6. A dbus method call will be exposed from the service. udev will
> notify the LEDs detected in the driver.
> >>
> >> Example :
> >>
> >> busctl tree xyz.openbmc_project.LED.Controller
> >> `-/xyz
> >>   `-/xyz/openbmc_project
> >>     `-/xyz/openbmc_project/led
> >>       `-/xyz/openbmc_project/led/physical
> >>         `-/xyz/openbmc_project/led/physical/led1
> >>         `-/xyz/openbmc_project/led/physical/led2
> >>
> >>
> >> This was already discussed in the previous mail thread. Please refer to
> the below link.
> >> https://lists.ozlabs.org/pipermail/openbmc/2022-April/030272.html
> >>
> >> Please provide your suggestions on this new proposal.
> >>
> >>
> >> Thanks
> >> Jayashree
>

[-- Attachment #2: Type: text/html, Size: 5679 bytes --]

Re: Physical LED Design Proposal

[Jayashree D]

[-- Attachment #1: Type: text/plain, Size: 4631 bytes --]

 Hi Team,

 Could you please provide your suggestions on the design for LED.

 Thanks,
 Jayashree

On Wed, Jun 8, 2022 at 7:42 PM Jayashree D <srid.11486@gmail.com> wrote:

>
>
> On Tue, Jun 7, 2022 at 9:41 PM Ed Tanous <edtanous@google.com> wrote:
>
>> On Tue, Jun 7, 2022 at 12:07 AM Jayashree D <srid.11486@gmail.com> wrote:
>> >
>> > Hi Team,
>> >
>> > Could you please provide your suggestions on the above design for LED.
>> >
>> > Thanks,
>> > Jayashree
>> >
>> >
>> > On Fri, May 27, 2022 at 12:42 PM Jayashree D <srid.11486@gmail.com>
>> wrote:
>> >>
>> >> Hi Team,
>> >>
>> >> Problem Description :
>> >>
>> >> In the existing phosphor-led-sysfs design, it exposes one service per
>> LED. Therefore, multiple services will be created for multiple GPIO pins
>> configured for LED. To abstract this method and also to create LEDs under a
>> single service, a new implementation is proposed.
>>
>> You've kind of jumped directly to a solution without spending any
>> details on why this design is necessary.  What are you trying to
>> achieve?  Why does the existing solution not work?  You allude to
>> multiple services being bad, but you don't really state why, or what's
>> preventing that from working.  This is a section labeled problem
>> description, it needs to describe the problem, ideally in much more
>> length than your solution itself.
>>
>>  The Yosemite V2 Platform combines a Power LED and a System
> Identification LED into a single bicolor blue-yellow LED per host.
> A total of 4 × LEDs will be placed along the front edge of the board in a
> grid.
> The grid will be 2×rows of 2 × LEDs to match the layout of the card slots.
>
> Based on each host status, blue or yellow led needs to be blink, OFF or
> ON. Therefore, bi-color led needs to be paired as a group and exposed in
> the userspace.
>
> Based on the existing implementation in phopshor-led-sysfs, pairing groups
> will be difficult, since it exposes one service per LED.
>
> Therefore, refactoring the phosphor-led-sysfs repo to run under a single
> service and pair a group of LED which represents each host.
>
> >>
>> >> Existing Implementation :
>> >>
>> >> 1. Physical Leds are defined in the device tree under "leds" section.
>> >> 2. Corresponding GPIO pin are defined for the physical LEDs.
>> >> 3. "udev rules" are used to monitor the physical LEDs.
>> >> 4. Once the LED in initialized in device tree, udev event will be
>> created and it will trigger a systemd service for that LED.
>> >> 5. Therefore, if multiple GPIO pins are configured for LEDs, then it
>> will create a multiple systemd services (xyz.openbmc_project.led.controller@.service)
>> for phosphor-led-sysfs based on the LED name.
>> >>
>> >> Example :
>> >>
>> >> busctl tree xyz.openbmc_project.LED.Controller.led1
>> >> `-/xyz
>> >>   `-/xyz/openbmc_project
>> >>     `-/xyz/openbmc_project/led
>> >>       `-/xyz/openbmc_project/led/physical
>> >>         `-/xyz/openbmc_project/led/physical/led1
>> >>
>> >> busctl tree xyz.openbmc_project.LED.Controller.led2
>> >> `-/xyz
>> >>   `-/xyz/openbmc_project
>> >>     `-/xyz/openbmc_project/led
>> >>       `-/xyz/openbmc_project/led/physical
>> >>         `-/xyz/openbmc_project/led/physical/led2
>> >>
>> >>
>> >>
>> >> New Implementation :
>> >>
>> >> 1. Physical Leds are defined in the device tree under "leds" section.
>> >> 2. Corresponding GPIO pin are defined for the physical LEDs.
>> >> 3. "udev rules" are used to monitor the physical LEDs.
>> >> 4. Once the udev event is initialized for the LED, it will store those
>> LED name using the script in udev instead of triggering systemd   service.
>> >> 5. Phosphor-led-sysfs will have a single systemd service
>> (xyz.openbmc_project.led.controller.service) running by default at system
>> startup.
>> >> 6. A dbus method call will be exposed from the service. udev will
>> notify the LEDs detected in the driver.
>> >>
>> >> Example :
>> >>
>> >> busctl tree xyz.openbmc_project.LED.Controller
>> >> `-/xyz
>> >>   `-/xyz/openbmc_project
>> >>     `-/xyz/openbmc_project/led
>> >>       `-/xyz/openbmc_project/led/physical
>> >>         `-/xyz/openbmc_project/led/physical/led1
>> >>         `-/xyz/openbmc_project/led/physical/led2
>> >>
>> >>
>> >> This was already discussed in the previous mail thread. Please refer
>> to the below link.
>> >> https://lists.ozlabs.org/pipermail/openbmc/2022-April/030272.html
>> >>
>> >> Please provide your suggestions on this new proposal.
>> >>
>> >>
>> >> Thanks
>> >> Jayashree
>>
>

[-- Attachment #2: Type: text/html, Size: 6188 bytes --]

Re: Physical LED Design Proposal

[Andrew Jeffery]

Hi Jayashree,

First up, apologies for the extreme length of time it took me to get to this.

On Fri, 27 May 2022, at 16:42, Jayashree D wrote:
> Hi Team,
>
> Problem Description :
>
> In the existing phosphor-led-sysfs design, it exposes one service per LED.
> Therefore, multiple services will be created for multiple GPIO pins
> configured for LED. To abstract this method and also to create LEDs under a
> single service, a new implementation is proposed.
>
> Existing Implementation :
>
> 1. Physical Leds are defined in the device tree under "leds" section.
> 2. Corresponding GPIO pin are defined for the physical LEDs.
> 3. "udev rules" are used to monitor the physical LEDs.
> 4. Once the LED in initialized in device tree, udev event will be created
> and it will trigger a systemd service for that LED.
> 5. Therefore, if multiple GPIO pins are configured for LEDs, then it will
> create a multiple systemd services
> (xyz.openbmc_project.led.controller@.service)
> for phosphor-led-sysfs based on the LED name.
>
> Example :
>
> busctl tree xyz.openbmc_project.LED.Controller.led1
> `-/xyz
>   `-/xyz/openbmc_project
>     `-/xyz/openbmc_project/led
>       `-/xyz/openbmc_project/led/physical
>         `-/xyz/openbmc_project/led/physical/led1
>
> busctl tree xyz.openbmc_project.LED.Controller.led2
> `-/xyz
>   `-/xyz/openbmc_project
>     `-/xyz/openbmc_project/led
>       `-/xyz/openbmc_project/led/physical
>         `-/xyz/openbmc_project/led/physical/led2
>
>
>
> New Implementation :
>
> 1. Physical Leds are defined in the device tree under "leds" section.
> 2. Corresponding GPIO pin are defined for the physical LEDs.
> 3. "udev rules" are used to monitor the physical LEDs.
> 4. Once the udev event is initialized for the LED, it will store those LED
> name using the script in udev instead of triggering systemd   service.
> 5. Phosphor-led-sysfs will have a single systemd service
> (xyz.openbmc_project.led.controller.service) running by default at system
> startup.
> 6. A dbus method call will be exposed from the service. udev will notify
> the LEDs detected in the driver.
>
> Example :
>
> busctl tree xyz.openbmc_project.LED.Controller
> `-/xyz
>   `-/xyz/openbmc_project
>     `-/xyz/openbmc_project/led
>       `-/xyz/openbmc_project/led/physical
>         `-/xyz/openbmc_project/led/physical/led1
>         `-/xyz/openbmc_project/led/physical/led2
>
>
> This was already discussed in the previous mail thread. Please refer to the
> below link.
> https://lists.ozlabs.org/pipermail/openbmc/2022-April/030272.html
>
> Please provide your suggestions on this new proposal.

This looks good to me.

Thanks.

Andrew

Re: Physical LED Design Proposal

[Andrew Jeffery]

On Wed, 8 Jun 2022, at 23:42, Jayashree D wrote:
> On Tue, Jun 7, 2022 at 9:41 PM Ed Tanous <edtanous@google.com> wrote:
>
>> On Tue, Jun 7, 2022 at 12:07 AM Jayashree D <srid.11486@gmail.com> wrote:
>> >
>> > Hi Team,
>> >
>> > Could you please provide your suggestions on the above design for LED.
>> >
>> > Thanks,
>> > Jayashree
>> >
>> >
>> > On Fri, May 27, 2022 at 12:42 PM Jayashree D <srid.11486@gmail.com>
>> wrote:
>> >>
>> >> Hi Team,
>> >>
>> >> Problem Description :
>> >>
>> >> In the existing phosphor-led-sysfs design, it exposes one service per
>> LED. Therefore, multiple services will be created for multiple GPIO pins
>> configured for LED. To abstract this method and also to create LEDs under a
>> single service, a new implementation is proposed.
>>
>> You've kind of jumped directly to a solution without spending any
>> details on why this design is necessary.  What are you trying to
>> achieve?  Why does the existing solution not work?  You allude to
>> multiple services being bad, but you don't really state why, or what's
>> preventing that from working.  This is a section labeled problem
>> description, it needs to describe the problem, ideally in much more
>> length than your solution itself.
>>
>>  The Yosemite V2 Platform combines a Power LED and a System Identification
> LED into a single bicolor blue-yellow LED per host.
> A total of 4 × LEDs will be placed along the front edge of the board in a
> grid.
> The grid will be 2×rows of 2 × LEDs to match the layout of the card slots.
>
> Based on each host status, blue or yellow led needs to be blink, OFF or ON.
> Therefore, bi-color led needs to be paired as a group and exposed in the
> userspace.
>
> Based on the existing implementation in phopshor-led-sysfs, pairing groups
> will be difficult, since it exposes one service per LED.
>
> Therefore, refactoring the phosphor-led-sysfs repo to run under a single
> service and pair a group of LED which represents each host.

It's important that you integrate this justification into your proposal.

Regardless, the proposal should also improve performance on systems 
that were tracking a large number of LEDs. Some p10bmc platforms suffer 
here.

Andrew