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: http://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
On 04/01/2014 04:20 AM, Michal Simek wrote:
> 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);
> }
>
Yes, that would make more sense.
Guenter
> 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
>
On 04/02/2014 02:32 AM, Guenter Roeck wrote:
> On 04/01/2014 04:20 AM, Michal Simek wrote:
>> 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);
>> }
>>
> Yes, that would make more sense.
ok good.
Harini: Please use this version instead.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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