[PATCH] microblaze: Add AXI GPIO driver

[Chris Johns]

Alex,

How do you handle single or dual GPIO channel configurations?

How do you handle 1 to 32 GPIO pins?

On 17/3/2022 3:25 am, Alex White wrote:
> On Wed, Mar 16, 2022 at 10:27 AM Gedare Bloom <gedare at rtems.org> wrote:
>>
>> On Tue, Mar 15, 2022 at 2:28 PM Alex White <alex.white at oarcorp.com> wrote:
>>>
>>> ---
>>>  bsps/include/dev/gpio/xilinx-axi-gpio.h       | 311 ++++++++++++++++++
>>>  bsps/shared/dev/gpio/xilinx-axi-gpio.c        | 221 +++++++++++++
>>
>> Is this AXI GPIO interface consistent across Xilinx IP?
> 
> Hi Gedare,
> 
> Thanks for taking a look at this.
> 
> As long as the Xilinx AXI GPIO v2.0 IP is being used, this interface
> should be usable across any platform.
> 
> I realize now that there is an older v1.0 AXI GPIO IP so it would be
> wise to specify which version is being supported somewhere.

I think it is PG144. This needs to be referenced. The document names seem to be
stable in Xilinx's documentation.

>>> +  /*
>>> +   * IP Interrupt Enable Register
>>> +   *
>>> +   * Provides the ability to independtly control whether interrupts for each
>> typo: independently
> 
> Will fix.
> 
>>
>>> +   * channel are enabled or disabled.
>>> +   *
>>> +   * 0 - No Channel input interrupt
>>> +   * 1 - Channel input interrupt
>>> +   */
>>> +  uint32_t ip_ier;
>>> +} xilinx_axi_gpio;
>>
>> This `xilinx_` is not a proper namespace for RTEMS. it doesn't
>> correspond with any existing components and so it requires some
>> discussion and approval to introduce it.
> 
> I used the same struct naming scheme as is used in bsps/include/dev/spi/xilinx-axi-spi-regs.h.
> 
> I guess this is different because it is exposed in the user-facing api?
>

Should there be an `rtems_` prefix to avoid any possible clashes with user or
3rd party code?

>>
>>> +
>>> +typedef struct {
>>> +  volatile xilinx_axi_gpio *regs;
>>> +  bool                      is_dual;
>>> +  uint32_t                  irq;
>>> +  bool                      has_interrupts;
>>> +} xilinx_axi_gpio_context;
>>> +
>>> +/**
>>> + * @brief Set pin configuration for the specified GPIO channel.
>>> + *
>>> + * Changes the pin configuration for a channel. Bits set to 0 are output, and
>>> + * bits set to 1 are input.
>>> + *
>>> + * @param[in] ctx the GPIO context
>>> + * @param[in] channel the GPIO channel
>>> + * @param[in] mask the mask to be applied to @ channel
>>> + *
>>> + * @retval None
>>> + */
>>> +void axi_gpio_set_data_direction(
>>
>> neither is this `axi_`
> 
> I realize now that this makes less sense than something like
> `xilinx_axi_gpio_set_data_direction` assuming the context struct name
> above was accepted.

And maybe `rtems_` as well?

Chris