| Message ID | 1475166715-7857-7-git-send-email-bgolaszewski@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Thu, Sep 29, 2016 at 06:31:55PM +0200, Bartosz Golaszewski wrote: > Default memory settings of da850 do not meet the throughput/latency > requirements of tilcdc. This results in the image displayed being > incorrect and the following warning being displayed by the LCDC > drm driver: > > tilcdc da8xx_lcdc.0: tilcdc_crtc_irq(0x00000020): FIFO underfow > > Reconfigure the LCDC priority to the highest. This is a workaround > for the da850-lcdk board which has the LCD controller enabled in > the device tree, but a long-term, system-wide fix is needed for > all davinci boards. > > This patch has been modified for mainline linux. It comes from a > downstream TI release for da850[1]. > > Original author: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> > > [1] http://arago-project.org/git/projects/linux-davinci.git?p=projects/linux-davinci.git;a=commitdiff;h=b9bd39a34cc02c3ba2fc15539a2f0bc2b68d25da;hp=6f6c795faa6366a4ebc1037a0235edba6018a991 > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- FWIW, the quirks could be applied conditionnally depending on the lcdc node presence in the DT, a bit like: https://github.com/kbeldan/linux/commit/cf15572ffef8e8a0d8110b3f6b29bd401d0538be https://github.com/kbeldan/linux/commit/07e4fff9958bc1625a96791dce284c163fbe9c43 Regards, Karl > arch/arm/mach-davinci/da8xx-dt.c | 43 ++++++++++++++++++++++++++++++ > arch/arm/mach-davinci/include/mach/da8xx.h | 4 +++ > 2 files changed, 47 insertions(+) > > diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c > index f8ecc02..9d29670 100644 > --- a/arch/arm/mach-davinci/da8xx-dt.c > +++ b/arch/arm/mach-davinci/da8xx-dt.c > @@ -44,9 +44,52 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = { > > #ifdef CONFIG_ARCH_DAVINCI_DA850 > > +/* > + * Adjust the default memory settings to cope with the LCDC > + * > + * REVISIT: This issue occurs on other davinci boards as well. Find > + * a proper system-wide fix. > + */ > +static void da850_lcdc_adjust_memory_bandwidth(void) > +{ > + void __iomem *cfg_mstpri1_base; > + void __iomem *cfg_mstpri2_base; > + void __iomem *emifb; > + u32 val; > + > + /* > + * Default master priorities in reg 0 are all lower by default than LCD > + * which is set below to 0. Hence don't need to change here. > + */ > + > + /* set EDMA30TC0 and TC1 to lower than LCDC (4 < 0) */ > + cfg_mstpri1_base = DA8XX_SYSCFG0_VIRT(DA8XX_MSTPRI1_REG); > + val = __raw_readl(cfg_mstpri1_base); > + val &= 0xFFFF00FF; > + val |= 4 << 8; /* 0-high, 7-low priority*/ > + val |= 4 << 12; /* 0-high, 7-low priority*/ > + __raw_writel(val, cfg_mstpri1_base); > + > + /* > + * Reconfigure the LCDC priority to the highest to ensure that > + * the throughput/latency requirements for the LCDC are met. > + */ > + cfg_mstpri2_base = DA8XX_SYSCFG0_VIRT(DA8XX_MSTPRI2_REG); > + > + val = __raw_readl(cfg_mstpri2_base); > + val &= 0x0fffffff; > + __raw_writel(val, cfg_mstpri2_base); > + > + /* set BPRIO */ > + emifb = ioremap(DA8XX_DDR_CTL_BASE, SZ_4K); > + __raw_writel(0x20, emifb + DA8XX_PBBPR_REG); > + iounmap(emifb); > +} > + > static void __init da850_init_machine(void) > { > of_platform_default_populate(NULL, da850_auxdata_lookup, NULL); > + da850_lcdc_adjust_memory_bandwidth(); > } > > static const char *const da850_boards_compat[] __initconst = { > diff --git a/arch/arm/mach-davinci/include/mach/da8xx.h b/arch/arm/mach-davinci/include/mach/da8xx.h > index f9f9713..5549eff 100644 > --- a/arch/arm/mach-davinci/include/mach/da8xx.h > +++ b/arch/arm/mach-davinci/include/mach/da8xx.h > @@ -56,6 +56,8 @@ extern unsigned int da850_max_speed; > #define DA8XX_SYSCFG0_VIRT(x) (da8xx_syscfg0_base + (x)) > #define DA8XX_JTAG_ID_REG 0x18 > #define DA8XX_HOST1CFG_REG 0x44 > +#define DA8XX_MSTPRI1_REG 0x114 > +#define DA8XX_MSTPRI2_REG 0x118 > #define DA8XX_CHIPSIG_REG 0x174 > #define DA8XX_CFGCHIP0_REG 0x17c > #define DA8XX_CFGCHIP1_REG 0x180 > @@ -79,6 +81,8 @@ extern unsigned int da850_max_speed; > #define DA8XX_AEMIF_CTL_BASE 0x68000000 > #define DA8XX_SHARED_RAM_BASE 0x80000000 > #define DA8XX_ARM_RAM_BASE 0xffff0000 > +#define DA8XX_DDR_CTL_BASE 0xB0000000 > +#define DA8XX_PBBPR_REG 0x00000020 > > void da830_init(void); > void da850_init(void); > -- > 2.7.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
2016-09-29 21:07 GMT+02:00 Karl Beldan <karl.beldan@gmail.com>: > Hi, > > On Thu, Sep 29, 2016 at 06:31:55PM +0200, Bartosz Golaszewski wrote: >> Default memory settings of da850 do not meet the throughput/latency >> requirements of tilcdc. This results in the image displayed being >> incorrect and the following warning being displayed by the LCDC >> drm driver: >> >> tilcdc da8xx_lcdc.0: tilcdc_crtc_irq(0x00000020): FIFO underfow >> >> Reconfigure the LCDC priority to the highest. This is a workaround >> for the da850-lcdk board which has the LCD controller enabled in >> the device tree, but a long-term, system-wide fix is needed for >> all davinci boards. >> >> This patch has been modified for mainline linux. It comes from a >> downstream TI release for da850[1]. >> >> Original author: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> >> >> [1] http://arago-project.org/git/projects/linux-davinci.git?p=projects/linux-davinci.git;a=commitdiff;h=b9bd39a34cc02c3ba2fc15539a2f0bc2b68d25da;hp=6f6c795faa6366a4ebc1037a0235edba6018a991 >> >> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> >> --- > > FWIW, the quirks could be applied conditionnally depending on the lcdc > node presence in the DT, a bit like: > https://github.com/kbeldan/linux/commit/cf15572ffef8e8a0d8110b3f6b29bd401d0538be > https://github.com/kbeldan/linux/commit/07e4fff9958bc1625a96791dce284c163fbe9c43 > > > Regards, > Karl Hi Karl, I decided to post the simplest possible way of this to get the lcdc working upstream. In parallel I'm working on a system-wide way of applying such quirks not only limited to device tree nodes' presence. Thanks for the info! Best regards, Bartosz Golaszewski
On Fri, Sep 30, 2016 at 9:31 AM, Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > 2016-09-29 21:07 GMT+02:00 Karl Beldan <karl.beldan@gmail.com>: >> Hi, >> >> On Thu, Sep 29, 2016 at 06:31:55PM +0200, Bartosz Golaszewski wrote: >>> Default memory settings of da850 do not meet the throughput/latency >>> requirements of tilcdc. This results in the image displayed being >>> incorrect and the following warning being displayed by the LCDC >>> drm driver: >>> >>> tilcdc da8xx_lcdc.0: tilcdc_crtc_irq(0x00000020): FIFO underfow >>> >>> Reconfigure the LCDC priority to the highest. This is a workaround >>> for the da850-lcdk board which has the LCD controller enabled in >>> the device tree, but a long-term, system-wide fix is needed for >>> all davinci boards. >>> >>> This patch has been modified for mainline linux. It comes from a >>> downstream TI release for da850[1]. >>> >>> Original author: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> >>> >>> [1] http://arago-project.org/git/projects/linux-davinci.git?p=projects/linux-davinci.git;a=commitdiff;h=b9bd39a34cc02c3ba2fc15539a2f0bc2b68d25da;hp=6f6c795faa6366a4ebc1037a0235edba6018a991 >>> >>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> >>> --- >> >> FWIW, the quirks could be applied conditionnally depending on the lcdc >> node presence in the DT, a bit like: >> https://github.com/kbeldan/linux/commit/cf15572ffef8e8a0d8110b3f6b29bd401d0538be >> https://github.com/kbeldan/linux/commit/07e4fff9958bc1625a96791dce284c163fbe9c43 >> >> >> Regards, >> Karl > > Hi Karl, > > I decided to post the simplest possible way of this to get the lcdc > working upstream. In parallel I'm working on a system-wide way of > applying such quirks not only limited to device tree nodes' presence. Ok, that'd be a good thing, apparently people have been looking to do such a thing for the am335x as well. Regards, Karl > Thanks for the info! > > Best regards, > Bartosz Golaszewski
On 09/29/16 19:31, Bartosz Golaszewski wrote: > Default memory settings of da850 do not meet the throughput/latency > requirements of tilcdc. This results in the image displayed being > incorrect and the following warning being displayed by the LCDC > drm driver: > > tilcdc da8xx_lcdc.0: tilcdc_crtc_irq(0x00000020): FIFO underfow > > Reconfigure the LCDC priority to the highest. This is a workaround > for the da850-lcdk board which has the LCD controller enabled in > the device tree, but a long-term, system-wide fix is needed for > all davinci boards. > > This patch has been modified for mainline linux. It comes from a > downstream TI release for da850[1]. > > Original author: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> > > [1] http://arago-project.org/git/projects/linux-davinci.git?p=projects/linux-davinci.git;a=commitdiff;h=b9bd39a34cc02c3ba2fc15539a2f0bc2b68d25da;hp=6f6c795faa6366a4ebc1037a0235edba6018a991 > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > arch/arm/mach-davinci/da8xx-dt.c | 43 ++++++++++++++++++++++++++++++ > arch/arm/mach-davinci/include/mach/da8xx.h | 4 +++ > 2 files changed, 47 insertions(+) > > diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c > index f8ecc02..9d29670 100644 > --- a/arch/arm/mach-davinci/da8xx-dt.c > +++ b/arch/arm/mach-davinci/da8xx-dt.c > @@ -44,9 +44,52 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = { > > #ifdef CONFIG_ARCH_DAVINCI_DA850 > > +/* > + * Adjust the default memory settings to cope with the LCDC > + * > + * REVISIT: This issue occurs on other davinci boards as well. Find > + * a proper system-wide fix. > + */ > +static void da850_lcdc_adjust_memory_bandwidth(void) > +{ > + void __iomem *cfg_mstpri1_base; > + void __iomem *cfg_mstpri2_base; > + void __iomem *emifb; > + u32 val; > + > + /* > + * Default master priorities in reg 0 are all lower by default than LCD > + * which is set below to 0. Hence don't need to change here. > + */ > + > + /* set EDMA30TC0 and TC1 to lower than LCDC (4 < 0) */ > + cfg_mstpri1_base = DA8XX_SYSCFG0_VIRT(DA8XX_MSTPRI1_REG); > + val = __raw_readl(cfg_mstpri1_base); > + val &= 0xFFFF00FF; > + val |= 4 << 8; /* 0-high, 7-low priority*/ > + val |= 4 << 12; /* 0-high, 7-low priority*/ > + __raw_writel(val, cfg_mstpri1_base); > + > + /* > + * Reconfigure the LCDC priority to the highest to ensure that > + * the throughput/latency requirements for the LCDC are met. > + */ > + cfg_mstpri2_base = DA8XX_SYSCFG0_VIRT(DA8XX_MSTPRI2_REG); > + > + val = __raw_readl(cfg_mstpri2_base); > + val &= 0x0fffffff; > + __raw_writel(val, cfg_mstpri2_base); > + > + /* set BPRIO */ > + emifb = ioremap(DA8XX_DDR_CTL_BASE, SZ_4K); > + __raw_writel(0x20, emifb + DA8XX_PBBPR_REG); > + iounmap(emifb); Is this safe to do for all da850 boards (to change PR_OLD_COUNT from 0xff to 0x20)? Most probably it is, but this setting has nothing to do with LCDC. The whole priority configuration has nothing to do with the LCDC, it is a system level priority. Now you have lowered the eDMA3_0-TPTC0/1 priority. Audio is serviced by eDMA3_0-TPTC1. So are we going to see issues in audio if LCDC is taking the highest priority? > +} > + > static void __init da850_init_machine(void) > { > of_platform_default_populate(NULL, da850_auxdata_lookup, NULL); > + da850_lcdc_adjust_memory_bandwidth(); > } > > static const char *const da850_boards_compat[] __initconst = { > diff --git a/arch/arm/mach-davinci/include/mach/da8xx.h b/arch/arm/mach-davinci/include/mach/da8xx.h > index f9f9713..5549eff 100644 > --- a/arch/arm/mach-davinci/include/mach/da8xx.h > +++ b/arch/arm/mach-davinci/include/mach/da8xx.h > @@ -56,6 +56,8 @@ extern unsigned int da850_max_speed; > #define DA8XX_SYSCFG0_VIRT(x) (da8xx_syscfg0_base + (x)) > #define DA8XX_JTAG_ID_REG 0x18 > #define DA8XX_HOST1CFG_REG 0x44 > +#define DA8XX_MSTPRI1_REG 0x114 > +#define DA8XX_MSTPRI2_REG 0x118 > #define DA8XX_CHIPSIG_REG 0x174 > #define DA8XX_CFGCHIP0_REG 0x17c > #define DA8XX_CFGCHIP1_REG 0x180 > @@ -79,6 +81,8 @@ extern unsigned int da850_max_speed; > #define DA8XX_AEMIF_CTL_BASE 0x68000000 > #define DA8XX_SHARED_RAM_BASE 0x80000000 > #define DA8XX_ARM_RAM_BASE 0xffff0000 > +#define DA8XX_DDR_CTL_BASE 0xB0000000 > +#define DA8XX_PBBPR_REG 0x00000020 > > void da830_init(void); > void da850_init(void); >
2016-09-30 14:59 GMT+02:00 Peter Ujfalusi <peter.ujfalusi@ti.com>: > On 09/29/16 19:31, Bartosz Golaszewski wrote: >> Default memory settings of da850 do not meet the throughput/latency >> requirements of tilcdc. This results in the image displayed being >> incorrect and the following warning being displayed by the LCDC >> drm driver: >> >> tilcdc da8xx_lcdc.0: tilcdc_crtc_irq(0x00000020): FIFO underfow >> >> Reconfigure the LCDC priority to the highest. This is a workaround >> for the da850-lcdk board which has the LCD controller enabled in >> the device tree, but a long-term, system-wide fix is needed for >> all davinci boards. >> >> This patch has been modified for mainline linux. It comes from a >> downstream TI release for da850[1]. >> >> Original author: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> >> [snip] > > Is this safe to do for all da850 boards (to change PR_OLD_COUNT from 0xff to > 0x20)? Most probably it is, but this setting has nothing to do with LCDC. > > The whole priority configuration has nothing to do with the LCDC, it is a > system level priority. > > Now you have lowered the eDMA3_0-TPTC0/1 priority. Audio is serviced by > eDMA3_0-TPTC1. So are we going to see issues in audio if LCDC is taking the > highest priority? > Just ran a quick test with speaker-test -c2 -twav. Besides the fact that the left and right channels are inverted (I'm looking into that), I didn't notice any problems. Even at 1024x768 resolution, playing audio at the same time seems to work fine. Best regards, Bartosz Golaszewski
On 09/30/2016 06:06 PM, Bartosz Golaszewski wrote: > 2016-09-30 14:59 GMT+02:00 Peter Ujfalusi <peter.ujfalusi@ti.com>: >> On 09/29/16 19:31, Bartosz Golaszewski wrote: >>> Default memory settings of da850 do not meet the throughput/latency >>> requirements of tilcdc. This results in the image displayed being >>> incorrect and the following warning being displayed by the LCDC >>> drm driver: >>> >>> tilcdc da8xx_lcdc.0: tilcdc_crtc_irq(0x00000020): FIFO underfow >>> >>> Reconfigure the LCDC priority to the highest. This is a workaround >>> for the da850-lcdk board which has the LCD controller enabled in >>> the device tree, but a long-term, system-wide fix is needed for >>> all davinci boards. >>> >>> This patch has been modified for mainline linux. It comes from a >>> downstream TI release for da850[1]. >>> >>> Original author: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> >>> > > [snip] > >> >> Is this safe to do for all da850 boards (to change PR_OLD_COUNT from 0xff to >> 0x20)? Most probably it is, but this setting has nothing to do with LCDC. >> >> The whole priority configuration has nothing to do with the LCDC, it is a >> system level priority. >> >> Now you have lowered the eDMA3_0-TPTC0/1 priority. Audio is serviced by >> eDMA3_0-TPTC1. So are we going to see issues in audio if LCDC is taking the >> highest priority? >> > > Just ran a quick test with speaker-test -c2 -twav. Besides the fact > that the left and right channels are inverted (I'm looking into that), > I didn't notice any problems. Even at 1024x768 resolution, playing > audio at the same time seems to work fine. That's good to hear, but I think the priorities should be set: LCDC and EDMA30TC1 to highest priority EDMA30TC0 to priority 2 The 0TC0 is used by MMC and if you want to play a video you might need the servicing TC to be higher priority then other masters. If audio playback would trigger sync losts in lcdc then we might need to move 0TC1 to priority 1. I agree that LCDC priority needs to be higher, but I do wonder why the default (5) is not working and if it is not working why it is 5... My guess is that the change in the PBBPR register is the one actually helping here.
On Saturday 01 October 2016 12:49 AM, Peter Ujfalusi wrote: > On 09/30/2016 06:06 PM, Bartosz Golaszewski wrote: >> 2016-09-30 14:59 GMT+02:00 Peter Ujfalusi <peter.ujfalusi@ti.com>: >>> On 09/29/16 19:31, Bartosz Golaszewski wrote: >>>> Default memory settings of da850 do not meet the throughput/latency >>>> requirements of tilcdc. This results in the image displayed being >>>> incorrect and the following warning being displayed by the LCDC >>>> drm driver: >>>> >>>> tilcdc da8xx_lcdc.0: tilcdc_crtc_irq(0x00000020): FIFO underfow >>>> >>>> Reconfigure the LCDC priority to the highest. This is a workaround >>>> for the da850-lcdk board which has the LCD controller enabled in >>>> the device tree, but a long-term, system-wide fix is needed for >>>> all davinci boards. >>>> >>>> This patch has been modified for mainline linux. It comes from a >>>> downstream TI release for da850[1]. >>>> >>>> Original author: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> >>>> >> >> [snip] >> >>> >>> Is this safe to do for all da850 boards (to change PR_OLD_COUNT from 0xff to >>> 0x20)? Most probably it is, but this setting has nothing to do with LCDC. >>> >>> The whole priority configuration has nothing to do with the LCDC, it is a >>> system level priority. >>> >>> Now you have lowered the eDMA3_0-TPTC0/1 priority. Audio is serviced by >>> eDMA3_0-TPTC1. So are we going to see issues in audio if LCDC is taking the >>> highest priority? >>> >> >> Just ran a quick test with speaker-test -c2 -twav. Besides the fact >> that the left and right channels are inverted (I'm looking into that), >> I didn't notice any problems. Even at 1024x768 resolution, playing >> audio at the same time seems to work fine. > > That's good to hear, but I think the priorities should be set: > LCDC and EDMA30TC1 to highest priority > EDMA30TC0 to priority 2 > > The 0TC0 is used by MMC and if you want to play a video you might need the > servicing TC to be higher priority then other masters. > > If audio playback would trigger sync losts in lcdc then we might need to move > 0TC1 to priority 1. > > I agree that LCDC priority needs to be higher, but I do wonder why the default > (5) is not working and if it is not working why it is 5... > > My guess is that the change in the PBBPR register is the one actually helping > here. Good point, Peter. If you are booting off NFS and not playing any audio, then there is pretty much no EDMA generated traffic on the interconnect. I would guess too that its the PBBPR setting that is making a difference. The EDMA vs LCDC priority adjustment might be needed in particular situations too, but specific experiments should be done to narrow down on that being the cause. In any case, to configure the PBBR, you will have to introduce a driver for it in drivers/memory. Then you can set it up per board using a DT parameter. Thanks, Sekhar
On 10/01/16 12:24, Sekhar Nori wrote: >> That's good to hear, but I think the priorities should be set: >> LCDC and EDMA30TC1 to highest priority >> EDMA30TC0 to priority 2 >> >> The 0TC0 is used by MMC and if you want to play a video you might need the >> servicing TC to be higher priority then other masters. >> >> If audio playback would trigger sync losts in lcdc then we might need to move >> 0TC1 to priority 1. >> >> I agree that LCDC priority needs to be higher, but I do wonder why the default >> (5) is not working and if it is not working why it is 5... >> >> My guess is that the change in the PBBPR register is the one actually helping >> here. > > Good point, Peter. If you are booting off NFS and not playing any audio, > then there is pretty much no EDMA generated traffic on the interconnect. Yes, this is my understanding as well. > I would guess too that its the PBBPR setting that is making a > difference. The EDMA vs LCDC priority adjustment might be needed in > particular situations too, but specific experiments should be done to > narrow down on that being the cause. True, we can use some conservative values for the priority, but the PBBPR register for sure needs to be updated. > In any case, to configure the PBBR, you will have to introduce a driver > for it in drivers/memory. Then you can set it up per board using a DT > parameter. and we can reuse the introduced bindings for am335x and OMAP1/2 as well. On OMAP the legacy DMA API provided a call to raise the priority of the sDMA in EMIF :o That needs to be removed and replaced.
2016-09-30 21:19 GMT+02:00 Peter Ujfalusi <peter.ujfalusi@ti.com>: > On 09/30/2016 06:06 PM, Bartosz Golaszewski wrote: >> >> Just ran a quick test with speaker-test -c2 -twav. Besides the fact >> that the left and right channels are inverted (I'm looking into that), >> I didn't notice any problems. Even at 1024x768 resolution, playing >> audio at the same time seems to work fine. > Hi Peter > That's good to hear, but I think the priorities should be set: > LCDC and EDMA30TC1 to highest priority > EDMA30TC0 to priority 2 > > The 0TC0 is used by MMC and if you want to play a video you might need the > servicing TC to be higher priority then other masters. > > If audio playback would trigger sync losts in lcdc then we might need to move > 0TC1 to priority 1. > Did you mean "set EDMA31TC0 to priority 2"? EDMA30TC0 is already at the highest priority. Or did you mean that we need to lower the EDMA30TC0 priority? In that case: is 2 the correct value? EDMA31TC0 is used by mmc1 and its priority is 4. Shouldn't we set both to be the same? > I agree that LCDC priority needs to be higher, but I do wonder why the default > (5) is not working and if it is not working why it is 5... > > My guess is that the change in the PBBPR register is the one actually helping > here. > While it seems that lowering the EDMA30TC0 priority is indeed unnecessary, if I don't set the LCDC master to priority 0, I still get FIFO underflows even with the change in PBBPR. Thanks, Bartosz
Peter Ujfalusi <peter.ujfalusi@ti.com> writes: > On 10/01/16 12:24, Sekhar Nori wrote: [...] >> In any case, to configure the PBBR, you will have to introduce a driver >> for it in drivers/memory. Then you can set it up per board using a DT >> parameter. > > and we can reuse the introduced bindings for am335x and OMAP1/2 as well. On > OMAP the legacy DMA API provided a call to raise the priority of the sDMA in > EMIF :o That needs to be removed and replaced. Can you point us to the bindings you're referring to? Also, a new driver in drivers/memory is fine for setting the PBBR, but what about the SYSCFG0 registers. Are you OK with leaving those in the init code as proposed in $SUBJECT patch? Kevin
On 10/04/16 12:20, Bartosz Golaszewski wrote: > 2016-09-30 21:19 GMT+02:00 Peter Ujfalusi <peter.ujfalusi@ti.com>: >> On 09/30/2016 06:06 PM, Bartosz Golaszewski wrote: >>> >>> Just ran a quick test with speaker-test -c2 -twav. Besides the fact >>> that the left and right channels are inverted (I'm looking into that), >>> I didn't notice any problems. Even at 1024x768 resolution, playing >>> audio at the same time seems to work fine. >> > > Hi Peter > >> That's good to hear, but I think the priorities should be set: >> LCDC and EDMA30TC1 to highest priority >> EDMA30TC0 to priority 2 >> >> The 0TC0 is used by MMC and if you want to play a video you might need the >> servicing TC to be higher priority then other masters. >> >> If audio playback would trigger sync losts in lcdc then we might need to move >> 0TC1 to priority 1. >> > > Did you mean "set EDMA31TC0 to priority 2"? EDMA30TC0 is already at > the highest priority. Or did you mean that we need to lower the > EDMA30TC0 priority? In that case: is 2 the correct value? EDMA31TC0 is > used by mmc1 and its priority is 4. Shouldn't we set both to be the > same? What I mean is: EMDA30TC1 = LCDC = 0; EDMA30TC0 = 2; I don't know what the MMC1 is used on the LCDK, but it is only used by add-on card on Logic's OMAP-L138 EVM (wifi if I recall right). I would leave EDMA31TC0 as 4. And while we are here: this is my problem, I don't want LCDC to be high priority on OMAP-L138 EVM as it has no display by default, no point of fiddling with the EMIF priority there. This is the reason why we should have a way via DT to set these priorities. A DT fragment for OMAP-L138 EVM's wifi module might want to increase the EDMA31TC0 priority to get better throughput, but on a bare board we don't want that. If I would have the display module for the EVM, I would like to rise the LCDC priority also, but I don't have one. If I have the aduio add-on module I would want to keep audio as the highest priority (and the MMC0 to be able to record/play audio files). etc. >> I agree that LCDC priority needs to be higher, but I do wonder why the default >> (5) is not working and if it is not working why it is 5... >> >> My guess is that the change in the PBBPR register is the one actually helping >> here. >> > > While it seems that lowering the EDMA30TC0 priority is indeed > unnecessary, if I don't set the LCDC master to priority 0, I still get > FIFO underflows even with the change in PBBPR. I believe it is valid to raise the LCDC priority, but I wonder what blocks the LCDC accesses when you don't really have anything going on in the background.
On 10/04/16 16:02, Kevin Hilman wrote: > Peter Ujfalusi <peter.ujfalusi@ti.com> writes: > >> On 10/01/16 12:24, Sekhar Nori wrote: > > [...] > >>> In any case, to configure the PBBR, you will have to introduce a driver >>> for it in drivers/memory. Then you can set it up per board using a DT >>> parameter. >> >> and we can reuse the introduced bindings for am335x and OMAP1/2 as well. On >> OMAP the legacy DMA API provided a call to raise the priority of the sDMA in >> EMIF :o That needs to be removed and replaced. > > Can you point us to the bindings you're referring to? We don't have one atm. And the DMA priority hack in legacy sDMA code is for OMAP1: omap_set_dma_priority(). Basically it can change the sDMA priority in OCPT1_PRIOR, OCPT2_PRIOR, EMIFF_PRIOR and EMIFS_PRIOR registers. > Also, a new driver in drivers/memory is fine for setting the PBBR, but > what about the SYSCFG0 registers. Are you OK with leaving those in the > init code as proposed in $SUBJECT patch? My problem is - as I described it in reply to Bartosz - is that for example I don't want the LCDC to get high priority on OMAP-L138 EVM from Logic as it does not have LCD/VGA by default. ifdef for LCDC is not good either since my kernel have LCDC compiled in, but it is disabled. The easiest way would be to have pdata quirk to handle the LCDK until we have proper handling of the priority configuration.
On Wednesday 05 October 2016 01:52 PM, Peter Ujfalusi wrote: > On 10/04/16 16:02, Kevin Hilman wrote: >> Peter Ujfalusi <peter.ujfalusi@ti.com> writes: >> >>> On 10/01/16 12:24, Sekhar Nori wrote: >> >> [...] >> >>>> In any case, to configure the PBBR, you will have to introduce a driver >>>> for it in drivers/memory. Then you can set it up per board using a DT >>>> parameter. >>> >>> and we can reuse the introduced bindings for am335x and OMAP1/2 as well. On >>> OMAP the legacy DMA API provided a call to raise the priority of the sDMA in >>> EMIF :o That needs to be removed and replaced. >> >> Can you point us to the bindings you're referring to? > > We don't have one atm. > > And the DMA priority hack in legacy sDMA code is for OMAP1: > omap_set_dma_priority(). Basically it can change the sDMA priority in > OCPT1_PRIOR, OCPT2_PRIOR, EMIFF_PRIOR and EMIFS_PRIOR registers. > >> Also, a new driver in drivers/memory is fine for setting the PBBR, but >> what about the SYSCFG0 registers. Are you OK with leaving those in the >> init code as proposed in $SUBJECT patch? > > My problem is - as I described it in reply to Bartosz - is that for example I > don't want the LCDC to get high priority on OMAP-L138 EVM from Logic as it > does not have LCD/VGA by default. ifdef for LCDC is not good either since my > kernel have LCDC compiled in, but it is disabled. > > The easiest way would be to have pdata quirk to handle the LCDK until we have > proper handling of the priority configuration. We don't have pdata quirks handling in mach-davinci. And I think instead of investing time in adding pdata quirks which are anyway a short term solution, it is better to work on a drivers/bus/ based driver which can help adjust master priorities via DT. Although as Peter asked, it is indeed intriguing as to why LCDC priority has to be raised even in supposed absence of any competing traffic. Thanks, Sekhar
diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c index f8ecc02..9d29670 100644 --- a/arch/arm/mach-davinci/da8xx-dt.c +++ b/arch/arm/mach-davinci/da8xx-dt.c @@ -44,9 +44,52 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = { #ifdef CONFIG_ARCH_DAVINCI_DA850 +/* + * Adjust the default memory settings to cope with the LCDC + * + * REVISIT: This issue occurs on other davinci boards as well. Find + * a proper system-wide fix. + */ +static void da850_lcdc_adjust_memory_bandwidth(void) +{ + void __iomem *cfg_mstpri1_base; + void __iomem *cfg_mstpri2_base; + void __iomem *emifb; + u32 val; + + /* + * Default master priorities in reg 0 are all lower by default than LCD + * which is set below to 0. Hence don't need to change here. + */ + + /* set EDMA30TC0 and TC1 to lower than LCDC (4 < 0) */ + cfg_mstpri1_base = DA8XX_SYSCFG0_VIRT(DA8XX_MSTPRI1_REG); + val = __raw_readl(cfg_mstpri1_base); + val &= 0xFFFF00FF; + val |= 4 << 8; /* 0-high, 7-low priority*/ + val |= 4 << 12; /* 0-high, 7-low priority*/ + __raw_writel(val, cfg_mstpri1_base); + + /* + * Reconfigure the LCDC priority to the highest to ensure that + * the throughput/latency requirements for the LCDC are met. + */ + cfg_mstpri2_base = DA8XX_SYSCFG0_VIRT(DA8XX_MSTPRI2_REG); + + val = __raw_readl(cfg_mstpri2_base); + val &= 0x0fffffff; + __raw_writel(val, cfg_mstpri2_base); + + /* set BPRIO */ + emifb = ioremap(DA8XX_DDR_CTL_BASE, SZ_4K); + __raw_writel(0x20, emifb + DA8XX_PBBPR_REG); + iounmap(emifb); +} + static void __init da850_init_machine(void) { of_platform_default_populate(NULL, da850_auxdata_lookup, NULL); + da850_lcdc_adjust_memory_bandwidth(); } static const char *const da850_boards_compat[] __initconst = { diff --git a/arch/arm/mach-davinci/include/mach/da8xx.h b/arch/arm/mach-davinci/include/mach/da8xx.h index f9f9713..5549eff 100644 --- a/arch/arm/mach-davinci/include/mach/da8xx.h +++ b/arch/arm/mach-davinci/include/mach/da8xx.h @@ -56,6 +56,8 @@ extern unsigned int da850_max_speed; #define DA8XX_SYSCFG0_VIRT(x) (da8xx_syscfg0_base + (x)) #define DA8XX_JTAG_ID_REG 0x18 #define DA8XX_HOST1CFG_REG 0x44 +#define DA8XX_MSTPRI1_REG 0x114 +#define DA8XX_MSTPRI2_REG 0x118 #define DA8XX_CHIPSIG_REG 0x174 #define DA8XX_CFGCHIP0_REG 0x17c #define DA8XX_CFGCHIP1_REG 0x180 @@ -79,6 +81,8 @@ extern unsigned int da850_max_speed; #define DA8XX_AEMIF_CTL_BASE 0x68000000 #define DA8XX_SHARED_RAM_BASE 0x80000000 #define DA8XX_ARM_RAM_BASE 0xffff0000 +#define DA8XX_DDR_CTL_BASE 0xB0000000 +#define DA8XX_PBBPR_REG 0x00000020 void da830_init(void); void da850_init(void);
Default memory settings of da850 do not meet the throughput/latency requirements of tilcdc. This results in the image displayed being incorrect and the following warning being displayed by the LCDC drm driver: tilcdc da8xx_lcdc.0: tilcdc_crtc_irq(0x00000020): FIFO underfow Reconfigure the LCDC priority to the highest. This is a workaround for the da850-lcdk board which has the LCD controller enabled in the device tree, but a long-term, system-wide fix is needed for all davinci boards. This patch has been modified for mainline linux. It comes from a downstream TI release for da850[1]. Original author: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> [1] http://arago-project.org/git/projects/linux-davinci.git?p=projects/linux-davinci.git;a=commitdiff;h=b9bd39a34cc02c3ba2fc15539a2f0bc2b68d25da;hp=6f6c795faa6366a4ebc1037a0235edba6018a991 Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> --- arch/arm/mach-davinci/da8xx-dt.c | 43 ++++++++++++++++++++++++++++++ arch/arm/mach-davinci/include/mach/da8xx.h | 4 +++ 2 files changed, 47 insertions(+)