[PATCH RTEMS] bsps/beagle: Refactored i2c driver
Vijay Kumar Banerjee
vijay at rtems.org
Thu Apr 8 02:17:17 UTC 2021
On Wed, Apr 7, 2021 at 7:56 PM Niteesh G. S. <niteesh.gs at gmail.com> wrote:
>
> 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
>
Ok, thanks for the links. Please update the documentation accordingly.
>> > {
>> > - 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?
Node index works and it would also be nice if you add an
rtems-bus-path field for FDT. Please document that and make sure that
it works based on node index even when the rtems-bus-path is absent.
>
>>
>>
>> > + 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()
>
My concern was removing the am335x_* call, I think it's Ok since that
would be removed. It'll be a more general approach.
>>
>> > + 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.
Thanks.
>>
>>
>> > +{
>> > + 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.
The discussions and the tickets make it clear that these need to go.
So this is Ok. Thanks for explaining.
>>
>>
>>
>>
>> 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.
>
Thanks for the good work and good luck for your GSoC application.
>>
>> 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.
:) Thanks
Best regards,
Vijay
>>
>>
>> > +
>> > 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
More information about the devel
mailing list