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