lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <01a229d4-74a0-4e74-bcba-dde95e166ea3@yoseli.org>
Date: Sun, 22 Dec 2024 15:30:47 +0100
From: Jean-Michel Hautbois <jeanmichel.hautbois@...eli.org>
To: Greg Ungerer <gerg@...ux-m68k.org>,
 Geert Uytterhoeven <geert@...ux-m68k.org>,
 Daniel Lezcano <daniel.lezcano@...aro.org>,
 Thomas Gleixner <tglx@...utronix.de>
Cc: linux-m68k@...ts.linux-m68k.org, linux-kernel@...r.kernel.org,
 linux-m68k@...r.kernel.org
Subject: Re: [PATCH 2/2] m68k: m5441x: Add DMA timer support

Hi Greg,

On 22/12/2024 14:41, Greg Ungerer wrote:
> Hi JM,
> 
> On 2/12/24 19:29, Jean-Michel Hautbois wrote:
>> Introduce support for the DMA Timer (DTIM) modules found in ColdFire
>> processors, covering DTIM0 to DTIM3. These are 32-bits timers with 8ns
>> resolution at 125MHz (fsys/2).
>>
>> We use it as a sched_clock as well.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@...eli.org>
>> ---
>>   MAINTAINERS                         |   6 +
>>   arch/m68k/include/asm/m5441xsim.h   |  18 +++
>>   drivers/clocksource/Kconfig         |   9 ++
>>   drivers/clocksource/Makefile        |   1 +
>>   drivers/clocksource/mcf_dma_timer.c | 240 ++++++++++++++++++++++++++ 
>> ++++++++++
>>   5 files changed, 274 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 
>> 1e930c7a58b13d8bbe6bf133ba7b36aa24c2b5e0..21e8646f29e2ee726798bbb28a31e921bcaf6011 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -9047,6 +9047,12 @@ S:    Maintained
>>   F:    drivers/mmc/host/sdhci-esdhc-mcf.c
>>   F:    include/linux/platform_data/mmc-esdhc-mcf.h
>> +FREESCALE COLDFIRE M5441X DMA TIMER DRIVER
>> +M:    Jean-Michel Hautbois <jeanmichel.hautbois@...eli.org>
>> +L:    linux-m68k@...r.kernel.org
>> +S:    Maintained
>> +F:    drivers/clocksource/mcf_dma_timer.c
>> +
>>   FREESCALE DIU FRAMEBUFFER DRIVER
>>   M:    Timur Tabi <timur@...nel.org>
>>   L:    linux-fbdev@...r.kernel.org
>> diff --git a/arch/m68k/include/asm/m5441xsim.h b/arch/m68k/include/ 
>> asm/m5441xsim.h
>> index 
>> f48cf63bd7822fd53c33788128f984585c0c421a..c46f874bf4a24137bc45300cb3013cb13b47f5ec 100644
>> --- a/arch/m68k/include/asm/m5441xsim.h
>> +++ b/arch/m68k/include/asm/m5441xsim.h
>> @@ -101,6 +101,24 @@
>>   #define MCFINT2_PIT3        16
>>   #define MCFINT2_RTC        26
>> +/*
>> + * DMA timer module.
>> + */
>> +#define MCFDMATIMER_BASE0    0xFC070000    /* Base address of DMA 
>> timer 0 */
>> +#define MCFDMATIMER_BASE1    0xFC074000    /* Base address of DMA 
>> timer 1 */
>> +#define MCFDMATIMER_BASE2    0xFC078000    /* Base address of DMA 
>> timer 2 */
>> +#define MCFDMATIMER_BASE3    0xFC07C000    /* Base address of DMA 
>> timer 3 */
>> +
>> +#define MCFDMATIMER_IRQ_DTIM0    (MCFINT0_VECBASE + MCFINT0_TIMER0)
>> +#define MCFDMATIMER_IRQ_DTIM1    (MCFINT0_VECBASE + MCFINT0_TIMER1)
>> +#define MCFDMATIMER_IRQ_DTIM2    (MCFINT0_VECBASE + MCFINT0_TIMER2)
>> +#define MCFDMATIMER_IRQ_DTIM3    (MCFINT0_VECBASE + MCFINT0_TIMER3)
>> +
>> +#define MCFDMATIMER_IRQ_PRIO0    (MCFINTC0_ICR0 + MCFINT0_TIMER0)
>> +#define MCFDMATIMER_IRQ_PRIO1    (MCFINTC0_ICR0 + MCFINT0_TIMER1)
>> +#define MCFDMATIMER_IRQ_PRIO2    (MCFINTC0_ICR0 + MCFINT0_TIMER2)
>> +#define MCFDMATIMER_IRQ_PRIO3    (MCFINTC0_ICR0 + MCFINT0_TIMER3)
> 
> None of these are actually used directly in the driver code below.
> Maybe separate them out into a separate patch.

