Re: [RFC PATCH v2 1/2] ata: ahci: Support state with min power and Partial low power state
Hi,
On 13-07-18 00:27, Srinivas Pandruvada wrote:
Currently when min_power policy is selected, the partial low power state
is not entered and link will try aggressively enter to only slumber state.
Add a new policy which still enable DEVSLP but also try to enter partial
low power state.
For information the difference between partial and slumber
Partial – PHY logic is powered up, and in a reduced power state. The link
PM exit latency to active state maximum is 10 ns.
Slumber – PHY logic is powered up, and in a reduced power state. The link
PM exit latency to active state maximum is 10 ms.
Devslp – PHY logic is powered down. The link PM exit latency from this
state to active state maximum is 20 ms, unless otherwise specified by
DETO.
Suggested-by: Hans de Goede
Signed-off-by: Srinivas Pandruvada
You got this the wrong way around, when ALP is not set you only get
partial, the ALP bit is active high, not active low as your commit
suggests. So the name of the new policy should be min_power_without_asp.
Regards,
Hans
---
drivers/ata/libahci.c | 6 +-
drivers/ata/libata-core.c | 1 +
drivers/ata/libata-scsi.c | 1 +
include/linux/libata.h| 3 ++-
4 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 511fb67f363d..8cf2cf49537d 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -799,8 +799,11 @@ static int ahci_set_lpm(struct ata_link *link, enum
ata_lpm_policy policy,
return 0;
} else {
cmd |= PORT_CMD_ALPE;
+
if (policy == ATA_LPM_MIN_POWER)
cmd |= PORT_CMD_ASP;
+ else if (policy == ATA_LPM_MIN_POWER_WITH_ASP)
+ cmd &= ~PORT_CMD_ASP;
/* write out new cmd value */
writel(cmd, port_mmio + PORT_CMD);
@@ -811,7 +814,8 @@ static int ahci_set_lpm(struct ata_link *link, enum
ata_lpm_policy policy,
if ((hpriv->cap2 & HOST_CAP2_SDS) &&
(hpriv->cap2 & HOST_CAP2_SADM) &&
(link->device->flags & ATA_DFLAG_DEVSLP)) {
- if (policy == ATA_LPM_MIN_POWER)
+ if (policy == ATA_LPM_MIN_POWER ||
+ policy == ATA_LPM_MIN_POWER_WITH_ASP)
ahci_set_aggressive_devslp(ap, true);
else
ahci_set_aggressive_devslp(ap, false);
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index cc71c63df381..245a59e6cb18 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3970,6 +3970,7 @@ int sata_link_scr_lpm(struct ata_link *link, enum
ata_lpm_policy policy,
scontrol |= (0x6 << 8);
break;
case ATA_LPM_MED_POWER_WITH_DIPM:
+ case ATA_LPM_MIN_POWER_WITH_ASP:
case ATA_LPM_MIN_POWER:
if (ata_link_nr_enabled(link) > 0)
/* no restrictions on LPM transitions */
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index aad1b01447de..2d683db50ceb 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -110,6 +110,7 @@ static const char *ata_lpm_policy_names[] = {
[ATA_LPM_MAX_POWER] = "max_performance",
[ATA_LPM_MED_POWER] = "medium_power",
[ATA_LPM_MED_POWER_WITH_DIPM] = "med_power_with_dipm",
+ [ATA_LPM_MIN_POWER_WITH_ASP]= "min_power_with_asp",
[ATA_LPM_MIN_POWER] = "min_power",
};
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 32f247cb5e9e..1e154f1f7e8f 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -523,7 +523,8 @@ enum ata_lpm_policy {
ATA_LPM_MAX_POWER,
ATA_LPM_MED_POWER,
ATA_LPM_MED_POWER_WITH_DIPM, /* Med power + DIPM as win IRST does */
- ATA_LPM_MIN_POWER,
+ ATA_LPM_MIN_POWER_WITH_ASP, /* Min Power + partial and slumber */
+ ATA_LPM_MIN_POWER, /* Min power + no partial (slumber only) */
};
enum ata_lpm_hints {
Re: [RFC PATCH v2 1/2] ata: ahci: Support state with min power and Partial low power state
Hi,
On 13-07-18 00:27, Srinivas Pandruvada wrote:
Currently when min_power policy is selected, the partial low power state
is not entered and link will try aggressively enter to only slumber state.
Add a new policy which still enable DEVSLP but also try to enter partial
low power state.
For information the difference between partial and slumber
Partial – PHY logic is powered up, and in a reduced power state. The link
PM exit latency to active state maximum is 10 ns.
Slumber – PHY logic is powered up, and in a reduced power state. The link
PM exit latency to active state maximum is 10 ms.
Devslp – PHY logic is powered down. The link PM exit latency from this
state to active state maximum is 20 ms, unless otherwise specified by
DETO.
Suggested-by: Hans de Goede
Signed-off-by: Srinivas Pandruvada
You got this the wrong way around, when ALP is not set you only get
partial, the ALP bit is active high, not active low as your commit
suggests. So the name of the new policy should be min_power_without_asp.
Regards,
Hans
---
drivers/ata/libahci.c | 6 +-
drivers/ata/libata-core.c | 1 +
drivers/ata/libata-scsi.c | 1 +
include/linux/libata.h| 3 ++-
4 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 511fb67f363d..8cf2cf49537d 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -799,8 +799,11 @@ static int ahci_set_lpm(struct ata_link *link, enum
ata_lpm_policy policy,
return 0;
} else {
cmd |= PORT_CMD_ALPE;
+
if (policy == ATA_LPM_MIN_POWER)
cmd |= PORT_CMD_ASP;
+ else if (policy == ATA_LPM_MIN_POWER_WITH_ASP)
+ cmd &= ~PORT_CMD_ASP;
/* write out new cmd value */
writel(cmd, port_mmio + PORT_CMD);
@@ -811,7 +814,8 @@ static int ahci_set_lpm(struct ata_link *link, enum
ata_lpm_policy policy,
if ((hpriv->cap2 & HOST_CAP2_SDS) &&
(hpriv->cap2 & HOST_CAP2_SADM) &&
(link->device->flags & ATA_DFLAG_DEVSLP)) {
- if (policy == ATA_LPM_MIN_POWER)
+ if (policy == ATA_LPM_MIN_POWER ||
+ policy == ATA_LPM_MIN_POWER_WITH_ASP)
ahci_set_aggressive_devslp(ap, true);
else
ahci_set_aggressive_devslp(ap, false);
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index cc71c63df381..245a59e6cb18 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3970,6 +3970,7 @@ int sata_link_scr_lpm(struct ata_link *link, enum
ata_lpm_policy policy,
scontrol |= (0x6 << 8);
break;
case ATA_LPM_MED_POWER_WITH_DIPM:
+ case ATA_LPM_MIN_POWER_WITH_ASP:
case ATA_LPM_MIN_POWER:
if (ata_link_nr_enabled(link) > 0)
/* no restrictions on LPM transitions */
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index aad1b01447de..2d683db50ceb 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -110,6 +110,7 @@ static const char *ata_lpm_policy_names[] = {
[ATA_LPM_MAX_POWER] = "max_performance",
[ATA_LPM_MED_POWER] = "medium_power",
[ATA_LPM_MED_POWER_WITH_DIPM] = "med_power_with_dipm",
+ [ATA_LPM_MIN_POWER_WITH_ASP]= "min_power_with_asp",
[ATA_LPM_MIN_POWER] = "min_power",
};
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 32f247cb5e9e..1e154f1f7e8f 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -523,7 +523,8 @@ enum ata_lpm_policy {
ATA_LPM_MAX_POWER,
ATA_LPM_MED_POWER,
ATA_LPM_MED_POWER_WITH_DIPM, /* Med power + DIPM as win IRST does */
- ATA_LPM_MIN_POWER,
+ ATA_LPM_MIN_POWER_WITH_ASP, /* Min Power + partial and slumber */
+ ATA_LPM_MIN_POWER, /* Min power + no partial (slumber only) */
};
enum ata_lpm_hints {
Re: [PATCH v2] tools/memory-model: Add extra ordering for locks and remove it for ordinary release/acquire
On Thu, Jul 12, 2018 at 07:05:39PM -0700, Daniel Lustig wrote: > On 7/12/2018 11:10 AM, Linus Torvalds wrote: > > On Thu, Jul 12, 2018 at 11:05 AM Peter Zijlstra > > wrote: > >> > >> The locking pattern is fairly simple and shows where RCpc comes apart > >> from expectation real nice. > > > > So who does RCpc right now for the unlock-lock sequence? Somebody > > mentioned powerpc. Anybody else? > > > > How nasty would be be to make powerpc conform? I will always advocate > > tighter locking and ordering rules over looser ones.. > > > > Linus > > RISC-V probably would have been RCpc if we weren't having this discussion. > Depending on how we map atomics/acquire/release/unlock/lock, we can end up > producing RCpc, "RCtso" (feel free to find a better name here...), or RCsc > behaviors, and we're trying to figure out which we actually need. > > I think the debate is this: > > Obviously programmers would prefer just to have RCsc and not have to figure > out > all the complexity of the other options. On x86 or architectures with native > RCsc operations (like ARMv8), that's generally easy enough to get. > > For weakly-ordered architectures that use fences for ordering (including > PowerPC and sometimes RISC-V, see below), though, it takes extra fences to go > from RCpc to either "RCtso" or RCsc. People using these architectures are > concerned about whether there's a negative performance impact from those extra > fences. > > However, some scheduler code, some RCU code, and probably some other examples > already implicitly or explicitly assume unlock()/lock() provides stronger > ordering than RCpc. So, we have to decide whether to: > 1) define unlock()/lock() to enforce "RCtso" or RCsc, insert more fences on > PowerPC and RISC-V accordingly, and probably negatively regress PowerPC > 2) leave unlock()/lock() as enforcing only RCpc, fix any code that currently > assumes something stronger than RCpc is being provided, and hope people don't > get it wrong in the future > 3) some mixture like having unlock()/lock() be "RCtso" but > smp_store_release()/ > smp_cond_load_acquire() be only RCpc > > Also, FWIW, if other weakly-ordered architectures come along in the future and > also use any kind of lightweight fence rather than native RCsc operations, > they'll likely be in the same boat as RISC-V and Power here, in the sense of > not providing RCsc by default either. > > Is that a fair assessment everyone? It's for me, thank you! And as we've seen, there are arguments for each of the above three choices. I'm afraid that (despite Linus's statement ;-)), my preference would currently go to (2). > > > > I can also not-so-briefly summarize RISC-V's status here, since I think > there's > been a bunch of confusion about where we're coming from: > > First of all, I promise we're not trying to start a fight about all this :) > We're trying to understand the LKMM requirements so we know what instructions > to use. > > With that, the easy case: RISC-V is RCsc if we use AMOs or load-reserved/ > store-conditional, all of which have RCsc .aq and .rl bits: > > (a) ... > amoswap.w.rl x0, x0, [lock] // unlock() > ... > loop: > amoswap.w.aq a0, t1, [lock] // lock() > bnez a0, loop// lock() > (b) ... > > (a) is ordered before (b) here, regardless of (a) and (b). Likewise for our > load-reserved/store-conditional instructions, which also have .aq and rl. > That's similiar to how ARM behaves, and is no problem. We're happy with that > too. > > Unfortunately, we don't (currently?) have plain load-acquire or store-release > opcodes in the ISA. (That's a different discussion...) For those, we need > fences instead. And that's where it gets messier. > > RISC-V *would* end up providing only RCpc if we use what I'd argue is the most > "natural" fence-based mapping for store-release operations, and then pair that > with LR/SC: > > (a) ... > fence rw,w // unlock() > sw x0, [lock] // unlock() > ... > loop: > lr.w.aq a0, [lock] // lock() > sc.w t1, [lock] // lock() > bnez loop // lock() > (b) ... > > However, if (a) and (b) are loads to different addresses, then (a) is not > ordered before (b) here. One unpaired RCsc operation is not a full fence. > Clearly "fence rw,w" is not sufficient if the scheduler, RCU, and elsewhere > depend on "RCtso" or RCsc. > > RISC-V can get back to "RCtso", matching PowerPC, by using a stronger fence: Or by using a "fence r,rw" in the lock() (without the .aq), as current code does ;-) though I'm not sure how the current solution would compare to the .tso mapping... Andrea > > (a) ... > fence.tso // unlock(), fence.tso == fence rw,w + fence r,r > sw x0, [lock] // unlock() > ... > loop: > lr.w.aq a0, [lock] // lock() > sc.w t1, [lock] // lock() > bnez loop // lock() > (b) ... > > (a) is ordered before (b), unless (a) is a store and (b) is a load to a >
Re: [PATCH v2] tools/memory-model: Add extra ordering for locks and remove it for ordinary release/acquire
On Thu, Jul 12, 2018 at 07:05:39PM -0700, Daniel Lustig wrote: > On 7/12/2018 11:10 AM, Linus Torvalds wrote: > > On Thu, Jul 12, 2018 at 11:05 AM Peter Zijlstra > > wrote: > >> > >> The locking pattern is fairly simple and shows where RCpc comes apart > >> from expectation real nice. > > > > So who does RCpc right now for the unlock-lock sequence? Somebody > > mentioned powerpc. Anybody else? > > > > How nasty would be be to make powerpc conform? I will always advocate > > tighter locking and ordering rules over looser ones.. > > > > Linus > > RISC-V probably would have been RCpc if we weren't having this discussion. > Depending on how we map atomics/acquire/release/unlock/lock, we can end up > producing RCpc, "RCtso" (feel free to find a better name here...), or RCsc > behaviors, and we're trying to figure out which we actually need. > > I think the debate is this: > > Obviously programmers would prefer just to have RCsc and not have to figure > out > all the complexity of the other options. On x86 or architectures with native > RCsc operations (like ARMv8), that's generally easy enough to get. > > For weakly-ordered architectures that use fences for ordering (including > PowerPC and sometimes RISC-V, see below), though, it takes extra fences to go > from RCpc to either "RCtso" or RCsc. People using these architectures are > concerned about whether there's a negative performance impact from those extra > fences. > > However, some scheduler code, some RCU code, and probably some other examples > already implicitly or explicitly assume unlock()/lock() provides stronger > ordering than RCpc. So, we have to decide whether to: > 1) define unlock()/lock() to enforce "RCtso" or RCsc, insert more fences on > PowerPC and RISC-V accordingly, and probably negatively regress PowerPC > 2) leave unlock()/lock() as enforcing only RCpc, fix any code that currently > assumes something stronger than RCpc is being provided, and hope people don't > get it wrong in the future > 3) some mixture like having unlock()/lock() be "RCtso" but > smp_store_release()/ > smp_cond_load_acquire() be only RCpc > > Also, FWIW, if other weakly-ordered architectures come along in the future and > also use any kind of lightweight fence rather than native RCsc operations, > they'll likely be in the same boat as RISC-V and Power here, in the sense of > not providing RCsc by default either. > > Is that a fair assessment everyone? It's for me, thank you! And as we've seen, there are arguments for each of the above three choices. I'm afraid that (despite Linus's statement ;-)), my preference would currently go to (2). > > > > I can also not-so-briefly summarize RISC-V's status here, since I think > there's > been a bunch of confusion about where we're coming from: > > First of all, I promise we're not trying to start a fight about all this :) > We're trying to understand the LKMM requirements so we know what instructions > to use. > > With that, the easy case: RISC-V is RCsc if we use AMOs or load-reserved/ > store-conditional, all of which have RCsc .aq and .rl bits: > > (a) ... > amoswap.w.rl x0, x0, [lock] // unlock() > ... > loop: > amoswap.w.aq a0, t1, [lock] // lock() > bnez a0, loop// lock() > (b) ... > > (a) is ordered before (b) here, regardless of (a) and (b). Likewise for our > load-reserved/store-conditional instructions, which also have .aq and rl. > That's similiar to how ARM behaves, and is no problem. We're happy with that > too. > > Unfortunately, we don't (currently?) have plain load-acquire or store-release > opcodes in the ISA. (That's a different discussion...) For those, we need > fences instead. And that's where it gets messier. > > RISC-V *would* end up providing only RCpc if we use what I'd argue is the most > "natural" fence-based mapping for store-release operations, and then pair that > with LR/SC: > > (a) ... > fence rw,w // unlock() > sw x0, [lock] // unlock() > ... > loop: > lr.w.aq a0, [lock] // lock() > sc.w t1, [lock] // lock() > bnez loop // lock() > (b) ... > > However, if (a) and (b) are loads to different addresses, then (a) is not > ordered before (b) here. One unpaired RCsc operation is not a full fence. > Clearly "fence rw,w" is not sufficient if the scheduler, RCU, and elsewhere > depend on "RCtso" or RCsc. > > RISC-V can get back to "RCtso", matching PowerPC, by using a stronger fence: Or by using a "fence r,rw" in the lock() (without the .aq), as current code does ;-) though I'm not sure how the current solution would compare to the .tso mapping... Andrea > > (a) ... > fence.tso // unlock(), fence.tso == fence rw,w + fence r,r > sw x0, [lock] // unlock() > ... > loop: > lr.w.aq a0, [lock] // lock() > sc.w t1, [lock] // lock() > bnez loop // lock() > (b) ... > > (a) is ordered before (b), unless (a) is a store and (b) is a load to a >
[PATCH] tty: serial: jsm: remove redundant pointer ch
From: Colin Ian King
Pointer ch is being assigned but is never used hence it is redundant
and can be removed.
Cleans up clang warning:
warning: variable 'ch' set but not used [-Wunused-but-set-variable]
Signed-off-by: Colin Ian King
---
drivers/tty/serial/jsm/jsm_tty.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/tty/serial/jsm/jsm_tty.c b/drivers/tty/serial/jsm/jsm_tty.c
index b6bd6e15e07b..689774c073ca 100644
--- a/drivers/tty/serial/jsm/jsm_tty.c
+++ b/drivers/tty/serial/jsm/jsm_tty.c
@@ -430,7 +430,6 @@ int jsm_uart_port_init(struct jsm_board *brd)
{
int i, rc;
unsigned int line;
- struct jsm_channel *ch;
if (!brd)
return -ENXIO;
@@ -444,7 +443,7 @@ int jsm_uart_port_init(struct jsm_board *brd)
brd->nasync = brd->maxports;
/* Set up channel variables */
- for (i = 0; i < brd->nasync; i++, ch = brd->channels[i]) {
+ for (i = 0; i < brd->nasync; i++) {
if (!brd->channels[i])
continue;
--
2.17.1
[PATCH] tty: serial: jsm: remove redundant pointer ch
From: Colin Ian King
Pointer ch is being assigned but is never used hence it is redundant
and can be removed.
Cleans up clang warning:
warning: variable 'ch' set but not used [-Wunused-but-set-variable]
Signed-off-by: Colin Ian King
---
drivers/tty/serial/jsm/jsm_tty.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/tty/serial/jsm/jsm_tty.c b/drivers/tty/serial/jsm/jsm_tty.c
index b6bd6e15e07b..689774c073ca 100644
--- a/drivers/tty/serial/jsm/jsm_tty.c
+++ b/drivers/tty/serial/jsm/jsm_tty.c
@@ -430,7 +430,6 @@ int jsm_uart_port_init(struct jsm_board *brd)
{
int i, rc;
unsigned int line;
- struct jsm_channel *ch;
if (!brd)
return -ENXIO;
@@ -444,7 +443,7 @@ int jsm_uart_port_init(struct jsm_board *brd)
brd->nasync = brd->maxports;
/* Set up channel variables */
- for (i = 0; i < brd->nasync; i++, ch = brd->channels[i]) {
+ for (i = 0; i < brd->nasync; i++) {
if (!brd->channels[i])
continue;
--
2.17.1
[PATCH] thunderbolt: remove redundant variable 'approved'
From: Colin Ian King
Variable 'approved' is being assigned but is never used hence it is
redundant and can be removed.
Cleans up clang warning:
warning: variable 'approved' set but not used [-Wunused-but-set-variable]
Signed-off-by: Colin Ian King
---
drivers/thunderbolt/icm.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index 500911f16498..2f50bc6a35fd 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -650,7 +650,6 @@ icm_fr_xdomain_connected(struct tb *tb, const struct
icm_pkg_header *hdr)
struct tb_xdomain *xd;
struct tb_switch *sw;
u8 link, depth;
- bool approved;
u64 route;
/*
@@ -664,7 +663,6 @@ icm_fr_xdomain_connected(struct tb *tb, const struct
icm_pkg_header *hdr)
link = pkg->link_info & ICM_LINK_INFO_LINK_MASK;
depth = (pkg->link_info & ICM_LINK_INFO_DEPTH_MASK) >>
ICM_LINK_INFO_DEPTH_SHIFT;
- approved = pkg->link_info & ICM_LINK_INFO_APPROVED;
if (link > ICM_MAX_LINK || depth > ICM_MAX_DEPTH) {
tb_warn(tb, "invalid topology %u.%u, ignoring\n", link, depth);
--
2.17.1
[PATCH] thunderbolt: remove redundant variable 'approved'
From: Colin Ian King
Variable 'approved' is being assigned but is never used hence it is
redundant and can be removed.
Cleans up clang warning:
warning: variable 'approved' set but not used [-Wunused-but-set-variable]
Signed-off-by: Colin Ian King
---
drivers/thunderbolt/icm.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index 500911f16498..2f50bc6a35fd 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -650,7 +650,6 @@ icm_fr_xdomain_connected(struct tb *tb, const struct
icm_pkg_header *hdr)
struct tb_xdomain *xd;
struct tb_switch *sw;
u8 link, depth;
- bool approved;
u64 route;
/*
@@ -664,7 +663,6 @@ icm_fr_xdomain_connected(struct tb *tb, const struct
icm_pkg_header *hdr)
link = pkg->link_info & ICM_LINK_INFO_LINK_MASK;
depth = (pkg->link_info & ICM_LINK_INFO_DEPTH_MASK) >>
ICM_LINK_INFO_DEPTH_SHIFT;
- approved = pkg->link_info & ICM_LINK_INFO_APPROVED;
if (link > ICM_MAX_LINK || depth > ICM_MAX_DEPTH) {
tb_warn(tb, "invalid topology %u.%u, ignoring\n", link, depth);
--
2.17.1
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
On Thu, Jul 12, 2018 at 11:42 PM Tomer Maimon wrote:
> Add Nuvoton BMC NPCM750/730/715/705 Pinmux and
> GPIO controller driver.
>
> Signed-off-by: Tomer Maimon
(...)
> +++ b/drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c
> @@ -0,0 +1,2089 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2016-2018 Nuvoton Technology corporation.
> +// Copyright (c) 2016, Dell Inc
> +
> +#include
> +#include
As this is a driver you should only include
> +#include
> +#include
> +#include
If you need syscon then the driver should select or depend
on MFD_SYSCON in Kconfig.
> +#include
> +#include
> +#include
> +#include
Do you really need this include?
> +/* Structure for register banks */
> +struct NPCM7XX_GPIO {
Can we have this lowercase? Please?
> + void __iomem*base;
> + struct gpio_chipgc;
> + int irqbase;
> + int irq;
> + spinlock_t lock;
> + void*priv;
> + struct irq_chip irq_chip;
> + u32 pinctrl_id;
> +};
So each GPIO bank has its own gpio chip and register
base, that is NICE! Because then it looks like you can
select GPIO_GENERIC and use the MMIO GPIO helper
library to handle the registers. Let's look into that
option!
> +struct NPCM7xx_pinctrl {
> + struct pinctrl_dev *pctldev;
> + struct device *dev;
> + struct NPCM7XX_GPIO gpio_bank[NPCM7XX_GPIO_BANK_NUM];
> + struct irq_domain *domain;
I wonder why the pin controller needs and IRQ domain but
I keep reading the code and I might find out...
> +enum operand {
> + op_set,
> + op_getbit,
> + op_setbit,
> + op_clrbit,
> +};
This looks complicated. I suspect you can use GPIO_GENERIC
to set/get and clear bits in the register(s).
> +/* Perform locked bit operations on GPIO registers */
> +static int gpio_bitop(struct NPCM7XX_GPIO *bank, int op, unsigned int offset,
> + int reg)
> +{
> + unsigned long flags;
> + u32 mask, val;
> +
> + mask = (1L << offset);
> + spin_lock_irqsave(>lock, flags);
> + switch (op) {
> + case op_set:
> + iowrite32(mask, bank->base + reg);
> + break;
> + case op_getbit:
> + mask &= ioread32(bank->base + reg);
> + break;
> + case op_setbit:
> + val = ioread32(bank->base + reg);
> + iowrite32(val | mask, bank->base + reg);
> + break;
> + case op_clrbit:
> + val = ioread32(bank->base + reg);
> + iowrite32(val & (~mask), bank->base + reg);
> + break;
> + }
> + spin_unlock_irqrestore(>lock, flags);
> + return !!mask;
> +}
This is essentially a reimplementation of drivers/gpio/gpio-mmio.c
(GPIO_GENERIC, also using a spinlock to protect the registers)
so let's use that instead :)
There are drivers already that reuse the spinlock inside the
generic GPIO chip to protect their other registers like for
IRQ registers.
> +static int npcmgpio_get_direction(struct gpio_chip *chip, unsigned int
> offset)
> +{
> + struct NPCM7XX_GPIO *bank = gpiochip_get_data(chip);
> + u32 oe, ie;
> +
> + /* Get Input & Output state */
> + ie = gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_IEM);
> + oe = gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_OE);
> + if (ie && !oe)
> + return GPIOF_DIR_IN;
> + else if (oe && !ie)
> + return GPIOF_DIR_OUT;
These are consumer flags and should not be used in drivers.
Use plain 0/1 instead.
Anyways the problem goes away with GPIO_GENERIC.
> +static int npcmgpio_direction_input(struct gpio_chip *chip, unsigned int
> offset)
> +{
> + return pinctrl_gpio_direction_input(offset + chip->base);
> +}
It's a bit tricksy to get this to work with GPIO_GENERIC.
After calling bgpio_init() you need to overwrite the assigned
.direction_input handler with this and then direct back to the
one assigned by GPIO_GENERIC.
Something like this:
1. Add two indirection pointers to the npcm7xx_gpio state container:
struct npcm7xx_gpio {
(...)
int (*direction_input)(struct gpio_chip *chip, unsigned offset);
int (*direction_output)(struct gpio_chip *chip, unsigned offset,
int value);
(...)
};
2. Save the pointers
struct npcm7xx_gpio *npcm;
bgpio_init( ... register setup ...)
npcm->direction_input = npcm->gc.direction_input;
npcm->direction_output = npcm->gc.direction_output;
npcm->gc.direction_input = npcmgpio_direction_input;
npcm->gc.direction_output = npcmgpio_direction_output;
3. Modify the functions like that:
static int npcmgpio_direction_input(struct gpio_chip *chip, unsigned int offset)
{
struct npcm7xx_gpio *npcm = gpiochip_get_data(chip);
int ret;
ret = pinctrl_gpio_direction_input(offset + chip->base);
if (ret)
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
On Thu, Jul 12, 2018 at 11:42 PM Tomer Maimon wrote:
> Add Nuvoton BMC NPCM750/730/715/705 Pinmux and
> GPIO controller driver.
>
> Signed-off-by: Tomer Maimon
(...)
> +++ b/drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c
> @@ -0,0 +1,2089 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2016-2018 Nuvoton Technology corporation.
> +// Copyright (c) 2016, Dell Inc
> +
> +#include
> +#include
As this is a driver you should only include
> +#include
> +#include
> +#include
If you need syscon then the driver should select or depend
on MFD_SYSCON in Kconfig.
> +#include
> +#include
> +#include
> +#include
Do you really need this include?
> +/* Structure for register banks */
> +struct NPCM7XX_GPIO {
Can we have this lowercase? Please?
> + void __iomem*base;
> + struct gpio_chipgc;
> + int irqbase;
> + int irq;
> + spinlock_t lock;
> + void*priv;
> + struct irq_chip irq_chip;
> + u32 pinctrl_id;
> +};
So each GPIO bank has its own gpio chip and register
base, that is NICE! Because then it looks like you can
select GPIO_GENERIC and use the MMIO GPIO helper
library to handle the registers. Let's look into that
option!
> +struct NPCM7xx_pinctrl {
> + struct pinctrl_dev *pctldev;
> + struct device *dev;
> + struct NPCM7XX_GPIO gpio_bank[NPCM7XX_GPIO_BANK_NUM];
> + struct irq_domain *domain;
I wonder why the pin controller needs and IRQ domain but
I keep reading the code and I might find out...
> +enum operand {
> + op_set,
> + op_getbit,
> + op_setbit,
> + op_clrbit,
> +};
This looks complicated. I suspect you can use GPIO_GENERIC
to set/get and clear bits in the register(s).
> +/* Perform locked bit operations on GPIO registers */
> +static int gpio_bitop(struct NPCM7XX_GPIO *bank, int op, unsigned int offset,
> + int reg)
> +{
> + unsigned long flags;
> + u32 mask, val;
> +
> + mask = (1L << offset);
> + spin_lock_irqsave(>lock, flags);
> + switch (op) {
> + case op_set:
> + iowrite32(mask, bank->base + reg);
> + break;
> + case op_getbit:
> + mask &= ioread32(bank->base + reg);
> + break;
> + case op_setbit:
> + val = ioread32(bank->base + reg);
> + iowrite32(val | mask, bank->base + reg);
> + break;
> + case op_clrbit:
> + val = ioread32(bank->base + reg);
> + iowrite32(val & (~mask), bank->base + reg);
> + break;
> + }
> + spin_unlock_irqrestore(>lock, flags);
> + return !!mask;
> +}
This is essentially a reimplementation of drivers/gpio/gpio-mmio.c
(GPIO_GENERIC, also using a spinlock to protect the registers)
so let's use that instead :)
There are drivers already that reuse the spinlock inside the
generic GPIO chip to protect their other registers like for
IRQ registers.
> +static int npcmgpio_get_direction(struct gpio_chip *chip, unsigned int
> offset)
> +{
> + struct NPCM7XX_GPIO *bank = gpiochip_get_data(chip);
> + u32 oe, ie;
> +
> + /* Get Input & Output state */
> + ie = gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_IEM);
> + oe = gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_OE);
> + if (ie && !oe)
> + return GPIOF_DIR_IN;
> + else if (oe && !ie)
> + return GPIOF_DIR_OUT;
These are consumer flags and should not be used in drivers.
Use plain 0/1 instead.
Anyways the problem goes away with GPIO_GENERIC.
> +static int npcmgpio_direction_input(struct gpio_chip *chip, unsigned int
> offset)
> +{
> + return pinctrl_gpio_direction_input(offset + chip->base);
> +}
It's a bit tricksy to get this to work with GPIO_GENERIC.
After calling bgpio_init() you need to overwrite the assigned
.direction_input handler with this and then direct back to the
one assigned by GPIO_GENERIC.
Something like this:
1. Add two indirection pointers to the npcm7xx_gpio state container:
struct npcm7xx_gpio {
(...)
int (*direction_input)(struct gpio_chip *chip, unsigned offset);
int (*direction_output)(struct gpio_chip *chip, unsigned offset,
int value);
(...)
};
2. Save the pointers
struct npcm7xx_gpio *npcm;
bgpio_init( ... register setup ...)
npcm->direction_input = npcm->gc.direction_input;
npcm->direction_output = npcm->gc.direction_output;
npcm->gc.direction_input = npcmgpio_direction_input;
npcm->gc.direction_output = npcmgpio_direction_output;
3. Modify the functions like that:
static int npcmgpio_direction_input(struct gpio_chip *chip, unsigned int offset)
{
struct npcm7xx_gpio *npcm = gpiochip_get_data(chip);
int ret;
ret = pinctrl_gpio_direction_input(offset + chip->base);
if (ret)
RE: [PATCH v2 4/6] arm: dts: add support for Laird WB50N cpu module and DVK
> Subject: Re: [PATCH v2 4/6] arm: dts: add support for Laird
> WB50N cpu module and DVK
>
> Hi,
>
> I've now applied the whole series after fixing two small
> whitespace
> issues.
Thanks!
> On 15/06/2018 14:40:53+0100, Ben Whitten wrote:
> > +_clk {
> > + atmel,clk-output-range = <0 13200>;
> > +};
> > +
>
> But this is not actually allowed by the hardware (well, it is
> but it
> will lead to issues) and will be removed once the clock
> binding is
> reworked.
Of course no problem, thanks again.
Ben
RE: [PATCH v2 4/6] arm: dts: add support for Laird WB50N cpu module and DVK
> Subject: Re: [PATCH v2 4/6] arm: dts: add support for Laird
> WB50N cpu module and DVK
>
> Hi,
>
> I've now applied the whole series after fixing two small
> whitespace
> issues.
Thanks!
> On 15/06/2018 14:40:53+0100, Ben Whitten wrote:
> > +_clk {
> > + atmel,clk-output-range = <0 13200>;
> > +};
> > +
>
> But this is not actually allowed by the hardware (well, it is
> but it
> will lead to issues) and will be removed once the clock
> binding is
> reworked.
Of course no problem, thanks again.
Ben
Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE
On 07/12/2018 07:33 PM, Eric W. Biederman wrote: > > Adrian Reber writes: > >> The CHECKPOINT_RESTORE configuration option was introduced in 2012 and >> combined with EXPERT. CHECKPOINT_RESTORE is already enabled in many >> distribution kernels and also part of the defconfigs of various >> architectures. >> >> To make it easier for distributions to enable CHECKPOINT_RESTORE this >> removes EXPERT and moves the configuration option out of the EXPERT >> block. > > I think we should change the help text at the same time, to match > our improve understanding of the situation. > > Does anyone remember why this option was added at all? Sure! Quoting Andrew's ~7 years ago akpm branch merge e-mail: However I'm less confident than the developers that it will all eventually work! So what I'm asking them to do is to wrap each piece of new code inside CONFIG_CHECKPOINT_RESTORE. So if it all eventually comes to tears and the project as a whole fails, it should be a simple matter to go through and delete all trace of it. the best link with full e-mail I googled for is https://gitlab.imag.fr/kaunetem/linux-kaunetem/commit/099469502f62fbe0d7e4f0b83a2f22538367f734 -- Pavel > Why this option was placed under expert? > > What is the value of disabling this functionality ever? > > Is there any reason why we don't just delete CONFIG_CHECKPOINT_RESTORE > entirely? > > Certainly we are at a point where distro's are enabling this so hiding > it behind CONFIG_EXPERT with a default of N seems inapparopriate. > > The only thing I can imagine might be sensible is changing the default > to Y and leaving it behind CONFIG_EXPERT. > > I want to know what is the point of maintaining all of the complexity of > the ifdefs. If no one can come up with a reason I say let's just remove > CONFIG_CHECKPOINT_RESTORE entirely. > > Eric > > >> Signed-off-by: Adrian Reber >> Cc: Oleg Nesterov >> Cc: Pavel Emelyanov >> Cc: Andrew Morton >> Cc: Eric W. Biederman >> Cc: Andrei Vagin >> Cc: Hendrik Brueckner >> --- >> init/Kconfig | 24 >> 1 file changed, 12 insertions(+), 12 deletions(-) >> >> diff --git a/init/Kconfig b/init/Kconfig >> index 041f3a022122..9c529c763326 100644 >> --- a/init/Kconfig >> +++ b/init/Kconfig >> @@ -932,6 +932,18 @@ config NET_NS >> >> endif # NAMESPACES >> >> +config CHECKPOINT_RESTORE >> +bool "Checkpoint/restore support" >> +select PROC_CHILDREN >> +default n >> +help >> + Enables additional kernel features in a sake of checkpoint/restore. >> + In particular it adds auxiliary prctl codes to setup process text, >> + data and heap segment sizes, and a few additional /proc filesystem >> + entries. >> + >> + If unsure, say N here. >> + >> config SCHED_AUTOGROUP >> bool "Automatic process group scheduling" >> select CGROUPS >> @@ -1348,18 +1360,6 @@ config MEMBARRIER >> >>If unsure, say Y. >> >> -config CHECKPOINT_RESTORE >> -bool "Checkpoint/restore support" if EXPERT >> -select PROC_CHILDREN >> -default n >> -help >> - Enables additional kernel features in a sake of checkpoint/restore. >> - In particular it adds auxiliary prctl codes to setup process text, >> - data and heap segment sizes, and a few additional /proc filesystem >> - entries. >> - >> - If unsure, say N here. >> - >> config KALLSYMS >> bool "Load all symbols for debugging/ksymoops" if EXPERT >> default y > . >
Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE
On 07/12/2018 07:33 PM, Eric W. Biederman wrote: > > Adrian Reber writes: > >> The CHECKPOINT_RESTORE configuration option was introduced in 2012 and >> combined with EXPERT. CHECKPOINT_RESTORE is already enabled in many >> distribution kernels and also part of the defconfigs of various >> architectures. >> >> To make it easier for distributions to enable CHECKPOINT_RESTORE this >> removes EXPERT and moves the configuration option out of the EXPERT >> block. > > I think we should change the help text at the same time, to match > our improve understanding of the situation. > > Does anyone remember why this option was added at all? Sure! Quoting Andrew's ~7 years ago akpm branch merge e-mail: However I'm less confident than the developers that it will all eventually work! So what I'm asking them to do is to wrap each piece of new code inside CONFIG_CHECKPOINT_RESTORE. So if it all eventually comes to tears and the project as a whole fails, it should be a simple matter to go through and delete all trace of it. the best link with full e-mail I googled for is https://gitlab.imag.fr/kaunetem/linux-kaunetem/commit/099469502f62fbe0d7e4f0b83a2f22538367f734 -- Pavel > Why this option was placed under expert? > > What is the value of disabling this functionality ever? > > Is there any reason why we don't just delete CONFIG_CHECKPOINT_RESTORE > entirely? > > Certainly we are at a point where distro's are enabling this so hiding > it behind CONFIG_EXPERT with a default of N seems inapparopriate. > > The only thing I can imagine might be sensible is changing the default > to Y and leaving it behind CONFIG_EXPERT. > > I want to know what is the point of maintaining all of the complexity of > the ifdefs. If no one can come up with a reason I say let's just remove > CONFIG_CHECKPOINT_RESTORE entirely. > > Eric > > >> Signed-off-by: Adrian Reber >> Cc: Oleg Nesterov >> Cc: Pavel Emelyanov >> Cc: Andrew Morton >> Cc: Eric W. Biederman >> Cc: Andrei Vagin >> Cc: Hendrik Brueckner >> --- >> init/Kconfig | 24 >> 1 file changed, 12 insertions(+), 12 deletions(-) >> >> diff --git a/init/Kconfig b/init/Kconfig >> index 041f3a022122..9c529c763326 100644 >> --- a/init/Kconfig >> +++ b/init/Kconfig >> @@ -932,6 +932,18 @@ config NET_NS >> >> endif # NAMESPACES >> >> +config CHECKPOINT_RESTORE >> +bool "Checkpoint/restore support" >> +select PROC_CHILDREN >> +default n >> +help >> + Enables additional kernel features in a sake of checkpoint/restore. >> + In particular it adds auxiliary prctl codes to setup process text, >> + data and heap segment sizes, and a few additional /proc filesystem >> + entries. >> + >> + If unsure, say N here. >> + >> config SCHED_AUTOGROUP >> bool "Automatic process group scheduling" >> select CGROUPS >> @@ -1348,18 +1360,6 @@ config MEMBARRIER >> >>If unsure, say Y. >> >> -config CHECKPOINT_RESTORE >> -bool "Checkpoint/restore support" if EXPERT >> -select PROC_CHILDREN >> -default n >> -help >> - Enables additional kernel features in a sake of checkpoint/restore. >> - In particular it adds auxiliary prctl codes to setup process text, >> - data and heap segment sizes, and a few additional /proc filesystem >> - entries. >> - >> - If unsure, say N here. >> - >> config KALLSYMS >> bool "Load all symbols for debugging/ksymoops" if EXPERT >> default y > . >
Re: [PATCH] staging: speakup: fix wraparound in uaccess length check
On Thu, Jul 12, 2018 at 04:12:39PM -0700, Jann Horn wrote: > On Thu, Jul 12, 2018 at 3:47 PM Al Viro wrote: > > > > On Fri, Jul 13, 2018 at 12:29:36AM +0200, Jann Horn wrote: > > > From: Samuel Thibault > > > > > > From: Samuel Thibault > > > > > > If softsynthx_read() is called with `count < 3`, `count - 3` wraps, > > > causing > > > the loop to copy as much data as available to the provided buffer. If > > > softsynthx_read() is invoked through sys_splice(), this causes an > > > unbounded kernel write; but even when userspace just reads from it > > > normally, a small size could cause userspace crashes. > > > > Or you could try this (completely untested, though): > > I think this has the same problem as my original buggy patch: At the > point where you notice that you'd overflow the buffer, you've already > consumed a character from the synth buffer. You'd have to put it back, > and since the spinlock protecting it has been dropped, that's a bit > weird. > > Also, I'm not sure whether Greg prefers fixes for stable kernels that > don't also contain cleanup? For staging code, I really don't care, as long as it's fixing an issue :) thanks, greg k-h
Re: [PATCH] staging: speakup: fix wraparound in uaccess length check
On Thu, Jul 12, 2018 at 04:12:39PM -0700, Jann Horn wrote: > On Thu, Jul 12, 2018 at 3:47 PM Al Viro wrote: > > > > On Fri, Jul 13, 2018 at 12:29:36AM +0200, Jann Horn wrote: > > > From: Samuel Thibault > > > > > > From: Samuel Thibault > > > > > > If softsynthx_read() is called with `count < 3`, `count - 3` wraps, > > > causing > > > the loop to copy as much data as available to the provided buffer. If > > > softsynthx_read() is invoked through sys_splice(), this causes an > > > unbounded kernel write; but even when userspace just reads from it > > > normally, a small size could cause userspace crashes. > > > > Or you could try this (completely untested, though): > > I think this has the same problem as my original buggy patch: At the > point where you notice that you'd overflow the buffer, you've already > consumed a character from the synth buffer. You'd have to put it back, > and since the spinlock protecting it has been dropped, that's a bit > weird. > > Also, I'm not sure whether Greg prefers fixes for stable kernels that > don't also contain cleanup? For staging code, I really don't care, as long as it's fixing an issue :) thanks, greg k-h
Re: [PATCH 1/2] dt-bindings: pfuze100: add optional pfuze-disable-sw binding
Hi Mark, On 18-07-12 16:31, Mark Brown wrote: > On Thu, Jul 12, 2018 at 01:02:39PM +0200, Marco Felsch wrote: > > > +Optional properties: > > +- pfuze-disable-sw: Disable all unused switch regulators to save power > > + consumption. Attention, some platforms are using the switch regulators > > as DDR > > + ref or supply voltage. Mark these regulators as "regulator-always-on" to > > skip > > + disabling these regulators. If not specified, the driver simualtes the > > + disabling. This means the state of the regulator is set to 'disabled' > > but the > > + driver don't disable the regulator. > > This is a bit of a confused way of specifying things that depends on the > Linux implementation, and the property sounds like a double negative > too. I'd say something like "pfuze-support-disable" and then explicitly > say that this is a workaround for backwards compatibility. I can't find the double negative. Anyway your binding sounds better. So I will use yours. Should we add a vendor prefix too to be clear? I will also add some more informations to mark it as workaround. > I'd also recommend changing the implementation patch to just register a > different version of the desc and ops that just doesn't have the disable > operation so that the framework knows what's going on. While the > current implementation works now there's the possibility that at some > point in the future we might start relying on the disable actually > having taken effect somehow and will get confused. There's some > existing drivers that optimize their resume paths if they know power > wasn't removed. Okay I will change that too. I didn't know that there are drivers with optimized resume paths. Thanks for your feedback. Regards, Marco
Re: [PATCH 1/2] dt-bindings: pfuze100: add optional pfuze-disable-sw binding
Hi Mark, On 18-07-12 16:31, Mark Brown wrote: > On Thu, Jul 12, 2018 at 01:02:39PM +0200, Marco Felsch wrote: > > > +Optional properties: > > +- pfuze-disable-sw: Disable all unused switch regulators to save power > > + consumption. Attention, some platforms are using the switch regulators > > as DDR > > + ref or supply voltage. Mark these regulators as "regulator-always-on" to > > skip > > + disabling these regulators. If not specified, the driver simualtes the > > + disabling. This means the state of the regulator is set to 'disabled' > > but the > > + driver don't disable the regulator. > > This is a bit of a confused way of specifying things that depends on the > Linux implementation, and the property sounds like a double negative > too. I'd say something like "pfuze-support-disable" and then explicitly > say that this is a workaround for backwards compatibility. I can't find the double negative. Anyway your binding sounds better. So I will use yours. Should we add a vendor prefix too to be clear? I will also add some more informations to mark it as workaround. > I'd also recommend changing the implementation patch to just register a > different version of the desc and ops that just doesn't have the disable > operation so that the framework knows what's going on. While the > current implementation works now there's the possibility that at some > point in the future we might start relying on the disable actually > having taken effect somehow and will get confused. There's some > existing drivers that optimize their resume paths if they know power > wasn't removed. Okay I will change that too. I didn't know that there are drivers with optimized resume paths. Thanks for your feedback. Regards, Marco
Re: [PATCH v3 3/3] clk: qcom: Add display clock controller driver for SDM845
On 2018-07-13 01:11, Stephen Boyd wrote: Quoting Taniya Das (2018-07-12 10:21:33) ++ Display driver team, On 7/9/2018 8:36 PM, Stephen Boyd wrote: > Quoting Taniya Das (2018-07-09 02:34:07) >> >> >> On 7/9/2018 1:07 PM, Stephen Boyd wrote: >>> Quoting Taniya Das (2018-07-09 00:07:21) On 7/9/2018 11:46 AM, Stephen Boyd wrote: >> >> > Why is the nocache flag needed? Applies to all clks in this file. >> > >> >> This flag is required for all RCGs whose PLLs are controlled outside the >> clock controller. The display code would require the recalculated rate >> always. > > Right. Why is the PLL controlled outside of the clock controller? The > rate should propagate upward to the PLL from here, so who's going > outside of that? > The DSI0/1 PLL are not part of the display clock controller, but in the display subsystem which are managed by the DRM drivers. When DRM drivers query for the rate clock driver should always return the non cached rates. >>> >>> Why? Is the DSI PLL changing rate all the time, randomly, without going >>> through the clk APIs to do so? >>> >> >> Hmm, I am afraid I do not have an answer for this, but this was the >> requirement to always return the non cached rates from the clock driver. >> > > Ok. Who knows about this requirement? Can we add someone from the > display driver to understand more? > As per my discussions offline with the display teams, There is a use-case where the clock framework is unaware of the PLL VCO frequency change and thus the drivers would query to get the actual HW frequency rather than the cached one. Do you think keeping these flags would have any impact other than always getting the non-cached rates? The flag will make it so clk_get_rate() works in spite of something changing the frequency behind the framework's back, but I want to understand what and why it's changing without framework involvement. We shouldn't need the flag here, because this flag is typically for clks that are controlled by some other entity that the kernel doesn't have control over. In this case, it seems like we have full control of the clk tree for the display PLL down to this clk, so it should be perfectly fine to not have this flag. The presence of the flag means that the display driver is doing something wrong. These clocks are sourced from DSI PLL. In dsi command mode there is an idle use case when there is no activity on display, we switch of the source DSI PLL to save power by calling clk_disable_unprepare(). In this scenario if some client queries the clk_get_rate(), then if NO_CACHE flag is used the clk driver will return the cached rate which is some non-zero value set when we last called clk_set_rate(), before enabling the clock, whereas actually in HW this clk is disabled. So we used the NO_CACHE flag for the call to land in clk_recalc_rate and we return zero if the PLL is disabled. I remember some customer had scripts that runs through all the clocks during idle screen scenario and call clk_get_rate(), where they complained display clk_get_rate returns none zero value.
Re: [PATCH v3 3/3] clk: qcom: Add display clock controller driver for SDM845
On 2018-07-13 01:11, Stephen Boyd wrote: Quoting Taniya Das (2018-07-12 10:21:33) ++ Display driver team, On 7/9/2018 8:36 PM, Stephen Boyd wrote: > Quoting Taniya Das (2018-07-09 02:34:07) >> >> >> On 7/9/2018 1:07 PM, Stephen Boyd wrote: >>> Quoting Taniya Das (2018-07-09 00:07:21) On 7/9/2018 11:46 AM, Stephen Boyd wrote: >> >> > Why is the nocache flag needed? Applies to all clks in this file. >> > >> >> This flag is required for all RCGs whose PLLs are controlled outside the >> clock controller. The display code would require the recalculated rate >> always. > > Right. Why is the PLL controlled outside of the clock controller? The > rate should propagate upward to the PLL from here, so who's going > outside of that? > The DSI0/1 PLL are not part of the display clock controller, but in the display subsystem which are managed by the DRM drivers. When DRM drivers query for the rate clock driver should always return the non cached rates. >>> >>> Why? Is the DSI PLL changing rate all the time, randomly, without going >>> through the clk APIs to do so? >>> >> >> Hmm, I am afraid I do not have an answer for this, but this was the >> requirement to always return the non cached rates from the clock driver. >> > > Ok. Who knows about this requirement? Can we add someone from the > display driver to understand more? > As per my discussions offline with the display teams, There is a use-case where the clock framework is unaware of the PLL VCO frequency change and thus the drivers would query to get the actual HW frequency rather than the cached one. Do you think keeping these flags would have any impact other than always getting the non-cached rates? The flag will make it so clk_get_rate() works in spite of something changing the frequency behind the framework's back, but I want to understand what and why it's changing without framework involvement. We shouldn't need the flag here, because this flag is typically for clks that are controlled by some other entity that the kernel doesn't have control over. In this case, it seems like we have full control of the clk tree for the display PLL down to this clk, so it should be perfectly fine to not have this flag. The presence of the flag means that the display driver is doing something wrong. These clocks are sourced from DSI PLL. In dsi command mode there is an idle use case when there is no activity on display, we switch of the source DSI PLL to save power by calling clk_disable_unprepare(). In this scenario if some client queries the clk_get_rate(), then if NO_CACHE flag is used the clk driver will return the cached rate which is some non-zero value set when we last called clk_set_rate(), before enabling the clock, whereas actually in HW this clk is disabled. So we used the NO_CACHE flag for the call to land in clk_recalc_rate and we return zero if the PLL is disabled. I remember some customer had scripts that runs through all the clocks during idle screen scenario and call clk_get_rate(), where they complained display clk_get_rate returns none zero value.
Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE
On 07/12/2018 04:07 PM, Adrian Reber wrote: > The CHECKPOINT_RESTORE configuration option was introduced in 2012 and > combined with EXPERT. CHECKPOINT_RESTORE is already enabled in many > distribution kernels and also part of the defconfigs of various > architectures. > > To make it easier for distributions to enable CHECKPOINT_RESTORE this > removes EXPERT and moves the configuration option out of the EXPERT > block. > > Signed-off-by: Adrian Reber > Cc: Oleg Nesterov > Cc: Pavel Emelyanov > Cc: Andrew Morton > Cc: Eric W. Biederman > Cc: Andrei Vagin > Cc: Hendrik Brueckner Acked-by: Pavel Emelyanov > --- > init/Kconfig | 24 > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/init/Kconfig b/init/Kconfig > index 041f3a022122..9c529c763326 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -932,6 +932,18 @@ config NET_NS > > endif # NAMESPACES > > +config CHECKPOINT_RESTORE > + bool "Checkpoint/restore support" > + select PROC_CHILDREN > + default n > + help > + Enables additional kernel features in a sake of checkpoint/restore. > + In particular it adds auxiliary prctl codes to setup process text, > + data and heap segment sizes, and a few additional /proc filesystem > + entries. > + > + If unsure, say N here. > + > config SCHED_AUTOGROUP > bool "Automatic process group scheduling" > select CGROUPS > @@ -1348,18 +1360,6 @@ config MEMBARRIER > > If unsure, say Y. > > -config CHECKPOINT_RESTORE > - bool "Checkpoint/restore support" if EXPERT > - select PROC_CHILDREN > - default n > - help > - Enables additional kernel features in a sake of checkpoint/restore. > - In particular it adds auxiliary prctl codes to setup process text, > - data and heap segment sizes, and a few additional /proc filesystem > - entries. > - > - If unsure, say N here. > - > config KALLSYMS >bool "Load all symbols for debugging/ksymoops" if EXPERT >default y >
Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE
On 07/12/2018 04:07 PM, Adrian Reber wrote: > The CHECKPOINT_RESTORE configuration option was introduced in 2012 and > combined with EXPERT. CHECKPOINT_RESTORE is already enabled in many > distribution kernels and also part of the defconfigs of various > architectures. > > To make it easier for distributions to enable CHECKPOINT_RESTORE this > removes EXPERT and moves the configuration option out of the EXPERT > block. > > Signed-off-by: Adrian Reber > Cc: Oleg Nesterov > Cc: Pavel Emelyanov > Cc: Andrew Morton > Cc: Eric W. Biederman > Cc: Andrei Vagin > Cc: Hendrik Brueckner Acked-by: Pavel Emelyanov > --- > init/Kconfig | 24 > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/init/Kconfig b/init/Kconfig > index 041f3a022122..9c529c763326 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -932,6 +932,18 @@ config NET_NS > > endif # NAMESPACES > > +config CHECKPOINT_RESTORE > + bool "Checkpoint/restore support" > + select PROC_CHILDREN > + default n > + help > + Enables additional kernel features in a sake of checkpoint/restore. > + In particular it adds auxiliary prctl codes to setup process text, > + data and heap segment sizes, and a few additional /proc filesystem > + entries. > + > + If unsure, say N here. > + > config SCHED_AUTOGROUP > bool "Automatic process group scheduling" > select CGROUPS > @@ -1348,18 +1360,6 @@ config MEMBARRIER > > If unsure, say Y. > > -config CHECKPOINT_RESTORE > - bool "Checkpoint/restore support" if EXPERT > - select PROC_CHILDREN > - default n > - help > - Enables additional kernel features in a sake of checkpoint/restore. > - In particular it adds auxiliary prctl codes to setup process text, > - data and heap segment sizes, and a few additional /proc filesystem > - entries. > - > - If unsure, say N here. > - > config KALLSYMS >bool "Load all symbols for debugging/ksymoops" if EXPERT >default y >
Re: [PATCH 1/3] tty/serial_core: add ISO7816 infrastructure
On Fri, Jul 13, 2018 at 09:56:48AM +0200, Ludovic Desroches wrote:
> On Thu, Jul 12, 2018 at 05:02:29PM +0200, Greg KH wrote:
> > On Wed, Jul 11, 2018 at 03:16:36PM +0200, Ludovic Desroches wrote:
> > > From: Nicolas Ferre
> > >
> > > Add the ISO7816 ioctl and associated accessors and data structure.
> > > Drivers can then use this common implementation to handle ISO7816.
> > >
> > > Signed-off-by: Nicolas Ferre
> > > [ludovic.desroc...@microchip.com: squash and rebase, removal of gpios,
> > > checkpatch fixes]
> > > Signed-off-by: Ludovic Desroches
> > > ---
> > > drivers/tty/serial/serial_core.c | 49
> > > +++
> > > include/linux/serial_core.h | 3 +++
> > > include/uapi/asm-generic/ioctls.h | 2 ++
> > > include/uapi/linux/serial.h | 17 ++
> > > 4 files changed, 71 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/serial_core.c
> > > b/drivers/tty/serial/serial_core.c
> > > index 9c14a453f73c..c89ac59f6f8c 100644
> > > --- a/drivers/tty/serial/serial_core.c
> > > +++ b/drivers/tty/serial/serial_core.c
> > > @@ -1301,6 +1301,47 @@ static int uart_set_rs485_config(struct uart_port
> > > *port,
> > > return 0;
> > > }
> > >
> > > +static int uart_get_iso7816_config(struct uart_port *port,
> > > +struct serial_iso7816 __user *iso7816)
> > > +{
> > > + unsigned long flags;
> > > + struct serial_iso7816 aux;
> > > +
> > > + spin_lock_irqsave(>lock, flags);
> > > + aux = port->iso7816;
> > > + spin_unlock_irqrestore(>lock, flags);
> > > +
> > > + if (copy_to_user(iso7816, , sizeof(aux)))
> > > + return -EFAULT;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int uart_set_iso7816_config(struct uart_port *port,
> > > +struct serial_iso7816 __user *iso7816_user)
> > > +{
> > > + struct serial_iso7816 iso7816;
> > > + int ret;
> > > + unsigned long flags;
> > > +
> > > + if (!port->iso7816_config)
> > > + return -ENOIOCTLCMD;
> > > +
> > > + if (copy_from_user(, iso7816_user, sizeof(*iso7816_user)))
> > > + return -EFAULT;
> > > +
> > > + spin_lock_irqsave(>lock, flags);
> > > + ret = port->iso7816_config(port, );
> > > + spin_unlock_irqrestore(>lock, flags);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (copy_to_user(iso7816_user, >iso7816, sizeof(port->iso7816)))
> > > + return -EFAULT;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > /*
> > > * Called via sys_ioctl. We can use spin_lock_irq() here.
> > > */
> > > @@ -1385,6 +1426,14 @@ uart_ioctl(struct tty_struct *tty, unsigned int
> > > cmd, unsigned long arg)
> > > case TIOCSRS485:
> > > ret = uart_set_rs485_config(uport, uarg);
> > > break;
> > > +
> > > + case TIOCSISO7816:
> > > + ret = uart_set_iso7816_config(state->uart_port, uarg);
> > > + break;
> > > +
> > > + case TIOCGISO7816:
> > > + ret = uart_get_iso7816_config(state->uart_port, uarg);
> > > + break;
> > > default:
> > > if (uport->ops->ioctl)
> > > ret = uport->ops->ioctl(uport, cmd, arg);
> > > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> > > index 06ea4eeb09ab..d6e7747ffd46 100644
> > > --- a/include/linux/serial_core.h
> > > +++ b/include/linux/serial_core.h
> > > @@ -137,6 +137,8 @@ struct uart_port {
> > > void(*handle_break)(struct uart_port *);
> > > int (*rs485_config)(struct uart_port *,
> > > struct serial_rs485 *rs485);
> > > + int (*iso7816_config)(struct uart_port *,
> > > + struct serial_iso7816
> > > *iso7816);
> > > unsigned intirq;/* irq number */
> > > unsigned long irqflags; /* irq flags */
> > > unsigned intuartclk;/* base uart clock */
> > > @@ -253,6 +255,7 @@ struct uart_port {
> > > struct attribute_group *attr_group;/* port specific
> > > attributes */
> > > const struct attribute_group **tty_groups; /* all attributes
> > > (serial core use only) */
> > > struct serial_rs485 rs485;
> > > + struct serial_iso7816 iso7816;
> > > void*private_data; /* generic platform
> > > data pointer */
> > > };
> > >
> > > diff --git a/include/uapi/asm-generic/ioctls.h
> > > b/include/uapi/asm-generic/ioctls.h
> > > index 040651735662..0e5c79726c2d 100644
> > > --- a/include/uapi/asm-generic/ioctls.h
> > > +++ b/include/uapi/asm-generic/ioctls.h
> > > @@ -66,6 +66,8 @@
> > > #ifndef TIOCSRS485
> > > #define TIOCSRS485 0x542F
> > > #endif
> > > +#define TIOCGISO7816 0x5430
> > > +#define TIOCSISO7816 0x5431
> > > #define TIOCGPTN _IOR('T', 0x30, unsigned int) /* Get Pty Number (of
> > > pty-mux device) */
> > > #define TIOCSPTLCK _IOW('T',
Re: [PATCH 1/3] tty/serial_core: add ISO7816 infrastructure
On Fri, Jul 13, 2018 at 09:56:48AM +0200, Ludovic Desroches wrote:
> On Thu, Jul 12, 2018 at 05:02:29PM +0200, Greg KH wrote:
> > On Wed, Jul 11, 2018 at 03:16:36PM +0200, Ludovic Desroches wrote:
> > > From: Nicolas Ferre
> > >
> > > Add the ISO7816 ioctl and associated accessors and data structure.
> > > Drivers can then use this common implementation to handle ISO7816.
> > >
> > > Signed-off-by: Nicolas Ferre
> > > [ludovic.desroc...@microchip.com: squash and rebase, removal of gpios,
> > > checkpatch fixes]
> > > Signed-off-by: Ludovic Desroches
> > > ---
> > > drivers/tty/serial/serial_core.c | 49
> > > +++
> > > include/linux/serial_core.h | 3 +++
> > > include/uapi/asm-generic/ioctls.h | 2 ++
> > > include/uapi/linux/serial.h | 17 ++
> > > 4 files changed, 71 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/serial_core.c
> > > b/drivers/tty/serial/serial_core.c
> > > index 9c14a453f73c..c89ac59f6f8c 100644
> > > --- a/drivers/tty/serial/serial_core.c
> > > +++ b/drivers/tty/serial/serial_core.c
> > > @@ -1301,6 +1301,47 @@ static int uart_set_rs485_config(struct uart_port
> > > *port,
> > > return 0;
> > > }
> > >
> > > +static int uart_get_iso7816_config(struct uart_port *port,
> > > +struct serial_iso7816 __user *iso7816)
> > > +{
> > > + unsigned long flags;
> > > + struct serial_iso7816 aux;
> > > +
> > > + spin_lock_irqsave(>lock, flags);
> > > + aux = port->iso7816;
> > > + spin_unlock_irqrestore(>lock, flags);
> > > +
> > > + if (copy_to_user(iso7816, , sizeof(aux)))
> > > + return -EFAULT;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int uart_set_iso7816_config(struct uart_port *port,
> > > +struct serial_iso7816 __user *iso7816_user)
> > > +{
> > > + struct serial_iso7816 iso7816;
> > > + int ret;
> > > + unsigned long flags;
> > > +
> > > + if (!port->iso7816_config)
> > > + return -ENOIOCTLCMD;
> > > +
> > > + if (copy_from_user(, iso7816_user, sizeof(*iso7816_user)))
> > > + return -EFAULT;
> > > +
> > > + spin_lock_irqsave(>lock, flags);
> > > + ret = port->iso7816_config(port, );
> > > + spin_unlock_irqrestore(>lock, flags);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (copy_to_user(iso7816_user, >iso7816, sizeof(port->iso7816)))
> > > + return -EFAULT;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > /*
> > > * Called via sys_ioctl. We can use spin_lock_irq() here.
> > > */
> > > @@ -1385,6 +1426,14 @@ uart_ioctl(struct tty_struct *tty, unsigned int
> > > cmd, unsigned long arg)
> > > case TIOCSRS485:
> > > ret = uart_set_rs485_config(uport, uarg);
> > > break;
> > > +
> > > + case TIOCSISO7816:
> > > + ret = uart_set_iso7816_config(state->uart_port, uarg);
> > > + break;
> > > +
> > > + case TIOCGISO7816:
> > > + ret = uart_get_iso7816_config(state->uart_port, uarg);
> > > + break;
> > > default:
> > > if (uport->ops->ioctl)
> > > ret = uport->ops->ioctl(uport, cmd, arg);
> > > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> > > index 06ea4eeb09ab..d6e7747ffd46 100644
> > > --- a/include/linux/serial_core.h
> > > +++ b/include/linux/serial_core.h
> > > @@ -137,6 +137,8 @@ struct uart_port {
> > > void(*handle_break)(struct uart_port *);
> > > int (*rs485_config)(struct uart_port *,
> > > struct serial_rs485 *rs485);
> > > + int (*iso7816_config)(struct uart_port *,
> > > + struct serial_iso7816
> > > *iso7816);
> > > unsigned intirq;/* irq number */
> > > unsigned long irqflags; /* irq flags */
> > > unsigned intuartclk;/* base uart clock */
> > > @@ -253,6 +255,7 @@ struct uart_port {
> > > struct attribute_group *attr_group;/* port specific
> > > attributes */
> > > const struct attribute_group **tty_groups; /* all attributes
> > > (serial core use only) */
> > > struct serial_rs485 rs485;
> > > + struct serial_iso7816 iso7816;
> > > void*private_data; /* generic platform
> > > data pointer */
> > > };
> > >
> > > diff --git a/include/uapi/asm-generic/ioctls.h
> > > b/include/uapi/asm-generic/ioctls.h
> > > index 040651735662..0e5c79726c2d 100644
> > > --- a/include/uapi/asm-generic/ioctls.h
> > > +++ b/include/uapi/asm-generic/ioctls.h
> > > @@ -66,6 +66,8 @@
> > > #ifndef TIOCSRS485
> > > #define TIOCSRS485 0x542F
> > > #endif
> > > +#define TIOCGISO7816 0x5430
> > > +#define TIOCSISO7816 0x5431
> > > #define TIOCGPTN _IOR('T', 0x30, unsigned int) /* Get Pty Number (of
> > > pty-mux device) */
> > > #define TIOCSPTLCK _IOW('T',
Re: [PATCH] pinctrl: mt7622: fix probe fail by misuse the selector
On Thu, Jul 12, 2018 at 7:50 AM wrote:
> From: Sean Wang
>
> After the commit acf137951367 ("pinctrl: core: Return selector to the
> pinctrl driver") and the commit 47f1242d19c3 ("pinctrl: pinmux: Return
> selector to the pinctrl driver"), it's necessary to add the fixes
> needed for the pin controller drivers to use the appropriate returned
> selector for a negative error number returned in case of the fail at
> these functions. Otherwise, the driver would have a failed probe and
> that causes boot message cannot correctly output and devices fail
> to acquire their own pins.
>
> Cc: Kevin Hilman
> Fixes: acf137951367 ("pinctrl: core: Return selector to the pinctrl driver")
> Fixes: 47f1242d19c3 ("pinctrl: pinmux: Return selector to the pinctrl driver")
> Signed-off-by: Sean Wang
Applied on top of Tony's patches on the fixes branch.
Now there are fixes piling on top of fixes and I am starting to feel
insecure of pushing this to v4.18 and I feel like letting these
fixes go to v4.19 (it can be picked to stable from there).
Tony: do you think there could be more fallout like this?
Yours,
Linus Walleij
Re: [PATCH] pinctrl: mt7622: fix probe fail by misuse the selector
On Thu, Jul 12, 2018 at 7:50 AM wrote:
> From: Sean Wang
>
> After the commit acf137951367 ("pinctrl: core: Return selector to the
> pinctrl driver") and the commit 47f1242d19c3 ("pinctrl: pinmux: Return
> selector to the pinctrl driver"), it's necessary to add the fixes
> needed for the pin controller drivers to use the appropriate returned
> selector for a negative error number returned in case of the fail at
> these functions. Otherwise, the driver would have a failed probe and
> that causes boot message cannot correctly output and devices fail
> to acquire their own pins.
>
> Cc: Kevin Hilman
> Fixes: acf137951367 ("pinctrl: core: Return selector to the pinctrl driver")
> Fixes: 47f1242d19c3 ("pinctrl: pinmux: Return selector to the pinctrl driver")
> Signed-off-by: Sean Wang
Applied on top of Tony's patches on the fixes branch.
Now there are fixes piling on top of fixes and I am starting to feel
insecure of pushing this to v4.18 and I feel like letting these
fixes go to v4.19 (it can be picked to stable from there).
Tony: do you think there could be more fallout like this?
Yours,
Linus Walleij
Re: [PATCH v2 3/6] ARM: trusted_foundations: do not use naked function
On 13.07.2018 01:01, Russell King - ARM Linux wrote:
> On Thu, Jul 12, 2018 at 03:43:10PM -0700, Kees Cook wrote:
>> On Tue, Apr 17, 2018 at 1:11 AM, Thierry Reding wrote:
>> > On Mon, Apr 16, 2018 at 08:21:09PM +0200, Stefan Agner wrote:
>> >> On 16.04.2018 18:08, Stephen Warren wrote:
>> >> > On 04/16/2018 09:56 AM, Stefan Agner wrote:
>> >> >> On 27.03.2018 14:16, Dmitry Osipenko wrote:
>> >> >>> On 27.03.2018 14:54, Robin Murphy wrote:
>> >> On 26/03/18 22:20, Dmitry Osipenko wrote:
>> >> > On 25.03.2018 21:09, Stefan Agner wrote:
>> >> >> As documented in GCC naked functions should only use Basic asm
>> >> >> syntax. The Extended asm or mixture of Basic asm and "C" code is
>> >> >> not guaranteed. Currently this works because it was hard coded
>> >> >> to follow and check GCC behavior for arguments and register
>> >> >> placement.
>> >> >>
>> >> >> Furthermore with clang using parameters in Extended asm in a
>> >> >> naked function is not supported:
>> >> >> arch/arm/firmware/trusted_foundations.c:47:10: error: parameter
>> >> >> references not allowed in naked functions
>> >> >> : "r" (type), "r" (arg1), "r" (arg2)
>> >> >> ^
>> >> >>
>> >> >> Use a regular function to be more portable. This aligns also with
>> >> >> the other smc call implementations e.g. in qcom_scm-32.c and
>> >> >> bcm_kona_smc.c.
>> >> >>
>> >> >> Cc: Dmitry Osipenko
>> >> >> Cc: Stephen Warren
>> >> >> Cc: Thierry Reding
>> >> >> Signed-off-by: Stefan Agner
>> >> >> ---
>> >> >> Changes in v2:
>> >> >> - Keep stmfd/ldmfd to avoid potential ABI issues
>> >> >>
>> >> >>arch/arm/firmware/trusted_foundations.c | 14 +-
>> >> >>1 file changed, 9 insertions(+), 5 deletions(-)
>> >> >>
>> >> >> diff --git a/arch/arm/firmware/trusted_foundations.c
>> >> >> b/arch/arm/firmware/trusted_foundations.c
>> >> >> index 3fb1b5a1dce9..689e6565abfc 100644
>> >> >> --- a/arch/arm/firmware/trusted_foundations.c
>> >> >> +++ b/arch/arm/firmware/trusted_foundations.c
>> >> >> @@ -31,21 +31,25 @@
>> >> >> static unsigned long cpu_boot_addr;
>> >> >>-static void __naked tf_generic_smc(u32 type, u32 arg1, u32
>> >> >> arg2)
>> >> >> +static void tf_generic_smc(u32 type, u32 arg1, u32 arg2)
>> >> >>{
>> >> >> +register u32 r0 asm("r0") = type;
>> >> >> +register u32 r1 asm("r1") = arg1;
>> >> >> +register u32 r2 asm("r2") = arg2;
>> >> >> +
>> >> >>asm volatile(
>> >> >>".arch_extensionsec\n\t"
>> >> >> -"stmfdsp!, {r4 - r11, lr}\n\t"
>> >> >> +"stmfdsp!, {r4 - r11}\n\t"
>> >> >>__asmeq("%0", "r0")
>> >> >>__asmeq("%1", "r1")
>> >> >>__asmeq("%2", "r2")
>> >> >>"movr3, #0\n\t"
>> >> >>"movr4, #0\n\t"
>> >> >>"smc#0\n\t"
>> >> >> -"ldmfdsp!, {r4 - r11, pc}"
>> >> >> +"ldmfdsp!, {r4 - r11}\n\t"
>> >> >>:
>> >> >> -: "r" (type), "r" (arg1), "r" (arg2)
>> >> >> -: "memory");
>> >> >> +: "r" (r0), "r" (r1), "r" (r2)
>> >> >> +: "memory", "r3", "r12", "lr");
>> >> >
>> >> > Although seems "lr" won't be affected by SMC invocation because it
>> >> > should be
>> >> > banked and hence could be omitted entirely from the code. Maybe
>> >> > somebody could
>> >> > confirm this.
>> >> Strictly per the letter of the architecture, the SMC could be
>> >> trapped to Hyp
>> >> mode, and a hypervisor might clobber LR_usr in the process of
>> >> forwarding the
>> >> call to the firmware secure monitor (since Hyp doesn't have a banked
>> >> LR of its
>> >> own). Admittedly there are probably no real systems with the
>> >> appropriate
>> >> hardware/software combination to hit that, but on the other hand if
>> >> this gets
>> >> inlined where the compiler has already created a stack frame then an
>> >> LR clobber
>> >> is essentially free, so I reckon we're better off keeping it for
>> >> reassurance.
>> >> This isn't exactly a critical fast path anyway.
>> >> >>>
>> >> >>> Okay, thank you for the clarification.
>> >> >>
>> >> >> So it seems this change is fine?
>> >> >>
>> >> >> Stephen, you picked up changes for this driver before, is this patch
>> >> >> going through your tree?
>> >> >
>> >> > You had best ask Thierry; he's taken over Tegra maintenance upstream.
>> >> > But that said, don't files in arch/arm go through Russell?
>> >>
>> >> I think the last patches applied to that file went through your tree.
>> >>
>> >> Thierry, Russel, any preferences?
>> >
>> > I don't mind picking this up into the Tegra tree. Might
Re: [PATCH v2 3/6] ARM: trusted_foundations: do not use naked function
On 13.07.2018 01:01, Russell King - ARM Linux wrote:
> On Thu, Jul 12, 2018 at 03:43:10PM -0700, Kees Cook wrote:
>> On Tue, Apr 17, 2018 at 1:11 AM, Thierry Reding wrote:
>> > On Mon, Apr 16, 2018 at 08:21:09PM +0200, Stefan Agner wrote:
>> >> On 16.04.2018 18:08, Stephen Warren wrote:
>> >> > On 04/16/2018 09:56 AM, Stefan Agner wrote:
>> >> >> On 27.03.2018 14:16, Dmitry Osipenko wrote:
>> >> >>> On 27.03.2018 14:54, Robin Murphy wrote:
>> >> On 26/03/18 22:20, Dmitry Osipenko wrote:
>> >> > On 25.03.2018 21:09, Stefan Agner wrote:
>> >> >> As documented in GCC naked functions should only use Basic asm
>> >> >> syntax. The Extended asm or mixture of Basic asm and "C" code is
>> >> >> not guaranteed. Currently this works because it was hard coded
>> >> >> to follow and check GCC behavior for arguments and register
>> >> >> placement.
>> >> >>
>> >> >> Furthermore with clang using parameters in Extended asm in a
>> >> >> naked function is not supported:
>> >> >> arch/arm/firmware/trusted_foundations.c:47:10: error: parameter
>> >> >> references not allowed in naked functions
>> >> >> : "r" (type), "r" (arg1), "r" (arg2)
>> >> >> ^
>> >> >>
>> >> >> Use a regular function to be more portable. This aligns also with
>> >> >> the other smc call implementations e.g. in qcom_scm-32.c and
>> >> >> bcm_kona_smc.c.
>> >> >>
>> >> >> Cc: Dmitry Osipenko
>> >> >> Cc: Stephen Warren
>> >> >> Cc: Thierry Reding
>> >> >> Signed-off-by: Stefan Agner
>> >> >> ---
>> >> >> Changes in v2:
>> >> >> - Keep stmfd/ldmfd to avoid potential ABI issues
>> >> >>
>> >> >>arch/arm/firmware/trusted_foundations.c | 14 +-
>> >> >>1 file changed, 9 insertions(+), 5 deletions(-)
>> >> >>
>> >> >> diff --git a/arch/arm/firmware/trusted_foundations.c
>> >> >> b/arch/arm/firmware/trusted_foundations.c
>> >> >> index 3fb1b5a1dce9..689e6565abfc 100644
>> >> >> --- a/arch/arm/firmware/trusted_foundations.c
>> >> >> +++ b/arch/arm/firmware/trusted_foundations.c
>> >> >> @@ -31,21 +31,25 @@
>> >> >> static unsigned long cpu_boot_addr;
>> >> >>-static void __naked tf_generic_smc(u32 type, u32 arg1, u32
>> >> >> arg2)
>> >> >> +static void tf_generic_smc(u32 type, u32 arg1, u32 arg2)
>> >> >>{
>> >> >> +register u32 r0 asm("r0") = type;
>> >> >> +register u32 r1 asm("r1") = arg1;
>> >> >> +register u32 r2 asm("r2") = arg2;
>> >> >> +
>> >> >>asm volatile(
>> >> >>".arch_extensionsec\n\t"
>> >> >> -"stmfdsp!, {r4 - r11, lr}\n\t"
>> >> >> +"stmfdsp!, {r4 - r11}\n\t"
>> >> >>__asmeq("%0", "r0")
>> >> >>__asmeq("%1", "r1")
>> >> >>__asmeq("%2", "r2")
>> >> >>"movr3, #0\n\t"
>> >> >>"movr4, #0\n\t"
>> >> >>"smc#0\n\t"
>> >> >> -"ldmfdsp!, {r4 - r11, pc}"
>> >> >> +"ldmfdsp!, {r4 - r11}\n\t"
>> >> >>:
>> >> >> -: "r" (type), "r" (arg1), "r" (arg2)
>> >> >> -: "memory");
>> >> >> +: "r" (r0), "r" (r1), "r" (r2)
>> >> >> +: "memory", "r3", "r12", "lr");
>> >> >
>> >> > Although seems "lr" won't be affected by SMC invocation because it
>> >> > should be
>> >> > banked and hence could be omitted entirely from the code. Maybe
>> >> > somebody could
>> >> > confirm this.
>> >> Strictly per the letter of the architecture, the SMC could be
>> >> trapped to Hyp
>> >> mode, and a hypervisor might clobber LR_usr in the process of
>> >> forwarding the
>> >> call to the firmware secure monitor (since Hyp doesn't have a banked
>> >> LR of its
>> >> own). Admittedly there are probably no real systems with the
>> >> appropriate
>> >> hardware/software combination to hit that, but on the other hand if
>> >> this gets
>> >> inlined where the compiler has already created a stack frame then an
>> >> LR clobber
>> >> is essentially free, so I reckon we're better off keeping it for
>> >> reassurance.
>> >> This isn't exactly a critical fast path anyway.
>> >> >>>
>> >> >>> Okay, thank you for the clarification.
>> >> >>
>> >> >> So it seems this change is fine?
>> >> >>
>> >> >> Stephen, you picked up changes for this driver before, is this patch
>> >> >> going through your tree?
>> >> >
>> >> > You had best ask Thierry; he's taken over Tegra maintenance upstream.
>> >> > But that said, don't files in arch/arm go through Russell?
>> >>
>> >> I think the last patches applied to that file went through your tree.
>> >>
>> >> Thierry, Russel, any preferences?
>> >
>> > I don't mind picking this up into the Tegra tree. Might
[PATCH 2/3] ARM: dts: imx6sll-evk: correct lcd regulator GPIO pin
On i.MX6SLL EVK board, lcd regulator is controlled
by GPIO4 IO03 using MX6SLL_PAD_KEY_ROW5__GPIO4_IO03 pin,
NOT MX6SLL_PAD_ECSPI1_SCLK__GPIO4_IO08, correct it.
Signed-off-by: Anson Huang
---
arch/arm/boot/dts/imx6sll-evk.dts | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/imx6sll-evk.dts
b/arch/arm/boot/dts/imx6sll-evk.dts
index cc5cf5e..41e87e6 100644
--- a/arch/arm/boot/dts/imx6sll-evk.dts
+++ b/arch/arm/boot/dts/imx6sll-evk.dts
@@ -74,7 +74,7 @@
pinctrl-names = "default";
pinctrl-0 = <_reg_lcd>;
regulator-name = "lcd-pwr";
- gpio = < 8 GPIO_ACTIVE_HIGH>;
+ gpio = < 3 GPIO_ACTIVE_HIGH>;
enable-active-high;
};
@@ -289,7 +289,7 @@
pinctrl_reg_lcd: reglcdgrp {
fsl,pins = <
- MX6SLL_PAD_ECSPI1_SCLK__GPIO4_IO08 0x17059
+ MX6SLL_PAD_KEY_ROW5__GPIO4_IO03 0x17059
>;
};
--
2.7.4
[PATCH 1/3] ARM: dts: imx6sll-evk: enable PWM1 for backlight driver
Enable pwm1 module on i.MX6SLL EVK board to make
backlight driver really work with LCD panel connected.
Signed-off-by: Anson Huang
---
arch/arm/boot/dts/imx6sll-evk.dts | 12
1 file changed, 12 insertions(+)
diff --git a/arch/arm/boot/dts/imx6sll-evk.dts
b/arch/arm/boot/dts/imx6sll-evk.dts
index bc8d155..cc5cf5e 100644
--- a/arch/arm/boot/dts/imx6sll-evk.dts
+++ b/arch/arm/boot/dts/imx6sll-evk.dts
@@ -213,6 +213,12 @@
};
};
+ {
+ pinctrl-names = "default";
+ pinctrl-0 = <_pwm1>;
+ status = "okay";
+};
+
{
pinctrl-names = "default";
pinctrl-0 = <_uart1>;
@@ -381,4 +387,10 @@
MX6SLL_PAD_I2C1_SDA__I2C1_SDA0x4001b8b1
>;
};
+
+ pinctrl_pwm1: pmw1grp {
+ fsl,pins = <
+ MX6SLL_PAD_PWM1__PWM1_OUT 0x110b0
+ >;
+ };
};
--
2.7.4
[PATCH 3/3] ARM: dts: imx6sll-evk: enable SEIKO 43WVF1G lcdif panel
Enable SEIKO 43WVF1G lcdif panel for DRM driver,
add necessary properties according to SEIKO 43WVF1G
driver's requirement, such as "dvdd-supply", "avdd-supply"
and "backlight" etc..
Signed-off-by: Anson Huang
---
arch/arm/boot/dts/imx6sll-evk.dts | 76 ---
1 file changed, 71 insertions(+), 5 deletions(-)
diff --git a/arch/arm/boot/dts/imx6sll-evk.dts
b/arch/arm/boot/dts/imx6sll-evk.dts
index 41e87e6..dc34da5 100644
--- a/arch/arm/boot/dts/imx6sll-evk.dts
+++ b/arch/arm/boot/dts/imx6sll-evk.dts
@@ -23,7 +23,7 @@
reg = <0x8000 0x8000>;
};
- backlight {
+ backlight_display: backlight-display {
compatible = "pwm-backlight";
pwms = < 0 500>;
brightness-levels = <0 4 8 16 32 64 128 255>;
@@ -69,15 +69,22 @@
regulator-boot-on;
};
- reg_lcd: regulator-lcd {
+ reg_lcd_3v3: regulator-lcd-3v3 {
compatible = "regulator-fixed";
pinctrl-names = "default";
- pinctrl-0 = <_reg_lcd>;
- regulator-name = "lcd-pwr";
+ pinctrl-0 = <_reg_lcd_3v3>;
+ regulator-name = "lcd-3v3";
gpio = < 3 GPIO_ACTIVE_HIGH>;
enable-active-high;
};
+ reg_lcd_5v: regulator-lcd-5v {
+ compatible = "regulator-fixed";
+ regulator-name = "lcd-5v0";
+ regulator-min-microvolt = <500>;
+ regulator-max-microvolt = <500>;
+ };
+
reg_sd1_vmmc: regulator-sd1-vmmc {
compatible = "regulator-fixed";
pinctrl-names = "default";
@@ -99,6 +106,19 @@
gpio = < 4 GPIO_ACTIVE_HIGH>;
enable-active-high;
};
+
+ panel {
+ compatible = "sii,43wvf1g";
+ backlight = <_display>;
+ dvdd-supply = <_lcd_3v3>;
+ avdd-supply = <_lcd_5v>;
+
+ port {
+ panel_in: endpoint {
+ remote-endpoint = <_out>;
+ };
+ };
+ };
};
{
@@ -213,6 +233,18 @@
};
};
+ {
+ pinctrl-names = "default";
+ pinctrl-0 = <_lcd>;
+ status = "okay";
+
+ port {
+ display_out: endpoint {
+ remote-endpoint = <_in>;
+ };
+ };
+};
+
{
pinctrl-names = "default";
pinctrl-0 = <_pwm1>;
@@ -287,7 +319,7 @@
>;
};
- pinctrl_reg_lcd: reglcdgrp {
+ pinctrl_reg_lcd_3v3: reglcd3v3grp {
fsl,pins = <
MX6SLL_PAD_KEY_ROW5__GPIO4_IO03 0x17059
>;
@@ -388,6 +420,40 @@
>;
};
+ pinctrl_lcd: lcdgrp {
+ fsl,pins = <
+ MX6SLL_PAD_LCD_DATA00__LCD_DATA00 0x79
+ MX6SLL_PAD_LCD_DATA01__LCD_DATA01 0x79
+ MX6SLL_PAD_LCD_DATA02__LCD_DATA02 0x79
+ MX6SLL_PAD_LCD_DATA03__LCD_DATA03 0x79
+ MX6SLL_PAD_LCD_DATA04__LCD_DATA04 0x79
+ MX6SLL_PAD_LCD_DATA05__LCD_DATA05 0x79
+ MX6SLL_PAD_LCD_DATA06__LCD_DATA06 0x79
+ MX6SLL_PAD_LCD_DATA07__LCD_DATA07 0x79
+ MX6SLL_PAD_LCD_DATA08__LCD_DATA08 0x79
+ MX6SLL_PAD_LCD_DATA09__LCD_DATA09 0x79
+ MX6SLL_PAD_LCD_DATA10__LCD_DATA10 0x79
+ MX6SLL_PAD_LCD_DATA11__LCD_DATA11 0x79
+ MX6SLL_PAD_LCD_DATA12__LCD_DATA12 0x79
+ MX6SLL_PAD_LCD_DATA13__LCD_DATA13 0x79
+ MX6SLL_PAD_LCD_DATA14__LCD_DATA14 0x79
+ MX6SLL_PAD_LCD_DATA15__LCD_DATA15 0x79
+ MX6SLL_PAD_LCD_DATA16__LCD_DATA16 0x79
+ MX6SLL_PAD_LCD_DATA17__LCD_DATA17 0x79
+ MX6SLL_PAD_LCD_DATA18__LCD_DATA18 0x79
+ MX6SLL_PAD_LCD_DATA19__LCD_DATA19 0x79
+ MX6SLL_PAD_LCD_DATA20__LCD_DATA20 0x79
+ MX6SLL_PAD_LCD_DATA21__LCD_DATA21 0x79
+ MX6SLL_PAD_LCD_DATA22__LCD_DATA22 0x79
+ MX6SLL_PAD_LCD_DATA23__LCD_DATA23 0x79
+ MX6SLL_PAD_LCD_CLK__LCD_CLK 0x79
+ MX6SLL_PAD_LCD_ENABLE__LCD_ENABLE 0x79
+ MX6SLL_PAD_LCD_HSYNC__LCD_HSYNC 0x79
+ MX6SLL_PAD_LCD_VSYNC__LCD_VSYNC 0x79
+ MX6SLL_PAD_LCD_RESET__LCD_RESET 0x79
+ >;
+ };
+
pinctrl_pwm1: pmw1grp {
fsl,pins = <
[PATCH 2/3] ARM: dts: imx6sll-evk: correct lcd regulator GPIO pin
On i.MX6SLL EVK board, lcd regulator is controlled
by GPIO4 IO03 using MX6SLL_PAD_KEY_ROW5__GPIO4_IO03 pin,
NOT MX6SLL_PAD_ECSPI1_SCLK__GPIO4_IO08, correct it.
Signed-off-by: Anson Huang
---
arch/arm/boot/dts/imx6sll-evk.dts | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/imx6sll-evk.dts
b/arch/arm/boot/dts/imx6sll-evk.dts
index cc5cf5e..41e87e6 100644
--- a/arch/arm/boot/dts/imx6sll-evk.dts
+++ b/arch/arm/boot/dts/imx6sll-evk.dts
@@ -74,7 +74,7 @@
pinctrl-names = "default";
pinctrl-0 = <_reg_lcd>;
regulator-name = "lcd-pwr";
- gpio = < 8 GPIO_ACTIVE_HIGH>;
+ gpio = < 3 GPIO_ACTIVE_HIGH>;
enable-active-high;
};
@@ -289,7 +289,7 @@
pinctrl_reg_lcd: reglcdgrp {
fsl,pins = <
- MX6SLL_PAD_ECSPI1_SCLK__GPIO4_IO08 0x17059
+ MX6SLL_PAD_KEY_ROW5__GPIO4_IO03 0x17059
>;
};
--
2.7.4
[PATCH 1/3] ARM: dts: imx6sll-evk: enable PWM1 for backlight driver
Enable pwm1 module on i.MX6SLL EVK board to make
backlight driver really work with LCD panel connected.
Signed-off-by: Anson Huang
---
arch/arm/boot/dts/imx6sll-evk.dts | 12
1 file changed, 12 insertions(+)
diff --git a/arch/arm/boot/dts/imx6sll-evk.dts
b/arch/arm/boot/dts/imx6sll-evk.dts
index bc8d155..cc5cf5e 100644
--- a/arch/arm/boot/dts/imx6sll-evk.dts
+++ b/arch/arm/boot/dts/imx6sll-evk.dts
@@ -213,6 +213,12 @@
};
};
+ {
+ pinctrl-names = "default";
+ pinctrl-0 = <_pwm1>;
+ status = "okay";
+};
+
{
pinctrl-names = "default";
pinctrl-0 = <_uart1>;
@@ -381,4 +387,10 @@
MX6SLL_PAD_I2C1_SDA__I2C1_SDA0x4001b8b1
>;
};
+
+ pinctrl_pwm1: pmw1grp {
+ fsl,pins = <
+ MX6SLL_PAD_PWM1__PWM1_OUT 0x110b0
+ >;
+ };
};
--
2.7.4
[PATCH 3/3] ARM: dts: imx6sll-evk: enable SEIKO 43WVF1G lcdif panel
Enable SEIKO 43WVF1G lcdif panel for DRM driver,
add necessary properties according to SEIKO 43WVF1G
driver's requirement, such as "dvdd-supply", "avdd-supply"
and "backlight" etc..
Signed-off-by: Anson Huang
---
arch/arm/boot/dts/imx6sll-evk.dts | 76 ---
1 file changed, 71 insertions(+), 5 deletions(-)
diff --git a/arch/arm/boot/dts/imx6sll-evk.dts
b/arch/arm/boot/dts/imx6sll-evk.dts
index 41e87e6..dc34da5 100644
--- a/arch/arm/boot/dts/imx6sll-evk.dts
+++ b/arch/arm/boot/dts/imx6sll-evk.dts
@@ -23,7 +23,7 @@
reg = <0x8000 0x8000>;
};
- backlight {
+ backlight_display: backlight-display {
compatible = "pwm-backlight";
pwms = < 0 500>;
brightness-levels = <0 4 8 16 32 64 128 255>;
@@ -69,15 +69,22 @@
regulator-boot-on;
};
- reg_lcd: regulator-lcd {
+ reg_lcd_3v3: regulator-lcd-3v3 {
compatible = "regulator-fixed";
pinctrl-names = "default";
- pinctrl-0 = <_reg_lcd>;
- regulator-name = "lcd-pwr";
+ pinctrl-0 = <_reg_lcd_3v3>;
+ regulator-name = "lcd-3v3";
gpio = < 3 GPIO_ACTIVE_HIGH>;
enable-active-high;
};
+ reg_lcd_5v: regulator-lcd-5v {
+ compatible = "regulator-fixed";
+ regulator-name = "lcd-5v0";
+ regulator-min-microvolt = <500>;
+ regulator-max-microvolt = <500>;
+ };
+
reg_sd1_vmmc: regulator-sd1-vmmc {
compatible = "regulator-fixed";
pinctrl-names = "default";
@@ -99,6 +106,19 @@
gpio = < 4 GPIO_ACTIVE_HIGH>;
enable-active-high;
};
+
+ panel {
+ compatible = "sii,43wvf1g";
+ backlight = <_display>;
+ dvdd-supply = <_lcd_3v3>;
+ avdd-supply = <_lcd_5v>;
+
+ port {
+ panel_in: endpoint {
+ remote-endpoint = <_out>;
+ };
+ };
+ };
};
{
@@ -213,6 +233,18 @@
};
};
+ {
+ pinctrl-names = "default";
+ pinctrl-0 = <_lcd>;
+ status = "okay";
+
+ port {
+ display_out: endpoint {
+ remote-endpoint = <_in>;
+ };
+ };
+};
+
{
pinctrl-names = "default";
pinctrl-0 = <_pwm1>;
@@ -287,7 +319,7 @@
>;
};
- pinctrl_reg_lcd: reglcdgrp {
+ pinctrl_reg_lcd_3v3: reglcd3v3grp {
fsl,pins = <
MX6SLL_PAD_KEY_ROW5__GPIO4_IO03 0x17059
>;
@@ -388,6 +420,40 @@
>;
};
+ pinctrl_lcd: lcdgrp {
+ fsl,pins = <
+ MX6SLL_PAD_LCD_DATA00__LCD_DATA00 0x79
+ MX6SLL_PAD_LCD_DATA01__LCD_DATA01 0x79
+ MX6SLL_PAD_LCD_DATA02__LCD_DATA02 0x79
+ MX6SLL_PAD_LCD_DATA03__LCD_DATA03 0x79
+ MX6SLL_PAD_LCD_DATA04__LCD_DATA04 0x79
+ MX6SLL_PAD_LCD_DATA05__LCD_DATA05 0x79
+ MX6SLL_PAD_LCD_DATA06__LCD_DATA06 0x79
+ MX6SLL_PAD_LCD_DATA07__LCD_DATA07 0x79
+ MX6SLL_PAD_LCD_DATA08__LCD_DATA08 0x79
+ MX6SLL_PAD_LCD_DATA09__LCD_DATA09 0x79
+ MX6SLL_PAD_LCD_DATA10__LCD_DATA10 0x79
+ MX6SLL_PAD_LCD_DATA11__LCD_DATA11 0x79
+ MX6SLL_PAD_LCD_DATA12__LCD_DATA12 0x79
+ MX6SLL_PAD_LCD_DATA13__LCD_DATA13 0x79
+ MX6SLL_PAD_LCD_DATA14__LCD_DATA14 0x79
+ MX6SLL_PAD_LCD_DATA15__LCD_DATA15 0x79
+ MX6SLL_PAD_LCD_DATA16__LCD_DATA16 0x79
+ MX6SLL_PAD_LCD_DATA17__LCD_DATA17 0x79
+ MX6SLL_PAD_LCD_DATA18__LCD_DATA18 0x79
+ MX6SLL_PAD_LCD_DATA19__LCD_DATA19 0x79
+ MX6SLL_PAD_LCD_DATA20__LCD_DATA20 0x79
+ MX6SLL_PAD_LCD_DATA21__LCD_DATA21 0x79
+ MX6SLL_PAD_LCD_DATA22__LCD_DATA22 0x79
+ MX6SLL_PAD_LCD_DATA23__LCD_DATA23 0x79
+ MX6SLL_PAD_LCD_CLK__LCD_CLK 0x79
+ MX6SLL_PAD_LCD_ENABLE__LCD_ENABLE 0x79
+ MX6SLL_PAD_LCD_HSYNC__LCD_HSYNC 0x79
+ MX6SLL_PAD_LCD_VSYNC__LCD_VSYNC 0x79
+ MX6SLL_PAD_LCD_RESET__LCD_RESET 0x79
+ >;
+ };
+
pinctrl_pwm1: pmw1grp {
fsl,pins = <
Re: [PATCH 1/3] tty/serial_core: add ISO7816 infrastructure
On Thu, Jul 12, 2018 at 04:58:08PM +0200, Greg KH wrote:
> On Wed, Jul 11, 2018 at 03:16:36PM +0200, Ludovic Desroches wrote:
> > From: Nicolas Ferre
> >
> > Add the ISO7816 ioctl and associated accessors and data structure.
> > Drivers can then use this common implementation to handle ISO7816.
> >
> > Signed-off-by: Nicolas Ferre
> > [ludovic.desroc...@microchip.com: squash and rebase, removal of gpios,
> > checkpatch fixes]
> > Signed-off-by: Ludovic Desroches
> > ---
> > drivers/tty/serial/serial_core.c | 49
> > +++
> > include/linux/serial_core.h | 3 +++
> > include/uapi/asm-generic/ioctls.h | 2 ++
> > include/uapi/linux/serial.h | 17 ++
> > 4 files changed, 71 insertions(+)
>
> Note, kbuild found build issues with this, please fix up.
>
> Also, here's some comments:
>
> >
> > diff --git a/drivers/tty/serial/serial_core.c
> > b/drivers/tty/serial/serial_core.c
> > index 9c14a453f73c..c89ac59f6f8c 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -1301,6 +1301,47 @@ static int uart_set_rs485_config(struct uart_port
> > *port,
> > return 0;
> > }
> >
> > +static int uart_get_iso7816_config(struct uart_port *port,
> > + struct serial_iso7816 __user *iso7816)
> > +{
> > + unsigned long flags;
> > + struct serial_iso7816 aux;
> > +
> > + spin_lock_irqsave(>lock, flags);
> > + aux = port->iso7816;
>
> Don't you want to check to see if there is a function for iso7816 before
> copying it? Otherwise it's just empty, right?
I'll add the check.
>
> > + if (!port->iso7816_config)
> > + return -ENOIOCTLCMD;
>
> Why this error value?
>
It was a mimic of RS485.
> > --- a/include/uapi/asm-generic/ioctls.h
> > +++ b/include/uapi/asm-generic/ioctls.h
> > @@ -66,6 +66,8 @@
> > #ifndef TIOCSRS485
> > #define TIOCSRS485 0x542F
> > #endif
> > +#define TIOCGISO7816 0x5430
> > +#define TIOCSISO7816 0x5431
>
> Where did you get these numbers from?
I will use the macros to create numbers. Any rules about nr choice?
Regards
Ludovic
>
> thanks,
>
> greg k-h
Re: [PATCH 1/3] tty/serial_core: add ISO7816 infrastructure
On Thu, Jul 12, 2018 at 04:58:08PM +0200, Greg KH wrote:
> On Wed, Jul 11, 2018 at 03:16:36PM +0200, Ludovic Desroches wrote:
> > From: Nicolas Ferre
> >
> > Add the ISO7816 ioctl and associated accessors and data structure.
> > Drivers can then use this common implementation to handle ISO7816.
> >
> > Signed-off-by: Nicolas Ferre
> > [ludovic.desroc...@microchip.com: squash and rebase, removal of gpios,
> > checkpatch fixes]
> > Signed-off-by: Ludovic Desroches
> > ---
> > drivers/tty/serial/serial_core.c | 49
> > +++
> > include/linux/serial_core.h | 3 +++
> > include/uapi/asm-generic/ioctls.h | 2 ++
> > include/uapi/linux/serial.h | 17 ++
> > 4 files changed, 71 insertions(+)
>
> Note, kbuild found build issues with this, please fix up.
>
> Also, here's some comments:
>
> >
> > diff --git a/drivers/tty/serial/serial_core.c
> > b/drivers/tty/serial/serial_core.c
> > index 9c14a453f73c..c89ac59f6f8c 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -1301,6 +1301,47 @@ static int uart_set_rs485_config(struct uart_port
> > *port,
> > return 0;
> > }
> >
> > +static int uart_get_iso7816_config(struct uart_port *port,
> > + struct serial_iso7816 __user *iso7816)
> > +{
> > + unsigned long flags;
> > + struct serial_iso7816 aux;
> > +
> > + spin_lock_irqsave(>lock, flags);
> > + aux = port->iso7816;
>
> Don't you want to check to see if there is a function for iso7816 before
> copying it? Otherwise it's just empty, right?
I'll add the check.
>
> > + if (!port->iso7816_config)
> > + return -ENOIOCTLCMD;
>
> Why this error value?
>
It was a mimic of RS485.
> > --- a/include/uapi/asm-generic/ioctls.h
> > +++ b/include/uapi/asm-generic/ioctls.h
> > @@ -66,6 +66,8 @@
> > #ifndef TIOCSRS485
> > #define TIOCSRS485 0x542F
> > #endif
> > +#define TIOCGISO7816 0x5430
> > +#define TIOCSISO7816 0x5431
>
> Where did you get these numbers from?
I will use the macros to create numbers. Any rules about nr choice?
Regards
Ludovic
>
> thanks,
>
> greg k-h
[PATCH] staging/rtl8192u: hide unused procfs helpers
When CONFIG_PROC_FS isn't set, gcc warning this:
drivers/staging/rtl8192u/r8192U_core.c:508:12: warning: ‘proc_get_stats_ap’
defined but not used [-Wunused-function]
static int proc_get_stats_ap(struct seq_file *m, void *v)
^
drivers/staging/rtl8192u/r8192U_core.c:527:12: warning: ‘proc_get_registers’
defined but not used [-Wunused-function]
static int proc_get_registers(struct seq_file *m, void *v)
^
drivers/staging/rtl8192u/r8192U_core.c:568:12: warning: ‘proc_get_stats_tx’
defined but not used [-Wunused-function]
static int proc_get_stats_tx(struct seq_file *m, void *v)
^
drivers/staging/rtl8192u/r8192U_core.c:627:12: warning: ‘proc_get_stats_rx’
defined but not used [-Wunused-function]
static int proc_get_stats_rx(struct seq_file *m, void *v)
^
fix this by adding #ifdef around them.
Signed-off-by: YueHaibing
---
drivers/staging/rtl8192u/r8192U_core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/staging/rtl8192u/r8192U_core.c
b/drivers/staging/rtl8192u/r8192U_core.c
index 8b17400..b9724d9 100644
--- a/drivers/staging/rtl8192u/r8192U_core.c
+++ b/drivers/staging/rtl8192u/r8192U_core.c
@@ -505,6 +505,7 @@ static void watch_dog_timer_callback(struct timer_list *t);
static struct proc_dir_entry *rtl8192_proc;
+#ifdef CONFIG_PROC_FS
static int proc_get_stats_ap(struct seq_file *m, void *v)
{
struct net_device *dev = m->private;
@@ -639,6 +640,7 @@ static int proc_get_stats_rx(struct seq_file *m, void *v)
return 0;
}
+#endif
static void rtl8192_proc_module_init(void)
{
--
2.7.0
[PATCH] staging/rtl8192u: hide unused procfs helpers
When CONFIG_PROC_FS isn't set, gcc warning this:
drivers/staging/rtl8192u/r8192U_core.c:508:12: warning: ‘proc_get_stats_ap’
defined but not used [-Wunused-function]
static int proc_get_stats_ap(struct seq_file *m, void *v)
^
drivers/staging/rtl8192u/r8192U_core.c:527:12: warning: ‘proc_get_registers’
defined but not used [-Wunused-function]
static int proc_get_registers(struct seq_file *m, void *v)
^
drivers/staging/rtl8192u/r8192U_core.c:568:12: warning: ‘proc_get_stats_tx’
defined but not used [-Wunused-function]
static int proc_get_stats_tx(struct seq_file *m, void *v)
^
drivers/staging/rtl8192u/r8192U_core.c:627:12: warning: ‘proc_get_stats_rx’
defined but not used [-Wunused-function]
static int proc_get_stats_rx(struct seq_file *m, void *v)
^
fix this by adding #ifdef around them.
Signed-off-by: YueHaibing
---
drivers/staging/rtl8192u/r8192U_core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/staging/rtl8192u/r8192U_core.c
b/drivers/staging/rtl8192u/r8192U_core.c
index 8b17400..b9724d9 100644
--- a/drivers/staging/rtl8192u/r8192U_core.c
+++ b/drivers/staging/rtl8192u/r8192U_core.c
@@ -505,6 +505,7 @@ static void watch_dog_timer_callback(struct timer_list *t);
static struct proc_dir_entry *rtl8192_proc;
+#ifdef CONFIG_PROC_FS
static int proc_get_stats_ap(struct seq_file *m, void *v)
{
struct net_device *dev = m->private;
@@ -639,6 +640,7 @@ static int proc_get_stats_rx(struct seq_file *m, void *v)
return 0;
}
+#endif
static void rtl8192_proc_module_init(void)
{
--
2.7.0
[PATCH] ARM: dts: imx6sl-evk: add missing GPIO iomux setting
On i.MX6SL EVK board, the MX6SL_PAD_KEY_ROW5 pin is
used as lcd 3v3 regulator control pin, need to make
sure MX6SL_PAD_KEY_ROW5 is muxed as GPIO function
for controlling lcd 3v3 regulator.
Signed-off-by: Anson Huang
---
arch/arm/boot/dts/imx6sl-evk.dts | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/imx6sl-evk.dts b/arch/arm/boot/dts/imx6sl-evk.dts
index 50bdc65..1294fba 100644
--- a/arch/arm/boot/dts/imx6sl-evk.dts
+++ b/arch/arm/boot/dts/imx6sl-evk.dts
@@ -283,6 +283,7 @@
imx6sl-evk {
pinctrl_hog: hoggrp {
fsl,pins = <
+ MX6SL_PAD_KEY_ROW5__GPIO4_IO030x17059
MX6SL_PAD_KEY_ROW7__GPIO4_IO070x17059
MX6SL_PAD_KEY_COL7__GPIO4_IO060x17059
MX6SL_PAD_SD2_DAT7__GPIO5_IO000x17059
--
2.7.4
Re: [Cocci] [PATCH 01/18] coccinelle: change strncpy+truncation to strlcpy
Himanshu Jha wrote on Fri, Jul 13, 2018: > > I expect each maintainer will pick their share of the patchs if they > > agree with it and the rest will just be dropped? > > Masahiro Yamada takes coccinelle patches, > so please cc him or your patch would be lost. Thanks, will do. > > +virtual patch > > +virtual context > > You might consider adding context rule or remove this line perhaps ? Victim of copypasta, I'll remove this. > > +-strncpy@p( > > ++strlcpy( > > + dest, src, sz); > > +-dest[sz - 1] = '\0'; > > The above rule produces an output that I think is not correct: > -- > diff = > diff -u -p a//ti/wl1251/acx.c b//ti/wl1251/acx.c > --- a//ti/wl1251/acx.c > +++ b//ti/wl1251/acx.c > @@ -150,14 +150,7 @@ int wl1251_acx_fw_version(struct wl1251 > } > > /* be careful with the buffer sizes */ > - strncpy(buf, rev->fw_version, min(len, sizeof(rev->fw_version))); > - > - /* > - * if the firmware version string is exactly > - * sizeof(rev->fw_version) long or fw_len is less than > - * sizeof(rev->fw_version) it won't be null terminated > - */ > - buf[min(len, sizeof(rev->fw_version)) - 1] = '\0'; > + strlcpy(buf, rev->fw_version, min(len, sizeof(rev->fw_version))); > > - > > I think the comment is useful and should not be removed. I agree this comment is useful now that I'm taking a closer look, I glanced at this too fast. I'm not sure how to make coccinelle not remove comments between lines though? > Also, consider changing Confidence level appropriately. I am (was?) pretty confident on the change itself, the only exceptions would be if someone relied on strncpy to fill the end of the buffer with zero to not leak data somewhere but that is not easy to judge by itself (although I hope rare enough) I'm honestly not sure what would be appropriate in this case. -- Dominique Martinet
[PATCH] ARM: dts: imx6sl-evk: add missing GPIO iomux setting
On i.MX6SL EVK board, the MX6SL_PAD_KEY_ROW5 pin is
used as lcd 3v3 regulator control pin, need to make
sure MX6SL_PAD_KEY_ROW5 is muxed as GPIO function
for controlling lcd 3v3 regulator.
Signed-off-by: Anson Huang
---
arch/arm/boot/dts/imx6sl-evk.dts | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/imx6sl-evk.dts b/arch/arm/boot/dts/imx6sl-evk.dts
index 50bdc65..1294fba 100644
--- a/arch/arm/boot/dts/imx6sl-evk.dts
+++ b/arch/arm/boot/dts/imx6sl-evk.dts
@@ -283,6 +283,7 @@
imx6sl-evk {
pinctrl_hog: hoggrp {
fsl,pins = <
+ MX6SL_PAD_KEY_ROW5__GPIO4_IO030x17059
MX6SL_PAD_KEY_ROW7__GPIO4_IO070x17059
MX6SL_PAD_KEY_COL7__GPIO4_IO060x17059
MX6SL_PAD_SD2_DAT7__GPIO5_IO000x17059
--
2.7.4
Re: [Cocci] [PATCH 01/18] coccinelle: change strncpy+truncation to strlcpy
Himanshu Jha wrote on Fri, Jul 13, 2018: > > I expect each maintainer will pick their share of the patchs if they > > agree with it and the rest will just be dropped? > > Masahiro Yamada takes coccinelle patches, > so please cc him or your patch would be lost. Thanks, will do. > > +virtual patch > > +virtual context > > You might consider adding context rule or remove this line perhaps ? Victim of copypasta, I'll remove this. > > +-strncpy@p( > > ++strlcpy( > > + dest, src, sz); > > +-dest[sz - 1] = '\0'; > > The above rule produces an output that I think is not correct: > -- > diff = > diff -u -p a//ti/wl1251/acx.c b//ti/wl1251/acx.c > --- a//ti/wl1251/acx.c > +++ b//ti/wl1251/acx.c > @@ -150,14 +150,7 @@ int wl1251_acx_fw_version(struct wl1251 > } > > /* be careful with the buffer sizes */ > - strncpy(buf, rev->fw_version, min(len, sizeof(rev->fw_version))); > - > - /* > - * if the firmware version string is exactly > - * sizeof(rev->fw_version) long or fw_len is less than > - * sizeof(rev->fw_version) it won't be null terminated > - */ > - buf[min(len, sizeof(rev->fw_version)) - 1] = '\0'; > + strlcpy(buf, rev->fw_version, min(len, sizeof(rev->fw_version))); > > - > > I think the comment is useful and should not be removed. I agree this comment is useful now that I'm taking a closer look, I glanced at this too fast. I'm not sure how to make coccinelle not remove comments between lines though? > Also, consider changing Confidence level appropriately. I am (was?) pretty confident on the change itself, the only exceptions would be if someone relied on strncpy to fill the end of the buffer with zero to not leak data somewhere but that is not easy to judge by itself (although I hope rare enough) I'm honestly not sure what would be appropriate in this case. -- Dominique Martinet
Re: [PATCH 1/2] dt-bindings: pfuze100: add optional pfuze-disable-sw binding
Hi Fabio, On 18-07-12 12:19, Fabio Estevam wrote: > On Thu, Jul 12, 2018 at 8:02 AM, Marco Felsch wrote: > > > +Optional properties: > > +- pfuze-disable-sw: Disable all unused switch regulators to save power > > + consumption. Attention, some platforms are using the switch regulators > > as DDR > > + ref or supply voltage. Mark these regulators as "regulator-always-on" to > > skip > > + disabling these regulators. If not specified, the driver simualtes the > > s/simualtes/simulates/ > > Reviewed-by: Fabio Estevam Thanks for the feedback, I'll fix it in v2. -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 1/2] dt-bindings: pfuze100: add optional pfuze-disable-sw binding
Hi Fabio, On 18-07-12 12:19, Fabio Estevam wrote: > On Thu, Jul 12, 2018 at 8:02 AM, Marco Felsch wrote: > > > +Optional properties: > > +- pfuze-disable-sw: Disable all unused switch regulators to save power > > + consumption. Attention, some platforms are using the switch regulators > > as DDR > > + ref or supply voltage. Mark these regulators as "regulator-always-on" to > > skip > > + disabling these regulators. If not specified, the driver simualtes the > > s/simualtes/simulates/ > > Reviewed-by: Fabio Estevam Thanks for the feedback, I'll fix it in v2. -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 1/3] tty/serial_core: add ISO7816 infrastructure
On Thu, Jul 12, 2018 at 05:02:29PM +0200, Greg KH wrote:
> On Wed, Jul 11, 2018 at 03:16:36PM +0200, Ludovic Desroches wrote:
> > From: Nicolas Ferre
> >
> > Add the ISO7816 ioctl and associated accessors and data structure.
> > Drivers can then use this common implementation to handle ISO7816.
> >
> > Signed-off-by: Nicolas Ferre
> > [ludovic.desroc...@microchip.com: squash and rebase, removal of gpios,
> > checkpatch fixes]
> > Signed-off-by: Ludovic Desroches
> > ---
> > drivers/tty/serial/serial_core.c | 49
> > +++
> > include/linux/serial_core.h | 3 +++
> > include/uapi/asm-generic/ioctls.h | 2 ++
> > include/uapi/linux/serial.h | 17 ++
> > 4 files changed, 71 insertions(+)
> >
> > diff --git a/drivers/tty/serial/serial_core.c
> > b/drivers/tty/serial/serial_core.c
> > index 9c14a453f73c..c89ac59f6f8c 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -1301,6 +1301,47 @@ static int uart_set_rs485_config(struct uart_port
> > *port,
> > return 0;
> > }
> >
> > +static int uart_get_iso7816_config(struct uart_port *port,
> > + struct serial_iso7816 __user *iso7816)
> > +{
> > + unsigned long flags;
> > + struct serial_iso7816 aux;
> > +
> > + spin_lock_irqsave(>lock, flags);
> > + aux = port->iso7816;
> > + spin_unlock_irqrestore(>lock, flags);
> > +
> > + if (copy_to_user(iso7816, , sizeof(aux)))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +
> > +static int uart_set_iso7816_config(struct uart_port *port,
> > + struct serial_iso7816 __user *iso7816_user)
> > +{
> > + struct serial_iso7816 iso7816;
> > + int ret;
> > + unsigned long flags;
> > +
> > + if (!port->iso7816_config)
> > + return -ENOIOCTLCMD;
> > +
> > + if (copy_from_user(, iso7816_user, sizeof(*iso7816_user)))
> > + return -EFAULT;
> > +
> > + spin_lock_irqsave(>lock, flags);
> > + ret = port->iso7816_config(port, );
> > + spin_unlock_irqrestore(>lock, flags);
> > + if (ret)
> > + return ret;
> > +
> > + if (copy_to_user(iso7816_user, >iso7816, sizeof(port->iso7816)))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +
> > /*
> > * Called via sys_ioctl. We can use spin_lock_irq() here.
> > */
> > @@ -1385,6 +1426,14 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd,
> > unsigned long arg)
> > case TIOCSRS485:
> > ret = uart_set_rs485_config(uport, uarg);
> > break;
> > +
> > + case TIOCSISO7816:
> > + ret = uart_set_iso7816_config(state->uart_port, uarg);
> > + break;
> > +
> > + case TIOCGISO7816:
> > + ret = uart_get_iso7816_config(state->uart_port, uarg);
> > + break;
> > default:
> > if (uport->ops->ioctl)
> > ret = uport->ops->ioctl(uport, cmd, arg);
> > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> > index 06ea4eeb09ab..d6e7747ffd46 100644
> > --- a/include/linux/serial_core.h
> > +++ b/include/linux/serial_core.h
> > @@ -137,6 +137,8 @@ struct uart_port {
> > void(*handle_break)(struct uart_port *);
> > int (*rs485_config)(struct uart_port *,
> > struct serial_rs485 *rs485);
> > + int (*iso7816_config)(struct uart_port *,
> > + struct serial_iso7816
> > *iso7816);
> > unsigned intirq;/* irq number */
> > unsigned long irqflags; /* irq flags */
> > unsigned intuartclk;/* base uart clock */
> > @@ -253,6 +255,7 @@ struct uart_port {
> > struct attribute_group *attr_group;/* port specific
> > attributes */
> > const struct attribute_group **tty_groups; /* all attributes
> > (serial core use only) */
> > struct serial_rs485 rs485;
> > + struct serial_iso7816 iso7816;
> > void*private_data; /* generic platform
> > data pointer */
> > };
> >
> > diff --git a/include/uapi/asm-generic/ioctls.h
> > b/include/uapi/asm-generic/ioctls.h
> > index 040651735662..0e5c79726c2d 100644
> > --- a/include/uapi/asm-generic/ioctls.h
> > +++ b/include/uapi/asm-generic/ioctls.h
> > @@ -66,6 +66,8 @@
> > #ifndef TIOCSRS485
> > #define TIOCSRS485 0x542F
> > #endif
> > +#define TIOCGISO7816 0x5430
> > +#define TIOCSISO7816 0x5431
> > #define TIOCGPTN _IOR('T', 0x30, unsigned int) /* Get Pty Number (of
> > pty-mux device) */
> > #define TIOCSPTLCK _IOW('T', 0x31, int) /* Lock/unlock Pty */
> > #define TIOCGDEV _IOR('T', 0x32, unsigned int) /* Get primary device
> > node of /dev/console */
> > diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> > index
Re: [PATCH 1/3] tty/serial_core: add ISO7816 infrastructure
On Thu, Jul 12, 2018 at 05:02:29PM +0200, Greg KH wrote:
> On Wed, Jul 11, 2018 at 03:16:36PM +0200, Ludovic Desroches wrote:
> > From: Nicolas Ferre
> >
> > Add the ISO7816 ioctl and associated accessors and data structure.
> > Drivers can then use this common implementation to handle ISO7816.
> >
> > Signed-off-by: Nicolas Ferre
> > [ludovic.desroc...@microchip.com: squash and rebase, removal of gpios,
> > checkpatch fixes]
> > Signed-off-by: Ludovic Desroches
> > ---
> > drivers/tty/serial/serial_core.c | 49
> > +++
> > include/linux/serial_core.h | 3 +++
> > include/uapi/asm-generic/ioctls.h | 2 ++
> > include/uapi/linux/serial.h | 17 ++
> > 4 files changed, 71 insertions(+)
> >
> > diff --git a/drivers/tty/serial/serial_core.c
> > b/drivers/tty/serial/serial_core.c
> > index 9c14a453f73c..c89ac59f6f8c 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -1301,6 +1301,47 @@ static int uart_set_rs485_config(struct uart_port
> > *port,
> > return 0;
> > }
> >
> > +static int uart_get_iso7816_config(struct uart_port *port,
> > + struct serial_iso7816 __user *iso7816)
> > +{
> > + unsigned long flags;
> > + struct serial_iso7816 aux;
> > +
> > + spin_lock_irqsave(>lock, flags);
> > + aux = port->iso7816;
> > + spin_unlock_irqrestore(>lock, flags);
> > +
> > + if (copy_to_user(iso7816, , sizeof(aux)))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +
> > +static int uart_set_iso7816_config(struct uart_port *port,
> > + struct serial_iso7816 __user *iso7816_user)
> > +{
> > + struct serial_iso7816 iso7816;
> > + int ret;
> > + unsigned long flags;
> > +
> > + if (!port->iso7816_config)
> > + return -ENOIOCTLCMD;
> > +
> > + if (copy_from_user(, iso7816_user, sizeof(*iso7816_user)))
> > + return -EFAULT;
> > +
> > + spin_lock_irqsave(>lock, flags);
> > + ret = port->iso7816_config(port, );
> > + spin_unlock_irqrestore(>lock, flags);
> > + if (ret)
> > + return ret;
> > +
> > + if (copy_to_user(iso7816_user, >iso7816, sizeof(port->iso7816)))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +
> > /*
> > * Called via sys_ioctl. We can use spin_lock_irq() here.
> > */
> > @@ -1385,6 +1426,14 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd,
> > unsigned long arg)
> > case TIOCSRS485:
> > ret = uart_set_rs485_config(uport, uarg);
> > break;
> > +
> > + case TIOCSISO7816:
> > + ret = uart_set_iso7816_config(state->uart_port, uarg);
> > + break;
> > +
> > + case TIOCGISO7816:
> > + ret = uart_get_iso7816_config(state->uart_port, uarg);
> > + break;
> > default:
> > if (uport->ops->ioctl)
> > ret = uport->ops->ioctl(uport, cmd, arg);
> > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> > index 06ea4eeb09ab..d6e7747ffd46 100644
> > --- a/include/linux/serial_core.h
> > +++ b/include/linux/serial_core.h
> > @@ -137,6 +137,8 @@ struct uart_port {
> > void(*handle_break)(struct uart_port *);
> > int (*rs485_config)(struct uart_port *,
> > struct serial_rs485 *rs485);
> > + int (*iso7816_config)(struct uart_port *,
> > + struct serial_iso7816
> > *iso7816);
> > unsigned intirq;/* irq number */
> > unsigned long irqflags; /* irq flags */
> > unsigned intuartclk;/* base uart clock */
> > @@ -253,6 +255,7 @@ struct uart_port {
> > struct attribute_group *attr_group;/* port specific
> > attributes */
> > const struct attribute_group **tty_groups; /* all attributes
> > (serial core use only) */
> > struct serial_rs485 rs485;
> > + struct serial_iso7816 iso7816;
> > void*private_data; /* generic platform
> > data pointer */
> > };
> >
> > diff --git a/include/uapi/asm-generic/ioctls.h
> > b/include/uapi/asm-generic/ioctls.h
> > index 040651735662..0e5c79726c2d 100644
> > --- a/include/uapi/asm-generic/ioctls.h
> > +++ b/include/uapi/asm-generic/ioctls.h
> > @@ -66,6 +66,8 @@
> > #ifndef TIOCSRS485
> > #define TIOCSRS485 0x542F
> > #endif
> > +#define TIOCGISO7816 0x5430
> > +#define TIOCSISO7816 0x5431
> > #define TIOCGPTN _IOR('T', 0x30, unsigned int) /* Get Pty Number (of
> > pty-mux device) */
> > #define TIOCSPTLCK _IOW('T', 0x31, int) /* Lock/unlock Pty */
> > #define TIOCGDEV _IOR('T', 0x32, unsigned int) /* Get primary device
> > node of /dev/console */
> > diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> > index
Re: [PATCH v3 00/12] kbuild/kconfig: do not update config during installation
Masahiro Yamada writes:
> 2018-07-10 20:34 GMT+09:00 Dirk Gouders :
>> Masahiro Yamada writes:
>>
>>> The main motivation of this patch series is to suppress the syncconfig
>>> during running installation targets.
>>>
>>> V1 consisted of only two patches:
>>> https://patchwork.kernel.org/patch/10468105/
>>> https://patchwork.kernel.org/patch/10468103/
>>>
>>> I noticed that installation targets would continue running
>>> even if the source tree is not configured at all
>>> because the inclusion of include/config/auto.conf was optional.
>>>
>>> So, I added one more patch in V2:
>>> https://patchwork.kernel.org/patch/10483637/
>>>
>>> However, kbuild test robot reported a new warning message was displayed:
>>>
>>> Makefile:592: include/config/auto.conf: No such file or directory
>>>
>>> This warning is displayed only for Make 4.1 or older.
>>>
>>> To fix this annoying warning, I changed Kconfig too,
>>> which leaded to more clean-up, improvements in Kconfig.
>>>
>>> So, V3 is a big patch series.
>>
>> Hello Masahiro,
>>
>> I tested your series for a while, now. I did not notice real issues
>> with it but want to leave some remarks about what I noticed in
>> the surroundings of your patches.
>>
>>
>>> Masahiro Yamada (12):
>>> kconfig: rename file_write_dep and move it to confdata.c
>>
>> I might be missing some trivial use-case, but when looking at this
>> patch, I noticed an inconsistency with the file names auto.conf and
>> auto.conf.cmd.
>>
>> The first can be modified by an environment variable but when this
>> happens, auto.conf.cmd remains as is. I noticed that only the
>> Documentation mentions that KCONFIG_AUTOCONFIG exists and confdata.c
>> uses it to serve the file name -- no other use anywhere.
>
> Indeed.
>
> I had also noticed this.
>
> Probably, replacing the hardcoded 'include/config/auto.conf.cmd'
> with get_env('KCONFIG_AUTOCONFIG') + 'cmd' will be nice.
>
>
> I did not touch it since it thought it was less important
> for this patch set.
>
> If you are willing to contribute to this,
> a patch is welcome (after this series).
Yes, it is not that important but it would probably help people new to
kbuild/kconfig if we make this a bit more consistent. I will send a
patch that also adds some words to the documentation of
KCONFIG_AUTOCONFIG.
>> Now, I am wondering if I just don't see an important case when the
>> use of KCONFIG_AUTOCONFIG is really helpful or even mandatory.
>
>
> I do not know the historical background,
> but I guess predecessors wanted to implement Kconfig
> as generic/flexible as possible.
>
>
>>
>>> kconfig: split out helpers to check file/directory, create directory
>>> kconfig: remove unneeded directory generation from local*config
>>> kconfig: create directories needed for syncconfig by itself
>>> kconfig: make syncconfig update .config regardless of sym_change_count
>>
>> For this patch, I already mentioned that `conf --help' perhaps could be
>> updated.
>
> What do you want 'conf --help' to look like ?
As I said, --help does not say a lot about which files are updated, so I
probably was too pedantic. For --syncconfig it currently says:
--syncconfigSimilar to oldconfig but generates configuration in
include/{generated/,config/}
which could let someone guess .config isn't touched (because of the
"but"). If you also think so, the text could perhaps say:
--syncconfigSimilar to oldconfig but generates configuration in
include/{generated/,config/} in addition.
>
>> On the other side, none of the entries there tells us such
>> details, so there is probably no need for syncconfig to do so.
>>
>>> kconfig: allow all config targets to write auto.conf if missing
>>> kbuild: use 'include' directive to load auto.conf from top Makefile
>>> kbuild: add .DELETE_ON_ERROR special target
>>> kbuild: do not update config when running install targets
>>> kbuild: do not update config for 'make kernelrelease'
>>> kbuild: remove auto.conf and tristate.conf from prerequisites
>>
>> In the surrounding of this patch I noticed -include of auto.conf and
>> tristate.conf in scripts/Makfile.modbuildin. I tried it in some ways
>> but was not able to trigger that file being used with a missing
>> auto.conf.
>
> Right. auto.conf and tristate.conf are mandatory there.
>
> '-include' should be replaced with 'include'.
>
> Cater to send a patch?
I will do.
Dirk
>
>> On the other hand, if I now manually remove tristate.conf,
>> that would not be fixed or even noticed, because of -include and I
>> wonder if it is safer to also change the -includes in that file.
>>
>> It seems, if one of those files is missing, one must have done it
>> manually or some other serious issue is present that we probably want to
>> notice.
>
> You are right. If somebody removes tristate.conf on purpose,
> it is not self-healing.
>
> I should be fixed by itself, or at least it should fail
> with clear
Re: [PATCH v3 00/12] kbuild/kconfig: do not update config during installation
Masahiro Yamada writes:
> 2018-07-10 20:34 GMT+09:00 Dirk Gouders :
>> Masahiro Yamada writes:
>>
>>> The main motivation of this patch series is to suppress the syncconfig
>>> during running installation targets.
>>>
>>> V1 consisted of only two patches:
>>> https://patchwork.kernel.org/patch/10468105/
>>> https://patchwork.kernel.org/patch/10468103/
>>>
>>> I noticed that installation targets would continue running
>>> even if the source tree is not configured at all
>>> because the inclusion of include/config/auto.conf was optional.
>>>
>>> So, I added one more patch in V2:
>>> https://patchwork.kernel.org/patch/10483637/
>>>
>>> However, kbuild test robot reported a new warning message was displayed:
>>>
>>> Makefile:592: include/config/auto.conf: No such file or directory
>>>
>>> This warning is displayed only for Make 4.1 or older.
>>>
>>> To fix this annoying warning, I changed Kconfig too,
>>> which leaded to more clean-up, improvements in Kconfig.
>>>
>>> So, V3 is a big patch series.
>>
>> Hello Masahiro,
>>
>> I tested your series for a while, now. I did not notice real issues
>> with it but want to leave some remarks about what I noticed in
>> the surroundings of your patches.
>>
>>
>>> Masahiro Yamada (12):
>>> kconfig: rename file_write_dep and move it to confdata.c
>>
>> I might be missing some trivial use-case, but when looking at this
>> patch, I noticed an inconsistency with the file names auto.conf and
>> auto.conf.cmd.
>>
>> The first can be modified by an environment variable but when this
>> happens, auto.conf.cmd remains as is. I noticed that only the
>> Documentation mentions that KCONFIG_AUTOCONFIG exists and confdata.c
>> uses it to serve the file name -- no other use anywhere.
>
> Indeed.
>
> I had also noticed this.
>
> Probably, replacing the hardcoded 'include/config/auto.conf.cmd'
> with get_env('KCONFIG_AUTOCONFIG') + 'cmd' will be nice.
>
>
> I did not touch it since it thought it was less important
> for this patch set.
>
> If you are willing to contribute to this,
> a patch is welcome (after this series).
Yes, it is not that important but it would probably help people new to
kbuild/kconfig if we make this a bit more consistent. I will send a
patch that also adds some words to the documentation of
KCONFIG_AUTOCONFIG.
>> Now, I am wondering if I just don't see an important case when the
>> use of KCONFIG_AUTOCONFIG is really helpful or even mandatory.
>
>
> I do not know the historical background,
> but I guess predecessors wanted to implement Kconfig
> as generic/flexible as possible.
>
>
>>
>>> kconfig: split out helpers to check file/directory, create directory
>>> kconfig: remove unneeded directory generation from local*config
>>> kconfig: create directories needed for syncconfig by itself
>>> kconfig: make syncconfig update .config regardless of sym_change_count
>>
>> For this patch, I already mentioned that `conf --help' perhaps could be
>> updated.
>
> What do you want 'conf --help' to look like ?
As I said, --help does not say a lot about which files are updated, so I
probably was too pedantic. For --syncconfig it currently says:
--syncconfigSimilar to oldconfig but generates configuration in
include/{generated/,config/}
which could let someone guess .config isn't touched (because of the
"but"). If you also think so, the text could perhaps say:
--syncconfigSimilar to oldconfig but generates configuration in
include/{generated/,config/} in addition.
>
>> On the other side, none of the entries there tells us such
>> details, so there is probably no need for syncconfig to do so.
>>
>>> kconfig: allow all config targets to write auto.conf if missing
>>> kbuild: use 'include' directive to load auto.conf from top Makefile
>>> kbuild: add .DELETE_ON_ERROR special target
>>> kbuild: do not update config when running install targets
>>> kbuild: do not update config for 'make kernelrelease'
>>> kbuild: remove auto.conf and tristate.conf from prerequisites
>>
>> In the surrounding of this patch I noticed -include of auto.conf and
>> tristate.conf in scripts/Makfile.modbuildin. I tried it in some ways
>> but was not able to trigger that file being used with a missing
>> auto.conf.
>
> Right. auto.conf and tristate.conf are mandatory there.
>
> '-include' should be replaced with 'include'.
>
> Cater to send a patch?
I will do.
Dirk
>
>> On the other hand, if I now manually remove tristate.conf,
>> that would not be fixed or even noticed, because of -include and I
>> wonder if it is safer to also change the -includes in that file.
>>
>> It seems, if one of those files is missing, one must have done it
>> manually or some other serious issue is present that we probably want to
>> notice.
>
> You are right. If somebody removes tristate.conf on purpose,
> it is not self-healing.
>
> I should be fixed by itself, or at least it should fail
> with clear
Re: [PATCH] pinctrl: ocelot: fix gpio4 twi function
On Wed, Jul 11, 2018 at 3:01 PM Alexandre Belloni wrote: > the TWI function on GPIO4 is actually a multiplexed SCL, not an original > TWI SDA or SCL. Fix it. > > Signed-off-by: Alexandre Belloni Patch applied. Yours, Linus Walleij
Re: [PATCH] pinctrl: ocelot: fix gpio4 twi function
On Wed, Jul 11, 2018 at 3:01 PM Alexandre Belloni wrote: > the TWI function on GPIO4 is actually a multiplexed SCL, not an original > TWI SDA or SCL. Fix it. > > Signed-off-by: Alexandre Belloni Patch applied. Yours, Linus Walleij
Re: [LINUX PATCH v11 2/3] memory: pl353: Add driver for arm pl353 static memory controller
On Wed, Jul 11, 2018 at 9:37 AM Naga Sureshkumar Relli wrote: > Add driver for arm pl353 static memory controller. This controller is used in > Xilinx Zynq SoC for interfacing the NAND and NOR/SRAM memory devices. > > Signed-off-by: Naga Sureshkumar Relli > --- > Changes in v11: > - Added amba device registration and removed platform device, since it >is an ARM Primecell Peripheral and it should register as an amba device >as per Linus Walleji. This looks good to me! Reviewed-by: Linus Walleij Yours, Linus Walleij
Re: [LINUX PATCH v11 2/3] memory: pl353: Add driver for arm pl353 static memory controller
On Wed, Jul 11, 2018 at 9:37 AM Naga Sureshkumar Relli wrote: > Add driver for arm pl353 static memory controller. This controller is used in > Xilinx Zynq SoC for interfacing the NAND and NOR/SRAM memory devices. > > Signed-off-by: Naga Sureshkumar Relli > --- > Changes in v11: > - Added amba device registration and removed platform device, since it >is an ARM Primecell Peripheral and it should register as an amba device >as per Linus Walleji. This looks good to me! Reviewed-by: Linus Walleij Yours, Linus Walleij
Re: [LINUX PATCH v11 1/3] dt-bindings: memory: Add pl353 smc controller devicetree binding information
On Wed, Jul 11, 2018 at 9:37 AM Naga Sureshkumar Relli wrote: > Add pl353 static memory controller devicetree binding information. > > Signed-off-by: Naga Sureshkumar Relli > --- > Changes in v11: Reviewed-by: Linus Walleij Thanks! Yours, Linus Walleij
Re: [LINUX PATCH v11 1/3] dt-bindings: memory: Add pl353 smc controller devicetree binding information
On Wed, Jul 11, 2018 at 9:37 AM Naga Sureshkumar Relli wrote: > Add pl353 static memory controller devicetree binding information. > > Signed-off-by: Naga Sureshkumar Relli > --- > Changes in v11: Reviewed-by: Linus Walleij Thanks! Yours, Linus Walleij
Re: [PATCH v10 0/6] Driver for at91 usart in spi mode
On Wed, 11 Jul 2018, Nicolas Ferre wrote: > On 25/06/2018 at 19:22, Radu Pirea wrote: > > Hello, > > > > This is the second version of driver. I added a mfd driver which by > > default probes atmel_serial driver and if in dt is specified to probe > > the spi driver, then the spi-at91-usart driver will be probed. The > > compatible for atmel_serial is now the compatible for at91-usart mfd > > driver and compatilbe for atmel_serial driver was changed in order to > > keep the bindings for serial as they are. > > > > Changes in v10: > > -fixed kbuild test robot warning > > > > Changes in v9: > > - minor changes > > - rebased on top of broonie/for-4.19 > > > > Changes in v8: > > - fixed passing an empty mfd cell if "atmel,usart-mode" value is invalid > > > > Changes in v7: > > - synced up SPDIX license with module license > > - numbering of usart modes starts from 0 insteand of 1 > > > > Changes in v6: > > - removed unused compatible strings from serial and spi drivers > > > > Changes in v5: > > - fixed usage of stdout-path property with atmel_serial driver > > > > Changes in v4: > > - modified the spi driver to use cs gpio support form spi subsystem > > - fixed dma transfers for serial driver > > - squashed binding for spi and serial and moved them to mfd/atmel-usart.txt > > > > Changes in v3: > > - fixed spi slaves probing > > > > Changes in v2: > > - added at91-usart mfd driver > > - modified spi-at91-usart driver to work as mfd driver child > > - modified atmel_serial driver to work as mfd driver child > > > > Changes in v1: > > - added spi-at91-usart driver > > > > > > Radu Pirea (6): > >MAINTAINERS: add at91 usart mfd driver > >dt-bindings: add binding for atmel-usart in SPI mode > >mfd: at91-usart: added mfd driver for usart > >MAINTAINERS: add at91 usart spi driver > >spi: at91-usart: add driver for at91-usart as spi > >tty/serial: atmel: change the driver to work under at91-usart mfd > > For the record or if needed by the MAINTAINERS changes, you can add my: > Acked-by: Nicolas Ferre > for the whole series. > > Once the remarks by Mark are addressed, you can certainly re-send the series > with all tags collected for Lee to take it, if he is okay with this process, > of course... That's fine. No problem. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v10 0/6] Driver for at91 usart in spi mode
On Wed, 11 Jul 2018, Nicolas Ferre wrote: > On 25/06/2018 at 19:22, Radu Pirea wrote: > > Hello, > > > > This is the second version of driver. I added a mfd driver which by > > default probes atmel_serial driver and if in dt is specified to probe > > the spi driver, then the spi-at91-usart driver will be probed. The > > compatible for atmel_serial is now the compatible for at91-usart mfd > > driver and compatilbe for atmel_serial driver was changed in order to > > keep the bindings for serial as they are. > > > > Changes in v10: > > -fixed kbuild test robot warning > > > > Changes in v9: > > - minor changes > > - rebased on top of broonie/for-4.19 > > > > Changes in v8: > > - fixed passing an empty mfd cell if "atmel,usart-mode" value is invalid > > > > Changes in v7: > > - synced up SPDIX license with module license > > - numbering of usart modes starts from 0 insteand of 1 > > > > Changes in v6: > > - removed unused compatible strings from serial and spi drivers > > > > Changes in v5: > > - fixed usage of stdout-path property with atmel_serial driver > > > > Changes in v4: > > - modified the spi driver to use cs gpio support form spi subsystem > > - fixed dma transfers for serial driver > > - squashed binding for spi and serial and moved them to mfd/atmel-usart.txt > > > > Changes in v3: > > - fixed spi slaves probing > > > > Changes in v2: > > - added at91-usart mfd driver > > - modified spi-at91-usart driver to work as mfd driver child > > - modified atmel_serial driver to work as mfd driver child > > > > Changes in v1: > > - added spi-at91-usart driver > > > > > > Radu Pirea (6): > >MAINTAINERS: add at91 usart mfd driver > >dt-bindings: add binding for atmel-usart in SPI mode > >mfd: at91-usart: added mfd driver for usart > >MAINTAINERS: add at91 usart spi driver > >spi: at91-usart: add driver for at91-usart as spi > >tty/serial: atmel: change the driver to work under at91-usart mfd > > For the record or if needed by the MAINTAINERS changes, you can add my: > Acked-by: Nicolas Ferre > for the whole series. > > Once the remarks by Mark are addressed, you can certainly re-send the series > with all tags collected for Lee to take it, if he is okay with this process, > of course... That's fine. No problem. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v4] regulator: fixed: Convert to use GPIO descriptor only
On Tue, Jul 10, 2018 at 7:56 PM Janusz Krzysztofik wrote:
> > - .gpio = AMS_DELTA_GPIO_PIN_MODEM_NRESET,
>
> This is OK but not enough for clean build of board-ams-delta.c when merged
> into current linux-next as one more struct fixed_voltage_config introduced
> there
> recently - keybrd_pwr_config - needs removal of .gpio member (respective
> lookup
> table with NULL function name is already there).
>
> > @@ -538,6 +546,7 @@ static struct gpiod_lookup_table
> > *ams_delta_gpio_tables[] __initdata = {
> > };
> >
> > static struct gpiod_lookup_table *late_gpio_tables[] __initdata = {
> > + _delta_nreset_gpiod_table,
>
> That is also OK but may raise a conflict when merged into current linux-next
> where late_gpio_tables[] has been removed from board-ams-delta.c and its
> content integrated into ams_delta_gpio_tables[].
>
> > _delta_lcd_gpio_table,
> > _delta_nand_gpio_table,
> > };
>
> If that makes your life easier, I can prepare a fix for board-ams-delta.c on
> top of your patch. In that case you can add my:
> Reviewed-by: Janusz Krzysztofik
Hm it's a bit of cross-tree conflict going on here I guess.
Do you have some idea about how serious the conflicts will be?
Is it just one patch to the ARM SoC OMAP tree or several?
It's a bit of Mark's pick, there are several ways to go about it:
1. Simply defer this to the next kernel cycle when your change is upstream
and avoid all fuzz (totally OK as long as one is not impatient).
I'm definately not in a hurry.
2. Mark applies this, conflicts appear in linux-next, you help Stephen
to solve it and later on Torvalds has to solve it. Then we need to
know how serious the conflicts are.
3. Apply this patch with fixes to the ARM SoC tree. Which makes it hard to
pull out so I'm not so sure about that.
4. An immutable branch with the ARM SoC change for Mark to pull
before applying this so I can rebase this patch on that.
5. Pick some patch from ARM SoC and apply it *also* to the regulator
tree and then this on top so I can rebase the changes and avoid
all conflicts. (We do this sometimes as some last resort.)
6. ...?
BTW I like your OMAP1 cleanups a lot!
Yours,
Linus Walleij
Re: [PATCH v4] regulator: fixed: Convert to use GPIO descriptor only
On Tue, Jul 10, 2018 at 7:56 PM Janusz Krzysztofik wrote:
> > - .gpio = AMS_DELTA_GPIO_PIN_MODEM_NRESET,
>
> This is OK but not enough for clean build of board-ams-delta.c when merged
> into current linux-next as one more struct fixed_voltage_config introduced
> there
> recently - keybrd_pwr_config - needs removal of .gpio member (respective
> lookup
> table with NULL function name is already there).
>
> > @@ -538,6 +546,7 @@ static struct gpiod_lookup_table
> > *ams_delta_gpio_tables[] __initdata = {
> > };
> >
> > static struct gpiod_lookup_table *late_gpio_tables[] __initdata = {
> > + _delta_nreset_gpiod_table,
>
> That is also OK but may raise a conflict when merged into current linux-next
> where late_gpio_tables[] has been removed from board-ams-delta.c and its
> content integrated into ams_delta_gpio_tables[].
>
> > _delta_lcd_gpio_table,
> > _delta_nand_gpio_table,
> > };
>
> If that makes your life easier, I can prepare a fix for board-ams-delta.c on
> top of your patch. In that case you can add my:
> Reviewed-by: Janusz Krzysztofik
Hm it's a bit of cross-tree conflict going on here I guess.
Do you have some idea about how serious the conflicts will be?
Is it just one patch to the ARM SoC OMAP tree or several?
It's a bit of Mark's pick, there are several ways to go about it:
1. Simply defer this to the next kernel cycle when your change is upstream
and avoid all fuzz (totally OK as long as one is not impatient).
I'm definately not in a hurry.
2. Mark applies this, conflicts appear in linux-next, you help Stephen
to solve it and later on Torvalds has to solve it. Then we need to
know how serious the conflicts are.
3. Apply this patch with fixes to the ARM SoC tree. Which makes it hard to
pull out so I'm not so sure about that.
4. An immutable branch with the ARM SoC change for Mark to pull
before applying this so I can rebase this patch on that.
5. Pick some patch from ARM SoC and apply it *also* to the regulator
tree and then this on top so I can rebase the changes and avoid
all conflicts. (We do this sometimes as some last resort.)
6. ...?
BTW I like your OMAP1 cleanups a lot!
Yours,
Linus Walleij
Re: [PATCH 6/6] mfd: rave-sp: Emulate CMD_GET_STATUS on device that don't support it
On Fri, 06 Jul 2018, Andrey Smirnov wrote: > CMD_GET_STATUS is not supported by some devices implementing > RDU2-compatible ICD as well as "legacy" devices. To account for that > fact, add code that obtains the same information (app/bootloader FW > version) using several different commands. > > Cc: linux-kernel@vger.kernel.org > Cc: cphe...@gmail.com > Cc: Lucas Stach > Cc: Nikita Yushchenko > Cc: Lee Jones > Signed-off-by: Andrey Smirnov > --- > drivers/mfd/rave-sp.c | 96 --- > 1 file changed, 63 insertions(+), 33 deletions(-) Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 5/6] mfd: rave-sp: Add legacy watchdog ping command translation
On Fri, 06 Jul 2018, Andrey Smirnov wrote: > This is needed to make rave-sp-wdt driver to properly ping the > watchdog on "legacy" firmware. > > Cc: linux-kernel@vger.kernel.org > Cc: cphe...@gmail.com > Cc: Lucas Stach > Cc: Nikita Yushchenko > Cc: Lee Jones > Signed-off-by: Andrey Smirnov > --- > drivers/mfd/rave-sp.c | 2 ++ > 1 file changed, 2 insertions(+) Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 6/6] mfd: rave-sp: Emulate CMD_GET_STATUS on device that don't support it
On Fri, 06 Jul 2018, Andrey Smirnov wrote: > CMD_GET_STATUS is not supported by some devices implementing > RDU2-compatible ICD as well as "legacy" devices. To account for that > fact, add code that obtains the same information (app/bootloader FW > version) using several different commands. > > Cc: linux-kernel@vger.kernel.org > Cc: cphe...@gmail.com > Cc: Lucas Stach > Cc: Nikita Yushchenko > Cc: Lee Jones > Signed-off-by: Andrey Smirnov > --- > drivers/mfd/rave-sp.c | 96 --- > 1 file changed, 63 insertions(+), 33 deletions(-) Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 5/6] mfd: rave-sp: Add legacy watchdog ping command translation
On Fri, 06 Jul 2018, Andrey Smirnov wrote: > This is needed to make rave-sp-wdt driver to properly ping the > watchdog on "legacy" firmware. > > Cc: linux-kernel@vger.kernel.org > Cc: cphe...@gmail.com > Cc: Lucas Stach > Cc: Nikita Yushchenko > Cc: Lee Jones > Signed-off-by: Andrey Smirnov > --- > drivers/mfd/rave-sp.c | 2 ++ > 1 file changed, 2 insertions(+) Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 4/6] mfd: rave-sp: Add legacy EEPROM access command translation
On Fri, 06 Jul 2018, Andrey Smirnov wrote: > This is needed to make rave-sp-eeprom driver work on "legacy" > firmware. > > Cc: linux-kernel@vger.kernel.org > Cc: cphe...@gmail.com > Cc: Lucas Stach > Cc: Nikita Yushchenko > Cc: Lee Jones > Signed-off-by: Andrey Smirnov > --- > drivers/mfd/rave-sp.c | 2 ++ > include/linux/mfd/rave-sp.h | 1 + > 2 files changed, 3 insertions(+) Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 4/6] mfd: rave-sp: Add legacy EEPROM access command translation
On Fri, 06 Jul 2018, Andrey Smirnov wrote: > This is needed to make rave-sp-eeprom driver work on "legacy" > firmware. > > Cc: linux-kernel@vger.kernel.org > Cc: cphe...@gmail.com > Cc: Lucas Stach > Cc: Nikita Yushchenko > Cc: Lee Jones > Signed-off-by: Andrey Smirnov > --- > drivers/mfd/rave-sp.c | 2 ++ > include/linux/mfd/rave-sp.h | 1 + > 2 files changed, 3 insertions(+) Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 2/6] mfd: rave-sp: Fix incorrectly specified checksum type
On Fri, 06 Jul 2018, Andrey Smirnov wrote: > RAVE SP firmware covered by "legacy" variant uses 16-bit CCITT > checksum algorithm. Change the code to correctly reflect that. > > Cc: linux-kernel@vger.kernel.org > Cc: cphe...@gmail.com > Cc: Lucas Stach > Cc: Nikita Yushchenko > Cc: Lee Jones > Signed-off-by: Andrey Smirnov > --- > drivers/mfd/rave-sp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 3/6] mfd: rave-sp: Initialize flow control and parity of the port
On Fri, 06 Jul 2018, Andrey Smirnov wrote: > Relying on serial port defaults for flow control and parity can result > in complete breakdown of communication with RAVE SP on some platforms > where defaults are not what we need them to be. One such case is > VF610-base ZII SPU3 board (not supported upstream). To avoid this > problem in the future, add code to explicitly configure both. > > Cc: linux-kernel@vger.kernel.org > Cc: cphe...@gmail.com > Cc: Lucas Stach > Cc: Nikita Yushchenko > Cc: Lee Jones > Signed-off-by: Andrey Smirnov > --- > drivers/mfd/rave-sp.c | 7 +++ > 1 file changed, 7 insertions(+) Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 2/6] mfd: rave-sp: Fix incorrectly specified checksum type
On Fri, 06 Jul 2018, Andrey Smirnov wrote: > RAVE SP firmware covered by "legacy" variant uses 16-bit CCITT > checksum algorithm. Change the code to correctly reflect that. > > Cc: linux-kernel@vger.kernel.org > Cc: cphe...@gmail.com > Cc: Lucas Stach > Cc: Nikita Yushchenko > Cc: Lee Jones > Signed-off-by: Andrey Smirnov > --- > drivers/mfd/rave-sp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 3/6] mfd: rave-sp: Initialize flow control and parity of the port
On Fri, 06 Jul 2018, Andrey Smirnov wrote: > Relying on serial port defaults for flow control and parity can result > in complete breakdown of communication with RAVE SP on some platforms > where defaults are not what we need them to be. One such case is > VF610-base ZII SPU3 board (not supported upstream). To avoid this > problem in the future, add code to explicitly configure both. > > Cc: linux-kernel@vger.kernel.org > Cc: cphe...@gmail.com > Cc: Lucas Stach > Cc: Nikita Yushchenko > Cc: Lee Jones > Signed-off-by: Andrey Smirnov > --- > drivers/mfd/rave-sp.c | 7 +++ > 1 file changed, 7 insertions(+) Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 1/6] mfd: rave-sp: Remove unused defines
On Fri, 06 Jul 2018, Andrey Smirnov wrote: > Remove unusded defines that are a leftover from earlier iterations of > the driver. > > Cc: linux-kernel@vger.kernel.org > Cc: cphe...@gmail.com > Cc: Lucas Stach > Cc: Nikita Yushchenko > Cc: Lee Jones > Signed-off-by: Andrey Smirnov > --- > drivers/mfd/rave-sp.c | 10 -- > 1 file changed, 10 deletions(-) Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 1/6] mfd: rave-sp: Remove unused defines
On Fri, 06 Jul 2018, Andrey Smirnov wrote: > Remove unusded defines that are a leftover from earlier iterations of > the driver. > > Cc: linux-kernel@vger.kernel.org > Cc: cphe...@gmail.com > Cc: Lucas Stach > Cc: Nikita Yushchenko > Cc: Lee Jones > Signed-off-by: Andrey Smirnov > --- > drivers/mfd/rave-sp.c | 10 -- > 1 file changed, 10 deletions(-) Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
Andy Lutomirski wrote: > > Also you can't currently directly create a bind mount from userspace as you > > can only bind from another path point - which you may not be able to access > > (either by permission failure or because it's not in your mount namespace). > > > > Are you trying to preserve the magic bind semantics with the new API? No, I'm pointing out that you can't emulate this by doing a bind mount from userspace if you can't access the thing you're binding from. Now, we could create a syscall that just picks up an extant superblock using a device and attaches it to a mount for you, but that would have to be at least partially parameterised - which would be very fs-dependent - so that it can know whether or not you're allowed to create another mount to that sb. What you're talking about is emulating sget() in userspace - when we have to do it in the kernel anyway if we still offer mount(2). David
Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
Andy Lutomirski wrote: > > Also you can't currently directly create a bind mount from userspace as you > > can only bind from another path point - which you may not be able to access > > (either by permission failure or because it's not in your mount namespace). > > > > Are you trying to preserve the magic bind semantics with the new API? No, I'm pointing out that you can't emulate this by doing a bind mount from userspace if you can't access the thing you're binding from. Now, we could create a syscall that just picks up an extant superblock using a device and attaches it to a mount for you, but that would have to be at least partially parameterised - which would be very fs-dependent - so that it can know whether or not you're allowed to create another mount to that sb. What you're talking about is emulating sget() in userspace - when we have to do it in the kernel anyway if we still offer mount(2). David
Re: [PATCH 2/3][RFC] PM / Hibernate: Encrypt the snapshot pages before submitted to the block device
Hi, On Thu, Jul 12, 2018 at 06:10:37PM +0800, joeyli wrote: > Hi Yu Chen, > > Sorry for my delay... > > On Fri, Jul 06, 2018 at 11:28:56PM +0800, Yu Chen wrote: > > Hi Joey Lee, > > On Fri, Jun 29, 2018 at 08:59:43PM +0800, joeyli wrote: > > > On Thu, Jun 28, 2018 at 10:52:07PM +0800, Yu Chen wrote: > > > > Hi, > > > > On Thu, Jun 28, 2018 at 10:28:56PM +0800, joeyli wrote: > > > > > On Thu, Jun 28, 2018 at 09:50:17PM +0800, Yu Chen wrote: > > > > > > Hi, > > > > > > On Thu, Jun 28, 2018 at 09:07:20PM +0800, joeyli wrote: > > > > > > > Hi Chen Yu, > > > > > > > > > > > > > > On Wed, Jun 20, 2018 at 05:40:32PM +0800, Chen Yu wrote: > > > > > > > > Use the helper functions introduced previously to encrypt > > > > > > > > the page data before they are submitted to the block device. > > > > > > > > Besides, for the case of hibernation compression, the data > > > > > > > > are firstly compressed and then encrypted, and vice versa > > > > > > > > for the resume process. > > > > > > > > > > > > > > > > > > > > > > I want to suggest my solution that it direct signs/encrypts the > > > > > > > memory snapshot image. This solution is already shipped with > > > > > > > SLE12 a couple of years: > > > > > > > > > > > > > > https://github.com/joeyli/linux-s4sign/commits/s4sign-hmac-encrypted-key-v0.2-v4.17-rc3 > > > > > > > > > > > > > I did not see image page encryption in above link, if I understand > > > > > > > > > > PM / hibernate: Generate and verify signature for snapshot image > > > > > https://github.com/joeyli/linux-s4sign/commit/bae39460393ada4c0226dd07cd5e3afcef86b71f > > > > > > > > > > PM / hibernate: snapshot image encryption > > > > > https://github.com/joeyli/linux-s4sign/commit/6a9a0113bb221c036ebd0f6321b7191283fe4929 > > > > > > > > > > The above patches sign and encrypt the data pages in snapshot image. > > > > > It puts the signature to header. > > > > > > > > > It looks like your signature code can be simplyly added on top of the > > > > solution we proposed here, how about we collaborating on this task? > > > > > > OK, I will base on your user key solution to respin my signature patches. > > > > > > > just my 2 cents, > > > > 1. The cryption code should be indepent of the snapshot code, and > > > >this is why we implement it as a kernel module for that in > > > > PATCH[1/3]. > > > > > > Why the cryption code must be indepent of snapshot code? > > > > > Modules can be easier to be maintained and customized/improved in the > > future IMO.. > > hm... currently I didn't see apparent benefit on this... > > About modularization, is it possible to separate the key handler code > from crypto_hibernation.c? Otherwise the snapshot signature needs > to depend on encrypt function. > I understand. It seems that we can reuse crypto_data() for the signature logic. For example, add one parameter for crypto_data(..., crypto_mode) the crypto_mode is a combination of ENCRYPT/DECRYPT/SIGNATURE_START/SIGNATURE_END, and can be controled by sysfs. If SIGNATURE_START is enabled, the crypto_data() invokes crypto_shash_update() to get "hmac(sha512)" hash, and if SIGNATURE_END is enabled, crypto_shash_final() stores the final result in global buffer struct hibernation_crypto.keys.digest[SNAPSHOT_DIGEST_SIZE] which can be passed to the restore kernel, the same as the salt. Besides, the swsusp_prepare_hash() in your code could be moved into init_crypto_helper(), thus the signature key could also reuse the same key passed from user space or via get_efi_secret_key(). In this way, the change in snapshot.c is minimal and the crypto facilities could be maintained in one file. > > > > 2. There's no need to traverse the snapshot image twice, if the > > > >image is large(there's requirement on servers now) we can > > > >simplyly do the encryption before the disk IO, and this is > > > >why PATCH[2/3] looks like this. > > > > > > If the encryption solution is only for block device, then the uswsusp > > > interface must be locked-down when kernel is in locked mode: > > > > > > uswsusp: Disable when the kernel is locked down > > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=lockdown-20180410=8732c1663d7c0305ae01ba5a1ee4d2299b7b4612 > > > > > > I still suggest to keep the solution to direct encript the snapshot > > > image for uswsusp because the snapshot can be encrypted by kernel > > > before user space get it. > > > > > > I mean that if the uswsusp be used, then kernel direct encrypts the > > > snapshot image, otherwise kernel encrypts pages before block io. > > > > > I did not quite get the point, if the kernel has been locked down, > > then the uswsusp is disabled, why the kernel encrypts the snapshot > > for uswsusp? > > There have some functions be locked-down because there have no > appropriate mechanisms to check the integrity of writing data. If > the snapshot image can be encrypted and authentication, then the > uswsusp interface is avaiable for
Re: [PATCH 2/3][RFC] PM / Hibernate: Encrypt the snapshot pages before submitted to the block device
Hi, On Thu, Jul 12, 2018 at 06:10:37PM +0800, joeyli wrote: > Hi Yu Chen, > > Sorry for my delay... > > On Fri, Jul 06, 2018 at 11:28:56PM +0800, Yu Chen wrote: > > Hi Joey Lee, > > On Fri, Jun 29, 2018 at 08:59:43PM +0800, joeyli wrote: > > > On Thu, Jun 28, 2018 at 10:52:07PM +0800, Yu Chen wrote: > > > > Hi, > > > > On Thu, Jun 28, 2018 at 10:28:56PM +0800, joeyli wrote: > > > > > On Thu, Jun 28, 2018 at 09:50:17PM +0800, Yu Chen wrote: > > > > > > Hi, > > > > > > On Thu, Jun 28, 2018 at 09:07:20PM +0800, joeyli wrote: > > > > > > > Hi Chen Yu, > > > > > > > > > > > > > > On Wed, Jun 20, 2018 at 05:40:32PM +0800, Chen Yu wrote: > > > > > > > > Use the helper functions introduced previously to encrypt > > > > > > > > the page data before they are submitted to the block device. > > > > > > > > Besides, for the case of hibernation compression, the data > > > > > > > > are firstly compressed and then encrypted, and vice versa > > > > > > > > for the resume process. > > > > > > > > > > > > > > > > > > > > > > I want to suggest my solution that it direct signs/encrypts the > > > > > > > memory snapshot image. This solution is already shipped with > > > > > > > SLE12 a couple of years: > > > > > > > > > > > > > > https://github.com/joeyli/linux-s4sign/commits/s4sign-hmac-encrypted-key-v0.2-v4.17-rc3 > > > > > > > > > > > > > I did not see image page encryption in above link, if I understand > > > > > > > > > > PM / hibernate: Generate and verify signature for snapshot image > > > > > https://github.com/joeyli/linux-s4sign/commit/bae39460393ada4c0226dd07cd5e3afcef86b71f > > > > > > > > > > PM / hibernate: snapshot image encryption > > > > > https://github.com/joeyli/linux-s4sign/commit/6a9a0113bb221c036ebd0f6321b7191283fe4929 > > > > > > > > > > The above patches sign and encrypt the data pages in snapshot image. > > > > > It puts the signature to header. > > > > > > > > > It looks like your signature code can be simplyly added on top of the > > > > solution we proposed here, how about we collaborating on this task? > > > > > > OK, I will base on your user key solution to respin my signature patches. > > > > > > > just my 2 cents, > > > > 1. The cryption code should be indepent of the snapshot code, and > > > >this is why we implement it as a kernel module for that in > > > > PATCH[1/3]. > > > > > > Why the cryption code must be indepent of snapshot code? > > > > > Modules can be easier to be maintained and customized/improved in the > > future IMO.. > > hm... currently I didn't see apparent benefit on this... > > About modularization, is it possible to separate the key handler code > from crypto_hibernation.c? Otherwise the snapshot signature needs > to depend on encrypt function. > I understand. It seems that we can reuse crypto_data() for the signature logic. For example, add one parameter for crypto_data(..., crypto_mode) the crypto_mode is a combination of ENCRYPT/DECRYPT/SIGNATURE_START/SIGNATURE_END, and can be controled by sysfs. If SIGNATURE_START is enabled, the crypto_data() invokes crypto_shash_update() to get "hmac(sha512)" hash, and if SIGNATURE_END is enabled, crypto_shash_final() stores the final result in global buffer struct hibernation_crypto.keys.digest[SNAPSHOT_DIGEST_SIZE] which can be passed to the restore kernel, the same as the salt. Besides, the swsusp_prepare_hash() in your code could be moved into init_crypto_helper(), thus the signature key could also reuse the same key passed from user space or via get_efi_secret_key(). In this way, the change in snapshot.c is minimal and the crypto facilities could be maintained in one file. > > > > 2. There's no need to traverse the snapshot image twice, if the > > > >image is large(there's requirement on servers now) we can > > > >simplyly do the encryption before the disk IO, and this is > > > >why PATCH[2/3] looks like this. > > > > > > If the encryption solution is only for block device, then the uswsusp > > > interface must be locked-down when kernel is in locked mode: > > > > > > uswsusp: Disable when the kernel is locked down > > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=lockdown-20180410=8732c1663d7c0305ae01ba5a1ee4d2299b7b4612 > > > > > > I still suggest to keep the solution to direct encript the snapshot > > > image for uswsusp because the snapshot can be encrypted by kernel > > > before user space get it. > > > > > > I mean that if the uswsusp be used, then kernel direct encrypts the > > > snapshot image, otherwise kernel encrypts pages before block io. > > > > > I did not quite get the point, if the kernel has been locked down, > > then the uswsusp is disabled, why the kernel encrypts the snapshot > > for uswsusp? > > There have some functions be locked-down because there have no > appropriate mechanisms to check the integrity of writing data. If > the snapshot image can be encrypted and authentication, then the > uswsusp interface is avaiable for
Re: [PATCH] mfd: hi655x: Fix regmap area declared size for hi655x
On Fri, 06 Jul 2018, Rafael David Tinoco wrote: > Fixes: https://bugs.linaro.org/show_bug.cgi?id=3903 > > LTP Functional tests have caused a bad paging request when triggering > the regmap_read_debugfs() logic of the device PMIC Hi6553 (reading > regmap/f800.pmic/registers file during read_all test): > > Unable to handle kernel paging request at virtual address 0 > [0984e000] pgd=77ffe803, pud=77ffd803,0 > Internal error: Oops: 9607 [#1] SMP > ... > Hardware name: HiKey Development Board (DT) > ... > Call trace: > regmap_mmio_read8+0x24/0x40 > regmap_mmio_read+0x48/0x70 > _regmap_bus_reg_read+0x38/0x48 > _regmap_read+0x68/0x170 > regmap_read+0x50/0x78 > regmap_read_debugfs+0x1a0/0x308 > regmap_map_read_file+0x48/0x58 > full_proxy_read+0x68/0x98 > __vfs_read+0x48/0x80 > vfs_read+0x94/0x150 > SyS_read+0x6c/0xd8 > el0_svc_naked+0x30/0x34 > Code: aa1e03e0 d503201f f9400280 8b334000 (3940) > > Investigations have showed that, when triggered by debugfs read() > handler, the mmio regmap logic was reading a bigger (16k) register area > than the one mapped by devm_ioremap_resource() during hi655x-pmic probe > time (4k). > > This commit changes hi655x's max register, according to HW specs, to be > the same as the one declared in the pmic device in hi6220's dts, fixing > the issue. > > Signed-off-by: Rafael David Tinoco > Cc: #v4.9 #v4.14 #v4.16 #v4.17 > --- > drivers/mfd/hi655x-pmic.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH] hostap: hide unused procfs helpers
+ Randy On 7/13/2018 9:03 AM, YueHaibing wrote: When CONFIG_PROC_FS isn't set, gcc warning this: drivers/net/wireless/intersil/hostap/hostap_hw.c:2901:12: warning: ‘prism2_registers_proc_show’ defined but not used [-Wunused-function] static int prism2_registers_proc_show(struct seq_file *m, void *v) drivers/net/wireless/intersil/hostap/hostap_proc.c:16:12: warning: ‘prism2_debug_proc_show’ defined but not used [-Wunused-function] static int prism2_debug_proc_show(struct seq_file *m, void *v) ^ drivers/net/wireless/intersil/hostap/hostap_proc.c:49:12: warning: ‘prism2_stats_proc_show’ defined but not used [-Wunused-function] static int prism2_stats_proc_show(struct seq_file *m, void *v) ^ drivers/net/wireless/intersil/hostap/hostap_proc.c:177:12: warning: ‘prism2_crypt_proc_show’ defined but not used [-Wunused-function] static int prism2_crypt_proc_show(struct seq_file *m, void *v) ^ fix this by adding #ifdef around them. hfa384x_read_reg is only used by prism2_registers_proc_show,so move it into #ifdef. There was already a fix for this posted by Randy Dunlap taking a different approach, ie. use __maybe_unused classifier. To be honest I prefer the ifdef approach as it is more explicit and does not feel like a cheat. Actually some of the functions are between a flag already PRISM2_NO_PROCFS_DEBUG which is in a private header file hostap_config.h. Seems like this would be better placed in Kconfig and depend on CONFIG_PROCFS. Anyway, this driver is old cruft. Maybe some people are still running it, but it is probably not worth the effort so fine with either fix. Regards, Arend
Re: [PATCH] mfd: hi655x: Fix regmap area declared size for hi655x
On Fri, 06 Jul 2018, Rafael David Tinoco wrote: > Fixes: https://bugs.linaro.org/show_bug.cgi?id=3903 > > LTP Functional tests have caused a bad paging request when triggering > the regmap_read_debugfs() logic of the device PMIC Hi6553 (reading > regmap/f800.pmic/registers file during read_all test): > > Unable to handle kernel paging request at virtual address 0 > [0984e000] pgd=77ffe803, pud=77ffd803,0 > Internal error: Oops: 9607 [#1] SMP > ... > Hardware name: HiKey Development Board (DT) > ... > Call trace: > regmap_mmio_read8+0x24/0x40 > regmap_mmio_read+0x48/0x70 > _regmap_bus_reg_read+0x38/0x48 > _regmap_read+0x68/0x170 > regmap_read+0x50/0x78 > regmap_read_debugfs+0x1a0/0x308 > regmap_map_read_file+0x48/0x58 > full_proxy_read+0x68/0x98 > __vfs_read+0x48/0x80 > vfs_read+0x94/0x150 > SyS_read+0x6c/0xd8 > el0_svc_naked+0x30/0x34 > Code: aa1e03e0 d503201f f9400280 8b334000 (3940) > > Investigations have showed that, when triggered by debugfs read() > handler, the mmio regmap logic was reading a bigger (16k) register area > than the one mapped by devm_ioremap_resource() during hi655x-pmic probe > time (4k). > > This commit changes hi655x's max register, according to HW specs, to be > the same as the one declared in the pmic device in hi6220's dts, fixing > the issue. > > Signed-off-by: Rafael David Tinoco > Cc: #v4.9 #v4.14 #v4.16 #v4.17 > --- > drivers/mfd/hi655x-pmic.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH] hostap: hide unused procfs helpers
+ Randy On 7/13/2018 9:03 AM, YueHaibing wrote: When CONFIG_PROC_FS isn't set, gcc warning this: drivers/net/wireless/intersil/hostap/hostap_hw.c:2901:12: warning: ‘prism2_registers_proc_show’ defined but not used [-Wunused-function] static int prism2_registers_proc_show(struct seq_file *m, void *v) drivers/net/wireless/intersil/hostap/hostap_proc.c:16:12: warning: ‘prism2_debug_proc_show’ defined but not used [-Wunused-function] static int prism2_debug_proc_show(struct seq_file *m, void *v) ^ drivers/net/wireless/intersil/hostap/hostap_proc.c:49:12: warning: ‘prism2_stats_proc_show’ defined but not used [-Wunused-function] static int prism2_stats_proc_show(struct seq_file *m, void *v) ^ drivers/net/wireless/intersil/hostap/hostap_proc.c:177:12: warning: ‘prism2_crypt_proc_show’ defined but not used [-Wunused-function] static int prism2_crypt_proc_show(struct seq_file *m, void *v) ^ fix this by adding #ifdef around them. hfa384x_read_reg is only used by prism2_registers_proc_show,so move it into #ifdef. There was already a fix for this posted by Randy Dunlap taking a different approach, ie. use __maybe_unused classifier. To be honest I prefer the ifdef approach as it is more explicit and does not feel like a cheat. Actually some of the functions are between a flag already PRISM2_NO_PROCFS_DEBUG which is in a private header file hostap_config.h. Seems like this would be better placed in Kconfig and depend on CONFIG_PROCFS. Anyway, this driver is old cruft. Maybe some people are still running it, but it is probably not worth the effort so fine with either fix. Regards, Arend
Re: [PATCH 1/2] gpio: mt7621: add OF_GPIO dependency
On Tue, Jul 10, 2018 at 5:19 PM Arnd Bergmann wrote:
> Compile-testing the driver fails unless OF_GPIO is enabled:
>
> drivers/gpio/gpio-mt7621.c: In function 'mediatek_gpio_bank_probe':
> drivers/gpio/gpio-mt7621.c:228:10: error: 'struct gpio_chip' has no member
> named 'of_node'
>
> Fixes: 4ba9c3afda41 ("gpio: mt7621: Add a driver for MT7621")
> Signed-off-by: Arnd Bergmann
Patch applied. Sorry for taking some days.
Yours,
Linus Walleij
Re: [PATCH 1/2] gpio: mt7621: add OF_GPIO dependency
On Tue, Jul 10, 2018 at 5:19 PM Arnd Bergmann wrote:
> Compile-testing the driver fails unless OF_GPIO is enabled:
>
> drivers/gpio/gpio-mt7621.c: In function 'mediatek_gpio_bank_probe':
> drivers/gpio/gpio-mt7621.c:228:10: error: 'struct gpio_chip' has no member
> named 'of_node'
>
> Fixes: 4ba9c3afda41 ("gpio: mt7621: Add a driver for MT7621")
> Signed-off-by: Arnd Bergmann
Patch applied. Sorry for taking some days.
Yours,
Linus Walleij
Re: [PATCH v13 13/18] x86/tsc: calibrate tsc only once
At 07/12/2018 08:04 AM, Pavel Tatashin wrote:
During boot tsc is calibrated twice: once in tsc_early_delay_calibrate(),
and the second time in tsc_init().
Rename tsc_early_delay_calibrate() to tsc_early_init(), and rework it so
the calibration is done only early, and make tsc_init() to use the values
already determined in tsc_early_init().
Sometimes it is not possible to determine tsc early, as the subsystem that
is required is not yet initialized, in such case try again later in
tsc_init().
Suggested-by: Thomas Gleixner
Signed-off-by: Pavel Tatashin
Hi Pavel,
Aha, a complex solution for a simple problem! ;-) And I did find any
benefits of doing that. did I miss something?
As the cpu_khz and tsc_khz are global variables and the tsc_khz may
be reset to cpu_khz. How about the following patch.
Thanks,
dou
8<---
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 74392d9d51e0..e54fa1037d45 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -1370,8 +1370,10 @@ void __init tsc_init(void)
return;
}
- cpu_khz = x86_platform.calibrate_cpu();
- tsc_khz = x86_platform.calibrate_tsc();
+ if (!tsc_khz) {
+ cpu_khz = x86_platform.calibrate_cpu();
+ tsc_khz = x86_platform.calibrate_tsc();
+ }
/*
* Trust non-zero tsc_khz as authorative,
---
arch/x86/include/asm/tsc.h | 2 +-
arch/x86/kernel/setup.c| 2 +-
arch/x86/kernel/tsc.c | 86 --
3 files changed, 48 insertions(+), 42 deletions(-)
diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
index 2701d221583a..c4368ff73652 100644
--- a/arch/x86/include/asm/tsc.h
+++ b/arch/x86/include/asm/tsc.h
@@ -33,7 +33,7 @@ static inline cycles_t get_cycles(void)
extern struct system_counterval_t convert_art_to_tsc(u64 art);
extern struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns);
-extern void tsc_early_delay_calibrate(void);
+extern void tsc_early_init(void);
extern void tsc_init(void);
extern void mark_tsc_unstable(char *reason);
extern int unsynchronized_tsc(void);
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 01fcc8bf7c8f..07445482bb57 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1014,7 +1014,7 @@ void __init setup_arch(char **cmdline_p)
*/
init_hypervisor_platform();
- tsc_early_delay_calibrate();
+ tsc_early_init();
x86_init.resources.probe_roms();
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 186395041725..bc8eb82050a3 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -33,6 +33,8 @@ EXPORT_SYMBOL(cpu_khz);
unsigned int __read_mostly tsc_khz;
EXPORT_SYMBOL(tsc_khz);
+#define KHZ 1000
+
/*
* TSC can be unstable due to cpufreq or due to unsynced TSCs
*/
@@ -1335,34 +1337,10 @@ static int __init init_tsc_clocksource(void)
*/
device_initcall(init_tsc_clocksource);
-void __init tsc_early_delay_calibrate(void)
-{
- unsigned long lpj;
-
- if (!boot_cpu_has(X86_FEATURE_TSC))
- return;
-
- cpu_khz = x86_platform.calibrate_cpu();
- tsc_khz = x86_platform.calibrate_tsc();
-
- tsc_khz = tsc_khz ? : cpu_khz;
- if (!tsc_khz)
- return;
-
- lpj = tsc_khz * 1000;
- do_div(lpj, HZ);
- loops_per_jiffy = lpj;
-}
-
-void __init tsc_init(void)
+static bool determine_cpu_tsc_frequncies(void)
{
- u64 lpj, cyc;
- int cpu;
-
- if (!boot_cpu_has(X86_FEATURE_TSC)) {
- setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE_TIMER);
- return;
- }
+ /* Make sure that cpu and tsc are not already calibrated */
+ WARN_ON(cpu_khz || tsc_khz);
cpu_khz = x86_platform.calibrate_cpu();
tsc_khz = x86_platform.calibrate_tsc();
@@ -1377,20 +1355,51 @@ void __init tsc_init(void)
else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
cpu_khz = tsc_khz;
- if (!tsc_khz) {
- mark_tsc_unstable("could not calculate TSC khz");
- setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE_TIMER);
- return;
- }
+ if (tsc_khz == 0)
+ return false;
pr_info("Detected %lu.%03lu MHz processor\n",
- (unsigned long)cpu_khz / 1000,
- (unsigned long)cpu_khz % 1000);
+ (unsigned long)cpu_khz / KHZ,
+ (unsigned long)cpu_khz % KHZ);
if (cpu_khz != tsc_khz) {
pr_info("Detected %lu.%03lu MHz TSC",
- (unsigned long)tsc_khz / 1000,
- (unsigned long)tsc_khz % 1000);
+ (unsigned long)tsc_khz / KHZ,
+ (unsigned long)tsc_khz % KHZ);
+ }
+ return true;
+}
+
+static unsigned long get_loops_per_jiffy(void)
+{
+
Re: [PATCH v13 13/18] x86/tsc: calibrate tsc only once
At 07/12/2018 08:04 AM, Pavel Tatashin wrote:
During boot tsc is calibrated twice: once in tsc_early_delay_calibrate(),
and the second time in tsc_init().
Rename tsc_early_delay_calibrate() to tsc_early_init(), and rework it so
the calibration is done only early, and make tsc_init() to use the values
already determined in tsc_early_init().
Sometimes it is not possible to determine tsc early, as the subsystem that
is required is not yet initialized, in such case try again later in
tsc_init().
Suggested-by: Thomas Gleixner
Signed-off-by: Pavel Tatashin
Hi Pavel,
Aha, a complex solution for a simple problem! ;-) And I did find any
benefits of doing that. did I miss something?
As the cpu_khz and tsc_khz are global variables and the tsc_khz may
be reset to cpu_khz. How about the following patch.
Thanks,
dou
8<---
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 74392d9d51e0..e54fa1037d45 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -1370,8 +1370,10 @@ void __init tsc_init(void)
return;
}
- cpu_khz = x86_platform.calibrate_cpu();
- tsc_khz = x86_platform.calibrate_tsc();
+ if (!tsc_khz) {
+ cpu_khz = x86_platform.calibrate_cpu();
+ tsc_khz = x86_platform.calibrate_tsc();
+ }
/*
* Trust non-zero tsc_khz as authorative,
---
arch/x86/include/asm/tsc.h | 2 +-
arch/x86/kernel/setup.c| 2 +-
arch/x86/kernel/tsc.c | 86 --
3 files changed, 48 insertions(+), 42 deletions(-)
diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
index 2701d221583a..c4368ff73652 100644
--- a/arch/x86/include/asm/tsc.h
+++ b/arch/x86/include/asm/tsc.h
@@ -33,7 +33,7 @@ static inline cycles_t get_cycles(void)
extern struct system_counterval_t convert_art_to_tsc(u64 art);
extern struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns);
-extern void tsc_early_delay_calibrate(void);
+extern void tsc_early_init(void);
extern void tsc_init(void);
extern void mark_tsc_unstable(char *reason);
extern int unsynchronized_tsc(void);
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 01fcc8bf7c8f..07445482bb57 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1014,7 +1014,7 @@ void __init setup_arch(char **cmdline_p)
*/
init_hypervisor_platform();
- tsc_early_delay_calibrate();
+ tsc_early_init();
x86_init.resources.probe_roms();
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 186395041725..bc8eb82050a3 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -33,6 +33,8 @@ EXPORT_SYMBOL(cpu_khz);
unsigned int __read_mostly tsc_khz;
EXPORT_SYMBOL(tsc_khz);
+#define KHZ 1000
+
/*
* TSC can be unstable due to cpufreq or due to unsynced TSCs
*/
@@ -1335,34 +1337,10 @@ static int __init init_tsc_clocksource(void)
*/
device_initcall(init_tsc_clocksource);
-void __init tsc_early_delay_calibrate(void)
-{
- unsigned long lpj;
-
- if (!boot_cpu_has(X86_FEATURE_TSC))
- return;
-
- cpu_khz = x86_platform.calibrate_cpu();
- tsc_khz = x86_platform.calibrate_tsc();
-
- tsc_khz = tsc_khz ? : cpu_khz;
- if (!tsc_khz)
- return;
-
- lpj = tsc_khz * 1000;
- do_div(lpj, HZ);
- loops_per_jiffy = lpj;
-}
-
-void __init tsc_init(void)
+static bool determine_cpu_tsc_frequncies(void)
{
- u64 lpj, cyc;
- int cpu;
-
- if (!boot_cpu_has(X86_FEATURE_TSC)) {
- setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE_TIMER);
- return;
- }
+ /* Make sure that cpu and tsc are not already calibrated */
+ WARN_ON(cpu_khz || tsc_khz);
cpu_khz = x86_platform.calibrate_cpu();
tsc_khz = x86_platform.calibrate_tsc();
@@ -1377,20 +1355,51 @@ void __init tsc_init(void)
else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
cpu_khz = tsc_khz;
- if (!tsc_khz) {
- mark_tsc_unstable("could not calculate TSC khz");
- setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE_TIMER);
- return;
- }
+ if (tsc_khz == 0)
+ return false;
pr_info("Detected %lu.%03lu MHz processor\n",
- (unsigned long)cpu_khz / 1000,
- (unsigned long)cpu_khz % 1000);
+ (unsigned long)cpu_khz / KHZ,
+ (unsigned long)cpu_khz % KHZ);
if (cpu_khz != tsc_khz) {
pr_info("Detected %lu.%03lu MHz TSC",
- (unsigned long)tsc_khz / 1000,
- (unsigned long)tsc_khz % 1000);
+ (unsigned long)tsc_khz / KHZ,
+ (unsigned long)tsc_khz % KHZ);
+ }
+ return true;
+}
+
+static unsigned long get_loops_per_jiffy(void)
+{
+
Re: [PATCH 08/32] genirq: Synchronize only with single thread on free_irq()
On Thu, Jul 12, 2018 at 05:21:09PM -0500, Bjorn Helgaas wrote: > On Sat, Jun 16, 2018 at 09:25:00PM +0200, Lukas Wunner wrote: > > When pciehp is converted to threaded IRQ handling, removal of unplugged > > devices below a PCIe hotplug port happens synchronously in the IRQ > > thread. Removal of devices typically entails a call to free_irq() by > > their drivers. > > > > If those devices share their IRQ with the hotplug port, free_irq() > > deadlocks because it calls synchronize_irq() to wait for all hard IRQ > > handlers as well as all threads sharing the IRQ to finish. > > > > Actually it's sufficient to wait only for the IRQ thread of the removed > > device, so call synchronize_hardirq() to wait for all hard IRQ handlers > > to finish, but no longer for any threads. Compensate by rearranging the > > control flow in irq_wait_for_interrupt() such that the device's thread > > is allowed to run one last time after kthread_stop() has been called. > > I assume this would need to be merged along with the rest of the > series, which should probably go through the PCI tree, but I'm > definitely not qualified to review this IRQ patch. And it would need > an ack from Thomas in any case. A v2 of this patch has already been merged through the tip tree on June 24, it's in linux-next as commit 519cc8652b3a, and ISTR that I marked this patch either as "Obsoleted" or "Not Applicable" in pci-patchwork. There was no build-dependency of the succeeding patches in the series on this patch, hence merging through a different tree was possible. Thanks! Lukas
Re: [PATCH 08/32] genirq: Synchronize only with single thread on free_irq()
On Thu, Jul 12, 2018 at 05:21:09PM -0500, Bjorn Helgaas wrote: > On Sat, Jun 16, 2018 at 09:25:00PM +0200, Lukas Wunner wrote: > > When pciehp is converted to threaded IRQ handling, removal of unplugged > > devices below a PCIe hotplug port happens synchronously in the IRQ > > thread. Removal of devices typically entails a call to free_irq() by > > their drivers. > > > > If those devices share their IRQ with the hotplug port, free_irq() > > deadlocks because it calls synchronize_irq() to wait for all hard IRQ > > handlers as well as all threads sharing the IRQ to finish. > > > > Actually it's sufficient to wait only for the IRQ thread of the removed > > device, so call synchronize_hardirq() to wait for all hard IRQ handlers > > to finish, but no longer for any threads. Compensate by rearranging the > > control flow in irq_wait_for_interrupt() such that the device's thread > > is allowed to run one last time after kthread_stop() has been called. > > I assume this would need to be merged along with the rest of the > series, which should probably go through the PCI tree, but I'm > definitely not qualified to review this IRQ patch. And it would need > an ack from Thomas in any case. A v2 of this patch has already been merged through the tip tree on June 24, it's in linux-next as commit 519cc8652b3a, and ISTR that I marked this patch either as "Obsoleted" or "Not Applicable" in pci-patchwork. There was no build-dependency of the succeeding patches in the series on this patch, hence merging through a different tree was possible. Thanks! Lukas
[PATCH] ACPI: button: hide unused procfs helpers
When CONFIG_PROC_FS isn't set, gcc warning this:
drivers/acpi/button.c:255:12: warning: ‘acpi_button_state_seq_show’ defined but
not used [-Wunused-function]
static int acpi_button_state_seq_show(struct seq_file *seq, void *offset)
^
fix this by adding #ifdef around it.
Signed-off-by: YueHaibing
---
drivers/acpi/button.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 2345a5e..8538e25 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -252,6 +252,7 @@ static int acpi_lid_notify_state(struct acpi_device
*device, int state)
return ret;
}
+#ifdef CONFIG_PROC_FS
static int acpi_button_state_seq_show(struct seq_file *seq, void *offset)
{
struct acpi_device *device = seq->private;
@@ -262,6 +263,7 @@ static int acpi_button_state_seq_show(struct seq_file *seq,
void *offset)
state < 0 ? "unsupported" : (state ? "open" : "closed"));
return 0;
}
+#endif
static int acpi_button_add_fs(struct acpi_device *device)
{
--
2.7.0
[PATCH] ACPI: button: hide unused procfs helpers
When CONFIG_PROC_FS isn't set, gcc warning this:
drivers/acpi/button.c:255:12: warning: ‘acpi_button_state_seq_show’ defined but
not used [-Wunused-function]
static int acpi_button_state_seq_show(struct seq_file *seq, void *offset)
^
fix this by adding #ifdef around it.
Signed-off-by: YueHaibing
---
drivers/acpi/button.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 2345a5e..8538e25 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -252,6 +252,7 @@ static int acpi_lid_notify_state(struct acpi_device
*device, int state)
return ret;
}
+#ifdef CONFIG_PROC_FS
static int acpi_button_state_seq_show(struct seq_file *seq, void *offset)
{
struct acpi_device *device = seq->private;
@@ -262,6 +263,7 @@ static int acpi_button_state_seq_show(struct seq_file *seq,
void *offset)
state < 0 ? "unsupported" : (state ? "open" : "closed"));
return 0;
}
+#endif
static int acpi_button_add_fs(struct acpi_device *device)
{
--
2.7.0
Re: [PATCH 10/18] brcmsmac: change strncpy+truncation to strlcpy
On 7/13/2018 3:25 AM, Dominique Martinet wrote: Generated by scripts/coccinelle/misc/strncpy_truncation.cocci Acked-by: Arend van Spriel Signed-off-by: Dominique Martinet --- Please see https://marc.info/?l=linux-kernel=153144450722324=2 (the first patch of the serie) for the motivation behind this patch I would prefer to have the motivation in the commit message of this patch. Regards, Arend
Re: [PATCH 10/18] brcmsmac: change strncpy+truncation to strlcpy
On 7/13/2018 3:25 AM, Dominique Martinet wrote: Generated by scripts/coccinelle/misc/strncpy_truncation.cocci Acked-by: Arend van Spriel Signed-off-by: Dominique Martinet --- Please see https://marc.info/?l=linux-kernel=153144450722324=2 (the first patch of the serie) for the motivation behind this patch I would prefer to have the motivation in the commit message of this patch. Regards, Arend
Re: [PATCH] gpio: mxc: add power management support
2018-07-13 9:11 GMT+02:00 Linus Walleij : > On Tue, Jul 10, 2018 at 9:53 AM Anson Huang wrote: > >> GPIO registers could lose context on some i.MX SoCs, >> like on i.MX7D, when enter LPSR mode, the whole SoC >> will be powered off except LPSR domain, GPIO banks >> will lose context in this case, need to restore >> the context after resume from LPSR mode. >> >> This patch adds GPIO save/restore for those necessary >> registers, and put the save/restore operations in noirq >> suspend/resume phase, since GPIO is fundamental module >> which could be used by other peripherals' resume phase. >> >> Signed-off-by: Anson Huang > > Hoping for some review on this before applying... > Fabio? Bartosz? > > Yours, > Linus Walleij I'm not familiar with these SoCs but the code looks good and clean to me. Reviewed-by: Bartosz Golaszewski
Re: [PATCH] gpio: mxc: add power management support
2018-07-13 9:11 GMT+02:00 Linus Walleij : > On Tue, Jul 10, 2018 at 9:53 AM Anson Huang wrote: > >> GPIO registers could lose context on some i.MX SoCs, >> like on i.MX7D, when enter LPSR mode, the whole SoC >> will be powered off except LPSR domain, GPIO banks >> will lose context in this case, need to restore >> the context after resume from LPSR mode. >> >> This patch adds GPIO save/restore for those necessary >> registers, and put the save/restore operations in noirq >> suspend/resume phase, since GPIO is fundamental module >> which could be used by other peripherals' resume phase. >> >> Signed-off-by: Anson Huang > > Hoping for some review on this before applying... > Fabio? Bartosz? > > Yours, > Linus Walleij I'm not familiar with these SoCs but the code looks good and clean to me. Reviewed-by: Bartosz Golaszewski
Re: [PATCH 2/4] phy: socionext: add USB3 PHY driver for UniPhier SoC
Hi,
On Wednesday 11 July 2018 05:35 PM, Kunihiko Hayashi wrote:
> On Mon, 9 Jul 2018 20:23:19 +0900 wrote:
>
>> Hi Kishon,
>> Thank you for your comments.
>>
>> On Mon, 9 Jul 2018 10:49:50 +0530 wrote:
>>
>>> Hi,
>>>
>>> On Friday 29 June 2018 02:08 PM, Kunihiko Hayashi wrote:
Add a driver for PHY interface built into USB3 controller
implemented in UniPhier SoCs.
This driver supports High-Speed PHY and Super-Speed PHY.
Signed-off-by: Kunihiko Hayashi
Signed-off-by: Motoya Tanigawa
Signed-off-by: Masami Hiramatsu
---
drivers/phy/Kconfig | 1 +
drivers/phy/Makefile| 1 +
drivers/phy/socionext/Kconfig | 12 +
drivers/phy/socionext/Makefile | 6 +
drivers/phy/socionext/phy-uniphier-usb3hs.c | 422
drivers/phy/socionext/phy-uniphier-usb3ss.c | 369
6 files changed, 811 insertions(+)
create mode 100644 drivers/phy/socionext/Kconfig
create mode 100644 drivers/phy/socionext/Makefile
create mode 100644 drivers/phy/socionext/phy-uniphier-usb3hs.c
create mode 100644 drivers/phy/socionext/phy-uniphier-usb3ss.c
>>
>> (snip...)
>>
--- /dev/null
+++ b/drivers/phy/socionext/phy-uniphier-usb3hs.c
@@ -0,0 +1,422 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * phy-uniphier-usb3hs.c - HS-PHY driver for Socionext UniPhier USB3
controller
+ * Copyright 2015-2018 Socionext Inc.
+ * Author:
+ *Kunihiko Hayashi
+ * Contributors:
+ * Motoya Tanigawa
+ * Masami Hiramatsu
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define HSPHY_CFG00x0
+#define HSPHY_CFG0_HS_I_MASK GENMASK(31, 28)
+#define HSPHY_CFG0_HSDISC_MASKGENMASK(27, 26)
+#define HSPHY_CFG0_SWING_MASK GENMASK(17, 16)
+#define HSPHY_CFG0_SEL_T_MASK GENMASK(15, 12)
+#define HSPHY_CFG0_RTERM_MASK GENMASK(7, 6)
+#define HSPHY_CFG0_TRIMMASK (HSPHY_CFG0_HS_I_MASK \
+ | HSPHY_CFG0_SEL_T_MASK \
+ | HSPHY_CFG0_RTERM_MASK)
+
+#define HSPHY_CFG10x4
+#define HSPHY_CFG1_DAT_EN BIT(29)
+#define HSPHY_CFG1_ADR_EN BIT(28)
+#define HSPHY_CFG1_ADR_MASK GENMASK(27, 16)
+#define HSPHY_CFG1_DAT_MASK GENMASK(23, 16)
+
+#define MAX_CLKS 3
+#define MAX_RSTS 2
+#define MAX_PHY_PARAMS1
+
+struct uniphier_u3hsphy_param {
+ u32 addr;
+ u32 mask;
+ u32 val;
+};
>>>
>>> I'd like to avoid configure the PHY this way, since it's impossible to know
>>> which register is being configured.
>>
>
> This way might be misunderstood.
> These HS-PHY and SS-PHY have "internal" registers, which are not
> memory-mapped.
>
> And to access these internal registers, we need to specify the number
> corresponding to the register.
>
> The "addr" in "uniphier_u3hsphy_param" is just the number of the register.
> The "mask" shows a bitfield of the register, that means one of PHY parameters.
> The "value" shows a parameter value to set to the bitfield.
What does each of these bitfields represent? Which PHY parameter does it
configure. Are they really PHY parameters or they also configure clocks/mux
etc? I would like the configurations to be more descriptive so that we get to
know at least what's getting configured here.
Thanks
Kishon
Re: [PATCH 2/4] phy: socionext: add USB3 PHY driver for UniPhier SoC
Hi,
On Wednesday 11 July 2018 05:35 PM, Kunihiko Hayashi wrote:
> On Mon, 9 Jul 2018 20:23:19 +0900 wrote:
>
>> Hi Kishon,
>> Thank you for your comments.
>>
>> On Mon, 9 Jul 2018 10:49:50 +0530 wrote:
>>
>>> Hi,
>>>
>>> On Friday 29 June 2018 02:08 PM, Kunihiko Hayashi wrote:
Add a driver for PHY interface built into USB3 controller
implemented in UniPhier SoCs.
This driver supports High-Speed PHY and Super-Speed PHY.
Signed-off-by: Kunihiko Hayashi
Signed-off-by: Motoya Tanigawa
Signed-off-by: Masami Hiramatsu
---
drivers/phy/Kconfig | 1 +
drivers/phy/Makefile| 1 +
drivers/phy/socionext/Kconfig | 12 +
drivers/phy/socionext/Makefile | 6 +
drivers/phy/socionext/phy-uniphier-usb3hs.c | 422
drivers/phy/socionext/phy-uniphier-usb3ss.c | 369
6 files changed, 811 insertions(+)
create mode 100644 drivers/phy/socionext/Kconfig
create mode 100644 drivers/phy/socionext/Makefile
create mode 100644 drivers/phy/socionext/phy-uniphier-usb3hs.c
create mode 100644 drivers/phy/socionext/phy-uniphier-usb3ss.c
>>
>> (snip...)
>>
--- /dev/null
+++ b/drivers/phy/socionext/phy-uniphier-usb3hs.c
@@ -0,0 +1,422 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * phy-uniphier-usb3hs.c - HS-PHY driver for Socionext UniPhier USB3
controller
+ * Copyright 2015-2018 Socionext Inc.
+ * Author:
+ *Kunihiko Hayashi
+ * Contributors:
+ * Motoya Tanigawa
+ * Masami Hiramatsu
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define HSPHY_CFG00x0
+#define HSPHY_CFG0_HS_I_MASK GENMASK(31, 28)
+#define HSPHY_CFG0_HSDISC_MASKGENMASK(27, 26)
+#define HSPHY_CFG0_SWING_MASK GENMASK(17, 16)
+#define HSPHY_CFG0_SEL_T_MASK GENMASK(15, 12)
+#define HSPHY_CFG0_RTERM_MASK GENMASK(7, 6)
+#define HSPHY_CFG0_TRIMMASK (HSPHY_CFG0_HS_I_MASK \
+ | HSPHY_CFG0_SEL_T_MASK \
+ | HSPHY_CFG0_RTERM_MASK)
+
+#define HSPHY_CFG10x4
+#define HSPHY_CFG1_DAT_EN BIT(29)
+#define HSPHY_CFG1_ADR_EN BIT(28)
+#define HSPHY_CFG1_ADR_MASK GENMASK(27, 16)
+#define HSPHY_CFG1_DAT_MASK GENMASK(23, 16)
+
+#define MAX_CLKS 3
+#define MAX_RSTS 2
+#define MAX_PHY_PARAMS1
+
+struct uniphier_u3hsphy_param {
+ u32 addr;
+ u32 mask;
+ u32 val;
+};
>>>
>>> I'd like to avoid configure the PHY this way, since it's impossible to know
>>> which register is being configured.
>>
>
> This way might be misunderstood.
> These HS-PHY and SS-PHY have "internal" registers, which are not
> memory-mapped.
>
> And to access these internal registers, we need to specify the number
> corresponding to the register.
>
> The "addr" in "uniphier_u3hsphy_param" is just the number of the register.
> The "mask" shows a bitfield of the register, that means one of PHY parameters.
> The "value" shows a parameter value to set to the bitfield.
What does each of these bitfields represent? Which PHY parameter does it
configure. Are they really PHY parameters or they also configure clocks/mux
etc? I would like the configurations to be more descriptive so that we get to
know at least what's getting configured here.
Thanks
Kishon
Re: [PATCH v3] time: Fix incorrect sleeptime injection when suspend fails
Hi John,
Thanks for your response
Please find my comments inline.
On 7/11/2018 1:43 AM, John Stultz wrote:
On Fri, Jul 6, 2018 at 6:17 AM, Mukesh Ojha wrote:
Currently, there exists a corner case assuming when there is
only one clocksource e.g RTC, and system failed to go to
suspend mode. While resume rtc_resume() injects the sleeptime
as timekeeping_rtc_skipresume() returned 'false' (default value
of sleeptime_injected) due to which we can see mismatch in
timestamps.
This issue can also come in a system where more than one
clocksource are present and very first suspend fails.
Fix this by handling `sleeptime_injected` flag properly.
Success case:
{sleeptime_injected=false}
rtc_suspend() => timekeeping_suspend() => timekeeping_resume() =>
(sleeptime injected)
rtc_resume()
Failure case:
{failure in sleep path} {sleeptime_injected=false}
rtc_suspend() => rtc_resume()
sleeptime injected again which was not required as the suspend failed)
Originally-by: Thomas Gleixner
Signed-off-by: Mukesh Ojha
---
Changes in v3:
* Updated commit subject and description.
* Updated the patch as per the fix given by Thomas Gleixner.
Changes in v2:
* Updated the commit text.
* Removed extra variable and used the earlier static
variable 'sleeptime_injected'.
kernel/time/timekeeping.c | 21 ++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 4786df9..32ae9ae 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1510,8 +1510,20 @@ void __weak read_boot_clock64(struct timespec64 *ts)
ts->tv_nsec = 0;
}
-/* Flag for if timekeeping_resume() has injected sleeptime */
-static bool sleeptime_injected;
+/*
+ * Flag reflecting whether timekeeping_resume() has injected sleeptime.
+ *
+ * The flag starts of true and is only cleared when a suspend reaches
+ * timekeeping_suspend(), timekeeping_resume() sets it when the timekeeper
+ * clocksource is not stopping across suspend and has been used to update
+ * sleep time. If the timekeeper clocksource has stopped then the flag
+ * stays false and is used by the RTC resume code to decide whether sleep
+ * time must be injected and if so the flag gets set then.
+ *
+ * If a suspend fails before reaching timekeeping_resume() then the flag
+ * stays true and prevents erroneous sleeptime injection.
+ */
+static bool sleeptime_injected = true;
I worry this upside-down logic is too subtle to be easily reasoned
about, and will just lead to future mistakes.
Can we instead call this "suspend_timing_needed" and only set it to
true when we don't inject any sleep time on resume?
I did not get your point "only set it to true when we don't inject any
sleep time on resume? "
How do we know this ?
This question itself depends on the "sleeptime_injected" if it is true
means no need to inject else need to inject.
Also, we need to make this variable back and forth true, false; suspends
path ensures it to make it false.
Just to add here there are already two path where `sleeptime_injected`
set to true one from
NON-stop clocksource and other from persistant clock and the RTC one was
missing, so we are adding
with this patch.
Cheers,
-Mukesh
Re: [PATCH v3] time: Fix incorrect sleeptime injection when suspend fails
Hi John,
Thanks for your response
Please find my comments inline.
On 7/11/2018 1:43 AM, John Stultz wrote:
On Fri, Jul 6, 2018 at 6:17 AM, Mukesh Ojha wrote:
Currently, there exists a corner case assuming when there is
only one clocksource e.g RTC, and system failed to go to
suspend mode. While resume rtc_resume() injects the sleeptime
as timekeeping_rtc_skipresume() returned 'false' (default value
of sleeptime_injected) due to which we can see mismatch in
timestamps.
This issue can also come in a system where more than one
clocksource are present and very first suspend fails.
Fix this by handling `sleeptime_injected` flag properly.
Success case:
{sleeptime_injected=false}
rtc_suspend() => timekeeping_suspend() => timekeeping_resume() =>
(sleeptime injected)
rtc_resume()
Failure case:
{failure in sleep path} {sleeptime_injected=false}
rtc_suspend() => rtc_resume()
sleeptime injected again which was not required as the suspend failed)
Originally-by: Thomas Gleixner
Signed-off-by: Mukesh Ojha
---
Changes in v3:
* Updated commit subject and description.
* Updated the patch as per the fix given by Thomas Gleixner.
Changes in v2:
* Updated the commit text.
* Removed extra variable and used the earlier static
variable 'sleeptime_injected'.
kernel/time/timekeeping.c | 21 ++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 4786df9..32ae9ae 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1510,8 +1510,20 @@ void __weak read_boot_clock64(struct timespec64 *ts)
ts->tv_nsec = 0;
}
-/* Flag for if timekeeping_resume() has injected sleeptime */
-static bool sleeptime_injected;
+/*
+ * Flag reflecting whether timekeeping_resume() has injected sleeptime.
+ *
+ * The flag starts of true and is only cleared when a suspend reaches
+ * timekeeping_suspend(), timekeeping_resume() sets it when the timekeeper
+ * clocksource is not stopping across suspend and has been used to update
+ * sleep time. If the timekeeper clocksource has stopped then the flag
+ * stays false and is used by the RTC resume code to decide whether sleep
+ * time must be injected and if so the flag gets set then.
+ *
+ * If a suspend fails before reaching timekeeping_resume() then the flag
+ * stays true and prevents erroneous sleeptime injection.
+ */
+static bool sleeptime_injected = true;
I worry this upside-down logic is too subtle to be easily reasoned
about, and will just lead to future mistakes.
Can we instead call this "suspend_timing_needed" and only set it to
true when we don't inject any sleep time on resume?
I did not get your point "only set it to true when we don't inject any
sleep time on resume? "
How do we know this ?
This question itself depends on the "sleeptime_injected" if it is true
means no need to inject else need to inject.
Also, we need to make this variable back and forth true, false; suspends
path ensures it to make it false.
Just to add here there are already two path where `sleeptime_injected`
set to true one from
NON-stop clocksource and other from persistant clock and the RTC one was
missing, so we are adding
with this patch.
Cheers,
-Mukesh
Re: [PATCH] gpio: mxc: add power management support
On Tue, Jul 10, 2018 at 9:53 AM Anson Huang wrote: > GPIO registers could lose context on some i.MX SoCs, > like on i.MX7D, when enter LPSR mode, the whole SoC > will be powered off except LPSR domain, GPIO banks > will lose context in this case, need to restore > the context after resume from LPSR mode. > > This patch adds GPIO save/restore for those necessary > registers, and put the save/restore operations in noirq > suspend/resume phase, since GPIO is fundamental module > which could be used by other peripherals' resume phase. > > Signed-off-by: Anson Huang Hoping for some review on this before applying... Fabio? Bartosz? Yours, Linus Walleij
Re: [PATCH] gpio: mxc: add power management support
On Tue, Jul 10, 2018 at 9:53 AM Anson Huang wrote: > GPIO registers could lose context on some i.MX SoCs, > like on i.MX7D, when enter LPSR mode, the whole SoC > will be powered off except LPSR domain, GPIO banks > will lose context in this case, need to restore > the context after resume from LPSR mode. > > This patch adds GPIO save/restore for those necessary > registers, and put the save/restore operations in noirq > suspend/resume phase, since GPIO is fundamental module > which could be used by other peripherals' resume phase. > > Signed-off-by: Anson Huang Hoping for some review on this before applying... Fabio? Bartosz? Yours, Linus Walleij
Re: [PATCH] gpio: aspeed: fix compile testing warning
On Tue, Jul 10, 2018 at 1:53 AM Benjamin Herrenschmidt
wrote:
> On Mon, 2018-07-09 at 16:56 +0200, Arnd Bergmann wrote:
> > Gcc cannot always see that BUG_ON(1) is guaranteed to not
> > return, so we get a warning message in some configurations:
> >
> > drivers/gpio/gpio-aspeed.c: In function 'bank_reg':
> > drivers/gpio/gpio-aspeed.c:244:1: error: control reaches end of non-void
> > function [-Werror=return-type]
> >
> > Using a plain BUG() is easier here and avoids the problem.
> >
> > Fixes: 44ddf559d579 ("gpio: aspeed: Rework register type accessors")
> > Signed-off-by: Arnd Bergmann
>
> Acked-by: Benjamin Herrenschmidt
>
> Linus, can you plonk that on top of the patches in that topic branch
> you created ?
I put it on top of my devel branch where I merged in the topic
branch.
As it's a fringe thing anyways I don't think we need to recreate the
topic branch for this.
Yours,
Linus Walleij
Re: [PATCH] gpio: aspeed: fix compile testing warning
On Tue, Jul 10, 2018 at 1:53 AM Benjamin Herrenschmidt
wrote:
> On Mon, 2018-07-09 at 16:56 +0200, Arnd Bergmann wrote:
> > Gcc cannot always see that BUG_ON(1) is guaranteed to not
> > return, so we get a warning message in some configurations:
> >
> > drivers/gpio/gpio-aspeed.c: In function 'bank_reg':
> > drivers/gpio/gpio-aspeed.c:244:1: error: control reaches end of non-void
> > function [-Werror=return-type]
> >
> > Using a plain BUG() is easier here and avoids the problem.
> >
> > Fixes: 44ddf559d579 ("gpio: aspeed: Rework register type accessors")
> > Signed-off-by: Arnd Bergmann
>
> Acked-by: Benjamin Herrenschmidt
>
> Linus, can you plonk that on top of the patches in that topic branch
> you created ?
I put it on top of my devel branch where I merged in the topic
branch.
As it's a fringe thing anyways I don't think we need to recreate the
topic branch for this.
Yours,
Linus Walleij
Re: [PATCH] gpio: aspeed: fix compile testing warning
On Mon, Jul 9, 2018 at 4:56 PM Arnd Bergmann wrote:
> Gcc cannot always see that BUG_ON(1) is guaranteed to not
> return, so we get a warning message in some configurations:
>
> drivers/gpio/gpio-aspeed.c: In function 'bank_reg':
> drivers/gpio/gpio-aspeed.c:244:1: error: control reaches end of non-void
> function [-Werror=return-type]
>
> Using a plain BUG() is easier here and avoids the problem.
>
> Fixes: 44ddf559d579 ("gpio: aspeed: Rework register type accessors")
> Signed-off-by: Arnd Bergmann
Patch applied with Benjamin's ACK.
Yours,
Linus Walleij
Re: [PATCH] gpio: aspeed: fix compile testing warning
On Mon, Jul 9, 2018 at 4:56 PM Arnd Bergmann wrote:
> Gcc cannot always see that BUG_ON(1) is guaranteed to not
> return, so we get a warning message in some configurations:
>
> drivers/gpio/gpio-aspeed.c: In function 'bank_reg':
> drivers/gpio/gpio-aspeed.c:244:1: error: control reaches end of non-void
> function [-Werror=return-type]
>
> Using a plain BUG() is easier here and avoids the problem.
>
> Fixes: 44ddf559d579 ("gpio: aspeed: Rework register type accessors")
> Signed-off-by: Arnd Bergmann
Patch applied with Benjamin's ACK.
Yours,
Linus Walleij
