Re: [PATCH 3/3] mfd: tqmx86: add support for TQMxE40M
On Thu, 2021-04-01 at 09:04 +0100, Lee Jones wrote: > On Wed, 31 Mar 2021, Andy Shevchenko wrote: > > > On Wed, Mar 31, 2021 at 4:33 PM Matthias Schiffer > > wrote: > > > On Wed, 2021-03-31 at 15:37 +0300, Andy Shevchenko wrote: > > > > On Wed, Mar 31, 2021 at 2:38 PM Matthias Schiffer > > > > wrote: > > > > ... > > > > > > > + return 24000; > > > > > > > > AFAICS it will return 24 MHz for "Unknown" board. Is it okay to be so > > > > brave? > > > > > > As noted in the commit message, our hardware developers intend to use > > > 24 MHz for all future x86 SoMs. > > > > What may go wrong in the future?.. (rhetorical question, obviously) > > My preference would be to be explicit. > > Rather than support boards implicitly i.e. by accident. > How about logging a warning for unknown boards, but still returning 24 MHz?
Re: [PATCH 1/3] gpio: tqmx86: really make IRQ optional
On Wed, 2021-03-31 at 15:39 +0300, Andy Shevchenko wrote: > On Wed, Mar 31, 2021 at 3:37 PM Matthias Schiffer > wrote: > > On Wed, 2021-03-31 at 15:29 +0300, Andy Shevchenko wrote: > > > On Wed, Mar 31, 2021 at 2:37 PM Matthias Schiffer > > > wrote: > > ... > > > > > - irq = platform_get_irq(pdev, 0); > > > > - if (irq < 0) > > > > + irq = platform_get_irq_optional(pdev, 0); > > > > + if (irq < 0 && irq != -ENXIO) > > > > return irq; > > > > > > This is a dead code now. I suggest you to do the opposite, i.e. > > > if (irq < 0) > > > irq = 0; > > > > I don't understand which part of the code is dead now. I assume the > > `return irq` case is still useful for unexpected errors, or things like > > EPROBE_DEFER? I'm not sure if EPROBE_DEFER is relevant for this driver, > > but just ignoring the error code completely doesn't seem right to me. > > platform_get_irq() AFAIK won't ever return such a code. > So, basically your conditional is always false. > > I would like to see the code path which makes my comment wrong. > EPROBE_DEFER appears a few times in platform_get_irq_optional() (drivers/base/platform.c), but it's possible that this is only relevant for OF-based platforms and not x86.
Re: [PATCH 3/3] mfd: tqmx86: add support for TQMxE40M
On Wed, 2021-03-31 at 15:37 +0300, Andy Shevchenko wrote:
> On Wed, Mar 31, 2021 at 2:38 PM Matthias Schiffer
> wrote:
> >
> > All future TQMx86 SoMs will use a 24MHz LPC clock, so we can use that as
> > a default instead of listing each new module individually.
>
> ...
>
> > case TQMX86_REG_BOARD_ID_90UC:
> > return "TQMx90UC";
> > + case TQMX86_REG_BOARD_ID_E40M:
> > + return "TQMxE40M";
> > default:
> > return "Unknown";
> > }
> > @@ -138,12 +141,6 @@ static const char *tqmx86_board_id_to_name(u8 board_id)
> > static int tqmx86_board_id_to_clk_rate(u8 board_id)
> > {
> > switch (board_id) {
> > - case TQMX86_REG_BOARD_ID_50UC:
> > - case TQMX86_REG_BOARD_ID_60EB:
> > - case TQMX86_REG_BOARD_ID_70EB:
> > - case TQMX86_REG_BOARD_ID_80UC:
> > - case TQMX86_REG_BOARD_ID_90UC:
> > - return 24000;
> > case TQMX86_REG_BOARD_ID_E39M:
> > case TQMX86_REG_BOARD_ID_E39C:
> > case TQMX86_REG_BOARD_ID_E39x:
> > @@ -152,7 +149,7 @@ static int tqmx86_board_id_to_clk_rate(u8 board_id)
> > case TQMX86_REG_BOARD_ID_E38C:
> > return 33000;
> > default:
> > - return 0;
> > + return 24000;
>
> AFAICS it will return 24 MHz for "Unknown" board. Is it okay to be so brave?
As noted in the commit message, our hardware developers intend to use
24 MHz for all future x86 SoMs.
Re: [PATCH 2/3] mfd: tqmx86: clear GPIO IRQ resource when no IRQ is set
On Wed, 2021-03-31 at 15:35 +0300, Andy Shevchenko wrote:
> On Wed, Mar 31, 2021 at 2:39 PM Matthias Schiffer
> wrote:
> >
> > The driver was registering IRQ 0 when no IRQ was set. This leads to
> > warnings with newer kernels.
> >
> > Clear the resource flags, so no resource is registered at all in this
> > case.
>
> ...
>
> > /* Assumes the IRQ resource is first. */
> > tqmx_gpio_resources[0].start = gpio_irq;
> > + } else {
> > + tqmx_gpio_resources[0].flags = 0;
>
> Please set IORESOURCE_DISABLED flag in the initial structure instead.
Is there any documentation for the correct usage of this flag? I think
I tried IORESOURCE_DISABLED originally, but it didn't have any effect
(platform_get_irq() ignored the flag and returned the resource
anyways). I might misremember though, I originally wrote the series
some time ago.
>
> > }
>
>
Re: (EXT) Re: [PATCH 1/3] gpio: tqmx86: really make IRQ optional
On Wed, 2021-03-31 at 15:29 +0300, Andy Shevchenko wrote:
> On Wed, Mar 31, 2021 at 2:37 PM Matthias Schiffer
> wrote:
> >
> > The tqmx86 MFD driver was passing IRQ 0 for "no IRQ" in the past. This
> > causes warnings with newer kernels.
> >
> > Prepare the gpio-tqmx86 driver for the fixed MFD driver by handling a
> > missing IRQ properly.
>
> ...
>
> > - irq = platform_get_irq(pdev, 0);
> > - if (irq < 0)
> > + irq = platform_get_irq_optional(pdev, 0);
> > + if (irq < 0 && irq != -ENXIO)
> > return irq;
>
> This is a dead code now. I suggest you to do the opposite, i.e.
> if (irq < 0)
> irq = 0;
I don't understand which part of the code is dead now. I assume the
`return irq` case is still useful for unexpected errors, or things like
EPROBE_DEFER? I'm not sure if EPROBE_DEFER is relevant for this driver,
but just ignoring the error code completely doesn't seem right to me.
>
> In such a case below change is not required.
>
> ...
>
> > - if (irq) {
> > + if (irq > 0) {
> > struct irq_chip *irq_chip = >irq_chip;
> > u8 irq_status;
>
>
[PATCH 1/3] gpio: tqmx86: really make IRQ optional
The tqmx86 MFD driver was passing IRQ 0 for "no IRQ" in the past. This
causes warnings with newer kernels.
Prepare the gpio-tqmx86 driver for the fixed MFD driver by handling a
missing IRQ properly.
Signed-off-by: Matthias Schiffer
---
drivers/gpio/gpio-tqmx86.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index 5022e0ad0fae..0f5d17f343f1 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -238,8 +238,8 @@ static int tqmx86_gpio_probe(struct platform_device *pdev)
struct resource *res;
int ret, irq;
- irq = platform_get_irq(pdev, 0);
- if (irq < 0)
+ irq = platform_get_irq_optional(pdev, 0);
+ if (irq < 0 && irq != -ENXIO)
return irq;
res = platform_get_resource(pdev, IORESOURCE_IO, 0);
@@ -278,7 +278,7 @@ static int tqmx86_gpio_probe(struct platform_device *pdev)
pm_runtime_enable(>dev);
- if (irq) {
+ if (irq > 0) {
struct irq_chip *irq_chip = >irq_chip;
u8 irq_status;
--
2.17.1
[PATCH 0/3] tqmx86: TQMxE40M support
This fixes a bug in the IRQ setup of the tqmx86 MFD / GPIO drivers and adds support for our upcoming Elkhart Lake module TQMxE40M (as well as future SoMs). As patch 2 depends on patch 1, it would make sense to push the whole series through the same tree. Matthias Schiffer (3): gpio: tqmx86: really make IRQ optional mfd: tqmx86: clear GPIO IRQ resource when no IRQ is set mfd: tqmx86: add support for TQMxE40M drivers/gpio/gpio-tqmx86.c | 6 +++--- drivers/mfd/tqmx86.c | 13 ++--- 2 files changed, 9 insertions(+), 10 deletions(-) -- 2.17.1
[PATCH 3/3] mfd: tqmx86: add support for TQMxE40M
All future TQMx86 SoMs will use a 24MHz LPC clock, so we can use that as
a default instead of listing each new module individually.
Signed-off-by: Matthias Schiffer
---
drivers/mfd/tqmx86.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/mfd/tqmx86.c b/drivers/mfd/tqmx86.c
index 732013f40e4e..1d5cebc4e72b 100644
--- a/drivers/mfd/tqmx86.c
+++ b/drivers/mfd/tqmx86.c
@@ -36,6 +36,7 @@
#define TQMX86_REG_BOARD_ID_70EB 8
#define TQMX86_REG_BOARD_ID_80UC 9
#define TQMX86_REG_BOARD_ID_90UC 10
+#define TQMX86_REG_BOARD_ID_E40M 12
#define TQMX86_REG_BOARD_REV 0x21
#define TQMX86_REG_IO_EXT_INT 0x26
#define TQMX86_REG_IO_EXT_INT_NONE 0
@@ -130,6 +131,8 @@ static const char *tqmx86_board_id_to_name(u8 board_id)
return "TQMx80UC";
case TQMX86_REG_BOARD_ID_90UC:
return "TQMx90UC";
+ case TQMX86_REG_BOARD_ID_E40M:
+ return "TQMxE40M";
default:
return "Unknown";
}
@@ -138,12 +141,6 @@ static const char *tqmx86_board_id_to_name(u8 board_id)
static int tqmx86_board_id_to_clk_rate(u8 board_id)
{
switch (board_id) {
- case TQMX86_REG_BOARD_ID_50UC:
- case TQMX86_REG_BOARD_ID_60EB:
- case TQMX86_REG_BOARD_ID_70EB:
- case TQMX86_REG_BOARD_ID_80UC:
- case TQMX86_REG_BOARD_ID_90UC:
- return 24000;
case TQMX86_REG_BOARD_ID_E39M:
case TQMX86_REG_BOARD_ID_E39C:
case TQMX86_REG_BOARD_ID_E39x:
@@ -152,7 +149,7 @@ static int tqmx86_board_id_to_clk_rate(u8 board_id)
case TQMX86_REG_BOARD_ID_E38C:
return 33000;
default:
- return 0;
+ return 24000;
}
}
--
2.17.1
[PATCH 2/3] mfd: tqmx86: clear GPIO IRQ resource when no IRQ is set
The driver was registering IRQ 0 when no IRQ was set. This leads to
warnings with newer kernels.
Clear the resource flags, so no resource is registered at all in this
case.
Signed-off-by: Matthias Schiffer
---
drivers/mfd/tqmx86.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/mfd/tqmx86.c b/drivers/mfd/tqmx86.c
index f08b6a4c..732013f40e4e 100644
--- a/drivers/mfd/tqmx86.c
+++ b/drivers/mfd/tqmx86.c
@@ -209,6 +209,8 @@ static int tqmx86_probe(struct platform_device *pdev)
/* Assumes the IRQ resource is first. */
tqmx_gpio_resources[0].start = gpio_irq;
+ } else {
+ tqmx_gpio_resources[0].flags = 0;
}
ocores_platfom_data.clock_khz = tqmx86_board_id_to_clk_rate(board_id);
--
2.17.1
[PATCH net v2] net: l2tp: reduce log level of messages in receive path, add counter instead
Commit 5ee759cda51b ("l2tp: use standard API for warning log messages")
changed a number of warnings about invalid packets in the receive path
so that they are always shown, instead of only when a special L2TP debug
flag is set. Even with rate limiting these warnings can easily cause
significant log spam - potentially triggered by a malicious party
sending invalid packets on purpose.
In addition these warnings were noticed by projects like Tunneldigger [1],
which uses L2TP for its data path, but implements its own control
protocol (which is sufficiently different from L2TP data packets that it
would always be passed up to userspace even with future extensions of
L2TP).
Some of the warnings were already redundant, as l2tp_stats has a counter
for these packets. This commit adds one additional counter for invalid
packets that are passed up to userspace. Packets with unknown session are
not counted as invalid, as there is nothing wrong with the format of
these packets.
With the additional counter, all of these messages are either redundant
or benign, so we reduce them to pr_debug_ratelimited().
[1] https://github.com/wlanslovenija/tunneldigger/issues/160
Fixes: 5ee759cda51b ("l2tp: use standard API for warning log messages")
Signed-off-by: Matthias Schiffer
---
v2:
- Add counter for invalid packets
- Reduce loglevel of more messages that can be abused for log spam
include/uapi/linux/l2tp.h | 1 +
net/l2tp/l2tp_core.c | 41 +--
net/l2tp/l2tp_core.h | 1 +
net/l2tp/l2tp_netlink.c | 6 ++
4 files changed, 30 insertions(+), 19 deletions(-)
diff --git a/include/uapi/linux/l2tp.h b/include/uapi/linux/l2tp.h
index 30c80d5ba4bf..bab8c9708611 100644
--- a/include/uapi/linux/l2tp.h
+++ b/include/uapi/linux/l2tp.h
@@ -145,6 +145,7 @@ enum {
L2TP_ATTR_RX_ERRORS,/* u64 */
L2TP_ATTR_STATS_PAD,
L2TP_ATTR_RX_COOKIE_DISCARDS, /* u64 */
+ L2TP_ATTR_RX_INVALID, /* u64 */
__L2TP_ATTR_STATS_MAX,
};
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 7be5103ff2a8..8ed889f44d23 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -649,9 +649,9 @@ void l2tp_recv_common(struct l2tp_session *session, struct
sk_buff *skb,
/* Parse and check optional cookie */
if (session->peer_cookie_len > 0) {
if (memcmp(ptr, >peer_cookie[0],
session->peer_cookie_len)) {
- pr_warn_ratelimited("%s: cookie mismatch (%u/%u).
Discarding.\n",
- tunnel->name, tunnel->tunnel_id,
- session->session_id);
+ pr_debug_ratelimited("%s: cookie mismatch (%u/%u).
Discarding.\n",
+tunnel->name, tunnel->tunnel_id,
+session->session_id);
atomic_long_inc(>stats.rx_cookie_discards);
goto discard;
}
@@ -702,8 +702,8 @@ void l2tp_recv_common(struct l2tp_session *session, struct
sk_buff *skb,
* If user has configured mandatory sequence numbers, discard.
*/
if (session->recv_seq) {
- pr_warn_ratelimited("%s: recv data has no seq numbers
when required. Discarding.\n",
- session->name);
+ pr_debug_ratelimited("%s: recv data has no seq numbers
when required. Discarding.\n",
+session->name);
atomic_long_inc(>stats.rx_seq_discards);
goto discard;
}
@@ -718,8 +718,8 @@ void l2tp_recv_common(struct l2tp_session *session, struct
sk_buff *skb,
session->send_seq = 0;
l2tp_session_set_header_len(session, tunnel->version);
} else if (session->send_seq) {
- pr_warn_ratelimited("%s: recv data has no seq numbers
when required. Discarding.\n",
- session->name);
+ pr_debug_ratelimited("%s: recv data has no seq numbers
when required. Discarding.\n",
+session->name);
atomic_long_inc(>stats.rx_seq_discards);
goto discard;
}
@@ -809,9 +809,9 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel,
struct sk_buff *skb)
/* Short packet? */
if (!pskb_may_pull(skb, L2TP_HDR_SIZE_MAX)) {
- pr_warn_ratelimited("%s: recv short packet (len=%d)\n",
- tunnel->name, skb->len);
- goto error;
+
[PATCH v2 2/3] power: supply: bq27xxx: fix power_avg for newer ICs
On all newer bq27xxx ICs, the AveragePower register contains a signed
value; in addition to handling the raw value as unsigned, the driver
code also didn't convert it to µW as expected.
At least for the BQ28Z610, the reference manual incorrectly states that
the value is in units of 1mW and not 10mW. I have no way of knowing
whether the manuals of other supported ICs contain the same error, or if
there are models that actually use 1mW. At least, the new code shouldn't
be *less* correct than the old version for any device.
power_avg is removed from the cache structure, se we don't have to
extend it to store both a signed value and an error code. Always getting
an up-to-date value may be desirable anyways, as it avoids inconsistent
current and power readings when switching between charging and
discharging.
Signed-off-by: Matthias Schiffer
---
v2: fixed units in commit message
drivers/power/supply/bq27xxx_battery.c | 51 ++
include/linux/power/bq27xxx_battery.h | 1 -
2 files changed, 27 insertions(+), 25 deletions(-)
diff --git a/drivers/power/supply/bq27xxx_battery.c
b/drivers/power/supply/bq27xxx_battery.c
index cb6ebd2f905e..20e1dc8a87cf 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1661,27 +1661,6 @@ static int bq27xxx_battery_read_time(struct
bq27xxx_device_info *di, u8 reg)
return tval * 60;
}
-/*
- * Read an average power register.
- * Return < 0 if something fails.
- */
-static int bq27xxx_battery_read_pwr_avg(struct bq27xxx_device_info *di)
-{
- int tval;
-
- tval = bq27xxx_read(di, BQ27XXX_REG_AP, false);
- if (tval < 0) {
- dev_err(di->dev, "error reading average power register %02x:
%d\n",
- BQ27XXX_REG_AP, tval);
- return tval;
- }
-
- if (di->opts & BQ27XXX_O_ZERO)
- return (tval * BQ27XXX_POWER_CONSTANT) / BQ27XXX_RS;
- else
- return tval;
-}
-
/*
* Returns true if a battery over temperature condition is detected
*/
@@ -1769,8 +1748,6 @@ void bq27xxx_battery_update(struct bq27xxx_device_info
*di)
}
if (di->regs[BQ27XXX_REG_CYCT] != INVALID_REG_ADDR)
cache.cycle_count = bq27xxx_battery_read_cyct(di);
- if (di->regs[BQ27XXX_REG_AP] != INVALID_REG_ADDR)
- cache.power_avg = bq27xxx_battery_read_pwr_avg(di);
/* We only have to read charge design full once */
if (di->charge_design_full <= 0)
@@ -1833,6 +1810,32 @@ static int bq27xxx_battery_current(struct
bq27xxx_device_info *di,
return 0;
}
+/*
+ * Get the average power in µW
+ * Return < 0 if something fails.
+ */
+static int bq27xxx_battery_pwr_avg(struct bq27xxx_device_info *di,
+ union power_supply_propval *val)
+{
+ int power;
+
+ power = bq27xxx_read(di, BQ27XXX_REG_AP, false);
+ if (power < 0) {
+ dev_err(di->dev,
+ "error reading average power register %02x: %d\n",
+ BQ27XXX_REG_AP, power);
+ return power;
+ }
+
+ if (di->opts & BQ27XXX_O_ZERO)
+ val->intval = (power * BQ27XXX_POWER_CONSTANT) / BQ27XXX_RS;
+ else
+ /* Other gauges return a signed value in units of 10mW */
+ val->intval = (int)((s16)power) * 1;
+
+ return 0;
+}
+
static int bq27xxx_battery_status(struct bq27xxx_device_info *di,
union power_supply_propval *val)
{
@@ -2020,7 +2023,7 @@ static int bq27xxx_battery_get_property(struct
power_supply *psy,
ret = bq27xxx_simple_value(di->cache.energy, val);
break;
case POWER_SUPPLY_PROP_POWER_AVG:
- ret = bq27xxx_simple_value(di->cache.power_avg, val);
+ ret = bq27xxx_battery_pwr_avg(di, val);
break;
case POWER_SUPPLY_PROP_HEALTH:
ret = bq27xxx_simple_value(di->cache.health, val);
diff --git a/include/linux/power/bq27xxx_battery.h
b/include/linux/power/bq27xxx_battery.h
index 111a40d0d3d5..8d5f4f40fb41 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -53,7 +53,6 @@ struct bq27xxx_reg_cache {
int capacity;
int energy;
int flags;
- int power_avg;
int health;
};
--
2.17.1
[PATCH v2 3/3] power: supply: bq27xxx: make status more robust
There are multiple issues in bq27xxx_battery_status():
- On BQ28Q610 is was observed that the "full" flag may be set even while
the battery is charging or discharging. With the current logic to make
"full" override everything else, it look a very long time (>20min) for
the status to change from "full" to "discharging" after unplugging the
supply on a device with low power consumption
- The POWER_SUPPLY_STATUS_NOT_CHARGING check depends on
power_supply_am_i_supplied(), which will not work when the supply
doesn't exist as a separate device known to Linux
We can solve both issues by deriving the status from the current instead
of the flags field. The flags are now only used to distinguish "full"
from "not charging", and to determine the sign of the current on
BQ27XXX_O_ZERO devices.
Signed-off-by: Matthias Schiffer
---
v2: no changes
drivers/power/supply/bq27xxx_battery.c | 88 +-
1 file changed, 43 insertions(+), 45 deletions(-)
diff --git a/drivers/power/supply/bq27xxx_battery.c
b/drivers/power/supply/bq27xxx_battery.c
index 20e1dc8a87cf..b62a8cfd9d09 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1777,14 +1777,27 @@ static void bq27xxx_battery_poll(struct work_struct
*work)
schedule_delayed_work(>work, poll_interval * HZ);
}
+static bool bq27xxx_battery_is_full(struct bq27xxx_device_info *di, int flags)
+{
+ if (di->opts & BQ27XXX_O_ZERO)
+ return (flags & BQ27000_FLAG_FC);
+ else if (di->opts & BQ27Z561_O_BITS)
+ return (flags & BQ27Z561_FLAG_FC);
+ else
+ return (flags & BQ27XXX_FLAG_FC);
+}
+
/*
- * Return the battery average current in µA
+ * Return the battery average current in µA and the status
* Note that current can be negative signed as well
* Or 0 if something fails.
*/
-static int bq27xxx_battery_current(struct bq27xxx_device_info *di,
- union power_supply_propval *val)
+static int bq27xxx_battery_current_and_status(
+ struct bq27xxx_device_info *di,
+ union power_supply_propval *val_curr,
+ union power_supply_propval *val_status)
{
+ bool single_flags = (di->opts & BQ27XXX_O_ZERO);
int curr;
int flags;
@@ -1794,17 +1807,39 @@ static int bq27xxx_battery_current(struct
bq27xxx_device_info *di,
return curr;
}
+ flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, single_flags);
+ if (flags < 0) {
+ dev_err(di->dev, "error reading flags\n");
+ return flags;
+ }
+
if (di->opts & BQ27XXX_O_ZERO) {
- flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, true);
if (!(flags & BQ27000_FLAG_CHGS)) {
dev_dbg(di->dev, "negative current!\n");
curr = -curr;
}
- val->intval = curr * BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS;
+ curr = curr * BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS;
} else {
/* Other gauges return signed value */
- val->intval = (int)((s16)curr) * 1000;
+ curr = (int)((s16)curr) * 1000;
+ }
+
+ if (val_curr)
+ val_curr->intval = curr;
+
+ if (val_status) {
+ if (curr > 0) {
+ val_status->intval = POWER_SUPPLY_STATUS_CHARGING;
+ } else if (curr < 0) {
+ val_status->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+ } else {
+ if (bq27xxx_battery_is_full(di, flags))
+ val_status->intval = POWER_SUPPLY_STATUS_FULL;
+ else
+ val_status->intval =
+ POWER_SUPPLY_STATUS_NOT_CHARGING;
+ }
}
return 0;
@@ -1836,43 +1871,6 @@ static int bq27xxx_battery_pwr_avg(struct
bq27xxx_device_info *di,
return 0;
}
-static int bq27xxx_battery_status(struct bq27xxx_device_info *di,
- union power_supply_propval *val)
-{
- int status;
-
- if (di->opts & BQ27XXX_O_ZERO) {
- if (di->cache.flags & BQ27000_FLAG_FC)
- status = POWER_SUPPLY_STATUS_FULL;
- else if (di->cache.flags & BQ27000_FLAG_CHGS)
- status = POWER_SUPPLY_STATUS_CHARGING;
- else
- status = POWER_SUPPLY_STATUS_DISCHARGING;
- } else if (di->opts & BQ27Z561_O_BITS) {
- if (di->cache.flags & BQ27Z561_FLAG_FC)
- status = POWER_SUPPLY_STATUS_FULL;
- else if (di-
[PATCH v2 1/3] power: supply: bq27xxx: fix sign of current_now for newer ICs
Commit cd060b4d0868 ("power: supply: bq27xxx: fix polarity of current_now")
changed the sign of current_now for all bq27xxx variants, but on BQ28Z610
I'm now seeing negated values *with* that patch.
The GTA04/Openmoko device that was used for testing uses a BQ27000 or
BQ27010 IC, so I assume only the BQ27XXX_O_ZERO code path was incorrect.
Revert the behaviour for newer ICs.
Fixes: cd060b4d0868 "power: supply: bq27xxx: fix polarity of current_now"
Signed-off-by: Matthias Schiffer
---
v2: no changes
drivers/power/supply/bq27xxx_battery.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/power/supply/bq27xxx_battery.c
b/drivers/power/supply/bq27xxx_battery.c
index 4c4a7b1c64c5..cb6ebd2f905e 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1827,7 +1827,7 @@ static int bq27xxx_battery_current(struct
bq27xxx_device_info *di,
val->intval = curr * BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS;
} else {
/* Other gauges return signed value */
- val->intval = -(int)((s16)curr) * 1000;
+ val->intval = (int)((s16)curr) * 1000;
}
return 0;
--
2.17.1
Re: [PATCH net] net: l2tp: reduce log level when passing up invalid packets
On 2/23/21 10:47 AM, Tom Parkin wrote: On Mon, Feb 22, 2021 at 14:31:38 -0800, Jakub Kicinski wrote: On Mon, 22 Feb 2021 17:40:16 +0100 Matthias Schiffer wrote: This will not be sufficient for my usecase: To stay compatible with older versions of fastd, I can't set the T flag in the first packet of the handshake, as it won't be known whether the peer has a new enough fastd version to understand packets that have this bit set. Luckily, the second handshake byte is always 0 in fastd's protocol, so these packets fail the tunnel version check and are passed to userspace regardless. I'm aware that this usecase is far outside of the original intentions of the code and can only be described as a hack, but I still consider this a regression in the kernel, as it was working fine in the past, without visible warnings. I'm sorry, but for the reasons stated above I disagree about it being a regression. Hmm, is it common for protocol implementations in the kernel to warn about invalid packets they receive? While L2TP uses connected sockets and thus usually no unrelated packets end up in the socket, a simple UDP port scan originating from the configured remote address/port will trigger the "short packet" warning now (nmap uses a zero-length payload for UDP scans by default). Log spam caused by a malicous party might also be a concern. Indeed, seems like appropriate counters would be a good fit here? The prints are both potentially problematic for security and lossy. Yes, I agree with this argument. Sounds good, I'll send an updated patch adding a counter for invalid packets. By now I've found another project affected by the kernel warnings: https://github.com/wlanslovenija/tunneldigger/issues/160 OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH 2/3] power: supply: bq27xxx: fix power_avg
On Tue, 2021-02-23 at 15:11 +0100, Matthias Schiffer wrote:
> On all newer bq27xxx ICs, the AveragePower register contains a signed
> value; in addition to handling the raw value as unsigned, the driver
> code also didn't convert it to µW as expected.
>
> At least for the BQ28Z610, the reference manual incorrectly states that
> the value is in units of 1mA and not 10mA. I have no way of knowing
> whether the manuals of other supported ICs contain the same error, or if
> there are models that actually use 1mA. At least, the new code shouldn't
> be *less* correct than the old version for any device
Ugh, of course this section should talk about mW and not mA. I'll wait
if there is more feedback and then send a v2.
> .
>
> power_avg is removed from the cache structure, se we don't have to
> extend it to store both a signed value and an error code. Always getting
> an up-to-date value may be desirable anyways, as it avoids inconsistent
> current and power readings when switching between charging and
> discharging.
>
> Signed-off-by: Matthias Schiffer
> ---
> drivers/power/supply/bq27xxx_battery.c | 51 ++
> include/linux/power/bq27xxx_battery.h | 1 -
> 2 files changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/power/supply/bq27xxx_battery.c
> b/drivers/power/supply/bq27xxx_battery.c
> index cb6ebd2f905e..20e1dc8a87cf 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -1661,27 +1661,6 @@ static int bq27xxx_battery_read_time(struct
> bq27xxx_device_info *di, u8 reg)
> return tval * 60;
> }
>
> -/*
> - * Read an average power register.
> - * Return < 0 if something fails.
> - */
> -static int bq27xxx_battery_read_pwr_avg(struct bq27xxx_device_info *di)
> -{
> - int tval;
> -
> - tval = bq27xxx_read(di, BQ27XXX_REG_AP, false);
> - if (tval < 0) {
> - dev_err(di->dev, "error reading average power register %02x:
> %d\n",
> - BQ27XXX_REG_AP, tval);
> - return tval;
> - }
> -
> - if (di->opts & BQ27XXX_O_ZERO)
> - return (tval * BQ27XXX_POWER_CONSTANT) / BQ27XXX_RS;
> - else
> - return tval;
> -}
> -
> /*
> * Returns true if a battery over temperature condition is detected
> */
> @@ -1769,8 +1748,6 @@ void bq27xxx_battery_update(struct bq27xxx_device_info
> *di)
> }
> if (di->regs[BQ27XXX_REG_CYCT] != INVALID_REG_ADDR)
> cache.cycle_count = bq27xxx_battery_read_cyct(di);
> - if (di->regs[BQ27XXX_REG_AP] != INVALID_REG_ADDR)
> - cache.power_avg = bq27xxx_battery_read_pwr_avg(di);
>
> /* We only have to read charge design full once */
> if (di->charge_design_full <= 0)
> @@ -1833,6 +1810,32 @@ static int bq27xxx_battery_current(struct
> bq27xxx_device_info *di,
> return 0;
> }
>
> +/*
> + * Get the average power in µW
> + * Return < 0 if something fails.
> + */
> +static int bq27xxx_battery_pwr_avg(struct bq27xxx_device_info *di,
> +union power_supply_propval *val)
> +{
> + int power;
> +
> + power = bq27xxx_read(di, BQ27XXX_REG_AP, false);
> + if (power < 0) {
> + dev_err(di->dev,
> + "error reading average power register %02x: %d\n",
> + BQ27XXX_REG_AP, power);
> + return power;
> + }
> +
> + if (di->opts & BQ27XXX_O_ZERO)
> + val->intval = (power * BQ27XXX_POWER_CONSTANT) / BQ27XXX_RS;
> + else
> + /* Other gauges return a signed value in units of 10mW */
> + val->intval = (int)((s16)power) * 1;
> +
> + return 0;
> +}
> +
> static int bq27xxx_battery_status(struct bq27xxx_device_info *di,
> union power_supply_propval *val)
> {
> @@ -2020,7 +2023,7 @@ static int bq27xxx_battery_get_property(struct
> power_supply *psy,
> ret = bq27xxx_simple_value(di->cache.energy, val);
> break;
> case POWER_SUPPLY_PROP_POWER_AVG:
> - ret = bq27xxx_simple_value(di->cache.power_avg, val);
> + ret = bq27xxx_battery_pwr_avg(di, val);
> break;
> case POWER_SUPPLY_PROP_HEALTH:
> ret = bq27xxx_simple_value(di->cache.health, val);
> diff --git a/include/linux/power/bq27xxx_battery.h
> b/include/linux/power/bq27xxx_battery.h
> index 111a40d0d3d5..8d5f4f40fb41 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -53,7 +53,6 @@ struct bq27xxx_reg_cache {
> int capacity;
> int energy;
> int flags;
> - int power_avg;
> int health;
> };
>
[PATCH 1/3] power: supply: bq27xxx: fix sign of current_now for newer ICs
Commit cd060b4d0868 ("power: supply: bq27xxx: fix polarity of current_now")
changed the sign of current_now for all bq27xxx variants, but on BQ28Z610
I'm now seeing negated values *with* that patch.
The GTA04/Openmoko device that was used for testing uses a BQ27000 or
BQ27010 IC, so I assume only the BQ27XXX_O_ZERO code path was incorrect.
Revert the behaviour for newer ICs.
Fixes: cd060b4d0868 "power: supply: bq27xxx: fix polarity of current_now"
Signed-off-by: Matthias Schiffer
---
@Andreas Kemnade: It would be great to get a confirmation that the
Openmoko battery indeed uses BQ27000/BQ27010 - I was having some trouble
finding that information.
drivers/power/supply/bq27xxx_battery.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/power/supply/bq27xxx_battery.c
b/drivers/power/supply/bq27xxx_battery.c
index 4c4a7b1c64c5..cb6ebd2f905e 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1827,7 +1827,7 @@ static int bq27xxx_battery_current(struct
bq27xxx_device_info *di,
val->intval = curr * BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS;
} else {
/* Other gauges return signed value */
- val->intval = -(int)((s16)curr) * 1000;
+ val->intval = (int)((s16)curr) * 1000;
}
return 0;
--
2.17.1
[PATCH 3/3] power: supply: bq27xxx: make status more robust
There are multiple issues in bq27xxx_battery_status():
- On BQ28Q610 is was observed that the "full" flag may be set even while
the battery is charging or discharging. With the current logic to make
"full" override everything else, it look a very long time (>20min) for
the status to change from "full" to "discharging" after unplugging the
supply on a device with low power consumption
- The POWER_SUPPLY_STATUS_NOT_CHARGING check depends on
power_supply_am_i_supplied(), which will not work when the supply
doesn't exist as a separate device known to Linux
We can solve both issues by deriving the status from the current instead
of the flags field. The flags are now only used to distinguish "full"
from "not charging", and to determine the sign of the current on
BQ27XXX_O_ZERO devices.
Signed-off-by: Matthias Schiffer
---
drivers/power/supply/bq27xxx_battery.c | 88 +-
1 file changed, 43 insertions(+), 45 deletions(-)
diff --git a/drivers/power/supply/bq27xxx_battery.c
b/drivers/power/supply/bq27xxx_battery.c
index 20e1dc8a87cf..b62a8cfd9d09 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1777,14 +1777,27 @@ static void bq27xxx_battery_poll(struct work_struct
*work)
schedule_delayed_work(>work, poll_interval * HZ);
}
+static bool bq27xxx_battery_is_full(struct bq27xxx_device_info *di, int flags)
+{
+ if (di->opts & BQ27XXX_O_ZERO)
+ return (flags & BQ27000_FLAG_FC);
+ else if (di->opts & BQ27Z561_O_BITS)
+ return (flags & BQ27Z561_FLAG_FC);
+ else
+ return (flags & BQ27XXX_FLAG_FC);
+}
+
/*
- * Return the battery average current in µA
+ * Return the battery average current in µA and the status
* Note that current can be negative signed as well
* Or 0 if something fails.
*/
-static int bq27xxx_battery_current(struct bq27xxx_device_info *di,
- union power_supply_propval *val)
+static int bq27xxx_battery_current_and_status(
+ struct bq27xxx_device_info *di,
+ union power_supply_propval *val_curr,
+ union power_supply_propval *val_status)
{
+ bool single_flags = (di->opts & BQ27XXX_O_ZERO);
int curr;
int flags;
@@ -1794,17 +1807,39 @@ static int bq27xxx_battery_current(struct
bq27xxx_device_info *di,
return curr;
}
+ flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, single_flags);
+ if (flags < 0) {
+ dev_err(di->dev, "error reading flags\n");
+ return flags;
+ }
+
if (di->opts & BQ27XXX_O_ZERO) {
- flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, true);
if (!(flags & BQ27000_FLAG_CHGS)) {
dev_dbg(di->dev, "negative current!\n");
curr = -curr;
}
- val->intval = curr * BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS;
+ curr = curr * BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS;
} else {
/* Other gauges return signed value */
- val->intval = (int)((s16)curr) * 1000;
+ curr = (int)((s16)curr) * 1000;
+ }
+
+ if (val_curr)
+ val_curr->intval = curr;
+
+ if (val_status) {
+ if (curr > 0) {
+ val_status->intval = POWER_SUPPLY_STATUS_CHARGING;
+ } else if (curr < 0) {
+ val_status->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+ } else {
+ if (bq27xxx_battery_is_full(di, flags))
+ val_status->intval = POWER_SUPPLY_STATUS_FULL;
+ else
+ val_status->intval =
+ POWER_SUPPLY_STATUS_NOT_CHARGING;
+ }
}
return 0;
@@ -1836,43 +1871,6 @@ static int bq27xxx_battery_pwr_avg(struct
bq27xxx_device_info *di,
return 0;
}
-static int bq27xxx_battery_status(struct bq27xxx_device_info *di,
- union power_supply_propval *val)
-{
- int status;
-
- if (di->opts & BQ27XXX_O_ZERO) {
- if (di->cache.flags & BQ27000_FLAG_FC)
- status = POWER_SUPPLY_STATUS_FULL;
- else if (di->cache.flags & BQ27000_FLAG_CHGS)
- status = POWER_SUPPLY_STATUS_CHARGING;
- else
- status = POWER_SUPPLY_STATUS_DISCHARGING;
- } else if (di->opts & BQ27Z561_O_BITS) {
- if (di->cache.flags & BQ27Z561_FLAG_FC)
- status = POWER_SUPPLY_STATUS_FULL;
- else if (di->cache.flags & BQ27Z561_FLAG_DI
[PATCH 2/3] power: supply: bq27xxx: fix power_avg
On all newer bq27xxx ICs, the AveragePower register contains a signed
value; in addition to handling the raw value as unsigned, the driver
code also didn't convert it to µW as expected.
At least for the BQ28Z610, the reference manual incorrectly states that
the value is in units of 1mA and not 10mA. I have no way of knowing
whether the manuals of other supported ICs contain the same error, or if
there are models that actually use 1mA. At least, the new code shouldn't
be *less* correct than the old version for any device.
power_avg is removed from the cache structure, se we don't have to
extend it to store both a signed value and an error code. Always getting
an up-to-date value may be desirable anyways, as it avoids inconsistent
current and power readings when switching between charging and
discharging.
Signed-off-by: Matthias Schiffer
---
drivers/power/supply/bq27xxx_battery.c | 51 ++
include/linux/power/bq27xxx_battery.h | 1 -
2 files changed, 27 insertions(+), 25 deletions(-)
diff --git a/drivers/power/supply/bq27xxx_battery.c
b/drivers/power/supply/bq27xxx_battery.c
index cb6ebd2f905e..20e1dc8a87cf 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1661,27 +1661,6 @@ static int bq27xxx_battery_read_time(struct
bq27xxx_device_info *di, u8 reg)
return tval * 60;
}
-/*
- * Read an average power register.
- * Return < 0 if something fails.
- */
-static int bq27xxx_battery_read_pwr_avg(struct bq27xxx_device_info *di)
-{
- int tval;
-
- tval = bq27xxx_read(di, BQ27XXX_REG_AP, false);
- if (tval < 0) {
- dev_err(di->dev, "error reading average power register %02x:
%d\n",
- BQ27XXX_REG_AP, tval);
- return tval;
- }
-
- if (di->opts & BQ27XXX_O_ZERO)
- return (tval * BQ27XXX_POWER_CONSTANT) / BQ27XXX_RS;
- else
- return tval;
-}
-
/*
* Returns true if a battery over temperature condition is detected
*/
@@ -1769,8 +1748,6 @@ void bq27xxx_battery_update(struct bq27xxx_device_info
*di)
}
if (di->regs[BQ27XXX_REG_CYCT] != INVALID_REG_ADDR)
cache.cycle_count = bq27xxx_battery_read_cyct(di);
- if (di->regs[BQ27XXX_REG_AP] != INVALID_REG_ADDR)
- cache.power_avg = bq27xxx_battery_read_pwr_avg(di);
/* We only have to read charge design full once */
if (di->charge_design_full <= 0)
@@ -1833,6 +1810,32 @@ static int bq27xxx_battery_current(struct
bq27xxx_device_info *di,
return 0;
}
+/*
+ * Get the average power in µW
+ * Return < 0 if something fails.
+ */
+static int bq27xxx_battery_pwr_avg(struct bq27xxx_device_info *di,
+ union power_supply_propval *val)
+{
+ int power;
+
+ power = bq27xxx_read(di, BQ27XXX_REG_AP, false);
+ if (power < 0) {
+ dev_err(di->dev,
+ "error reading average power register %02x: %d\n",
+ BQ27XXX_REG_AP, power);
+ return power;
+ }
+
+ if (di->opts & BQ27XXX_O_ZERO)
+ val->intval = (power * BQ27XXX_POWER_CONSTANT) / BQ27XXX_RS;
+ else
+ /* Other gauges return a signed value in units of 10mW */
+ val->intval = (int)((s16)power) * 1;
+
+ return 0;
+}
+
static int bq27xxx_battery_status(struct bq27xxx_device_info *di,
union power_supply_propval *val)
{
@@ -2020,7 +2023,7 @@ static int bq27xxx_battery_get_property(struct
power_supply *psy,
ret = bq27xxx_simple_value(di->cache.energy, val);
break;
case POWER_SUPPLY_PROP_POWER_AVG:
- ret = bq27xxx_simple_value(di->cache.power_avg, val);
+ ret = bq27xxx_battery_pwr_avg(di, val);
break;
case POWER_SUPPLY_PROP_HEALTH:
ret = bq27xxx_simple_value(di->cache.health, val);
diff --git a/include/linux/power/bq27xxx_battery.h
b/include/linux/power/bq27xxx_battery.h
index 111a40d0d3d5..8d5f4f40fb41 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -53,7 +53,6 @@ struct bq27xxx_reg_cache {
int capacity;
int energy;
int flags;
- int power_avg;
int health;
};
--
2.17.1
Re: [PATCH net] net: l2tp: reduce log level when passing up invalid packets
On 2/22/21 12:49 PM, Tom Parkin wrote:
On Sat, Feb 20, 2021 at 10:56:33 +0100, Matthias Schiffer wrote:
On 2/19/21 9:12 PM, Tom Parkin wrote:
On Fri, Feb 19, 2021 at 20:06:15 +0100, Matthias Schiffer wrote:
Before commit 5ee759cda51b ("l2tp: use standard API for warning log
messages"), it was possible for userspace applications to use their own
control protocols on the backing sockets of an L2TP kernel device, and as
long as a packet didn't look like a proper L2TP data packet, it would be
passed up to userspace just fine.
Hum. I appreciate we're now logging where we previously were not, but
I would say these warning messages are valid.
It's still perfectly possible to use the L2TP socket for the L2TP control
protocol: packets per the RFCs won't trigger these warnings to the
best of my knowledge, although I'm happy to be proven wrong!
I wonder whether your application is sending non-L2TP packets over the
L2TP socket? Could you describe the usecase?
I'm the developer of the UDP-based VPN/tunnel application fastd [1]. This is
currently a pure userspace implementation, supporting both encrypted and
unencrypted tunnels, with a protocol that doesn't look anything like L2TP.
To improve performance of unencrypted tunnels, I'm looking into using L2TP
to offload the data plane to the kernel. Whether to make use of this would
be negotiated in a session handshake (that is also used for key exchange in
encrypted mode).
As the (IPv4) internet is stupid and everything is NATted, and I even want
to support broken NAT routers that somehow break UDP hole punching, I use
only a single socket for both control (handshake) and data packets.
Thanks for describing the usecase a bit more. It looks an interesting
project.
To be honest I'm not sure the L2TP subsystem is a good match for
fastd's datapath offload though.
This is for the following reasons:
Firstly, the warnings referenced in your patch are early in the L2TP recv
path, called shortly after our hook into the UDP recv code.
So at this point, I don't believe the L2TP subsystem is really buying you
anything over a plain UPD recv.
Now, I'm perhaps reading too much into what you've said, but I imagine
that fastd using the L2TP tunnel context is just a first step. I
assume the end goal for datapath offload would be to use the L2TP
pseudowire code in order to have session data appear from e.g. a
virtual Ethernet interface. That way you get to avoid copying data
to userspace, and then stuffing it back into the kernel via. tun/tap.
And that makes sense to me as a goal!
That is indeed what I want to achieve.
However, if that is indeed the goal, I really can't see it working
without fastd's protocol being modified to look like L2TP. (I also,
btw, can't see it working without some kind of kernel-space L2TP
subsystem support for fastd's various encryption protocols, but that's
another matter).
Only unencrypted connections would be offloaded.
fastd's data protocol will be modified to use L2TP Ethernet pseudowire as
specified by the RFC (I actually finished the userspace implementation of
this yesterday, in branch method-l2tp for now). Two peers can negotiate the
protocol to use (called "method" in fastd terms) in the session handshake.
A session would only be offloaded to kernel-space L2TP when both sides
agree that they indeed want to use the L2TP method; otherwise fastd will
continue to use TUN/TAP.
The only part of the fastd protocol that I can't modify arbitrarily is the
first packet of the handshake, as it must be compatible with older versions
of fastd. There may be a point when I can set the T flag in handshakes
unconditionally, but that would be 3~5 years in the future.
If you take a look at the session recv datapath from l2tp_recv_common
onwards you'll see that there is a lot of code you have to avoid
confusing in l2tp_core.c alone, even before you get into any
pseudowire-specifics. I can't see that being possible with fastd's
current data packet format >
In summary, I think at this point in the L2TP recv path a normal UDP
socket would serve you just as well, and I can't see the L2TP subsystem
being useful as a data offload mechanism for fastd in the future
without effectively changing fastd's packet format to look like L2TP.
Secondly, given the above (and apologies if I've missed the mark); I
really wouldn't want to encourage you to use the L2TP subsystem for
future fastd improvements.
From fastd's perspective it is IMO a bad idea, since it would be easy
for a future (perfectly valid) change in the L2TP code to accidentally
break fastd. And next time it might not be some easily-tweaked thing
like logging levels, but rather e.g. a security fix or bug fix which
cannot be undone.
From the L2TP subsystem's perspective it is a bad idea, since by
encouraging fastd to use the L2TP code, we end up hampering future L2TP
development in order to support a project which doesn't actually use
t
Re: [PATCH net] net: l2tp: reduce log level when passing up invalid packets
Hi Tom,
On 2/19/21 9:12 PM, Tom Parkin wrote:
Hi Matthias,
Thanks for your patch!
On Fri, Feb 19, 2021 at 20:06:15 +0100, Matthias Schiffer wrote:
Before commit 5ee759cda51b ("l2tp: use standard API for warning log
messages"), it was possible for userspace applications to use their own
control protocols on the backing sockets of an L2TP kernel device, and as
long as a packet didn't look like a proper L2TP data packet, it would be
passed up to userspace just fine.
Hum. I appreciate we're now logging where we previously were not, but
I would say these warning messages are valid.
It's still perfectly possible to use the L2TP socket for the L2TP control
protocol: packets per the RFCs won't trigger these warnings to the
best of my knowledge, although I'm happy to be proven wrong!
I wonder whether your application is sending non-L2TP packets over the
L2TP socket? Could you describe the usecase?
I'm the developer of the UDP-based VPN/tunnel application fastd [1]. This
is currently a pure userspace implementation, supporting both encrypted and
unencrypted tunnels, with a protocol that doesn't look anything like L2TP.
To improve performance of unencrypted tunnels, I'm looking into using L2TP
to offload the data plane to the kernel. Whether to make use of this would
be negotiated in a session handshake (that is also used for key exchange in
encrypted mode).
As the (IPv4) internet is stupid and everything is NATted, and I even want
to support broken NAT routers that somehow break UDP hole punching, I use
only a single socket for both control (handshake) and data packets.
After the mentioned change, this approach would lead to significant log
spam, as the previously hidden warnings are now shown by default. Not
even setting the T flag on the custom control packets is sufficient to
surpress these warnings, as packet length and L2TP version are checked
before the T flag.
Possibly we could sidestep some of these warnings by moving the T flag
check further up in the function.
The code would need to pull the first byte of the header, check the type
bit, and skip further processing if the bit was set. Otherwise go on to
pull the rest of the header.
I think I'd prefer this approach assuming the warnings are not
triggered by valid L2TP messages.
This will not be sufficient for my usecase: To stay compatible with older
versions of fastd, I can't set the T flag in the first packet of the
handshake, as it won't be known whether the peer has a new enough fastd
version to understand packets that have this bit set. Luckily, the second
handshake byte is always 0 in fastd's protocol, so these packets fail the
tunnel version check and are passed to userspace regardless.
I'm aware that this usecase is far outside of the original intentions of
the code and can only be described as a hack, but I still consider this a
regression in the kernel, as it was working fine in the past, without
visible warnings.
[1] https://github.com/NeoRaider/fastd/
Reduce all warnings debug level when packets are passed to userspace.
Fixes: 5ee759cda51b ("l2tp: use standard API for warning log messages")
Signed-off-by: Matthias Schiffer
---
I'm unsure what to do about the pr_warn_ratelimited() in
l2tp_recv_common(). It feels wrong to me that an incoming network packet
can trigger a kernel message above debug level at all, so maybe they
should be downgraded as well? I believe the only reason these were ever
warnings is that they were not shown by default.
net/l2tp/l2tp_core.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 7be5103ff2a8..40852488c62a 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -809,8 +809,8 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel,
struct sk_buff *skb)
/* Short packet? */
if (!pskb_may_pull(skb, L2TP_HDR_SIZE_MAX)) {
- pr_warn_ratelimited("%s: recv short packet (len=%d)\n",
- tunnel->name, skb->len);
+ pr_debug_ratelimited("%s: recv short packet (len=%d)\n",
+tunnel->name, skb->len);
goto error;
}
@@ -824,8 +824,8 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel,
struct sk_buff *skb)
/* Check protocol version */
version = hdrflags & L2TP_HDR_VER_MASK;
if (version != tunnel->version) {
- pr_warn_ratelimited("%s: recv protocol version mismatch: got %d
expected %d\n",
- tunnel->name, version, tunnel->version);
+ pr_debug_ratelimited("%s: recv protocol version mismatch: got %d
expected %d\n",
+tunnel->name, version, tunnel->version);
goto error;
}
@@ -86
[PATCH net] net: l2tp: reduce log level when passing up invalid packets
Before commit 5ee759cda51b ("l2tp: use standard API for warning log
messages"), it was possible for userspace applications to use their own
control protocols on the backing sockets of an L2TP kernel device, and as
long as a packet didn't look like a proper L2TP data packet, it would be
passed up to userspace just fine.
After the mentioned change, this approach would lead to significant log
spam, as the previously hidden warnings are now shown by default. Not
even setting the T flag on the custom control packets is sufficient to
surpress these warnings, as packet length and L2TP version are checked
before the T flag.
Reduce all warnings debug level when packets are passed to userspace.
Fixes: 5ee759cda51b ("l2tp: use standard API for warning log messages")
Signed-off-by: Matthias Schiffer
---
I'm unsure what to do about the pr_warn_ratelimited() in
l2tp_recv_common(). It feels wrong to me that an incoming network packet
can trigger a kernel message above debug level at all, so maybe they
should be downgraded as well? I believe the only reason these were ever
warnings is that they were not shown by default.
net/l2tp/l2tp_core.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 7be5103ff2a8..40852488c62a 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -809,8 +809,8 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel,
struct sk_buff *skb)
/* Short packet? */
if (!pskb_may_pull(skb, L2TP_HDR_SIZE_MAX)) {
- pr_warn_ratelimited("%s: recv short packet (len=%d)\n",
- tunnel->name, skb->len);
+ pr_debug_ratelimited("%s: recv short packet (len=%d)\n",
+tunnel->name, skb->len);
goto error;
}
@@ -824,8 +824,8 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel,
struct sk_buff *skb)
/* Check protocol version */
version = hdrflags & L2TP_HDR_VER_MASK;
if (version != tunnel->version) {
- pr_warn_ratelimited("%s: recv protocol version mismatch: got %d
expected %d\n",
- tunnel->name, version, tunnel->version);
+ pr_debug_ratelimited("%s: recv protocol version mismatch: got
%d expected %d\n",
+tunnel->name, version, tunnel->version);
goto error;
}
@@ -863,8 +863,8 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel,
struct sk_buff *skb)
l2tp_session_dec_refcount(session);
/* Not found? Pass to userspace to deal with */
- pr_warn_ratelimited("%s: no session found (%u/%u). Passing
up.\n",
- tunnel->name, tunnel_id, session_id);
+ pr_debug_ratelimited("%s: no session found (%u/%u). Passing
up.\n",
+tunnel->name, tunnel_id, session_id);
goto error;
}
--
2.30.1
[PATCH] dt-bindings: vendor-prefixes: correct the spelling of TQ-Systems GmbH
From: Max Merchel "TQ-Systems" is written with a dash, as can be seen on https://www.tq-group.com/en/imprint/ Signed-off-by: Max Merchel Signed-off-by: Matthias Schiffer --- Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml index 3a76c226771b..5b7ee0e059a2 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml @@ -1125,7 +1125,7 @@ patternProperties: "^tpo,.*": description: TPO "^tq,.*": -description: TQ Systems GmbH +description: TQ-Systems GmbH "^tronfy,.*": description: Tronfy "^tronsmart,.*": -- 2.17.1
[PATCH v2 1/2] ARM: dts: imx7-tqma7: add SPI-NOR flash
The SPI-NOR flash on the SoM was missing from the device tree.
Signed-off-by: Matthias Schiffer
---
arch/arm/boot/dts/imx7-tqma7.dtsi | 30 ++
1 file changed, 30 insertions(+)
v2: change node name to flash@0
diff --git a/arch/arm/boot/dts/imx7-tqma7.dtsi
b/arch/arm/boot/dts/imx7-tqma7.dtsi
index 8773344b54aa..22f4194322ed 100644
--- a/arch/arm/boot/dts/imx7-tqma7.dtsi
+++ b/arch/arm/boot/dts/imx7-tqma7.dtsi
@@ -160,6 +160,20 @@
>;
};
+ pinctrl_qspi: qspigrp {
+ fsl,pins = <
+ MX7D_PAD_EPDC_DATA00__QSPI_A_DATA0 0x5A
+ MX7D_PAD_EPDC_DATA01__QSPI_A_DATA1 0x5A
+ MX7D_PAD_EPDC_DATA02__QSPI_A_DATA2 0x5A
+ MX7D_PAD_EPDC_DATA03__QSPI_A_DATA3 0x5A
+ MX7D_PAD_EPDC_DATA05__QSPI_A_SCLK 0x11
+ MX7D_PAD_EPDC_DATA06__QSPI_A_SS0_B 0x54
+ MX7D_PAD_EPDC_DATA07__QSPI_A_SS1_B 0x54
+ /* #QSPI_RESET */
+ MX7D_PAD_EPDC_DATA04__GPIO2_IO4 0x4052
+ >;
+ };
+
pinctrl_usdhc3: usdhc3grp {
fsl,pins = <
MX7D_PAD_SD3_CMD__SD3_CMD 0x59
@@ -217,6 +231,22 @@
};
};
+ {
+ pinctrl-names = "default";
+ pinctrl-0 = <_qspi>;
+ status = "okay";
+
+ flash0: flash@0 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "jedec,spi-nor";
+ spi-max-frequency = <2900>;
+ spi-rx-bus-width = <4>;
+ spi-tx-bus-width = <4>;
+ reg = <0>;
+ };
+};
+
{
status = "okay";
};
--
2.17.1
[PATCH v2 2/2] ARM: dts: imx7-mba7: add default SPI-NOR flash partition layout
Add the partition layout also used by the bootloader.
Signed-off-by: Matthias Schiffer
---
arch/arm/boot/dts/imx7-mba7.dtsi | 32
1 file changed, 32 insertions(+)
v2: no changes
diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi b/arch/arm/boot/dts/imx7-mba7.dtsi
index c6d1c63f7905..3683f97f946f 100644
--- a/arch/arm/boot/dts/imx7-mba7.dtsi
+++ b/arch/arm/boot/dts/imx7-mba7.dtsi
@@ -237,6 +237,38 @@
};
};
+ {
+ uboot@0 {
+ label = "U-Boot";
+ reg = <0x0 0xd>;
+ };
+
+ env1@d {
+ label = "ENV1";
+ reg = <0xd 0x1>;
+ };
+
+ env2@e {
+ label = "ENV2";
+ reg = <0xe 0x1>;
+ };
+
+ dtb@f {
+ label = "DTB";
+ reg = <0xf 0x1>;
+ };
+
+ linux@10 {
+ label = "Linux";
+ reg = <0x10 0x70>;
+ };
+
+ rootfs@80 {
+ label = "RootFS";
+ reg = <0x80 0x380>;
+ };
+};
+
{
pinctrl-names = "default";
pinctrl-0 = <_flexcan1>;
--
2.17.1
Re: [PATCH 01/13] dt-bindings: arm: fsl: update TQ-Systems SoMs and boards based on i.MX7
On Fri, 2020-09-18 at 13:29 +0200, Matthias Schiffer wrote: > Introduce compatible strings for the TQMa7x SoMs. > > Signed-off-by: Matthias Schiffer Hello Shawn, what is the status of this patch series? I've found the series marked as "archived" in patchwork [1], but I can't find it in any public Git repo. Am I looking at the wrong patchwork project? Kind regards, Matthias [1] https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=351407=%2A=both > --- > Documentation/devicetree/bindings/arm/fsl.yaml | 14 -- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml > b/Documentation/devicetree/bindings/arm/fsl.yaml > index 71acf14da715..d7233cef4d38 100644 > --- a/Documentation/devicetree/bindings/arm/fsl.yaml > +++ b/Documentation/devicetree/bindings/arm/fsl.yaml > @@ -298,7 +298,12 @@ properties: >- toradex,colibri-imx7s # Colibri iMX7 Solo Module >- toradex,colibri-imx7s-aster # Colibri iMX7 Solo Module > on Aster Carrier Board >- toradex,colibri-imx7s-eval-v3 # Colibri iMX7 Solo Module > on Colibri Evaluation Board V3 > - - tq,imx7s-mba7 # i.MX7S TQ MBa7 with TQMa7S SoM > + - const: fsl,imx7s > + > + - description: TQ-Systems TQMa7S SoM on MBa7x board > +items: > + - const: tq,imx7s-mba7 > + - const: tq,imx7s-tqma7 >- const: fsl,imx7s > >- description: i.MX7D based Boards > @@ -320,11 +325,16 @@ properties: > # Colibri > Evaluation Board V3 >- toradex,colibri-imx7d-eval-v3 # Colibri iMX7 Dual > Module on > # Colibri > Evaluation Board V3 > - - tq,imx7d-mba7 # i.MX7D TQ MBa7 with TQMa7D SoM >- zii,imx7d-rmu2# ZII RMU2 Board >- zii,imx7d-rpu2# ZII RPU2 Board >- const: fsl,imx7d > > + - description: TQ-Systems TQMa7D SoM on MBa7x board > +items: > + - const: tq,imx7d-mba7 > + - const: tq,imx7d-tqma7 > + - const: fsl,imx7d > + >- description: >Compulab SBC-iMX7 is a single board computer based on the >Freescale i.MX7 system-on-chip. SBC-iMX7 is implemented with
[PATCH] tty: serial: imx: disable TXDC IRQ in imx_uart_shutdown() to avoid IRQ storm
The IPG clock is disabled at the end of imx_uart_shutdown(); we really don't want to run any IRQ handlers after this point. At least on i.MX8MN, the UART will happily continue to generate interrupts even with its clocks disabled, but in this state, all register writes are ignored (which will cause the shadow registers to differ from the actual register values, resulting in all kinds of weirdness). In a transfer without DMA, this could lead to the following sequence of events: - The UART finishes its transmission while imx_uart_shutdown() is run, triggering the TXDC interrupt (we can trigger this fairly reliably by writing a single byte to the TTY and closing it right away) - imx_uart_shutdown() finishes, disabling the UART clocks - imx_uart_int() -> imx_uart_transmit_buffer() -> imx_uart_stop_tx() imx_uart_stop_tx() should now clear UCR4_TCEN to disable the TXDC interrupt, but this register write is ineffective. This results in an interrupt storm. To disable all interrupts in the same place, and to avoid setting UCR4 twice, clearing UCR4_OREN is moved below del_timer_sync() as well; this should be harmless. Signed-off-by: Matthias Schiffer --- While debugging this, I found one more instance of register writes with disabled clock: The IPG clock is disabled before calling uart_add_one_port() at the end of imx_uart_probe(). This results in the following call stack: imx_uart_writel+0x168/0x188 imx_uart_set_mctrl+0x3c/0xb8 uart_add_one_port+0x394/0x4c8 imx_uart_probe+0x530/0x810 Fortunately, in this case the register already matches the value that is written, so no inconsistent state results. I assume we'll have to do something about the way we handle the clocks in this driver to fix this... drivers/tty/serial/imx.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index 4e6ead1f650e..1731d9728865 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -1552,10 +1552,6 @@ static void imx_uart_shutdown(struct uart_port *port) ucr2 = imx_uart_readl(sport, UCR2); ucr2 &= ~(UCR2_TXEN | UCR2_ATEN); imx_uart_writel(sport, ucr2, UCR2); - - ucr4 = imx_uart_readl(sport, UCR4); - ucr4 &= ~UCR4_OREN; - imx_uart_writel(sport, ucr4, UCR4); spin_unlock_irqrestore(>port.lock, flags); /* @@ -1568,10 +1564,15 @@ static void imx_uart_shutdown(struct uart_port *port) */ spin_lock_irqsave(>port.lock, flags); + ucr1 = imx_uart_readl(sport, UCR1); ucr1 &= ~(UCR1_TRDYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_UARTEN | UCR1_RXDMAEN | UCR1_ATDMAEN); - imx_uart_writel(sport, ucr1, UCR1); + + ucr4 = imx_uart_readl(sport, UCR4); + ucr4 &= ~(UCR4_OREN | UCR4_TCEN); + imx_uart_writel(sport, ucr4, UCR4); + spin_unlock_irqrestore(>port.lock, flags); clk_disable_unprepare(sport->clk_per); -- 2.17.1
[PATCH 09/13] ARM: dts: imx7-mba7: add audio support
The MBa7x is equipped with a TI TLV320AIC3204 audio codec.
Signed-off-by: Matthias Schiffer
---
arch/arm/boot/dts/imx7-mba7.dtsi | 38 +++-
1 file changed, 37 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi b/arch/arm/boot/dts/imx7-mba7.dtsi
index 215730e0453e..9cfaf0a91100 100644
--- a/arch/arm/boot/dts/imx7-mba7.dtsi
+++ b/arch/arm/boot/dts/imx7-mba7.dtsi
@@ -170,6 +170,20 @@
regulator-max-microvolt = <330>;
regulator-always-on;
};
+
+ sound {
+ compatible = "fsl,imx-audio-tlv320aic32x4";
+ model = "imx-audio-tlv320aic32x4";
+ ssi-controller = <>;
+ audio-codec = <>;
+ audio-routing =
+ "IN3_L", "Mic Jack",
+ "Mic Jack", "Mic Bias",
+ "IN1_L", "Line In Jack",
+ "IN1_R", "Line In Jack",
+ "Line Out Jack", "LOL",
+ "Line Out Jack", "LOR";
+ };
};
{
@@ -363,13 +377,25 @@
>;
};
-
pinctrl_pca9555: pca95550grp {
fsl,pins = <
MX7D_PAD_ENET1_TX_CLK__GPIO7_IO12 0x78
>;
};
+ pinctrl_sai1: sai1grp {
+ fsl,pins = <
+ MX7D_PAD_SAI1_MCLK__SAI1_MCLK 0x11
+ MX7D_PAD_SAI1_RX_BCLK__SAI1_RX_BCLK 0x1c
+ MX7D_PAD_SAI1_RX_DATA__SAI1_RX_DATA00x1c
+ MX7D_PAD_SAI1_RX_SYNC__SAI2_RX_SYNC 0x1c
+
+ MX7D_PAD_SAI1_TX_BCLK__SAI1_TX_BCLK 0x1c
+ MX7D_PAD_SAI1_TX_DATA__SAI1_TX_DATA00x14
+ MX7D_PAD_SAI1_TX_SYNC__SAI1_TX_SYNC 0x14
+ >;
+ };
+
pinctrl_uart3: uart3grp {
fsl,pins = <
MX7D_PAD_UART3_RX_DATA__UART3_DCE_RX0x7e
@@ -487,6 +513,16 @@
status = "okay";
};
+ {
+ pinctrl-names = "default";
+ pinctrl-0 = <_sai1>;
+ assigned-clocks = < IMX7D_SAI1_ROOT_SRC>,
+ < IMX7D_SAI1_ROOT_CLK>;
+ assigned-clock-parents = < IMX7D_PLL_AUDIO_POST_DIV>;
+ assigned-clock-rates = <0>, <36864000>;
+ status = "okay";
+};
+
{
pinctrl-names = "default";
pinctrl-0 = <_uart3>;
--
2.17.1
[PATCH 07/13] ARM: dts: imx7-mba7: configure watchdog
The external watchdog reset is necessary, as the internal reset is
unreliable on i.MX7.
Signed-off-by: Matthias Schiffer
---
arch/arm/boot/dts/imx7-mba7.dtsi | 12
1 file changed, 12 insertions(+)
diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi b/arch/arm/boot/dts/imx7-mba7.dtsi
index 9be225bb135a..84b5809f384c 100644
--- a/arch/arm/boot/dts/imx7-mba7.dtsi
+++ b/arch/arm/boot/dts/imx7-mba7.dtsi
@@ -467,6 +467,12 @@
MX7D_PAD_LPSR_GPIO1_IO05__GPIO1_IO5 0x59
>;
};
+
+ pinctrl_wdog1: wdog1grp {
+ fsl,pins = <
+ MX7D_PAD_LPSR_GPIO1_IO00__WDOG1_WDOG_B 0x30
+ >;
+ };
};
{
@@ -543,3 +549,9 @@
no-1-8-v;
status = "okay";
};
+
+ {
+ pinctrl-names = "default";
+ pinctrl-0 = <_wdog1>;
+ fsl,ext-reset-output;
+};
--
2.17.1
[PATCH 11/13] ARM: dts: imx7-mba7: enable RS485 on UART7
The UART7 interface is connected to a full-duplex RS485 transceiver. Signed-off-by: Matthias Schiffer --- arch/arm/boot/dts/imx7-mba7.dtsi | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi b/arch/arm/boot/dts/imx7-mba7.dtsi index f1e50bcd21a5..cd29844bc76b 100644 --- a/arch/arm/boot/dts/imx7-mba7.dtsi +++ b/arch/arm/boot/dts/imx7-mba7.dtsi @@ -593,6 +593,9 @@ assigned-clocks = < IMX7D_UART7_ROOT_SRC>; assigned-clock-parents = < IMX7D_OSC_24M_CLK>; uart-has-rtscts; + linux,rs485-enabled-at-boot-time; + rs485-rts-active-low; + rs485-rx-during-tx; status = "okay"; }; -- 2.17.1
[PATCH 02/13] ARM: dts: imx7-tqma7: add SPI-NOR flash
The SPI-NOR flash on the SoM was missing from the device tree.
Signed-off-by: Matthias Schiffer
---
arch/arm/boot/dts/imx7-tqma7.dtsi | 30 ++
1 file changed, 30 insertions(+)
diff --git a/arch/arm/boot/dts/imx7-tqma7.dtsi
b/arch/arm/boot/dts/imx7-tqma7.dtsi
index 8773344b54aa..fd23f764399a 100644
--- a/arch/arm/boot/dts/imx7-tqma7.dtsi
+++ b/arch/arm/boot/dts/imx7-tqma7.dtsi
@@ -160,6 +160,20 @@
>;
};
+ pinctrl_qspi: qspigrp {
+ fsl,pins = <
+ MX7D_PAD_EPDC_DATA00__QSPI_A_DATA0 0x5A
+ MX7D_PAD_EPDC_DATA01__QSPI_A_DATA1 0x5A
+ MX7D_PAD_EPDC_DATA02__QSPI_A_DATA2 0x5A
+ MX7D_PAD_EPDC_DATA03__QSPI_A_DATA3 0x5A
+ MX7D_PAD_EPDC_DATA05__QSPI_A_SCLK 0x11
+ MX7D_PAD_EPDC_DATA06__QSPI_A_SS0_B 0x54
+ MX7D_PAD_EPDC_DATA07__QSPI_A_SS1_B 0x54
+ /* #QSPI_RESET */
+ MX7D_PAD_EPDC_DATA04__GPIO2_IO4 0x4052
+ >;
+ };
+
pinctrl_usdhc3: usdhc3grp {
fsl,pins = <
MX7D_PAD_SD3_CMD__SD3_CMD 0x59
@@ -217,6 +231,22 @@
};
};
+ {
+ pinctrl-names = "default";
+ pinctrl-0 = <_qspi>;
+ status = "okay";
+
+ flash0: spinor@0 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "jedec,spi-nor";
+ spi-max-frequency = <2900>;
+ spi-rx-bus-width = <4>;
+ spi-tx-bus-width = <4>;
+ reg = <0>;
+ };
+};
+
{
status = "okay";
};
--
2.17.1
[PATCH 12/13] ARM: dts: imx7-mba7: specify USB over-current polarity
Add over-current-active-low to usbotg1. Signed-off-by: Matthias Schiffer --- arch/arm/boot/dts/imx7-mba7.dtsi | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi b/arch/arm/boot/dts/imx7-mba7.dtsi index cd29844bc76b..52b8f66f9982 100644 --- a/arch/arm/boot/dts/imx7-mba7.dtsi +++ b/arch/arm/boot/dts/imx7-mba7.dtsi @@ -610,6 +610,7 @@ srp-disable; hnp-disable; adp-disable; + over-current-active-low; dr_mode = "host"; status = "okay"; }; -- 2.17.1
[PATCH 10/13] ARM: dts: imx7-mba7: add default SPI-NOR flash partition layout
Add the partition layout also used by the bootloader.
Signed-off-by: Matthias Schiffer
---
arch/arm/boot/dts/imx7-mba7.dtsi | 32
1 file changed, 32 insertions(+)
diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi b/arch/arm/boot/dts/imx7-mba7.dtsi
index 9cfaf0a91100..f1e50bcd21a5 100644
--- a/arch/arm/boot/dts/imx7-mba7.dtsi
+++ b/arch/arm/boot/dts/imx7-mba7.dtsi
@@ -237,6 +237,38 @@
};
};
+ {
+ uboot@0 {
+ label = "U-Boot";
+ reg = <0x0 0xd>;
+ };
+
+ env1@d {
+ label = "ENV1";
+ reg = <0xd 0x1>;
+ };
+
+ env2@e {
+ label = "ENV2";
+ reg = <0xe 0x1>;
+ };
+
+ dtb@f {
+ label = "DTB";
+ reg = <0xf 0x1>;
+ };
+
+ linux@10 {
+ label = "Linux";
+ reg = <0x10 0x70>;
+ };
+
+ rootfs@80 {
+ label = "RootFS";
+ reg = <0x80 0x380>;
+ };
+};
+
{
pinctrl-names = "default";
pinctrl-0 = <_flexcan1>;
--
2.17.1
[PATCH 05/13] ARM: dts: imx7-mba7: remove unsupported PHY LED setup
These properties were never supported by the DP83867, and a patch implementing them was rejected in favor of a different solution. Remove them. Signed-off-by: Matthias Schiffer --- arch/arm/boot/dts/imx7-mba7.dtsi | 4 arch/arm/boot/dts/imx7d-mba7.dts | 4 2 files changed, 8 deletions(-) diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi b/arch/arm/boot/dts/imx7-mba7.dtsi index d99912ade947..1af40032ab17 100644 --- a/arch/arm/boot/dts/imx7-mba7.dtsi +++ b/arch/arm/boot/dts/imx7-mba7.dtsi @@ -212,10 +212,6 @@ ti,rx-internal-delay = ; ti,tx-internal-delay = ; ti,fifo-depth = ; - /* LED1: Link/Activity, LED2: Error */ - ti,led-function = <0x0db0>; - /* Active low, LED1 and LED2 driven by phy */ - ti,led-ctrl = <0x1001>; }; }; }; diff --git a/arch/arm/boot/dts/imx7d-mba7.dts b/arch/arm/boot/dts/imx7d-mba7.dts index 9f4f7112e598..1101be373ddf 100644 --- a/arch/arm/boot/dts/imx7d-mba7.dts +++ b/arch/arm/boot/dts/imx7d-mba7.dts @@ -39,10 +39,6 @@ ti,rx-internal-delay = ; ti,tx-internal-delay = ; ti,fifo-depth = ; - /* LED1: Link/Activity, LED2: error */ - ti,led-function = <0x0db0>; - /* active low, LED1/2 driven by phy */ - ti,led-ctrl = <0x1001>; }; }; }; -- 2.17.1
[PATCH 13/13] ARM: dts: imx7-mba7: set dr_mode to otg on usbotg1
USBOTG1 has a Micro-USB port that can be used in host mode (using an OTG cable) or device mode. Signed-off-by: Matthias Schiffer --- arch/arm/boot/dts/imx7-mba7.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi b/arch/arm/boot/dts/imx7-mba7.dtsi index 52b8f66f9982..3683f97f946f 100644 --- a/arch/arm/boot/dts/imx7-mba7.dtsi +++ b/arch/arm/boot/dts/imx7-mba7.dtsi @@ -611,7 +611,7 @@ hnp-disable; adp-disable; over-current-active-low; - dr_mode = "host"; + dr_mode = "otg"; status = "okay"; }; -- 2.17.1
[PATCH 06/13] ARM: dts: imx7-mba7: disable ethernet PHY clock outputs
The clock outputs are not connected. Disable them to improve EMI behaviour. Signed-off-by: Matthias Schiffer --- arch/arm/boot/dts/imx7-mba7.dtsi | 1 + arch/arm/boot/dts/imx7d-mba7.dts | 1 + 2 files changed, 2 insertions(+) diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi b/arch/arm/boot/dts/imx7-mba7.dtsi index 1af40032ab17..9be225bb135a 100644 --- a/arch/arm/boot/dts/imx7-mba7.dtsi +++ b/arch/arm/boot/dts/imx7-mba7.dtsi @@ -212,6 +212,7 @@ ti,rx-internal-delay = ; ti,tx-internal-delay = ; ti,fifo-depth = ; + ti,clk-output-sel = ; }; }; }; diff --git a/arch/arm/boot/dts/imx7d-mba7.dts b/arch/arm/boot/dts/imx7d-mba7.dts index 1101be373ddf..5ef86de53013 100644 --- a/arch/arm/boot/dts/imx7d-mba7.dts +++ b/arch/arm/boot/dts/imx7d-mba7.dts @@ -39,6 +39,7 @@ ti,rx-internal-delay = ; ti,tx-internal-delay = ; ti,fifo-depth = ; + ti,clk-output-sel = ; }; }; }; -- 2.17.1
[PATCH 01/13] dt-bindings: arm: fsl: update TQ-Systems SoMs and boards based on i.MX7
Introduce compatible strings for the TQMa7x SoMs. Signed-off-by: Matthias Schiffer --- Documentation/devicetree/bindings/arm/fsl.yaml | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml index 71acf14da715..d7233cef4d38 100644 --- a/Documentation/devicetree/bindings/arm/fsl.yaml +++ b/Documentation/devicetree/bindings/arm/fsl.yaml @@ -298,7 +298,12 @@ properties: - toradex,colibri-imx7s # Colibri iMX7 Solo Module - toradex,colibri-imx7s-aster # Colibri iMX7 Solo Module on Aster Carrier Board - toradex,colibri-imx7s-eval-v3 # Colibri iMX7 Solo Module on Colibri Evaluation Board V3 - - tq,imx7s-mba7 # i.MX7S TQ MBa7 with TQMa7S SoM + - const: fsl,imx7s + + - description: TQ-Systems TQMa7S SoM on MBa7x board +items: + - const: tq,imx7s-mba7 + - const: tq,imx7s-tqma7 - const: fsl,imx7s - description: i.MX7D based Boards @@ -320,11 +325,16 @@ properties: # Colibri Evaluation Board V3 - toradex,colibri-imx7d-eval-v3 # Colibri iMX7 Dual Module on # Colibri Evaluation Board V3 - - tq,imx7d-mba7 # i.MX7D TQ MBa7 with TQMa7D SoM - zii,imx7d-rmu2# ZII RMU2 Board - zii,imx7d-rpu2# ZII RPU2 Board - const: fsl,imx7d + - description: TQ-Systems TQMa7D SoM on MBa7x board +items: + - const: tq,imx7d-mba7 + - const: tq,imx7d-tqma7 + - const: fsl,imx7d + - description: Compulab SBC-iMX7 is a single board computer based on the Freescale i.MX7 system-on-chip. SBC-iMX7 is implemented with -- 2.17.1
[PATCH 08/13] ARM: dts: imx7-mba7: update MMC aliases
Together with the recently merged support for alias-based MMC host
numbering, this makes the MMC devices names match what the bootloader
expects.
Signed-off-by: Matthias Schiffer
---
arch/arm/boot/dts/imx7-mba7.dtsi | 6 ++
1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi b/arch/arm/boot/dts/imx7-mba7.dtsi
index 84b5809f384c..215730e0453e 100644
--- a/arch/arm/boot/dts/imx7-mba7.dtsi
+++ b/arch/arm/boot/dts/imx7-mba7.dtsi
@@ -14,6 +14,12 @@
#include
/ {
+ aliases {
+ mmc0 =
+ mmc1 =
+ /delete-property/ mmc2;
+ };
+
beeper {
compatible = "gpio-beeper";
gpios = < 0 GPIO_ACTIVE_HIGH>;
--
2.17.1
[PATCH 03/13] ARM: dts: imx7-mba7: update compatible strings
Include the SoM compatible string.
Signed-off-by: Matthias Schiffer
---
arch/arm/boot/dts/imx7d-mba7.dts | 2 +-
arch/arm/boot/dts/imx7s-mba7.dts | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/imx7d-mba7.dts b/arch/arm/boot/dts/imx7d-mba7.dts
index 221274c73dbd..9f4f7112e598 100644
--- a/arch/arm/boot/dts/imx7d-mba7.dts
+++ b/arch/arm/boot/dts/imx7d-mba7.dts
@@ -14,7 +14,7 @@
/ {
model = "TQ Systems TQMa7D board on MBa7 carrier board";
- compatible = "tq,imx7d-mba7", "fsl,imx7d";
+ compatible = "tq,imx7d-mba7", "tq,imx7d-tqma7", "fsl,imx7d";
};
{
diff --git a/arch/arm/boot/dts/imx7s-mba7.dts b/arch/arm/boot/dts/imx7s-mba7.dts
index a143d566a38b..d7d3f530f843 100644
--- a/arch/arm/boot/dts/imx7s-mba7.dts
+++ b/arch/arm/boot/dts/imx7s-mba7.dts
@@ -14,5 +14,5 @@
/ {
model = "TQ Systems TQMa7S board on MBa7 carrier board";
- compatible = "tq,imx7s-mba7", "fsl,imx7s";
+ compatible = "tq,imx7s-mba7", "tq,imx7s-tqma7", "fsl,imx7s";
};
--
2.17.1
[PATCH 04/13] ARM: dts: imx7-mba7: drop incorrect num-chipselects property
This property was never set correctly; it should have been num-cs. As
num-cs support is being removed as well, simply drop it.
Signed-off-by: Matthias Schiffer
---
arch/arm/boot/dts/imx7-mba7.dtsi | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi b/arch/arm/boot/dts/imx7-mba7.dtsi
index 50abf18ad30b..d99912ade947 100644
--- a/arch/arm/boot/dts/imx7-mba7.dtsi
+++ b/arch/arm/boot/dts/imx7-mba7.dtsi
@@ -179,7 +179,6 @@
{
pinctrl-names = "default";
pinctrl-0 = <_ecspi1>;
- num-chipselects = <3>;
cs-gpios = < 0 GPIO_ACTIVE_LOW>, < 1 GPIO_ACTIVE_LOW>,
< 2 GPIO_ACTIVE_LOW>;
status = "okay";
@@ -188,7 +187,6 @@
{
pinctrl-names = "default";
pinctrl-0 = <_ecspi2>;
- num-chipselects = <1>;
status = "okay";
};
--
2.17.1
Re: [PATCH] drm: fsl-dcu: enable PIXCLK on LS1021A
On Fri, 2020-08-21 at 15:41 +0200, Stefan Agner wrote:
> Hi Matthias,
>
> On 2020-08-20 12:58, Matthias Schiffer wrote:
> > The PIXCLK needs to be enabled in SCFG before accessing the DCU on LS1021A,
> > or the access will hang.
>
> Hm, this seems a rather ad-hoc access to SCFG from the DCU. We do
> support a pixel clock in the device tree bindings of fsl-dcu, so ideally
> we should enable the pixel clock through the clock framework.
>
> On the other hand, I guess that would mean adding a clock driver to flip
> a single bit, which seems a bit excessive too.
>
> I'd like a second opinion on that. Adding clk framework maintainers.
>
> --
> Stefan
How do we proceed with this patch?
Kind regards,
Matthias
>
> >
> > Signed-off-by: Matthias Schiffer
> > ---
> > drivers/gpu/drm/fsl-dcu/Kconfig | 1 +
> > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 25 +++
> > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h | 3 +++
> > 3 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig
> > b/drivers/gpu/drm/fsl-dcu/Kconfig
> > index d7dd8ba90e3a..9e5a35e7c00c 100644
> > --- a/drivers/gpu/drm/fsl-dcu/Kconfig
> > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig
> > @@ -8,6 +8,7 @@ config DRM_FSL_DCU
> > select DRM_PANEL
> > select REGMAP_MMIO
> > select VIDEOMODE_HELPERS
> > + select MFD_SYSCON if SOC_LS1021A
> > help
> > Choose this option if you have an Freescale DCU chipset.
> > If M is selected the module will be called fsl-dcu-drm.
> > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> > index abbc1ddbf27f..8a7556655581 100644
> > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> > @@ -51,6 +51,23 @@ static const struct regmap_config fsl_dcu_regmap_config
> > = {
> > .volatile_reg = fsl_dcu_drm_is_volatile_reg,
> > };
> >
> > +static int fsl_dcu_scfg_config_ls1021a(struct device_node *np)
> > +{
> > + struct regmap *scfg;
> > +
> > + scfg = syscon_regmap_lookup_by_compatible("fsl,ls1021a-scfg");
> > + if (IS_ERR(scfg))
> > + return PTR_ERR(scfg);
> > +
> > + /*
> > +* For simplicity, enable the PIXCLK unconditionally. It might
> > +* be possible to disable the clock in PM or on unload as a future
> > +* improvement.
> > +*/
> > + return regmap_update_bits(scfg, SCFG_PIXCLKCR, SCFG_PIXCLKCR_PXCEN,
> > + SCFG_PIXCLKCR_PXCEN);
> > +}
> > +
> > static void fsl_dcu_irq_uninstall(struct drm_device *dev)
> > {
> > struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
> > @@ -70,6 +87,14 @@ static int fsl_dcu_load(struct drm_device *dev,
> > unsigned long flags)
> > return ret;
> > }
> >
> > + if (of_device_is_compatible(fsl_dev->np, "fsl,ls1021a-dcu")) {
> > + ret = fsl_dcu_scfg_config_ls1021a(fsl_dev->np);
> > + if (ret < 0) {
> > + dev_err(dev->dev, "failed to enable pixclk\n");
> > + goto done;
> > + }
> > + }
> > +
> > ret = drm_vblank_init(dev, dev->mode_config.num_crtc);
> > if (ret < 0) {
> > dev_err(dev->dev, "failed to initialize vblank\n");
> > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> > index e2049a0e8a92..566396013c04 100644
> > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> > @@ -160,6 +160,9 @@
> > #define FSL_DCU_ARGB 12
> > #define FSL_DCU_YUV422 14
> >
> > +#define SCFG_PIXCLKCR 0x28
> > +#define SCFG_PIXCLKCR_PXCENBIT(31)
> > +
> > #define VF610_LAYER_REG_NUM9
> > #define LS1021A_LAYER_REG_NUM 10
[PATCH net] net: dsa: microchip: ksz8795: really set the correct number of ports
The KSZ9477 and KSZ8795 use the port_cnt field differently: For the
KSZ9477, it includes the CPU port(s), while for the KSZ8795, it doesn't.
It would be a good cleanup to make the handling of both drivers match,
but as a first step, fix the recently broken assignment of num_ports in
the KSZ8795 driver (which completely broke probing, as the CPU port
index was always failing the num_ports check).
Fixes: af199a1a9cb0 ("net: dsa: microchip: set the correct number of
ports")
Signed-off-by: Matthias Schiffer
---
drivers/net/dsa/microchip/ksz8795.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/dsa/microchip/ksz8795.c
b/drivers/net/dsa/microchip/ksz8795.c
index f7d59d7b2cbe..f5779e152377 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -1260,7 +1260,7 @@ static int ksz8795_switch_init(struct ksz_device *dev)
}
/* set the real number of ports */
- dev->ds->num_ports = dev->port_cnt;
+ dev->ds->num_ports = dev->port_cnt + 1;
return 0;
}
--
2.17.1
[PATCH] i2c: mxs: use MXS_DMA_CTRL_WAIT4END instead of DMA_CTRL_ACK
The driver-specific usage of the DMA_CTRL_ACK flag was replaced with a
custom flag in commit ceeeb99cd821 ("dmaengine: mxs: rename custom flag"),
but i2c-mxs was not updated to use the new flag, completely breaking I2C
transactions using DMA.
Fixes: ceeeb99cd821 ("dmaengine: mxs: rename custom flag")
Signed-off-by: Matthias Schiffer
---
I'm a bit out of my depth here - I have no idea what these flags are
supposed to do. Looking at ceeeb99cd821, this is what I came up with,
and it fixes I2C communication with multiple devices (an EEPROM and a
PCF85063TP RTC) on one of our i.MX28 boards.
I run-tested this on a 5.4.y kernel; given how little is happening in
these drivers nowadays, I assume that the fix is still valid on newer
kernels...
drivers/i2c/busses/i2c-mxs.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 9587347447f0..c4b08a924461 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -25,6 +25,7 @@
#include
#include
#include
+#include
#define DRIVER_NAME "mxs-i2c"
@@ -200,7 +201,8 @@ static int mxs_i2c_dma_setup_xfer(struct i2c_adapter *adap,
dma_map_sg(i2c->dev, >sg_io[0], 1, DMA_TO_DEVICE);
desc = dmaengine_prep_slave_sg(i2c->dmach, >sg_io[0], 1,
DMA_MEM_TO_DEV,
- DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+ DMA_PREP_INTERRUPT |
+ MXS_DMA_CTRL_WAIT4END);
if (!desc) {
dev_err(i2c->dev,
"Failed to get DMA data write descriptor.\n");
@@ -228,7 +230,8 @@ static int mxs_i2c_dma_setup_xfer(struct i2c_adapter *adap,
dma_map_sg(i2c->dev, >sg_io[1], 1, DMA_FROM_DEVICE);
desc = dmaengine_prep_slave_sg(i2c->dmach, >sg_io[1], 1,
DMA_DEV_TO_MEM,
- DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+ DMA_PREP_INTERRUPT |
+ MXS_DMA_CTRL_WAIT4END);
if (!desc) {
dev_err(i2c->dev,
"Failed to get DMA data write descriptor.\n");
@@ -260,7 +263,8 @@ static int mxs_i2c_dma_setup_xfer(struct i2c_adapter *adap,
dma_map_sg(i2c->dev, i2c->sg_io, 2, DMA_TO_DEVICE);
desc = dmaengine_prep_slave_sg(i2c->dmach, i2c->sg_io, 2,
DMA_MEM_TO_DEV,
- DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+ DMA_PREP_INTERRUPT |
+ MXS_DMA_CTRL_WAIT4END);
if (!desc) {
dev_err(i2c->dev,
"Failed to get DMA data write descriptor.\n");
--
2.17.1
Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs
On Thu, 2020-09-10 at 17:00 +0200, Andrew Lunn wrote: > > I propose that at least these HW modes should be available (and > > documented) for ethernet PHY controlled LEDs: > > mode to determine link on: > > - `link` > > mode for activity (these should blink): > > - `activity` (both rx and tx), `rx`, `tx` > > mode for link (on) and activity (blink) > > - `link/activity`, maybe `link/rx` and `link/tx` > > mode for every supported speed: > > - `1Gbps`, `100Mbps`, `10Mbps`, ... > > mode for every supported cable type: > > - `copper`, `fiber`, ... (are there others?) > > In theory, there is AUI and BNC, but no modern device will have > these. > > > mode that allows the user to determine link speed > > - `speed` (or maybe `linkspeed` ?) > > - on some Marvell PHYs the speed can be determined by how fast > > the LED is blinking (ie. 1Gbps blinks with default blinking > > frequency, 100Mbps with half blinking frequeny of 1Gbps, > > 10Mbps > > of half blinking frequency of 100Mbps) > > - on other Marvell PHYs this is instead: > > 1Gpbs blinks 3 times, pause, 3 times, pause, ... > > 100Mpbs blinks 2 times, pause, 2 times, pause, ... > > 10Mpbs blinks 1 time, pause, 1 time, pause, ... > > - we don't need to differentiate these modes with different > > names, > > because the important thing is just that this mode allows the > > user to determine the speed from how the LED blinks > > mode to just force blinking > > - `blink` > > The nice thing is that all this can be documented and done in > > software > > as well. > > Have you checked include/dt-bindings/net/microchip-lan78xx.h and > mscc-phy-vsc8531.h ? If you are defining something generic, we need > to > make sure the majority of PHYs can actually do it. There is no > standardization in this area. I'm sure there is some similarity, > there > is only so many ways you can blink an LED, but i suspect we need a > mixture of standardized modes which we hope most PHYs implement, and > the option to support hardware specific modes. > > Andrew FWIW, these are the LED HW trigger modes supported by the TI DP83867 PHY: - Receive Error - Receive Error or Transmit Error - Link established, blink for transmit or receive activity - Full duplex - 100/1000BT link established - 10/100BT link established - 10BT link established - 100BT link established - 1000BT link established - Collision detected - Receive activity - Transmit activity - Receive or Transmit activity - Link established AFAIK, the "Link established, blink for transmit or receive activity" is the only trigger that involves blinking; all other modes simply make the LED light up when the condition is met. Setting the output level in software is also possible. Regarding the option to emulate unsupported HW triggers in software, two questions come to my mind: - Do all PHYs support manual setting of the LED level, or are the PHYs that can only work with HW triggers? - Is setting PHY registers always efficiently possible, or should SW triggers be avoided in certain cases? I'm thinking about setups like mdio-gpio. I guess this can only become an issue for triggers that blink. Kind regards, Matthias
Re: [PATCH] of: skip disabled CPU nodes
On Thu, 2020-08-27 at 09:10 +0200, Matthias Schiffer wrote: > On Wed, 2020-08-26 at 13:26 -0600, Rob Herring wrote: > > On Wed, Aug 26, 2020 at 8:47 AM Frank Rowand < > > frowand.l...@gmail.com> > > wrote: > > > > > > Hi Rob, > > > > > > On 2020-08-26 08:54, Matthias Schiffer wrote: [snip] > > > > > > > > Hmm, I see. This difference in behaviour is quite unfortunate, > > > > as > > > > I'm > > > > currently looking for a way to *really* disable a CPU core. > > > > > > > > In arch/arm64/boot/dts/freescale/imx8mn.dtsi (and other > > > > variants > > > > of the > > > > i.MX8M), there are 4 CPU nodes for the full-featured quad-core > > > > version. > > > > The reduced single- and dual-core versions are currently > > > > handled > > > > in > > > > NXP's U-Boot fork by deleting the additional nodes. > > > > > > > > Not doing so causes the kernel to hang for a while when trying > > > > to > > > > online the non-existent cores during boot (at least in linux- > > > > imx > > > > 5.4 - > > > > I have not checked a more recent mainline kernel yet), but the > > > > deletion > > > > is non-trivial to do without leaving dangling phandle > > > > references. > > > > > > Any thoughts on implementing another universal property that > > > means > > > something like "the hardware described by this node does not > > > exist > > > or is so broken that you better not use it". > > > > There's a couple of options: > > > > The DT spec defines 'fail' value for status. We could use that > > instead > > of 'disabled'. > > > > The spec behavior with cpu 'disabled' is only on PPC AFAIK. On > > arm/arm64 (probably riscv now too) we've never followed it where we > > online 'disabled' CPUs. So we could just make the check conditional > > on > > !IS_ENABLED(CONFIG_PPC). This would need some spec update. > > On ARM(64), the "disabled" status on CPUs doesn't have any effect. I > assume this changed with the mentioned commit c961cb3be906 "of: Fix > cpu > node iterator to not ignore disabled cpu nodes", as reverting it > gives > me the desired behaviour of considering the disabled CPUs non- > existent. > > So it seems that we already changed the interpretation in a non- > compatible way once (back in v4.20), and somehow PPC has yet another > different behaviour? > > How do we get out of this mess? Is going back to the v4.19 logic for > non-PPC platforms an acceptable regression fix, or would this be > considered another breaking change? > Any new insights on this? I'll gladly provide patches or proposals for a spec update if we can decide where we want to go with this. Kind regards, Matthias
[PATCH v3 2/3] ARM: dts: imx6qdl: tqma6: remove obsolete fsl,spi-num-chipselects
This property is unneeded and not supported by the spi-imx driver.
Fixes: cac849e9bbc8 ("ARM: dts: imx6qdl: add TQMa6{S,Q,QP} SoM")
Signed-off-by: Matthias Schiffer
---
v3: split patch
v2: drop fsl,spi-num-chipselects instead of replacing with num-cs
arch/arm/boot/dts/imx6qdl-tqma6.dtsi | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
index 9513020ddd1a..b18b83ac6aee 100644
--- a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
@@ -20,7 +20,6 @@
{
pinctrl-names = "default";
pinctrl-0 = <_ecspi1>;
- fsl,spi-num-chipselects = <1>;
cs-gpios = < 19 GPIO_ACTIVE_HIGH>;
status = "okay";
--
2.17.1
[PATCH v3 3/3] ARM: dts: imx6qdl: tqma6: fix LM75 compatible string
Specify the National LM75 sensor including its vendor name, as mandated
by the binding docs.
Fixes: cac849e9bbc8 ("ARM: dts: imx6qdl: add TQMa6{S,Q,QP} SoM")
Signed-off-by: Matthias Schiffer
---
v3: split patch
arch/arm/boot/dts/imx6qdl-tqma6a.dtsi | 2 +-
arch/arm/boot/dts/imx6qdl-tqma6b.dtsi | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi
b/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi
index c18a06cf7929..b679bec78e6c 100644
--- a/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi
@@ -16,7 +16,7 @@
};
sensor@48 {
- compatible = "lm75";
+ compatible = "national,lm75";
reg = <0x48>;
};
diff --git a/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi
b/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi
index a7460075f517..49c472285c06 100644
--- a/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi
@@ -16,7 +16,7 @@
};
sensor@48 {
- compatible = "lm75";
+ compatible = "national,lm75";
reg = <0x48>;
};
--
2.17.1
[PATCH v3 1/3] ARM: dts: imx6qdl: tqma6: fix indentation
The PMIC configuration is indented one level too deep.
Fixes: cac849e9bbc8 ("ARM: dts: imx6qdl: add TQMa6{S,Q,QP} SoM")
Signed-off-by: Matthias Schiffer
---
v3: extended commit message
v2: no changes
arch/arm/boot/dts/imx6qdl-tqma6.dtsi | 188 +--
1 file changed, 94 insertions(+), 94 deletions(-)
diff --git a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
index 29bcce20f5f3..9513020ddd1a 100644
--- a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
@@ -83,101 +83,101 @@
};
{
- pinctrl-names = "default";
- pinctrl-0 = <_pmic>;
- interrupt-parent = <>;
- interrupts = <10 IRQ_TYPE_LEVEL_LOW>;
-
- regulators {
- reg_vddcore: sw1ab {
- regulator-min-microvolt = <30>;
- regulator-max-microvolt = <1875000>;
- regulator-always-on;
- };
-
- reg_vddsoc: sw1c {
- regulator-min-microvolt = <30>;
- regulator-max-microvolt = <1875000>;
- regulator-always-on;
- };
-
- reg_gen_3v3: sw2 {
- regulator-min-microvolt = <80>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
-
- reg_ddr_1v5a: sw3a {
- regulator-min-microvolt = <40>;
- regulator-max-microvolt = <1975000>;
- regulator-always-on;
- };
-
- reg_ddr_1v5b: sw3b {
- regulator-min-microvolt = <40>;
- regulator-max-microvolt = <1975000>;
- regulator-always-on;
- };
-
- sw4_reg: sw4 {
- regulator-min-microvolt = <80>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
-
- reg_5v_600mA: swbst {
- regulator-min-microvolt = <500>;
- regulator-max-microvolt = <515>;
- regulator-always-on;
- };
-
- reg_snvs_3v: vsnvs {
- regulator-min-microvolt = <150>;
- regulator-max-microvolt = <300>;
- regulator-always-on;
- };
-
- reg_vrefddr: vrefddr {
- regulator-boot-on;
- regulator-always-on;
- };
-
- reg_vgen1_1v5: vgen1 {
- regulator-min-microvolt = <80>;
- regulator-max-microvolt = <155>;
- /* not used */
- };
-
- reg_vgen2_1v2_eth: vgen2 {
- regulator-min-microvolt = <80>;
- regulator-max-microvolt = <155>;
- regulator-always-on;
- };
-
- reg_vgen3_2v8: vgen3 {
- regulator-min-microvolt = <180>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
-
- reg_vgen4_1v8: vgen4 {
- regulator-min-microvolt = <180>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
-
- reg_vgen5_1v8_eth: vgen5 {
- regulator-min-microvolt = <180>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
-
- reg_vgen6_3v3: vgen6 {
- regulator-min-microvolt = <180>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
+ pinctrl-names = "default";
+ pinctrl-0 = <_pmic>;
+ interrupt-parent = <&g
Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH] spi-imx: remove num-cs support, set num_chipselect to 4
On Fri, 2020-09-04 at 12:42 -0300, Fabio Estevam wrote:
> Hi Mark,
>
> On Fri, Sep 4, 2020 at 12:05 PM Mark Brown
> wrote:
> >
> > On Fri, Sep 04, 2020 at 04:34:43PM +0200, Matthias Schiffer wrote:
> >
> > > Nevertheless, I don't see why we should deliberately remove the
> > > native
> > > CS support - my understanding was that we try to avoid breaking
> > > changes
> > > to DT interpretation even for unknown/out-of-tree DTS.
> >
> > Yes, we should try to maintain compatibility for anyone that was
> > using
> > it.
>
> We are not breaking compatibility.
>
> Prior to 8cdcd8aeee281 ("spi: imx/fsl-lpspi: Convert to GPIO
> descriptors") num_chipselect was 1 for all device tree users.
> i.MX board files will be removed, so we don't need to worry about
> them.
>
> What will cause breakage is to say that the driver supports the
> native
> chip-select.
>
> I have just done a quick test on an imx6q-sabresd.
>
> With the original dts that uses cs-gpios the SPI NOR is correctly
> probed:
>
> [5.402627] spi-nor spi0.0: m25p32 (4096 Kbytes)
>
> However, using native chip select with this dts change:
>
> --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> @@ -189,7 +189,6 @@
> };
>
> {
> - cs-gpios = < 9 GPIO_ACTIVE_LOW>;
> pinctrl-names = "default";
> pinctrl-0 = <_ecspi1>;
> status = "okay";
> @@ -506,7 +505,7 @@
> MX6QDL_PAD_KEY_COL1__ECSPI1_MISO
>0x100b1
> MX6QDL_PAD_KEY_ROW0__ECSPI1_MOSI
>0x100b1
> MX6QDL_PAD_KEY_COL0__ECSPI1_SCLK
>0x100b1
> - MX6QDL_PAD_KEY_ROW1__GPIO4_IO09
>0x1b0b0
> + MX6QDL_PAD_KEY_ROW1__ECSPI1_SS0
>0x1b0b0
> >;
> };
>
> Causes SPI NOR probe to fail:
>
> [5.388704] spi-nor spi0.0: unrecognized JEDEC id bytes: 00 00 00
> 00 00 00
>
> That's why I prefer we do not advertise that we can use the native
> chip-select with this driver.
My rationale here is the following: As broken as the native CS of these
controllers is, it isn't an unreasonable assumption that it is working
fine with *some* devices or for some usecases - after all the support
was implemented at some point, and has existed for a long time now. If
we really want to remove this feature, a deprecation period with a
warning message seems like the proper way to deal with this.
Hypothetically, existing out-of-tree DTS could have used the native CS
with num-cs set to 4. Always setting num_chipselect to 4 ensures that
we don't break such DTS with the num-cs removal.
Kind regards,
Matthias
Re: (EXT) Re: (EXT) Re: [PATCH] spi-imx: remove num-cs support, set num_chipselect to 4
On Fri, 2020-09-04 at 10:57 -0300, Fabio Estevam wrote: > On Fri, Sep 4, 2020 at 10:02 AM Matthias Schiffer > wrote: > > > This would make num_chipselect default to 1 again (set by > > __spi_alloc_controller()), breaking every i.MX board that uses more > > than 1 native CS. > > Which boards are that? Are you referring to non-DT i.MX boards? > > If so, I have sent a patch removing all non-DT i.MX boards. > > > I'm aware that using cs-gpios instead of native CS is probably a > > good > > idea in any case, as the native CS of this SPI controller is kinda > > flaky (and at a glance it looks like all in-tree DTs do this; not > > sure > > about board files that don't use DTs?), but I'm not convinced that > > breaking native CS support completely is desirable either. > > Initial i.MX chips with this SPI IP had issues in using chip-select > in > native mode and GPIO chip-select has been used since them. > > Do we have i.MX dts that make use of native chip select? As mentioned in my previous mail, all in-tree DTS use cs-gpios (unless I've overlooked something - I grepped for CSPIn_SSm pinmuxings), so removing support for native CS should not break anything we know of. Nevertheless, I don't see why we should deliberately remove the native CS support - my understanding was that we try to avoid breaking changes to DT interpretation even for unknown/out-of-tree DTS.
Re: (EXT) Re: [PATCH] spi-imx: remove num-cs support, set num_chipselect to 4
On Fri, 2020-09-04 at 09:35 -0300, Fabio Estevam wrote:
> Hi Matthias,
>
> On Thu, Sep 3, 2020 at 11:40 AM Matthias Schiffer
> wrote:
> >
> > The num-cs property is not considered useful, and no in-tree Device
> > Trees define it for spi-imx.
> >
> > The default value to be used when no cs-gpios are defined is set to
> > 4 to
> > give access to all native CS pins of modern i.MX SoCs (i.MX6 and
> > newer).
> >
> > In older SoCs, the number of CS pins varies (for example the i.MX27
> > has 3
> > CS pins on CSPI1 and CSPI2, and only a single CS on CSPI3).
> > Attempting
> > to use the nonexisting CS pin would be an easy to notice DT
> > misconfiguration; making the driver catch this doesn't seem
> > worthwhile.
> >
> > Signed-off-by: Matthias Schiffer > >
> > ---
> > drivers/spi/spi-imx.c | 13 +
> > 1 file changed, 1 insertion(+), 12 deletions(-)
> >
> > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> > index 197f60632072..aece8482739b 100644
> > --- a/drivers/spi/spi-imx.c
> > +++ b/drivers/spi/spi-imx.c
> > @@ -1581,7 +1581,6 @@ static int spi_imx_probe(struct
> > platform_device *pdev)
> > const struct spi_imx_devtype_data *devtype_data = of_id ?
> > of_id->data :
> > (struct spi_imx_devtype_data *)pdev->id_entry-
> > >driver_data;
> > bool slave_mode;
> > - u32 val;
> >
> > slave_mode = devtype_data->has_slavemode &&
> > of_property_read_bool(np, "spi-slave");
> > @@ -1605,6 +1604,7 @@ static int spi_imx_probe(struct
> > platform_device *pdev)
> > master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
> > master->bus_num = np ? -1 : pdev->id;
> > master->use_gpio_descriptors = true;
> > + master->num_chipselect = 4;
>
> On an imx6q-sabresd, which only has one SPI chip-select via GPIO,
> this
> makes the SPI core to understand that it has 4 chip selects.
>
> From spi_get_gpio_descs() in drivers/spi/spi.c:
>
> ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect);
>
> It is 4 now after your patch, it was 3 after 8cdcd8aeee28 ("spi:
> imx/fsl-lpspi: Convert to GPIO descriptors") and 1 before such
> commit.
>
> Couldn't we just remove master->num_chipselect from the spi-imx.c
> driver?
This would make num_chipselect default to 1 again (set by
__spi_alloc_controller()), breaking every i.MX board that uses more
than 1 native CS.
I'm aware that using cs-gpios instead of native CS is probably a good
idea in any case, as the native CS of this SPI controller is kinda
flaky (and at a glance it looks like all in-tree DTs do this; not sure
about board files that don't use DTs?), but I'm not convinced that
breaking native CS support completely is desirable either.
Re: spi-imx: correct interpretation of num-cs DT property?
On Thu, 2020-09-03 at 14:22 +0100, Mark Brown wrote: > * PGP Signed by an unknown key > > On Thu, Sep 03, 2020 at 11:22:20AM +0200, Matthias Schiffer wrote: > > > - If num-cs is set, use that > > - If num-cs is unset, use the number of cs-gpios > > - If num-cs is unset and no cs-gpios are defined, use a driver- > > provided > > default (which is 3 for spi-imx; this matches the number of native > > CS > > pins in older implementations of this SPI controller; i.MX6 and > > newer > > support up to 4) > > That sounds like what's expected, though we coould just skip the > first > step. > > > Also, would it make sense to add num-cs to all DTS files for boards > > that actually use fewer than 3 CS pins? > > No, it was never a good idea to have that property in the first place > and there should be no case where it helps anything. Oh, thank you for the clarification. As currently no in-tree DTs use the num-cs property for spi-imx and it's not documented, should support for it be dropped from the driver altogether? > > > At the moment, the num-cs property is not explicitly documented for > > the > > spi-imx driver, although the driver understands it. I also > > suggested to > > add this to the docs, which Fabio didn't deem a good idea (I don't > > quite understand the reasoning here - isn't num-cs generally a > > useful > > property to have?) > > Could you explain what benefit you would expect having num-cs to > offer? > > * Unknown Key > * 0x5D5487D0
[PATCH v2 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
- Fix national,lm75 compatible string
- Remove obsolete fsl,spi-num-chipselects
Fixes: cac849e9bbc8 ("ARM: dts: imx6qdl: add TQMa6{S,Q,QP} SoM")
Signed-off-by: Matthias Schiffer
---
v2: drop fsl,spi-num-chipselects instead of replacing with num-cs
arch/arm/boot/dts/imx6qdl-tqma6.dtsi | 1 -
arch/arm/boot/dts/imx6qdl-tqma6a.dtsi | 2 +-
arch/arm/boot/dts/imx6qdl-tqma6b.dtsi | 2 +-
3 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
index 9513020ddd1a..b18b83ac6aee 100644
--- a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
@@ -20,7 +20,6 @@
{
pinctrl-names = "default";
pinctrl-0 = <_ecspi1>;
- fsl,spi-num-chipselects = <1>;
cs-gpios = < 19 GPIO_ACTIVE_HIGH>;
status = "okay";
diff --git a/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi
b/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi
index c18a06cf7929..b679bec78e6c 100644
--- a/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi
@@ -16,7 +16,7 @@
};
sensor@48 {
- compatible = "lm75";
+ compatible = "national,lm75";
reg = <0x48>;
};
diff --git a/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi
b/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi
index a7460075f517..49c472285c06 100644
--- a/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi
@@ -16,7 +16,7 @@
};
sensor@48 {
- compatible = "lm75";
+ compatible = "national,lm75";
reg = <0x48>;
};
--
2.17.1
[PATCH v2 1/2] ARM: dts: imx6qdl: tqma6: fix indentation
Fixes: cac849e9bbc8 ("ARM: dts: imx6qdl: add TQMa6{S,Q,QP} SoM")
Signed-off-by: Matthias Schiffer
---
v2: no changes
arch/arm/boot/dts/imx6qdl-tqma6.dtsi | 188 +--
1 file changed, 94 insertions(+), 94 deletions(-)
diff --git a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
index 29bcce20f5f3..9513020ddd1a 100644
--- a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
@@ -83,101 +83,101 @@
};
{
- pinctrl-names = "default";
- pinctrl-0 = <_pmic>;
- interrupt-parent = <>;
- interrupts = <10 IRQ_TYPE_LEVEL_LOW>;
-
- regulators {
- reg_vddcore: sw1ab {
- regulator-min-microvolt = <30>;
- regulator-max-microvolt = <1875000>;
- regulator-always-on;
- };
-
- reg_vddsoc: sw1c {
- regulator-min-microvolt = <30>;
- regulator-max-microvolt = <1875000>;
- regulator-always-on;
- };
-
- reg_gen_3v3: sw2 {
- regulator-min-microvolt = <80>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
-
- reg_ddr_1v5a: sw3a {
- regulator-min-microvolt = <40>;
- regulator-max-microvolt = <1975000>;
- regulator-always-on;
- };
-
- reg_ddr_1v5b: sw3b {
- regulator-min-microvolt = <40>;
- regulator-max-microvolt = <1975000>;
- regulator-always-on;
- };
-
- sw4_reg: sw4 {
- regulator-min-microvolt = <80>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
-
- reg_5v_600mA: swbst {
- regulator-min-microvolt = <500>;
- regulator-max-microvolt = <515>;
- regulator-always-on;
- };
-
- reg_snvs_3v: vsnvs {
- regulator-min-microvolt = <150>;
- regulator-max-microvolt = <300>;
- regulator-always-on;
- };
-
- reg_vrefddr: vrefddr {
- regulator-boot-on;
- regulator-always-on;
- };
-
- reg_vgen1_1v5: vgen1 {
- regulator-min-microvolt = <80>;
- regulator-max-microvolt = <155>;
- /* not used */
- };
-
- reg_vgen2_1v2_eth: vgen2 {
- regulator-min-microvolt = <80>;
- regulator-max-microvolt = <155>;
- regulator-always-on;
- };
-
- reg_vgen3_2v8: vgen3 {
- regulator-min-microvolt = <180>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
-
- reg_vgen4_1v8: vgen4 {
- regulator-min-microvolt = <180>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
-
- reg_vgen5_1v8_eth: vgen5 {
- regulator-min-microvolt = <180>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
-
- reg_vgen6_3v3: vgen6 {
- regulator-min-microvolt = <180>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
+ pinctrl-names = "default";
+ pinctrl-0 = <_pmic>;
+ interrupt-parent = <>;
+ interrupts = <10 IRQ_TYPE_LEVEL_LOW>;
+
+ regulators {
+
[PATCH] spi-imx: remove num-cs support, set num_chipselect to 4
The num-cs property is not considered useful, and no in-tree Device Trees define it for spi-imx. The default value to be used when no cs-gpios are defined is set to 4 to give access to all native CS pins of modern i.MX SoCs (i.MX6 and newer). In older SoCs, the number of CS pins varies (for example the i.MX27 has 3 CS pins on CSPI1 and CSPI2, and only a single CS on CSPI3). Attempting to use the nonexisting CS pin would be an easy to notice DT misconfiguration; making the driver catch this doesn't seem worthwhile. Signed-off-by: Matthias Schiffer --- drivers/spi/spi-imx.c | 13 + 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index 197f60632072..aece8482739b 100644 --- a/drivers/spi/spi-imx.c +++ b/drivers/spi/spi-imx.c @@ -1581,7 +1581,6 @@ static int spi_imx_probe(struct platform_device *pdev) const struct spi_imx_devtype_data *devtype_data = of_id ? of_id->data : (struct spi_imx_devtype_data *)pdev->id_entry->driver_data; bool slave_mode; - u32 val; slave_mode = devtype_data->has_slavemode && of_property_read_bool(np, "spi-slave"); @@ -1605,6 +1604,7 @@ static int spi_imx_probe(struct platform_device *pdev) master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32); master->bus_num = np ? -1 : pdev->id; master->use_gpio_descriptors = true; + master->num_chipselect = 4; spi_imx = spi_master_get_devdata(master); spi_imx->bitbang.master = master; @@ -1613,17 +1613,6 @@ static int spi_imx_probe(struct platform_device *pdev) spi_imx->devtype_data = devtype_data; - /* -* Get number of chip selects from device properties. This can be -* coming from device tree or boardfiles, if it is not defined, -* a default value of 3 chip selects will be used, as all the legacy -* board files have <= 3 chip selects. -*/ - if (!device_property_read_u32(>dev, "num-cs", )) - master->num_chipselect = val; - else - master->num_chipselect = 3; - spi_imx->bitbang.setup_transfer = spi_imx_setupxfer; spi_imx->bitbang.txrx_bufs = spi_imx_transfer; spi_imx->bitbang.master->setup = spi_imx_setup; -- 2.17.1
spi-imx: correct interpretation of num-cs DT property?
Hi,
I've recently had a discussion about the correct way for an SPI driver
to handle the num-cs property: https://lkml.org/lkml/2020/8/25/184
Since 8cdcd8aeee281 ("spi: imx/fsl-lpspi: Convert to GPIO
descriptors"), the logic of the spi-imx driver is somewhat confusing:
If fewer than 3 cs-gpios are defined, and no explicit num-cs property
exists, the driver will set num_chipselect to 3 by default, instead of
the number of cs-gpios entries.
To avoid having to specify num-cs when the number cs-gpios would
suffice, I suggested to modify the logic to the following:
- If num-cs is set, use that
- If num-cs is unset, use the number of cs-gpios
- If num-cs is unset and no cs-gpios are defined, use a driver-provided
default (which is 3 for spi-imx; this matches the number of native CS
pins in older implementations of this SPI controller; i.MX6 and newer
support up to 4)
Also, would it make sense to add num-cs to all DTS files for boards
that actually use fewer than 3 CS pins?
At the moment, the num-cs property is not explicitly documented for the
spi-imx driver, although the driver understands it. I also suggested to
add this to the docs, which Fabio didn't deem a good idea (I don't
quite understand the reasoning here - isn't num-cs generally a useful
property to have?)
Kind regards,
Matthias
[PATCH 1/2] ASoC: codec: tlv320aic32x4: fix missing aic32x4_disable_regulators() in error path
The regulators need to be disabled in the aic32x4_register_clocks()
failure case as well.
Fixes: 9d4befff5a95 ("ASoC: codec: tlv3204: Moving GPIO reset and add ADC
reset")
Signed-off-by: Matthias Schiffer
---
sound/soc/codecs/tlv320aic32x4.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/tlv320aic32x4.c b/sound/soc/codecs/tlv320aic32x4.c
index 8dcea566b375..a45fb496082c 100644
--- a/sound/soc/codecs/tlv320aic32x4.c
+++ b/sound/soc/codecs/tlv320aic32x4.c
@@ -1230,8 +1230,7 @@ int aic32x4_probe(struct device *dev, struct regmap
*regmap)
_component_dev_aic32x4, _dai, 1);
if (ret) {
dev_err(dev, "Failed to register component\n");
- aic32x4_disable_regulators(aic32x4);
- return ret;
+ goto err_disable_regulators;
}
if (gpio_is_valid(aic32x4->rstn_gpio)) {
@@ -1242,9 +1241,14 @@ int aic32x4_probe(struct device *dev, struct regmap
*regmap)
ret = aic32x4_register_clocks(dev, aic32x4->mclk_name);
if (ret)
- return ret;
+ goto err_disable_regulators;
return 0;
+
+err_disable_regulators:
+ aic32x4_disable_regulators(aic32x4);
+
+ return ret;
}
EXPORT_SYMBOL(aic32x4_probe);
--
2.17.1
[PATCH 2/2] ASoC: codec: tlv320aic32x4: do software reset before clock registration
To avoid the actual PLL settings to differ from the state expected by
the clock driver, the codec should only be fully reset before the clocks
are registered. But we also need to ensure that the software reset
happens at all before clock registration, as not all boards have a reset
GPIO.
Move the software reset from aic32x4_component_probe() to
aic32x4_probe() and reorder the reset and registration sequence:
1. Reset via GPIO (if available)
2. Reset via software
3. Register component
4. Register clocks
Note that aic32x4_component_probe() is only called after aic32x4_probe()
has finished, so the reset in aic32x4_component_probe() was happening too
late.
Signed-off-by: Matthias Schiffer
---
sound/soc/codecs/tlv320aic32x4.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/sound/soc/codecs/tlv320aic32x4.c b/sound/soc/codecs/tlv320aic32x4.c
index a45fb496082c..470dc0ef0359 100644
--- a/sound/soc/codecs/tlv320aic32x4.c
+++ b/sound/soc/codecs/tlv320aic32x4.c
@@ -981,8 +981,6 @@ static int aic32x4_component_probe(struct snd_soc_component
*component)
if (ret)
return ret;
- snd_soc_component_write(component, AIC32X4_RESET, 0x01);
-
if (aic32x4->setup)
aic32x4_setup_gpios(component);
@@ -1226,6 +1224,16 @@ int aic32x4_probe(struct device *dev, struct regmap
*regmap)
return ret;
}
+ if (gpio_is_valid(aic32x4->rstn_gpio)) {
+ ndelay(10);
+ gpio_set_value_cansleep(aic32x4->rstn_gpio, 1);
+ mdelay(1);
+ }
+
+ ret = regmap_write(regmap, AIC32X4_RESET, 0x01);
+ if (ret)
+ goto err_disable_regulators;
+
ret = devm_snd_soc_register_component(dev,
_component_dev_aic32x4, _dai, 1);
if (ret) {
@@ -1233,12 +1241,6 @@ int aic32x4_probe(struct device *dev, struct regmap
*regmap)
goto err_disable_regulators;
}
- if (gpio_is_valid(aic32x4->rstn_gpio)) {
- ndelay(10);
- gpio_set_value_cansleep(aic32x4->rstn_gpio, 1);
- mdelay(1);
- }
-
ret = aic32x4_register_clocks(dev, aic32x4->mclk_name);
if (ret)
goto err_disable_regulators;
--
2.17.1
[PATCH mmc-next v4 2/2] mmc: allow setting slot index via device tree alias
As with GPIO, UART and others, allow specifying the device index via the
aliases node in the device tree.
On embedded devices, there is often a combination of removable (e.g.
SD card) and non-removable MMC devices (e.g. eMMC).
Therefore the index might change depending on
* host of removable device
* removable card present or not
This makes it difficult to hardcode the root device, if it is on the
non-removable device. E.g. if SD card is present eMMC will be mmcblk1,
if SD card is not present at boot, eMMC will be mmcblk0.
Alternative solutions like PARTUUIDs do not cover the case where multiple
mmcblk devices contain the same image. This is a common issue on devices
that can boot both from eMMC (for regular boot) and SD cards (as a
temporary boot medium for development). When a firmware image is
installed to eMMC after a test boot via SD card, there will be no
reliable way to refer to a specific device using (PART)UUIDs oder
LABELs.
The demand for this feature has led to multiple attempts to implement
it, dating back at least to 2012 (see
https://www.spinics.net/lists/linux-mmc/msg26586.html for a previous
discussion from 2014).
All indices defined in the aliases node will be reserved for use by the
respective MMC device, moving the indices of devices that don't have an
alias up into the non-reserved range. If the aliases node is not found,
the driver will act as before.
This is a rebased and cleaned up version of
https://www.spinics.net/lists/linux-mmc/msg26588.html .
Based-on-patch-by: Sascha Hauer
Link: https://lkml.org/lkml/2020/8/5/194
Signed-off-by: Matthias Schiffer
---
v4:
- minor adjustments to commit message
v3:
- remove unneeded mmcblock changes
- remove most helper functions to simplify code
- extended commit message
v2:
- fix missing symbols for modular mmcblock
drivers/mmc/core/host.c | 26 +-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index c8fae6611b73..96b2ca1f1b06 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -376,6 +376,20 @@ int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
}
EXPORT_SYMBOL(mmc_of_parse_voltage);
+/**
+ * mmc_first_nonreserved_index() - get the first index that is not reserved
+ */
+static int mmc_first_nonreserved_index(void)
+{
+ int max;
+
+ max = of_alias_get_highest_id("mmc");
+ if (max < 0)
+ return 0;
+
+ return max + 1;
+}
+
/**
* mmc_alloc_host - initialise the per-host structure.
* @extra: sizeof private data structure
@@ -387,6 +401,7 @@ struct mmc_host *mmc_alloc_host(int extra, struct device
*dev)
{
int err;
struct mmc_host *host;
+ int alias_id, min_idx, max_idx;
host = kzalloc(sizeof(struct mmc_host) + extra, GFP_KERNEL);
if (!host)
@@ -395,7 +410,16 @@ struct mmc_host *mmc_alloc_host(int extra, struct device
*dev)
/* scanning will be enabled when we're ready */
host->rescan_disable = 1;
- err = ida_simple_get(_host_ida, 0, 0, GFP_KERNEL);
+ alias_id = of_alias_get_id(dev->of_node, "mmc");
+ if (alias_id >= 0) {
+ min_idx = alias_id;
+ max_idx = alias_id + 1;
+ } else {
+ min_idx = mmc_first_nonreserved_index();
+ max_idx = 0;
+ }
+
+ err = ida_simple_get(_host_ida, min_idx, max_idx, GFP_KERNEL);
if (err < 0) {
kfree(host);
return NULL;
--
2.17.1
[PATCH mmc-next v4 1/2] dt-bindings: mmc: document alias support
As for I2C and SPI, it now is possible to reserve a fixed index for mmc/mmcblk devices. Signed-off-by: Matthias Schiffer --- v4: moved alias documentation from example to description v3: new patch Documentation/devicetree/bindings/mmc/mmc-controller.yaml | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml index b96da0c7f819..f928f66fc59a 100644 --- a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml +++ b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml @@ -14,6 +14,10 @@ description: | that requires the respective functionality should implement them using these definitions. + It is possible to assign a fixed index mmcN to an MMC host controller + (and the corresponding mmcblkN devices) by defining an alias in the + /aliases device tree node. + properties: $nodename: pattern: "^mmc(@.*)?$" -- 2.17.1
Re: [PATCH mmc-next v3 1/2] dt-bindings: mmc: add alias example
On Fri, 2020-08-28 at 16:24 -0600, Rob Herring wrote:
> On Tue, Aug 25, 2020 at 03:44:40PM +0200, Matthias Schiffer wrote:
> > As for I2C and SPI, it now is possible to reserve a fixed index for
> > mmc/mmcblk devices.
> >
> > Signed-off-by: Matthias Schiffer > >
> > ---
> >
> > v3: new patch
> >
> > Documentation/devicetree/bindings/mmc/mmc-controller.yaml | 8
> >
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/mmc-
> > controller.yaml b/Documentation/devicetree/bindings/mmc/mmc-
> > controller.yaml
> > index b96da0c7f819..22ed4a36c65d 100644
> > --- a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> > @@ -367,6 +367,14 @@ examples:
> > };
> >
> >- |
> > +/*
> > + * Optionally define an alias to reserve a fixed index for the
> > + * mmc and mmcblk devices
> > + */
> > +aliases {
> > +mmc0 =
> > +};
>
> This will break if we improve schemas because this node is actually
> /example-1/aliases.
>
> So please drop. If you want, I'd really like to have a defined set
> (i.e.
> a schema) of alias names. This would require deleting a bunch on
> some
> platforms that just made up a bunch of them.
Ulf suggested that I add some kind of documentation about the new mmc
alias support to the binding docs.
As long as we don't have a proper schema for aliases, should I just add
an explanation to the toplevel description of
Documentation/devicetree/bindings/mmc/mmc-controller.yaml, or maybe a
comment?
>
> > +
> > mmc3: mmc@1c12000 {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > --
> > 2.17.1
> >
Re: [PATCH 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
On Wed, 2020-08-26 at 10:49 -0300, Fabio Estevam wrote: > On Wed, Aug 26, 2020 at 10:13 AM Matthias Schiffer > wrote: > > > Using GPIOs for chipselect would require different pinmuxing. Also, > > why > > use GPIOs, when the SPI controller has this built in? > > In the initial chips with the ECSPI controller there was a bug with > the native chipselect controller and we had to use GPIO. Ah, I see. Nevertheless, hardware that uses the native chipselects of newer chips exists (for example our TQ-Systems starterkit mainboards, the DTS of which we're currently preparing for mainline submission). Shouldn't num-cs be set for boards (or SoM DTSI) where not all CS pins of the SoC are usable? In any case, my original question was about the intended logic for num_chipselects for SPI drivers. My proposal was: - If num-cs is set, use that - If num-cs is unset, use the number of cs-gpios - If num-cs is unset and no cs-gpios are defined, use a driver-provided default How do other SPI controller drivers deal with this?
Re: (EXT) Re: (EXT) Re: [PATCH] of: skip disabled CPU nodes
On Wed, 2020-08-26 at 13:26 -0600, Rob Herring wrote:
> On Wed, Aug 26, 2020 at 8:47 AM Frank Rowand
> wrote:
> >
> > Hi Rob,
> >
> > On 2020-08-26 08:54, Matthias Schiffer wrote:
> > > On Wed, 2020-08-26 at 08:01 -0500, Frank Rowand wrote:
> > > > On 2020-08-26 07:02, Matthias Schiffer wrote:
> > > > > Allow disabling CPU nodes using status = "disabled".
> > > > >
> > > > > This allows a bootloader to change the number of available
> > > > > CPUs
> > > > > (for
> > > > > example when a common DTS is used for SoC variants with
> > > > > different
> > > > > numbers
> > > > > of cores) without deleting the nodes altogether (which may
> > > > > require
> > > > > additional fixups where the CPU nodes are referenced, e.g. a
> > > > > cooling
> > > > > map).
> > > > >
> > > > > Signed-off-by: Matthias Schiffer <
> > > > > matthias.schif...@ew.tq-group.com
> > > > > >
> > > > >
> > > > > ---
> > > > > drivers/of/base.c | 2 ++
> > > > > 1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > > > > index ea44fea99813..d547e9deced1 100644
> > > > > --- a/drivers/of/base.c
> > > > > +++ b/drivers/of/base.c
> > > > > @@ -796,6 +796,8 @@ struct device_node
> > > > > *of_get_next_cpu_node(struct
> > > > > device_node *prev)
> > > > > of_node_put(node);
> > > > > }
> > > > > for (; next; next = next->sibling) {
> > > > > + if (!__of_device_is_available(next))
> > > > > + continue;
> > > > > if (!(of_node_name_eq(next, "cpu") ||
> > > > > __of_node_is_type(next, "cpu")))
> > > > > continue;
> > > > >
> > > >
> > > > The original implementation of of_get_next_cpu_node() had
> > > > that check, but status disabled for cpu nodes has different
> > > > semantics than other nodes, and the check broke some systems.
> > > > The check was removed by c961cb3be906 "of: Fix cpu node
> > > > iterator to not ignore disabled cpu nodes".
> > > >
> > > > It would be useful to document that difference in the
> > > > header comment of of_get_next_cpu_node().
> > > >
> > > > -Frank
> > >
> > > Hmm, I see. This difference in behaviour is quite unfortunate, as
> > > I'm
> > > currently looking for a way to *really* disable a CPU core.
> > >
> > > In arch/arm64/boot/dts/freescale/imx8mn.dtsi (and other variants
> > > of the
> > > i.MX8M), there are 4 CPU nodes for the full-featured quad-core
> > > version.
> > > The reduced single- and dual-core versions are currently handled
> > > in
> > > NXP's U-Boot fork by deleting the additional nodes.
> > >
> > > Not doing so causes the kernel to hang for a while when trying to
> > > online the non-existent cores during boot (at least in linux-imx
> > > 5.4 -
> > > I have not checked a more recent mainline kernel yet), but the
> > > deletion
> > > is non-trivial to do without leaving dangling phandle references.
> >
> > Any thoughts on implementing another universal property that means
> > something like "the hardware described by this node does not exist
> > or is so broken that you better not use it".
>
> There's a couple of options:
>
> The DT spec defines 'fail' value for status. We could use that
> instead
> of 'disabled'.
>
> The spec behavior with cpu 'disabled' is only on PPC AFAIK. On
> arm/arm64 (probably riscv now too) we've never followed it where we
> online 'disabled' CPUs. So we could just make the check conditional
> on
> !IS_ENABLED(CONFIG_PPC). This would need some spec update.
On ARM(64), the "disabled" status on CPUs doesn't have any effect. I
assume this changed with the mentioned commit c961cb3be906 "of: Fix cpu
node iterator to not ignore disabled cpu nodes", as reverting it gives
me the desired behaviour of considering the disabled CPUs non-existent.
So it seems that we already changed the interpretation in a non-
compatible way once (back in v4.20), and somehow PPC has yet another
different behaviour?
How do we get out of this mess? Is going back to the v4.19 logic for
non-PPC platforms an acceptable regression fix, or would this be
considered another breaking change?
>
> > Matthias, if Rob thinks that is a good idea, then you should start
> > with a new proposal that is also sent to
> > devicetree-s...@vger.kernel.org
> >
> > -Frank
> >
> > >
> > > Kind regards,
> > > Matthias
> > >
Re: (EXT) Re: [PATCH] of: skip disabled CPU nodes
On Wed, 2020-08-26 at 08:01 -0500, Frank Rowand wrote:
> On 2020-08-26 07:02, Matthias Schiffer wrote:
> > Allow disabling CPU nodes using status = "disabled".
> >
> > This allows a bootloader to change the number of available CPUs
> > (for
> > example when a common DTS is used for SoC variants with different
> > numbers
> > of cores) without deleting the nodes altogether (which may require
> > additional fixups where the CPU nodes are referenced, e.g. a
> > cooling
> > map).
> >
> > Signed-off-by: Matthias Schiffer > >
> > ---
> > drivers/of/base.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index ea44fea99813..d547e9deced1 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -796,6 +796,8 @@ struct device_node *of_get_next_cpu_node(struct
> > device_node *prev)
> > of_node_put(node);
> > }
> > for (; next; next = next->sibling) {
> > + if (!__of_device_is_available(next))
> > + continue;
> > if (!(of_node_name_eq(next, "cpu") ||
> > __of_node_is_type(next, "cpu")))
> > continue;
> >
>
> The original implementation of of_get_next_cpu_node() had
> that check, but status disabled for cpu nodes has different
> semantics than other nodes, and the check broke some systems.
> The check was removed by c961cb3be906 "of: Fix cpu node
> iterator to not ignore disabled cpu nodes".
>
> It would be useful to document that difference in the
> header comment of of_get_next_cpu_node().
>
> -Frank
Hmm, I see. This difference in behaviour is quite unfortunate, as I'm
currently looking for a way to *really* disable a CPU core.
In arch/arm64/boot/dts/freescale/imx8mn.dtsi (and other variants of the
i.MX8M), there are 4 CPU nodes for the full-featured quad-core version.
The reduced single- and dual-core versions are currently handled in
NXP's U-Boot fork by deleting the additional nodes.
Not doing so causes the kernel to hang for a while when trying to
online the non-existent cores during boot (at least in linux-imx 5.4 -
I have not checked a more recent mainline kernel yet), but the deletion
is non-trivial to do without leaving dangling phandle references.
Kind regards,
Matthias
Re: (EXT) Re: (EXT) Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
On Wed, 2020-08-26 at 10:01 -0300, Fabio Estevam wrote: > On Wed, Aug 26, 2020 at 8:54 AM Matthias Schiffer > wrote: > > > Before 8cdcd8aeee281, num_chipselect was set to 3 for spi0 and to 1 > > for > > spi1 in arch/arm/mach-imx/mach-mx31lite.c. My understanding is that > > it > > would make sense to add this as num-cs to > > arch/arm/boot/dts/imx31-lite.dts. > > Or just pass cs-gpios instead? Using GPIOs for chipselect would require different pinmuxing. Also, why use GPIOs, when the SPI controller has this built in?
[PATCH] of: skip disabled CPU nodes
Allow disabling CPU nodes using status = "disabled".
This allows a bootloader to change the number of available CPUs (for
example when a common DTS is used for SoC variants with different numbers
of cores) without deleting the nodes altogether (which may require
additional fixups where the CPU nodes are referenced, e.g. a cooling
map).
Signed-off-by: Matthias Schiffer
---
drivers/of/base.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index ea44fea99813..d547e9deced1 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -796,6 +796,8 @@ struct device_node *of_get_next_cpu_node(struct device_node
*prev)
of_node_put(node);
}
for (; next; next = next->sibling) {
+ if (!__of_device_is_available(next))
+ continue;
if (!(of_node_name_eq(next, "cpu") ||
__of_node_is_type(next, "cpu")))
continue;
--
2.17.1
Re: (EXT) Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
On Wed, 2020-08-26 at 07:59 -0300, Fabio Estevam wrote:
> Hi Matthias,
>
> On Wed, Aug 26, 2020 at 7:32 AM Matthias Schiffer
> wrote:
>
> > But the previous platform data that was removed in 8cdcd8aeee281
> > ("spi:
> > imx/fsl-lpspi: Convert to GPIO descriptors") set different values
> > for
> > different boards. So maybe some DTS should be using num-cs?
>
> Could you provide an example of an imx dts that should use num-cs?
I'm not sure, does anything break when num_chipselect is set too high?
Before 8cdcd8aeee281, num_chipselect was set to 3 for spi0 and to 1 for
spi1 in arch/arm/mach-imx/mach-mx31lite.c. My understanding is that it
would make sense to add this as num-cs to
arch/arm/boot/dts/imx31-lite.dts.
Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
On Wed, 2020-08-26 at 12:32 +0200, Matthias Schiffer wrote:
> On Tue, 2020-08-25 at 14:16 -0300, Fabio Estevam wrote:
> > On Tue, Aug 25, 2020 at 11:40 AM Matthias Schiffer
> > wrote:
> >
> > > Makes sense. Does the following logic sound correct?
> > >
> > > - If num-cs is set, use that (and add it to the docs)
> >
> > I would not add num-cs to the docs. As far as I can see there is no
> > imx dts that uses num-cs currently.
>
> But the previous platform data that was removed in 8cdcd8aeee281
> ("spi:
> imx/fsl-lpspi: Convert to GPIO descriptors") set different values for
> different boards. So maybe some DTS should be using num-cs?
>
>
> >
> > > - If num-cs is unset, use the number of cs-gpios
> > > - If num-cs is unset and no cs-gpios are defined, use a driver-
> > > provided
> > > default
> > >
> > >
> > > I'm not sure if 3 is a particularly useful default either, but it
> > > seems
> > > it was chosen to accommodate boards that previously set this via
> > > platform data. All SoCs I've checked (i.MX6Q/DL, i.MX6UL, i.MX7)
> > > have 4
> > > internal CS pins per ECSPI instance, so maybe the driver should
> > > use
> > > that as its default instead?
> >
> > I think it is time to get rid of i.MX board files. I will try to
> > work
> > on this when I have a chance.
> >
> > bout using 4 as default chip select number, please also check some
> > older SoCs like imx25, imx35, imx51, imx53, etc
>
> Hmm, I just checked i.MX28, and it has only 3 chip selects per
> instance.
Ah sorry, I got confused, the i.MX28 has a different SPI IP.
Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
On Tue, 2020-08-25 at 14:16 -0300, Fabio Estevam wrote:
> On Tue, Aug 25, 2020 at 11:40 AM Matthias Schiffer
> wrote:
>
> > Makes sense. Does the following logic sound correct?
> >
> > - If num-cs is set, use that (and add it to the docs)
>
> I would not add num-cs to the docs. As far as I can see there is no
> imx dts that uses num-cs currently.
But the previous platform data that was removed in 8cdcd8aeee281 ("spi:
imx/fsl-lpspi: Convert to GPIO descriptors") set different values for
different boards. So maybe some DTS should be using num-cs?
>
> > - If num-cs is unset, use the number of cs-gpios
> > - If num-cs is unset and no cs-gpios are defined, use a driver-
> > provided
> > default
> >
> >
> > I'm not sure if 3 is a particularly useful default either, but it
> > seems
> > it was chosen to accommodate boards that previously set this via
> > platform data. All SoCs I've checked (i.MX6Q/DL, i.MX6UL, i.MX7)
> > have 4
> > internal CS pins per ECSPI instance, so maybe the driver should use
> > that as its default instead?
>
> I think it is time to get rid of i.MX board files. I will try to work
> on this when I have a chance.
>
> bout using 4 as default chip select number, please also check some
> older SoCs like imx25, imx35, imx51, imx53, etc
Hmm, I just checked i.MX28, and it has only 3 chip selects per
instance.
Re: (EXT) Re: (EXT) Re: [PATCH 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
On Tue, 2020-08-25 at 11:24 -0300, Fabio Estevam wrote:
> Hi Matthias,
>
> On Tue, Aug 25, 2020 at 4:22 AM Matthias Schiffer
> wrote:
>
> > Hmm, unless I'm overlooking something, this is not going to work:
> >
> > - spi_get_gpio_descs() sets num_chipselect to the maximum of the
> > num_chipselect set in the driver and the number of cs-gpios
> >
> > - spi_imx_probe() sets num_chipselect to 3 if not specified in the
> > device tree
> >
> > So I think we would end up with 3 instead of 1 chipselect.
>
> Oh, this has changed recently in 8cdcd8aeee281 ("spi: imx/fsl-lpspi:
> Convert to GPIO descriptors"):
>
>
> - } else {
> - u32 num_cs;
> -
> - if (!of_property_read_u32(np, "num-cs", _cs))
> - master->num_chipselect = num_cs;
> - /* If not preset, default value of 1 is used */
>
> Initially, if num-cs was not present the default value for
> num_chipselect was 1.
>
> - }
> + /*
> +* Get number of chip selects from device properties. This
> can be
> +* coming from device tree or boardfiles, if it is not
> defined,
> +* a default value of 3 chip selects will be used, as all the
> legacy
> +* board files have <= 3 chip selects.
> +*/
> + if (!device_property_read_u32(>dev, "num-cs", ))
> + master->num_chipselect = val;
> + else
> + master->num_chipselect = 3;
>
> Now it became 3.
>
> I think this is a driver issue and we should fix the driver instead
> of
> requiring to pass num-cs to the device tree.
>
>
> num-cs is not even documented in the spi-imx binding.
Makes sense. Does the following logic sound correct?
- If num-cs is set, use that (and add it to the docs)
- If num-cs is unset, use the number of cs-gpios
- If num-cs is unset and no cs-gpios are defined, use a driver-provided
default
I'm not sure if 3 is a particularly useful default either, but it seems
it was chosen to accommodate boards that previously set this via
platform data. All SoCs I've checked (i.MX6Q/DL, i.MX6UL, i.MX7) have 4
internal CS pins per ECSPI instance, so maybe the driver should use
that as its default instead?
[PATCH mmc-next v3 2/2] mmc: allow setting slot index via device tree alias
As with GPIO, UART and others, allow specifying the device index via the
aliases node in the device tree.
On embedded devices, there is often a combination of removable (e.g.
SD card) and non-removable MMC devices (e.g. eMMC).
Therefore the index might change depending on
* host of removable device
* removable card present or not
This makes it difficult to hardcode the root device, if it is on the
non-removable device. E.g. if SD card is present eMMC will be mmcblk1,
if SD card is not present at boot, eMMC will be mmcblk0.
Alternative solutions like PARTUUIDs do not cover the case where multiple
mmcblk devices contain the same image. This is a common issue on devices
that can boot both from eMMC (for regular boot) and SD cards (as a
temporary boot medium for development). When a firmware image is
installed to eMMC after a test boot via SD card, there will be no
reliable way to refer to a specific device using (PART)UUIDs oder
LABELs.
The demand for this feature has led to multiple attempts to implement
it, dating back at least to 2012 (see
https://www.spinics.net/lists/linux-mmc/msg26586.html for a previous
discussion from 2014).
All indices defined in the aliases node will be reserved for use by the
respective MMC device, moving the indices of devices that don't have an
alias up into the non-reserved range. If the aliases node is not found,
the driver will act as before.
This is a rebased and slightly cleaned up version of
https://www.spinics.net/lists/linux-mmc/msg26588.html .
Based-on-patch-by: Sascha Hauer
Link: https://lkml.org/lkml/2020/8/5/194
Signed-off-by: Matthias Schiffer
---
v3:
- remove unneeded mmcblock changes
- remove most helper functions to simplify code
- extended commit message
v2:
- fix missing symbols for modular mmcblock
drivers/mmc/core/host.c | 26 +-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index c8fae6611b73..96b2ca1f1b06 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -376,6 +376,20 @@ int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
}
EXPORT_SYMBOL(mmc_of_parse_voltage);
+/**
+ * mmc_first_nonreserved_index() - get the first index that is not reserved
+ */
+static int mmc_first_nonreserved_index(void)
+{
+ int max;
+
+ max = of_alias_get_highest_id("mmc");
+ if (max < 0)
+ return 0;
+
+ return max + 1;
+}
+
/**
* mmc_alloc_host - initialise the per-host structure.
* @extra: sizeof private data structure
@@ -387,6 +401,7 @@ struct mmc_host *mmc_alloc_host(int extra, struct device
*dev)
{
int err;
struct mmc_host *host;
+ int alias_id, min_idx, max_idx;
host = kzalloc(sizeof(struct mmc_host) + extra, GFP_KERNEL);
if (!host)
@@ -395,7 +410,16 @@ struct mmc_host *mmc_alloc_host(int extra, struct device
*dev)
/* scanning will be enabled when we're ready */
host->rescan_disable = 1;
- err = ida_simple_get(_host_ida, 0, 0, GFP_KERNEL);
+ alias_id = of_alias_get_id(dev->of_node, "mmc");
+ if (alias_id >= 0) {
+ min_idx = alias_id;
+ max_idx = alias_id + 1;
+ } else {
+ min_idx = mmc_first_nonreserved_index();
+ max_idx = 0;
+ }
+
+ err = ida_simple_get(_host_ida, min_idx, max_idx, GFP_KERNEL);
if (err < 0) {
kfree(host);
return NULL;
--
2.17.1
[PATCH mmc-next v3 1/2] dt-bindings: mmc: add alias example
As for I2C and SPI, it now is possible to reserve a fixed index for
mmc/mmcblk devices.
Signed-off-by: Matthias Schiffer
---
v3: new patch
Documentation/devicetree/bindings/mmc/mmc-controller.yaml | 8
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
index b96da0c7f819..22ed4a36c65d 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
+++ b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
@@ -367,6 +367,14 @@ examples:
};
- |
+/*
+ * Optionally define an alias to reserve a fixed index for the
+ * mmc and mmcblk devices
+ */
+aliases {
+mmc0 =
+};
+
mmc3: mmc@1c12000 {
#address-cells = <1>;
#size-cells = <0>;
--
2.17.1
Re: (EXT) Re: (EXT) Re: [PATCH mmc-next v2] mmc: allow setting slot index via device tree alias
On Tue, 2020-08-25 at 14:27 +0200, Ulf Hansson wrote:
> On Tue, 25 Aug 2020 at 14:00, Matthias Schiffer
> wrote:
> >
> > On Tue, 2020-08-25 at 11:39 +0200, Matthias Schiffer wrote:
> > > On Tue, 2020-08-25 at 11:14 +0200, Ulf Hansson wrote:
> > > > On Thu, 20 Aug 2020 at 09:59, Matthias Schiffer
> > > > wrote:
> > > > > --- a/drivers/mmc/core/host.c
> > > > > +++ b/drivers/mmc/core/host.c
> > > > > @@ -387,6 +387,7 @@ struct mmc_host *mmc_alloc_host(int
> > > > > extra,
> > > > > struct device *dev)
> > > > > {
> > > > > int err;
> > > > > struct mmc_host *host;
> > > > > + int alias_id, min_idx, max_idx;
> > > > >
> > > > > host = kzalloc(sizeof(struct mmc_host) + extra,
> > > > > GFP_KERNEL);
> > > > > if (!host)
> > > > > @@ -395,7 +396,18 @@ struct mmc_host *mmc_alloc_host(int
> > > > > extra,
> > > > > struct device *dev)
> > > > > /* scanning will be enabled when we're ready */
> > > > > host->rescan_disable = 1;
> > > > >
> > > > > - err = ida_simple_get(_host_ida, 0, 0,
> > > > > GFP_KERNEL);
> > > > > + host->parent = dev;
> > > > > +
> > > > > + alias_id = mmc_get_reserved_index(host);
> > > > > + if (alias_id >= 0) {
> > > > > + min_idx = alias_id;
> > > > > + max_idx = alias_id + 1;
> > > > > + } else {
> > > > > + min_idx = mmc_first_nonreserved_index();
> > > > > + max_idx = 0;
> > > > > + }
> > > > > +
> > > > > + err = ida_simple_get(_host_ida, min_idx, max_idx,
> > > > > GFP_KERNEL);
> >
> >
> > One more question I came across when reworking my patch: Do we need
> > a
> > fallback here for the case where the reserved index is already
> > taken?
> > To handle an SD card being replaced while still mounted?
>
> Removal/insertion of an SD card should be fine, as that doesn't mean
> that the host is removed. In other words, host->index remains the
> same.
>
> Although, for a bad DT configuration, where for example the same
> aliases id is used twice, a fallback could make sense. On the other
> hand, as such configuration would be wrong, we might as well just
> print a message and return an error.
I don't think this can happen as long as we don't have DTs changing at
runtime: Each alias is a DT property name in /aliases, which can only exist
once.
>
> [...]
>
> Kind regards
> Uffe
Re: (EXT) Re: [PATCH mmc-next v2] mmc: allow setting slot index via device tree alias
On Tue, 2020-08-25 at 11:39 +0200, Matthias Schiffer wrote:
> On Tue, 2020-08-25 at 11:14 +0200, Ulf Hansson wrote:
> > On Thu, 20 Aug 2020 at 09:59, Matthias Schiffer
> > wrote:
> > > --- a/drivers/mmc/core/host.c
> > > +++ b/drivers/mmc/core/host.c
> > > @@ -387,6 +387,7 @@ struct mmc_host *mmc_alloc_host(int extra,
> > > struct device *dev)
> > > {
> > > int err;
> > > struct mmc_host *host;
> > > + int alias_id, min_idx, max_idx;
> > >
> > > host = kzalloc(sizeof(struct mmc_host) + extra,
> > > GFP_KERNEL);
> > > if (!host)
> > > @@ -395,7 +396,18 @@ struct mmc_host *mmc_alloc_host(int extra,
> > > struct device *dev)
> > > /* scanning will be enabled when we're ready */
> > > host->rescan_disable = 1;
> > >
> > > - err = ida_simple_get(_host_ida, 0, 0, GFP_KERNEL);
> > > + host->parent = dev;
> > > +
> > > + alias_id = mmc_get_reserved_index(host);
> > > + if (alias_id >= 0) {
> > > + min_idx = alias_id;
> > > + max_idx = alias_id + 1;
> > > + } else {
> > > + min_idx = mmc_first_nonreserved_index();
> > > + max_idx = 0;
> > > + }
> > > +
> > > + err = ida_simple_get(_host_ida, min_idx, max_idx,
> > > GFP_KERNEL);
One more question I came across when reworking my patch: Do we need a
fallback here for the case where the reserved index is already taken?
To handle an SD card being replaced while still mounted?
> > > if (err < 0) {
> > > kfree(host);
> > > return NULL;
> > > @@ -406,7 +418,6 @@ struct mmc_host *mmc_alloc_host(int extra,
> > > struct device *dev)
> > > dev_set_name(>class_dev, "mmc%d", host->index);
> > > host->ws = wakeup_source_register(NULL, dev_name(
> > > > class_dev));
> > >
> > > - host->parent = dev;
> > > host->class_dev.parent = dev;
> > > host->class_dev.class = _host_class;
> > > device_initialize(>class_dev);
> > > --
> > > 2.17.1
> > >
> >
> > Kind regards
> > Uffe
Re: (EXT) Re: [PATCH mmc-next v2] mmc: allow setting slot index via device tree alias
On Tue, 2020-08-25 at 11:39 +0200, Matthias Schiffer wrote:
> On Tue, 2020-08-25 at 11:14 +0200, Ulf Hansson wrote:
> > On Thu, 20 Aug 2020 at 09:59, Matthias Schiffer
> > wrote:
> > > +
> > > +static void __init mmc_of_reserve_idx(void)
> > > +{
> > > + int max;
> > > +
> > > + max = of_alias_get_highest_id("mmc");
> >
> > Is calling of_alias_get_highest_id("mmc") costly from an execution
> > point of view?
> >
> > If not, we might as well call it directly from mmc_alloc_host()
> > each
> > time a host is allocated instead, to simplify the code a bit.
>
> It's not particularly costly (it just walks the list of aliases once
> and does a string comparison for each entry), but it does so while
> holding the of_mutex.
>
> Both variants exist in the current kernel: The I2C core stores the
> hightest index in a global variable, while of_alias_get_highest_id()
> is
> called once for each registered SPI controller. I have a slight
> preference for the global variable solution.
Looking at this again, it's pretty much the same as of_alias_get_id().
I'll remove the extra functions and global variable.
Kind regards,
Matthias
Re: (EXT) Re: [PATCH mmc-next v2] mmc: allow setting slot index via device tree alias
On Tue, 2020-08-25 at 11:14 +0200, Ulf Hansson wrote:
> On Thu, 20 Aug 2020 at 09:59, Matthias Schiffer
> wrote:
> >
> > As with GPIO, UART and others, allow specifying the device index
> > via the
> > aliases node in the device tree.
> >
> > On embedded devices, there is often a combination of removable
> > (e.g.
> > SD card) and non-removable MMC devices (e.g. eMMC).
> > Therefore the index might change depending on
> >
> > * host of removable device
> > * removable card present or not
> >
> > This makes it difficult to hardcode the root device, if it is on
> > the
> > non-removable device. E.g. if SD card is present eMMC will be
> > mmcblk1,
> > if SD card is not present at boot, eMMC will be mmcblk0.
> >
> > All indices defined in the aliases node will be reserved for use by
> > the
> > respective MMC device, moving the indices of devices that don't
> > have an
> > alias up into the non-reserved range. If the aliases node is not
> > found,
> > the driver will act as before.
> >
> > This is a rebased and slightly cleaned up version of
> > https://www.spinics.net/lists/linux-mmc/msg26588.html .
> >
> > Based-on-patch-by: Sascha Hauer
> > Link: https://lkml.org/lkml/2020/8/5/194
> > Signed-off-by: Matthias Schiffer > >
> > ---
> >
> > v2: fix missing symbols for modular mmcblock
> >
> > drivers/mmc/core/block.c | 13 +++--
> > drivers/mmc/core/core.c | 40
> >
> > drivers/mmc/core/core.h | 3 +++
> > drivers/mmc/core/host.c | 15 +--
> > 4 files changed, 67 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> > index 7896952de1ac..4620afaf0e50 100644
> > --- a/drivers/mmc/core/block.c
> > +++ b/drivers/mmc/core/block.c
> > @@ -38,6 +38,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -2260,9 +2261,17 @@ static struct mmc_blk_data
> > *mmc_blk_alloc_req(struct mmc_card *card,
> > int area_type)
> > {
> > struct mmc_blk_data *md;
> > - int devidx, ret;
> > + int rsvidx, devidx = -1, ret;
> > +
> > + rsvidx = mmc_get_reserved_index(card->host);
> > + if (rsvidx >= 0)
> > + devidx = ida_simple_get(_blk_ida, rsvidx,
> > rsvidx + 1,
> > + GFP_KERNEL);
> > + if (devidx < 0)
> > + devidx = ida_simple_get(_blk_ida,
> > + mmc_first_nonreserved_index
> > (),
> > + max_devices, GFP_KERNEL);
> >
> > - devidx = ida_simple_get(_blk_ida, 0, max_devices,
> > GFP_KERNEL);
>
> I am not sure why you need to change any of this. Currently we use
> the
> host->index, when creating the blockpartion name (dev.init_name).
>
> This is done for both rpmb and regular partitions. Isn't that
> sufficient?
You are right, that should be sufficient.
>
> > if (devidx < 0) {
> > /*
> > * We get -ENOSPC because there are no more any
> > available
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 8ccae6452b9c..5bce281a5faa 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -2419,10 +2419,50 @@ void mmc_unregister_pm_notifier(struct
> > mmc_host *host)
> > }
> > #endif
> >
> > +static int mmc_max_reserved_idx = -1;
> > +
> > +/**
> > + * mmc_first_nonreserved_index() - get the first index that is not
> > reserved
> > + */
> > +int mmc_first_nonreserved_index(void)
> > +{
> > + return mmc_max_reserved_idx + 1;
> > +}
> > +EXPORT_SYMBOL(mmc_first_nonreserved_index);
> > +
> > +/**
> > + * mmc_get_reserved_index() - get the index reserved for this MMC
> > host
> > + *
> > + * Returns:
> > + * The index reserved for this host on success,
> > + * negative error if no index is reserved for this host
> > + */
> > +int mmc_get_reserved_index(struct mmc_host *host)
> > +{
> > + return of_alias_get_id(host->parent->of_node, "mmc");
> > +}
> > +EXPORT_SYMBOL(mmc_get_reserved_index);
>
> I think you can drop this function, just call of_alias_get_id() from
> w
Re: [PATCH RFC leds + net-next v4 0/2] Add support for LEDs on Marvell PHYs
On Tue, 2020-07-28 at 17:05 +0200, Marek Behún wrote: > Hi, > > this is v4 of my RFC adding support for LEDs connected to Marvell > PHYs. > > Please note that if you want to test this, you still need to first > apply > the patch adding the LED private triggers support from Pavel's tree. > https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next=93690cdf3060c61dfce813121d0bfc055e7fa30d > > What I still don't like about this is that the LEDs created by the > code > don't properly support device names. LEDs should have name in format > "device:color:function", for example "eth0:green:activity". > > The code currently looks for attached netdev for a given PHY, but > at the time this happens there is no netdev attached, so the LEDs > gets > names without the device part (ie ":green:activity"). > > This can be addressed in next version by renaming the LED when a > netdev > is attached to the PHY, but first a API for LED device renaming needs > to > be proposed. I am going to try to do that. This would also solve the > same problem when userspace renames an interface. > > And no, I don't want phydev name there. Hello Marek, thanks for your patches - Andrew suggested me to have a look at them as I'm currently trying to add LED trigger support to the TI DP83867 PHY. Is there already a plan to add support for polarity and similiar settings, at least to the generic part of your changes? In the TI DP83867, there are 2 separate settings for each LED: - Trigger event - Polarity or override (active-high/active-low/force-high/force-low - the latter two would be used for led_brightness_set) - (There is also a 3rd register that defines the blink frequency, but as it allows only a single setting for all LEDs, I would ignore it for now) At least the per-LED polarity setting would be essential to have for this feature to be useful for our TQ-Systems mainboards with TI PHYs. Kind regards, Matthias > > Changes since v3: > - addressed some of Andrew's suggestions > - phy_hw_led_mode.c renamed to phy_led.c > - the DT reading code is now also generic, moved to phy_led.c and > called > from phy_probe > - the function registering the phydev-hw-mode trigger is now called > from > phy_device.c function phy_init before registering genphy drivers > - PHY LED functionality now depends on CONFIG_LEDS_TRIGGERS > > Changes since v2: > - to share code with other drivers which may want to also offer PHY > HW > control of LEDs some of the code was refactored and now resides in > phy_hw_led_mode.c. This code is compiled in when config option > LED_TRIGGER_PHY_HW is enabled. Drivers wanting to offer PHY HW > control > of LEDs should depend on this option. > - the "hw-control" trigger is renamed to "phydev-hw-mode" and is > registered by the code in phy_hw_led_mode.c > - the "hw_control" sysfs file is renamed to "hw_mode" > - struct phy_driver is extended by three methods to support PHY HW > LED > control > - I renamed the various HW control modes offeret by Marvell PHYs to > conform to other Linux mode names, for example the > "1000/100/10/else" > mode was renamed to "1Gbps/100Mbps/10Mbps", or "recv/else" was > renamed > to "rx" (this is the name of the mode in netdev trigger). > > Marek > > > Marek Behún (2): > net: phy: add API for LEDs controlled by PHY HW > net: phy: marvell: add support for PHY LEDs via LED class > > drivers/net/phy/Kconfig | 4 + > drivers/net/phy/Makefile | 1 + > drivers/net/phy/marvell.c| 287 > +++ > drivers/net/phy/phy_device.c | 25 ++- > drivers/net/phy/phy_led.c| 176 + > include/linux/phy.h | 51 +++ > 6 files changed, 537 insertions(+), 7 deletions(-) > create mode 100644 drivers/net/phy/phy_led.c >
Re: (EXT) Re: [PATCH 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
On Mon, 2020-08-24 at 18:36 -0300, Fabio Estevam wrote:
> Hi Matthias,
>
> On Mon, Aug 24, 2020 at 6:10 AM Matthias Schiffer
> wrote:
>
> > diff --git a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
> > b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
> > index 9513020ddd1a..7aaae83c1fae 100644
> > --- a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
> > +++ b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
> > @@ -20,7 +20,7 @@
> > {
> > pinctrl-names = "default";
> > pinctrl-0 = <_ecspi1>;
> > - fsl,spi-num-chipselects = <1>;
> > + num-cs = <1>;
>
> You could simply remove fsl,spi-num-chipselects without passing num-
> cs.
>
> The spi core is able to count the number of chipselects passed via
> cs-gpios in the device tree.
Hmm, unless I'm overlooking something, this is not going to work:
- spi_get_gpio_descs() sets num_chipselect to the maximum of the
num_chipselect set in the driver and the number of cs-gpios
- spi_imx_probe() sets num_chipselect to 3 if not specified in the
device tree
So I think we would end up with 3 instead of 1 chipselect.
Kind regards,
Matthias
Re: (EXT) Re: [PATCH mmc-next v2] mmc: allow setting slot index via device tree alias
On Fri, 2020-08-21 at 13:39 +0200, Ulf Hansson wrote:
> On Thu, 20 Aug 2020 at 09:59, Matthias Schiffer
> wrote:
> >
> > As with GPIO, UART and others, allow specifying the device index
> > via the
> > aliases node in the device tree.
> >
> > On embedded devices, there is often a combination of removable
> > (e.g.
> > SD card) and non-removable MMC devices (e.g. eMMC).
> > Therefore the index might change depending on
> >
> > * host of removable device
> > * removable card present or not
> >
> > This makes it difficult to hardcode the root device, if it is on
> > the
> > non-removable device. E.g. if SD card is present eMMC will be
> > mmcblk1,
> > if SD card is not present at boot, eMMC will be mmcblk0.
>
> Can you please add some information why Part-UUID/labels don't work
> well to solve this problem on some embedded systems?
>
> I think that deserves to be in the changelog, after all the long
> discussions we had in the history around this.
Makes sense. I'll wait for your review before I send a v3 with an
updated description.
Matthias
>
> I will continue to review the code in a separate email, in a while.
>
> Kind regards
> Uffe
>
> >
> > All indices defined in the aliases node will be reserved for use by
> > the
> > respective MMC device, moving the indices of devices that don't
> > have an
> > alias up into the non-reserved range. If the aliases node is not
> > found,
> > the driver will act as before.
> >
> > This is a rebased and slightly cleaned up version of
> > https://www.spinics.net/lists/linux-mmc/msg26588.html .
> >
> > Based-on-patch-by: Sascha Hauer
> > Link: https://lkml.org/lkml/2020/8/5/194
> > Signed-off-by: Matthias Schiffer > >
> > ---
> >
> > v2: fix missing symbols for modular mmcblock
> >
> > drivers/mmc/core/block.c | 13 +++--
> > drivers/mmc/core/core.c | 40
> >
> > drivers/mmc/core/core.h | 3 +++
> > drivers/mmc/core/host.c | 15 +--
> > 4 files changed, 67 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> > index 7896952de1ac..4620afaf0e50 100644
> > --- a/drivers/mmc/core/block.c
> > +++ b/drivers/mmc/core/block.c
> > @@ -38,6 +38,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -2260,9 +2261,17 @@ static struct mmc_blk_data
> > *mmc_blk_alloc_req(struct mmc_card *card,
> > int area_type)
> > {
> > struct mmc_blk_data *md;
> > - int devidx, ret;
> > + int rsvidx, devidx = -1, ret;
> > +
> > + rsvidx = mmc_get_reserved_index(card->host);
> > + if (rsvidx >= 0)
> > + devidx = ida_simple_get(_blk_ida, rsvidx,
> > rsvidx + 1,
> > + GFP_KERNEL);
> > + if (devidx < 0)
> > + devidx = ida_simple_get(_blk_ida,
> > + mmc_first_nonreserved_index
> > (),
> > + max_devices, GFP_KERNEL);
>
>
>
> >
> > - devidx = ida_simple_get(_blk_ida, 0, max_devices,
> > GFP_KERNEL);
> > if (devidx < 0) {
> > /*
> > * We get -ENOSPC because there are no more any
> > available
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 8ccae6452b9c..5bce281a5faa 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -2419,10 +2419,50 @@ void mmc_unregister_pm_notifier(struct
> > mmc_host *host)
> > }
> > #endif
> >
> > +static int mmc_max_reserved_idx = -1;
> > +
> > +/**
> > + * mmc_first_nonreserved_index() - get the first index that is not
> > reserved
> > + */
> > +int mmc_first_nonreserved_index(void)
> > +{
> > + return mmc_max_reserved_idx + 1;
> > +}
> > +EXPORT_SYMBOL(mmc_first_nonreserved_index);
> > +
> > +/**
> > + * mmc_get_reserved_index() - get the index reserved for this MMC
> > host
> > + *
> > + * Returns:
> > + * The index reserved for this host on success,
> > + * negative error if no index is reserved for this host
> > + */
> > +int mmc_get_reserved_index(struct mmc_host *host)
> > +{
> > + return of_alias_get
[PATCH 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
- Fix national,lm75 compatible string
- Replace obsolete fsl,spi-num-chipselects with num-cs
Fixes: cac849e9bbc8 ("ARM: dts: imx6qdl: add TQMa6{S,Q,QP} SoM")
Signed-off-by: Matthias Schiffer
---
arch/arm/boot/dts/imx6qdl-tqma6.dtsi | 2 +-
arch/arm/boot/dts/imx6qdl-tqma6a.dtsi | 2 +-
arch/arm/boot/dts/imx6qdl-tqma6b.dtsi | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
index 9513020ddd1a..7aaae83c1fae 100644
--- a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
@@ -20,7 +20,7 @@
{
pinctrl-names = "default";
pinctrl-0 = <_ecspi1>;
- fsl,spi-num-chipselects = <1>;
+ num-cs = <1>;
cs-gpios = < 19 GPIO_ACTIVE_HIGH>;
status = "okay";
diff --git a/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi
b/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi
index c18a06cf7929..b679bec78e6c 100644
--- a/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi
@@ -16,7 +16,7 @@
};
sensor@48 {
- compatible = "lm75";
+ compatible = "national,lm75";
reg = <0x48>;
};
diff --git a/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi
b/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi
index a7460075f517..49c472285c06 100644
--- a/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi
@@ -16,7 +16,7 @@
};
sensor@48 {
- compatible = "lm75";
+ compatible = "national,lm75";
reg = <0x48>;
};
--
2.17.1
[PATCH 1/2] ARM: dts: imx6qdl: tqma6: fix indentation
Fixes: cac849e9bbc8 ("ARM: dts: imx6qdl: add TQMa6{S,Q,QP} SoM")
Signed-off-by: Matthias Schiffer
---
arch/arm/boot/dts/imx6qdl-tqma6.dtsi | 188 +--
1 file changed, 94 insertions(+), 94 deletions(-)
diff --git a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
index 29bcce20f5f3..9513020ddd1a 100644
--- a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
@@ -83,101 +83,101 @@
};
{
- pinctrl-names = "default";
- pinctrl-0 = <_pmic>;
- interrupt-parent = <>;
- interrupts = <10 IRQ_TYPE_LEVEL_LOW>;
-
- regulators {
- reg_vddcore: sw1ab {
- regulator-min-microvolt = <30>;
- regulator-max-microvolt = <1875000>;
- regulator-always-on;
- };
-
- reg_vddsoc: sw1c {
- regulator-min-microvolt = <30>;
- regulator-max-microvolt = <1875000>;
- regulator-always-on;
- };
-
- reg_gen_3v3: sw2 {
- regulator-min-microvolt = <80>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
-
- reg_ddr_1v5a: sw3a {
- regulator-min-microvolt = <40>;
- regulator-max-microvolt = <1975000>;
- regulator-always-on;
- };
-
- reg_ddr_1v5b: sw3b {
- regulator-min-microvolt = <40>;
- regulator-max-microvolt = <1975000>;
- regulator-always-on;
- };
-
- sw4_reg: sw4 {
- regulator-min-microvolt = <80>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
-
- reg_5v_600mA: swbst {
- regulator-min-microvolt = <500>;
- regulator-max-microvolt = <515>;
- regulator-always-on;
- };
-
- reg_snvs_3v: vsnvs {
- regulator-min-microvolt = <150>;
- regulator-max-microvolt = <300>;
- regulator-always-on;
- };
-
- reg_vrefddr: vrefddr {
- regulator-boot-on;
- regulator-always-on;
- };
-
- reg_vgen1_1v5: vgen1 {
- regulator-min-microvolt = <80>;
- regulator-max-microvolt = <155>;
- /* not used */
- };
-
- reg_vgen2_1v2_eth: vgen2 {
- regulator-min-microvolt = <80>;
- regulator-max-microvolt = <155>;
- regulator-always-on;
- };
-
- reg_vgen3_2v8: vgen3 {
- regulator-min-microvolt = <180>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
-
- reg_vgen4_1v8: vgen4 {
- regulator-min-microvolt = <180>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
-
- reg_vgen5_1v8_eth: vgen5 {
- regulator-min-microvolt = <180>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
-
- reg_vgen6_3v3: vgen6 {
- regulator-min-microvolt = <180>;
- regulator-max-microvolt = <330>;
- regulator-always-on;
- };
+ pinctrl-names = "default";
+ pinctrl-0 = <_pmic>;
+ interrupt-parent = <>;
+ interrupts = <10 IRQ_TYPE_LEVEL_LOW>;
+
+ regulators {
+ reg_vd
Re: (EXT) Re: [PATCH net-next 2/2] net: phy: dp83867: apply ti, led-function and ti, led-ctrl to registers
On Sat, 2020-08-22 at 18:08 +0200, Andrew Lunn wrote: > On Fri, Aug 21, 2020 at 09:21:46AM +0200, Matthias Schiffer wrote: > > These DT bindings are already in use by the imx7-mba7 DTS, but they > > were > > not supported by the PHY driver so far. > > > > Signed-off-by: Matthias Schiffer > > > > Sorry, but NACK. > > Please look at the work Marek Behún is doing > > https://lkml.org/lkml/2020/7/28/765 > > Andrew > Thanks, this is looking quite nice. I hope Marek's patches are finalized soon. Matthias
[PATCH net-next 1/2] dt-bindings: dp83867: add ti,led-function and ti,led-ctrl properties
With the TQ-Systems MBa7x (imx7-mba7.dtsi), a user of these properties already sneaked in before they were properly specified. Add them to the binding docs. On top of the existing use (requiring to specify the raw register value in the DTS), we propose a few convenience macros and defines. Signed-off-by: Matthias Schiffer --- .../devicetree/bindings/net/ti,dp83867.yaml | 18 ++ include/dt-bindings/net/ti-dp83867.h | 60 +++ 2 files changed, 78 insertions(+) diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.yaml b/Documentation/devicetree/bindings/net/ti,dp83867.yaml index c6716ac6cbcc..f91d40edab39 100644 --- a/Documentation/devicetree/bindings/net/ti,dp83867.yaml +++ b/Documentation/devicetree/bindings/net/ti,dp83867.yaml @@ -106,6 +106,18 @@ properties: Transmitt FIFO depth- see dt-bindings/net/ti-dp83867.h for applicable values. + ti,led-function: +$ref: /schemas/types.yaml#definitions/uint32 +description: | + Value of LED configuration register 1, controlling the triggers for the + PHY LED outputs. See dt-bindings/net/ti-dp83867.h. + + ti,led-ctrl: +$ref: /schemas/types.yaml#definitions/uint32 +description: | + Value of LED configuration register 2, controlling polarity and related + settings for the PHY LED outputs. See dt-bindings/net/ti-dp83867.h. + required: - reg @@ -123,5 +135,11 @@ examples: ti,clk-output-sel = ; ti,rx-internal-delay = ; ti,tx-internal-delay = ; +ti,led-function = <(DP83867_LED0(FUNC_LINK_ACT) | +DP83867_LED1(FUNC_LINK_1000))>; +ti,led-ctrl = <(DP83867_LED0(CTRL_ACTIVE_HIGH) | +DP83867_LED1(CTRL_ACTIVE_HIGH) | +DP83867_LED2(CTRL_FORCE_LOW) | +DP83867_LED3(CTRL_FORCE_LOW))>; }; }; diff --git a/include/dt-bindings/net/ti-dp83867.h b/include/dt-bindings/net/ti-dp83867.h index 6fc4b445d3a1..f3e3866d26ee 100644 --- a/include/dt-bindings/net/ti-dp83867.h +++ b/include/dt-bindings/net/ti-dp83867.h @@ -50,4 +50,64 @@ #define DP83867_CLK_O_SEL_REF_CLK 0xC /* Special flag to indicate clock should be off */ #define DP83867_CLK_O_SEL_OFF 0x + +/* + * Register values and helper macros for ti,led-function and ti,led-ctrl + * + * Example: + * + * ti,led-function = <(DP83867_LED0(FUNC_LINK_ACT) | + * DP83867_LED1(FUNC_LINK_1000))>; + * ti,led-ctrl = <(DP83867_LED0(CTRL_ACTIVE_HIGH) | + * DP83867_LED1(CTRL_ACTIVE_HIGH) | + * DP83867_LED2(CTRL_FORCE_LOW) | + * DP83867_LED3(CTRL_FORCE_LOW))>; + * + * It is recommended to force all unused LED pins to high or low level via + * led-ctrl (led-function is ignored in this case). LEDs that are missing from + * the configured value will be set to value 0x0 (FUNC_LINK and + * CTRL_ACTIVE_LOW). + */ + +/* Link established */ +#define DP83867_LED_FUNC_LINK 0x0 +/* Receive or transmit activity */ +#define DP83867_LED_FUNC_ACT 0x1 +/* Transmit activity */ +#define DP83867_LED_FUNC_ACT_TX0x2 +/* Receive activity */ +#define DP83867_LED_FUNC_ACT_RX0x3 +/* Collision detected */ +#define DP83867_LED_FUNC_COLLISION 0x4 +/* 1000BT link established */ +#define DP83867_LED_FUNC_LINK_1000 0x5 +/* 100BTX link established */ +#define DP83867_LED_FUNC_LINK_100 0x6 +/* 10BT link established */ +#define DP83867_LED_FUNC_LINK_10 0x7 +/* 10/100BT link established */ +#define DP83867_LED_FUNC_LINK_10_100 0x8 +/* 100/1000BT link established */ +#define DP83867_LED_FUNC_LINK_100_1000 0x9 +/* Full duplex */ +#define DP83867_LED_FUNC_FULL_DUPLEX 0xa +/* Link established, blink for transmit or receive activity */ +#define DP83867_LED_FUNC_LINK_ACT 0xb +/* Receive Error or Transmit Error */ +#define DP83867_LED_FUNC_ERR 0xd +/* Receive Error */ +#define DP83867_LED_FUNC_ERR_RX0xe + +#define DP83867_LED_CTRL_ACTIVE_HIGH 0x4 +#define DP83867_LED_CTRL_ACTIVE_LOW0x0 +#define DP83867_LED_CTRL_FORCE_HIGH0x3 +#define DP83867_LED_CTRL_FORCE_LOW 0x1 + +#define DP83867_LED_SHIFT(v, s)((DP83867_LED_##v) << (s)) + +#define DP83867_LED0(v)DP83867_LED_SHIFT(v, 0) +#define DP83867_LED1(v)DP83867_LED_SHIFT(v, 4) +#define DP83867_LED2(v)DP83867_LED_SHIFT(v, 8) +#define DP83867_LED3(v)DP83867_LED_SHIFT(v, 12) + #endif -- 2.17.1
[PATCH net-next 2/2] net: phy: dp83867: apply ti,led-function and ti,led-ctrl to registers
These DT bindings are already in use by the imx7-mba7 DTS, but they were
not supported by the PHY driver so far.
Signed-off-by: Matthias Schiffer
---
drivers/net/phy/dp83867.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index f3c04981b8da..972824e25c1c 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -26,6 +26,8 @@
#define MII_DP83867_MICR 0x12
#define MII_DP83867_ISR0x13
#define DP83867_CFG2 0x14
+#define DP83867_LEDCR1 0x18
+#define DP83867_LEDCR2 0x19
#define DP83867_CFG3 0x1e
#define DP83867_CTRL 0x1f
@@ -163,6 +165,8 @@ struct dp83867_private {
u32 rx_fifo_depth;
int io_impedance;
int port_mirroring;
+ u32 led_function;
+ u32 led_ctrl;
bool rxctrl_strap_quirk;
bool set_clk_output;
u32 clk_output_sel;
@@ -583,6 +587,27 @@ static int dp83867_of_init(struct phy_device *phydev)
return -EINVAL;
}
+ ret = of_property_read_u32(of_node, "ti,led-function",
+ >led_function);
+ if (ret) {
+ dp83867->led_function = U32_MAX;
+ } else if (dp83867->led_function > U16_MAX) {
+ phydev_err(phydev,
+ "ti,led-function value %x out of range\n",
+ dp83867->led_function);
+ return -EINVAL;
+ }
+
+ ret = of_property_read_u32(of_node, "ti,led-ctrl", >led_ctrl);
+ if (ret) {
+ dp83867->led_ctrl = U32_MAX;
+ } else if (dp83867->led_ctrl > U16_MAX) {
+ phydev_err(phydev,
+ "ti,led-ctrl value %x out of range\n",
+ dp83867->led_ctrl);
+ return -EINVAL;
+ }
+
return 0;
}
#else
@@ -788,6 +813,11 @@ static int dp83867_config_init(struct phy_device *phydev)
mask, val);
}
+ if (dp83867->led_function != U32_MAX)
+ phy_write(phydev, DP83867_LEDCR1, dp83867->led_function);
+ if (dp83867->led_ctrl != U32_MAX)
+ phy_write(phydev, DP83867_LEDCR2, dp83867->led_ctrl);
+
return 0;
}
--
2.17.1
[PATCH 2/2] ASoC: fsl-asoc-card: add support for TLV320AIC32x4 codec
The TLV320AIC32x4 is commonly used on TQ-Systems starterkit mainboards
for i.MX-based SoMs (i.MX6Q/DL, i.MX6UL, i.MX7) and LS1021A.
Signed-off-by: Matthias Schiffer
---
sound/soc/fsl/Kconfig | 2 +-
sound/soc/fsl/fsl-asoc-card.c | 4
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig
index 1c4ca5ec8caf..3f76ff71ea47 100644
--- a/sound/soc/fsl/Kconfig
+++ b/sound/soc/fsl/Kconfig
@@ -324,7 +324,7 @@ config SND_SOC_FSL_ASOC_CARD
help
ALSA SoC Audio support with ASRC feature for Freescale SoCs that have
ESAI/SAI/SSI and connect with external CODECs such as WM8962, CS42888,
-CS4271, CS4272 and SGTL5000.
+CS4271, CS4272, SGTL5000 and TLV320AIC32x4.
Say Y if you want to add support for Freescale Generic ASoC Sound Card.
config SND_SOC_IMX_AUDMIX
diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c
index 32f8f756e6bb..a2dd3b6b7fec 100644
--- a/sound/soc/fsl/fsl-asoc-card.c
+++ b/sound/soc/fsl/fsl-asoc-card.c
@@ -617,6 +617,9 @@ static int fsl_asoc_card_probe(struct platform_device *pdev)
codec_dai_name = "sgtl5000";
priv->codec_priv.mclk_id = SGTL5000_SYSCLK;
priv->dai_fmt |= SND_SOC_DAIFMT_CBM_CFM;
+ } else if (of_device_is_compatible(np, "fsl,imx-audio-tlv320aic32x4")) {
+ codec_dai_name = "tlv320aic32x4-hifi";
+ priv->dai_fmt |= SND_SOC_DAIFMT_CBM_CFM;
} else if (of_device_is_compatible(np, "fsl,imx-audio-wm8962")) {
codec_dai_name = "wm8962";
priv->codec_priv.mclk_id = WM8962_SYSCLK_MCLK;
@@ -860,6 +863,7 @@ static const struct of_device_id fsl_asoc_card_dt_ids[] = {
{ .compatible = "fsl,imx-audio-ac97", },
{ .compatible = "fsl,imx-audio-cs42888", },
{ .compatible = "fsl,imx-audio-cs427x", },
+ { .compatible = "fsl,imx-audio-tlv320aic32x4", },
{ .compatible = "fsl,imx-audio-sgtl5000", },
{ .compatible = "fsl,imx-audio-wm8962", },
{ .compatible = "fsl,imx-audio-wm8960", },
--
2.17.1
[PATCH 1/2] ASoC: bindings: fsl-asoc-card: add compatible string for TLV320AIC32x4 codec
The TLV320AIC32x4 is commonly used on TQ-Systems starterkit mainboards for i.MX-based SoMs (i.MX6Q/DL, i.MX6UL, i.MX7) and LS1021A. Signed-off-by: Matthias Schiffer --- Documentation/devicetree/bindings/sound/fsl-asoc-card.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt b/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt index 63ebf52b43e8..f339be62e7e4 100644 --- a/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt +++ b/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt @@ -38,6 +38,8 @@ The compatible list for this generic sound card currently: "fsl,imx-audio-wm8524" + "fsl,imx-audio-tlv320aic32x4" + Required properties: - compatible : Contains one of entries in the compatible list. -- 2.17.1
[PATCH] drm: fsl-dcu: enable PIXCLK on LS1021A
The PIXCLK needs to be enabled in SCFG before accessing the DCU on LS1021A,
or the access will hang.
Signed-off-by: Matthias Schiffer
---
drivers/gpu/drm/fsl-dcu/Kconfig | 1 +
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 25 +++
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h | 3 +++
3 files changed, 29 insertions(+)
diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig b/drivers/gpu/drm/fsl-dcu/Kconfig
index d7dd8ba90e3a..9e5a35e7c00c 100644
--- a/drivers/gpu/drm/fsl-dcu/Kconfig
+++ b/drivers/gpu/drm/fsl-dcu/Kconfig
@@ -8,6 +8,7 @@ config DRM_FSL_DCU
select DRM_PANEL
select REGMAP_MMIO
select VIDEOMODE_HELPERS
+ select MFD_SYSCON if SOC_LS1021A
help
Choose this option if you have an Freescale DCU chipset.
If M is selected the module will be called fsl-dcu-drm.
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
index abbc1ddbf27f..8a7556655581 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -51,6 +51,23 @@ static const struct regmap_config fsl_dcu_regmap_config = {
.volatile_reg = fsl_dcu_drm_is_volatile_reg,
};
+static int fsl_dcu_scfg_config_ls1021a(struct device_node *np)
+{
+ struct regmap *scfg;
+
+ scfg = syscon_regmap_lookup_by_compatible("fsl,ls1021a-scfg");
+ if (IS_ERR(scfg))
+ return PTR_ERR(scfg);
+
+ /*
+* For simplicity, enable the PIXCLK unconditionally. It might
+* be possible to disable the clock in PM or on unload as a future
+* improvement.
+*/
+ return regmap_update_bits(scfg, SCFG_PIXCLKCR, SCFG_PIXCLKCR_PXCEN,
+ SCFG_PIXCLKCR_PXCEN);
+}
+
static void fsl_dcu_irq_uninstall(struct drm_device *dev)
{
struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
@@ -70,6 +87,14 @@ static int fsl_dcu_load(struct drm_device *dev, unsigned
long flags)
return ret;
}
+ if (of_device_is_compatible(fsl_dev->np, "fsl,ls1021a-dcu")) {
+ ret = fsl_dcu_scfg_config_ls1021a(fsl_dev->np);
+ if (ret < 0) {
+ dev_err(dev->dev, "failed to enable pixclk\n");
+ goto done;
+ }
+ }
+
ret = drm_vblank_init(dev, dev->mode_config.num_crtc);
if (ret < 0) {
dev_err(dev->dev, "failed to initialize vblank\n");
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
index e2049a0e8a92..566396013c04 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
@@ -160,6 +160,9 @@
#define FSL_DCU_ARGB 12
#define FSL_DCU_YUV422 14
+#define SCFG_PIXCLKCR 0x28
+#define SCFG_PIXCLKCR_PXCENBIT(31)
+
#define VF610_LAYER_REG_NUM9
#define LS1021A_LAYER_REG_NUM 10
--
2.17.1
[PATCH mmc-next v2] mmc: allow setting slot index via device tree alias
As with GPIO, UART and others, allow specifying the device index via the
aliases node in the device tree.
On embedded devices, there is often a combination of removable (e.g.
SD card) and non-removable MMC devices (e.g. eMMC).
Therefore the index might change depending on
* host of removable device
* removable card present or not
This makes it difficult to hardcode the root device, if it is on the
non-removable device. E.g. if SD card is present eMMC will be mmcblk1,
if SD card is not present at boot, eMMC will be mmcblk0.
All indices defined in the aliases node will be reserved for use by the
respective MMC device, moving the indices of devices that don't have an
alias up into the non-reserved range. If the aliases node is not found,
the driver will act as before.
This is a rebased and slightly cleaned up version of
https://www.spinics.net/lists/linux-mmc/msg26588.html .
Based-on-patch-by: Sascha Hauer
Link: https://lkml.org/lkml/2020/8/5/194
Signed-off-by: Matthias Schiffer
---
v2: fix missing symbols for modular mmcblock
drivers/mmc/core/block.c | 13 +++--
drivers/mmc/core/core.c | 40
drivers/mmc/core/core.h | 3 +++
drivers/mmc/core/host.c | 15 +--
4 files changed, 67 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 7896952de1ac..4620afaf0e50 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -38,6 +38,7 @@
#include
#include
#include
+#include
#include
#include
@@ -2260,9 +2261,17 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct
mmc_card *card,
int area_type)
{
struct mmc_blk_data *md;
- int devidx, ret;
+ int rsvidx, devidx = -1, ret;
+
+ rsvidx = mmc_get_reserved_index(card->host);
+ if (rsvidx >= 0)
+ devidx = ida_simple_get(_blk_ida, rsvidx, rsvidx + 1,
+ GFP_KERNEL);
+ if (devidx < 0)
+ devidx = ida_simple_get(_blk_ida,
+ mmc_first_nonreserved_index(),
+ max_devices, GFP_KERNEL);
- devidx = ida_simple_get(_blk_ida, 0, max_devices, GFP_KERNEL);
if (devidx < 0) {
/*
* We get -ENOSPC because there are no more any available
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 8ccae6452b9c..5bce281a5faa 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2419,10 +2419,50 @@ void mmc_unregister_pm_notifier(struct mmc_host *host)
}
#endif
+static int mmc_max_reserved_idx = -1;
+
+/**
+ * mmc_first_nonreserved_index() - get the first index that is not reserved
+ */
+int mmc_first_nonreserved_index(void)
+{
+ return mmc_max_reserved_idx + 1;
+}
+EXPORT_SYMBOL(mmc_first_nonreserved_index);
+
+/**
+ * mmc_get_reserved_index() - get the index reserved for this MMC host
+ *
+ * Returns:
+ * The index reserved for this host on success,
+ * negative error if no index is reserved for this host
+ */
+int mmc_get_reserved_index(struct mmc_host *host)
+{
+ return of_alias_get_id(host->parent->of_node, "mmc");
+}
+EXPORT_SYMBOL(mmc_get_reserved_index);
+
+static void __init mmc_of_reserve_idx(void)
+{
+ int max;
+
+ max = of_alias_get_highest_id("mmc");
+ if (max < 0)
+ return;
+
+ mmc_max_reserved_idx = max;
+
+ pr_debug("MMC: reserving %d slots for OF aliases\n",
+mmc_max_reserved_idx + 1);
+}
+
static int __init mmc_init(void)
{
int ret;
+ mmc_of_reserve_idx();
+
ret = mmc_register_bus();
if (ret)
return ret;
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 575ac0257af2..6aef6cf4e90f 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -79,6 +79,9 @@ int mmc_attach_mmc(struct mmc_host *host);
int mmc_attach_sd(struct mmc_host *host);
int mmc_attach_sdio(struct mmc_host *host);
+int mmc_first_nonreserved_index(void);
+int mmc_get_reserved_index(struct mmc_host *host);
+
/* Module parameters */
extern bool use_spi_crc;
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index ce43f7573d80..386e15afde83 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -387,6 +387,7 @@ struct mmc_host *mmc_alloc_host(int extra, struct device
*dev)
{
int err;
struct mmc_host *host;
+ int alias_id, min_idx, max_idx;
host = kzalloc(sizeof(struct mmc_host) + extra, GFP_KERNEL);
if (!host)
@@ -395,7 +396,18 @@ struct mmc_host *mmc_alloc_host(int extra, struct device
*dev)
/* scanning will be enabled when we're ready */
host->rescan_disable = 1;
- err = ida_simple_get(_host_ida, 0, 0, GFP_KERNEL);
+ host->parent = dev;
+
+ alia
[PATCH mmc-next] mmc: allow setting slot index via device tree alias
As with GPIO, UART and others, allow specifying the device index via the
aliases node in the device tree.
On embedded devices, there is often a combination of removable (e.g.
SD card) and non-removable MMC devices (e.g. eMMC).
Therefore the index might change depending on
* host of removable device
* removable card present or not
This makes it difficult to hardcode the root device, if it is on the
non-removable device. E.g. if SD card is present eMMC will be mmcblk1,
if SD card is not present at boot, eMMC will be mmcblk0.
All indices defined in the aliases node will be reserved for use by the
respective MMC device, moving the indices of devices that don't have an
alias up into the non-reserved range. If the aliases node is not found,
the driver will act as before.
This is a rebased and slightly cleaned up version of
https://www.spinics.net/lists/linux-mmc/msg26588.html .
Based-on-patch-by: Sascha Hauer
Signed-off-by: Matthias Schiffer
Link: https://lkml.org/lkml/2020/8/5/194
---
drivers/mmc/core/block.c | 13 +++--
drivers/mmc/core/core.c | 38 ++
drivers/mmc/core/core.h | 3 +++
drivers/mmc/core/host.c | 15 +--
4 files changed, 65 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 7896952de1ac..4620afaf0e50 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -38,6 +38,7 @@
#include
#include
#include
+#include
#include
#include
@@ -2260,9 +2261,17 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct
mmc_card *card,
int area_type)
{
struct mmc_blk_data *md;
- int devidx, ret;
+ int rsvidx, devidx = -1, ret;
+
+ rsvidx = mmc_get_reserved_index(card->host);
+ if (rsvidx >= 0)
+ devidx = ida_simple_get(_blk_ida, rsvidx, rsvidx + 1,
+ GFP_KERNEL);
+ if (devidx < 0)
+ devidx = ida_simple_get(_blk_ida,
+ mmc_first_nonreserved_index(),
+ max_devices, GFP_KERNEL);
- devidx = ida_simple_get(_blk_ida, 0, max_devices, GFP_KERNEL);
if (devidx < 0) {
/*
* We get -ENOSPC because there are no more any available
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 8ccae6452b9c..39aca8adacd1 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2419,10 +2419,48 @@ void mmc_unregister_pm_notifier(struct mmc_host *host)
}
#endif
+static int mmc_max_reserved_idx = -1;
+
+/**
+ * mmc_first_nonreserved_index() - get the first index that is not reserved
+ */
+int mmc_first_nonreserved_index(void)
+{
+ return mmc_max_reserved_idx + 1;
+}
+
+/**
+ * mmc_get_reserved_index() - get the index reserved for this MMC host
+ *
+ * Returns:
+ * The index reserved for this host on success,
+ * negative error if no index is reserved for this host
+ */
+int mmc_get_reserved_index(struct mmc_host *host)
+{
+ return of_alias_get_id(host->parent->of_node, "mmc");
+}
+
+static void __init mmc_of_reserve_idx(void)
+{
+ int max;
+
+ max = of_alias_get_highest_id("mmc");
+ if (max < 0)
+ return;
+
+ mmc_max_reserved_idx = max;
+
+ pr_debug("MMC: reserving %d slots for OF aliases\n",
+mmc_max_reserved_idx + 1);
+}
+
static int __init mmc_init(void)
{
int ret;
+ mmc_of_reserve_idx();
+
ret = mmc_register_bus();
if (ret)
return ret;
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 575ac0257af2..6aef6cf4e90f 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -79,6 +79,9 @@ int mmc_attach_mmc(struct mmc_host *host);
int mmc_attach_sd(struct mmc_host *host);
int mmc_attach_sdio(struct mmc_host *host);
+int mmc_first_nonreserved_index(void);
+int mmc_get_reserved_index(struct mmc_host *host);
+
/* Module parameters */
extern bool use_spi_crc;
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index ce43f7573d80..386e15afde83 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -387,6 +387,7 @@ struct mmc_host *mmc_alloc_host(int extra, struct device
*dev)
{
int err;
struct mmc_host *host;
+ int alias_id, min_idx, max_idx;
host = kzalloc(sizeof(struct mmc_host) + extra, GFP_KERNEL);
if (!host)
@@ -395,7 +396,18 @@ struct mmc_host *mmc_alloc_host(int extra, struct device
*dev)
/* scanning will be enabled when we're ready */
host->rescan_disable = 1;
- err = ida_simple_get(_host_ida, 0, 0, GFP_KERNEL);
+ host->parent = dev;
+
+ alias_id = mmc_get_reserved_index(host);
+ if (alias_id >= 0) {
+ min_idx = alias_id;
+ max_
Re: (EXT) Re: (EXT) Re: Consistent block device references for root= cmdline
On Tue, 2020-07-07 at 16:14 +0200, Ulf Hansson wrote: > On Thu, 11 Jun 2020 at 13:20, Matthias Schiffer > wrote: > > > > On Wed, 2020-06-10 at 16:52 +0200, Ulf Hansson wrote: > > > On Wed, 10 Jun 2020 at 15:15, Matthias Schiffer > > > wrote: > > > > > > > > Hello all, > > > > > > > > there have been numerous attempts to make the numbering of > > > > mmcblk > > > > devices consistent, mostly by using aliases from the DTS ([1], > > > > [2], > > > > [3]), but all have been (rightfully) rejected. Unless I have > > > > overlooked > > > > a more recent development, no attempts for a different solution > > > > were > > > > made. > > > > > > According to aliases attempts, I think those have failed, mainly > > > because of two reasons. > > > > > > 1. Arguments stating that LABELs/UUIDs are variable alternatives. > > > This > > > isn't the case, which I think was also concluded from the several > > > earlier discussions. > > > 2. Patches that tried adding support for mmc aliases, were not > > > correctly coded. More precisely, what needs to be addressed is > > > that > > > the mmc core also preserves the same ids to be set for the host > > > class > > > as the block device, mmc[n] must correspond to mmcblk[n]. > > > > > > > > > > > As far as I can tell, the core of the issue seems to be the > > > > following: > > > > > > > > The existing solutions like LABELs and UUIDs are viable > > > > alternatives in > > > > many cases, but in particular on embedded systems, this is not > > > > quite > > > > sufficient: In addition to the problem that more knowledge > > > > about > > > > the > > > > system to boot is required in the bootloader, this approach > > > > fails > > > > completely when the same firmware image exists on multiple > > > > devices, > > > > for > > > > example on an eMMC and an SD card - not an entirely uncommon > > > > situation > > > > during the development of embedded systems. > > > > > > > > With udev, I can refer to a specific partition using a path > > > > like > > > > /dev/disk/by-path/platform-2194000.usdhc-part2. In [4] it was > > > > proposed > > > > to add a way to refer to a device path/phandle from the kernel > > > > command > > > > line. Has there been any progress on this proposal? > > > > > > Lots of time during the years I have been approached, both > > > publicly > > > and offlist, about whether it would be possible to add support > > > for > > > "consistent" mmcblk devices. To me, I am fine with the aliases > > > approach, as long as it gets implemented correctly. > > > > > > It seems the principal technical problem is the one described here: > > > > https://www.spinics.net/lists/linux-mmc/msg26602.html > > > > I don't see any way to solve this completely, as there seem to be > > two > > fundamentally conflicting requirements: > > > > 1) If a mounted SD card is replaced, it must be assigned a new > > /dev/mmcblkN > > 2) /dev/mmcblkN should always match the configured alias IDs > > > > What is the reason we need 1) - is it possible to have multiple > > eMMCs > > or SD cards on a single bus, with detection at runtime? > > Yes. The mmc_bus_type holds all cards - all discovered at runtime. > > > Otherwise I'd > > expect this to be handled like other drives with removable media > > (CD, > > floppy), with static device assignment. > > > > If we can't give up on 1) for some reason, we'll have to accept > > that we > > can't guarantee 2) unconditionally. As far as I can tell, the > > patches > > provided by Sascha and others did that in a reasonable way: The > > aliases > > would work in most cases - in particular for the first assignment > > on > > boot, which is required to find the correct rootfs. > > Well, if we would pre-parse the DTB to look for all "mmc block > aliases" and keep a mark of those ids as being reserved, then we > should be able to cope with both 1) and 2). Hello Ulf, it seems to me like Sascha's patches from 2014 do precisely that: https://www.spinics.net/lists/linux-mmc/msg26587.html https://www.spinics.net/lists/linux-mmc/msg26588.html I haven't looked into porting this to a modern kernel yet, but do you think that the approach is sound? > > > > > > > > > > > > > > Kind regards, > > > > Matthias > > > > > > > > > > > > [1] https://patchwork.kernel.org/patch/8685711/ > > > > [2] https://lore.kernel.org/patchwork/cover/674381/ > > > > [3] https://www.spinics.net/lists/linux-mmc/msg26586.html > > > > [4] https://www.spinics.net/lists/linux-mmc/msg26708.html > > > > > > Kind regards > Uffe
[PATCH v3] arm: dts: imx7: add QSPI
In preparation for an update of the TQ-Systems TQMa7x/MBa7x DTS, add the
QSPI controller to imx7s.dtsi.
Based-on-patch-by: Han Xu
Signed-off-by: Matthias Schiffer
---
v3:
- reverted node name change
v2:
- renamed node and label
- reordered properties
arch/arm/boot/dts/imx7s.dtsi | 13 +
1 file changed, 13 insertions(+)
diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index 1cfaf410aa43..85b8eddd77f2 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -1162,6 +1162,19 @@
status = "disabled";
};
+ qspi: spi@30bb {
+ compatible = "fsl,imx7d-qspi";
+ reg = <0x30bb 0x1>, <0x6000
0x1000>;
+ reg-names = "QuadSPI", "QuadSPI-memory";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ interrupts = ;
+ clocks = < IMX7D_QSPI_ROOT_CLK>,
+ < IMX7D_QSPI_ROOT_CLK>;
+ clock-names = "qspi_en", "qspi";
+ status = "disabled";
+ };
+
sdma: sdma@30bd {
compatible = "fsl,imx7d-sdma", "fsl,imx35-sdma";
reg = <0x30bd 0x1>;
--
2.17.1
Re: [PATCH v2] arm: dts: imx7: add QSPI
On Wed, 2020-07-29 at 09:11 +0200, Matthias Schiffer wrote:
> In preparation for an update of the TQ-Systems TQMa7x/MBa7x DTS, add
> the
> QSPI controller to imx7s.dtsi.
>
> Based-on-patch-by: Han Xu
> Signed-off-by: Matthias Schiffer
> ---
>
> v2:
> - renamed node and label
> - reordered properties
> (as suggested by Marco Felsch)
Ugh, I neglected to check for compile warnings after adjusting the node
name. This now gives the following warning:
arch/arm/boot/dts/imx7s.dtsi:1165.24-1176.6: Warning (spi_bus_bridge):
/soc/bus@3080/qspi@30bb: node name for SPI buses should be
'spi'
So I guess this should be called spi@ after all?
>
>
> arch/arm/boot/dts/imx7s.dtsi | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/arch/arm/boot/dts/imx7s.dtsi
> b/arch/arm/boot/dts/imx7s.dtsi
> index 1cfaf410aa43..22e4c38223bd 100644
> --- a/arch/arm/boot/dts/imx7s.dtsi
> +++ b/arch/arm/boot/dts/imx7s.dtsi
> @@ -1162,6 +1162,19 @@
> status = "disabled";
> };
>
> + qspi: qspi@30bb {
> + compatible = "fsl,imx7d-qspi";
> + reg = <0x30bb 0x1>, <0x6000
> 0x1000>;
> + reg-names = "QuadSPI", "QuadSPI-
> memory";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + interrupts = IRQ_TYPE_LEVEL_HIGH>;
> + clocks = < IMX7D_QSPI_ROOT_CLK>,
> + < IMX7D_QSPI_ROOT_CLK>;
> + clock-names = "qspi_en", "qspi";
> + status = "disabled";
> + };
> +
> sdma: sdma@30bd {
> compatible = "fsl,imx7d-sdma",
> "fsl,imx35-sdma";
> reg = <0x30bd 0x1>;
[PATCH v2] arm: dts: imx7: add QSPI
In preparation for an update of the TQ-Systems TQMa7x/MBa7x DTS, add the
QSPI controller to imx7s.dtsi.
Based-on-patch-by: Han Xu
Signed-off-by: Matthias Schiffer
---
v2:
- renamed node and label
- reordered properties
(as suggested by Marco Felsch)
arch/arm/boot/dts/imx7s.dtsi | 13 +
1 file changed, 13 insertions(+)
diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index 1cfaf410aa43..22e4c38223bd 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -1162,6 +1162,19 @@
status = "disabled";
};
+ qspi: qspi@30bb {
+ compatible = "fsl,imx7d-qspi";
+ reg = <0x30bb 0x1>, <0x6000
0x1000>;
+ reg-names = "QuadSPI", "QuadSPI-memory";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ interrupts = ;
+ clocks = < IMX7D_QSPI_ROOT_CLK>,
+ < IMX7D_QSPI_ROOT_CLK>;
+ clock-names = "qspi_en", "qspi";
+ status = "disabled";
+ };
+
sdma: sdma@30bd {
compatible = "fsl,imx7d-sdma", "fsl,imx35-sdma";
reg = <0x30bd 0x1>;
--
2.17.1
Re: (EXT) Re: [PATCH] arm: dts: imx7: add QSPI
On Tue, 2020-07-28 at 15:51 +0200, Marco Felsch wrote:
> Hi Matthias,
>
> thanks for the patch.
>
> On 20-07-28 13:28, Matthias Schiffer wrote:
> > In preparation for an update of the TQ-Systems TQMa7x/MBa7x DTS,
> > add the
> > QSPI controller to imx7s.dtsi.
> >
> > Based-on-patch-by: Han Xu
> > Signed-off-by: Matthias Schiffer > >
> > ---
> > arch/arm/boot/dts/imx7s.dtsi | 13 +
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/imx7s.dtsi
> > b/arch/arm/boot/dts/imx7s.dtsi
> > index 1cfaf410aa43..e45683e61593 100644
> > --- a/arch/arm/boot/dts/imx7s.dtsi
> > +++ b/arch/arm/boot/dts/imx7s.dtsi
> > @@ -1162,6 +1162,19 @@
> > status = "disabled";
> > };
> >
> > + qspi1: spi@30bb {
>
> Are there more controllers and why not using "qspi@30bb" as node
> name?
The vast majority of QSPI controllers use spi@ node names, qspi@ only
appears in a single example in Documentation/devicetree/bindings/, and
in no actual DTS(I) files.
There is only one controller. The label "qspi1" is chosen as this has
been in use in the linux-imx vendor kernels for years; IMHO, switching
to "qspi" would just cause unnecessary churn for dependent device
trees. I have no strong opinions on this though.
>
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + compatible = "fsl,imx7d-qspi";
> > + reg = <0x30bb 0x1>, <0x6000
> > 0x1000>;
> > + reg-names = "QuadSPI", "QuadSPI-
> > memory";
>
> The node should begin with compatible, reg, reg-names properties.
> Pls check the current .dtsi file for examples.
Thanks, will fix.
>
> > + interrupts = > IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = < IMX7D_QSPI_ROOT_CLK>,
> > + < IMX7D_QSPI_ROOT_CLK>;
> > + clock-names = "qspi_en", "qspi";
> > + status = "disabled";
> > + };
> > +
> > sdma: sdma@30bd {
> > compatible = "fsl,imx7d-sdma",
> > "fsl,imx35-sdma";
> > reg = <0x30bd 0x1>;
>
> Regards,
> Marco
[PATCH] arm: dts: imx7: add QSPI
In preparation for an update of the TQ-Systems TQMa7x/MBa7x DTS, add the
QSPI controller to imx7s.dtsi.
Based-on-patch-by: Han Xu
Signed-off-by: Matthias Schiffer
---
arch/arm/boot/dts/imx7s.dtsi | 13 +
1 file changed, 13 insertions(+)
diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index 1cfaf410aa43..e45683e61593 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -1162,6 +1162,19 @@
status = "disabled";
};
+ qspi1: spi@30bb {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "fsl,imx7d-qspi";
+ reg = <0x30bb 0x1>, <0x6000
0x1000>;
+ reg-names = "QuadSPI", "QuadSPI-memory";
+ interrupts = ;
+ clocks = < IMX7D_QSPI_ROOT_CLK>,
+ < IMX7D_QSPI_ROOT_CLK>;
+ clock-names = "qspi_en", "qspi";
+ status = "disabled";
+ };
+
sdma: sdma@30bd {
compatible = "fsl,imx7d-sdma", "fsl,imx35-sdma";
reg = <0x30bd 0x1>;
--
2.17.1
[PATCH] arm: dts: ls1021a: fix QuadSPI-memory reg range
According to the Reference Manual, the correct size is 512 MiB.
Without this fix, probing the QSPI fails:
fsl-quadspi 155.spi: ioremap failed for resource
[mem 0x4000-0x7fff]
fsl-quadspi 155.spi: Freescale QuadSPI probe failed
fsl-quadspi: probe of 155.spi failed with error -12
Fixes: 85f8ee78ab72 ("ARM: dts: ls1021a: Add support for QSPI with ls1021a SoC")
Signed-off-by: Matthias Schiffer
---
arch/arm/boot/dts/ls1021a.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
index b2ff27af090e..9435ce527e85 100644
--- a/arch/arm/boot/dts/ls1021a.dtsi
+++ b/arch/arm/boot/dts/ls1021a.dtsi
@@ -181,7 +181,7 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0x0 0x155 0x0 0x1>,
- <0x0 0x4000 0x0 0x4000>;
+ <0x0 0x4000 0x0 0x2000>;
reg-names = "QuadSPI", "QuadSPI-memory";
interrupts = ;
clock-names = "qspi_en", "qspi";
--
2.17.1
Re: (EXT) Re: [PATCH] mtd: spi-nor: micron-st: enable 4-byte opcodes for n25q512a
On Mon, 2020-06-29 at 13:02 +, tudor.amba...@microchip.com wrote: > Hi, Matthias, > > On 6/10/20 12:16 PM, Matthias Schiffer wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > know the content is safe > > > > From: Michael Krummsdorf > > > > Set SPI_NOR_4B_OPCODES, as the flash supports 4-byte opcodes. > > I remember that we stripped SPI_NOR_4B_OPCODES intentionally for the > n25q > flashes, check https://lkml.org/lkml/2019/12/5/50. > > Cheers, > ta Ah, thank you for the pointer. Looking at a boot log of a more recent kernel again, the SPI-NOR flash on our hardware is actually detected as mt25qu512a and not n25q512a now, so everything should work as expected without this patch.
[PATCH RESEND v2 3/4] drm/panel: simple: add CDTech S070PWS19HP-FC21 and S070SWV29HG-DC44
From: Michael Krummsdorf
Add support for the CDTech Electronics displays S070PWS19HP-FC21
(7.0" WSVGA) and S070SWV29HG-DC44 (7.0" WVGA) to panel-simple.
Signed-off-by: Michael Krummsdorf
Signed-off-by: Matthias Schiffer
---
v2:
- removed vrefresh
- added connector_type
drivers/gpu/drm/panel/panel-simple.c | 60
1 file changed, 60 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-simple.c
b/drivers/gpu/drm/panel/panel-simple.c
index 6764ac630e22..ee9815e5eee8 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1296,6 +1296,60 @@ static const struct panel_desc cdtech_s043wq26h_ct7 = {
.bus_flags = DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
};
+/* S070PWS19HP-FC21 2017/04/22 */
+static const struct drm_display_mode cdtech_s070pws19hp_fc21_mode = {
+ .clock = 51200,
+ .hdisplay = 1024,
+ .hsync_start = 1024 + 160,
+ .hsync_end = 1024 + 160 + 20,
+ .htotal = 1024 + 160 + 20 + 140,
+ .vdisplay = 600,
+ .vsync_start = 600 + 12,
+ .vsync_end = 600 + 12 + 3,
+ .vtotal = 600 + 12 + 3 + 20,
+ .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+};
+
+static const struct panel_desc cdtech_s070pws19hp_fc21 = {
+ .modes = _s070pws19hp_fc21_mode,
+ .num_modes = 1,
+ .bpc = 6,
+ .size = {
+ .width = 154,
+ .height = 86,
+ },
+ .bus_format = MEDIA_BUS_FMT_RGB666_1X18,
+ .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_POSEDGE,
+ .connector_type = DRM_MODE_CONNECTOR_DPI,
+};
+
+/* S070SWV29HG-DC44 2017/09/21 */
+static const struct drm_display_mode cdtech_s070swv29hg_dc44_mode = {
+ .clock = 33300,
+ .hdisplay = 800,
+ .hsync_start = 800 + 210,
+ .hsync_end = 800 + 210 + 2,
+ .htotal = 800 + 210 + 2 + 44,
+ .vdisplay = 480,
+ .vsync_start = 480 + 22,
+ .vsync_end = 480 + 22 + 2,
+ .vtotal = 480 + 22 + 2 + 21,
+ .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+};
+
+static const struct panel_desc cdtech_s070swv29hg_dc44 = {
+ .modes = _s070swv29hg_dc44_mode,
+ .num_modes = 1,
+ .bpc = 6,
+ .size = {
+ .width = 154,
+ .height = 86,
+ },
+ .bus_format = MEDIA_BUS_FMT_RGB666_1X18,
+ .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_POSEDGE,
+ .connector_type = DRM_MODE_CONNECTOR_DPI,
+};
+
static const struct drm_display_mode cdtech_s070wv95_ct16_mode = {
.clock = 35000,
.hdisplay = 800,
@@ -3674,6 +3728,12 @@ static const struct of_device_id platform_of_match[] = {
}, {
.compatible = "cdtech,s043wq26h-ct7",
.data = _s043wq26h_ct7,
+ }, {
+ .compatible = "cdtech,s070pws19hp-fc21",
+ .data = _s070pws19hp_fc21,
+ }, {
+ .compatible = "cdtech,s070swv29hg-dc44",
+ .data = _s070swv29hg_dc44,
}, {
.compatible = "cdtech,s070wv95-ct16",
.data = _s070wv95_ct16,
--
2.17.1
[PATCH RESEND v2 1/4] dt-bindings: display: simple: add CDTech S070PWS19HP-FC21 and S070SWV29HG-DC44
Add the CDTech Electronics displays S070PWS19HP-FC21 (7.0" WSVGA) and S070SWV29HG-DC44 (7.0" WVGA) to the panel-simple compatible list. Signed-off-by: Matthias Schiffer --- v2: no changes .../devicetree/bindings/display/panel/panel-simple.yaml | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml index 31e3efc73e00..2ddb520edc6d 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml @@ -81,6 +81,10 @@ properties: - boe,nv140fhmn49 # CDTech(H.K.) Electronics Limited 4.3" 480x272 color TFT-LCD panel - cdtech,s043wq26h-ct7 +# CDTech(H.K.) Electronics Limited 7" WSVGA (1024x600) TFT LCD Panel + - cdtech,s070pws19hp-fc21 +# CDTech(H.K.) Electronics Limited 7" WVGA (800x480) TFT LCD Panel + - cdtech,s070swv29hg-dc44 # CDTech(H.K.) Electronics Limited 7" 800x480 color TFT-LCD panel - cdtech,s070wv95-ct16 # Chunghwa Picture Tubes Ltd. 7" WXGA TFT LCD panel -- 2.17.1
[PATCH RESEND v2 0/4] panel-simple: CDTech S070PWS19HP-FC21 and S070SWV29HG-DC44, Tianma TM070JVHG33
From: Matthias Schiffer This adds a few panels TQ-Systems uses with various starterkit mainboards. Device trees actually using these panels will be added with a later submission. Matthias Schiffer (2): dt-bindings: display: simple: add CDTech S070PWS19HP-FC21 and S070SWV29HG-DC44 dt-bindings: display: simple: add Tianma TM070JVHG33 Max Merchel (1): drm/panel: simple: add Tianma TM070JVHG33 Michael Krummsdorf (1): drm/panel: simple: add CDTech S070PWS19HP-FC21 and S070SWV29HG-DC44 .../bindings/display/panel/panel-simple.yaml | 6 ++ drivers/gpu/drm/panel/panel-simple.c | 75 +++ 2 files changed, 81 insertions(+) -- 2.17.1
[PATCH RESEND v2 4/4] drm/panel: simple: add Tianma TM070JVHG33
From: Max Merchel
Add support for the Tianma Micro-electronics TM070JVHG33 7.0" WXGA display
to panel-simple.
Signed-off-by: Max Merchel
Signed-off-by: Matthias Schiffer
---
v2:
- added connector_type
- fixed bus_format
drivers/gpu/drm/panel/panel-simple.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-simple.c
b/drivers/gpu/drm/panel/panel-simple.c
index ee9815e5eee8..54f121256832 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -3380,6 +3380,18 @@ static const struct panel_desc tianma_tm070jdhg30 = {
.connector_type = DRM_MODE_CONNECTOR_LVDS,
};
+static const struct panel_desc tianma_tm070jvhg33 = {
+ .timings = _tm070jdhg30_timing,
+ .num_timings = 1,
+ .bpc = 8,
+ .size = {
+ .width = 150,
+ .height = 94,
+ },
+ .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
+ .connector_type = DRM_MODE_CONNECTOR_LVDS,
+};
+
static const struct display_timing tianma_tm070rvhg71_timing = {
.pixelclock = { 2770, 2920, 3960 },
.hactive = { 800, 800, 800 },
@@ -3983,6 +3995,9 @@ static const struct of_device_id platform_of_match[] = {
}, {
.compatible = "tianma,tm070jdhg30",
.data = _tm070jdhg30,
+ }, {
+ .compatible = "tianma,tm070jvhg33",
+ .data = _tm070jvhg33,
}, {
.compatible = "tianma,tm070rvhg71",
.data = _tm070rvhg71,
--
2.17.1
