[U-Boot] [RFC] Add 'led' command
Ulf Samuelsson
aprichelieu at gmail.com
Fri Dec 16 08:58:11 CET 2011
I think that before we go ahead and do changes, we should ask ourselves:
Why does a bootloader need LED support?
Just because its on the board?
Do we need to support all LEDs because they are there?
i implemented the LED command because it seemed to have some application,
even though I could not think of any specific use at the time.
Would be curious to find out what people would use the LED function for, except for debugging.
Before that is known, it is hard to Judge if a change is warranted.
Admit that using green_LED_on for my second red LED on the bord
has some drawbacks, but how do you make a generalized command.
"led 1 on" sucks as a user interface.
"led red on" is much much better.
Implementing a change which results in broken boards support is a no no.
IMHO it also has very little incremental value to do the modifications.
Best Regards
Ulf Samuelsson
ulf_samuelsson at telia.com
+46 (722) 427 437
15 dec 2011 kl. 19:17 skrev Jason Kridner <jkridner at beagleboard.org>:
> I hope I'm not too far off from understanding the comments made at the
> end of this thread...
>
> On Wed, Dec 14, 2011 at 4:31 PM, Simon Glass <sjg at chromium.org> wrote:
>> Hi,
>>
>> On Wed, Dec 14, 2011 at 11:38 AM, Ulf Samuelsson
>> <ulf_samuelsson at telia.com> wrote:
>>> On 2011-12-14 20:11, Simon Glass wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Tue, Dec 13, 2011 at 3:55 PM, Ulf Samuelsson
>>>> <ulf_samuelsson at telia.com> wrote:
>>>>>
>>>>> On 2010-11-05 13:21, Wolfgang Denk wrote:
>>>>>>
>>>>>> Dear Jason Kridner,
>>>>>>
>>>>>> In message<1288936236-30603-1-git-send-email-jkridner at beagleboard.org>
>>>>>> you wrote:
>>>>>>>
>>>>>>> It is desired to have the led command on the BeagleBoard to allow for
>>>>>>> some
>>>>>>> interaction in the scripts.
>>>>>>>
>>>>>>> This patch allows any board implementing the coloured LED API
>>>>>>> to control the LEDs from the console.
>>>>>>>
>>>>>>> led [green | yellow | red | all ] [ on | off ]
>>>>>>>
>>>>>>> or
>>>>>>>
>>>>>>> led [ 1 | 2 | 3 | all ] [ on | off ]
>>>>>>>
>>>>>>> Adds configuration item CONFIG_CMD_LED enabling the command.
>>>>>>>
>>>>>>> Partially based on patch from Ulf Samuelsson:
>>>>>>> http://www.mail-archive.com/u-boot@lists.denx.de/msg09593.html.
>>>>>>>
>>>>>>> Signed-off-by: Jason Kridner<jkridner at beagleboard.org>
>>>>>>> ---
>>>>>>> common/Makefile | 1 +
>>>>>>> common/cmd_led.c | 207
>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>> 2 files changed, 208 insertions(+), 0 deletions(-)
>>>>>>> create mode 100644 common/cmd_led.c
>>>>>>
>>>>>> I understand the requirement, but I think it is more than time to come
>>>>>> up with a common solution here instead of adding more and more copies
>>>>>> of very similar code.
>>>>>>
>>>>>> We already have:
>>>>>>
>>>>>> arch/arm/cpu/arm920t/ep93xx/led.c
>>>>>
>>>>> The file below is mapping the led commands I.E: red_led_on
>>>>> to at91 I/O calls like at91_set_gpio_value.
>>>>> A merge, means that a common way of setting GPIO must be available
>>>>>
>>>>>> arch/arm/cpu/arm926ejs/at91/led.c
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> The only thing the files below do are to initialize the pins for the LED.
>>>>> While the actual pins are hidden in the configs,
>>>>> you have a variation of which LEDs are available.
>>>>> Also you need to enable different clocks.
>>>>> It is really an extension of the board init file.
>>>>> Maybe they should be merged with the board file instead.
>>>>> I personally don't see a problem with keeping the separate file.
>>>>>
>>>>> There is no commonality between these files and the led.c
>>>>> in "arch/arm/cpu/arm926ejs/at91/led.c"
>>>>>
>>>>>
>>>>>> board/atmel/at91cap9adk/led.c
>>>>>> board/atmel/at91rm9200dk/led.c
>>>>>> board/atmel/at91rm9200ek/led.c
>>>>>> board/atmel/at91sam9260ek/led.c
>>>>>> board/atmel/at91sam9261ek/led.c
>>>>>> board/atmel/at91sam9263ek/led.c
>>>>>> board/atmel/at91sam9m10g45ek/led.c
>>>>>> board/atmel/at91sam9rlek/led.c
>>>>>> board/ronetix/pm9261/led.c
>>>>>> board/ronetix/pm9263/led.c
>>>>>> board/eukrea/cpu9260/led.c
>>>>>
>>>>>
>>>>>
>>>>>> board/logicpd/zoom2/led.c
>>>>>> board/ns9750dev/led.c
>>>>>> board/psyent/pk1c20/led.c
>>>>>>
>>>>>> Please, let's unify these.
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>> Wolfgang Denk
>>>>>>
>>>>> The proposed patch is adding a command, and
>>>>> which uses the coloured LED interface and there
>>>>> is no commonality between this code and
>>>>> the code in the board and cpu directories.
>>>>
>>>> IMO this LED interface is not very nice. Is there a reason there is
>>>> not just one function? Perhaps like:
>>>>
>>>> enum led_colour {
>>>> LED_GREEN,
>>>> LED_YELLOW,
>>>> LED_RED,
>>>> LED_BLUE,
>>>> };
>>>>
>>>> void led_init(void);
>>>>
>>>> /**
>>>> * Set the state of a coloured LED
>>>> *
>>>> * @param led LED to adjust
>>>> * @param on 1 to turn it on, 0 to turn it off
>>>> */
>>>> void led_set_state(enum led_colour led, int on);
>
> The concept of selecting an LED by color seems broken to me. Boards
> may have many LEDs of the same color.
>
> There is value in having additional meta-data regarding an LED, such
> as its color, location, silk-screen text or anticipated use. It seems
> to me all of that should be ancillary to the index of the LED. The
> information to control the LED, such as the GPIO pin or associated
> register, could further be stored in the data structure associated
> with an LED index.
>
> The only reason I implemented a function with the existing API was
> that the API was established across several platforms. Trying to find
> time to dedicate to cleaning up those several implementations without
> having any of the hardware (meaning lots of communications to other
> folks rather than just making the code work) is difficult, but I'm
> happy to contribute code fragments/patches if there is interest.
>
> My approach would be to provide an array of structures to the command
> that is initialized by the board file that contains the meta data for
> the LED and pointers to the functions that perform the required
> manipulation. Default functions would be the platform GPIO functions.
> The array would also have the required arguments for those
> manipulation functions, ie. the pin numbers to pass to the GPIO
> functions.
>
> The base definition is the easy part. The hard part is following up
> with all of the platforms and still making sure that the super-simple
> assembly functions can work efficiently.
>
>
>>>>
>>>> instead of:
>>>>
>>>> extern void coloured_LED_init (void);
>>>> extern void red_LED_on(void);
>>>> extern void red_LED_off(void);
>>>> extern void green_LED_on(void);
>>>> extern void green_LED_off(void);
>>>> extern void yellow_LED_on(void);
>>>> extern void yellow_LED_off(void);
>>>> extern void blue_LED_on(void);
>>>> extern void blue_LED_off(void);
>>>>
>>>> and associated weak symbols.
>>>>
>>>> It may even be possible to tidy up the existing status_led.h file at
>>>> the same time.
>>>
>>> The reason is simple.
>>> The API was developed to figure out why u-boot never reached the prompt.
>>> The API should be so simple to use in assembler that it did not introduce
>>> further bugs.
>>>
>>> In assembler the call maps to something like (IIRC):
>>> bl green_LED_on
>>>
>>> Much easier than passing parameters in registers.
>>
>> Yes I see what you mean
>>
>> mov r0, #LED_GREEN
>> mov r1, #1
>> bl led_set_state
>>
>> presumably you have r0 and r1 used for other things. Still it seems
>> painful to subject board.c to this very assembler-specific API.
>> Perhaps the current API could be relegated to a driver somewhere.
>
> In order to enable integrating such simple, well-optimized functions
> into an LED command, would it work to pass pointers to those functions
> in through a data structure configured by the board.c file?
>
> I see the other big challenge being low-level reuse of the LED
> functions. Managing allocation of the LEDs to those functions, such
> as network activity or heartbeats, and away from the LED command seems
> like a necessary task for the board.c file as well. Is there a better
> resource management concept we can work from?
>
>>
>>> There are other advantages of doing it your way of course.
>>>
>>> If you want to change, make sure you don't break any boards.
>>
>> I am cleaning this up as part of the new board.c effort, waiting on
>> some responses on relocation still.
>>
>> Regards,
>> Simon
>>
>>>
>>> BR
>>> Ulf Samuelsson
>>>
>>>
>>>> I agree that the command is sort-of sideways, but really I think this
>>>> should be cleaned up before adding more code that uses the API. It
>>>> just makes the job harder.
>>>>
>>>> Regards,
>>>> Simon
>>>>
>>>>> --
>>>>> Best Regards
>>>>> Ulf Samuelsson
>>>>>
>>>>> _______________________________________________
>>>>> U-Boot mailing list
>>>>> U-Boot at lists.denx.de
>>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>
>>>
>>>
>>> --
>>> Best Regards
>>> Ulf Samuelsson
>>> +46 722 427437
>>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
More information about the U-Boot
mailing list