Hi Benoit, > -----Original Message----- > From: Cousson, Benoit > Sent: Friday, September 17, 2010 12:55 PM > To: G, Manjunath Kondaiah > Cc: Kevin Hilman; linux-omap@xxxxxxxxxxxxxxx; Shilimkar, Santosh > Subject: Re: [PATCH v2 09/11] OMAP: DMA: Implement generic > errata handling > > Hi Manjunath, > > On 9/17/2010 7:05 AM, G, Manjunath Kondaiah wrote: > > Kevin, > > > >> -----Original Message----- > >> From: G, Manjunath Kondaiah > >> Sent: Tuesday, September 14, 2010 3:42 PM > >> To: G, Manjunath Kondaiah; Kevin Hilman > >> Cc: linux-omap@xxxxxxxxxxxxxxx; Cousson, Benoit; Shilimkar, Santosh > >> Subject: RE: [PATCH v2 09/11] OMAP: DMA: Implement generic > >> errata handling > >> > >> Kevin, > >> > >>> -----Original Message----- > >>> From: linux-omap-owner@xxxxxxxxxxxxxxx > >>> [mailto:linux-omap-owner@xxxxxxxxxxxxxxx] On Behalf Of G, > Manjunath > >>> Kondaiah > >>> Sent: Tuesday, September 07, 2010 5:18 PM > >>> To: Kevin Hilman > >>> Cc: linux-omap@xxxxxxxxxxxxxxx; Cousson, Benoit; > Shilimkar, Santosh > >>> Subject: RE: [PATCH v2 09/11] OMAP: DMA: Implement generic errata > >>> handling > >>> > >>> > >>> > >>>> -----Original Message----- > >>>> From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] > >>>> Sent: Saturday, September 04, 2010 4:12 AM > >>>> To: G, Manjunath Kondaiah > >>>> Cc: linux-omap@xxxxxxxxxxxxxxx; Cousson, Benoit; > >> Shilimkar, Santosh > >>>> Subject: Re: [PATCH v2 09/11] OMAP: DMA: Implement generic errata > >>>> handling > >>>> > >>>> Manjunatha GK<manjugk@xxxxxx> writes: > >>>> > >>>>> This patch introduces generic way of handling all OMAP > >>> DMA errata's > >>>>> which are applicable for OMAP1 and OMAP2PLUS processors. > >>>>> > >>>>> Signed-off-by: Manjunatha GK<manjugk@xxxxxx> > >>>>> Cc: Benoit Cousson<b-cousson@xxxxxx> > >>>>> Cc: Kevin Hilman<khilman@xxxxxxxxxxxxxxxxxxx> > >>>>> Cc: Santosh Shilimkar<santosh.shilimkar@xxxxxx> > >>>>> --- > >>>>> arch/arm/mach-omap1/dma.c | 6 ++++ > >>>>> arch/arm/mach-omap2/dma.c | 34 > >>>> +++++++++++++++++++++++ > >>>>> arch/arm/plat-omap/dma.c | 48 > >>>> ++++++++++++++++++-------------- > >>>>> arch/arm/plat-omap/include/plat/dma.h | 9 ++++++ > >>>>> 4 files changed, 76 insertions(+), 21 deletions(-) > >>>>> > >>>>> diff --git a/arch/arm/mach-omap1/dma.c > >>> b/arch/arm/mach-omap1/dma.c > >>>>> index 26ab6e3..615c5f5 100644 > >>>>> --- a/arch/arm/mach-omap1/dma.c > >>>>> +++ b/arch/arm/mach-omap1/dma.c > >>>>> @@ -170,6 +170,12 @@ static int __init > >> omap1_system_dma_init(void) > >>>>> goto exit_device_put; > >>>>> } > >>>>> > >>>>> + /* Errata handling for all omap1 plus processors */ > >>>>> + pdata->errata = 0; > >>>> > >>>> This isn't needed as you just kzalloc'd pdata. > >>> ok > >>>> > >>>>> + if (cpu_class_is_omap1()&& !cpu_is_omap15xx()) > >>>> > >>>> You don't need cpu_class_is_omap1() as this is OMAP1 > >> specific code. > >>> Ok. Looks like copy, paste issue from plat-omap dma.c. I > >> will fix it. > >>>> > >>>>> + pdata->errata |= OMAP3_3_ERRATUM; > >>>>> + > >>>>> d = pdata->dma_attr; > >>>>> > >>>>> /* Valid attributes for omap1 plus processors > >> */ diff --git > >>>>> a/arch/arm/mach-omap2/dma.c b/arch/arm/mach-omap2/dma.c index > >>>>> f369bee..8832bd1 100644 > >>>>> --- a/arch/arm/mach-omap2/dma.c > >>>>> +++ b/arch/arm/mach-omap2/dma.c > >>>>> @@ -80,6 +80,40 @@ static int __init > >>>> omap2_system_dma_init_dev(struct omap_hwmod *oh, void *user) > >>>>> > >>>>> pdata->dma_attr = (struct omap_dma_dev_attr > >>>> *)oh->dev_attr; > >>>>> > >>>>> + /* Handling Errata's for all OMAP2PLUS processors */ > >>>>> + pdata->errata = 0; > >>>> > >>>> not needed, see above > >>> ok > >>>> > >>>>> + if (cpu_is_omap242x() || > >>>>> + (cpu_is_omap243x()&& omap_type()<= > >>>> OMAP2430_REV_ES1_0)) > >>>>> + pdata->errata = DMA_CHAINING_ERRATA; > >>>>> + > >>>>> + /* > >>>>> + * Errata: On ES2.0 BUFFERING disable must be set. > >>>>> + * This will always fail on ES1.0 > >>>>> + */ > >>>>> + if (cpu_is_omap24xx()) > >>>>> + pdata->errata |= > >> DMA_BUFF_DISABLE_ERRATA; > >>>>> + > >>>>> + /* > >>>>> + * Errata: OMAP2: sDMA Channel is not disabled > >>>>> + * after a transaction error. So we explicitely > >>>>> + * disable the channel > >>>>> + */ > >>>>> + if (cpu_class_is_omap2()) > >>>>> + pdata->errata |= > >> DMA_CH_DISABLE_ERRATA; > >>>>> + > >>>>> + /* Errata: OMAP3 : > >>>> > >>>> fix multi-line comment style > >>> Ok. > >>> > >>>> > >>>>> + * A bug in ROM code leaves IRQ status for channels 0 > >>>> and 1 uncleared > >>>>> + * after secure sram context save and restore. > >> Hence we need to > >>>>> + * manually clear those IRQs to avoid spurious > >> interrupts. This > >>>>> + * affects only secure devices. > >>>>> + */ > >>>>> + if (cpu_is_omap34xx()&& (omap_type() != > >> OMAP2_DEVICE_TYPE_GP)) > >>>>> + pdata->errata |= > >> DMA_IRQ_STATUS_ERRATA; > >>>>> + > >>>>> + /* Errata3.3: Applicable for all omap2 plus */ > >>>>> + pdata->errata |= OMAP3_3_ERRATUM; > >>>>> + > >>>>> od = omap_device_build(name, 0, oh, pdata, > >> sizeof(*pdata), > >>>>> omap2_dma_latency, > >>>> ARRAY_SIZE(omap2_dma_latency), 0); > >>>>> > >>>>> diff --git a/arch/arm/plat-omap/dma.c > >> b/arch/arm/plat-omap/dma.c > >>>>> index 36c3dde..409586a 100644 > >>>>> --- a/arch/arm/plat-omap/dma.c > >>>>> +++ b/arch/arm/plat-omap/dma.c > >>>>> @@ -187,6 +187,25 @@ static inline void set_gdma_dev(int > >>>> req, int dev) > >>>>> #define set_gdma_dev(req, dev) do {} while (0) > >>>>> #endif > >>>>> > >>>>> +static void dma_ocpsysconfig_errata(u32 *sys_cf, bool flag) > >>>> > >>>> Please use (or extend) hwmod layer for modifying device > SYSCONFIG. > >>> > >>> I will check this. > >> > >> I propose following options for addressing this errata: > >> Option 1: > >> Creating 2 new API's with 1 static function in omap_hwmod.c > >> for handling midle mode errata issues. > >> > >> static int _get_master_standbymode(oh, u32 *idlemode, u32 v); > >> int omap_hwmod_set_master_idlemode(struct omap_hwmod *oh, u8 > >> idlemode); int omap_hwmod_get_master_idlemode(struct > >> omap_hwmod *oh, u32 *idlemode); > >> > >> In dma driver, the api's will be used as: > >> omap_hwmod_get_master_idlemode(oh, midle_mode); > >> omap_hwmod_set_master_idlemode(oh, 1); > >> /* disable channels which completed data transfer */ ... > >> /* Restore original standby mode values */ > >> omap_hwmod_set_master_idlemode(oh, *midle_mode) > >> > >> > >> Option 2: > >> Create 1 new exported API for modifying sysconfig standby > mode bits. > >> In this case, restoring original standby mode value is > >> through accessing sysc flags and _sysc_cache values through > >> oh in DMA driver. > >> > >> I feel option #1 seems to be cleaner approach since it avoids > >> accessing oh contents in driver. Let me know if you have > >> objections for the same. > >> > >> /Code snippet for option 1 */ > >> static int _get_master_standbymode(oh, u32 *idlemode) { > >> u32 mstandby_mask; > >> u8 mstandby_shift; > >> > >> ... > >> > >> mstandby_shift = oh->class->sysc->sysc_fields->midle_shift; > >> mstandby_mask = (0x3<< mstandby_shift); > >> *idle_mode = ((oh->_sysc_cache)& mstandby_mask)>> > >> mstandby_shift; > >> > >> return 0; > >> } > >> > >> int omap_hwmod_set_master_idlemode(struct omap_hwmod *oh, u8 > >> idlemode) { > >> u32 v; > >> int retval = 0; > >> ... > >> v = oh->_sysc_cache; > >> > >> retval = _set_master_standbymode(oh, idlemode,&v); > >> if (!retval) > >> _write_sysconfig(v, oh); > >> > >> return retval; > >> } > >> > >> int omap_hwmod_get_master_idlemode(struct omap_hwmod *oh, u32 > >> *idlemode) { > >> u32 v; > >> int retval = 0; > >> ... > >> retval = _get_master_standbymode(oh, idlemode); > >> return retval; > >> } > >> > > > > I assume you are ok with option #1. Let me know if you have any > > issues/concenrs with above approach. I am in the process of > consolidating > > all the review comments and addressing all applicable > review comments. > > Not really, the option #1 will still require you to use the > oh pointer, > which is supposed to be private to the omap_device. > > What is still not clear is why and when you need to change > the sysconfig > setting. It is required before stopping DMA chain transfer. > > Do you have a details explanation? Ideally you should try to > couple the > sysconfig change along with pm_runtime / hwmod state change, then we > will be able to handle that smoothly in the framework. Since channel is already requested(and there is possibility of other channels also used at the same time), using pm_runtime API's will only increase usage count and will not invoke omap_device_enable. > > If you cannot do that, you will need to add an omap_device > API as well. There is already one such API exists in hwmod layer for handling this type of errata(omap_hwmod_set_slave_idlemode in omap_hwmod.c). Above proposal is based on similar implementation. -Manjunath -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