Sure, along with a DMA timer in device.c, this would make more sense I 
suppose ?

> 
>> +
>>   /*
>>    *  PIT timer module.
>>    */
>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> index 
>> 487c8525996724fbf9c6e9726dabb478d86513b9..3cb3237ef788170f1c2d2bf5db3d47adbeca5664 100644
>> --- a/drivers/clocksource/Kconfig
>> +++ b/drivers/clocksource/Kconfig
>> @@ -763,4 +763,13 @@ config RALINK_TIMER
>>         Enables support for system tick counter present on
>>         Ralink SoCs RT3352 and MT7620.
>> +config COLDFIRE_DMA_TIMERS
>> +    bool "Coldfire DMA timer clocksource"
>> +    depends on M5441x
>> +    select GENERIC_SCHED_CLOCK
>> +    help
>> +      Enables support for the Coldfire M5441x DMA timers as
>> +      a clocksource and a clockevent. It supports oneshot and
>> +      high resolution.
>> +
>>   endmenu
>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> index 
>> 43ef16a4efa6a33bf69ca05a8d66d682826841c9..3a740cd5d52a6f5e68c4e8a0831a2f3306f56941 100644
>> --- a/drivers/clocksource/Makefile
>> +++ b/drivers/clocksource/Makefile
>> @@ -92,3 +92,4 @@ obj-$(CONFIG_GXP_TIMER)            += timer-gxp.o
>>   obj-$(CONFIG_CLKSRC_LOONGSON1_PWM)    += timer-loongson1-pwm.o
>>   obj-$(CONFIG_EP93XX_TIMER)        += timer-ep93xx.o
>>   obj-$(CONFIG_RALINK_TIMER)        += timer-ralink.o
>> +obj-$(CONFIG_COLDFIRE_DMA_TIMERS)    += mcf_dma_timer.o
>> diff --git a/drivers/clocksource/mcf_dma_timer.c b/drivers/ 
>> clocksource/mcf_dma_timer.c
>> new file mode 100644
>> index 
>> 0000000000000000000000000000000000000000..f5cb0f92fcecffcd51a6e73ca38e5a8d8bade0a9
>> --- /dev/null
>> +++ b/drivers/clocksource/mcf_dma_timer.c
>> @@ -0,0 +1,240 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * mcf_dma_timer.c -- Freescale ColdFire DMA Timer.
>> + *
>> + * Copyright (C) 2024, Jean-Michel Hautbois 
>> <jeanmichel.hautbois@...eli.org>
>> + *
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clockchips.h>
>> +#include <linux/clocksource.h>
>> +#include <linux/io.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/sched/clock.h>
>> +#include <linux/sched_clock.h>
>> +
>> +#include <asm/machdep.h>
>> +#include <asm/coldfire.h>
>> +#include <asm/mcfpit.h>
>> +#include <asm/mcfsim.h>
>> +
>> +struct dmatmr_regs {
>> +    u16 *dtmr;
>> +    u8 *dtxmr;
>> +    u8 *dter;
>> +    u32 *dtrr;
>> +    u32 *dtcr;
>> +    u32 *dtcn;
>> +};
>> +
>> +struct dmatmr_priv {
>> +    void __iomem *base;
>> +    struct clk *clk;
>> +    struct platform_device *pdev;
>> +    unsigned long rate;
>> +    raw_spinlock_t lock;
>> +    struct clock_event_device ced;
>> +    struct clocksource cs;
>> +    int id;
>> +    struct dmatmr_regs regs;
>> +};
>> +
>> +static u64 sched_dtim_clk_val;
>> +static void __iomem *dmatmr_sched_clk_counter;
>> +
>> +static struct dmatmr_priv *ced_to_priv(struct clock_event_device *ced)
>> +{
>> +    return container_of(ced, struct dmatmr_priv, ced);
>> +}
>> +
>> +static void sys_dtim_init(struct dmatmr_priv *priv)
>> +{
>> +    __raw_writel((priv->rate / HZ) - 1, priv->regs.dtrr);
>> +    __raw_writew(BIT(4) | BIT(1) | BIT(0), priv->regs.dtmr);
>> +    __raw_writeb(0, priv->regs.dtxmr);
>> +    __raw_writeb(1, priv->regs.dter);
>> +
>> +    dev_info(&priv->pdev->dev, "Initialized for sched_clock at %d 
>> hz", HZ);
>> +}
>> +
>> +static inline u64 notrace read_dtcn(void)
>> +{
>> +    return __raw_readl(dmatmr_sched_clk_counter);
>> +}
>> +
>> +static u64 notrace sys_dtim_read(void)
>> +{
>> +    return sched_dtim_clk_val + read_dtcn();
>> +}
>> +
>> +static u64 cfv4_read_dtimvalue(struct clocksource *cs)
>> +{
>> +    return sys_dtim_read();
>> +}
>> +
>> +static int cfv4_set_next_event(unsigned long delta,
>> +    struct clock_event_device *dev)
>> +{
>> +    struct dmatmr_priv *priv = ced_to_priv(dev);
>> +    unsigned long flags;
>> +
>> +    raw_spin_lock_irqsave(&priv->lock, flags);
>> +    /* read timer value */
>> +    sched_dtim_clk_val += read_dtcn();
>> +    raw_spin_unlock_irqrestore(&priv->lock, flags);
>> +
>> +    /* reset timer with delta cycle */
>> +    __raw_writew(0, priv->regs.dtmr);
>> +    __raw_writel(delta, priv->regs.dtrr);
>> +    __raw_writew(BIT(4) | BIT(1) | BIT(0), priv->regs.dtmr);
>> +
>> +    return 0;
>> +}
>> +
>> +static int cfv4_set_oneshot(struct clock_event_device *dev)
>> +{
>> +    struct dmatmr_priv *priv = ced_to_priv(dev);
>> +    unsigned long flags;
>> +
>> +    /* read timer value */
>> +    raw_spin_lock_irqsave(&priv->lock, flags);
>> +    sched_dtim_clk_val += read_dtcn();
>> +    raw_spin_unlock_irqrestore(&priv->lock, flags);
>> +
>> +    __raw_writew(0, priv->regs.dtmr);
>> +    return 0;
>> +}
>> +
>> +static irqreturn_t coldfire_dtim_clk_irq(int irq, void *dev)
>> +{
>> +    struct dmatmr_priv *priv = dev;
>> +    unsigned long flags;
>> +
>> +    /* acknowledge the IRQ */
>> +    __raw_writeb(BIT(0) | BIT(1), priv->regs.dter);
>> +
>> +    /* read timer value */
>> +    raw_spin_lock_irqsave(&priv->lock, flags);
>> +    sched_dtim_clk_val += read_dtcn();
>> +    raw_spin_unlock_irqrestore(&priv->lock, flags);
>> +
>> +    /* restart counter */
>> +    __raw_writel(0, dmatmr_sched_clk_counter);
>> +
>> +    priv->ced.event_handler(&priv->ced);
>> +
>> +    return IRQ_HANDLED;
>> +}
>> +
>> +static void mcf_dma_register_clocksource(struct dmatmr_priv *priv)
>> +{
>> +    struct clocksource *cs = &priv->cs;
>> +
>> +    cs->name = dev_name(&priv->pdev->dev);
>> +    cs->rating = 250;
>> +    cs->mask = CLOCKSOURCE_MASK(30);
>> +    cs->read = cfv4_read_dtimvalue;
>> +    cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
>> +
>> +    dev_info(&priv->pdev->dev, "registering clocksource\n");
>> +
>> +    clocksource_register_hz(cs, priv->rate);
>> +}
>> +
>> +static void mcf_dma_register_clockevent(struct dmatmr_priv *priv)
>> +{
>> +    struct clock_event_device *ced = &priv->ced;
>> +
>> +    ced->name = dev_name(&priv->pdev->dev);
>> +    ced->features = CLOCK_EVT_FEAT_ONESHOT;
>> +    ced->rating = 250;
>> +    ced->shift = 20;
>> +    ced->cpumask = cpumask_of(smp_processor_id());
>> +    ced->set_state_oneshot = cfv4_set_oneshot;
>> +    ced->set_next_event = cfv4_set_next_event;
>> +
>> +    dev_info(&priv->pdev->dev, "registering clockevent\n");
>> +
>> +    clockevents_config_and_register(ced, priv->rate, 2, 0xffffffff);
>> +}
>> +
>> +static void mcf_dma_init_registers(struct dmatmr_priv *priv)
>> +{
>> +    struct dmatmr_regs *regs = &priv->regs;
>> +
>> +    regs->dtmr = priv->base + 0x0;
>> +    regs->dtxmr = priv->base + 0x2;
>> +    regs->dter = priv->base + 0x3;
>> +    regs->dtrr = priv->base + 0x4;
>> +    regs->dtcr = priv->base + 0x8;
>> +    regs->dtcn = priv->base + 0xc;
> 
> Why define pointers for each register like this and not define
> the offsets and use those directly with the read/write calls?
> Something like this:
> 
>      #define DTER 3
> 
>      __raw_writeb(BIT(0) | BIT(1), priv->base + DTER)
> 
> For one thing the setup of variables here is losing the "void __iomem *" 
> type.
> I guess you get away with in this case because the __raw_read/__raw_write
> macros force volatile and casting and directly access.

Thanks, I will do that.
I also removed the raw_spin_locks as we always are in a IRQ disabled 
context when I use those. And in
sys_dtim_read() I changed it to:
return READ_ONCE(sched_dtim_clk_val) + read_dtcn();

If it makes sense ?

I sometimes have the issue I mentionned in 
https://lore.kernel.org/linux-m68k/125431ab-e486-43a7-a903-76c831da4985@yoseli.org/ 
: using cyclictest with high prio and short interval with a very high 
cpu usage and network with iperf3 or hping3 I get an unexpected IRQ vector.

I don't know how to debug this, but I think it is related to this 
driver, as I don't think I observed those in another context.

Thanks !
JM

> Regards
> Greg
> 
> 
>> +}
>> +
>> +static int __init mcf_dma_timer_probe(struct platform_device *pdev)
>> +{
>> +    struct dmatmr_priv *priv;
>> +    struct resource *prio_reg;
>> +    int irq, ret;
>> +
>> +    priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>> +    if (!priv)
>> +        return -ENOMEM;
>> +
>> +    priv->pdev = pdev;
>> +    platform_set_drvdata(pdev, priv);
>> +
>> +    irq = platform_get_irq(pdev, 0);
>> +    if (irq < 0)
>> +        return irq;
>> +
>> +    priv->base = devm_platform_ioremap_resource(pdev, 0);
>> +    if (IS_ERR(priv->base))
>> +        return PTR_ERR(priv->base);
>> +
>> +    priv->id = pdev->id;
>> +
>> +    ret = devm_request_irq(&pdev->dev, irq, coldfire_dtim_clk_irq, 
>> IRQF_TIMER,
>> +                   dev_name(&pdev->dev), priv);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "failed to request irq %d\n", irq);
>> +        return ret;
>> +    }
>> +
>> +    prio_reg = platform_get_resource_byname(pdev, IORESOURCE_REG, 
>> "prio_reg");
>> +    if (prio_reg) {
>> +        /* Enhance the interrupt priority */
>> +        __raw_writeb(5, prio_reg->start);
>> +    }
>> +
>> +    priv->clk = devm_clk_get_enabled(&pdev->dev, NULL);
>> +    if (IS_ERR(priv->clk)) {
>> +        dev_err(&pdev->dev, "failed to get clock\n");
>> +        return PTR_ERR(priv->clk);
>> +    }
>> +
>> +    priv->rate = clk_get_rate(priv->clk);
>> +
>> +    raw_spin_lock_init(&priv->lock);
>> +
>> +    mcf_dma_init_registers(priv);
>> +    dmatmr_sched_clk_counter = priv->regs.dtcn;
>> +
>> +    mcf_dma_register_clockevent(priv);
>> +    mcf_dma_register_clocksource(priv);
>> +
>> +    /* initialize the system timer */
>> +    sys_dtim_init(priv);
>> +
>> +    sched_clock_register(sys_dtim_read, 32, priv->rate);
>> +
>> +    return 0;
>> +}
>> +
>> +static struct platform_driver mcf_platform_driver = {
>> +    .driver        = {
>> +        .name    = "mcftmr",
>> +    },
>> +};
>> +
>> +builtin_platform_driver_probe(mcf_platform_driver, mcf_dma_timer_probe);
>>
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