Hello Vijay and Niteesh,
Am 08.04.21 um 04:17 schrieb Vijay Kumar Banerjee:
On Wed, Apr 7, 2021 at 7:56 PM Niteesh G. S. <niteesh...@gmail.com> wrote:
Hello Vijay,
On Sun, Mar 28, 2021 at 1:42 AM Vijay Kumar Banerjee <vi...@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...@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.
Please follow the usual FDT conventions for the name. System or vendor
specific names are normally separated with a ','. For Example:
linux,keymap
ibm,opal-rtc
So in this case it should be a
rtems,bus-path
or maybe
rtems,device-path
or just
rtems,path
I used the last one in the imxrt BSP.
+ 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.
Please add a
Update #3784
line in the commit description to make it clear that the change belongs
to that ticket.
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.
+1
Best regards
Christian
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@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
--
--------------------------------------------
embedded brains GmbH
Herr Christian MAUDERER
Dornierstr. 4
82178 Puchheim
Germany
email: christian.maude...@embedded-brains.de
phone: +49-89-18 94 741 - 18
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
