[Sound-open-firmware] [PATCH v2] cnl: Add missing host and link DMA init
CONFIG_DMA_GW is set for Cannonlake, so add host and link initialization to avoid FW crash.
Signed-off-by: Tomasz Lauda tomasz.lauda@linux.intel.com --- src/drivers/hda-dma.c | 2 +- src/platform/cannonlake/dma.c | 13 +++++++++---- src/platform/cannonlake/platform.c | 10 ++++++++++ 3 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/src/drivers/hda-dma.c b/src/drivers/hda-dma.c index 226a34e..6526b74 100644 --- a/src/drivers/hda-dma.c +++ b/src/drivers/hda-dma.c @@ -77,7 +77,7 @@ /* DGBS */ #define DGBS_MASK 0xfffff0
-#define HDA_DMA_MAX_CHANS 8 +#define HDA_DMA_MAX_CHANS 9
struct hda_chan_data { uint32_t stream_id; diff --git a/src/platform/cannonlake/dma.c b/src/platform/cannonlake/dma.c index beac6cf..a47ce9f 100644 --- a/src/platform/cannonlake/dma.c +++ b/src/platform/cannonlake/dma.c @@ -32,6 +32,7 @@
#include <reef/dma.h> #include <reef/dw-dma.h> +#include <reef/hd-dma.h> #include <platform/memory.h> #include <platform/interrupt.h> #include <platform/dma.h> @@ -133,33 +134,37 @@ static struct dma dma[] = { .plat_data = { .id = DMA_HOST_IN_DMAC, .base = GTW_HOST_IN_STREAM_BASE(0), - .channels = 7, + .channels = 9, .irq = IRQ_EXT_HOST_DMA_IN_LVL3(0, 0), }, + .ops = &hda_host_dma_ops, }, { /* Host out DMAC */ .plat_data = { .id = DMA_HOST_OUT_DMAC, .base = GTW_HOST_OUT_STREAM_BASE(0), - .channels = 6, + .channels = 7, .irq = IRQ_EXT_HOST_DMA_OUT_LVL3(0, 0), }, + .ops = &hda_host_dma_ops, }, { /* Link In DMAC */ .plat_data = { .id = DMA_LINK_IN_DMAC, .base = GTW_LINK_IN_STREAM_BASE(0), - .channels = 8, + .channels = 9, .irq = IRQ_EXT_LINK_DMA_IN_LVL4(0, 0), }, + .ops = &hda_link_dma_ops, }, { /* Link out DMAC */ .plat_data = { .id = DMA_LINK_OUT_DMAC, .base = GTW_LINK_OUT_STREAM_BASE(0), - .channels = 8, + .channels = 7, .irq = IRQ_EXT_LINK_DMA_OUT_LVL4(0, 0), }, + .ops = &hda_link_dma_ops, },};
struct dma *dma_get(int dmac_id) diff --git a/src/platform/cannonlake/platform.c b/src/platform/cannonlake/platform.c index f387109..beb6582 100644 --- a/src/platform/cannonlake/platform.c +++ b/src/platform/cannonlake/platform.c @@ -253,6 +253,16 @@ int platform_init(struct reef *reef) return -ENODEV; dma_probe(dmac);
+ dmac = dma_get(DMA_HOST_OUT_DMAC); + if (!dmac) + return -ENODEV; + dma_probe(dmac); + + dmac = dma_get(DMA_HOST_IN_DMAC); + if (!dmac) + return -ENODEV; + dma_probe(dmac); + /* init SSP ports */ trace_point(TRACE_BOOT_PLATFORM_SSP); for(i = 0; i < PLATFORM_SSP_COUNT; i++) {
Looking at the code and latest patches I see quite a few differences between APL and CNL that makes no sense to me.
1. why would use a workqueue based on SSP (APL) and CPU (CNL)? 2. why would we initialize the SSP to 25MHz for CNL? It's clearly not a supported frequency, 25MHz can only be reached on BYT and CHT. On a similar note not sure why APL makes a reference to 24 MHz, it should be 24576000. 3. why would we use DMAC class 1/2 on APL and class 6/7 on CNL?
Conversely I always thought there were differences in the HDaudio DMA channel count between APL and CNL, so can we really share common definitions in src/drivers/hda-dma.c ?
Thanks -Pierre
-----Original Message----- From: sound-open-firmware-bounces@alsa-project.org [mailto:sound-open- firmware-bounces@alsa-project.org] On Behalf Of Pierre-Louis Bossart Sent: Wednesday, February 21, 2018 3:34 AM To: sound-open-firmware@alsa-project.org Subject: [Sound-open-firmware] CNL/APL alignment
Looking at the code and latest patches I see quite a few differences between APL and CNL that makes no sense to me.
- why would use a workqueue based on SSP (APL) and CPU (CNL)?
Currently the workqueue is using wall clock on both APL and CNL, this is 19.2MHz on APL and 24MHz on CNL, so I think we should use SSP(or another new created type?) on both APL and CNL.
- why would we initialize the SSP to 25MHz for CNL? It's clearly not a
supported frequency, 25MHz can only be reached on BYT and CHT. On a similar note not sure why APL makes a reference to 24 MHz, it should be 24576000.
On APL, it is now set to 19.2MHz at platform_init, I had plan to enable this (without ECS set) before CNY but failed to get it work.
For CNL, I agree it should be 24MHz, let's change it.
- why would we use DMAC class 1/2 on APL and class 6/7 on CNL?
Actually this doesn't impact the usage of DMAC for us during my testing, but right, we should make it consistent for APL and CNL.
Liam, any comment on this?
Conversely I always thought there were differences in the HDaudio DMA channel count between APL and CNL, so can we really share common definitions in src/drivers/hda-dma.c ?
Yes we can, the difference(channel count, irq num, dmac list, ...) is listed in platform/dma.c dma array.
Thanks, ~Keyon
Thanks -Pierre _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On 02/21/2018 05:18 AM, Jie, Yang wrote:
-----Original Message----- From: sound-open-firmware-bounces@alsa-project.org [mailto:sound-open- firmware-bounces@alsa-project.org] On Behalf Of Pierre-Louis Bossart Sent: Wednesday, February 21, 2018 3:34 AM To: sound-open-firmware@alsa-project.org Subject: [Sound-open-firmware] CNL/APL alignment
Looking at the code and latest patches I see quite a few differences between APL and CNL that makes no sense to me.
- why would use a workqueue based on SSP (APL) and CPU (CNL)?
Currently the workqueue is using wall clock on both APL and CNL, this is 19.2MHz on APL and 24MHz on CNL, so I think we should use SSP(or another new created type?) on both APL and CNL.
Nope, it's CLK_CPU for CNL, see code below:
CNL: static struct work_queue_timesource platform_generic_queue = { .timer = { .id = TIMER3, /* external timer */ .irq = IRQ_EXT_TSTAMP0_LVL2(0), }, .clk = CLK_CPU, .notifier = NOTIFIER_ID_CPU_FREQ, .timer_set = platform_timer_set, .timer_clear = platform_timer_clear, .timer_get = platform_timer_get, };
APL: static struct work_queue_timesource platform_generic_queue = { .timer = { .id = TIMER3, /* external timer, XTAL 19.2M */ .irq = IRQ_EXT_TSTAMP0_LVL2(0), }, .clk = CLK_SSP, .notifier = NOTIFIER_ID_SSP_FREQ, .timer_set = platform_timer_set, .timer_clear = platform_timer_clear, .timer_get = platform_timer_get, };
- why would we initialize the SSP to 25MHz for CNL? It's clearly not a
supported frequency, 25MHz can only be reached on BYT and CHT. On a similar note not sure why APL makes a reference to 24 MHz, it should be 24576000.
On APL, it is now set to 19.2MHz at platform_init, I had plan to enable this (without ECS set) before CNY but failed to get it work.
For CNL, I agree it should be 24MHz, let's change it.
ok
- why would we use DMAC class 1/2 on APL and class 6/7 on CNL?
Actually this doesn't impact the usage of DMAC for us during my testing, but right, we should make it consistent for APL and CNL.
Liam, any comment on this?
Conversely I always thought there were differences in the HDaudio DMA channel count between APL and CNL, so can we really share common definitions in src/drivers/hda-dma.c ?
Yes we can, the difference(channel count, irq num, dmac list, ...) is listed in platform/dma.c dma array.
for APL, the code shows host in: 7 channels host out: 6 channels link in: 8 channels link out: 8 channels
for CNL, the last patch from Tomasz says: host in: 9 channels host out: 7 channels link in: 9 channels link out: 7 channels
with HDA_DMA_MAX_CHANS modified to 9.
But if src/drivers/hda-dma.c, we do this: /* init channel status */ for (i = 0; i < HDA_DMA_MAX_CHANS; i++) hda_pdata->chan[i].status = COMP_STATE_INIT;
So that means we declare some DMA channels as initialized, even though they might not actually exist, and we are not making any distinctions between in and out channels, which is somewhat odd.
Thanks, ~Keyon
Thanks -Pierre _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
-----Original Message----- From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com] Sent: Wednesday, February 21, 2018 11:15 PM To: Jie, Yang yang.jie@intel.com; sound-open-firmware@alsa-project.org Subject: Re: [Sound-open-firmware] CNL/APL alignment
On 02/21/2018 05:18 AM, Jie, Yang wrote:
-----Original Message----- From: sound-open-firmware-bounces@alsa-project.org [mailto:sound-open- firmware-bounces@alsa-project.org] On Behalf Of Pierre-Louis Bossart Sent: Wednesday, February 21, 2018 3:34 AM To: sound-open-firmware@alsa-project.org Subject: [Sound-open-firmware] CNL/APL alignment
Looking at the code and latest patches I see quite a few differences between APL and CNL that makes no sense to me.
- why would use a workqueue based on SSP (APL) and CPU (CNL)?
Currently the workqueue is using wall clock on both APL and CNL, this is 19.2MHz on APL and 24MHz on CNL, so I think we should use SSP(or another new created type?) on both APL and CNL.
Nope, it's CLK_CPU for CNL, see code below:
CNL: static struct work_queue_timesource platform_generic_queue = { .timer = { .id = TIMER3, /* external timer */ .irq = IRQ_EXT_TSTAMP0_LVL2(0), }, .clk = CLK_CPU, .notifier = NOTIFIER_ID_CPU_FREQ, .timer_set = platform_timer_set, .timer_clear = platform_timer_clear, .timer_get = platform_timer_get, };
Let's correct this.
APL: static struct work_queue_timesource platform_generic_queue = { .timer = { .id = TIMER3, /* external timer, XTAL 19.2M */ .irq = IRQ_EXT_TSTAMP0_LVL2(0), }, .clk = CLK_SSP, .notifier = NOTIFIER_ID_SSP_FREQ, .timer_set = platform_timer_set, .timer_clear = platform_timer_clear, .timer_get = platform_timer_get, };
- why would we initialize the SSP to 25MHz for CNL? It's clearly not a
supported frequency, 25MHz can only be reached on BYT and CHT. On a similar note not sure why APL makes a reference to 24 MHz, it should be 24576000.
On APL, it is now set to 19.2MHz at platform_init, I had plan to enable this (without ECS set) before CNY but failed to get it work.
For CNL, I agree it should be 24MHz, let's change it.
ok
- why would we use DMAC class 1/2 on APL and class 6/7 on CNL?
Actually this doesn't impact the usage of DMAC for us during my testing, but right, we should make it consistent for APL and CNL.
Liam, any comment on this?
Conversely I always thought there were differences in the HDaudio DMA channel count between APL and CNL, so can we really share common definitions in src/drivers/hda-dma.c ?
Yes we can, the difference(channel count, irq num, dmac list, ...) is listed in platform/dma.c dma array.
for APL, the code shows host in: 7 channels host out: 6 channels link in: 8 channels link out: 8 channels
for CNL, the last patch from Tomasz says: host in: 9 channels host out: 7 channels link in: 9 channels link out: 7 channels
with HDA_DMA_MAX_CHANS modified to 9.
But if src/drivers/hda-dma.c, we do this: /* init channel status */ for (i = 0; i < HDA_DMA_MAX_CHANS; i++) hda_pdata->chan[i].status = COMP_STATE_INIT;
So that means we declare some DMA channels as initialized, even though they might not actually exist, and we are not making any distinctions between in and out channels, which is somewhat odd.
Good finding, we should change to use dma->plat_data.channels, let me do a patch for that.
Thanks, ~Keyon
Thanks, ~Keyon
Thanks -Pierre _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
participants (3)
-
Jie, Yang -
Pierre-Louis Bossart -
Tomasz Lauda