diff mbox series

[v8,05/26] clocksource: Add driver for the Ingenic JZ47xx OST

Message ID 20181212220922.18759-6-paul@crapouillou.net
State Changes Requested
Headers show
Series Ingenic TCU patchset v8 | expand

Commit Message

Paul Cercueil Dec. 12, 2018, 10:09 p.m. UTC
From: Maarten ter Huurne <maarten@treewalker.org>

OST is the OS Timer, a 64-bit timer/counter with buffered reading.

SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
JZ4780 have a 64-bit OST.

This driver will register both a clocksource and a sched_clock to the
system.

Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
     v5: New patch
    
     v6: - Get rid of SoC IDs; pass pointer to ingenic_ost_soc_info as
           devicetree match data instead.
         - Use device_get_match_data() instead of the of_* variant
         - Handle error of dev_get_regmap() properly
    
     v7: Fix section mismatch by using builtin_platform_driver_probe()

     v8: builtin_platform_driver_probe() does not work anymore in
         4.20-rc6? The probe function won't be called. Work around this
	 for now by using late_initcall.

 drivers/clocksource/Kconfig       |   8 ++
 drivers/clocksource/Makefile      |   1 +
 drivers/clocksource/ingenic-ost.c | 215 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 224 insertions(+)
 create mode 100644 drivers/clocksource/ingenic-ost.c

Comments

