From: Angelo Dureghello <angelo@kernel-space.org>
To: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: gerg@linux-m68k.org, wg@grandegger.com, geert@linux-m68k.org,
linux-m68k@vger.kernel.org, linux-can@vger.kernel.org,
qiangqing.zhang@nxp.com
Subject: Re: [PATCH v2 5/5] can: flexcan: add mcf5441x support
Date: Fri, 18 Jun 2021 00:14:32 +0200 [thread overview]
Message-ID: <b6b948f4-dcdb-5f75-9fe8-138de0acf1f7@kernel-space.org> (raw)
In-Reply-To: <20210617130053.yovl3b3p5mn2srnd@pengutronix.de>
Hi Mark,
On 17/06/21 3:00 PM, Marc Kleine-Budde wrote:
> On 17.06.2021 01:16:52, Angelo Dureghello wrote:
>> Add flexcan support for NXP ColdFire mcf5441x family.
>>
>> This flexcan module is quite similar to imx6 flexcan module, but
>> with some exceptions:
>>
>> - 3 separate interrupt sources, MB, BOFF and ERR,
>> - implements 16 mb only,
>> - m68k architecture is not supporting devicetrees, so a
>> platform data check/case has been added,
>> - ColdFire is m68k, so big-endian cpu, with a little-endian flexcan
>> module.
>
> Please add the mcg5441x to the "FLEXCAN hardware feature flags" table in
> the driver, and add a new column for the number of mailboxes.
>
need a little help here: where can i find IP version and
FlexCAN version ? I am 90% sure mcf5441x is FlexCAN2 (no FD).
No trace of IP version in the datasheets.
>> ---
>> Changes for v2:
>> - re-add platform data handling restarting from 2c0ac9208135
>> - re-add flexcan_id_table, as from 2c0ac9208135
>> - usinig irq resources for ERR and BOFF interrupts
>> - add missing free_irq() for interrupts
>> - minor syntax fixes
>>
>> Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
>> ---
>> drivers/net/can/flexcan.c | 95 ++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 84 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>> index 57f3635ad8d7..2188dc36a010 100644
>> --- a/drivers/net/can/flexcan.c
>> +++ b/drivers/net/can/flexcan.c
>> @@ -28,6 +28,7 @@
>> #include <linux/of_device.h>
>> #include <linux/pinctrl/consumer.h>
>> #include <linux/platform_device.h>
>> +#include <linux/can/platform/mcf5441x.h>
>> #include <linux/pm_runtime.h>
>> #include <linux/regmap.h>
>> #include <linux/regulator/consumer.h>
>> @@ -246,6 +247,8 @@
>> #define FLEXCAN_QUIRK_SUPPORT_ECC BIT(10)
>> /* Setup stop mode with SCU firmware to support wakeup */
>> #define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW BIT(11)
>> +/* Setup for flexcan module as in mcf, 16 mb, 3 separate interrupts */
>> +#define FLEXCAN_QUIRK_SETUP_MCF BIT(12)
>>
>> /* Structure of the message buffer */
>> struct flexcan_mb {
>> @@ -363,6 +366,9 @@ struct flexcan_priv {
>> struct regulator *reg_xceiver;
>> struct flexcan_stop_mode stm;
>>
>> + int irq_boff;
>> + int irq_err;
>> +
>> /* IPC handle when setup stop mode by System Controller firmware(scfw) */
>> struct imx_sc_ipc *sc_ipc_handle;
>>
>> @@ -371,6 +377,12 @@ struct flexcan_priv {
>> void (*write)(u32 val, void __iomem *addr);
>> };
>>
>> +static const struct flexcan_devtype_data fsl_mcf_devtype_data = {
>> + .quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE |
>> + FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>> + FLEXCAN_QUIRK_SETUP_MCF,
>> +};
>> +
>> static const struct flexcan_devtype_data fsl_p1010_devtype_data = {
>> .quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE |
>> FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>> @@ -637,13 +649,17 @@ static int flexcan_clks_enable(const struct flexcan_priv *priv)
>> {
>> int err;
>>
>> - err = clk_prepare_enable(priv->clk_ipg);
>> - if (err)
>> - return err;
>> + if (priv->clk_ipg) {
>> + err = clk_prepare_enable(priv->clk_ipg);
>
> Does the coldfire clk_disable_unprepare() work with NULL pointers? I'm
> asking for flexcan_clks_disable()...
>
tested, it's ok with NULL.
>> + if (err)
>> + return err;
>> + }
>>
>> - err = clk_prepare_enable(priv->clk_per);
>> - if (err)
>> - clk_disable_unprepare(priv->clk_ipg);
>> + if (priv->clk_per) {
>> + err = clk_prepare_enable(priv->clk_per);
>> + if (err)
>> + clk_disable_unprepare(priv->clk_ipg);
>> + }
>>
>> return err;
>> }
>> @@ -1401,8 +1417,12 @@ static int flexcan_rx_offload_setup(struct net_device *dev)
>> priv->mb_size = sizeof(struct flexcan_mb) + CANFD_MAX_DLEN;
>> else
>> priv->mb_size = sizeof(struct flexcan_mb) + CAN_MAX_DLEN;
>> - priv->mb_count = (sizeof(priv->regs->mb[0]) / priv->mb_size) +
>> - (sizeof(priv->regs->mb[1]) / priv->mb_size);
>> +
>> + if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_MCF)
>> + priv->mb_count = FLEXCAN_MCF5411X_MB_CNT_MCF;
>> + else
>> + priv->mb_count = (sizeof(priv->regs->mb[0]) / priv->mb_size) +
>> + (sizeof(priv->regs->mb[1]) / priv->mb_size);
>>
>> if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP)
>> priv->tx_mb_reserved =
>> @@ -1774,6 +1794,18 @@ static int flexcan_open(struct net_device *dev)
>> if (err)
>> goto out_can_rx_offload_disable;
>>
>> + if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_MCF) {
>> + err = request_irq(priv->irq_boff,
>> + flexcan_irq, IRQF_SHARED, dev->name, dev);
>> + if (err)
>> + goto out_can_rx_offload_disable;
>
> Please add proper error handling.
done
>
>> +
>> + err = request_irq(priv->irq_err,
>> + flexcan_irq, IRQF_SHARED, dev->name, dev);
>> + if (err)
>> + goto out_can_rx_offload_disable;
>
> Please add proper error handling.
done
>
>> + }
>> +
>> flexcan_chip_interrupts_enable(dev);
>>
>> can_led_event(dev, CAN_LED_EVENT_OPEN);
>> @@ -1804,6 +1836,12 @@ static int flexcan_close(struct net_device *dev)
>> netif_stop_queue(dev);
>> flexcan_chip_interrupts_disable(dev);
>> free_irq(dev->irq, dev);
>> +
>> + if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_MCF) {
>> + free_irq(priv->irq_boff, dev);
>> + free_irq(priv->irq_err, dev);
>> + }
>
> please free in the opposite order of requesting
>
done
>> +
>> can_rx_offload_disable(&priv->offload);
>> flexcan_chip_stop_disable_on_error(dev);
>>
>> @@ -2039,14 +2077,26 @@ static const struct of_device_id flexcan_of_match[] = {
>> };
>> MODULE_DEVICE_TABLE(of, flexcan_of_match);
>>
>> +static const struct platform_device_id flexcan_id_table[] = {
>> + {
>> + .name = "flexcan-mcf",
>> + .driver_data = (kernel_ulong_t)&fsl_mcf_devtype_data,
>> + }, {
>> + /* sentinel */
>> + },
>> +};
>> +MODULE_DEVICE_TABLE(platform, flexcan_id_table);
>> +
>> static int flexcan_probe(struct platform_device *pdev)
>> {
>> + const struct of_device_id *of_id;
>> const struct flexcan_devtype_data *devtype_data;
>> struct net_device *dev;
>> struct flexcan_priv *priv;
>> struct regulator *reg_xceiver;
>> struct clk *clk_ipg = NULL, *clk_per = NULL;
>> struct flexcan_regs __iomem *regs;
>> + struct mcf_flexcan_platform_data *pdata;
>> int err, irq;
>> u8 clk_src = 1;
>> u32 clock_freq = 0;
>> @@ -2061,9 +2111,15 @@ static int flexcan_probe(struct platform_device *pdev)
>>
>> if (pdev->dev.of_node) {
>> of_property_read_u32(pdev->dev.of_node,
>> - "clock-frequency", &clock_freq);
>> + "clock-frequency", &clock_freq);
>
> unrelated change
removed
>
>> of_property_read_u8(pdev->dev.of_node,
>> - "fsl,clk-source", &clk_src);
>> + "fsl,clk-source", &clk_src);
>
> unrelated change
removed
>
>> + } else {
>> + pdata = dev_get_platdata(&pdev->dev);
>> + if (pdata) {
>> + clock_freq = pdata->clock_frequency;
>> + clk_src = pdata->clk_src;
>> + }
>> }
>>
>> if (!clock_freq) {
>> @@ -2089,7 +2145,14 @@ static int flexcan_probe(struct platform_device *pdev)
>> if (IS_ERR(regs))
>> return PTR_ERR(regs);
>>
>> - devtype_data = of_device_get_match_data(&pdev->dev);
>> + of_id = of_match_device(flexcan_of_match, &pdev->dev);
>> + if (of_id)
>> + devtype_data = of_id->data;
>> + else if (platform_get_device_id(pdev)->driver_data)
>> + devtype_data = (struct flexcan_devtype_data *)
>> + platform_get_device_id(pdev)->driver_data;
>> + else
>> + return -ENODEV;
>>
>> if ((devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) &&
>> !(devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP)) {
>> @@ -2133,6 +2196,15 @@ static int flexcan_probe(struct platform_device *pdev)
>> priv->devtype_data = devtype_data;
>> priv->reg_xceiver = reg_xceiver;
>>
>> + if (devtype_data->quirks & FLEXCAN_QUIRK_SETUP_MCF) {
>> + priv->irq_boff = platform_get_irq(pdev, 1);
>> + if (priv->irq_boff <= 0)
>> + return -ENODEV;
>
> Please add proper error handling.
done
>
>> + priv->irq_err = platform_get_irq(pdev, 2);
>> + if (priv->irq_err <= 0)
>> + return -ENODEV;
>
> Please add proper error handling.
done
>
>> + }
>> +
>> if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
>> priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD |
>> CAN_CTRLMODE_FD_NON_ISO;
>> @@ -2322,6 +2394,7 @@ static struct platform_driver flexcan_driver = {
>> },
>> .probe = flexcan_probe,
>> .remove = flexcan_remove,
>> + .id_table = flexcan_id_table,
>> };
>>
>> module_platform_driver(flexcan_driver);
>> --
>> 2.31.1
>>
>>
>
> regards,
> Marc
>
Regards,
angelo
next prev parent reply other threads:[~2021-06-17 22:15 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-16 23:16 [PATCH v2 1/5] can: flexcan: add platform data for ColdFire Angelo Dureghello
2021-06-16 23:16 ` [PATCH v2 2/5] m68k: stmark2: update board setup Angelo Dureghello
2021-06-16 23:16 ` [PATCH v2 3/5] m68k: m5441x: add flexcan support Angelo Dureghello
2021-06-17 0:18 ` Greg Ungerer
2021-06-17 7:17 ` Angelo Dureghello
2021-06-16 23:16 ` [PATCH v2 4/5] can: flexcan: enable Kconfig for ColdFire Angelo Dureghello
2021-06-17 15:27 ` Marc Kleine-Budde
2021-06-16 23:16 ` [PATCH v2 5/5] can: flexcan: add mcf5441x support Angelo Dureghello
2021-06-17 13:00 ` Marc Kleine-Budde
2021-06-17 13:32 ` Angelo Dureghello
2021-06-17 22:14 ` Angelo Dureghello [this message]
2021-06-18 6:57 ` Marc Kleine-Budde
2021-06-17 19:38 ` Marc Kleine-Budde
2021-06-18 7:59 ` Angelo Dureghello
2021-06-17 0:17 ` [PATCH v2 1/5] can: flexcan: add platform data for ColdFire Greg Ungerer
2021-06-17 7:23 ` Angelo Dureghello
2021-06-17 8:29 ` Marc Kleine-Budde
2021-06-17 11:45 ` Marc Kleine-Budde
2021-06-17 8:28 ` Geert Uytterhoeven
2021-06-17 8:31 ` Angelo Dureghello
2021-06-17 19:40 ` Marc Kleine-Budde
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=b6b948f4-dcdb-5f75-9fe8-138de0acf1f7@kernel-space.org \
--to=angelo@kernel-space.org \
--cc=geert@linux-m68k.org \
--cc=gerg@linux-m68k.org \
--cc=linux-can@vger.kernel.org \
--cc=linux-m68k@vger.kernel.org \
--cc=mkl@pengutronix.de \
--cc=qiangqing.zhang@nxp.com \
--cc=wg@grandegger.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.