devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] CAST Controller Area Network driver support
@ 2024-09-22 14:51 Hal Feng
  2024-09-22 14:51 ` [PATCH v2 1/4] dt-bindings: vendor-prefixes: Add cast vendor prefix Hal Feng
                   ` (3 more replies)
  0 siblings, 4 replies; 20+ messages in thread
From: Hal Feng @ 2024-09-22 14:51 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Marc Kleine-Budde,
	Vincent Mailhol, David S . Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Philipp Zabel, Palmer Dabbelt, Paul Walmsley,
	Albert Ou
  Cc: Emil Renner Berthing, William Qiu, Hal Feng, devicetree,
	linux-can, netdev, linux-riscv, linux-kernel

This patchset adds support for the CAST Controller Area Network Bus
Controller (version fd-7x10N00S00) which is used in StarFive JH7110 SoC.
Note that the CAN FD license for JH7110 has expired, so JH7110 only
supports CAN CC now.

Changes since v1:
Patch 1:
- Add company information in the commit message.
Patch 2:
- Add description for the hardware.
- Move "allOf" stuff down after the property definitions.
- Rename compatible names, clock names, reset names and syscon register
  names.
- Rewrite the example.
Patch 3:
- Reorder all functions for readability.
- Simplify register definitions and register access functions.
- Improve syscon related code.
- Use clk_bulk interface.
- Enable the clocks during .ndo_open() and disable during .ndo_stop().
- Use can_put_echo_skb() and can_get_echo_skb().
- Stop the TX queue when entering .ndo_start_xmit() and restart the TX
  queue after the transmission finished.
- Simplify logic and remove redundant code.
- Improve coding style.
Patch 4:
- Update the nodes according to the new dt-bindings.

History:
v1: https://lore.kernel.org/all/20240129031239.17037-1-william.qiu@starfivetech.com/

William Qiu (4):
  dt-bindings: vendor-prefixes: Add cast vendor prefix
  dt-bindings: can: Add CAST CAN Bus Controller
  can: Add driver for CAST CAN Bus Controller
  riscv: dts: starfive: jh7110: Add CAN nodes

 .../bindings/net/can/cast,can-ctrl.yaml       | 106 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   8 +
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |  32 +
 drivers/net/can/Kconfig                       |   7 +
 drivers/net/can/Makefile                      |   1 +
 drivers/net/can/cast_can.c                    | 936 ++++++++++++++++++
 7 files changed, 1092 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml
 create mode 100644 drivers/net/can/cast_can.c


base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652
-- 
2.43.2