Mathieu Malaterre Jan. 23, 2019, 12:58 p.m. UTC | #1
On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> From: Maarten ter Huurne <maarten@treewalker.org>
>
> OST is the OS Timer, a 64-bit timer/counter with buffered reading.
>
> SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
> JZ4780 have a 64-bit OST.
>
> This driver will register both a clocksource and a sched_clock to the
> system.
>
> Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>
> Notes:
>      v5: New patch
>
>      v6: - Get rid of SoC IDs; pass pointer to ingenic_ost_soc_info as
>            devicetree match data instead.
>          - Use device_get_match_data() instead of the of_* variant
>          - Handle error of dev_get_regmap() properly
>
>      v7: Fix section mismatch by using builtin_platform_driver_probe()
>
>      v8: builtin_platform_driver_probe() does not work anymore in
>          4.20-rc6? The probe function won't be called. Work around this
>          for now by using late_initcall.
>
>  drivers/clocksource/Kconfig       |   8 ++
>  drivers/clocksource/Makefile      |   1 +
>  drivers/clocksource/ingenic-ost.c | 215 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 224 insertions(+)
>  create mode 100644 drivers/clocksource/ingenic-ost.c
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 4e69af15c3e7..e0646878b0de 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -648,4 +648,12 @@ config INGENIC_TIMER
>         help
>           Support for the timer/counter unit of the Ingenic JZ SoCs.
>
> +config INGENIC_OST
> +       bool "Ingenic JZ47xx Operating System Timer"
> +       depends on MIPS || COMPILE_TEST
> +       depends on COMMON_CLK
> +       select INGENIC_TIMER
> +       help
> +         Support for the OS Timer of the Ingenic JZ4770 or similar SoC.
> +
>  endmenu
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 7c8f790dcf67..7faa8108574a 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -75,6 +75,7 @@ obj-$(CONFIG_ASM9260_TIMER)           += asm9260_timer.o
>  obj-$(CONFIG_H8300_TMR8)               += h8300_timer8.o
>  obj-$(CONFIG_H8300_TMR16)              += h8300_timer16.o
>  obj-$(CONFIG_H8300_TPU)                        += h8300_tpu.o
> +obj-$(CONFIG_INGENIC_OST)              += ingenic-ost.o
>  obj-$(CONFIG_INGENIC_TIMER)            += ingenic-timer.o
>  obj-$(CONFIG_CLKSRC_ST_LPC)            += clksrc_st_lpc.o
>  obj-$(CONFIG_X86_NUMACHIP)             += numachip.o
> diff --git a/drivers/clocksource/ingenic-ost.c b/drivers/clocksource/ingenic-ost.c
> new file mode 100644
> index 000000000000..cc0fee3efd29
> --- /dev/null
> +++ b/drivers/clocksource/ingenic-ost.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * JZ47xx SoCs TCU Operating System Timer driver
> + *
> + * Copyright (C) 2016 Maarten ter Huurne <maarten@treewalker.org>
> + * Copyright (C) 2018 Paul Cercueil <paul@crapouillou.net>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clocksource.h>
> +#include <linux/mfd/ingenic-tcu.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/regmap.h>
> +#include <linux/sched_clock.h>
> +
> +#include "ingenic-timer.h"
> +
> +#define TCU_OST_TCSR_MASK      0xc0
> +#define TCU_OST_TCSR_CNT_MD    BIT(15)
> +
> +#define TCU_OST_CHANNEL                15
> +
> +struct ingenic_ost_soc_info {
> +       bool is64bit;
> +};
> +
> +struct ingenic_ost {
> +       struct regmap *map;
> +       struct clk *clk;
> +
> +       struct clocksource cs;
> +};
> +
> +static u64 notrace ingenic_ost_read_cntl(void)
> +{
> +       /* Bypass the regmap here as we must return as soon as possible */
> +       return readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
> +}
> +
> +static u64 notrace ingenic_ost_read_cnth(void)
> +{
> +       /* Bypass the regmap here as we must return as soon as possible */
> +       return readl(ingenic_tcu_base + TCU_REG_OST_CNTH);
> +}
> +
> +static u64 notrace ingenic_ost_clocksource_read(struct clocksource *cs)
> +{
> +       u32 val1, val2;
> +       u64 count, recount;
> +       s64 diff;
> +
> +       /*
> +        * The buffering of the upper 32 bits of the timer prevents wrong
> +        * results from the bottom 32 bits overflowing due to the timer ticking
> +        * along. However, it does not prevent wrong results from simultaneous
> +        * reads of the timer, which could reset the buffer mid-read.
> +        * Since this kind of wrong read can happen only when the bottom bits
> +        * overflow, there will be minutes between wrong reads, so if we read
> +        * twice in succession, at least one of the reads will be correct.
> +        */
> +
> +       /* Bypass the regmap here as we must return as soon as possible */
> +       val1 = readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
> +       val2 = readl(ingenic_tcu_base + TCU_REG_OST_CNTHBUF);
> +       count = (u64)val1 | (u64)val2 << 32;
> +
> +       val1 = readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
> +       val2 = readl(ingenic_tcu_base + TCU_REG_OST_CNTHBUF);
> +       recount = (u64)val1 | (u64)val2 << 32;
> +
> +       /*
> +        * A wrong read will produce a result that is 1<<32 too high: the bottom
> +        * part from before overflow and the upper part from after overflow.
> +        * Therefore, the lower value of the two reads is the correct value.
> +        */
> +
> +       diff = (s64)(recount - count);
> +       if (unlikely(diff < 0))
> +               count = recount;
> +
> +       return count;
> +}
> +
> +static int __init ingenic_ost_probe(struct platform_device *pdev)
> +{
> +       const struct ingenic_ost_soc_info *soc_info;
> +       struct device *dev = &pdev->dev;
> +       struct ingenic_ost *ost;
> +       struct clocksource *cs;
> +       unsigned long rate, flags;
> +       int err;
> +
> +       soc_info = device_get_match_data(dev);
> +       if (!soc_info)
> +               return -EINVAL;
> +
> +       ost = devm_kzalloc(dev, sizeof(*ost), GFP_KERNEL);
> +       if (!ost)
> +               return -ENOMEM;
> +
> +       ost->map = dev_get_regmap(dev->parent, NULL);
> +       if (!ost->map) {
> +               dev_err(dev, "regmap not found\n");
> +               return -EINVAL;
> +       }
> +
> +       ost->clk = devm_clk_get(dev, "ost");
> +       if (IS_ERR(ost->clk))
> +               return PTR_ERR(ost->clk);
> +
> +       err = clk_prepare_enable(ost->clk);
> +       if (err)
> +               return err;
> +
> +       /* Clear counter high/low registers */
> +       if (soc_info->is64bit)
> +               regmap_write(ost->map, TCU_REG_OST_CNTL, 0);
> +       regmap_write(ost->map, TCU_REG_OST_CNTH, 0);
> +
> +       /* Don't reset counter at compare value. */
> +       regmap_update_bits(ost->map, TCU_REG_OST_TCSR,
> +                          TCU_OST_TCSR_MASK, TCU_OST_TCSR_CNT_MD);
> +
> +       rate = clk_get_rate(ost->clk);
> +
> +       /* Enable OST TCU channel */
> +       regmap_write(ost->map, TCU_REG_TESR, BIT(TCU_OST_CHANNEL));
> +
> +       cs = &ost->cs;
> +       cs->name        = "ingenic-ost";
> +       cs->rating      = 320;
> +       cs->flags       = CLOCK_SOURCE_IS_CONTINUOUS;
> +
> +       if (soc_info->is64bit) {
> +               cs->mask = CLOCKSOURCE_MASK(64);
> +               cs->read = ingenic_ost_clocksource_read;
> +       } else {
> +               cs->mask = CLOCKSOURCE_MASK(32);
> +               cs->read = (u64 (*)(struct clocksource *))ingenic_ost_read_cnth;

The function is declared as ingenic_ost_read_cnth(void), are you sure
this is going to work ?

> +       }
> +
> +       err = clocksource_register_hz(cs, rate);
> +       if (err) {
> +               dev_err(dev, "clocksource registration failed: %d\n", err);
> +               clk_disable_unprepare(ost->clk);
> +               return err;
> +       }
> +
> +       /* Cannot register a sched_clock with interrupts on */
> +       local_irq_save(flags);
> +       if (soc_info->is64bit)
> +               sched_clock_register(ingenic_ost_read_cntl, 32, rate);
> +       else
> +               sched_clock_register(ingenic_ost_read_cnth, 32, rate);
> +       local_irq_restore(flags);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int ingenic_ost_suspend(struct device *dev)
> +{
> +       struct ingenic_ost *ost = dev_get_drvdata(dev);
> +
> +       clk_disable(ost->clk);
> +       return 0;
> +}
> +
> +static int ingenic_ost_resume(struct device *dev)
> +{
> +       struct ingenic_ost *ost = dev_get_drvdata(dev);
> +
> +       return clk_enable(ost->clk);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(ingenic_ost_pm_ops, ingenic_ost_suspend,
> +                        ingenic_ost_resume);
> +#define INGENIC_OST_PM_OPS (&ingenic_ost_pm_ops)
> +#else
> +#define INGENIC_OST_PM_OPS NULL
> +#endif /* CONFIG_PM_SUSPEND */
> +
> +static const struct ingenic_ost_soc_info jz4725b_ost_soc_info = {
> +       .is64bit = false,
> +};
> +
> +static const struct ingenic_ost_soc_info jz4770_ost_soc_info = {
> +       .is64bit = true,
> +};
> +
> +static const struct of_device_id ingenic_ost_of_match[] = {
> +       { .compatible = "ingenic,jz4725b-ost", .data = &jz4725b_ost_soc_info, },
> +       { .compatible = "ingenic,jz4770-ost",  .data = &jz4770_ost_soc_info,  },
> +       { }
> +};
> +
> +static struct platform_driver ingenic_ost_driver = {
> +       .driver = {
> +               .name   = "ingenic-ost",
> +               .pm     = INGENIC_OST_PM_OPS,
> +               .of_match_table = ingenic_ost_of_match,
> +       },
> +};
> +
> +/* FIXME: Using device_initcall (or buildin_platform_driver_probe) results
> + * in the driver not being probed at all. It worked in 4.18...
> + */
> +static int __init ingenic_ost_drv_register(void)
> +{
> +       return platform_driver_probe(&ingenic_ost_driver,
> +                                    ingenic_ost_probe);
> +}
> +late_initcall(ingenic_ost_drv_register);
> --
> 2.11.0
>
Guenter Roeck Jan. 23, 2019, 2:31 p.m. UTC | #2
On 1/23/19 4:58 AM, Mathieu Malaterre wrote:
> On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil <paul@crapouillou.net> wrote:
>>
>> From: Maarten ter Huurne <maarten@treewalker.org>
>>
>> OST is the OS Timer, a 64-bit timer/counter with buffered reading.
>>
>> SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
>> JZ4780 have a 64-bit OST.
>>
>> This driver will register both a clocksource and a sched_clock to the
>> system.
>>
>> Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> ---
>>
>> Notes:
>>       v5: New patch
>>
>>       v6: - Get rid of SoC IDs; pass pointer to ingenic_ost_soc_info as
>>             devicetree match data instead.
>>           - Use device_get_match_data() instead of the of_* variant
>>           - Handle error of dev_get_regmap() properly
>>
>>       v7: Fix section mismatch by using builtin_platform_driver_probe()
>>
>>       v8: builtin_platform_driver_probe() does not work anymore in
>>           4.20-rc6? The probe function won't be called. Work around this
>>           for now by using late_initcall.
>>

Did anyone notice this ? Either something is wrong with the driver, or
with the kernel core. Hacking around it seems like the worst possible
"solution".

Guenter
Paul Cercueil Jan. 23, 2019, 5:25 p.m. UTC | #3
Hi,

Le mer. 23 janv. 2019 à 11:31, Guenter Roeck <linux@roeck-us.net> a 
écrit :
> On 1/23/19 4:58 AM, Mathieu Malaterre wrote:
>> On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil 
>> <paul@crapouillou.net> wrote:
>>> 
>>> From: Maarten ter Huurne <maarten@treewalker.org>
>>> 
>>> OST is the OS Timer, a 64-bit timer/counter with buffered reading.
>>> 
>>> SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
>>> JZ4780 have a 64-bit OST.
>>> 
>>> This driver will register both a clocksource and a sched_clock to 
>>> the
>>> system.
>>> 
>>> Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>> ---
>>> 
>>> Notes:
>>>       v5: New patch
>>> 
>>>       v6: - Get rid of SoC IDs; pass pointer to 
>>> ingenic_ost_soc_info as
>>>             devicetree match data instead.
>>>           - Use device_get_match_data() instead of the of_* variant
>>>           - Handle error of dev_get_regmap() properly
>>> 
>>>       v7: Fix section mismatch by using 
>>> builtin_platform_driver_probe()
>>> 
>>>       v8: builtin_platform_driver_probe() does not work anymore in
>>>           4.20-rc6? The probe function won't be called. Work around 
>>> this
>>>           for now by using late_initcall.
>>> 
> 
> Did anyone notice this ? Either something is wrong with the driver, or
> with the kernel core. Hacking around it seems like the worst possible
> "solution".

I can confirm it still happens on 5.0-rc3.

Just to explain what I'm doing:

My ingenic-timer driver probes with builtin_platform_driver_probe (this 
works),
and then calls of_platform_populate to probe its children. This driver,
ingenic-ost, is one of them, and will fail to probe with
builtin_platform_driver_probe.

-Paul
Paul Cercueil Jan. 23, 2019, 5:27 p.m. UTC | #4
(re-send, in plain text this time, sorry for those who got it twice)


Le mer. 23 janv. 2019 à 9:58, Mathieu Malaterre <malat@debian.org> a 
écrit :
> On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil <paul@crapouillou.net> 
> wrote:
>> 
>>  From: Maarten ter Huurne <maarten@treewalker.org>
>> 
>>  OST is the OS Timer, a 64-bit timer/counter with buffered reading.
>> 
>>  SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
>>  JZ4780 have a 64-bit OST.
>> 
>>  This driver will register both a clocksource and a sched_clock to 
>> the
>>  system.
>> 
>>  Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>> 
>>  Notes:
>>       v5: New patch
>> 
>>       v6: - Get rid of SoC IDs; pass pointer to ingenic_ost_soc_info 
>> as
>>             devicetree match data instead.
>>           - Use device_get_match_data() instead of the of_* variant
>>           - Handle error of dev_get_regmap() properly
>> 
>>       v7: Fix section mismatch by using 
>> builtin_platform_driver_probe()
>> 
>>       v8: builtin_platform_driver_probe() does not work anymore in
>>           4.20-rc6? The probe function won't be called. Work around 
>> this
>>           for now by using late_initcall.
>> 
>>   drivers/clocksource/Kconfig       |   8 ++
>>   drivers/clocksource/Makefile      |   1 +
>>   drivers/clocksource/ingenic-ost.c | 215 
>> ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 224 insertions(+)
>>   create mode 100644 drivers/clocksource/ingenic-ost.c
>> 
>>  diff --git a/drivers/clocksource/Kconfig 
>> b/drivers/clocksource/Kconfig
>>  index 4e69af15c3e7..e0646878b0de 100644
>>  --- a/drivers/clocksource/Kconfig
>>  +++ b/drivers/clocksource/Kconfig
>>  @@ -648,4 +648,12 @@ config INGENIC_TIMER
>>          help
>>            Support for the timer/counter unit of the Ingenic JZ SoCs.
>> 
>>  +config INGENIC_OST
>>  +       bool "Ingenic JZ47xx Operating System Timer"
>>  +       depends on MIPS || COMPILE_TEST
>>  +       depends on COMMON_CLK
>>  +       select INGENIC_TIMER
>>  +       help
>>  +         Support for the OS Timer of the Ingenic JZ4770 or similar 
>> SoC.
>>  +
>>   endmenu
>>  diff --git a/drivers/clocksource/Makefile 
>> b/drivers/clocksource/Makefile
>>  index 7c8f790dcf67..7faa8108574a 100644
>>  --- a/drivers/clocksource/Makefile
>>  +++ b/drivers/clocksource/Makefile
>>  @@ -75,6 +75,7 @@ obj-$(CONFIG_ASM9260_TIMER)           += 
>> asm9260_timer.o
>>   obj-$(CONFIG_H8300_TMR8)               += h8300_timer8.o
>>   obj-$(CONFIG_H8300_TMR16)              += h8300_timer16.o
>>   obj-$(CONFIG_H8300_TPU)                        += h8300_tpu.o
>>  +obj-$(CONFIG_INGENIC_OST)              += ingenic-ost.o
>>   obj-$(CONFIG_INGENIC_TIMER)            += ingenic-timer.o
>>   obj-$(CONFIG_CLKSRC_ST_LPC)            += clksrc_st_lpc.o
>>   obj-$(CONFIG_X86_NUMACHIP)             += numachip.o
>>  diff --git a/drivers/clocksource/ingenic-ost.c 
>> b/drivers/clocksource/ingenic-ost.c
>>  new file mode 100644
>>  index 000000000000..cc0fee3efd29
>>  --- /dev/null
>>  +++ b/drivers/clocksource/ingenic-ost.c
>>  @@ -0,0 +1,215 @@
>>  +// SPDX-License-Identifier: GPL-2.0
>>  +/*
>>  + * JZ47xx SoCs TCU Operating System Timer driver
>>  + *
>>  + * Copyright (C) 2016 Maarten ter Huurne <maarten@treewalker.org>
>>  + * Copyright (C) 2018 Paul Cercueil <paul@crapouillou.net>
>>  + */
>>  +
>>  +#include <linux/clk.h>
>>  +#include <linux/clocksource.h>
>>  +#include <linux/mfd/ingenic-tcu.h>
>>  +#include <linux/of.h>
>>  +#include <linux/platform_device.h>
>>  +#include <linux/pm.h>
>>  +#include <linux/regmap.h>
>>  +#include <linux/sched_clock.h>
>>  +
>>  +#include "ingenic-timer.h"
>>  +
>>  +#define TCU_OST_TCSR_MASK      0xc0
>>  +#define TCU_OST_TCSR_CNT_MD    BIT(15)
>>  +
>>  +#define TCU_OST_CHANNEL                15
>>  +
>>  +struct ingenic_ost_soc_info {
>>  +       bool is64bit;
>>  +};
>>  +
>>  +struct ingenic_ost {
>>  +       struct regmap *map;
>>  +       struct clk *clk;
>>  +
>>  +       struct clocksource cs;
>>  +};
>>  +
>>  +static u64 notrace ingenic_ost_read_cntl(void)
>>  +{
>>  +       /* Bypass the regmap here as we must return as soon as 
>> possible */
>>  +       return readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
>>  +}
>>  +
>>  +static u64 notrace ingenic_ost_read_cnth(void)
>>  +{
>>  +       /* Bypass the regmap here as we must return as soon as 
>> possible */
>>  +       return readl(ingenic_tcu_base + TCU_REG_OST_CNTH);
>>  +}
>>  +
>>  +static u64 notrace ingenic_ost_clocksource_read(struct clocksource 
>> *cs)
>>  +{
>>  +       u32 val1, val2;
>>  +       u64 count, recount;
>>  +       s64 diff;
>>  +
>>  +       /*
>>  +        * The buffering of the upper 32 bits of the timer prevents 
>> wrong
>>  +        * results from the bottom 32 bits overflowing due to the 
>> timer ticking
>>  +        * along. However, it does not prevent wrong results from 
>> simultaneous
>>  +        * reads of the timer, which could reset the buffer 
>> mid-read.
>>  +        * Since this kind of wrong read can happen only when the 
>> bottom bits
>>  +        * overflow, there will be minutes between wrong reads, so 
>> if we read
>>  +        * twice in succession, at least one of the reads will be 
>> correct.
>>  +        */
>>  +
>>  +       /* Bypass the regmap here as we must return as soon as 
>> possible */
>>  +       val1 = readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
>>  +       val2 = readl(ingenic_tcu_base + TCU_REG_OST_CNTHBUF);
>>  +       count = (u64)val1 | (u64)val2 << 32;
>>  +
>>  +       val1 = readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
>>  +       val2 = readl(ingenic_tcu_base + TCU_REG_OST_CNTHBUF);
>>  +       recount = (u64)val1 | (u64)val2 << 32;
>>  +
>>  +       /*
>>  +        * A wrong read will produce a result that is 1<<32 too 
>> high: the bottom
>>  +        * part from before overflow and the upper part from after 
>> overflow.
>>  +        * Therefore, the lower value of the two reads is the 
>> correct value.
>>  +        */
>>  +
>>  +       diff = (s64)(recount - count);
>>  +       if (unlikely(diff < 0))
>>  +               count = recount;
>>  +
>>  +       return count;
>>  +}
>>  +
>>  +static int __init ingenic_ost_probe(struct platform_device *pdev)
>>  +{
>>  +       const struct ingenic_ost_soc_info *soc_info;
>>  +       struct device *dev = &pdev->dev;
>>  +       struct ingenic_ost *ost;
>>  +       struct clocksource *cs;
>>  +       unsigned long rate, flags;
>>  +       int err;
>>  +
>>  +       soc_info = device_get_match_data(dev);
>>  +       if (!soc_info)
>>  +               return -EINVAL;
>>  +
>>  +       ost = devm_kzalloc(dev, sizeof(*ost), GFP_KERNEL);
>>  +       if (!ost)
>>  +               return -ENOMEM;
>>  +
>>  +       ost->map = dev_get_regmap(dev->parent, NULL);
>>  +       if (!ost->map) {
>>  +               dev_err(dev, "regmap not found\n");
>>  +               return -EINVAL;
>>  +       }
>>  +
>>  +       ost->clk = devm_clk_get(dev, "ost");
>>  +       if (IS_ERR(ost->clk))
>>  +               return PTR_ERR(ost->clk);
>>  +
>>  +       err = clk_prepare_enable(ost->clk);
>>  +       if (err)
>>  +               return err;
>>  +
>>  +       /* Clear counter high/low registers */
>>  +       if (soc_info->is64bit)
>>  +               regmap_write(ost->map, TCU_REG_OST_CNTL, 0);
>>  +       regmap_write(ost->map, TCU_REG_OST_CNTH, 0);
>>  +
>>  +       /* Don't reset counter at compare value. */
>>  +       regmap_update_bits(ost->map, TCU_REG_OST_TCSR,
>>  +                          TCU_OST_TCSR_MASK, TCU_OST_TCSR_CNT_MD);
>>  +
>>  +       rate = clk_get_rate(ost->clk);
>>  +
>>  +       /* Enable OST TCU channel */
>>  +       regmap_write(ost->map, TCU_REG_TESR, BIT(TCU_OST_CHANNEL));
>>  +
>>  +       cs = &ost->cs;
>>  +       cs->name        = "ingenic-ost";
>>  +       cs->rating      = 320;
>>  +       cs->flags       = CLOCK_SOURCE_IS_CONTINUOUS;
>>  +
>>  +       if (soc_info->is64bit) {
>>  +               cs->mask = CLOCKSOURCE_MASK(64);
>>  +               cs->read = ingenic_ost_clocksource_read;
>>  +       } else {
>>  +               cs->mask = CLOCKSOURCE_MASK(32);
>>  +               cs->read = (u64 (*)(struct clocksource 
>> *))ingenic_ost_read_cnth;
> 
> The function is declared as ingenic_ost_read_cnth(void), are you sure
> this is going to work ?

Hence the cast. The clocksource pointer passed as the first argument 
will
simply be ignored by the function. This works fine.

>>  +       }
>>  +
>>  +       err = clocksource_register_hz(cs, rate);
>>  +       if (err) {
>>  +               dev_err(dev, "clocksource registration failed: 
>> %d\n", err);
>>  +               clk_disable_unprepare(ost->clk);
>>  +               return err;
>>  +       }
>>  +
>>  +       /* Cannot register a sched_clock with interrupts on */
>>  +       local_irq_save(flags);
>>  +       if (soc_info->is64bit)
>>  +               sched_clock_register(ingenic_ost_read_cntl, 32, 
>> rate);
>>  +       else
>>  +               sched_clock_register(ingenic_ost_read_cnth, 32, 
>> rate);
>>  +       local_irq_restore(flags);
>>  +
>>  +       return 0;
>>  +}
>>  +
>>  +#ifdef CONFIG_PM_SLEEP
>>  +static int ingenic_ost_suspend(struct device *dev)
>>  +{
>>  +       struct ingenic_ost *ost = dev_get_drvdata(dev);
>>  +
>>  +       clk_disable(ost->clk);
>>  +       return 0;
>>  +}
>>  +
>>  +static int ingenic_ost_resume(struct device *dev)
>>  +{
>>  +       struct ingenic_ost *ost = dev_get_drvdata(dev);
>>  +
>>  +       return clk_enable(ost->clk);
>>  +}
>>  +
>>  +static SIMPLE_DEV_PM_OPS(ingenic_ost_pm_ops, ingenic_ost_suspend,
>>  +                        ingenic_ost_resume);
>>  +#define INGENIC_OST_PM_OPS (&ingenic_ost_pm_ops)
>>  +#else
>>  +#define INGENIC_OST_PM_OPS NULL
>>  +#endif /* CONFIG_PM_SUSPEND */
>>  +
>>  +static const struct ingenic_ost_soc_info jz4725b_ost_soc_info = {
>>  +       .is64bit = false,
>>  +};
>>  +
>>  +static const struct ingenic_ost_soc_info jz4770_ost_soc_info = {
>>  +       .is64bit = true,
>>  +};
>>  +
>>  +static const struct of_device_id ingenic_ost_of_match[] = {
>>  +       { .compatible = "ingenic,jz4725b-ost", .data = 
>> &jz4725b_ost_soc_info, },
>>  +       { .compatible = "ingenic,jz4770-ost",  .data = 
>> &jz4770_ost_soc_info,  },
>>  +       { }
>>  +};
>>  +
>>  +static struct platform_driver ingenic_ost_driver = {
>>  +       .driver = {
>>  +               .name   = "ingenic-ost",
>>  +               .pm     = INGENIC_OST_PM_OPS,
>>  +               .of_match_table = ingenic_ost_of_match,
>>  +       },
>>  +};
>>  +
>>  +/* FIXME: Using device_initcall (or buildin_platform_driver_probe) 
>> results
>>  + * in the driver not being probed at all. It worked in 4.18...
>>  + */
>>  +static int __init ingenic_ost_drv_register(void)
>>  +{
>>  +       return platform_driver_probe(&ingenic_ost_driver,
>>  +                                    ingenic_ost_probe);
>>  +}
>>  +late_initcall(ingenic_ost_drv_register);
>>  --
>>  2.11.0
>>
Guenter Roeck Jan. 23, 2019, 6:01 p.m. UTC | #5
On Wed, Jan 23, 2019 at 02:25:53PM -0300, Paul Cercueil wrote:
> Hi,
> 
> Le mer. 23 janv. 2019 à 11:31, Guenter Roeck <linux@roeck-us.net> a écrit :
> >On 1/23/19 4:58 AM, Mathieu Malaterre wrote:
> >>On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil <paul@crapouillou.net>
> >>wrote:
> >>>
> >>>From: Maarten ter Huurne <maarten@treewalker.org>
> >>>
> >>>OST is the OS Timer, a 64-bit timer/counter with buffered reading.
> >>>
> >>>SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
> >>>JZ4780 have a 64-bit OST.
> >>>
> >>>This driver will register both a clocksource and a sched_clock to the
> >>>system.
> >>>
> >>>Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
> >>>Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> >>>---
> >>>
> >>>Notes:
> >>>      v5: New patch
> >>>
> >>>      v6: - Get rid of SoC IDs; pass pointer to ingenic_ost_soc_info
> >>>as
> >>>            devicetree match data instead.
> >>>          - Use device_get_match_data() instead of the of_* variant
> >>>          - Handle error of dev_get_regmap() properly
> >>>
> >>>      v7: Fix section mismatch by using
> >>>builtin_platform_driver_probe()
> >>>
> >>>      v8: builtin_platform_driver_probe() does not work anymore in
> >>>          4.20-rc6? The probe function won't be called. Work around
> >>>this
> >>>          for now by using late_initcall.
> >>>
> >
> >Did anyone notice this ? Either something is wrong with the driver, or
> >with the kernel core. Hacking around it seems like the worst possible
> >"solution".
> 
> I can confirm it still happens on 5.0-rc3.
> 
> Just to explain what I'm doing:
> 
> My ingenic-timer driver probes with builtin_platform_driver_probe (this
> works),
> and then calls of_platform_populate to probe its children. This driver,
> ingenic-ost, is one of them, and will fail to probe with
> builtin_platform_driver_probe.
> 

The big question is _why_ it fails to probe.

Guenter
Stephen Boyd Jan. 24, 2019, 7:28 p.m. UTC | #6
Quoting Guenter Roeck (2019-01-23 10:01:55)
> On Wed, Jan 23, 2019 at 02:25:53PM -0300, Paul Cercueil wrote:
> > Hi,
> > 
> > Le mer. 23 janv. 2019 à 11:31, Guenter Roeck <linux@roeck-us.net> a écrit :
> > >On 1/23/19 4:58 AM, Mathieu Malaterre wrote:
> > >>On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil <paul@crapouillou.net>
> > >>wrote:
> > >>>
> > >>>From: Maarten ter Huurne <maarten@treewalker.org>
> > >>>
> > >>>OST is the OS Timer, a 64-bit timer/counter with buffered reading.
> > >>>
> > >>>SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
> > >>>JZ4780 have a 64-bit OST.
> > >>>
> > >>>This driver will register both a clocksource and a sched_clock to the
> > >>>system.
> > >>>
> > >>>Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
> > >>>Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >>>---
> > >>>
> > >>>Notes:
> > >>>      v5: New patch
> > >>>
> > >>>      v6: - Get rid of SoC IDs; pass pointer to ingenic_ost_soc_info
> > >>>as
> > >>>            devicetree match data instead.
> > >>>          - Use device_get_match_data() instead of the of_* variant
> > >>>          - Handle error of dev_get_regmap() properly
> > >>>
> > >>>      v7: Fix section mismatch by using
> > >>>builtin_platform_driver_probe()
> > >>>
> > >>>      v8: builtin_platform_driver_probe() does not work anymore in
> > >>>          4.20-rc6? The probe function won't be called. Work around
> > >>>this
> > >>>          for now by using late_initcall.
> > >>>
> > >
> > >Did anyone notice this ? Either something is wrong with the driver, or
> > >with the kernel core. Hacking around it seems like the worst possible
> > >"solution".
> > 
> > I can confirm it still happens on 5.0-rc3.
> > 
> > Just to explain what I'm doing:
> > 
> > My ingenic-timer driver probes with builtin_platform_driver_probe (this
> > works),
> > and then calls of_platform_populate to probe its children. This driver,
> > ingenic-ost, is one of them, and will fail to probe with
> > builtin_platform_driver_probe.
> > 
> 
> The big question is _why_ it fails to probe.
> 

Are you sharing the device tree node between a 'normal' platform device
driver and something more low level DT that marks the device's backing
DT node as OF_POPULATED early on? That's my only guess why it's not
working.
Paul Cercueil Jan. 24, 2019, 8:46 p.m. UTC | #7
Le jeu. 24 janv. 2019 à 16:28, Stephen Boyd <sboyd@kernel.org> a 
écrit :
> Quoting Guenter Roeck (2019-01-23 10:01:55)
>>  On Wed, Jan 23, 2019 at 02:25:53PM -0300, Paul Cercueil wrote:
>>  > Hi,
>>  >
>>  > Le mer. 23 janv. 2019 Ã  11:31, Guenter Roeck 
>> <linux@roeck-us.net> a écrit :
>>  > >On 1/23/19 4:58 AM, Mathieu Malaterre wrote:
>>  > >>On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil 
>> <paul@crapouillou.net>
>>  > >>wrote:
>>  > >>>
>>  > >>>From: Maarten ter Huurne <maarten@treewalker.org>
>>  > >>>
>>  > >>>OST is the OS Timer, a 64-bit timer/counter with buffered 
>> reading.
>>  > >>>
>>  > >>>SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 
>> and
>>  > >>>JZ4780 have a 64-bit OST.
>>  > >>>
>>  > >>>This driver will register both a clocksource and a sched_clock 
>> to the
>>  > >>>system.
>>  > >>>
>>  > >>>Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
>>  > >>>Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  > >>>---
>>  > >>>
>>  > >>>Notes:
>>  > >>>      v5: New patch
>>  > >>>
>>  > >>>      v6: - Get rid of SoC IDs; pass pointer to 
>> ingenic_ost_soc_info
>>  > >>>as
>>  > >>>            devicetree match data instead.
>>  > >>>          - Use device_get_match_data() instead of the of_* 
>> variant
>>  > >>>          - Handle error of dev_get_regmap() properly
>>  > >>>
>>  > >>>      v7: Fix section mismatch by using
>>  > >>>builtin_platform_driver_probe()
>>  > >>>
>>  > >>>      v8: builtin_platform_driver_probe() does not work 
>> anymore in
>>  > >>>          4.20-rc6? The probe function won't be called. Work 
>> around
>>  > >>>this
>>  > >>>          for now by using late_initcall.
>>  > >>>
>>  > >
>>  > >Did anyone notice this ? Either something is wrong with the 
>> driver, or
>>  > >with the kernel core. Hacking around it seems like the worst 
>> possible
>>  > >"solution".
>>  >
>>  > I can confirm it still happens on 5.0-rc3.
>>  >
>>  > Just to explain what I'm doing:
>>  >
>>  > My ingenic-timer driver probes with builtin_platform_driver_probe 
>> (this
>>  > works),
>>  > and then calls of_platform_populate to probe its children. This 
>> driver,
>>  > ingenic-ost, is one of them, and will fail to probe with
>>  > builtin_platform_driver_probe.
>>  >
>> 
>>  The big question is _why_ it fails to probe.
>> 
> 
> Are you sharing the device tree node between a 'normal' platform 
> device
> driver and something more low level DT that marks the device's backing
> DT node as OF_POPULATED early on? That's my only guess why it's not
> working.

I do, but I clear the OF_POPULATED flag so that it is then probed as a
normal platform device, and it's not on this driver's node but its 
parent.
Stephen Boyd Jan. 24, 2019, 10:46 p.m. UTC | #8
Quoting Paul Cercueil (2019-01-24 12:46:28)
> 
> 
> Le jeu. 24 janv. 2019 à 16:28, Stephen Boyd <sboyd@kernel.org> a 
> écrit :
> > Quoting Guenter Roeck (2019-01-23 10:01:55)
> >>  On Wed, Jan 23, 2019 at 02:25:53PM -0300, Paul Cercueil wrote:
> >>  > Hi,
> >>  >
> >>  > Le mer. 23 janv. 2019 Ã  11:31, Guenter Roeck 
> >> <linux@roeck-us.net> a écrit :
> >>  > >On 1/23/19 4:58 AM, Mathieu Malaterre wrote:
> >>  > >>On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil 
> >> <paul@crapouillou.net>
> >>  > >>wrote:
> >>  > >>>
> >>  > >>>From: Maarten ter Huurne <maarten@treewalker.org>
> >>  > >>>
> >>  > >>>OST is the OS Timer, a 64-bit timer/counter with buffered 
> >> reading.
> >>  > >>>
> >>  > >>>SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 
> >> and
> >>  > >>>JZ4780 have a 64-bit OST.
> >>  > >>>
> >>  > >>>This driver will register both a clocksource and a sched_clock 
> >> to the
> >>  > >>>system.
> >>  > >>>
> >>  > >>>Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
> >>  > >>>Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> >>  > >>>---
> >>  > >>>
> >>  > >>>Notes:
> >>  > >>>      v5: New patch
> >>  > >>>
> >>  > >>>      v6: - Get rid of SoC IDs; pass pointer to 
> >> ingenic_ost_soc_info
> >>  > >>>as
> >>  > >>>            devicetree match data instead.
> >>  > >>>          - Use device_get_match_data() instead of the of_* 
> >> variant
> >>  > >>>          - Handle error of dev_get_regmap() properly
> >>  > >>>
> >>  > >>>      v7: Fix section mismatch by using
> >>  > >>>builtin_platform_driver_probe()
> >>  > >>>
> >>  > >>>      v8: builtin_platform_driver_probe() does not work 
> >> anymore in
> >>  > >>>          4.20-rc6? The probe function won't be called. Work 
> >> around
> >>  > >>>this
> >>  > >>>          for now by using late_initcall.
> >>  > >>>
> >>  > >
> >>  > >Did anyone notice this ? Either something is wrong with the 
> >> driver, or
> >>  > >with the kernel core. Hacking around it seems like the worst 
> >> possible
> >>  > >"solution".
> >>  >
> >>  > I can confirm it still happens on 5.0-rc3.
> >>  >
> >>  > Just to explain what I'm doing:
> >>  >
> >>  > My ingenic-timer driver probes with builtin_platform_driver_probe 
> >> (this
> >>  > works),
> >>  > and then calls of_platform_populate to probe its children. This 
> >> driver,
> >>  > ingenic-ost, is one of them, and will fail to probe with
> >>  > builtin_platform_driver_probe.
> >>  >
> >> 
> >>  The big question is _why_ it fails to probe.
> >> 
> > 
> > Are you sharing the device tree node between a 'normal' platform 
> > device
> > driver and something more low level DT that marks the device's backing
> > DT node as OF_POPULATED early on? That's my only guess why it's not
> > working.
> 
> I do, but I clear the OF_POPULATED flag so that it is then probed as a
> normal platform device, and it's not on this driver's node but its 
> parent.
> 

Where do you clear the OF_POPULATED flag?
Paul Cercueil Jan. 24, 2019, 10:53 p.m. UTC | #9
Le jeu. 24 janv. 2019 à 19:46, Stephen Boyd <sboyd@kernel.org> a 
écrit :
> Quoting Paul Cercueil (2019-01-24 12:46:28)
>> 
>> 
>>  Le jeu. 24 janv. 2019 à 16:28, Stephen Boyd <sboyd@kernel.org> a
>>  écrit :
>>  > Quoting Guenter Roeck (2019-01-23 10:01:55)
>>  >>  On Wed, Jan 23, 2019 at 02:25:53PM -0300, Paul Cercueil wrote:
>>  >>  > Hi,
>>  >>  >
>>  >>  > Le mer. 23 janv. 2019 Ã  11:31, Guenter Roeck
>>  >> <linux@roeck-us.net> a écrit :
>>  >>  > >On 1/23/19 4:58 AM, Mathieu Malaterre wrote:
>>  >>  > >>On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil
>>  >> <paul@crapouillou.net>
>>  >>  > >>wrote:
>>  >>  > >>>
>>  >>  > >>>From: Maarten ter Huurne <maarten@treewalker.org>
>>  >>  > >>>
>>  >>  > >>>OST is the OS Timer, a 64-bit timer/counter with buffered
>>  >> reading.
>>  >>  > >>>
>>  >>  > >>>SoCs before the JZ4770 had (if any) a 32-bit OST; the 
>> JZ4770
>>  >> and
>>  >>  > >>>JZ4780 have a 64-bit OST.
>>  >>  > >>>
>>  >>  > >>>This driver will register both a clocksource and a 
>> sched_clock
>>  >> to the
>>  >>  > >>>system.
>>  >>  > >>>
>>  >>  > >>>Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
>>  >>  > >>>Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  >>  > >>>---
>>  >>  > >>>
>>  >>  > >>>Notes:
>>  >>  > >>>      v5: New patch
>>  >>  > >>>
>>  >>  > >>>      v6: - Get rid of SoC IDs; pass pointer to
>>  >> ingenic_ost_soc_info
>>  >>  > >>>as
>>  >>  > >>>            devicetree match data instead.
>>  >>  > >>>          - Use device_get_match_data() instead of the of_*
>>  >> variant
>>  >>  > >>>          - Handle error of dev_get_regmap() properly
>>  >>  > >>>
>>  >>  > >>>      v7: Fix section mismatch by using
>>  >>  > >>>builtin_platform_driver_probe()
>>  >>  > >>>
>>  >>  > >>>      v8: builtin_platform_driver_probe() does not work
>>  >> anymore in
>>  >>  > >>>          4.20-rc6? The probe function won't be called. 
>> Work
>>  >> around
>>  >>  > >>>this
>>  >>  > >>>          for now by using late_initcall.
>>  >>  > >>>
>>  >>  > >
>>  >>  > >Did anyone notice this ? Either something is wrong with the
>>  >> driver, or
>>  >>  > >with the kernel core. Hacking around it seems like the worst
>>  >> possible
>>  >>  > >"solution".
>>  >>  >
>>  >>  > I can confirm it still happens on 5.0-rc3.
>>  >>  >
>>  >>  > Just to explain what I'm doing:
>>  >>  >
>>  >>  > My ingenic-timer driver probes with 
>> builtin_platform_driver_probe
>>  >> (this
>>  >>  > works),
>>  >>  > and then calls of_platform_populate to probe its children. 
>> This
>>  >> driver,
>>  >>  > ingenic-ost, is one of them, and will fail to probe with
>>  >>  > builtin_platform_driver_probe.
>>  >>  >
>>  >>
>>  >>  The big question is _why_ it fails to probe.
>>  >>
>>  >
>>  > Are you sharing the device tree node between a 'normal' platform
>>  > device
>>  > driver and something more low level DT that marks the device's 
>> backing
>>  > DT node as OF_POPULATED early on? That's my only guess why it's 
>> not
>>  > working.
>> 
>>  I do, but I clear the OF_POPULATED flag so that it is then probed 
>> as a
>>  normal platform device, and it's not on this driver's node but its
>>  parent.
>> 
> 
> Where do you clear the OF_POPULATED flag?
> 

In the ingenic-timer driver introduced in patch [04/26], inside the 
probe function.
Paul Cercueil Feb. 23, 2019, 3:17 a.m. UTC | #10
Hi,

Le jeu. 24 janv. 2019 à 19:53, Paul Cercueil <paul@crapouillou.net> a 
écrit :
> 
> 
> Le jeu. 24 janv. 2019 à 19:46, Stephen Boyd <sboyd@kernel.org> a 
> écrit :
>> Quoting Paul Cercueil (2019-01-24 12:46:28)
>>> 
>>> 
>>>  Le jeu. 24 janv. 2019 à 16:28, Stephen Boyd <sboyd@kernel.org> a
>>>  écrit :
>>>  > Quoting Guenter Roeck (2019-01-23 10:01:55)
>>>  >>  On Wed, Jan 23, 2019 at 02:25:53PM -0300, Paul Cercueil wrote:
>>>  >>  > Hi,
>>>  >>  >
>>>  >>  > Le mer. 23 janv. 2019 Ã  11:31, Guenter Roeck
>>>  >> <linux@roeck-us.net> a écrit :
>>>  >>  > >On 1/23/19 4:58 AM, Mathieu Malaterre wrote:
>>>  >>  > >>On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil
>>>  >> <paul@crapouillou.net>
>>>  >>  > >>wrote:
>>>  >>  > >>>
>>>  >>  > >>>From: Maarten ter Huurne <maarten@treewalker.org>
>>>  >>  > >>>
>>>  >>  > >>>OST is the OS Timer, a 64-bit timer/counter with buffered
>>>  >> reading.
>>>  >>  > >>>
>>>  >>  > >>>SoCs before the JZ4770 had (if any) a 32-bit OST; the 
>>> JZ4770
>>>  >> and
>>>  >>  > >>>JZ4780 have a 64-bit OST.
>>>  >>  > >>>
>>>  >>  > >>>This driver will register both a clocksource and a 
>>> sched_clock
>>>  >> to the
>>>  >>  > >>>system.
>>>  >>  > >>>
>>>  >>  > >>>Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
>>>  >>  > >>>Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>  >>  > >>>---
>>>  >>  > >>>
>>>  >>  > >>>Notes:
>>>  >>  > >>>      v5: New patch
>>>  >>  > >>>
>>>  >>  > >>>      v6: - Get rid of SoC IDs; pass pointer to
>>>  >> ingenic_ost_soc_info
>>>  >>  > >>>as
>>>  >>  > >>>            devicetree match data instead.
>>>  >>  > >>>          - Use device_get_match_data() instead of the 
>>> of_*
>>>  >> variant
>>>  >>  > >>>          - Handle error of dev_get_regmap() properly
>>>  >>  > >>>
>>>  >>  > >>>      v7: Fix section mismatch by using
>>>  >>  > >>>builtin_platform_driver_probe()
>>>  >>  > >>>
>>>  >>  > >>>      v8: builtin_platform_driver_probe() does not work
>>>  >> anymore in
>>>  >>  > >>>          4.20-rc6? The probe function won't be called. 
>>> Work
>>>  >> around
>>>  >>  > >>>this
>>>  >>  > >>>          for now by using late_initcall.
>>>  >>  > >>>
>>>  >>  > >
>>>  >>  > >Did anyone notice this ? Either something is wrong with the
>>>  >> driver, or
>>>  >>  > >with the kernel core. Hacking around it seems like the worst
>>>  >> possible
>>>  >>  > >"solution".
>>>  >>  >
>>>  >>  > I can confirm it still happens on 5.0-rc3.
>>>  >>  >
>>>  >>  > Just to explain what I'm doing:
>>>  >>  >
>>>  >>  > My ingenic-timer driver probes with 
>>> builtin_platform_driver_probe
>>>  >> (this
>>>  >>  > works),
>>>  >>  > and then calls of_platform_populate to probe its children. 
>>> This
>>>  >> driver,
>>>  >>  > ingenic-ost, is one of them, and will fail to probe with
>>>  >>  > builtin_platform_driver_probe.
>>>  >>  >
>>>  >>
>>>  >>  The big question is _why_ it fails to probe.
>>>  >>
>>>  >
>>>  > Are you sharing the device tree node between a 'normal' platform
>>>  > device
>>>  > driver and something more low level DT that marks the device's 
>>> backing
>>>  > DT node as OF_POPULATED early on? That's my only guess why it's 
>>> not
>>>  > working.
>>> 
>>>  I do, but I clear the OF_POPULATED flag so that it is then probed 
>>> as a
>>>  normal platform device, and it's not on this driver's node but its
>>>  parent.
>>> 
>> 
>> Where do you clear the OF_POPULATED flag?
>> 
> 
> In the ingenic-timer driver introduced in patch [04/26], inside the 
> probe function.

Anything new on this? It still happens on 5.0-rc7.
It probes with late_initcall, and not with device_initcall.
I have no clue what's going on.

-Paul
Stephen Boyd Feb. 25, 2019, 6:05 p.m. UTC | #11
Quoting Paul Cercueil (2019-02-22 19:17:25)
> Hi,
> 
> Anything new on this? It still happens on 5.0-rc7.
> It probes with late_initcall, and not with device_initcall.
> I have no clue what's going on.
> 

I'm not sure what's going on either. You'll probably have to debug when
the device is created and when it is probed by enabling the debug
printing in the driver core or by adding in extra debug prints to narrow
down the problem. For example, add a '#define DEBUG 1' at the top of
drivers/base/dd.c and see if that helps give some info on what's going
on with the drivers and devices.
Paul Cercueil Feb. 27, 2019, 11:54 p.m. UTC | #12
Hi,

Le lun. 25 févr. 2019 à 15:05, Stephen Boyd <sboyd@kernel.org> a 
écrit :
> Quoting Paul Cercueil (2019-02-22 19:17:25)
>>  Hi,
>> 
>>  Anything new on this? It still happens on 5.0-rc7.
>>  It probes with late_initcall, and not with device_initcall.
>>  I have no clue what's going on.
>> 
> 
> I'm not sure what's going on either. You'll probably have to debug 
> when
> the device is created and when it is probed by enabling the debug
> printing in the driver core or by adding in extra debug prints to 
> narrow
> down the problem. For example, add a '#define DEBUG 1' at the top of
> drivers/base/dd.c and see if that helps give some info on what's going
> on with the drivers and devices.

The doc of __platform_driver_probe says:
"Use this instead of platform_driver_register() when you know the device
is not hotpluggable and has already been registered".

When the parent device and child device are both probed with
builtin_platform_driver_probe(), and the parent calls
devm_of_platform_populate(), it is not certain that the parent's
probe will happen before the child's, and if it does not, the child
device has not been registered and its probe is not allowed to defer.
So it turned out not to be a core bug, rather a misuse of the API.

So I will keep the builtin_platform_driver_probe() in the child, and 
use a
subsys_initcall() in the parent. That works fine.

Regards,
-Paul
diff mbox series

Patch

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 4e69af15c3e7..e0646878b0de 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -648,4 +648,12 @@  config INGENIC_TIMER
 	help
 	  Support for the timer/counter unit of the Ingenic JZ SoCs.
 
+config INGENIC_OST
+	bool "Ingenic JZ47xx Operating System Timer"
+	depends on MIPS || COMPILE_TEST
+	depends on COMMON_CLK
+	select INGENIC_TIMER
+	help
+	  Support for the OS Timer of the Ingenic JZ4770 or similar SoC.
+
 endmenu
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 7c8f790dcf67..7faa8108574a 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -75,6 +75,7 @@  obj-$(CONFIG_ASM9260_TIMER)		+= asm9260_timer.o
 obj-$(CONFIG_H8300_TMR8)		+= h8300_timer8.o
 obj-$(CONFIG_H8300_TMR16)		+= h8300_timer16.o
 obj-$(CONFIG_H8300_TPU)			+= h8300_tpu.o
+obj-$(CONFIG_INGENIC_OST)		+= ingenic-ost.o
 obj-$(CONFIG_INGENIC_TIMER)		+= ingenic-timer.o
 obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
 obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
diff --git a/drivers/clocksource/ingenic-ost.c b/drivers/clocksource/ingenic-ost.c
new file mode 100644
index 000000000000..cc0fee3efd29
--- /dev/null
+++ b/drivers/clocksource/ingenic-ost.c
@@ -0,0 +1,215 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * JZ47xx SoCs TCU Operating System Timer driver
+ *
+ * Copyright (C) 2016 Maarten ter Huurne <maarten@treewalker.org>
+ * Copyright (C) 2018 Paul Cercueil <paul@crapouillou.net>
+ */
+
+#include <linux/clk.h>
+#include <linux/clocksource.h>
+#include <linux/mfd/ingenic-tcu.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+#include <linux/sched_clock.h>
+
+#include "ingenic-timer.h"
+
+#define TCU_OST_TCSR_MASK	0xc0
+#define TCU_OST_TCSR_CNT_MD	BIT(15)
+
+#define TCU_OST_CHANNEL		15
+
+struct ingenic_ost_soc_info {
+	bool is64bit;
+};
+
+struct ingenic_ost {
+	struct regmap *map;
+	struct clk *clk;
+
+	struct clocksource cs;
+};
+
+static u64 notrace ingenic_ost_read_cntl(void)
+{
+	/* Bypass the regmap here as we must return as soon as possible */
+	return readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
+}
+
+static u64 notrace ingenic_ost_read_cnth(void)
+{
+	/* Bypass the regmap here as we must return as soon as possible */
+	return readl(ingenic_tcu_base + TCU_REG_OST_CNTH);
+}
+
+static u64 notrace ingenic_ost_clocksource_read(struct clocksource *cs)
+{
+	u32 val1, val2;
+	u64 count, recount;
+	s64 diff;
+
+	/*
+	 * The buffering of the upper 32 bits of the timer prevents wrong
+	 * results from the bottom 32 bits overflowing due to the timer ticking
+	 * along. However, it does not prevent wrong results from simultaneous
+	 * reads of the timer, which could reset the buffer mid-read.
+	 * Since this kind of wrong read can happen only when the bottom bits
+	 * overflow, there will be minutes between wrong reads, so if we read
+	 * twice in succession, at least one of the reads will be correct.
+	 */
+
+	/* Bypass the regmap here as we must return as soon as possible */
+	val1 = readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
+	val2 = readl(ingenic_tcu_base + TCU_REG_OST_CNTHBUF);
+	count = (u64)val1 | (u64)val2 << 32;
+
+	val1 = readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
+	val2 = readl(ingenic_tcu_base + TCU_REG_OST_CNTHBUF);
+	recount = (u64)val1 | (u64)val2 << 32;
+
+	/*
+	 * A wrong read will produce a result that is 1<<32 too high: the bottom
+	 * part from before overflow and the upper part from after overflow.
+	 * Therefore, the lower value of the two reads is the correct value.
+	 */
+
+	diff = (s64)(recount - count);
+	if (unlikely(diff < 0))
+		count = recount;
+
+	return count;
+}
+
+static int __init ingenic_ost_probe(struct platform_device *pdev)
+{
+	const struct ingenic_ost_soc_info *soc_info;
+	struct device *dev = &pdev->dev;
+	struct ingenic_ost *ost;
+	struct clocksource *cs;
+	unsigned long rate, flags;
+	int err;
+
+	soc_info = device_get_match_data(dev);
+	if (!soc_info)
+		return -EINVAL;
+
+	ost = devm_kzalloc(dev, sizeof(*ost), GFP_KERNEL);
+	if (!ost)
+		return -ENOMEM;
+
+	ost->map = dev_get_regmap(dev->parent, NULL);
+	if (!ost->map) {
+		dev_err(dev, "regmap not found\n");
+		return -EINVAL;
+	}
+
+	ost->clk = devm_clk_get(dev, "ost");
+	if (IS_ERR(ost->clk))
+		return PTR_ERR(ost->clk);
+
+	err = clk_prepare_enable(ost->clk);
+	if (err)
+		return err;
+
+	/* Clear counter high/low registers */
+	if (soc_info->is64bit)
+		regmap_write(ost->map, TCU_REG_OST_CNTL, 0);
+	regmap_write(ost->map, TCU_REG_OST_CNTH, 0);
+
+	/* Don't reset counter at compare value. */
+	regmap_update_bits(ost->map, TCU_REG_OST_TCSR,
+			   TCU_OST_TCSR_MASK, TCU_OST_TCSR_CNT_MD);
+
+	rate = clk_get_rate(ost->clk);
+
+	/* Enable OST TCU channel */
+	regmap_write(ost->map, TCU_REG_TESR, BIT(TCU_OST_CHANNEL));
+
+	cs = &ost->cs;
+	cs->name	= "ingenic-ost";
+	cs->rating	= 320;
+	cs->flags	= CLOCK_SOURCE_IS_CONTINUOUS;
+
+	if (soc_info->is64bit) {
+		cs->mask = CLOCKSOURCE_MASK(64);
+		cs->read = ingenic_ost_clocksource_read;
+	} else {
+		cs->mask = CLOCKSOURCE_MASK(32);
+		cs->read = (u64 (*)(struct clocksource *))ingenic_ost_read_cnth;
+	}
+
+	err = clocksource_register_hz(cs, rate);
+	if (err) {
+		dev_err(dev, "clocksource registration failed: %d\n", err);
+		clk_disable_unprepare(ost->clk);
+		return err;
+	}
+
+	/* Cannot register a sched_clock with interrupts on */
+	local_irq_save(flags);
+	if (soc_info->is64bit)
+		sched_clock_register(ingenic_ost_read_cntl, 32, rate);
+	else
+		sched_clock_register(ingenic_ost_read_cnth, 32, rate);
+	local_irq_restore(flags);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int ingenic_ost_suspend(struct device *dev)
+{
+	struct ingenic_ost *ost = dev_get_drvdata(dev);
+
+	clk_disable(ost->clk);
+	return 0;
+}
+
+static int ingenic_ost_resume(struct device *dev)
+{
+	struct ingenic_ost *ost = dev_get_drvdata(dev);
+
+	return clk_enable(ost->clk);
+}
+
+static SIMPLE_DEV_PM_OPS(ingenic_ost_pm_ops, ingenic_ost_suspend,
+			 ingenic_ost_resume);
+#define INGENIC_OST_PM_OPS (&ingenic_ost_pm_ops)
+#else
+#define INGENIC_OST_PM_OPS NULL
+#endif /* CONFIG_PM_SUSPEND */
+
+static const struct ingenic_ost_soc_info jz4725b_ost_soc_info = {
+	.is64bit = false,
+};
+
+static const struct ingenic_ost_soc_info jz4770_ost_soc_info = {
+	.is64bit = true,
+};
+
+static const struct of_device_id ingenic_ost_of_match[] = {
+	{ .compatible = "ingenic,jz4725b-ost", .data = &jz4725b_ost_soc_info, },
+	{ .compatible = "ingenic,jz4770-ost",  .data = &jz4770_ost_soc_info,  },
+	{ }
+};
+
+static struct platform_driver ingenic_ost_driver = {
+	.driver = {
+		.name	= "ingenic-ost",
+		.pm	= INGENIC_OST_PM_OPS,
+		.of_match_table = ingenic_ost_of_match,
+	},
+};
+
+/* FIXME: Using device_initcall (or buildin_platform_driver_probe) results
+ * in the driver not being probed at all. It worked in 4.18...
+ */
+static int __init ingenic_ost_drv_register(void)
+{
+	return platform_driver_probe(&ingenic_ost_driver,
+				     ingenic_ost_probe);
+}
+late_initcall(ingenic_ost_drv_register);