* [PATCH 02/14] arm64: Call ARCH_WORKAROUND_2 on transitions between EL0 and EL1
From: Marc Zyngier @ 2018-05-22 15:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522150648.28297-1-marc.zyngier@arm.com>
In order for the kernel to protect itself, let's call the SSBD mitigation
implemented by the higher exception level (either hypervisor or firmware)
on each transition between userspace and kernel.
We must take the PSCI conduit into account in order to target the
right exception level, hence the introduction of a runtime patching
callback.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
arch/arm64/kernel/cpu_errata.c | 18 ++++++++++++++++++
arch/arm64/kernel/entry.S | 22 ++++++++++++++++++++++
include/linux/arm-smccc.h | 5 +++++
3 files changed, 45 insertions(+)
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index a900befadfe8..46b3aafb631a 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -232,6 +232,24 @@ enable_smccc_arch_workaround_1(const struct arm64_cpu_capabilities *entry)
}
#endif /* CONFIG_HARDEN_BRANCH_PREDICTOR */
+#ifdef CONFIG_ARM64_SSBD
+void __init arm64_update_smccc_conduit(struct alt_instr *alt,
+ __le32 *origptr, __le32 *updptr,
+ int nr_inst)
+{
+ u32 insn;
+
+ BUG_ON(nr_inst != 1);
+
+ if (psci_ops.conduit == PSCI_CONDUIT_HVC)
+ insn = aarch64_insn_get_hvc_value();
+ else
+ insn = aarch64_insn_get_smc_value();
+
+ *updptr = cpu_to_le32(insn);
+}
+#endif /* CONFIG_ARM64_SSBD */
+
#define CAP_MIDR_RANGE(model, v_min, r_min, v_max, r_max) \
.matches = is_affected_midr_range, \
.midr_range = MIDR_RANGE(model, v_min, r_min, v_max, r_max)
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index ec2ee720e33e..f33e6aed3037 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -18,6 +18,7 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
+#include <linux/arm-smccc.h>
#include <linux/init.h>
#include <linux/linkage.h>
@@ -137,6 +138,18 @@ alternative_else_nop_endif
add \dst, \dst, #(\sym - .entry.tramp.text)
.endm
+ // This macro corrupts x0-x3. It is the caller's duty
+ // to save/restore them if required.
+ .macro apply_ssbd, state
+#ifdef CONFIG_ARM64_SSBD
+ mov w0, #ARM_SMCCC_ARCH_WORKAROUND_2
+ mov w1, #\state
+alternative_cb arm64_update_smccc_conduit
+ nop // Patched to SMC/HVC #0
+alternative_cb_end
+#endif
+ .endm
+
.macro kernel_entry, el, regsize = 64
.if \regsize == 32
mov w0, w0 // zero upper 32 bits of x0
@@ -163,6 +176,13 @@ alternative_else_nop_endif
ldr x19, [tsk, #TSK_TI_FLAGS] // since we can unmask debug
disable_step_tsk x19, x20 // exceptions when scheduling.
+ apply_ssbd 1
+
+#ifdef CONFIG_ARM64_SSBD
+ ldp x0, x1, [sp, #16 * 0]
+ ldp x2, x3, [sp, #16 * 1]
+#endif
+
mov x29, xzr // fp pointed to user-space
.else
add x21, sp, #S_FRAME_SIZE
@@ -303,6 +323,8 @@ alternative_if ARM64_WORKAROUND_845719
alternative_else_nop_endif
#endif
3:
+ apply_ssbd 0
+
.endif
msr elr_el1, x21 // set up the return data
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index c89da86de99f..ca1d2cc2cdfa 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -80,6 +80,11 @@
ARM_SMCCC_SMC_32, \
0, 0x8000)
+#define ARM_SMCCC_ARCH_WORKAROUND_2 \
+ ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+ ARM_SMCCC_SMC_32, \
+ 0, 0x7fff)
+
#ifndef __ASSEMBLY__
#include <linux/linkage.h>
--
2.14.2
^ permalink raw reply related
* [PATCH 01/14] arm/arm64: smccc: Add SMCCC-specific return codes
From: Marc Zyngier @ 2018-05-22 15:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522150648.28297-1-marc.zyngier@arm.com>
We've so far used the PSCI return codes for SMCCC because they
were extremely similar. But with the new ARM DEN 0070A specification,
"NOT_REQUIRED" (-2) is clashing with PSCI's "PSCI_RET_INVALID_PARAMS".
Let's bite the bullet and add SMCCC specific return codes. Users
can be repainted as and when required.
Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
include/linux/arm-smccc.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index a031897fca76..c89da86de99f 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -291,5 +291,10 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
*/
#define arm_smccc_1_1_hvc(...) __arm_smccc_1_1(SMCCC_HVC_INST, __VA_ARGS__)
+/* Return codes defined in ARM DEN 0070A */
+#define SMCCC_RET_SUCCESS 0
+#define SMCCC_RET_NOT_SUPPORTED -1
+#define SMCCC_RET_NOT_REQUIRED -2
+
#endif /*__ASSEMBLY__*/
#endif /*__LINUX_ARM_SMCCC_H*/
--
2.14.2
^ permalink raw reply related
* [PATCH 00/14] arm64 SSBD (aka Spectre-v4) mitigation
From: Marc Zyngier @ 2018-05-22 15:06 UTC (permalink / raw)
To: linux-arm-kernel
Hi all,
This patch series implements the Linux kernel side of the "Spectre-v4"
(CVE-2018-3639) mitigation known as "Speculative Store Bypass Disable"
(SSBD).
More information can be found at:
https://bugs.chromium.org/p/project-zero/issues/detail?id=1528
https://developer.arm.com/support/arm-security-updates/speculative-processor-vulnerability
For all released Arm Cortex-A CPUs that are affected by this issue, then
the preferred mitigation is simply to set a chicken bit in the firmware
during CPU initialisation and therefore no change to Linux is required.
Other CPUs may require the chicken bit to be toggled dynamically (for
example, when switching between user-mode and kernel-mode) and this is
achieved by calling into EL3 via an SMC which has been published as part
of the latest SMCCC specification:
https://developer.arm.com/cache-speculation-vulnerability-firmware-specification
as well as an ATF update for the released ARM cores affected by SSDB:
https://github.com/ARM-software/arm-trusted-firmware/pull/1392
These patches provide the following:
1. Safe probing of firmware to establish which CPUs in the system
require calling into EL3 as part of the mitigation.
2. For CPUs that require it, call into EL3 on exception entry/exit
from EL0 to apply the SSBD mitigation when running at EL1.
3. A command-line option to force the SSBD mitigation to be always on,
always off, or dymamically toggled (default) for CPUs that require
the EL3 call.
4. An initial implementation of a prctl() backend for arm64 that allows
userspace tasks to opt-in to the mitigation explicitly. This is
intended to match the interface provided by x86, and so we rely on
their core changes here. There still is an annoying issue with
multithreaded seccomp tasks that get flagged with the mitigation
whilst they are running in userspace.
5. An initial implementation of the call via KVM, which exposes the
mitigation to the guest via an HVC interface. This isn't yet
complete and doesn't include save/restore functionality for the
workaround state.
All comments welcome,
M.
Marc Zyngier (14):
arm/arm64: smccc: Add SMCCC-specific return codes
arm64: Call ARCH_WORKAROUND_2 on transitions between EL0 and EL1
arm64: Add per-cpu infrastructure to call ARCH_WORKAROUND_2
arm64: Add ARCH_WORKAROUND_2 probing
arm64: Add 'ssbd' command-line option
arm64: ssbd: Add global mitigation state accessor
arm64: ssbd: Skip apply_ssbd if not using dynamic mitigation
arm64: ssbd: Disable mitigation on CPU resume if required by user
arm64: ssbd: Introduce thread flag to control userspace mitigation
arm64: ssbd: Add prctl interface for per-thread mitigation
arm64: KVM: Add HYP per-cpu accessors
arm64: KVM: Add ARCH_WORKAROUND_2 support for guests
arm64: KVM: Handle guest's ARCH_WORKAROUND_2 requests
arm64: KVM: Add ARCH_WORKAROUND_2 discovery through
ARCH_FEATURES_FUNC_ID
Documentation/admin-guide/kernel-parameters.txt | 17 +++
arch/arm/include/asm/kvm_host.h | 12 ++
arch/arm/include/asm/kvm_mmu.h | 5 +
arch/arm64/Kconfig | 9 ++
arch/arm64/include/asm/cpucaps.h | 3 +-
arch/arm64/include/asm/cpufeature.h | 22 +++
arch/arm64/include/asm/kvm_asm.h | 30 +++-
arch/arm64/include/asm/kvm_host.h | 26 ++++
arch/arm64/include/asm/kvm_mmu.h | 24 ++++
arch/arm64/include/asm/thread_info.h | 1 +
arch/arm64/kernel/Makefile | 1 +
arch/arm64/kernel/asm-offsets.c | 1 +
arch/arm64/kernel/cpu_errata.c | 173 ++++++++++++++++++++++++
arch/arm64/kernel/entry.S | 30 ++++
arch/arm64/kernel/ssbd.c | 107 +++++++++++++++
arch/arm64/kernel/suspend.c | 8 ++
arch/arm64/kvm/hyp/hyp-entry.S | 38 +++++-
arch/arm64/kvm/hyp/switch.c | 42 ++++++
arch/arm64/kvm/reset.c | 4 +
include/linux/arm-smccc.h | 10 ++
virt/kvm/arm/arm.c | 4 +
virt/kvm/arm/psci.c | 18 ++-
22 files changed, 579 insertions(+), 6 deletions(-)
create mode 100644 arch/arm64/kernel/ssbd.c
--
2.14.2
^ permalink raw reply
* [patch v21 1/4] drivers: jtag: Add JTAG core driver
From: Oleksandr Shamray @ 2018-05-22 15:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAHp75VctG5RBtLF3nT5e_j7_e9O12u=5gerJ6OMxWbABM9ft2w@mail.gmail.com>
Hi Andy.
Thanks for review.
Please read my answers inline.
> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko at gmail.com]
> Sent: 16 ??? 2018 ?. 0:22
> To: Oleksandr Shamray <oleksandrs@mellanox.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Arnd Bergmann
> <arnd@arndb.de>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org>; linux-arm Mailing List
> <linux-arm-kernel@lists.infradead.org>; devicetree
> <devicetree@vger.kernel.org>; openbmc at lists.ozlabs.org; Joel Stanley
> <joel@jms.id.au>; Ji?? P?rko <jiri@resnulli.us>; Tobias Klauser
> <tklauser@distanz.ch>; open list:SERIAL DRIVERS <linux-
> serial at vger.kernel.org>; Vadim Pasternak <vadimp@mellanox.com>;
> system-sw-low-level <system-sw-low-level@mellanox.com>; Rob Herring
> <robh+dt@kernel.org>; openocd-devel-owner at lists.sourceforge.net;
> linux- api at vger.kernel.org; David S. Miller <davem@davemloft.net>;
> Mauro Carvalho Chehab <mchehab@kernel.org>; Jiri Pirko
> <jiri@mellanox.com>
> Subject: Re: [patch v21 1/4] drivers: jtag: Add JTAG core driver
>
> On Tue, May 15, 2018 at 5:21 PM, Oleksandr Shamray
> <oleksandrs@mellanox.com> wrote:
> > Initial patch for JTAG driver
> > JTAG class driver provide infrastructure to support
> > hardware/software JTAG platform drivers. It provide user layer API
> > interface for flashing and debugging external devices which equipped
> > with JTAG interface using standard transactions.
> >
> > Driver exposes set of IOCTL to user space for:
> > - XFER:
> > - SIR (Scan Instruction Register, IEEE 1149.1 Data Register scan);
> > - SDR (Scan Data Register, IEEE 1149.1 Instruction Register scan);
> > - RUNTEST (Forces the IEEE 1149.1 bus to a run state for a specified
> > number of clocks).
> > - SIOCFREQ/GIOCFREQ for setting and reading JTAG frequency.
> >
> > Driver core provides set of internal APIs for allocation and
> > registration:
> > - jtag_register;
> > - jtag_unregister;
> > - jtag_alloc;
> > - jtag_free;
> >
> > Platform driver on registration with jtag-core creates the next
> > entry in dev folder:
> > /dev/jtagX
>
> > 0xB0 all RATIO devices in development:
> > <mailto:vgo@ratio.de>
> > 0xB1 00-1F PPPoX <mailto:mostrows@styx.uwaterloo.ca>
> > +0xB2 00-0f linux/jtag.h JTAG driver
> > +
> > +<mailto:oleksandrs@mellanox.com>
>
> Consider to preserve style (upper vs. lower).
JTAG in code is lower (jtag) cane but in descriptions and notes it is upper (JTAG).
In all places which do not correspond to this I will fix.
>
> > + This provides basic core functionality support for JTAG class devices.
> > + Hardware that is equipped with a JTAG microcontroller can be
> > + supported by using this driver's interfaces.
> > + This driver exposes a set of IOCTLs to the user space for
> > + the following commands:
> > + SDR: (Scan Data Register) Performs an IEEE 1149.1 Data Register scan
> > + SIR: (Scan Instruction Register) Performs an IEEE 1149.1 Instruction
> > + Register scan.
> > + RUNTEST: Forces the IEEE 1149.1 bus to a run state for a specified
> > + number of clocks or a specified time period.
>
> Something feels wrong with formatting here.
>
Will fix
> > +#define MAX_JTAG_NAME_LEN (sizeof("jtag") + 5)
>
> Interesting definition. Why not to set to 10 explicitly? And why 10?
> (16 sounds better)
>
5 - is a max len of JTAG device id in device name. I will add define to it.
In general I don't see the case for the system with hundreds JTAG interfaces.
I will limit maximum jtag master to 255 devices and change id len to 3
#define MAX_JTAG_ID_STR_LEN 5
#define MAX_JTAG_NAME_LEN (sizeof("jtag") + MAX_JTAG_ID_STR_LEN)
> > +struct jtag {
> > + struct miscdevice miscdev;
>
> > + struct device *dev;
>
> Doesn't miscdev parent contain exactly this one?
Yes.
Will fix.
>
> > + const struct jtag_ops *ops;
> > + int id;
> > + bool opened;
> > + struct mutex open_lock;
> > + unsigned long priv[0];
> > +};
>
> > + err = copy_to_user(u64_to_user_ptr(xfer.tdio),
> > + (void *)(xfer_data), data_size);
>
> Redundant parens in one case. Check the rest similar places.
>
Yes.
> > +static int jtag_open(struct inode *inode, struct file *file) {
>
> > + struct jtag *jtag = container_of(file->private_data, struct jtag,
> > + miscdev);
>
> I would don't care about length and put it on one line.
>
But following to LINUX kernel style, it should be no longer than 80 symbols.
It will not pass by ./scripts/checkpatch.pl
Will it be OK to send a patch which failed 80 symbols limitation check?
> > + if (jtag->opened) {
> > + jtag->opened = true;
> > + jtag->opened = false;
>
> Can it be opened non exclusively several times? If so, this needs to
> be a ref counter instead.
It can be opened only once.
>
> > + if (!ops->idle || !ops->mode_set || !ops->status_get || !ops->xfer)
> > + return NULL;
>
> Are all of them mandatory?
>
Yes, except "mode_set"
Will remove mode_set from check
> > +int jtag_register(struct jtag *jtag)
>
> Perhaps devm_ variant.
Jtag driver uses miscdevice and related misc_register and misc_deregister
calls for creation and destruction. There is no device object prior to
call to misc_register, which could be used in devm_jtag_register.
>
> > +#define jtag_u64_to_ptr(arg) ((void *)(uintptr_t)arg)
>
Redundant. Removed.
> Where is this used or supposed to be used?
>
> > +#define JTAG_MAX_XFER_DATA_LEN 65535
>
> Is this limitation from some spec?
> Otherwise why not to allow 64K?
>
It not limited by specification.
But we enforce an upper bound for the length here, to prevent users from draining
kernel memory with giant buffers.
So it was limited by size of unsigned short int (65535)
> > +/**
> > + * struct jtag_ops - callbacks for jtag control functions:
> > + *
> > + * @freq_get: get frequency function. Filled by device driver
> > + * @freq_set: set frequency function. Filled by device driver
> > + * @status_get: set status function. Filled by device driver
> > + * @idle: set JTAG to idle state function. Filled by device driver
> > + * @xfer: send JTAG xfer function. Filled by device driver */
>
> Perhaps you need to describe which of them are _really_ mandatory and
> which are optional.
>
Ok
> --
> With Best Regards,
> Andy Shevchenko
Best Regards,
Oleksandr Shamray
^ permalink raw reply
* [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver
From: Oleksandr Shamray @ 2018-05-22 15:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAHp75Ve7EYiWBiE73i0CmyJhPMOdSKsGzCr0SUvta3Uya=Ov1Q@mail.gmail.com>
Hi Andy.
Thanks for review.
Please read my answers inline.
> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko at gmail.com]
> Sent: 16 ??? 2018 ?. 0:00
> To: Oleksandr Shamray <oleksandrs@mellanox.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Arnd Bergmann
> <arnd@arndb.de>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org>; linux-arm Mailing List
> <linux-arm-kernel@lists.infradead.org>; devicetree
> <devicetree@vger.kernel.org>; openbmc at lists.ozlabs.org; Joel Stanley
> <joel@jms.id.au>; Ji?? P?rko <jiri@resnulli.us>; Tobias Klauser
> <tklauser@distanz.ch>; open list:SERIAL DRIVERS <linux-
> serial at vger.kernel.org>; Vadim Pasternak <vadimp@mellanox.com>;
> system-sw-low-level <system-sw-low-level@mellanox.com>; Rob Herring
> <robh+dt@kernel.org>; openocd-devel-owner at lists.sourceforge.net;
> linux- api at vger.kernel.org; David S. Miller <davem@davemloft.net>;
> Mauro Carvalho Chehab <mchehab@kernel.org>; Jiri Pirko
> <jiri@mellanox.com>
> Subject: Re: [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and
> 25xx families JTAG master driver
>
> On Tue, May 15, 2018 at 5:21 PM, Oleksandr Shamray
> <oleksandrs@mellanox.com> wrote:
> > Driver adds support of Aspeed 2500/2400 series SOC JTAG master
> controller.
> >
> > Driver implements the following jtag ops:
> > - freq_get;
> > - freq_set;
> > - status_get;
> > - idle;
> > - xfer;
> >
> > It has been tested on Mellanox system with BMC equipped with Aspeed
> > 2520 SoC for programming CPLD devices.
>
> > +#define ASPEED_JTAG_DATA 0x00
> > +#define ASPEED_JTAG_INST 0x04
> > +#define ASPEED_JTAG_CTRL 0x08
> > +#define ASPEED_JTAG_ISR 0x0C
> > +#define ASPEED_JTAG_SW 0x10
> > +#define ASPEED_JTAG_TCK 0x14
> > +#define ASPEED_JTAG_EC 0x18
> > +
> > +#define ASPEED_JTAG_DATA_MSB 0x01
> > +#define ASPEED_JTAG_DATA_CHUNK_SIZE 0x20
>
>
> > +#define ASPEED_JTAG_IOUT_LEN(len) (ASPEED_JTAG_CTL_ENG_EN |\
> > + ASPEED_JTAG_CTL_ENG_OUT_EN
> > +|\
> > +
> > +ASPEED_JTAG_CTL_INST_LEN(len))
>
> Better to read
>
> #define MY_COOL_CONST_OR_MACRO(xxx) \
> ...
>
> > +#define ASPEED_JTAG_DOUT_LEN(len) (ASPEED_JTAG_CTL_ENG_EN
> |\
> > + ASPEED_JTAG_CTL_ENG_OUT_EN
> > + |\
> > +
> > +ASPEED_JTAG_CTL_DATA_LEN(len))
>
> Ditto.
Ok. Changed to:
#define ASPEED_JTAG_IOUT_LEN(len) \
(ASPEED_JTAG_CTL_ENG_EN | \
ASPEED_JTAG_CTL_ENG_OUT_EN | \
ASPEED_JTAG_CTL_INST_LEN(len))
#define ASPEED_JTAG_DOUT_LEN(len) \
(ASPEED_JTAG_CTL_ENG_EN | \
ASPEED_JTAG_CTL_ENG_OUT_EN | \
ASPEED_JTAG_CTL_DATA_LEN(len))
>
> > +static char *end_status_str[] = {"idle", "ir pause", "drpause"};
>
> > +static int aspeed_jtag_freq_set(struct jtag *jtag, u32 freq) {
> > + struct aspeed_jtag *aspeed_jtag = jtag_priv(jtag);
> > + unsigned long apb_frq;
> > + u32 tck_val;
> > + u16 div;
> > +
> > + apb_frq = clk_get_rate(aspeed_jtag->pclk);
>
> > + div = (apb_frq % freq == 0) ? (apb_frq / freq) - 1 :
> > + (apb_frq / freq);
>
> Isn't it the same as
>
> div = (apb_frq - 1) / freq;
>
> ?
>
Seems it is same. Thanks.
> > + tck_val = aspeed_jtag_read(aspeed_jtag, ASPEED_JTAG_TCK);
> > + aspeed_jtag_write(aspeed_jtag,
> > + (tck_val & ASPEED_JTAG_TCK_DIVISOR_MASK) | div,
> > + ASPEED_JTAG_TCK);
> > + return 0;
> > +}
>
> > +static void aspeed_jtag_sw_delay(struct aspeed_jtag *aspeed_jtag,
> > +int
> > +cnt) {
> > + int i;
> > +
> > + for (i = 0; i < cnt; i++)
> > + aspeed_jtag_read(aspeed_jtag, ASPEED_JTAG_SW);
>
> Isn't it readsl() (or how it's called, I don't remember).
>
No, readsl reads data into buffer. But in this place read used for make
software delay.
Aspeed jtag driver supports 2 modes:
1 - hw mode with the hardware controlled JTAG states
and pins
2 - with software controlled pins.
This part of code used in sw-mode and generates delay for JTAG bit-bang .
I will change it to ndelay().
> > +}
>
> > +static void aspeed_jtag_wait_instruction_pause(struct aspeed_jtag
> > +*aspeed_jtag) {
> > + wait_event_interruptible(aspeed_jtag->jtag_wq, aspeed_jtag->flag &
> > + ASPEED_JTAG_ISR_INST_PAUSE);
>
> In such cases I prefer to see a new line with a parameter in full.
> Check all places.
>
> > + aspeed_jtag->flag &= ~ASPEED_JTAG_ISR_INST_PAUSE; }
>
> > +static void aspeed_jtag_sm_cycle(struct aspeed_jtag *aspeed_jtag,
> > +const
> u8 *tms,
> > + int len) {
> > + int i;
> > +
> > + for (i = 0; i < len; i++)
> > + aspeed_jtag_tck_cycle(aspeed_jtag, tms[i], 0); }
> > +
> > +static void aspeed_jtag_run_test_idle_sw(struct aspeed_jtag
> *aspeed_jtag,
> > + struct jtag_run_test_idle
> > +*runtest) {
> > + static const u8 sm_pause_irpause[] = {1, 1, 1, 1, 0, 1, 0};
> > + static const u8 sm_pause_drpause[] = {1, 1, 1, 0, 1, 0};
> > + static const u8 sm_idle_irpause[] = {1, 1, 0, 1, 0};
> > + static const u8 sm_idle_drpause[] = {1, 0, 1, 0};
> > + static const u8 sm_pause_idle[] = {1, 1, 0};
> > + int i;
> > +
> > + /* SW mode from idle/pause-> to pause/idle */
> > + if (runtest->reset) {
> > + for (i = 0; i < ASPEED_JTAG_RESET_CNTR; i++)
> > + aspeed_jtag_tck_cycle(aspeed_jtag, 1, 0);
> > + }
>
> I would rather split this big switch to a few helper functions per
> each status of surrounding switch.
>
Ok.
Will do it.
> > +
> > + /* Stay on IDLE for at least TCK cycle */
> > + for (i = 0; i < runtest->tck; i++)
> > + aspeed_jtag_tck_cycle(aspeed_jtag, 0, 0); }
>
>
> > +/**
> > + * aspeed_jtag_run_test_idle:
> > + * JTAG reset: generates at least 9 TMS high and 1 TMS low to force
> > + * devices into Run-Test/Idle State.
> > + */
>
> It's rather broken kernel doc.
Deleted.
>
> > + aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_CTL_ENG_EN |
> > + ASPEED_JTAG_CTL_ENG_OUT_EN |
> > + ASPEED_JTAG_CTL_FORCE_TMS,
> > + ASPEED_JTAG_CTRL);
>
> > + aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_EC_GO_IDLE,
> > + ASPEED_JTAG_EC);
>
> > + aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_SW_MODE_EN |
> > + ASPEED_JTAG_SW_MODE_TDIO, ASPEED_JTAG_SW);
>
> Here you have permutations of flag some of which are repeatetive in
> the code. Perhaps make additional definitions instead.
> Check other similar places.
>
Ok. Will add defined for repeated flags
>
> > + char tdo;
>
> Indentation.
Ok.
>
> > + if (xfer->direction == JTAG_READ_XFER)
> > + tdi = UINT_MAX;
> > + else
> > + tdi = data[index];
>
> > + if (xfer->direction == JTAG_READ_XFER)
> > + tdi = UINT_MAX;
> > + else
> > + tdi = data[index];
>
> Take your time to think how the above duplication can be avoided.
>
In both cases data[] is different, so I should check it twice, but I will
change it to, macro like:
#define ASPEED_JTAG_GET_TDI(direction, data) \
(direction == JTAG_READ_XFER) ? UNIT_MAX : data
> > + }
> > + }
> > +
> > + tdo = aspeed_jtag_tck_cycle(aspeed_jtag, 1, tdi &
> ASPEED_JTAG_DATA_MSB);
> > + data[index] |= tdo << (shift_bits %
> > +ASPEED_JTAG_DATA_CHUNK_SIZE); }
>
>
> > + if (endstate != JTAG_STATE_IDLE) {
>
> Why not to use positive check?
>
Will restructure to have positive check
if (endstate == JTAG_STATE_IDLE) {
...
} else {
...
}
> > + int i;
> > +
> > + for (i = 0; i <= xfer->length / BITS_PER_BYTE; i++) {
> > + pos += snprintf(&dbg_str[pos], sizeof(dbg_str) - pos,
> > + "0x%02x ", xfer_data[i]);
> > + }
>
> Oh, NO! Consider reading printk-formats (for %*ph) and other
> documentation about available APIs.
Ok.
>
> > + if (!(aspeed_jtag->mode & JTAG_XFER_HW_MODE)) {
> > + /* SW mode */
>
> This is rather too complex to be in one function.
>
Will split to separate functions.
> > + } else {
>
> > + /* hw mode */
> > + aspeed_jtag_write(aspeed_jtag, 0, ASPEED_JTAG_SW);
> > + aspeed_jtag_xfer_hw(aspeed_jtag, xfer, data);
>
> For symmetry it might be another function.
>
> > + }
>
> > + dev_dbg(aspeed_jtag->dev, "status %x\n", status);
>
> Perhaps someone should become familiar with tracepoints?
>
> > + dev_err(aspeed_jtag->dev, "irq status:%x\n",
> > + status);
>
>
> Huh, really?! SPAM.
I will review and delete redundant debug messages.
>
> (I would drop it completely, though you may use ratelimited variant)
>
> > + ret = IRQ_NONE;
> > + }
> > + return ret;
> > +}
>
> > + clk_prepare_enable(aspeed_jtag->pclk);
>
> This might fail.
Will add error check
>
> > + dev_dbg(&pdev->dev, "IRQ %d.\n", aspeed_jtag->irq);
>
> Noise even for debug.
Agree.
>
> > + err = jtag_register(jtag);
>
> Perhaps we might have devm_ variant of this. Check how SPI framework
> deal with a such.
>
Jtag driver uses miscdevice and related misc_register and misc_deregister
calls for creation and destruction. There is no device object prior
to call to misc_register, which could be used in devm_jtag_register.
> > +static int aspeed_jtag_remove(struct platform_device *pdev) {
>
> > + struct jtag *jtag;
> > +
> > + jtag = platform_get_drvdata(pdev);
>
> Usually we put this on one line
+
>
> > + aspeed_jtag_deinit(pdev, jtag_priv(jtag));
> > + jtag_unregister(jtag);
> > + jtag_free(jtag);
> > + return 0;
> > +}
>
>
> --
> With Best Regards,
> Andy Shevchenko
Best Regards,
Oleksandr Shamray
^ permalink raw reply
* [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd
From: Ulf Hansson @ 2018-05-22 14:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <f5589b88-acf8-b491-df79-e359e596c8d0@nvidia.com>
[...]
>>
>> +/**
>> + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
>> + * @dev: Device to attach.
>> + * @index: The index of the PM domain.
>> + *
>> + * Parse device's OF node to find a PM domain specifier at the provided @index.
>> + * If such is found, allocates a new device and attaches it to retrieved
>> + * pm_domain ops.
>> + *
>> + * Returns the allocated device if successfully attached PM domain, NULL when
>> + * the device don't need a PM domain or have a single PM domain, else PTR_ERR()
>> + * in case of failures. Note that if a power-domain exists for the device, but
>> + * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to ensure
>> + * that the device is not probed and to re-try again later.
>> + */
>> +struct device *genpd_dev_pm_attach_by_id(struct device *dev,
>> + unsigned int index)
>> +{
>> + struct device *genpd_dev;
>> + int num_domains;
>> + int ret;
>> +
>> + if (!dev->of_node)
>> + return NULL;
>> +
>> + /* Deal only with devices using multiple PM domains. */
>> + num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
>> + "#power-domain-cells");
>> + if (num_domains < 2 || index >= num_domains)
>> + return NULL;
>> +
>> + /* Allocate and register device on the genpd bus. */
>> + genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
>> + if (!genpd_dev)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
>> + genpd_dev->bus = &genpd_bus_type;
>> + genpd_dev->release = genpd_release_dev;
>> +
>> + ret = device_register(genpd_dev);
>> + if (ret) {
>> + kfree(genpd_dev);
>> + return ERR_PTR(ret);
>> + }
>> +
>> + /* Try to attach the device to the PM domain at the specified index. */
>> + ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
>> + if (ret < 1) {
>> + device_unregister(genpd_dev);
>> + return ret ? ERR_PTR(ret) : NULL;
>> + }
>> +
>> + pm_runtime_set_active(genpd_dev);
>> + pm_runtime_enable(genpd_dev);
>> +
>> + return genpd_dev;
>> +}
>> +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);
>
> Thanks for sending this. Believe it or not this has still been on my to-do list
> and so we definitely need a solution for Tegra.
>
> Looking at the above it appears that additional power-domains exposed as devices
> to the client device. So I assume that this means that the drivers for devices
> with multiple power-domains will need to call RPM APIs for each of these
> additional power-domains. Is that correct?
They can, but should not!
Instead, the driver shall use device_link_add() and device_link_del(),
dynamically, depending on what PM domain that their original device
needs for the current running use case.
In that way, they keep existing runtime PM deployment, operating on
its original device.
>
> If so, I can see that that would work, but it does not seem to fit the RPM model
> where currently for something like device clocks, the RPM callbacks can handle
> all clocks at once.
>
> I was wondering why we could not add a list of genpd domains to the
> dev_pm_domain struct for the device? For example ...
See above answer, hopefully that explains it.
FYI, that's why I also discovered the bug around using device links
with runtime PM during probe.
https://patchwork.kernel.org/patch/10408825/
>
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index e723b78d8357..a11d6db3c077 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -659,6 +659,7 @@ extern void dev_pm_put_subsys_data(struct device *dev);
> * subsystem-level and driver-level callbacks.
> */
> struct dev_pm_domain {
> + struct list_head genpd_list;
> struct dev_pm_ops ops;
> void (*detach)(struct device *dev, bool power_off);
> int (*activate)(struct device *dev);
> @@ -666,6 +667,11 @@ struct dev_pm_domain {
> void (*dismiss)(struct device *dev);
> };
>
> +struct dev_pm_domain_link {
> + struct generic_pm_domain *genpd;
> + struct list_head node;
> +};
> +
> /*
> * The PM_EVENT_ messages are also used by drivers implementing the legacy
> * suspend framework, based on the ->suspend() and ->resume() callbacks common
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index e61b5cd79fe2..019593804670 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -51,7 +51,6 @@ struct dev_pm_opp;
>
> struct generic_pm_domain {
> struct device dev;
> - struct dev_pm_domain domain; /* PM domain operations */
> struct list_head gpd_list_node; /* Node in the global PM domains list */
> struct list_head master_links; /* Links with PM domain as a master */
> struct list_head slave_links; /* Links with PM domain as a slave */
> @@ -99,11 +98,6 @@ struct generic_pm_domain {
>
> };
>
> -static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
> -{
> - return container_of(pd, struct generic_pm_domain, domain);
> -}
> -
>
> Obviously the above will not compile but the intent would be to allocate a
> dev_pm_domain_link struct per power-domain that the device needs and add
> to the genpd_list for the device. It would be a much bigger change in
> having to iterate through all the power-domains when turning devices on
> and off, however, it would simplify the client driver.
>
> Cheers
> Jon
>
> --
> nvpublic
Kind regards
Uffe
^ permalink raw reply
* [PATCH 9/9] ARM: dts: silk: Drop MTD partitioning from DT
From: Geert Uytterhoeven @ 2018-05-22 14:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522120257.13232-9-marek.vasut+renesas@gmail.com>
On Tue, May 22, 2018 at 2:02 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Drop the MTD partitioning from DT, since it does not describe HW
> and to give way to a more flexible kernel command line partition
> passing.
>
> To retain the original partitioning, assure you have enabled
> CONFIG_MTD_CMDLINE_PARTS in your kernel config and add the
> following to your kernel command line:
>
> mtdparts=spi0.0:256k at 0(loader),4096k(user),-(flash)
Drop @0?
4m?
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH 8/9] ARM: dts: alt: Drop MTD partitioning from DT
From: Geert Uytterhoeven @ 2018-05-22 14:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522120257.13232-8-marek.vasut+renesas@gmail.com>
On Tue, May 22, 2018 at 2:02 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Drop the MTD partitioning from DT, since it does not describe HW
> and to give way to a more flexible kernel command line partition
> passing.
>
> To retain the original partitioning, assure you have enabled
> CONFIG_MTD_CMDLINE_PARTS in your kernel config and add the
> following to your kernel command line:
>
> mtdparts=spi0.0:256k at 0(loader),256k(system),-(user)
Drop @0?
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH 7/9] ARM: dts: gose: Drop MTD partitioning from DT
From: Geert Uytterhoeven @ 2018-05-22 14:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522120257.13232-7-marek.vasut+renesas@gmail.com>
On Tue, May 22, 2018 at 2:02 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Drop the MTD partitioning from DT, since it does not describe HW
> and to give way to a more flexible kernel command line partition
> passing.
>
> To retain the original partitioning, assure you have enabled
> CONFIG_MTD_CMDLINE_PARTS in your kernel config and add the
> following to your kernel command line:
>
> mtdparts=spi0.0:256k at 0(loader),4096k(user),-(flash)
Drop @0? 4m?
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH 6/9] ARM: dts: wheat: Drop MTD partitioning from DT
From: Geert Uytterhoeven @ 2018-05-22 14:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522120257.13232-6-marek.vasut+renesas@gmail.com>
On Tue, May 22, 2018 at 2:02 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Drop the MTD partitioning from DT, since it does not describe HW
> and to give way to a more flexible kernel command line partition
> passing.
>
> To retain the original partitioning, assure you have enabled
> CONFIG_MTD_CMDLINE_PARTS in your kernel config and add the
> following to your kernel command line:
>
> mtdparts=spi0.0:256k at 0(loader),4096k(user),-(flash)
I think the "@0" can be dropped, as it's optional?
4m?
(Gaining more knowledge during reviewing ;-)
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH 5/9] ARM: dts: porter: Drop MTD partitioning from DT
From: Geert Uytterhoeven @ 2018-05-22 14:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522120257.13232-5-marek.vasut+renesas@gmail.com>
On Tue, May 22, 2018 at 2:02 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Drop the MTD partitioning from DT, since it does not describe HW
> and to give way to a more flexible kernel command line partition
> passing.
>
> To retain the original partitioning, assure you have enabled
> CONFIG_MTD_CMDLINE_PARTS in your kernel config and add the
> following to your kernel command line:
>
> mtdparts=spi0.0:256k at 0(loader_prg),4096k(user_prg),-(flash_fs)
4m?
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH 3/9] ARM: dts: stout: Drop MTD partitioning from DT
From: Geert Uytterhoeven @ 2018-05-22 14:33 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522120257.13232-3-marek.vasut+renesas@gmail.com>
On Tue, May 22, 2018 at 2:02 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Drop the MTD partitioning from DT, since it does not describe HW
> and to give way to a more flexible kernel command line partition
> passing.
>
> To retain the original partitioning, assure you have enabled
> CONFIG_MTD_CMDLINE_PARTS in your kernel config and add the
> following to your kernel command line:
>
> mtdparts=spi0.0:512k at 0(loader),256k(uboot),256k(uboot-env),-(flash)
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH 2/9] ARM: dts: lager: Drop MTD partitioning from DT
From: Geert Uytterhoeven @ 2018-05-22 14:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522120257.13232-2-marek.vasut+renesas@gmail.com>
On Tue, May 22, 2018 at 2:02 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Drop the MTD partitioning from DT, since it does not describe HW
> and to give way to a more flexible kernel command line partition
> passing.
>
> To retain the original partitioning, assure you have enabled
> CONFIG_MTD_CMDLINE_PARTS in your kernel config and add the
> following to your kernel command line:
>
> mtdparts=spi0.0:256k at 0(loader),4096k(user),-(flash)
I guess s/4096k/4m/ works, too?
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd
From: Jon Hunter @ 2018-05-22 14:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1526639490-12167-9-git-send-email-ulf.hansson@linaro.org>
Hi Ulf,
On 18/05/18 11:31, Ulf Hansson wrote:
> To support devices being partitioned across multiple PM domains, let's
> start by extending genpd to cope with these configurations.
>
> More precisely, add a new exported function, genpd_dev_pm_attach_by_id(),
> similar to genpd_dev_pm_attach(), but the new function also allows the
> caller to provide an index to what PM domain it wants to attach.
>
> Furthermore, let genpd register a new virtual struct device via calling
> device_register() and attach it to the corresponding PM domain, which is
> looked up via calling the existing genpd OF functions. Note that the new
> device is needed, because only one PM domain can be attached per device. At
> successful attachment, genpd_dev_pm_attach_by_id() returns the new device,
> allowing the caller to operate on it to deal with power management.
>
> To deal with detaching of a PM domain for multiple PM domain case, we can
> still re-use the existing genpd_dev_pm_detach() function, although we need
> to extend it to cover cleanup of the earlier registered device, via calling
> device_unregister().
>
> An important note, genpd_dev_pm_attach_by_id() shall only be called by the
> driver core / PM core, similar to how genpd_dev_pm_attach() is used.
> Following changes deploys this.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/base/power/domain.c | 79 +++++++++++++++++++++++++++++++++++++++++++++
> include/linux/pm_domain.h | 8 +++++
> 2 files changed, 87 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index d538640..ffeb6ea 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2171,6 +2171,15 @@ struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
> }
> EXPORT_SYMBOL_GPL(of_genpd_remove_last);
>
> +static void genpd_release_dev(struct device *dev)
> +{
> + kfree(dev);
> +}
> +
> +static struct bus_type genpd_bus_type = {
> + .name = "genpd",
> +};
> +
> /**
> * genpd_dev_pm_detach - Detach a device from its PM domain.
> * @dev: Device to detach.
> @@ -2208,6 +2217,10 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
>
> /* Check if PM domain can be powered off after removing this device. */
> genpd_queue_power_off_work(pd);
> +
> + /* Unregister the device if it was created by genpd. */
> + if (dev->bus == &genpd_bus_type)
> + device_unregister(dev);
> }
>
> static void genpd_dev_pm_sync(struct device *dev)
> @@ -2298,6 +2311,66 @@ int genpd_dev_pm_attach(struct device *dev)
> }
> EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
>
> +/**
> + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
> + * @dev: Device to attach.
> + * @index: The index of the PM domain.
> + *
> + * Parse device's OF node to find a PM domain specifier at the provided @index.
> + * If such is found, allocates a new device and attaches it to retrieved
> + * pm_domain ops.
> + *
> + * Returns the allocated device if successfully attached PM domain, NULL when
> + * the device don't need a PM domain or have a single PM domain, else PTR_ERR()
> + * in case of failures. Note that if a power-domain exists for the device, but
> + * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to ensure
> + * that the device is not probed and to re-try again later.
> + */
> +struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> + unsigned int index)
> +{
> + struct device *genpd_dev;
> + int num_domains;
> + int ret;
> +
> + if (!dev->of_node)
> + return NULL;
> +
> + /* Deal only with devices using multiple PM domains. */
> + num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
> + "#power-domain-cells");
> + if (num_domains < 2 || index >= num_domains)
> + return NULL;
> +
> + /* Allocate and register device on the genpd bus. */
> + genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
> + if (!genpd_dev)
> + return ERR_PTR(-ENOMEM);
> +
> + dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
> + genpd_dev->bus = &genpd_bus_type;
> + genpd_dev->release = genpd_release_dev;
> +
> + ret = device_register(genpd_dev);
> + if (ret) {
> + kfree(genpd_dev);
> + return ERR_PTR(ret);
> + }
> +
> + /* Try to attach the device to the PM domain at the specified index. */
> + ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
> + if (ret < 1) {
> + device_unregister(genpd_dev);
> + return ret ? ERR_PTR(ret) : NULL;
> + }
> +
> + pm_runtime_set_active(genpd_dev);
> + pm_runtime_enable(genpd_dev);
> +
> + return genpd_dev;
> +}
> +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);
Thanks for sending this. Believe it or not this has still been on my to-do list
and so we definitely need a solution for Tegra.
Looking at the above it appears that additional power-domains exposed as devices
to the client device. So I assume that this means that the drivers for devices
with multiple power-domains will need to call RPM APIs for each of these
additional power-domains. Is that correct?
If so, I can see that that would work, but it does not seem to fit the RPM model
where currently for something like device clocks, the RPM callbacks can handle
all clocks at once.
I was wondering why we could not add a list of genpd domains to the
dev_pm_domain struct for the device? For example ...
diff --git a/include/linux/pm.h b/include/linux/pm.h
index e723b78d8357..a11d6db3c077 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -659,6 +659,7 @@ extern void dev_pm_put_subsys_data(struct device *dev);
* subsystem-level and driver-level callbacks.
*/
struct dev_pm_domain {
+ struct list_head genpd_list;
struct dev_pm_ops ops;
void (*detach)(struct device *dev, bool power_off);
int (*activate)(struct device *dev);
@@ -666,6 +667,11 @@ struct dev_pm_domain {
void (*dismiss)(struct device *dev);
};
+struct dev_pm_domain_link {
+ struct generic_pm_domain *genpd;
+ struct list_head node;
+};
+
/*
* The PM_EVENT_ messages are also used by drivers implementing the legacy
* suspend framework, based on the ->suspend() and ->resume() callbacks common
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index e61b5cd79fe2..019593804670 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -51,7 +51,6 @@ struct dev_pm_opp;
struct generic_pm_domain {
struct device dev;
- struct dev_pm_domain domain; /* PM domain operations */
struct list_head gpd_list_node; /* Node in the global PM domains list */
struct list_head master_links; /* Links with PM domain as a master */
struct list_head slave_links; /* Links with PM domain as a slave */
@@ -99,11 +98,6 @@ struct generic_pm_domain {
};
-static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
-{
- return container_of(pd, struct generic_pm_domain, domain);
-}
-
Obviously the above will not compile but the intent would be to allocate a
dev_pm_domain_link struct per power-domain that the device needs and add
to the genpd_list for the device. It would be a much bigger change in
having to iterate through all the power-domains when turning devices on
and off, however, it would simplify the client driver.
Cheers
Jon
--
nvpublic
^ permalink raw reply related
* [PATCH v3] arm64: allwinner: a64: Add Amarula A64-Relic initial support
From: Maxime Ripard @ 2018-05-22 14:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522132228.6564-1-jagan@amarulasolutions.com>
On Tue, May 22, 2018 at 06:52:28PM +0530, Jagan Teki wrote:
> Amarula A64-Relic is Allwinner A64 based IoT device, which support
> - Allwinner A64 Cortex-A53
> - Mali-400MP2 GPU
> - AXP803 PMIC
> - 1GB DDR3 RAM
> - 8GB eMMC
> - AP6330 Wifi/BLE
> - MIPI-DSI
> - CSI: OV5640 sensor
> - USB OTG
You claim that this is doing OTG...
[..]
> +&usb_otg {
> + dr_mode = "peripheral";
> + status = "okay";
> +};
... and yet you're setting it as peripheral...
> +&usbphy {
> + usb0_id_det-gpios = <&pio 7 9 GPIO_ACTIVE_HIGH>; /* PH9 */
> + usb0_vbus-supply = <®_drivevbus>;
While you have an ID pin and a controllable VBUS. Which one is it?
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180522/f71e03b3/attachment-0001.sig>
^ permalink raw reply
* [RFC PATCH v2] arm64: fault: Don't leak data in ESR context for user fault on kernel VA
From: Peter Maydell @ 2018-05-22 14:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522141146.GF13470@e103592.cambridge.arm.com>
On 22 May 2018 at 15:11, Dave Martin <Dave.Martin@arm.com> wrote:
> On Tue, May 22, 2018 at 02:48:29PM +0100, Peter Maydell wrote:
>> On 22 May 2018 at 14:38, Will Deacon <will.deacon@arm.com> wrote:
>> > Hi Peter,
>> >
>> > Sorry for the delay in getting to this! Comments inline.
>> >
>> > On Thu, Apr 19, 2018 at 04:48:33PM +0100, Peter Maydell wrote:
>>
>> >> + /*
>> >> + * These bits provide only information about the
>> >> + * faulting instruction, which userspace knows already.
>> >> + * We explicitly clear bits which are architecturally
>> >> + * RES0 in case they are given meanings in future.
>> >> + */
>> >> + if (esr & ESR_ELx_ISV)
>> >> + esr &= ESR_ELx_EC_MASK | ESR_ELx_IL |
>> >> + ESR_ELx_ISV | ESR_ELx_SAS |
>> >> + ESR_ELx_SSE | ESR_ELx_SRT_MASK |
>> >> + ESR_ELx_SF | ESR_ELx_AR | ESR_ELx_CM |
>> >> + ESR_ELx_WNR;
>> >
>> > Reading through the ARM ARM, it seems to say that ISV is always 0 for
>> > faults reported in ESR_EL1, which implies we can drop ISV, SAS, SSE, SRT,
>> > SF and AR from this list and actually drop the conditional altogether.
>>
>> Mmm, I guess so, if we're guaranteed to only be working with ESRs
>> taken to EL1 (or we want to present userspace with an ESR that
>
> There is no direct interface between EL0 and EL2 with the stage2
> translation enabled, so even if this data is available for a fault at
> EL2, we won't be signalling the fault via delivering a signal to EL0.
Right -- and even if we did somehow end up with that, we probably
wouldn't want to leak to userspace that their access was trapped
to EL2 rather than EL1, so we should present the illusion that
it was trapped at EL1 regardless.
thanks
-- PMM
^ permalink raw reply
* [PATCH 4/9] ARM: dts: koelsch: Drop MTD partitioning from DT
From: Geert Uytterhoeven @ 2018-05-22 14:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522120257.13232-4-marek.vasut+renesas@gmail.com>
On Tue, May 22, 2018 at 2:02 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Drop the MTD partitioning from DT, since it does not describe HW
> and to give way to a more flexible kernel command line partition
> passing.
>
> To retain the original partitioning, assure you have enabled
> CONFIG_MTD_CMDLINE_PARTS in your kernel config and add the
> following to your kernel command line:
>
> mtdparts=spi0.0:512k at 0(loader),5632k(user),-(flash)
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH] arm64: Kconfig: Enable LSE atomics by default
From: Marc Zyngier @ 2018-05-22 14:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1526926462-19214-1-git-send-email-will.deacon@arm.com>
On 21/05/18 19:14, Will Deacon wrote:
> Now that we're seeing CPUs shipping with LSE atomics, default them to
> 'on' in Kconfig. CPUs without the instructions will continue to use
> LDXR/STXR-based sequences, but they will be placed out-of-line by the
> compiler.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH 1/9] ARM: shmobile: defconfig: Enable MTD command line partition parsing
From: Geert Uytterhoeven @ 2018-05-22 14:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522120257.13232-1-marek.vasut+renesas@gmail.com>
On Tue, May 22, 2018 at 2:02 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> In preparation for removing MTD partitioning from the DTs and moving
> it over to kernel command line partition parsing, enable the support
> for kernel command line MTD partition parsing.
>
> The argument for not having MTD partitions in the DT is the same as
> for not having hard drive partitions in DT, neither describes the
> hardware itself, so it shouldn't be in the DT. Furthermore, kernel
> command line MTD partition passing allows greater flexibility in
> case someone decided to repartition the flash, which is well in the
> realm of possibility with these systems.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH v2] ARM64: dts: sun50i: h5: Add spi flash node for OrangePi PC2
From: Maxime Ripard @ 2018-05-22 14:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180521115426.92115-1-manu@freebsd.org>
On Mon, May 21, 2018 at 01:54:26PM +0200, Emmanuel Vadot wrote:
> The OrangePi PC2 have an mx25l1606e spi flash.
> Add a node for it.
>
> Signed-off-by: Emmanuel Vadot <manu@freebsd.org>
Fixed the prefix and applied, thanks!
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180522/3426bebf/attachment-0001.sig>
^ permalink raw reply
* [PATCH v2] ARM64: dts: sun50i: a64: Add spi flash node for sopine
From: Maxime Ripard @ 2018-05-22 14:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180521115413.92070-1-manu@freebsd.org>
On Mon, May 21, 2018 at 01:54:13PM +0200, Emmanuel Vadot wrote:
> The Sopine and Pine64-LTS have a winbond w25q128 spi flash on spi0.
> Add a node for it.
>
> Signed-off-by: Emmanuel Vadot <manu@freebsd.org>
Fixed the prefix and applied, thanks!
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180522/bf0e2273/attachment.sig>
^ permalink raw reply
* [RFC PATCH v2] arm64: fault: Don't leak data in ESR context for user fault on kernel VA
From: Dave Martin @ 2018-05-22 14:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAFEAcA9i4_neKwaWPm_gV=fAgASL4B9X8Z++c+NvGA+JYSdDZA@mail.gmail.com>
On Tue, May 22, 2018 at 02:48:29PM +0100, Peter Maydell wrote:
> On 22 May 2018 at 14:38, Will Deacon <will.deacon@arm.com> wrote:
> > Hi Peter,
> >
> > Sorry for the delay in getting to this! Comments inline.
> >
> > On Thu, Apr 19, 2018 at 04:48:33PM +0100, Peter Maydell wrote:
>
> >> + /*
> >> + * These bits provide only information about the
> >> + * faulting instruction, which userspace knows already.
> >> + * We explicitly clear bits which are architecturally
> >> + * RES0 in case they are given meanings in future.
> >> + */
> >> + if (esr & ESR_ELx_ISV)
> >> + esr &= ESR_ELx_EC_MASK | ESR_ELx_IL |
> >> + ESR_ELx_ISV | ESR_ELx_SAS |
> >> + ESR_ELx_SSE | ESR_ELx_SRT_MASK |
> >> + ESR_ELx_SF | ESR_ELx_AR | ESR_ELx_CM |
> >> + ESR_ELx_WNR;
> >
> > Reading through the ARM ARM, it seems to say that ISV is always 0 for
> > faults reported in ESR_EL1, which implies we can drop ISV, SAS, SSE, SRT,
> > SF and AR from this list and actually drop the conditional altogether.
>
> Mmm, I guess so, if we're guaranteed to only be working with ESRs
> taken to EL1 (or we want to present userspace with an ESR that
There is no direct interface between EL0 and EL2 with the stage2
translation enabled, so even if this data is available for a fault at
EL2, we won't be signalling the fault via delivering a signal to EL0.
> looks like that regardless of what EL we took it to). I'll respin
> without the conditional.
Sounds fair. It might have been me that suggested this list of fields
in the first place: I'd not completely understood the ISV behaviour
previously, it seems.
Cheers
---Dave
^ permalink raw reply
* [PATCH RFC V2 2/6] hwmon: Add support for RPi voltage sensor
From: Guenter Roeck @ 2018-05-22 14:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <659923372.11518.1526997096662@email.1und1.de>
On 05/22/2018 06:51 AM, Stefan Wahren wrote:
> Hi Guenter,
>
>> Guenter Roeck <linux@roeck-us.net> hat am 22. Mai 2018 um 15:41 geschrieben:
>>
>>
>> On 05/22/2018 04:21 AM, Stefan Wahren wrote:
>>> Currently there is no easy way to detect undervoltage conditions on a
>>> remote Raspberry Pi. This hwmon driver retrieves the state of the
>>> undervoltage sensor via mailbox interface. The handling based on
>>> Noralf's modifications to the downstream firmware driver. In case of
>>> an undervoltage condition only an entry is written to the kernel log.
>>>
>>> CC: "Noralf Tr?nnes" <noralf@tronnes.org>
>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>> ---
>>> Documentation/hwmon/raspberrypi-hwmon | 22 +++++
>>> drivers/hwmon/Kconfig | 10 ++
>>> drivers/hwmon/Makefile | 1 +
>>> drivers/hwmon/raspberrypi-hwmon.c | 168 ++++++++++++++++++++++++++++++++++
>>> 4 files changed, 201 insertions(+)
>>> create mode 100644 Documentation/hwmon/raspberrypi-hwmon
>>> create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
>>>
>>> diff --git a/Documentation/hwmon/raspberrypi-hwmon b/Documentation/hwmon/raspberrypi-hwmon
>>> new file mode 100644
>>> index 0000000..3c92e2c
>>> --- /dev/null
>>> +++ b/Documentation/hwmon/raspberrypi-hwmon
>>> @@ -0,0 +1,22 @@
>>> +Kernel driver raspberrypi-hwmon
>>> +===============================
>>> +
>>> +Supported boards:
>>> + * Raspberry Pi A+ (via GPIO on SoC)
>>> + * Raspberry Pi B+ (via GPIO on SoC)
>>> + * Raspberry Pi 2 B (via GPIO on SoC)
>>> + * Raspberry Pi 3 B (via GPIO on port expander)
>>> + * Raspberry Pi 3 B+ (via PMIC)
>>> +
>>> +Author: Stefan Wahren <stefan.wahren@i2se.com>
>>> +
>>> +Description
>>> +-----------
>>> +
>>> +This driver periodically polls a mailbox property of the VC4 firmware to detect
>>> +undervoltage conditions.
>>> +
>>> +Sysfs entries
>>> +-------------
>>> +
>>> +in0_lcrit_alarm Undervoltage alarm
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index 768aed5..9a5bdb0 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
>>> This driver can also be built as a module. If so, the module
>>> will be called pwm-fan.
>>>
>>> +config SENSORS_RASPBERRYPI_HWMON
>>> + tristate "Raspberry Pi voltage monitor"
>>> + depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
>>> + help
>>> + If you say yes here you get support for voltage sensor on the
>>> + Raspberry Pi.
>>> +
>>> + This driver can also be built as a module. If so, the module
>>> + will be called raspberrypi-hwmon.
>>> +
>>> config SENSORS_SHT15
>>> tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
>>> depends on GPIOLIB || COMPILE_TEST
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index e7d52a3..a929770 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
>>> obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
>>> obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
>>> obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
>>> +obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
>>> obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
>>> obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
>>> obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
>>> diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
>>> new file mode 100644
>>> index 0000000..6233e84
>>> --- /dev/null
>>> +++ b/drivers/hwmon/raspberrypi-hwmon.c
>>> @@ -0,0 +1,168 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Raspberry Pi voltage sensor driver
>>> + *
>>> + * Based on firmware/raspberrypi.c by Noralf Tr?nnes
>>> + *
>>> + * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
>>> + */
>>> +#include <linux/device.h>
>>> +#include <linux/err.h>
>>> +#include <linux/hwmon.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/workqueue.h>
>>> +#include <soc/bcm2835/raspberrypi-firmware.h>
>>> +
>>> +#define UNDERVOLTAGE_STICKY_BIT BIT(16)
>>> +
>>> +struct rpi_hwmon_data {
>>> + struct device *hwmon_dev;
>>> + struct rpi_firmware *fw;
>>> + u32 last_throttled;
>>> + struct delayed_work get_values_poll_work;
>>> +};
>>> +
>>> +static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
>>> +{
>>> + u32 new_uv, old_uv, value;
>>> + int ret;
>>> +
>>> + /* Request firmware to clear sticky bits */
>>> + value = 0xffff;
>>> +
>>> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
>>> + &value, sizeof(value));
>>> + if (ret) {
>>> + dev_err_once(data->hwmon_dev, "Failed to get throttled (%d)\n",
>>> + ret);
>>> + return;
>>> + }
>>> +
>>> + new_uv = value & UNDERVOLTAGE_STICKY_BIT;
>>> + old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
>>> + data->last_throttled = value;
>>> +
>>> + if (new_uv == old_uv)
>>> + return;
>>> +
>>> + if (new_uv)
>>> + dev_crit(data->hwmon_dev, "Undervoltage detected!\n");
>>> + else
>>> + dev_info(data->hwmon_dev, "Voltage normalised\n");
>>> +
>>> + sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
>>> +}
>>> +
>>> +static void get_values_poll(struct work_struct *work)
>>> +{
>>> + struct rpi_hwmon_data *data;
>>> +
>>> + data = container_of(work, struct rpi_hwmon_data,
>>> + get_values_poll_work.work);
>>> +
>>> + rpi_firmware_get_throttled(data);
>>> +
>>> + /*
>>> + * We can't run faster than the sticky shift (100ms) since we get
>>> + * flipping in the sticky bits that are cleared.
>>> + */
>>> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
>>> +}
>>> +
>>> +static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
>>> + u32 attr, int channel, long *val)
>>> +{
>>> + struct rpi_hwmon_data *data = dev_get_drvdata(dev);
>>> +
>>> + *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
>>> + return 0;
>>> +}
>>> +
>>> +static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
>>> + u32 attr, int channel)
>>> +{
>>> + return 0444;
>>> +}
>>> +
>>> +static const u32 rpi_in_config[] = {
>>> + HWMON_I_LCRIT_ALARM,
>>> + 0
>>> +};
>>> +
>>> +static const struct hwmon_channel_info rpi_in = {
>>> + .type = hwmon_in,
>>> + .config = rpi_in_config,
>>> +};
>>> +
>>> +static const struct hwmon_channel_info *rpi_info[] = {
>>> + &rpi_in,
>>> + NULL
>>> +};
>>> +
>>> +static const struct hwmon_ops rpi_hwmon_ops = {
>>> + .is_visible = rpi_is_visible,
>>> + .read = rpi_read,
>>> +};
>>> +
>>> +static const struct hwmon_chip_info rpi_chip_info = {
>>> + .ops = &rpi_hwmon_ops,
>>> + .info = rpi_info,
>>> +};
>>> +
>>> +static int rpi_hwmon_probe(struct platform_device *pdev)
>>> +{
>>> + struct device *dev = &pdev->dev;
>>> + struct rpi_hwmon_data *data;
>>> + int ret;
>>> +
>>> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>>> + if (!data)
>>> + return -ENOMEM;
>>> +
>>> + data->fw = platform_get_drvdata(to_platform_device(dev->parent));
>>> + if (!data->fw)
>>> + return -EPROBE_DEFER;
>>> +
>>
>> I am a bit at loss here (and sorry I didn't bring this up before).
>> How would this ever be possible, given that the driver is registered
>> from the firmware driver ?
>
> Do you refer to the (wrong) return code, the assumption that the parent must be a platform driver or a possible race?
>
The return code is one thing. My question was how the driver would ever be instantiated
with platform_get_drvdata(to_platform_device(dev->parent)) == NULL (but dev->parent != NULL),
so I referred to the race. But, sure, a second question would be how that would indicate
that the parent is not instantiated yet (which by itself seems like an odd question).
Yet another question, as you point out, is why to use platform_get_drvdata(to_platform_device(dev->parent))
instead of dev_get_drvdata(dev->parent).
Guenter
>>
>>> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
>>> + &data->last_throttled,
>>> + sizeof(data->last_throttled));
>>> + if (ret)
>>> + return -ENODEV;
>>> +
>>> + data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
>>> + data,
>>> + &rpi_chip_info,
>>> + NULL);
>>> +
>>> + INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
>>> + platform_set_drvdata(pdev, data);
>>> +
>>> + if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
>>> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
>>> +
>>> + return PTR_ERR_OR_ZERO(data->hwmon_dev);
>>> +}
>>> +
>>> +static int rpi_hwmon_remove(struct platform_device *pdev)
>>> +{
>>> + struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
>>> +
>>> + cancel_delayed_work_sync(&data->get_values_poll_work);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static struct platform_driver rpi_hwmon_driver = {
>>> + .probe = rpi_hwmon_probe,
>>> + .remove = rpi_hwmon_remove,
>>> + .driver = {
>>> + .name = "raspberrypi-hwmon",
>>> + },
>>> +};
>>> +module_platform_driver(rpi_hwmon_driver);
>>> +
>>> +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
>>> +MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>
^ permalink raw reply
* [reset-control] How to initialize hardware state with the shared reset line?
From: Philipp Zabel @ 2018-05-22 14:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAFBinCBTTmPqD0UP8zzMBjsjzP9fJx9_pmhGz0_Vy-V+wmhLXQ@mail.gmail.com>
Hi Martin,
On Mon, 2018-05-21 at 12:40 +0200, Martin Blumenstingl wrote:
> Hello,
>
> On Mon, May 21, 2018 at 3:27 AM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> > Hi.
> >
> >
> > 2018-05-20 19:57 GMT+09:00 Martin Blumenstingl
> > <martin.blumenstingl@googlemail.com>:
> > > Hi,
> > >
> > > On Thu, May 10, 2018 at 11:16 AM, Masahiro Yamada
> > > <yamada.masahiro@socionext.com> wrote:
> > > [snip]
> > > > I may be missing something, but
> > > > one solution might be reset hogging on the
> > > > reset provider side. This allows us to describe
> > > > the initial state of reset lines in the reset controller.
> > > >
> > > > The idea for "reset-hog" is similar to:
> > > > - "gpio-hog" defined in
> > > > Documentation/devicetree/bindings/gpio/gpio.txt
> > > > - "assigned-clocks" defined in
> > > > Documetation/devicetree/bindings/clock/clock-bindings.txt
> > > >
> > > >
> > > >
> > > > For example,
> > > >
> > > > reset-controller {
> > > > ....
> > > >
> > > > line_a {
> > > > reset-hog;
> > > > resets = <1>;
> > > > reset-assert;
> > > > };
> > > > }
> > > >
> > > >
> > > > When the reset controller is registered,
> > > > the reset ID '1' is asserted.
> > > >
> > > >
> > > > So, all reset consumers that share the reset line '1'
> > > > will start from the asserted state
> > > > (i.e. defined state machine state).
> > >
> > > I wonder if a "reset hog" can be board specific:
> > > - GPIO hogs are definitely board specific (meson-gxbb-odroidc2.dts for
> > > example uses it to take the USB hub out of reset)
> > > - assigned-clock-parents (and the like) can also be board specific (I
> > > made up a use-case since I don't know of any actual examples: board A
> > > uses an external XTAL while board B uses some other internal
> > > clock-source because it doesn't have an external XTAL)
> > >
> > > however, can reset lines be board specific? or in other words: do we
> > > need to describe them in device-tree?
> >
> > Indeed.
> >
> > I did not come up with board-specific cases.
> >
> > The problem we are discussing is SoC-specific,
> > and reset-controller drivers are definitely SoC-specific.
> >
> > So, I think the initial state can be coded in drivers instead of DT.
>
> OK, let's also hear Philipp's (reset framework maintainer) opinion on this
I'd like to know if there are other SoC families besides Amlogic Meson
that potentially could have this problem and about how many of the
resets that are documented in include/dt-bindings/reset/amlogic,meson*
we are actually talking. Are all of those initially deasserted and none
of the connected peripherals have power-on reset mechanisms?
> > > we could extend struct reset_controller_dev (= reset controller
> > > driver) if they are not board specific:
> > > - either assert all reset lines by default except if they are listed
> > > in a new field (may break backwards compatibility, requires testing of
> > > all reset controller drivers)
> >
> > This is quite simple, but I am afraid there are some cases where the forcible
> > reset-assert is not preferred.
> >
> > For example, the earlycon. When we use earlycon, we would expect it has been
> > initialized by a boot-loader, or something.
> > If it is reset-asserted on the while, the console output
> > will not be good.
>
> indeed, so let's skip this idea
Maybe we should at first add initial reset assertion to the Meson driver
on a case by case bases?
We can't add required reset hog DT bindings to the Meson reset
controller anyway without breaking DT backwards compatibility.
> > > - specify a list of reset lines and their desired state (or to keep it
> > > easy: specify a list of reset lines that should be asserted)
> > > (I must admit that this is basically your idea but the definition is
> > > moved from device-tree to the reset controller driver)
> >
> > Yes, I think the list of "reset line ID" and "init state" pairs
> > would be nicer.
>
> $ grep -R "of_reset_n_cells = [^1]" drivers/reset/
> drivers/reset/reset-berlin.c: priv->rcdev.of_reset_n_cells = 2;
> drivers/reset/hisilicon/reset-hi3660.c: rc->rst.of_reset_n_cells = 2;
> drivers/reset/reset-ti-sci.c: data->rcdev.of_reset_n_cells = 2;
> drivers/reset/reset-lantiq.c: priv->rcdev.of_reset_n_cells = 2;
>
> everything else uses only one reset cell
> from the lantiq reset dt-binding documentation: "The first cell takes
> the reset set bit and the second cell takes the status bit."
>
> I'm not sure what to do with drivers that specify != 1 reset-cell
> though if we use a simple "init state pair"
> maybe Philipp can share his opinion on this one as well
See above, so far I am not convinced (either way) whether this should be
described in the DT at all.
> > > any "chip" specific differences could be expressed by using a
> > > different of_device_id
> > >
> > > one the other hand: your "reset hog" solution looks fine to me if
> > > reset lines can be board specific
> > >
> > > > From the discussion with Martin Blumenstingl
> > > > (https://lkml.org/lkml/2018/4/28/115),
> > > > the problem for Amlogic is that
> > > > the reset line is "de-asserted" by default.
> > > > If so, the 'reset-hog' would fix the problem,
> > > > and DWC3 driver would be able to use
> > > > shared, level reset, I think.
> > >
> > > I think you are right: if we could control the initial state then we
> > > should be able to use level resets
> >
> >
> > Even further, can we drop the shared reset_control_reset() support, maybe?
> > (in other words, revert commit 7da33a37b48f11)
>
> I believe we need to keep this if there's hardware out there:
> - where the reset controller only supports reset pulses
> - at least one reset line is shared between multiple devices
>
> I didn't have a closer look at the Amlogic Meson6 SoC yet, but I think
> it matches above criteria. as far as I know:
> - the USB situation there is similar to Meson8b (USB controllers and
> PHYs share a reset line)
> - it uses an older reset controller IP block which does not support
> level resets (only reset pulses)
See my answer to Masahiro's first mail. I think somebody suggested in
the past to add a fallback from the deassert to the reset op. I think
this is something that should work in this case.
regards
Philipp
^ permalink raw reply
* [reset-control] How to initialize hardware state with the shared reset line?
From: Philipp Zabel @ 2018-05-22 13:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAK7LNATZgJ4MxOFLUCNARWv3Zz=gpL-jGReDnBnArquiaXRWoQ@mail.gmail.com>
Hi Masahiro,
On Thu, 2018-05-10 at 18:16 +0900, Masahiro Yamada wrote:
> Hi.
>
>
> The previous thread was:
> "usb: dwc3: support clocks and resets for DWC3 core"
> https://patchwork.kernel.org/patch/10349623/
>
>
> I changed the subject because
> I think this is rather reset-control topic than USB.
Thank you for taking the time to write this up in such detail.
Indeed we were not expecting hardware that both defaults to deasserted
resets and does not have a power-on reset mechanism for the IP cores, so
yes, the framework currently assumes that all shared reset lines are
initially (or at some point in the past since power-on were) asserted.
I wonder if we are going to see this more often in the future, or
whether the Amlogic Meson SoCs will stay the exception.
> I am searching for a good way to initialize hardware devices
> in the following situation:
>
> - two or more hardware devices share the same reset line
So in general, at least during runtime, the driver must not expect reset
assertion to actually work, so the sharing device is not disturbed.
> - those devices are not reset before booting the kernel
> (i.e. flip flops in RTL are random state at driver probe)
>
> - the hardware IP is used by various SoCs,
> and this issue only appears only on some.
Unfortunately for configurable IP cores like the DW ones I am not sure
to what extent these can actually be considered identical when
implemented in different SoCs.
I would expect that the hardware IP's basic requirements, like whether
the reset must be asserted at some defined point during operation, or
whether it has to be a pulse of da specific duration, does not change
between implementations.
> Specifically, the DWC3 USB IP case is in my mind,
> but this should apply to any hardware in general.
>
>
>
>
> Issue in detail
> ---------------
>
> The DWC3 USB IP is very popular and used by various SoCs
> as you see in drivers/usb/dwc3/.
>
> SoCs are using the same RTL as provided by Synopsys.
> (SoC vendors could tweak the RTL if they like,
> but it would rarely happen because it would just be troublesome)
>
> So, let's assume we use the same IP with the same RTL version.
> It is *compatible* in terms of device tree.
> In this case, we should avoid differentiating the driver code per-SoC.
>
> In fact, we ended up with
> commit ff0a632 ("usb: dwc3: of-simple: add support for shared and
> pulsed reset lines")
> commit e8284db ("usb: dwc3: of-simple: add support for the Amlogic
> Meson GXL and AXG SoCs")
I'm surprised, didn't all Meson SoCs gain level reset support for this
purpose?
I agree this should not be needed in the drivers, and I'd be very happy
if we could make this work on Meson with the dwc3 driver always
requesting shared resets.
> This means, the difference that comes from the reset _provider_
> must be implemented in the reset _consumer_.
> This is unfortunate.
I am not entirely happy about the shared vs. exclusive naming of the
API, but I have no better names. By choosing between the two, the
driver?states its requirements of the API, not whether the SoC it is
implemented in actually shares the reset line with other devices.
> So, I wonder if we can support it in sub-system level somehow
> instead of messing up the reset consumer driver.
I'd like to deflect and, hoping that this is an exception, ask whether
we could just do this in the driver?
> - The reset topology is SoC-dependent.
> A reset line is dedicated for single hardware for some SoCs,
> and shared by multiple hardware for others.
In which case the driver must be able to cope with shared reset lines,
so it should use the shared reset API.
> - The reset policy (pulse reset, level reset, or both of them)
> are SoC-dependent (reset controller dependent).
While that may be the case for some hardware IP, I don't think it is the
case for the dwc3 driver. It doesn't need to assert the reset at any
time during normal operation, and it works with level resets. The
pulsed/exclusive reset handling really is just a workaround for the
missing power-on reset.
> RTL generally contains state machines.
> The initial state of flip flops are undefined on power-on
> hence the initial state of state machines are also undefined.
>
> So, digital circuits needs explicit reset in general.
>
> In some cases, the reset is performed before booting the kernel.
>
> - power-on reset
> (FFs are put into defined state on power-on automatically)
>
> - a reset controller assert reset lines by default
> (FFs are fixed to defined state. If a reset consumer
> driver deasserts the reset line, HW starts from the define state)
>
> - Firmware may reset the hardware before booting the kernel
>
>
> If nothing above is done, and the reset line is shared by multiple devices,
> we do not have a good way to put the hardware into the sane state.
>
>
> Reset consumers choose the required reset type:
>
> - Shared reset:
> reset_control_assert / reset_control_deassert work like
> clock_disable / clock_enable.
> The consumer must be tolerant to the situation where the HW may
> not reset-asserted.
>
> - Exclusive reset:
> It is guaranteed reset_control_assert() really asserts the reset line,
> but only one reset consumer grabs the reset line at the same time.
Yes. The last part is a limitation of the current framework
implementation, but one that I'd like to keep as long as possible.
To support shared resets with guaranteed assertion, we'd need a
notification framework with a possibility for drivers to temporarily
suspend their own operations to allow a reset request from a different
device, or to veto reset requests if that is not possible. And drivers
that try to reset need to properly handle failed or deferred reset
requests. That would introduce a level of complexity that I'd very much
like to avoid.
> As above, the state machines in digital circuits must start from a
> defined state.
> So, we want exclusive reset to reset the FFs.
> However, this is too much requirement
> since it is a reset line is often shared.
For missing power-on resets, what we really want is not "exclusive
reset", and not even "guaranteed reset assertion". We want that after
the initial reset_deassert, the reset line is deasserted and the device
has been in reset *at any point in the past* since power-on.
> How to save such an Amlogic case?
I think this case can be solved by either just initializing the reset
lines to asserted in the reset controller driver (in which case the
controller driver needs to know about which lines have to be asserted,
and needs to support assert/deassert ops) or by adding a fallback from
.deassert to .reset for reset controllers that don't have level reset
hardware support. That is, if the first driver calls
reset_control_deassert, the reset controller driver actually issues a
reset pulse.
> Hogging solve the problem?
> --------------------------
>
>
> I was thinking of a solution.
>
> I may be missing something, but
> one solution might be reset hogging on the
> reset provider side. This allows us to describe
> the initial state of reset lines in the reset controller.
>
> The idea for "reset-hog" is similar to:
> - "gpio-hog" defined in
> Documentation/devicetree/bindings/gpio/gpio.txt
> - "assigned-clocks" defined in
> Documetation/devicetree/bindings/clock/clock-bindings.txt
>
>
>
> For example,
>
> reset-controller {
> ....
>
> line_a {
> reset-hog;
> resets = <1>;
> reset-assert;
> };
> }
>
>
> When the reset controller is registered,
> the reset ID '1' is asserted.
>
>
> So, all reset consumers that share the reset line '1'
> will start from the asserted state
> (i.e. defined state machine state).
>
>
> From the discussion with Martin Blumenstingl
> (https://lkml.org/lkml/2018/4/28/115),
> the problem for Amlogic is that
> the reset line is "de-asserted" by default.
> If so, the 'reset-hog' would fix the problem,
> and DWC3 driver would be able to use
> shared, level reset, I think.
>
>
> I think something may be missing from my mind,
> but I hope this will prompt the discussion,
> and we will find a better idea.
My question would is: do we need this complexity, or can this be solved
by just adding a workaround in the meson driver? If a lot of SoCs using
the simple reset driver had this requirement, I'd seriously consider
this, but it currently seems to me that this is just an issue with a
single SoC family.
regards
Philipp
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox