[PATCH kernel] powerpc/pseries/iommu: Add cond_resched() for huge updates
Mapping ~5.000.000 TCEs currently takes about 40s; this is the amount required for a 300GB VM with 64k IOMMU page size. Anything bigger than this produces RCU stall warnings. This adds cond_resched() to allow the scheduler to do context switching when it decides to. This loop is called from dma_set_mask() which is a sleepable context. Signed-off-by: Alexey Kardashevskiy --- arch/powerpc/platforms/pseries/iommu.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c index 889dc2e44b89..2b8de822272f 100644 --- a/arch/powerpc/platforms/pseries/iommu.c +++ b/arch/powerpc/platforms/pseries/iommu.c @@ -459,6 +459,7 @@ static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn, static int tce_setrange_multi_pSeriesLP_walk(unsigned long start_pfn, unsigned long num_pfn, void *arg) { + cond_resched(); return tce_setrange_multi_pSeriesLP(start_pfn, num_pfn, arg); } -- 2.17.1
Re: Non deterministic kernel crashes after minimal devicetree changes.
Any ideas how to deal with this problem? Best regards -- kernel concepts GmbH Maik Nassauer Hauptstraße 16 maik.nassa...@kernelconcepts.de D-57074 Siegen Tel: +49 271-338857-21 http://www.kernelconcepts.de/ HR Siegen, HR B 9613 Geschäftsführer: Ole Reinhardt signature.asc Description: This is a digitally signed message part
Re: question on "powerpc/pseries/dma: Allow SWIOTLB"
On Sat, Jul 20, 2019 at 09:22:49PM +1000, Alexey Kardashevskiy wrote: > > > On 19/07/2019 22:25, Christoph Hellwig wrote: > > On Fri, Jul 19, 2019 at 06:23:59PM +1000, Alexey Kardashevskiy wrote: > >> It is getting there and I still do not see why "swiotlb=force" should not > >> work if chosed in the cmdline. > > > > Ok, makes sense. But that means we also have the issue in a few > > other places.. > > Hmm, where? I got broadcom ethernet working with this. In all the other IOMMU drivers that conditionally forward to the dma direct ops. So it shouldn't affect powerpc.
Re: [PATCH v2] powerpc: slightly improve cache helpers
Segher Boessenkool writes: > On Sun, Jul 21, 2019 at 12:58:46AM -0700, Nathan Chancellor wrote: >> I have attached the disassembly of arch/powerpc/kernel/mem.o with >> clear_page (working) and broken_clear_page (broken), along with the side >> by side diff. My assembly knowledge is fairly limited as it stands and >> it is certainly not up to snuff on PowerPC so I have no idea what I am >> looking for. Please let me know if anything immediately looks off or if >> there is anything else I can do to help out. > > You might want to use a disassembler that shows most simplified mnemonics, > and you crucially should show the relocations. "objdump -dr" works nicely. > >> 017c clear_user_page: >> 17c: 38 80 00 80li 4, 128 >> 180: 7c 89 03 a6mtctr 4 >> 184: 7c 00 1f ecdcbz 0, 3 >> 188: 38 63 00 20addi 3, 3, 32 >> 18c: 42 00 ff f8bdnz .+65528 > > That offset is incorrectly disassembled, btw (it's a signed field, not > unsigned). > >> 017c clear_user_page: >> 17c: 94 21 ff f0stwu 1, -16(1) >> 180: 38 80 00 80li 4, 128 >> 184: 38 63 ff e0addi 3, 3, -32 >> 188: 7c 89 03 a6mtctr 4 >> 18c: 38 81 00 0faddi 4, 1, 15 >> 190: 8c c3 00 20lbzu 6, 32(3) >> 194: 98 c1 00 0fstb 6, 15(1) >> 198: 7c 00 27 ecdcbz 0, 4 >> 19c: 42 00 ff f4bdnz .+65524 > > Uh, yeah, well, I have no idea what clang tried here, but that won't > work. It's copying a byte from each target cache line to the stack, > and then does clears the cache line containing that byte on the stack. So it seems like this is a clang bug. None of the distros we support use clang, but we would still like to keep it working if we can. Looking at the original patch, the only upside is that the compiler can use both RA and RB to compute the address, rather than us forcing RA to 0. But at least with my compiler here (GCC 8 vintage) I don't actually see GCC ever using both GPRs even with the patch. Or at least, there's no difference before/after the patch as far as I can see. So my inclination is to revert the original patch. We can try again in a few years :D Thoughts? cheers
Re: [PATCH 2/2] powerpc: expose secure variables via sysfs
On Thu, 2019-06-13 at 16:50 -0400, Nayna Jain wrote: > As part of PowerNV secure boot support, OS verification keys are stored > and controlled by OPAL as secure variables. These need to be exposed to > the userspace so that sysadmins can perform key management tasks. > > This patch adds the support to expose secure variables via a sysfs > interface It reuses the the existing efi defined hooks and backend in > order to maintain the compatibility with the userspace tools. > > Though it reuses a great deal of efi, POWER platforms do not use EFI. > A new config, POWER_SECVAR_SYSFS, is defined to enable this new sysfs > interface. I spent all day staring at the skiboot bits of this, so I figured I should see what the kernel bits looked like too... On the last version of this patch set (posted by Claudio) Matthew Garret was pretty explicit about the interface here not conforming to the expectations of efivars[1], so what's changed in this patch set to fix that? As far as I can tell it's has the same basic problems as before, just with a slightly different OPAL interface. [1] https://www.spinics.net/lists/linux-efi/msg15897.html > > Signed-off-by: Nayna Jain > --- > arch/powerpc/Kconfig | 2 + > drivers/firmware/Makefile| 1 + > drivers/firmware/efi/efivars.c | 2 +- > drivers/firmware/powerpc/Kconfig | 12 + > drivers/firmware/powerpc/Makefile| 3 + > drivers/firmware/powerpc/efi_error.c | 46 > drivers/firmware/powerpc/secvar.c| 326 +++ > 7 files changed, 391 insertions(+), 1 deletion(-) > create mode 100644 drivers/firmware/powerpc/Kconfig > create mode 100644 drivers/firmware/powerpc/Makefile > create mode 100644 drivers/firmware/powerpc/efi_error.c > create mode 100644 drivers/firmware/powerpc/secvar.c > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index 9de77bb14f54..1548dd8cf1a0 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -916,6 +916,8 @@ config PPC_SECURE_BOOT > allows user to enable OS Secure Boot on PowerPC systems that > have firmware secure boot support. > > +source "drivers/firmware/powerpc/Kconfig" > + > endmenu All of this is *very* powernv specific, so why is it here and not in arch/powerpc/platforms/powernv/ with the rest of the powernv platform code? Also, nothing about this is generic to powerpc as a whole so the Kconfig symbol should probably be change the Kconfig symbol to POWERNV_SECURE_BOOT or OPAL_SECURE_BOOT > config ISA_DMA_API > diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile > index 3fa0b34eb72f..8cfaf7e6769d 100644 > --- a/drivers/firmware/Makefile > +++ b/drivers/firmware/Makefile > @@ -33,3 +33,4 @@ obj-$(CONFIG_UEFI_CPER) += efi/ > obj-y+= imx/ > obj-y+= tegra/ > obj-y+= xilinx/ > +obj-$(CONFIG_POWER_SECVAR_SYSFS) += powerpc/ > diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c > index 7576450c8254..30ef53003c24 100644 > --- a/drivers/firmware/efi/efivars.c > +++ b/drivers/firmware/efi/efivars.c > @@ -664,7 +664,7 @@ int efivars_sysfs_init(void) > struct kobject *parent_kobj = efivars_kobject(); > int error = 0; > > - if (!efi_enabled(EFI_RUNTIME_SERVICES)) > + if (IS_ENABLED(CONFIG_EFI) && !efi_enabled(EFI_RUNTIME_SERVICES)) > return -ENODEV; > > /* No efivars has been registered yet */ Seems a bit sketch. > diff --git a/drivers/firmware/powerpc/Kconfig > b/drivers/firmware/powerpc/Kconfig > new file mode 100644 > index ..e0303fc517d5 > --- /dev/null > +++ b/drivers/firmware/powerpc/Kconfig > @@ -0,0 +1,12 @@ > +config POWER_SECVAR_SYSFS > + tristate "Enable sysfs interface for POWER secure variables" > + default n > + depends on PPC_SECURE_BOOT > + select UCS2_STRING > + help > + POWER secure variables are managed and controlled by OPAL. > + These variables are exposed to userspace via sysfs to allow > + user to read/write these variables. Say Y if you have secure > + boot enabled and want to expose variables to userspace. > + > +source "drivers/firmware/efi/Kconfig" Again, this is all powernv specific and not generic to all powerpc platforms. > diff --git a/drivers/firmware/powerpc/Makefile > b/drivers/firmware/powerpc/Makefile > new file mode 100644 > index ..d5fa3b007315 > --- /dev/null > +++ b/drivers/firmware/powerpc/Makefile > @@ -0,0 +1,3 @@ > +# SPDX-License-Identifier: GPL-2.0 > + > +obj-$(CONFIG_POWER_SECVAR_SYSFS) += ../efi/efivars.o efi_error.o > ../efi/vars.o secvar.o Uh... That is going to need an Ack from someone in EFI land. > diff --git a/drivers/firmware/powerpc/efi_error.c > b/drivers/firmware/powerpc/efi_error.c > new file mode 100644 > index ..b5cabd52e6b4 > --- /dev/null > +++ b/drivers/firmware/powerpc/efi_er
Re: [PATCH v4 7/8] KVM: PPC: Ultravisor: Enter a secure guest
Sukadev Bhattiprolu writes: > Michael Ellerman [m...@ellerman.id.au] wrote: >> Claudio Carvalho writes: >> > From: Sukadev Bhattiprolu >> > >> > To enter a secure guest, we have to go through the ultravisor, therefore >> > we do a ucall when we are entering a secure guest. >> > >> > This change is needed for any sort of entry to the secure guest from the >> > hypervisor, whether it is a return from an hcall, a return from a >> > hypervisor interrupt, or the first time that a secure guest vCPU is run. >> > >> > If we are returning from an hcall, the results are already in the >> > appropriate registers R3:12, except for R3, R6 and R7. R3 has the status >> > of the reflected hcall, therefore we move it to R0 for the ultravisor and >> > set R3 to the UV_RETURN ucall number. R6,7 were used as temporary >> > registers, hence we restore them. >> >> This is another case where some documentation would help people to >> review the code. >> >> > Have fast_guest_return check the kvm_arch.secure_guest field so that a >> > new CPU enters UV when started (in response to a RTAS start-cpu call). >> > >> > Thanks to input from Paul Mackerras, Ram Pai and Mike Anderson. >> > >> > Signed-off-by: Sukadev Bhattiprolu >> > [ Pass SRR1 in r11 for UV_RETURN, fix kvmppc_msr_interrupt to preserve >> > the MSR_S bit ] >> > Signed-off-by: Paul Mackerras >> > [ Fix UV_RETURN ucall number and arch.secure_guest check ] >> > Signed-off-by: Ram Pai >> > [ Save the actual R3 in R0 for the ultravisor and use R3 for the >> > UV_RETURN ucall number. Update commit message and ret_to_ultra comment ] >> > Signed-off-by: Claudio Carvalho >> > --- >> > arch/powerpc/include/asm/kvm_host.h | 1 + >> > arch/powerpc/include/asm/ultravisor-api.h | 1 + >> > arch/powerpc/kernel/asm-offsets.c | 1 + >> > arch/powerpc/kvm/book3s_hv_rmhandlers.S | 40 +++ >> > 4 files changed, 37 insertions(+), 6 deletions(-) >> > >> > diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S >> > b/arch/powerpc/kvm/book3s_hv_rmhandlers.S >> > index cffb365d9d02..89813ca987c2 100644 >> > --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S >> > +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S >> > @@ -36,6 +36,7 @@ >> > #include >> > #include >> > #include >> > +#include >> > >> > /* Sign-extend HDEC if not on POWER9 */ >> > #define EXTEND_HDEC(reg) \ >> > @@ -1092,16 +1093,12 @@ BEGIN_FTR_SECTION >> > END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) >> > >> >ld r5, VCPU_LR(r4) >> > - ld r6, VCPU_CR(r4) >> >mtlrr5 >> > - mtcrr6 >> > >> >ld r1, VCPU_GPR(R1)(r4) >> >ld r2, VCPU_GPR(R2)(r4) >> >ld r3, VCPU_GPR(R3)(r4) >> >ld r5, VCPU_GPR(R5)(r4) >> > - ld r6, VCPU_GPR(R6)(r4) >> > - ld r7, VCPU_GPR(R7)(r4) >> >ld r8, VCPU_GPR(R8)(r4) >> >ld r9, VCPU_GPR(R9)(r4) >> >ld r10, VCPU_GPR(R10)(r4) >> > @@ -1119,10 +1116,38 @@ BEGIN_FTR_SECTION >> >mtspr SPRN_HDSISR, r0 >> > END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300) >> > >> > + ld r6, VCPU_KVM(r4) >> > + lbz r7, KVM_SECURE_GUEST(r6) >> > + cmpdi r7, 0 >> >> You could hoist the load of r6 and r7 to here? > > we could move 'ld r7' here. r6 is used to restore CR below so > it (r6) has to stay there? It's used to restore CR in both paths, so both paths load VCPU_CR(r4) into r6. So we could instead do that load once, before the branch? >> > + bne ret_to_ultra >> > + >> > + lwz r6, VCPU_CR(r4) >> > + mtcrr6 >> > + >> > + ld r7, VCPU_GPR(R7)(r4) >> > + ld r6, VCPU_GPR(R6)(r4) >> >ld r0, VCPU_GPR(R0)(r4) >> >ld r4, VCPU_GPR(R4)(r4) >> >HRFI_TO_GUEST >> >b . >> > +/* >> > + * We are entering a secure guest, so we have to invoke the ultravisor to >> > do >> > + * that. If we are returning from a hcall, the results are already in the >> > + * appropriate registers R3:12, except for R3, R6 and R7. R3 has the >> > status of >> > + * the reflected hcall, therefore we move it to R0 for the ultravisor and >> > set >> > + * R3 to the UV_RETURN ucall number. R6,7 were used as temporary registers >> > + * above, hence we restore them. >> > + */ >> > +ret_to_ultra: >> > + lwz r6, VCPU_CR(r4) >> > + mtcrr6 >> > + mfspr r11, SPRN_SRR1 >> > + mr r0, r3 >> > + LOAD_REG_IMMEDIATE(r3, UV_RETURN) >> >> Worth open coding to save three instructions? > > Yes, good point: > > - LOAD_REG_IMMEDIATE(r3, UV_RETURN) > + > + li r3, 0 > + orisr3, r3, (UV_RETURN)@__AS_ATHIGH > + ori r3, r3, (UV_RETURN)@l This should do it no? li r3, 0 orisr3, r3, UV_RETURN cheers
Re: [PATCH] powerpc/dma: Fix invalid DMA mmap behavior
Arnd Bergmann writes: > On Thu, Jul 18, 2019 at 11:52 AM Christoph Hellwig wrote: >> On Thu, Jul 18, 2019 at 10:49:34AM +0200, Christoph Hellwig wrote: >> > On Thu, Jul 18, 2019 at 01:45:16PM +1000, Oliver O'Halloran wrote: >> > > > Other than m68k, mips, and arm64, everybody else that doesn't have >> > > > ARCH_NO_COHERENT_DMA_MMAP set uses this default implementation, so >> > > > I assume this behavior is acceptable on those architectures. >> > > >> > > It might be acceptable, but there's no reason to use pgport_noncached >> > > if the platform supports cache-coherent DMA. >> > > >> > > Christoph (+cc) made the change so maybe he saw something we're missing. >> > >> > I always found the forcing of noncached access even for coherent >> > devices a little odd, but this was inherited from the previous >> > implementation, which surprised me a bit as the different attributes >> > are usually problematic even on x86. Let me dig into the history a >> > bit more, but I suspect the righ fix is to default to cached mappings >> > for coherent devices. >> >> Ok, some history: >> >> The generic dma mmap implementation, which we are effectively still >> using today was added by: >> >> commit 64ccc9c033c6089b2d426dad3c56477ab066c999 >> Author: Marek Szyprowski >> Date: Thu Jun 14 13:03:04 2012 +0200 >> >> common: dma-mapping: add support for generic dma_mmap_* calls >> >> and unconditionally uses pgprot_noncached in dma_common_mmap, which is >> then used as the fallback by dma_mmap_attrs if no ->mmap method is >> present. At that point we already had the powerpc implementation >> that only uses pgprot_noncached for non-coherent mappings, and >> the arm one, which uses pgprot_writecombine if DMA_ATTR_WRITE_COMBINE >> is set and otherwise pgprot_dmacoherent, which seems to be uncached. >> Arm did support coherent platforms at that time, but they might have >> been an afterthought and not handled properly. > > Cache-coherent devices are still very rare on 32-bit ARM. > > Among the callers of dma_mmap_coherent(), almost all are in platform > specific device drivers that only ever run on noncoherent ARM SoCs, > which explains why nobody would have noticed problems. > > There is also a difference in behavior between ARM and PowerPC > when dealing with mismatched cacheability attributes: If the same > page is mapped as both cached and uncached to, this may > cause silent undefined behavior on ARM, while PowerPC should > enter a checkstop as soon as it notices. On newer Power CPUs it's actually more like the ARM behaviour. I don't know for sure that it will *never* checkstop but there are at least cases where it won't. There's some (not much) detail in the Power8/9 user manuals. cheers
Applied "ASoC: fsl_esai: Wrap some operations to be functions" to the asoc tree
The patch ASoC: fsl_esai: Wrap some operations to be functions has been applied to the asoc tree at https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.4 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From 5be6155b50bbf7083b4bfa219e4ce6d1491f42f0 Mon Sep 17 00:00:00 2001 From: Shengjiu Wang Date: Thu, 11 Jul 2019 18:49:45 +0800 Subject: [PATCH] ASoC: fsl_esai: Wrap some operations to be functions Extract the operation to be functions, to improve the readability. In this patch, fsl_esai_hw_init, fsl_esai_register_restore, fsl_esai_trigger_start and fsl_esai_trigger_stop are extracted. Signed-off-by: Shengjiu Wang Acked-by: Nicolin Chen Link: https://lore.kernel.org/r/804d7e75ae7e06a913479912b578b3538ca7cd3f.1562842206.git.shengjiu.w...@nxp.com Signed-off-by: Mark Brown --- sound/soc/fsl/fsl_esai.c | 188 --- 1 file changed, 117 insertions(+), 71 deletions(-) diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index 10d2210c91ef..ab460d6d7432 100644 --- a/sound/soc/fsl/fsl_esai.c +++ b/sound/soc/fsl/fsl_esai.c @@ -35,6 +35,7 @@ * @fifo_depth: depth of tx/rx FIFO * @slot_width: width of each DAI slot * @slots: number of slots + * @channels: channel num for tx or rx * @hck_rate: clock rate of desired HCKx clock * @sck_rate: clock rate of desired SCKx clock * @hck_dir: the direction of HCKx pads @@ -57,6 +58,7 @@ struct fsl_esai { u32 slots; u32 tx_mask; u32 rx_mask; + u32 channels[2]; u32 hck_rate[2]; u32 sck_rate[2]; bool hck_dir[2]; @@ -543,64 +545,132 @@ static int fsl_esai_hw_params(struct snd_pcm_substream *substream, return 0; } +static int fsl_esai_hw_init(struct fsl_esai *esai_priv) +{ + struct platform_device *pdev = esai_priv->pdev; + int ret; + + /* Reset ESAI unit */ + ret = regmap_update_bits(esai_priv->regmap, REG_ESAI_ECR, +ESAI_ECR_ESAIEN_MASK | ESAI_ECR_ERST_MASK, +ESAI_ECR_ESAIEN | ESAI_ECR_ERST); + if (ret) { + dev_err(&pdev->dev, "failed to reset ESAI: %d\n", ret); + return ret; + } + + /* +* We need to enable ESAI so as to access some of its registers. +* Otherwise, we would fail to dump regmap from user space. +*/ + ret = regmap_update_bits(esai_priv->regmap, REG_ESAI_ECR, +ESAI_ECR_ESAIEN_MASK | ESAI_ECR_ERST_MASK, +ESAI_ECR_ESAIEN); + if (ret) { + dev_err(&pdev->dev, "failed to enable ESAI: %d\n", ret); + return ret; + } + + regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC, + ESAI_PRRC_PDC_MASK, 0); + regmap_update_bits(esai_priv->regmap, REG_ESAI_PCRC, + ESAI_PCRC_PC_MASK, 0); + + return 0; +} + +static int fsl_esai_register_restore(struct fsl_esai *esai_priv) +{ + int ret; + + /* FIFO reset for safety */ + regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR, + ESAI_xFCR_xFR, ESAI_xFCR_xFR); + regmap_update_bits(esai_priv->regmap, REG_ESAI_RFCR, + ESAI_xFCR_xFR, ESAI_xFCR_xFR); + + regcache_mark_dirty(esai_priv->regmap); + ret = regcache_sync(esai_priv->regmap); + if (ret) + return ret; + + /* FIFO reset done */ + regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR, ESAI_xFCR_xFR, 0); + regmap_update_bits(esai_priv->regmap, REG_ESAI_RFCR, ESAI_xFCR_xFR, 0); + + return 0; +} + +static void fsl_esai_trigger_start(struct fsl_esai *esai_priv, bool tx) +{ + u8 i, channels = esai_priv->channels[tx]; + u32 pins = DIV_ROUND_UP(channels, esai_priv->slots); + u32 mask; + + regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx), + ESAI_xFCR_xFEN_MASK, ESAI_xFCR_xFEN); + + /* Write initial words reqiured by ESAI as normal procedure */ + for (i = 0; tx && i < channels; i++) + regmap_write(esai_priv->regmap, REG_ESAI_ETDR, 0x0); + + /* +* When set the TE/RE in the end of enablement
[PATCH 05/10] ASoC: fsl_sai: Add support to enable multiple data lines
SAI supports up to 8 Rx/Tx data lines which can be enabled using TCE/RCE bits of TCR3/RCR3 registers. Data lines to be enabled are read from DT fsl,dl_mask property. By default (if no DT entry is provided) only data line 0 is enabled. Note: We can only enable consecutive data lines starting with data line #0. Signed-off-by: Daniel Baluta --- sound/soc/fsl/fsl_sai.c | 10 +- sound/soc/fsl/fsl_sai.h | 6 -- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c index 768341608695..d0fa02188b7c 100644 --- a/sound/soc/fsl/fsl_sai.c +++ b/sound/soc/fsl/fsl_sai.c @@ -601,7 +601,7 @@ static int fsl_sai_startup(struct snd_pcm_substream *substream, regmap_update_bits(sai->regmap, FSL_SAI_xCR3(tx), FSL_SAI_CR3_TRCE_MASK, - FSL_SAI_CR3_TRCE); + FSL_SAI_CR3_TRCE(sai->soc_data->dl_mask[tx]); ret = snd_pcm_hw_constraint_list(substream->runtime, 0, SNDRV_PCM_HW_PARAM_RATE, &fsl_sai_rate_constraints); @@ -887,6 +887,14 @@ static int fsl_sai_probe(struct platform_device *pdev) } } + /* active data lines mask for TX/RX, defaults to 1 (only the first +* data line is enabled +*/ + sai->dl_mask[RX] = 1; + sai->dl_mask[TX] = 1; + of_property_read_u32_index(np, "fsl,dl_mask", RX, &sai->dl_mask[RX]); + of_property_read_u32_index(np, "fsl,dl_mask", TX, &sai->dl_mask[TX]); + irq = platform_get_irq(pdev, 0); if (irq < 0) { dev_err(&pdev->dev, "no irq for node %s\n", pdev->name); diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h index b1abeed2f78e..6d32f0950ec5 100644 --- a/sound/soc/fsl/fsl_sai.h +++ b/sound/soc/fsl/fsl_sai.h @@ -109,8 +109,8 @@ #define FSL_SAI_CR2_DIV_MASK 0xff /* SAI Transmit and Receive Configuration 3 Register */ -#define FSL_SAI_CR3_TRCE BIT(16) -#define FSL_SAI_CR3_TRCE_MASK GENMASK(16, 23) +#define FSL_SAI_CR3_TRCE(x)((x) << 16) +#define FSL_SAI_CR3_TRCE_MASK GENMASK(23, 16) #define FSL_SAI_CR3_WDFL(x)(x) #define FSL_SAI_CR3_WDFL_MASK 0x1f @@ -176,6 +176,8 @@ struct fsl_sai { unsigned int slots; unsigned int slot_width; + unsigned int dl_mask[2]; + const struct fsl_sai_soc_data *soc_data; struct snd_dmaengine_dai_dma_data dma_params_rx; struct snd_dmaengine_dai_dma_data dma_params_tx; -- 2.17.1
[PATCH 06/10] ASoC: dt-bindings: Document dl_mask property
SAI supports up to 8 data lines. This property let the user configure how many data lines should be used per transfer direction (Tx/Rx). Signed-off-by: Daniel Baluta --- Documentation/devicetree/bindings/sound/fsl-sai.txt | 5 + 1 file changed, 5 insertions(+) diff --git a/Documentation/devicetree/bindings/sound/fsl-sai.txt b/Documentation/devicetree/bindings/sound/fsl-sai.txt index 2e726b983845..59f4d965a5fb 100644 --- a/Documentation/devicetree/bindings/sound/fsl-sai.txt +++ b/Documentation/devicetree/bindings/sound/fsl-sai.txt @@ -49,6 +49,11 @@ Optional properties: - big-endian : Boolean property, required if all the SAI registers are big-endian rather than little-endian. + - fsl,dl_mask: list of two integers (bitmask, first for RX, second + for TX) representing enabled datalines. Bit 0 + represents first data line, bit 1 represents second + data line and so on. Data line is enabled if + corresponding bit is set to 1. Optional properties (for mx6ul): -- 2.17.1
[PATCH 03/10] ASoC: fsl_sai: Add registers definition for multiple datalines
SAI IP supports up to 8 data lines. The configuration of supported number of data lines is decided at SoC integration time. This patch adds definitions for all related data TX/RX registers: * TDR0..7, Transmit data register * TFR0..7, Transmit FIFO register * RDR0..7, Receive data register * RFR0..7, Receive FIFO register Signed-off-by: Daniel Baluta --- sound/soc/fsl/fsl_sai.c | 76 +++-- sound/soc/fsl/fsl_sai.h | 36 --- 2 files changed, 98 insertions(+), 14 deletions(-) diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c index 1d1a447163e3..7f8823fe4b90 100644 --- a/sound/soc/fsl/fsl_sai.c +++ b/sound/soc/fsl/fsl_sai.c @@ -685,7 +685,14 @@ static struct reg_default fsl_sai_reg_defaults[] = { {FSL_SAI_TCR3, 0}, {FSL_SAI_TCR4, 0}, {FSL_SAI_TCR5, 0}, - {FSL_SAI_TDR, 0}, + {FSL_SAI_TDR0, 0}, + {FSL_SAI_TDR1, 0}, + {FSL_SAI_TDR2, 0}, + {FSL_SAI_TDR3, 0}, + {FSL_SAI_TDR4, 0}, + {FSL_SAI_TDR5, 0}, + {FSL_SAI_TDR6, 0}, + {FSL_SAI_TDR7, 0}, {FSL_SAI_TMR, 0}, {FSL_SAI_RCR1, 0}, {FSL_SAI_RCR2, 0}, @@ -704,7 +711,14 @@ static bool fsl_sai_readable_reg(struct device *dev, unsigned int reg) case FSL_SAI_TCR3: case FSL_SAI_TCR4: case FSL_SAI_TCR5: - case FSL_SAI_TFR: + case FSL_SAI_TFR0: + case FSL_SAI_TFR1: + case FSL_SAI_TFR2: + case FSL_SAI_TFR3: + case FSL_SAI_TFR4: + case FSL_SAI_TFR5: + case FSL_SAI_TFR6: + case FSL_SAI_TFR7: case FSL_SAI_TMR: case FSL_SAI_RCSR: case FSL_SAI_RCR1: @@ -712,8 +726,22 @@ static bool fsl_sai_readable_reg(struct device *dev, unsigned int reg) case FSL_SAI_RCR3: case FSL_SAI_RCR4: case FSL_SAI_RCR5: - case FSL_SAI_RDR: - case FSL_SAI_RFR: + case FSL_SAI_RDR0: + case FSL_SAI_RDR1: + case FSL_SAI_RDR2: + case FSL_SAI_RDR3: + case FSL_SAI_RDR4: + case FSL_SAI_RDR5: + case FSL_SAI_RDR6: + case FSL_SAI_RDR7: + case FSL_SAI_RFR0: + case FSL_SAI_RFR1: + case FSL_SAI_RFR2: + case FSL_SAI_RFR3: + case FSL_SAI_RFR4: + case FSL_SAI_RFR5: + case FSL_SAI_RFR6: + case FSL_SAI_RFR7: case FSL_SAI_RMR: return true; default: @@ -726,9 +754,30 @@ static bool fsl_sai_volatile_reg(struct device *dev, unsigned int reg) switch (reg) { case FSL_SAI_TCSR: case FSL_SAI_RCSR: - case FSL_SAI_TFR: - case FSL_SAI_RFR: - case FSL_SAI_RDR: + case FSL_SAI_TFR0: + case FSL_SAI_TFR1: + case FSL_SAI_TFR2: + case FSL_SAI_TFR3: + case FSL_SAI_TFR4: + case FSL_SAI_TFR5: + case FSL_SAI_TFR6: + case FSL_SAI_TFR7: + case FSL_SAI_RFR0: + case FSL_SAI_RFR1: + case FSL_SAI_RFR2: + case FSL_SAI_RFR3: + case FSL_SAI_RFR4: + case FSL_SAI_RFR5: + case FSL_SAI_RFR6: + case FSL_SAI_RFR7: + case FSL_SAI_RDR0: + case FSL_SAI_RDR1: + case FSL_SAI_RDR2: + case FSL_SAI_RDR3: + case FSL_SAI_RDR4: + case FSL_SAI_RDR5: + case FSL_SAI_RDR6: + case FSL_SAI_RDR7: return true; default: return false; @@ -744,7 +793,14 @@ static bool fsl_sai_writeable_reg(struct device *dev, unsigned int reg) case FSL_SAI_TCR3: case FSL_SAI_TCR4: case FSL_SAI_TCR5: - case FSL_SAI_TDR: + case FSL_SAI_TDR0: + case FSL_SAI_TDR1: + case FSL_SAI_TDR2: + case FSL_SAI_TDR3: + case FSL_SAI_TDR4: + case FSL_SAI_TDR5: + case FSL_SAI_TDR6: + case FSL_SAI_TDR7: case FSL_SAI_TMR: case FSL_SAI_RCSR: case FSL_SAI_RCR1: @@ -884,8 +940,8 @@ static int fsl_sai_probe(struct platform_device *pdev) MCLK_DIR(index)); } - sai->dma_params_rx.addr = res->start + FSL_SAI_RDR; - sai->dma_params_tx.addr = res->start + FSL_SAI_TDR; + sai->dma_params_rx.addr = res->start + FSL_SAI_RDR0; + sai->dma_params_tx.addr = res->start + FSL_SAI_TDR0; sai->dma_params_rx.maxburst = FSL_SAI_MAXBURST_RX; sai->dma_params_tx.maxburst = FSL_SAI_MAXBURST_TX; diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h index 7c1ef671da28..4bb478041d67 100644 --- a/sound/soc/fsl/fsl_sai.h +++ b/sound/soc/fsl/fsl_sai.h @@ -20,8 +20,22 @@ #define FSL_SAI_TCR3 0x0c /* SAI Transmit Configuration 3 */ #define FSL_SAI_TCR4 0x10 /* SAI Transmit Configuration 4 */ #define FSL_SAI_TCR5 0x14 /* SAI Transmit Configuration 5 */ -#define FSL_SAI_TDR0x20 /* SAI Transmit Data */ -#define FSL_SAI_TFR0x40 /* SAI Transmit FIFO */ +#define FSL_SAI_TDR0 0x20 /* SAI Transmit Data 0 */ +#define FSL_SAI_TDR1 0x24 /* SAI Transmit Data 1 */ +#d
[PATCH 00/10] Add support for new SAI IP version
So far SAI IPs integrated with imx6 only supported one data line. Starting with imx7 and imx8 SAI integration support up to 8 data lines and multiple ways of combining the fifos for each data line. New SAI IP version introduces two new registers (Version and Parmeter registers) which are placed at the beginning of register address space. For this reason we need to fix the register's address. Patches 1 and 2 from Lucas enhance per SOC handling of SAI properties. Patches 3,4,5,6,7,8 allow SAI driver to read active data lines and fifo combine mode from DT. Patch 9 fixes new SAI register address space. Patch 10 enable SAI for imx7ulp and imx8mq. This patch introduces Daniel Baluta (8): ASoC: fsl_sai: Add registers definition for multiple datalines ASoC: fsl_sai: Update Tx/Rx channel enable mask ASoC: fsl_sai: Add support to enable multiple data lines ASoC: dt-bindings: Document dl_mask property ASoC: fsl_sai: Add support for FIFO combine mode ASoC: dt-bindings: Document fcomb_mode property ASoC: fsl_sai: Add support for SAI new version ASoC: fsl_sai: Add support for imx7ulp/imx8mq Lucas Stach (2): ASoC: fsl_sai: add of_match data ASoC: fsl_sai: derive TX FIFO watermark from FIFO depth .../devicetree/bindings/sound/fsl-sai.txt | 9 + sound/soc/fsl/fsl_sai.c | 393 +- sound/soc/fsl/fsl_sai.h | 98 +++-- 3 files changed, 361 insertions(+), 139 deletions(-) -- 2.17.1
[PATCH 08/10] ASoC: dt-bindings: Document fcomb_mode property
This allows combining multiple-data-line FIFOs into a single-data-line FIFO. Signed-off-by: Daniel Baluta --- Documentation/devicetree/bindings/sound/fsl-sai.txt | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/devicetree/bindings/sound/fsl-sai.txt b/Documentation/devicetree/bindings/sound/fsl-sai.txt index 59f4d965a5fb..ca27afd840ba 100644 --- a/Documentation/devicetree/bindings/sound/fsl-sai.txt +++ b/Documentation/devicetree/bindings/sound/fsl-sai.txt @@ -54,6 +54,10 @@ Optional properties: represents first data line, bit 1 represents second data line and so on. Data line is enabled if corresponding bit is set to 1. + - fsl,fcomb_mode : list of two integers (first for RX, second for TX) + representing FIFO combine mode. Possible values for + combined mode are: 0 - disabled, 1 - Rx/Tx from shift + registers, 2 - Rx/Tx by software, 3 - both. Optional properties (for mx6ul): -- 2.17.1
[PATCH 02/10] ASoC: fsl_sai: derive TX FIFO watermark from FIFO depth
From: Lucas Stach The DMA request schould be triggered as soon as the FIFO has space for another burst. As different versions of the SAI block have different FIFO sizes, the watrmark level needs to be derived from version specific data. Signed-off-by: Lucas Stach --- sound/soc/fsl/fsl_sai.c | 4 +++- sound/soc/fsl/fsl_sai.h | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c index ed0432e7327a..1d1a447163e3 100644 --- a/sound/soc/fsl/fsl_sai.c +++ b/sound/soc/fsl/fsl_sai.c @@ -640,7 +640,7 @@ static int fsl_sai_dai_probe(struct snd_soc_dai *cpu_dai) regmap_write(sai->regmap, FSL_SAI_RCSR, 0); regmap_update_bits(sai->regmap, FSL_SAI_TCR1, FSL_SAI_CR1_RFW_MASK, - FSL_SAI_MAXBURST_TX * 2); + sai->soc_data->fifo_depth - FSL_SAI_MAXBURST_TX); regmap_update_bits(sai->regmap, FSL_SAI_RCR1, FSL_SAI_CR1_RFW_MASK, FSL_SAI_MAXBURST_RX - 1); @@ -913,10 +913,12 @@ static int fsl_sai_remove(struct platform_device *pdev) static const struct fsl_sai_soc_data fsl_sai_vf610_data = { .use_imx_pcm = false, + .fifo_depth = 32, }; static const struct fsl_sai_soc_data fsl_sai_imx6sx_data = { .use_imx_pcm = true, + .fifo_depth = 32, }; static const struct of_device_id fsl_sai_ids[] = { diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h index 83e2bfe05b1b..7c1ef671da28 100644 --- a/sound/soc/fsl/fsl_sai.h +++ b/sound/soc/fsl/fsl_sai.h @@ -128,6 +128,7 @@ struct fsl_sai_soc_data { bool use_imx_pcm; + unsigned int fifo_depth; }; struct fsl_sai { -- 2.17.1
[PATCH 10/10] ASoC: fsl_sai: Add support for imx7ulp/imx8mq
SAI module on imx7ulp/imx8m features 2 new registers (VERID and PARAM) at the beginning of register address space. On imx7ulp FIFOs can held up to 16 x 32 bit samples. On imx8mq FIFOs can held up to 128 x 32 bit samples. Signed-off-by: Daniel Baluta --- sound/soc/fsl/fsl_sai.c | 14 ++ 1 file changed, 14 insertions(+) diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c index f2441b84877e..b05837465b5a 100644 --- a/sound/soc/fsl/fsl_sai.c +++ b/sound/soc/fsl/fsl_sai.c @@ -1065,10 +1065,24 @@ static const struct fsl_sai_soc_data fsl_sai_imx6sx_data = { .reg_offset = 0, }; +static const struct fsl_sai_soc_data fsl_sai_imx7ulp_data = { + .use_imx_pcm = true, + .fifo_depth = 16, + .reg_offset = 8, +}; + +static const struct fsl_sai_soc_data fsl_sai_imx8mq_data = { + .use_imx_pcm = true, + .fifo_depth = 128, + .reg_offset = 8, +}; + static const struct of_device_id fsl_sai_ids[] = { { .compatible = "fsl,vf610-sai", .data = &fsl_sai_vf610_data }, { .compatible = "fsl,imx6sx-sai", .data = &fsl_sai_imx6sx_data }, { .compatible = "fsl,imx6ul-sai", .data = &fsl_sai_imx6sx_data }, + { .compatible = "fsl,imx7ulp-sai", .data = &fsl_sai_imx7ulp_data }, + { .compatible = "fsl,imx8mq-sai", .data = &fsl_sai_imx8mq_data }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, fsl_sai_ids); -- 2.17.1
[PATCH 01/10] ASoC: fsl_sai: add of_match data
From: Lucas Stach New revisions of the SAI IP block have even more differences that need be taken into account by the driver. To avoid sprinking compatible checks all over the driver move the current differences into of_match_data. Signed-off-by: Lucas Stach --- sound/soc/fsl/fsl_sai.c | 22 ++ sound/soc/fsl/fsl_sai.h | 6 +- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c index d58cc3ae90d8..ed0432e7327a 100644 --- a/sound/soc/fsl/fsl_sai.c +++ b/sound/soc/fsl/fsl_sai.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -788,10 +789,7 @@ static int fsl_sai_probe(struct platform_device *pdev) return -ENOMEM; sai->pdev = pdev; - - if (of_device_is_compatible(np, "fsl,imx6sx-sai") || - of_device_is_compatible(np, "fsl,imx6ul-sai")) - sai->sai_on_imx = true; + sai->soc_data = of_device_get_match_data(&pdev->dev); sai->is_lsb_first = of_property_read_bool(np, "lsb-first"); @@ -900,7 +898,7 @@ static int fsl_sai_probe(struct platform_device *pdev) if (ret) return ret; - if (sai->sai_on_imx) + if (sai->soc_data->use_imx_pcm) return imx_pcm_dma_init(pdev, IMX_SAI_DMABUF_SIZE); else return devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0); @@ -913,10 +911,18 @@ static int fsl_sai_remove(struct platform_device *pdev) return 0; } +static const struct fsl_sai_soc_data fsl_sai_vf610_data = { + .use_imx_pcm = false, +}; + +static const struct fsl_sai_soc_data fsl_sai_imx6sx_data = { + .use_imx_pcm = true, +}; + static const struct of_device_id fsl_sai_ids[] = { - { .compatible = "fsl,vf610-sai", }, - { .compatible = "fsl,imx6sx-sai", }, - { .compatible = "fsl,imx6ul-sai", }, + { .compatible = "fsl,vf610-sai", .data = &fsl_sai_vf610_data }, + { .compatible = "fsl,imx6sx-sai", .data = &fsl_sai_imx6sx_data }, + { .compatible = "fsl,imx6ul-sai", .data = &fsl_sai_imx6sx_data }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, fsl_sai_ids); diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h index 24cb156bf995..83e2bfe05b1b 100644 --- a/sound/soc/fsl/fsl_sai.h +++ b/sound/soc/fsl/fsl_sai.h @@ -126,6 +126,10 @@ #define FSL_SAI_MAXBURST_TX 6 #define FSL_SAI_MAXBURST_RX 6 +struct fsl_sai_soc_data { + bool use_imx_pcm; +}; + struct fsl_sai { struct platform_device *pdev; struct regmap *regmap; @@ -135,7 +139,6 @@ struct fsl_sai { bool is_slave_mode; bool is_lsb_first; bool is_dsp_mode; - bool sai_on_imx; bool synchronous[2]; unsigned int mclk_id[2]; @@ -143,6 +146,7 @@ struct fsl_sai { unsigned int slots; unsigned int slot_width; + const struct fsl_sai_soc_data *soc_data; struct snd_dmaengine_dai_dma_data dma_params_rx; struct snd_dmaengine_dai_dma_data dma_params_tx; }; -- 2.17.1
[PATCH 07/10] ASoC: fsl_sai: Add support for FIFO combine mode
FIFO combining mode allows the separate FIFOs for multiple data channels to be used as a single FIFO for either software accesses or a single data channel or both. FIFO combined mode is described in chapter 13.10.3.5.4 from i.MX8MQ reference manual [1]. For each direction (RX/TX) fifo combine mode is read from fsl,fcomb-mode DT property. By default, if no property is specified fifo combine mode is disabled. [1]https://cache.nxp.com/secured/assets/documents/en/reference-manual/IMX8MDQLQRM.pdf?__gda__=1563728701_38bea7f0f726472cc675cb141b91bec7&fileExt=.pdf Signed-off-by: Daniel Baluta --- sound/soc/fsl/fsl_sai.c | 37 + sound/soc/fsl/fsl_sai.h | 9 + 2 files changed, 46 insertions(+) diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c index d0fa02188b7c..140014901fce 100644 --- a/sound/soc/fsl/fsl_sai.c +++ b/sound/soc/fsl/fsl_sai.c @@ -475,6 +475,35 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream, } } + switch (sai->soc_data->fcomb_mode[tx]) { + case FSL_SAI_FCOMB_NONE: + regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx), + FSL_SAI_CR4_FCOMB_SOFT | + FSL_SAI_CR4_FCOMB_SHIFT, 0); + break; + case FSL_SAI_FCOMB_SHIFT: + regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx), + FSL_SAI_CR4_FCOMB_SOFT | + FSL_SAI_CR4_FCOMB_SHIFT, + FSL_SAI_CR4_FCOMB_SHIFT); + break; + case FSL_SAI_FCOMB_SOFT: + regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx), + FSL_SAI_CR4_FCOMB_SOFT | + FSL_SAI_CR4_FCOMB_SHIFT, + FSL_SAI_CR4_FCOMB_SOFT); + break; + case FSL_SAI_FCOMB_BOTH: + regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx), + FSL_SAI_CR4_FCOMB_SOFT | + FSL_SAI_CR4_FCOMB_SHIFT, + FSL_SAI_CR4_FCOMB_SOFT | + FSL_SAI_CR4_FCOMB_SHIFT); + break; + default: + break; + } + regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx), FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK, val_cr4); @@ -887,6 +916,14 @@ static int fsl_sai_probe(struct platform_device *pdev) } } + /* FIFO combine mode for TX/RX, defaults to disabled */ + sai->fcomb_mode[RX] = FSL_SAI_FCOMB_NONE; + sai->fcomb_mode[TX] = FSL_SAI_FCOMB_NONE; + of_property_read_u32_index(np, "fsl,fcomb-mode", RX, + &sai->fcomb_mode[RX]); + of_property_read_u32_index(np, "fsl,fcomb-mode", TX, + &sai->fcomb_mode[TX]); + /* active data lines mask for TX/RX, defaults to 1 (only the first * data line is enabled */ diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h index 6d32f0950ec5..abf140951187 100644 --- a/sound/soc/fsl/fsl_sai.h +++ b/sound/soc/fsl/fsl_sai.h @@ -115,6 +115,8 @@ #define FSL_SAI_CR3_WDFL_MASK 0x1f /* SAI Transmit and Receive Configuration 4 Register */ +#define FSL_SAI_CR4_FCOMB_SHIFT BIT(26) +#define FSL_SAI_CR4_FCOMB_SOFT BIT(27) #define FSL_SAI_CR4_FRSZ(x)(((x) - 1) << 16) #define FSL_SAI_CR4_FRSZ_MASK (0x1f << 16) #define FSL_SAI_CR4_SYWD(x)(((x) - 1) << 8) @@ -155,6 +157,12 @@ #define FSL_SAI_MAXBURST_TX 6 #define FSL_SAI_MAXBURST_RX 6 +/* FIFO combine modes */ +#define FSL_SAI_FCOMB_NONE 0 +#define FSL_SAI_FCOMB_SHIFT1 +#define FSL_SAI_FCOMB_SOFT 2 +#define FSL_SAI_FCOMB_BOTH 3 + struct fsl_sai_soc_data { bool use_imx_pcm; unsigned int fifo_depth; @@ -177,6 +185,7 @@ struct fsl_sai { unsigned int slot_width; unsigned int dl_mask[2]; + unsigned int fcomb_mode[2]; const struct fsl_sai_soc_data *soc_data; struct snd_dmaengine_dai_dma_data dma_params_rx; -- 2.17.1
[PATCH 04/10] ASoC: fsl_sai: Update Tx/Rx channel enable mask
Tx channel enable (TCE) / Rx channel enable (RCE) bits enable corresponding data channel for Tx/Rx operation. Because SAI supports up the 8 channels TCE/RCE occupy up the 8 bits inside TCR3/RCR3 registers we need to extend the mask to reflect this. Signed-off-by: Daniel Baluta --- sound/soc/fsl/fsl_sai.c | 6 -- sound/soc/fsl/fsl_sai.h | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c index 7f8823fe4b90..768341608695 100644 --- a/sound/soc/fsl/fsl_sai.c +++ b/sound/soc/fsl/fsl_sai.c @@ -599,7 +599,8 @@ static int fsl_sai_startup(struct snd_pcm_substream *substream, bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK; int ret; - regmap_update_bits(sai->regmap, FSL_SAI_xCR3(tx), FSL_SAI_CR3_TRCE, + regmap_update_bits(sai->regmap, FSL_SAI_xCR3(tx), + FSL_SAI_CR3_TRCE_MASK, FSL_SAI_CR3_TRCE); ret = snd_pcm_hw_constraint_list(substream->runtime, 0, @@ -614,7 +615,8 @@ static void fsl_sai_shutdown(struct snd_pcm_substream *substream, struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai); bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK; - regmap_update_bits(sai->regmap, FSL_SAI_xCR3(tx), FSL_SAI_CR3_TRCE, 0); + regmap_update_bits(sai->regmap, FSL_SAI_xCR3(tx), + FSL_SAI_CR3_TRCE_MASK, 0); } static const struct snd_soc_dai_ops fsl_sai_pcm_dai_ops = { diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h index 4bb478041d67..b1abeed2f78e 100644 --- a/sound/soc/fsl/fsl_sai.h +++ b/sound/soc/fsl/fsl_sai.h @@ -110,6 +110,7 @@ /* SAI Transmit and Receive Configuration 3 Register */ #define FSL_SAI_CR3_TRCE BIT(16) +#define FSL_SAI_CR3_TRCE_MASK GENMASK(16, 23) #define FSL_SAI_CR3_WDFL(x)(x) #define FSL_SAI_CR3_WDFL_MASK 0x1f -- 2.17.1
[PATCH 09/10] ASoC: fsl_sai: Add support for SAI new version
New IP version introduces Version ID and Parameter registers and optionally added Timestamp feature. VERID and PARAM registers are placed at the top of registers address space and some registers are shifted according to the following table: Tx/Rx data registers and Tx/Rx FIFO registers keep their addresses, all other registers are shifted by 8. SAI Memory map is described in chapter 13.10.4.1.1 I2S Memory map of the Reference Manual [1]. In order to make as less changes as possible we attach an offset to each register offset to each changed register definition. The offset is read from each board private data. [1]https://cache.nxp.com/secured/assets/documents/en/reference-manual/IMX8MDQLQRM.pdf?__gda__=1563728701_38bea7f0f726472cc675cb141b91bec7&fileExt=.pdf Signed-off-by: Daniel Baluta --- sound/soc/fsl/fsl_sai.c | 240 +++- sound/soc/fsl/fsl_sai.h | 41 +++ 2 files changed, 162 insertions(+), 119 deletions(-) diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c index 140014901fce..f2441b84877e 100644 --- a/sound/soc/fsl/fsl_sai.c +++ b/sound/soc/fsl/fsl_sai.c @@ -40,6 +40,7 @@ static const struct snd_pcm_hw_constraint_list fsl_sai_rate_constraints = { static irqreturn_t fsl_sai_isr(int irq, void *devid) { struct fsl_sai *sai = (struct fsl_sai *)devid; + unsigned int ofs = sai->soc_data->reg_offset; struct device *dev = &sai->pdev->dev; u32 flags, xcsr, mask; bool irq_none = true; @@ -52,7 +53,7 @@ static irqreturn_t fsl_sai_isr(int irq, void *devid) mask = (FSL_SAI_FLAGS >> FSL_SAI_CSR_xIE_SHIFT) << FSL_SAI_CSR_xF_SHIFT; /* Tx IRQ */ - regmap_read(sai->regmap, FSL_SAI_TCSR, &xcsr); + regmap_read(sai->regmap, FSL_SAI_TCSR(ofs), &xcsr); flags = xcsr & mask; if (flags) @@ -82,11 +83,11 @@ static irqreturn_t fsl_sai_isr(int irq, void *devid) xcsr &= ~FSL_SAI_CSR_xF_MASK; if (flags) - regmap_write(sai->regmap, FSL_SAI_TCSR, flags | xcsr); + regmap_write(sai->regmap, FSL_SAI_TCSR(ofs), flags | xcsr); irq_rx: /* Rx IRQ */ - regmap_read(sai->regmap, FSL_SAI_RCSR, &xcsr); + regmap_read(sai->regmap, FSL_SAI_RCSR(ofs), &xcsr); flags = xcsr & mask; if (flags) @@ -116,7 +117,7 @@ static irqreturn_t fsl_sai_isr(int irq, void *devid) xcsr &= ~FSL_SAI_CSR_xF_MASK; if (flags) - regmap_write(sai->regmap, FSL_SAI_RCSR, flags | xcsr); + regmap_write(sai->regmap, FSL_SAI_RCSR(ofs), flags | xcsr); out: if (irq_none) @@ -140,6 +141,7 @@ static int fsl_sai_set_dai_sysclk_tr(struct snd_soc_dai *cpu_dai, int clk_id, unsigned int freq, int fsl_dir) { struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai); + unsigned int ofs = sai->soc_data->reg_offset; bool tx = fsl_dir == FSL_FMT_TRANSMITTER; u32 val_cr2 = 0; @@ -160,7 +162,7 @@ static int fsl_sai_set_dai_sysclk_tr(struct snd_soc_dai *cpu_dai, return -EINVAL; } - regmap_update_bits(sai->regmap, FSL_SAI_xCR2(tx), + regmap_update_bits(sai->regmap, FSL_SAI_xCR2(tx, ofs), FSL_SAI_CR2_MSEL_MASK, val_cr2); return 0; @@ -193,6 +195,7 @@ static int fsl_sai_set_dai_fmt_tr(struct snd_soc_dai *cpu_dai, unsigned int fmt, int fsl_dir) { struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai); + unsigned int ofs = sai->soc_data->reg_offset; bool tx = fsl_dir == FSL_FMT_TRANSMITTER; u32 val_cr2 = 0, val_cr4 = 0; @@ -287,9 +290,9 @@ static int fsl_sai_set_dai_fmt_tr(struct snd_soc_dai *cpu_dai, return -EINVAL; } - regmap_update_bits(sai->regmap, FSL_SAI_xCR2(tx), + regmap_update_bits(sai->regmap, FSL_SAI_xCR2(tx, ofs), FSL_SAI_CR2_BCP | FSL_SAI_CR2_BCD_MSTR, val_cr2); - regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx), + regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx, ofs), FSL_SAI_CR4_MF | FSL_SAI_CR4_FSE | FSL_SAI_CR4_FSP | FSL_SAI_CR4_FSD_MSTR, val_cr4); @@ -316,6 +319,7 @@ static int fsl_sai_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt) static int fsl_sai_set_bclk(struct snd_soc_dai *dai, bool tx, u32 freq) { struct fsl_sai *sai = snd_soc_dai_get_drvdata(dai); + unsigned int ofs = sai->soc_data->reg_offset; unsigned long clk_rate; u32 savediv = 0, ratio, savesub = freq; u32 id; @@ -378,17 +382,17 @@ static int fsl_sai_set_bclk(struct snd_soc_dai *dai, bool tx, u32 freq) */ if ((sai->synchronous[TX] && !sai->synchronous[RX]) || (!tx && !sai->synchronous[RX])) { - regmap_update_bits(sai->regmap, FSL_SAI_RCR2, + regmap_update_bits(sai->regmap, FSL_SAI_RCR2(ofs),
Re: [PATCH 07/10] ASoC: fsl_sai: Add support for FIFO combine mode
Am Montag, den 22.07.2019, 15:48 +0300 schrieb Daniel Baluta: > FIFO combining mode allows the separate FIFOs for multiple data > channels > to be used as a single FIFO for either software accesses or a single > data > channel or both. > > FIFO combined mode is described in chapter 13.10.3.5.4 from i.MX8MQ > reference manual [1]. > > For each direction (RX/TX) fifo combine mode is read from fsl,fcomb- > mode > DT property. By default, if no property is specified fifo combine > mode > is disabled. > > [1]https://cache.nxp.com/secured/assets/documents/en/reference-manual > /IMX8MDQLQRM.pdf?__gda__=1563728701_38bea7f0f726472cc675cb141b91bec7& > fileExt=.pdf > > Signed-off-by: Daniel Baluta > --- > sound/soc/fsl/fsl_sai.c | 37 + > sound/soc/fsl/fsl_sai.h | 9 + > 2 files changed, 46 insertions(+) > > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c > index d0fa02188b7c..140014901fce 100644 > --- a/sound/soc/fsl/fsl_sai.c > +++ b/sound/soc/fsl/fsl_sai.c > @@ -475,6 +475,35 @@ static int fsl_sai_hw_params(struct > snd_pcm_substream *substream, > } > } > > + switch (sai->soc_data->fcomb_mode[tx]) { > + case FSL_SAI_FCOMB_NONE: > + regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx), > + FSL_SAI_CR4_FCOMB_SOFT | > + FSL_SAI_CR4_FCOMB_SHIFT, 0); > + break; > + case FSL_SAI_FCOMB_SHIFT: > + regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx), > + FSL_SAI_CR4_FCOMB_SOFT | > + FSL_SAI_CR4_FCOMB_SHIFT, > + FSL_SAI_CR4_FCOMB_SHIFT); > + break; > + case FSL_SAI_FCOMB_SOFT: > + regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx), > + FSL_SAI_CR4_FCOMB_SOFT | > + FSL_SAI_CR4_FCOMB_SHIFT, > + FSL_SAI_CR4_FCOMB_SOFT); > + break; > + case FSL_SAI_FCOMB_BOTH: > + regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx), > + FSL_SAI_CR4_FCOMB_SOFT | > + FSL_SAI_CR4_FCOMB_SHIFT, > + FSL_SAI_CR4_FCOMB_SOFT | > + FSL_SAI_CR4_FCOMB_SHIFT); > + break; > + default: > + break; > + } This would probably look less redundant if you only select the bits to set in the switch statement and move the regmap_update_bits after the switch. Regards, Lucas > regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx), > FSL_SAI_CR4_SYWD_MASK | > FSL_SAI_CR4_FRSZ_MASK, > val_cr4); > @@ -887,6 +916,14 @@ static int fsl_sai_probe(struct platform_device > *pdev) > } > } > > + /* FIFO combine mode for TX/RX, defaults to disabled */ > + sai->fcomb_mode[RX] = FSL_SAI_FCOMB_NONE; > + sai->fcomb_mode[TX] = FSL_SAI_FCOMB_NONE; > + of_property_read_u32_index(np, "fsl,fcomb-mode", RX, > + &sai->fcomb_mode[RX]); > + of_property_read_u32_index(np, "fsl,fcomb-mode", TX, > + &sai->fcomb_mode[TX]); > + > /* active data lines mask for TX/RX, defaults to 1 (only the > first > * data line is enabled > */ > diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h > index 6d32f0950ec5..abf140951187 100644 > --- a/sound/soc/fsl/fsl_sai.h > +++ b/sound/soc/fsl/fsl_sai.h > @@ -115,6 +115,8 @@ > #define FSL_SAI_CR3_WDFL_MASK0x1f > > /* SAI Transmit and Receive Configuration 4 Register */ > +#define FSL_SAI_CR4_FCOMB_SHIFT BIT(26) > +#define FSL_SAI_CR4_FCOMB_SOFT BIT(27) > #define FSL_SAI_CR4_FRSZ(x) (((x) - 1) << 16) > #define FSL_SAI_CR4_FRSZ_MASK(0x1f << 16) > #define FSL_SAI_CR4_SYWD(x) (((x) - 1) << 8) > @@ -155,6 +157,12 @@ > #define FSL_SAI_MAXBURST_TX 6 > #define FSL_SAI_MAXBURST_RX 6 > > +/* FIFO combine modes */ > +#define FSL_SAI_FCOMB_NONE 0 > +#define FSL_SAI_FCOMB_SHIFT1 > +#define FSL_SAI_FCOMB_SOFT 2 > +#define FSL_SAI_FCOMB_BOTH 3 > + > struct fsl_sai_soc_data { > bool use_imx_pcm; > unsigned int fifo_depth; > @@ -177,6 +185,7 @@ struct fsl_sai { > unsigned int slot_width; > > unsigned int dl_mask[2]; > + unsigned int fcomb_mode[2]; > > const struct fsl_sai_soc_data *soc_data; > struct snd_dmaengine_dai_dma_data dma_params_rx;
Re: [PATCH 10/10] ASoC: fsl_sai: Add support for imx7ulp/imx8mq
Am Montag, den 22.07.2019, 15:48 +0300 schrieb Daniel Baluta: > SAI module on imx7ulp/imx8m features 2 new registers (VERID and PARAM) > at the beginning of register address space. > > On imx7ulp FIFOs can held up to 16 x 32 bit samples. > On imx8mq FIFOs can held up to 128 x 32 bit samples. > > > Signed-off-by: Daniel Baluta > --- > sound/soc/fsl/fsl_sai.c | 14 ++ > 1 file changed, 14 insertions(+) > > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c > index f2441b84877e..b05837465b5a 100644 > --- a/sound/soc/fsl/fsl_sai.c > +++ b/sound/soc/fsl/fsl_sai.c > @@ -1065,10 +1065,24 @@ static const struct fsl_sai_soc_data > fsl_sai_imx6sx_data = { > > .reg_offset = 0, > }; > > +static const struct fsl_sai_soc_data fsl_sai_imx7ulp_data = { > > + .use_imx_pcm = true, > > + .fifo_depth = 16, > > + .reg_offset = 8, > +}; > + > +static const struct fsl_sai_soc_data fsl_sai_imx8mq_data = { > > + .use_imx_pcm = true, > > + .fifo_depth = 128, > > + .reg_offset = 8, > +}; > + > static const struct of_device_id fsl_sai_ids[] = { > > { .compatible = "fsl,vf610-sai", .data = &fsl_sai_vf610_data }, > > { .compatible = "fsl,imx6sx-sai", .data = &fsl_sai_imx6sx_data }, > > { .compatible = "fsl,imx6ul-sai", .data = &fsl_sai_imx6sx_data }, > > + { .compatible = "fsl,imx7ulp-sai", .data = &fsl_sai_imx7ulp_data }, > > + { .compatible = "fsl,imx8mq-sai", .data = &fsl_sai_imx8mq_data }, > > Those two new compatibles need to be documented in the DT bindings. Regards, Lucas
Re: [PATCH 05/10] ASoC: fsl_sai: Add support to enable multiple data lines
Am Montag, den 22.07.2019, 15:48 +0300 schrieb Daniel Baluta: > SAI supports up to 8 Rx/Tx data lines which can be enabled > using TCE/RCE bits of TCR3/RCR3 registers. > > Data lines to be enabled are read from DT fsl,dl_mask property. > By default (if no DT entry is provided) only data line 0 is enabled. > > Note: > We can only enable consecutive data lines starting with data line #0. Why is the property a bitmask then? To me this sounds like we want to have the number of lanes in the DT and convert to the bitmask inside the driver. > > Signed-off-by: Daniel Baluta > --- > sound/soc/fsl/fsl_sai.c | 10 +- > sound/soc/fsl/fsl_sai.h | 6 -- > 2 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c > index 768341608695..d0fa02188b7c 100644 > --- a/sound/soc/fsl/fsl_sai.c > +++ b/sound/soc/fsl/fsl_sai.c > @@ -601,7 +601,7 @@ static int fsl_sai_startup(struct snd_pcm_substream > *substream, > > > regmap_update_bits(sai->regmap, FSL_SAI_xCR3(tx), > > FSL_SAI_CR3_TRCE_MASK, > > - FSL_SAI_CR3_TRCE); > > + FSL_SAI_CR3_TRCE(sai->soc_data->dl_mask[tx]); > > > ret = snd_pcm_hw_constraint_list(substream->runtime, 0, > > SNDRV_PCM_HW_PARAM_RATE, &fsl_sai_rate_constraints); > @@ -887,6 +887,14 @@ static int fsl_sai_probe(struct platform_device *pdev) > > } > > } > > > + /* active data lines mask for TX/RX, defaults to 1 (only the first > > + * data line is enabled > + */ Comment style not in line with Linux coding style. Regards, Lucas > + sai->dl_mask[RX] = 1; > > + sai->dl_mask[TX] = 1; > > + of_property_read_u32_index(np, "fsl,dl_mask", RX, &sai->dl_mask[RX]); > > + of_property_read_u32_index(np, "fsl,dl_mask", TX, &sai->dl_mask[TX]); > + > > irq = platform_get_irq(pdev, 0); > > if (irq < 0) { > > dev_err(&pdev->dev, "no irq for node %s\n", pdev->name); > diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h > index b1abeed2f78e..6d32f0950ec5 100644 > --- a/sound/soc/fsl/fsl_sai.h > +++ b/sound/soc/fsl/fsl_sai.h > @@ -109,8 +109,8 @@ > > #define FSL_SAI_CR2_DIV_MASK 0xff > > /* SAI Transmit and Receive Configuration 3 Register */ > > -#define FSL_SAI_CR3_TRCE BIT(16) > > -#define FSL_SAI_CR3_TRCE_MASK GENMASK(16, 23) > > +#define FSL_SAI_CR3_TRCE(x)((x) << 16) > > +#define FSL_SAI_CR3_TRCE_MASK GENMASK(23, 16) > > #define FSL_SAI_CR3_WDFL(x)(x) > > #define FSL_SAI_CR3_WDFL_MASK 0x1f > > @@ -176,6 +176,8 @@ struct fsl_sai { > > unsigned int slots; > > unsigned int slot_width; > > > + unsigned int dl_mask[2]; > + > > const struct fsl_sai_soc_data *soc_data; > > struct snd_dmaengine_dai_dma_data dma_params_rx; > > struct snd_dmaengine_dai_dma_data dma_params_tx;
Re: [PATCH 06/10] ASoC: dt-bindings: Document dl_mask property
Am Montag, den 22.07.2019, 15:48 +0300 schrieb Daniel Baluta: > SAI supports up to 8 data lines. This property let the user > configure how many data lines should be used per transfer > direction (Tx/Rx). > > > Signed-off-by: Daniel Baluta > --- > Documentation/devicetree/bindings/sound/fsl-sai.txt | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/sound/fsl-sai.txt > b/Documentation/devicetree/bindings/sound/fsl-sai.txt > index 2e726b983845..59f4d965a5fb 100644 > --- a/Documentation/devicetree/bindings/sound/fsl-sai.txt > +++ b/Documentation/devicetree/bindings/sound/fsl-sai.txt > @@ -49,6 +49,11 @@ Optional properties: > > > - big-endian : Boolean property, required if all the SAI > > registers are big-endian rather than little-endian. > > + - fsl,dl_mask: list of two integers (bitmask, first for RX, > > second > > + for TX) representing enabled datalines. Bit 0 > > + represents first data line, bit 1 represents second > > + data line and so on. Data line is enabled if > > + corresponding bit is set to 1. No underscores in property names, please. Also this should document the default value used by the driver when the property is absent. Regards, Lucas
[PATCH] powerpc: Wire up clone3 syscall
Wire up the new clone3 syscall added in commit 7f192e3cd316 ("fork: add clone3"). This requires a ppc_clone3 wrapper, in order to save the non-volatile GPRs before calling into the generic syscall code. Otherwise we hit the BUG_ON in CHECK_FULL_REGS in copy_thread(). Lightly tested using Christian's test code on a Power8 LE VM. Signed-off-by: Michael Ellerman --- arch/powerpc/include/asm/unistd.h| 1 + arch/powerpc/kernel/entry_64.S | 5 + arch/powerpc/kernel/syscalls/syscall.tbl | 1 + 3 files changed, 7 insertions(+) diff --git a/arch/powerpc/include/asm/unistd.h b/arch/powerpc/include/asm/unistd.h index 68473c3c471c..b0720c7c3fcf 100644 --- a/arch/powerpc/include/asm/unistd.h +++ b/arch/powerpc/include/asm/unistd.h @@ -49,6 +49,7 @@ #define __ARCH_WANT_SYS_FORK #define __ARCH_WANT_SYS_VFORK #define __ARCH_WANT_SYS_CLONE +#define __ARCH_WANT_SYS_CLONE3 #endif /* __ASSEMBLY__ */ #endif /* _ASM_POWERPC_UNISTD_H_ */ diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S index d9105fcf4021..0a0b5310f54a 100644 --- a/arch/powerpc/kernel/entry_64.S +++ b/arch/powerpc/kernel/entry_64.S @@ -487,6 +487,11 @@ _GLOBAL(ppc_clone) bl sys_clone b .Lsyscall_exit +_GLOBAL(ppc_clone3) + bl save_nvgprs + bl sys_clone3 + b .Lsyscall_exit + _GLOBAL(ppc32_swapcontext) bl save_nvgprs bl compat_sys_swapcontext diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl index f2c3bda2d39f..6886ecb590d5 100644 --- a/arch/powerpc/kernel/syscalls/syscall.tbl +++ b/arch/powerpc/kernel/syscalls/syscall.tbl @@ -516,3 +516,4 @@ 432common fsmount sys_fsmount 433common fspick sys_fspick 434common pidfd_open sys_pidfd_open +435common clone3 ppc_clone3 -- 2.20.1
Re: [PATCH] powerpc: Wire up clone3 syscall
On Mon, Jul 22, 2019 at 11:22:31PM +1000, Michael Ellerman wrote: > Wire up the new clone3 syscall added in commit 7f192e3cd316 ("fork: > add clone3"). > > This requires a ppc_clone3 wrapper, in order to save the non-volatile > GPRs before calling into the generic syscall code. Otherwise we hit > the BUG_ON in CHECK_FULL_REGS in copy_thread(). > > Lightly tested using Christian's test code on a Power8 LE VM. > > Signed-off-by: Michael Ellerman Thank you, Michael! One comment below, otherwise: Acked-by: Christian Brauner > --- > arch/powerpc/include/asm/unistd.h| 1 + > arch/powerpc/kernel/entry_64.S | 5 + > arch/powerpc/kernel/syscalls/syscall.tbl | 1 + > 3 files changed, 7 insertions(+) > > diff --git a/arch/powerpc/include/asm/unistd.h > b/arch/powerpc/include/asm/unistd.h > index 68473c3c471c..b0720c7c3fcf 100644 > --- a/arch/powerpc/include/asm/unistd.h > +++ b/arch/powerpc/include/asm/unistd.h > @@ -49,6 +49,7 @@ > #define __ARCH_WANT_SYS_FORK > #define __ARCH_WANT_SYS_VFORK > #define __ARCH_WANT_SYS_CLONE > +#define __ARCH_WANT_SYS_CLONE3 > > #endif /* __ASSEMBLY__ */ > #endif /* _ASM_POWERPC_UNISTD_H_ */ > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S > index d9105fcf4021..0a0b5310f54a 100644 > --- a/arch/powerpc/kernel/entry_64.S > +++ b/arch/powerpc/kernel/entry_64.S > @@ -487,6 +487,11 @@ _GLOBAL(ppc_clone) > bl sys_clone > b .Lsyscall_exit > > +_GLOBAL(ppc_clone3) > + bl save_nvgprs > + bl sys_clone3 > + b .Lsyscall_exit > + > _GLOBAL(ppc32_swapcontext) > bl save_nvgprs > bl compat_sys_swapcontext > diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl > b/arch/powerpc/kernel/syscalls/syscall.tbl > index f2c3bda2d39f..6886ecb590d5 100644 > --- a/arch/powerpc/kernel/syscalls/syscall.tbl > +++ b/arch/powerpc/kernel/syscalls/syscall.tbl > @@ -516,3 +516,4 @@ > 432 common fsmount sys_fsmount > 433 common fspick sys_fspick > 434 common pidfd_open sys_pidfd_open > +435 common clone3 ppc_clone3 Note that in v5.3-rc1 there's now a comment that 435 is reserved in there. So this will likely cause a merge conflict. You might want to base your change off of v5.3-rc1 instead to avoid that. :) So basically: >From 10b2e4176d712e45c7cb22af4aed4ce09818785c Mon Sep 17 00:00:00 2001 From: Michael Ellerman Date: Mon, 22 Jul 2019 23:22:31 +1000 Subject: [PATCH] powerpc: Wire up clone3 syscall Wire up the new clone3 syscall added in commit 7f192e3cd316 ("fork: add clone3"). This requires a ppc_clone3 wrapper, in order to save the non-volatile GPRs before calling into the generic syscall code. Otherwise we hit the BUG_ON in CHECK_FULL_REGS in copy_thread(). Lightly tested using Christian's test code on a Power8 LE VM. Signed-off-by: Michael Ellerman Acked-by: Christian Brauner Link: https://lore.kernel.org/r/20190722132231.10169-1-...@ellerman.id.au --- arch/powerpc/include/asm/unistd.h| 1 + arch/powerpc/kernel/entry_64.S | 5 + arch/powerpc/kernel/syscalls/syscall.tbl | 2 +- 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/include/asm/unistd.h b/arch/powerpc/include/asm/unistd.h index 68473c3c471c..b0720c7c3fcf 100644 --- a/arch/powerpc/include/asm/unistd.h +++ b/arch/powerpc/include/asm/unistd.h @@ -49,6 +49,7 @@ #define __ARCH_WANT_SYS_FORK #define __ARCH_WANT_SYS_VFORK #define __ARCH_WANT_SYS_CLONE +#define __ARCH_WANT_SYS_CLONE3 #endif /* __ASSEMBLY__ */ #endif /* _ASM_POWERPC_UNISTD_H_ */ diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S index d9105fcf4021..0a0b5310f54a 100644 --- a/arch/powerpc/kernel/entry_64.S +++ b/arch/powerpc/kernel/entry_64.S @@ -487,6 +487,11 @@ _GLOBAL(ppc_clone) bl sys_clone b .Lsyscall_exit +_GLOBAL(ppc_clone3) + bl save_nvgprs + bl sys_clone3 + b .Lsyscall_exit + _GLOBAL(ppc32_swapcontext) bl save_nvgprs bl compat_sys_swapcontext diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl index 3331749aab20..6886ecb590d5 100644 --- a/arch/powerpc/kernel/syscalls/syscall.tbl +++ b/arch/powerpc/kernel/syscalls/syscall.tbl @@ -516,4 +516,4 @@ 432common fsmount sys_fsmount 433common fspick sys_fspick 434common pidfd_open sys_pidfd_open -# 435 reserved for clone3 +435common clone3 ppc_clone3 -- 2.22.0
Re: [PATCH v2] powerpc: slightly improve cache helpers
On Mon, Jul 22, 2019 at 08:15:14PM +1000, Michael Ellerman wrote: > Segher Boessenkool writes: > > On Sun, Jul 21, 2019 at 12:58:46AM -0700, Nathan Chancellor wrote: > >> 017c clear_user_page: > >> 17c: 94 21 ff f0 stwu 1, -16(1) > >> 180: 38 80 00 80 li 4, 128 > >> 184: 38 63 ff e0 addi 3, 3, -32 > >> 188: 7c 89 03 a6 mtctr 4 > >> 18c: 38 81 00 0f addi 4, 1, 15 > >> 190: 8c c3 00 20 lbzu 6, 32(3) > >> 194: 98 c1 00 0f stb 6, 15(1) > >> 198: 7c 00 27 ec dcbz 0, 4 > >> 19c: 42 00 ff f4 bdnz .+65524 > > > > Uh, yeah, well, I have no idea what clang tried here, but that won't > > work. It's copying a byte from each target cache line to the stack, > > and then does clears the cache line containing that byte on the stack. > > So it seems like this is a clang bug. > > None of the distros we support use clang, but we would still like to > keep it working if we can. Which version? Which versions *are* broken? > Looking at the original patch, the only upside is that the compiler > can use both RA and RB to compute the address, rather than us forcing RA > to 0. > > But at least with my compiler here (GCC 8 vintage) I don't actually see > GCC ever using both GPRs even with the patch. Or at least, there's no > difference before/after the patch as far as I can see. The benefit is small, certainly. > So my inclination is to revert the original patch. We can try again in a > few years :D > > Thoughts? I think you should give the clang people time to figure out what is going on. Segher
[PATCH v9 05/21] powerpc: mm: Add p?d_leaf() definitions
walk_page_range() is going to be allowed to walk page tables other than those of user space. For this it needs to know when it has reached a 'leaf' entry in the page tables. This information is provided by the p?d_leaf() functions/macros. For powerpc pmd_large() already exists and does what we want, so hoist it out of the CONFIG_TRANSPARENT_HUGEPAGE condition and implement the other levels. Macros are used to provide the generic p?d_leaf() names. CC: Benjamin Herrenschmidt CC: Paul Mackerras CC: Michael Ellerman CC: linuxppc-dev@lists.ozlabs.org CC: kvm-...@vger.kernel.org Signed-off-by: Steven Price --- arch/powerpc/include/asm/book3s/64/pgtable.h | 30 ++-- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h index 8308f32e9782..84270666355c 100644 --- a/arch/powerpc/include/asm/book3s/64/pgtable.h +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h @@ -921,6 +921,12 @@ static inline int pud_present(pud_t pud) return !!(pud_raw(pud) & cpu_to_be64(_PAGE_PRESENT)); } +#define pud_leaf pud_large +static inline int pud_large(pud_t pud) +{ + return !!(pud_raw(pud) & cpu_to_be64(_PAGE_PTE)); +} + extern struct page *pud_page(pud_t pud); extern struct page *pmd_page(pmd_t pmd); static inline pte_t pud_pte(pud_t pud) @@ -964,6 +970,12 @@ static inline int pgd_present(pgd_t pgd) return !!(pgd_raw(pgd) & cpu_to_be64(_PAGE_PRESENT)); } +#define pgd_leaf pgd_large +static inline int pgd_large(pgd_t pgd) +{ + return !!(pgd_raw(pgd) & cpu_to_be64(_PAGE_PTE)); +} + static inline pte_t pgd_pte(pgd_t pgd) { return __pte_raw(pgd_raw(pgd)); @@ -1131,6 +1143,15 @@ static inline bool pmd_access_permitted(pmd_t pmd, bool write) return pte_access_permitted(pmd_pte(pmd), write); } +#define pmd_leaf pmd_large +/* + * returns true for pmd migration entries, THP, devmap, hugetlb + */ +static inline int pmd_large(pmd_t pmd) +{ + return !!(pmd_raw(pmd) & cpu_to_be64(_PAGE_PTE)); +} + #ifdef CONFIG_TRANSPARENT_HUGEPAGE extern pmd_t pfn_pmd(unsigned long pfn, pgprot_t pgprot); extern pmd_t mk_pmd(struct page *page, pgprot_t pgprot); @@ -1157,15 +1178,6 @@ pmd_hugepage_update(struct mm_struct *mm, unsigned long addr, pmd_t *pmdp, return hash__pmd_hugepage_update(mm, addr, pmdp, clr, set); } -/* - * returns true for pmd migration entries, THP, devmap, hugetlb - * But compile time dependent on THP config - */ -static inline int pmd_large(pmd_t pmd) -{ - return !!(pmd_raw(pmd) & cpu_to_be64(_PAGE_PTE)); -} - static inline pmd_t pmd_mknotpresent(pmd_t pmd) { return __pmd(pmd_val(pmd) & ~_PAGE_PRESENT); -- 2.20.1
Re: [PATCH v2] powerpc: slightly improve cache helpers
On Sun, Jul 21, 2019 at 11:19 PM Segher Boessenkool wrote: > > On Sun, Jul 21, 2019 at 07:41:40PM -0700, Nathan Chancellor wrote: > > Hi Segher, > > > > On Sun, Jul 21, 2019 at 01:01:50PM -0500, Segher Boessenkool wrote: > > > On Sun, Jul 21, 2019 at 12:58:46AM -0700, Nathan Chancellor wrote: > > > > 017c clear_user_page: > > > > 17c: 94 21 ff f0 stwu 1, -16(1) > > > > 180: 38 80 00 80 li 4, 128 > > > > 184: 38 63 ff e0 addi 3, 3, -32 > > > > 188: 7c 89 03 a6 mtctr 4 > > > > 18c: 38 81 00 0f addi 4, 1, 15 > > > > 190: 8c c3 00 20 lbzu 6, 32(3) > > > > 194: 98 c1 00 0f stb 6, 15(1) > > > > 198: 7c 00 27 ec dcbz 0, 4 > > > > 19c: 42 00 ff f4 bdnz .+65524 > > > > > > Uh, yeah, well, I have no idea what clang tried here, but that won't > > > work. It's copying a byte from each target cache line to the stack, > > > and then does clears the cache line containing that byte on the stack. > > > > > > I *guess* this is about "Z" and not about "%y", but you'll have to ask > > > the clang people. > > > > > > Or it may be that they do not treat inline asm operands as lvalues > > > properly? That rings some bells. Yeah that looks like it. > > The code is > __asm__ __volatile__ ("dcbz %y0" : : "Z"(*(u8 *)addr) : "memory"); > > so yeah it looks like clang took that *(u8 *)addr as rvalue, and > stored that in stack, and then used *that* as memory. What's the %y modifier supposed to mean here? addr is in the list of inputs, so what's wrong with using it as an rvalue? > > Maybe clang simply does not not to treat "Z" the same as "m"? (And "Y" > and "Q" and "es" and a whole bunch of "w*", what about those?) -- Thanks, ~Nick Desaulniers
[RFC PATCH 0/4] assorted virtual / physical address fixes
Implementing VIRTUAL_BUG_ON to catch incorrect usage of __va and __pa showed up a few possible issues. Actually patch 1 was found by inspection (I will check whether I may attribute the reporter). Thanks, Nick Nicholas Piggin (4): powerpc/64s/radix: Fix memory hotplug section page table creation powerpc/64s/radix: Fix memory hot-unplug page table split powerpc/perf: fix imc allocation failure powerpc/64: Add VIRTUAL_BUG_ON checks for __va and __pa addresses arch/powerpc/include/asm/page.h | 14 ++-- arch/powerpc/mm/book3s64/radix_pgtable.c | 6 ++--- arch/powerpc/perf/imc-pmu.c | 29 +++- 3 files changed, 33 insertions(+), 16 deletions(-) -- 2.20.1
[RFC PATCH 1/4] powerpc/64s/radix: Fix memory hotplug section page table creation
create_physical_mapping expects physical addresses, but creating and splitting these mappings after boot is supplying virtual (effective) addresses. This can be hit by booting with limited memory then probing new physical memory sections. Cc: Reza Arbab Fixes: 6cc27341b21a8 ("powerpc/mm: add radix__create_section_mapping()") Signed-off-by: Nicholas Piggin --- arch/powerpc/mm/book3s64/radix_pgtable.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c index b4ca9e95e678..c5cc16ab1954 100644 --- a/arch/powerpc/mm/book3s64/radix_pgtable.c +++ b/arch/powerpc/mm/book3s64/radix_pgtable.c @@ -902,7 +902,7 @@ int __meminit radix__create_section_mapping(unsigned long start, unsigned long e return -1; } - return create_physical_mapping(start, end, nid); + return create_physical_mapping(__pa(start), __pa(end), nid); } int __meminit radix__remove_section_mapping(unsigned long start, unsigned long end) -- 2.20.1
[RFC PATCH 2/4] powerpc/64s/radix: Fix memory hot-unplug page table split
create_physical_mapping expects physical addresses, but splitting these mapping on hot unplug is supplying virtual (effective) addresses. [I'm not sure how to test this one] Cc: Balbir Singh Fixes: 4dd5f8a99e791 ("powerpc/mm/radix: Split linear mapping on hot-unplug") Signed-off-by: Nicholas Piggin --- arch/powerpc/mm/book3s64/radix_pgtable.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c index c5cc16ab1954..2204d8eeb784 100644 --- a/arch/powerpc/mm/book3s64/radix_pgtable.c +++ b/arch/powerpc/mm/book3s64/radix_pgtable.c @@ -737,8 +737,8 @@ static int __meminit stop_machine_change_mapping(void *data) spin_unlock(&init_mm.page_table_lock); pte_clear(&init_mm, params->aligned_start, params->pte); - create_physical_mapping(params->aligned_start, params->start, -1); - create_physical_mapping(params->end, params->aligned_end, -1); + create_physical_mapping(__pa(params->aligned_start), __pa(params->start), -1); + create_physical_mapping(__pa(params->end), __pa(params->aligned_end), -1); spin_lock(&init_mm.page_table_lock); return 0; } -- 2.20.1
[RFC PATCH 3/4] powerpc/perf: fix imc allocation failure
alloc_pages_node return value should be tested before applying page_address. Cc: Anju T Sudhakar Cc: Madhavan Srinivasan Signed-off-by: Nicholas Piggin --- arch/powerpc/perf/imc-pmu.c | 29 ++--- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c index dea243185ea4..cb50a9e1fd2d 100644 --- a/arch/powerpc/perf/imc-pmu.c +++ b/arch/powerpc/perf/imc-pmu.c @@ -577,6 +577,7 @@ static int core_imc_mem_init(int cpu, int size) { int nid, rc = 0, core_id = (cpu / threads_per_core); struct imc_mem_info *mem_info; + struct page *page; /* * alloc_pages_node() will allocate memory for core in the @@ -587,11 +588,12 @@ static int core_imc_mem_init(int cpu, int size) mem_info->id = core_id; /* We need only vbase for core counters */ - mem_info->vbase = page_address(alloc_pages_node(nid, - GFP_KERNEL | __GFP_ZERO | __GFP_THISNODE | - __GFP_NOWARN, get_order(size))); - if (!mem_info->vbase) + page = alloc_pages_node(nid, + GFP_KERNEL | __GFP_ZERO | __GFP_THISNODE | + __GFP_NOWARN, get_order(size)); + if (!page) return -ENOMEM; + mem_info->vbase = page_address(page); /* Init the mutex */ core_imc_refc[core_id].id = core_id; @@ -849,15 +851,17 @@ static int thread_imc_mem_alloc(int cpu_id, int size) int nid = cpu_to_node(cpu_id); if (!local_mem) { + struct page *page; /* * This case could happen only once at start, since we dont * free the memory in cpu offline path. */ - local_mem = page_address(alloc_pages_node(nid, + page = alloc_pages_node(nid, GFP_KERNEL | __GFP_ZERO | __GFP_THISNODE | - __GFP_NOWARN, get_order(size))); - if (!local_mem) + __GFP_NOWARN, get_order(size)); + if (!page) return -ENOMEM; + local_mem = page_address(page); per_cpu(thread_imc_mem, cpu_id) = local_mem; } @@ -1095,11 +1099,14 @@ static int trace_imc_mem_alloc(int cpu_id, int size) int core_id = (cpu_id / threads_per_core); if (!local_mem) { - local_mem = page_address(alloc_pages_node(phys_id, - GFP_KERNEL | __GFP_ZERO | __GFP_THISNODE | - __GFP_NOWARN, get_order(size))); - if (!local_mem) + struct page *page; + + page = alloc_pages_node(phys_id, + GFP_KERNEL | __GFP_ZERO | __GFP_THISNODE | + __GFP_NOWARN, get_order(size)); + if (!page) return -ENOMEM; + local_mem = page_address(page); per_cpu(trace_imc_mem, cpu_id) = local_mem; /* Initialise the counters for trace mode */ -- 2.20.1
[RFC PATCH 4/4] powerpc/64: Add VIRTUAL_BUG_ON checks for __va and __pa addresses
Ensure __va is given an address below PAGE_OFFSET, and __pa is given one above PAGE_OFFSET. Signed-off-by: Nicholas Piggin --- arch/powerpc/include/asm/page.h | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h index 0d52f57fca04..c8bb14ff4713 100644 --- a/arch/powerpc/include/asm/page.h +++ b/arch/powerpc/include/asm/page.h @@ -215,9 +215,19 @@ static inline bool pfn_valid(unsigned long pfn) /* * gcc miscompiles (unsigned long)(&static_var) - PAGE_OFFSET * with -mcmodel=medium, so we use & and | instead of - and + on 64-bit. + * This also results in better code generation. */ -#define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET)) -#define __pa(x) ((unsigned long)(x) & 0x0fffUL) +#define __va(x) \ +({ \ + VIRTUAL_BUG_ON((unsigned long)(x) >= PAGE_OFFSET); \ + (void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET);\ +}) + +#define __pa(x) \ +({ \ + VIRTUAL_BUG_ON((unsigned long)(x) < PAGE_OFFSET); \ + (unsigned long)(x) & 0x0fffUL; \ +}) #else /* 32-bit, non book E */ #define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) + PAGE_OFFSET - MEMORY_START)) -- 2.20.1
Re: [PATCH 2/2] powerpc: avoid adjusting memory_limit for capture kernel memory reservation
On Fri, 28 Jun 2019 00:51:19 +0530 Hari Bathini wrote: > Currently, if memory_limit is specified and it overlaps with memory to > be reserved for capture kernel, memory_limit is adjusted to accommodate > capture kernel. With memory reservation for capture kernel moved later > (after enforcing memory limit), this adjustment no longer holds water. > So, avoid adjusting memory_limit and error out instead. Can you split out the memory limit adjustment out of memory reservation so it can still be adjusted? Thanks Michal > > Signed-off-by: Hari Bathini > --- > arch/powerpc/kernel/fadump.c| 16 > arch/powerpc/kernel/machine_kexec.c | 22 +++--- > 2 files changed, 11 insertions(+), 27 deletions(-) > > diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c > index 4eab972..a784695 100644 > --- a/arch/powerpc/kernel/fadump.c > +++ b/arch/powerpc/kernel/fadump.c > @@ -476,22 +476,6 @@ int __init fadump_reserve_mem(void) > #endif > } > > - /* > - * Calculate the memory boundary. > - * If memory_limit is less than actual memory boundary then reserve > - * the memory for fadump beyond the memory_limit and adjust the > - * memory_limit accordingly, so that the running kernel can run with > - * specified memory_limit. > - */ > - if (memory_limit && memory_limit < memblock_end_of_DRAM()) { > - size = get_fadump_area_size(); > - if ((memory_limit + size) < memblock_end_of_DRAM()) > - memory_limit += size; > - else > - memory_limit = memblock_end_of_DRAM(); > - printk(KERN_INFO "Adjusted memory_limit for firmware-assisted" > - " dump, now %#016llx\n", memory_limit); > - } > if (memory_limit) > memory_boundary = memory_limit; > else > diff --git a/arch/powerpc/kernel/machine_kexec.c > b/arch/powerpc/kernel/machine_kexec.c > index c4ed328..fc5533b 100644 > --- a/arch/powerpc/kernel/machine_kexec.c > +++ b/arch/powerpc/kernel/machine_kexec.c > @@ -125,10 +125,8 @@ void __init reserve_crashkernel(void) > crashk_res.end = crash_base + crash_size - 1; > } > > - if (crashk_res.end == crashk_res.start) { > - crashk_res.start = crashk_res.end = 0; > - return; > - } > + if (crashk_res.end == crashk_res.start) > + goto error_out; > > /* We might have got these values via the command line or the >* device tree, either way sanitise them now. */ > @@ -170,15 +168,13 @@ void __init reserve_crashkernel(void) > if (overlaps_crashkernel(__pa(_stext), _end - _stext)) { > printk(KERN_WARNING > "Crash kernel can not overlap current kernel\n"); > - crashk_res.start = crashk_res.end = 0; > - return; > + goto error_out; > } > > /* Crash kernel trumps memory limit */ > if (memory_limit && memory_limit <= crashk_res.end) { > - memory_limit = crashk_res.end + 1; > - printk("Adjusted memory limit for crashkernel, now 0x%llx\n", > -memory_limit); > + pr_err("Crash kernel size can't exceed memory_limit\n"); > + goto error_out; > } > > printk(KERN_INFO "Reserving %ldMB of memory at %ldMB " > @@ -190,9 +186,13 @@ void __init reserve_crashkernel(void) > if (!memblock_is_region_memory(crashk_res.start, crash_size) || > memblock_reserve(crashk_res.start, crash_size)) { > pr_err("Failed to reserve memory for crashkernel!\n"); > - crashk_res.start = crashk_res.end = 0; > - return; > + goto error_out; > } > + > + return; > +error_out: > + crashk_res.start = crashk_res.end = 0; > + return; > } > > int overlaps_crashkernel(unsigned long start, unsigned long size) >
Re: [PATCH v2] powerpc: slightly improve cache helpers
On Mon, Jul 22, 2019 at 10:21:07AM -0700, Nick Desaulniers wrote: > On Sun, Jul 21, 2019 at 11:19 PM Segher Boessenkool > wrote: > > On Sun, Jul 21, 2019 at 07:41:40PM -0700, Nathan Chancellor wrote: > > > On Sun, Jul 21, 2019 at 01:01:50PM -0500, Segher Boessenkool wrote: > > > > On Sun, Jul 21, 2019 at 12:58:46AM -0700, Nathan Chancellor wrote: > > > > > 017c clear_user_page: > > > > > 17c: 94 21 ff f0 stwu 1, -16(1) > > > > > 180: 38 80 00 80 li 4, 128 > > > > > 184: 38 63 ff e0 addi 3, 3, -32 > > > > > 188: 7c 89 03 a6 mtctr 4 > > > > > 18c: 38 81 00 0f addi 4, 1, 15 > > > > > 190: 8c c3 00 20 lbzu 6, 32(3) > > > > > 194: 98 c1 00 0f stb 6, 15(1) > > > > > 198: 7c 00 27 ec dcbz 0, 4 > > > > > 19c: 42 00 ff f4 bdnz .+65524 > > > > > > > > Uh, yeah, well, I have no idea what clang tried here, but that won't > > > > work. It's copying a byte from each target cache line to the stack, > > > > and then does clears the cache line containing that byte on the stack. > > > > > > > > I *guess* this is about "Z" and not about "%y", but you'll have to ask > > > > the clang people. > > > > > > > > Or it may be that they do not treat inline asm operands as lvalues > > > > properly? That rings some bells. Yeah that looks like it. > > > > The code is > > __asm__ __volatile__ ("dcbz %y0" : : "Z"(*(u8 *)addr) : "memory"); > > > > so yeah it looks like clang took that *(u8 *)addr as rvalue, and > > stored that in stack, and then used *that* as memory. > > What's the %y modifier supposed to mean here? It prints a memory address for an indexed operand. If you write just "%0" it prints addresses that are a single register as "0(r3)" instead of "0,r3". Some instructions do not allow offset form. > addr is in the list of > inputs, so what's wrong with using it as an rvalue? It seems to use *(u8 *)addr as rvalue. Asm operands are lvalues. It matters a lot for memory operands. > > Maybe clang simply does not not to treat "Z" the same as "m"? (And "Y" > > and "Q" and "es" and a whole bunch of "w*", what about those?) Segher
Re: [PATCH] powerpc/dma: Fix invalid DMA mmap behavior
On 7/22/19 7:16 AM, Michael Ellerman wrote: Arnd Bergmann writes: On Thu, Jul 18, 2019 at 11:52 AM Christoph Hellwig wrote: On Thu, Jul 18, 2019 at 10:49:34AM +0200, Christoph Hellwig wrote: On Thu, Jul 18, 2019 at 01:45:16PM +1000, Oliver O'Halloran wrote: Other than m68k, mips, and arm64, everybody else that doesn't have ARCH_NO_COHERENT_DMA_MMAP set uses this default implementation, so I assume this behavior is acceptable on those architectures. It might be acceptable, but there's no reason to use pgport_noncached if the platform supports cache-coherent DMA. Christoph (+cc) made the change so maybe he saw something we're missing. I always found the forcing of noncached access even for coherent devices a little odd, but this was inherited from the previous implementation, which surprised me a bit as the different attributes are usually problematic even on x86. Let me dig into the history a bit more, but I suspect the righ fix is to default to cached mappings for coherent devices. Ok, some history: The generic dma mmap implementation, which we are effectively still using today was added by: commit 64ccc9c033c6089b2d426dad3c56477ab066c999 Author: Marek Szyprowski Date: Thu Jun 14 13:03:04 2012 +0200 common: dma-mapping: add support for generic dma_mmap_* calls and unconditionally uses pgprot_noncached in dma_common_mmap, which is then used as the fallback by dma_mmap_attrs if no ->mmap method is present. At that point we already had the powerpc implementation that only uses pgprot_noncached for non-coherent mappings, and the arm one, which uses pgprot_writecombine if DMA_ATTR_WRITE_COMBINE is set and otherwise pgprot_dmacoherent, which seems to be uncached. Arm did support coherent platforms at that time, but they might have been an afterthought and not handled properly. Cache-coherent devices are still very rare on 32-bit ARM. Among the callers of dma_mmap_coherent(), almost all are in platform specific device drivers that only ever run on noncoherent ARM SoCs, which explains why nobody would have noticed problems. There is also a difference in behavior between ARM and PowerPC when dealing with mismatched cacheability attributes: If the same page is mapped as both cached and uncached to, this may cause silent undefined behavior on ARM, while PowerPC should enter a checkstop as soon as it notices. On newer Power CPUs it's actually more like the ARM behaviour. I don't know for sure that it will *never* checkstop but there are at least cases where it won't. There's some (not much) detail in the Power8/9 user manuals. The issue was discovered due to sporadic checkstops on P9, so it seems like it will happen at least sometimes. cheers
Re: [PATCH] powerpc/dma: Fix invalid DMA mmap behavior
Shawn Anastasio writes: > On 7/22/19 7:16 AM, Michael Ellerman wrote: >> Arnd Bergmann writes: >>> On Thu, Jul 18, 2019 at 11:52 AM Christoph Hellwig wrote: On Thu, Jul 18, 2019 at 10:49:34AM +0200, Christoph Hellwig wrote: > On Thu, Jul 18, 2019 at 01:45:16PM +1000, Oliver O'Halloran wrote: >>> Other than m68k, mips, and arm64, everybody else that doesn't have >>> ARCH_NO_COHERENT_DMA_MMAP set uses this default implementation, so >>> I assume this behavior is acceptable on those architectures. >> >> It might be acceptable, but there's no reason to use pgport_noncached >> if the platform supports cache-coherent DMA. >> >> Christoph (+cc) made the change so maybe he saw something we're missing. > > I always found the forcing of noncached access even for coherent > devices a little odd, but this was inherited from the previous > implementation, which surprised me a bit as the different attributes > are usually problematic even on x86. Let me dig into the history a > bit more, but I suspect the righ fix is to default to cached mappings > for coherent devices. Ok, some history: The generic dma mmap implementation, which we are effectively still using today was added by: commit 64ccc9c033c6089b2d426dad3c56477ab066c999 Author: Marek Szyprowski Date: Thu Jun 14 13:03:04 2012 +0200 common: dma-mapping: add support for generic dma_mmap_* calls and unconditionally uses pgprot_noncached in dma_common_mmap, which is then used as the fallback by dma_mmap_attrs if no ->mmap method is present. At that point we already had the powerpc implementation that only uses pgprot_noncached for non-coherent mappings, and the arm one, which uses pgprot_writecombine if DMA_ATTR_WRITE_COMBINE is set and otherwise pgprot_dmacoherent, which seems to be uncached. Arm did support coherent platforms at that time, but they might have been an afterthought and not handled properly. >>> >>> Cache-coherent devices are still very rare on 32-bit ARM. >>> >>> Among the callers of dma_mmap_coherent(), almost all are in platform >>> specific device drivers that only ever run on noncoherent ARM SoCs, >>> which explains why nobody would have noticed problems. >>> >>> There is also a difference in behavior between ARM and PowerPC >>> when dealing with mismatched cacheability attributes: If the same >>> page is mapped as both cached and uncached to, this may >>> cause silent undefined behavior on ARM, while PowerPC should >>> enter a checkstop as soon as it notices. >> >> On newer Power CPUs it's actually more like the ARM behaviour. >> >> I don't know for sure that it will *never* checkstop but there are at >> least cases where it won't. There's some (not much) detail in the >> Power8/9 user manuals. > > The issue was discovered due to sporadic checkstops on P9, so it > seems like it will happen at least sometimes. Yeah true. I wasn't sure if that checkstop was actually caused by a cached/uncached mismatch or something else, but looks like it was, from the hostboot issue (https://github.com/open-power/hostboot/issues/180): 12.47015| Signature Description: pu.ex:k0:n0:s0:p00:c0 (L2FIR[16]) Cache line inhibited hit cacheable space So I'm not really sure how to square that with the documentation in the user manual: If a caching-inhibited load instruction hits in the L1 data cache, the load data is serviced from the L1 data cache and no request is sent to the NCU. If a caching-inhibited store instruction hits in the L1 data cache, the store data is written to the L1 data cache and sent to the NCU. Note that the L1 data cache and L2 cache are no longer coherent. I guess I'm either misinterpreting that section or there's some *other* documentation somewhere I haven't found that says that it will also checkstop. cheers
Re: [PATCH v2] powerpc: slightly improve cache helpers
Segher Boessenkool writes: > On Mon, Jul 22, 2019 at 08:15:14PM +1000, Michael Ellerman wrote: >> Segher Boessenkool writes: >> > On Sun, Jul 21, 2019 at 12:58:46AM -0700, Nathan Chancellor wrote: >> >> 017c clear_user_page: >> >> 17c: 94 21 ff f0 stwu 1, -16(1) >> >> 180: 38 80 00 80 li 4, 128 >> >> 184: 38 63 ff e0 addi 3, 3, -32 >> >> 188: 7c 89 03 a6 mtctr 4 >> >> 18c: 38 81 00 0f addi 4, 1, 15 >> >> 190: 8c c3 00 20 lbzu 6, 32(3) >> >> 194: 98 c1 00 0f stb 6, 15(1) >> >> 198: 7c 00 27 ec dcbz 0, 4 >> >> 19c: 42 00 ff f4 bdnz .+65524 >> > >> > Uh, yeah, well, I have no idea what clang tried here, but that won't >> > work. It's copying a byte from each target cache line to the stack, >> > and then does clears the cache line containing that byte on the stack. >> >> So it seems like this is a clang bug. >> >> None of the distros we support use clang, but we would still like to >> keep it working if we can. > > Which version? Which versions *are* broken? AFAIK clang 8 is the first version that we could build with, without hacks. >> Looking at the original patch, the only upside is that the compiler >> can use both RA and RB to compute the address, rather than us forcing RA >> to 0. >> >> But at least with my compiler here (GCC 8 vintage) I don't actually see >> GCC ever using both GPRs even with the patch. Or at least, there's no >> difference before/after the patch as far as I can see. > > The benefit is small, certainly. Zero is small, but I guess some things are smaller? :P >> So my inclination is to revert the original patch. We can try again in a >> few years :D >> >> Thoughts? > > I think you should give the clang people time to figure out what is > going on. Yeah fair enough, will wait and see what their diagnosis is. cheers
[PATCH v3 4/9] powerpc/eeh: Initialize EEH address cache earlier
The EEH address cache is currently initialized and populated by a single function: eeh_addr_cache_build(). While the initial population of the cache can only be done once resources are allocated, initialization (just setting up a spinlock) could be done much earlier. So move the initialization step into a separate function and call it from a core_initcall (rather than a subsys initcall). This will allow future work to make use of the cache during boot time PCI scanning. Signed-off-by: Sam Bobroff Reviewed-by: Alexey Kardashevskiy --- arch/powerpc/include/asm/eeh.h | 3 +++ arch/powerpc/kernel/eeh.c | 2 ++ arch/powerpc/kernel/eeh_cache.c | 13 +++-- 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h index 45c9b26e3cce..20105964287a 100644 --- a/arch/powerpc/include/asm/eeh.h +++ b/arch/powerpc/include/asm/eeh.h @@ -275,6 +275,7 @@ int __init eeh_ops_register(struct eeh_ops *ops); int __exit eeh_ops_unregister(const char *name); int eeh_check_failure(const volatile void __iomem *token); int eeh_dev_check_failure(struct eeh_dev *edev); +void eeh_addr_cache_init(void); void eeh_addr_cache_build(void); void eeh_add_device_early(struct pci_dn *); void eeh_add_device_tree_early(struct pci_dn *); @@ -335,6 +336,8 @@ static inline int eeh_check_failure(const volatile void __iomem *token) #define eeh_dev_check_failure(x) (0) +static inline void eeh_addr_cache_init(void) { } + static inline void eeh_addr_cache_build(void) { } static inline void eeh_add_device_early(struct pci_dn *pdn) { } diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c index 846cc697030c..ca8b0c58a6a7 100644 --- a/arch/powerpc/kernel/eeh.c +++ b/arch/powerpc/kernel/eeh.c @@ -1200,6 +1200,8 @@ static int eeh_init(void) list_for_each_entry_safe(hose, tmp, &hose_list, list_node) eeh_dev_phb_init_dynamic(hose); + eeh_addr_cache_init(); + /* Initialize EEH event */ return eeh_event_init(); } diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c index 320472373122..a790fa49c62d 100644 --- a/arch/powerpc/kernel/eeh_cache.c +++ b/arch/powerpc/kernel/eeh_cache.c @@ -254,6 +254,17 @@ void eeh_addr_cache_rmv_dev(struct pci_dev *dev) spin_unlock_irqrestore(&pci_io_addr_cache_root.piar_lock, flags); } +/** + * eeh_addr_cache_init - Initialize a cache of I/O addresses + * + * Initialize a cache of pci i/o addresses. This cache will be used to + * find the pci device that corresponds to a given address. + */ +void eeh_addr_cache_init(void) +{ + spin_lock_init(&pci_io_addr_cache_root.piar_lock); +} + /** * eeh_addr_cache_build - Build a cache of I/O addresses * @@ -269,8 +280,6 @@ void eeh_addr_cache_build(void) struct eeh_dev *edev; struct pci_dev *dev = NULL; - spin_lock_init(&pci_io_addr_cache_root.piar_lock); - for_each_pci_dev(dev) { pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn); if (!pdn) -- 2.22.0.216.g00a2a96fc9
[PATCH v3 1/9] powerpc/64: Adjust order in pcibios_init()
The pcibios_init() function for 64 bit PowerPC currently calls pci_bus_add_devices() before pcibios_resource_survey(), which seems incorrect because it adds devices and attempts to bind their drivers before allocating their resources (although no problems seem to be apparent). So move the call to pci_bus_add_devices() to after pcibios_resource_survey(), while extracting call to the pcibios_fixup() hook so that it remains in the same location. This will also allow the ppc_md.pcibios_bus_add_device() hooks to perform actions that depend on PCI resources, both during rescanning (where this is already the case) and at boot time, to support future work. Signed-off-by: Sam Bobroff Reviewed-by: Alexey Kardashevskiy --- arch/powerpc/kernel/pci-common.c | 4 arch/powerpc/kernel/pci_32.c | 4 arch/powerpc/kernel/pci_64.c | 12 +--- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c index f627e15bb43c..1c448cf25506 100644 --- a/arch/powerpc/kernel/pci-common.c +++ b/arch/powerpc/kernel/pci-common.c @@ -1379,10 +1379,6 @@ void __init pcibios_resource_survey(void) pr_debug("PCI: Assigning unassigned resources...\n"); pci_assign_unassigned_resources(); } - - /* Call machine dependent fixup */ - if (ppc_md.pcibios_fixup) - ppc_md.pcibios_fixup(); } /* This is used by the PCI hotplug driver to allocate resource diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c index 50942a1d1a5f..b49e1060a3bf 100644 --- a/arch/powerpc/kernel/pci_32.c +++ b/arch/powerpc/kernel/pci_32.c @@ -263,6 +263,10 @@ static int __init pcibios_init(void) /* Call common code to handle resource allocation */ pcibios_resource_survey(); + /* Call machine dependent fixup */ + if (ppc_md.pcibios_fixup) + ppc_md.pcibios_fixup(); + /* Call machine dependent post-init code */ if (ppc_md.pcibios_after_init) ppc_md.pcibios_after_init(); diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c index b7030b1189d0..f83d1f69b1dd 100644 --- a/arch/powerpc/kernel/pci_64.c +++ b/arch/powerpc/kernel/pci_64.c @@ -54,14 +54,20 @@ static int __init pcibios_init(void) pci_add_flags(PCI_ENABLE_PROC_DOMAINS | PCI_COMPAT_DOMAIN_0); /* Scan all of the recorded PCI controllers. */ - list_for_each_entry_safe(hose, tmp, &hose_list, list_node) { + list_for_each_entry_safe(hose, tmp, &hose_list, list_node) pcibios_scan_phb(hose); - pci_bus_add_devices(hose->bus); - } /* Call common code to handle resource allocation */ pcibios_resource_survey(); + /* Add devices. */ + list_for_each_entry_safe(hose, tmp, &hose_list, list_node) + pci_bus_add_devices(hose->bus); + + /* Call machine dependent fixup */ + if (ppc_md.pcibios_fixup) + ppc_md.pcibios_fixup(); + printk(KERN_DEBUG "PCI: Probing PCI hardware done\n"); return 0; -- 2.22.0.216.g00a2a96fc9
[PATCH v3 8/9] powerpc/eeh: Introduce EEH edev logging macros
Now that struct eeh_dev includes the BDFN of it's PCI device, make use of it to replace eeh_edev_info() with a set of dev_dbg()-style macros that only need a struct edev. With the BDFN available without the struct pci_dev, eeh_pci_name() is now unnecessary, so remove it. While only the "info" level function is used here, the others will be used in followup work. Signed-off-by: Sam Bobroff --- v3 * New in this version. arch/powerpc/include/asm/eeh.h | 11 +++ arch/powerpc/kernel/eeh_driver.c | 17 - 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h index bbe0798f6624..e1023a556721 100644 --- a/arch/powerpc/include/asm/eeh.h +++ b/arch/powerpc/include/asm/eeh.h @@ -138,6 +138,17 @@ struct eeh_dev { struct pci_dev *physfn; /* Associated SRIOV PF */ }; +/* "fmt" must be a simple literal string */ +#define EEH_EDEV_PRINT(level, edev, fmt, ...) \ + pr_##level("PCI %04x:%02x:%02x.%x#%04x: EEH: " fmt, \ + (edev)->controller->global_number, PCI_BUSNO((edev)->bdfn), \ + PCI_SLOT((edev)->bdfn), PCI_FUNC((edev)->bdfn), \ + ((edev)->pe ? (edev)->pe_config_addr : 0x), ##__VA_ARGS__) +#define eeh_edev_dbg(edev, fmt, ...) EEH_EDEV_PRINT(debug, (edev), fmt, ##__VA_ARGS__) +#define eeh_edev_info(edev, fmt, ...) EEH_EDEV_PRINT(info, (edev), fmt, ##__VA_ARGS__) +#define eeh_edev_warn(edev, fmt, ...) EEH_EDEV_PRINT(warn, (edev), fmt, ##__VA_ARGS__) +#define eeh_edev_err(edev, fmt, ...) EEH_EDEV_PRINT(err, (edev), fmt, ##__VA_ARGS__) + static inline struct pci_dn *eeh_dev_to_pdn(struct eeh_dev *edev) { return edev ? edev->pdn : NULL; diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c index d6f54840a3a9..29424d5e5fea 100644 --- a/arch/powerpc/kernel/eeh_driver.c +++ b/arch/powerpc/kernel/eeh_driver.c @@ -82,23 +82,6 @@ static const char *pci_ers_result_name(enum pci_ers_result result) } }; -static __printf(2, 3) void eeh_edev_info(const struct eeh_dev *edev, -const char *fmt, ...) -{ - struct va_format vaf; - va_list args; - - va_start(args, fmt); - - vaf.fmt = fmt; - vaf.va = &args; - - printk(KERN_INFO "EEH: PE#%x (PCI %s): %pV\n", edev->pe_config_addr, - edev->pdev ? dev_name(&edev->pdev->dev) : "none", &vaf); - - va_end(args); -} - static enum pci_ers_result pci_ers_merge_result(enum pci_ers_result old, enum pci_ers_result new) { -- 2.22.0.216.g00a2a96fc9
[PATCH v3 7/9] powerpc/eeh: Add bdfn field to eeh_dev
From: Oliver O'Halloran Preparation for removing pci_dn from the powernv EEH code. The only thing we really use pci_dn for is to get the bdfn of the device for config space accesses, so adding that information to eeh_dev reduces the need to carry around the pci_dn. Signed-off-by: Oliver O'Halloran [SB: Re-wrapped commit message, fixed whitespace damage.] Signed-off-by: Sam Bobroff --- v3 * New in this version. arch/powerpc/include/asm/eeh.h | 2 ++ arch/powerpc/include/asm/ppc-pci.h | 2 ++ arch/powerpc/kernel/eeh_dev.c | 2 ++ 3 files changed, 6 insertions(+) diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h index 7f9404a0c3bb..bbe0798f6624 100644 --- a/arch/powerpc/include/asm/eeh.h +++ b/arch/powerpc/include/asm/eeh.h @@ -121,6 +121,8 @@ static inline bool eeh_pe_passed(struct eeh_pe *pe) struct eeh_dev { int mode; /* EEH mode */ int class_code; /* Class code of the device */ + int bdfn; /* bdfn of device (for cfg ops) */ + struct pci_controller *controller; int pe_config_addr; /* PE config address*/ u32 config_space[16]; /* Saved PCI config space */ int pcix_cap; /* Saved PCIx capability*/ diff --git a/arch/powerpc/include/asm/ppc-pci.h b/arch/powerpc/include/asm/ppc-pci.h index cec2d6409515..72860de205a0 100644 --- a/arch/powerpc/include/asm/ppc-pci.h +++ b/arch/powerpc/include/asm/ppc-pci.h @@ -74,6 +74,8 @@ static inline const char *eeh_driver_name(struct pci_dev *pdev) #endif /* CONFIG_EEH */ +#define PCI_BUSNO(bdfn) ((bdfn >> 8) & 0xff) + #else /* CONFIG_PCI */ static inline void init_pci_config_tokens(void) { } #endif /* !CONFIG_PCI */ diff --git a/arch/powerpc/kernel/eeh_dev.c b/arch/powerpc/kernel/eeh_dev.c index c4317c452d98..7370185c7a05 100644 --- a/arch/powerpc/kernel/eeh_dev.c +++ b/arch/powerpc/kernel/eeh_dev.c @@ -47,6 +47,8 @@ struct eeh_dev *eeh_dev_init(struct pci_dn *pdn) /* Associate EEH device with OF node */ pdn->edev = edev; edev->pdn = pdn; + edev->bdfn = (pdn->busno << 8) | pdn->devfn; + edev->controller = pdn->phb; return edev; } -- 2.22.0.216.g00a2a96fc9