* [PATCH] nvmem: uniphier: change access unit from 32bit to 8bit
From: Srinivas Kandagatla @ 2017-12-13 9:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171213183152.A835.4A936039@socionext.com>
On 13/12/17 09:31, Kunihiko Hayashi wrote:
> Hello Srinivas,
>
> This patch was already reviewed and tested for all UniPhier SoCs:
> https://patchwork.kernel.org/patch/10069557/
>
> Do you have any comments, or would you please pick it up?
Thanks for your patience,
I will send it along with other fixes to Greg Sometime this week!
Rgrds,
Srini
>
> Thank you,
>
> On Wed, 22 Nov 2017 14:14:59 +0900 Kunihiko Hayashi <hayashi.kunihiko@socionext.com> wrote:
>
>> The efuse on UniPhier allows 8bit access according to the specification.
>> Since bit offset of nvmem is limited to 0-7, it is desiable to change
>> access unit of nvmem to 8bit.
>>
>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
>> ---
>> drivers/nvmem/uniphier-efuse.c | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/nvmem/uniphier-efuse.c b/drivers/nvmem/uniphier-efuse.c
>> index 2bb45c4..fac3122 100644
>> --- a/drivers/nvmem/uniphier-efuse.c
>> +++ b/drivers/nvmem/uniphier-efuse.c
>> @@ -27,11 +27,11 @@ static int uniphier_reg_read(void *context,
>> unsigned int reg, void *_val, size_t bytes)
>> {
>> struct uniphier_efuse_priv *priv = context;
>> - u32 *val = _val;
>> + u8 *val = _val;
>> int offs;
>>
>> - for (offs = 0; offs < bytes; offs += sizeof(u32))
>> - *val++ = readl(priv->base + reg + offs);
>> + for (offs = 0; offs < bytes; offs += sizeof(u8))
>> + *val++ = readb(priv->base + reg + offs);
>>
>> return 0;
>> }
>> @@ -53,8 +53,8 @@ static int uniphier_efuse_probe(struct platform_device *pdev)
>> if (IS_ERR(priv->base))
>> return PTR_ERR(priv->base);
>>
>> - econfig.stride = 4;
>> - econfig.word_size = 4;
>> + econfig.stride = 1;
>> + econfig.word_size = 1;
>> econfig.read_only = true;
>> econfig.reg_read = uniphier_reg_read;
>> econfig.size = resource_size(res);
>> --
>> 2.7.4
>
> ---
> Best Regards,
> Kunihiko Hayashi
>
>
^ permalink raw reply
* [PATCH] KVM: arm/arm64: don't set vtimer->cnt_ctl in kvm_arch_timer_handler
From: Christoffer Dall @ 2017-12-13 9:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <daece017-50be-494f-4176-6e5f081edfa2@arm.com>
On Wed, Dec 13, 2017 at 10:27 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 13/12/17 09:08, Auger Eric wrote:
>> Marc,
>> On 13/12/17 09:56, Marc Zyngier wrote:
>>> Hi Jia,
>>>
>>> On 13/12/17 07:00, Jia He wrote:
>>>> In our Armv8a server (qualcomm Amberwing, non VHE), after applying
>>>> Christoffer's timer optimizing patchset(Optimize arch timer register
>>>> handling), the guest is hang during kernel booting.
>>>>
>>>> The error root cause might be as follows:
>>>> 1. in kvm_arch_timer_handler, it reset vtimer->cnt_ctl with current
>>>> cntv_ctl register value. And then it missed some cases to update timer's
>>>> irq (irq.level) when kvm_timer_irq_can_fire() is false
>>>> 2. It causes kvm_vcpu_check_block return 0 instead of -EINTR
>>>> kvm_vcpu_check_block
>>>> kvm_cpu_has_pending_timer
>>>> kvm_timer_is_pending
>>>> kvm_timer_should_fire
>>>> 3. Thus, the kvm hyp code can not break the loop in kvm_vcpu_block (halt
>>>> poll process) and the guest is hang forever
>>>>
>>>> Fixes: b103cc3f10c0 ("KVM: arm/arm64: Avoid timer save/restore in vcpu entry/exit")
>>>> Signed-off-by: Jia He <jia.he@hxt-semitech.com>
>>>> ---
>>>> virt/kvm/arm/arch_timer.c | 1 -
>>>> 1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>>>> index f9555b1..bb86433 100644
>>>> --- a/virt/kvm/arm/arch_timer.c
>>>> +++ b/virt/kvm/arm/arch_timer.c
>>>> @@ -100,7 +100,6 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
>>>> vtimer = vcpu_vtimer(vcpu);
>>>>
>>>> if (!vtimer->irq.level) {
>>>> - vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
>>>> if (kvm_timer_irq_can_fire(vtimer))
>>>> kvm_timer_update_irq(vcpu, true, vtimer);
>>>> }
>>>>
>>>
>>> Which patches are you looking at? The current code in mainline looks
>>> like this:
>>>
>>> vtimer = vcpu_vtimer(vcpu);
>>>
>>> vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
>>> if (kvm_timer_irq_can_fire(vtimer))
>>> kvm_timer_update_irq(vcpu, true, vtimer);
>>>
>>> I'd suggest you use mainline and report if this doesn't work
>> the removal of if (!vtimer->irq.level) test happened in:
>> [PATCH v7 3/8] KVM: arm/arm64: Don't cache the timer IRQ level
>>
>> which is not upstream.
> Ah, my bad (I have that series in my working tree already...).
>
> I still think Jia's approach to this is not quite right. If you don't
> update the status of the timer by reading the HW value, how can you
> decide whether the timer can fire or not?
>
Exactly. We need to know the exact kernel source, symptoms, how to
reproduce, and then trace what's going on. It may be needed to tweak
kvm_timer_is_pending(), but I don't yet see a case where it breaks.
Thanks,
-Christoffer
^ permalink raw reply
* [PATCH] nvmem: uniphier: change access unit from 32bit to 8bit
From: Kunihiko Hayashi @ 2017-12-13 9:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1511327699-26396-1-git-send-email-hayashi.kunihiko@socionext.com>
Hello Srinivas,
This patch was already reviewed and tested for all UniPhier SoCs:
https://patchwork.kernel.org/patch/10069557/
Do you have any comments, or would you please pick it up?
Thank you,
On Wed, 22 Nov 2017 14:14:59 +0900 Kunihiko Hayashi <hayashi.kunihiko@socionext.com> wrote:
> The efuse on UniPhier allows 8bit access according to the specification.
> Since bit offset of nvmem is limited to 0-7, it is desiable to change
> access unit of nvmem to 8bit.
>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
> drivers/nvmem/uniphier-efuse.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/nvmem/uniphier-efuse.c b/drivers/nvmem/uniphier-efuse.c
> index 2bb45c4..fac3122 100644
> --- a/drivers/nvmem/uniphier-efuse.c
> +++ b/drivers/nvmem/uniphier-efuse.c
> @@ -27,11 +27,11 @@ static int uniphier_reg_read(void *context,
> unsigned int reg, void *_val, size_t bytes)
> {
> struct uniphier_efuse_priv *priv = context;
> - u32 *val = _val;
> + u8 *val = _val;
> int offs;
>
> - for (offs = 0; offs < bytes; offs += sizeof(u32))
> - *val++ = readl(priv->base + reg + offs);
> + for (offs = 0; offs < bytes; offs += sizeof(u8))
> + *val++ = readb(priv->base + reg + offs);
>
> return 0;
> }
> @@ -53,8 +53,8 @@ static int uniphier_efuse_probe(struct platform_device *pdev)
> if (IS_ERR(priv->base))
> return PTR_ERR(priv->base);
>
> - econfig.stride = 4;
> - econfig.word_size = 4;
> + econfig.stride = 1;
> + econfig.word_size = 1;
> econfig.read_only = true;
> econfig.reg_read = uniphier_reg_read;
> econfig.size = resource_size(res);
> --
> 2.7.4
---
Best Regards,
Kunihiko Hayashi
^ permalink raw reply
* [PATCH] KVM: arm/arm64: don't set vtimer->cnt_ctl in kvm_arch_timer_handler
From: Marc Zyngier @ 2017-12-13 9:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <f0252a5a-cb81-ce24-5d46-f19fce054b13@redhat.com>
On 13/12/17 09:08, Auger Eric wrote:
> Marc,
> On 13/12/17 09:56, Marc Zyngier wrote:
>> Hi Jia,
>>
>> On 13/12/17 07:00, Jia He wrote:
>>> In our Armv8a server (qualcomm Amberwing, non VHE), after applying
>>> Christoffer's timer optimizing patchset(Optimize arch timer register
>>> handling), the guest is hang during kernel booting.
>>>
>>> The error root cause might be as follows:
>>> 1. in kvm_arch_timer_handler, it reset vtimer->cnt_ctl with current
>>> cntv_ctl register value. And then it missed some cases to update timer's
>>> irq (irq.level) when kvm_timer_irq_can_fire() is false
>>> 2. It causes kvm_vcpu_check_block return 0 instead of -EINTR
>>> kvm_vcpu_check_block
>>> kvm_cpu_has_pending_timer
>>> kvm_timer_is_pending
>>> kvm_timer_should_fire
>>> 3. Thus, the kvm hyp code can not break the loop in kvm_vcpu_block (halt
>>> poll process) and the guest is hang forever
>>>
>>> Fixes: b103cc3f10c0 ("KVM: arm/arm64: Avoid timer save/restore in vcpu entry/exit")
>>> Signed-off-by: Jia He <jia.he@hxt-semitech.com>
>>> ---
>>> virt/kvm/arm/arch_timer.c | 1 -
>>> 1 file changed, 1 deletion(-)
>>>
>>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>>> index f9555b1..bb86433 100644
>>> --- a/virt/kvm/arm/arch_timer.c
>>> +++ b/virt/kvm/arm/arch_timer.c
>>> @@ -100,7 +100,6 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
>>> vtimer = vcpu_vtimer(vcpu);
>>>
>>> if (!vtimer->irq.level) {
>>> - vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
>>> if (kvm_timer_irq_can_fire(vtimer))
>>> kvm_timer_update_irq(vcpu, true, vtimer);
>>> }
>>>
>>
>> Which patches are you looking at? The current code in mainline looks
>> like this:
>>
>> vtimer = vcpu_vtimer(vcpu);
>>
>> vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
>> if (kvm_timer_irq_can_fire(vtimer))
>> kvm_timer_update_irq(vcpu, true, vtimer);
>>
>> I'd suggest you use mainline and report if this doesn't work
> the removal of if (!vtimer->irq.level) test happened in:
> [PATCH v7 3/8] KVM: arm/arm64: Don't cache the timer IRQ level
>
> which is not upstream.
Ah, my bad (I have that series in my working tree already...).
I still think Jia's approach to this is not quite right. If you don't
update the status of the timer by reading the HW value, how can you
decide whether the timer can fire or not?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH 0/5] Add Sound support for iWave RZ/G1M board
From: Biju Das @ 2017-12-13 9:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171213090240.xqgihh4bqsfzeaip@verge.net.au>
Hi Simon,
Thanks.
There is a typo in the documentation dependency link.
The correct one is https://patchwork.kernel.org/patch/10108015/
Regards,
Biju
> -----Original Message-----
> From: Simon Horman [mailto:horms at verge.net.au]
> Sent: 13 December 2017 09:03
> To: Biju Das <biju.das@bp.renesas.com>
> Cc: Rob Herring <robh+dt@kernel.org>; Mark Rutland
> <mark.rutland@arm.com>; Russell King <linux@armlinux.org.uk>; Magnus
> Damm <magnus.damm@gmail.com>; Chris Paterson
> <Chris.Paterson2@renesas.com>; devicetree at vger.kernel.org; linux-renesas-
> soc at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 0/5] Add Sound support for iWave RZ/G1M board
>
> On Tue, Dec 12, 2017 at 06:25:06PM +0000, Biju Das wrote:
> > This series aims to add sound support for iWave RZ/G1M board.
> >
> > This patch series has documentation dependency on
> > https://patchwork.kernel.org/patch/10108014/
> >
> > Biju Das (5):
> > ARM: shmobile: defconfig: Enable SGTL5000 audio codec
> > ARM: dts: r8a7743: Add audio clocks
> > ARM: dts: r8a7743: Add audio DMAC support
> > ARM: dts: r8a7743: Add sound support
> > ARM: dts: iwg20d-q7-common: Enable SGTL5000 audio codec
> >
> > arch/arm/boot/dts/iwg20d-q7-common.dtsi | 24 +++
> > arch/arm/boot/dts/r8a7743.dtsi | 270
> ++++++++++++++++++++++++++++++++
> > arch/arm/configs/shmobile_defconfig | 1 +
> > 3 files changed, 295 insertions(+)
>
> These patches seem clean to me although I do not have sufficient
> documentation to properly review the last patch.
>
> I will leave these sit for a few days to allow others to review them.
[https://www2.renesas.eu/media/email/unicef_2017.jpg]
This Christmas, instead of sending out cards, Renesas Electronics Europe have decided to support Unicef with a donation. For further details click here<https://www.unicef.org/> to find out about the valuable work they do, helping children all over the world.
We would like to take this opportunity to wish you a Merry Christmas and a prosperous New Year.
Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
^ permalink raw reply
* [PATCH] arm64: mm: Fix false positives in set_pte_at access/dirty race detection
From: Will Deacon @ 2017-12-13 9:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <8761ce54-ad67-1113-44e1-0d1606d12dbf@huawei.com>
Hi Yisheng,
On Wed, Dec 13, 2017 at 09:01:23AM +0800, Yisheng Xie wrote:
> On 2017/12/12 19:43, Will Deacon wrote:
> > Jiankang reports that our race detection in set_pte_at is firing when
> > copying the page tables in dup_mmap as a result of a fork(). In this
> > situation, the page table isn't actually live and so there is no way
> > that we can race with a concurrent update from the hardware page table
> > walker.
> >
> > This patch reworks the race detection so that we require either the
> > mm to match the current active_mm (i.e. currently installed in our TTBR0)
> > or the mm_users count to be greater than 1, implying that the page table
> > could be live in another CPU. The mm_users check might still be racy,
> > but we'll avoid false positives and it's not realistic to validate that
> > all the necessary locks are held as part of this assertion.
> >
> > Cc: Yisheng Xie <xieyisheng1@huawei.com>
> > Reported-by: Jiankang Chen <chenjiankang1@huawei.com>
> > Tested-by: Jiankang Chen <chenjiankang1@huawei.com>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> > arch/arm64/include/asm/pgtable.h | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> > index 3ff03a755c32..bdcc7f1c9d06 100644
> > --- a/arch/arm64/include/asm/pgtable.h
> > +++ b/arch/arm64/include/asm/pgtable.h
> > @@ -42,6 +42,8 @@
> > #include <asm/cmpxchg.h>
> > #include <asm/fixmap.h>
> > #include <linux/mmdebug.h>
> > +#include <linux/mm_types.h>
> > +#include <linux/sched.h>
> >
>
> Do you have compiled kernel after apply this patch? In our environment, it will
> fail to compile kernel if include file here(I will attach some log later).
> Instead, we move these included file after mmdebug.h, and I do not know whether
> this is just my compiler's problem:
It compiles fine for me. Are you seeing a problem building this on top of
mainline? If so, what is your .config?
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -42,6 +42,8 @@
> #include <asm/cmpxchg.h>
> #include <asm/fixmap.h>
> #include <linux/mmdebug.h>
> +#include <linux/mm_types.h>
> +#include <linux/sched.h>
>
> Sorry for not having told you this information.
>
> Thanks
> Yisheng Xie
>
> compiler err log: ==========
> [...]
> include/linux/mm_types_task.h:60: Error: unknown mnemonic `struct' -- `struct page_frag{'
> include/asm-generic/preempt.h:9: Error: unknown mnemonic `static' -- `static inline int preempt_count(void)'
> include/linux/mm_types_task.h:61: Error: unknown mnemonic `struct' -- `struct page*page'
> include/asm-generic/preempt.h:10: Error: junk at end of line, first unrecognized character is `{'
> include/linux/mm_types_task.h:63: Error: unknown mnemonic `__u32' -- `__u32 offset'
> include/asm-generic/preempt.h:11: Error: unknown mnemonic `return' -- `return READ_ONCE(((struct thread_info*)current)->preempt_count)'
> include/linux/mm_types_task.h:64: Error: unknown mnemonic `__u32' -- `__u32 size'
> [...]
This looks like the includes are being pulled into an assembly file, but
this part is guarded by #ifndef __ASSEMBLY__ so I can't see how that could
happen.
Will
^ permalink raw reply
* [PATCH v2 3/4] thermal: armada: add support for CP110
From: Gregory CLEMENT @ 2017-12-13 9:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171213091040.jwsphlax4yidm4qp@sapphire.tkos.co.il>
Hi Baruch,
On mer., d?c. 13 2017, Baruch Siach <baruch@tkos.co.il> wrote:
> Hi Miquel,
>
> On Wed, Dec 13, 2017 at 09:55:01AM +0100, Miquel RAYNAL wrote:
>> > > > How would a separate init_sensor routine improve things?
>> > >
>> > > So yes please do it, thanks to this you won't have to add the
>> > > control_msb_offset member and can use a clean function. Moreover if
>> > > in the future we see some usefulness for this LSB register then we
>> > > could use the new compatible for the Armada 38x.
>> >
>> > There are two separate issues here:
>> >
>> > 1. DT binding
>> >
>> > 2. init_sensor callback implementation
>> >
>> > We both agree on #1. The A38x and CP110 need separate compatible
>> > strings. In case we want to access the LSB control register on Armada
>> > 38x, we will need yet another compatible string
>> > (marvell,armada380-v2-thermal maybe?).
>> >
>> > As for #2, I'm all for sharing as much code as possible. I find the
>> > vendor kernel approach of duplicating the init routines[1] unhelpful
>> > as it violates the DRY principle. The differences between
>> > armada380_init_sensor() and cp110_init_sensor() are minor. In my
>> > opinion, these differences should be expressed explicitly in the
>> > armada_thermal_data, in a similar way to my suggested
>> > control_msb_offset field. The vendor code hides these differences in
>> > slight variations of duplicated code.
>> >
>> > What is the advantage of a separate init routine?
>>
>> The advantage is that is the very near future I plan to add the
>> overheat interrupt only on CP110 (not on 38x) and this needs some
>> initialization. So if we don't make different routines now, I will
>> have to do it right after.
>
> I don't think so. The code of these functions in the vendor kernel overheat
> support implementation is the same, duplicated. The variations are only in
> registers/bits offsets. A single routine with one or two added
> armada_thermal_data fields would be much easier to comprehend and maintain.
>
>> What would be fine is to have the shared code in a separate function,
>> like it is done in Marvell kernel. What do you think about that?
>
> The Marvell code does not "share" the code. Separate functions means
> duplicated code that obscures the hardware details, making maintenance harder
> on the long run.
Well, Miquel speak about writting new code, so I don't see why you refer
the Marvell LSP code. Also, I don't see how having common function will
duplicate the code.
Gregory
>
> https://en.wikipedia.org/wiki/Don%27t_repeat_yourself
>
> baruch
>
> --
> http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
> - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* [PATCH] KVM: arm/arm64: don't set vtimer->cnt_ctl in kvm_arch_timer_handler
From: Christoffer Dall @ 2017-12-13 9:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <b2d0234c-e763-d4c1-02f7-c50cd0914b13@arm.com>
On Wed, Dec 13, 2017 at 08:56:12AM +0000, Marc Zyngier wrote:
> Hi Jia,
>
> On 13/12/17 07:00, Jia He wrote:
> > In our Armv8a server (qualcomm Amberwing, non VHE), after applying
> > Christoffer's timer optimizing patchset(Optimize arch timer register
> > handling), the guest is hang during kernel booting.
> >
> > The error root cause might be as follows:
> > 1. in kvm_arch_timer_handler, it reset vtimer->cnt_ctl with current
> > cntv_ctl register value. And then it missed some cases to update timer's
> > irq (irq.level) when kvm_timer_irq_can_fire() is false
> > 2. It causes kvm_vcpu_check_block return 0 instead of -EINTR
> > kvm_vcpu_check_block
> > kvm_cpu_has_pending_timer
> > kvm_timer_is_pending
> > kvm_timer_should_fire
> > 3. Thus, the kvm hyp code can not break the loop in kvm_vcpu_block (halt
> > poll process) and the guest is hang forever
> >
> > Fixes: b103cc3f10c0 ("KVM: arm/arm64: Avoid timer save/restore in vcpu entry/exit")
> > Signed-off-by: Jia He <jia.he@hxt-semitech.com>
> > ---
> > virt/kvm/arm/arch_timer.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > index f9555b1..bb86433 100644
> > --- a/virt/kvm/arm/arch_timer.c
> > +++ b/virt/kvm/arm/arch_timer.c
> > @@ -100,7 +100,6 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
> > vtimer = vcpu_vtimer(vcpu);
> >
> > if (!vtimer->irq.level) {
> > - vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
> > if (kvm_timer_irq_can_fire(vtimer))
> > kvm_timer_update_irq(vcpu, true, vtimer);
> > }
> >
>
> Which patches are you looking at? The current code in mainline looks
> like this:
>
> vtimer = vcpu_vtimer(vcpu);
>
> vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
> if (kvm_timer_irq_can_fire(vtimer))
> kvm_timer_update_irq(vcpu, true, vtimer);
>
> I'd suggest you use mainline and report if this doesn't work.
>
That looks like you have the level-triggered mapped series applied?
That would be an interesting data point to get from Jia as well though.
Jia, can you try applying this series and see if it helps?
git://git.kernel.org/pub/scm/linux/kernel/git/cdall/linux.git level-mapped-v7
Thanks,
-Christoffer
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH v8 10/13] IIO: ADC: add stm32 DFSDM support for PDM microphone
From: Jonathan Cameron @ 2017-12-13 9:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <ed92f7e5-f321-866f-3e22-8a14e3982c9f@st.com>
On Wed, 13 Dec 2017 09:42:02 +0100
Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
> Hi Jonathan,
>
>
> On 12/12/2017 09:27 PM, Jonathan Cameron wrote:
> > On Mon, 11 Dec 2017 11:18:41 +0100
> > Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
> >
> >> This code offers a way to handle PDM audio microphones in
> >> ASOC framework. Audio driver should use consumer API.
> >> A specific management is implemented for DMA, with a
> >> callback, to allows to handle audio buffers efficiently.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>
>
> > Hi Arnaud,
> >
> > I raise a few queries on v7 of this patch.
> >
> > https://marc.info/?l=linux-iio&m=151292965915376&w=2
> Never received the associated mail (and no in my spam list):( ,I just
> discover it...
>
> Thanks to have highlighted this and sorry for the inconvenience, I will
> send a v9.
Don't worry about it! Happens to me from time to time as well.
Thanks,
Jonathan
>
> Regards
> Arnaud
>
>
> >
> > Jonathan
> >
> >> ---
> >> .../ABI/testing/sysfs-bus-iio-dfsdm-adc-stm32 | 16 +
> >> drivers/iio/adc/stm32-dfsdm-adc.c | 508 ++++++++++++++++++++-
> >> include/linux/iio/adc/stm32-dfsdm-adc.h | 18 +
> >> 3 files changed, 534 insertions(+), 8 deletions(-)
> >> create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dfsdm-adc-stm32
> >> create mode 100644 include/linux/iio/adc/stm32-dfsdm-adc.h
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dfsdm-adc-stm32 b/Documentation/ABI/testing/sysfs-bus-iio-dfsdm-adc-stm32
> >> new file mode 100644
> >> index 0000000..da98223
> >> --- /dev/null
> >> +++ b/Documentation/ABI/testing/sysfs-bus-iio-dfsdm-adc-stm32
> >> @@ -0,0 +1,16 @@
> >> +What: /sys/bus/iio/devices/iio:deviceX/in_voltage_spi_clk_freq
> >> +KernelVersion: 4.14
> >> +Contact: arnaud.pouliquen at st.com
> >> +Description:
> >> + For audio purpose only.
> >> + Used by audio driver to set/get the spi input frequency.
> >> + This is mandatory if DFSDM is slave on SPI bus, to
> >> + provide information on the SPI clock frequency during runtime
> >> + Notice that the SPI frequency should be a multiple of sample
> >> + frequency to ensure the precision.
> >> + if DFSDM input is SPI master
> >> + Reading SPI clkout frequency,
> >> + error on writing
> >> + If DFSDM input is SPI Slave:
> >> + Reading returns value previously set.
> >> + Writing value before starting conversions.
> >> \ No newline at end of file
> >> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> >> index 68b5920..2d6aed5 100644
> >> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> >> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> >> @@ -6,19 +6,25 @@
> >> * Author: Arnaud Pouliquen <arnaud.pouliquen@st.com>.
> >> */
> >>
> >> +#include <linux/dmaengine.h>
> >> +#include <linux/dma-mapping.h>
> >> #include <linux/interrupt.h>
> >> #include <linux/iio/buffer.h>
> >> #include <linux/iio/hw-consumer.h>
> >> #include <linux/iio/iio.h>
> >> #include <linux/iio/sysfs.h>
> >> +#include <linux/iio/trigger_consumer.h>
> >> +#include <linux/iio/triggered_buffer.h>
> >> #include <linux/module.h>
> >> -#include <linux/of.h>
> >> +#include <linux/of_device.h>
> >> #include <linux/platform_device.h>
> >> #include <linux/regmap.h>
> >> #include <linux/slab.h>
> >>
> >> #include "stm32-dfsdm.h"
> >>
> >> +#define DFSDM_DMA_BUFFER_SIZE (4 * PAGE_SIZE)
> >> +
> >> /* Conversion timeout */
> >> #define DFSDM_TIMEOUT_US 100000
> >> #define DFSDM_TIMEOUT (msecs_to_jiffies(DFSDM_TIMEOUT_US / 1000))
> >> @@ -58,6 +64,18 @@ struct stm32_dfsdm_adc {
> >> struct completion completion;
> >> u32 *buffer;
> >>
> >> + /* Audio specific */
> >> + unsigned int spi_freq; /* SPI bus clock frequency */
> >> + unsigned int sample_freq; /* Sample frequency after filter decimation */
> >> + int (*cb)(const void *data, size_t size, void *cb_priv);
> >> + void *cb_priv;
> >> +
> >> + /* DMA */
> >> + u8 *rx_buf;
> >> + unsigned int bufi; /* Buffer current position */
> >> + unsigned int buf_sz; /* Buffer size */
> >> + struct dma_chan *dma_chan;
> >> + dma_addr_t dma_buf;
> >> };
> >>
> >> struct stm32_dfsdm_str2field {
> >> @@ -351,10 +369,63 @@ int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
> >> return 0;
> >> }
> >>
> >> +static ssize_t dfsdm_adc_audio_get_spiclk(struct iio_dev *indio_dev,
> >> + uintptr_t priv,
> >> + const struct iio_chan_spec *chan,
> >> + char *buf)
> >> +{
> >> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> >> +
> >> + return snprintf(buf, PAGE_SIZE, "%d\n", adc->spi_freq);
> >> +}
> >> +
> >> +static ssize_t dfsdm_adc_audio_set_spiclk(struct iio_dev *indio_dev,
> >> + uintptr_t priv,
> >> + const struct iio_chan_spec *chan,
> >> + const char *buf, size_t len)
> >> +{
> >> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> >> + struct stm32_dfsdm_filter *fl = &adc->dfsdm->fl_list[adc->fl_id];
> >> + struct stm32_dfsdm_channel *ch = &adc->dfsdm->ch_list[adc->ch_id];
> >> + unsigned int sample_freq = adc->sample_freq;
> >> + unsigned int spi_freq;
> >> + int ret;
> >> +
> >> + dev_err(&indio_dev->dev, "enter %s\n", __func__);
> >> + /* If DFSDM is master on SPI, SPI freq can not be updated */
> >> + if (ch->src != DFSDM_CHANNEL_SPI_CLOCK_EXTERNAL)
> >> + return -EPERM;
> >> +
> >> + ret = kstrtoint(buf, 0, &spi_freq);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + if (!spi_freq)
> >> + return -EINVAL;
> >> +
> >> + if (sample_freq) {
> >> + if (spi_freq % sample_freq)
> >> + dev_warn(&indio_dev->dev,
> >> + "Sampling rate not accurate (%d)\n",
> >> + spi_freq / (spi_freq / sample_freq));
> >> +
> >> + ret = stm32_dfsdm_set_osrs(fl, 0, (spi_freq / sample_freq));
> >> + if (ret < 0) {
> >> + dev_err(&indio_dev->dev,
> >> + "No filter parameters that match!\n");
> >> + return ret;
> >> + }
> >> + }
> >> + adc->spi_freq = spi_freq;
> >> +
> >> + return len;
> >> +}
> >> +
> >> static int stm32_dfsdm_start_conv(struct stm32_dfsdm_adc *adc, bool dma)
> >> {
> >> struct regmap *regmap = adc->dfsdm->regmap;
> >> int ret;
> >> + unsigned int dma_en = 0, cont_en = 0;
> >>
> >> ret = stm32_dfsdm_start_channel(adc->dfsdm, adc->ch_id);
> >> if (ret < 0)
> >> @@ -365,6 +436,24 @@ static int stm32_dfsdm_start_conv(struct stm32_dfsdm_adc *adc, bool dma)
> >> if (ret < 0)
> >> goto stop_channels;
> >>
> >> + if (dma) {
> >> + /* Enable DMA transfer*/
> >> + dma_en = DFSDM_CR1_RDMAEN(1);
> >> + /* Enable conversion triggered by SPI clock*/
> >> + cont_en = DFSDM_CR1_RCONT(1);
> >> + }
> >> + /* Enable DMA transfer*/
> >> + ret = regmap_update_bits(regmap, DFSDM_CR1(adc->fl_id),
> >> + DFSDM_CR1_RDMAEN_MASK, dma_en);
> >> + if (ret < 0)
> >> + goto stop_channels;
> >> +
> >> + /* Enable conversion triggered by SPI clock*/
> >> + ret = regmap_update_bits(regmap, DFSDM_CR1(adc->fl_id),
> >> + DFSDM_CR1_RCONT_MASK, cont_en);
> >> + if (ret < 0)
> >> + goto stop_channels;
> >> +
> >> ret = stm32_dfsdm_start_filter(adc->dfsdm, adc->fl_id);
> >> if (ret < 0)
> >> goto stop_channels;
> >> @@ -398,6 +487,231 @@ static void stm32_dfsdm_stop_conv(struct stm32_dfsdm_adc *adc)
> >> stm32_dfsdm_stop_channel(adc->dfsdm, adc->ch_id);
> >> }
> >>
> >> +static int stm32_dfsdm_set_watermark(struct iio_dev *indio_dev,
> >> + unsigned int val)
> >> +{
> >> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> >> + unsigned int watermark = DFSDM_DMA_BUFFER_SIZE / 2;
> >> +
> >> + /*
> >> + * DMA cyclic transfers are used, buffer is split into two periods.
> >> + * There should be :
> >> + * - always one buffer (period) DMA is working on
> >> + * - one buffer (period) driver pushed to ASoC side.
> >> + */
> >> + watermark = min(watermark, val * (unsigned int)(sizeof(u32)));
> >> + adc->buf_sz = watermark * 2;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static unsigned int stm32_dfsdm_adc_dma_residue(struct stm32_dfsdm_adc *adc)
> >> +{
> >> + struct dma_tx_state state;
> >> + enum dma_status status;
> >> +
> >> + status = dmaengine_tx_status(adc->dma_chan,
> >> + adc->dma_chan->cookie,
> >> + &state);
> >> + if (status == DMA_IN_PROGRESS) {
> >> + /* Residue is size in bytes from end of buffer */
> >> + unsigned int i = adc->buf_sz - state.residue;
> >> + unsigned int size;
> >> +
> >> + /* Return available bytes */
> >> + if (i >= adc->bufi)
> >> + size = i - adc->bufi;
> >> + else
> >> + size = adc->buf_sz + i - adc->bufi;
> >> +
> >> + return size;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void stm32_dfsdm_audio_dma_buffer_done(void *data)
> >> +{
> >> + struct iio_dev *indio_dev = data;
> >> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> >> + int available = stm32_dfsdm_adc_dma_residue(adc);
> >> + size_t old_pos;
> >> +
> >> + /*
> >> + * FIXME: In Kernel interface does not support cyclic DMA buffer,and
> >> + * offers only an interface to push data samples per samples.
> >> + * For this reason IIO buffer interface is not used and interface is
> >> + * bypassed using a private callback registered by ASoC.
> >> + * This should be a temporary solution waiting a cyclic DMA engine
> >> + * support in IIO.
> >> + */
> >> +
> >> + dev_dbg(&indio_dev->dev, "%s: pos = %d, available = %d\n", __func__,
> >> + adc->bufi, available);
> >> + old_pos = adc->bufi;
> >> +
> >> + while (available >= indio_dev->scan_bytes) {
> >> + u32 *buffer = (u32 *)&adc->rx_buf[adc->bufi];
> >> +
> >> + /* Mask 8 LSB that contains the channel ID */
> >> + *buffer = (*buffer & 0xFFFFFF00) << 8;
> >> + available -= indio_dev->scan_bytes;
> >> + adc->bufi += indio_dev->scan_bytes;
> >> + if (adc->bufi >= adc->buf_sz) {
> >> + if (adc->cb)
> >> + adc->cb(&adc->rx_buf[old_pos],
> >> + adc->buf_sz - old_pos, adc->cb_priv);
> >> + adc->bufi = 0;
> >> + old_pos = 0;
> >> + }
> >> + }
> >> + if (adc->cb)
> >> + adc->cb(&adc->rx_buf[old_pos], adc->bufi - old_pos,
> >> + adc->cb_priv);
> >> +}
> >> +
> >> +static int stm32_dfsdm_adc_dma_start(struct iio_dev *indio_dev)
> >> +{
> >> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> >> + struct dma_async_tx_descriptor *desc;
> >> + dma_cookie_t cookie;
> >> + int ret;
> >> +
> >> + if (!adc->dma_chan)
> >> + return -EINVAL;
> >> +
> >> + dev_dbg(&indio_dev->dev, "%s size=%d watermark=%d\n", __func__,
> >> + adc->buf_sz, adc->buf_sz / 2);
> >> +
> >> + /* Prepare a DMA cyclic transaction */
> >> + desc = dmaengine_prep_dma_cyclic(adc->dma_chan,
> >> + adc->dma_buf,
> >> + adc->buf_sz, adc->buf_sz / 2,
> >> + DMA_DEV_TO_MEM,
> >> + DMA_PREP_INTERRUPT);
> >> + if (!desc)
> >> + return -EBUSY;
> >> +
> >> + desc->callback = stm32_dfsdm_audio_dma_buffer_done;
> >> + desc->callback_param = indio_dev;
> >> +
> >> + cookie = dmaengine_submit(desc);
> >> + ret = dma_submit_error(cookie);
> >> + if (ret) {
> >> + dmaengine_terminate_all(adc->dma_chan);
> >> + return ret;
> >> + }
> >> +
> >> + /* Issue pending DMA requests */
> >> + dma_async_issue_pending(adc->dma_chan);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
> >> +{
> >> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> >> + int ret;
> >> +
> >> + /* Reset adc buffer index */
> >> + adc->bufi = 0;
> >> +
> >> + ret = stm32_dfsdm_start_dfsdm(adc->dfsdm);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + ret = stm32_dfsdm_start_conv(adc, true);
> >> + if (ret) {
> >> + dev_err(&indio_dev->dev, "Can't start conversion\n");
> >> + goto stop_dfsdm;
> >> + }
> >> +
> >> + if (adc->dma_chan) {
> >> + ret = stm32_dfsdm_adc_dma_start(indio_dev);
> >> + if (ret) {
> >> + dev_err(&indio_dev->dev, "Can't start DMA\n");
> >> + goto err_stop_conv;
> >> + }
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +err_stop_conv:
> >> + stm32_dfsdm_stop_conv(adc);
> >> +stop_dfsdm:
> >> + stm32_dfsdm_stop_dfsdm(adc->dfsdm);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
> >> +{
> >> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> >> +
> >> + if (adc->dma_chan)
> >> + dmaengine_terminate_all(adc->dma_chan);
> >> +
> >> + stm32_dfsdm_stop_conv(adc);
> >> +
> >> + stm32_dfsdm_stop_dfsdm(adc->dfsdm);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static const struct iio_buffer_setup_ops stm32_dfsdm_buffer_setup_ops = {
> >> + .postenable = &stm32_dfsdm_postenable,
> >> + .predisable = &stm32_dfsdm_predisable,
> >> +};
> >> +
> >> +/**
> >> + * stm32_dfsdm_get_buff_cb() - register a callback that will be called when
> >> + * DMA transfer period is achieved.
> >> + *
> >> + * @iio_dev: Handle to IIO device.
> >> + * @cb: Pointer to callback function:
> >> + * - data: pointer to data buffer
> >> + * - size: size in byte of the data buffer
> >> + * - private: pointer to consumer private structure.
> >> + * @private: Pointer to consumer private structure.
> >> + */
> >> +int stm32_dfsdm_get_buff_cb(struct iio_dev *iio_dev,
> >> + int (*cb)(const void *data, size_t size,
> >> + void *private),
> >> + void *private)
> >> +{
> >> + struct stm32_dfsdm_adc *adc;
> >> +
> >> + if (!iio_dev)
> >> + return -EINVAL;
> >> + adc = iio_priv(iio_dev);
> >> +
> >> + adc->cb = cb;
> >> + adc->cb_priv = private;
> >> +
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(stm32_dfsdm_get_buff_cb);
> >> +
> >> +/**
> >> + * stm32_dfsdm_release_buff_cb - unregister buffer callback
> >> + *
> >> + * @iio_dev: Handle to IIO device.
> >> + */
> >> +int stm32_dfsdm_release_buff_cb(struct iio_dev *iio_dev)
> >> +{
> >> + struct stm32_dfsdm_adc *adc;
> >> +
> >> + if (!iio_dev)
> >> + return -EINVAL;
> >> + adc = iio_priv(iio_dev);
> >> +
> >> + adc->cb = NULL;
> >> + adc->cb_priv = NULL;
> >> +
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(stm32_dfsdm_release_buff_cb);
> >> +
> >> static int stm32_dfsdm_single_conv(struct iio_dev *indio_dev,
> >> const struct iio_chan_spec *chan, int *res)
> >> {
> >> @@ -453,15 +767,41 @@ static int stm32_dfsdm_write_raw(struct iio_dev *indio_dev,
> >> {
> >> struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> >> struct stm32_dfsdm_filter *fl = &adc->dfsdm->fl_list[adc->fl_id];
> >> + struct stm32_dfsdm_channel *ch = &adc->dfsdm->ch_list[adc->ch_id];
> >> + unsigned int spi_freq = adc->spi_freq;
> >> int ret = -EINVAL;
> >>
> >> - if (mask == IIO_CHAN_INFO_OVERSAMPLING_RATIO) {
> >> + switch (mask) {
> >> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> >> ret = stm32_dfsdm_set_osrs(fl, 0, val);
> >> if (!ret)
> >> adc->oversamp = val;
> >> +
> >> + return ret;
> >> +
> >> + case IIO_CHAN_INFO_SAMP_FREQ:
> >> + if (!val)
> >> + return -EINVAL;
> >> + if (ch->src != DFSDM_CHANNEL_SPI_CLOCK_EXTERNAL)
> >> + spi_freq = adc->dfsdm->spi_master_freq;
> >> +
> >> + if (spi_freq % val)
> >> + dev_warn(&indio_dev->dev,
> >> + "Sampling rate not accurate (%d)\n",
> >> + spi_freq / (spi_freq / val));
> >> +
> >> + ret = stm32_dfsdm_set_osrs(fl, 0, (spi_freq / val));
> >> + if (ret < 0) {
> >> + dev_err(&indio_dev->dev,
> >> + "Not able to find parameter that match!\n");
> >> + return ret;
> >> + }
> >> + adc->sample_freq = val;
> >> +
> >> + return 0;
> >> }
> >>
> >> - return ret;
> >> + return -EINVAL;
> >> }
> >>
> >> static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
> >> @@ -494,11 +834,22 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
> >> *val = adc->oversamp;
> >>
> >> return IIO_VAL_INT;
> >> +
> >> + case IIO_CHAN_INFO_SAMP_FREQ:
> >> + *val = adc->sample_freq;
> >> +
> >> + return IIO_VAL_INT;
> >> }
> >>
> >> return -EINVAL;
> >> }
> >>
> >> +static const struct iio_info stm32_dfsdm_info_audio = {
> >> + .hwfifo_set_watermark = stm32_dfsdm_set_watermark,
> >> + .read_raw = stm32_dfsdm_read_raw,
> >> + .write_raw = stm32_dfsdm_write_raw,
> >> +};
> >> +
> >> static const struct iio_info stm32_dfsdm_info_adc = {
> >> .read_raw = stm32_dfsdm_read_raw,
> >> .write_raw = stm32_dfsdm_write_raw,
> >> @@ -531,6 +882,60 @@ static irqreturn_t stm32_dfsdm_irq(int irq, void *arg)
> >> return IRQ_HANDLED;
> >> }
> >>
> >> +/*
> >> + * Define external info for SPI Frequency and audio sampling rate that can be
> >> + * configured by ASoC driver through consumer.h API
> >> + */
> >> +static const struct iio_chan_spec_ext_info dfsdm_adc_audio_ext_info[] = {
> >> + /* spi_clk_freq : clock freq on SPI/manchester bus used by channel */
> >> + {
> >> + .name = "spi_clk_freq",
> >> + .shared = IIO_SHARED_BY_TYPE,
> >> + .read = dfsdm_adc_audio_get_spiclk,
> >> + .write = dfsdm_adc_audio_set_spiclk,
> >> + },
> >> + {},
> >> +};
> >> +
> >> +static int stm32_dfsdm_dma_request(struct iio_dev *indio_dev)
> >> +{
> >> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> >> + struct dma_slave_config config;
> >> + int ret;
> >> +
> >> + adc->dma_chan = dma_request_slave_channel(&indio_dev->dev, "rx");
> >> + if (!adc->dma_chan)
> >> + return -EINVAL;
> >> +
> >> + adc->rx_buf = dma_alloc_coherent(adc->dma_chan->device->dev,
> >> + DFSDM_DMA_BUFFER_SIZE,
> >> + &adc->dma_buf, GFP_KERNEL);
> >> + if (!adc->rx_buf) {
> >> + ret = -ENOMEM;
> >> + goto err_release;
> >> + }
> >> +
> >> + /* Configure DMA channel to read data register */
> >> + memset(&config, 0, sizeof(config));
> >> + config.src_addr = (dma_addr_t)adc->dfsdm->phys_base;
> >> + config.src_addr += DFSDM_RDATAR(adc->fl_id);
> >> + config.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> >> +
> >> + ret = dmaengine_slave_config(adc->dma_chan, &config);
> >> + if (ret)
> >> + goto err_free;
> >> +
> >> + return 0;
> >> +
> >> +err_free:
> >> + dma_free_coherent(adc->dma_chan->device->dev, DFSDM_DMA_BUFFER_SIZE,
> >> + adc->rx_buf, adc->dma_buf);
> >> +err_release:
> >> + dma_release_channel(adc->dma_chan);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
> >> struct iio_chan_spec *ch)
> >> {
> >> @@ -551,7 +956,12 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
> >> ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> >> ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO);
> >>
> >> - ch->scan_type.sign = 'u';
> >> + if (adc->dev_data->type == DFSDM_AUDIO) {
> >> + ch->scan_type.sign = 's';
> >> + ch->ext_info = dfsdm_adc_audio_ext_info;
> >> + } else {
> >> + ch->scan_type.sign = 'u';
> >> + }
> >> ch->scan_type.realbits = 24;
> >> ch->scan_type.storagebits = 32;
> >> adc->ch_id = ch->channel;
> >> @@ -560,6 +970,64 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
> >> &adc->dfsdm->ch_list[ch->channel]);
> >> }
> >>
> >> +static int stm32_dfsdm_audio_init(struct iio_dev *indio_dev)
> >> +{
> >> + struct iio_chan_spec *ch;
> >> + struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> >> + struct stm32_dfsdm_channel *d_ch;
> >> + int ret;
> >> +
> >> + ret = stm32_dfsdm_dma_request(indio_dev);
> >> + if (ret) {
> >> + dev_err(&indio_dev->dev, "DMA request failed\n");
> >> + return ret;
> >> + }
> >> +
> >> + indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
> >> +
> >> + ret = iio_triggered_buffer_setup(indio_dev,
> >> + &iio_pollfunc_store_time,
> >> + NULL,
> >> + &stm32_dfsdm_buffer_setup_ops);
> >> + if (ret) {
> >> + dev_err(&indio_dev->dev, "Buffer setup failed\n");
> >> + goto err_dma_disable;
> >> + }
> >> +
> >> + ch = devm_kzalloc(&indio_dev->dev, sizeof(*ch), GFP_KERNEL);
> >> + if (!ch)
> >> + return -ENOMEM;
> >> +
> >> + ch->scan_index = 0;
> >> + ret = stm32_dfsdm_adc_chan_init_one(indio_dev, ch);
> >> + if (ret < 0) {
> >> + dev_err(&indio_dev->dev, "channels init failed\n");
> >> + goto err_buffer_cleanup;
> >> + }
> >> + ch->info_mask_separate = BIT(IIO_CHAN_INFO_SAMP_FREQ);
> >> +
> >> + d_ch = &adc->dfsdm->ch_list[adc->ch_id];
> >> + if (d_ch->src != DFSDM_CHANNEL_SPI_CLOCK_EXTERNAL)
> >> + adc->spi_freq = adc->dfsdm->spi_master_freq;
> >> +
> >> + indio_dev->num_channels = 1;
> >> + indio_dev->channels = ch;
> >> +
> >> + return 0;
> >> +
> >> +err_buffer_cleanup:
> >> + iio_triggered_buffer_cleanup(indio_dev);
> >> +
> >> +err_dma_disable:
> >> + if (adc->dma_chan) {
> >> + dma_free_coherent(adc->dma_chan->device->dev,
> >> + DFSDM_DMA_BUFFER_SIZE,
> >> + adc->rx_buf, adc->dma_buf);
> >> + dma_release_channel(adc->dma_chan);
> >> + }
> >> + return ret;
> >> +}
> >> +
> >> static int stm32_dfsdm_adc_init(struct iio_dev *indio_dev)
> >> {
> >> struct iio_chan_spec *ch;
> >> @@ -612,11 +1080,20 @@ static const struct stm32_dfsdm_dev_data stm32h7_dfsdm_adc_data = {
> >> .init = stm32_dfsdm_adc_init,
> >> };
> >>
> >> +static const struct stm32_dfsdm_dev_data stm32h7_dfsdm_audio_data = {
> >> + .type = DFSDM_AUDIO,
> >> + .init = stm32_dfsdm_audio_init,
> >> +};
> >> +
> >> static const struct of_device_id stm32_dfsdm_adc_match[] = {
> >> {
> >> .compatible = "st,stm32-dfsdm-adc",
> >> .data = &stm32h7_dfsdm_adc_data,
> >> },
> >> + {
> >> + .compatible = "st,stm32-dfsdm-dmic",
> >> + .data = &stm32h7_dfsdm_audio_data,
> >> + },
> >> {}
> >> };
> >>
> >> @@ -667,8 +1144,13 @@ static int stm32_dfsdm_adc_probe(struct platform_device *pdev)
> >> name = devm_kzalloc(dev, sizeof("dfsdm-adc0"), GFP_KERNEL);
> >> if (!name)
> >> return -ENOMEM;
> >> - iio->info = &stm32_dfsdm_info_adc;
> >> - snprintf(name, sizeof("dfsdm-adc0"), "dfsdm-adc%d", adc->fl_id);
> >> + if (dev_data->type == DFSDM_AUDIO) {
> >> + iio->info = &stm32_dfsdm_info_audio;
> >> + snprintf(name, sizeof("dfsdm-pdm0"), "dfsdm-pdm%d", adc->fl_id);
> >> + } else {
> >> + iio->info = &stm32_dfsdm_info_adc;
> >> + snprintf(name, sizeof("dfsdm-adc0"), "dfsdm-adc%d", adc->fl_id);
> >> + }
> >> iio->name = name;
> >>
> >> /*
> >> @@ -700,7 +1182,10 @@ static int stm32_dfsdm_adc_probe(struct platform_device *pdev)
> >> if (ret < 0)
> >> return ret;
> >>
> >> - return iio_device_register(iio);
> >> + iio_device_register(iio);
> >> + if (dev_data->type == DFSDM_AUDIO)
> >> + return devm_of_platform_populate(&pdev->dev);
> >> + return 0;
> >> }
> >>
> >> static int stm32_dfsdm_adc_remove(struct platform_device *pdev)
> >> @@ -709,7 +1194,14 @@ static int stm32_dfsdm_adc_remove(struct platform_device *pdev)
> >> struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> >>
> >> iio_device_unregister(indio_dev);
> >> -
> >> + if (indio_dev->pollfunc)
> >> + iio_triggered_buffer_cleanup(indio_dev);
> >> + if (adc->dma_chan) {
> >> + dma_free_coherent(adc->dma_chan->device->dev,
> >> + DFSDM_DMA_BUFFER_SIZE,
> >> + adc->rx_buf, adc->dma_buf);
> >> + dma_release_channel(adc->dma_chan);
> >> + }
> >> return 0;
> >> }
> >>
> >> diff --git a/include/linux/iio/adc/stm32-dfsdm-adc.h b/include/linux/iio/adc/stm32-dfsdm-adc.h
> >> new file mode 100644
> >> index 0000000..e7dc7a5
> >> --- /dev/null
> >> +++ b/include/linux/iio/adc/stm32-dfsdm-adc.h
> >> @@ -0,0 +1,18 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * This file discribe the STM32 DFSDM IIO driver API for audio part
> >> + *
> >> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> >> + * Author(s): Arnaud Pouliquen <arnaud.pouliquen@st.com>.
> >> + */
> >> +
> >> +#ifndef STM32_DFSDM_ADC_H
> >> +#define STM32_DFSDM_ADC_H
> >> +
> >> +int stm32_dfsdm_get_buff_cb(struct iio_dev *iio_dev,
> >> + int (*cb)(const void *data, size_t size,
> >> + void *private),
> >> + void *private);
> >> +int stm32_dfsdm_release_buff_cb(struct iio_dev *iio_dev);
> >> +
> >> +#endif
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] KVM: arm/arm64: don't set vtimer->cnt_ctl in kvm_arch_timer_handler
From: Christoffer Dall @ 2017-12-13 9:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513148407-2611-1-git-send-email-hejianet@gmail.com>
On Tue, Dec 12, 2017 at 11:00:07PM -0800, Jia He wrote:
> In our Armv8a server (qualcomm Amberwing, non VHE), after applying
> Christoffer's timer optimizing patchset(Optimize arch timer register
> handling), the guest is hang during kernel booting.
>
> The error root cause might be as follows:
> 1. in kvm_arch_timer_handler, it reset vtimer->cnt_ctl with current
> cntv_ctl register value. And then it missed some cases to update timer's
> irq (irq.level) when kvm_timer_irq_can_fire() is false
Why should it set the irq level to true when the timer cannot fire?
> 2. It causes kvm_vcpu_check_block return 0 instead of -EINTR
> kvm_vcpu_check_block
> kvm_cpu_has_pending_timer
> kvm_timer_is_pending
> kvm_timer_should_fire
> 3. Thus, the kvm hyp code can not break the loop in kvm_vcpu_block (halt
> poll process) and the guest is hang forever
This is just a polling loop which will expire after some time, so it
shouldn't halt the guest indefinitely, but merely slow it down for some
while, if we have a bug. Is that the behavior you're seeing or are you
seeing the guest coming to a complete halt?
>
> Fixes: b103cc3f10c0 ("KVM: arm/arm64: Avoid timer save/restore in vcpu entry/exit")
> Signed-off-by: Jia He <jia.he@hxt-semitech.com>
> ---
> virt/kvm/arm/arch_timer.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index f9555b1..bb86433 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -100,7 +100,6 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
> vtimer = vcpu_vtimer(vcpu);
>
> if (!vtimer->irq.level) {
> - vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
This fix is clearly not correct, as it would prevent forwarding timer
interrupts in some cases.
> if (kvm_timer_irq_can_fire(vtimer))
> kvm_timer_update_irq(vcpu, true, vtimer);
> }
> --
> 2.7.4
>
I actually don't see how the above scenario you painted can happen.
If you're in the polling loop, that means that the timer state is loaded
on the vcpu, and that means you can take interrupts from the timer, and
when you take interrupts, you will set the irq.level.
And here's the first bit of logic in kvm_timer_is_pending():
if (vtimer->irq.level || ptimer->irq.level)
return true;
So that would break the loop.
I'm not able to reproduce on my side with a non-VHE platform.
What is the workload you're running to reproduce this, and what is the
exact kernel tree and kernel configuration you're using?
Thanks,
-Christoffer
^ permalink raw reply
* [PATCH v5 1/8] clocksource: dmtimer: Remove all the exports
From: Ladislav Michl @ 2017-12-13 9:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171212182150.GF14441@atomide.com>
On Tue, Dec 12, 2017 at 10:21:50AM -0800, Tony Lindgren wrote:
> * Ladislav Michl <ladis@linux-mips.org> [171212 18:06]:
> > I do not follow. Each general-purpose timer module has its own interrupt line,
> > so claiming that irq directly using request_irq seems enough. Could you
> > explain interrupt controller idea a bit more?
>
> Well let's assume we have drivers/clocksource/timer-dm.c implement
> an irq controller. Then the pwm driver would just do:
>
> pwm9: dmtimer-pwm {
> compatible = "ti,omap-dmtimer-pwm";
> #pwm-cells = <3>;
> ti,timers = <&timer9>;
> ti,clock-source = <0x00>; /* timer_sys_ck */
> interrupts-extended = <&timer9 IRQ_TYPE_SOMETHING>;
> };
>
> Then you can do whatever you need to in the pwm driver with
> enable_irq/disable_irq + a handler?
That seems to work. Now should we map 1:1 to timer interrupt or
have separate interrupt for match, overflow and capture?
Former would need some more dm_timer_ops to determine interrupt
source, while later would work "automagically" - but I haven't
tested it yet.
> If reading the line status is needed.. Then maybe the GPIO framework
> needs to have hardware timer support instead?
It does not seem OMAP can read event pin value in event capture mode.
> Anyways, just thinking out loud how we could have a Linux generic
> hardware timer framework that drivers like pwm could then use.
I need a bit longer chain:
dmtimer -> pwm -> rc (which calls ir_raw_event_store from interrupt)
Is extending pwm core with interrpt callback the right thing there?
Something like:
(*pulse_captured)(ktime_t width, ktime_t last_edge);
Thank you,
ladis
^ permalink raw reply
* [PATCH v2 3/4] thermal: armada: add support for CP110
From: Gregory CLEMENT @ 2017-12-13 9:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171213083848.j5het742yavxvkkw@sapphire.tkos.co.il>
Hi Baruch,
On mer., d?c. 13 2017, Baruch Siach <baruch@tkos.co.il> wrote:
> Hi Gregory,
>
> On Mon, Dec 11, 2017 at 06:02:49PM +0100, Gregory CLEMENT wrote:
>> On lun., d?c. 11 2017, Baruch Siach <baruch@tkos.co.il> wrote:
>> > On Mon, Dec 11, 2017 at 04:09:32PM +0100, Miquel RAYNAL wrote:
>> >> On Sun, 3 Dec 2017 13:11:23 +0200
>> >> Baruch Siach <baruch@tkos.co.il> wrote:
>> >>
>> >> > The CP110 component is integrated in the Armada 8k and 7k lines of
>> >> > processors.
>> >> >
>> >> > This patch also adds an option of offset to the MSB of the control
>> >> > register. The existing DT binding for Armada 38x refers to a single
>> >> > 32 bit control register. It turns out that this is actually only the
>> >> > MSB of the control area. Changing the binding to fix that would break
>> >> > existing DT files, so the Armada 38x binding is left as is.
>> >> >
>> >> > The new CP110 binding increases the size of the control area to 64
>> >> > bits, thus moving the MSB to offset 4.
>> >> >
>> >> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>> >> > ---
>> >> > v2: No change
>> >> > ---
>> >> > drivers/thermal/armada_thermal.c | 24 ++++++++++++++++++++++--
>> >> > 1 file changed, 22 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/drivers/thermal/armada_thermal.c
>> >> > b/drivers/thermal/armada_thermal.c index 0eb82097571f..59b75f63945d
>> >> > 100644 --- a/drivers/thermal/armada_thermal.c
>> >> > +++ b/drivers/thermal/armada_thermal.c
>> >> > @@ -73,6 +73,7 @@ struct armada_thermal_data {
>> >> > unsigned int temp_shift;
>> >> > unsigned int temp_mask;
>> >> > unsigned int is_valid_shift;
>> >> > + unsigned int control_msb_offset;
>> >> > };
>> >> >
>> >> > static void armadaxp_init_sensor(struct platform_device *pdev,
>> >> > @@ -142,12 +143,14 @@ static void armada375_init_sensor(struct
>> >> > platform_device *pdev, static void armada380_init_sensor(struct
>> >> > platform_device *pdev, struct armada_thermal_priv *priv)
>> >> > {
>> >> > - unsigned long reg = readl_relaxed(priv->control);
>> >> > + void __iomem *control_msb =
>> >> > + priv->control + priv->data->control_msb_offset;
>> >> > + unsigned long reg = readl_relaxed(control_msb);
>> >> >
>> >> > /* Reset hardware once */
>> >> > if (!(reg & A380_HW_RESET)) {
>> >> > reg |= A380_HW_RESET;
>> >> > - writel(reg, priv->control);
>> >> > + writel(reg, control_msb);
>> >> > mdelay(10);
>> >> > }
>> >> > }
>> >> > @@ -266,6 +269,19 @@ static const struct armada_thermal_data
>> >> > armada_ap806_data = { .signed_sample = true,
>> >> > };
>> >> >
>> >> > +static const struct armada_thermal_data armada_cp110_data = {
>> >> > + .is_valid = armada_is_valid,
>> >> > + .init_sensor = armada380_init_sensor,
>> >>
>> >> I see the initialization for CP110 thermal IP is close to
>> >> Armada-380's, but, as you point it in the commit log it is still
>> >> different.
>> >>
>> >> I don't know what is the best way to handle this but until now each
>> >> new compatible had his own ->init_sensor function, shouldn't we do
>> >> the same here as changes are requested? This would naturally avoid the
>> >> situation with Armada-380 bindings.
>> >
>> > I'm not sure I understand your suggestion.
>> >
>> > There is no difference between the CP110 and the Armada 38x, as far as I can
>> > see. The only quirk is that the existing Armada 38x DT binding is wrong I that
>> > the 'reg' property references the control MSB, while leaving the LSB
>> > out. We
>>
>> Well I would not say it was wrong but more incomplete :)
>>
>> > can't change the Armada 38x binding without breaking existing DTs. The
>> > 'control_msb_offset' field that this patch adds allows correct binding for
>> > CP110, while keeping compatibility with the existing Armada 38x
>> > binding.
>>
>> I am not against adding a new compatible string for CP110 but ot be
>> honest the new binding for CP110 does not bring anything as you don't
>> use at all the LSB register.
>
> We don't use the LSB yet in mainline driver. But the vendor kernel uses it to
> "change temperature band gap circuit curve" (quoting vendor kernel commit
> 4ff2d8a7d3 log). Chances are that we want to do this as well. But said commit
> changed the DT binding in an incompatible way. We can't do that, and we both
> agree on that.
>
>> Actually, if on Armada 375 we initially mapped the LSB register it was
>> to support an very early release of the SoC (stepping Z) and only for
>> resetting its value. So I guess you started to write the AP860 part
>> based on the Armada 375 and then found that we could map a more complete
>> range of the registers.
>>
>> > How would a separate init_sensor routine improve things?
>>
>> So yes please do it, thanks to this you won't have to add the
>> control_msb_offset member and can use a clean function. Moreover if in
>> the future we see some usefulness for this LSB register then we could use
>> the new compatible for the Armada 38x.
>
> There are two separate issues here:
>
> 1. DT binding
>
> 2. init_sensor callback implementation
>
> We both agree on #1. The A38x and CP110 need separate compatible strings. In
> case we want to access the LSB control register on Armada 38x, we will need
> yet another compatible string (marvell,armada380-v2-thermal maybe?).
Actually, if it is _compatible_ then we will use the same compatible, ie
"marvell,armadacp110-thermal"
>
> As for #2, I'm all for sharing as much code as possible. I find the vendor
> kernel approach of duplicating the init routines[1] unhelpful as it violates
> the DRY principle. The differences between armada380_init_sensor() and
> cp110_init_sensor() are minor. In my opinion, these differences should be
> expressed explicitly in the armada_thermal_data, in a similar way to my
> suggested control_msb_offset field. The vendor code hides these differences in
> slight variations of duplicated code.
>
> What is the advantage of a separate init routine?
The main advantage is to be able keep the armada380_init_sensor as the
legacy init, and then being able to use the new armadacp110_init_sensor
for the new binding.
Gregory
>
> baruch
>
> [1] https://github.com/MarvellEmbeddedProcessors/linux-marvell/blob/linux-4.4.52-armada-17.10/drivers/thermal/armada_thermal.c
>
> --
> http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
> - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* WARNING: suspicious RCU usage
From: Russell King - ARM Linux @ 2017-12-13 9:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOMZO5DBMpCHfgmNND6=qJD+sxOLMPFqEsyz+GiBVhMzN3H+Zg@mail.gmail.com>
On Tue, Dec 12, 2017 at 04:11:07PM -0200, Fabio Estevam wrote:
> Hi Russell,
>
> On Tue, Dec 12, 2017 at 3:34 PM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
>
> > It's fundamentally unsafe.
> >
> > You need to test with CONFIG_BL_SWITCHER enabled - there's spinlocks
> > in smp_call_function_single() path that are conditional on that symbol.
> > If CONFIG_BL_SWITCHER is disabled, then the spinlocks are not present.
>
> Ok, just tested with CONFIG_BL_SWITCHER=y on a imx6q-cubox-i:
>
> # echo enabled > /sys/class/tty/ttymxc0/power/wakeup
> # echo mem > /sys/power/state
> [ 10.503462] PM: suspend entry (deep)
> [ 10.507479] PM: Syncing filesystems ... done.
> [ 10.555024] Freezing user space processes ... (elapsed 0.002 seconds) done.
> [ 10.564511] OOM killer disabled.
> [ 10.567760] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) d.
> [ 10.577420] Suspending console(s) (use no_console_suspend to debug)
> [ 10.657748] PM: suspend devices took 0.080 seconds
> [ 10.669329] Disabling non-boot CPUs ...
> [ 10.717049] IRQ17 no longer affine to CPU1
> [ 10.837141] Enabling non-boot CPUs ...
> [ 10.839386] CPU1 is up
> [ 10.840342] CPU2 is up
> [ 10.841300] CPU3 is up
> [ 11.113735] mmc0: queuing unknown CIS tuple 0x80 (2 bytes)
> [ 11.115676] mmc0: queuing unknown CIS tuple 0x80 (3 bytes)
> [ 11.117595] mmc0: queuing unknown CIS tuple 0x80 (3 bytes)
> [ 11.121014] mmc0: queuing unknown CIS tuple 0x80 (7 bytes)
> [ 11.124454] mmc0: queuing unknown CIS tuple 0x80 (7 bytes)
> [ 11.177299] ata1: SATA link down (SStatus 0 SControl 300)
> [ 11.181930] PM: resume devices took 0.330 seconds
> [ 11.243729] OOM killer enabled.
> [ 11.246886] Restarting tasks ... done.
> [ 11.253012] PM: suspend exit
Right, one test. What makes this safe, and what does this prove?
It's probably worth quoting a discussion I had with Will Deacon on
this subject back in 2013 - it's on that complete(), but the points
discussed there are entirely relevant to the spinlock in the GIC
code.
imx6 won't see a problem because you have additional synchronisation
between the calling CPU and the dying CPU, so I'm afraid that any
testing done on imx6 is meaningless wrt the safety of Paul's change
from an architecture point of view.
And that's the problem - once that complete() happens, the dying CPU
can have power removed _at any moment_, and that could happen when
the cache line for "cpu_map_lock" in drivers/irqchip/irq-gic.c is
owned by the dying CPU.
If you read the discussion below, that was one of Will's concerns
with using complete() before we nailed down complete() works. I'm
sorry, but I'm not wrapping this...
18:00 < rmk> wildea01: can you think of any reason not to use flush_cache_louis() in cpu_die() ?
18:20 < wildea01> let me see...
18:20 < rmk> what I'm thinking of is:
18:20 < rmk> idle_task_exit();
18:20 < rmk> local_irq_disable();
18:20 < rmk> flush_cache_louis();
18:20 < rmk> mb();
18:20 < rmk> RCU_NONIDLE(complete(&cpu_died));
18:20 < rmk> mb();
18:20 < rmk> if (smp_ops.cpu_die)
18:20 < rmk> smp_ops.cpu_die(cpu);
18:21 < rmk> and then killing most of the flush_cache_all() calls in smp_ops.cpu_die()
18:22 < rmk> the only thing I haven't worked out is why some places disable the L1 cache and then flush - especially as that can make any dirty data in the L1 cache suddenly vanish from the CPUs visibility
18:22 < wildea01> might need to be careful with the completion
18:23 < rmk> that's why the mb() is there - another CPU will read the cpu_died thing which means it must have become visible to the other CPUs
18:24 < wildea01> but the cacheline could still be exclusive in our L1 I think
18:24 < rmk> how? surely it must have become shared because another CPU has read from it?
18:25 < wildea01> I'm thinking of the spin lock -- can we guarantee that another core will have read that before we turn off our cache?
18:27 < rmk> well, can we get out of wait_for_completion without the spin lock having been unlocked?
18:27 < rmk> one of the points of completions is that it should be safe for stuff like this
18:59 < rmk> yes, one of the things that wait_for_completion/complete was invented for was for synchronising a kernel thread exit with cleaning up after it
19:01 < rmk> and if you look at the above functions, the spinlock can't be owned by the CPU calling complete() because wait_for_completion() must reacquire it after complete() has woken the wait_for_completion thread u p
19:04 < rmk> well, I just tried it out on omap4430 and it seems to work fine
19:04 < wildea01> rmk: but complete does a spin_unlock_irqrestore(&x->wait.lock, flags);, now if that sits exclusive in our cache and we power-off before the waiting CPU gets the lock, then we're dead
19:04 < rmk> yes it does, but...
19:04 < wildea01> maybe that's so incredibly unlikely that we don't mind
19:04 < rmk> the other CPU must exit wait_for_completion()
19:05 < rmk> which involves reading/writing that lock too
19:05 < rmk> spin_lock_irq(&x->wait.lock);
19:05 < rmk> } while (!x->done && timeout);
19:05 < rmk> __remove_wait_queue(&x->wait, &wait);
19:05 < rmk> }
19:05 < rmk> x->done--;
19:05 < rmk> return timeout ?: 1;
19:05 < rmk> spin_unlock_irq(&x->wait.lock);
19:06 < wildea01> hmm, where is that code?
19:06 < rmk> will all be executed on another CPU after that spin_unlock
19:06 < rmk> wait_for_completion->wait_for_common->__wait_for_common->do_wait_for_common
19:07 < wildea01> gotcha, I didn't go as far as do_wait_for_common
19:07 * wildea01 scrolls up
19:07 < rmk> the bits I quoted is the exit path from do_wait_for_common back to wait_for_completion
19:15 < wildea01> I guess the only bit I'm missing is why the the other CPU must exit wait_for_completion before we can proceed with the cpu_die
19:16 < rmk> wrong way round.
19:16 < rmk> complete() must exit completely and be visible before wait_for_completion can return
19:17 < rmk> so there's no way that platform_cpu_kill() can end up being called before that complete() has unlocked that spinlock
19:17 < rmk> and as platform_cpu_kill() is what should be removing power to the dead CPU
19:17 < wildea01> but I don't see how we can guarantee that the other guy has read it
19:17 < wildea01> he might be *just* about to read it
19:18 < wildea01> but it might not have happened
19:18 < rmk> if (!wait_for_completion_timeout(&cpu_died, msecs_to_jiffies(5000))) {
19:18 < rmk> }
19:18 < rmk> printk(KERN_NOTICE "CPU%u: shutdown\n", cpu);
19:18 < rmk> you can't get to that printk until complete() has unlocked the completions spinlock.
19:18 < wildea01> agreed
19:18 < rmk> and that unlock has become visible to the CPU executing the above code
19:19 < wildea01> wait a second: I'm assuming that cpu_die is killing the lights, which some people seem to do iirc?
19:20 < wildea01> if that's all done in platform_cpu_kill, then I think we're ok19:20 < wildea01> as you say
19:20 < rmk> indeed.
19:20 < rmk> it actually depends on how the offlining works.
19:20 < wildea01> so the question is: are there smp_ops.cpu_die which hit the power controller?
19:20 < rmk> yes, but weirdly... because the CPU goes into dead mode when it executes the WFI
19:21 < wildea01> yeah, there's some signal that goes high when that happens, so people like to tie that to the power switch
19:22 < rmk> but that is also fine, because if that's what triggers the power off, we will get there anyway (because platform_cpu_kill won't do that)
19:22 < rmk> err, won't kill the power
19:24 < wildea01> hmm, not sure I follow
19:24 < wildea01> highbank does what you're saying, so we could take that as an example
19:25 < wildea01> (pretending that the cache flush isn't there)
19:26 < rmk> ok.
19:26 < rmk> so what will happen is...
19:27 < rmk> one CPU (not the dying CPU) will end up calling __cpu_die()
19:27 < rmk> the dying CPU will call cpu_die()
19:27 < wildea01> yup
19:27 < rmk> lets call the first one the calling CPU (even though it may not be)
19:28 < wildea01> sure -- it's the guy waiting for the offline to happen
19:28 < rmk> the calling CPU calls into wait_for_completion_timeout() and sits there waiting for the dying CPU to call that complete()
19:28 < rmk> meanwhile, the dying CPU does the idle task exit, disables IRQs, and flushes its caches of any dirty data.
19:29 < rmk> now, the calling CPU has had to take the completion's spinlock, check the counter, release the spinlock, and is waiting for the completion...
19:30 < rmk> so, the dying CPU takes the spinlock, increments the counter, and triggers a wakeup of the calling CPU, and then releases the spinlock.
19:30 < wildea01> yep
19:30 < rmk> the dying CPU now has dirty cache lines again which it probably exclusively owns
19:30 < wildea01> at this point, can we assume that the calling CPU goes off to handle an interrupt or something?
19:31 < rmk> it can do, it could even be the scheduler IPI
19:31 < wildea01> good
19:31 < wildea01> so the dying CPU can proceed past the complete(, with the calling CPU occupied somewhere else
19:31 < rmk> it can do, yes.
19:32 < wildea01> and off into smp_ops.cpu_die => highbank_cpu_die
19:32 < rmk> *OH*, I see what you're getting at
19:32 < wildea01> :)
19:33 < rmk> hmm, how can we get around that...
19:34 < wildea01> it's tricky, because platform_cpu_kill runs on the caller cpu
19:34 < rmk> because, in the case where the power is cut from platform_cpu_kill(), the dying CPU can loose power at any point after that complete()
19:34 < wildea01> so we'd need to split up the `next wfi on core n should kill it' from the `here's my wfi'
19:36 < rmk> I think we could do another flush_cache_louis() after it
19:37 < rmk> if, in the case of platform_cpu_kill() doing the killing of the CPU, that should be fine.
19:37 < wildea01> actually... why do we even need the one before it?
19:37 < wildea01> why not just have one after the complete has returned?
19:37 < rmk> because if platform_cpu_kill() is the call which removes the power, we need to ensure the cache contents have been written out
19:38 < wildea01> ah yeah, I was thinking of requiring both the kill and the die in order for the powerdown, but that's not alwasy necessary
19:38 < wildea01> *always
19:38 < wildea01> last time I checked, nobody used plaform_cpu_kill
19:39 < rmk> umm, lots do
19:39 < rmk> and some do use it to do stuff
19:39 < wildea01> damn, I must've been thinking of something else
19:40 < rmk> well, imx and tegra seem to, but they have their own custom waits implemented probably because of the lack of cache handling in the generic code
19:42 < wildea01> I don't know enough about tegra to understand why their kill and die don't race
19:44 < rmk> ok, looking at the locking barrier doc, we don't need the mb() after the complete() call
19:44 < rmk> but I think to address your concern, we must have another flush_cache_louis() there
19:44 < wildea01> yeah, the unlock should give you that mb
19:45 < wildea01> just seems a shame to have two flushes when they're not usually both needed
19:45 < wildea01> (he assumes)
19:45 < wildea01> like I said, the tegra could looks broken to me
19:45 < wildea01> *code
19:45 < rmk> if we had a louis() version which could flush the cpu_died completion...
19:51 < wildea01> do we even need the mb before the complete?
19:52 < rmk> I've been debating about that, and I think not
19:52 < rmk> I'm just augmenting this with comments as well
19:53 < wildea01> good thinking, I think we've established that it's not simple to understand!
19:59 < rmk> http://www.home.arm.linux.org.uk/~rmk/misc/smp-hotplug.diff
19:59 < rmk> new version of it with lots of comments :)
20:03 < wildea01> looks good to me. The additional flush is a pity, but required, and it's hotplug-off anyway, so not exactly speedy
20:03 < wildea01> one typo in a comment: s/loosing/losing/
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply
* [PATCH v2 3/4] thermal: armada: add support for CP110
From: Baruch Siach @ 2017-12-13 9:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171213095501.1988749d@xps13>
Hi Miquel,
On Wed, Dec 13, 2017 at 09:55:01AM +0100, Miquel RAYNAL wrote:
> > > > How would a separate init_sensor routine improve things?
> > >
> > > So yes please do it, thanks to this you won't have to add the
> > > control_msb_offset member and can use a clean function. Moreover if
> > > in the future we see some usefulness for this LSB register then we
> > > could use the new compatible for the Armada 38x.
> >
> > There are two separate issues here:
> >
> > 1. DT binding
> >
> > 2. init_sensor callback implementation
> >
> > We both agree on #1. The A38x and CP110 need separate compatible
> > strings. In case we want to access the LSB control register on Armada
> > 38x, we will need yet another compatible string
> > (marvell,armada380-v2-thermal maybe?).
> >
> > As for #2, I'm all for sharing as much code as possible. I find the
> > vendor kernel approach of duplicating the init routines[1] unhelpful
> > as it violates the DRY principle. The differences between
> > armada380_init_sensor() and cp110_init_sensor() are minor. In my
> > opinion, these differences should be expressed explicitly in the
> > armada_thermal_data, in a similar way to my suggested
> > control_msb_offset field. The vendor code hides these differences in
> > slight variations of duplicated code.
> >
> > What is the advantage of a separate init routine?
>
> The advantage is that is the very near future I plan to add the
> overheat interrupt only on CP110 (not on 38x) and this needs some
> initialization. So if we don't make different routines now, I will
> have to do it right after.
I don't think so. The code of these functions in the vendor kernel overheat
support implementation is the same, duplicated. The variations are only in
registers/bits offsets. A single routine with one or two added
armada_thermal_data fields would be much easier to comprehend and maintain.
> What would be fine is to have the shared code in a separate function,
> like it is done in Marvell kernel. What do you think about that?
The Marvell code does not "share" the code. Separate functions means
duplicated code that obscures the hardware details, making maintenance harder
on the long run.
https://en.wikipedia.org/wiki/Don%27t_repeat_yourself
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
^ permalink raw reply
* [PATCH] KVM: arm/arm64: don't set vtimer->cnt_ctl in kvm_arch_timer_handler
From: Auger Eric @ 2017-12-13 9:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <b2d0234c-e763-d4c1-02f7-c50cd0914b13@arm.com>
Marc,
On 13/12/17 09:56, Marc Zyngier wrote:
> Hi Jia,
>
> On 13/12/17 07:00, Jia He wrote:
>> In our Armv8a server (qualcomm Amberwing, non VHE), after applying
>> Christoffer's timer optimizing patchset(Optimize arch timer register
>> handling), the guest is hang during kernel booting.
>>
>> The error root cause might be as follows:
>> 1. in kvm_arch_timer_handler, it reset vtimer->cnt_ctl with current
>> cntv_ctl register value. And then it missed some cases to update timer's
>> irq (irq.level) when kvm_timer_irq_can_fire() is false
>> 2. It causes kvm_vcpu_check_block return 0 instead of -EINTR
>> kvm_vcpu_check_block
>> kvm_cpu_has_pending_timer
>> kvm_timer_is_pending
>> kvm_timer_should_fire
>> 3. Thus, the kvm hyp code can not break the loop in kvm_vcpu_block (halt
>> poll process) and the guest is hang forever
>>
>> Fixes: b103cc3f10c0 ("KVM: arm/arm64: Avoid timer save/restore in vcpu entry/exit")
>> Signed-off-by: Jia He <jia.he@hxt-semitech.com>
>> ---
>> virt/kvm/arm/arch_timer.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index f9555b1..bb86433 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -100,7 +100,6 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
>> vtimer = vcpu_vtimer(vcpu);
>>
>> if (!vtimer->irq.level) {
>> - vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
>> if (kvm_timer_irq_can_fire(vtimer))
>> kvm_timer_update_irq(vcpu, true, vtimer);
>> }
>>
>
> Which patches are you looking at? The current code in mainline looks
> like this:
>
> vtimer = vcpu_vtimer(vcpu);
>
> vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
> if (kvm_timer_irq_can_fire(vtimer))
> kvm_timer_update_irq(vcpu, true, vtimer);
>
> I'd suggest you use mainline and report if this doesn't work
the removal of if (!vtimer->irq.level) test happened in:
[PATCH v7 3/8] KVM: arm/arm64: Don't cache the timer IRQ level
which is not upstream.
Thanks
Eric
>
> Thanks,
>
> M.
>
^ permalink raw reply
* [PATCH] drm: Fix possible deadlock in drm_mode_config_cleanup()
From: Chris Wilson @ 2017-12-13 9:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171213085720.6059-1-m.szyprowski@samsung.com>
Quoting Marek Szyprowski (2017-12-13 08:57:20)
> drm_mode_config_cleanup() might be called from a workqueue context (for
> example in error path handling of deferred probe), so it must not call
> flush_scheduled_work(), because it would deadlock in such case. Replace
> that call with explicit counting of the scheduled connector free works
> and waiting until it reaches zero.
>
> Fixes: a703c55004e1 ("drm: safely free connectors from connector_iter")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> This fixes the issue discussed in the following thread:
> https://www.spinics.net/lists/arm-kernel/msg622332.html
> ---
> drivers/gpu/drm/drm_connector.c | 6 +++++-
> drivers/gpu/drm/drm_mode_config.c | 6 +++++-
> include/drm/drm_mode_config.h | 13 +++++++++++++
> 3 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 0b7e0974e6da..7620ac1ad1b1 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -161,6 +161,8 @@ static void drm_connector_free_work_fn(struct work_struct *work)
>
> drm_mode_object_unregister(dev, &connector->base);
> connector->funcs->destroy(connector);
> + atomic_dec(&dev->mode_config.connector_free_works);
> + wake_up_all(&dev->mode_config.connector_free_queue);
> }
>
> /**
> @@ -552,8 +554,10 @@ EXPORT_SYMBOL(drm_connector_list_iter_begin);
> static void
> drm_connector_put_safe(struct drm_connector *conn)
> {
> - if (refcount_dec_and_test(&conn->base.refcount.refcount))
> + if (refcount_dec_and_test(&conn->base.refcount.refcount)) {
> + atomic_inc(&conn->dev->mode_config.connector_free_works);
> schedule_work(&conn->free_work);
> + }
> }
>
> /**
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 6ffe952142e6..cca443faebd8 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -381,6 +381,7 @@ void drm_mode_config_init(struct drm_device *dev)
> idr_init(&dev->mode_config.tile_idr);
> ida_init(&dev->mode_config.connector_ida);
> spin_lock_init(&dev->mode_config.connector_list_lock);
> + init_waitqueue_head(&dev->mode_config.connector_free_queue);
>
> drm_mode_create_standard_properties(dev);
>
> @@ -431,8 +432,11 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> drm_connector_put(connector);
> }
> drm_connector_list_iter_end(&conn_iter);
> +
> /* connector_iter drops references in a work item. */
> - flush_scheduled_work();
> + wait_event(dev->mode_config.connector_free_queue,
> + !atomic_read(&dev->mode_config.connector_free_works));
> +
> if (WARN_ON(!list_empty(&dev->mode_config.connector_list))) {
> drm_connector_list_iter_begin(dev, &conn_iter);
> drm_for_each_connector_iter(connector, &conn_iter)
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index a0afeb591dcb..83a7db997e83 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -413,6 +413,19 @@ struct drm_mode_config {
> * &struct drm_connector_list_iter to walk this list.
> */
> struct list_head connector_list;
> + /**
> + * @pending_connector_free_works:
> + *
> + * Number of scheduled connector->free_work instances, see
> + * drm_connector_put_safe().
> + */
> + atomic_t connector_free_works;
> + /**
> + * @pending_connector_free_queue:
> + *
> + * Wait queue for waiting until connector->free_work is finished.
> + */
> + wait_queue_head_t connector_free_queue;
Just move from a system_wq to a local wq?
-Chris
^ permalink raw reply
* [PATCH 2/6] ARM: stm32: add initial support for STM32MP157
From: Ludovic BARRE @ 2017-12-13 9:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171212232425.shf56g2uivl57fdp@rob-hp-laptop>
On 12/13/2017 12:24 AM, Rob Herring wrote:
> On Fri, Dec 08, 2017 at 03:11:13PM +0100, Ludovic Barre wrote:
>> From: Ludovic Barre <ludovic.barre@st.com>
>>
>> This patch adds initial support of STM32MP157 microprocessor (MPU)
>> based on Arm Cortex-A7. Under new ARCH_STM32_MPU flag we select the
>> needed Cortex-A infrastructure (like gic, timer,...)
>>
>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
>> ---
>> Documentation/arm/stm32/stm32mp157-overview.txt | 12 ++++++++++++
>> Documentation/devicetree/bindings/arm/stm32.txt | 1 +
>
> Please split bindings to separate patches.
OK, I will split stm32.txt in separate commit
>
>> arch/arm/mach-stm32/Kconfig | 22 ++++++++++++++++++++--
>> arch/arm/mach-stm32/Makefile | 1 +
>> arch/arm/mach-stm32/board-mpu-dt.c | 16 ++++++++++++++++
>> 5 files changed, 50 insertions(+), 2 deletions(-)
>> create mode 100644 Documentation/arm/stm32/stm32mp157-overview.txt
>> create mode 100644 arch/arm/mach-stm32/board-mpu-dt.c
>>
>> diff --git a/Documentation/arm/stm32/stm32mp157-overview.txt b/Documentation/arm/stm32/stm32mp157-overview.txt
>> new file mode 100644
>> index 0000000..8a3e7cb
>> --- /dev/null
>> +++ b/Documentation/arm/stm32/stm32mp157-overview.txt
>
> I think new documentation files should be rst format and fit into the
> built documentation. We don't have an SoC description doc for most SoCs.
the existing documentation of stm32 are txt format
-overview.txt
-stm32f429-overwiew.txt
-stm32f746-overview.txt
-stm32h743-overview.txt
what do you prefer:
-omit stm32mp157-overview.txt of this serie and change all in next commit.
-write only this file in rst format
-change all in this serie?
>
>> @@ -0,0 +1,12 @@
>> + STM32MP157 Overview
>> + ===================
>> +
>> + Introduction
>> + ------------
>> + The STM32MP157 is a Cortex-A MPU aimed at various applications.
>> + It features:
>> + - Dual core Cortex-A7 application core
>> + - 2D/3D image composition with GPU
>> + - Standard memories interface support
>> + - Standard connectivity, widely inherited from the STM32 MCU family
>> + - Comprehensive security support
>
> Perhaps make this part of the kconfig entry help.
yes, I could add some details in MACH_STM32MP157 kconfig entry
>
>> diff --git a/Documentation/devicetree/bindings/arm/stm32.txt b/Documentation/devicetree/bindings/arm/stm32.txt
>> index 05762b0..6808ed9 100644
>> --- a/Documentation/devicetree/bindings/arm/stm32.txt
>> +++ b/Documentation/devicetree/bindings/arm/stm32.txt
>> @@ -7,3 +7,4 @@ using one of the following compatible strings:
>> st,stm32f469
>> st,stm32f746
>> st,stm32h743
>> + st,stm32mp157
>> diff --git a/arch/arm/mach-stm32/Kconfig b/arch/arm/mach-stm32/Kconfig
>> index c8059ea..2b227c7 100644
>> --- a/arch/arm/mach-stm32/Kconfig
>> +++ b/arch/arm/mach-stm32/Kconfig
>> @@ -1,12 +1,12 @@
>> menuconfig ARCH_STM32
>> - bool "STMicrolectronics STM32 family" if ARM_SINGLE_ARMV7M
>> + bool "STMicrolectronics STM32 family" if ARM_SINGLE_ARMV7M || ARCH_MULTI_V7
>> select ARCH_HAS_RESET_CONTROLLER
>> select CLKSRC_STM32
>> select PINCTRL
>> select RESET_CONTROLLER
>> select STM32_EXTI
>> help
>> - Support for STMicroelectronics STM32 MCU family
>> + Support for STMicroelectronics STM32 MCU/MPU family
>>
>> if ARCH_STM32
>>
>> @@ -40,4 +40,22 @@ config MACH_STM32H743
>>
>> endif
>>
>> +if ARCH_MULTI_V7
>> +
>> +config ARCH_STM32_MPU
>> + bool "STMicrolectronics STM32 MPU"
>> + default y
>> + select ARM_GIC
>> + select HAVE_ARM_ARCH_TIMER
>> + select ARM_PSCI
>> + help
>> + Support for STMicroelectronics STM32 Microprocessors.
>> +
>> +config MACH_STM32MP157
>
> Is this actually used?
Yes, it's used in pinctrl driver.
>
>> + bool "STMicrolectronics STM32MP157"
>> + depends on ARCH_STM32_MPU
>> + default y
>> +
>> +endif
>> +
>> endif
^ permalink raw reply
* [PATCH 0/5] Add Sound support for iWave RZ/G1M board
From: Simon Horman @ 2017-12-13 9:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513103111-45830-1-git-send-email-biju.das@bp.renesas.com>
On Tue, Dec 12, 2017 at 06:25:06PM +0000, Biju Das wrote:
> This series aims to add sound support for iWave RZ/G1M board.
>
> This patch series has documentation dependency on
> https://patchwork.kernel.org/patch/10108014/
>
> Biju Das (5):
> ARM: shmobile: defconfig: Enable SGTL5000 audio codec
> ARM: dts: r8a7743: Add audio clocks
> ARM: dts: r8a7743: Add audio DMAC support
> ARM: dts: r8a7743: Add sound support
> ARM: dts: iwg20d-q7-common: Enable SGTL5000 audio codec
>
> arch/arm/boot/dts/iwg20d-q7-common.dtsi | 24 +++
> arch/arm/boot/dts/r8a7743.dtsi | 270 ++++++++++++++++++++++++++++++++
> arch/arm/configs/shmobile_defconfig | 1 +
> 3 files changed, 295 insertions(+)
These patches seem clean to me although I do not have sufficient
documentation to properly review the last patch.
I will leave these sit for a few days to allow others to review them.
^ permalink raw reply
* mainline/master boot bisection: v4.15-rc3 on peach-pi #3228-staging
From: Marek Szyprowski @ 2017-12-13 9:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKMK7uF75WFrVMX=7zQyZp=y8GSZBQTMzjLDVf46rYqq=aFumQ@mail.gmail.com>
Hi Daniel,
On 2017-12-12 19:14, Daniel Vetter wrote:
> On Tue, Dec 12, 2017 at 12:38 PM, Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> Hi All,
>>
>>
>> On 2017-12-11 23:28, Javier Martinez Canillas wrote:
>>> [adding Marek and Shuah to cc list]
>>>
>>> On Mon, Dec 11, 2017 at 6:05 PM, Daniel Vetter <daniel.vetter@ffwll.ch>
>>> wrote:
>>>> On Mon, Dec 11, 2017 at 11:30 AM, Guillaume Tucker
>>>> <guillaume.tucker@collabora.com> wrote:
>>>>> Hi Daniel,
>>>>>
>>>>> Please see below, I've had several bisection results pointing at
>>>>> that commit over the week-end on mainline but also on linux-next
>>>>> and net-next. While the peach-pi is a bit flaky at the moment
>>>>> and is likely to have more than one issue, it does seem like this
>>>>> commit is causing some well reproducible kernel hang.
>>>>>
>>>>> Here's a re-run with v4.15-rc3 showing the issue:
>>>>>
>>>>> https://lava.collabora.co.uk/scheduler/job/1018478
>>>>>
>>>>> and here's another one with the change mentioned below reverted:
>>>>>
>>>>> https://lava.collabora.co.uk/scheduler/job/1018479
>>>>>
>>>>> They both show a warning about "unbalanced disables for lcd_vdd",
>>>>> I don't know if this is related as I haven't investigated any
>>>>> further. It does appear to reliably hang with v4.15-rc3 and
>>>>> boot most of the time with the commit reverted though.
>>>>>
>>>>> The automated kernelci.org bisection is still an experimental
>>>>> tool and it may well be a false positive, so please take this
>>>>> result with a pinch of salt...
>>>> The patch just very minimal moves the connector cleanup around (so
>>>> timing change), but except when you unload a driver (or maybe that
>>>> funny EPROBE_DEFER stuff) it shouldn't matter. So if you don't have
>>>> more info than "seems to hang a bit more" I have no idea what's wrong.
>>>> The patch itself should work, at least it survived quite some serious
>>>> testing we do on everything.
>>>> -Daniel
>>>>
>>> Marek was pointing to a different culprit [0] in this [1] thread. I
>>> see that both commits made it to v4.15-rc3, which is the first version
>>> where boot fails. So maybe is a combination of both? Or rather
>>> reverting one patch masks the error in the other.
>>>
>>> I've access to the machine but unfortunately not a lot of time to dig
>>> on this, I could try to do it in the weekend though.
>>
>> After a recent discussion on the Javier's patch:
>> https://patchwork.kernel.org/patch/10106417/
>> I've managed to reproduce this issue also on Exynos5250 based Samsung
>> Snow Chromebook and investigate a bit.
>>
>> It is caused by a deadlock in the main kernel workqueue. Here are details:
>>
>> 1. Exynos DRM fails to initialize due to missing regulators and gets moved
>> to deferred probe device list
>>
>> 2. Deferred probe is triggered and kernel "events" workqueue calls
>> deferred_probe_work_func()
>>
>> 3. exynos_drm_bind() is called, component_bind_all() fails due to missing
>> Exynos Mixer device
>>
>> 4. error handling path is executed in exynos_drm_bind(), which calls
>> drm_mode_config_cleanup()
>>
>> 5. drm_mode_config_cleanup() calls flush_scheduled_work(), what causes
>> deadlock.
>>
>> Do You have idea how to fix this issue properly?
>>
>> Taking a look at git blame, this indeed shows that the issue has been
>> introduced by the commit a703c55004e1 ("drm: safely free connectors from
>> connector_ite"), which added a call to flush_scheduled_work() in
>> drm_mode_config_cleanup().
>>
>> drm_mode_config_cleanup() should avoid calling flush_scheduled_work() if
>> called from the workqueue, but I don't have idea how to check that. The
>> other way of fixing it would be to resurrect separate workqueue for DRM
>> related events.
> We need to flush the work there, or things will go wrong on unload. I
> think the real fix is to make sure that the connector cleanup work
> isn't run on the same work stuff as any driver bind stuff, which yes
> means new separate workqueue just for this.
>
> I guess the simple fix is to do that in the drm, but tbh I'm surprised
> that driver bind/deferred probe hasn't run into this problem anywhere
> else yet.
Well, this means that no-one tested the error paths in deferred probe
case. It's not that surprising, if we assume that typically platform
devices are deferred only once. Second probe() call (which is done from
workqueue) is successful in that case.
I've managed to fix this deadlock without additional workqueue:
https://patchwork.freedesktop.org/patch/193069/
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
* [PATCH] drm: Fix possible deadlock in drm_mode_config_cleanup()
From: Marek Szyprowski @ 2017-12-13 8:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CGME20171213085736eucas1p28d10718330c5c2eac834bd26786c84fc@eucas1p2.samsung.com>
drm_mode_config_cleanup() might be called from a workqueue context (for
example in error path handling of deferred probe), so it must not call
flush_scheduled_work(), because it would deadlock in such case. Replace
that call with explicit counting of the scheduled connector free works
and waiting until it reaches zero.
Fixes: a703c55004e1 ("drm: safely free connectors from connector_iter")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
This fixes the issue discussed in the following thread:
https://www.spinics.net/lists/arm-kernel/msg622332.html
---
drivers/gpu/drm/drm_connector.c | 6 +++++-
drivers/gpu/drm/drm_mode_config.c | 6 +++++-
include/drm/drm_mode_config.h | 13 +++++++++++++
3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 0b7e0974e6da..7620ac1ad1b1 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -161,6 +161,8 @@ static void drm_connector_free_work_fn(struct work_struct *work)
drm_mode_object_unregister(dev, &connector->base);
connector->funcs->destroy(connector);
+ atomic_dec(&dev->mode_config.connector_free_works);
+ wake_up_all(&dev->mode_config.connector_free_queue);
}
/**
@@ -552,8 +554,10 @@ EXPORT_SYMBOL(drm_connector_list_iter_begin);
static void
drm_connector_put_safe(struct drm_connector *conn)
{
- if (refcount_dec_and_test(&conn->base.refcount.refcount))
+ if (refcount_dec_and_test(&conn->base.refcount.refcount)) {
+ atomic_inc(&conn->dev->mode_config.connector_free_works);
schedule_work(&conn->free_work);
+ }
}
/**
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 6ffe952142e6..cca443faebd8 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -381,6 +381,7 @@ void drm_mode_config_init(struct drm_device *dev)
idr_init(&dev->mode_config.tile_idr);
ida_init(&dev->mode_config.connector_ida);
spin_lock_init(&dev->mode_config.connector_list_lock);
+ init_waitqueue_head(&dev->mode_config.connector_free_queue);
drm_mode_create_standard_properties(dev);
@@ -431,8 +432,11 @@ void drm_mode_config_cleanup(struct drm_device *dev)
drm_connector_put(connector);
}
drm_connector_list_iter_end(&conn_iter);
+
/* connector_iter drops references in a work item. */
- flush_scheduled_work();
+ wait_event(dev->mode_config.connector_free_queue,
+ !atomic_read(&dev->mode_config.connector_free_works));
+
if (WARN_ON(!list_empty(&dev->mode_config.connector_list))) {
drm_connector_list_iter_begin(dev, &conn_iter);
drm_for_each_connector_iter(connector, &conn_iter)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index a0afeb591dcb..83a7db997e83 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -413,6 +413,19 @@ struct drm_mode_config {
* &struct drm_connector_list_iter to walk this list.
*/
struct list_head connector_list;
+ /**
+ * @pending_connector_free_works:
+ *
+ * Number of scheduled connector->free_work instances, see
+ * drm_connector_put_safe().
+ */
+ atomic_t connector_free_works;
+ /**
+ * @pending_connector_free_queue:
+ *
+ * Wait queue for waiting until connector->free_work is finished.
+ */
+ wait_queue_head_t connector_free_queue;
/**
* @num_encoder:
*
--
2.15.0
^ permalink raw reply related
* [PATCH] KVM: arm/arm64: don't set vtimer->cnt_ctl in kvm_arch_timer_handler
From: Marc Zyngier @ 2017-12-13 8:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513148407-2611-1-git-send-email-hejianet@gmail.com>
Hi Jia,
On 13/12/17 07:00, Jia He wrote:
> In our Armv8a server (qualcomm Amberwing, non VHE), after applying
> Christoffer's timer optimizing patchset(Optimize arch timer register
> handling), the guest is hang during kernel booting.
>
> The error root cause might be as follows:
> 1. in kvm_arch_timer_handler, it reset vtimer->cnt_ctl with current
> cntv_ctl register value. And then it missed some cases to update timer's
> irq (irq.level) when kvm_timer_irq_can_fire() is false
> 2. It causes kvm_vcpu_check_block return 0 instead of -EINTR
> kvm_vcpu_check_block
> kvm_cpu_has_pending_timer
> kvm_timer_is_pending
> kvm_timer_should_fire
> 3. Thus, the kvm hyp code can not break the loop in kvm_vcpu_block (halt
> poll process) and the guest is hang forever
>
> Fixes: b103cc3f10c0 ("KVM: arm/arm64: Avoid timer save/restore in vcpu entry/exit")
> Signed-off-by: Jia He <jia.he@hxt-semitech.com>
> ---
> virt/kvm/arm/arch_timer.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index f9555b1..bb86433 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -100,7 +100,6 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
> vtimer = vcpu_vtimer(vcpu);
>
> if (!vtimer->irq.level) {
> - vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
> if (kvm_timer_irq_can_fire(vtimer))
> kvm_timer_update_irq(vcpu, true, vtimer);
> }
>
Which patches are you looking at? The current code in mainline looks
like this:
vtimer = vcpu_vtimer(vcpu);
vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
if (kvm_timer_irq_can_fire(vtimer))
kvm_timer_update_irq(vcpu, true, vtimer);
I'd suggest you use mainline and report if this doesn't work.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH v2 3/4] thermal: armada: add support for CP110
From: Miquel RAYNAL @ 2017-12-13 8:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171213083848.j5het742yavxvkkw@sapphire.tkos.co.il>
Hello Baruch,
> > > How would a separate init_sensor routine improve things?
> >
> > So yes please do it, thanks to this you won't have to add the
> > control_msb_offset member and can use a clean function. Moreover if
> > in the future we see some usefulness for this LSB register then we
> > could use the new compatible for the Armada 38x.
>
> There are two separate issues here:
>
> 1. DT binding
>
> 2. init_sensor callback implementation
>
> We both agree on #1. The A38x and CP110 need separate compatible
> strings. In case we want to access the LSB control register on Armada
> 38x, we will need yet another compatible string
> (marvell,armada380-v2-thermal maybe?).
>
> As for #2, I'm all for sharing as much code as possible. I find the
> vendor kernel approach of duplicating the init routines[1] unhelpful
> as it violates the DRY principle. The differences between
> armada380_init_sensor() and cp110_init_sensor() are minor. In my
> opinion, these differences should be expressed explicitly in the
> armada_thermal_data, in a similar way to my suggested
> control_msb_offset field. The vendor code hides these differences in
> slight variations of duplicated code.
>
> What is the advantage of a separate init routine?
The advantage is that is the very near future I plan to add the
overheat interrupt only on CP110 (not on 38x) and this needs some
initialization. So if we don't make different routines now, I will
have to do it right after.
What would be fine is to have the shared code in a separate function,
like it is done in Marvell kernel. What do you think about that?
Thanks,
Miqu?l
^ permalink raw reply
* [PATCH v5 0/4] ARM: ep93xx: ts72xx: Add support for BK3 board
From: Lukasz Majewski @ 2017-12-13 8:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513153607.2439.2.camel@Nokia-N900>
Hi Alexander, Linus
> Hello Linus!
>
> On Wed Dec 13 08:34:22 2017 Linus Walleij <linus.walleij@linaro.org>
> wrote:
> > On Tue, Dec 12, 2017 at 12:36 AM, Lukasz Majewski <lukma@denx.de>
> > wrote: Out of curiosity: Liebherr is obviously doing heavy-duty
> > industrial control systems. Likewise Hartley is doing similar
> > business over at Vision Engravings.
> >
> > Is the situation such that there is a whole bunch of industrial
> > systems out there, in active use and needing future upgrades,
> > that use the EP93xx?
>
> That's definitely the case. I'm as well aware of several thousands of
> industrial devices which are expected to run 24/7 for the next 5
> years at least. And they are updated from time to time.
I can agree with this statement.
>
> > Arnd has been nudging me to do DT conversion for EP93xx
> > so if there are many active industrial users of these
> > I should prioritize it, because these things have 20+ years
> > support cycles.
>
> I'm not sure how important or necessary at all is to change anything
> in these legacy platforms.
+1
>
> > We also need to think about upholding support in GCC for
> > ARMv4(t) for the foreseeable future if there is a big web of
> > random deeply embedded systems out there that will need
> > updates.
>
> But we should definitely preserve at least what we have.
I'm using OE to build toolchain (SDK). I can confirm that gcc 7.2 works
with it.
And yes, armv4 support shall be preserved in GCC ....
>
> --
> Alexander.
>
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20171213/d6dfd68c/attachment-0001.sig>
^ permalink raw reply
* [PATCH V8 0/7] dmaengine: qcom_hidma: add support for bugfixed HW
From: Vinod Koul @ 2017-12-13 8:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513149653-19451-1-git-send-email-okaya@codeaurora.org>
On Wed, Dec 13, 2017 at 02:20:46AM -0500, Sinan Kaya wrote:
> Introduce new ACPI and OF device ids for thw HW along with the helper
> functions.
Applied, thanks
--
~Vinod
^ permalink raw reply
* [PATCH 3/3] ARM: dts: r8a7745: Add CMT SoC specific support
From: Simon Horman @ 2017-12-13 8:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513104579-6333-4-git-send-email-fabrizio.castro@bp.renesas.com>
On Tue, Dec 12, 2017 at 06:49:39PM +0000, Fabrizio Castro wrote:
> Add CMT[01] support to SoC DT.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das@bp.renesas.com>
> ---
> arch/arm/boot/dts/r8a7745.dtsi | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
Please see my review of the r8a7743 patch in this series.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox