Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH] drm/rockchip: Suspend DP late
From: Sean Paul @ 2019-08-05 15:11 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Heiko Stuebner, Tomeu Vizoso, David Airlie, Sandy Huang,
	dri-devel, linux-kernel, linux-rockchip, mka, seanpaul,
	Daniel Vetter, linux-arm-kernel
In-Reply-To: <20190802192629.GX104440@art_vandelay>

On Fri, Aug 02, 2019 at 03:26:29PM -0400, Sean Paul wrote:
> On Fri, Aug 02, 2019 at 11:46:16AM -0700, Douglas Anderson wrote:
> > In commit fe64ba5c6323 ("drm/rockchip: Resume DP early") we moved
> > resume to be early but left suspend at its normal time.  This seems
> > like it could be OK, but casues problems if a suspend gets interrupted
> > partway through.  The OS only balances matching suspend/resume levels.
> > ...so if suspend was called then resume will be called.  If suspend
> > late was called then resume early will be called.  ...but if suspend
> > was called resume early might not get called.  This leads to an
> > unbalance in the clock enables / disables.
> > 
> > Lets take the simple fix and just move suspend to be late to match.
> > This makes the PM core take proper care in keeping things balanced.
> > 
> > Fixes: fe64ba5c6323 ("drm/rockchip: Resume DP early")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> 
> Reviewed-by: Sean Paul <sean@poorly.run>
> 
> This should go in -misc-fixes and due to some... administrative reasons... I
> will leave it on the list until Maarten has a chance to ff to -rc4 on Monday.
> I'll apply it then so as to not require a backmerge.

We're no longer able to ff drm-misc-fixes since a commit came in on Saturday, so
I've piled this on as well.

Thanks,

Sean

> 
> Sean
> 
> > ---
> > 
> >  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > index 7d7cb57410fc..f38f5e113c6b 100644
> > --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > @@ -436,7 +436,7 @@ static int rockchip_dp_resume(struct device *dev)
> >  
> >  static const struct dev_pm_ops rockchip_dp_pm_ops = {
> >  #ifdef CONFIG_PM_SLEEP
> > -	.suspend = rockchip_dp_suspend,
> > +	.suspend_late = rockchip_dp_suspend,
> >  	.resume_early = rockchip_dp_resume,
> >  #endif
> >  };
> > -- 
> > 2.22.0.770.g0f2c4a37fd-goog
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: next/master boot: 256 boots: 6 failed, 177 passed with 72 offline, 1 conflict (next-20190805)
From: Mark Brown @ 2019-08-05 14:24 UTC (permalink / raw)
  To: Nick Desaulniers, Nathan Chancellor, Tri Vo
  Cc: Kevin Hilman, linux-next, Matt Hart, linux-arm-kernel,
	kernel-build-reports
In-Reply-To: <5d482ab9.1c69fb81.684ee.d95e@mx.google.com>


[-- Attachment #1.1: Type: text/plain, Size: 850 bytes --]

On Mon, Aug 05, 2019 at 06:10:17AM -0700, kernelci.org bot wrote:

> arm64:
>     defconfig+CONFIG_CPU_BIG_ENDIAN=y:
>         clang-8:
>             meson-gxl-s805x-p241: 1 failed lab
>             meson-gxl-s905x-khadas-vim: 1 failed lab
>             meson-gxl-s905x-libretech-cc: 1 failed lab

We're still seeing failures with all clang builds failing to understand
the executable format of /init in big endian arm64 builds; GCC builds of
the same configuration boot successfully.  This has been going on for
about as long as we've been trying to boot clang builds AFAICT, it's a
huge proportion of the failures we're seeing in -next.

I did notice one similar failure for v7 GCC today:

	https://kernelci.org/boot/id/5d4807f559b514d97f31b28e/

which is also happening for other trees so that one at least looks
suspiciously like infrastructure.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 4/9] KVM: arm64: Support stolen time reporting via shared structure
From: Steven Price @ 2019-08-05 14:18 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: kvm, linux-doc, Catalin Marinas, Russell King, linux-kernel,
	Paolo Bonzini, Will Deacon, kvmarm, linux-arm-kernel
In-Reply-To: <20190803191303.02e9bcc9@why>

On 03/08/2019 19:13, Marc Zyngier wrote:
> On Sat, 3 Aug 2019 18:58:17 +0100
> Marc Zyngier <maz@kernel.org> wrote:
> 
>> On Fri,  2 Aug 2019 15:50:12 +0100
>> Steven Price <steven.price@arm.com> wrote:
>>
>>> Implement the service call for configuring a shared structre between a
>>> VCPU and the hypervisor in which the hypervisor can write the time
>>> stolen from the VCPU's execution time by other tasks on the host.
>>>
>>> The hypervisor allocates memory which is placed at an IPA chosen by user
>>> space. The hypervisor then uses WRITE_ONCE() to update the shared
>>> structre ensuring single copy atomicity of the 64-bit unsigned value
>>> that reports stolen time in nanoseconds.
>>>
>>> Whenever stolen time is enabled by the guest, the stolen time counter is
>>> reset.
>>>
>>> The stolen time itself is retrieved from the sched_info structure
>>> maintained by the Linux scheduler code. We enable SCHEDSTATS when
>>> selecting KVM Kconfig to ensure this value is meaningful.
>>>
>>> Signed-off-by: Steven Price <steven.price@arm.com>
>>> ---
>>>  arch/arm64/include/asm/kvm_host.h | 13 +++++-
>>>  arch/arm64/kvm/Kconfig            |  1 +
>>>  include/kvm/arm_hypercalls.h      |  1 +
>>>  include/linux/kvm_types.h         |  2 +
>>>  virt/kvm/arm/arm.c                | 18 ++++++++
>>>  virt/kvm/arm/hypercalls.c         | 70 +++++++++++++++++++++++++++++++
>>>  6 files changed, 104 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>> index f656169db8c3..78f270190d43 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -44,6 +44,7 @@
>>>  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
>>>  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
>>>  #define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
>>> +#define KVM_REQ_RECORD_STEAL	KVM_ARCH_REQ(3)
>>>  
>>>  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
>>>  
>>> @@ -83,6 +84,11 @@ struct kvm_arch {
>>>  
>>>  	/* Mandated version of PSCI */
>>>  	u32 psci_version;
>>> +
>>> +	struct kvm_arch_pvtime {
>>> +		void *st;
>>> +		gpa_t st_base;
>>> +	} pvtime;
>>>  };
>>>  
>>>  #define KVM_NR_MEM_OBJS     40
>>> @@ -338,8 +344,13 @@ struct kvm_vcpu_arch {
>>>  	/* True when deferrable sysregs are loaded on the physical CPU,
>>>  	 * see kvm_vcpu_load_sysregs and kvm_vcpu_put_sysregs. */
>>>  	bool sysregs_loaded_on_cpu;
>>> -};
>>>  
>>> +	/* Guest PV state */
>>> +	struct {
>>> +		u64 steal;
>>> +		u64 last_steal;
>>> +	} steal;
>>> +};
>>>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
>>>  #define vcpu_sve_pffr(vcpu) ((void *)((char *)((vcpu)->arch.sve_state) + \
>>>  				      sve_ffr_offset((vcpu)->arch.sve_max_vl)))
>>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
>>> index a67121d419a2..d8b88e40d223 100644
>>> --- a/arch/arm64/kvm/Kconfig
>>> +++ b/arch/arm64/kvm/Kconfig
>>> @@ -39,6 +39,7 @@ config KVM
>>>  	select IRQ_BYPASS_MANAGER
>>>  	select HAVE_KVM_IRQ_BYPASS
>>>  	select HAVE_KVM_VCPU_RUN_PID_CHANGE
>>> +	select SCHEDSTATS
>>>  	---help---
>>>  	  Support hosting virtualized guest machines.
>>>  	  We don't support KVM with 16K page tables yet, due to the multiple
>>> diff --git a/include/kvm/arm_hypercalls.h b/include/kvm/arm_hypercalls.h
>>> index 35a5abcc4ca3..9f0710ab4292 100644
>>> --- a/include/kvm/arm_hypercalls.h
>>> +++ b/include/kvm/arm_hypercalls.h
>>> @@ -7,6 +7,7 @@
>>>  #include <asm/kvm_emulate.h>
>>>  
>>>  int kvm_hvc_call_handler(struct kvm_vcpu *vcpu);
>>> +int kvm_update_stolen_time(struct kvm_vcpu *vcpu);
>>>  
>>>  static inline u32 smccc_get_function(struct kvm_vcpu *vcpu)
>>>  {
>>> diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
>>> index bde5374ae021..1c88e69db3d9 100644
>>> --- a/include/linux/kvm_types.h
>>> +++ b/include/linux/kvm_types.h
>>> @@ -35,6 +35,8 @@ typedef unsigned long  gva_t;
>>>  typedef u64            gpa_t;
>>>  typedef u64            gfn_t;
>>>  
>>> +#define GPA_INVALID	(~(gpa_t)0)
>>> +
>>>  typedef unsigned long  hva_t;
>>>  typedef u64            hpa_t;
>>>  typedef u64            hfn_t;
>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>> index f645c0fbf7ec..ebd963d2580b 100644
>>> --- a/virt/kvm/arm/arm.c
>>> +++ b/virt/kvm/arm/arm.c
>>> @@ -40,6 +40,10 @@
>>>  #include <asm/kvm_coproc.h>
>>>  #include <asm/sections.h>
>>>  
>>> +#include <kvm/arm_hypercalls.h>
>>> +#include <kvm/arm_pmu.h>
>>> +#include <kvm/arm_psci.h>
>>> +
>>>  #ifdef REQUIRES_VIRT
>>>  __asm__(".arch_extension	virt");
>>>  #endif
>>> @@ -135,6 +139,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>>>  	kvm->arch.max_vcpus = vgic_present ?
>>>  				kvm_vgic_get_max_vcpus() : KVM_MAX_VCPUS;
>>>  
>>> +	kvm->arch.pvtime.st_base = GPA_INVALID;
>>>  	return ret;
>>>  out_free_stage2_pgd:
>>>  	kvm_free_stage2_pgd(kvm);
>>> @@ -371,6 +376,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>>  	kvm_vcpu_load_sysregs(vcpu);
>>>  	kvm_arch_vcpu_load_fp(vcpu);
>>>  	kvm_vcpu_pmu_restore_guest(vcpu);
>>> +	kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
>>>  
>>>  	if (single_task_running())
>>>  		vcpu_clear_wfe_traps(vcpu);
>>> @@ -617,6 +623,15 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
>>>  	smp_rmb();
>>>  }
>>>  
>>> +static void vcpu_req_record_steal(struct kvm_vcpu *vcpu)
>>> +{
>>> +	int idx;
>>> +
>>> +	idx = srcu_read_lock(&vcpu->kvm->srcu);
>>> +	kvm_update_stolen_time(vcpu);
>>> +	srcu_read_unlock(&vcpu->kvm->srcu, idx);
>>> +}
>>> +
>>>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>>>  {
>>>  	return vcpu->arch.target >= 0;
>>> @@ -636,6 +651,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
>>>  		 * that a VCPU sees new virtual interrupts.
>>>  		 */
>>>  		kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu);
>>> +
>>> +		if (kvm_check_request(KVM_REQ_RECORD_STEAL, vcpu))
>>> +			vcpu_req_record_steal(vcpu);  
>>
>> Something troubles me. Here, you've set the request on load. But you
>> can be preempted at any time (preemption gets disabled just after).
>>
>> I have the feeling that should you get preempted right here, you'll
>> end-up having accumulated the wrong amount of steal time, as the
>> request put via load when you'll get scheduled back in will only get
>> processed after a full round of entry/exit/entry, which doesn't look
>> great.
> 
> Ah, no. We're saved by the check for pending requests right before we
> jump in the guest, causing an early exit and the whole shebang to be
> restarted.

Yes, that's my understanding. Obviously not ideal if it happens in that
small window, but everything is redone to get the right values in the end.

Steve

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 4/9] KVM: arm64: Support stolen time reporting via shared structure
From: Steven Price @ 2019-08-05 14:09 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Radim Krčmář, kvm, Suzuki K Pouloze,
	Catalin Marinas, linux-doc, Russell King, linux-kernel,
	James Morse, Julien Thierry, Paolo Bonzini, Will Deacon, kvmarm,
	linux-arm-kernel
In-Reply-To: <20190803125515.6aa50084@why>

On 03/08/2019 12:55, Marc Zyngier wrote:
> On Fri,  2 Aug 2019 15:50:12 +0100
> Steven Price <steven.price@arm.com> wrote:
> 
>> Implement the service call for configuring a shared structre between a
> 
> structure
> 
>> VCPU and the hypervisor in which the hypervisor can write the time
>> stolen from the VCPU's execution time by other tasks on the host.
>>
>> The hypervisor allocates memory which is placed at an IPA chosen by user
>> space. The hypervisor then uses WRITE_ONCE() to update the shared
>> structre ensuring single copy atomicity of the 64-bit unsigned value
> 
> structure

Twice in one commit message... thanks for spotting! :)

>> that reports stolen time in nanoseconds.
>>
>> Whenever stolen time is enabled by the guest, the stolen time counter is
>> reset.
>>
>> The stolen time itself is retrieved from the sched_info structure
>> maintained by the Linux scheduler code. We enable SCHEDSTATS when
>> selecting KVM Kconfig to ensure this value is meaningful.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>>  arch/arm64/include/asm/kvm_host.h | 13 +++++-
>>  arch/arm64/kvm/Kconfig            |  1 +
>>  include/kvm/arm_hypercalls.h      |  1 +
>>  include/linux/kvm_types.h         |  2 +
>>  virt/kvm/arm/arm.c                | 18 ++++++++
>>  virt/kvm/arm/hypercalls.c         | 70 +++++++++++++++++++++++++++++++
>>  6 files changed, 104 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index f656169db8c3..78f270190d43 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -44,6 +44,7 @@
>>  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
>>  #define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
>>  #define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
>> +#define KVM_REQ_RECORD_STEAL	KVM_ARCH_REQ(3)
>>  
>>  DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
>>  
>> @@ -83,6 +84,11 @@ struct kvm_arch {
>>  
>>  	/* Mandated version of PSCI */
>>  	u32 psci_version;
>> +
>> +	struct kvm_arch_pvtime {
>> +		void *st;
> 
> Is it really a void *? I'm sure you can use a proper type here...

Indeed that sounds like a good idea!

>> +		gpa_t st_base;
>> +	} pvtime;
>>  };
>>  
>>  #define KVM_NR_MEM_OBJS     40
>> @@ -338,8 +344,13 @@ struct kvm_vcpu_arch {
>>  	/* True when deferrable sysregs are loaded on the physical CPU,
>>  	 * see kvm_vcpu_load_sysregs and kvm_vcpu_put_sysregs. */
>>  	bool sysregs_loaded_on_cpu;
>> -};
>>  
>> +	/* Guest PV state */
>> +	struct {
>> +		u64 steal;
>> +		u64 last_steal;
>> +	} steal;
>> +};
>>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
>>  #define vcpu_sve_pffr(vcpu) ((void *)((char *)((vcpu)->arch.sve_state) + \
>>  				      sve_ffr_offset((vcpu)->arch.sve_max_vl)))
>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
>> index a67121d419a2..d8b88e40d223 100644
>> --- a/arch/arm64/kvm/Kconfig
>> +++ b/arch/arm64/kvm/Kconfig
>> @@ -39,6 +39,7 @@ config KVM
>>  	select IRQ_BYPASS_MANAGER
>>  	select HAVE_KVM_IRQ_BYPASS
>>  	select HAVE_KVM_VCPU_RUN_PID_CHANGE
>> +	select SCHEDSTATS
>>  	---help---
>>  	  Support hosting virtualized guest machines.
>>  	  We don't support KVM with 16K page tables yet, due to the multiple
>> diff --git a/include/kvm/arm_hypercalls.h b/include/kvm/arm_hypercalls.h
>> index 35a5abcc4ca3..9f0710ab4292 100644
>> --- a/include/kvm/arm_hypercalls.h
>> +++ b/include/kvm/arm_hypercalls.h
>> @@ -7,6 +7,7 @@
>>  #include <asm/kvm_emulate.h>
>>  
>>  int kvm_hvc_call_handler(struct kvm_vcpu *vcpu);
>> +int kvm_update_stolen_time(struct kvm_vcpu *vcpu);
>>  
>>  static inline u32 smccc_get_function(struct kvm_vcpu *vcpu)
>>  {
>> diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
>> index bde5374ae021..1c88e69db3d9 100644
>> --- a/include/linux/kvm_types.h
>> +++ b/include/linux/kvm_types.h
>> @@ -35,6 +35,8 @@ typedef unsigned long  gva_t;
>>  typedef u64            gpa_t;
>>  typedef u64            gfn_t;
>>  
>> +#define GPA_INVALID	(~(gpa_t)0)
>> +
>>  typedef unsigned long  hva_t;
>>  typedef u64            hpa_t;
>>  typedef u64            hfn_t;
>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index f645c0fbf7ec..ebd963d2580b 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -40,6 +40,10 @@
>>  #include <asm/kvm_coproc.h>
>>  #include <asm/sections.h>
>>  
>> +#include <kvm/arm_hypercalls.h>
>> +#include <kvm/arm_pmu.h>
>> +#include <kvm/arm_psci.h>
>> +
>>  #ifdef REQUIRES_VIRT
>>  __asm__(".arch_extension	virt");
>>  #endif
>> @@ -135,6 +139,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>>  	kvm->arch.max_vcpus = vgic_present ?
>>  				kvm_vgic_get_max_vcpus() : KVM_MAX_VCPUS;
>>  
>> +	kvm->arch.pvtime.st_base = GPA_INVALID;
>>  	return ret;
>>  out_free_stage2_pgd:
>>  	kvm_free_stage2_pgd(kvm);
>> @@ -371,6 +376,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>  	kvm_vcpu_load_sysregs(vcpu);
>>  	kvm_arch_vcpu_load_fp(vcpu);
>>  	kvm_vcpu_pmu_restore_guest(vcpu);
>> +	kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
>>  
>>  	if (single_task_running())
>>  		vcpu_clear_wfe_traps(vcpu);
>> @@ -617,6 +623,15 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
>>  	smp_rmb();
>>  }
>>  
>> +static void vcpu_req_record_steal(struct kvm_vcpu *vcpu)
>> +{
>> +	int idx;
>> +
>> +	idx = srcu_read_lock(&vcpu->kvm->srcu);
>> +	kvm_update_stolen_time(vcpu);
>> +	srcu_read_unlock(&vcpu->kvm->srcu, idx);
>> +}
>> +
>>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>>  {
>>  	return vcpu->arch.target >= 0;
>> @@ -636,6 +651,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
>>  		 * that a VCPU sees new virtual interrupts.
>>  		 */
>>  		kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu);
>> +
>> +		if (kvm_check_request(KVM_REQ_RECORD_STEAL, vcpu))
>> +			vcpu_req_record_steal(vcpu);
>>  	}
>>  }
>>  
>> diff --git a/virt/kvm/arm/hypercalls.c b/virt/kvm/arm/hypercalls.c
>> index 2906b2df99df..196c71c8dd87 100644
>> --- a/virt/kvm/arm/hypercalls.c
>> +++ b/virt/kvm/arm/hypercalls.c
>> @@ -10,6 +10,70 @@
>>  #include <kvm/arm_hypercalls.h>
>>  #include <kvm/arm_psci.h>
>>  
>> +
>> +static struct pvclock_vcpu_stolen_time_info *pvtime_get_st(
>> +		struct kvm_vcpu *vcpu)
> 
> nit: on a single line.
> 
>> +{
>> +	struct pvclock_vcpu_stolen_time_info *st = vcpu->kvm->arch.pvtime.st;
>> +
>> +	if (!st)
>> +		return NULL;
>> +
>> +	return &st[kvm_vcpu_get_idx(vcpu)];
>> +}
>> +
>> +int kvm_update_stolen_time(struct kvm_vcpu *vcpu)
>> +{
>> +	u64 steal;
>> +	struct pvclock_vcpu_stolen_time_info *kaddr;
>> +
>> +	if (vcpu->kvm->arch.pvtime.st_base == GPA_INVALID)
>> +		return -ENOTSUPP;
> 
> So for a guest that doesn't have stolen time support (which is 100% of
> them for the foreseeable future), we still set a request, take the srcu
> lock and end-up here for nothing. I'd rather we test this st_base
> early, as it should never change once the guest has started.

Good point - I'll make the call to kvm_make_request() conditional on
st_base being set.

>> +
>> +	kaddr = pvtime_get_st(vcpu);
>> +
>> +	if (!kaddr)
>> +		return -ENOTSUPP;
> 
> How can this happen?

Good question, and it makes the pvtime_get_st() helper rather pointless.
Will remove.

>> +
>> +	kaddr->revision = 0;
>> +	kaddr->attributes = 0;
> 
> Why does this need to be written each time we update the stolen time? I
> have the feeling this would be better moved to the hypercall
> initializing the data structure.

Sure.

>> +
>> +	/* Let's do the local bookkeeping */
>> +	steal = vcpu->arch.steal.steal;
>> +	steal += current->sched_info.run_delay - vcpu->arch.steal.last_steal;
>> +	vcpu->arch.steal.last_steal = current->sched_info.run_delay;
>> +	vcpu->arch.steal.steal = steal;
>> +
>> +	/* Now write out the value to the shared page */
>> +	WRITE_ONCE(kaddr->stolen_time, cpu_to_le64(steal));
> 
> Is there any requirement for this to be visible to another CPU than the
> one this is being written from?

I don't believe there is a requirement for this to be visible
synchronously - there is an implicit race here if another vCPU is
accessing the structure (because the hypervisor can preempt and update
at any point), so we don't need to push this out with a barrier. However
we must use 64-bit single-copy atomicity writes to ensure that another
vCPU can't see a half-updated value.

Thanks,

Steve

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH V13 12/12] PCI: tegra: Add Tegra194 PCIe support
From: Lorenzo Pieralisi @ 2019-08-05 14:01 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: mark.rutland, devicetree, mperttunen, mmaddireddy, linux-pci,
	catalin.marinas, will.deacon, linux-kernel, kthota, kishon,
	linux-tegra, robh+dt, thierry.reding, gustavo.pimentel,
	jingoohan1, bhelgaas, digetx, jonathanh, linux-arm-kernel,
	sagar.tv
In-Reply-To: <f09c79fc-c724-5290-d630-cac3fdd7a996@nvidia.com>