^ permalink raw reply	[flat|nested] 20+ messages in thread
* RE: [PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller
@ 2024-10-25  2:11 Hal Feng
  0 siblings, 0 replies; 20+ messages in thread
From: Hal Feng @ 2024-10-25  2:11 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Marc Kleine-Budde,
	Vincent Mailhol, David S . Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Philipp Zabel, Palmer Dabbelt, Paul Walmsley,
	Albert Ou, Emil Renner Berthing, William Qiu,
	devicetree@vger.kernel.org, linux-can@vger.kernel.org,
	netdev@vger.kernel.org, linux-riscv@lists.infradead.org,
	linux-kernel@vger.kernel.org

> On 23.9.24 20:13, Andrew Lunn wrote:
> > > > +	reset_test = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
> > > > +
> > > > +	if (!(reset_test & CCAN_RST_MASK)) {
> > > > +		netdev_alert(ndev, "Not in reset mode, cannot set bit
> > > timing\n");
> > > > +		return -EPERM;
> > > > +	}
> > >
> > >
> > > You don't see nedev_alert() used very often. If this is fatal then
> netdev_err().
> > >
> > > Also, EPERM? man 3 errno say:
> > >
> > >        EPERM           Operation not permitted (POSIX.1-2001).
> > >
> > > Why is this a permission issue?
> >
> > Will use netdev_err() and return -EWOULDBLOCK instead.
> 
> I'm not sure that is any better.
> 
>        EAGAIN          Resource  temporarily unavailable (may be the same value
>                        as EWOULDBLOCK) (POSIX.1-2001).
> 
> This is generally used when the kernel expects user space to try a system call
> again, and it might then work. Is that what you expect here?

The bit timing should only be set when the module is in reset mode.
So we return an error here if the module is not in the correct mode.
Could you give me some suggestions about the return value here?

> 
> > > > +static irqreturn_t ccan_interrupt(int irq, void *dev_id) {
> > > > +	struct net_device *ndev = (struct net_device *)dev_id;
> > >
> > > dev_id is a void *, so you don't need the cast.
> >
> > OK, drop it.
> 
> Please look at the whole patch. There might be other instances where a void *
> is used with a cast, which can be removed. This was just the first i spotted.

OK. Will modify for the whole patch.

Best regards,
Hal

^ permalink raw reply	[flat|nested] 20+ messages in thread
* RE: [PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller
@ 2024-10-25  2:49 Hal Feng
  0 siblings, 0 replies; 20+ messages in thread
From: Hal Feng @ 2024-10-25  2:49 UTC (permalink / raw)
  To: Vincent MAILHOL, Hal Feng
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Marc Kleine-Budde,
	David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Philipp Zabel, Palmer Dabbelt, Paul Walmsley, Albert Ou,
	Emil Renner Berthing, William Qiu, devicetree@vger.kernel.org,
	linux-can@vger.kernel.org, netdev@vger.kernel.org,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org

> On 16.10.24 22:16, Vincent MAILHOL wrote:
> On Wed. 16 Oct. 2024 at 14:05, Vincent MAILHOL
> <mailhol.vincent@wanadoo.fr> wrote:
> > On Tue. 15 Oct. 2024 at 18:33, Hal Feng <hal.feng@linux.starfivetech.com>
> wrote:
> > > On 9/23/2024 11:41 AM, Vincent MAILHOL wrote:
> > > > Hi Hal,
> > > >
> > > > A few more comments on top of what Andrew already wrote.
> > > >
> > > > On Mon. 23 Sep. 2024 at 00:09, Hal Feng <hal.feng@starfivetech.com>
> wrote:
> > > >> From: William Qiu <william.qiu@starfivetech.com>
> > > >>
> > > >> Add driver for CAST CAN Bus Controller used on StarFive JH7110
> > > >> SoC.
> > > >>
> > > >> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> > > >> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
> > > >> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > > >> ---
> 
> (...)
> 
> > > >> +
> > > >> +       if (priv->cantype == CAST_CAN_TYPE_CANFD) {
> > > >> +               priv->can.ctrlmode_supported =
> CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_FD;
> > > >> +               priv->can.data_bittiming_const =
> &ccan_data_bittiming_const_canfd;
> > > >> +       } else {
> > > >> +               priv->can.ctrlmode_supported =
> CAN_CTRLMODE_LOOPBACK;
> > > >> +       }
> > > >
> > > > Nitpick, consider doing this:
> > > >
> > > >   priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK;
> > > >   if (priv->cantype == CAST_CAN_TYPE_CANFD) {
> > > >           priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD;
> > > >           priv->can.data_bittiming_const =
> &ccan_data_bittiming_const_canfd;
> > > >   }
> > >
> > > OK.
> > >
> > > >
> > > > Also, does you hardware support dlc greater than 8 (c.f.
> > > > CAN_CTRLMODE_CC_LEN8_DLC)?
> > >
> > > The class CAN (CC) mode does not support, but the CAN FD mode
> supports.
> >
> > So, CAN_CTRLMODE_CC_LEN8_DLC is a Classical CAN feature. Strictly
> > speaking, this does not exist in CAN FD. Do you mean that only the
> > CAST_CAN_TYPE_CANFD supports sending Classical CAN frames with a DLC
> > greater than 8?
> >
> > If none of the Classical CAN or CAN FD variants of your device is able
> > to send Classical CAN frames with a DLC greater than 8, then this is
> > just not supported by your device.
> >
> > Could you share the datasheet so that I can double check this?
> 
> I received the datasheet from a good samaritan. With this, I was able to
> confirm a few things.
> 
> 1/ Your device can support CAN_CTRLMODE_CC_LEN8_DLC:
> 
> This is shown in the datasheet at:
> 
>   Table 3-52 Definition of the DLC (according to the CAN 2.0 / FD specification)
> 
> DLC values 9 to 15 (binary 1001 to 1111) are accepted by the device.
> When sending and receiving such frames, can_frame->len is set to 8 and
> can_frame->len8_dlc is set to the actual DLC value. Use the
> can_cc_dlc2len() and can_get_cc_dlc() helpers for this.
> 
> 
> 2/ Your device can support CAN_CTRLMODE_TDC_AUTO:
> 
> This is documented in the datasheet at:
> 
>   8.8 TDC and RDC
> 
> This will allow the use of higher bitrates (e.g. 4 Mbits/s) in CAN-FD.
> You can refer to this commit for an example of how to implement it:
> 
>   https://git.kernel.org/torvalds/c/1010a8fa9608
> 
> 
> 3/ Your device can support CAN_CTRLMODE_3_SAMPLES:
> 
> This is called triple mode redundancy (TMR) in your datasheet.
> 
> 
> 4/ Your device can support CAN_CTRLMODE_LISTENONLY:
> 
> This is documented in the datasheet at:
> 
>   3.9.10.2. Listen Only Mode (LOM)
> 
> 
> 5/ Your device can support CAN_CTRLMODE_ONE_SHOT:
> 
> This is documented in the datasheet at:
> 
>   6.5.3 Single Shot Transmit Trigger
> 
> 
> 6/ Your device can support CAN_CTRLMODE_BERR_REPORTING:
> 
> This is shown in the datasheet at:
> 
>   Table 3-24 Error Counter Registers RECNT (0xb2) and TECNT (0xb3)
> 
> 
> 7/ Your device can support CAN_CTRLMODE_PRESUME_ACK:
> 
> c.f. the SACK (self acknowledge) register
> 
> 
> So your device comes with MANY features. I would like to see those
> implemented in your driver. Most of the time, adding a feature just means
> writing one value to a register.
> 
> Please let me know if any of this is unclear.

I will confirm the above features with my colleagues. If these features can
really be supported, let's implement them. Thanks.

Best regards,
Hal

^ permalink raw reply	[flat|nested] 20+ messages in thread
end of thread, other threads:[~2024-10-28 15:29 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-22 14:51 [PATCH v2 0/4] CAST Controller Area Network driver support Hal Feng
2024-09-22 14:51 ` [PATCH v2 1/4] dt-bindings: vendor-prefixes: Add cast vendor prefix Hal Feng
2024-09-22 14:51 ` [PATCH v2 2/4] dt-bindings: can: Add CAST CAN Bus Controller Hal Feng
2024-09-24 18:01   ` Rob Herring (Arm)
2024-09-24 20:03   ` Rob Herring
2024-09-22 14:51 ` [PATCH v2 3/4] can: Add driver for " Hal Feng
2024-09-22 16:33   ` Andrew Lunn
2024-09-23  7:53     ` Hal Feng
2024-09-23 12:12       ` Andrew Lunn
2024-10-28 14:18       ` Marc Kleine-Budde
2024-09-22 21:13   ` Marc Kleine-Budde
2024-10-25  1:45     ` Hal Feng
2024-10-28 15:28       ` Marc Kleine-Budde
2024-09-23  3:41   ` Vincent MAILHOL
2024-10-15  9:30     ` Hal Feng
2024-10-16  5:05       ` Vincent MAILHOL
2024-10-16 14:16         ` Vincent MAILHOL
2024-09-22 14:51 ` [PATCH v2 4/4] riscv: dts: starfive: jh7110: Add CAN nodes Hal Feng
  -- strict thread matches above, loose matches on Subject: below --
2024-10-25  2:11 [PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller Hal Feng
2024-10-25  2:49 Hal Feng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).