SB7xx watchdog: new driver for review and testing

[Andriy Gapon]

Please review and/or test a new driver for watchdog driver included into AMD SB7xx:
http://people.freebsd.org/~avg/amdsbwd.tgz
I have tested this driver only with SB700 on Gigabyte GA-MA780G-UD3H motherboard.

ichwd driver was used as a starting point for this driver. This can be seen from
some function names, general code organization and some small code snippets.
Many thanks to ichwd authors and maintainers!

Right now I have infrastructure only for building this driver as a module.

Things for which that I need the most feedback/ideas:
1. If the driver actually works on your hardware and the hardware description.
The driver can be tested by loading the driver and doing 'watchdog -t <small
number>'. Having debug.bootverbose=1 may provide additional useful info.
And better to test this from single-user mode with filesystems mounted r/o.

2. Better name for the driver. amdsbwd stands for "AMD S(outh)B(ridge)
WatchDog", but this abbreviation could be cryptic to decipher.

3. Proper location for this driver.
At least on my system this driver needs resources (I/O ports and MEM range) that
are claimed by ACPI, thus I've made it a child of acpi bus. But this driver
doesn't have anything else ACPI-ish in it, so I decided that it doesn't belong
under acpica/ or acpi_support/. Am I correct about this?

Anything else you would like to report or comment or advise to me.
Thank you very much for your help.

--
Andriy Gapon
_______________________________________________
freebsd-acpi@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscribe@..."

[Andriy Gapon-3]

on 18/10/2009 23:10 Andriy Gapon said the following:
> Please review and/or test a new driver for watchdog driver included into AMD
                                                    ^^^^^^^^^-hardware

Oh, and please note things marked with XXX and TODO in the code.


--
Andriy Gapon
_______________________________________________
freebsd-acpi@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscribe@..."

[Rui Paulo-3]

On 18 Oct 2009, at 21:10, Andriy Gapon wrote:

> Please review and/or test a new driver for watchdog driver included  
> into AMD SB7xx:
> http://people.freebsd.org/~avg/amdsbwd.tgz
> I have tested this driver only with SB700 on Gigabyte GA-MA780G-UD3H  
> motherboard.
>
> ichwd driver was used as a starting point for this driver. This can  
> be seen from
> some function names, general code organization and some small code  
> snippets.
> Many thanks to ichwd authors and maintainers!
>
> Right now I have infrastructure only for building this driver as a  
> module.
>
> Things for which that I need the most feedback/ideas:
> 1. If the driver actually works on your hardware and the hardware  
> description.
> The driver can be tested by loading the driver and doing 'watchdog -
> t <small
> number>'. Having debug.bootverbose=1 may provide additional useful  
> info.
> And better to test this from single-user mode with filesystems  
> mounted r/o.
>
> 2. Better name for the driver. amdsbwd stands for "AMD S(outh)B(ridge)
> WatchDog", but this abbreviation could be cryptic to decipher.
>
> 3. Proper location for this driver.
> At least on my system this driver needs resources (I/O ports and MEM  
> range) that
> are claimed by ACPI, thus I've made it a child of acpi bus. But this  
> driver
> doesn't have anything else ACPI-ish in it, so I decided that it  
> doesn't belong
> under acpica/ or acpi_support/. Am I correct about this?
>
> Anything else you would like to report or comment or advise to me.
> Thank you very much for your help.

The driver looks good in general. A few questions:
- Can you make the magic numbers a define ? Where did they come from ?
- Are you missing a device_set_desc() call ?
- If this is what you want to commit, C++ comments are not allowed per-
style

Regards,
--
Rui Paulo

_______________________________________________
freebsd-acpi@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscribe@..."

[Andriy Gapon]

on 19/10/2009 14:17 Rui Paulo said the following:

> On 18 Oct 2009, at 21:10, Andriy Gapon wrote:
>
>> Please review and/or test a new driver for watchdog driver included
>> into AMD SB7xx:
>> http://people.freebsd.org/~avg/amdsbwd.tgz
>> I have tested this driver only with SB700 on Gigabyte GA-MA780G-UD3H
>> motherboard.
>>
>> ichwd driver was used as a starting point for this driver. This can be
>> seen from
>> some function names, general code organization and some small code
>> snippets.
>> Many thanks to ichwd authors and maintainers!
>>
>> Right now I have infrastructure only for building this driver as a
>> module.
>>
>> Things for which that I need the most feedback/ideas:
>> 1. If the driver actually works on your hardware and the hardware
>> description.
>> The driver can be tested by loading the driver and doing 'watchdog -t
>> <small
>> number>'. Having debug.bootverbose=1 may provide additional useful info.
>> And better to test this from single-user mode with filesystems mounted
>> r/o.
>>
>> 2. Better name for the driver. amdsbwd stands for "AMD S(outh)B(ridge)
>> WatchDog", but this abbreviation could be cryptic to decipher.
>>
>> 3. Proper location for this driver.
>> At least on my system this driver needs resources (I/O ports and MEM
>> range) that
>> are claimed by ACPI, thus I've made it a child of acpi bus. But this
>> driver
>> doesn't have anything else ACPI-ish in it, so I decided that it
>> doesn't belong
>> under acpica/ or acpi_support/. Am I correct about this?
>>
>> Anything else you would like to report or comment or advise to me.
>> Thank you very much for your help.
>
> The driver looks good in general. A few questions:
> - Can you make the magic numbers a define ? Where did they come from ?

Yes, will do this.
The numbers are from register definitions in AMD SB700/710/750 Register Reference
Guide:
http://developer.amd.com/assets/43009_sb7xx_rrg_pub_1.00.pdf
I will add a link to the document too.

> - Are you missing a device_set_desc() call ?

Yes, I missed this. Thanks!

> - If this is what you want to commit, C++ comments are not allowed
> per-style

Those lines were a result of quick hacking.
I will remove them altogether,

--
Andriy Gapon
_______________________________________________
freebsd-acpi@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscribe@..."

[Andriy Gapon]

I have put updated version of the driver (C file only) here:
http://people.freebsd.org/~avg/amdsbwd.c

Please let me know how it looks now.
Thank you!

--
Andriy Gapon
_______________________________________________
freebsd-acpi@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscribe@..."

[Rui Paulo-3]

On 19 Oct 2009, at 16:41, Andriy Gapon wrote:

>
> I have put updated version of the driver (C file only) here:
> http://people.freebsd.org/~avg/amdsbwd.c

Looks good to me.

--
Rui Paulo

_______________________________________________
freebsd-acpi@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscribe@..."

[Andriy Gapon]

on 19/10/2009 18:47 Rui Paulo said the following:
> On 19 Oct 2009, at 16:41, Andriy Gapon wrote:
>
>>
>> I have put updated version of the driver (C file only) here:
>> http://people.freebsd.org/~avg/amdsbwd.c
>
> Looks good to me.

Thank you for the review and the help!
I have now produced a diff against the main tree for full integration of this
driver:
http://people.freebsd.org/~avg/amdsbwd.diff


--
Andriy Gapon
_______________________________________________
freebsd-acpi@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscribe@..."