Hi All,
This patch series adds CANFD support to Renesas RZ/G2L family.
CANFD block on RZ/G2L SoC is almost identical to one found on
R-Car Gen3 SoC's. On RZ/G2L SoC interrupt sources for each channel
are split into individual sources.
Cheers,
Prabhakar
Changes for v2:
* Added interrupt-names property and marked it as required for
RZ/G2L family
* Added descriptions for reset property
* Re-used irq handlers on RZ/G2L SoC
* Added new enum for chip_id
* Dropped R9A07G044_LAST_CORE_CLK
* Dropped patch (clk: renesas: r9a07g044-cpg: Add clock and reset
entries for CANFD) as its been merged into renesas tree
Lad Prabhakar (5):
dt-bindings: net: can: renesas,rcar-canfd: Document RZ/G2L SoC
can: rcar_canfd: Add support for RZ/G2L family
dt-bindings: clk: r9a07g044-cpg: Add entry for P0_DIV2 core clock
clk: renesas: r9a07g044-cpg: Add entry for fixed clock P0_DIV2
arm64: dts: renesas: r9a07g044: Add CANFD node
.../bindings/net/can/renesas,rcar-canfd.yaml | 66 ++++++-
arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 42 +++++
drivers/clk/renesas/r9a07g044-cpg.c | 3 +-
drivers/net/can/rcar/rcar_canfd.c | 178 +++++++++++++++---
include/dt-bindings/clock/r9a07g044-cpg.h | 1 +
5 files changed, 252 insertions(+), 38 deletions(-)
base-commit: 2734d6c1b1a089fb593ef6a23d4b70903526fe0c
--
2.17.1
Add CANFD binding documentation for Renesas RZ/G2L SoC.
Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Biju Das <[email protected]>
---
.../bindings/net/can/renesas,rcar-canfd.yaml | 66 +++++++++++++++++--
1 file changed, 60 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
index 0b33ba9ccb47..4fb6dd370904 100644
--- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
+++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
@@ -30,13 +30,15 @@ properties:
- renesas,r8a77995-canfd # R-Car D3
- const: renesas,rcar-gen3-canfd # R-Car Gen3 and RZ/G2
+ - items:
+ - enum:
+ - renesas,r9a07g044-canfd # RZ/G2{L,LC}
+ - const: renesas,rzg2l-canfd # RZ/G2L family
+
reg:
maxItems: 1
- interrupts:
- items:
- - description: Channel interrupt
- - description: Global interrupt
+ interrupts: true
clocks:
maxItems: 3
@@ -50,8 +52,7 @@ properties:
power-domains:
maxItems: 1
- resets:
- maxItems: 1
+ resets: true
renesas,no-can-fd:
$ref: /schemas/types.yaml#/definitions/flag
@@ -91,6 +92,59 @@ required:
- channel0
- channel1
+if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - renesas,rzg2l-canfd
+then:
+ properties:
+ interrupts:
+ items:
+ - description: CAN global error interrupt
+ - description: CAN receive FIFO interrupt
+ - description: CAN0 error interrupt
+ - description: CAN0 transmit interrupt
+ - description: CAN0 transmit/receive FIFO receive completion interrupt
+ - description: CAN1 error interrupt
+ - description: CAN1 transmit interrupt
+ - description: CAN1 transmit/receive FIFO receive completion interrupt
+
+ interrupt-names:
+ items:
+ - const: g_error
+ - const: g_rx_fifo
+ - const: can0_error
+ - const: can0_tx
+ - const: can0_tx_rx_fifo_receive_completion
+ - const: can1_error
+ - const: can1_tx
+ - const: can1_tx_rx_fifo_receive_completion
+
+ resets:
+ items:
+ - description: CANFD_RSTP_N
+ - description: CANFD_RSTC_N
+
+ required:
+ - interrupt-names
+else:
+ properties:
+ interrupts:
+ items:
+ - description: Channel interrupt
+ - description: Global interrupt
+
+ interrupt-names:
+ items:
+ - const: ch_int
+ - const: g_int
+
+ resets:
+ items:
+ - description: CANFD reset
+
unevaluatedProperties: false
examples:
--
2.17.1
CANFD block on RZ/G2L SoC is almost identical to one found on
R-Car Gen3 SoC's. On RZ/G2L SoC interrupt sources for each channel
are split into different sources and the IP doesn't divide (1/2)
CANFD clock within the IP.
This patch adds compatible string for RZ/G2L family and registers
the irq handlers required for CANFD operation. IRQ numbers are now
fetched based on names instead of indices. For backward compatibility
on non RZ/G2L SoC's we fallback reading based on indices.
Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Biju Das <[email protected]>
---
drivers/net/can/rcar/rcar_canfd.c | 178 ++++++++++++++++++++++++------
1 file changed, 147 insertions(+), 31 deletions(-)
diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 311e6ca3bdc4..d4affc002fb3 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -37,9 +37,15 @@
#include <linux/bitmap.h>
#include <linux/bitops.h>
#include <linux/iopoll.h>
+#include <linux/reset.h>
#define RCANFD_DRV_NAME "rcar_canfd"
+enum rcanfd_chip_id {
+ RENESAS_RCAR_GEN3 = 0,
+ RENESAS_RZG2L,
+};
+
/* Global register bits */
/* RSCFDnCFDGRMCFG */
@@ -513,6 +519,9 @@ struct rcar_canfd_global {
enum rcar_canfd_fcanclk fcan; /* CANFD or Ext clock */
unsigned long channels_mask; /* Enabled channels mask */
bool fdmode; /* CAN FD or Classical CAN only mode */
+ struct reset_control *rstc1; /* Pointer to reset source1 */
+ struct reset_control *rstc2; /* Pointer to reset source2 */
+ enum rcanfd_chip_id chip_id;
};
/* CAN FD mode nominal rate constants */
@@ -1577,6 +1586,45 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
priv->can.clock.freq = fcan_freq;
dev_info(&pdev->dev, "can_clk rate is %u\n", priv->can.clock.freq);
+ if (gpriv->chip_id == RENESAS_RZG2L) {
+ char *irq_name;
+ int err_irq;
+ int tx_irq;
+
+ err_irq = platform_get_irq_byname(pdev, ch == 0 ? "can0_error" : "can1_error");
+ if (err_irq < 0) {
+ err = err_irq;
+ goto fail;
+ }
+
+ tx_irq = platform_get_irq_byname(pdev, ch == 0 ? "can0_tx" : "can1_tx");
+ if (tx_irq < 0) {
+ err = tx_irq;
+ goto fail;
+ }
+
+ irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+ "canfd.chnerr%d", ch);
+ err = devm_request_irq(&pdev->dev, err_irq,
+ rcar_canfd_channel_interrupt, 0,
+ irq_name, gpriv);
+ if (err) {
+ dev_err(&pdev->dev, "devm_request_irq CH Err(%d) failed, error %d\n",
+ err_irq, err);
+ goto fail;
+ }
+ irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+ "canfd.chntx%d", ch);
+ err = devm_request_irq(&pdev->dev, tx_irq,
+ rcar_canfd_channel_interrupt, 0,
+ irq_name, gpriv);
+ if (err) {
+ dev_err(&pdev->dev, "devm_request_irq Tx (%d) failed, error %d\n",
+ tx_irq, err);
+ goto fail;
+ }
+ }
+
if (gpriv->fdmode) {
priv->can.bittiming_const = &rcar_canfd_nom_bittiming_const;
priv->can.data_bittiming_const =
@@ -1635,8 +1683,11 @@ static int rcar_canfd_probe(struct platform_device *pdev)
struct rcar_canfd_global *gpriv;
struct device_node *of_child;
unsigned long channels_mask = 0;
- int err, ch_irq, g_irq;
+ int err, ch_irq, g_irq, g_rx_irq;
bool fdmode = true; /* CAN FD only mode - default */
+ enum rcanfd_chip_id chip_id;
+
+ chip_id = (enum rcanfd_chip_id)of_device_get_match_data(&pdev->dev);
if (of_property_read_bool(pdev->dev.of_node, "renesas,no-can-fd"))
fdmode = false; /* Classical CAN only mode */
@@ -1649,27 +1700,64 @@ static int rcar_canfd_probe(struct platform_device *pdev)
if (of_child && of_device_is_available(of_child))
channels_mask |= BIT(1); /* Channel 1 */
- ch_irq = platform_get_irq(pdev, 0);
- if (ch_irq < 0) {
- err = ch_irq;
- goto fail_dev;
- }
+ if (chip_id == RENESAS_RCAR_GEN3) {
+ ch_irq = platform_get_irq_byname(pdev, "ch_int");
+ if (ch_irq < 0) {
+ /* For backward compatibility get irq by index */
+ ch_irq = platform_get_irq(pdev, 0);
+ if (ch_irq < 0)
+ return ch_irq;
+ }
- g_irq = platform_get_irq(pdev, 1);
- if (g_irq < 0) {
- err = g_irq;
- goto fail_dev;
+ g_irq = platform_get_irq_byname(pdev, "g_int");
+ if (g_irq < 0) {
+ /* For backward compatibility get irq by index */
+ g_irq = platform_get_irq(pdev, 1);
+ if (g_irq < 0)
+ return g_irq;
+ }
+ } else {
+ g_irq = platform_get_irq_byname(pdev, "g_error");
+ if (g_irq < 0)
+ return g_irq;
+
+ g_rx_irq = platform_get_irq_byname(pdev, "g_rx_fifo");
+ if (g_rx_irq < 0)
+ return g_rx_irq;
}
/* Global controller context */
gpriv = devm_kzalloc(&pdev->dev, sizeof(*gpriv), GFP_KERNEL);
- if (!gpriv) {
- err = -ENOMEM;
- goto fail_dev;
- }
+ if (!gpriv)
+ return -ENOMEM;
+
gpriv->pdev = pdev;
gpriv->channels_mask = channels_mask;
gpriv->fdmode = fdmode;
+ gpriv->chip_id = chip_id;
+
+ if (gpriv->chip_id == RENESAS_RZG2L) {
+ gpriv->rstc1 = devm_reset_control_get_exclusive_by_index(&pdev->dev, 0);
+ if (IS_ERR(gpriv->rstc1)) {
+ dev_err(&pdev->dev, "failed to get reset index 0\n");
+ return PTR_ERR(gpriv->rstc1);
+ }
+
+ err = reset_control_reset(gpriv->rstc1);
+ if (err)
+ return err;
+
+ gpriv->rstc2 = devm_reset_control_get_exclusive_by_index(&pdev->dev, 1);
+ if (IS_ERR(gpriv->rstc2)) {
+ dev_err(&pdev->dev, "failed to get reset index 1\n");
+ return PTR_ERR(gpriv->rstc2);
+ }
+ err = reset_control_reset(gpriv->rstc2);
+ if (err) {
+ reset_control_assert(gpriv->rstc1);
+ return err;
+ }
+ }
/* Peripheral clock */
gpriv->clkp = devm_clk_get(&pdev->dev, "fck");
@@ -1699,7 +1787,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
}
fcan_freq = clk_get_rate(gpriv->can_clk);
- if (gpriv->fcan == RCANFD_CANFDCLK)
+ if (gpriv->fcan == RCANFD_CANFDCLK && gpriv->chip_id == RENESAS_RCAR_GEN3)
/* CANFD clock is further divided by (1/2) within the IP */
fcan_freq /= 2;
@@ -1711,21 +1799,43 @@ static int rcar_canfd_probe(struct platform_device *pdev)
gpriv->base = addr;
/* Request IRQ that's common for both channels */
- err = devm_request_irq(&pdev->dev, ch_irq,
- rcar_canfd_channel_interrupt, 0,
- "canfd.chn", gpriv);
- if (err) {
- dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
- ch_irq, err);
- goto fail_dev;
- }
- err = devm_request_irq(&pdev->dev, g_irq,
- rcar_canfd_global_interrupt, 0,
- "canfd.gbl", gpriv);
- if (err) {
- dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
- g_irq, err);
- goto fail_dev;
+ if (gpriv->chip_id == RENESAS_RCAR_GEN3) {
+ err = devm_request_irq(&pdev->dev, ch_irq,
+ rcar_canfd_channel_interrupt, 0,
+ "canfd.chn", gpriv);
+ if (err) {
+ dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
+ ch_irq, err);
+ goto fail_dev;
+ }
+
+ err = devm_request_irq(&pdev->dev, g_irq,
+ rcar_canfd_global_interrupt, 0,
+ "canfd.gbl", gpriv);
+ if (err) {
+ dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
+ g_irq, err);
+ goto fail_dev;
+ }
+ } else {
+ err = devm_request_irq(&pdev->dev, g_rx_irq,
+ rcar_canfd_global_interrupt, 0,
+ "canfd.gblrx", gpriv);
+
+ if (err) {
+ dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
+ g_rx_irq, err);
+ goto fail_dev;
+ }
+
+ err = devm_request_irq(&pdev->dev, g_irq,
+ rcar_canfd_global_interrupt, 0,
+ "canfd.gblerr", gpriv);
+ if (err) {
+ dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
+ g_irq, err);
+ goto fail_dev;
+ }
}
/* Enable peripheral clock for register access */
@@ -1791,6 +1901,8 @@ static int rcar_canfd_probe(struct platform_device *pdev)
fail_clk:
clk_disable_unprepare(gpriv->clkp);
fail_dev:
+ reset_control_assert(gpriv->rstc1);
+ reset_control_assert(gpriv->rstc2);
return err;
}
@@ -1810,6 +1922,9 @@ static int rcar_canfd_remove(struct platform_device *pdev)
/* Enter global sleep mode */
rcar_canfd_set_bit(gpriv->base, RCANFD_GCTR, RCANFD_GCTR_GSLPR);
clk_disable_unprepare(gpriv->clkp);
+ reset_control_assert(gpriv->rstc1);
+ reset_control_assert(gpriv->rstc2);
+
return 0;
}
@@ -1827,7 +1942,8 @@ static SIMPLE_DEV_PM_OPS(rcar_canfd_pm_ops, rcar_canfd_suspend,
rcar_canfd_resume);
static const struct of_device_id rcar_canfd_of_table[] = {
- { .compatible = "renesas,rcar-gen3-canfd" },
+ { .compatible = "renesas,rcar-gen3-canfd", .data = (void *)RENESAS_RCAR_GEN3 },
+ { .compatible = "renesas,rzg2l-canfd", .data = (void *)RENESAS_RZG2L },
{ }
};
--
2.17.1
Add P0_DIV2 core clock required for CANFD module. CANFD core clock is
sourced from P0_DIV2 referenced from HW manual Rev.0.50.
Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Biju Das <[email protected]>
---
include/dt-bindings/clock/r9a07g044-cpg.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/dt-bindings/clock/r9a07g044-cpg.h b/include/dt-bindings/clock/r9a07g044-cpg.h
index 0728ad07ff7a..0bb17ff1a01a 100644
--- a/include/dt-bindings/clock/r9a07g044-cpg.h
+++ b/include/dt-bindings/clock/r9a07g044-cpg.h
@@ -30,6 +30,7 @@
#define R9A07G044_CLK_P2 19
#define R9A07G044_CLK_AT 20
#define R9A07G044_OSCCLK 21
+#define R9A07G044_CLK_P0_DIV2 22
/* R9A07G044 Module Clocks */
#define R9A07G044_CA55_SCLK 0
--
2.17.1
Add entry for fixed core clock P0_DIV2 and assign LAST_DT_CORE_CLK
to R9A07G044_CLK_P0_DIV2.
Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Biju Das <[email protected]>
---
drivers/clk/renesas/r9a07g044-cpg.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index f4ebbde358c6..523521a87713 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -16,7 +16,7 @@
enum clk_ids {
/* Core Clock Outputs exported to DT */
- LAST_DT_CORE_CLK = R9A07G044_OSCCLK,
+ LAST_DT_CORE_CLK = R9A07G044_CLK_P0_DIV2,
/* External Input Clocks */
CLK_EXTAL,
@@ -76,6 +76,7 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
DEF_FIXED("I", R9A07G044_CLK_I, CLK_PLL1, 1, 1),
DEF_DIV("P0", R9A07G044_CLK_P0, CLK_PLL2_DIV16, DIVPL2A,
dtable_1_32, CLK_DIVIDER_HIWORD_MASK),
+ DEF_FIXED("P0_DIV2", R9A07G044_CLK_P0_DIV2, R9A07G044_CLK_P0, 1, 2),
DEF_FIXED("TSU", R9A07G044_CLK_TSU, CLK_PLL2_DIV20, 1, 1),
DEF_DIV("P1", R9A07G044_CLK_P1, CLK_PLL3_DIV2_4,
DIVPL3B, dtable_1_32, CLK_DIVIDER_HIWORD_MASK),
--
2.17.1
Add CANFD node to R9A07G044 (RZ/G2L) SoC DTSI.
Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Biju Das <[email protected]>
---
arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 42 ++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
index 28aafa34d583..24032f38e3a1 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
@@ -13,6 +13,13 @@
#address-cells = <2>;
#size-cells = <2>;
+ /* External CAN clock - to be overridden by boards that provide it */
+ can_clk: can {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <0>;
+ };
+
/* clock can be either from exclk or crystal oscillator (XIN/XOUT) */
extal_clk: extal {
compatible = "fixed-clock";
@@ -89,6 +96,41 @@
status = "disabled";
};
+ canfd: can@10050000 {
+ compatible = "renesas,r9a07g044-canfd", "renesas,rzg2l-canfd";
+ reg = <0 0x10050000 0 0x8000>;
+ interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 427 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 422 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 428 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 424 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 423 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 429 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "g_error", "g_rx_fifo",
+ "can0_error", "can0_tx",
+ "can0_tx_rx_fifo_receive_completion",
+ "can1_error", "can1_tx",
+ "can1_tx_rx_fifo_receive_completion";
+ clocks = <&cpg CPG_MOD R9A07G044_CANFD_PCLK>,
+ <&cpg CPG_CORE R9A07G044_CLK_P0_DIV2>,
+ <&can_clk>;
+ clock-names = "fck", "canfd", "can_clk";
+ assigned-clocks = <&cpg CPG_CORE R9A07G044_CLK_P0_DIV2>;
+ assigned-clock-rates = <50000000>;
+ resets = <&cpg R9A07G044_CANFD_RSTP_N>,
+ <&cpg R9A07G044_CANFD_RSTC_N>;
+ power-domains = <&cpg>;
+ status = "disabled";
+
+ channel0 {
+ status = "disabled";
+ };
+ channel1 {
+ status = "disabled";
+ };
+ };
+
i2c0: i2c@10058000 {
#address-cells = <1>;
#size-cells = <0>;
--
2.17.1
Hi Prabhakar,
On Mon, Jul 19, 2021 at 4:39 PM Lad Prabhakar
<[email protected]> wrote:
> Add CANFD binding documentation for Renesas RZ/G2L SoC.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Reviewed-by: Biju Das <[email protected]>
Thanks for your patch!
Reviewed-by: Geert Uytterhoeven <[email protected]>
Just some bikeshedding on the exact naming below ;-)
> --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> @@ -91,6 +92,59 @@ required:
> - channel0
> - channel1
>
> +if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - renesas,rzg2l-canfd
> +then:
> + properties:
> + interrupts:
> + items:
> + - description: CAN global error interrupt
> + - description: CAN receive FIFO interrupt
> + - description: CAN0 error interrupt
> + - description: CAN0 transmit interrupt
> + - description: CAN0 transmit/receive FIFO receive completion interrupt
> + - description: CAN1 error interrupt
> + - description: CAN1 transmit interrupt
> + - description: CAN1 transmit/receive FIFO receive completion interrupt
> +
> + interrupt-names:
> + items:
> + - const: g_error
> + - const: g_rx_fifo
> + - const: can0_error
s/error/err/?
> + - const: can0_tx
> + - const: can0_tx_rx_fifo_receive_completion
> + - const: can1_error
> + - const: can1_tx
> + - const: can1_tx_rx_fifo_receive_completion
s/receive/rx/?
Some are also a bit long to type.
Perhaps use naming closer to the User's Manual?
INTRCANGERR => g_err
INTRCANGRECC => g_recc
INTRCAN0ERR => ch0_err
INTRCAN0REC => ch0_rec
INTRCAN0TRX => ch0_trx
INTRCAN1ERR => ch1_err
INTRCAN1REC => ch1_rec
INTRCAN1TRX => ch1_trx
These do not have "_int" suffixes...
> +
> + resets:
> + items:
> + - description: CANFD_RSTP_N
> + - description: CANFD_RSTC_N
> +
> + required:
> + - interrupt-names
> +else:
> + properties:
> + interrupts:
> + items:
> + - description: Channel interrupt
> + - description: Global interrupt
> +
> + interrupt-names:
> + items:
> + - const: ch_int
> + - const: g_int
... and these do have "_int" suffixes.
> +
> + resets:
> + items:
> + - description: CANFD reset
> +
> unevaluatedProperties: false
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
Hi Lad,
On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar wrote:
> Add CANFD binding documentation for Renesas RZ/G2L SoC.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Reviewed-by: Biju Das <[email protected]>
> ---
> .../bindings/net/can/renesas,rcar-canfd.yaml | 66 +++++++++++++++++--
> 1 file changed, 60 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> index 0b33ba9ccb47..4fb6dd370904 100644
> --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> @@ -30,13 +30,15 @@ properties:
> - renesas,r8a77995-canfd # R-Car D3
> - const: renesas,rcar-gen3-canfd # R-Car Gen3 and RZ/G2
>
> + - items:
> + - enum:
> + - renesas,r9a07g044-canfd # RZ/G2{L,LC}
> + - const: renesas,rzg2l-canfd # RZ/G2L family
> +
> reg:
> maxItems: 1
>
> - interrupts:
> - items:
> - - description: Channel interrupt
> - - description: Global interrupt
> + interrupts: true
>
> clocks:
> maxItems: 3
> @@ -50,8 +52,7 @@ properties:
> power-domains:
> maxItems: 1
>
> - resets:
> - maxItems: 1
> + resets: true
>
> renesas,no-can-fd:
> $ref: /schemas/types.yaml#/definitions/flag
> @@ -91,6 +92,59 @@ required:
> - channel0
> - channel1
>
> +if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - renesas,rzg2l-canfd
> +then:
> + properties:
> + interrupts:
> + items:
> + - description: CAN global error interrupt
> + - description: CAN receive FIFO interrupt
> + - description: CAN0 error interrupt
> + - description: CAN0 transmit interrupt
> + - description: CAN0 transmit/receive FIFO receive completion interrupt
> + - description: CAN1 error interrupt
> + - description: CAN1 transmit interrupt
> + - description: CAN1 transmit/receive FIFO receive completion interrupt
> +
> + interrupt-names:
> + items:
> + - const: g_error
> + - const: g_rx_fifo
> + - const: can0_error
> + - const: can0_tx
> + - const: can0_tx_rx_fifo_receive_completion
> + - const: can1_error
> + - const: can1_tx
> + - const: can1_tx_rx_fifo_receive_completion
> +
> + resets:
> + items:
> + - description: CANFD_RSTP_N
> + - description: CANFD_RSTC_N
Do you know what the "P" and "C" stands for? It would be nice if the
description could tell us what the reset lines are used for.
I would prefer if you used these names (or shortened versions, for
example "rstp_n", "rstc_n") as "reset-names" and let the driver
reference the resets by name instead of by index.
regards
Philipp
On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar wrote:
> CANFD block on RZ/G2L SoC is almost identical to one found on
> R-Car Gen3 SoC's. On RZ/G2L SoC interrupt sources for each channel
> are split into different sources and the IP doesn't divide (1/2)
> CANFD clock within the IP.
>
> This patch adds compatible string for RZ/G2L family and registers
> the irq handlers required for CANFD operation. IRQ numbers are now
> fetched based on names instead of indices. For backward compatibility
> on non RZ/G2L SoC's we fallback reading based on indices.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Reviewed-by: Biju Das <[email protected]>
> ---
> drivers/net/can/rcar/rcar_canfd.c | 178 ++++++++++++++++++++++++------
> 1 file changed, 147 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index 311e6ca3bdc4..d4affc002fb3 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -37,9 +37,15 @@
[...]
> + if (gpriv->chip_id == RENESAS_RZG2L) {
> + gpriv->rstc1 = devm_reset_control_get_exclusive_by_index(&pdev->dev, 0);
> + if (IS_ERR(gpriv->rstc1)) {
> + dev_err(&pdev->dev, "failed to get reset index 0\n");
Please consider requesting the reset controls by name instead of by
index. See also my reply to the binding patch.
> + return PTR_ERR(gpriv->rstc1);
> + }
> +
> + err = reset_control_reset(gpriv->rstc1);
> + if (err)
> + return err;
I suggest to wait until after all resource requests have succeeded
before triggering the resets, i.e. first get all reset controls and
clocks, etc., and only then trigger resets, enable clocks, and so on.
That way there will be no spurious resets in case of probe deferrals.
regards
Philipp
Hi Prabhakar,
On Mon, Jul 19, 2021 at 4:39 PM Lad Prabhakar
<[email protected]> wrote:
> CANFD block on RZ/G2L SoC is almost identical to one found on
> R-Car Gen3 SoC's. On RZ/G2L SoC interrupt sources for each channel
> are split into different sources and the IP doesn't divide (1/2)
> CANFD clock within the IP.
>
> This patch adds compatible string for RZ/G2L family and registers
> the irq handlers required for CANFD operation. IRQ numbers are now
> fetched based on names instead of indices. For backward compatibility
> on non RZ/G2L SoC's we fallback reading based on indices.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Reviewed-by: Biju Das <[email protected]>
Thanks for your patch!
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -37,9 +37,15 @@
> #include <linux/bitmap.h>
> #include <linux/bitops.h>
> #include <linux/iopoll.h>
> +#include <linux/reset.h>
>
> #define RCANFD_DRV_NAME "rcar_canfd"
>
> +enum rcanfd_chip_id {
> + RENESAS_RCAR_GEN3 = 0,
> + RENESAS_RZG2L,
> +};
> +
> /* Global register bits */
>
> /* RSCFDnCFDGRMCFG */
> @@ -513,6 +519,9 @@ struct rcar_canfd_global {
> enum rcar_canfd_fcanclk fcan; /* CANFD or Ext clock */
> unsigned long channels_mask; /* Enabled channels mask */
> bool fdmode; /* CAN FD or Classical CAN only mode */
> + struct reset_control *rstc1; /* Pointer to reset source1 */
> + struct reset_control *rstc2; /* Pointer to reset source2 */
Are these comments helpful? IMHO they're stating the obvious.
> + enum rcanfd_chip_id chip_id;
> };
>
> /* CAN FD mode nominal rate constants */
> @@ -1577,6 +1586,45 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
> priv->can.clock.freq = fcan_freq;
> dev_info(&pdev->dev, "can_clk rate is %u\n", priv->can.clock.freq);
>
> + if (gpriv->chip_id == RENESAS_RZG2L) {
> + char *irq_name;
> + int err_irq;
> + int tx_irq;
> +
> + err_irq = platform_get_irq_byname(pdev, ch == 0 ? "can0_error" : "can1_error");
> + if (err_irq < 0) {
> + err = err_irq;
> + goto fail;
> + }
> +
> + tx_irq = platform_get_irq_byname(pdev, ch == 0 ? "can0_tx" : "can1_tx");
> + if (tx_irq < 0) {
> + err = tx_irq;
> + goto fail;
> + }
> +
> + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "canfd.chnerr%d", ch);
if (!irq_name) {
ret = -ENOMEM;
goto fail;
}
> + err = devm_request_irq(&pdev->dev, err_irq,
> + rcar_canfd_channel_interrupt, 0,
> + irq_name, gpriv);
> + if (err) {
> + dev_err(&pdev->dev, "devm_request_irq CH Err(%d) failed, error %d\n",
> + err_irq, err);
> + goto fail;
> + }
> + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "canfd.chntx%d", ch);
Likewise.
> + err = devm_request_irq(&pdev->dev, tx_irq,
> + rcar_canfd_channel_interrupt, 0,
> + irq_name, gpriv);
> + if (err) {
> + dev_err(&pdev->dev, "devm_request_irq Tx (%d) failed, error %d\n",
> + tx_irq, err);
> + goto fail;
> + }
> + }
> +
> if (gpriv->fdmode) {
> priv->can.bittiming_const = &rcar_canfd_nom_bittiming_const;
> priv->can.data_bittiming_const =
> @@ -1649,27 +1700,64 @@ static int rcar_canfd_probe(struct platform_device *pdev)
> if (of_child && of_device_is_available(of_child))
> channels_mask |= BIT(1); /* Channel 1 */
>
> - ch_irq = platform_get_irq(pdev, 0);
> - if (ch_irq < 0) {
> - err = ch_irq;
> - goto fail_dev;
> - }
> + if (chip_id == RENESAS_RCAR_GEN3) {
> + ch_irq = platform_get_irq_byname(pdev, "ch_int");
platform_get_irq_byname_optional()?
Unless you want to urge people to update their DTB.
> + if (ch_irq < 0) {
> + /* For backward compatibility get irq by index */
> + ch_irq = platform_get_irq(pdev, 0);
> + if (ch_irq < 0)
> + return ch_irq;
> + }
>
> - g_irq = platform_get_irq(pdev, 1);
> - if (g_irq < 0) {
> - err = g_irq;
> - goto fail_dev;
> + g_irq = platform_get_irq_byname(pdev, "g_int");
Likewise,
> + if (g_irq < 0) {
> + /* For backward compatibility get irq by index */
> + g_irq = platform_get_irq(pdev, 1);
> + if (g_irq < 0)
> + return g_irq;
> + }
> + } else {
> + g_irq = platform_get_irq_byname(pdev, "g_error");
> + if (g_irq < 0)
> + return g_irq;
> +
> + g_rx_irq = platform_get_irq_byname(pdev, "g_rx_fifo");
> + if (g_rx_irq < 0)
> + return g_rx_irq;
> }
>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On Mon, Jul 19, 2021 at 4:40 PM Lad Prabhakar
<[email protected]> wrote:
> Add entry for fixed core clock P0_DIV2 and assign LAST_DT_CORE_CLK
> to R9A07G044_CLK_P0_DIV2.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Reviewed-by: Biju Das <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
i.e. will queue in renesas-clk-for-v5.15.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On Mon, Jul 19, 2021 at 4:39 PM Lad Prabhakar
<[email protected]> wrote:
> Add P0_DIV2 core clock required for CANFD module. CANFD core clock is
> sourced from P0_DIV2 referenced from HW manual Rev.0.50.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Reviewed-by: Biju Das <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
i.e. will queue in renesas-r9a07g044-dt-binding-defs, to be shared by
renesas-clk-for-v5.15 and renesas-devel for v5.15.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
Hi Philipp,
Thank you for the review.
On Tue, Jul 20, 2021 at 11:22 AM Philipp Zabel <[email protected]> wrote:
>
> Hi Lad,
>
> On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar wrote:
> > Add CANFD binding documentation for Renesas RZ/G2L SoC.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > Reviewed-by: Biju Das <[email protected]>
> > ---
> > .../bindings/net/can/renesas,rcar-canfd.yaml | 66 +++++++++++++++++--
> > 1 file changed, 60 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > index 0b33ba9ccb47..4fb6dd370904 100644
> > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > @@ -30,13 +30,15 @@ properties:
> > - renesas,r8a77995-canfd # R-Car D3
> > - const: renesas,rcar-gen3-canfd # R-Car Gen3 and RZ/G2
> >
> > + - items:
> > + - enum:
> > + - renesas,r9a07g044-canfd # RZ/G2{L,LC}
> > + - const: renesas,rzg2l-canfd # RZ/G2L family
> > +
> > reg:
> > maxItems: 1
> >
> > - interrupts:
> > - items:
> > - - description: Channel interrupt
> > - - description: Global interrupt
> > + interrupts: true
> >
> > clocks:
> > maxItems: 3
> > @@ -50,8 +52,7 @@ properties:
> > power-domains:
> > maxItems: 1
> >
> > - resets:
> > - maxItems: 1
> > + resets: true
> >
> > renesas,no-can-fd:
> > $ref: /schemas/types.yaml#/definitions/flag
> > @@ -91,6 +92,59 @@ required:
> > - channel0
> > - channel1
> >
> > +if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - renesas,rzg2l-canfd
> > +then:
> > + properties:
> > + interrupts:
> > + items:
> > + - description: CAN global error interrupt
> > + - description: CAN receive FIFO interrupt
> > + - description: CAN0 error interrupt
> > + - description: CAN0 transmit interrupt
> > + - description: CAN0 transmit/receive FIFO receive completion interrupt
> > + - description: CAN1 error interrupt
> > + - description: CAN1 transmit interrupt
> > + - description: CAN1 transmit/receive FIFO receive completion interrupt
> > +
> > + interrupt-names:
> > + items:
> > + - const: g_error
> > + - const: g_rx_fifo
> > + - const: can0_error
> > + - const: can0_tx
> > + - const: can0_tx_rx_fifo_receive_completion
> > + - const: can1_error
> > + - const: can1_tx
> > + - const: can1_tx_rx_fifo_receive_completion
> > +
> > + resets:
> > + items:
> > + - description: CANFD_RSTP_N
> > + - description: CANFD_RSTC_N
>
> Do you know what the "P" and "C" stands for? It would be nice if the
> description could tell us what the reset lines are used for.
>
unfortunately the HW manual does not mention anything about "P" and "C" :(
> I would prefer if you used these names (or shortened versions, for
> example "rstp_n", "rstc_n") as "reset-names" and let the driver
> reference the resets by name instead of by index.
>
OK will do that and maxItems:2 for resets.
@Geert, for R-Car Gen3 does "canfd_rst" (as it's a module reset)
sounds good for reset-names? Or do you have any other suggestions?
Cheers,
Prabhakar
Hi Geert,
Thank you for the review.
On Tue, Jul 20, 2021 at 11:21 AM Geert Uytterhoeven
<[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Mon, Jul 19, 2021 at 4:39 PM Lad Prabhakar
> <[email protected]> wrote:
> > Add CANFD binding documentation for Renesas RZ/G2L SoC.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > Reviewed-by: Biju Das <[email protected]>
>
> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven <[email protected]>
> Just some bikeshedding on the exact naming below ;-)
>
> > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > @@ -91,6 +92,59 @@ required:
> > - channel0
> > - channel1
> >
> > +if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - renesas,rzg2l-canfd
> > +then:
> > + properties:
> > + interrupts:
> > + items:
> > + - description: CAN global error interrupt
> > + - description: CAN receive FIFO interrupt
> > + - description: CAN0 error interrupt
> > + - description: CAN0 transmit interrupt
> > + - description: CAN0 transmit/receive FIFO receive completion interrupt
> > + - description: CAN1 error interrupt
> > + - description: CAN1 transmit interrupt
> > + - description: CAN1 transmit/receive FIFO receive completion interrupt
> > +
> > + interrupt-names:
> > + items:
> > + - const: g_error
> > + - const: g_rx_fifo
> > + - const: can0_error
>
> s/error/err/?
>
> > + - const: can0_tx
> > + - const: can0_tx_rx_fifo_receive_completion
> > + - const: can1_error
> > + - const: can1_tx
> > + - const: can1_tx_rx_fifo_receive_completion
>
> s/receive/rx/?
>
> Some are also a bit long to type.
> Perhaps use naming closer to the User's Manual?
>
> INTRCANGERR => g_err
> INTRCANGRECC => g_recc
> INTRCAN0ERR => ch0_err
> INTRCAN0REC => ch0_rec
> INTRCAN0TRX => ch0_trx
> INTRCAN1ERR => ch1_err
> INTRCAN1REC => ch1_rec
> INTRCAN1TRX => ch1_trx
>
> These do not have "_int" suffixes...
>
Agreed thanks for the input.
> > +
> > + resets:
> > + items:
> > + - description: CANFD_RSTP_N
> > + - description: CANFD_RSTC_N
> > +
> > + required:
> > + - interrupt-names
> > +else:
> > + properties:
> > + interrupts:
> > + items:
> > + - description: Channel interrupt
> > + - description: Global interrupt
> > +
> > + interrupt-names:
> > + items:
> > + - const: ch_int
> > + - const: g_int
>
> ... and these do have "_int" suffixes.
>
indeed
Cheers,
Prabhakar
> > +
> > + resets:
> > + items:
> > + - description: CANFD reset
> > +
> > unevaluatedProperties: false
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> 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
Hi Philipp,
Thank you for the review.
On Tue, Jul 20, 2021 at 11:23 AM Philipp Zabel <[email protected]> wrote:
>
> On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar wrote:
> > CANFD block on RZ/G2L SoC is almost identical to one found on
> > R-Car Gen3 SoC's. On RZ/G2L SoC interrupt sources for each channel
> > are split into different sources and the IP doesn't divide (1/2)
> > CANFD clock within the IP.
> >
> > This patch adds compatible string for RZ/G2L family and registers
> > the irq handlers required for CANFD operation. IRQ numbers are now
> > fetched based on names instead of indices. For backward compatibility
> > on non RZ/G2L SoC's we fallback reading based on indices.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > Reviewed-by: Biju Das <[email protected]>
> > ---
> > drivers/net/can/rcar/rcar_canfd.c | 178 ++++++++++++++++++++++++------
> > 1 file changed, 147 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> > index 311e6ca3bdc4..d4affc002fb3 100644
> > --- a/drivers/net/can/rcar/rcar_canfd.c
> > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > @@ -37,9 +37,15 @@
> [...]
> > + if (gpriv->chip_id == RENESAS_RZG2L) {
> > + gpriv->rstc1 = devm_reset_control_get_exclusive_by_index(&pdev->dev, 0);
> > + if (IS_ERR(gpriv->rstc1)) {
> > + dev_err(&pdev->dev, "failed to get reset index 0\n");
>
> Please consider requesting the reset controls by name instead of by
> index. See also my reply to the binding patch.
>
Will do.
> > + return PTR_ERR(gpriv->rstc1);
> > + }
> > +
> > + err = reset_control_reset(gpriv->rstc1);
> > + if (err)
> > + return err;
>
> I suggest to wait until after all resource requests have succeeded
> before triggering the resets, i.e. first get all reset controls and
> clocks, etc., and only then trigger resets, enable clocks, and so on.
>
> That way there will be no spurious resets in case of probe deferrals.
>
Agreed, will update the code.
Cheers,
Prabhakar
> regards
> Philipp
Hi Prabhakar,
On Tue, Jul 20, 2021 at 4:31 PM Lad, Prabhakar
<[email protected]> wrote:
> On Tue, Jul 20, 2021 at 11:22 AM Philipp Zabel <[email protected]> wrote:
> > On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar wrote:
> > > Add CANFD binding documentation for Renesas RZ/G2L SoC.
> > >
> > > Signed-off-by: Lad Prabhakar <[email protected]>
> > > Reviewed-by: Biju Das <[email protected]>
> > > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > > + resets:
> > > + items:
> > > + - description: CANFD_RSTP_N
> > > + - description: CANFD_RSTC_N
> >
> > Do you know what the "P" and "C" stands for? It would be nice if the
> > description could tell us what the reset lines are used for.
> >
> unfortunately the HW manual does not mention anything about "P" and "C" :(
>
> > I would prefer if you used these names (or shortened versions, for
> > example "rstp_n", "rstc_n") as "reset-names" and let the driver
> > reference the resets by name instead of by index.
> >
> OK will do that and maxItems:2 for resets.
>
> @Geert, for R-Car Gen3 does "canfd_rst" (as it's a module reset)
> sounds good for reset-names? Or do you have any other suggestions?
I wouldn't bother with reset-names on R-Car, as there is only a
single reset.
BTW, does there exist a generally-accepted reset-equivalent of "fck"
("Functional ClocK")?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
Hi Geert,
Thank you for the review.
On Tue, Jul 20, 2021 at 11:31 AM Geert Uytterhoeven
<[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Mon, Jul 19, 2021 at 4:39 PM Lad Prabhakar
> <[email protected]> wrote:
> > CANFD block on RZ/G2L SoC is almost identical to one found on
> > R-Car Gen3 SoC's. On RZ/G2L SoC interrupt sources for each channel
> > are split into different sources and the IP doesn't divide (1/2)
> > CANFD clock within the IP.
> >
> > This patch adds compatible string for RZ/G2L family and registers
> > the irq handlers required for CANFD operation. IRQ numbers are now
> > fetched based on names instead of indices. For backward compatibility
> > on non RZ/G2L SoC's we fallback reading based on indices.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > Reviewed-by: Biju Das <[email protected]>
>
> Thanks for your patch!
>
> > --- a/drivers/net/can/rcar/rcar_canfd.c
> > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > @@ -37,9 +37,15 @@
> > #include <linux/bitmap.h>
> > #include <linux/bitops.h>
> > #include <linux/iopoll.h>
> > +#include <linux/reset.h>
> >
> > #define RCANFD_DRV_NAME "rcar_canfd"
> >
> > +enum rcanfd_chip_id {
> > + RENESAS_RCAR_GEN3 = 0,
> > + RENESAS_RZG2L,
> > +};
> > +
> > /* Global register bits */
> >
> > /* RSCFDnCFDGRMCFG */
> > @@ -513,6 +519,9 @@ struct rcar_canfd_global {
> > enum rcar_canfd_fcanclk fcan; /* CANFD or Ext clock */
> > unsigned long channels_mask; /* Enabled channels mask */
> > bool fdmode; /* CAN FD or Classical CAN only mode */
> > + struct reset_control *rstc1; /* Pointer to reset source1 */
> > + struct reset_control *rstc2; /* Pointer to reset source2 */
>
> Are these comments helpful? IMHO they're stating the obvious.
>
No :D will drop those.
> > + enum rcanfd_chip_id chip_id;
> > };
> >
> > /* CAN FD mode nominal rate constants */
> > @@ -1577,6 +1586,45 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
> > priv->can.clock.freq = fcan_freq;
> > dev_info(&pdev->dev, "can_clk rate is %u\n", priv->can.clock.freq);
> >
> > + if (gpriv->chip_id == RENESAS_RZG2L) {
> > + char *irq_name;
> > + int err_irq;
> > + int tx_irq;
> > +
> > + err_irq = platform_get_irq_byname(pdev, ch == 0 ? "can0_error" : "can1_error");
> > + if (err_irq < 0) {
> > + err = err_irq;
> > + goto fail;
> > + }
> > +
> > + tx_irq = platform_get_irq_byname(pdev, ch == 0 ? "can0_tx" : "can1_tx");
> > + if (tx_irq < 0) {
> > + err = tx_irq;
> > + goto fail;
> > + }
> > +
> > + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> > + "canfd.chnerr%d", ch);
>
> if (!irq_name) {
> ret = -ENOMEM;
> goto fail;
> }
>
> > + err = devm_request_irq(&pdev->dev, err_irq,
> > + rcar_canfd_channel_interrupt, 0,
> > + irq_name, gpriv);
> > + if (err) {
> > + dev_err(&pdev->dev, "devm_request_irq CH Err(%d) failed, error %d\n",
> > + err_irq, err);
> > + goto fail;
> > + }
> > + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> > + "canfd.chntx%d", ch);
>
> Likewise.
>
> > + err = devm_request_irq(&pdev->dev, tx_irq,
> > + rcar_canfd_channel_interrupt, 0,
> > + irq_name, gpriv);
> > + if (err) {
> > + dev_err(&pdev->dev, "devm_request_irq Tx (%d) failed, error %d\n",
> > + tx_irq, err);
> > + goto fail;
> > + }
> > + }
> > +
> > if (gpriv->fdmode) {
> > priv->can.bittiming_const = &rcar_canfd_nom_bittiming_const;
> > priv->can.data_bittiming_const =
>
> > @@ -1649,27 +1700,64 @@ static int rcar_canfd_probe(struct platform_device *pdev)
> > if (of_child && of_device_is_available(of_child))
> > channels_mask |= BIT(1); /* Channel 1 */
> >
> > - ch_irq = platform_get_irq(pdev, 0);
> > - if (ch_irq < 0) {
> > - err = ch_irq;
> > - goto fail_dev;
> > - }
> > + if (chip_id == RENESAS_RCAR_GEN3) {
> > + ch_irq = platform_get_irq_byname(pdev, "ch_int");
>
> platform_get_irq_byname_optional()?
> Unless you want to urge people to update their DTB.
>
Good point will change it to platform_get_irq_byname_optional().
> > + if (ch_irq < 0) {
> > + /* For backward compatibility get irq by index */
> > + ch_irq = platform_get_irq(pdev, 0);
> > + if (ch_irq < 0)
> > + return ch_irq;
> > + }
> >
> > - g_irq = platform_get_irq(pdev, 1);
> > - if (g_irq < 0) {
> > - err = g_irq;
> > - goto fail_dev;
> > + g_irq = platform_get_irq_byname(pdev, "g_int");
>
> Likewise,
>
agreed
Cheers,
Prabhakar
> > + if (g_irq < 0) {
> > + /* For backward compatibility get irq by index */
> > + g_irq = platform_get_irq(pdev, 1);
> > + if (g_irq < 0)
> > + return g_irq;
> > + }
> > + } else {
> > + g_irq = platform_get_irq_byname(pdev, "g_error");
> > + if (g_irq < 0)
> > + return g_irq;
> > +
> > + g_rx_irq = platform_get_irq_byname(pdev, "g_rx_fifo");
> > + if (g_rx_irq < 0)
> > + return g_rx_irq;
> > }
> >
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> 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
Hi Geert,
On Tue, Jul 20, 2021 at 4:11 PM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Tue, Jul 20, 2021 at 4:31 PM Lad, Prabhakar
> <[email protected]> wrote:
> > On Tue, Jul 20, 2021 at 11:22 AM Philipp Zabel <[email protected]> wrote:
> > > On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar wrote:
> > > > Add CANFD binding documentation for Renesas RZ/G2L SoC.
> > > >
> > > > Signed-off-by: Lad Prabhakar <[email protected]>
> > > > Reviewed-by: Biju Das <[email protected]>
>
> > > > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > > > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
>
> > > > + resets:
> > > > + items:
> > > > + - description: CANFD_RSTP_N
> > > > + - description: CANFD_RSTC_N
> > >
> > > Do you know what the "P" and "C" stands for? It would be nice if the
> > > description could tell us what the reset lines are used for.
> > >
> > unfortunately the HW manual does not mention anything about "P" and "C" :(
> >
> > > I would prefer if you used these names (or shortened versions, for
> > > example "rstp_n", "rstc_n") as "reset-names" and let the driver
> > > reference the resets by name instead of by index.
> > >
> > OK will do that and maxItems:2 for resets.
> >
> > @Geert, for R-Car Gen3 does "canfd_rst" (as it's a module reset)
> > sounds good for reset-names? Or do you have any other suggestions?
>
> I wouldn't bother with reset-names on R-Car, as there is only a
> single reset.
>
OK will keep "description: CANFD reset" for R-Car as done in the
current patch and just add reset-names only for RZ/G2L SoC.
> BTW, does there exist a generally-accepted reset-equivalent of "fck"
> ("Functional ClocK")?
>
None that I am aware of (Couple of binding docs have "rst"), but maybe
Philipp could have some suggestions.
Cheers,
Prabhakar
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> 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
Hi Prabhakar,
On Tue, 2021-07-20 at 15:31 +0100, Lad, Prabhakar wrote:
> Hi Philipp,
>
> Thank you for the review.
>
> On Tue, Jul 20, 2021 at 11:22 AM Philipp Zabel <[email protected]> wrote:
> > Hi Lad,
Sorry I mixed up your name.
> > On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar wrote:
> > > Add CANFD binding documentation for Renesas RZ/G2L SoC.
> > >
> > > Signed-off-by: Lad Prabhakar <[email protected]>
> > > Reviewed-by: Biju Das <[email protected]>
> > > ---
> > > .../bindings/net/can/renesas,rcar-canfd.yaml | 66 +++++++++++++++++--
> > > 1 file changed, 60 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > > index 0b33ba9ccb47..4fb6dd370904 100644
> > > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> > > @@ -30,13 +30,15 @@ properties:
> > > - renesas,r8a77995-canfd # R-Car D3
> > > - const: renesas,rcar-gen3-canfd # R-Car Gen3 and RZ/G2
> > >
> > > + - items:
> > > + - enum:
> > > + - renesas,r9a07g044-canfd # RZ/G2{L,LC}
> > > + - const: renesas,rzg2l-canfd # RZ/G2L family
> > > +
> > > reg:
> > > maxItems: 1
> > >
> > > - interrupts:
> > > - items:
> > > - - description: Channel interrupt
> > > - - description: Global interrupt
> > > + interrupts: true
> > >
> > > clocks:
> > > maxItems: 3
> > > @@ -50,8 +52,7 @@ properties:
> > > power-domains:
> > > maxItems: 1
> > >
> > > - resets:
> > > - maxItems: 1
> > > + resets: true
> > >
> > > renesas,no-can-fd:
> > > $ref: /schemas/types.yaml#/definitions/flag
> > > @@ -91,6 +92,59 @@ required:
> > > - channel0
> > > - channel1
> > >
> > > +if:
> > > + properties:
> > > + compatible:
> > > + contains:
> > > + enum:
> > > + - renesas,rzg2l-canfd
> > > +then:
> > > + properties:
> > > + interrupts:
> > > + items:
> > > + - description: CAN global error interrupt
> > > + - description: CAN receive FIFO interrupt
> > > + - description: CAN0 error interrupt
> > > + - description: CAN0 transmit interrupt
> > > + - description: CAN0 transmit/receive FIFO receive completion interrupt
> > > + - description: CAN1 error interrupt
> > > + - description: CAN1 transmit interrupt
> > > + - description: CAN1 transmit/receive FIFO receive completion interrupt
> > > +
> > > + interrupt-names:
> > > + items:
> > > + - const: g_error
> > > + - const: g_rx_fifo
> > > + - const: can0_error
> > > + - const: can0_tx
> > > + - const: can0_tx_rx_fifo_receive_completion
> > > + - const: can1_error
> > > + - const: can1_tx
> > > + - const: can1_tx_rx_fifo_receive_completion
> > > +
> > > + resets:
> > > + items:
> > > + - description: CANFD_RSTP_N
> > > + - description: CANFD_RSTC_N
> >
> > Do you know what the "P" and "C" stands for? It would be nice if the
> > description could tell us what the reset lines are used for.
> >
> unfortunately the HW manual does not mention anything about "P" and "C" :(
Yes, unfortunately this is all too common.
> > I would prefer if you used these names (or shortened versions, for
> > example "rstp_n", "rstc_n") as "reset-names" and let the driver
> > reference the resets by name instead of by index.
> >
> OK will do that and maxItems:2 for resets.
>
> @Geert, for R-Car Gen3 does "canfd_rst" (as it's a module reset)
> sounds good for reset-names? Or do you have any other suggestions?
I agree with Geert here. Assuming no second reset will be discovered for
R-Car Gen3 later, there is no need to invent a name.
regards
Philipp
Hi Geert,
On Tue, 2021-07-20 at 17:11 +0200, Geert Uytterhoeven wrote:
[...]
> I wouldn't bother with reset-names on R-Car, as there is only a
> single reset.
>
> BTW, does there exist a generally-accepted reset-equivalent of "fck"
> ("Functional ClocK")?
Not really. There is "rst", which seems to be slightly more popular than
"reset". Some bindings use "core" to differentiate between the
functional reset and peripheral resets like bus or phy resets.
Ideally the reset-names would match the names of the reset inputs in the
IP core documentation (not the global names of the reset signals in the
SoC documentation). But more often than not they are not known.
regards
Philipp