Re: [PATCH] sp5100_tco: Add SB8x0 chipset support

[Paul Menzel <paulepanter@users.sourceforge.net>]

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

Am Sonntag, den 12.08.2012, 20:57 +0900 schrieb Takahisa Tanaka:
> The current sp5100_tco driver only supports SP5100/SB7x0 chipset, doesn't
> support SB8x0 chipset, because current sp5100_tco driver doesn't know that the
> offset address for watchdog timer was changed from SB8x0 chipset.
> 
> The offset address of SP5100 and SB7x0 chipsets are as follows, quote from the
> AMD SB700/710/750 Register Reference Guide(Page 164) and the AMD SP5100
> Register Reference Guide(Page 166).
> 
>   WatchDogTimerControl 69h
>   WatchDogTimerBase0   6Ch
>   WatchDogTimerBase1   6Dh
>   WatchDogTimerBase2   6Eh
>   WatchDogTimerBase3   6Fh
> 
> In contrast, the offset address of SB8x0 chipset is as follows, quote from
> AMD SB800-Series Southbridges Register Reference Guide(Page 147).
> 
>   WatchDogTimerEn      48h
>   WatchDogTimerConfig  4Ch
> 
> So, In the case of SB8x0 chipset, sp5100_tco reads meaningless MMIO
> address(for example, 0xbafe00) from wrong offset address, and the following
> message is logged.
> 
>    SP5100 TCO timer: mmio address 0xbafe00 already in use
> 
> With this patch, sp5100_tco driver supports SB8x0 chipset, and can avoid
> iomem resource conflict. The processing of this patch is as follows.
> 
>  Step 1) Attempt to get the watchdog base address from indirect I/O(0xCD6
>          and 0xCD7).
>   - Go to the step 7 if obtained address hasn't conflicted with other
>     resource. But, currently, the address(0xfec000f0) conflicts with the
>     IOAPIC MMIO address, and the following message is logged.
> 
>        SP5100 TCO timer: mmio address 0xfec000f0 already in use
> 
>     0xfec000f0 is recommended by AMD BIOS Developer's Guide. So, go to the
>     next step.
> 
>  Step 2) Attempt to get the SBResource_MMIO base address from AcpiMmioEN(for
>          SB8x0,  PM_Reg:24h) or SBResource_MMIO(SP5100/SB7x0, PCI_Reg:9Ch)
>          register.
>   - Go to the step 7 if these register has enabled by BIOS, and obtained
>     address hasn't conflicted with other resource.
>   - If above condition isn't true, go to the next step.
> 
>  Step 3) Attempt to get the free MMIO address from allocate_resource().
>   - Go to the step 7 if these register has enabled by BIOS, and obtained
>     address hasn't conflicted with other resource.
>   - Driver initialization has failed if obtained address has conflicted
>     with other resource, and no 'force_addr' parameter is specified.
> 
>  Step 4) Use the specified address If 'force_addr' parameter is specified.
>   - allocate_resource() function may fail, when the PCI bridge device occupies
>     iomem resource from 0xf0000000 to 0xffffffff. To handle such a case,
>     I added 'force_addr' parameter to sp5100_tco driver. With 'force_addr'
>     parameter, sp5100_tco driver directly can assign MMIO address for watchdog
>     timer from free iomem region. Note that It's dangerous to specify wrong
>     address in the 'force_addr' parameter.
> 
>       Example of force_addr parameter use
>         # cat /proc/iomem
>         ...snip...
>         fec00000-fec003ff : IOAPIC 0
>                                       <--- free MMIO region
>         fec10000-fec1001f : pnp 00:0b
>         fec20000-fec203ff : IOAPIC 1
>         ...snip...
>         # cat /etc/modprobe.d/sp5100_tco.conf
>         options sp5100_tco force_addr=0xfec00800
>         # modprobe sp5100_tco
>         # cat /proc/iomem
>         ...snip...
>         fec00000-fec003ff : IOAPIC 0
>         fec00800-fec00807 : SP5100 TCO  <--- watchdog timer MMIO address
>         fec10000-fec1001f : pnp 00:0b
>         fec20000-fec203ff : IOAPIC 1
>         ...snip...
>         #
> 
>   - Driver initialization has failed if specified address has conflicted
>     with other resource.
> 
>  Step 5) Disable the watchdog timer
>   - To rewrite the watchdog timer register of the chipset, absolutely
>     guarantee that the watchdog timer is disabled.
> 
>  Step 6) Re-program the watchdog timer MMIO address to chipset.
>   - Re-program the obtained MMIO address in Step 3 or Step 4 to chipset via
>     indirect I/O(0xCD6 and 0xCD7).
> 
>  Step 7) Enable and setup the watchdog timer
> 
> This patch has worked fine on my test environment(ASUS M4A89GTD-PRO/USB3 and
> DL165G7). therefore I believe that it's no problem to re-program the MMIO
> address for watchdog timer to chipset during disabled watchdog. However,
> I'm not sure about it, because I don't know much about chipset programming.
> 
> So, any comments will be welcome.
> 
> Tested-by: Paul Menzel <paulepanter@users.sourceforge.net>

