[PATCH RTEMS] bsps/beagle: Refactored i2c driver
Niteesh G. S.
niteesh.gs at gmail.com
Thu Apr 8 01:56:20 UTC 2021
Hello Vijay,
On Sun, Mar 28, 2021 at 1:42 AM Vijay Kumar Banerjee <vijay at rtems.org>
wrote:
> Hi Niteesh,
>
> Thanks for the patch. I have some questions below.
>
> On Mon, Mar 22, 2021 at 11:48 AM G S Niteesh Babu <niteesh.gs at gmail.com>
> wrote:
> >
> > Refactored the i2c driver to parse register values from the device
> > tree rather than hardcoding them. But still the clocks have to
> > initialized manually.
> > ---
> > bsps/arm/beagle/i2c/bbb-i2c.c | 100 ++++++++++++++++--------------
> > bsps/arm/beagle/include/bsp.h | 4 ++
> > bsps/arm/beagle/include/bsp/i2c.h | 32 +---------
> > bsps/arm/beagle/start/bspstart.c | 53 +++++++++++-----
> > 4 files changed, 96 insertions(+), 93 deletions(-)
> >
> > diff --git a/bsps/arm/beagle/i2c/bbb-i2c.c
> b/bsps/arm/beagle/i2c/bbb-i2c.c
> > index b2a7cf814d..c315b6fc3b 100644
> > --- a/bsps/arm/beagle/i2c/bbb-i2c.c
> > +++ b/bsps/arm/beagle/i2c/bbb-i2c.c
> > @@ -25,6 +25,7 @@
> > #include <bsp/bbb-gpio.h>
> > #include <rtems/score/assert.h>
> > #include <dev/i2c/i2c.h>
> > +#include <ofw/ofw.h>
> >
> > typedef struct bbb_i2c_bus {
> > i2c_bus base;
> > @@ -34,12 +35,6 @@ typedef struct bbb_i2c_bus {
> > volatile uint32_t *i2c_clkctrl;
> > volatile uint32_t *clkstctrl;
> > } clkregs;
> > - struct {
> > - volatile uint32_t *conf_sda;
> > - uint32_t mmode_sda;
> > - volatile uint32_t *conf_scl;
> > - uint32_t mmode_scl;
> > - } pinregs;
> > rtems_id task_id;
> > rtems_vector_number irq;
> > i2c_msg *buffer;
> > @@ -56,19 +51,29 @@ typedef struct bbb_i2c_bus {
> > #else
> > #define debug_print(fmt, args...)
> > #endif
> > +/*
> > + * Here we assume the number of i2c nodes
> > + * will be less than 100.
> > + */
> > +#define PATH_LEN strlen("/dev/i2c-xx")
> >
> > static int am335x_i2c_fill_registers(
> > bbb_i2c_bus *bus,
> > - uintptr_t register_base
> > + phandle_t node
> > )
> > {
> > - /* FIXME: The pin handling should be replaced by a proper pin
> handling during
> > - * initialization. This one is heavily board specific. */
> > -#if ! IS_AM335X
> > - printk ("The I2C driver currently only works on Beagle Bone. Please
> add your pin configs.");
> > - return EINVAL;
> > -#endif
> > - bus->regs = (volatile bbb_i2c_regs *) register_base;
> > + ssize_t rv;
> > + rtems_ofw_memory_area reg;
> > +
> > + rv = rtems_ofw_get_reg(node, ®, sizeof(reg));
> > + if (rv <= 0)
> > + return EINVAL;
> > +
> > + bus->regs = (volatile bbb_i2c_regs *)reg.start;
> > +
> > + /*
> > + * FIXME: Implement a clock driver to parse and setup clocks
> > + */
> > switch ((intptr_t) bus->regs) {
> > case AM335X_I2C0_BASE:
> > bus->clkregs.ctrl_clkctrl = ®(AM335X_SOC_CM_WKUP_REGS +
> > @@ -77,10 +82,6 @@ static int am335x_i2c_fill_registers(
> > AM335X_CM_WKUP_I2C0_CLKCTRL);
> > bus->clkregs.clkstctrl = ®(AM335X_SOC_CM_WKUP_REGS +
> > AM335X_CM_WKUP_CLKSTCTRL);
> > - bus->pinregs.conf_sda = ®(AM335X_PADCONF_BASE +
> AM335X_CONF_I2C0_SDA);
> > - bus->pinregs.mmode_sda = 0;
> > - bus->pinregs.conf_scl = ®(AM335X_PADCONF_BASE +
> AM335X_CONF_I2C0_SCL);
> > - bus->pinregs.mmode_scl = 0;
> > break;
> > case AM335X_I2C1_BASE:
> > bus->clkregs.ctrl_clkctrl = ®(AM335X_SOC_CM_WKUP_REGS +
> > @@ -88,10 +89,6 @@ static int am335x_i2c_fill_registers(
> > bus->clkregs.i2c_clkctrl = ®(AM335X_CM_PER_ADDR +
> > AM335X_CM_PER_I2C1_CLKCTRL);
> > bus->clkregs.clkstctrl = NULL;
> > - bus->pinregs.conf_sda = ®(AM335X_PADCONF_BASE +
> AM335X_CONF_SPI0_D1);
> > - bus->pinregs.mmode_sda = 2;
> > - bus->pinregs.conf_scl = ®(AM335X_PADCONF_BASE +
> AM335X_CONF_SPI0_CS0);
> > - bus->pinregs.mmode_scl = 2;
> > break;
> > case AM335X_I2C2_BASE:
> > bus->clkregs.ctrl_clkctrl = ®(AM335X_SOC_CM_WKUP_REGS +
> > @@ -99,24 +96,12 @@ static int am335x_i2c_fill_registers(
> > bus->clkregs.i2c_clkctrl = ®(AM335X_CM_PER_ADDR +
> > AM335X_CM_PER_I2C2_CLKCTRL);
> > bus->clkregs.clkstctrl = NULL;
> > - bus->pinregs.conf_sda = ®(AM335X_PADCONF_BASE +
> AM335X_CONF_UART1_CTSN);
> > - bus->pinregs.mmode_sda = 3;
> > - bus->pinregs.conf_scl = ®(AM335X_PADCONF_BASE +
> AM335X_CONF_UART1_RTSN);
> > - bus->pinregs.mmode_scl = 3;
> > break;
> > default:
> > return EINVAL;
> > }
> > - return 0;
> > -}
> >
> > -static void am335x_i2c_pinmux( bbb_i2c_bus *bus )
> > -{
> > - *bus->pinregs.conf_sda =
> > - ( BBB_RXACTIVE | BBB_SLEWCTRL | bus->pinregs.mmode_sda);
> > -
> > - *bus->pinregs.conf_scl =
> > - ( BBB_RXACTIVE | BBB_SLEWCTRL | bus->pinregs.mmode_scl);
> > + return 0;
> > }
> >
> > static void am335x_i2c_module_clk_enable( bbb_i2c_bus *bus )
> > @@ -453,18 +438,16 @@ static void am335x_i2c_destroy( i2c_bus *base )
> > i2c_bus_destroy_and_free( &bus->base );
> > }
> >
> > -int am335x_i2c_bus_register(
> > - const char *bus_path,
> > - uintptr_t register_base,
> > - uint32_t input_clock,
> > - rtems_vector_number irq
> > +static int am335x_i2c_bus_register(
> > + phandle_t node
> > )
> This is method can be used by the application to register i2c device, why
> is it being removed? It is also documented here:
> https://docs.rtems.org/branches/master/user/bsps/bsps-arm.html#id2
> If we really want to make it static, then it at least needs to be removed
> from the documentation. But I'm not sure if we want to make it static.
>
The registration of the devices will happen based on the device tree, if a
user wants to add any device it is
expected from him to provide an overlay or enable the device node in the
FDT. This removes the need for
registration functions.
https://devel.rtems.org/ticket/3784
https://lists.rtems.org/pipermail/devel/2019-August/027288.html
> {
> > - bbb_i2c_bus *bus;
> > - rtems_status_code sc;
> > - int err;
> > -
> > - (void) input_clock; /* FIXME: Unused. Left for compatibility. */
> > + bbb_i2c_bus *bus;
> > + rtems_status_code sc;
> > + rtems_vector_number irq;
> > + int err;
> > + int unit;
> > + char bus_path[PATH_LEN];
> >
> > bus = (bbb_i2c_bus *) i2c_bus_alloc_and_init( sizeof( *bus ) );
> >
> > @@ -472,15 +455,24 @@ int am335x_i2c_bus_register(
> > return -1;
> > }
> >
> > + unit = beagle_get_node_unit(node);
> > +
>
> How would a user register a custom path? Does it mean that the only way to
> register a custom path is through device tree?
>
Currently, one cannot, the bus path is determined based on the node index.
This can be fixed by allowing the user to pass
the path in the device tree as you said.
something like
i2c0 {
rtems-bus-path = "/dev/i2c-eeprom"
}
If this is OK, I'll add this feature to the patch?
>
> > + snprintf(bus_path, PATH_LEN, "/dev/i2c-%d", unit);
> If suppose the unit is 99, I think it'll still return "/dev/i2c-9.
> Should this be PATH_LEN + 1 here (or +1 can be added in the macro
> definition itself) to accommodate the 2 digit unit?
>
> > +
> > + err = rtems_ofw_get_interrupts(node, &irq, sizeof(irq));
>
> Can this be done from the application and then pass irq as an argument to
> am335x_i2c_bus_register, in order to keep the function signature same?
>
A similar approach to the bus path can be done here. Or if possible we can
let the user reinitialize the IRQ at runtime
by calling something like beagle_i2c_set_irq()
> > + if (err < 1) {
> > + ( *bus->base.destroy )( &bus->base );
> > + rtems_set_errno_and_return_minus_one( err );
> > + }
> > bus->irq = irq;
> >
> > - err = am335x_i2c_fill_registers(bus, register_base);
> > + err = am335x_i2c_fill_registers(bus, node);
> > if (err != 0) {
> > ( *bus->base.destroy )( &bus->base );
> > rtems_set_errno_and_return_minus_one( err );
> > }
> > - am335x_i2c_module_clk_enable(bus);
> > - am335x_i2c_pinmux( bus );
> > +
> > + am335x_i2c_module_clk_enable( bus );
> > err = am335x_i2c_reset( bus );
> > if (err != 0) {
> > ( *bus->base.destroy )( &bus->base );
> > @@ -506,3 +498,15 @@ int am335x_i2c_bus_register(
> >
> > return i2c_bus_register( &bus->base, bus_path );
> > }
> > +
> > +void beagle_i2c_init(phandle_t node)
>
> This is a nice wrapper and maybe you could add all the needed ofw calls
> here to get the values and then call am335x_i2c_bus_register. Please also
> document this function so that it also acts as a reference for someone on
> how to use the am335x_i2c_bus_register.
>
OK. I'll refactor and document this.
>
> > +{
> > + int rv;
> > +
> > + if (!rtems_ofw_is_node_compatible(node, "ti,omap4-i2c"))
> > + return ;
> > +
> > + rv = am335x_i2c_bus_register(node);
> > + if (rv != 0)
> > + printk("i2c: Could not register device (%d)\n", rv);
> > +}
> > diff --git a/bsps/arm/beagle/include/bsp.h
> b/bsps/arm/beagle/include/bsp.h
> > index cb415fda89..80a9cc291d 100644
> > --- a/bsps/arm/beagle/include/bsp.h
> > +++ b/bsps/arm/beagle/include/bsp.h
> > @@ -49,6 +49,8 @@
> > #include <libcpu/omap3.h>
> > #include <libcpu/am335x.h>
> >
> > +#include <ofw/ofw.h>
> > +
> > #define BSP_FEATURE_IRQ_EXTENSION
> >
> > /* UART base clock frequency */
> > @@ -68,6 +70,8 @@
> >
> > #define udelay(u) rtems_task_wake_after(1 +
> ((u)/rtems_configuration_get_microseconds_per_tick()))
> >
> > +int beagle_get_node_unit(phandle_t node);
> > +
> > /* Write a uint32_t value to a memory address. */
> > static inline void
> > write32(uint32_t address, uint32_t value)
> > diff --git a/bsps/arm/beagle/include/bsp/i2c.h
> b/bsps/arm/beagle/include/bsp/i2c.h
> > index 60f71194bf..d80b376fa0 100644
> > --- a/bsps/arm/beagle/include/bsp/i2c.h
> > +++ b/bsps/arm/beagle/include/bsp/i2c.h
> > @@ -26,6 +26,7 @@
> > #include <rtems.h>
> > #include <bsp.h>
> > #include <dev/i2c/i2c.h>
> > +#include <ofw/ofw.h>
> >
> > #ifdef __cplusplus
> > extern "C" {
> > @@ -38,10 +39,6 @@ extern "C" {
> > #define BBB_I2C_1_BUS_PATH "/dev/i2c-1"
> > #define BBB_I2C_2_BUS_PATH "/dev/i2c-2"
> >
> > -#define BBB_I2C0_IRQ 70
> > -#define BBB_I2C1_IRQ 71
> > -#define BBB_I2C2_IRQ 30
> > -
> > typedef enum {
> > I2C0,
> > I2C1,
> > @@ -87,32 +84,7 @@ typedef struct i2c_regs {
> > uint32_t BBB_I2C_SBLOCK;
> > } bbb_i2c_regs;
> >
> > -int am335x_i2c_bus_register(
> > - const char *bus_path,
> > - uintptr_t register_base,
> > - uint32_t input_clock, /* FIXME: Unused. Left for
> compatibility. */
> > - rtems_vector_number irq
> > -);
> > -
> > -static inline int bbb_register_i2c_1( void )
>
> Instead of removing these wrappers, could you use these to register the
> i2c-1 or i2c-2 paths, just for compatibility reasons?
>
If we have these registration functions, we will have to remove the
automatic initialization of the i2c driver during boot time right?
I am not sure of how can we have both user-based registration and Device
Tree based registration at the same time.
https://devel.rtems.org/ticket/3784
As per this ticket, I think the goal is to remove these kinds of
registration functions so that the user doesn't have to
worry about the number of devices or type of the board.
>
>
>
> Once again, thank you for working on the patch. The bbb-i2c does need
> refactoring to work with the update ofw changes.
>
Really sorry for the delay, I have been working with QEMU to participate in
GSoC this year. And it is really hard to manage
between this and other academic activities.
> Best regards,
> Vijay
>
> > -{
> > - return am335x_i2c_bus_register(
> > - BBB_I2C_1_BUS_PATH,
> > - AM335X_I2C1_BASE,
> > - I2C_BUS_CLOCK_DEFAULT,
> > - BBB_I2C1_IRQ
> > - );
> > -}
> > -
> > -static inline int bbb_register_i2c_2( void )
> > -{
> > - return am335x_i2c_bus_register(
> > - BBB_I2C_2_BUS_PATH,
> > - AM335X_I2C2_BASE,
> > - I2C_BUS_CLOCK_DEFAULT,
> > - BBB_I2C2_IRQ
> > - );
> > -}
> > +void beagle_i2c_init( phandle_t node );
> >
> > #ifdef __cplusplus
> > }
> > diff --git a/bsps/arm/beagle/start/bspstart.c
> b/bsps/arm/beagle/start/bspstart.c
> > index a0736294c9..d45eec9e92 100644
> > --- a/bsps/arm/beagle/start/bspstart.c
> > +++ b/bsps/arm/beagle/start/bspstart.c
> > @@ -22,9 +22,15 @@
> > #include "bsp-soc-detect.h"
> > #include <arm/ti/ti_pinmux.h>
> > #include <ofw/ofw.h>
> > +#include <bsp/i2c.h>
> > +#include <string.h>
> > +#include <stdlib.h>
> > +#include <ctype.h>
> >
> > #include "bspdebug.h"
> >
> > +#define MIN(l,r) ((l) < (r) ? (l) : (r))
>
> Where is it used?
>
Used it during testing. Will be removed.
>
> > +
> > void bsp_start(void)
> > {
> > const char *type;
> > @@ -70,6 +76,7 @@ static void traverse_fdt_nodes( phandle_t node )
> > * Put all driver initialization functions here
> > */
> > beagle_pinmux_init(node);
> > + beagle_i2c_init(node);
> > }
> > }
> >
> > @@ -80,24 +87,40 @@ static void bbb_drivers_initialize(void)
> > traverse_fdt_nodes(node);
> > }
> >
> > -static void bbb_i2c_0_initialize(void)
> > +int beagle_get_node_unit(phandle_t node)
> > {
> > - int err;
> > -
> > - err = am335x_i2c_bus_register(BBB_I2C_0_BUS_PATH,
> > - AM335X_I2C0_BASE,
> > - I2C_BUS_CLOCK_DEFAULT,
> > - BBB_I2C0_IRQ);
> > - if (err != 0) {
> > - printk("rtems i2c-0: Device could not be registered (%d)", err);
> > + char name[30];
> > + char prop[30];
> > + char prop_val[30];
> > + static int unit;
> > + phandle_t aliases;
> > + int rv;
> > + int i;
> > +
> > + rv = rtems_ofw_get_prop(node, "name", name, sizeof(name));
> > + if (rv <= 0)
> > + return unit++;
> > +
> > + aliases = rtems_ofw_find_device("/aliases");
> > +
> > + rv = rtems_ofw_next_prop(aliases, NULL, &prop[0], sizeof(prop));
> > +
> > + while (rv > 0) {
> > + rv = rtems_ofw_get_prop(aliases, prop, prop_val, sizeof(prop_val));
> > +
> > + if (strstr(prop_val, name) != NULL) {
> > + for (i = strlen(prop) - 1; i >= 0; i--) {
> > + if (!isdigit(prop[i]))
> > + break;
> > + }
> > +
> > + return strtol(&prop[i + 1], NULL, 10);
> > + }
> > + rv = rtems_ofw_next_prop(aliases, prop, prop, sizeof(prop));
> > }
> > -}
> >
> > -RTEMS_SYSINIT_ITEM(
> > - bbb_i2c_0_initialize,
> > - RTEMS_SYSINIT_LAST,
> > - RTEMS_SYSINIT_ORDER_LAST_BUT_5
> > -);
> > + return unit++;
> > +}
> >
> > RTEMS_SYSINIT_ITEM(
> > bbb_drivers_initialize,
> > --
> > 2.17.1
> >
> > _______________________________________________
> > devel mailing list
> > devel at rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210408/6b51ad76/attachment-0001.html>
More information about the devel
mailing list