| Message ID | 20201016134320.20321-1-qiangqing.zhang@nxp.com |
|---|---|
| Headers | show |
| Series | can: flexcan: add stop mode support for i.MX8QM | expand |
On 10/16/20 3:43 PM, Joakim Zhang wrote: > The System Controller Firmware (SCFW) is a low-level system function > which runs on a dedicated Cortex-M core to provide power, clock, and > resource management. It exists on some i.MX8 processors. e.g. i.MX8QM > (QM, QP), and i.MX8QX (QXP, DX). SCU driver manages the IPC interface > between host CPU and the SCU firmware running on M4. > > For i.MX8QM, stop mode request is controlled by System Controller Unit(SCU) > firmware, this patch introduces FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW quirk > for this function. > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > --- > drivers/net/can/flexcan.c | 125 ++++++++++++++++++++++++++++++++------ > 1 file changed, 107 insertions(+), 18 deletions(-) > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > index e708e7bf28db..a55ea8f27f7c 100644 > --- a/drivers/net/can/flexcan.c > +++ b/drivers/net/can/flexcan.c > @@ -9,6 +9,7 @@ > // > // Based on code originally by Andrey Volkov <avolkov@varma-el.com> > > +#include <dt-bindings/firmware/imx/rsrc.h> > #include <linux/bitfield.h> > #include <linux/can.h> > #include <linux/can/dev.h> > @@ -17,6 +18,7 @@ > #include <linux/can/rx-offload.h> > #include <linux/clk.h> > #include <linux/delay.h> > +#include <linux/firmware/imx/sci.h> > #include <linux/interrupt.h> > #include <linux/io.h> > #include <linux/mfd/syscon.h> > @@ -242,6 +244,8 @@ > #define FLEXCAN_QUIRK_SUPPORT_FD BIT(9) > /* support memory detection and correction */ > #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) > > /* Structure of the message buffer */ > struct flexcan_mb { > @@ -347,6 +351,7 @@ struct flexcan_priv { > u8 mb_count; > u8 mb_size; > u8 clk_src; /* clock source of CAN Protocol Engine */ > + u8 can_idx; > > u64 rx_mask; > u64 tx_mask; > @@ -358,6 +363,9 @@ struct flexcan_priv { > struct regulator *reg_xceiver; > struct flexcan_stop_mode stm; > > + /* IPC handle when setup stop mode by System Controller firmware(scfw) */ > + struct imx_sc_ipc *sc_ipc_handle; > + > /* Read and Write APIs */ > u32 (*read)(void __iomem *addr); > void (*write)(u32 val, void __iomem *addr); > @@ -387,7 +395,7 @@ static const struct flexcan_devtype_data fsl_imx6q_devtype_data = { > static const struct flexcan_devtype_data fsl_imx8qm_devtype_data = { > .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS | > FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_BROKEN_PERR_STATE | > - FLEXCAN_QUIRK_SUPPORT_FD, > + FLEXCAN_QUIRK_SUPPORT_FD | FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW, > }; > > static struct flexcan_devtype_data fsl_imx8mp_devtype_data = { > @@ -546,18 +554,46 @@ static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable) > priv->write(reg_mcr, ®s->mcr); > } > > +static int flexcan_stop_mode_enable_scfw(struct flexcan_priv *priv, bool enabled) > +{ > + u8 idx = priv->can_idx; > + u32 rsrc_id, val; > + > + if (idx == 0) > + rsrc_id = IMX_SC_R_CAN_0; > + else if (idx == 1) > + rsrc_id = IMX_SC_R_CAN_1; > + else > + rsrc_id = IMX_SC_R_CAN_2; > + > + if (enabled) > + val = 1; > + else > + val = 0; > + > + /* stop mode request via scu firmware */ > + return imx_sc_misc_set_control(priv->sc_ipc_handle, rsrc_id, IMX_SC_C_IPG_STOP, val); > +} > + > static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv) > { > struct flexcan_regs __iomem *regs = priv->regs; > u32 reg_mcr; > + int ret; > > reg_mcr = priv->read(®s->mcr); > reg_mcr |= FLEXCAN_MCR_SLF_WAK; > priv->write(reg_mcr, ®s->mcr); > > /* enable stop request */ > - regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr, > - 1 << priv->stm.req_bit, 1 << priv->stm.req_bit); > + if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) { > + ret = flexcan_stop_mode_enable_scfw(priv, true); > + if (ret < 0) > + return ret; > + } else { > + regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr, > + 1 << priv->stm.req_bit, 1 << priv->stm.req_bit); > + } > > return flexcan_low_power_enter_ack(priv); > } > @@ -566,10 +602,17 @@ static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv) > { > struct flexcan_regs __iomem *regs = priv->regs; > u32 reg_mcr; > + int ret; > > /* remove stop request */ > - regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr, > - 1 << priv->stm.req_bit, 0); > + if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) { > + ret = flexcan_stop_mode_enable_scfw(priv, false); > + if (ret < 0) > + return ret; > + } else { > + regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr, > + 1 << priv->stm.req_bit, 0); > + } > > reg_mcr = priv->read(®s->mcr); > reg_mcr &= ~FLEXCAN_MCR_SLF_WAK; > @@ -1838,7 +1881,7 @@ static void unregister_flexcandev(struct net_device *dev) > unregister_candev(dev); > } > > -static int flexcan_setup_stop_mode(struct platform_device *pdev) > +static int flexcan_setup_stop_mode_gpr(struct platform_device *pdev) > { > struct net_device *dev = platform_get_drvdata(pdev); > struct device_node *np = pdev->dev.of_node; > @@ -1883,11 +1926,6 @@ static int flexcan_setup_stop_mode(struct platform_device *pdev) > "gpr %s req_gpr=0x02%x req_bit=%u\n", > gpr_np->full_name, priv->stm.req_gpr, priv->stm.req_bit); > > - device_set_wakeup_capable(&pdev->dev, true); > - > - if (of_property_read_bool(np, "wakeup-source")) > - device_set_wakeup_enable(&pdev->dev, true); > - > return 0; > > out_put_node: > @@ -1895,6 +1933,56 @@ static int flexcan_setup_stop_mode(struct platform_device *pdev) > return ret; > } > > +static int flexcan_setup_stop_mode_scfw(struct platform_device *pdev) > +{ > + struct net_device *dev = platform_get_drvdata(pdev); > + struct flexcan_priv *priv; > + int ret; > + > + priv = netdev_priv(dev); > + > + /* this function could be defer probe, return -EPROBE_DEFER */ > + ret = imx_scu_get_handle(&priv->sc_ipc_handle); > + if (ret < 0) > + dev_dbg(&pdev->dev, "get ipc handle used by SCU failed\n"); > + > + return ret; > +} > + > +/* flexcan_setup_stop_mode - Setup stop mode > + * > + * Return: 0 setup stop mode successfully or doesn't support this feature > + * -EPROBE_DEFER defer probe > + * < 0 fail to setup stop mode > + */ > +static int flexcan_setup_stop_mode(struct platform_device *pdev) > +{ > + struct net_device *dev = platform_get_drvdata(pdev); > + struct flexcan_priv *priv; > + int ret; > + > + priv = netdev_priv(dev); > + > + if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) > + ret = flexcan_setup_stop_mode_scfw(pdev); > + else if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR) > + ret = flexcan_setup_stop_mode_gpr(pdev); > + else > + /* return 0 directly if stop mode is unsupport */ > + return 0; > + > + if (ret) { > + dev_warn(&pdev->dev, "failed to setup stop mode\n"); > + } else { > + device_set_wakeup_capable(&pdev->dev, true); > + > + if (of_property_read_bool(pdev->dev.of_node, "wakeup-source")) > + device_set_wakeup_enable(&pdev->dev, true); > + } > + > + return ret; > +} > + > static const struct of_device_id flexcan_of_match[] = { > { .compatible = "fsl,imx8qm-flexcan", .data = &fsl_imx8qm_devtype_data, }, > { .compatible = "fsl,imx8mp-flexcan", .data = &fsl_imx8mp_devtype_data, }, > @@ -1927,7 +2015,7 @@ static int flexcan_probe(struct platform_device *pdev) > struct clk *clk_ipg = NULL, *clk_per = NULL; > struct flexcan_regs __iomem *regs; > int err, irq; > - u8 clk_src = 1; > + u8 clk_src = 1, can_idx = 0; > u32 clock_freq = 0; > > reg_xceiver = devm_regulator_get_optional(&pdev->dev, "xceiver"); > @@ -1943,6 +2031,8 @@ static int flexcan_probe(struct platform_device *pdev) > "clock-frequency", &clock_freq); > of_property_read_u8(pdev->dev.of_node, > "fsl,clk-source", &clk_src); > + of_property_read_u8(pdev->dev.of_node, > + "fsl,can-index", &can_idx); > } > > if (!clock_freq) { > @@ -2019,6 +2109,7 @@ static int flexcan_probe(struct platform_device *pdev) > priv->clk_src = clk_src; > priv->devtype_data = devtype_data; > priv->reg_xceiver = reg_xceiver; > + priv->can_idx = can_idx; > > if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) { > priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD | > @@ -2030,6 +2121,10 @@ static int flexcan_probe(struct platform_device *pdev) > priv->can.bittiming_const = &flexcan_bittiming_const; > } > > + err = flexcan_setup_stop_mode(pdev); > + if (err == -EPROBE_DEFER) > + return -EPROBE_DEFER; You need to free "dev". What about moving this directly before allocating dev. Do you have to undo device_set_wakeup_capable() and device_set_wakeup_enable() in case of a failure and/or on flexcan_remove()? > + > pm_runtime_get_noresume(&pdev->dev); > pm_runtime_set_active(&pdev->dev); > pm_runtime_enable(&pdev->dev); > @@ -2043,12 +2138,6 @@ static int flexcan_probe(struct platform_device *pdev) > of_can_transceiver(dev); > devm_can_led_init(dev); > > - if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR) { > - err = flexcan_setup_stop_mode(pdev); > - if (err) > - dev_dbg(&pdev->dev, "failed to setup stop-mode\n"); > - } > - > return 0; > > failed_register: > Marc
On 10/16/20 3:43 PM, Joakim Zhang wrote: > After double check with Layerscape CAN owner (Pankaj Bansal), confirm > that LS1021A doesn't support ECC, and LX2160A indeed supports ECC. > > For SoCs with ECC supported, even use FLEXCAN_QUIRK_DISABLE_MECR quirk to > disable non-correctable errors interrupt and freeze mode, had better use > FLEXCAN_QUIRK_SUPPORT_ECC quirk to initialize all memory. > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > --- > drivers/net/can/flexcan.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > index a55ea8f27f7c..7b0eb608fc9d 100644 > --- a/drivers/net/can/flexcan.c > +++ b/drivers/net/can/flexcan.c > @@ -219,7 +219,7 @@ > * MX8MP FlexCAN3 03.00.17.01 yes yes no yes yes yes > * VF610 FlexCAN3 ? no yes no yes yes? no > * LS1021A FlexCAN2 03.00.04.00 no yes no no yes no > - * LX2160A FlexCAN3 03.00.23.00 no yes no no yes yes > + * LX2160A FlexCAN3 03.00.23.00 no yes no yes yes yes > * > * Some SOCs do not have the RX_WARN & TX_WARN interrupt line connected. > */ > @@ -408,19 +408,19 @@ static struct flexcan_devtype_data fsl_imx8mp_devtype_data = { > static const struct flexcan_devtype_data fsl_vf610_devtype_data = { > .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS | > FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | > - FLEXCAN_QUIRK_BROKEN_PERR_STATE, > + FLEXCAN_QUIRK_BROKEN_PERR_STATE | FLEXCAN_QUIRK_SUPPORT_ECC, You add the missing ECC init for vf610, but don't mention it in the patch subject nor description. Please make this a seperate patch and add a Fixes: line. > }; > > static const struct flexcan_devtype_data fsl_ls1021a_r2_devtype_data = { > .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS | > - FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE | > - FLEXCAN_QUIRK_USE_OFF_TIMESTAMP, > + FLEXCAN_QUIRK_BROKEN_PERR_STATE | FLEXCAN_QUIRK_USE_OFF_TIMESTAMP, > }; Please make this a seperate patch, too, along with the Fixes line. > static const struct flexcan_devtype_data fsl_lx2160a_r1_devtype_data = { > .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS | > FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE | > - FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_SUPPORT_FD, > + FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_SUPPORT_FD | > + FLEXCAN_QUIRK_SUPPORT_ECC, > }; > > static const struct can_bittiming_const flexcan_bittiming_const = { >
On 10/16/20 3:43 PM, Joakim Zhang wrote: > The System Controller Firmware (SCFW) is a low-level system function > which runs on a dedicated Cortex-M core to provide power, clock, and > resource management. It exists on some i.MX8 processors. e.g. i.MX8QM > (QM, QP), and i.MX8QX (QXP, DX). SCU driver manages the IPC interface > between host CPU and the SCU firmware running on M4. > > For i.MX8QM, stop mode request is controlled by System Controller Unit(SCU) > firmware, this patch introduces FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW quirk > for this function. > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > --- > drivers/net/can/flexcan.c | 125 ++++++++++++++++++++++++++++++++------ > 1 file changed, 107 insertions(+), 18 deletions(-) > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > index e708e7bf28db..a55ea8f27f7c 100644 > --- a/drivers/net/can/flexcan.c > +++ b/drivers/net/can/flexcan.c > @@ -9,6 +9,7 @@ > // > // Based on code originally by Andrey Volkov <avolkov@varma-el.com> > > +#include <dt-bindings/firmware/imx/rsrc.h> > #include <linux/bitfield.h> > #include <linux/can.h> > #include <linux/can/dev.h> > @@ -17,6 +18,7 @@ > #include <linux/can/rx-offload.h> > #include <linux/clk.h> > #include <linux/delay.h> > +#include <linux/firmware/imx/sci.h> > #include <linux/interrupt.h> > #include <linux/io.h> > #include <linux/mfd/syscon.h> > @@ -242,6 +244,8 @@ > #define FLEXCAN_QUIRK_SUPPORT_FD BIT(9) > /* support memory detection and correction */ > #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) > > /* Structure of the message buffer */ > struct flexcan_mb { > @@ -347,6 +351,7 @@ struct flexcan_priv { > u8 mb_count; > u8 mb_size; > u8 clk_src; /* clock source of CAN Protocol Engine */ > + u8 can_idx; > > u64 rx_mask; > u64 tx_mask; > @@ -358,6 +363,9 @@ struct flexcan_priv { > struct regulator *reg_xceiver; > struct flexcan_stop_mode stm; > > + /* IPC handle when setup stop mode by System Controller firmware(scfw) */ > + struct imx_sc_ipc *sc_ipc_handle; > + > /* Read and Write APIs */ > u32 (*read)(void __iomem *addr); > void (*write)(u32 val, void __iomem *addr); > @@ -387,7 +395,7 @@ static const struct flexcan_devtype_data fsl_imx6q_devtype_data = { > static const struct flexcan_devtype_data fsl_imx8qm_devtype_data = { > .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS | > FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_BROKEN_PERR_STATE | > - FLEXCAN_QUIRK_SUPPORT_FD, > + FLEXCAN_QUIRK_SUPPORT_FD | FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW, > }; > > static struct flexcan_devtype_data fsl_imx8mp_devtype_data = { > @@ -546,18 +554,46 @@ static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable) > priv->write(reg_mcr, ®s->mcr); > } > > +static int flexcan_stop_mode_enable_scfw(struct flexcan_priv *priv, bool enabled) > +{ > + u8 idx = priv->can_idx; > + u32 rsrc_id, val; > + > + if (idx == 0) > + rsrc_id = IMX_SC_R_CAN_0; > + else if (idx == 1) > + rsrc_id = IMX_SC_R_CAN_1; > + else > + rsrc_id = IMX_SC_R_CAN_2; Can you introduce something like and make use of it: #define IMX_SC_R_CAN(x) (105 + (x)) > + > + if (enabled) > + val = 1; > + else > + val = 0; > + > + /* stop mode request via scu firmware */ > + return imx_sc_misc_set_control(priv->sc_ipc_handle, rsrc_id, IMX_SC_C_IPG_STOP, val); > +} > + > static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv) > { > struct flexcan_regs __iomem *regs = priv->regs; > u32 reg_mcr; > + int ret; > > reg_mcr = priv->read(®s->mcr); > reg_mcr |= FLEXCAN_MCR_SLF_WAK; > priv->write(reg_mcr, ®s->mcr); > > /* enable stop request */ > - regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr, > - 1 << priv->stm.req_bit, 1 << priv->stm.req_bit); > + if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) { > + ret = flexcan_stop_mode_enable_scfw(priv, true); > + if (ret < 0) > + return ret; > + } else { > + regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr, > + 1 << priv->stm.req_bit, 1 << priv->stm.req_bit); > + } > > return flexcan_low_power_enter_ack(priv); > } > @@ -566,10 +602,17 @@ static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv) > { > struct flexcan_regs __iomem *regs = priv->regs; > u32 reg_mcr; > + int ret; > > /* remove stop request */ > - regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr, > - 1 << priv->stm.req_bit, 0); > + if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) { > + ret = flexcan_stop_mode_enable_scfw(priv, false); > + if (ret < 0) > + return ret; > + } else { > + regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr, > + 1 << priv->stm.req_bit, 0); > + } > > reg_mcr = priv->read(®s->mcr); > reg_mcr &= ~FLEXCAN_MCR_SLF_WAK; > @@ -1838,7 +1881,7 @@ static void unregister_flexcandev(struct net_device *dev) > unregister_candev(dev); > } > > -static int flexcan_setup_stop_mode(struct platform_device *pdev) > +static int flexcan_setup_stop_mode_gpr(struct platform_device *pdev) > { > struct net_device *dev = platform_get_drvdata(pdev); > struct device_node *np = pdev->dev.of_node; > @@ -1883,11 +1926,6 @@ static int flexcan_setup_stop_mode(struct platform_device *pdev) > "gpr %s req_gpr=0x02%x req_bit=%u\n", > gpr_np->full_name, priv->stm.req_gpr, priv->stm.req_bit); > > - device_set_wakeup_capable(&pdev->dev, true); > - > - if (of_property_read_bool(np, "wakeup-source")) > - device_set_wakeup_enable(&pdev->dev, true); > - > return 0; > > out_put_node: > @@ -1895,6 +1933,56 @@ static int flexcan_setup_stop_mode(struct platform_device *pdev) > return ret; > } > > +static int flexcan_setup_stop_mode_scfw(struct platform_device *pdev) > +{ > + struct net_device *dev = platform_get_drvdata(pdev); > + struct flexcan_priv *priv; > + int ret; > + > + priv = netdev_priv(dev); > + > + /* this function could be defer probe, return -EPROBE_DEFER */ > + ret = imx_scu_get_handle(&priv->sc_ipc_handle); > + if (ret < 0) > + dev_dbg(&pdev->dev, "get ipc handle used by SCU failed\n"); > + > + return ret; > +} > + > +/* flexcan_setup_stop_mode - Setup stop mode > + * > + * Return: 0 setup stop mode successfully or doesn't support this feature > + * -EPROBE_DEFER defer probe > + * < 0 fail to setup stop mode > + */ > +static int flexcan_setup_stop_mode(struct platform_device *pdev) > +{ > + struct net_device *dev = platform_get_drvdata(pdev); > + struct flexcan_priv *priv; > + int ret; > + > + priv = netdev_priv(dev); > + > + if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) > + ret = flexcan_setup_stop_mode_scfw(pdev); > + else if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR) > + ret = flexcan_setup_stop_mode_gpr(pdev); > + else > + /* return 0 directly if stop mode is unsupport */ > + return 0; > + > + if (ret) { > + dev_warn(&pdev->dev, "failed to setup stop mode\n"); return here... > + } else { ...and remove the else > + device_set_wakeup_capable(&pdev->dev, true); > + > + if (of_property_read_bool(pdev->dev.of_node, "wakeup-source")) > + device_set_wakeup_enable(&pdev->dev, true); > + } > + > + return ret; > +} > + > static const struct of_device_id flexcan_of_match[] = { > { .compatible = "fsl,imx8qm-flexcan", .data = &fsl_imx8qm_devtype_data, }, > { .compatible = "fsl,imx8mp-flexcan", .data = &fsl_imx8mp_devtype_data, }, > @@ -1927,7 +2015,7 @@ static int flexcan_probe(struct platform_device *pdev) > struct clk *clk_ipg = NULL, *clk_per = NULL; > struct flexcan_regs __iomem *regs; > int err, irq; > - u8 clk_src = 1; > + u8 clk_src = 1, can_idx = 0; > u32 clock_freq = 0; > > reg_xceiver = devm_regulator_get_optional(&pdev->dev, "xceiver"); > @@ -1943,6 +2031,8 @@ static int flexcan_probe(struct platform_device *pdev) > "clock-frequency", &clock_freq); > of_property_read_u8(pdev->dev.of_node, > "fsl,clk-source", &clk_src); > + of_property_read_u8(pdev->dev.of_node, > + "fsl,can-index", &can_idx); What happens if the DT doesn't contain the can-index? Move this into the flexcan_setup_stop_mode_scfw() and add error handling. > } > > if (!clock_freq) { > @@ -2019,6 +2109,7 @@ static int flexcan_probe(struct platform_device *pdev) > priv->clk_src = clk_src; > priv->devtype_data = devtype_data; > priv->reg_xceiver = reg_xceiver; > + priv->can_idx = can_idx; Assign priv->can_idx in flexcan_setup_stop_mode_scfw(), too. > > if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) { > priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD | > @@ -2030,6 +2121,10 @@ static int flexcan_probe(struct platform_device *pdev) > priv->can.bittiming_const = &flexcan_bittiming_const; > } > > + err = flexcan_setup_stop_mode(pdev); > + if (err == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > pm_runtime_get_noresume(&pdev->dev); > pm_runtime_set_active(&pdev->dev); > pm_runtime_enable(&pdev->dev); > @@ -2043,12 +2138,6 @@ static int flexcan_probe(struct platform_device *pdev) > of_can_transceiver(dev); > devm_can_led_init(dev); > > - if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR) { > - err = flexcan_setup_stop_mode(pdev); > - if (err) > - dev_dbg(&pdev->dev, "failed to setup stop-mode\n"); > - } > - > return 0; > > failed_register: > Marc
On 10/16/20 3:43 PM, Joakim Zhang wrote: > This patch intends to rename FLEXCAN_QUIRK_SETUP_STOP_MODE quirk > to FLEXCAN_QUIRK_SETUP_STOP_MODE_GRP for non-scu SoCs, coming patch will > add quirk for scu SoCs. > > For non-scu SoCs, setup stop mode with GPR register. > For scu SoCs, setup stop mode with SCU firmware. > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> Looks good. Marc
Hi Marc, > -----Original Message----- > From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: 2020年10月16日 14:00 > To: Joakim Zhang <qiangqing.zhang@nxp.com>; robh+dt@kernel.org; > shawnguo@kernel.org; s.hauer@pengutronix.de > Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>; Ying Liu > <victor.liu@nxp.com>; Peng Fan <peng.fan@nxp.com>; > linux-can@vger.kernel.org; Pankaj Bansal <pankaj.bansal@nxp.com>; > netdev@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH 5/6] can: flexcan: add CAN wakeup function for i.MX8QM > > On 10/16/20 3:43 PM, Joakim Zhang wrote: > > The System Controller Firmware (SCFW) is a low-level system function > > which runs on a dedicated Cortex-M core to provide power, clock, and > > resource management. It exists on some i.MX8 processors. e.g. i.MX8QM > > (QM, QP), and i.MX8QX (QXP, DX). SCU driver manages the IPC interface > > between host CPU and the SCU firmware running on M4. > > > > For i.MX8QM, stop mode request is controlled by System Controller > > Unit(SCU) firmware, this patch introduces > > FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW quirk for this function. > > > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > > --- > > drivers/net/can/flexcan.c | 125 > > ++++++++++++++++++++++++++++++++------ > > 1 file changed, 107 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > > index e708e7bf28db..a55ea8f27f7c 100644 > > --- a/drivers/net/can/flexcan.c > > +++ b/drivers/net/can/flexcan.c > > @@ -9,6 +9,7 @@ > > // > > // Based on code originally by Andrey Volkov <avolkov@varma-el.com> > > > > +#include <dt-bindings/firmware/imx/rsrc.h> > > #include <linux/bitfield.h> > > #include <linux/can.h> > > #include <linux/can/dev.h> > > @@ -17,6 +18,7 @@ > > #include <linux/can/rx-offload.h> > > #include <linux/clk.h> > > #include <linux/delay.h> > > +#include <linux/firmware/imx/sci.h> > > #include <linux/interrupt.h> > > #include <linux/io.h> > > #include <linux/mfd/syscon.h> > > @@ -242,6 +244,8 @@ > > #define FLEXCAN_QUIRK_SUPPORT_FD BIT(9) > > /* support memory detection and correction */ #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) > > > > /* Structure of the message buffer */ struct flexcan_mb { @@ -347,6 > > +351,7 @@ struct flexcan_priv { > > u8 mb_count; > > u8 mb_size; > > u8 clk_src; /* clock source of CAN Protocol Engine */ > > + u8 can_idx; > > > > u64 rx_mask; > > u64 tx_mask; > > @@ -358,6 +363,9 @@ struct flexcan_priv { > > struct regulator *reg_xceiver; > > struct flexcan_stop_mode stm; > > > > + /* IPC handle when setup stop mode by System Controller firmware(scfw) > */ > > + struct imx_sc_ipc *sc_ipc_handle; > > + > > /* Read and Write APIs */ > > u32 (*read)(void __iomem *addr); > > void (*write)(u32 val, void __iomem *addr); @@ -387,7 +395,7 @@ > > static const struct flexcan_devtype_data fsl_imx6q_devtype_data = { > > static const struct flexcan_devtype_data fsl_imx8qm_devtype_data = { > > .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | > FLEXCAN_QUIRK_ENABLE_EACEN_RRS | > > FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | > FLEXCAN_QUIRK_BROKEN_PERR_STATE | > > - FLEXCAN_QUIRK_SUPPORT_FD, > > + FLEXCAN_QUIRK_SUPPORT_FD | > FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW, > > }; > > > > static struct flexcan_devtype_data fsl_imx8mp_devtype_data = { @@ > > -546,18 +554,46 @@ static void flexcan_enable_wakeup_irq(struct > flexcan_priv *priv, bool enable) > > priv->write(reg_mcr, ®s->mcr); > > } > > > > +static int flexcan_stop_mode_enable_scfw(struct flexcan_priv *priv, > > +bool enabled) { > > + u8 idx = priv->can_idx; > > + u32 rsrc_id, val; > > + > > + if (idx == 0) > > + rsrc_id = IMX_SC_R_CAN_0; > > + else if (idx == 1) > > + rsrc_id = IMX_SC_R_CAN_1; > > + else > > + rsrc_id = IMX_SC_R_CAN_2; > > + > > + if (enabled) > > + val = 1; > > + else > > + val = 0; > > + > > + /* stop mode request via scu firmware */ > > + return imx_sc_misc_set_control(priv->sc_ipc_handle, rsrc_id, > > +IMX_SC_C_IPG_STOP, val); } > > + > > static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv) > > { > > struct flexcan_regs __iomem *regs = priv->regs; > > u32 reg_mcr; > > + int ret; > > > > reg_mcr = priv->read(®s->mcr); > > reg_mcr |= FLEXCAN_MCR_SLF_WAK; > > priv->write(reg_mcr, ®s->mcr); > > > > /* enable stop request */ > > - regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr, > > - 1 << priv->stm.req_bit, 1 << priv->stm.req_bit); > > + if (priv->devtype_data->quirks & > FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) { > > + ret = flexcan_stop_mode_enable_scfw(priv, true); > > + if (ret < 0) > > + return ret; > > + } else { > > + regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr, > > + 1 << priv->stm.req_bit, 1 << priv->stm.req_bit); > > + } > > > > return flexcan_low_power_enter_ack(priv); > > } > > @@ -566,10 +602,17 @@ static inline int flexcan_exit_stop_mode(struct > > flexcan_priv *priv) { > > struct flexcan_regs __iomem *regs = priv->regs; > > u32 reg_mcr; > > + int ret; > > > > /* remove stop request */ > > - regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr, > > - 1 << priv->stm.req_bit, 0); > > + if (priv->devtype_data->quirks & > FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) { > > + ret = flexcan_stop_mode_enable_scfw(priv, false); > > + if (ret < 0) > > + return ret; > > + } else { > > + regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr, > > + 1 << priv->stm.req_bit, 0); > > + } > > > > reg_mcr = priv->read(®s->mcr); > > reg_mcr &= ~FLEXCAN_MCR_SLF_WAK; > > @@ -1838,7 +1881,7 @@ static void unregister_flexcandev(struct > net_device *dev) > > unregister_candev(dev); > > } > > > > -static int flexcan_setup_stop_mode(struct platform_device *pdev) > > +static int flexcan_setup_stop_mode_gpr(struct platform_device *pdev) > > { > > struct net_device *dev = platform_get_drvdata(pdev); > > struct device_node *np = pdev->dev.of_node; @@ -1883,11 +1926,6 @@ > > static int flexcan_setup_stop_mode(struct platform_device *pdev) > > "gpr %s req_gpr=0x02%x req_bit=%u\n", > > gpr_np->full_name, priv->stm.req_gpr, priv->stm.req_bit); > > > > - device_set_wakeup_capable(&pdev->dev, true); > > - > > - if (of_property_read_bool(np, "wakeup-source")) > > - device_set_wakeup_enable(&pdev->dev, true); > > - > > return 0; > > > > out_put_node: > > @@ -1895,6 +1933,56 @@ static int flexcan_setup_stop_mode(struct > platform_device *pdev) > > return ret; > > } > > > > +static int flexcan_setup_stop_mode_scfw(struct platform_device *pdev) > > +{ > > + struct net_device *dev = platform_get_drvdata(pdev); > > + struct flexcan_priv *priv; > > + int ret; > > + > > + priv = netdev_priv(dev); > > + > > + /* this function could be defer probe, return -EPROBE_DEFER */ > > + ret = imx_scu_get_handle(&priv->sc_ipc_handle); > > + if (ret < 0) > > + dev_dbg(&pdev->dev, "get ipc handle used by SCU failed\n"); > > + > > + return ret; > > +} > > + > > +/* flexcan_setup_stop_mode - Setup stop mode > > + * > > + * Return: 0 setup stop mode successfully or doesn't support this feature > > + * -EPROBE_DEFER defer probe > > + * < 0 fail to setup stop mode > > + */ > > +static int flexcan_setup_stop_mode(struct platform_device *pdev) { > > + struct net_device *dev = platform_get_drvdata(pdev); > > + struct flexcan_priv *priv; > > + int ret; > > + > > + priv = netdev_priv(dev); > > + > > + if (priv->devtype_data->quirks & > FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) > > + ret = flexcan_setup_stop_mode_scfw(pdev); > > + else if (priv->devtype_data->quirks & > FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR) > > + ret = flexcan_setup_stop_mode_gpr(pdev); > > + else > > + /* return 0 directly if stop mode is unsupport */ > > + return 0; > > + > > + if (ret) { > > + dev_warn(&pdev->dev, "failed to setup stop mode\n"); > > + } else { > > + device_set_wakeup_capable(&pdev->dev, true); > > + > > + if (of_property_read_bool(pdev->dev.of_node, "wakeup-source")) > > + device_set_wakeup_enable(&pdev->dev, true); > > + } > > + > > + return ret; > > +} > > + > > static const struct of_device_id flexcan_of_match[] = { > > { .compatible = "fsl,imx8qm-flexcan", .data = > &fsl_imx8qm_devtype_data, }, > > { .compatible = "fsl,imx8mp-flexcan", .data = > > &fsl_imx8mp_devtype_data, }, @@ -1927,7 +2015,7 @@ static int > flexcan_probe(struct platform_device *pdev) > > struct clk *clk_ipg = NULL, *clk_per = NULL; > > struct flexcan_regs __iomem *regs; > > int err, irq; > > - u8 clk_src = 1; > > + u8 clk_src = 1, can_idx = 0; > > u32 clock_freq = 0; > > > > reg_xceiver = devm_regulator_get_optional(&pdev->dev, "xceiver"); @@ > > -1943,6 +2031,8 @@ static int flexcan_probe(struct platform_device *pdev) > > "clock-frequency", &clock_freq); > > of_property_read_u8(pdev->dev.of_node, > > "fsl,clk-source", &clk_src); > > + of_property_read_u8(pdev->dev.of_node, > > + "fsl,can-index", &can_idx); > > } > > > > if (!clock_freq) { > > @@ -2019,6 +2109,7 @@ static int flexcan_probe(struct platform_device > *pdev) > > priv->clk_src = clk_src; > > priv->devtype_data = devtype_data; > > priv->reg_xceiver = reg_xceiver; > > + priv->can_idx = can_idx; > > > > if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) { > > priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD | @@ > -2030,6 > > +2121,10 @@ static int flexcan_probe(struct platform_device *pdev) > > priv->can.bittiming_const = &flexcan_bittiming_const; > > } > > > > + err = flexcan_setup_stop_mode(pdev); > > + if (err == -EPROBE_DEFER) > > + return -EPROBE_DEFER; > > You need to free "dev". What about moving this directly before allocating dev. Yes, need free "dev" here if defer probe. Flexcan_priv has not allocated before allocating dev, but we need initialize and check it when setup stop mode. > Do you have to undo device_set_wakeup_capable() and > device_set_wakeup_enable() in case of a failure and/or on flexcan_remove()? Yes, should invoke device_wakeup_disable() in flexcan_remove. Best Regards, Joakim Zhang > > + > > pm_runtime_get_noresume(&pdev->dev); > > pm_runtime_set_active(&pdev->dev); > > pm_runtime_enable(&pdev->dev); > > @@ -2043,12 +2138,6 @@ static int flexcan_probe(struct platform_device > *pdev) > > of_can_transceiver(dev); > > devm_can_led_init(dev); > > > > - if (priv->devtype_data->quirks & > FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR) { > > - err = flexcan_setup_stop_mode(pdev); > > - if (err) > > - dev_dbg(&pdev->dev, "failed to setup stop-mode\n"); > > - } > > - > > return 0; > > > > failed_register: > > > > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung West/Dortmund | Phone: +49-231-2826-924 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Marc, > -----Original Message----- > From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: 2020年10月16日 14:05 > To: Joakim Zhang <qiangqing.zhang@nxp.com>; robh+dt@kernel.org; > shawnguo@kernel.org; s.hauer@pengutronix.de > Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>; Ying Liu > <victor.liu@nxp.com>; Peng Fan <peng.fan@nxp.com>; > linux-can@vger.kernel.org; Pankaj Bansal <pankaj.bansal@nxp.com>; > netdev@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH 6/6] can: flexcan: fix ECC function on LS1021A/LX2160A > > On 10/16/20 3:43 PM, Joakim Zhang wrote: > > After double check with Layerscape CAN owner (Pankaj Bansal), confirm > > that LS1021A doesn't support ECC, and LX2160A indeed supports ECC. > > > > For SoCs with ECC supported, even use FLEXCAN_QUIRK_DISABLE_MECR > quirk > > to disable non-correctable errors interrupt and freeze mode, had > > better use FLEXCAN_QUIRK_SUPPORT_ECC quirk to initialize all memory. > > > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > > --- > > drivers/net/can/flexcan.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > > index a55ea8f27f7c..7b0eb608fc9d 100644 > > --- a/drivers/net/can/flexcan.c > > +++ b/drivers/net/can/flexcan.c > > @@ -219,7 +219,7 @@ > > * MX8MP FlexCAN3 03.00.17.01 yes yes no > yes yes yes > > * VF610 FlexCAN3 ? no yes no > yes yes? no > > * LS1021A FlexCAN2 03.00.04.00 no yes no > no yes no > > - * LX2160A FlexCAN3 03.00.23.00 no yes no > no yes yes > > + * LX2160A FlexCAN3 03.00.23.00 no yes no > yes yes yes > > * > > * Some SOCs do not have the RX_WARN & TX_WARN interrupt line > connected. > > */ > > @@ -408,19 +408,19 @@ static struct flexcan_devtype_data > > fsl_imx8mp_devtype_data = { static const struct flexcan_devtype_data > fsl_vf610_devtype_data = { > > .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | > FLEXCAN_QUIRK_ENABLE_EACEN_RRS | > > FLEXCAN_QUIRK_DISABLE_MECR | > FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | > > - FLEXCAN_QUIRK_BROKEN_PERR_STATE, > > + FLEXCAN_QUIRK_BROKEN_PERR_STATE | > FLEXCAN_QUIRK_SUPPORT_ECC, > > You add the missing ECC init for vf610, but don't mention it in the patch subject > nor description. Please make this a seperate patch and add a Fixes: line. OK. > > }; > > > > static const struct flexcan_devtype_data fsl_ls1021a_r2_devtype_data = { > > .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | > FLEXCAN_QUIRK_ENABLE_EACEN_RRS | > > - FLEXCAN_QUIRK_DISABLE_MECR | > FLEXCAN_QUIRK_BROKEN_PERR_STATE | > > - FLEXCAN_QUIRK_USE_OFF_TIMESTAMP, > > + FLEXCAN_QUIRK_BROKEN_PERR_STATE | > FLEXCAN_QUIRK_USE_OFF_TIMESTAMP, > > }; > > Please make this a seperate patch, too, along with the Fixes line. OK. Best Regards, Joakim Zhang > > static const struct flexcan_devtype_data fsl_lx2160a_r1_devtype_data = { > > .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | > FLEXCAN_QUIRK_ENABLE_EACEN_RRS | > > FLEXCAN_QUIRK_DISABLE_MECR | > FLEXCAN_QUIRK_BROKEN_PERR_STATE | > > - FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | > FLEXCAN_QUIRK_SUPPORT_FD, > > + FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | > FLEXCAN_QUIRK_SUPPORT_FD | > > + FLEXCAN_QUIRK_SUPPORT_ECC, > > }; > > > > static const struct can_bittiming_const flexcan_bittiming_const = { > > > > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung West/Dortmund | Phone: +49-231-2826-924 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Marc, > -----Original Message----- > From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: 2020年10月16日 14:19 > To: Joakim Zhang <qiangqing.zhang@nxp.com>; robh+dt@kernel.org; > shawnguo@kernel.org; s.hauer@pengutronix.de > Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>; Ying Liu > <victor.liu@nxp.com>; Peng Fan <peng.fan@nxp.com>; > linux-can@vger.kernel.org; Pankaj Bansal <pankaj.bansal@nxp.com>; > netdev@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH 5/6] can: flexcan: add CAN wakeup function for i.MX8QM > > On 10/16/20 3:43 PM, Joakim Zhang wrote: > > The System Controller Firmware (SCFW) is a low-level system function > > which runs on a dedicated Cortex-M core to provide power, clock, and > > resource management. It exists on some i.MX8 processors. e.g. i.MX8QM > > (QM, QP), and i.MX8QX (QXP, DX). SCU driver manages the IPC interface > > between host CPU and the SCU firmware running on M4. > > > > For i.MX8QM, stop mode request is controlled by System Controller > > Unit(SCU) firmware, this patch introduces > > FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW quirk for this function. > > > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > > --- > > drivers/net/can/flexcan.c | 125 > > ++++++++++++++++++++++++++++++++------ > > 1 file changed, 107 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > > index e708e7bf28db..a55ea8f27f7c 100644 > > --- a/drivers/net/can/flexcan.c > > +++ b/drivers/net/can/flexcan.c > > @@ -9,6 +9,7 @@ > > // > > // Based on code originally by Andrey Volkov <avolkov@varma-el.com> > > > > +#include <dt-bindings/firmware/imx/rsrc.h> > > #include <linux/bitfield.h> > > #include <linux/can.h> > > #include <linux/can/dev.h> > > @@ -17,6 +18,7 @@ > > #include <linux/can/rx-offload.h> > > #include <linux/clk.h> > > #include <linux/delay.h> > > +#include <linux/firmware/imx/sci.h> > > #include <linux/interrupt.h> > > #include <linux/io.h> > > #include <linux/mfd/syscon.h> > > @@ -242,6 +244,8 @@ > > #define FLEXCAN_QUIRK_SUPPORT_FD BIT(9) > > /* support memory detection and correction */ #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) > > > > /* Structure of the message buffer */ struct flexcan_mb { @@ -347,6 > > +351,7 @@ struct flexcan_priv { > > u8 mb_count; > > u8 mb_size; > > u8 clk_src; /* clock source of CAN Protocol Engine */ > > + u8 can_idx; > > > > u64 rx_mask; > > u64 tx_mask; > > @@ -358,6 +363,9 @@ struct flexcan_priv { > > struct regulator *reg_xceiver; > > struct flexcan_stop_mode stm; > > > > + /* IPC handle when setup stop mode by System Controller firmware(scfw) > */ > > + struct imx_sc_ipc *sc_ipc_handle; > > + > > /* Read and Write APIs */ > > u32 (*read)(void __iomem *addr); > > void (*write)(u32 val, void __iomem *addr); @@ -387,7 +395,7 @@ > > static const struct flexcan_devtype_data fsl_imx6q_devtype_data = { > > static const struct flexcan_devtype_data fsl_imx8qm_devtype_data = { > > .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | > FLEXCAN_QUIRK_ENABLE_EACEN_RRS | > > FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | > FLEXCAN_QUIRK_BROKEN_PERR_STATE | > > - FLEXCAN_QUIRK_SUPPORT_FD, > > + FLEXCAN_QUIRK_SUPPORT_FD | > FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW, > > }; > > > > static struct flexcan_devtype_data fsl_imx8mp_devtype_data = { @@ > > -546,18 +554,46 @@ static void flexcan_enable_wakeup_irq(struct > flexcan_priv *priv, bool enable) > > priv->write(reg_mcr, ®s->mcr); > > } > > > > +static int flexcan_stop_mode_enable_scfw(struct flexcan_priv *priv, > > +bool enabled) { > > + u8 idx = priv->can_idx; > > + u32 rsrc_id, val; > > + > > + if (idx == 0) > > + rsrc_id = IMX_SC_R_CAN_0; > > + else if (idx == 1) > > + rsrc_id = IMX_SC_R_CAN_1; > > + else > > + rsrc_id = IMX_SC_R_CAN_2; > > Can you introduce something like and make use of it: > > #define IMX_SC_R_CAN(x) (105 + (x)) OK. > > + > > + if (enabled) > > + val = 1; > > + else > > + val = 0; > > + > > + /* stop mode request via scu firmware */ > > + return imx_sc_misc_set_control(priv->sc_ipc_handle, rsrc_id, > > +IMX_SC_C_IPG_STOP, val); } > > + > > static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv) > > { > > struct flexcan_regs __iomem *regs = priv->regs; > > u32 reg_mcr; > > + int ret; > > > > reg_mcr = priv->read(®s->mcr); > > reg_mcr |= FLEXCAN_MCR_SLF_WAK; > > priv->write(reg_mcr, ®s->mcr); > > > > /* enable stop request */ > > - regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr, > > - 1 << priv->stm.req_bit, 1 << priv->stm.req_bit); > > + if (priv->devtype_data->quirks & > FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) { > > + ret = flexcan_stop_mode_enable_scfw(priv, true); > > + if (ret < 0) > > + return ret; > > + } else { > > + regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr, > > + 1 << priv->stm.req_bit, 1 << priv->stm.req_bit); > > + } > > > > return flexcan_low_power_enter_ack(priv); > > } > > @@ -566,10 +602,17 @@ static inline int flexcan_exit_stop_mode(struct > > flexcan_priv *priv) { > > struct flexcan_regs __iomem *regs = priv->regs; > > u32 reg_mcr; > > + int ret; > > > > /* remove stop request */ > > - regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr, > > - 1 << priv->stm.req_bit, 0); > > + if (priv->devtype_data->quirks & > FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) { > > + ret = flexcan_stop_mode_enable_scfw(priv, false); > > + if (ret < 0) > > + return ret; > > + } else { > > + regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr, > > + 1 << priv->stm.req_bit, 0); > > + } > > > > reg_mcr = priv->read(®s->mcr); > > reg_mcr &= ~FLEXCAN_MCR_SLF_WAK; > > @@ -1838,7 +1881,7 @@ static void unregister_flexcandev(struct > net_device *dev) > > unregister_candev(dev); > > } > > > > -static int flexcan_setup_stop_mode(struct platform_device *pdev) > > +static int flexcan_setup_stop_mode_gpr(struct platform_device *pdev) > > { > > struct net_device *dev = platform_get_drvdata(pdev); > > struct device_node *np = pdev->dev.of_node; @@ -1883,11 +1926,6 @@ > > static int flexcan_setup_stop_mode(struct platform_device *pdev) > > "gpr %s req_gpr=0x02%x req_bit=%u\n", > > gpr_np->full_name, priv->stm.req_gpr, priv->stm.req_bit); > > > > - device_set_wakeup_capable(&pdev->dev, true); > > - > > - if (of_property_read_bool(np, "wakeup-source")) > > - device_set_wakeup_enable(&pdev->dev, true); > > - > > return 0; > > > > out_put_node: > > @@ -1895,6 +1933,56 @@ static int flexcan_setup_stop_mode(struct > platform_device *pdev) > > return ret; > > } > > > > +static int flexcan_setup_stop_mode_scfw(struct platform_device *pdev) > > +{ > > + struct net_device *dev = platform_get_drvdata(pdev); > > + struct flexcan_priv *priv; > > + int ret; > > + > > + priv = netdev_priv(dev); > > + > > + /* this function could be defer probe, return -EPROBE_DEFER */ > > + ret = imx_scu_get_handle(&priv->sc_ipc_handle); > > + if (ret < 0) > > + dev_dbg(&pdev->dev, "get ipc handle used by SCU failed\n"); > > + > > + return ret; > > +} > > + > > +/* flexcan_setup_stop_mode - Setup stop mode > > + * > > + * Return: 0 setup stop mode successfully or doesn't support this feature > > + * -EPROBE_DEFER defer probe > > + * < 0 fail to setup stop mode > > + */ > > +static int flexcan_setup_stop_mode(struct platform_device *pdev) { > > + struct net_device *dev = platform_get_drvdata(pdev); > > + struct flexcan_priv *priv; > > + int ret; > > + > > + priv = netdev_priv(dev); > > + > > + if (priv->devtype_data->quirks & > FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW) > > + ret = flexcan_setup_stop_mode_scfw(pdev); > > + else if (priv->devtype_data->quirks & > FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR) > > + ret = flexcan_setup_stop_mode_gpr(pdev); > > + else > > + /* return 0 directly if stop mode is unsupport */ > > + return 0; > > + > > + if (ret) { > > + dev_warn(&pdev->dev, "failed to setup stop mode\n"); > > return here... My thought was that, CAN still work even failed to setup stop mode. If you think it is better, I will change the way of setup stop mode, let probe failed if can't setup it successfully. > > + } else { > > ...and remove the else OK. > > + device_set_wakeup_capable(&pdev->dev, true); > > + > > + if (of_property_read_bool(pdev->dev.of_node, "wakeup-source")) > > + device_set_wakeup_enable(&pdev->dev, true); > > + } > > + > > + return ret; > > +} > > + > > static const struct of_device_id flexcan_of_match[] = { > > { .compatible = "fsl,imx8qm-flexcan", .data = > &fsl_imx8qm_devtype_data, }, > > { .compatible = "fsl,imx8mp-flexcan", .data = > > &fsl_imx8mp_devtype_data, }, @@ -1927,7 +2015,7 @@ static int > flexcan_probe(struct platform_device *pdev) > > struct clk *clk_ipg = NULL, *clk_per = NULL; > > struct flexcan_regs __iomem *regs; > > int err, irq; > > - u8 clk_src = 1; > > + u8 clk_src = 1, can_idx = 0; > > u32 clock_freq = 0; > > > > reg_xceiver = devm_regulator_get_optional(&pdev->dev, "xceiver"); @@ > > -1943,6 +2031,8 @@ static int flexcan_probe(struct platform_device *pdev) > > "clock-frequency", &clock_freq); > > of_property_read_u8(pdev->dev.of_node, > > "fsl,clk-source", &clk_src); > > + of_property_read_u8(pdev->dev.of_node, > > + "fsl,can-index", &can_idx); > > What happens if the DT doesn't contain the can-index? Move this into the > flexcan_setup_stop_mode_scfw() and add error handling. Make sense. > > } > > > > if (!clock_freq) { > > @@ -2019,6 +2109,7 @@ static int flexcan_probe(struct platform_device > *pdev) > > priv->clk_src = clk_src; > > priv->devtype_data = devtype_data; > > priv->reg_xceiver = reg_xceiver; > > + priv->can_idx = can_idx; > > Assign priv->can_idx in flexcan_setup_stop_mode_scfw(), too. OK. Best Regards, Joakim Zhang > > > > if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) { > > priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD | @@ > -2030,6 > > +2121,10 @@ static int flexcan_probe(struct platform_device *pdev) > > priv->can.bittiming_const = &flexcan_bittiming_const; > > } > > > > + err = flexcan_setup_stop_mode(pdev); > > + if (err == -EPROBE_DEFER) > > + return -EPROBE_DEFER; > > + > > pm_runtime_get_noresume(&pdev->dev); > > pm_runtime_set_active(&pdev->dev); > > pm_runtime_enable(&pdev->dev); > > @@ -2043,12 +2138,6 @@ static int flexcan_probe(struct platform_device > *pdev) > > of_can_transceiver(dev); > > devm_can_led_init(dev); > > > > - if (priv->devtype_data->quirks & > FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR) { > > - err = flexcan_setup_stop_mode(pdev); > > - if (err) > > - dev_dbg(&pdev->dev, "failed to setup stop-mode\n"); > > - } > > - > > return 0; > > > > failed_register: > > > > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung West/Dortmund | Phone: +49-231-2826-924 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 10/16/20 8:46 AM, Joakim Zhang wrote: >>> @@ -2019,6 +2109,7 @@ static int flexcan_probe(struct platform_device >> *pdev) >>> priv->clk_src = clk_src; >>> priv->devtype_data = devtype_data; >>> priv->reg_xceiver = reg_xceiver; >>> + priv->can_idx = can_idx; >>> >>> if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) { >>> priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD | @@ >> -2030,6 >>> +2121,10 @@ static int flexcan_probe(struct platform_device *pdev) >>> priv->can.bittiming_const = &flexcan_bittiming_const; >>> } >>> >>> + err = flexcan_setup_stop_mode(pdev); >>> + if (err == -EPROBE_DEFER) >>> + return -EPROBE_DEFER; >> >> You need to free "dev". What about moving this directly before allocating dev. > > Yes, need free "dev" here if defer probe. Flexcan_priv has not allocated > before allocating dev, but we need initialize and check it when setup stop > mode. Right, please take care of freeing all ressouces in case of defered probe. >> Do you have to undo device_set_wakeup_capable() and >> device_set_wakeup_enable() in case of a failure and/or on flexcan_remove()? > > Yes, should invoke device_wakeup_disable() in flexcan_remove. Make it so. regards, Marc
Hi Marc, [...] > > > +static int flexcan_stop_mode_enable_scfw(struct flexcan_priv *priv, > > > +bool enabled) { > > > + u8 idx = priv->can_idx; > > > + u32 rsrc_id, val; > > > + > > > + if (idx == 0) > > > + rsrc_id = IMX_SC_R_CAN_0; > > > + else if (idx == 1) > > > + rsrc_id = IMX_SC_R_CAN_1; > > > + else > > > + rsrc_id = IMX_SC_R_CAN_2; > > > > Can you introduce something like and make use of it: > > > > #define IMX_SC_R_CAN(x) (105 + (x)) > OK. I thought it over again, from my point of view, use macro here directly could be more intuitive, and can achieve a direct jump. If change to above wrapper, on the contrary make confusion, and generate the magic number 105. ☹ > > > + > > > + if (enabled) > > > + val = 1; > > > + else > > > + val = 0; > > > + > > > + /* stop mode request via scu firmware */ > > > + return imx_sc_misc_set_control(priv->sc_ipc_handle, rsrc_id, > > > +IMX_SC_C_IPG_STOP, val); } We still need use IMX_SC_C_IPG_STOP, why not be consistent? Best Regards, Joakim Zhang
On 10/16/20 12:00 PM, Joakim Zhang wrote: >>>> +static int flexcan_stop_mode_enable_scfw(struct flexcan_priv *priv, >>>> +bool enabled) { >>>> + u8 idx = priv->can_idx; >>>> + u32 rsrc_id, val; >>>> + >>>> + if (idx == 0) >>>> + rsrc_id = IMX_SC_R_CAN_0; >>>> + else if (idx == 1) >>>> + rsrc_id = IMX_SC_R_CAN_1; >>>> + else >>>> + rsrc_id = IMX_SC_R_CAN_2; >>> >>> Can you introduce something like and make use of it: >>> >>> #define IMX_SC_R_CAN(x) (105 + (x)) >> OK. > > I thought it over again, from my point of view, use macro here directly could > be more intuitive, and can achieve a direct jump. > If change to above wrapper, on the contrary make confusion, and generate the > magic number 105. ☹ The define should go into the rsrc.h, and probably be: #define IMX_SC_R_CAN(x) (IMX_SC_R_CAN_0 + (x)) and if you change the firmware interface, you probably have more problems :) >>>> + >>>> + if (enabled) >>>> + val = 1; >>>> + else >>>> + val = 0; >>>> + >>>> + /* stop mode request via scu firmware */ >>>> + return imx_sc_misc_set_control(priv->sc_ipc_handle, rsrc_id, >>>> +IMX_SC_C_IPG_STOP, val); } > > We still need use IMX_SC_C_IPG_STOP, why not be consistent? Sorry I don't get what you want to tell me here. Marc
Hi Marc, > -----Original Message----- > From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: 2020年10月16日 18:40 > To: Joakim Zhang <qiangqing.zhang@nxp.com>; robh+dt@kernel.org; > shawnguo@kernel.org; s.hauer@pengutronix.de > Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>; Ying Liu > <victor.liu@nxp.com>; Peng Fan <peng.fan@nxp.com>; > linux-can@vger.kernel.org; Pankaj Bansal <pankaj.bansal@nxp.com>; > netdev@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH 5/6] can: flexcan: add CAN wakeup function for i.MX8QM > > On 10/16/20 12:00 PM, Joakim Zhang wrote: > >>>> +static int flexcan_stop_mode_enable_scfw(struct flexcan_priv > >>>> +*priv, bool enabled) { > >>>> + u8 idx = priv->can_idx; > >>>> + u32 rsrc_id, val; > >>>> + > >>>> + if (idx == 0) > >>>> + rsrc_id = IMX_SC_R_CAN_0; > >>>> + else if (idx == 1) > >>>> + rsrc_id = IMX_SC_R_CAN_1; > >>>> + else > >>>> + rsrc_id = IMX_SC_R_CAN_2; > >>> > >>> Can you introduce something like and make use of it: > >>> > >>> #define IMX_SC_R_CAN(x) (105 + (x)) > >> OK. > > > > I thought it over again, from my point of view, use macro here > > directly could be more intuitive, and can achieve a direct jump. > > If change to above wrapper, on the contrary make confusion, and > > generate the magic number 105. ☹ > > The define should go into the rsrc.h, and probably be: > > #define IMX_SC_R_CAN(x) (IMX_SC_R_CAN_0 + (x)) > > and if you change the firmware interface, you probably have more problems :) rsrc.h: /* * These defines are used to indicate a resource. Resources include peripherals * and bus masters (but not memory regions). Note items from list should * never be changed or removed (only added to at the end of the list). */ Hmm, it just list all resource id, and never be changed. Anyway, if you think above way is better, I will turn to it. > >>>> + > >>>> + if (enabled) > >>>> + val = 1; > >>>> + else > >>>> + val = 0; > >>>> + > >>>> + /* stop mode request via scu firmware */ > >>>> + return imx_sc_misc_set_control(priv->sc_ipc_handle, rsrc_id, > >>>> +IMX_SC_C_IPG_STOP, val); } > > > > We still need use IMX_SC_C_IPG_STOP, why not be consistent? > > Sorry I don't get what you want to tell me here. Need me add IMX_SC_C_IPG_STOP macro in the driver directly? Such as, #define IMX_SC_C_IPG_STOP 52, so that no need to include rsrc.h file in the driver. Best Regards, Joakim Zhang > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung West/Dortmund | Phone: +49-231-2826-924 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
The first patch from Liu Ying aims to export SCU symbols for SoCs w/wo SCU, so that no need to check CONFIG_IMX_SCU in the specific driver. The following patches are for flexcan to add stop mode support for i.MX8QM. Joakim Zhang (5): dt-bindings: can: flexcan: fix fsl,clk-source property dt-bindings: can: flexcan: add fsl,can-index property to indicate a resource can: flexcan: rename macro FLEXCAN_QUIRK_SETUP_STOP_MODE -> FLEXCAN_QUIRK_SETUP_STOP_MODE_GPR can: flexcan: add CAN wakeup function for i.MX8QM can: flexcan: fix ECC function on LS1021A/LX2160A Liu Ying (1): firmware: imx: always export SCU symbols .../bindings/net/can/fsl-flexcan.txt | 7 +- drivers/net/can/flexcan.c | 143 ++++++++++++++---- include/linux/firmware/imx/ipc.h | 15 ++ include/linux/firmware/imx/svc/misc.h | 23 +++ 4 files changed, 160 insertions(+), 28 deletions(-)