| Message ID | 20220702140130.218409-7-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 |
On 02.07.2022 15:01:30, Biju Das wrote: > The SJA1000 CAN controller on RZ/N1 SoC has some differences compared > to others like it has no clock divider register (CDR) support and it has > no HW loopback(HW doesn't see tx messages on rx). > > This patch adds support for RZ/N1 SJA1000 CAN Controller. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > drivers/net/can/sja1000/sja1000_platform.c | 34 ++++++++++++++++++---- > 1 file changed, 29 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c > index 5f3d362e0da5..8e63af76a013 100644 > --- a/drivers/net/can/sja1000/sja1000_platform.c > +++ b/drivers/net/can/sja1000/sja1000_platform.c > @@ -14,6 +14,7 @@ > #include <linux/irq.h> > #include <linux/can/dev.h> > #include <linux/can/platform/sja1000.h> > +#include <linux/clk.h> > #include <linux/io.h> > #include <linux/of.h> > #include <linux/of_device.h> > @@ -103,6 +104,11 @@ static void sp_technologic_init(struct sja1000_priv *priv, struct device_node *o > spin_lock_init(&tp->io_lock); > } > > +static void sp_rzn1_init(struct sja1000_priv *priv, struct device_node *of) > +{ > + priv->flags = SJA1000_NO_CDR_REG_QUIRK | SJA1000_NO_HW_LOOPBACK_QUIRK; > +} > + > static void sp_populate(struct sja1000_priv *priv, > struct sja1000_platform_data *pdata, > unsigned long resource_mem_flags) > @@ -153,11 +159,13 @@ static void sp_populate_of(struct sja1000_priv *priv, struct device_node *of) > priv->write_reg = sp_write_reg8; > } > > - err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop); > - if (!err) > - priv->can.clock.freq = prop / 2; > - else > - priv->can.clock.freq = SP_CAN_CLOCK; /* default */ > + if (!priv->can.clock.freq) { > + err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop); > + if (!err) > + priv->can.clock.freq = prop / 2; > + else > + priv->can.clock.freq = SP_CAN_CLOCK; /* default */ > + } > > err = of_property_read_u32(of, "nxp,tx-output-mode", &prop); > if (!err) > @@ -192,8 +200,13 @@ static struct sja1000_of_data technologic_data = { > .init = sp_technologic_init, > }; > > +static struct sja1000_of_data renesas_data = { > + .init = sp_rzn1_init, > +}; > + > static const struct of_device_id sp_of_table[] = { > { .compatible = "nxp,sja1000", .data = NULL, }, > + { .compatible = "renesas,rzn1-sja1000", .data = &renesas_data, }, > { .compatible = "technologic,sja1000", .data = &technologic_data, }, > { /* sentinel */ }, > }; > @@ -210,6 +223,7 @@ static int sp_probe(struct platform_device *pdev) > struct device_node *of = pdev->dev.of_node; > const struct sja1000_of_data *of_data = NULL; > size_t priv_sz = 0; > + struct clk *clk; > > pdata = dev_get_platdata(&pdev->dev); > if (!pdata && !of) { > @@ -262,6 +276,16 @@ static int sp_probe(struct platform_device *pdev) > priv->reg_base = addr; > > if (of) { > + clk = devm_clk_get_optional(&pdev->dev, "can_clk"); > + if (IS_ERR(clk)) > + return dev_err_probe(&pdev->dev, PTR_ERR(clk), "no CAN clk"); Please take care of releasing all acquired resources. > + > + if (clk) { > + priv->can.clock.freq = clk_get_rate(clk) / 2; > + if (!priv->can.clock.freq) > + return dev_err_probe(&pdev->dev, -EINVAL, "Zero CAN clk rate"); same here. Marc
On 02.07.2022 15:01:30, Biju Das wrote: > The SJA1000 CAN controller on RZ/N1 SoC has some differences compared > to others like it has no clock divider register (CDR) support and it has > no HW loopback(HW doesn't see tx messages on rx). > > This patch adds support for RZ/N1 SJA1000 CAN Controller. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > drivers/net/can/sja1000/sja1000_platform.c | 34 ++++++++++++++++++---- > 1 file changed, 29 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c > index 5f3d362e0da5..8e63af76a013 100644 > --- a/drivers/net/can/sja1000/sja1000_platform.c > +++ b/drivers/net/can/sja1000/sja1000_platform.c [...] > @@ -262,6 +276,16 @@ static int sp_probe(struct platform_device *pdev) > priv->reg_base = addr; > > if (of) { > + clk = devm_clk_get_optional(&pdev->dev, "can_clk"); > + if (IS_ERR(clk)) > + return dev_err_probe(&pdev->dev, PTR_ERR(clk), "no CAN clk"); > + > + if (clk) { > + priv->can.clock.freq = clk_get_rate(clk) / 2; > + if (!priv->can.clock.freq) > + return dev_err_probe(&pdev->dev, -EINVAL, "Zero CAN clk rate"); > + } There's no clk_prepare_enable in the driver. You might go the quick and dirty way an enable the clock right here. IIRC there's a new convenience function to get and enable a clock, managed bei devm. Uwe (Cc'ed) can point you in the right direction. Marc
Hi Marc and Uwe, > Subject: Re: [PATCH 6/6] can: sja1000: Add support for RZ/N1 SJA1000 CAN > Controller > > On 02.07.2022 15:01:30, Biju Das wrote: > > The SJA1000 CAN controller on RZ/N1 SoC has some differences compared > > to others like it has no clock divider register (CDR) support and it > > has no HW loopback(HW doesn't see tx messages on rx). > > > > This patch adds support for RZ/N1 SJA1000 CAN Controller. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > drivers/net/can/sja1000/sja1000_platform.c | 34 > > ++++++++++++++++++---- > > 1 file changed, 29 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/can/sja1000/sja1000_platform.c > > b/drivers/net/can/sja1000/sja1000_platform.c > > index 5f3d362e0da5..8e63af76a013 100644 > > --- a/drivers/net/can/sja1000/sja1000_platform.c > > +++ b/drivers/net/can/sja1000/sja1000_platform.c > [...] > > @@ -262,6 +276,16 @@ static int sp_probe(struct platform_device *pdev) > > priv->reg_base = addr; > > > > if (of) { > > + clk = devm_clk_get_optional(&pdev->dev, "can_clk"); > > + if (IS_ERR(clk)) > > + return dev_err_probe(&pdev->dev, PTR_ERR(clk), "no CAN > clk"); > > + > > + if (clk) { > > + priv->can.clock.freq = clk_get_rate(clk) / 2; > > + if (!priv->can.clock.freq) > > + return dev_err_probe(&pdev->dev, -EINVAL, "Zero > CAN clk rate"); > > + } > > There's no clk_prepare_enable in the driver. You might go the quick and > dirty way an enable the clock right here. IIRC there's a new convenience > function to get and enable a clock, managed bei devm. Uwe (Cc'ed) can > point you in the right direction. + clk As per the patch history devm version for clk_prepare_enable is rejected[1], so the individual drivers implemented the same using devm_add_action_or_reset [2]. So shall I implement devm version here as well? [1]https://lkml.iu.edu/hypermail/linux/kernel/2103.1/01556.html [2] https://elixir.bootlin.com/linux/v5.19-rc4/source/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c#L266 Cheers, Biju
On Sun, Jul 03, 2022 at 07:15:16AM +0000, Biju Das wrote: > Hi Marc and Uwe, > > > Subject: Re: [PATCH 6/6] can: sja1000: Add support for RZ/N1 SJA1000 CAN > > Controller > > > > On 02.07.2022 15:01:30, Biju Das wrote: > > > The SJA1000 CAN controller on RZ/N1 SoC has some differences compared > > > to others like it has no clock divider register (CDR) support and it > > > has no HW loopback(HW doesn't see tx messages on rx). > > > > > > This patch adds support for RZ/N1 SJA1000 CAN Controller. > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > --- > > > drivers/net/can/sja1000/sja1000_platform.c | 34 > > > ++++++++++++++++++---- > > > 1 file changed, 29 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/net/can/sja1000/sja1000_platform.c > > > b/drivers/net/can/sja1000/sja1000_platform.c > > > index 5f3d362e0da5..8e63af76a013 100644 > > > --- a/drivers/net/can/sja1000/sja1000_platform.c > > > +++ b/drivers/net/can/sja1000/sja1000_platform.c > > [...] > > > @@ -262,6 +276,16 @@ static int sp_probe(struct platform_device *pdev) > > > priv->reg_base = addr; > > > > > > if (of) { > > > + clk = devm_clk_get_optional(&pdev->dev, "can_clk"); > > > + if (IS_ERR(clk)) > > > + return dev_err_probe(&pdev->dev, PTR_ERR(clk), "no CAN > > clk"); > > > + > > > + if (clk) { > > > + priv->can.clock.freq = clk_get_rate(clk) / 2; > > > + if (!priv->can.clock.freq) > > > + return dev_err_probe(&pdev->dev, -EINVAL, "Zero > > CAN clk rate"); > > > + } > > > > There's no clk_prepare_enable in the driver. You might go the quick and > > dirty way an enable the clock right here. IIRC there's a new convenience > > function to get and enable a clock, managed bei devm. Uwe (Cc'ed) can > > point you in the right direction. > > + clk > > As per the patch history devm version for clk_prepare_enable is rejected[1], so the individual drivers implemented the same using devm_add_action_or_reset [2]. > So shall I implement devm version here as well? You want to make use of 7ef9651e9792b08eb310c6beb202cbc947f43cab (which is currently in next). If you cherry-pick this to an older kernel version, make sure to also pick 8b3d743fc9e2542822826890b482afabf0e7522a. Best regards Uwe
Hi Uwe, Thanks for the feedback. > Subject: Re: [PATCH 6/6] can: sja1000: Add support for RZ/N1 SJA1000 CAN > Controller > > On Sun, Jul 03, 2022 at 07:15:16AM +0000, Biju Das wrote: > > Hi Marc and Uwe, > > > > > Subject: Re: [PATCH 6/6] can: sja1000: Add support for RZ/N1 SJA1000 > > > CAN Controller > > > > > > On 02.07.2022 15:01:30, Biju Das wrote: > > > > The SJA1000 CAN controller on RZ/N1 SoC has some differences > > > > compared to others like it has no clock divider register (CDR) > > > > support and it has no HW loopback(HW doesn't see tx messages on > rx). > > > > > > > > This patch adds support for RZ/N1 SJA1000 CAN Controller. > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > --- > > > > drivers/net/can/sja1000/sja1000_platform.c | 34 > > > > ++++++++++++++++++---- > > > > 1 file changed, 29 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/net/can/sja1000/sja1000_platform.c > > > > b/drivers/net/can/sja1000/sja1000_platform.c > > > > index 5f3d362e0da5..8e63af76a013 100644 > > > > --- a/drivers/net/can/sja1000/sja1000_platform.c > > > > +++ b/drivers/net/can/sja1000/sja1000_platform.c > > > [...] > > > > @@ -262,6 +276,16 @@ static int sp_probe(struct platform_device > *pdev) > > > > priv->reg_base = addr; > > > > > > > > if (of) { > > > > + clk = devm_clk_get_optional(&pdev->dev, "can_clk"); > > > > + if (IS_ERR(clk)) > > > > + return dev_err_probe(&pdev->dev, PTR_ERR(clk), > "no CAN > > > clk"); > > > > + > > > > + if (clk) { > > > > + priv->can.clock.freq = clk_get_rate(clk) / 2; > > > > + if (!priv->can.clock.freq) > > > > + return dev_err_probe(&pdev->dev, -EINVAL, > "Zero > > > CAN clk rate"); > > > > + } > > > > > > There's no clk_prepare_enable in the driver. You might go the quick > > > and dirty way an enable the clock right here. IIRC there's a new > > > convenience function to get and enable a clock, managed bei devm. > > > Uwe (Cc'ed) can point you in the right direction. > > > > + clk > > > > As per the patch history devm version for clk_prepare_enable is > rejected[1], so the individual drivers implemented the same using > devm_add_action_or_reset [2]. > > So shall I implement devm version here as well? > > You want to make use of 7ef9651e9792b08eb310c6beb202cbc947f43cab (which > is currently in next). If you cherry-pick this to an older kernel > version, make sure to also pick > 8b3d743fc9e2542822826890b482afabf0e7522a. Ok will use "devm_clk_get_optional_enabled" and send V2. Cheers, Biju
diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c index 5f3d362e0da5..8e63af76a013 100644 --- a/drivers/net/can/sja1000/sja1000_platform.c +++ b/drivers/net/can/sja1000/sja1000_platform.c @@ -14,6 +14,7 @@ #include <linux/irq.h> #include <linux/can/dev.h> #include <linux/can/platform/sja1000.h> +#include <linux/clk.h> #include <linux/io.h> #include <linux/of.h> #include <linux/of_device.h> @@ -103,6 +104,11 @@ static void sp_technologic_init(struct sja1000_priv *priv, struct device_node *o spin_lock_init(&tp->io_lock); } +static void sp_rzn1_init(struct sja1000_priv *priv, struct device_node *of) +{ + priv->flags = SJA1000_NO_CDR_REG_QUIRK | SJA1000_NO_HW_LOOPBACK_QUIRK; +} + static void sp_populate(struct sja1000_priv *priv, struct sja1000_platform_data *pdata, unsigned long resource_mem_flags) @@ -153,11 +159,13 @@ static void sp_populate_of(struct sja1000_priv *priv, struct device_node *of) priv->write_reg = sp_write_reg8; } - err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop); - if (!err) - priv->can.clock.freq = prop / 2; - else - priv->can.clock.freq = SP_CAN_CLOCK; /* default */ + if (!priv->can.clock.freq) { + err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop); + if (!err) + priv->can.clock.freq = prop / 2; + else + priv->can.clock.freq = SP_CAN_CLOCK; /* default */ + } err = of_property_read_u32(of, "nxp,tx-output-mode", &prop); if (!err) @@ -192,8 +200,13 @@ static struct sja1000_of_data technologic_data = { .init = sp_technologic_init, }; +static struct sja1000_of_data renesas_data = { + .init = sp_rzn1_init, +}; + static const struct of_device_id sp_of_table[] = { { .compatible = "nxp,sja1000", .data = NULL, }, + { .compatible = "renesas,rzn1-sja1000", .data = &renesas_data, }, { .compatible = "technologic,sja1000", .data = &technologic_data, }, { /* sentinel */ }, }; @@ -210,6 +223,7 @@ static int sp_probe(struct platform_device *pdev) struct device_node *of = pdev->dev.of_node; const struct sja1000_of_data *of_data = NULL; size_t priv_sz = 0; + struct clk *clk; pdata = dev_get_platdata(&pdev->dev); if (!pdata && !of) { @@ -262,6 +276,16 @@ static int sp_probe(struct platform_device *pdev) priv->reg_base = addr; if (of) { + clk = devm_clk_get_optional(&pdev->dev, "can_clk"); + if (IS_ERR(clk)) + return dev_err_probe(&pdev->dev, PTR_ERR(clk), "no CAN clk"); + + if (clk) { + priv->can.clock.freq = clk_get_rate(clk) / 2; + if (!priv->can.clock.freq) + return dev_err_probe(&pdev->dev, -EINVAL, "Zero CAN clk rate"); + } + sp_populate_of(priv, of); if (of_data && of_data->init)
The SJA1000 CAN controller on RZ/N1 SoC has some differences compared to others like it has no clock divider register (CDR) support and it has no HW loopback(HW doesn't see tx messages on rx). This patch adds support for RZ/N1 SJA1000 CAN Controller. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- drivers/net/can/sja1000/sja1000_platform.c | 34 ++++++++++++++++++---- 1 file changed, 29 insertions(+), 5 deletions(-)