Re: [PATCH] microblaze: Add AXI GPIO driver
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 wrote:
>>
>> On Tue, Mar 15, 2022 at 2:28 PM Alex White 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
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH] microblaze: Add AXI GPIO driver
On Wed, Mar 16, 2022 at 10:25 AM Alex White wrote:
>
> On Wed, Mar 16, 2022 at 10:27 AM Gedare Bloom wrote:
> >
> > On Tue, Mar 15, 2022 at 2:28 PM Alex White 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?
>
Yes, that will be best.
> Thanks,
>
> Alex
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH] microblaze: Add AXI GPIO driver
On Wed, Mar 16, 2022 at 10:27 AM Gedare Bloom wrote:
>
> On Tue, Mar 15, 2022 at 2:28 PM Alex White 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
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH] microblaze: Add AXI GPIO driver
On Tue, Mar 15, 2022 at 2:28 PM Alex White 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?
> .../bsps/microblaze/microblaze_fpga/obj.yml | 2 +
> 3 files changed, 534 insertions(+)
> create mode 100644 bsps/include/dev/gpio/xilinx-axi-gpio.h
> create mode 100644 bsps/shared/dev/gpio/xilinx-axi-gpio.c
>
> diff --git a/bsps/include/dev/gpio/xilinx-axi-gpio.h
> b/bsps/include/dev/gpio/xilinx-axi-gpio.h
> new file mode 100644
> index 00..dbd8748f34
> --- /dev/null
> +++ b/bsps/include/dev/gpio/xilinx-axi-gpio.h
> @@ -0,0 +1,311 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +
> +/**
> + * @file
> + *
> + * @ingroup RTEMSBSPsShared
> + *
> + * @brief Xilinx AXI GPIO definitions
> + */
> +
> +/*
> + * Copyright (C) 2022 On-Line Applications Research Corporation (OAR)
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *notice, this list of conditions and the following disclaimer in the
> + *documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
> IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef XILINX_AXI_GPIO_H
> +#define XILINX_AXI_GPIO_H
> +
> +#include
> +#include
> +#include
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +typedef struct {
> + /* Channel 1 data values */
> +
> + /*
> + * Used to read general purpose input ports and write to general purpose
> + * output ports from channel 1.
> + */
> + uint32_t gpio_data;
> +
> + /*
> + * The 3-state control register for channel 1 is used for the dynamic
> + * configuration of ports as input or output. When a bit is set to 1, the
> + * corresponding I/O port is an input port. When a bit is set to 0, it is
> an
> + * output port.
> + */
> + uint32_t gpio_tri;
> +
> + /* Channel 2 data values */
> +
> + /*
> + * Used to read general purpose input ports and write to general purpose
> + * output ports from channel 2.
> + */
> + uint32_t gpio2_data;
> +
> + /*
> + * The 3-state control register for channel 2 is used for the dynamic
> + * configuration of ports as input or output. When a bit is set to 1, the
> + * corresponding I/O port is an input port. When a bit is set to 0, it is
> an
> + * output port.
> + */
> + uint32_t gpio2_tri;
> +
> + char _unused[272];
> +
> + /* Only the 31st bit is used to enable interrupts globally */
> + #define GLOBAL_INTERRUPT_REGISTER_ENABLE BSP_BIT32(31)
> +
> + /*
> + * Global Interrupt Enable Register
> + *
> + * Determines whether interrupts are enabled or disabled.
> + *
> + * 0 - Disabled
> + * 1 - Enabled
> + */
> + uint32_t gier;
> +
> + char _unused2[12];
> +
> + /* Used with ip_isr and ip_ier member variables */
> + #define CHANNEL_1_INTERRUPT_REGISTER BSP_BIT32(0)
> + #define CHANNEL_2_INTERRUPT_REGISTER BSP_BIT32(1)
> +
> + /*
> + * IP Status Registers
> + *
> + * Contains the status bit for each channel.
> + *
> + * 0 - Disabled
> + * 1 - Enabled
> + */
> + uint32_t ip_isr;
> +
> + char _unused3[4];
> +
> + /*
> + * IP Interrupt Enable Register
> + *
> + * Provides the ability to independtly control whether interrupts for each
typo: independently
> + * 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.
> +
> +typedef struct {
> + volatile xilinx_axi_gpio *regs;
> + bool is_dual;
> + uint32_t irq;
> + bool has_interrupts;
> +}
Re: [PATCH] microblaze: Add AXI GPIO driver
Hello Alex, maybe someone from OAR should step in as a microblaze maintainer. -- embedded brains GmbH Herr Sebastian HUBER Dornierstr. 4 82178 Puchheim Germany email: sebastian.hu...@embedded-brains.de phone: +49-89-18 94 741 - 16 fax: +49-89-18 94 741 - 08 Registergericht: Amtsgericht München Registernummer: HRB 157899 Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler Unsere Datenschutzerklärung finden Sie hier: https://embedded-brains.de/datenschutzerklaerung/ ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
[PATCH] microblaze: Add AXI GPIO driver
---
bsps/include/dev/gpio/xilinx-axi-gpio.h | 311 ++
bsps/shared/dev/gpio/xilinx-axi-gpio.c| 221 +
.../bsps/microblaze/microblaze_fpga/obj.yml | 2 +
3 files changed, 534 insertions(+)
create mode 100644 bsps/include/dev/gpio/xilinx-axi-gpio.h
create mode 100644 bsps/shared/dev/gpio/xilinx-axi-gpio.c
diff --git a/bsps/include/dev/gpio/xilinx-axi-gpio.h
b/bsps/include/dev/gpio/xilinx-axi-gpio.h
new file mode 100644
index 00..dbd8748f34
--- /dev/null
+++ b/bsps/include/dev/gpio/xilinx-axi-gpio.h
@@ -0,0 +1,311 @@
+/* SPDX-License-Identifier: BSD-2-Clause */
+
+/**
+ * @file
+ *
+ * @ingroup RTEMSBSPsShared
+ *
+ * @brief Xilinx AXI GPIO definitions
+ */
+
+/*
+ * Copyright (C) 2022 On-Line Applications Research Corporation (OAR)
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *notice, this list of conditions and the following disclaimer in the
+ *documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef XILINX_AXI_GPIO_H
+#define XILINX_AXI_GPIO_H
+
+#include
+#include
+#include
+
+#ifdef __cplusplus
+extern "C" {
+#endif /* __cplusplus */
+
+typedef struct {
+ /* Channel 1 data values */
+
+ /*
+ * Used to read general purpose input ports and write to general purpose
+ * output ports from channel 1.
+ */
+ uint32_t gpio_data;
+
+ /*
+ * The 3-state control register for channel 1 is used for the dynamic
+ * configuration of ports as input or output. When a bit is set to 1, the
+ * corresponding I/O port is an input port. When a bit is set to 0, it is an
+ * output port.
+ */
+ uint32_t gpio_tri;
+
+ /* Channel 2 data values */
+
+ /*
+ * Used to read general purpose input ports and write to general purpose
+ * output ports from channel 2.
+ */
+ uint32_t gpio2_data;
+
+ /*
+ * The 3-state control register for channel 2 is used for the dynamic
+ * configuration of ports as input or output. When a bit is set to 1, the
+ * corresponding I/O port is an input port. When a bit is set to 0, it is an
+ * output port.
+ */
+ uint32_t gpio2_tri;
+
+ char _unused[272];
+
+ /* Only the 31st bit is used to enable interrupts globally */
+ #define GLOBAL_INTERRUPT_REGISTER_ENABLE BSP_BIT32(31)
+
+ /*
+ * Global Interrupt Enable Register
+ *
+ * Determines whether interrupts are enabled or disabled.
+ *
+ * 0 - Disabled
+ * 1 - Enabled
+ */
+ uint32_t gier;
+
+ char _unused2[12];
+
+ /* Used with ip_isr and ip_ier member variables */
+ #define CHANNEL_1_INTERRUPT_REGISTER BSP_BIT32(0)
+ #define CHANNEL_2_INTERRUPT_REGISTER BSP_BIT32(1)
+
+ /*
+ * IP Status Registers
+ *
+ * Contains the status bit for each channel.
+ *
+ * 0 - Disabled
+ * 1 - Enabled
+ */
+ uint32_t ip_isr;
+
+ char _unused3[4];
+
+ /*
+ * IP Interrupt Enable Register
+ *
+ * Provides the ability to independtly control whether interrupts for each
+ * channel are enabled or disabled.
+ *
+ * 0 - No Channel input interrupt
+ * 1 - Channel input interrupt
+ */
+ uint32_t ip_ier;
+} xilinx_axi_gpio;
+
+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(
+ xilinx_axi_gpio_context *ctx,
+ uint32_t channel,
+ uint32_t mask
+);
+
+/**
+ * @brief Get pin configuration for specified GPIO channel.
+ *
+ * Gets the c