ASRock A780FullHD <http://www.asrock.com/mb/overview.asp?model=a780fullhd>

> CC: stable@kernel.org

I am sorry. This should have been <stable@vger.kernel.org>.

> Signed-off-by: Takahisa Tanaka <mc74hc00@gmail.com>

After review from the maintainers, please also add

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43176

when sending `PATCH v2` (`--subject-prefix` [1]).

[1] http://wireless.kernel.org/en/developers/Documentation/git-guide#Annotating_new_revision

> ---
>  drivers/watchdog/sp5100_tco.c | 291 ++++++++++++++++++++++++++++++++++++------
>  drivers/watchdog/sp5100_tco.h |  46 +++++--
>  2 files changed, 286 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> index ae5e82c..36e917f 100644
> --- a/drivers/watchdog/sp5100_tco.c
> +++ b/drivers/watchdog/sp5100_tco.c
> @@ -13,7 +13,9 @@
>   *	as published by the Free Software Foundation; either version
>   *	2 of the License, or (at your option) any later version.
>   *
> - *	See AMD Publication 43009 "AMD SB700/710/750 Register Reference Guide"
> + *	See AMD Publication 43009 "AMD SB700/710/750 Register Reference Guide",
> + *	    AMD Publication 45482 "AMD SB800-Series Sourthbridges Register

Sou*thbridges

> + *	                                                      Reference Guide"
>   */
>  
>  /*
> @@ -38,18 +40,23 @@
>  #include "sp5100_tco.h"
>  
>  /* Module and version information */
> -#define TCO_VERSION "0.01"
> +#define TCO_VERSION "0.03"
>  #define TCO_MODULE_NAME "SP5100 TCO timer"
>  #define TCO_DRIVER_NAME   TCO_MODULE_NAME ", v" TCO_VERSION
>  
>  /* internal variables */
>  static u32 tcobase_phys;
> +static u32 resbase_phys;
>  static void __iomem *tcobase;
>  static unsigned int pm_iobase;
>  static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */
>  static unsigned long timer_alive;
>  static char tco_expect_close;
>  static struct pci_dev *sp5100_tco_pci;
> +static struct resource wdt_res = {
> +	.name = "Watchdog Timer",
> +	.flags = IORESOURCE_MEM,
> +};
>  
>  /* the watchdog platform device */
>  static struct platform_device *sp5100_tco_platform_device;
> @@ -67,6 +74,12 @@ module_param(nowayout, bool, 0);
>  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started"
>  		" (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>  
> +static unsigned int force_addr;
> +module_param(force_addr, uint, 0);
> +MODULE_PARM_DESC(force_addr, "Force the use of specified MMIO address, "
> +		"default is disabled. DON'T USE THIS PARAMETER ONLY "
> +		"IF YOU REALLY KNOW WHAT YOU ARE DOING");

ONLY USE THIS PARAMETER IF YOU REALLY KNOW WHAT YOU ARE DOING!

[…]


Thanks,

Paul

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]