| Message ID | 20220702140130.218409-4-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for RZ/N1 SJA1000 CAN controller | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 12 of 12 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 50 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On 02.07.2022 15:01:27, Biju Das wrote: > Chapter 6.5.16 of the RZ/N1 Peripheral Manual mentions the below > differences compared to the reference Philips SJA1000 device. > > Handling of Transmitted Messages: > * The CAN controller does not copy transmitted messages to the receive > buffer, unlike the reference device. > > Clock Divider Register: > * This register is not supported > > This patch adds device quirks to handle these differences. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > drivers/net/can/sja1000/sja1000.c | 17 +++++++++++------ > drivers/net/can/sja1000/sja1000.h | 4 +++- > 2 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c > index 2e7638f98cf1..49cf4fc4d896 100644 > --- a/drivers/net/can/sja1000/sja1000.c > +++ b/drivers/net/can/sja1000/sja1000.c > @@ -183,8 +183,9 @@ static void chipset_init(struct net_device *dev) > { > struct sja1000_priv *priv = netdev_priv(dev); > > - /* set clock divider and output control register */ > - priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN); > + if (!(priv->flags & SJA1000_NO_CDR_REG_QUIRK)) > + /* set clock divider and output control register */ > + priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN); > > /* set acceptance filter (accept all) */ > priv->write_reg(priv, SJA1000_ACCC0, 0x00); > @@ -208,9 +209,11 @@ static void sja1000_start(struct net_device *dev) > if (priv->can.state != CAN_STATE_STOPPED) > set_reset_mode(dev); > > - /* Initialize chip if uninitialized at this stage */ > - if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN)) > - chipset_init(dev); > + if (!(priv->flags & SJA1000_NO_CDR_REG_QUIRK)) { > + /* Initialize chip if uninitialized at this stage */ > + if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN)) > + chipset_init(dev); > + } > > /* Clear error counters and error code capture */ > priv->write_reg(priv, SJA1000_TXERR, 0x0); > @@ -652,12 +655,14 @@ static const struct net_device_ops sja1000_netdev_ops = { > > int register_sja1000dev(struct net_device *dev) > { > + struct sja1000_priv *priv = netdev_priv(dev); > int ret; > > if (!sja1000_probe_chip(dev)) > return -ENODEV; > > - dev->flags |= IFF_ECHO; /* we support local echo */ > + if (!(priv->flags & SJA1000_NO_HW_LOOPBACK_QUIRK)) > + dev->flags |= IFF_ECHO; /* we support local echo */ > dev->netdev_ops = &sja1000_netdev_ops; > > set_reset_mode(dev); > diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h > index 9d46398f8154..d0b8ce3f70ec 100644 > --- a/drivers/net/can/sja1000/sja1000.h > +++ b/drivers/net/can/sja1000/sja1000.h > @@ -145,7 +145,9 @@ > /* > * Flags for sja1000priv.flags > */ > -#define SJA1000_CUSTOM_IRQ_HANDLER 0x1 > +#define SJA1000_CUSTOM_IRQ_HANDLER BIT(0) > +#define SJA1000_NO_CDR_REG_QUIRK BIT(1) > +#define SJA1000_NO_HW_LOOPBACK_QUIRK BIT(2) Please name these defines SJA1000_QUIRK_* Marc
On 02.07.2022 15:01:27, Biju Das wrote: > Chapter 6.5.16 of the RZ/N1 Peripheral Manual mentions the below > differences compared to the reference Philips SJA1000 device. > > Handling of Transmitted Messages: > * The CAN controller does not copy transmitted messages to the receive > buffer, unlike the reference device. This is something different than.... > int register_sja1000dev(struct net_device *dev) > { > + struct sja1000_priv *priv = netdev_priv(dev); > int ret; > > if (!sja1000_probe_chip(dev)) > return -ENODEV; > > - dev->flags |= IFF_ECHO; /* we support local echo */ > + if (!(priv->flags & SJA1000_NO_HW_LOOPBACK_QUIRK)) > + dev->flags |= IFF_ECHO; /* we support local echo */ ... the IFF_ECHO. IFF_ECHO set means the driver cals can_put_echo_skb() before TX and can_get_echo_skb() after TX complete interrupt. | irqreturn_t sja1000_interrupt(int irq, void *dev_id) [...] | while ((isrc = priv->read_reg(priv, SJA1000_IR)) && | (n < SJA1000_MAX_IRQ)) { | | status = priv->read_reg(priv, SJA1000_SR); | /* check for absent controller due to hw unplug */ | if (status == 0xFF && sja1000_is_absent(priv)) | goto out; | | if (isrc & IRQ_WUI) | netdev_warn(dev, "wakeup interrupt\n"); | | if (isrc & IRQ_TI) { | /* transmission buffer released */ | if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT && | !(status & SR_TCS)) { | stats->tx_errors++; | can_free_echo_skb(dev, 0, NULL); | } else { Please add a netdev_info() for debugging and verify that you get a TX complete IRQ. | /* transmission complete */ | stats->tx_bytes += can_get_echo_skb(dev, 0, NULL); | stats->tx_packets++; | } | netif_wake_queue(dev); | } If your hardware doesn't support hardware loopback (configured via CMD_SRR): | if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) | cmd_reg_val |= CMD_SRR; | else | cmd_reg_val |= CMD_TR; then don't set CAN_CTRLMODE_LOOPBACK in priv->can.ctrlmode_supported. Marc
Hi Marc, Thanks for the feedback. > Subject: Re: [PATCH 3/6] can: sja1000: Add Quirks for RZ/N1 SJA1000 CAN > controller > > On 02.07.2022 15:01:27, Biju Das wrote: > > Chapter 6.5.16 of the RZ/N1 Peripheral Manual mentions the below > > differences compared to the reference Philips SJA1000 device. > > > > Handling of Transmitted Messages: > > * The CAN controller does not copy transmitted messages to the > receive > > buffer, unlike the reference device. > > This is something different than.... > > > int register_sja1000dev(struct net_device *dev) { > > + struct sja1000_priv *priv = netdev_priv(dev); > > int ret; > > > > if (!sja1000_probe_chip(dev)) > > return -ENODEV; > > > > - dev->flags |= IFF_ECHO; /* we support local echo */ > > + if (!(priv->flags & SJA1000_NO_HW_LOOPBACK_QUIRK)) > > + dev->flags |= IFF_ECHO; /* we support local echo */ > > ... the IFF_ECHO. > > IFF_ECHO set means the driver cals can_put_echo_skb() before TX and > can_get_echo_skb() after TX complete interrupt. OK. > > | irqreturn_t sja1000_interrupt(int irq, void *dev_id) > [...] > | while ((isrc = priv->read_reg(priv, SJA1000_IR)) && > | (n < SJA1000_MAX_IRQ)) { > | > | status = priv->read_reg(priv, SJA1000_SR); > | /* check for absent controller due to hw unplug */ > | if (status == 0xFF && sja1000_is_absent(priv)) > | goto out; > | > | if (isrc & IRQ_WUI) > | netdev_warn(dev, "wakeup interrupt\n"); > | > | if (isrc & IRQ_TI) { > | /* transmission buffer released */ > | if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT && > | !(status & SR_TCS)) { > | stats->tx_errors++; > | can_free_echo_skb(dev, 0, NULL); > | } else { > > Please add a netdev_info() for debugging and verify that you get a TX > complete IRQ. I have put prints and I confirm I get Tx complete IRQ. --- a/drivers/net/can/sja1000/sja1000.c +++ b/drivers/net/can/sja1000/sja1000.c @@ -526,6 +526,7 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id) stats->tx_errors++; can_free_echo_skb(dev, 0, NULL); } else { + netdev_info(dev,"#### Completed message in IRQ"); /* transmission complete */ root@rzn1d400-db:/rzn1_tests/linux# ./test-can.sh Testing sending from can0 to can1 at 100000 bps [ 30.032757] sja1000_platform 52105000.can can1: setting BTR0=0x0e BTR1=0x1c [ 30.062123] IPv6: ADDRCONF(NETDEV_CHANGE): can1: link becomes ready [ 30.128729] sja1000_platform 52104000.can can0: setting BTR0=0x0e BTR1=0x1c [ 31.130232] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready [ 32.237998] sja1000_platform 52104000.can can0: #### Completed message in IRQ [ 32.246774] sja1000_platform 52104000.can can0: #### Completed message in IRQ .... ..... [ 33.210809] sja1000_platform 52104000.can can0: #### Completed message in IRQ [ 33.221272] sja1000_platform 52104000.can can0: #### Completed message in IRQ Testing sending from can0 to can1 at 500000 bps [ 33.366862] sja1000_platform 52105000.can can1: setting BTR0=0x02 BTR1=0x1c [ 33.455829] sja1000_platform 52104000.can can0: setting BTR0=0x02 BTR1=0x1c [ 35.546544] sja1000_platform 52104000.can can0: #### Completed message in IRQ [ 35.554487] sja1000_platform 52104000.can can0: #### Completed message in IRQ [ 35.562322] sja1000_platform 52104000.can can0: #### Completed message in IRQ ..... [ 36.520433] sja1000_platform 52104000.can can0: #### Completed message in IRQ [ 36.530376] sja1000_platform 52104000.can can0: #### Completed message in IRQ Testing sending from can0 to can1 at 1000000 bps [ 36.667887] sja1000_platform 52105000.can can1: setting BTR0=0x01 BTR1=0x27 [ 36.759561] sja1000_platform 52104000.can can0: setting BTR0=0x01 BTR1=0x27 [ 38.849459] sja1000_platform 52104000.can can0: #### Completed message in IRQ [ 38.857118] sja1000_platform 52104000.can can0: #### Completed message in IRQ [ 38.864611] sja1000_platform 52104000.can can0: #### Completed message in IRQ .... [ 39.830284] sja1000_platform 52104000.can can0: #### Completed message in IRQ root@rzn1d400-db:/rzn1_tests/linux# cat /proc/interrupts | grep can 35: 300 0 GIC-0 127 Level can0 36: 300 0 GIC-0 128 Level can1 root@rzn1d400-db:/rzn1_tests/linux# ./test-can.sh can1 can0 Testing sending from can1 to can0 at 100000 bps [ 111.972815] sja1000_platform 52104000.can can0: setting BTR0=0x0e BTR1=0x1c [ 112.064622] sja1000_platform 52105000.can can1: setting BTR0=0x0e BTR1=0x1c [ 114.159798] sja1000_platform 52105000.can can1: #### Completed message in IRQ [ 114.168361] sja1000_platform 52105000.can can1: #### Completed message in IRQ ..... [ 115.141437] sja1000_platform 52105000.can can1: #### Completed message in IRQ Testing sending from can1 to can0 at 500000 bps [ 115.272323] sja1000_platform 52104000.can can0: setting BTR0=0x02 BTR1=0x1c [ 115.365008] sja1000_platform 52105000.can can1: setting BTR0=0x02 BTR1=0x1c [ 117.445187] sja1000_platform 52105000.can can1: #### Completed message in IRQ [ 117.460489] sja1000_platform 52105000.can can1: #### Completed message in IRQ [ 117.470574] sja1000_platform 52105000.can can1: #### Completed message in IRQ .... [ 118.430442] sja1000_platform 52105000.can can1: #### Completed message in IRQ [ 118.440381] sja1000_platform 52105000.can can1: #### Completed message in IRQ Testing sending from can1 to can0 at 1000000 bps [ 118.564981] sja1000_platform 52104000.can can0: setting BTR0=0x01 BTR1=0x27 [ 118.655100] sja1000_platform 52105000.can can1: setting BTR0=0x01 BTR1=0x27 [ 120.731882] sja1000_platform 52105000.can can1: #### Completed message in IRQ [ 120.740605] sja1000_platform 52105000.can can1: #### Completed message in IRQ ... [ 121.710274] sja1000_platform 52105000.can can1: #### Completed message in IRQ [ 121.720370] sja1000_platform 52105000.can can1: #### Completed message in IRQ root@rzn1d400-db:/rzn1_tests/linux#root@rzn1d400-db:/rzn1_tests/linux# cat /proc/interrupts | grep can 35: 600 0 GIC-0 127 Level can0 36: 600 0 GIC-0 128 Level can1 > > | /* transmission complete */ > | stats->tx_bytes += can_get_echo_skb(dev, 0, > NULL); > | stats->tx_packets++; > | } > | netif_wake_queue(dev); > | } > > > If your hardware doesn't support hardware loopback (configured via > CMD_SRR): But our HW supports, CMD_SRR. b4 bCan_SRR Self Reception Request 1: Set when a message is to be transmitted and received simultaneously [Condition of “Cleared to 0”] ● Software Reset (Set “1” in bCan_RM) ● Switch to “Bus Off” (Set “1” in bCan_BS) Cheers, Biju > > | if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) > | cmd_reg_val |= CMD_SRR; > | else > | cmd_reg_val |= CMD_TR; > > then don't set CAN_CTRLMODE_LOOPBACK in priv->can.ctrlmode_supported.
Hi Marc, Thanks for the feedback. > Subject: Re: [PATCH 3/6] can: sja1000: Add Quirks for RZ/N1 SJA1000 CAN > controller > > On 02.07.2022 15:01:27, Biju Das wrote: > > Chapter 6.5.16 of the RZ/N1 Peripheral Manual mentions the below > > differences compared to the reference Philips SJA1000 device. > > > > Handling of Transmitted Messages: > > * The CAN controller does not copy transmitted messages to the > receive > > buffer, unlike the reference device. > > > > Clock Divider Register: > > * This register is not supported > > > > This patch adds device quirks to handle these differences. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > drivers/net/can/sja1000/sja1000.c | 17 +++++++++++------ > > drivers/net/can/sja1000/sja1000.h | 4 +++- > > 2 files changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/can/sja1000/sja1000.c > > b/drivers/net/can/sja1000/sja1000.c > > index 2e7638f98cf1..49cf4fc4d896 100644 > > --- a/drivers/net/can/sja1000/sja1000.c > > +++ b/drivers/net/can/sja1000/sja1000.c > > @@ -183,8 +183,9 @@ static void chipset_init(struct net_device *dev) > > { > > struct sja1000_priv *priv = netdev_priv(dev); > > > > - /* set clock divider and output control register */ > > - priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN); > > + if (!(priv->flags & SJA1000_NO_CDR_REG_QUIRK)) > > + /* set clock divider and output control register */ > > + priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN); > > > > /* set acceptance filter (accept all) */ > > priv->write_reg(priv, SJA1000_ACCC0, 0x00); @@ -208,9 +209,11 @@ > > static void sja1000_start(struct net_device *dev) > > if (priv->can.state != CAN_STATE_STOPPED) > > set_reset_mode(dev); > > > > - /* Initialize chip if uninitialized at this stage */ > > - if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN)) > > - chipset_init(dev); > > + if (!(priv->flags & SJA1000_NO_CDR_REG_QUIRK)) { > > + /* Initialize chip if uninitialized at this stage */ > > + if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN)) > > + chipset_init(dev); > > + } > > > > /* Clear error counters and error code capture */ > > priv->write_reg(priv, SJA1000_TXERR, 0x0); @@ -652,12 +655,14 @@ > > static const struct net_device_ops sja1000_netdev_ops = { > > > > int register_sja1000dev(struct net_device *dev) { > > + struct sja1000_priv *priv = netdev_priv(dev); > > int ret; > > > > if (!sja1000_probe_chip(dev)) > > return -ENODEV; > > > > - dev->flags |= IFF_ECHO; /* we support local echo */ > > + if (!(priv->flags & SJA1000_NO_HW_LOOPBACK_QUIRK)) > > + dev->flags |= IFF_ECHO; /* we support local echo */ > > dev->netdev_ops = &sja1000_netdev_ops; > > > > set_reset_mode(dev); > > diff --git a/drivers/net/can/sja1000/sja1000.h > > b/drivers/net/can/sja1000/sja1000.h > > index 9d46398f8154..d0b8ce3f70ec 100644 > > --- a/drivers/net/can/sja1000/sja1000.h > > +++ b/drivers/net/can/sja1000/sja1000.h > > @@ -145,7 +145,9 @@ > > /* > > * Flags for sja1000priv.flags > > */ > > -#define SJA1000_CUSTOM_IRQ_HANDLER 0x1 > > +#define SJA1000_CUSTOM_IRQ_HANDLER BIT(0) > > +#define SJA1000_NO_CDR_REG_QUIRK BIT(1) > > +#define SJA1000_NO_HW_LOOPBACK_QUIRK BIT(2) > > Please name these defines SJA1000_QUIRK_* Agreed, Will fix this in V2. Cheers, Biju
diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c index 2e7638f98cf1..49cf4fc4d896 100644 --- a/drivers/net/can/sja1000/sja1000.c +++ b/drivers/net/can/sja1000/sja1000.c @@ -183,8 +183,9 @@ static void chipset_init(struct net_device *dev) { struct sja1000_priv *priv = netdev_priv(dev); - /* set clock divider and output control register */ - priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN); + if (!(priv->flags & SJA1000_NO_CDR_REG_QUIRK)) + /* set clock divider and output control register */ + priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN); /* set acceptance filter (accept all) */ priv->write_reg(priv, SJA1000_ACCC0, 0x00); @@ -208,9 +209,11 @@ static void sja1000_start(struct net_device *dev) if (priv->can.state != CAN_STATE_STOPPED) set_reset_mode(dev); - /* Initialize chip if uninitialized at this stage */ - if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN)) - chipset_init(dev); + if (!(priv->flags & SJA1000_NO_CDR_REG_QUIRK)) { + /* Initialize chip if uninitialized at this stage */ + if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN)) + chipset_init(dev); + } /* Clear error counters and error code capture */ priv->write_reg(priv, SJA1000_TXERR, 0x0); @@ -652,12 +655,14 @@ static const struct net_device_ops sja1000_netdev_ops = { int register_sja1000dev(struct net_device *dev) { + struct sja1000_priv *priv = netdev_priv(dev); int ret; if (!sja1000_probe_chip(dev)) return -ENODEV; - dev->flags |= IFF_ECHO; /* we support local echo */ + if (!(priv->flags & SJA1000_NO_HW_LOOPBACK_QUIRK)) + dev->flags |= IFF_ECHO; /* we support local echo */ dev->netdev_ops = &sja1000_netdev_ops; set_reset_mode(dev); diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h index 9d46398f8154..d0b8ce3f70ec 100644 --- a/drivers/net/can/sja1000/sja1000.h +++ b/drivers/net/can/sja1000/sja1000.h @@ -145,7 +145,9 @@ /* * Flags for sja1000priv.flags */ -#define SJA1000_CUSTOM_IRQ_HANDLER 0x1 +#define SJA1000_CUSTOM_IRQ_HANDLER BIT(0) +#define SJA1000_NO_CDR_REG_QUIRK BIT(1) +#define SJA1000_NO_HW_LOOPBACK_QUIRK BIT(2) /* * SJA1000 private data structure
Chapter 6.5.16 of the RZ/N1 Peripheral Manual mentions the below differences compared to the reference Philips SJA1000 device. Handling of Transmitted Messages: * The CAN controller does not copy transmitted messages to the receive buffer, unlike the reference device. Clock Divider Register: * This register is not supported This patch adds device quirks to handle these differences. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- drivers/net/can/sja1000/sja1000.c | 17 +++++++++++------ drivers/net/can/sja1000/sja1000.h | 4 +++- 2 files changed, 14 insertions(+), 7 deletions(-)