On Fri, Aug 02, 2019 at 05:36:43PM +0530, Vidya Sagar wrote:
> On 7/30/2019 9:19 PM, Lorenzo Pieralisi wrote:
> > On Tue, Jul 23, 2019 at 08:14:08PM +0530, Vidya Sagar wrote:
> > > On 7/16/2019 4:52 PM, Lorenzo Pieralisi wrote:
> > > > On Sat, Jul 13, 2019 at 12:34:34PM +0530, Vidya Sagar wrote:
> > > > 
> > > > [...]
> > > > 
> > > > > > > > > +static int tegra_pcie_bpmp_set_ctrl_state(struct tegra_pcie_dw *pcie,
> > > > > > > > > +					  bool enable)
> > > > > > > > > +{
> > > > > > > > > +	struct mrq_uphy_response resp;
> > > > > > > > > +	struct tegra_bpmp_message msg;
> > > > > > > > > +	struct mrq_uphy_request req;
> > > > > > > > > +	int err;
> > > > > > > > > +
> > > > > > > > > +	if (pcie->cid == 5)
> > > > > > > > > +		return 0;
> > > > > > > > 
> > > > > > > > What's wrong with cid == 5 ? Explain please.
> > > > > > > Controller with ID=5 doesn't need any programming to enable it which is
> > > > > > > done here through calling firmware API.
> > > > > > > 
> > > > > > > > 
> > > > > > > > > +	memset(&req, 0, sizeof(req));
> > > > > > > > > +	memset(&resp, 0, sizeof(resp));
> > > > > > > > > +
> > > > > > > > > +	req.cmd = CMD_UPHY_PCIE_CONTROLLER_STATE;
> > > > > > > > > +	req.controller_state.pcie_controller = pcie->cid;
> > > > > > > > > +	req.controller_state.enable = enable;
> > > > > > > > > +
> > > > > > > > > +	memset(&msg, 0, sizeof(msg));
> > > > > > > > > +	msg.mrq = MRQ_UPHY;
> > > > > > > > > +	msg.tx.data = &req;
> > > > > > > > > +	msg.tx.size = sizeof(req);
> > > > > > > > > +	msg.rx.data = &resp;
> > > > > > > > > +	msg.rx.size = sizeof(resp);
> > > > > > > > > +
> > > > > > > > > +	if (irqs_disabled())
> > > > > > > > 
> > > > > > > > Can you explain to me what this check is meant to achieve please ?
> > > > > > > Firmware interface provides different APIs to be called when there are
> > > > > > > no interrupts enabled in the system (noirq context) and otherwise
> > > > > > > hence checking that situation here and calling appropriate API.
> > > > > > 
> > > > > > That's what I am questioning. Being called from {suspend/resume}_noirq()
> > > > > > callbacks (if that's the code path this check caters for) does not mean
> > > > > > irqs_disabled() == true.
> > > > > Agree.
> > > > > Actually, I got a hint of having this check from the following.
> > > > > Both tegra_bpmp_transfer_atomic() and tegra_bpmp_transfer() are indirectly
> > > > > called by APIs registered with .master_xfer() and .master_xfer_atomic() hooks of
> > > > > struct i2c_algorithm and the decision to call which one of these is made using the
> > > > > following check in i2c-core.h file.
> > > > > static inline bool i2c_in_atomic_xfer_mode(void)
> > > > > {
> > > > > 	return system_state > SYSTEM_RUNNING && irqs_disabled();
> > > > > }
> > > > > I think I should use this condition as is IIUC.
> > > > > Please let me know if there are any concerns with this.
> > > > 
> > > > It is not a concern, it is just that I don't understand how this code
> > > > can be called with IRQs disabled, if you can give me an execution path I
> > > > am happy to leave the check there. On top of that, when called from
> > > > suspend NOIRQ context, it is likely to use the blocking API (because
> > > > IRQs aren't disabled at CPU level) behind which there is most certainly
> > > > an IRQ required to wake the thread up and if the IRQ in question was
> > > > disabled in the suspend NOIRQ phase this code is likely to deadlock.
> > > > 
> > > > I want to make sure we can justify adding this check, I do not
> > > > want to add it because we think it can be needed when it may not
> > > > be needed at all (and it gets copy and pasted over and over again
> > > > in other drivers).
> > > I had a discussion internally about this and the prescribed usage of these APIs
> > > seem to be that
> > > use tegra_bpmp_transfer() in .probe() and other paths where interrupts are
> > > enabled as this API needs interrupts to be enabled for its working.
> > > Use tegra_bpmp_transfer_atomic() surrounded by local_irq_save()/local_irq_restore()
> > > in other paths where interrupt servicing is disabled.
> > 
> > Why tegra_bpmp_transfer_atomic() needs IRQs to be disabled ? And why
> > is it needed in this piece of code where IRQs are _never_ disabled
> > at CPU level ?
> > 
> > IRQs are enabled when you call a suspend_noirq() callback, so the
> > blocking API can be used as long as the IRQ descriptor backing
> > the IRQ that will wake-up the blocked call is marked as
> > IRQF_NO_SUSPEND.
> > 
> > The problem is not IRQs enabled/disabled at CPU level, the problem is
> > the IRQ descriptor of the IRQ required to handle the blocking BPMP call,
> > mark it as IRQF_NO_SUSPEND and remove the tegra_bpmp_transfer_atomic()
> > call from this code (or please give me a concrete example pinpointing
> > why it is needed).
> Ideally, using tegra_bpmp_transfer() alone in all paths (.probe() as
> well as .resume_noirq()) should have worked as the corresponding IRQ
> is already flagged as IRQF_NO_SUSPEND, but, because of the way BPMP-FW
> driver in kernel making its interface available through
> .resume_early(), tegra_bpmp_transfer() wasn't working as expected and
> I pushed a patch (CC'ing you) at
> http://patchwork.ozlabs.org/patch/1140973/ to make it .resume_noirq()
> from .resume_early().  With that in place, we can just use
> tegra_bpmp_trasnfer().  I'll push a new patch with this change once my
> BPMP-FW driver patch is approved.

Does this leave you with a resume_noirq() callbacks ordering issue to
sort out ?

a.k.a How will you guarantee that the BPMP will resume before the host
bridge ?

Thanks,
Lorenzo

> Thanks,
> Vidya Sagar
> > 
> > Thanks,
> > Lorenzo
> > 
> > > I'll go ahead and make next patch series with this if this looks fine to you.
> > > 
> > > > 
> > > > > > Actually, if tegra_bpmp_transfer() requires IRQs to be enabled you may
> > > > > > even end up in a situation where that blocking call does not wake up
> > > > > > because the IRQ in question was disabled in the NOIRQ suspend/resume
> > > > > > phase.
> > > > > > 
> > > > > > [...]
> > > > > > 
> > > > > > > > > +static int tegra_pcie_dw_probe(struct platform_device *pdev)
> > > > > > > > > +{
> > > > > > > > > +	const struct tegra_pcie_soc *data;
> > > > > > > > > +	struct device *dev = &pdev->dev;
> > > > > > > > > +	struct resource *atu_dma_res;
> > > > > > > > > +	struct tegra_pcie_dw *pcie;
> > > > > > > > > +	struct resource *dbi_res;
> > > > > > > > > +	struct pcie_port *pp;
> > > > > > > > > +	struct dw_pcie *pci;
> > > > > > > > > +	struct phy **phys;
> > > > > > > > > +	char *name;
> > > > > > > > > +	int ret;
> > > > > > > > > +	u32 i;
> > > > > > > > > +
> > > > > > > > > +	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> > > > > > > > > +	if (!pcie)
> > > > > > > > > +		return -ENOMEM;
> > > > > > > > > +
> > > > > > > > > +	pci = &pcie->pci;
> > > > > > > > > +	pci->dev = &pdev->dev;
> > > > > > > > > +	pci->ops = &tegra_dw_pcie_ops;
> > > > > > > > > +	pp = &pci->pp;
> > > > > > > > > +	pcie->dev = &pdev->dev;
> > > > > > > > > +
> > > > > > > > > +	data = (struct tegra_pcie_soc *)of_device_get_match_data(dev);
> > > > > > > > > +	if (!data)
> > > > > > > > > +		return -EINVAL;
> > > > > > > > > +	pcie->mode = (enum dw_pcie_device_mode)data->mode;
> > > > > > > > > +
> > > > > > > > > +	ret = tegra_pcie_dw_parse_dt(pcie);
> > > > > > > > > +	if (ret < 0) {
> > > > > > > > > +		dev_err(dev, "Failed to parse device tree: %d\n", ret);
> > > > > > > > > +		return ret;
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	pcie->pex_ctl_supply = devm_regulator_get(dev, "vddio-pex-ctl");
> > > > > > > > > +	if (IS_ERR(pcie->pex_ctl_supply)) {
> > > > > > > > > +		dev_err(dev, "Failed to get regulator: %ld\n",
> > > > > > > > > +			PTR_ERR(pcie->pex_ctl_supply));
> > > > > > > > > +		return PTR_ERR(pcie->pex_ctl_supply);
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	pcie->core_clk = devm_clk_get(dev, "core");
> > > > > > > > > +	if (IS_ERR(pcie->core_clk)) {
> > > > > > > > > +		dev_err(dev, "Failed to get core clock: %ld\n",
> > > > > > > > > +			PTR_ERR(pcie->core_clk));
> > > > > > > > > +		return PTR_ERR(pcie->core_clk);
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	pcie->appl_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > > > > > > > +						      "appl");
> > > > > > > > > +	if (!pcie->appl_res) {
> > > > > > > > > +		dev_err(dev, "Failed to find \"appl\" region\n");
> > > > > > > > > +		return PTR_ERR(pcie->appl_res);
> > > > > > > > > +	}
> > > > > > > > > +	pcie->appl_base = devm_ioremap_resource(dev, pcie->appl_res);
> > > > > > > > > +	if (IS_ERR(pcie->appl_base))
> > > > > > > > > +		return PTR_ERR(pcie->appl_base);
> > > > > > > > > +
> > > > > > > > > +	pcie->core_apb_rst = devm_reset_control_get(dev, "apb");
> > > > > > > > > +	if (IS_ERR(pcie->core_apb_rst)) {
> > > > > > > > > +		dev_err(dev, "Failed to get APB reset: %ld\n",
> > > > > > > > > +			PTR_ERR(pcie->core_apb_rst));
> > > > > > > > > +		return PTR_ERR(pcie->core_apb_rst);
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	phys = devm_kcalloc(dev, pcie->phy_count, sizeof(*phys), GFP_KERNEL);
> > > > > > > > > +	if (!phys)
> > > > > > > > > +		return PTR_ERR(phys);
> > > > > > > > > +
> > > > > > > > > +	for (i = 0; i < pcie->phy_count; i++) {
> > > > > > > > > +		name = kasprintf(GFP_KERNEL, "p2u-%u", i);
> > > > > > > > > +		if (!name) {
> > > > > > > > > +			dev_err(dev, "Failed to create P2U string\n");
> > > > > > > > > +			return -ENOMEM;
> > > > > > > > > +		}
> > > > > > > > > +		phys[i] = devm_phy_get(dev, name);
> > > > > > > > > +		kfree(name);
> > > > > > > > > +		if (IS_ERR(phys[i])) {
> > > > > > > > > +			ret = PTR_ERR(phys[i]);
> > > > > > > > > +			dev_err(dev, "Failed to get PHY: %d\n", ret);
> > > > > > > > > +			return ret;
> > > > > > > > > +		}
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	pcie->phys = phys;
> > > > > > > > > +
> > > > > > > > > +	dbi_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> > > > > > > > > +	if (!dbi_res) {
> > > > > > > > > +		dev_err(dev, "Failed to find \"dbi\" region\n");
> > > > > > > > > +		return PTR_ERR(dbi_res);
> > > > > > > > > +	}
> > > > > > > > > +	pcie->dbi_res = dbi_res;
> > > > > > > > > +
> > > > > > > > > +	pci->dbi_base = devm_ioremap_resource(dev, dbi_res);
> > > > > > > > > +	if (IS_ERR(pci->dbi_base))
> > > > > > > > > +		return PTR_ERR(pci->dbi_base);
> > > > > > > > > +
> > > > > > > > > +	/* Tegra HW locates DBI2 at a fixed offset from DBI */
> > > > > > > > > +	pci->dbi_base2 = pci->dbi_base + 0x1000;
> > > > > > > > > +
> > > > > > > > > +	atu_dma_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > > > > > > > +						   "atu_dma");
> > > > > > > > > +	if (!atu_dma_res) {
> > > > > > > > > +		dev_err(dev, "Failed to find \"atu_dma\" region\n");
> > > > > > > > > +		return PTR_ERR(atu_dma_res);
> > > > > > > > > +	}
> > > > > > > > > +	pcie->atu_dma_res = atu_dma_res;
> > > > > > > > > +	pci->atu_base = devm_ioremap_resource(dev, atu_dma_res);
> > > > > > > > > +	if (IS_ERR(pci->atu_base))
> > > > > > > > > +		return PTR_ERR(pci->atu_base);
> > > > > > > > > +
> > > > > > > > > +	pcie->core_rst = devm_reset_control_get(dev, "core");
> > > > > > > > > +	if (IS_ERR(pcie->core_rst)) {
> > > > > > > > > +		dev_err(dev, "Failed to get core reset: %ld\n",
> > > > > > > > > +			PTR_ERR(pcie->core_rst));
> > > > > > > > > +		return PTR_ERR(pcie->core_rst);
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	pp->irq = platform_get_irq_byname(pdev, "intr");
> > > > > > > > > +	if (!pp->irq) {
> > > > > > > > > +		dev_err(dev, "Failed to get \"intr\" interrupt\n");
> > > > > > > > > +		return -ENODEV;
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	ret = devm_request_irq(dev, pp->irq, tegra_pcie_irq_handler,
> > > > > > > > > +			       IRQF_SHARED, "tegra-pcie-intr", pcie);
> > > > > > > > > +	if (ret) {
> > > > > > > > > +		dev_err(dev, "Failed to request IRQ %d: %d\n", pp->irq, ret);
> > > > > > > > > +		return ret;
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	pcie->bpmp = tegra_bpmp_get(dev);
> > > > > > > > > +	if (IS_ERR(pcie->bpmp))
> > > > > > > > > +		return PTR_ERR(pcie->bpmp);
> > > > > > > > > +
> > > > > > > > > +	platform_set_drvdata(pdev, pcie);
> > > > > > > > > +
> > > > > > > > > +	if (pcie->mode == DW_PCIE_RC_TYPE) {
> > > > > > > > > +		ret = tegra_pcie_config_rp(pcie);
> > > > > > > > > +		if (ret && ret != -ENOMEDIUM)
> > > > > > > > > +			goto fail;
> > > > > > > > > +		else
> > > > > > > > > +			return 0;
> > > > > > > > 
> > > > > > > > So if the link is not up we still go ahead and make probe
> > > > > > > > succeed. What for ?
> > > > > > > We may need root port to be available to support hot-plugging of
> > > > > > > endpoint devices, so, we don't fail the probe.
> > > > > > 
> > > > > > We need it or we don't. If you do support hotplugging of endpoint
> > > > > > devices point me at the code, otherwise link up failure means
> > > > > > failure to probe.
> > > > > Currently hotplugging of endpoint is not supported, but it is one of
> > > > > the use cases that we may add support for in future.
> > > > 
> > > > You should elaborate on this, I do not understand what you mean,
> > > > either the root port(s) supports hotplug or it does not.
> > > > 
> > > > > But, why should we fail probe if link up doesn't happen? As such,
> > > > > nothing went wrong in terms of root port initialization right?  I
> > > > > checked other DWC based implementations and following are not failing
> > > > > the probe pci-dra7xx.c, pcie-armada8k.c, pcie-artpec6.c, pcie-histb.c,
> > > > > pcie-kirin.c, pcie-spear13xx.c, pci-exynos.c, pci-imx6.c,
> > > > > pci-keystone.c, pci-layerscape.c
> > > > > 
> > > > > Although following do fail the probe if link is not up.  pcie-qcom.c,
> > > > > pcie-uniphier.c, pci-meson.c
> > > > > 
> > > > > So, to me, it looks more like a choice we can make whether to fail the
> > > > > probe or not and in this case we are choosing not to fail.
> > > > 
> > > > I disagree. I had an offline chat with Bjorn and whether link-up should
> > > > fail the probe or not depends on whether the root port(s) is hotplug
> > > > capable or not and this in turn relies on the root port "Slot
> > > > implemented" bit in the PCI Express capabilities register.
> > > > 
> > > > It is a choice but it should be based on evidence.
> > > > 
> > > > Lorenzo
> > > With Bjorn's latest comment on top of this, I think we are good not to fail
> > > the probe here.
> > > 
> > > - Vidya Sagar
> > > > 
> > > 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH] arm64/cache: fix -Woverride-init compiler warnings
From: Will Deacon @ 2019-08-05 14:01 UTC (permalink / raw)
  To: Qian Cai; +Cc: Mark Rutland, Catalin Marinas, linux-kernel, linux-arm-kernel
In-Reply-To: <771C4B4C-6D79-419D-9778-5DE1BFC67FBE@lca.pw>

On Mon, Aug 05, 2019 at 07:47:37AM -0400, Qian Cai wrote:
> 
> 
> > On Aug 5, 2019, at 5:52 AM, Will Deacon <will@kernel.org> wrote:
> > 
> > On Fri, Aug 02, 2019 at 11:32:24AM -0400, Qian Cai wrote:
> >> The commit 155433cb365e ("arm64: cache: Remove support for ASID-tagged
> >> VIVT I-caches") introduced some compiation warnings from GCC,
> >> 
> >> arch/arm64/kernel/cpuinfo.c:38:26: warning: initialized field
> >> overwritten [-Woverride-init]
> >>  [ICACHE_POLICY_VIPT]  = "VIPT",
> >>                          ^~~~~~
> >> arch/arm64/kernel/cpuinfo.c:38:26: note: (near initialization for
> >> 'icache_policy_str[2]')
> >> arch/arm64/kernel/cpuinfo.c:39:26: warning: initialized field
> >> overwritten [-Woverride-init]
> >>  [ICACHE_POLICY_PIPT]  = "PIPT",
> >>                          ^~~~~~
> >> arch/arm64/kernel/cpuinfo.c:39:26: note: (near initialization for
> >> 'icache_policy_str[3]')
> >> arch/arm64/kernel/cpuinfo.c:40:27: warning: initialized field
> >> overwritten [-Woverride-init]
> >>  [ICACHE_POLICY_VPIPT]  = "VPIPT",
> >>                           ^~~~~~~
> >> arch/arm64/kernel/cpuinfo.c:40:27: note: (near initialization for
> >> 'icache_policy_str[0]')
> >> 
> >> because it initializes icache_policy_str[0 ... 3] twice.
> >> 
> >> Fixes: 155433cb365e ("arm64: cache: Remove support for ASID-tagged VIVT I-caches")
> >> Signed-off-by: Qian Cai <cai@lca.pw>
> >> ---
> >> arch/arm64/kernel/cpuinfo.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> >> index 876055e37352..193b38da8d96 100644
> >> --- a/arch/arm64/kernel/cpuinfo.c
> >> +++ b/arch/arm64/kernel/cpuinfo.c
> >> @@ -34,10 +34,10 @@
> >> static struct cpuinfo_arm64 boot_cpu_data;
> >> 
> >> static char *icache_policy_str[] = {
> >> -	[0 ... ICACHE_POLICY_PIPT]	= "RESERVED/UNKNOWN",
> >> +	[ICACHE_POLICY_VPIPT]		= "VPIPT",
> >> +	[ICACHE_POLICY_VPIPT + 1]	= "RESERVED/UNKNOWN",
> >> 	[ICACHE_POLICY_VIPT]		= "VIPT",
> >> 	[ICACHE_POLICY_PIPT]		= "PIPT",
> >> -	[ICACHE_POLICY_VPIPT]		= "VPIPT",
> > 
> > I really don't like this patch. Using "[0 ... MAXIDX] = <default>" is a
> > useful idiom and I think the code is more error-prone the way you have
> > restructured it.
> > 
> > Why are you passing -Woverride-init to the compiler anyway? There's only
> > one Makefile that references that option, and it's specific to a pinctrl
> > driver.
> 
> Those extra warnings can be enabled by “make W=1”. “-Woverride-init “ seems to be useful
> to catch potential developer mistakes with unintented double-initializations. It is normal to
> start to fix the most of false-positives first before globally enabling the flag by default just like
> “-Wimplicit-fallthrough” mentioned in,
> 
> https://lwn.net/Articles/794944/

I think this case is completely different to the implicit fallthrough stuff.
The solution there was simply to add a comment without restructuring the
surrounding code. What your patch does here is actively make the code harder
to understand.

Initialising a static array with a non-zero pattern is a useful idiom and I
don't think we should throw that away just to appease a silly compiler
warning that appears only with non-default build options. Have a look at
the way we use PERF_MAP_ALL_UNSUPPORTED in the Arm PMU code, for example.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v3] tracing: Function stack size and its name mismatch in arm64
From: Steven Rostedt @ 2019-08-05 13:59 UTC (permalink / raw)
  To: Will Deacon
  Cc: Jiping Ma, catalin.marinas, will.deacon, linux-kernel,
	takahiro.akashi, mingo, Joel Fernandes, kernel-team,
	linux-arm-kernel
In-Reply-To: <20190805112524.ajlmouutqckwpqyd@willie-the-truck>

On Mon, 5 Aug 2019 12:25:25 +0100
Will Deacon <will@kernel.org> wrote:

> So, I completely agree with Steve that we shouldn't be littering the core code
> with #ifdef CONFIG_ARM64, but we probably do need something in the arch backend
> if we're going to do this properly, and that in turn is likely to need a very
> different algorithm from the one currently in use.

And I'm perfectly fine if someone wants to tear up the current
implementation, and make one that has callbacks to arch code (with
defaults for those that don't need it).

But it has to be clean. We worked hard to get rid of most "#ifdef"
being scattered in the kernel, and I'm not bringing it back in my code.

-- Steve

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH] arm64/efi: fix variable 'si' set but not used
From: Dave Martin @ 2019-08-05 13:53 UTC (permalink / raw)
  To: Qian Cai
  Cc: linux-efi, ard.biesheuvel, catalin.marinas, linux-kernel, will,
	linux-arm-kernel
In-Reply-To: <1564521828-4528-1-git-send-email-cai@lca.pw>

On Tue, Jul 30, 2019 at 05:23:48PM -0400, Qian Cai wrote:
> GCC throws out this warning on arm64.
> 
> drivers/firmware/efi/libstub/arm-stub.c: In function 'efi_entry':
> drivers/firmware/efi/libstub/arm-stub.c:132:22: warning: variable 'si'
> set but not used [-Wunused-but-set-variable]
> 
> Fix it by making free_screen_info() a static inline function.
> 
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
>  arch/arm64/include/asm/efi.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 8e79ce9c3f5c..76a144702586 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -105,7 +105,11 @@ static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
>  	((protocol##_t *)instance)->f(instance, ##__VA_ARGS__)
>  
>  #define alloc_screen_info(x...)		&screen_info
> -#define free_screen_info(x...)
> +
> +static inline void free_screen_info(efi_system_table_t *sys_table_arg,
> +				    struct screen_info *si)
> +{
> +}

Is this issue caused by the EFI stub being built with non-default
CFLAGS?

The toplevel Makefile specifies -Wno-unused-but-set-variable, which
would silence this warning.

It's debatable whether calling an empty inline function "uses" the
arguments, so I think your patch only silences the warning by accident:
different GCC versions, or clang, might still warn.


I wonder if we're missing any other options that would make sense for
the EFI stub.

[...]

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v3 05/10] rtc: mt6397: add compatible for mt6323
From: Alexandre Belloni @ 2019-08-05 13:50 UTC (permalink / raw)
  To: Frank Wunderlich
  Cc: Kate Stewart, Mark Rutland, linux-kernel, Richard Fontana,
	Mauro Carvalho Chehab, Lee Jones, linux-rtc, Allison Randal,
	devicetree, linux-pm, Sean Wang, Tianping . Fang, Rob Herring,
	linux-mediatek, Jonathan Cameron, Matthias Brugger,
	Thomas Gleixner, Eddie Huang, linux-arm-kernel, Alessandro Zummo,
	Josef Friedl, Greg Kroah-Hartman, Sebastian Reichel,
	David S. Miller
In-Reply-To: <20190729174154.4335-6-frank-w@public-files.de>

On 29/07/2019 19:41:49+0200, Frank Wunderlich wrote:
> From: Josef Friedl <josef.friedl@speed.at>
> 
> use mt6397 rtc driver also for mt6323
> compatible needed because of different rtc-base/size see part 7

I still don't think this is a good reason so I would remove that
sentence. Also, it refers to part 7 which is an info that will not be
available when looking at the git history.

> 
> changes since v2: splitted this from v2.3 suggested-by Alexandre Belloni
> 

Changelog after the --- marker please.

> Signed-off-by: Josef Friedl <josef.friedl@speed.at>
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>  drivers/rtc/rtc-mt6397.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> index 9370b7fc9f81..21cd9cc8b4c7 100644
> --- a/drivers/rtc/rtc-mt6397.c
> +++ b/drivers/rtc/rtc-mt6397.c
> @@ -325,6 +325,7 @@ static SIMPLE_DEV_PM_OPS(mt6397_pm_ops, mt6397_rtc_suspend,
>  			mt6397_rtc_resume);
> 
>  static const struct of_device_id mt6397_rtc_of_match[] = {
> +	{ .compatible = "mediatek,mt6323-rtc", },
>  	{ .compatible = "mediatek,mt6397-rtc", },
>  	{ }
>  };
> --
> 2.17.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v3 03/10] rtc: mt6397: move some common definitions into rtc.h
From: Alexandre Belloni @ 2019-08-05 13:48 UTC (permalink / raw)
  To: Frank Wunderlich
  Cc: Kate Stewart, Mark Rutland, linux-kernel, Richard Fontana,
	Mauro Carvalho Chehab, Lee Jones, linux-rtc, Allison Randal,
	devicetree, linux-pm, Sean Wang, Tianping . Fang, Rob Herring,
	linux-mediatek, Jonathan Cameron, Matthias Brugger,
	Thomas Gleixner, Eddie Huang, linux-arm-kernel, Alessandro Zummo,
	Josef Friedl, Greg Kroah-Hartman, Sebastian Reichel,
	David S. Miller
In-Reply-To: <20190729174154.4335-4-frank-w@public-files.de>

On 29/07/2019 19:41:47+0200, Frank Wunderlich wrote:
> From: Josef Friedl <josef.friedl@speed.at>
> 
> move code to separate header-file to reuse definitions later
> in poweroff-driver (drivers/power/reset/mt6323-poweroff.c)
> 
> changes since v2: add missing commit-message

The changelog should be after the --- marker.
> 
> Suggested-by: Frank Wunderlich <frank-w@public-files.de>
> Signed-off-by: Josef Friedl <josef.friedl@speed.at>
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>  drivers/rtc/rtc-mt6397.c       | 55 +-------------------------
>  include/linux/mfd/mt6397/rtc.h | 71 ++++++++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+), 54 deletions(-)
>  create mode 100644 include/linux/mfd/mt6397/rtc.h
> 
> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> index b46ed4dc7015..c08ee5edf865 100644
> --- a/drivers/rtc/rtc-mt6397.c
> +++ b/drivers/rtc/rtc-mt6397.c
> @@ -9,60 +9,7 @@
>  #include <linux/module.h>
>  #include <linux/regmap.h>
>  #include <linux/rtc.h>
> -#include <linux/irqdomain.h>
> -#include <linux/platform_device.h>
> -#include <linux/of_address.h>
> -#include <linux/of_irq.h>
> -#include <linux/io.h>
> -#include <linux/mfd/mt6397/core.h>
> -
> -#define RTC_BBPU		0x0000
> -#define RTC_BBPU_CBUSY		BIT(6)
> -
> -#define RTC_WRTGR		0x003c
> -
> -#define RTC_IRQ_STA		0x0002
> -#define RTC_IRQ_STA_AL		BIT(0)
> -#define RTC_IRQ_STA_LP		BIT(3)
> -
> -#define RTC_IRQ_EN		0x0004
> -#define RTC_IRQ_EN_AL		BIT(0)
> -#define RTC_IRQ_EN_ONESHOT	BIT(2)
> -#define RTC_IRQ_EN_LP		BIT(3)
> -#define RTC_IRQ_EN_ONESHOT_AL	(RTC_IRQ_EN_ONESHOT | RTC_IRQ_EN_AL)
> -
> -#define RTC_AL_MASK		0x0008
> -#define RTC_AL_MASK_DOW		BIT(4)
> -
> -#define RTC_TC_SEC		0x000a
> -/* Min, Hour, Dom... register offset to RTC_TC_SEC */
> -#define RTC_OFFSET_SEC		0
> -#define RTC_OFFSET_MIN		1
> -#define RTC_OFFSET_HOUR		2
> -#define RTC_OFFSET_DOM		3
> -#define RTC_OFFSET_DOW		4
> -#define RTC_OFFSET_MTH		5
> -#define RTC_OFFSET_YEAR		6
> -#define RTC_OFFSET_COUNT	7
> -
> -#define RTC_AL_SEC		0x0018
> -
> -#define RTC_PDN2		0x002e
> -#define RTC_PDN2_PWRON_ALARM	BIT(4)
> -
> -#define RTC_MIN_YEAR		1968
> -#define RTC_BASE_YEAR		1900
> -#define RTC_NUM_YEARS		128
> -#define RTC_MIN_YEAR_OFFSET	(RTC_MIN_YEAR - RTC_BASE_YEAR)
> -
> -struct mt6397_rtc {
> -	struct device		*dev;
> -	struct rtc_device	*rtc_dev;
> -	struct mutex		lock;
> -	struct regmap		*regmap;
> -	int			irq;
> -	u32			addr_base;
> -};
> +#include <linux/mfd/mt6397/rtc.h>
> 
>  static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
>  {
> diff --git a/include/linux/mfd/mt6397/rtc.h b/include/linux/mfd/mt6397/rtc.h
> new file mode 100644
> index 000000000000..b702c29e8c74
> --- /dev/null
> +++ b/include/linux/mfd/mt6397/rtc.h
> @@ -0,0 +1,71 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2014-2018 MediaTek Inc.
> + *
> + * Author: Tianping.Fang <tianping.fang@mediatek.com>
> + *        Sean Wang <sean.wang@mediatek.com>
> + */
> +
> +#ifndef _LINUX_MFD_MT6397_RTC_H_
> +#define _LINUX_MFD_MT6397_RTC_H_
> +
> +#include <linux/jiffies.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/rtc.h>
> +
> +#define RTC_BBPU               0x0000
> +#define RTC_BBPU_CBUSY         BIT(6)
> +#define RTC_BBPU_KEY            (0x43 << 8)
> +
> +#define RTC_WRTGR              0x003c
> +
> +#define RTC_IRQ_STA            0x0002
> +#define RTC_IRQ_STA_AL         BIT(0)
> +#define RTC_IRQ_STA_LP         BIT(3)
> +
> +#define RTC_IRQ_EN             0x0004
> +#define RTC_IRQ_EN_AL          BIT(0)
> +#define RTC_IRQ_EN_ONESHOT     BIT(2)
> +#define RTC_IRQ_EN_LP          BIT(3)
> +#define RTC_IRQ_EN_ONESHOT_AL  (RTC_IRQ_EN_ONESHOT | RTC_IRQ_EN_AL)
> +
> +#define RTC_AL_MASK            0x0008
> +#define RTC_AL_MASK_DOW                BIT(4)
> +
> +#define RTC_TC_SEC             0x000a
> +/* Min, Hour, Dom... register offset to RTC_TC_SEC */
> +#define RTC_OFFSET_SEC         0
> +#define RTC_OFFSET_MIN         1
> +#define RTC_OFFSET_HOUR                2
> +#define RTC_OFFSET_DOM         3
> +#define RTC_OFFSET_DOW         4
> +#define RTC_OFFSET_MTH         5
> +#define RTC_OFFSET_YEAR                6
> +#define RTC_OFFSET_COUNT       7
> +
> +#define RTC_AL_SEC             0x0018
> +
> +#define RTC_PDN2               0x002e
> +#define RTC_PDN2_PWRON_ALARM   BIT(4)
> +
> +#define RTC_MIN_YEAR           1968
> +#define RTC_BASE_YEAR          1900
> +#define RTC_NUM_YEARS          128
> +#define RTC_MIN_YEAR_OFFSET    (RTC_MIN_YEAR - RTC_BASE_YEAR)
> +
> +#define MTK_RTC_POLL_DELAY_US  10
> +#define MTK_RTC_POLL_TIMEOUT   (jiffies_to_usecs(HZ))
> +
> +struct mt6397_rtc {
> +	struct device           *dev;
> +	struct rtc_device       *rtc_dev;
> +
> +	/* Protect register access from multiple tasks */
> +	struct mutex            lock;
> +	struct regmap           *regmap;
> +	int                     irq;
> +	u32                     addr_base;
> +};
> +
> +#endif /* _LINUX_MFD_MT6397_RTC_H_ */
> --
> 2.17.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v3 02/10] dt-bindings: add missing mt6397 rtc
From: Alexandre Belloni @ 2019-08-05 13:47 UTC (permalink / raw)
  To: Frank Wunderlich
  Cc: Kate Stewart, Mark Rutland, linux-kernel, Richard Fontana,
	Mauro Carvalho Chehab, Lee Jones, linux-rtc, Allison Randal,
	devicetree, linux-pm, Sean Wang, Tianping . Fang, Rob Herring,
	linux-mediatek, Jonathan Cameron, Matthias Brugger,
	Thomas Gleixner, Eddie Huang, linux-arm-kernel, Alessandro Zummo,
	Josef Friedl, Greg Kroah-Hartman, Sebastian Reichel,
	David S. Miller
In-Reply-To: <20190729174154.4335-3-frank-w@public-files.de>

On 29/07/2019 19:41:46+0200, Frank Wunderlich wrote:
> From: Josef Friedl <josef.friedl@speed.at>
> 
> add missing devicetree-binding document for mt6397 rtc
> in later patch driver is extended with mt6323 chip
> 
> changes since v2: splitted rtc-mt6397.txt from first patch

This line should be after the --- marker.
> 
> Suggested-By: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>

Your SoB should appear last.

> Signed-off-by: Josef Friedl <josef.friedl@speed.at>
> ---
>  .../devicetree/bindings/rtc/rtc-mt6397.txt    | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> 
> diff --git a/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> new file mode 100644
> index 000000000000..ebd1cf80dcc8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> @@ -0,0 +1,29 @@
> +Device-Tree bindings for MediaTek PMIC based RTC
> +
> +MediaTek PMIC based RTC is an independent function of MediaTek PMIC that works
> +as a type of multi-function device (MFD). The RTC can be configured and set up
> +with PMIC wrapper bus which is a common resource shared with the other
> +functions found on the same PMIC.
> +
> +For MediaTek PMIC MFD bindings, see:
> +Documentation/devicetree/bindings/mfd/mt6397.txt
> +
> +For MediaTek PMIC wrapper bus bindings, see:
> +Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> +
> +Required properties:
> +- compatible: Should be one of follows
> +       "mediatek,mt6323-rtc": for MT6323 PMIC
> +       "mediatek,mt6397-rtc": for MT6397 PMIC
> +
> +Example:
> +
> +       pmic {
> +               compatible = "mediatek,mt6323";
> +
> +               ...
> +
> +               rtc {
> +                       compatible = "mediatek,mt6323-rtc";
> +               };
> +       };
> --
> 2.17.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [RFC PATCH] ARM: UNWINDER_FRAME_POINTER implementation for Clang
From: Dave Martin @ 2019-08-05 13:39 UTC (permalink / raw)
  To: Nathan Huckleberry
  Cc: Tri Vo, linux, linux-kernel, clang-built-linux, Robin Murphy,
	linux-arm-kernel
In-Reply-To: <CAJkfWY6StuyMuKG7XdEJrqMsA_Xy02QZKp8r0K2jwSZwBCt+9Q@mail.gmail.com>

On Fri, Aug 02, 2019 at 10:27:30AM -0700, Nathan Huckleberry wrote:
> You're right. Would pushing an extra register be an adequate fix?

Would forcing CONFIG_ARM_UNWIND=y for clang work as an alternative to
this?

Assuming clang supports -funwind-tables or equivalent, this may just
work.

[...]

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v4 2/2] mmc: Add support for the ASPEED SD controller
From: Adrian Hunter @ 2019-08-05 13:36 UTC (permalink / raw)
  To: Andrew Jeffery, linux-mmc
  Cc: mark.rutland, devicetree, ulf.hansson, linux-aspeed,
	ryanchen.aspeed, linux-kernel, robh+dt, joel, linux-arm-kernel
In-Reply-To: <20190805025155.9020-3-andrew@aj.id.au>

On 5/08/19 5:51 AM, Andrew Jeffery wrote:
> Add a minimal driver for ASPEED's SD controller, which exposes two
> SDHCIs.
> 
> The ASPEED design implements a common register set for the SDHCIs, and
> moves some of the standard configuration elements out to this common
> area (e.g. 8-bit mode, and card detect configuration which is not
> currently supported).
> 
> The SD controller has a dedicated hardware interrupt that is shared
> between the slots. The common register set exposes information on which
> slot triggered the interrupt; early revisions of the patch introduced an
> irqchip for the register, but reality is it doesn't behave as an
> irqchip, and the result fits awkwardly into the irqchip APIs. Instead
> I've taken the simple approach of using the IRQ as a shared IRQ with
> some minor performance impact for the second slot.
> 
> Ryan was the original author of the patch - I've taken his work and
> massaged it to drop the irqchip support and rework the devicetree
> integration. The driver has been smoke tested under qemu against a
> minimal SD controller model and lightly tested on an ast2500-evb.
> 
> Signed-off-by: Ryan Chen <ryanchen.aspeed@gmail.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

One minor comment otherwise:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> 
> ---
> v3: No change
> v2:
> * Add AST2600 compatible
> * Drop SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN
> * Ensure slot number is valid
> * Fix build with CONFIG_MODULES
> * Fix module license string
> * Non-PCI devices won't die
> * Rename aspeed_sdc_configure_8bit_mode()
> * Rename aspeed_sdhci_pdata
> * Switch to sdhci_enable_clk()
> * Use PTR_ERR() on the right `struct platform_device *`
> ---
>  drivers/mmc/host/Kconfig           |  12 ++
>  drivers/mmc/host/Makefile          |   1 +
>  drivers/mmc/host/sdhci-of-aspeed.c | 328 +++++++++++++++++++++++++++++
>  3 files changed, 341 insertions(+)
>  create mode 100644 drivers/mmc/host/sdhci-of-aspeed.c
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 14d89a108edd..0f8a230de2f3 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -154,6 +154,18 @@ config MMC_SDHCI_OF_ARASAN
>  
>  	  If unsure, say N.
>  
> +config MMC_SDHCI_OF_ASPEED
> +	tristate "SDHCI OF support for the ASPEED SDHCI controller"
> +	depends on MMC_SDHCI_PLTFM
> +	depends on OF
> +	help
> +	  This selects the ASPEED Secure Digital Host Controller Interface.
> +
> +	  If you have a controller with this interface, say Y or M here. You
> +	  also need to enable an appropriate bus interface.
> +
> +	  If unsure, say N.
> +
>  config MMC_SDHCI_OF_AT91
>  	tristate "SDHCI OF support for the Atmel SDMMC controller"
>  	depends on MMC_SDHCI_PLTFM
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 73578718f119..390ee162fe71 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -84,6 +84,7 @@ obj-$(CONFIG_MMC_SDHCI_ESDHC_IMX)	+= sdhci-esdhc-imx.o
>  obj-$(CONFIG_MMC_SDHCI_DOVE)		+= sdhci-dove.o
>  obj-$(CONFIG_MMC_SDHCI_TEGRA)		+= sdhci-tegra.o
>  obj-$(CONFIG_MMC_SDHCI_OF_ARASAN)	+= sdhci-of-arasan.o
> +obj-$(CONFIG_MMC_SDHCI_OF_ASPEED)	+= sdhci-of-aspeed.o
>  obj-$(CONFIG_MMC_SDHCI_OF_AT91)		+= sdhci-of-at91.o
>  obj-$(CONFIG_MMC_SDHCI_OF_ESDHC)	+= sdhci-of-esdhc.o
>  obj-$(CONFIG_MMC_SDHCI_OF_HLWD)		+= sdhci-of-hlwd.o
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> new file mode 100644
> index 000000000000..d31785ec90d7
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> @@ -0,0 +1,328 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* Copyright (C) 2019 ASPEED Technology Inc. */
> +/* Copyright (C) 2019 IBM Corp. */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/mmc/host.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +#include "sdhci-pltfm.h"
> +
> +#define ASPEED_SDC_INFO		0x00
> +#define   ASPEED_SDC_S1MMC8	BIT(25)
> +#define   ASPEED_SDC_S0MMC8	BIT(24)
> +
> +struct aspeed_sdc {
> +	struct clk *clk;
> +	struct resource *res;
> +
> +	spinlock_t lock;
> +	void __iomem *regs;
> +};
> +
> +struct aspeed_sdhci {
> +	struct aspeed_sdc *parent;
> +	u32 width_mask;
> +};
> +
> +static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
> +					   struct aspeed_sdhci *sdhci,
> +					   bool bus8)
> +{
> +	u32 info;
> +
> +	/* Set/clear 8 bit mode */
> +	spin_lock(&sdc->lock);
> +	info = readl(sdc->regs + ASPEED_SDC_INFO);
> +	if (bus8)
> +		info |= sdhci->width_mask;
> +	else
> +		info &= ~sdhci->width_mask;
> +	writel(info, sdc->regs + ASPEED_SDC_INFO);
> +	spin_unlock(&sdc->lock);
> +}
> +
> +static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> +{
> +	int div;
> +	u16 clk;
> +
> +	if (clock == host->clock)
> +		return;
> +
> +	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> +
> +	if (clock == 0)
> +		goto out;
> +
> +	for (div = 1; div < 256; div *= 2) {
> +		if ((host->max_clk / div) <= clock)
> +			break;
> +	}
> +	div >>= 1;
> +
> +	clk = div << SDHCI_DIVIDER_SHIFT;
> +
> +	sdhci_enable_clk(host, clk);
> +
> +out:
> +	host->clock = clock;
> +}
> +
> +static void aspeed_sdhci_set_bus_width(struct sdhci_host *host, int width)
> +{
> +	struct sdhci_pltfm_host *pltfm_priv;
> +	struct aspeed_sdhci *aspeed_sdhci;
> +	struct aspeed_sdc *aspeed_sdc;
> +	u8 ctrl;
> +
> +	pltfm_priv = sdhci_priv(host);
> +	aspeed_sdhci = sdhci_pltfm_priv(pltfm_priv);
> +	aspeed_sdc = aspeed_sdhci->parent;
> +
> +	/* Set/clear 8-bit mode */
> +	aspeed_sdc_configure_8bit_mode(aspeed_sdc, aspeed_sdhci,
> +				       width == MMC_BUS_WIDTH_8);
> +
> +	/* Set/clear 1 or 4 bit mode */
> +	ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> +	if (width == MMC_BUS_WIDTH_4)
> +		ctrl |= SDHCI_CTRL_4BITBUS;
> +	else
> +		ctrl &= ~SDHCI_CTRL_4BITBUS;
> +	sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> +}
> +
> +static const struct sdhci_ops aspeed_sdhci_ops = {
> +	.set_clock = aspeed_sdhci_set_clock,
> +	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
> +	.set_bus_width = aspeed_sdhci_set_bus_width,
> +	.get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
> +	.reset = sdhci_reset,
> +	.set_uhs_signaling = sdhci_set_uhs_signaling,
> +};
> +
> +static const struct sdhci_pltfm_data aspeed_sdhci_pdata = {
> +	.ops = &aspeed_sdhci_ops,
> +	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> +};
> +
> +static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev,
> +					      struct resource *res)
> +{
> +	resource_size_t delta;
> +
> +	if (!res || resource_type(res) != IORESOURCE_MEM)
> +		return -EINVAL;
> +
> +	if (res->start < dev->parent->res->start)
> +		return -EINVAL;
> +
> +	delta = res->start - dev->parent->res->start;
> +	if (delta & (0x100 - 1))
> +		return -EINVAL;
> +
> +	return (delta / 0x100) - 1;
> +}
> +
> +static int aspeed_sdhci_probe(struct platform_device *pdev)
> +{
> +	struct sdhci_pltfm_host *pltfm_host;
> +	struct aspeed_sdhci *dev;
> +	struct sdhci_host *host;
> +	struct resource *res;
> +	int slot;
> +	int ret;
> +
> +	host = sdhci_pltfm_init(pdev, &aspeed_sdhci_pdata, sizeof(*dev));
> +	if (IS_ERR(host))
> +		return PTR_ERR(host);
> +
> +	pltfm_host = sdhci_priv(host);
> +	dev = sdhci_pltfm_priv(pltfm_host);
> +	dev->parent = dev_get_drvdata(pdev->dev.parent);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	slot = aspeed_sdhci_calculate_slot(dev, res);
> +
> +	if (slot < 0)
> +		return slot;
> +	else if (slot >= 2)
> +		return -EINVAL;
> +
> +	dev_info(&pdev->dev, "Configuring for slot %d\n", slot);
> +	dev->width_mask = !slot ? ASPEED_SDC_S0MMC8 : ASPEED_SDC_S1MMC8;
> +
> +	sdhci_get_of_property(pdev);
> +
> +	pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(pltfm_host->clk))
> +		return PTR_ERR(pltfm_host->clk);
> +
> +	ret = clk_prepare_enable(pltfm_host->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Unable to enable SDIO clock\n");
> +		goto err_pltfm_free;
> +	}
> +
> +	ret = mmc_of_parse(host->mmc);
> +	if (ret)
> +		goto err_sdhci_add;
> +
> +	ret = sdhci_add_host(host);
> +	if (ret)
> +		goto err_sdhci_add;
> +
> +	return 0;
> +
> +err_sdhci_add:
> +	clk_disable_unprepare(pltfm_host->clk);
> +err_pltfm_free:
> +	sdhci_pltfm_free(pdev);
> +	return ret;
> +}
> +
> +static int aspeed_sdhci_remove(struct platform_device *pdev)
> +{
> +	struct sdhci_pltfm_host *pltfm_host;
> +	struct sdhci_host *host;
> +	int dead = 0;
> +
> +	host = platform_get_drvdata(pdev);
> +	pltfm_host = sdhci_priv(host);
> +
> +	sdhci_remove_host(host, dead);
> +
> +	clk_disable_unprepare(pltfm_host->clk);
> +
> +	sdhci_pltfm_free(pdev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id aspeed_sdhci_of_match[] = {
> +	{ .compatible = "aspeed,ast2400-sdhci", },
> +	{ .compatible = "aspeed,ast2500-sdhci", },
> +	{ .compatible = "aspeed,ast2600-sdhci", },
> +	{ }
> +};
> +
> +static struct platform_driver aspeed_sdhci_driver = {
> +	.driver		= {
> +		.name	= "sdhci-aspeed",
> +		.of_match_table = aspeed_sdhci_of_match,
> +	},
> +	.probe		= aspeed_sdhci_probe,
> +	.remove		= aspeed_sdhci_remove,
> +};
> +
> +static int aspeed_sdc_probe(struct platform_device *pdev)
> +
> +{
> +	struct device_node *parent, *child;
> +	struct aspeed_sdc *sdc;
> +	int ret;
> +
> +	sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL);
> +	if (!sdc)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&sdc->lock);
> +
> +	sdc->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(sdc->clk))
> +		return PTR_ERR(sdc->clk);
> +
> +	ret = clk_prepare_enable(sdc->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Unable to enable SDCLK\n");
> +		return ret;
> +	}
> +
> +	sdc->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	sdc->regs = devm_ioremap_resource(&pdev->dev, sdc->res);
> +	if (IS_ERR(sdc->regs)) {
> +		ret = PTR_ERR(sdc->regs);
> +		goto err_clk;
> +	}
> +
> +	dev_set_drvdata(&pdev->dev, sdc);
> +
> +	parent = pdev->dev.of_node;
> +	for_each_available_child_of_node(parent, child) {
> +		struct platform_device *cpdev;
> +
> +		cpdev = of_platform_device_create(child, NULL, &pdev->dev);
> +		if (IS_ERR(cpdev)) {
> +			of_node_put(child);
> +			ret = PTR_ERR(cpdev);
> +			goto err_clk;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_clk:
> +	clk_disable_unprepare(sdc->clk);
> +	return ret;
> +}
> +
> +static int aspeed_sdc_remove(struct platform_device *pdev)
> +{
> +	struct aspeed_sdc *sdc = dev_get_drvdata(&pdev->dev);
> +
> +	clk_disable_unprepare(sdc->clk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id aspeed_sdc_of_match[] = {
> +	{ .compatible = "aspeed,ast2400-sd-controller", },
> +	{ .compatible = "aspeed,ast2500-sd-controller", },
> +	{ .compatible = "aspeed,ast2600-sd-controller", },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, aspeed_sdc_of_match);
> +
> +static struct platform_driver aspeed_sdc_driver = {
> +	.driver		= {
> +		.name	= "sd-controller-aspeed",
> +		.pm	= &sdhci_pltfm_pmops,
> +		.of_match_table = aspeed_sdc_of_match,
> +	},
> +	.probe		= aspeed_sdc_probe,
> +	.remove		= aspeed_sdc_remove,
> +};
> +
> +static int __init aspeed_sdc_init(void)
> +{
> +	int rc;
> +
> +	rc = platform_driver_register(&aspeed_sdhci_driver);
> +	if (rc < 0)
> +		return rc;
> +
> +	return platform_driver_register(&aspeed_sdc_driver);

Shouldn't aspeed_sdhci_driver be unregistered here if there was an error.

> +}
> +module_init(aspeed_sdc_init);
> +
> +static void __exit aspeed_sdc_exit(void)
> +{
> +	platform_driver_unregister(&aspeed_sdc_driver);
> +	platform_driver_unregister(&aspeed_sdhci_driver);
> +}
> +module_exit(aspeed_sdc_exit);
> +
> +MODULE_DESCRIPTION("Driver for the ASPEED SD/SDIO/SDHCI Controllers");
> +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
> +MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>");
> +MODULE_LICENSE("GPL");
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 2/5] firmware: arm_scmi: Make use SCMI v2.0 fastchannel for performance protocol
From: Sudeep Holla @ 2019-08-05 13:28 UTC (permalink / raw)
  To: Etienne Carriere
  Cc: Peng Fan, Souvik Chakravarty, wesleys, Ionela Voinescu, aidapala,
	Sudeep Holla, linux-kernel, Saeed Nowshadi, Quentin Perret,
	Bo Zhang, Felix Burton, Jim Quinlan, Chris Redpath, pajay,
	Gaku Inami, Volodymyr Babchuk, linux-arm-kernel
In-Reply-To: <CAN5uoS-dgtz55O+AAxEFkgtijpHj_-NSy7SkNRAkhEJN5v4PWA@mail.gmail.com>

On Mon, Aug 05, 2019 at 03:16:41PM +0200, Etienne Carriere wrote:
> On Fri, 26 Jul 2019 at 16:00, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > SCMI v2.0 adds support for "FastChannel" which do not use a message
> > header as they are specialized for a single message.
> >
> > Only PERFORMANCE_LIMITS_{SET,GET} and PERFORMANCE_LEVEL_{SET,GET}
> > commands are supported over fastchannels. As they are optional, they
> > need to be discovered by PERFORMANCE_DESCRIBE_FASTCHANNEL command.
> > Further {LIMIT,LEVEL}_SET commands can have optional doorbell support.
> >
> > Add support for making use of these fastchannels.
> >
> > Cc: Ionela Voinescu <Ionela.Voinescu@arm.com>
> > Cc: Chris Redpath <Chris.Redpath@arm.com>
> > Cc: Quentin Perret <Quentin.Perret@arm.com>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/perf.c | 104 +++++++++++++++++++++++++++++--
> >  1 file changed, 100 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> > index 6cce3e82e81e..b9144efbd6fb 100644
> > --- a/drivers/firmware/arm_scmi/perf.c
> > +++ b/drivers/firmware/arm_scmi/perf.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/bits.h>
> >  #include <linux/of.h>
> >  #include <linux/io.h>
> > +#include <linux/io-64-nonatomic-hi-lo.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_opp.h>
> >  #include <linux/sort.h>
> > @@ -293,7 +294,42 @@ scmi_perf_describe_levels_get(const struct scmi_handle *handle, u32 domain,
> >         return ret;
> >  }
> >
> > -static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
> > +#define SCMI_PERF_FC_RING_DB(doorbell, w)              \
>
> Suggest to reformat into a macro style:
>     #define SCMI_PERF_FC_RING_DB(doorbell, w)              \
>         do { \
>                 (...) \
>         } while (0)
>

Sure I can try that.

> > +{                                                      \
> > +       u##w val = 0;                                   \
> > +       struct scmi_fc_db_info *db = doorbell;          \
> > +                                                       \
> > +       if ((db)->mask)                                 \
>
> remove parentheses. I would say, could simply remove `if (db->mask)` here.

Ah, missed to drop this one. We can avoid reading the value if mask = 0,
so I prefer to keep it.
>
> > +               val = ioread##w(db->addr) & db->mask;   \
> > +       iowrite##w((u##w)db->set | val, db->addr);      \
> > +}
> > +
> > +static void scmi_perf_fc_ring_db(struct scmi_fc_db_info *db)
> > +{
> > +       if (!db || !db->addr)
> > +               return;
> > +
> > +       if (db->width == 1)
> > +               SCMI_PERF_FC_RING_DB(db, 8)
> > +       else if (db->width == 2)
> > +               SCMI_PERF_FC_RING_DB(db, 16)
> > +       else if (db->width == 4)
> > +               SCMI_PERF_FC_RING_DB(db, 32)
> > +       else /* db->width == 8 */
> > +#ifdef CONFIG_64BIT
> > +               SCMI_PERF_FC_RING_DB(db, 64)
> > +#else
> > +       {
> > +               u64 val = 0;
> > +
> > +               if (db->mask)
> > +                       val = ioread64_hi_lo(db->addr) & db->mask;
> > +               iowrite64_hi_lo(db->set, db->addr);
>
> Is `value` missing here?
> Why not using SCMI_PERF_FC_RING_DB(db, 64) here?
>

I am still using it. Just if CONFIG_64BIT is enabled and  io{read,write}64
are defined. ARM32/MIPS and other 32-bit platform build might fail
otherwise. I don't want to restrict SCMI to 64-bit platforms only.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 0/9] arm64: Stolen time support
From: Marc Zyngier @ 2019-08-05 13:26 UTC (permalink / raw)
  To: Steven Price
  Cc: Radim Krčmář, kvm, Suzuki K Pouloze,
	Catalin Marinas, linux-doc, Russell King, linux-kernel,
	James Morse, Julien Thierry, Paolo Bonzini, Will Deacon, kvmarm,
	linux-arm-kernel
In-Reply-To: <6789f477-8ab5-cc54-1ad2-8627917b07c9@arm.com>

On 05/08/2019 14:06, Steven Price wrote:
> On 03/08/2019 19:05, Marc Zyngier wrote:
>> On Fri,  2 Aug 2019 15:50:08 +0100
>> Steven Price <steven.price@arm.com> wrote:
>>
>> Hi Steven,
>>
>>> This series add support for paravirtualized time for arm64 guests and
>>> KVM hosts following the specification in Arm's document DEN 0057A:
>>>
>>> https://developer.arm.com/docs/den0057/a
>>>
>>> It implements support for stolen time, allowing the guest to
>>> identify time when it is forcibly not executing.
>>>
>>> It doesn't implement support for Live Physical Time (LPT) as there are
>>> some concerns about the overheads and approach in the above
>>> specification, and I expect an updated version of the specification to
>>> be released soon with just the stolen time parts.
>>
>> Thanks for posting this.
>>
>> My current concern with this series is around the fact that we allocate
>> memory from the kernel on behalf of the guest. It is the first example
>> of such thing in the ARM port, and I can't really say I'm fond of it.
>>
>> x86 seems to get away with it by having the memory allocated from
>> userspace, why I tend to like more. Yes, put_user is more
>> expensive than a straight store, but this isn't done too often either.
>>
>> What is the rational for your current approach?
> 
> As I see it there are 3 approaches that can be taken here:
> 
> 1. Hypervisor allocates memory and adds it to the virtual machine. This
> means that everything to do with the 'device' is encapsulated behind the
> KVM_CREATE_DEVICE / KVM_[GS]ET_DEVICE_ATTR ioctls. But since we want the
> stolen time structure to be fast it cannot be a trapping region and has
> to be backed by real memory - in this case allocated by the host kernel.
> 
> 2. Host user space allocates memory. Similar to above, but this time
> user space needs to manage the memory region as well as the usual
> KVM_CREATE_DEVICE dance. I've no objection to this, but it means
> kvmtool/QEMU needs to be much more aware of what is going on (e.g. how
> to size the memory region).
> 
> 3. Guest kernel "donates" the memory to the hypervisor for the
> structure. As far as I'm aware this is what x86 does. The problems I see
> this approach are:
> 
>  a) kexec becomes much more tricky - there needs to be a disabling
> mechanism for the guest to stop the hypervisor scribbling on memory
> before starting the new kernel.
> 
>  b) If there is more than one entity that is interested in the
> information (e.g. firmware and kernel) then this requires some form of
> arbitration in the guest because the hypervisor doesn't want to have to
> track an arbitrary number of regions to update.
> 
>  c) Performance can suffer if the host kernel doesn't have a suitably
> aligned/sized area to use. As you say - put_user() is more expensive.
> The structure is updated on every return to the VM.
> 
> 
> Of course x86 does prove the third approach can work, but I'm not sure
> which is actually better. Avoid the kexec cancellation requirements was
> the main driver of the current approach. Although many of the
> conversations about this were also tied up with Live Physical Time which
> adds its own complications.

My current train of thoughts is around (2):

- We don't need a new mechanism to track pages or deal with overlapping
IPA ranges
- We can get rid of the save/restore interface

The drawback is that the amount of memory required per vcpu becomes ABI.
I don't think that's a huge deal, as the hypervisor has the same
contract with the guest.

We also take a small hit with put_user(), but this is only done as a
consequence of vcpu_load() (and not on every entry as you suggest
above). It'd be worth quantifying this overhead before making any
decision one way or another.

Thanks,

	M.
-- 
Jazz is not dead, it just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v1 0/16] drm: panel related updates
From: Emil Velikov @ 2019-08-05 13:18 UTC (permalink / raw)
  To: Sam Ravnborg
  Cc: Neil Armstrong, David Airlie, dri-devel, Thierry Reding,
	Laurent Pinchart, Marek Vasut, linux-samsung-soc, Vincent Abriou,
	Krzysztof Kozlowski, Jonathan Hunter, Maxime Ripard, Kukjin Kim,
	NXP Linux Team, Jonas Karlman, Alison Wang, Gwan-gyeong Mun,
	Alexios Zavras, Laurent Pinchart, linux-tegra, Thomas Gleixner,
	Sean Paul, Allison Randal, linux-arm-kernel, Jernej Skrabec,
	Enrico Weigelt, Seung-Woo Kim, Kyungmin Park,
	Pengutronix Kernel Team, Shawn Guo
In-Reply-To: <20190804201637.1240-1-sam@ravnborg.org>

On 2019/08/04, Sam Ravnborg wrote:
> The first 9 patches replaces direct use of the drm_panel
> function pointers with their drm_panel_* counterparts.
> The function pointers are only supposed to be used by
> the drm_panel infrastructure and direct use are discouraged.
> 
> ili9322 is updated to handle bus_flags in get_modes like everyone else.
> This is in preparation for a later patch series where controller
> becomes an arugument to get_modes() and not like today where drm_panel
> is attached to a controller.
> 
> The remaining patches move functionality to the drm_panel core that
> today are repeated in many drivers.
> As preparation for this the inline functions are moved to drm_panel.c
> and kernel-doc is made inline.
> panel-simple is updated to benefit from the additional infrastructure
> and is an example for the simplifications that can be done.
> 
> The patchset has been tested on my embedded target,
> and build tested.
> 
> Feedback welcome!
> 
> The "fix opencoded" patches are all independent and can be applied
> out of order. They were kept here to keep panel related patches in one series.
> 
> 	Sam
> 
Thanks for working on this Sam.

Patches 1-13 are:
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

-Emil

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 2/5] firmware: arm_scmi: Make use SCMI v2.0 fastchannel for performance protocol
From: Etienne Carriere @ 2019-08-05 13:16 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Peng Fan, Souvik Chakravarty, wesleys, Ionela Voinescu, aidapala,
	linux-kernel, Saeed Nowshadi, Quentin Perret, Bo Zhang,
	Felix Burton, Jim Quinlan, Chris Redpath, pajay, Gaku Inami,
	Volodymyr Babchuk, linux-arm-kernel
In-Reply-To: <20190726135954.11078-3-sudeep.holla@arm.com>

On Fri, 26 Jul 2019 at 16:00, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> SCMI v2.0 adds support for "FastChannel" which do not use a message
> header as they are specialized for a single message.
>
> Only PERFORMANCE_LIMITS_{SET,GET} and PERFORMANCE_LEVEL_{SET,GET}
> commands are supported over fastchannels. As they are optional, they
> need to be discovered by PERFORMANCE_DESCRIBE_FASTCHANNEL command.
> Further {LIMIT,LEVEL}_SET commands can have optional doorbell support.
>
> Add support for making use of these fastchannels.
>
> Cc: Ionela Voinescu <Ionela.Voinescu@arm.com>
> Cc: Chris Redpath <Chris.Redpath@arm.com>
> Cc: Quentin Perret <Quentin.Perret@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/perf.c | 104 +++++++++++++++++++++++++++++--
>  1 file changed, 100 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index 6cce3e82e81e..b9144efbd6fb 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -8,6 +8,7 @@
>  #include <linux/bits.h>
>  #include <linux/of.h>
>  #include <linux/io.h>
> +#include <linux/io-64-nonatomic-hi-lo.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_opp.h>
>  #include <linux/sort.h>
> @@ -293,7 +294,42 @@ scmi_perf_describe_levels_get(const struct scmi_handle *handle, u32 domain,
>         return ret;
>  }
>
> -static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
> +#define SCMI_PERF_FC_RING_DB(doorbell, w)              \

Suggest to reformat into a macro style:
    #define SCMI_PERF_FC_RING_DB(doorbell, w)              \
        do { \
                (...) \
        } while (0)

> +{                                                      \
> +       u##w val = 0;                                   \
> +       struct scmi_fc_db_info *db = doorbell;          \
> +                                                       \
> +       if ((db)->mask)                                 \

remove parentheses. I would say, could simply remove `if (db->mask)` here.

> +               val = ioread##w(db->addr) & db->mask;   \
> +       iowrite##w((u##w)db->set | val, db->addr);      \
> +}
> +
> +static void scmi_perf_fc_ring_db(struct scmi_fc_db_info *db)
> +{
> +       if (!db || !db->addr)
> +               return;
> +
> +       if (db->width == 1)
> +               SCMI_PERF_FC_RING_DB(db, 8)
> +       else if (db->width == 2)
> +               SCMI_PERF_FC_RING_DB(db, 16)
> +       else if (db->width == 4)
> +               SCMI_PERF_FC_RING_DB(db, 32)
> +       else /* db->width == 8 */
> +#ifdef CONFIG_64BIT
> +               SCMI_PERF_FC_RING_DB(db, 64)
> +#else
> +       {
> +               u64 val = 0;
> +
> +               if (db->mask)
> +                       val = ioread64_hi_lo(db->addr) & db->mask;
> +               iowrite64_hi_lo(db->set, db->addr);

Is `value` missing here?
Why not using SCMI_PERF_FC_RING_DB(db, 64) here?



> +       }
> +#endif
> +}
> +
> +static int scmi_perf_mb_limits_set(const struct scmi_handle *handle, u32 domain,
>                                    u32 max_perf, u32 min_perf)
>  {
>         int ret;
> @@ -316,7 +352,23 @@ static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
>         return ret;
>  }
>
> -static int scmi_perf_limits_get(const struct scmi_handle *handle, u32 domain,
> +static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
> +                               u32 max_perf, u32 min_perf)
> +{
> +       struct scmi_perf_info *pi = handle->perf_priv;
> +       struct perf_dom_info *dom = pi->dom_info + domain;
> +
> +       if (dom->fc_info && dom->fc_info->limit_set_addr) {
> +               iowrite32(max_perf, dom->fc_info->limit_set_addr);
> +               iowrite32(min_perf, dom->fc_info->limit_set_addr + 4);
> +               scmi_perf_fc_ring_db(dom->fc_info->limit_set_db);
> +               return 0;
> +       }
> +
> +       return scmi_perf_mb_limits_set(handle, domain, max_perf, min_perf);
> +}
> +
> +static int scmi_perf_mb_limits_get(const struct scmi_handle *handle, u32 domain,
>                                    u32 *max_perf, u32 *min_perf)
>  {
>         int ret;
> @@ -342,7 +394,22 @@ static int scmi_perf_limits_get(const struct scmi_handle *handle, u32 domain,
>         return ret;
>  }
>
> -static int scmi_perf_level_set(const struct scmi_handle *handle, u32 domain,
> +static int scmi_perf_limits_get(const struct scmi_handle *handle, u32 domain,
> +                               u32 *max_perf, u32 *min_perf)
> +{
> +       struct scmi_perf_info *pi = handle->perf_priv;
> +       struct perf_dom_info *dom = pi->dom_info + domain;
> +
> +       if (dom->fc_info && dom->fc_info->limit_get_addr) {
> +               *max_perf = ioread32(dom->fc_info->limit_get_addr);
> +               *min_perf = ioread32(dom->fc_info->limit_get_addr + 4);
> +               return 0;
> +       }
> +
> +       return scmi_perf_mb_limits_get(handle, domain, max_perf, min_perf);
> +}
> +
> +static int scmi_perf_mb_level_set(const struct scmi_handle *handle, u32 domain,
>                                   u32 level, bool poll)
>  {
>         int ret;
> @@ -365,7 +432,22 @@ static int scmi_perf_level_set(const struct scmi_handle *handle, u32 domain,
>         return ret;
>  }
>
> -static int scmi_perf_level_get(const struct scmi_handle *handle, u32 domain,
> +static int scmi_perf_level_set(const struct scmi_handle *handle, u32 domain,
> +                              u32 level, bool poll)
> +{
> +       struct scmi_perf_info *pi = handle->perf_priv;
> +       struct perf_dom_info *dom = pi->dom_info + domain;
> +
> +       if (dom->fc_info && dom->fc_info->level_set_addr) {
> +               iowrite32(level, dom->fc_info->level_set_addr);
> +               scmi_perf_fc_ring_db(dom->fc_info->level_set_db);
> +               return 0;
> +       }
> +
> +       return scmi_perf_mb_level_set(handle, domain, level, poll);
> +}
> +
> +static int scmi_perf_mb_level_get(const struct scmi_handle *handle, u32 domain,
>                                   u32 *level, bool poll)
>  {
>         int ret;
> @@ -387,6 +469,20 @@ static int scmi_perf_level_get(const struct scmi_handle *handle, u32 domain,
>         return ret;
>  }
>
> +static int scmi_perf_level_get(const struct scmi_handle *handle, u32 domain,
> +                              u32 *level, bool poll)
> +{
> +       struct scmi_perf_info *pi = handle->perf_priv;
> +       struct perf_dom_info *dom = pi->dom_info + domain;
> +
> +       if (dom->fc_info && dom->fc_info->level_get_addr) {
> +               *level = ioread32(dom->fc_info->level_get_addr);
> +               return 0;
> +       }
> +
> +       return scmi_perf_mb_level_get(handle, domain, level, poll);
> +}
> +
>  static bool scmi_perf_fc_size_is_valid(u32 msg, u32 size)
>  {
>         if ((msg == PERF_LEVEL_GET || msg == PERF_LEVEL_SET) && size == 4)
> --
> 2.17.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v1 14/16] drm/panel: call prepare/enable only once
From: Emil Velikov @ 2019-08-05 13:15 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Neil Armstrong, David Airlie, dri-devel, Thierry Reding,
	Sam Ravnborg, Marek Vasut, linux-samsung-soc, Sean Paul,
	Allison Randal, Krzysztof Kozlowski, Jonathan Hunter,
	Maxime Ripard, Kukjin Kim, NXP Linux Team, Jonas Karlman,
	Alison Wang, Gwan-gyeong Mun, Alexios Zavras, Laurent Pinchart,
	linux-tegra, Thomas Gleixner, Vincent Abriou, linux-arm-kernel,
	Jernej Skrabec, Enrico Weigelt, Seung-Woo Kim, Kyungmin Park,
	Pengutronix Kernel Team, Shawn Guo
In-Reply-To: <20190805105928.GI29747@pendragon.ideasonboard.com>

On 2019/08/05, Laurent Pinchart wrote:
> Hi Sam,
> 
> Thank you for the patch.
> 
> On Sun, Aug 04, 2019 at 10:16:35PM +0200, Sam Ravnborg wrote:
> > Many panel drivers duplicate logic to prevent prepare to be called
> > for a panel that is already prepared.
> > Likewise for enable.
> > 
> > Implement this logic in drm_panel so the individual drivers
> > no longer needs this.
> > A panel is considered prepared/enabled only if the prepare/enable call
> > succeeds.
> > For disable/unprepare it is unconditionally considered
> > disabled/unprepared.
> > 
> > This allows calls to prepare/enable again, even if there were
> > some issue disabling a regulator or similar during disable/unprepare.
> 
> Is this the right place to handle this ? Shouldn't the upper layers
> ensure than enable/disable and prepare/unprepare are correcty balanced,
> and not called multiple times ? Adding enabled and prepared state to
> drm_panel not only doesn't align well with atomic state handling, but
> also would hide issues in upper layers that should really be fixed
> there.
> 
Fully agreed. Mistakes happen - hiding them, by returning "success" does
not sound like a wise approach.

HTH
Emil

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 3/9] KVM: arm64: Implement PV_FEATURES call
From: Steven Price @ 2019-08-05 13:14 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Radim Krčmář, kvm, Suzuki K Pouloze,
	Catalin Marinas, linux-doc, Russell King, linux-kernel,
	James Morse, Julien Thierry, Paolo Bonzini, Will Deacon, kvmarm,
	linux-arm-kernel
In-Reply-To: <20190803122124.7700f700@why>

On 03/08/2019 12:21, Marc Zyngier wrote:
> On Fri,  2 Aug 2019 15:50:11 +0100
> Steven Price <steven.price@arm.com> wrote:
> 
>> This provides a mechanism for querying which paravirtualized features
>> are available in this hypervisor.
>>
>> Also add the header file which defines the ABI for the paravirtualized
>> clock features we're about to add.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>>  arch/arm64/include/asm/pvclock-abi.h | 20 ++++++++++++++++++++
>>  include/linux/arm-smccc.h            | 14 ++++++++++++++
>>  virt/kvm/arm/hypercalls.c            |  9 +++++++++
>>  3 files changed, 43 insertions(+)
>>  create mode 100644 arch/arm64/include/asm/pvclock-abi.h
>>
>> diff --git a/arch/arm64/include/asm/pvclock-abi.h b/arch/arm64/include/asm/pvclock-abi.h
>> new file mode 100644
>> index 000000000000..1f7cdc102691
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/pvclock-abi.h
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (C) 2019 Arm Ltd. */
>> +
>> +#ifndef __ASM_PVCLOCK_ABI_H
>> +#define __ASM_PVCLOCK_ABI_H
>> +
>> +/* The below structures and constants are defined in ARM DEN0057A */
>> +
>> +struct pvclock_vcpu_stolen_time_info {
>> +	__le32 revision;
>> +	__le32 attributes;
>> +	__le64 stolen_time;
>> +	/* Structure must be 64 byte aligned, pad to that size */
>> +	u8 padding[48];
>> +} __packed;
>> +
>> +#define PV_VM_TIME_NOT_SUPPORTED	-1
> 
> Isn't the intent for this to be the same value as
> SMCCC_RET_NOT_SUPPORTED?

Yes it is, I guess there's not much point defining it again.

>> +#define PV_VM_TIME_INVALID_PARAMETERS	-2
> 
> It overlaps with SMCCC_RET_NOT_REQUIRED. Is that a problem? Should we
> consider a spec change for this?

Actually INVALID_PARAMETERS is only for Live Physical Time, since we're
not implementing it here, this can go as well.

Thanks,

Steve

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v4 07/10] regulator: mt6358: Add support for MT6358 regulator
From: Mark Brown @ 2019-08-05 13:10 UTC (permalink / raw)
  To: Hsin-Hsiung Wang
  Cc: Mark Rutland, Alessandro Zummo, Alexandre Belloni, srv_heupstream,
	devicetree, Sean Wang, Liam Girdwood, linux-kernel,
	Richard Fontana, Rob Herring, linux-mediatek, Allison Randal,
	linux-rtc, Matthias Brugger, Thomas Gleixner, Eddie Huang,
	Lee Jones, Kate Stewart, linux-arm-kernel
In-Reply-To: <1564982518-32163-8-git-send-email-hsin-hsiung.wang@mediatek.com>


[-- Attachment #1.1: Type: text/plain, Size: 1282 bytes --]

On Mon, Aug 05, 2019 at 01:21:55PM +0800, Hsin-Hsiung Wang wrote:

> +static const u32 vmch_voltages[] = {
> +	2900000, 3000000, 3300000,
> +};

> +static const u32 vemc_voltages[] = {
> +	2900000, 3000000, 3300000,
> +};

Several of these tables appear to be identical.

> +static inline unsigned int mt6358_map_mode(unsigned int mode)
> +{
> +	return mode == MT6358_BUCK_MODE_AUTO ?
> +		REGULATOR_MODE_NORMAL : REGULATOR_MODE_FAST;
> +}

There is no need for this to be an inline and please write normal
conditional statements to improve legibility.  There's other examples in
the driver.

> +static int mt6358_get_buck_voltage_sel(struct regulator_dev *rdev)
> +{
> +	int ret, regval;
> +	struct mt6358_regulator_info *info = rdev_get_drvdata(rdev);
> +
> +	ret = regmap_read(rdev->regmap, info->da_vsel_reg, &regval);
> +	if (ret != 0) {
> +		dev_info(&rdev->dev,
> +			 "Failed to get mt6358 Buck %s vsel reg: %d\n",
> +			 info->desc.name, ret);

dev_err() for errors here and throughout the driver.

> +		return ret;
> +	}
> +
> +	ret = (regval >> info->da_vsel_shift) & info->da_vsel_mask;
> +
> +	return ret;
> +}

This looks like a standard get_voltage_sel_regmap()?

> +err_mode:
> +	if (ret != 0)
> +		return ret;
> +
> +	return 0;

Or just return ret unconditionally?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 1/9] KVM: arm64: Document PV-time interface
From: Steven Price @ 2019-08-05 13:06 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: peter.maydell, Radim Krčmář, kvm, Suzuki K Pouloze,
	Catalin Marinas, linux-doc, Russell King, linux-kernel,
	James Morse, Julien Thierry, Paolo Bonzini, Will Deacon, kvmarm,
	linux-arm-kernel
In-Reply-To: <20190803121343.2f482200@why>

On 03/08/2019 12:13, Marc Zyngier wrote:
> On Fri,  2 Aug 2019 15:50:09 +0100
> Steven Price <steven.price@arm.com> wrote:
> 
> [+Peter for the userspace aspect of things]
> 
> Hi Steve,
> 
>> Introduce a paravirtualization interface for KVM/arm64 based on the
>> "Arm Paravirtualized Time for Arm-Base Systems" specification DEN 0057A.
>>
>> This only adds the details about "Stolen Time" as the details of "Live
>> Physical Time" have not been fully agreed.
>>
>> User space can specify a reserved area of memory for the guest and
>> inform KVM to populate the memory with information on time that the host
>> kernel has stolen from the guest.
>>
>> A hypercall interface is provided for the guest to interrogate the
>> hypervisor's support for this interface and the location of the shared
>> memory structures.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>>  Documentation/virtual/kvm/arm/pvtime.txt | 107 +++++++++++++++++++++++
>>  1 file changed, 107 insertions(+)
>>  create mode 100644 Documentation/virtual/kvm/arm/pvtime.txt
>>
>> diff --git a/Documentation/virtual/kvm/arm/pvtime.txt b/Documentation/virtual/kvm/arm/pvtime.txt
>> new file mode 100644
>> index 000000000000..e6ae9799e1d5
>> --- /dev/null
>> +++ b/Documentation/virtual/kvm/arm/pvtime.txt
>> @@ -0,0 +1,107 @@
>> +Paravirtualized time support for arm64
>> +======================================
>> +
>> +Arm specification DEN0057/A defined a standard for paravirtualised time
>> +support for Aarch64 guests:
> 
> nit: AArch64
> 
>> +
>> +https://developer.arm.com/docs/den0057/a
> 
> Between this file and the above document, which one is authoritative?

The above document should be authoritative - although I'm still waiting
for the final version to be published. I'm not expecting any changes to
the stolen time part though.

>> +
>> +KVM/Arm64 implements the stolen time part of this specification by providing
> 
> nit: KVM/arm64
> 
>> +some hypervisor service calls to support a paravirtualized guest obtaining a
>> +view of the amount of time stolen from its execution.
>> +
>> +Two new SMCCC compatible hypercalls are defined:
>> +
>> +PV_FEATURES 0xC5000020
>> +PV_TIME_ST  0xC5000022
>> +
>> +These are only available in the SMC64/HVC64 calling convention as
>> +paravirtualized time is not available to 32 bit Arm guests.
>> +
>> +PV_FEATURES
>> +    Function ID:  (uint32)  : 0xC5000020
>> +    PV_func_id:   (uint32)  : Either PV_TIME_LPT or PV_TIME_ST
>> +    Return value: (int32)   : NOT_SUPPORTED (-1) or SUCCESS (0) if the relevant
>> +                              PV-time feature is supported by the hypervisor.
> 
> How is PV_FEATURES discovered? Is the intention to make it a generic
> ARM-wide PV discovery mechanism, not specific to PV time?

SMCCC is mandated for PV time. So, assuming the hypervisor supports
SMCCC, the "NOT_SUPPORTED" return is mandated by SMCCC if PV time isn't
supported.

However, we do also use the SMCCC_ARCH_FEATURES mechanism to check the
existence of PV_FEATURES before use. I'll update the document to call
this out.

>> +
>> +PV_TIME_ST
>> +    Function ID:  (uint32)  : 0xC5000022
>> +    Return value: (int64)   : IPA of the stolen time data structure for this
>> +                              (V)CPU. On failure:
>> +                              NOT_SUPPORTED (-1)
>> +
> 
> Is the size implicit? What are the memory attributes? This either needs
> documenting here, or point to the right bit to the spec.

The size is implicit - it's a pointer to the below structure, so the
guest can only rely on the first 16 bytes being valid. The memory
attributes are described in the specification as:

"The calling guest can map the IPA into normal memory with inner and
outer write back caching attributes, in the inner sharable domain"

I'll put those details in this document for completeness.

>> +Stolen Time
>> +-----------
>> +
>> +The structure pointed to by the PV_TIME_ST hypercall is as follows:
>> +
>> +  Field       | Byte Length | Byte Offset | Description
>> +  ----------- | ----------- | ----------- | --------------------------
>> +  Revision    |      4      |      0      | Must be 0 for version 0.1
>> +  Attributes  |      4      |      4      | Must be 0
>> +  Stolen time |      8      |      8      | Stolen time in unsigned
>> +              |             |             | nanoseconds indicating how
>> +              |             |             | much time this VCPU thread
>> +              |             |             | was involuntarily not
>> +              |             |             | running on a physical CPU.
>> +
>> +The structure will be updated by the hypervisor periodically as time is stolen
> 
> Is it really periodic? If so, when is the update frequency?

Hmm, periodic might be the wrong term - there is no guaranteed frequency
of update. The spec says:

"The hypervisor must update this value prior to scheduling a virtual CPU"

I guess that's probably the best description.

>> +from the VCPU. It will be present within a reserved region of the normal
>> +memory given to the guest. The guest should not attempt to write into this
>> +memory. There is a structure by VCPU of the guest.
> 
> What if the vcpu writes to it? Does it get a fault?

From the perspective from the specification this is undefined. A fault
would therefore be acceptable but isn't generated in the implementation
defined here.

> If there is a
> structure per vcpu, what is the layout in memory? How does a vcpu find
> its own data structure? Is that the address returned by PV_TIME_ST?

A call to PV_TIME_ST returns the structure for the calling vCPU - I'll
make that explicit. The layout is therefore defined by the hypervisor
and cannot be relied on by the guest. As below this implementation uses
a simple array of structures.

>> +
>> +User space interface
>> +====================
>> +
>> +User space can request that KVM provide the paravirtualized time interface to
>> +a guest by creating a KVM_DEV_TYPE_ARM_PV_TIME device, for example:
>> +
>> +    struct kvm_create_device pvtime_device = {
>> +            .type = KVM_DEV_TYPE_ARM_PV_TIME,
>> +            .attr = 0,
>> +            .flags = 0,
>> +    };
>> +
>> +    pvtime_fd = ioctl(vm_fd, KVM_CREATE_DEVICE, &pvtime_device);
>> +
>> +The guest IPA of the structures must be given to KVM. This is the base address
> 
> nit: s/guest //
> 
>> +of an array of stolen time structures (one for each VCPU). For example:
>> +
>> +    struct kvm_device_attr st_base = {
>> +            .group = KVM_DEV_ARM_PV_TIME_PADDR,
>> +            .attr = KVM_DEV_ARM_PV_TIME_ST,
>> +            .addr = (u64)(unsigned long)&st_paddr
>> +    };
>> +
>> +    ioctl(pvtime_fd, KVM_SET_DEVICE_ATTR, &st_base);
> 
> So the allocation itself is performed by the kernel? What are the
> ordering requirements between creating vcpus and the device? What are
> the alignment requirements for the base address?

The base address should be page aligned - I'll spell that out.

There are currently no ordering requirements between creating vcpus and
the device. However...

>> +
>> +For migration (or save/restore) of a guest it is necessary to save the contents
>> +of the shared page(s) and later restore them. KVM_DEV_ARM_PV_TIME_STATE_SIZE
>> +provides the size of this data and KVM_DEV_ARM_PV_TIME_STATE allows the state
>> +to be read/written.
> 
> Is the size variable depending on the number of vcpus?

...yes - so restoring the state after migration must be done after
creating the vcpus. I'll point out that the device should created after.

Thanks for the review,

Steve

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 0/9] arm64: Stolen time support
From: Steven Price @ 2019-08-05 13:06 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Radim Krčmář, kvm, Suzuki K Pouloze,
	Catalin Marinas, linux-doc, Russell King, linux-kernel,
	James Morse, Julien Thierry, Paolo Bonzini, Will Deacon, kvmarm,
	linux-arm-kernel
In-Reply-To: <20190803190522.5fec8f7d@why>

On 03/08/2019 19:05, Marc Zyngier wrote:
> On Fri,  2 Aug 2019 15:50:08 +0100
> Steven Price <steven.price@arm.com> wrote:
> 
> Hi Steven,
> 
>> This series add support for paravirtualized time for arm64 guests and
>> KVM hosts following the specification in Arm's document DEN 0057A:
>>
>> https://developer.arm.com/docs/den0057/a
>>
>> It implements support for stolen time, allowing the guest to
>> identify time when it is forcibly not executing.
>>
>> It doesn't implement support for Live Physical Time (LPT) as there are
>> some concerns about the overheads and approach in the above
>> specification, and I expect an updated version of the specification to
>> be released soon with just the stolen time parts.
> 
> Thanks for posting this.
> 
> My current concern with this series is around the fact that we allocate
> memory from the kernel on behalf of the guest. It is the first example
> of such thing in the ARM port, and I can't really say I'm fond of it.
> 
> x86 seems to get away with it by having the memory allocated from
> userspace, why I tend to like more. Yes, put_user is more
> expensive than a straight store, but this isn't done too often either.
> 
> What is the rational for your current approach?

As I see it there are 3 approaches that can be taken here:

1. Hypervisor allocates memory and adds it to the virtual machine. This
means that everything to do with the 'device' is encapsulated behind the
KVM_CREATE_DEVICE / KVM_[GS]ET_DEVICE_ATTR ioctls. But since we want the
stolen time structure to be fast it cannot be a trapping region and has
to be backed by real memory - in this case allocated by the host kernel.

2. Host user space allocates memory. Similar to above, but this time
user space needs to manage the memory region as well as the usual
KVM_CREATE_DEVICE dance. I've no objection to this, but it means
kvmtool/QEMU needs to be much more aware of what is going on (e.g. how
to size the memory region).

3. Guest kernel "donates" the memory to the hypervisor for the
structure. As far as I'm aware this is what x86 does. The problems I see
this approach are:

 a) kexec becomes much more tricky - there needs to be a disabling
mechanism for the guest to stop the hypervisor scribbling on memory
before starting the new kernel.

 b) If there is more than one entity that is interested in the
information (e.g. firmware and kernel) then this requires some form of
arbitration in the guest because the hypervisor doesn't want to have to
track an arbitrary number of regions to update.

 c) Performance can suffer if the host kernel doesn't have a suitably
aligned/sized area to use. As you say - put_user() is more expensive.
The structure is updated on every return to the VM.


Of course x86 does prove the third approach can work, but I'm not sure
which is actually better. Avoid the kexec cancellation requirements was
the main driver of the current approach. Although many of the
conversations about this were also tied up with Live Physical Time which
adds its own complications.

Steve

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 1/9] KVM: arm64: Document PV-time interface
From: Steven Price @ 2019-08-05 13:06 UTC (permalink / raw)
  To: Zenghui Yu
  Cc: kvm, linux-doc, Marc Zyngier, linux-kernel, Russell King,
	Catalin Marinas, Paolo Bonzini, Will Deacon, kvmarm,
	linux-arm-kernel
In-Reply-To: <3bdd764a-b6f5-d17e-a703-d8eb13838efc@huawei.com>

On 05/08/2019 04:23, Zenghui Yu wrote:
> Hi Steven,
> 
> On 2019/8/2 22:50, Steven Price wrote:
>> Introduce a paravirtualization interface for KVM/arm64 based on the
>> "Arm Paravirtualized Time for Arm-Base Systems" specification DEN 0057A.
>>
>> This only adds the details about "Stolen Time" as the details of "Live
>> Physical Time" have not been fully agreed.
>>
>> User space can specify a reserved area of memory for the guest and
>> inform KVM to populate the memory with information on time that the host
>> kernel has stolen from the guest.
>>
>> A hypercall interface is provided for the guest to interrogate the
>> hypervisor's support for this interface and the location of the shared
>> memory structures.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>>   Documentation/virtual/kvm/arm/pvtime.txt | 107 +++++++++++++++++++++++
>>   1 file changed, 107 insertions(+)
>>   create mode 100644 Documentation/virtual/kvm/arm/pvtime.txt
>                                     ^^^^^^^
> This directory has been renamed recently, see:
> 
> https://patchwork.ozlabs.org/patch/1136104/

Thanks for pointing that out - I'll move it in the next version.

Steve

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* [PATCH] dt-bindings: rng: mtk-rng: Add documentation for MT8516
From: Fabien Parent @ 2019-08-05 13:02 UTC (permalink / raw)
  To: robh+dt, matthias.bgg
  Cc: devicetree, linux-kernel, Fabien Parent, linux-mediatek,
	linux-crypto, linux-arm-kernel

This commit adds the device-tree documentation for the RNG IP on the
MediaTek MT8516 SoC.

Signed-off-by: Fabien Parent <fparent@baylibre.com>
---
 Documentation/devicetree/bindings/rng/mtk-rng.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/rng/mtk-rng.txt b/Documentation/devicetree/bindings/rng/mtk-rng.txt
index 2bc89f133701..dfdcb5cd2ea8 100644
--- a/Documentation/devicetree/bindings/rng/mtk-rng.txt
+++ b/Documentation/devicetree/bindings/rng/mtk-rng.txt
@@ -6,6 +6,7 @@ Required properties:
 			"mediatek,mt7622-rng", 	"mediatek,mt7623-rng" : for MT7622
 			"mediatek,mt7629-rng",  "mediatek,mt7623-rng" : for MT7629
 			"mediatek,mt7623-rng" : for MT7623
+			"mediatek,mt8516-rng", "mediatek,mt7623-rng" : for MT8516
 - clocks	    : list of clock specifiers, corresponding to
 		      entries in clock-names property;
 - clock-names	    : Should contain "rng" entries;
-- 
2.23.0.rc1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH 2/2] pwm: pwm-mediatek: Add MT8516 SoC support
From: Fabien Parent @ 2019-08-05 12:58 UTC (permalink / raw)
  To: thierry.reding, robh+dt, matthias.bgg
  Cc: mark.rutland, linux-pwm, devicetree, linux-kernel, Fabien Parent,
	linux-mediatek, linux-arm-kernel
In-Reply-To: <20190805125848.15751-1-fparent@baylibre.com>

Add the compatible and the platform data to support PWM on the MT8516
SoC.

Signed-off-by: Fabien Parent <fparent@baylibre.com>
---
 drivers/pwm/pwm-mediatek.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index eb6674ce995f..6697e30811e7 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -302,11 +302,18 @@ static const struct mtk_pwm_platform_data mt7628_pwm_data = {
 	.has_clks = false,
 };
 
+static const struct mtk_pwm_platform_data mt8516_pwm_data = {
+	.num_pwms = 5,
+	.pwm45_fixup = false,
+	.has_clks = true,
+};
+
 static const struct of_device_id mtk_pwm_of_match[] = {
 	{ .compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data },
 	{ .compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data },
 	{ .compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data },
 	{ .compatible = "mediatek,mt7628-pwm", .data = &mt7628_pwm_data },
+	{ .compatible = "mediatek,mt8516-pwm", .data = &mt8516_pwm_data },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, mtk_pwm_of_match);
-- 
2.23.0.rc1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox