| Message ID | 1346843564-25343-4-git-send-email-anilkumar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, * AnilKumar Ch <anilkumar@ti.com> [120905 04:14]: > Add of_dev_auxdata to pass d_can raminit callback APIs to initialize > d_can RAM. D_CAN RAM initialization bits are present in CONTROL module > address space, which can be accessed by platform specific code. So > callback functions are added to serve this purpose, this can done by > using of_dev_auxdata. > > Two callback APIs are added to of_dev_auxdata used by two instances of > D_CAN IP. These callback functions are used to enable/disable D_CAN RAM > from CAN driver. I'd like to avoid the callbacks to the platform code where possible as that's the biggest pain we already have moving things to work with device tree for the existing drivers. And I'm pretty convinced that whatever is done with callbacks should be done with some Linux generic framework from the driver that has it's own binding, such as clock framework, regulator framework, pinctrl framework, runtime PM etc. > --- a/arch/arm/mach-omap2/board-generic.c > +++ b/arch/arm/mach-omap2/board-generic.c > @@ -37,11 +40,46 @@ static struct of_device_id omap_dt_match_table[] __initdata = { > { } > }; > > +void d_can_hw_raminit(unsigned int instance, bool enable) > +{ > + u32 val; > + > + val = readl(AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT)); > + if (enable) { > + val &= ~AM33XX_DCAN_RAMINIT_START_MASK(instance); > + val |= AM33XX_DCAN_RAMINIT_START_MASK(instance); > + writel(val, AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT)); > + } else { > + val &= ~AM33XX_DCAN_RAMINIT_START_MASK(instance); > + writel(val, AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT)); > + } > +} This part does not look good to me, this is tweaking the omap control module register bits directly. To me it seems that the above should be implemented in the omap/am33xx hwmod code that gets initialized when the dcan driver calls pm_runtime_enable()? Paul, got any other ideas? Regards, Tony
On 9/6/2012 4:48 AM, Tony Lindgren wrote: > Hi, > > * AnilKumar Ch <anilkumar@ti.com> [120905 04:14]: >> Add of_dev_auxdata to pass d_can raminit callback APIs to initialize >> d_can RAM. D_CAN RAM initialization bits are present in CONTROL module >> address space, which can be accessed by platform specific code. So >> callback functions are added to serve this purpose, this can done by >> using of_dev_auxdata. >> >> Two callback APIs are added to of_dev_auxdata used by two instances of >> D_CAN IP. These callback functions are used to enable/disable D_CAN RAM >> from CAN driver. > > I'd like to avoid the callbacks to the platform code where possible as > that's the biggest pain we already have moving things to work with device > tree for the existing drivers. > > And I'm pretty convinced that whatever is done with callbacks should be > done with some Linux generic framework from the driver that has it's own > binding, such as clock framework, regulator framework, pinctrl framework, > runtime PM etc. > >> --- a/arch/arm/mach-omap2/board-generic.c >> +++ b/arch/arm/mach-omap2/board-generic.c >> @@ -37,11 +40,46 @@ static struct of_device_id omap_dt_match_table[] __initdata = { >> { } >> }; >> >> +void d_can_hw_raminit(unsigned int instance, bool enable) >> +{ >> + u32 val; >> + >> + val = readl(AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT)); >> + if (enable) { >> + val &= ~AM33XX_DCAN_RAMINIT_START_MASK(instance); >> + val |= AM33XX_DCAN_RAMINIT_START_MASK(instance); >> + writel(val, AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT)); >> + } else { >> + val &= ~AM33XX_DCAN_RAMINIT_START_MASK(instance); >> + writel(val, AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT)); >> + } >> +} > > This part does not look good to me, this is tweaking the omap control > module register bits directly. To me it seems that the above should > be implemented in the omap/am33xx hwmod code that gets initialized when > the dcan driver calls pm_runtime_enable()? Paul, got any other ideas? > Technically yes, this is required during module enable/disable sequence. But there is no way currently supported in hwmod layer. Also I am not quite sure how many other modules/devices may use this. Couple of more examples I have here, In case of AM3517 we have similar SoC integration, where VPFE, MAC and USB required clock control (handled by clock-tree) and interrupt status (handled by callbacks) from control module. So not sure whether we can get rid of callbacks until we have control module MFD driver (on which Konstantin is working on) Thanks, Vaibhav > Regards, > > Tony > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Thu, Sep 06, 2012 at 09:26:07, Hiremath, Vaibhav wrote: > > > On 9/6/2012 4:48 AM, Tony Lindgren wrote: > > Hi, > > > > * AnilKumar Ch <anilkumar@ti.com> [120905 04:14]: > >> Add of_dev_auxdata to pass d_can raminit callback APIs to initialize > >> d_can RAM. D_CAN RAM initialization bits are present in CONTROL module > >> address space, which can be accessed by platform specific code. So > >> callback functions are added to serve this purpose, this can done by > >> using of_dev_auxdata. > >> > >> Two callback APIs are added to of_dev_auxdata used by two instances of > >> D_CAN IP. These callback functions are used to enable/disable D_CAN RAM > >> from CAN driver. > > > > I'd like to avoid the callbacks to the platform code where possible as > > that's the biggest pain we already have moving things to work with device > > tree for the existing drivers. > > > > And I'm pretty convinced that whatever is done with callbacks should be > > done with some Linux generic framework from the driver that has it's own > > binding, such as clock framework, regulator framework, pinctrl framework, > > runtime PM etc. > > > >> --- a/arch/arm/mach-omap2/board-generic.c > >> +++ b/arch/arm/mach-omap2/board-generic.c > >> @@ -37,11 +40,46 @@ static struct of_device_id omap_dt_match_table[] __initdata = { > >> { } > >> }; > >> > >> +void d_can_hw_raminit(unsigned int instance, bool enable) > >> +{ > >> + u32 val; > >> + > >> + val = readl(AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT)); > >> + if (enable) { > >> + val &= ~AM33XX_DCAN_RAMINIT_START_MASK(instance); > >> + val |= AM33XX_DCAN_RAMINIT_START_MASK(instance); > >> + writel(val, AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT)); > >> + } else { > >> + val &= ~AM33XX_DCAN_RAMINIT_START_MASK(instance); > >> + writel(val, AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT)); > >> + } > >> +} > > > > This part does not look good to me, this is tweaking the omap control > > module register bits directly. To me it seems that the above should > > be implemented in the omap/am33xx hwmod code that gets initialized when > > the dcan driver calls pm_runtime_enable()? Paul, got any other ideas? > > > > Technically yes, this is required during module enable/disable sequence. > But there is no way currently supported in hwmod layer. Also I am not > quite sure how many other modules/devices may use this. > It should be possible to do this by providing custom activate/deactivate functions similar to what was done for EMAC on AM35x [1]. However, right now on DT based systems omap_device_alloc() is called without any pm_lats od = omap_device_alloc(pdev, hwmods, oh_cnt, NULL, 0); The function pointers from the of_dev_auxdata somehow needs to be passed to omap_device_alloc(). Regards, Vaibhav B. > Couple of more examples I have here, > > In case of AM3517 we have similar SoC integration, where VPFE, MAC and > USB required clock control (handled by clock-tree) and interrupt status > (handled by callbacks) from control module. > So not sure whether we can get rid of callbacks until we have control > module MFD driver (on which Konstantin is working on) > > Thanks, > Vaibhav > > > Regards, > > > > Tony > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Thu, Sep 06, 2012 at 10:33:32, Bedia, Vaibhav wrote: > On Thu, Sep 06, 2012 at 09:26:07, Hiremath, Vaibhav wrote: > > > > > > On 9/6/2012 4:48 AM, Tony Lindgren wrote: > > > Hi, > > > > > > * AnilKumar Ch <anilkumar@ti.com> [120905 04:14]: > > >> Add of_dev_auxdata to pass d_can raminit callback APIs to initialize > > >> d_can RAM. D_CAN RAM initialization bits are present in CONTROL module > > >> address space, which can be accessed by platform specific code. So > > >> callback functions are added to serve this purpose, this can done by > > >> using of_dev_auxdata. > > >> > > >> Two callback APIs are added to of_dev_auxdata used by two instances of > > >> D_CAN IP. These callback functions are used to enable/disable D_CAN RAM > > >> from CAN driver. > > > > > > I'd like to avoid the callbacks to the platform code where possible as > > > that's the biggest pain we already have moving things to work with device > > > tree for the existing drivers. > > > > > > And I'm pretty convinced that whatever is done with callbacks should be > > > done with some Linux generic framework from the driver that has it's own > > > binding, such as clock framework, regulator framework, pinctrl framework, > > > runtime PM etc. > > > > > >> --- a/arch/arm/mach-omap2/board-generic.c > > >> +++ b/arch/arm/mach-omap2/board-generic.c > > >> @@ -37,11 +40,46 @@ static struct of_device_id omap_dt_match_table[] __initdata = { > > >> { } > > >> }; > > >> > > >> +void d_can_hw_raminit(unsigned int instance, bool enable) > > >> +{ > > >> + u32 val; > > >> + > > >> + val = readl(AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT)); > > >> + if (enable) { > > >> + val &= ~AM33XX_DCAN_RAMINIT_START_MASK(instance); > > >> + val |= AM33XX_DCAN_RAMINIT_START_MASK(instance); > > >> + writel(val, AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT)); > > >> + } else { > > >> + val &= ~AM33XX_DCAN_RAMINIT_START_MASK(instance); > > >> + writel(val, AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT)); > > >> + } > > >> +} > > > > > > This part does not look good to me, this is tweaking the omap control > > > module register bits directly. To me it seems that the above should > > > be implemented in the omap/am33xx hwmod code that gets initialized when > > > the dcan driver calls pm_runtime_enable()? Paul, got any other ideas? > > > > > > > Technically yes, this is required during module enable/disable sequence. > > But there is no way currently supported in hwmod layer. Also I am not > > quite sure how many other modules/devices may use this. > > > > It should be possible to do this by providing custom activate/deactivate > functions similar to what was done for EMAC on AM35x [1]. > Still didn't make it to mainline :) > However, right now on DT based systems omap_device_alloc() is called without > any pm_lats > > od = omap_device_alloc(pdev, hwmods, oh_cnt, NULL, 0); > > The function pointers from the of_dev_auxdata somehow needs to be passed to > omap_device_alloc(). > It is doable, and I believe it is the only option we have currently unless we add something newly. Thanks, Vaibhav > Regards, > Vaibhav B. > > > Couple of more examples I have here, > > > > In case of AM3517 we have similar SoC integration, where VPFE, MAC and > > USB required clock control (handled by clock-tree) and interrupt status > > (handled by callbacks) from control module. > > So not sure whether we can get rid of callbacks until we have control > > module MFD driver (on which Konstantin is working on) > > > > Thanks, > > Vaibhav > > > > > Regards, > > > > > > Tony > > > > > > _______________________________________________ > > > linux-arm-kernel mailing list > > > linux-arm-kernel@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > >
diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c index 6f93a20..b68e642 100644 --- a/arch/arm/mach-omap2/board-generic.c +++ b/arch/arm/mach-omap2/board-generic.c @@ -15,6 +15,7 @@ #include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/irqdomain.h> +#include <linux/can/platform/c_can.h> #include <mach/hardware.h> #include <asm/hardware/gic.h> @@ -22,6 +23,8 @@ #include <plat/board.h> #include "common.h" +#include "control.h" +#include "iomap.h" #include "common-board-devices.h" #if !(defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)) @@ -37,11 +40,46 @@ static struct of_device_id omap_dt_match_table[] __initdata = { { } }; +void d_can_hw_raminit(unsigned int instance, bool enable) +{ + u32 val; + + val = readl(AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT)); + if (enable) { + val &= ~AM33XX_DCAN_RAMINIT_START_MASK(instance); + val |= AM33XX_DCAN_RAMINIT_START_MASK(instance); + writel(val, AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT)); + } else { + val &= ~AM33XX_DCAN_RAMINIT_START_MASK(instance); + writel(val, AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT)); + } +} + +static struct c_can_platform_data d_can0_pdata = { + .ram_init = d_can_hw_raminit, + .instance = 0, +}; + +static struct c_can_platform_data d_can1_pdata = { + .ram_init = d_can_hw_raminit, + .instance = 1, +}; + +static const struct of_dev_auxdata am33xx_auxdata_lookup[] __initconst = { + OF_DEV_AUXDATA("bosch,d_can", 0x481cc000, NULL, &d_can0_pdata), + OF_DEV_AUXDATA("bosch,d_can", 0x481d0000, NULL, &d_can1_pdata), + { }, +}; + static void __init omap_generic_init(void) { omap_sdrc_init(NULL, NULL); - of_platform_populate(NULL, omap_dt_match_table, NULL, NULL); + if (of_machine_is_compatible("ti,am33xx")) + of_platform_populate(NULL, omap_dt_match_table, + am33xx_auxdata_lookup, NULL); + else + of_platform_populate(NULL, omap_dt_match_table, NULL, NULL); } #ifdef CONFIG_SOC_OMAP2420 diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h index b8cdc85..afd189b 100644 --- a/arch/arm/mach-omap2/control.h +++ b/arch/arm/mach-omap2/control.h @@ -356,6 +356,10 @@ #define AM33XX_CONTROL_STATUS_SYSBOOT1_SHIFT 22 #define AM33XX_CONTROL_STATUS_SYSBOOT1_MASK (0x3 << 22) +/* AM33XX DCAN bitfields */ +#define AM33XX_CONTROL_DCAN_RAMINIT 0x644 +#define AM33XX_DCAN_RAMINIT_START_MASK(i) (1 << (i)) + /* CONTROL OMAP STATUS register to identify OMAP3 features */ #define OMAP3_CONTROL_OMAP_STATUS 0x044c
Add of_dev_auxdata to pass d_can raminit callback APIs to initialize d_can RAM. D_CAN RAM initialization bits are present in CONTROL module address space, which can be accessed by platform specific code. So callback functions are added to serve this purpose, this can done by using of_dev_auxdata. Two callback APIs are added to of_dev_auxdata used by two instances of D_CAN IP. These callback functions are used to enable/disable D_CAN RAM from CAN driver. Signed-off-by: AnilKumar Ch <anilkumar@ti.com> --- arch/arm/mach-omap2/board-generic.c | 40 ++++++++++++++++++++++++++++++++++- arch/arm/mach-omap2/control.h | 4 ++++ 2 files changed, 43 insertions(+), 1 deletion(-)