[PATCH] Beagle BSP Improvements (GPIO driver)

Fri Apr 24 20:03:07 UTC 2015

Hi All,

First time emailing this list.

I'll chime in as a interested party with development experience.

I agree wholeheartedly with Joel.

Having a single API for all GPIO access would be great.

A few additional things to consider:

   - Sometimes applications will want 'blindingly fast' access to GPIO. 
Some abstractions may add too many levels of indirection in order to be 
generic enough. It might be worthwhile making the generic RTEMS API as 
generic as possible, but still making an effort to make a consistent low 
level API that uses trivial inline functions using address and bit 
manipulation.

   - Another potential issues is atomic writing of individual bits. A 
gerneric RTEMS API would be nice. Of course, only some hardware supports 
atomic bit sets, so unsupported hardware would have to disable 
interrupts/lock the scheduler (1).

There are some good thoughts about this topic here: 
http://www.ethernut.de/nutwiki/Unified_GPIO_Implementation

They raise the point that some architectures have special properties you 
can set on IO pins, such as pullup strength, drive mode (Open collector, 
push pull, etc). This could be exposed via a 
rtems_gpio_board_specific_config(...). I haven't given this as much thought.

Isaac

On 04/24/2015 01:11 PM, Joel Sherrill wrote:
>
>
> On 4/24/2015 11:48 AM, Ben Gras wrote:
>> All,
>>
>> Big improvement. The video shows that the bit logic for setting &
>> clearing is likely sound - for the first byte at least ;-). Good test.
>>
>> My comments about the code are
>>    - gpio_select_pin write a whole 0 byte instead of updating a single
>> bit, as we talked about on IRC
>>    - gedare's suggestion to use a single function to update bits in
>> registers is good - you can use this for gpio_select_pin, gpio_set and
>> gpio_clear.
>>    - i don't like the addition of arm_delay() in the bsp code. if it's
>> not bsp-specific (and it isn't), it shouldn't be there.
>>    - the AM335X-specific code isn't made conditional
>>
>> My comment about the API that is:
>>
>> I don't like it that hardware-specific info (GPIO base registers) is
>> somewhat external to the BSP. To my mind, the application should only
>> deal with bank numbers and pin numbers. So either we need 128 more
>> #defines of the GPIO_PIN struct in the current vein, meaning inline
>> struct initializers are passed around (ew) and we rely on quite a lot
>> of app discipline to keep it clean (ew); or we need an api like
>>
>> gpio_config(&gpio_conf);  /* retrieve number of gpio banks & pins per
>> bank, possibly more info */
>> gpio_select_pin(&gpio_pin, bank_number, pin_number, DIGITAL_OUTPUT);
>> /* fill in gpio_pin struct to use this pin */
>> gpio_set(&gpio_pin);
>> gpio_clear(&gpio_pin);
>> gpio_release_pin(&gpio_pin);  /* allows repurposing this pin */
>>
>> this way the configuring of gpio_pin (containing hardware-specific
>> info) is more internal to the BSP, and the app would have to actively
>> subvert the api (look in the gpio_pin struct) and is less prone to be
>> written badly by accident (hardcoding hardware addresses in the app).
>>
>> Thoughts anyone?
> We need a generic rtems_gpio_XXX API set. That was what I took at initial
> shot at with the multio code. I started mentally with separate APIs
> completely for analog and discretes but since I was handed a baseboard
> with 32 bit GPIO pins and an add-on board with ADCs, DACs, and more
> GPIOs, I needed a unified interface. The collection of various IO types
> on a board is common.
>
> The application should be completely independent of the BSP. My
> app will have to know that "the red warning light" is bank 1, pin 3
> on board X and bank 7, pin 12 when using another board. That
> is an application issue. The BSP/system configuration has to
> present a single API to the app.
>
> Remember that the system integrator may add boards. So the
> "system configuration" may include things not on the baseboard.
> The SIGAda 2009 presentation I sent privately a few days ago
> had that configuration and was what I did the API work I did for.
>>
>>
>> On Thu, Apr 23, 2015 at 8:35 PM, Ketul Shah <ketulshah1993 at gmail.com> wrote:
>>> Hi all,
>>>
>>> Ben Gras thanks for your comments and encouragement for merging with
>>> mainline as a motivation for me.
>>>
>>> I worked and redeveloped code for gpio driver also tried to approach towards
>>> common api. I tried to address most of the comments. Herewith I attached my
>>> patch. Would be happy to hear comments on that.
>>>
>>> You can also find out my gist or commit on https://github.com/RTEMS/rtems .
>>> I have been updating my work on https://github.com/ketul93/RTEMS-on-BBB.
>>>
>>> Live demo of the updated code is found on :- https://youtu.be/bXurelOA3i0
>>>
>>> Thanks.
>>>
>>> Regards,
>>> ketul
>>>
>>>
>>> On 20 April 2015 at 19:50, Ben Gras <beng at shrike-systems.com> wrote:
>>>> All,
>>>>
>>>> Good contribution, thank you.
>>>>
>>>> For GSOC, this is good proof of being able to progress and get
>>>> something real working based on documentation. Great!
>>>>
>>>> If you want this merged with mainline - which I fully encourage! -
>>>> then I suggest the following should be added to/changed first:
>>>>
>>>>    - make the code that uses AM335X (beaglebone) specific registers,
>>>> AM335X-specific :) i.e. add  #if IS_AM335X around code that should
>>>> only execute there. this BSP can be compiled for 2 different SOCs.
>>>>    - let it control all GPIO's - there are 4 banks of 32 each on the
>>>> AM335x, but you only let the user control GPIO1. there should be a
>>>> clean interface to control them all (clean means mostly: without
>>>> duplicating code)
>>>>    - as Gedare mentioned on IRC, copy the RPi API. this is a first
>>>> (second?) step to finding a generalized GPIO API.
>>>>    - don't add beagleboard.h, but add your definitions to
>>>> libcpu/arm/shared/include/am335x.h
>>>>    - the code should use a more consistent indenting style, and make
>>>> variable names more descriptive than 'tmp'.
>>>>    - don't leave the configure changes in like in acinclude.m4
>>>>
>>>> bonus:
>>>>    - add DM3730 (beagleboard) support.
>>>>
>>>> Good luck!
>>>>
>>>>
>>>> On Fri, Apr 17, 2015 at 7:51 PM, Ketul Shah <ketulshah1993 at gmail.com>
>>>> wrote:
>>>>> Hello,
>>>>>
>>>>> I have proposed in GSoC on Beagle BSP Improvements. As starting I
>>>>> started
>>>>> working for gpio driver development . At this stage I am able to
>>>>> demonstrate
>>>>> USR LEDs pattern.
>>>>>
>>>>> Videos of that can be found here on YouTube.
>>>>> https://youtu.be/B3mSsfo-PAQ &
>>>>> https://youtu.be/M1aKpOhUvv4.
>>>>>
>>>>> Herewith I have attached patch.txt file. Alternatively you can have
>>>>> https://gist.github.com/ketul93/2e0d2c4b8b586be62e1d that includes the
>>>>> newly
>>>>> added files code. I would be happy to hear your reviews and changes on
>>>>> my
>>>>> work.
>>>>>
>>>>> And also I have been updating my work on
>>>>> https://github.com/ketul93/RTEMS-on-BBB repo.
>>>>>
>>>>> It would be great to have your comments on my proposal.
>>>>>
>>>>> Thanks and regards,
>>>>> ketul
>>>>>
>>>>> _______________________________________________
>>>>> devel mailing list
>>>>> devel at rtems.org
>>>>> http://lists.rtems.org/mailman/listinfo/devel
>>>
>> _______________________________________________
>> devel mailing list
>> devel at rtems.org
>> http://lists.rtems.org/mailman/listinfo/devel
>

-- 
Isaac Gutekunst
Embedded Systems Software Engineer
isaac.gutekunst at vecna.com
www.vecna.com