From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Ulrich Hecht <uli+renesas@fpond.eu>
Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
netdev <netdev@vger.kernel.org>,
"David S. Miller" <davem@davemloft.net>,
linux-can@vger.kernel.org, "Lad,
Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com>,
Biju Das <biju.das.jz@bp.renesas.com>,
Wolfram Sang <wsa@kernel.org>,
Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
Wolfgang Grandegger <wg@grandegger.com>,
Marc Kleine-Budde <mkl@pengutronix.de>,
Jakub Kicinski <kuba@kernel.org>,
mailhol.vincent@wanadoo.fr, socketcan@hartkopp.net
Subject: Re: [PATCH 1/3] can: rcar_canfd: Add support for r8a779a0 SoC
Date: Tue, 5 Oct 2021 15:06:22 +0200 [thread overview]
Message-ID: <CAMuHMdXk2mZntTBe3skSVkcNVjC-PzMwEv_MbH85Mvn1ZkFpHw@mail.gmail.com> (raw)
In-Reply-To: <20210924153113.10046-2-uli+renesas@fpond.eu>
Hi Uli,
On Fri, Sep 24, 2021 at 5:38 PM Ulrich Hecht <uli+renesas@fpond.eu> wrote:
> Adds support for the CANFD IP variant in the V3U SoC.
>
> Differences to controllers in other SoCs are limited to an increase in
> the number of channels from two to eight, an absence of dedicated
> registers for "classic" CAN mode, and a number of differences in magic
> numbers (register offsets and layouts).
>
> Inspired by BSP patch by Kazuya Mizuguchi.
>
> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
Thanks for your patch!
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -44,10 +44,13 @@
> enum rcanfd_chip_id {
> RENESAS_RCAR_GEN3 = 0,
> RENESAS_RZG2L,
> + RENESAS_R8A779A0,
RENESAS_RCAR_V3U? ...
> };
>
> /* Global register bits */
>
> +#define IS_V3U (gpriv->chip_id == RENESAS_R8A779A0)
... As you use V3U here.
> +
> /* RSCFDnCFDGRMCFG */
> #define RCANFD_GRMCFG_RCMC BIT(0)
>
> @@ -79,6 +82,7 @@ enum rcanfd_chip_id {
> #define RCANFD_GSTS_GNOPM (BIT(0) | BIT(1) | BIT(2) | BIT(3))
>
> /* RSCFDnCFDGERFL / RSCFDnGERFL */
> +#define RCANFD_GERFL_EEF0_7 GENMASK(23, 16)
> #define RCANFD_GERFL_EEF1 BIT(17)
> #define RCANFD_GERFL_EEF0 BIT(16)
> #define RCANFD_GERFL_CMPOF BIT(3) /* CAN FD only */
> @@ -86,20 +90,24 @@ enum rcanfd_chip_id {
> #define RCANFD_GERFL_MES BIT(1)
> #define RCANFD_GERFL_DEF BIT(0)
>
> -#define RCANFD_GERFL_ERR(gpriv, x) ((x) & (RCANFD_GERFL_EEF1 |\
> - RCANFD_GERFL_EEF0 | RCANFD_GERFL_MES |\
> - (gpriv->fdmode ?\
> - RCANFD_GERFL_CMPOF : 0)))
> +#define RCANFD_GERFL_ERR(gpriv, x) ((x) & ((IS_V3U ? RCANFD_GERFL_EEF0_7 : \
> + (RCANFD_GERFL_EEF0 | RCANFD_GERFL_EEF1)) | \
> + RCANFD_GERFL_MES | ((gpriv)->fdmode ? \
> + RCANFD_GERFL_CMPOF : 0)))
I'm wondering if some of these IS_V3U checks can be avoided, improving
legibility, by storing a feature struct instead of a chip_id in
rcar_canfd_of_table[].data?
> /* RSCFDnCFDRFCCx / RSCFDnRFCCx */
> -#define RCANFD_RFCC(x) (0x00b8 + (0x04 * (x)))
> +#define RCANFD_RFCC(x) ((IS_V3U ? 0x00c0 : 0x00b8) + \
> + (0x04 * (x)))
> /* RSCFDnCFDRFSTSx / RSCFDnRFSTSx */
> -#define RCANFD_RFSTS(x) (0x00d8 + (0x04 * (x)))
> +#define RCANFD_RFSTS(x) ((IS_V3U ? 0x00e0 : 0x00d8) + \
> + (0x04 * (x)))
> /* RSCFDnCFDRFPCTRx / RSCFDnRFPCTRx */
> -#define RCANFD_RFPCTR(x) (0x00f8 + (0x04 * (x)))
> +#define RCANFD_RFPCTR(x) ((IS_V3U ? 0x0100 : 0x00f8) + \
> + (0x04 * (x)))
There's some logic in the offsets: they're 32 bytes apart, regardless
of IS_V3U. Can we make use of that?
>
> /* Common FIFO Control registers */
>
> /* RSCFDnCFDCFCCx / RSCFDnCFCCx */
> -#define RCANFD_CFCC(ch, idx) (0x0118 + (0x0c * (ch)) + \
> - (0x04 * (idx)))
> +#define RCANFD_CFCC(ch, idx) ((IS_V3U ? 0x0120 : 0x0118) + \
> + (0x0c * (ch)) + (0x04 * (idx)))
> /* RSCFDnCFDCFSTSx / RSCFDnCFSTSx */
> -#define RCANFD_CFSTS(ch, idx) (0x0178 + (0x0c * (ch)) + \
> - (0x04 * (idx)))
> +#define RCANFD_CFSTS(ch, idx) ((IS_V3U ? 0x01e0 : 0x0178) + \
> + (0x0c * (ch)) + (0x04 * (idx)))
> /* RSCFDnCFDCFPCTRx / RSCFDnCFPCTRx */
> -#define RCANFD_CFPCTR(ch, idx) (0x01d8 + (0x0c * (ch)) + \
> - (0x04 * (idx)))
> +#define RCANFD_CFPCTR(ch, idx) ((IS_V3U ? 0x0240 : 0x01d8) + \
> + (0x0c * (ch)) + (0x04 * (idx)))
Same here, 96 bytes spacing.
> @@ -1488,22 +1545,29 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
> static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
> {
> struct net_device_stats *stats = &priv->ndev->stats;
> + struct rcar_canfd_global *gpriv = priv->gpriv;
> struct canfd_frame *cf;
> struct sk_buff *skb;
> u32 sts = 0, id, dlc;
> u32 ch = priv->channel;
> u32 ridx = ch + RCANFD_RFFIFO_IDX;
>
> - if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> + if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) ||
> + gpriv->chip_id == RENESAS_R8A779A0) {
> id = rcar_canfd_read(priv->base, RCANFD_F_RFID(ridx));
> dlc = rcar_canfd_read(priv->base, RCANFD_F_RFPTR(ridx));
>
> sts = rcar_canfd_read(priv->base, RCANFD_F_RFFDSTS(ridx));
> - if (sts & RCANFD_RFFDSTS_RFFDF)
> - skb = alloc_canfd_skb(priv->ndev, &cf);
> - else
> + if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> + if (sts & RCANFD_RFFDSTS_RFFDF)
> + skb = alloc_canfd_skb(priv->ndev, &cf);
> + else
> + skb = alloc_can_skb(priv->ndev,
> + (struct can_frame **)&cf);
> + } else {
> skb = alloc_can_skb(priv->ndev,
> (struct can_frame **)&cf);
The two else branches do the same, so they can be combined.
> + }
> } else {
> id = rcar_canfd_read(priv->base, RCANFD_C_RFID(ridx));
> dlc = rcar_canfd_read(priv->base, RCANFD_C_RFPTR(ridx));
> @@ -1563,6 +1633,7 @@ static int rcar_canfd_rx_poll(struct napi_struct *napi, int quota)
> {
> struct rcar_canfd_channel *priv =
> container_of(napi, struct rcar_canfd_channel, napi);
> + struct rcar_canfd_global *gpriv = priv->gpriv;
> int num_pkts;
> u32 sts;
> u32 ch = priv->channel;
> @@ -1762,19 +1833,23 @@ static int rcar_canfd_probe(struct platform_device *pdev)
> int g_err_irq, g_recc_irq;
> bool fdmode = true; /* CAN FD only mode - default */
> enum rcanfd_chip_id chip_id;
> + int max_channels;
> + char name[9];
> + int i;
>
> chip_id = (uintptr_t)of_device_get_match_data(&pdev->dev);
> + max_channels = chip_id == RENESAS_R8A779A0 ? 8 : 2;
max_channels is a good candidate to store in the feature struct.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
next prev parent reply other threads:[~2021-10-05 13:06 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-24 15:31 [PATCH 0/3] can: rcar_canfd: Add support for V3U flavor Ulrich Hecht
2021-09-24 15:31 ` [PATCH 1/3] can: rcar_canfd: Add support for r8a779a0 SoC Ulrich Hecht
2021-09-24 16:34 ` Wolfram Sang
2021-09-28 8:47 ` Ulrich Hecht
2021-10-05 13:06 ` Geert Uytterhoeven [this message]
2021-10-18 12:50 ` Marc Kleine-Budde
2022-01-11 16:21 ` Ulrich Hecht
2021-09-24 15:31 ` [PATCH 2/3] dt-bindings: can: renesas,rcar-canfd: Document r8a779a0 support Ulrich Hecht
2021-10-05 13:15 ` Geert Uytterhoeven
2021-09-24 15:31 ` [PATCH 3/3] arm64: dts: r8a779a0: Add CANFD device node Ulrich Hecht
2021-10-05 13:20 ` Geert Uytterhoeven
2022-01-11 16:21 ` Ulrich Hecht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMuHMdXk2mZntTBe3skSVkcNVjC-PzMwEv_MbH85Mvn1ZkFpHw@mail.gmail.com \
--to=geert@linux-m68k.org \
--cc=biju.das.jz@bp.renesas.com \
--cc=davem@davemloft.net \
--cc=kuba@kernel.org \
--cc=linux-can@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=mailhol.vincent@wanadoo.fr \
--cc=mkl@pengutronix.de \
--cc=netdev@vger.kernel.org \
--cc=prabhakar.mahadev-lad.rj@bp.renesas.com \
--cc=socketcan@hartkopp.net \
--cc=uli+renesas@fpond.eu \
--cc=wg@grandegger.com \
--cc=wsa@kernel.org \
--cc=yoshihiro.shimoda.uh@renesas.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).