Re: [PATCH 1/2] watchdog: Add Cadence WDT driver

[Michal Simek <monstr@monstr.eu>]

[-- Attachment #1: Type: text/plain, Size: 2360 bytes --]

Hi Guenter,

>> +/**
>> + * struct cdns_wdt - Watchdog device structure
>> + * @regs: baseaddress of device
>> + * @rst: reset flag
>> + * @clk: struct clk * of a clock source
>> + * @prescaler: for saving prescaler value
>> + * @ctrl_clksel: counter clock prescaler selection
>> + * @io_lock: spinlock for IO register access
>> + * @cdns_wdt_device: watchdog device structure
>> + * @cdns_wdt_notifier: notifier structure
>> + *
>> + * Structure containing parameters specific to cadence watchdog.
>> + */
>> +struct cdns_wdt {
>> +    void __iomem        *regs;
>> +    u32            rst;
>> +    struct clk        *clk;
>> +    u32            prescaler;
>> +    u32            ctrl_clksel;
>> +    spinlock_t        io_lock;
>> +    struct watchdog_device    cdns_wdt_device;
>> +    struct notifier_block    cdns_wdt_notifier;
>> +};
>> +
>> +/* Write access to Registers */
>> +static inline void cdns_wdt_writereg(void __iomem *offset, u32 val)
>> +{
>> +    writel_relaxed(val, offset);
>> +}
>> +
> 
> Not really sure if this function provides any value.

I can't see any problem to use this helper IO function
but maybe we could do it a little bit differently.
Currently implementation is just passing values to writel_relaxed()

What about to do it like this?

static inline void cdns_wdt_writereg(struct cdns_wdt *wdt, u32 offset, u32 val)
{
    writel_relaxed(val, wdt->regs + offset);
}

This solution was suggested by Mark here too.
https://lkml.org/lkml/2014/3/17/234

The reason for having one IO helper function is
1. Simper debugging when you add printks to one location and
you get full list for accesses
2. This driver can be also used by BE Linux on Microblaze in PL
that's why there could be a need to autodetect endianess directly
on the IP itself. Then using helper function is necessary.
3. We have met with secure monitor implementation where all these
io accesses should be done via smc calls and having
IO helper function is easier for change.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]