| Message ID | 20230518193613.15185-3-jm@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Enable multiple MCAN on AM62x | expand |
On 18.05.2023 14:36:13, Judith Mendez wrote: > Add an hrtimer to MCAN class device. Each MCAN will have its own > hrtimer instantiated if there is no hardware interrupt found and > poll-interval property is defined in device tree M_CAN node. > > The hrtimer will generate a software interrupt every 1 ms. In > hrtimer callback, we check if there is a transaction pending by > reading a register, then process by calling the isr if there is. > > Signed-off-by: Judith Mendez <jm@ti.com> [...] > diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c > index 94dc82644113..3e60cebd9d12 100644 > --- a/drivers/net/can/m_can/m_can_platform.c > +++ b/drivers/net/can/m_can/m_can_platform.c > @@ -5,6 +5,7 @@ > // > // Copyright (C) 2018-19 Texas Instruments Incorporated - http://www.ti.com/ > > +#include <linux/hrtimer.h> > #include <linux/phy/phy.h> > #include <linux/platform_device.h> > > @@ -96,12 +97,40 @@ static int m_can_plat_probe(struct platform_device *pdev) > goto probe_fail; > > addr = devm_platform_ioremap_resource_byname(pdev, "m_can"); > - irq = platform_get_irq_byname(pdev, "int0"); > - if (IS_ERR(addr) || irq < 0) { > - ret = -EINVAL; > + if (IS_ERR(addr)) { > + ret = PTR_ERR(addr); > goto probe_fail; > } > As we don't use an explicit "poll-interval" anymore, this needs some cleanup. The flow should be (pseudo code, error handling omitted): if (device_property_present("interrupts") { platform_get_irq_byname(); polling = false; } else { hrtimer_init(); polling = true; } > + irq = platform_get_irq_byname_optional(pdev, "int0"); Remove the "_optional" and.... > + if (irq == -EPROBE_DEFER) { > + ret = -EPROBE_DEFER; > + goto probe_fail; > + } > + > + if (device_property_present(mcan_class->dev, "interrupts") || > + device_property_present(mcan_class->dev, "interrupt-names")) > + mcan_class->polling = false; ...move the platform_get_irq_byname() here > + else > + mcan_class->polling = true; > + > + if (!mcan_class->polling && irq < 0) { > + ret = -ENXIO; > + dev_err_probe(mcan_class->dev, ret, "IRQ int0 not found, polling not activated\n"); > + goto probe_fail; > + } Remove this check. > + > + if (mcan_class->polling) { > + if (irq > 0) { > + mcan_class->polling = false; > + dev_info(mcan_class->dev, "Polling enabled, using hardware IRQ\n"); Remove this. > + } else { > + dev_dbg(mcan_class->dev, "Polling enabled, initialize hrtimer"); > + hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC, > + HRTIMER_MODE_REL_PINNED); move this backwards, where you set "polling = true" > + } > + } > + > /* message ram could be shared */ > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram"); > if (!res) { > -- > 2.17.1 Marc
Hello Marc, On 5/19/23 2:16 AM, Marc Kleine-Budde wrote: > On 18.05.2023 14:36:13, Judith Mendez wrote: >> Add an hrtimer to MCAN class device. Each MCAN will have its own >> hrtimer instantiated if there is no hardware interrupt found and >> poll-interval property is defined in device tree M_CAN node. >> >> The hrtimer will generate a software interrupt every 1 ms. In >> hrtimer callback, we check if there is a transaction pending by >> reading a register, then process by calling the isr if there is. >> >> Signed-off-by: Judith Mendez <jm@ti.com> > > [...] Missed this poll-interval, thanks. > >> diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c >> index 94dc82644113..3e60cebd9d12 100644 >> --- a/drivers/net/can/m_can/m_can_platform.c >> +++ b/drivers/net/can/m_can/m_can_platform.c >> @@ -5,6 +5,7 @@ >> // >> // Copyright (C) 2018-19 Texas Instruments Incorporated - http://www.ti.com/ >> >> +#include <linux/hrtimer.h> >> #include <linux/phy/phy.h> >> #include <linux/platform_device.h> >> >> @@ -96,12 +97,40 @@ static int m_can_plat_probe(struct platform_device *pdev) >> goto probe_fail; >> >> addr = devm_platform_ioremap_resource_byname(pdev, "m_can"); >> - irq = platform_get_irq_byname(pdev, "int0"); >> - if (IS_ERR(addr) || irq < 0) { >> - ret = -EINVAL; >> + if (IS_ERR(addr)) { >> + ret = PTR_ERR(addr); >> goto probe_fail; >> } >> > > As we don't use an explicit "poll-interval" anymore, this needs some > cleanup. The flow should be (pseudo code, error handling omitted): > > if (device_property_present("interrupts") { > platform_get_irq_byname(); > polling = false; > } else { > hrtimer_init(); > polling = true; > } Ok. > >> + irq = platform_get_irq_byname_optional(pdev, "int0"); > > Remove the "_optional" and.... On V2, you asked to add the _optional?..... > irq = platform_get_irq_byname(pdev, "int0"); use platform_get_irq_byname_optional(), it doesn't print an error message. > >> + if (irq == -EPROBE_DEFER) { >> + ret = -EPROBE_DEFER; >> + goto probe_fail; >> + } >> + >> + if (device_property_present(mcan_class->dev, "interrupts") || >> + device_property_present(mcan_class->dev, "interrupt-names")) >> + mcan_class->polling = false; > > ...move the platform_get_irq_byname() here ok, > >> + else >> + mcan_class->polling = true; >> + >> + if (!mcan_class->polling && irq < 0) { >> + ret = -ENXIO; >> + dev_err_probe(mcan_class->dev, ret, "IRQ int0 not found, polling not activated\n"); >> + goto probe_fail; >> + } > > Remove this check. Should we not go to 'probe fail' if polling is not activated and irq is not found? > >> + >> + if (mcan_class->polling) { >> + if (irq > 0) { >> + mcan_class->polling = false; >> + dev_info(mcan_class->dev, "Polling enabled, using hardware IRQ\n"); > > Remove this. Remove the dev_info? > >> + } else { >> + dev_dbg(mcan_class->dev, "Polling enabled, initialize hrtimer"); >> + hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC, >> + HRTIMER_MODE_REL_PINNED); > > move this backwards, where you set "polling = true" ok, > >> + } >> + } >> + >> /* message ram could be shared */ >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram"); >> if (!res) { >> -- >> 2.17.1 - judith
On 22.05.2023 10:17:38, Judith Mendez wrote: > > > diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c > > > index 94dc82644113..3e60cebd9d12 100644 > > > --- a/drivers/net/can/m_can/m_can_platform.c > > > +++ b/drivers/net/can/m_can/m_can_platform.c > > > @@ -5,6 +5,7 @@ > > > // > > > // Copyright (C) 2018-19 Texas Instruments Incorporated - http://www.ti.com/ > > > +#include <linux/hrtimer.h> > > > #include <linux/phy/phy.h> > > > #include <linux/platform_device.h> > > > @@ -96,12 +97,40 @@ static int m_can_plat_probe(struct platform_device *pdev) > > > goto probe_fail; > > > addr = devm_platform_ioremap_resource_byname(pdev, "m_can"); > > > - irq = platform_get_irq_byname(pdev, "int0"); > > > - if (IS_ERR(addr) || irq < 0) { > > > - ret = -EINVAL; > > > + if (IS_ERR(addr)) { > > > + ret = PTR_ERR(addr); > > > goto probe_fail; > > > } > > > > As we don't use an explicit "poll-interval" anymore, this needs some > > cleanup. The flow should be (pseudo code, error handling omitted): > > > > if (device_property_present("interrupts") { > > platform_get_irq_byname(); > > polling = false; > > } else { > > hrtimer_init(); > > polling = true; > > } > > Ok. > > > > > > + irq = platform_get_irq_byname_optional(pdev, "int0"); > > > > Remove the "_optional" and.... > > On V2, you asked to add the _optional?..... > > > irq = platform_get_irq_byname(pdev, "int0"); > > use platform_get_irq_byname_optional(), it doesn't print an error > message. ACK - I said that back in v2, when there was "poll-interval". But now we don't use "poll-interval" anymore, but test if interrupt properties are present. See again pseudo-code I posted in my last mail: | if (device_property_present("interrupts") { | platform_get_irq_byname(); If this throws an error, it's fatal, bail out. | polling = false; | } else { | hrtimer_init(); | polling = true; | } > > > > > > + if (irq == -EPROBE_DEFER) { > > > + ret = -EPROBE_DEFER; > > > + goto probe_fail; > > > + } > > > + > > > + if (device_property_present(mcan_class->dev, "interrupts") || > > > + device_property_present(mcan_class->dev, "interrupt-names")) > > > + mcan_class->polling = false; > > > > ...move the platform_get_irq_byname() here > > ok, > > > > > > + else > > > + mcan_class->polling = true; > > > + > > > + if (!mcan_class->polling && irq < 0) { > > > + ret = -ENXIO; > > > + dev_err_probe(mcan_class->dev, ret, "IRQ int0 not found, polling not activated\n"); > > > + goto probe_fail; > > > + } > > > > Remove this check. > > Should we not go to 'probe fail' if polling is not activated and irq is not > found? If an interrupt property is present in the DT, we use it - if request IRQ fails, something is broken and we've already bailed out. See above. If there is no interrupt property we use polling. > > > > > > + > > > + if (mcan_class->polling) { > > > + if (irq > 0) { > > > + mcan_class->polling = false; > > > + dev_info(mcan_class->dev, "Polling enabled, using hardware IRQ\n"); > > > > Remove this. > > Remove the dev_info? ACK, this is not possible anymore - we cannot have polling enabled and HW IRQs configured. Marc
Hello, On 5/22/23 1:37 PM, Marc Kleine-Budde wrote: > On 22.05.2023 10:17:38, Judith Mendez wrote: >>>> diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c >>>> index 94dc82644113..3e60cebd9d12 100644 >>>> --- a/drivers/net/can/m_can/m_can_platform.c >>>> +++ b/drivers/net/can/m_can/m_can_platform.c >>>> @@ -5,6 +5,7 @@ >>>> // >>>> // Copyright (C) 2018-19 Texas Instruments Incorporated - http://www.ti.com/ >>>> +#include <linux/hrtimer.h> >>>> #include <linux/phy/phy.h> >>>> #include <linux/platform_device.h> >>>> @@ -96,12 +97,40 @@ static int m_can_plat_probe(struct platform_device *pdev) >>>> goto probe_fail; >>>> addr = devm_platform_ioremap_resource_byname(pdev, "m_can"); >>>> - irq = platform_get_irq_byname(pdev, "int0"); >>>> - if (IS_ERR(addr) || irq < 0) { >>>> - ret = -EINVAL; >>>> + if (IS_ERR(addr)) { >>>> + ret = PTR_ERR(addr); >>>> goto probe_fail; >>>> } >>> >>> As we don't use an explicit "poll-interval" anymore, this needs some >>> cleanup. The flow should be (pseudo code, error handling omitted): >>> >>> if (device_property_present("interrupts") { >>> platform_get_irq_byname(); >>> polling = false; >>> } else { >>> hrtimer_init(); >>> polling = true; >>> } >> >> Ok. >> >>> >>>> + irq = platform_get_irq_byname_optional(pdev, "int0"); >>> >>> Remove the "_optional" and.... >> >> On V2, you asked to add the _optional?..... >> >>> irq = platform_get_irq_byname(pdev, "int0"); >> >> use platform_get_irq_byname_optional(), it doesn't print an error >> message. > > ACK - I said that back in v2, when there was "poll-interval". But now we > don't use "poll-interval" anymore, but test if interrupt properties are > present. > > See again pseudo-code I posted in my last mail: > > | if (device_property_present("interrupts") { > | platform_get_irq_byname(); > > If this throws an error, it's fatal, bail out. > > | polling = false; > | } else { > | hrtimer_init(); > | polling = true; > | } > Ok, will add this then.. >> >>> >>>> + if (irq == -EPROBE_DEFER) { >>>> + ret = -EPROBE_DEFER; >>>> + goto probe_fail; >>>> + } >>>> + >>>> + if (device_property_present(mcan_class->dev, "interrupts") || >>>> + device_property_present(mcan_class->dev, "interrupt-names")) >>>> + mcan_class->polling = false; >>> >>> ...move the platform_get_irq_byname() here >> >> ok, >> >>> >>>> + else >>>> + mcan_class->polling = true; >>>> + >>>> + if (!mcan_class->polling && irq < 0) { >>>> + ret = -ENXIO; >>>> + dev_err_probe(mcan_class->dev, ret, "IRQ int0 not found, polling not activated\n"); >>>> + goto probe_fail; >>>> + } >>> >>> Remove this check. >> >> Should we not go to 'probe fail' if polling is not activated and irq is not >> found? > > If an interrupt property is present in the DT, we use it - if request > IRQ fails, something is broken and we've already bailed out. See above. > If there is no interrupt property we use polling. Got it, thanks. >> >>> >>>> + >>>> + if (mcan_class->polling) { >>>> + if (irq > 0) { >>>> + mcan_class->polling = false; >>>> + dev_info(mcan_class->dev, "Polling enabled, using hardware IRQ\n"); >>> >>> Remove this. >> >> Remove the dev_info? > > ACK, this is not possible anymore - we cannot have polling enabled and > HW IRQs configured. Sounds good, will submit a v7 with these cleanup changes. regards, Judith
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index a5003435802b..cfb3e433c0dd 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -11,6 +11,7 @@ #include <linux/bitfield.h> #include <linux/can/dev.h> #include <linux/ethtool.h> +#include <linux/hrtimer.h> #include <linux/interrupt.h> #include <linux/io.h> #include <linux/iopoll.h> @@ -308,6 +309,9 @@ enum m_can_reg { #define TX_EVENT_MM_MASK GENMASK(31, 24) #define TX_EVENT_TXTS_MASK GENMASK(15, 0) +/* Hrtimer polling interval */ +#define HRTIMER_POLL_INTERVAL 1 + /* The ID and DLC registers are adjacent in M_CAN FIFO memory, * and we can save a (potentially slow) bus round trip by combining * reads and writes to them. @@ -1414,6 +1418,12 @@ static int m_can_start(struct net_device *dev) m_can_enable_all_interrupts(cdev); + if (cdev->polling) { + dev_dbg(cdev->dev, "Start hrtimer\n"); + hrtimer_start(&cdev->hrtimer, ms_to_ktime(HRTIMER_POLL_INTERVAL), + HRTIMER_MODE_REL_PINNED); + } + return 0; } @@ -1571,6 +1581,11 @@ static void m_can_stop(struct net_device *dev) /* disable all interrupts */ m_can_disable_all_interrupts(cdev); + if (cdev->polling) { + dev_dbg(cdev->dev, "Disabling the hrtimer\n"); + hrtimer_cancel(&cdev->hrtimer); + } + /* Set init mode to disengage from the network */ m_can_config_endisable(cdev, true); @@ -1793,6 +1808,18 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb, return NETDEV_TX_OK; } +static enum hrtimer_restart hrtimer_callback(struct hrtimer *timer) +{ + struct m_can_classdev *cdev = container_of(timer, struct + m_can_classdev, hrtimer); + + m_can_isr(0, cdev->net); + + hrtimer_forward_now(timer, ms_to_ktime(HRTIMER_POLL_INTERVAL)); + + return HRTIMER_RESTART; +} + static int m_can_open(struct net_device *dev) { struct m_can_classdev *cdev = netdev_priv(dev); @@ -1831,9 +1858,11 @@ static int m_can_open(struct net_device *dev) err = request_threaded_irq(dev->irq, NULL, m_can_isr, IRQF_ONESHOT, dev->name, dev); - } else { + } else if (!cdev->polling) { err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name, dev); + } else { + cdev->hrtimer.function = &hrtimer_callback; } if (err < 0) { diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h index a839dc71dc9b..e9db5cce4e68 100644 --- a/drivers/net/can/m_can/m_can.h +++ b/drivers/net/can/m_can/m_can.h @@ -15,6 +15,7 @@ #include <linux/device.h> #include <linux/dma-mapping.h> #include <linux/freezer.h> +#include <linux/hrtimer.h> #include <linux/interrupt.h> #include <linux/io.h> #include <linux/iopoll.h> @@ -93,6 +94,9 @@ struct m_can_classdev { int is_peripheral; struct mram_cfg mcfg[MRAM_CFG_NUM]; + + struct hrtimer hrtimer; + bool polling; }; struct m_can_classdev *m_can_class_allocate_dev(struct device *dev, int sizeof_priv); diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c index 94dc82644113..3e60cebd9d12 100644 --- a/drivers/net/can/m_can/m_can_platform.c +++ b/drivers/net/can/m_can/m_can_platform.c @@ -5,6 +5,7 @@ // // Copyright (C) 2018-19 Texas Instruments Incorporated - http://www.ti.com/ +#include <linux/hrtimer.h> #include <linux/phy/phy.h> #include <linux/platform_device.h> @@ -96,12 +97,40 @@ static int m_can_plat_probe(struct platform_device *pdev) goto probe_fail; addr = devm_platform_ioremap_resource_byname(pdev, "m_can"); - irq = platform_get_irq_byname(pdev, "int0"); - if (IS_ERR(addr) || irq < 0) { - ret = -EINVAL; + if (IS_ERR(addr)) { + ret = PTR_ERR(addr); goto probe_fail; } + irq = platform_get_irq_byname_optional(pdev, "int0"); + if (irq == -EPROBE_DEFER) { + ret = -EPROBE_DEFER; + goto probe_fail; + } + + if (device_property_present(mcan_class->dev, "interrupts") || + device_property_present(mcan_class->dev, "interrupt-names")) + mcan_class->polling = false; + else + mcan_class->polling = true; + + if (!mcan_class->polling && irq < 0) { + ret = -ENXIO; + dev_err_probe(mcan_class->dev, ret, "IRQ int0 not found, polling not activated\n"); + goto probe_fail; + } + + if (mcan_class->polling) { + if (irq > 0) { + mcan_class->polling = false; + dev_info(mcan_class->dev, "Polling enabled, using hardware IRQ\n"); + } else { + dev_dbg(mcan_class->dev, "Polling enabled, initialize hrtimer"); + hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC, + HRTIMER_MODE_REL_PINNED); + } + } + /* message ram could be shared */ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram"); if (!res) {
Add an hrtimer to MCAN class device. Each MCAN will have its own hrtimer instantiated if there is no hardware interrupt found and poll-interval property is defined in device tree M_CAN node. The hrtimer will generate a software interrupt every 1 ms. In hrtimer callback, we check if there is a transaction pending by reading a register, then process by calling the isr if there is. Signed-off-by: Judith Mendez <jm@ti.com> --- Changelog: v6: - Move hrtimer stop/start function calls to m_can_open and m_can_close to support power suspend/resume v5: - Change dev_dbg to dev_info if hardware interrupt exists and polling is enabled v4: - No changes v3: - Create a define for 1 ms polling interval - Change plarform_get_irq to optional to not print error msg v2: - Add functionality to check for 'poll-interval' property in MCAN node - Add 'polling' flag in driver to check if device is using polling method - Check for timer polling and hardware interrupt cases, default to hardware interrupt method - Change ns_to_ktime() to ms_to_ktime() --- drivers/net/can/m_can/m_can.c | 31 ++++++++++++++++++++++- drivers/net/can/m_can/m_can.h | 4 +++ drivers/net/can/m_can/m_can_platform.c | 35 +++++++++++++++++++++++--- 3 files changed, 66 insertions(+), 4 deletions(-)