[PATCH] microblaze: Add AXI GPIO driver

[Alex White]

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.


> > +  /*
> > +   * 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?

>
> > +
> > +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.

>
> > +  xilinx_axi_gpio_context *ctx,
>
> Mixing the newly proposed namespaces is also confusing.

I agree.

What would make this easiest? Should I start a new thread on Xilinx AXI
peripheral driver naming in the shared namespace?

Thanks,

Alex