[PATCH] can: ti_hecc: fix close when napi poll is active
When closing this CAN interface while napi poll is active, for example with:
`ip link set can0 down` several interfaces freeze. This seemed to be caused
by napi_disable called from ti_hecc_close expecting the scheduled probe to
either return quota or call napi_complete. Since the poll functions has a
check for netif_running it returns 0 and doesn't call napi_complete and hence
violates the napi its expectation.
So remove this check, so either napi_complete is called or quota is returned.
Signed-off-by: Jeroen Hofstee
---
drivers/net/can/ti_hecc.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index db6ea93..42813d3 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -603,9 +603,6 @@ static int ti_hecc_rx_poll(struct napi_struct *napi, int
quota)
u32 mbx_mask;
unsigned long pending_pkts, flags;
- if (!netif_running(ndev))
- return 0;
-
while ((pending_pkts = hecc_read(priv, HECC_CANRMP)) &&
num_pkts < quota) {
mbx_mask = BIT(priv->rx_next); /* next rx mailbox to process */
--
2.7.4
Re: Device Tree Blob (DTB) licence
Hi, On 22-05-15 12:05, Yann Droneaud wrote: Le mardi 05 mai 2015 à 11:41 -0500, Rob Herring a écrit : On Tue, May 5, 2015 at 5:05 AM, Yann Droneaud wrote: I believe Device Tree Blob (.dtb file) built from kernel's Device Tree Sources (.dts, which #include .dtsi, which #include .h) using Device Tree Compiler (dtc) are covered by GNU General Public Licence v2 (GPLv2), but cannot find any reference. By default yes, but we've been steering people to dual license them GPL/BSD. obviously these files should be reusable. If there is a license issue with that it should be fixed. cc-ing freebsd-...@freebsd.org. I am not a lawyer, Jeroen -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] can: dev: add software tx timestamps
Hello Vincent,
On 1/10/21 11:35 AM, Vincent Mailhol wrote:
Call skb_tx_timestamp() within can_put_echo_skb() so that a software
tx timestamp gets attached on the skb.
[..]
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 3486704c8a95..3904e0874543 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -484,6 +484,8 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device
*dev,
/* save this skb for tx interrupt echo handling */
priv->echo_skb[idx] = skb;
+
+ skb_tx_timestamp(skb);
} else {
/* locking problem with netif_stop_queue() ?? */
netdev_err(dev, "%s: BUG! echo_skb %d is occupied!\n",
__func__, idx);
Personally, I would put the skb_tx_timestamp, before adding it to the array:
/* make settings for echo to reduce code in irq context */
skb->pkt_type = PACKET_BROADCAST;
skb->ip_summed = CHECKSUM_UNNECESSARY;
skb->dev = dev;
+ skb_tx_timestamp(skb);
/* save this skb for tx interrupt echo handling */
priv->echo_skb[idx] = skb;
I don't think it actually matters though.
Regards,
Jeroen
[PATCH] can: don't count arbitration lose as an error
Losing arbitration is normal in a CAN-bus network, it means that a
higher priority frame is being send and the pending message will be
retried later. Hence most driver only increment arbitration_lost, but
the sja1000 and sun4i driver also incremeant tx_error, causing errors
to be reported on a normal functioning CAN-bus. So stop counting them
as errors.
For completeness, the Kvaser USB hybra also increments the tx_error
on arbitration lose, but it does so in single shot. Since in that
case the message is not retried, that behaviour is kept.
Signed-off-by: Jeroen Hofstee
---
drivers/net/can/sja1000/sja1000.c | 1 -
drivers/net/can/sun4i_can.c | 1 -
2 files changed, 2 deletions(-)
diff --git a/drivers/net/can/sja1000/sja1000.c
b/drivers/net/can/sja1000/sja1000.c
index 9f107798f904..25a4d7d0b349 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -474,7 +474,6 @@ static int sja1000_err(struct net_device *dev, uint8_t
isrc, uint8_t status)
netdev_dbg(dev, "arbitration lost interrupt\n");
alc = priv->read_reg(priv, SJA1000_ALC);
priv->can.can_stats.arbitration_lost++;
- stats->tx_errors++;
cf->can_id |= CAN_ERR_LOSTARB;
cf->data[0] = alc & 0x1f;
}
diff --git a/drivers/net/can/sun4i_can.c b/drivers/net/can/sun4i_can.c
index e2c6cf4b2228..b3f2f4fe5ee0 100644
--- a/drivers/net/can/sun4i_can.c
+++ b/drivers/net/can/sun4i_can.c
@@ -604,7 +604,6 @@ static int sun4i_can_err(struct net_device *dev, u8 isrc,
u8 status)
netdev_dbg(dev, "arbitration lost interrupt\n");
alc = readl(priv->base + SUN4I_REG_STA_ADDR);
priv->can.can_stats.arbitration_lost++;
- stats->tx_errors++;
if (likely(skb)) {
cf->can_id |= CAN_ERR_LOSTARB;
cf->data[0] = (alc >> 8) & 0x1f;
--
2.17.1
Re: [U-Boot] [PATCH] kconfig: Fix compiler warning in menu.c
Hello Hans,
On 31-07-14 16:21, Hans de Goede wrote:
This fixes the following compiler warning:
In file included from scripts/kconfig/zconf.tab.c:2537:0:
scripts/kconfig/menu.c: In function ‘get_symbol_str’:
scripts/kconfig/menu.c:590:18: warning: ‘jump’ may be used uninitialized in
this function [-Wmaybe-uninitialized]
jump->offset = strlen(r->s);
^
In file included from scripts/kconfig/zconf.tab.c:2537:0:
scripts/kconfig/menu.c:551:19: note: ‘jump’ was declared here
struct jump_key *jump;
Signed-off-by: Hans de Goede
---
scripts/kconfig/menu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index a26cc5d..584e0fc 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -548,7 +548,7 @@ static void get_prompt_str(struct gstr *r, struct property
*prop,
{
int i, j;
struct menu *submenu[8], *menu, *location = NULL;
- struct jump_key *jump;
+ struct jump_key *jump = NULL;
str_printf(r, _("Prompt: %s\n"), _(prop->text));
menu = prop->menu->parent;
just curious, which compiler is this? Since it is a false positive.
(and it could know)
Regards,
Jeroen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [U-Boot] [PATCH] kconfig: Fix compiler warning in menu.c
Hello Hans,
On 31-07-14 16:21, Hans de Goede wrote:
This fixes the following compiler warning:
In file included from scripts/kconfig/zconf.tab.c:2537:0:
scripts/kconfig/menu.c: In function ‘get_symbol_str’:
scripts/kconfig/menu.c:590:18: warning: ‘jump’ may be used uninitialized in
this function [-Wmaybe-uninitialized]
jump->offset = strlen(r->s);
^
In file included from scripts/kconfig/zconf.tab.c:2537:0:
scripts/kconfig/menu.c:551:19: note: ‘jump’ was declared here
struct jump_key *jump;
Signed-off-by: Hans de Goede
---
scripts/kconfig/menu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index a26cc5d..584e0fc 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -548,7 +548,7 @@ static void get_prompt_str(struct gstr *r, struct property
*prop,
{
int i, j;
struct menu *submenu[8], *menu, *location = NULL;
- struct jump_key *jump;
+ struct jump_key *jump = NULL;
str_printf(r, _("Prompt: %s\n"), _(prop->text));
menu = prop->menu->parent;
And if you combine head && location into a single boolean,
does this warning still occur? Not that it matters that much
in this case, since it is a host tool, but I guess the compiler
does intend to assign NULL while there is no reason to do so.
Regards,
Jeroen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [U-Boot] [RFC] kbuild.h: workaround for llvm IAS
Hello Masahiro, On 27-06-14 08:38, Masahiro Yamada wrote: KBuild (ab)uses the asm statement to write to a file and llvm integrated as chokes about these invalid asm statements. Workaround it by making it look like valid asm code. Signed-off-by: Jeroen Hofstee I think Linux has the same problem. Are you willing to this patch to linux-kbuild ML? Or fixing U-Boot only? I don't mind in general, but it is just noise for them (cc-ing them to create some). For u-boot (ARM) you actually get a valid binary with this patch after clang support has landed, for linux you just get other errors as far as I tried (native only), patch below. However in linux there seem more spots relying on the format, e.g. arch/ia64/kvm/Makefile arch/ia64/kernel/Makefile arch/um/Makefile So if anything, I think this should be made a general rules first in the makefiles. It seems stupid to potentially break something while it gains nothing. So yes, u-boot only afaic, or does that make your syncing more difficult? I don't think syncing would be difficult. BTW, do you know how they resolve this build error in other projects, for example, in llvmlinux ? http://llvm.linuxfoundation.org/index.php/Main_Page Linux folks merged Clang support into the top Makefile, but not into ./Kbuild. I don't know why. I don't know how the llvmlinux people do it, but the alternative is to add -no-integrated-as for clang when compiling such files (or use an older clang version, since that used to be the default). Since gcc's LTO dislikes the asm-offset.c technique as well, I think it is better to actually create valid asm, so it no longer depends on compiler features at all. I will leave it up to the llvmlinux folks to come up with a solution for linux though... Regards, Jeroen -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [U-Boot] [RFC] kbuild.h: workaround for llvm IAS
Hi Masahiro,
On 24-06-14 14:53, Masahiro Yamada wrote:
On Thu, 12 Jun 2014 23:40:54 +0200
Jeroen Hofstee wrote:
KBuild (ab)uses the asm statement to write to a file and
llvm integrated as chokes about these invalid asm statements.
Workaround it by making it look like valid asm code.
Signed-off-by: Jeroen Hofstee
I think Linux has the same problem.
Are you willing to this patch to linux-kbuild ML?
Or fixing U-Boot only?
I don't mind in general, but it is just noise for them (cc-ing them to
create some). For u-boot (ARM) you actually get a valid binary with
this patch after clang support has landed, for linux you just get other
errors as far as I tried (native only), patch below.
However in linux there seem more spots relying on the format, e.g.
arch/ia64/kvm/Makefile
arch/ia64/kernel/Makefile
arch/um/Makefile
So if anything, I think this should be made a general rules first
in the makefiles. It seems stupid to potentially break something
while it gains nothing.
So yes, u-boot only afaic, or does that make your syncing more difficult?
Regards,
Jeroen
--- a/Kbuild
+++ b/Kbuild
@@ -52,7 +52,8 @@ targets += arch/$(SRCARCH)/kernel/asm-offsets.s
# Default sed regexp - multiline due to syntax constraints
define sed-y
- "/^->/{s:->#\(.*\):/* \1 */:; \
+ "s:[[:space:]]*\.ascii[[:space:]]*\"\(.*\)\":\1:; \
+ /^->/{s:->#\(.*\):/* \1 */:; \
s:^->\([^ ]*\) [\$$#]*\([-0-9]*\) \(.*\):#define \1 \2 /* \3 */:; \
s:^->\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; \
s:->::; p;}"
diff --git a/include/linux/kbuild.h b/include/linux/kbuild.h
index 22a7219..4e80f3a 100644
--- a/include/linux/kbuild.h
+++ b/include/linux/kbuild.h
@@ -2,14 +2,14 @@
#define __LINUX_KBUILD_H
#define DEFINE(sym, val) \
-asm volatile("\n->" #sym " %0 " #val : : "i" (val))
+ asm volatile("\n.ascii \"->" #sym " %0 " #val "\"" : : "i" (val))
-#define BLANK() asm volatile("\n->" : : )
+#define BLANK() asm volatile("\n.ascii \"->\"" : : )
#define OFFSET(sym, str, mem) \
DEFINE(sym, offsetof(struct str, mem))
#define COMMENT(x) \
- asm volatile("\n->#" x)
+ asm volatile("\n.ascii \"->#" x "\"")
#endif
diff --git a/scripts/mod/Makefile b/scripts/mod/Makefile
index c11212f..0698af3 100644
--- a/scripts/mod/Makefile
+++ b/scripts/mod/Makefile
@@ -6,7 +6,8 @@ modpost-objs:= modpost.o file2alias.o sumversion.o
devicetable-offsets-file := devicetable-offsets.h
define sed-y
- "/^->/{s:->#\(.*\):/* \1 */:; \
+ "s:[[:space:]]*\.ascii[[:space:]]*\"\(.*\)\":\1:; \
+ /^->/{s:->#\(.*\):/* \1 */:; \
s:^->\([^ ]*\) [\$$#]*\([-0-9]*\) \(.*\):#define \1 \2 /* \3 */:; \
s:^->\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; \
s:->::; p;}"
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [U-Boot] [PATCH resend] kconfig: Fix compiler warning in menu.c
Hello Hans, On 12-10-14 12:25, Hans de Goede wrote: Hi, This one seems to have fallen through the cracks. Regards, Hans (for U-boot) nope, you replace an innocent warning (_might_ be) with bad code, without any comment it is just because gcc failed to recognize it is fine. Nor did you respond to the suggestion if it helps gcc to recognize that if the two booleans are merged into a single one. [or even split it in an if () if ()]. With this patch you prevent any serious warning in case the variable is actually used but not initialized, which is even worse if you ask me. Regards, Jeroen -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [U-Boot] [PATCH resend] kconfig: Fix compiler warning in menu.c
Hello Simon, On 13-10-14 07:14, Simon Glass wrote: Hi Jeroen, On 12 October 2014 10:13, Jeroen Hofstee wrote: Hello Hans, On 12-10-14 12:25, Hans de Goede wrote: Hi, This one seems to have fallen through the cracks. Regards, Hans (for U-boot) nope, you replace an innocent warning (_might_ be) with bad code, without any comment it is just because gcc failed to recognize it is fine. Nor did you respond to the suggestion if it helps gcc to recognize that if the two booleans are merged into a single one. [or even split it in an if () if ()]. With this patch you prevent any serious warning in case the variable is actually used but not initialized, which is even worse if you ask me. That is a pretty acerbic tone to take on the U-Boot list at least. Are you two drinking buddies or something? no, it is because we have discussed this patch before and resending it won't address the issue raised. But you are right, it is likely done with less evil intends then I took it for, so let me explain my concern again in a politer way. The problem is that gcc 4.9 starts warning in the following case: int *ptr; if (a) ptr = something; if (a && b) ptr->bla = value; else do_something_else(); it will warn that ptr _might_ be used uninitialized (but it always is). This is fixed in this patch by assigning NULL to ptr, and while that makes the warning go away it actually prevents the valid warning, ptr _is_ used uninitialized if you start using it in the else case. Hence my request if we can't find a better solution for this. Does anyone know a better solution for this or should we consider disabling the might be unused warning? Regards, Jeroen -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ARM: dts: am335x-sancloud-bbe: Fix PHY mode for ethernet
cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode") broke
the ethernet networking on the beaglebone enhanced.
The board relied on the bug in the at803x driver to always enable the rx
delay. So change the phy-mode to rgmii-id so it is enabled again.
Signed-off-by: Jeroen Hofstee
cc: Koen Kooi
---
arch/arm/boot/dts/am335x-sancloud-bbe.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/am335x-sancloud-bbe.dts
b/arch/arm/boot/dts/am335x-sancloud-bbe.dts
index 8678e6e35493..e5fdb7abb0d5 100644
--- a/arch/arm/boot/dts/am335x-sancloud-bbe.dts
+++ b/arch/arm/boot/dts/am335x-sancloud-bbe.dts
@@ -108,7 +108,7 @@
&cpsw_emac0 {
phy-handle = <ðphy0>;
- phy-mode = "rgmii-txid";
+ phy-mode = "rgmii-id";
};
&i2c0 {
--
2.17.1
Re: [PATCH] ARM: dts: am335x-sancloud-bbe: Fix PHY mode for ethernet
Hello Grygorri,
On 10/2/19 4:48 PM, Grygorii Strashko wrote:
>
>
> On 02/10/2019 12:54, Jeroen Hofstee wrote:
>> cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode")
>> broke
>> the ethernet networking on the beaglebone enhanced.
>
> Above commit is incorrect (by itself) and there are few more commits
> on top of
> it, so pls. update reference to commit(s)
>
> bb0ce4c1517d net: phy: at803x: stop switching phy delay config needlessly
> 6d4cd041f0af net: phy: at803x: disable delay only for RGMII mode
>
>
I don't see why that is relevant. The mention patch introduces a
backwards incompatibility for the device tree. The patches you
mention don't fix that and hence are unrelated to this patch.
Furthermore 4.19 is fine, so there is no need to include it in stable
and have a note to make sure also other patches are required etc.
Regards,
Jeroen
[PATCH v2 1/2] can: D_CAN: perform a sofware reset on open
When the C_CAN interface is closed it is put in power down mode, but
does not reset the error counters / state. So reset the D_CAN on open,
so the reported state and the actual state match.
According to [1], the C_CAN module doesn't have the software reset.
[1]
http://www.bosch-semiconductors.com/media/ip_modules/pdf_2/c_can_fd8/users_manual_c_can_fd8_r210_1.pdf
Signed-off-by: Jeroen Hofstee
---
drivers/net/can/c_can/c_can.c | 26 ++
1 file changed, 26 insertions(+)
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 606b7d8ffe13..2332687fa6dc 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -52,6 +52,7 @@
#define CONTROL_EX_PDR BIT(8)
/* control register */
+#define CONTROL_SWRBIT(15)
#define CONTROL_TEST BIT(7)
#define CONTROL_CCEBIT(6)
#define CONTROL_DISABLE_AR BIT(5)
@@ -569,6 +570,26 @@ static void c_can_configure_msg_objects(struct net_device
*dev)
IF_MCONT_RCV_EOB);
}
+static int c_can_software_reset(struct net_device *dev)
+{
+ struct c_can_priv *priv = netdev_priv(dev);
+ int retry = 0;
+
+ if (priv->type != BOSCH_D_CAN)
+ return 0;
+
+ priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_SWR | CONTROL_INIT);
+ while (priv->read_reg(priv, C_CAN_CTRL_REG) & CONTROL_SWR) {
+ msleep(20);
+ if (retry++ > 100) {
+ netdev_err(dev, "CCTRL: software reset failed\n");
+ return -EIO;
+ }
+ }
+
+ return 0;
+}
+
/*
* Configure C_CAN chip:
* - enable/disable auto-retransmission
@@ -578,6 +599,11 @@ static void c_can_configure_msg_objects(struct net_device
*dev)
static int c_can_chip_config(struct net_device *dev)
{
struct c_can_priv *priv = netdev_priv(dev);
+ int err;
+
+ err = c_can_software_reset(dev);
+ if (err)
+ return err;
/* enable automatic retransmission */
priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_ENABLE_AR);
--
2.17.1
[PATCH v2 2/2] can: C_CAN: add bus recovery events
While the state is updated when the error counters increase and decrease,
there is no event when the bus recovers and the error counters decrease
again. So add that event as well.
Change the state going downward to be ERROR_PASSIVE -> ERROR_WARNING ->
ERROR_ACTIVE instead of directly to ERROR_ACTIVE again.
Signed-off-by: Jeroen Hofstee
Acked-by: Kurt Van Dijck
Tested-by: Kurt Van Dijck
---
drivers/net/can/c_can/c_can.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 2332687fa6dc..e1e9b87dd4d2 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -912,6 +912,9 @@ static int c_can_handle_state_change(struct net_device *dev,
struct can_berr_counter bec;
switch (error_type) {
+ case C_CAN_NO_ERROR:
+ priv->can.state = CAN_STATE_ERROR_ACTIVE;
+ break;
case C_CAN_ERROR_WARNING:
/* error warning state */
priv->can.can_stats.error_warning++;
@@ -942,6 +945,13 @@ static int c_can_handle_state_change(struct net_device
*dev,
ERR_CNT_RP_SHIFT;
switch (error_type) {
+ case C_CAN_NO_ERROR:
+ /* error warning state */
+ cf->can_id |= CAN_ERR_CRTL;
+ cf->data[1] = CAN_ERR_CRTL_ACTIVE;
+ cf->data[6] = bec.txerr;
+ cf->data[7] = bec.rxerr;
+ break;
case C_CAN_ERROR_WARNING:
/* error warning state */
cf->can_id |= CAN_ERR_CRTL;
@@ -1080,11 +1090,17 @@ static int c_can_poll(struct napi_struct *napi, int
quota)
/* handle bus recovery events */
if ((!(curr & STATUS_BOFF)) && (last & STATUS_BOFF)) {
netdev_dbg(dev, "left bus off state\n");
- priv->can.state = CAN_STATE_ERROR_ACTIVE;
+ work_done += c_can_handle_state_change(dev,
C_CAN_ERROR_PASSIVE);
}
+
if ((!(curr & STATUS_EPASS)) && (last & STATUS_EPASS)) {
netdev_dbg(dev, "left error passive state\n");
- priv->can.state = CAN_STATE_ERROR_ACTIVE;
+ work_done += c_can_handle_state_change(dev,
C_CAN_ERROR_WARNING);
+ }
+
+ if ((!(curr & STATUS_EWARN)) && (last & STATUS_EWARN)) {
+ netdev_dbg(dev, "left error warning state\n");
+ work_done += c_can_handle_state_change(dev, C_CAN_NO_ERROR);
}
/* handle lec errors on the bus */
--
2.17.1
Re: [PATCH 1/2] can: D_CAN: perform a sofware reset on open
Hello Marc, On 10/1/19 4:32 PM, Marc Kleine-Budde wrote: > On 9/26/19 10:50 AM, Jeroen Hofstee wrote: >> When the C_CAN interface is closed it is put in power down mode, but >> does not reset the error counters / state. So reset the D_CAN on open, >> so the reported state and the actual state match. >> >> According to [1], the C_CAN module doesn't have the software reset. >> >> [1] >> http://www.bosch-semiconductors.com/media/ip_modules/pdf_2/c_can_fd8/users_manual_c_can_fd8_r210_1.pdf >> >> Signed-off-by: Jeroen Hofstee >> --- >> drivers/net/can/c_can/c_can.c | 26 ++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c >> index 606b7d8ffe13..502a181d02e7 100644 >> --- a/drivers/net/can/c_can/c_can.c >> +++ b/drivers/net/can/c_can/c_can.c >> @@ -52,6 +52,7 @@ >> #define CONTROL_EX_PDR BIT(8) >> >> /* control register */ >> +#define CONTROL_SWR BIT(15) >> #define CONTROL_TEST BIT(7) >> #define CONTROL_CCEBIT(6) >> #define CONTROL_DISABLE_AR BIT(5) >> @@ -569,6 +570,26 @@ static void c_can_configure_msg_objects(struct >> net_device *dev) >> IF_MCONT_RCV_EOB); >> } >> >> +static int software_reset(struct net_device *dev) > Please add the common prefix "c_can_" to the function Fine with me, I did sent a v2. Regards, Jeroen
Re: [PATCH] ARM: dts: am335x-sancloud-bbe: Fix PHY mode for ethernet
Hello Tony, On 10/8/19 4:23 PM, Tony Lindgren wrote: > * Grygorii Strashko [191003 02:32]: >> On 03/10/2019 11:16, Jeroen Hofstee wrote: >>> Furthermore 4.19 is fine, so there is no need to include it in stable >>> and have a note to make sure also other patches are required etc. >> Hence all above patches went in 5.1 it would be correct to mention only >> 6d4cd041f0af net: phy: at803x: disable delay only for RGMII mode > Jeroen, can you please send an updated patch with the fixes > tag changed? Not at the moment. I am right that the mentioned commit is the first one to break the ethernet. Grygorii is right it seems that that commit shouldn't affect it, yet it does. So I would like to understand how it breaks things so I can up with a sensible commit message (or we just drop reference to other commits so I don't have to dig through the 5.1 history, the patch by itself is also valid). Regards, Jeroen
Re: [PATCH] ARM: dts: am335x-sancloud-bbe: Fix PHY mode for ethernet
Hello Tony, On 10/8/19 4:23 PM, Tony Lindgren wrote: > * Grygorii Strashko [191003 02:32]: >> On 03/10/2019 11:16, Jeroen Hofstee wrote: >>> Furthermore 4.19 is fine, so there is no need to include it in stable >>> and have a note to make sure also other patches are required etc. >> Hence all above patches went in 5.1 it would be correct to mention only >> 6d4cd041f0af net: phy: at803x: disable delay only for RGMII mode > Jeroen, can you please send an updated patch with the fixes > tag changed? > For completeness, there is no "Fixes tag" as you mentioned. The commit only refers to another commit which introduces a problem. Regards, Jeroen
Re: [PATCH] ARM: dts: am335x-sancloud-bbe: Fix PHY mode for ethernet
Hello Tony, On 10/8/19 6:14 PM, Tony Lindgren wrote: > * Jeroen Hofstee [191008 16:03]: >> Hello Tony, >> >> On 10/8/19 4:23 PM, Tony Lindgren wrote: >>> * Grygorii Strashko [191003 02:32]: >>>> On 03/10/2019 11:16, Jeroen Hofstee wrote: >>>>> Furthermore 4.19 is fine, so there is no need to include it in stable >>>>> and have a note to make sure also other patches are required etc. >>>> Hence all above patches went in 5.1 it would be correct to mention only >>>> 6d4cd041f0af net: phy: at803x: disable delay only for RGMII mode >>> Jeroen, can you please send an updated patch with the fixes >>> tag changed? >>> >> For completeness, there is no "Fixes tag" as you mentioned. >> The commit only refers to another commit which introduces >> a problem. > Well please add the fixes tag, that way this will get > properly applied to earlier stable kernels too :) But 4.19 is fine, this is an issue in 5.1 as in EOL... I really don't understand why I should waste time to figure out what happened exactly during the 5.1 release cycle... Regards, Jeroen
Re: [PATCH] ARM: dts: am335x-sancloud-bbe: Fix PHY mode for ethernet
Hi,
On 10/8/19 6:51 PM, Tony Lindgren wrote:
> * Jeroen Hofstee [191008 16:43]:
>> Hello Tony,
>>
>> On 10/8/19 6:14 PM, Tony Lindgren wrote:
>>> * Jeroen Hofstee [191008 16:03]:
>>>> Hello Tony,
>>>>
>>>> On 10/8/19 4:23 PM, Tony Lindgren wrote:
>>>>> * Grygorii Strashko [191003 02:32]:
>>>>>> On 03/10/2019 11:16, Jeroen Hofstee wrote:
>>>>>>> Furthermore 4.19 is fine, so there is no need to include it in stable
>>>>>>> and have a note to make sure also other patches are required etc.
>>>>>> Hence all above patches went in 5.1 it would be correct to mention only
>>>>>> 6d4cd041f0af net: phy: at803x: disable delay only for RGMII mode
>>>>> Jeroen, can you please send an updated patch with the fixes
>>>>> tag changed?
>>>>>
>>>> For completeness, there is no "Fixes tag" as you mentioned.
>>>> The commit only refers to another commit which introduces
>>>> a problem.
>>> Well please add the fixes tag, that way this will get
>>> properly applied to earlier stable kernels too :)
>> But 4.19 is fine, this is an issue in 5.1 as in EOL...
>> I really don't understand why I should waste time
>> to figure out what happened exactly during the 5.1
>> release cycle...
> Hmm so what's the issue with just adding the fixes tag Grygorii
> suggested:
>
> 6d4cd041f0af ("net: phy: at803x: disable delay only for RGMII mode")
>
> No need to dig further?
Grygorii doesn't suggest to add a fixes tag, just to change the referenced
commit to another. Obviously I would like to understand why another commit
should be referenced. And then I should read and parse the response, so there
is no special reason, just time...
Regards,
Jeroen
Re: [PATCH] ARM: dts: am335x-sancloud-bbe: Fix PHY mode for ethernet
Hello Grygorri,
On 10/9/19 11:43 AM, Grygorii Strashko wrote:
>
>
>>> Grygorii doesn't suggest to add a fixes tag, just to change the
>>> referenced
>>> commit to another. Obviously I would like to understand why another
>>> commit
>>> should be referenced. And then I should read and parse the response,
>>> so there
>>> is no special reason, just time...
>>
>> OK sure. Well once you guys have the commit figured out, let me
>> know what to apply. And we know Grygorii is mostly right based
>> on his history of comments so best to not ignore that :)
>
> Sry, but I do not think my request is somehow special.
> Yes, your patch is correct by itself, but commit description is not:
> 1) commit cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for
> RGMII mode") which you've mentioned is A BUG
> and should not be merged first of all (which you can find out by
> reading corresponding thread).
>
> just try checkout that commit and apply your patch on top - networking
> should not work.
>
> But it was merged and not reverted - instead two more patches were
> applied to fix regression.
>
> 2) Those commits are defined final behavior (which i again explained
> above) and that new behavior hardly can
> be called "the bug in the at803x driver" as, unfortunately, there were
> no common conclusion how default values for
> RX/TX delay should be handled vs phy-mode = "rgmii-txid"/"rgmii-rxid".
> Originally many PHY driver kept them default (as per boot
> strapping/bootloader configuration), but now
> some driver (including at803x) started disabling RX delay if
> "rgmii-txid" or TX delay if "rgmii-rxid".
>
> Hence, pls update commit message and add proper fixes tag. smth like:
> "Now after commit 6d4cd041f0af net: phy: at803x: disable delay only
> for RGMII mode the driver will forcibly disable
> RGMII RX delay if phy-mode = "rgmii-txid" is specified in DT which
> will break networking on ..
>
> Hence change .. to ensure ...
> Fixes: "
>
>
Yes, that part is clear to me, and I am not saying you are incorrect,
only that I would like to understand why above commit pops up
(since it makes no sense to me as well, my own commit). And the reason
is rather silly, I guess... I fixed it on master, thereafter checked the
5.1 branch,
while keeping the fixed dtb... :(
Let me check that and I will send a v2. Likely tomorrow
(I am not near the device at the moment).
Regards,
Jeroen
[PATCH 1/7] can: rx-offload: continue on error
While can_rx_offload_offload_one will call mailbox_read to discard the mailbox in case of an error, can_rx_offload_irq_offload_timestamp bails out in the error case. Since it is typically called from a while loop, all message will eventually be discarded. So lets continue on error instead to discard them directly. Signed-off-by: Jeroen Hofstee --- drivers/net/can/rx-offload.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/can/rx-offload.c b/drivers/net/can/rx-offload.c index e6a668ee7730..39df41280e2d 100644 --- a/drivers/net/can/rx-offload.c +++ b/drivers/net/can/rx-offload.c @@ -158,7 +158,7 @@ int can_rx_offload_irq_offload_timestamp(struct can_rx_offload *offload, u64 pen skb = can_rx_offload_offload_one(offload, i); if (!skb) - break; + continue; __skb_queue_add_sort(&skb_queue, skb, can_rx_offload_compare); } -- 2.17.1
[PATCH 2/7] can: ti_hecc: release the mailbox a bit earlier
Release the mailbox after reading it, so it can be reused a bit earlier.
Since "can: rx-offload: continue on error" all pending message bits are
cleared directly, so remove clearing them in ti_hecc.
Suggested-by: Marc Kleine-Budde
Signed-off-by: Jeroen Hofstee
---
drivers/net/can/ti_hecc.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index f8b19eef5d26..461c28ab6d66 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -526,8 +526,9 @@ static unsigned int ti_hecc_mailbox_read(struct
can_rx_offload *offload,
u32 *timestamp, unsigned int mbxno)
{
struct ti_hecc_priv *priv = rx_offload_to_priv(offload);
- u32 data;
+ u32 data, mbx_mask;
+ mbx_mask = BIT(mbxno);
data = hecc_read_mbx(priv, mbxno, HECC_CANMID);
if (data & HECC_CANMID_IDE)
cf->can_id = (data & CAN_EFF_MASK) | CAN_EFF_FLAG;
@@ -547,6 +548,7 @@ static unsigned int ti_hecc_mailbox_read(struct
can_rx_offload *offload,
}
*timestamp = hecc_read_stamp(priv, mbxno);
+ hecc_write(priv, HECC_CANRMP, mbx_mask);
return 1;
}
@@ -695,7 +697,6 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
while ((rx_pending = hecc_read(priv, HECC_CANRMP))) {
can_rx_offload_irq_offload_timestamp(&priv->offload,
rx_pending);
- hecc_write(priv, HECC_CANRMP, rx_pending);
}
}
--
2.17.1
[PATCH 3/7] can: ti_hecc: stop the CPK on down
When the interface goes down, the CPK should no longer take an active
part in the CAN-bus communication, like sending acks and error frames.
So enable configuration mode in ti_hecc_stop, so the CPK is no longer
active.
When a transceiver switch is present the acks and errors don't make it
to the bus, but disabling the CPK then does prevent oddities, like
ti_hecc_reset failing, since the CPK can become bus-off and starts
counting the 11 bit recessive bits, which seems to block the reset. It
can also cause invalid interrupts and disrupt the CAN-bus, since
transmission can be stopped in the middle of a message, by disabling
the tranceiver while the CPK is sending.
Since the CPK is disabled after normal power on, it is typically only
seen when the interface is restarted.
Signed-off-by: Jeroen Hofstee
---
drivers/net/can/ti_hecc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index 461c28ab6d66..b82c011ddbec 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -400,6 +400,9 @@ static void ti_hecc_stop(struct net_device *ndev)
{
struct ti_hecc_priv *priv = netdev_priv(ndev);
+ /* Disable the CPK; stop sending, erroring and acking */
+ hecc_set_bit(priv, HECC_CANMC, HECC_CANMC_CCR);
+
/* Disable interrupts and disable mailboxes */
hecc_write(priv, HECC_CANGIM, 0);
hecc_write(priv, HECC_CANMIM, 0);
--
2.17.1
[PATCH 5/7] can: ti_hecc: add fifo underflow error reporting
When the rx fifo overflows the ti_hecc would silently drop them since
the overwrite protection is enabled for all mailboxes. So disable it
for the lowest priority mailbox and increment the rx_fifo_errors when
receive message lost is set. Drop the message itself in that case,
since it might be partially updated.
Signed-off-by: Jeroen Hofstee
---
drivers/net/can/ti_hecc.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index 35c82289f2a3..4206ad5cb666 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -82,7 +82,7 @@ MODULE_VERSION(HECC_MODULE_VERSION);
#define HECC_CANTA 0x10/* Transmission acknowledge */
#define HECC_CANAA 0x14/* Abort acknowledge */
#define HECC_CANRMP0x18/* Receive message pending */
-#define HECC_CANRML0x1C/* Remote message lost */
+#define HECC_CANRML0x1C/* Receive message lost */
#define HECC_CANRFP0x20/* Remote frame pending */
#define HECC_CANGAM0x24/* SECC only:Global acceptance mask */
#define HECC_CANMC 0x28/* Master control */
@@ -385,8 +385,17 @@ static void ti_hecc_start(struct net_device *ndev)
/* Enable tx interrupts */
hecc_set_bit(priv, HECC_CANMIM, BIT(HECC_MAX_TX_MBOX) - 1);
- /* Prevent message over-write & Enable interrupts */
- hecc_write(priv, HECC_CANOPC, HECC_SET_REG);
+ /* Prevent message over-write to create a rx fifo, but not for the
+* lowest priority mailbox, since that allows detecting overflows
+* instead of the hardware silently dropping the messages. The lowest
+* rx mailbox is one above the tx ones, hence its mbxno is the number
+* of tx mailboxes.
+*/
+ mbxno = HECC_MAX_TX_MBOX;
+ mbx_mask = ~BIT(mbxno);
+ hecc_write(priv, HECC_CANOPC, mbx_mask);
+
+ /* Enable interrupts */
if (priv->use_hecc1int) {
hecc_write(priv, HECC_CANMIL, HECC_SET_REG);
hecc_write(priv, HECC_CANGIM, HECC_CANGIM_DEF_MASK |
@@ -531,6 +540,7 @@ static unsigned int ti_hecc_mailbox_read(struct
can_rx_offload *offload,
{
struct ti_hecc_priv *priv = rx_offload_to_priv(offload);
u32 data, mbx_mask;
+ int lost;
mbx_mask = BIT(mbxno);
data = hecc_read_mbx(priv, mbxno, HECC_CANMID);
@@ -552,9 +562,12 @@ static unsigned int ti_hecc_mailbox_read(struct
can_rx_offload *offload,
}
*timestamp = hecc_read_stamp(priv, mbxno);
+ lost = hecc_read(priv, HECC_CANRML) & mbx_mask;
+ if (unlikely(lost))
+ priv->offload.dev->stats.rx_fifo_errors++;
hecc_write(priv, HECC_CANRMP, mbx_mask);
- return 1;
+ return !lost;
}
static int ti_hecc_error(struct net_device *ndev, int int_status,
--
2.17.1
[PATCH 6/7] can: ti_hecc: properly report state changes
The HECC_CANES register handles the flags specially, it only updates
the flags after a one is written to them. Since the interrupt for
frame errors is not enabled an old error can hence been seen when a
state interrupt arrives. For example if the device is not connected
to the CAN-bus the error warning interrupt will have HECC_CANES
indicating there is no ack. The error passive interrupt thereafter
will have HECC_CANES flagging that there is a warning level. And if
thereafter there is a message successfully send HECC_CANES points to
an error passive event, while in reality it became error warning
again. In summary, the state is not always reported correctly.
So handle the state changes and frame errors separately. The state
changes are now based on the interrupt flags and handled directly
when they occur. The reporting of the frame errors is still done as
before, as a side effect of another interrupt.
note: the hecc_clear_bit will do a read, modify, write. So it will
not only clear the bit, but also reset all other bits being set as
a side affect, hence it is replaced with only clearing the flags.
note: The HECC_CANMC_CCR is no longer cleared in the state change
interrupt, it is completely unrelated.
And use net_ratelimit to make checkpatch happy.
Signed-off-by: Jeroen Hofstee
---
drivers/net/can/ti_hecc.c | 156 --
1 file changed, 82 insertions(+), 74 deletions(-)
diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index 4206ad5cb666..6098725bfdea 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -149,6 +149,8 @@ MODULE_VERSION(HECC_MODULE_VERSION);
#define HECC_BUS_ERROR (HECC_CANES_FE | HECC_CANES_BE |\
HECC_CANES_CRCE | HECC_CANES_SE |\
HECC_CANES_ACKE)
+#define HECC_CANES_FLAGS (HECC_BUS_ERROR | HECC_CANES_BO |\
+ HECC_CANES_EP | HECC_CANES_EW)
#define HECC_CANMCF_RTRBIT(4) /* Remote transmit request */
@@ -578,91 +580,63 @@ static int ti_hecc_error(struct net_device *ndev, int
int_status,
struct sk_buff *skb;
u32 timestamp;
- /* propagate the error condition to the can stack */
- skb = alloc_can_err_skb(ndev, &cf);
- if (!skb) {
- if (printk_ratelimit())
- netdev_err(priv->ndev,
- "%s: alloc_can_err_skb() failed\n",
- __func__);
- return -ENOMEM;
- }
-
- if (int_status & HECC_CANGIF_WLIF) { /* warning level int */
- if ((int_status & HECC_CANGIF_BOIF) == 0) {
- priv->can.state = CAN_STATE_ERROR_WARNING;
- ++priv->can.can_stats.error_warning;
- cf->can_id |= CAN_ERR_CRTL;
- if (hecc_read(priv, HECC_CANTEC) > 96)
- cf->data[1] |= CAN_ERR_CRTL_TX_WARNING;
- if (hecc_read(priv, HECC_CANREC) > 96)
- cf->data[1] |= CAN_ERR_CRTL_RX_WARNING;
- }
- hecc_set_bit(priv, HECC_CANES, HECC_CANES_EW);
- netdev_dbg(priv->ndev, "Error Warning interrupt\n");
- hecc_clear_bit(priv, HECC_CANMC, HECC_CANMC_CCR);
- }
-
- if (int_status & HECC_CANGIF_EPIF) { /* error passive int */
- if ((int_status & HECC_CANGIF_BOIF) == 0) {
- priv->can.state = CAN_STATE_ERROR_PASSIVE;
- ++priv->can.can_stats.error_passive;
- cf->can_id |= CAN_ERR_CRTL;
- if (hecc_read(priv, HECC_CANTEC) > 127)
- cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
- if (hecc_read(priv, HECC_CANREC) > 127)
- cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
+ if (err_status & HECC_BUS_ERROR) {
+ /* propagate the error condition to the can stack */
+ skb = alloc_can_err_skb(ndev, &cf);
+ if (!skb) {
+ if (net_ratelimit())
+ netdev_err(priv->ndev,
+ "%s: alloc_can_err_skb() failed\n",
+ __func__);
+ return -ENOMEM;
}
- hecc_set_bit(priv, HECC_CANES, HECC_CANES_EP);
- netdev_dbg(priv->ndev, "Error passive interrupt\n");
- hecc_clear_bit(priv, HECC_CANMC, HECC_CANMC_CCR);
- }
-
- /* Need to check busoff condition in error status register too to
-* ensure warning interrupts don't hog the system
-*/
- if ((int_status & HECC_CANGIF_BOIF) |
[PATCH 7/7] can: ti_hecc: add missing state changes
While the ti_hecc has interrupts to report when the error counters increase
to a certain level and which change state it doesn't handle the case that
the error counters go down again, so the reported state can actually be
wrong. Since there is no interrupt for that, do update state based on the
error counters, when the state is not error active and goes down again.
Signed-off-by: Jeroen Hofstee
---
drivers/net/can/ti_hecc.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index 6098725bfdea..c7c866da9c6a 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -689,6 +689,23 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
can_bus_off(ndev);
change_state(priv, rx_state, tx_state);
}
+ } else if (unlikely(priv->can.state != CAN_STATE_ERROR_ACTIVE)) {
+ enum can_state new_state, tx_state, rx_state;
+ u32 rec = hecc_read(priv, HECC_CANREC);
+ u32 tec = hecc_read(priv, HECC_CANTEC);
+
+ if (rec >= 128 || tec >= 128)
+ new_state = CAN_STATE_ERROR_PASSIVE;
+ else if (rec >= 96 || tec >= 96)
+ new_state = CAN_STATE_ERROR_WARNING;
+ else
+ new_state = CAN_STATE_ERROR_ACTIVE;
+
+ if (new_state < priv->can.state) {
+ rx_state = rec >= tec ? new_state : 0;
+ tx_state = rec <= tec ? new_state : 0;
+ change_state(priv, rx_state, tx_state);
+ }
}
if (int_status & HECC_CANGIF_GMIF) {
--
2.17.1
[PATCH 4/7] can: ti_hecc: keep MIM and MD set
The HECC_CANMIM is set in the xmit path and cleared in the interrupt.
Since this is done with a read, modify, write action the register might
end up with some more MIM enabled then intended, since it is not
protected. That doesn't matter at all, since the tx interrupt disables
the mailbox with HECC_CANME (while holding a spinlock). So lets just
always keep MIM set.
While at it, since the mailbox direction never changes, don't set it
every time a message is send, ti_hecc_reset already sets them to tx.
Signed-off-by: Jeroen Hofstee
---
drivers/net/can/ti_hecc.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index b82c011ddbec..35c82289f2a3 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -382,6 +382,9 @@ static void ti_hecc_start(struct net_device *ndev)
hecc_set_bit(priv, HECC_CANMIM, mbx_mask);
}
+ /* Enable tx interrupts */
+ hecc_set_bit(priv, HECC_CANMIM, BIT(HECC_MAX_TX_MBOX) - 1);
+
/* Prevent message over-write & Enable interrupts */
hecc_write(priv, HECC_CANOPC, HECC_SET_REG);
if (priv->use_hecc1int) {
@@ -511,8 +514,6 @@ static netdev_tx_t ti_hecc_xmit(struct sk_buff *skb, struct
net_device *ndev)
hecc_set_bit(priv, HECC_CANME, mbx_mask);
spin_unlock_irqrestore(&priv->mbx_lock, flags);
- hecc_clear_bit(priv, HECC_CANMD, mbx_mask);
- hecc_set_bit(priv, HECC_CANMIM, mbx_mask);
hecc_write(priv, HECC_CANTRS, mbx_mask);
return NETDEV_TX_OK;
@@ -675,7 +676,6 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
mbx_mask = BIT(mbxno);
if (!(mbx_mask & hecc_read(priv, HECC_CANTA)))
break;
- hecc_clear_bit(priv, HECC_CANMIM, mbx_mask);
hecc_write(priv, HECC_CANTA, mbx_mask);
spin_lock_irqsave(&priv->mbx_lock, flags);
hecc_clear_bit(priv, HECC_CANME, mbx_mask);
--
2.17.1
[PATCH] can: peakcan: report bus recovery as well
While the state changes are reported when the error counters increase
and decrease, there is no event when the bus recovers and the error
counters decrease again. So add those as well.
Change the state going downward to be ERROR_PASSIVE -> ERROR_WARNING ->
ERROR_ACTIVE instead of directly to ERROR_ACTIVE again.
Signed-off-by: Jeroen Hofstee
Cc: Stephane Grosjean
---
drivers/net/can/usb/peak_usb/pcan_usb.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c
b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 617da295b6c1..dd2a7f529012 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -436,8 +436,8 @@ static int pcan_usb_decode_error(struct
pcan_usb_msg_context *mc, u8 n,
}
if ((n & PCAN_USB_ERROR_BUS_LIGHT) == 0) {
/* no error (back to active state) */
- mc->pdev->dev.can.state = CAN_STATE_ERROR_ACTIVE;
- return 0;
+ new_state = CAN_STATE_ERROR_ACTIVE;
+ break;
}
break;
@@ -460,9 +460,9 @@ static int pcan_usb_decode_error(struct
pcan_usb_msg_context *mc, u8 n,
}
if ((n & PCAN_USB_ERROR_BUS_HEAVY) == 0) {
- /* no error (back to active state) */
- mc->pdev->dev.can.state = CAN_STATE_ERROR_ACTIVE;
- return 0;
+ /* no error (back to warning state) */
+ new_state = CAN_STATE_ERROR_WARNING;
+ break;
}
break;
@@ -501,6 +501,11 @@ static int pcan_usb_decode_error(struct
pcan_usb_msg_context *mc, u8 n,
mc->pdev->dev.can.can_stats.error_warning++;
break;
+ case CAN_STATE_ERROR_ACTIVE:
+ cf->can_id |= CAN_ERR_CRTL;
+ cf->data[1] = CAN_ERR_CRTL_ACTIVE;
+ break;
+
default:
/* CAN_STATE_MAX (trick to handle other errors) */
cf->can_id |= CAN_ERR_CRTL;
--
2.17.1
[PATCH 1/2] can: D_CAN: perform a sofware reset on open
When the C_CAN interface is closed it is put in power down mode, but
does not reset the error counters / state. So reset the D_CAN on open,
so the reported state and the actual state match.
According to [1], the C_CAN module doesn't have the software reset.
[1]
http://www.bosch-semiconductors.com/media/ip_modules/pdf_2/c_can_fd8/users_manual_c_can_fd8_r210_1.pdf
Signed-off-by: Jeroen Hofstee
---
drivers/net/can/c_can/c_can.c | 26 ++
1 file changed, 26 insertions(+)
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 606b7d8ffe13..502a181d02e7 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -52,6 +52,7 @@
#define CONTROL_EX_PDR BIT(8)
/* control register */
+#define CONTROL_SWRBIT(15)
#define CONTROL_TEST BIT(7)
#define CONTROL_CCEBIT(6)
#define CONTROL_DISABLE_AR BIT(5)
@@ -569,6 +570,26 @@ static void c_can_configure_msg_objects(struct net_device
*dev)
IF_MCONT_RCV_EOB);
}
+static int software_reset(struct net_device *dev)
+{
+ struct c_can_priv *priv = netdev_priv(dev);
+ int retry = 0;
+
+ if (priv->type != BOSCH_D_CAN)
+ return 0;
+
+ priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_SWR | CONTROL_INIT);
+ while (priv->read_reg(priv, C_CAN_CTRL_REG) & CONTROL_SWR) {
+ msleep(20);
+ if (retry++ > 100) {
+ netdev_err(dev, "CCTRL: software reset failed\n");
+ return -EIO;
+ }
+ }
+
+ return 0;
+}
+
/*
* Configure C_CAN chip:
* - enable/disable auto-retransmission
@@ -578,6 +599,11 @@ static void c_can_configure_msg_objects(struct net_device
*dev)
static int c_can_chip_config(struct net_device *dev)
{
struct c_can_priv *priv = netdev_priv(dev);
+ int err;
+
+ err = software_reset(dev);
+ if (err)
+ return err;
/* enable automatic retransmission */
priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_ENABLE_AR);
--
2.17.1
[PATCH 2/2] can: C_CAN: add bus recovery events
While the state is update when the error counters increase and decrease,
there is no event when the bus recovers and the error counters decrease
again. So add that event as well.
Change the state going downward to be ERROR_PASSIVE -> ERROR_WARNING ->
ERROR_ACTIVE instead of directly to ERROR_ACTIVE again.
Signed-off-by: Jeroen Hofstee
---
drivers/net/can/c_can/c_can.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 502a181d02e7..5cfaab18e10b 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -912,6 +912,9 @@ static int c_can_handle_state_change(struct net_device *dev,
struct can_berr_counter bec;
switch (error_type) {
+ case C_CAN_NO_ERROR:
+ priv->can.state = CAN_STATE_ERROR_ACTIVE;
+ break;
case C_CAN_ERROR_WARNING:
/* error warning state */
priv->can.can_stats.error_warning++;
@@ -942,6 +945,13 @@ static int c_can_handle_state_change(struct net_device
*dev,
ERR_CNT_RP_SHIFT;
switch (error_type) {
+ case C_CAN_NO_ERROR:
+ /* error warning state */
+ cf->can_id |= CAN_ERR_CRTL;
+ cf->data[1] = CAN_ERR_CRTL_ACTIVE;
+ cf->data[6] = bec.txerr;
+ cf->data[7] = bec.rxerr;
+ break;
case C_CAN_ERROR_WARNING:
/* error warning state */
cf->can_id |= CAN_ERR_CRTL;
@@ -1080,11 +1090,17 @@ static int c_can_poll(struct napi_struct *napi, int
quota)
/* handle bus recovery events */
if ((!(curr & STATUS_BOFF)) && (last & STATUS_BOFF)) {
netdev_dbg(dev, "left bus off state\n");
- priv->can.state = CAN_STATE_ERROR_ACTIVE;
+ work_done += c_can_handle_state_change(dev,
C_CAN_ERROR_PASSIVE);
}
+
if ((!(curr & STATUS_EPASS)) && (last & STATUS_EPASS)) {
netdev_dbg(dev, "left error passive state\n");
- priv->can.state = CAN_STATE_ERROR_ACTIVE;
+ work_done += c_can_handle_state_change(dev,
C_CAN_ERROR_WARNING);
+ }
+
+ if ((!(curr & STATUS_EWARN)) && (last & STATUS_EWARN)) {
+ netdev_dbg(dev, "left error warning state\n");
+ work_done += c_can_handle_state_change(dev, C_CAN_NO_ERROR);
}
/* handle lec errors on the bus */
--
2.17.1
Re: [PATCH 1/7] can: rx-offload: continue on error
Hello Marc,
On 10/9/19 3:18 PM, Marc Kleine-Budde wrote:
> Hello Jeroen,
>
> I'm currently looking at the rx-offload error handling in detail.
>
> TLDR: I've added the patch to linux-can.
>
> Thanks,
> Marc
>
> For the record, the details:
>
> On 9/24/19 8:45 PM, Jeroen Hofstee wrote:
>> While can_rx_offload_offload_one will call mailbox_read to discard
>> the mailbox in case of an error,
> Yes.
>
> can_rx_offload_offload_one() will read into the discard mailbox in case
> of an error.
>
> Currently there are two kinds of errors:
> 1) the rx-offoad skb queue (between the IRQ handler and the NAPI)
> is full
> 2) alloc_can_skb() fails to allocate a skb, due to OOM
>
>> can_rx_offload_irq_offload_timestamp bails out in the error case.
> Yes, in case of an error both can_rx_offload_irq_offload_timestamp() and
> can_rx_offload_irq_offload_fifo() will stop reading mailboxes, add the
> already filled skbs to the queue and schedule NAPI if needed.
>
> Currently both can_rx_offload_irq_offload_timestamp() and
> can_rx_offload_irq_offload_fifo() will return the number of queued
> mailboxes.
>
> This means in case of queue overflow or OOM, only one mailbox is
> discaded, before can_rx_offload_irq_offload_*() returning the number of
> successfully queued mailboxes so far.
>
> Looking at the flexcan driver:
>
> https://elixir.bootlin.com/linux/latest/source/drivers/net/can/flexcan.c#L867
>
>> while ((reg_iflag = flexcan_read_reg_iflag_rx(priv))) {
>> handled = IRQ_HANDLED;
>> ret =
>> can_rx_offload_irq_offload_timestamp(&priv->offload,
>> reg_iflag);
>> if (!ret)
>> break;
>> }
> [...]
>> if (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE) {
>> handled = IRQ_HANDLED;
>> can_rx_offload_irq_offload_fifo(&priv->offload);
>> }
> This means for the timestamp mode, at least one mailbox is discarded or
> if the error occurred after reading one or more mailboxes the while loop
> will try again. If the error persists a second mailbox is discarded.
>
> For the fifo mode, only one mailbox is discarded.
>
> Then the flexcan's IRQ is exited. If there are messages in mailboxes are
> pending another IRQ is triggered I doubt that this is a good idea.
>
> On the other hand the ti_hecc driver:
>
>> /* offload RX mailboxes and let NAPI deliver them */
>> while ((rx_pending = hecc_read(priv, HECC_CANRMP))) {
>> can_rx_offload_irq_offload_timestamp(&priv->offload,
>> rx_pending);
>> hecc_write(priv, HECC_CANRMP, rx_pending);
>> }
> The error is ignored and the _all_ mailboxes are discarded (given the
> error persists).
>
>> Since it is typically called from a while loop, all message will
>> eventually be discarded. So lets continue on error instead to discard
>> them directly.
> After reading my own code and writing it up, your patch totally makes sense.
>
> If there is a shortage of resources, queue overrun or OOM, it makes no
> sense to return from the IRQ handler, if a mailbox is still active as it
> will trigger the IRQ again. Entering the IRQ handler again probably
> doesn't give the system time to recover from the resource problem.
Indeed, I have nothing to comment on that, besides thanks for
being willing to reconsider your own code.
With kind regards,
Jeroen
Re: [PATCH] can: m_can: fix boolreturn.cocci warnings
Hi,
On 10/15/19 7:57 AM, Simon Horman wrote:
> On Mon, Oct 14, 2019 at 11:04:28PM +0800, kbuild test robot wrote:
>> From: kbuild test robot
>>
>> drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in function
>> 'is_protocol_err' with return type bool
>>
>> Return statements in functions returning bool should use
>> true/false instead of 1/0.
>> Generated by: scripts/coccinelle/misc/boolreturn.cocci
>>
>> Fixes: 46946163ac61 ("can: m_can: add support for handling arbitration
>> error")
>> CC: Pankaj Sharma
>> Signed-off-by: kbuild test robot
>> ---
>>
>> url:
>> https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532
>>
>> m_can.c |4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -780,9 +780,9 @@ static inline bool is_lec_err(u32 psr)
>> static inline bool is_protocol_err(u32 irqstatus)
>> {
>> if (irqstatus & IR_ERR_LEC_31X)
>> -return 1;
>> +return true;
>> else
>> -return 0;
>> +return false;
>> }
>>
>> static int m_can_handle_protocol_error(struct net_device *dev, u32
>> irqstatus)
>>
> <2c>
> Perhaps the following is a nicer way to express this (completely untested):
>
> return !!(irqstatus & IR_ERR_LEC_31X);
>
Really, !! for bool / _Bool types? why not simply:
static inline bool is_protocol_err(u32 irqstatus)
return irqstatus & IR_ERR_LEC_31X;
}
Regards,
Jeroen
Re: [PATCH] can: m_can: fix boolreturn.cocci warnings
Hello Simon,
On 10/15/19 9:13 AM, Simon Horman wrote:
> On Tue, Oct 15, 2019 at 06:37:54AM +0000, Jeroen Hofstee wrote:
>> Hi,
>>
>> On 10/15/19 7:57 AM, Simon Horman wrote:
>>> On Mon, Oct 14, 2019 at 11:04:28PM +0800, kbuild test robot wrote:
>>>> From: kbuild test robot
>>>>
>>>> drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in function
>>>> 'is_protocol_err' with return type bool
>>>>
>>>>Return statements in functions returning bool should use
>>>>true/false instead of 1/0.
>>>> Generated by: scripts/coccinelle/misc/boolreturn.cocci
>>>>
>>>> Fixes: 46946163ac61 ("can: m_can: add support for handling arbitration
>>>> error")
>>>> CC: Pankaj Sharma
>>>> Signed-off-by: kbuild test robot
>>>> ---
>>>>
>>>> url:
>>>> https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532
>>>>
>>>>m_can.c |4 ++--
>>>>1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> --- a/drivers/net/can/m_can/m_can.c
>>>> +++ b/drivers/net/can/m_can/m_can.c
>>>> @@ -780,9 +780,9 @@ static inline bool is_lec_err(u32 psr)
>>>>static inline bool is_protocol_err(u32 irqstatus)
>>>>{
>>>>if (irqstatus & IR_ERR_LEC_31X)
>>>> - return 1;
>>>> + return true;
>>>>else
>>>> - return 0;
>>>> + return false;
>>>>}
>>>>
>>>>static int m_can_handle_protocol_error(struct net_device *dev, u32
>>>> irqstatus)
>>>>
>>> <2c>
>>> Perhaps the following is a nicer way to express this (completely untested):
>>>
>>> return !!(irqstatus & IR_ERR_LEC_31X);
>>>
>>
>> Really, !! for bool / _Bool types? why not simply:
>>
>> static inline bool is_protocol_err(u32 irqstatus)
>> return irqstatus & IR_ERR_LEC_31X;
>> }
> Good point, silly me.
For clarity, I am commenting on the suggestion made by
the tool, not the patch itself..
Regards,
Jeroen
Re: [PATCH] can: ti_hecc: use timestamp based rx-offloading
Hello Marc,
On 7/26/19 1:48 PM, Marc Kleine-Budde wrote:
> On 4/29/19 2:03 PM, Jeroen Hofstee wrote:
>
>> @@ -744,8 +652,8 @@ static irqreturn_t ti_hecc_interrupt(int irq, void
>> *dev_id)
>> struct net_device *ndev = (struct net_device *)dev_id;
>> struct ti_hecc_priv *priv = netdev_priv(ndev);
>> struct net_device_stats *stats = &ndev->stats;
>> -u32 mbxno, mbx_mask, int_status, err_status;
>> -unsigned long ack, flags;
>> +u32 mbxno, mbx_mask, int_status, err_status, stamp;
>> +unsigned long flags, rx_pending;
>>
>> int_status = hecc_read(priv,
>> (priv->use_hecc1int) ? HECC_CANGIF1 : HECC_CANGIF0);
>> @@ -769,11 +677,11 @@ static irqreturn_t ti_hecc_interrupt(int irq, void
>> *dev_id)
>> spin_lock_irqsave(&priv->mbx_lock, flags);
>> hecc_clear_bit(priv, HECC_CANME, mbx_mask);
>> spin_unlock_irqrestore(&priv->mbx_lock, flags);
>> -stats->tx_bytes += hecc_read_mbx(priv, mbxno,
>> -HECC_CANMCF) & 0xF;
>> +stamp = hecc_read_stamp(priv, mbxno);
>> +stats->tx_bytes +=
>> can_rx_offload_get_echo_skb(&priv->offload,
>> +mbxno,
>> stamp);
>> stats->tx_packets++;
>> can_led_event(ndev, CAN_LED_EVENT_TX);
>> -can_get_echo_skb(ndev, mbxno);
>> --priv->tx_tail;
>> }
>>
>> @@ -784,12 +692,11 @@ static irqreturn_t ti_hecc_interrupt(int irq, void
>> *dev_id)
>> ((priv->tx_head & HECC_TX_MASK) == HECC_TX_MASK)))
>> netif_wake_queue(ndev);
>>
>> -/* Disable RX mailbox interrupts and let NAPI reenable them */
>> -if (hecc_read(priv, HECC_CANRMP)) {
>> -ack = hecc_read(priv, HECC_CANMIM);
>> -ack &= BIT(HECC_MAX_TX_MBOX) - 1;
>> -hecc_write(priv, HECC_CANMIM, ack);
>> -napi_schedule(&priv->napi);
>> +/* offload RX mailboxes and let NAPI deliver them */
>> +while ((rx_pending = hecc_read(priv, HECC_CANRMP))) {
>> +can_rx_offload_irq_offload_timestamp(&priv->offload,
>> + rx_pending);
>> +hecc_write(priv, HECC_CANRMP, rx_pending);
> Can prepare a patch to move the RMP writing into the mailbox_read()
> function. This makes the mailbox available a bit earlier and works
> better for memory pressure situations, where no skb can be allocated.
For my understanding, is there any other reason for alloc_can_skb,
to fail, besides being out of memory. I couldn't easily find an other
limit enforced on it.
If it is actually _moved_, as you suggested, it does loose the ability to
handle the newly received messages while the messages are read
in the same interrupt, so it needs to interrupt again. That will work,
but seems a bit a silly thing to do.
Perhaps we can do both? Mark the mailbox as available in
mailbox_read, so it is available as soon as possible and clear
them all in the irq (under the assumption that alloc_can_skb
failure means real big trouble, why would we want to keep the
old messages around and eventually ignore the new messages?).
Another question, not related to this patch, but this driver..
Most of the times the irq handles 1 or sometimes 2 messages.
Do you happen to know if it is possible to optionally delay the irq
a bit in the millisecond range, so more work can be done in a single
interrupt? Since there are now 28 rx hardware mailboxes available,
it shouldn't run out easily.
And as last, I guess you want a patch which applies to
linux-can-next/testing, right?
With kind regards,
Jeroen
Re: [PATCH] can: ti_hecc: use timestamp based rx-offloading
Hello Marc, On 7/24/19 10:26 AM, Marc Kleine-Budde wrote: > On 4/29/19 2:03 PM, Jeroen Hofstee wrote: >> As already mentioned in [1] and included in [2], there is an off by one >> issue since the high bank is already enabled when the _next_ mailbox to >> be read has index 12, so the mailbox being read was 13. The message can >> therefore go into mailbox 31 and the driver will be repolled until the >> mailbox 12 eventually receives a msg. Or the message might end up in the >> 12th mailbox, but then it would become disabled after reading it and only >> be enabled again in the next "round" after mailbox 13 was read, which can >> cause out of order messages, since the lower priority mailboxes can >> accept messages in the meantime. >> >> As mentioned in [3] there is a hardware race condition when changing the >> CANME register while messages are being received. Even when including a >> busy poll on reception, like in [2] there are still overflows and out of >> order messages at times, but less then without the busy loop polling. >> Unlike what the patch suggests, the polling time is not in the microsecond >> range, but takes as long as a current CAN bus reception needs to finish, >> so typically more in the fraction of millisecond range. Since the timeout >> is in jiffies it won't timeout. >> >> Even with these additional fixes the driver is still not able to provide a >> proper FIFO which doesn't drop packages. So change the driver to use >> rx-offload and base order on timestamp instead of message box numbers. As >> a side affect, this also fixes [4] and [5]. >> >> Before this change messages with a single byte counter were dropped / >> received out of order at a bitrate of 250kbit/s on an am3517. With this >> patch that no longer occurs up to and including 1Mbit/s. >> >> [1] >> https://linux-can.vger.kernel.narkive.com/zgO9inVi/patch-can-ti-hecc-fix-rx-wrong-sequence-issue#post6 >> [2] >> http://arago-project.org/git/projects/?p=linux-omap3.git;a=commit;h=02346892777f07245de4d5af692513ebd852dcb2 >> [3] >> https://linux-can.vger.kernel.narkive.com/zgO9inVi/patch-can-ti-hecc-fix-rx-wrong-sequence-issue#post5 >> [4] https://patchwork.ozlabs.org/patch/895956/ >> [5] https://www.spinics.net/lists/netdev/msg494971.html >> >> Cc: Anant Gole >> Cc: AnilKumar Ch >> Signed-off-by: Jeroen Hofstee >> --- >> drivers/net/can/ti_hecc.c | 189 >> +- >> 1 file changed, 53 insertions(+), 136 deletions(-) >> >> diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c >> index db6ea93..fe7 100644 >> --- a/drivers/net/can/ti_hecc.c >> +++ b/drivers/net/can/ti_hecc.c >> @@ -5,6 +5,7 @@ >>* specs for the same is available at <http://www.ti.com> >>* >>* Copyright (C) 2009 Texas Instruments Incorporated - http://www.ti.com/ >> + * Copyright (C) 2019 Jeroen Hofstee >>* >>* This program is free software; you can redistribute it and/or >>* modify it under the terms of the GNU General Public License as >> @@ -34,6 +35,7 @@ >> #include >> #include >> #include >> +#include >> >> #define DRV_NAME "ti_hecc" >> #define HECC_MODULE_VERSION "0.7" >> @@ -63,29 +65,16 @@ MODULE_VERSION(HECC_MODULE_VERSION); >> #define HECC_TX_PRIO_MASK (MAX_TX_PRIO << HECC_MB_TX_SHIFT) >> #define HECC_TX_MB_MASK(HECC_MAX_TX_MBOX - 1) >> #define HECC_TX_MASK ((HECC_MAX_TX_MBOX - 1) | >> HECC_TX_PRIO_MASK) >> -#define HECC_TX_MBOX_MASK (~(BIT(HECC_MAX_TX_MBOX) - 1)) >> -#define HECC_DEF_NAPI_WEIGHTHECC_MAX_RX_MBOX >> >> /* >> - * Important Note: RX mailbox configuration >> - * RX mailboxes are further logically split into two - main and buffer >> - * mailboxes. The goal is to get all packets into main mailboxes as >> - * driven by mailbox number and receive priority (higher to lower) and >> - * buffer mailboxes are used to receive pkts while main mailboxes are being >> - * processed. This ensures in-order packet reception. >> - * >> - * Here are the recommended values for buffer mailbox. Note that RX >> mailboxes >> - * start after TX mailboxes: >> - * >> - * HECC_MAX_RX_MBOX HECC_RX_BUFFER_MBOX No of buffer mailboxes >> - * 28 12 8 >> - * 16 20 4 >> + * RX mailbox configuration >> + * The remaining ma
Re: [PATCH v2] net: cpsw: fix obtaining mac address for am3517
Hello Tony,
On 28-10-16 17:52, Tony Lindgren wrote:
* Jeroen Hofstee [161028 08:33]:
Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac
id to common file") did not only move the code for an am3517, it also
added the slave parameter, resulting in an invalid (all zero) mac address
being returned for an am3517, since it only has a single emac and the slave
parameter is pointing to the second. So simply always read the first and
valid mac-address for a ti,am3517-emac.
And others davinci_emac.c users can have more than one. So is the
reason the slave parameter points to the second instance because
of the location in the hardware?
Sort of, the slave parameter gets determined by the fact if there is one
or two register range(s) associated with the davinci_emac. In davinci_emac.c
res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
...
rc = davinci_emac_try_get_mac(pdev, res_ctrl ? 0 : 1,
priv->mac_addr);
So it there are two ranges, the slave param becomes 0. It there is only
one, it
will be 1. Since the am3517 only has a single regs entry it ends up with
slave 1,
while there is only a single davinci_emac.
Regards,
Jeroen
arch/arm/boot/dts/dm816x.dtsi
-
eth0: ethernet@4a10 {
compatible = "ti,dm816-emac";
ti,hwmods = "emac0";
reg = <0x4a10 0x800
0x4a100900 0x3700>;
clocks = <&sysclk24_ck>;
syscon = <&scm_conf>;
ti,davinci-ctrl-reg-offset = <0>;
ti,davinci-ctrl-mod-reg-offset = <0x900>;
ti,davinci-ctrl-ram-offset = <0x2000>;
ti,davinci-ctrl-ram-size = <0x2000>;
interrupts = <40 41 42 43>;
phy-handle = <&phy0>;
};
eth1: ethernet@4a12 {
compatible = "ti,dm816-emac";
ti,hwmods = "emac1";
reg = <0x4a12 0x4000>;
clocks = <&sysclk24_ck>;
syscon = <&scm_conf>;
ti,davinci-ctrl-reg-offset = <0>;
ti,davinci-ctrl-mod-reg-offset = <0x900>;
ti,davinci-ctrl-ram-offset = <0x2000>;
ti,davinci-ctrl-ram-size = <0x2000>;
interrupts = <44 45 46 47>;
phy-handle = <&phy1>;
};
arch/arm/boot/dts/am3517.dtsi
---
davinci_emac: ethernet@0x5c00 {
compatible = "ti,am3517-emac";
ti,hwmods = "davinci_emac";
status = "disabled";
reg = <0x5c00 0x3>;
interrupts = <67 68 69 70>;
syscon = <&scm_conf>;
ti,davinci-ctrl-reg-offset = <0x1>;
ti,davinci-ctrl-mod-reg-offset = <0>;
ti,davinci-ctrl-ram-offset = <0x2>;
ti,davinci-ctrl-ram-size = <0x2000>;
ti,davinci-rmii-en = /bits/ 8 <1>;
local-mac-address = [ 00 00 00 00 00 00 ];
};
Re: [PATCH] net: cpsw: fix obtaining mac address for am3517
Hello Tony,
On 21-10-16 08:38, Tony Lindgren wrote:
* Jeroen Hofstee [161020 12:57]:
Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac
id to common file") did not only move the code for an am3517, it also
added the slave parameter, resulting in a invalid (all zero) mac address
being returned. So change it back to always read from slave zero, so it
works again.
Hmm doesn't this now break it for cpsw with two instances?
Yes, well, they get the same mac address at least. But does it matter?
This changes davinci_emac_3517_get_macid, the only way to get there
is:
if (of_device_is_compatible(dev->of_node, "ti,am3517-emac"))
return davinci_emac_3517_get_macid(dev, 0x110, slave, mac_addr)
and the only user of ti,am3517-emac is arch/arm/boot/dts/am3517.dtsi,
which only has one emac. So the change is already am3517 specific.
We may need am3517 specific quirk flag instead?
Given above, it is already am3517 specific. Let me know if you prefer this
route then I will have a look at it.
Regards,
Jeroen
Re: [PATCH] net: cpsw: fix obtaining mac address for am3517
Hello Tony,
On 21-10-16 09:53, Tony Lindgren wrote:
* Jeroen Hofstee [161021 00:37]:
Hello Tony,
On 21-10-16 08:38, Tony Lindgren wrote:
* Jeroen Hofstee [161020 12:57]:
Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac
id to common file") did not only move the code for an am3517, it also
added the slave parameter, resulting in a invalid (all zero) mac address
being returned. So change it back to always read from slave zero, so it
works again.
Hmm doesn't this now break it for cpsw with two instances?
Yes, well, they get the same mac address at least. But does it matter?
This changes davinci_emac_3517_get_macid, the only way to get there
is:
if (of_device_is_compatible(dev->of_node, "ti,am3517-emac"))
return davinci_emac_3517_get_macid(dev, 0x110, slave, mac_addr)
and the only user of ti,am3517-emac is arch/arm/boot/dts/am3517.dtsi,
which only has one emac. So the change is already am3517 specific.
We may need am3517 specific quirk flag instead?
Given above, it is already am3517 specific. Let me know if you prefer this
route then I will have a look at it.
Oh OK thanks for explaining it :) As it's already am3517 specific:
Acked-by: Tony Lindgren
Aaah, lets wait a sec. I just saw there is another user of this function,
so above is simply not true
if (of_machine_is_compatible("ti,dra7"))
return davinci_emac_3517_get_macid(dev, 0x514, slave, mac_addr);
So let me check if I don't break that one. or better, let me send a
v2, which
changes the caller to pass slave as 0 here?
if (of_device_is_compatible(dev->of_node, "ti,am3517-emac"))
return davinci_emac_3517_get_macid(dev, 0x110, slave, mac_addr);
Regards,
Jeroen
