Messages in this thread | | | Subject | Re: [PATCH V2 1/3] watchdog: stm32: add pclk feature for stm32mp1 | From | Ludovic BARRE <> | Date | Wed, 20 Jun 2018 15:35:02 +0200 |
| |
On 06/20/2018 03:29 PM, Guenter Roeck wrote: > On 06/20/2018 06:24 AM, Ludovic BARRE wrote: >> >> >> On 06/20/2018 11:19 AM, Guenter Roeck wrote: >>> On 06/20/2018 12:53 AM, Ludovic Barre wrote: >>>> From: Ludovic Barre <ludovic.barre@st.com> >>>> >>>> This patch adds config data to manage specific properties by >>>> compatible. Adds stm32mp1 config which requires pclk clock. >>>> >>>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> >>>> --- >>>> .../devicetree/bindings/watchdog/st,stm32-iwdg.txt | 21 +++- >>>> drivers/watchdog/stm32_iwdg.c | 132 >>>> ++++++++++++++------- >>>> 2 files changed, 104 insertions(+), 49 deletions(-) >>>> >>>> diff --git >>>> a/Documentation/devicetree/bindings/watchdog/st,stm32-iwdg.txt >>>> b/Documentation/devicetree/bindings/watchdog/st,stm32-iwdg.txt >>>> index cc13b10a..f07f6d89 100644 >>>> --- a/Documentation/devicetree/bindings/watchdog/st,stm32-iwdg.txt >>>> +++ b/Documentation/devicetree/bindings/watchdog/st,stm32-iwdg.txt >>>> @@ -2,18 +2,31 @@ STM32 Independent WatchDoG (IWDG) >>>> --------------------------------- >>>> Required properties: >>>> -- compatible: "st,stm32-iwdg" >>>> -- reg: physical base address and length of the registers set for >>>> the device >>>> -- clocks: must contain a single entry describing the clock input >>>> +- compatible: Should be either "st,stm32-iwdg" or "st,stm32mp1-iwdg" >>>> +- reg: Physical base address and length of the registers set for >>>> the device >>>> +- clocks: Reference to the clock entry lsi. Additional pclk clock >>>> entry >>>> + is required only for st,stm32mp1-iwdg. >>>> +- clock-names: Name of the clocks used. >>>> + "lsi" for st,stm32-iwdg >>>> + "pclk", "lsi" for st,stm32mp1-iwdg >>>> Optional Properties: >>>> - timeout-sec: Watchdog timeout value in seconds. >>>> -Example: >>>> +Examples: >>>> iwdg: watchdog@40003000 { >>>> compatible = "st,stm32-iwdg"; >>>> reg = <0x40003000 0x400>; >>>> clocks = <&clk_lsi>; >>>> + clock-names = "lsi"; >>>> + timeout-sec = <32>; >>>> +}; >>>> + >>>> +iwdg: iwdg@5a002000 { >>>> + compatible = "st,stm32mp1-iwdg"; >>>> + reg = <0x5a002000 0x400>; >>>> + clocks = <&rcc IWDG2>, <&clk_lsi>; >>>> + clock-names = "pclk", "lsi"; >>>> timeout-sec = <32>; >>>> }; >>>> diff --git a/drivers/watchdog/stm32_iwdg.c >>>> b/drivers/watchdog/stm32_iwdg.c >>>> index c97ad56..fc96670 100644 >>>> --- a/drivers/watchdog/stm32_iwdg.c >>>> +++ b/drivers/watchdog/stm32_iwdg.c >>>> @@ -11,12 +11,13 @@ >>>> #include <linux/clk.h> >>>> #include <linux/delay.h> >>>> -#include <linux/kernel.h> >>>> -#include <linux/module.h> >>>> #include <linux/interrupt.h> >>>> #include <linux/io.h> >>>> #include <linux/iopoll.h> >>>> +#include <linux/kernel.h> >>>> +#include <linux/module.h> >>>> #include <linux/of.h> >>>> +#include <linux/of_device.h> >>>> #include <linux/platform_device.h> >>>> #include <linux/watchdog.h> >>>> @@ -54,11 +55,17 @@ >>>> #define TIMEOUT_US 100000 >>>> #define SLEEP_US 1000 >>>> +struct stm32_iwdg_config { >>>> + bool has_pclk; >>>> +}; >>>> + >>> >>> This data structure is unnecessary. Just assign the boolean directly >>> to .data >>> and ... >>> >> >> Ok, I send a v3, with boolean directly to .data >> like: >> >> #define NO_PCLK false >> #define HAS_PCLK true >> ... > > Just use true/false directly. There is no need for those defines. > > If you want the reader to understand what is defined, I would be ok with > > #define HAS_PCLK true > > static const struct of_device_id stm32_iwdg_of_match[] = { > { .compatible = "st,stm32-iwdg", .data = (void *) !HAS_PCLK }, > { .compatible = "st,stm32mp1-iwdg", .data = (void *) HAS_PCLK }, > { /* end node */ } > > Guenter
Ok, thanks
> >> static const struct of_device_id stm32_iwdg_of_match[] = { >> { .compatible = "st,stm32-iwdg", .data = (void *) NO_PCLK }, >> { .compatible = "st,stm32mp1-iwdg", .data = (void *) HAS_PCLK }, >> { /* end node */ } >> }; >> >> Note: >> V3, because I sent my original version with V2 >> (it's mistake) >> >>>> struct stm32_iwdg { >>>> - struct watchdog_device wdd; >>>> - void __iomem *regs; >>>> - struct clk *clk; >>>> - unsigned int rate; >>>> + struct watchdog_device wdd; >>>> + void __iomem *regs; >>>> + struct stm32_iwdg_config *config; >>> >>> declare bool has_pclk here. >>> >>>> + struct clk *clk_lsi; >>>> + struct clk *clk_pclk; >>>> + unsigned int rate; >>>> }; >>>> static inline u32 reg_read(void __iomem *base, u32 reg) >>>> @@ -133,6 +140,44 @@ static int stm32_iwdg_set_timeout(struct >>>> watchdog_device *wdd, >>>> return 0; >>>> } >>>> +static int stm32_iwdg_clk_init(struct platform_device *pdev, >>>> + struct stm32_iwdg *wdt) >>>> +{ >>>> + u32 ret; >>>> + >>>> + wdt->clk_lsi = devm_clk_get(&pdev->dev, "lsi"); >>>> + if (IS_ERR(wdt->clk_lsi)) { >>>> + dev_err(&pdev->dev, "Unable to get lsi clock\n"); >>>> + return PTR_ERR(wdt->clk_lsi); >>>> + } >>>> + >>>> + /* optional peripheral clock */ >>>> + if (wdt->config->has_pclk) { >>>> + wdt->clk_pclk = devm_clk_get(&pdev->dev, "pclk"); >>>> + if (IS_ERR(wdt->clk_pclk)) { >>>> + dev_err(&pdev->dev, "Unable to get pclk clock\n"); >>>> + return PTR_ERR(wdt->clk_pclk); >>>> + } >>>> + >>>> + ret = clk_prepare_enable(wdt->clk_pclk); >>>> + if (ret) { >>>> + dev_err(&pdev->dev, "Unable to prepare pclk clock\n"); >>>> + return ret; >>>> + } >>>> + } >>>> + >>>> + ret = clk_prepare_enable(wdt->clk_lsi); >>>> + if (ret) { >>>> + dev_err(&pdev->dev, "Unable to prepare lsi clock\n"); >>>> + clk_disable_unprepare(wdt->clk_pclk); >>>> + return ret; >>>> + } >>>> + >>>> + wdt->rate = clk_get_rate(wdt->clk_lsi); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static const struct watchdog_info stm32_iwdg_info = { >>>> .options = WDIOF_SETTIMEOUT | >>>> WDIOF_MAGICCLOSE | >>>> @@ -147,49 +192,50 @@ static const struct watchdog_ops >>>> stm32_iwdg_ops = { >>>> .set_timeout = stm32_iwdg_set_timeout, >>>> }; >>>> +static const struct stm32_iwdg_config stm32_iwdg_cfg = { >>>> + .has_pclk = false, >>>> +}; >>>> + >>>> +static const struct stm32_iwdg_config stm32mp1_iwdg_cfg = { >>>> + .has_pclk = true, >>>> +}; >>>> + >>>> +static const struct of_device_id stm32_iwdg_of_match[] = { >>>> + { .compatible = "st,stm32-iwdg", .data = &stm32_iwdg_cfg }, >>>> + { .compatible = "st,stm32mp1-iwdg", .data = &stm32mp1_iwdg_cfg }, >>>> + { /* end node */ } >>>> +}; >>>> +MODULE_DEVICE_TABLE(of, stm32_iwdg_of_match); >>>> + >>>> static int stm32_iwdg_probe(struct platform_device *pdev) >>>> { >>>> struct watchdog_device *wdd; >>>> + const struct of_device_id *match; >>>> struct stm32_iwdg *wdt; >>>> struct resource *res; >>>> - void __iomem *regs; >>>> - struct clk *clk; >>>> int ret; >>>> + match = of_match_device(stm32_iwdg_of_match, &pdev->dev); >>>> + if (!match || !match->data) >>>> + return -ENODEV; >>>> + >>>> + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); >>>> + if (!wdt) >>>> + return -ENOMEM; >>>> + >>>> + wdt->config = (struct stm32_iwdg_config *)match->data; >>>> + >>>> /* This is the timer base. */ >>>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>> - regs = devm_ioremap_resource(&pdev->dev, res); >>>> - if (IS_ERR(regs)) { >>>> + wdt->regs = devm_ioremap_resource(&pdev->dev, res); >>>> + if (IS_ERR(wdt->regs)) { >>>> dev_err(&pdev->dev, "Could not get resource\n"); >>>> - return PTR_ERR(regs); >>>> + return PTR_ERR(wdt->regs); >>>> } >>>> - clk = devm_clk_get(&pdev->dev, NULL); >>>> - if (IS_ERR(clk)) { >>>> - dev_err(&pdev->dev, "Unable to get clock\n"); >>>> - return PTR_ERR(clk); >>>> - } >>>> - >>>> - ret = clk_prepare_enable(clk); >>>> - if (ret) { >>>> - dev_err(&pdev->dev, "Unable to prepare clock %p\n", clk); >>>> + ret = stm32_iwdg_clk_init(pdev, wdt); >>>> + if (ret) >>>> return ret; >>>> - } >>>> - >>>> - /* >>>> - * Allocate our watchdog driver data, which has the >>>> - * struct watchdog_device nested within it. >>>> - */ >>>> - wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); >>>> - if (!wdt) { >>>> - ret = -ENOMEM; >>>> - goto err; >>>> - } >>>> - >>>> - /* Initialize struct stm32_iwdg. */ >>>> - wdt->regs = regs; >>>> - wdt->clk = clk; >>>> - wdt->rate = clk_get_rate(clk); >>>> /* Initialize struct watchdog_device. */ >>>> wdd = &wdt->wdd; >>>> @@ -217,7 +263,8 @@ static int stm32_iwdg_probe(struct >>>> platform_device *pdev) >>>> return 0; >>>> err: >>>> - clk_disable_unprepare(clk); >>>> + clk_disable_unprepare(wdt->clk_lsi); >>>> + clk_disable_unprepare(wdt->clk_pclk); >>>> return ret; >>>> } >>>> @@ -227,23 +274,18 @@ static int stm32_iwdg_remove(struct >>>> platform_device *pdev) >>>> struct stm32_iwdg *wdt = platform_get_drvdata(pdev); >>>> watchdog_unregister_device(&wdt->wdd); >>>> - clk_disable_unprepare(wdt->clk); >>>> + clk_disable_unprepare(wdt->clk_lsi); >>>> + clk_disable_unprepare(wdt->clk_pclk); >>>> return 0; >>>> } >>>> -static const struct of_device_id stm32_iwdg_of_match[] = { >>>> - { .compatible = "st,stm32-iwdg" }, >>>> - { /* end node */ } >>>> -}; >>>> -MODULE_DEVICE_TABLE(of, stm32_iwdg_of_match); >>>> - >>>> static struct platform_driver stm32_iwdg_driver = { >>>> .probe = stm32_iwdg_probe, >>>> .remove = stm32_iwdg_remove, >>>> .driver = { >>>> .name = "iwdg", >>>> - .of_match_table = stm32_iwdg_of_match, >>>> + .of_match_table = of_match_ptr(stm32_iwdg_of_match), >>>> }, >>>> }; >>>> module_platform_driver(stm32_iwdg_driver); >>>> >>> >> >
| |