4.13.0-next-20170904 WARNING: possible circular locking dependency detected
found on booting. kvm bare metal host machine. == WARNING: possible circular locking dependency detected 4.13.0-next-20170904 #1 Tainted: GW -- ppc64_cpu/4940 is trying to acquire lock: ((complete)&st->done){+.+.}, at: [] takedown_cpu+0xe8/0x1c0 but task is already holding lock: (sparse_irq_lock){+.+.}, at: [] irq_lock_sparse+0x2c/0x40 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #2 (sparse_irq_lock){+.+.}: lock_acquire+0xdc/0x2f0 __mutex_lock+0xac/0xb60 irq_lock_sparse+0x2c/0x40 irq_affinity_online_cpu+0x2c/0x1f0 cpuhp_invoke_callback+0x1b4/0xd90 cpuhp_up_callbacks+0x58/0x150 cpuhp_thread_fun+0x1cc/0x210 smpboot_thread_fn+0x2fc/0x370 kthread+0x200/0x210 ret_from_kernel_thread+0x5c/0x70 -> #1 (cpuhp_state){+.+.}: smpboot_thread_fn+0x2fc/0x370 kthread+0x200/0x210 ret_from_kernel_thread+0x5c/0x70 -> #0 ((complete)&st->done){+.+.}: __lock_acquire+0x16a4/0x17f0 lock_acquire+0xdc/0x2f0 wait_for_common+0x8c/0x250 takedown_cpu+0xe8/0x1c0 cpuhp_invoke_callback+0x1b4/0xd90 cpuhp_down_callbacks+0x78/0xf0 _cpu_down+0x150/0x1b0 do_cpu_down+0x68/0xb0 cpu_subsys_offline+0x2c/0x50 device_offline+0xcc/0x150 online_store+0x6c/0xc0 dev_attr_store+0x68/0xa0 sysfs_kf_write+0x74/0xc0 kernfs_fop_write+0x194/0x270 __vfs_write+0x6c/0x1d0 vfs_write+0xd8/0x240 SyS_write+0x6c/0x110 system_call+0x58/0x6c other info that might help us debug this: Chain exists of: (complete)&st->done --> cpuhp_state --> sparse_irq_lock Possible unsafe locking scenario: CPU0CPU1 lock(sparse_irq_lock); lock(cpuhp_state); lock(sparse_irq_lock); lock((complete)&st->done); *** DEADLOCK *** 8 locks held by ppc64_cpu/4940: #0: (sb_writers#6){.+.+}, at: [] vfs_write+0x210/0x240 #1: (&of->mutex){+.+.}, at: [] kernfs_fop_write+0x148/0x270 #2: (kn->count#130){.+.+}, at: [] kernfs_fop_write+0x154/0x270 #3: (device_hotplug_lock){+.+.}, at: [] lock_device_hotplug_sysfs+0x28/0x80 #4: (&dev->mutex){}, at: [] device_offline+0x84/0x150 #5: (cpu_add_remove_lock){+.+.}, at: [] do_cpu_down+0x44/0xb0 #6: (cpu_hotplug_lock.rw_sem){}, at: [] percpu_down_write+0x4c/0x160 #7: (sparse_irq_lock){+.+.}, at: [] irq_lock_sparse+0x2c/0x40 stack backtrace: CPU: 2 PID: 4940 Comm: ppc64_cpu Tainted: GW 4.13.0-next-20170904 #1 Call Trace: [c01fb5d83510] [c0bcb7ec] dump_stack+0xe8/0x15c (unreliable) [c01fb5d83550] [c0199478] print_circular_bug+0x2e8/0x4f0 [c01fb5d83600] [c019ab58] check_prev_add+0x468/0xa90 [c01fb5d83700] [c019e684] __lock_acquire+0x16a4/0x17f0 [c01fb5d83830] [c019f47c] lock_acquire+0xdc/0x2f0 [c01fb5d83900] [c0be7dcc] wait_for_common+0x8c/0x250 [c01fb5d83980] [c010e198] takedown_cpu+0xe8/0x1c0 [c01fb5d839e0] [c010e5b4] cpuhp_invoke_callback+0x1b4/0xd90 [c01fb5d83a70] [c010f318] cpuhp_down_callbacks+0x78/0xf0 [c01fb5d83ac0] [c0112c30] _cpu_down+0x150/0x1b0 [c01fb5d83b20] [c0110598] do_cpu_down+0x68/0xb0 [c01fb5d83b60] [c07a54ac] cpu_subsys_offline+0x2c/0x50 [c01fb5d83b90] [c079c37c] device_offline+0xcc/0x150 [c01fb5d83bd0] [c079c58c] online_store+0x6c/0xc0 [c01fb5d83c20] [c0796aa8] dev_attr_store+0x68/0xa0 [c01fb5d83c60] [c04bd954] sysfs_kf_write+0x74/0xc0 [c01fb5d83ca0] [c04bc1d4] kernfs_fop_write+0x194/0x270 [c01fb5d83cf0] [c03ec6bc] __vfs_write+0x6c/0x1d0 [c01fb5d83d90] [c03ee718] vfs_write+0xd8/0x240 [c01fb5d83de0] [c03f06ac] SyS_write+0x6c/0x110 [c01fb5d83e30] [c000b96c] system_call+0x58/0x6c [ OK ] Started User Manager for UID 1004. nf_conntrack version 0.5.0 (65536 buckets, 262144 max) ip_tables: (C) 2000-2006 Netfilter Core Team Ubuntu 16.04.2 LTS p87 hvc0 p87 login: Initializing XFRM netlink socket [ cut here ] WARNING: CPU: 48 PID: 5464 at net/netfilter/core.c:218 __nf_hook_entries_try_shrink+0xd4/0x270 Modules linked in: ipt_MASQUERADE nf_nat_masquerade_ipv4 xfrm_user xfrm_algo iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 xt_addrtype iptable_filter ip_tables xt_conntrack x_tables nf_nat nf_conntrack overlay bridge stp llc binfmt_misc vmx_crypto leds_powernv led_class powernv_op_panel powernv_rng rng_core kvm_hv kvm autofs4 xfs btrfs zstd_decompress zstd_compress xxhash lzo_compress raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq libcrc32c multipath mlx4_en crc32c_vpmsum raid10 lpfc be2n
oops: arch_update_cpu_topology -> lockdep_assert_cpus_held
4.13-rc3 mainline at b1b6f83ac938d176742c85757960dec2cf10e468 Backtrace occurred for every CPU during boot. its a kvm host machine. [ 254.131688] new mount options do not match the existing superblock, will be ignored [ 255.654622] [ cut here ] [ 255.654658] WARNING: CPU: 1 PID: 14 at kernel/cpu.c:240 lockdep_assert_cpus_held+0x54/0x80 [ 255.654661] Modules linked in: bridge stp llc binfmt_misc kvm_hv kvm leds_powernv vmx_crypto powernv_rng rng_core led_class powernv_op_panel autofs4 xfs btrfs lzo_compress raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq libcrc32c multipath mlx4_en raid10 crc32c_vpmsum lpfc be2net crc_t10dif crct10dif_generic crct10dif_common mlx4_core [ 255.654734] CPU: 1 PID: 14 Comm: cpuhp/1 Tainted: G W 4.13.0 #1 [ 255.654737] task: c01ff25fa100 task.stack: c01ff2578000 [ 255.654740] NIP: c00f8624 LR: c00f8618 CTR: c01763e0 [ 255.654742] REGS: c01ff257b780 TRAP: 0700 Tainted: G W (4.13.0) [ 255.654744] MSR: 90029033 [ 255.654764] CR: 24200422 XER: [ 255.654766] CFAR: c01783d0 SOFTE: 1 GPR00: c00f8618 c01ff257ba00 c1042100 GPR04: 0001 GPR08: 001ffe49 c07fee782f50 GPR12: cfd80500 c012c878 c01ff6209180 GPR16: c0ef2728 GPR20: 0001 GPR24: c1087d50 c0fe0e7d c01448c0 GPR28: 001ffe49 0002 c1088050 [ 255.654842] NIP [c00f8624] lockdep_assert_cpus_held+0x54/0x80 [ 255.654845] LR [c00f8618] lockdep_assert_cpus_held+0x48/0x80 [ 255.654847] Call Trace: [ 255.654851] [c01ff257ba00] [c00f8618] lockdep_assert_cpus_held+0x48/0x80 (unreliable) [ 255.654858] [c01ff257ba20] [c007a848] arch_update_cpu_topology+0x18/0x30 [ 255.654864] [c01ff257ba40] [c016bd0c] partition_sched_domains+0x8c/0x4e0 [ 255.654870] [c01ff257baf0] [c020a914] cpuset_update_active_cpus+0x24/0x60 [ 255.654876] [c01ff257bb10] [c01449bc] sched_cpu_deactivate+0xfc/0x1a0 [ 255.654882] [c01ff257bc20] [c00f5a8c] cpuhp_invoke_callback+0x19c/0xe00 [ 255.654888] [c01ff257bcb0] [c00f6868] cpuhp_down_callbacks+0x78/0xf0 [ 255.654893] [c01ff257bd00] [c00f6c1c] cpuhp_thread_fun+0x1fc/0x210 [ 255.654899] [c01ff257bd50] [c0132f4c] smpboot_thread_fn+0x2fc/0x370 [ 255.654905] [c01ff257bdc0] [c012ca24] kthread+0x1b4/0x1c0 [ 255.654911] [c01ff257be30] [c000bcec] ret_from_kernel_thread+0x5c/0x70 [ 255.654914] Instruction dump: [ 255.654919] 4e800020 6000 6042 7c0802a6 3c62ffeb 3880 38639280 f8010030 [ 255.654934] 4807fd45 6000 2fa3 409e0020 <0fe0> e8010030 38210020 7c0803a6 [ 255.654950] ---[ end trace 06efa323f571f14b ]--- [ 255.894356] [ cut here ] [ 255.894367] WARNING: CPU: 2 PID: 20 at kernel/cpu.c:240 lockdep_assert_cpus_held+0x54/0x80 [ 255.894370] Modules linked in: bridge stp llc binfmt_misc kvm_hv kvm leds_powernv vmx_crypto powernv_rng rng_core led_class powernv_op_panel autofs4 xfs btrfs lzo_compress raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq libcrc32c multipath mlx4_en raid10 crc32c_vpmsum lpfc be2net crc_t10dif crct10dif_generic crct10dif_common mlx4_core [ 255.894438] CPU: 2 PID: 20 Comm: cpuhp/2 Tainted: G W 4.13.0 #1 [ 255.894441] task: c01ff25e8a00 task.stack: c01ff270 [ 255.89] NIP: c00f8624 LR: c00f8618 CTR: c0146650 [ 255.894447] REGS: c01ff2703780 TRAP: 0700 Tainted: G W (4.13.0) [ 255.894448] MSR: 90029033 [ 255.894468] CR: 24200422 XER: [ 255.894470] CFAR: c01783d0 SOFTE: 1 GPR00: c00f8618 c01ff2703a00 c1042100 GPR04: 0006 c01fe60ec000 0001 GPR08: c07fe9acf550 GPR12: cfd80a00 c012c878 c01ff6209180 GPR16: c0ef2728 GPR20: 0001 GPR24: c1087d50 c0fe0e7d c01448c0 GPR28: 001ffe4d 0004 c1088050 [ 255.894543] NIP [c00f8624] lockdep_assert_cpus_held+0x54/0x80 [ 255.894546] LR [c00f8618] lockdep_assert_cpus_held+0x48/0x80 [ 255.894549] Call Trace: [ 255.894552] [c01ff2703a00] [c00f8618] lockdep_assert_cpus_held+0x48/0x80 (unreliable) [ 255.894560] [c01ff2703a20] [c007a848] arch_update_cpu_topology+0x18/0x30 [ 255.894566] [c000
Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq
On 09/05/2017 07:06 AM, Nicolin Chen wrote: On Sun, Sep 03, 2017 at 01:05:01PM +0200, Lukasz Majewski wrote: The problem is visible in the following setup (on the imx6q): "simple-audio-card" -> ssi2 -> I2S + I2C -> codec The function call log (simple-card probe -> CONFIG_SND_SIMPLE_CARD): asoc_simple_card_init_dai() @ sound/soc/generic/simple-card-utils.c snd_soc_dai_set_sysclk() fsl_ssi_set_dai_sysclk() @ sound/soc/fsl/fsl_ssi.c The last call is changing the bit clock (BCLK) frequency to SSI's IP block clock (ipg = 66 MHz) [1]. I think a bigger question here is why the routine sets BCLK to 66MHz. Yes, exactly. In my case the bclk is set to ipg clock, which is the SSI IP block clock (ipg). This is wrong, since IMX SSI block requires the I2S BCLK to be less than 1/5 of [1]. As a result the driver initialization passes without any errors, but the speaker-test test case breaks. This commit checks if the fsl_ssi_set_dai_sysclk() frequency passed is not equal to [1]. I don't feel it's quite comprehensive...what if it's being set to 67MHz. I think that this clock is not changing for the SoC. It should be 66 MHz fixed. Thanks Nicolin Signed-off-by: Lukasz Majewski --- sound/soc/fsl/fsl_ssi.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 173cb84..1186fa9 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -809,6 +809,8 @@ static int fsl_ssi_set_dai_sysclk(struct snd_soc_dai *cpu_dai, int clk_id, unsigned int freq, int dir) { struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(cpu_dai); + if (clk_get_rate(ssi_private->clk) == freq) + return 0; ssi_private->bitclk_freq = freq; -- 2.1.4 -- Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq
On Tue, Sep 05, 2017 at 09:37:43AM +0200, Łukasz Majewski wrote: > >>The last call is changing the bit clock (BCLK) frequency to SSI's IP > >>block clock (ipg = 66 MHz) [1]. > > > >I think a bigger question here is why the routine sets BCLK to 66MHz. > > Yes, exactly. > > In my case the bclk is set to ipg clock, which is the SSI IP block clock > (ipg). Can you elaborate why you set ipg clock as bclk? I don't remember SSI could derive bitclock from ipg clock. > >>This is wrong, since IMX SSI block requires the I2S BCLK to be less > >>than 1/5 of [1]. > >> > >>As a result the driver initialization passes without any errors, but the > >>speaker-test test case breaks. > >> > >>This commit checks if the fsl_ssi_set_dai_sysclk() frequency passed is > >>not equal to [1]. > > > >I don't feel it's quite comprehensive...what if it's being set to 67MHz. > > I think that this clock is not changing for the SoC. It should be 66 MHz > fixed. What I mean is that we cannot just look at this SoC. Today is 66MHz for this SoC. Tomorrow could be 133MHz for another one. We should put a check that none of these shall pass -- the 1/5 limit.
Re: [PATCH 2/2] powerpc/powernv/npu: Don't explicitly flush nmmu tlb
Le 05/09/2017 à 05:57, Alistair Popple a écrit : The nest mmu required an explicit flush as a tlbi would not flush it in the same way as the core. However an alternate firmware fix exists which should eliminate the need for this flush, so instead add a device-tree property (ibm,nmmu-flush) on the NVLink2 PHB to enable it only if required. Signed-off-by: Alistair Popple --- arch/powerpc/platforms/powernv/npu-dma.c | 28 +++- arch/powerpc/platforms/powernv/pci.h | 3 +++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c index 2fff9a65..4b4fcac 100644 --- a/arch/powerpc/platforms/powernv/npu-dma.c +++ b/arch/powerpc/platforms/powernv/npu-dma.c @@ -395,6 +395,7 @@ struct npu_context { struct pci_dev *npdev[NV_MAX_NPUS][NV_MAX_LINKS]; struct mmu_notifier mn; struct kref kref; + bool nmmu_flush; /* Callback to stop translation requests on a given GPU */ struct npu_context *(*release_cb)(struct npu_context *, void *); @@ -545,11 +546,13 @@ static void mmio_invalidate(struct npu_context *npu_context, int va, struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS]; unsigned long pid = npu_context->mm->context.id; - /* -* Unfortunately the nest mmu does not support flushing specific -* addresses so we have to flush the whole mm. -*/ - flush_all_mm(npu_context->mm); + if (npu_context->nmmu_flush) + /* +* Unfortunately the nest mmu does not support flushing specific +* addresses so we have to flush the whole mm once before +* shooting down the GPU translation. +*/ + flush_all_mm(npu_context->mm); /* * Loop over all the NPUs this process is active on and launch @@ -722,6 +725,16 @@ struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, return ERR_PTR(-ENODEV); npu_context->npdev[npu->index][nvlink_index] = npdev; + if (!nphb->npu.nmmu_flush) { + /* +* If we're not explicitly flushing ourselves we need to mark +* the thread for global flushes +*/ + npu_context->nmmu_flush = false; + inc_mm_active_cpus(mm); I can see that you use the inc_mm_active_cpus()/dec_mm_active_cpus() and not the new mm_context_add_copro()/mm_context_remove_copro(). The only difference is that mm_context_remove_copro() adds an extra flush of the full mm (pwc and tlb) before decrementing the active cpu count. But don't you need the flush when releasing the context? As soon as the active cpu count is decremented, the next TLBI may be local and not visible to the nMMU, so the nMMU could keep obsolete/wrong entries in its cache. Fred + } else + npu_context->nmmu_flush = true; + return npu_context; } EXPORT_SYMBOL(pnv_npu2_init_context); @@ -731,6 +744,9 @@ static void pnv_npu2_release_context(struct kref *kref) struct npu_context *npu_context = container_of(kref, struct npu_context, kref); + if (!npu_context->nmmu_flush) + dec_mm_active_cpus(npu_context->mm); + npu_context->mm->context.npu_context = NULL; mmu_notifier_unregister(&npu_context->mn, npu_context->mm); @@ -819,6 +835,8 @@ int pnv_npu2_init(struct pnv_phb *phb) static int npu_index; uint64_t rc = 0; + phb->npu.nmmu_flush = + of_property_read_bool(phb->hose->dn, "ibm,nmmu-flush"); for_each_child_of_node(phb->hose->dn, dn) { gpdev = pnv_pci_get_gpu_dev(get_pci_dev(dn)); if (gpdev) { diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h index a95273c..22025c6 100644 --- a/arch/powerpc/platforms/powernv/pci.h +++ b/arch/powerpc/platforms/powernv/pci.h @@ -187,6 +187,9 @@ struct pnv_phb { /* Bitmask for MMIO register usage */ unsigned long mmio_atsd_usage; + + /* Do we need to explicitly flush the nest mmu? */ + bool nmmu_flush; } npu; #ifdef CONFIG_CXL_BASE
Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq
On 09/05/2017 09:52 AM, Nicolin Chen wrote: On Tue, Sep 05, 2017 at 09:37:43AM +0200, Łukasz Majewski wrote: The last call is changing the bit clock (BCLK) frequency to SSI's IP block clock (ipg = 66 MHz) [1]. I think a bigger question here is why the routine sets BCLK to 66MHz. Yes, exactly. In my case the bclk is set to ipg clock, which is the SSI IP block clock (ipg). Can you elaborate why you set ipg clock as bclk? I don't remember SSI could derive bitclock from ipg clock. Just to be clear: What clock shall be set with: struct snd_soc_dai_ops { int (*set_sysclk)(struct snd_soc_dai *dai, int clk_id, unsigned int freq, int dir); } callback? The SSI IP block or BCLK ? This is wrong, since IMX SSI block requires the I2S BCLK to be less than 1/5 of [1]. As a result the driver initialization passes without any errors, but the speaker-test test case breaks. This commit checks if the fsl_ssi_set_dai_sysclk() frequency passed is not equal to [1]. I don't feel it's quite comprehensive...what if it's being set to 67MHz. I think that this clock is not changing for the SoC. It should be 66 MHz fixed. What I mean is that we cannot just look at this SoC. Today is 66MHz for this SoC. Tomorrow could be 133MHz for another one. We should put a check that none of these shall pass -- the 1/5 limit. Ok. Good point. -- Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq
On 09/05/2017 07:20 AM, Nicolin Chen wrote: On Sun, Sep 03, 2017 at 04:40:21PM +0200, Łukasz Majewski wrote: /* * Hardware limitation: The bclk rate must be * never greater than 1/5 IPG clock rate */ if (freq * 5 > clk_get_rate(ssi_private->clk)) { dev_err(cpu_dai->dev, "bitclk > ipgclk/5\n"); return -EINVAL; } Unfortunately not. This is the part of fsl_ssi_set_bclk() function which is called after fsl_ssi_set_dai_sysclk() (which sets ssi_private->bitclk_freq = freq;). Before the aforementioned check we do have: if (ssi_private->bitclk_freq) freq = ssi_private->bitclk_freq; else freq = params_channels(hw_params) * 32 * params_rate(hw_params); Which assigns freq = bitclk_freq (66 MHz) [...] And then we break on this particular check: 66MHz * 5 > 66 MHz. [...] Does the check fail and return an error here, or not? Yes, this check fails and return error (-EINVAL). The culprit IMHO is the ssi_private->bitclk_freq = freq; in the fsl_ssi_set_dai_sysclk(), since we _should_ set SSI's IP block clock (ssi_private->clk), not the bit clock (BCLK). No. We should not set the IP block clock. That's from IPG bus on certain IMX SoCs. Setting it might change IPG bus clocks which might break the system. And apparently, we shouldn't set bitclk to 66MHz either. Can you help to find where this 66MHz comes from? OK. The fsi_ssi.c file defines: ops->set_sysclk() routine: .set_sysclk = fsl_ssi_set_dai_sysclk, This routine is called in: int snd_soc_dai_set_sysclk() @ soc-core.c which is called in two places for my setup: 1. asoc_simple_card_hw_params() @ simple-card.c and 2. int asoc_simple_card_init_dai() @ simple-card-utils.c In this function (point 2.) the simple_dai->sysclk is set and: snd_soc_dai_set_sysclk(dai, 0, simple_dai->sysclk, 0) which sets frequency to 66 MHz [*]. The asoc_simple_card_init_dai() is called in asoc_simple_card_dai_init() @ simple-card.c which is assigned to dai_link->init dai_link->init = asoc_simple_card_dai_init; @ simple_card.c And the sysclk itself is defined at: - dai_props->codec_dai->sysclk, which is used at: asoc_simple_card_startup(), asoc_simple_card_shutdown() and others functions at simple-card.c It is setup at: asoc_simple_card_parse_clk() @ simple-card-utils.c from macro: #define asoc_simple_card_parse_clk_cpu() And the problem is: --- At the asoc_simple_card_parse_clk() we finally go to dts node: /soc/aips-bus@0210/i2c@021a/tfa9879@6C which has clock from I2C (66 MHz). (So the [*] hack may help here, since it is checked before checking the i2c node). Note: [*] - I could workaround this problem by setting: system-clock-frequency = <0> in dailink_master: cpu { sound-dai = <&ssi2>; }; but this is IMHO even worse hack than this patch. This patch just quits early if it detects change, which don't need to be done. I kinda see your point is to error out before hw_params(). So I feel it would be better to have a similar 5-times-check in the set_sysclk() too, if it's really necessary. Btw, I don't see simple card calling set_sysclk() function in any earlier stage than hw_params(). I am wondering how did you encounter the problem? Thanks Nicolin -- Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Re: Machine Check in P2010(e500v2)
So after some debugging I found this bug: @@ -996,7 +998,7 @@ int fsl_pci_mcheck_exception(struct pt_regs *regs) if (is_in_pci_mem_space(addr)) { if (user_mode(regs)) { pagefault_disable(); - ret = get_user(regs->nip, &inst); + ret = get_user(inst, (__u32 __user *)regs->nip); pagefault_enable(); } else { ret = probe_kernel_address(regs->nip, inst); However, the kernel still locked up after fixing that. Now I wonder why this fixup is there in the first place? The routine will not really fixup the insn, just return 0x for the failing read and then advance the process NIP. Removing the fixup does not help either, kernel still locks up: [ 28.170532] Machine check in kernel mode. [ 28.174538] Caused by (from MCSR=10008): [ 28.182804] Bus - Read Data Bus Error: DAR:b7013000 [ 28.197079] Oops: Machine check, sig: 7 [#1] [ 28.201343] P1010 RDB [ 28.203608] Modules linked in: linux_bcm_knet(PO) linux_user_bde(PO) linux_kernel_bde(PO) [ 28.211796] CPU: 0 PID: 470 Comm: emxp2_hw_bl Tainted: P O 4.1.38+ #201 [ 28.219540] task: db16ed10 ti: df122000 task.ti: df122000 [ 28.224935] NIP: 10a4e2f4 LR: 10a4e404 CTR: 10046c38 [ 28.229896] REGS: df123f10 TRAP: 0204 Tainted: P O (4.1.38+) [ 28.236942] MSR: 0002d000 CR: 44002428 XER: [ 28.243306] DEAR: b7013000 ESR: GPR00: 10a4e404 bfab2730 b7b354a0 132f9fa8 07006000 0700 132f9fd8 GPR08: b6fd5000 b6fe5000 0003e000 bfab2720 24004424 11d6cf7c GPR16: 10f6e29c 10f6c872 10f6db01 b541 b541 11d92fcc 0011 0001 GPR24: 01a5bd3e 132ffbf0 11d6 07006000 132f9fa8 [ 28.275547] NIP [10a4e2f4] 0x10a4e2f4 [ 28.279204] LR [10a4e404] 0x10a4e404 [ 28.282772] Call Trace: [ 28.285213] ---[ end trace 9f8b64ab1e83f449 ]--- [ 28.289825] Jocke On Fri, 2017-09-01 at 13:32 +0200, Joakim Tjernlund wrote: > I am trying to debug a Machine Check for a P2010 (e500v2) CPU: > > [ 28.111816] Caused by (from MCSR=10008): Bus - Read Data Bus Error > [ 28.117998] Oops: Machine check, sig: 7 [#1] > [ 28.122263] P1010 RDB > [ 28.124529] Modules linked in: linux_bcm_knet(PO) linux_user_bde(PO) > linux_kernel_bde(PO) > [ 28.132718] CPU: 0 PID: 470 Comm: emxp2_hw_bl Tainted: P O > 4.1.38+ #49 > [ 28.140376] task: db16cd10 ti: df128000 task.ti: df128000 > [ 28.145770] NIP: LR: 10a4e404 CTR: 10046c38 > [ 28.150730] REGS: df129f10 TRAP: 0204 Tainted: P O > (4.1.38+) > [ 28.157776] MSR: 0002d000 CR: 44002428 XER: > [ 28.164140] DEAR: b7187000 ESR: > GPR00: 10a4e404 bf86ea30 b7ca94a0 132f9fa8 07006000 0700 132f9fd8 > GPR08: b7149000 b7159000 0003e000 bf86ea20 24004424 11d6cf7c > GPR16: 10f6e29c 10f6c872 10f6db01 b541 b541 11d92fcc 0011 0001 > GPR24: 01a4d12d 132ffbf0 11d6 07006000 132f9fa8 > [ 28.196375] NIP [] (null) > [ 28.199859] LR [10a4e404] 0x10a4e404 > [ 28.203426] Call Trace: > [ 28.205866] ---[ end trace f456255ddf9bee83 ]--- > > I cannot figure out why NIP is NULL ? It LOOKs like NIP is set to > MCSRR0 early on but maybe it is lost somehow? > > Anyhow, looking at entry_32.S: > .globl mcheck_transfer_to_handler > mcheck_transfer_to_handler: > mfspr r0,SPRN_DSRR0 > stw r0,_DSRR0(r11) > mfspr r0,SPRN_DSRR1 > stw r0,_DSRR1(r11) > /* fall through */ > > .globl debug_transfer_to_handler > debug_transfer_to_handler: > mfspr r0,SPRN_CSRR0 > stw r0,_CSRR0(r11) > mfspr r0,SPRN_CSRR1 > stw r0,_CSRR1(r11) > /* fall through */ > > .globl crit_transfer_to_handler > crit_transfer_to_handler: > > It looks odd that DSRRx is assigned in mcheck and CSRRx in debug and > crit has none. Should not this assigment be shifted down one level? > > Jocke
Re: [PATCH V2] cxl: Add support for POWER9 DD2
Le 04/09/2017 à 17:29, Christophe Lombard a écrit : The PSL initialization sequence has been updated to DD2. This patch adapts to the changes, retaining compatibility with DD1. The patch includes some changes to DD1 fix-ups as well. Tests performed on some of the old/new hardware. The function is_page_fault(), for POWER9, lists the Translation Checkout Responses where the page fault will be handled by copro_handle_mm_fault(). This list is too restrictive and not necessary. This patches removes this restriction and all page faults, whatever the reason, will be handled. In this case, the interruption is always acknowledged. Signed-off-by: Christophe Lombard --- I'm ok with those changes. We should add to the commit message that we'll have more changes coming, at least to add the phb reset when switching to capi mode (if we still can't get rid of it). The other item is capp recovery, which is still being worked on, but any change required there would hopefully be limited to skiboot. With the above: Acked-by: Frederic Barrat Fred Changelog[v2] - Rebase to latest upstream. - Update the function is_page_fault() --- drivers/misc/cxl/cxl.h | 2 ++ drivers/misc/cxl/fault.c | 15 ++- drivers/misc/cxl/pci.c | 46 +++--- 3 files changed, 27 insertions(+), 36 deletions(-) diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h index b1afecc..0167df8 100644 --- a/drivers/misc/cxl/cxl.h +++ b/drivers/misc/cxl/cxl.h @@ -100,6 +100,8 @@ static const cxl_p1_reg_t CXL_XSL_FEC = {0x0158}; static const cxl_p1_reg_t CXL_XSL_DSNCTL= {0x0168}; /* PSL registers - CAIA 2 */ static const cxl_p1_reg_t CXL_PSL9_CONTROL = {0x0020}; +static const cxl_p1_reg_t CXL_XSL9_INV = {0x0110}; +static const cxl_p1_reg_t CXL_XSL9_DEF = {0x0140}; static const cxl_p1_reg_t CXL_XSL9_DSNCTL = {0x0168}; static const cxl_p1_reg_t CXL_PSL9_FIR1 = {0x0300}; static const cxl_p1_reg_t CXL_PSL9_FIR2 = {0x0308}; diff --git a/drivers/misc/cxl/fault.c b/drivers/misc/cxl/fault.c index 6eed7d0..0cf7f4a 100644 --- a/drivers/misc/cxl/fault.c +++ b/drivers/misc/cxl/fault.c @@ -204,22 +204,11 @@ static bool cxl_is_segment_miss(struct cxl_context *ctx, u64 dsisr) static bool cxl_is_page_fault(struct cxl_context *ctx, u64 dsisr) { - u64 crs; /* Translation Checkout Response Status */ - if ((cxl_is_power8()) && (dsisr & CXL_PSL_DSISR_An_DM)) return true; - if (cxl_is_power9()) { - crs = (dsisr & CXL_PSL9_DSISR_An_CO_MASK); - if ((crs == CXL_PSL9_DSISR_An_PF_SLR) || - (crs == CXL_PSL9_DSISR_An_PF_RGC) || - (crs == CXL_PSL9_DSISR_An_PF_RGP) || - (crs == CXL_PSL9_DSISR_An_PF_HRH) || - (crs == CXL_PSL9_DSISR_An_PF_STEG) || - (crs == CXL_PSL9_DSISR_An_URTCH)) { - return true; - } - } + if (cxl_is_power9()) + return true; return false; } diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c index d18b3d9..3edc991 100644 --- a/drivers/misc/cxl/pci.c +++ b/drivers/misc/cxl/pci.c @@ -401,7 +401,8 @@ int cxl_calc_capp_routing(struct pci_dev *dev, u64 *chipid, *capp_unit_id = get_capp_unit_id(np, *phb_index); of_node_put(np); if (!*capp_unit_id) { - pr_err("cxl: invalid capp unit id\n"); + pr_err("cxl: invalid capp unit id (phb_index: %d)\n", + *phb_index); return -ENODEV; } @@ -475,37 +476,36 @@ static int init_implementation_adapter_regs_psl9(struct cxl *adapter, psl_fircntl |= 0x1ULL; /* ce_thresh */ cxl_p1_write(adapter, CXL_PSL9_FIR_CNTL, psl_fircntl); - /* vccredits=0x1 pcklat=0x4 */ - cxl_p1_write(adapter, CXL_PSL9_DSNDCTL, 0x1810ULL); - - /* -* For debugging with trace arrays. -* Configure RX trace 0 segmented mode. -* Configure CT trace 0 segmented mode. -* Configure LA0 trace 0 segmented mode. -* Configure LA1 trace 0 segmented mode. + /* Setup the PSL to transmit packets on the PCIe before the +* CAPP is enabled */ - cxl_p1_write(adapter, CXL_PSL9_TRACECFG, 0x804080008000ULL); - cxl_p1_write(adapter, CXL_PSL9_TRACECFG, 0x804080008003ULL); - cxl_p1_write(adapter, CXL_PSL9_TRACECFG, 0x804080008005ULL); - cxl_p1_write(adapter, CXL_PSL9_TRACECFG, 0x804080008006ULL); + cxl_p1_write(adapter, CXL_PSL9_DSNDCTL, 0x000100102A10ULL); /* * A response to an ASB_Notify request is returned by the * system as an MMIO write to the address defined in -* the PSL_TNR_ADDR register +* the PSL_TNR_ADDR register. +* keep the Reset Value: 0x0002E000 */ - /* PSL_TNR_ADDR */ - /* NORST *
Re: [PATCH 2/2] powerpc/powernv/npu: Don't explicitly flush nmmu tlb
Hi, On Tue, 5 Sep 2017 10:10:03 AM Frederic Barrat wrote: > > > > + if (!nphb->npu.nmmu_flush) { > > + /* > > +* If we're not explicitly flushing ourselves we need to mark > > +* the thread for global flushes > > +*/ > > + npu_context->nmmu_flush = false; > > + inc_mm_active_cpus(mm); > > > I can see that you use the inc_mm_active_cpus()/dec_mm_active_cpus() and > not the new mm_context_add_copro()/mm_context_remove_copro(). The only > difference is that mm_context_remove_copro() adds an extra flush of the > full mm (pwc and tlb) before decrementing the active cpu count. Thanks for pointing that out, I'd overlooked the difference between the two. > But don't you need the flush when releasing the context? As soon as the > active cpu count is decremented, the next TLBI may be local and not > visible to the nMMU, so the nMMU could keep obsolete/wrong entries in > its cache. Yes, I think you're right. In practice I doubt we'd ever hit it as the driver never calls pnv_npu2_destroy_context() prior to the process being torn down (which should trigger the right global tlbies). But obviously something that needs fixing so I will post a v2 ... thanks for reviewing! Regards, Alistair >Fred > > > > > + } else > > + npu_context->nmmu_flush = true; > > + > > return npu_context; > > } > > EXPORT_SYMBOL(pnv_npu2_init_context); > > @@ -731,6 +744,9 @@ static void pnv_npu2_release_context(struct kref *kref) > > struct npu_context *npu_context = > > container_of(kref, struct npu_context, kref); > > > > + if (!npu_context->nmmu_flush) > > + dec_mm_active_cpus(npu_context->mm); > > + > > npu_context->mm->context.npu_context = NULL; > > mmu_notifier_unregister(&npu_context->mn, > > npu_context->mm); > > @@ -819,6 +835,8 @@ int pnv_npu2_init(struct pnv_phb *phb) > > static int npu_index; > > uint64_t rc = 0; > > > > + phb->npu.nmmu_flush = > > + of_property_read_bool(phb->hose->dn, "ibm,nmmu-flush"); > > for_each_child_of_node(phb->hose->dn, dn) { > > gpdev = pnv_pci_get_gpu_dev(get_pci_dev(dn)); > > if (gpdev) { > > diff --git a/arch/powerpc/platforms/powernv/pci.h > > b/arch/powerpc/platforms/powernv/pci.h > > index a95273c..22025c6 100644 > > --- a/arch/powerpc/platforms/powernv/pci.h > > +++ b/arch/powerpc/platforms/powernv/pci.h > > @@ -187,6 +187,9 @@ struct pnv_phb { > > > > /* Bitmask for MMIO register usage */ > > unsigned long mmio_atsd_usage; > > + > > + /* Do we need to explicitly flush the nest mmu? */ > > + bool nmmu_flush; > > } npu; > > > > #ifdef CONFIG_CXL_BASE > > >
Re: powerpc/eeh: Delete an error message for a failed memory allocation in two functions
> Applied to powerpc next, thanks. > > https://git.kernel.org/powerpc/c/6ab41161b44a3b4d504ac29c9dd997 Thanks that you picked another update suggestion up. * It might matter to mention that only one patch hunk was accepted in this case. * Can it be that the adjusted commit subject does contain a typo at the moment? Would the wording “powerpc/eeh: Delete a message for an "out of memory error" (at init time)” be more appropriate there? Regards, Markus
[PATCH] fsl_pci: Correct fsl_pci_mcheck_exception
get_user() had it args reversed causing NIP to be NULL:ed instead of fixing up the PCI access. Note: This still hangs my P1020 Freescale CPU hard, but at least I get a NIP now. Signed-off-by: Joakim Tjernlund --- arch/powerpc/sysdev/fsl_pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c index 7c8b779c329a..9e64c12dff6a 100644 --- a/arch/powerpc/sysdev/fsl_pci.c +++ b/arch/powerpc/sysdev/fsl_pci.c @@ -996,7 +996,7 @@ int fsl_pci_mcheck_exception(struct pt_regs *regs) if (is_in_pci_mem_space(addr)) { if (user_mode(regs)) { pagefault_disable(); - ret = get_user(regs->nip, &inst); + ret = get_user(inst, (__u32 __user *)regs->nip); pagefault_enable(); } else { ret = probe_kernel_address(regs->nip, inst); -- 2.13.5
Re: [PATCH] devicetree: Remove remaining references/tests for "chosen@0"
On Sun, Sep 3, 2017 at 5:43 AM, Robert P. J. Day wrote: > On Sun, 3 Sep 2017, Benjamin Herrenschmidt wrote: > >> On Sat, 2017-09-02 at 04:43 -0400, Robert P. J. Day wrote: >> > Since, according to a recent devicetree ML posting by Rob Herring, >> > the node "/chosen@0" is most likely for real Open Firmware and >> > does not apply to DTSpec, remove all remaining tests and >> > references for that node, of which there are very few left: >> >> Technically that would break Open Firmware systems where the node is >> really called chosen@0 >> >> Now I'm not sure such a thing actually exist however. >> >> My collection of DTs don't seem to have one, except in the ancient >> html variants that were extracted by the pengionppc folks for the >> original PowerMac 8600 but I wonder if that's a bug in the >> extraction script since they also have @0 on /packages etc... > > obviously, this isn't a priority issue, i was just working off a > comment by rob herring that "chosen@0" is not defined by the current > DTSpec 0.1, so it seemed appropriate to toss it. if there's a reason > to hang onto it, that's fine with me. > > however, given the diff stat of the change to remove every single > reference to that node name in the current kernel source: > > arch/microblaze/kernel/prom.c | 3 +-- > arch/mips/generic/yamon-dt.c | 4 > arch/powerpc/boot/oflib.c | 7 ++- > drivers/of/base.c | 2 -- > drivers/of/fdt.c | 5 + > 5 files changed, 4 insertions(+), 17 deletions(-) > > it seems inconsistent that three architectures would be testing for > that node, but none of the rest. consistency suggests that every > architecture should take it into account, or none should. I generally agree and have moved various things from arch to drivers/of/ to ensure that. But for legacy things, we have to allow for exceptions. I agree with Ben and think that microblaze (they generally just copied PPC), MIPS, and the FDT code in drivers/of/ can be changed. Rob
Re: [PATCH] devicetree: Remove remaining references/tests for "chosen@0"
On Sat, Sep 2, 2017 at 3:43 AM, Robert P. J. Day wrote: > > Since, according to a recent devicetree ML posting by Rob Herring, > the node "/chosen@0" is most likely for real Open Firmware and does > not apply to DTSpec, remove all remaining tests and references for > that node, of which there are very few left: > > arch/microblaze/kernel/prom.c | 3 +-- > arch/mips/generic/yamon-dt.c | 4 > arch/powerpc/boot/oflib.c | 7 ++- > drivers/of/base.c | 2 -- > drivers/of/fdt.c | 5 + > 5 files changed, 4 insertions(+), 17 deletions(-) > > This should be innocuous as, in all of the three arch/ files above, > there is a test for "chosen" immediately before the test for > "chosen@0", so nothing should change. > > Signed-off-by: Robert P. J. Day > > --- > > if this patch is premature, then just ignore it, thanks. > > diff --git a/arch/microblaze/kernel/prom.c b/arch/microblaze/kernel/prom.c > index 68f0999..c81bfd7 100644 > --- a/arch/microblaze/kernel/prom.c > +++ b/arch/microblaze/kernel/prom.c > @@ -53,8 +53,7 @@ static int __init early_init_dt_scan_chosen_serial(unsigned > long node, > > pr_debug("%s: depth: %d, uname: %s\n", __func__, depth, uname); > > - if (depth == 1 && (strcmp(uname, "chosen") == 0 || > - strcmp(uname, "chosen@0") == 0)) { > + if (depth == 1 && (strcmp(uname, "chosen") == 0)) { I'd really hoped to remove early_init_dt_scan_chosen_serial() altogether. It may now be just a matter of adding the compatible strings to the uartlite earlycon. Rob
Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq
On Tue, Sep 05, 2017 at 10:19:05AM +0200, Łukasz Majewski wrote: > On 09/05/2017 09:52 AM, Nicolin Chen wrote: > > Can you elaborate why you set ipg clock as bclk? I don't remember SSI could > > derive bitclock from ipg clock. > Just to be clear: > What clock shall be set with: > struct snd_soc_dai_ops { > int (*set_sysclk)(struct snd_soc_dai *dai, > int clk_id, unsigned int freq, int dir); > } > callback? > The SSI IP block or BCLK ? Not the BCLK, probably the IP clock but perhaps nothing. The bclk is usually derived from the sample rate and width, the system clock is the clock rate going into the device. It doesn't *have* to be configured (particuarly if it's discoverable by the IP and managable via the clock API). signature.asc Description: PGP signature
[PATCH v2] axonram: Delete an unnecessary variable initialisation in axon_ram_probe()
From: Markus Elfring Date: Tue, 5 Sep 2017 18:47:02 +0200 The local variable "rc" will eventually be set only to an error code. Thus omit the explicit initialisation at the beginning. Signed-off-by: Markus Elfring --- v2: Three update steps were integrated for this software module on 2017-09-01. Thus improve another source code place. arch/powerpc/sysdev/axonram.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/powerpc/sysdev/axonram.c b/arch/powerpc/sysdev/axonram.c index c60e84e4558d..1b307c80b401 100644 --- a/arch/powerpc/sysdev/axonram.c +++ b/arch/powerpc/sysdev/axonram.c @@ -184,7 +184,7 @@ static int axon_ram_probe(struct platform_device *device) static int axon_ram_bank_id = -1; struct axon_ram_bank *bank; struct resource resource; - int rc = 0; + int rc; axon_ram_bank_id++; -- 2.14.1
Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq
On Tue, Sep 05, 2017 at 04:15:50PM +0100, Mark Brown wrote: > > Just to be clear: > > > What clock shall be set with: > > > struct snd_soc_dai_ops { > > int (*set_sysclk)(struct snd_soc_dai *dai, > > int clk_id, unsigned int freq, int dir); > > } > > > callback? > > > The SSI IP block or BCLK ? > > Not the BCLK, probably the IP clock but perhaps nothing. The bclk is > usually derived from the sample rate and width, the system clock is the > clock rate going into the device. It doesn't *have* to be configured > (particuarly if it's discoverable by the IP and managable via the clock > API). Hmm...to clarify the things, some background here: SSI has mainly two different clock inputs internally (not external pads such as bclk or lrclk): IP block clock (ipg_clk in Reference Manual) and sys clock (ccm_ssi_clk in Reference Manual). According to RM, ccm_ssi_clk: "module/system clock for bit clock generation". The ipg clock is merely used to access registers, and has nothing (directly) to do with external clock outputs. The driver shall not change the ipg clock as the system ipg clock (its parent clock) might be messed and even system time would get weird -- happened once when the fsl_spdif driver used to call clk_set_rate() on its ipg clock. Although the clock controller should have some kind of protection in my opinion, we just avoid IP clock rate change in all audio drivers as well. On the other hand, the sys clock (baudclk in the driver) should be configured whenever it's related to external clock outputs. When I implemented this set_sysclk() for fsl_ssi.c, I used it to set this sys clock (baudclk) by a machine driver, in order to set bit clock. Then someone patched the driver by moving all the code to set_bclk() to make machine drivers simpler. Now the set_sysclk() is remained to give machine drivers a chance to override clock configurations in the hw_params(). This could be used in TDM or some other special cases (It could also have a purpose for backwards compatibility). So here, we should set baudclk (BCLK generator).
Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq
On Tue, Sep 05, 2017 at 10:35:34AM +0200, Łukasz Majewski wrote: > >And apparently, we shouldn't set bitclk to 66MHz either. Can > >you help to find where this 66MHz comes from? > 2. int asoc_simple_card_init_dai() @ simple-card-utils.c Oh, I just searched in the simple-card.c but missed this file. > In this function (point 2.) the > simple_dai->sysclk is set and: > snd_soc_dai_set_sysclk(dai, 0, simple_dai->sysclk, 0) > which sets frequency to 66 MHz [*]. > > The asoc_simple_card_init_dai() is called in > asoc_simple_card_dai_init() @ simple-card.c > which is assigned to dai_link->init > dai_link->init= asoc_simple_card_dai_init; @ simple_card.c > > And the sysclk itself is defined at: > - > dai_props->codec_dai->sysclk, which is used at:a Why codec_dai? Why not dai_props->cpu_dai->sysclk since we are talking about SSI? > asoc_simple_card_startup(), asoc_simple_card_shutdown() and others > functions at simple-card.c > It is setup at: > asoc_simple_card_parse_clk() @ simple-card-utils.c from macro: > #define asoc_simple_card_parse_clk_cpu() > And the problem is: > --- > > At the > asoc_simple_card_parse_clk() > we finally go to dts node: > /soc/aips-bus@0210/i2c@021a/tfa9879@6C This tfa9879 should be the CODEC right? > which has clock from I2C (66 MHz). You mean I2C scl or I2S sclk? - But anyway, I feel very confused here as you have 66MHz clock rate (regardless of it purpose) for a codec dai but it's been passed to a cpu dai (SSI). > [*] - I could workaround this problem by setting: > > system-clock-frequency = <0> in > > dailink_master: cpu { > sound-dai = <&ssi2>; > }; > > but this is IMHO even worse hack than this patch. I haven't used simple-card for a while so I forgot how to define its DT bindings specifically. But you should assign ssi2 as the CPU dai and assign tfa9879 as a CODEC dai.
Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq
Hi Lukasz, On Tue, Sep 5, 2017 at 5:35 AM, Łukasz Majewski wrote: > Note: > [*] - I could workaround this problem by setting: > > system-clock-frequency = <0> in > > dailink_master: cpu { > sound-dai = <&ssi2>; > }; > Which mx6 type are you using? Can you post the complete audio related bindings of your dts? Still trying to understand why we do not see this error on other imx6 boards.
Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq
Hi Nicolin, On Tue, Sep 05, 2017 at 10:35:34AM +0200, Łukasz Majewski wrote: And apparently, we shouldn't set bitclk to 66MHz either. Can you help to find where this 66MHz comes from? 2. int asoc_simple_card_init_dai() @ simple-card-utils.c Oh, I just searched in the simple-card.c but missed this file. In this function (point 2.) the simple_dai->sysclk is set and: snd_soc_dai_set_sysclk(dai, 0, simple_dai->sysclk, 0) which sets frequency to 66 MHz [*]. The asoc_simple_card_init_dai() is called in asoc_simple_card_dai_init() @ simple-card.c which is assigned to dai_link->init dai_link->init = asoc_simple_card_dai_init; @ simple_card.c And the sysclk itself is defined at: - dai_props->codec_dai->sysclk, which is used at:a Why codec_dai? Why not dai_props->cpu_dai->sysclk since we are talking about SSI? This is how the simple-card (simple-sound-card) is written. asoc_simple_card_startup(), asoc_simple_card_shutdown() and others functions at simple-card.c It is setup at: asoc_simple_card_parse_clk() @ simple-card-utils.c from macro: #define asoc_simple_card_parse_clk_cpu() And the problem is: --- At the asoc_simple_card_parse_clk() we finally go to dts node: /soc/aips-bus@0210/i2c@021a/tfa9879@6C This tfa9879 should be the CODEC right? Yes. The tfa9879 is a codec (very simple -> I2S + I2C, mono). They key point here is the asoc_simple_card_parse_clk() function from simple-card-utils.c Please look how the clock is assigned; It first checks for cpu clock, then for "system-clock-frequency" DTS node and _finally_ looks for another "child" clock [1], which is the codec attached to I2C. And from there it takes the 66 MHz CLK: /soc/aips-bus@0210/i2c@021a/tfa9879@6C which has clock from I2C (66 MHz). You mean I2C scl or I2S sclk? I2C scl. - But anyway, I feel very confused here as you have 66MHz clock rate (regardless of it purpose) for a codec dai but it's been passed to a cpu dai (SSI). Please look into asoc_simple_card_parse_clk(). My DTS [1] (it is different than other in-tree supported codecs - at least I did not find similar setup in DTSes): sound { compatible = "simple-audio-card"; label = "tfa9879-mono"; simple-audio-card,dai-link { /* DAC */ format = "i2s"; bitclock-master = <&dailink_master>; frame-master = <&dailink_master>; dailink_master: cpu { sound-dai = <&ssi2>; }; codec { sound-dai = <&codec>; }; }; }; &i2c1 { clock-frequency = <40>; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_i2c1>; status = "okay"; codec: tfa9879@6C { #sound-dai-cells = <0>; compatible = "tfa9879"; reg = <0x6C>; }; }; &ssi2 { fsl,mode = "i2s-master"; status = "okay"; }; the ssi2 node is defined in imx6qdl.dtsi file (no changes). The SOC is IMX6Q. The TFA9879 is a slave for I2S transmission. [*] - I could workaround this problem by setting: system-clock-frequency = <0> in dailink_master: cpu { sound-dai = <&ssi2>; }; but this is IMHO even worse hack than this patch. I haven't used simple-card for a while so I forgot how to define its DT bindings specifically. But you should assign ssi2 as the CPU dai and assign tfa9879 as a CODEC dai. -- Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq
Hi Fabio, Hi Lukasz, On Tue, Sep 5, 2017 at 5:35 AM, Łukasz Majewski wrote: Note: [*] - I could workaround this problem by setting: system-clock-frequency = <0> in dailink_master: cpu { sound-dai = <&ssi2>; }; Which mx6 type are you using? Can you post the complete audio related bindings of your dts? Still trying to understand why we do not see this error on other imx6 boards. I've sent my DTS in the other e-mail. -- Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Re: [PATCH] devicetree: Remove remaining references/tests for "chosen@0"
On Tue, 5 Sep 2017, Rob Herring wrote: > On Sun, Sep 3, 2017 at 5:43 AM, Robert P. J. Day > wrote: > > On Sun, 3 Sep 2017, Benjamin Herrenschmidt wrote: > > > >> On Sat, 2017-09-02 at 04:43 -0400, Robert P. J. Day wrote: > >> > Since, according to a recent devicetree ML posting by Rob Herring, > >> > the node "/chosen@0" is most likely for real Open Firmware and > >> > does not apply to DTSpec, remove all remaining tests and > >> > references for that node, of which there are very few left: > >> > >> Technically that would break Open Firmware systems where the node is > >> really called chosen@0 > >> > >> Now I'm not sure such a thing actually exist however. > >> > >> My collection of DTs don't seem to have one, except in the ancient > >> html variants that were extracted by the pengionppc folks for the > >> original PowerMac 8600 but I wonder if that's a bug in the > >> extraction script since they also have @0 on /packages etc... > > > > obviously, this isn't a priority issue, i was just working off a > > comment by rob herring that "chosen@0" is not defined by the current > > DTSpec 0.1, so it seemed appropriate to toss it. if there's a reason > > to hang onto it, that's fine with me. > > > > however, given the diff stat of the change to remove every single > > reference to that node name in the current kernel source: > > > > arch/microblaze/kernel/prom.c | 3 +-- > > arch/mips/generic/yamon-dt.c | 4 > > arch/powerpc/boot/oflib.c | 7 ++- > > drivers/of/base.c | 2 -- > > drivers/of/fdt.c | 5 + > > 5 files changed, 4 insertions(+), 17 deletions(-) > > > > it seems inconsistent that three architectures would be testing for > > that node, but none of the rest. consistency suggests that every > > architecture should take it into account, or none should. > > I generally agree and have moved various things from arch to > drivers/of/ to ensure that. But for legacy things, we have to allow > for exceptions. I agree with Ben and think that microblaze (they > generally just copied PPC), MIPS, and the FDT code in drivers/of/ can > be changed. i'm not quite sure what people have agreed on, so i'll just leave it in the hands of others here to submit patches for what they think is appropriate. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq
On Tue, Sep 05, 2017 at 11:13:40PM +0200, Łukasz Majewski wrote: > They key point here is the asoc_simple_card_parse_clk() function > from simple-card-utils.c > > Please look how the clock is assigned; It first checks for cpu > clock, then for "system-clock-frequency" DTS node and _finally_ > looks for another "child" clock [1], which is the codec attached to > I2C. Here is the routine that I understood from the code: 1) asoc_simple_card_parse_clk_cpu(dev, cpu, dai_link, cpu_dai); => asoc_simple_card_parse_clk(dev, cpu,// cpu node in sound{} [1] dai_link->cpu_of_node, // node ssi2 [2] cpu_dai, dai_link->cpu_dai_name); ==> 1.1) devm_get_clk_from_child(dev, node, NULL); // [1] ==> 1.2) of_property_read_u32(node, "system-clock-frequency", &val)// [1] ==> 1.3) devm_get_clk_from_child(dev, dai_of_node, NULL); // [2] 2) asoc_simple_card_parse_clk_codec(dev, codec, dai_link, codec_dai); => asoc_simple_card_parse_clk(dev, codec,// codec node in sound{} [3] dai_link->codec_of_node,// node tfa9879 [4] codec_dai, dai_link->codec_dai_name); ==> 2.1) devm_get_clk_from_child(dev, node, NULL); // [3] ==> 2.2) of_property_read_u32(node, "system-clock-frequency", &val)// [3] ==> 2.3) devm_get_clk_from_child(dev, dai_of_node, NULL); // [4] For the cpu routine, it first checks for clock property under cpu node of simple-card, then for "system-clock-frequency" in the cpu node of simple-card, and finally looks for clock property in ssi2 node. For codec routine, it first checks for clock property under codec node of simple-card, then for "system-clock-frequency" in codec node of simple-card, and finally looks for clock property in the tfa9879 node. The cpu and codec are totally separate, aren't they? And I don't think it's right if not. > >>which has clock from I2C (66 MHz). > > > >You mean I2C scl or I2S sclk? > > I2C scl. A couple of things here. 1) I don't think I2C scl can run at 66MHz...The 66MHz is probably the ipg clock of I2C controller. 2) Even if the scl does run at 66MHz, there is no point in passing the clock of I2C scl into an I2S dai-link. So I feel something must be wrong. I suggest you to add some printk to check the names of those nodes and x_of_node in the simple card driver. > My DTS [1] (it is different than other in-tree supported codecs - at > least I did not find similar setup in DTSes): > > sound { > compatible = "simple-audio-card"; > label = "tfa9879-mono"; > > simple-audio-card,dai-link { > /* DAC */ > format = "i2s"; > bitclock-master = <&dailink_master>; > frame-master = <&dailink_master>; > > dailink_master: cpu { > sound-dai = <&ssi2>; > }; > codec { > sound-dai = <&codec>; > }; > }; I remember there is another style for a single dai-link. Could you please try that one? There is an example in: Documentation/devicetree/bindings/sound/simple-card.txt
Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq
On Tue, Sep 5, 2017 at 6:13 PM, Łukasz Majewski wrote: > &i2c1 { > clock-frequency = <40>; > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_i2c1>; > status = "okay"; > > codec: tfa9879@6C { > #sound-dai-cells = <0>; > compatible = "tfa9879"; This codec seems to miss device tree support. Don't you need something like this? --- a/sound/soc/codecs/tfa9879.c +++ b/sound/soc/codecs/tfa9879.c @@ -312,9 +312,15 @@ static const struct i2c_device_id tfa9879_i2c_id[] = { }; MODULE_DEVICE_TABLE(i2c, tfa9879_i2c_id); +static const struct of_device_id tfa9879_of_match[] = { + { .compatible = "nxp,tfa9879", }, + { } +}; + static struct i2c_driver tfa9879_i2c_driver = { .driver = { .name = "tfa9879", + .of_match_table = tfa9879_of_match, }, .probe = tfa9879_i2c_probe, .remove = tfa9879_i2c_remove,
[PATCH 2/2] PCI: Remove unused function __pci_reset_function()
The last caller of __pci_reset_function() has been removed. Remove the function as well. Signed-off-by: Jan H. Schönherr --- drivers/pci/pci.c | 35 +++ include/linux/pci.h | 1 - 2 files changed, 3 insertions(+), 33 deletions(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index fdf65a6..dabcf5a 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4115,35 +4115,6 @@ static void pci_dev_restore(struct pci_dev *dev) } /** - * __pci_reset_function - reset a PCI device function - * @dev: PCI device to reset - * - * Some devices allow an individual function to be reset without affecting - * other functions in the same device. The PCI device must be responsive - * to PCI config space in order to use this function. - * - * The device function is presumed to be unused when this function is called. - * Resetting the device will make the contents of PCI configuration space - * random, so any caller of this must be prepared to reinitialise the - * device including MSI, bus mastering, BARs, decoding IO and memory spaces, - * etc. - * - * Returns 0 if the device function was successfully reset or negative if the - * device doesn't support resetting a single function. - */ -int __pci_reset_function(struct pci_dev *dev) -{ - int ret; - - pci_dev_lock(dev); - ret = __pci_reset_function_locked(dev); - pci_dev_unlock(dev); - - return ret; -} -EXPORT_SYMBOL_GPL(__pci_reset_function); - -/** * __pci_reset_function_locked - reset a PCI device function while holding * the @dev mutex lock. * @dev: PCI device to reset @@ -4233,8 +4204,8 @@ int pci_probe_reset_function(struct pci_dev *dev) * * This function does not just reset the PCI portion of a device, but * clears all the state associated with the device. This function differs - * from __pci_reset_function in that it saves and restores device state - * over the reset. + * from __pci_reset_function_locked() in that it saves and restores device state + * over the reset and takes the PCI device lock. * * Returns 0 if the device function was successfully reset or negative if the * device doesn't support resetting a single function. @@ -4269,7 +4240,7 @@ EXPORT_SYMBOL_GPL(pci_reset_function); * * This function does not just reset the PCI portion of a device, but * clears all the state associated with the device. This function differs - * from __pci_reset_function() in that it saves and restores device state + * from __pci_reset_function_locked() in that it saves and restores device state * over the reset. It also differs from pci_reset_function() in that it * requires the PCI device lock to be held. * diff --git a/include/linux/pci.h b/include/linux/pci.h index 4acf321..b55e0cf 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1066,7 +1066,6 @@ int pcie_set_mps(struct pci_dev *dev, int mps); int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed, enum pcie_link_width *width); void pcie_flr(struct pci_dev *dev); -int __pci_reset_function(struct pci_dev *dev); int __pci_reset_function_locked(struct pci_dev *dev); int pci_reset_function(struct pci_dev *dev); int pci_reset_function_locked(struct pci_dev *dev); -- 2.10.0.1.g70cd14e
[PATCH 1/2] PCI: Remove reset argument from pci_iov_{add, remove}_virtfn()
The reset argument passed to pci_iov_add_virtfn() and pci_iov_remove_virtfn() is always zero since commit 46cb7b1bd86fc227a ("PCI: Remove unused SR-IOV VF Migration support"). Remove the argument together with the associated code. Signed-off-by: Jan H. Schönherr --- arch/powerpc/kernel/eeh_driver.c | 4 ++-- drivers/pci/iov.c| 18 +- include/linux/pci.h | 8 3 files changed, 11 insertions(+), 19 deletions(-) diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c index c405c79..23ea86f 100644 --- a/arch/powerpc/kernel/eeh_driver.c +++ b/arch/powerpc/kernel/eeh_driver.c @@ -441,7 +441,7 @@ static void *eeh_add_virt_device(void *data, void *userdata) } #ifdef CONFIG_PPC_POWERNV - pci_iov_add_virtfn(edev->physfn, pdn->vf_index, 0); + pci_iov_add_virtfn(edev->physfn, pdn->vf_index); #endif return NULL; } @@ -499,7 +499,7 @@ static void *eeh_rmv_device(void *data, void *userdata) #ifdef CONFIG_PPC_POWERNV struct pci_dn *pdn = eeh_dev_to_pdn(edev); - pci_iov_remove_virtfn(edev->physfn, pdn->vf_index, 0); + pci_iov_remove_virtfn(edev->physfn, pdn->vf_index); edev->pdev = NULL; /* diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c index 120485d..21b55ca 100644 --- a/drivers/pci/iov.c +++ b/drivers/pci/iov.c @@ -113,7 +113,7 @@ resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno) return dev->sriov->barsz[resno - PCI_IOV_RESOURCES]; } -int pci_iov_add_virtfn(struct pci_dev *dev, int id, int reset) +int pci_iov_add_virtfn(struct pci_dev *dev, int id) { int i; int rc = -ENOMEM; @@ -157,9 +157,6 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id, int reset) BUG_ON(rc); } - if (reset) - __pci_reset_function(virtfn); - pci_device_add(virtfn, virtfn->bus); pci_bus_add_device(virtfn); @@ -187,7 +184,7 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id, int reset) return rc; } -void pci_iov_remove_virtfn(struct pci_dev *dev, int id, int reset) +void pci_iov_remove_virtfn(struct pci_dev *dev, int id) { char buf[VIRTFN_ID_LEN]; struct pci_dev *virtfn; @@ -198,11 +195,6 @@ void pci_iov_remove_virtfn(struct pci_dev *dev, int id, int reset) if (!virtfn) return; - if (reset) { - device_release_driver(&virtfn->dev); - __pci_reset_function(virtfn); - } - sprintf(buf, "virtfn%u", id); sysfs_remove_link(&dev->dev.kobj, buf); /* @@ -317,7 +309,7 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn) pci_cfg_access_unlock(dev); for (i = 0; i < initial; i++) { - rc = pci_iov_add_virtfn(dev, i, 0); + rc = pci_iov_add_virtfn(dev, i); if (rc) goto failed; } @@ -329,7 +321,7 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn) failed: while (i--) - pci_iov_remove_virtfn(dev, i, 0); + pci_iov_remove_virtfn(dev, i); pcibios_sriov_disable(dev); err_pcibios: @@ -355,7 +347,7 @@ static void sriov_disable(struct pci_dev *dev) return; for (i = 0; i < iov->num_VFs; i++) - pci_iov_remove_virtfn(dev, i, 0); + pci_iov_remove_virtfn(dev, i); pcibios_sriov_disable(dev); diff --git a/include/linux/pci.h b/include/linux/pci.h index f958d07..4acf321 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1903,8 +1903,8 @@ int pci_iov_virtfn_devfn(struct pci_dev *dev, int id); int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn); void pci_disable_sriov(struct pci_dev *dev); -int pci_iov_add_virtfn(struct pci_dev *dev, int id, int reset); -void pci_iov_remove_virtfn(struct pci_dev *dev, int id, int reset); +int pci_iov_add_virtfn(struct pci_dev *dev, int id); +void pci_iov_remove_virtfn(struct pci_dev *dev, int id); int pci_num_vf(struct pci_dev *dev); int pci_vfs_assigned(struct pci_dev *dev); int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs); @@ -1921,12 +1921,12 @@ static inline int pci_iov_virtfn_devfn(struct pci_dev *dev, int id) } static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn) { return -ENODEV; } -static inline int pci_iov_add_virtfn(struct pci_dev *dev, int id, int reset) +static inline int pci_iov_add_virtfn(struct pci_dev *dev, int id) { return -ENOSYS; } static inline void pci_iov_remove_virtfn(struct pci_dev *dev, -int id, int reset) { } +int id) { } static inline void pci_disable_sriov(struct pci_dev *dev) { } static inline int pci_num_vf(struct pci_dev *dev) { return 0; } static inline int pci_vfs_assigned(struct pci_dev *dev) -- 2.10.0.1.g70cd1
Re: [rfc 2/3] powerpc/mce: Extract physical_address for UE errors
On Tue, 5 Sep 2017 14:15:54 +1000 Balbir Singh wrote: > Walk the page table for NIP and extract the instruction. Then > use the instruction to find the effective address via analyse_instr(). > > We might have page table walking races, but we expect them to > be rare, the physical address extraction is best effort. The idea > is to then hook up this infrastructure to memory failure eventually. Cool. Too bad hardware doesn't give us the RA. > > Signed-off-by: Balbir Singh > --- > arch/powerpc/include/asm/mce.h | 2 +- > arch/powerpc/kernel/mce.c | 6 - > arch/powerpc/kernel/mce_power.c | 60 > + > 3 files changed, 61 insertions(+), 7 deletions(-) > > diff --git a/arch/powerpc/include/asm/mce.h b/arch/powerpc/include/asm/mce.h > index 75292c7..3a1226e 100644 > --- a/arch/powerpc/include/asm/mce.h > +++ b/arch/powerpc/include/asm/mce.h > @@ -204,7 +204,7 @@ struct mce_error_info { > > extern void save_mce_event(struct pt_regs *regs, long handled, > struct mce_error_info *mce_err, uint64_t nip, > -uint64_t addr); > +uint64_t addr, uint64_t phys_addr); > extern int get_mce_event(struct machine_check_event *mce, bool release); > extern void release_mce_event(void); > extern void machine_check_queue_event(void); > diff --git a/arch/powerpc/kernel/mce.c b/arch/powerpc/kernel/mce.c > index e254399..f41a75d 100644 > --- a/arch/powerpc/kernel/mce.c > +++ b/arch/powerpc/kernel/mce.c > @@ -82,7 +82,7 @@ static void mce_set_error_info(struct machine_check_event > *mce, > */ > void save_mce_event(struct pt_regs *regs, long handled, > struct mce_error_info *mce_err, > - uint64_t nip, uint64_t addr) > + uint64_t nip, uint64_t addr, uint64_t phys_addr) > { > int index = __this_cpu_inc_return(mce_nest_count) - 1; > struct machine_check_event *mce = this_cpu_ptr(&mce_event[index]); > @@ -140,6 +140,10 @@ void save_mce_event(struct pt_regs *regs, long handled, > } else if (mce->error_type == MCE_ERROR_TYPE_UE) { > mce->u.ue_error.effective_address_provided = true; > mce->u.ue_error.effective_address = addr; > + if (phys_addr != ULONG_MAX) { > + mce->u.ue_error.physical_address_provided = true; > + mce->u.ue_error.physical_address = phys_addr; > + } > } > return; > } > diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c > index b76ca19..b77a698 100644 > --- a/arch/powerpc/kernel/mce_power.c > +++ b/arch/powerpc/kernel/mce_power.c > @@ -27,6 +27,25 @@ > #include > #include > #include > +#include > +#include > +#include > + > +static unsigned long addr_to_pfn(struct mm_struct *mm, unsigned long addr) > +{ > + pte_t *ptep; > + unsigned long flags; > + > + local_irq_save(flags); > + if (mm == current->mm) > + ptep = find_current_mm_pte(mm->pgd, addr, NULL, NULL); > + else > + ptep = find_init_mm_pte(addr, NULL); > + local_irq_restore(flags); > + if (!ptep) > + return ULONG_MAX; > + return pte_pfn(*ptep); I think you need to check that it's still cacheable memory here? !pte_speical && pfn <= highest_memmap_pfn? > +} > > static void flush_tlb_206(unsigned int num_sets, unsigned int action) > { > @@ -489,7 +508,8 @@ static int mce_handle_ierror(struct pt_regs *regs, > > static int mce_handle_derror(struct pt_regs *regs, > const struct mce_derror_table table[], > - struct mce_error_info *mce_err, uint64_t *addr) > + struct mce_error_info *mce_err, uint64_t *addr, > + uint64_t *phys_addr) > { > uint64_t dsisr = regs->dsisr; > int handled = 0; > @@ -555,7 +575,37 @@ static int mce_handle_derror(struct pt_regs *regs, > mce_err->initiator = table[i].initiator; > if (table[i].dar_valid) > *addr = regs->dar; > - > + else if (mce_err->severity == MCE_SEV_ERROR_SYNC && > + table[i].error_type == MCE_ERROR_TYPE_UE) { > + /* > + * Carefully look at the NIP to determine > + * the instruction to analyse. Reading the NIP > + * in real-mode is tricky and can lead to recursive > + * faults > + */ What recursive faults? If you ensure NIP is cacheable memory, I guess you can get a recursive machine check from reading it, but that's probably tolerable. > + int instr; > + struct mm_struct *mm; > + unsigned long nip = regs->nip; > + unsigned long pfn = 0, instr_addr; > + struct instruction_op op; > + struct pt_regs tmp = *regs; > + > +
[PATCH v2 1/2] powerpc/npu: Use flush_all_mm() instead of flush_tlb_mm()
With the optimisations introduced by commit a46cc7a908 ("powerpc/mm/radix: Improve TLB/PWC flushes"), flush_tlb_mm() no longer flushes the page walk cache with radix. Switch to using flush_all_mm() to ensure the pwc and tlb are properly flushed on the nmmu. Signed-off-by: Alistair Popple --- arch/powerpc/platforms/powernv/npu-dma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c index 2cb6cbe..2fff9a65 100644 --- a/arch/powerpc/platforms/powernv/npu-dma.c +++ b/arch/powerpc/platforms/powernv/npu-dma.c @@ -549,7 +549,7 @@ static void mmio_invalidate(struct npu_context *npu_context, int va, * Unfortunately the nest mmu does not support flushing specific * addresses so we have to flush the whole mm. */ - flush_tlb_mm(npu_context->mm); + flush_all_mm(npu_context->mm); /* * Loop over all the NPUs this process is active on and launch -- 2.1.4
[PATCH v2 2/2] powerpc/powernv/npu: Don't explicitly flush nmmu tlb
The nest mmu required an explicit flush as a tlbi would not flush it in the same way as the core. However an alternate firmware fix exists which should eliminate the need for this flush, so instead add a device-tree property (ibm,nmmu-flush) on the NVLink2 PHB to enable it only if required. Signed-off-by: Alistair Popple --- Changes for v2: - Use mm_context_add_copro()/mm_context_remove_copro() instead of inc_mm_active_cpus()/dec_mm_active_cpus() arch/powerpc/platforms/powernv/npu-dma.c | 28 +++- arch/powerpc/platforms/powernv/pci.h | 3 +++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c index 2fff9a65..f6cbc1a 100644 --- a/arch/powerpc/platforms/powernv/npu-dma.c +++ b/arch/powerpc/platforms/powernv/npu-dma.c @@ -395,6 +395,7 @@ struct npu_context { struct pci_dev *npdev[NV_MAX_NPUS][NV_MAX_LINKS]; struct mmu_notifier mn; struct kref kref; + bool nmmu_flush; /* Callback to stop translation requests on a given GPU */ struct npu_context *(*release_cb)(struct npu_context *, void *); @@ -545,11 +546,13 @@ static void mmio_invalidate(struct npu_context *npu_context, int va, struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS]; unsigned long pid = npu_context->mm->context.id; - /* -* Unfortunately the nest mmu does not support flushing specific -* addresses so we have to flush the whole mm. -*/ - flush_all_mm(npu_context->mm); + if (npu_context->nmmu_flush) + /* +* Unfortunately the nest mmu does not support flushing specific +* addresses so we have to flush the whole mm once before +* shooting down the GPU translation. +*/ + flush_all_mm(npu_context->mm); /* * Loop over all the NPUs this process is active on and launch @@ -722,6 +725,16 @@ struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, return ERR_PTR(-ENODEV); npu_context->npdev[npu->index][nvlink_index] = npdev; + if (!nphb->npu.nmmu_flush) { + /* +* If we're not explicitly flushing ourselves we need to mark +* the thread for global flushes +*/ + npu_context->nmmu_flush = false; + mm_context_add_copro(mm); + } else + npu_context->nmmu_flush = true; + return npu_context; } EXPORT_SYMBOL(pnv_npu2_init_context); @@ -731,6 +744,9 @@ static void pnv_npu2_release_context(struct kref *kref) struct npu_context *npu_context = container_of(kref, struct npu_context, kref); + if (!npu_context->nmmu_flush) + mm_context_remove_copro(npu_context->mm); + npu_context->mm->context.npu_context = NULL; mmu_notifier_unregister(&npu_context->mn, npu_context->mm); @@ -819,6 +835,8 @@ int pnv_npu2_init(struct pnv_phb *phb) static int npu_index; uint64_t rc = 0; + phb->npu.nmmu_flush = + of_property_read_bool(phb->hose->dn, "ibm,nmmu-flush"); for_each_child_of_node(phb->hose->dn, dn) { gpdev = pnv_pci_get_gpu_dev(get_pci_dev(dn)); if (gpdev) { diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h index a95273c..22025c6 100644 --- a/arch/powerpc/platforms/powernv/pci.h +++ b/arch/powerpc/platforms/powernv/pci.h @@ -187,6 +187,9 @@ struct pnv_phb { /* Bitmask for MMIO register usage */ unsigned long mmio_atsd_usage; + + /* Do we need to explicitly flush the nest mmu? */ + bool nmmu_flush; } npu; #ifdef CONFIG_CXL_BASE -- 2.1.4
Re: [PATCH 1/2] PCI: Remove reset argument from pci_iov_{add,remove}_virtfn()
On Wed, 2017-09-06 at 01:21 +0200, Jan H. Schönherr wrote: > The reset argument passed to pci_iov_add_virtfn() and > pci_iov_remove_virtfn() is always zero since commit 46cb7b1bd86fc227a > ("PCI: Remove unused SR-IOV VF Migration support"). > > Remove the argument together with the associated code. > > Signed-off-by: Jan H. Schönherr Acked-by: Russell Currey
Re: [rfc 2/3] powerpc/mce: Extract physical_address for UE errors
On Wed, Sep 6, 2017 at 10:36 AM, Nicholas Piggin wrote: > On Tue, 5 Sep 2017 14:15:54 +1000 > Balbir Singh wrote: > >> Walk the page table for NIP and extract the instruction. Then >> use the instruction to find the effective address via analyse_instr(). >> >> We might have page table walking races, but we expect them to >> be rare, the physical address extraction is best effort. The idea >> is to then hook up this infrastructure to memory failure eventually. > > Cool. Too bad hardware doesn't give us the RA. > >> >> Signed-off-by: Balbir Singh >> --- >> arch/powerpc/include/asm/mce.h | 2 +- >> arch/powerpc/kernel/mce.c | 6 - >> arch/powerpc/kernel/mce_power.c | 60 >> + >> 3 files changed, 61 insertions(+), 7 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/mce.h b/arch/powerpc/include/asm/mce.h >> index 75292c7..3a1226e 100644 >> --- a/arch/powerpc/include/asm/mce.h >> +++ b/arch/powerpc/include/asm/mce.h >> @@ -204,7 +204,7 @@ struct mce_error_info { >> >> extern void save_mce_event(struct pt_regs *regs, long handled, >> struct mce_error_info *mce_err, uint64_t nip, >> -uint64_t addr); >> +uint64_t addr, uint64_t phys_addr); >> extern int get_mce_event(struct machine_check_event *mce, bool release); >> extern void release_mce_event(void); >> extern void machine_check_queue_event(void); >> diff --git a/arch/powerpc/kernel/mce.c b/arch/powerpc/kernel/mce.c >> index e254399..f41a75d 100644 >> --- a/arch/powerpc/kernel/mce.c >> +++ b/arch/powerpc/kernel/mce.c >> @@ -82,7 +82,7 @@ static void mce_set_error_info(struct machine_check_event >> *mce, >> */ >> void save_mce_event(struct pt_regs *regs, long handled, >> struct mce_error_info *mce_err, >> - uint64_t nip, uint64_t addr) >> + uint64_t nip, uint64_t addr, uint64_t phys_addr) >> { >> int index = __this_cpu_inc_return(mce_nest_count) - 1; >> struct machine_check_event *mce = this_cpu_ptr(&mce_event[index]); >> @@ -140,6 +140,10 @@ void save_mce_event(struct pt_regs *regs, long handled, >> } else if (mce->error_type == MCE_ERROR_TYPE_UE) { >> mce->u.ue_error.effective_address_provided = true; >> mce->u.ue_error.effective_address = addr; >> + if (phys_addr != ULONG_MAX) { >> + mce->u.ue_error.physical_address_provided = true; >> + mce->u.ue_error.physical_address = phys_addr; >> + } >> } >> return; >> } >> diff --git a/arch/powerpc/kernel/mce_power.c >> b/arch/powerpc/kernel/mce_power.c >> index b76ca19..b77a698 100644 >> --- a/arch/powerpc/kernel/mce_power.c >> +++ b/arch/powerpc/kernel/mce_power.c >> @@ -27,6 +27,25 @@ >> #include >> #include >> #include >> +#include >> +#include >> +#include >> + >> +static unsigned long addr_to_pfn(struct mm_struct *mm, unsigned long addr) >> +{ >> + pte_t *ptep; >> + unsigned long flags; >> + >> + local_irq_save(flags); >> + if (mm == current->mm) >> + ptep = find_current_mm_pte(mm->pgd, addr, NULL, NULL); >> + else >> + ptep = find_init_mm_pte(addr, NULL); >> + local_irq_restore(flags); >> + if (!ptep) >> + return ULONG_MAX; >> + return pte_pfn(*ptep); > > I think you need to check that it's still cacheable memory here? > !pte_speical && pfn <= highest_memmap_pfn? > find_*pte will return a NULL PTE, so we do have a check there. !pte_special is a good check to have, I'll add it > >> +} >> >> static void flush_tlb_206(unsigned int num_sets, unsigned int action) >> { >> @@ -489,7 +508,8 @@ static int mce_handle_ierror(struct pt_regs *regs, >> >> static int mce_handle_derror(struct pt_regs *regs, >> const struct mce_derror_table table[], >> - struct mce_error_info *mce_err, uint64_t *addr) >> + struct mce_error_info *mce_err, uint64_t *addr, >> + uint64_t *phys_addr) >> { >> uint64_t dsisr = regs->dsisr; >> int handled = 0; >> @@ -555,7 +575,37 @@ static int mce_handle_derror(struct pt_regs *regs, >> mce_err->initiator = table[i].initiator; >> if (table[i].dar_valid) >> *addr = regs->dar; >> - >> + else if (mce_err->severity == MCE_SEV_ERROR_SYNC && >> + table[i].error_type == MCE_ERROR_TYPE_UE) { >> + /* >> + * Carefully look at the NIP to determine >> + * the instruction to analyse. Reading the NIP >> + * in real-mode is tricky and can lead to recursive >> + * faults >> + */ > > What recursive faults? If you ensure NIP is cacheable memory, I guess you > can get a recursive machine check from reading it, but that's probably > tolerable. Yep, jus
[PATCH] powerpc/kvm/xive: Don't access TIMA using byte accesses
The TIMA only supports byte stores to the CPPR, for everything else, we need to do a 32-bit or 64-bit load. Signed-off-by: Benjamin Herrenschmidt Fixes: 2c4fb78f78b6e420604ee1b05bdfb5c1d637869f --- arch/powerpc/kvm/book3s_hv_rm_xive.c| 1 - arch/powerpc/kvm/book3s_xive.c | 1 - arch/powerpc/kvm/book3s_xive_template.c | 7 --- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/arch/powerpc/kvm/book3s_hv_rm_xive.c b/arch/powerpc/kvm/book3s_hv_rm_xive.c index abf5f01b6eb1..5b81a807d742 100644 --- a/arch/powerpc/kvm/book3s_hv_rm_xive.c +++ b/arch/powerpc/kvm/book3s_hv_rm_xive.c @@ -38,7 +38,6 @@ static inline void __iomem *get_tima_phys(void) #define __x_tima get_tima_phys() #define __x_eoi_page(xd) ((void __iomem *)((xd)->eoi_page)) #define __x_trig_page(xd) ((void __iomem *)((xd)->trig_page)) -#define __x_readb __raw_rm_readb #define __x_writeb __raw_rm_writeb #define __x_readw __raw_rm_readw #define __x_readq __raw_rm_readq diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c index 08b200a0bbce..13304622ab1c 100644 --- a/arch/powerpc/kvm/book3s_xive.c +++ b/arch/powerpc/kvm/book3s_xive.c @@ -48,7 +48,6 @@ #define __x_tima xive_tima #define __x_eoi_page(xd) ((void __iomem *)((xd)->eoi_mmio)) #define __x_trig_page(xd) ((void __iomem *)((xd)->trig_mmio)) -#define __x_readb __raw_readb #define __x_writeb __raw_writeb #define __x_readw __raw_readw #define __x_readq __raw_readq diff --git a/arch/powerpc/kvm/book3s_xive_template.c b/arch/powerpc/kvm/book3s_xive_template.c index d1ed2c41b5d2..c7a5deadd1cc 100644 --- a/arch/powerpc/kvm/book3s_xive_template.c +++ b/arch/powerpc/kvm/book3s_xive_template.c @@ -28,7 +28,8 @@ static void GLUE(X_PFX,ack_pending)(struct kvmppc_xive_vcpu *xc) * bit. */ if (cpu_has_feature(CPU_FTR_POWER9_DD1)) { - u8 pipr = __x_readb(__x_tima + TM_QW1_OS + TM_PIPR); + __be64 qw1 = __x_readq(__x_tima + TM_QW1_OS); + u8 pipr = be64_to_cpu(qw1) & 0xff; if (pipr >= xc->hw_cppr) return; } @@ -336,7 +337,6 @@ X_STATIC unsigned long GLUE(X_PFX,h_ipoll)(struct kvm_vcpu *vcpu, unsigned long struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu; u8 pending = xc->pending; u32 hirq; - u8 pipr; pr_devel("H_IPOLL(server=%ld)\n", server); @@ -353,7 +353,8 @@ X_STATIC unsigned long GLUE(X_PFX,h_ipoll)(struct kvm_vcpu *vcpu, unsigned long pending = 0xff; } else { /* Grab pending interrupt if any */ - pipr = __x_readb(__x_tima + TM_QW1_OS + TM_PIPR); + __be64 qw1 = __x_readq(__x_tima + TM_QW1_OS); + u8 pipr = be64_to_cpu(qw1) & 0xff; if (pipr < 8) pending |= 1 << pipr; }
Re: [PATCH v2 2/2] powerpc/powernv/npu: Don't explicitly flush nmmu tlb
Le 06/09/2017 à 03:49, Alistair Popple a écrit : The nest mmu required an explicit flush as a tlbi would not flush it in the same way as the core. However an alternate firmware fix exists which should eliminate the need for this flush, so instead add a device-tree property (ibm,nmmu-flush) on the NVLink2 PHB to enable it only if required. Signed-off-by: Alistair Popple --- FWIW, Reviewed-by: Frederic Barrat Changes for v2: - Use mm_context_add_copro()/mm_context_remove_copro() instead of inc_mm_active_cpus()/dec_mm_active_cpus() arch/powerpc/platforms/powernv/npu-dma.c | 28 +++- arch/powerpc/platforms/powernv/pci.h | 3 +++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c index 2fff9a65..f6cbc1a 100644 --- a/arch/powerpc/platforms/powernv/npu-dma.c +++ b/arch/powerpc/platforms/powernv/npu-dma.c @@ -395,6 +395,7 @@ struct npu_context { struct pci_dev *npdev[NV_MAX_NPUS][NV_MAX_LINKS]; struct mmu_notifier mn; struct kref kref; + bool nmmu_flush; /* Callback to stop translation requests on a given GPU */ struct npu_context *(*release_cb)(struct npu_context *, void *); @@ -545,11 +546,13 @@ static void mmio_invalidate(struct npu_context *npu_context, int va, struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS]; unsigned long pid = npu_context->mm->context.id; - /* -* Unfortunately the nest mmu does not support flushing specific -* addresses so we have to flush the whole mm. -*/ - flush_all_mm(npu_context->mm); + if (npu_context->nmmu_flush) + /* +* Unfortunately the nest mmu does not support flushing specific +* addresses so we have to flush the whole mm once before +* shooting down the GPU translation. +*/ + flush_all_mm(npu_context->mm); /* * Loop over all the NPUs this process is active on and launch @@ -722,6 +725,16 @@ struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, return ERR_PTR(-ENODEV); npu_context->npdev[npu->index][nvlink_index] = npdev; + if (!nphb->npu.nmmu_flush) { + /* +* If we're not explicitly flushing ourselves we need to mark +* the thread for global flushes +*/ + npu_context->nmmu_flush = false; + mm_context_add_copro(mm); + } else + npu_context->nmmu_flush = true; + return npu_context; } EXPORT_SYMBOL(pnv_npu2_init_context); @@ -731,6 +744,9 @@ static void pnv_npu2_release_context(struct kref *kref) struct npu_context *npu_context = container_of(kref, struct npu_context, kref); + if (!npu_context->nmmu_flush) + mm_context_remove_copro(npu_context->mm); + npu_context->mm->context.npu_context = NULL; mmu_notifier_unregister(&npu_context->mn, npu_context->mm); @@ -819,6 +835,8 @@ int pnv_npu2_init(struct pnv_phb *phb) static int npu_index; uint64_t rc = 0; + phb->npu.nmmu_flush = + of_property_read_bool(phb->hose->dn, "ibm,nmmu-flush"); for_each_child_of_node(phb->hose->dn, dn) { gpdev = pnv_pci_get_gpu_dev(get_pci_dev(dn)); if (gpdev) { diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h index a95273c..22025c6 100644 --- a/arch/powerpc/platforms/powernv/pci.h +++ b/arch/powerpc/platforms/powernv/pci.h @@ -187,6 +187,9 @@ struct pnv_phb { /* Bitmask for MMIO register usage */ unsigned long mmio_atsd_usage; + + /* Do we need to explicitly flush the nest mmu? */ + bool nmmu_flush; } npu; #ifdef CONFIG_CXL_BASE