| Message ID | 20220118202234.410555-3-terry.bowman@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Watchdog: sp5100_tco: Replace cd6h/cd7h port I/O accesses with MMIO accesses | expand |
On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <terry.bowman@amd.com> wrote: > > Combine MMIO base address and alternate base address detection. Combine > based on layout type. This will simplify the function by eliminating > a switch case. > > Move existing request/release code into functions. This currently only > supports port I/O request/release. The move into a separate function > will make it ready for adding MMIO region support. ... > To: Guenter Roeck <linux@roeck-us.net> > To: linux-watchdog@vger.kernel.org > To: Jean Delvare <jdelvare@suse.com> > To: linux-i2c@vger.kernel.org > To: Wolfram Sang <wsa@kernel.org> > To: Andy Shevchenko <andy.shevchenko@gmail.com> > To: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: linux-kernel@vger.kernel.org > Cc: Wim Van Sebroeck <wim@linux-watchdog.org> > Cc: Robert Richter <rrichter@amd.com> > Cc: Thomas Lendacky <thomas.lendacky@amd.com> Same comment to all your patches. ... > +static int __sp5100_tco_prepare_base(struct sp5100_tco *tco, > + u32 mmio_addr, > + const char *dev_name) > +{ > + struct device *dev = tco->wdd.parent; > + int ret = 0; Not really used variable. > + if (!mmio_addr) > + return -ENOMEM; > + > + if (!devm_request_mem_region(dev, mmio_addr, > + SP5100_WDT_MEM_MAP_SIZE, > + dev_name)) { > + dev_dbg(dev, "MMIO address 0x%08x already in use\n", > + mmio_addr); > + return -EBUSY; > + } > + > + tco->tcobase = devm_ioremap(dev, mmio_addr, > + SP5100_WDT_MEM_MAP_SIZE); > + if (!tco->tcobase) { > + dev_dbg(dev, "MMIO address 0x%08x failed mapping.\n", > + mmio_addr); > + devm_release_mem_region(dev, mmio_addr, > + SP5100_WDT_MEM_MAP_SIZE); Why? If it's a short live mapping, do not use devm. > + return -ENOMEM; > + } > + dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", > + mmio_addr); Unneeded noise. > + return ret; On top of above it's a NIH devm_ioremap_resource(). > +} ... > + int ret = 0; Redundant assignment. ... > + /* Check MMIO address conflict */ > + ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name); > + > + /* Check alternate MMIO address conflict */ Unify this with the previous comment. > + if (ret) > + ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr, > + dev_name); ... > + if (alt_mmio_addr & ((SB800_ACPI_MMIO_DECODE_EN | > + SB800_ACPI_MMIO_SEL) != > + SB800_ACPI_MMIO_DECODE_EN)) { The split looks ugly. Consider to use temporary variables or somehow rearrange the condition that it doesn't break in the middle of the one logical token. > + alt_mmio_addr &= ~0xFFF; Why capital letters? > + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET; > + } ... > + if (!(alt_mmio_addr & (((SB800_ACPI_MMIO_DECODE_EN | > + SB800_ACPI_MMIO_SEL)) != > + SB800_ACPI_MMIO_DECODE_EN))) { Ditto. > + alt_mmio_addr &= ~0xFFF; Ditto. > + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET; ... Okay, I see this is the original code like this... Perhaps it makes sense to reshuffle them (indentation-wise) at the same time and mention this in the changelog. ... > release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE); Is it still needed? I have no context to say if devm_iomap() and this are not colliding, please double check the correctness.
On 1/19/22 3:53 AM, Andy Shevchenko wrote: > On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <terry.bowman@amd.com> wrote: >> >> Combine MMIO base address and alternate base address detection. Combine >> based on layout type. This will simplify the function by eliminating >> a switch case. >> >> Move existing request/release code into functions. This currently only >> supports port I/O request/release. The move into a separate function >> will make it ready for adding MMIO region support. > > ... > >> To: Guenter Roeck <linux@roeck-us.net> >> To: linux-watchdog@vger.kernel.org >> To: Jean Delvare <jdelvare@suse.com> >> To: linux-i2c@vger.kernel.org >> To: Wolfram Sang <wsa@kernel.org> >> To: Andy Shevchenko <andy.shevchenko@gmail.com> >> To: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> Cc: linux-kernel@vger.kernel.org >> Cc: Wim Van Sebroeck <wim@linux-watchdog.org> >> Cc: Robert Richter <rrichter@amd.com> >> Cc: Thomas Lendacky <thomas.lendacky@amd.com> > > Same comment to all your patches. > > ... > >> +static int __sp5100_tco_prepare_base(struct sp5100_tco *tco, >> + u32 mmio_addr, >> + const char *dev_name) >> +{ >> + struct device *dev = tco->wdd.parent; > >> + int ret = 0; > > Not really used variable. > >> + if (!mmio_addr) >> + return -ENOMEM; >> + >> + if (!devm_request_mem_region(dev, mmio_addr, >> + SP5100_WDT_MEM_MAP_SIZE, >> + dev_name)) { >> + dev_dbg(dev, "MMIO address 0x%08x already in use\n", >> + mmio_addr); >> + return -EBUSY; >> + } >> + >> + tco->tcobase = devm_ioremap(dev, mmio_addr, >> + SP5100_WDT_MEM_MAP_SIZE); Talking about line splits, is this one necessary ? >> + if (!tco->tcobase) { >> + dev_dbg(dev, "MMIO address 0x%08x failed mapping.\n", >> + mmio_addr); > >> + devm_release_mem_region(dev, mmio_addr, >> + SP5100_WDT_MEM_MAP_SIZE); > > Why? If it's a short live mapping, do not use devm. > This is not short lived; it is needed by the driver. The release is an artifact of calling this function twice and ignoring the error from devm_ioremap() if the first call fails. devm_release_mem_region() isn't strictly needed but that would result in keeping the memory region reserved even though it isn't used by the driver. There is a functional difference to the original code, though. The failing devm_ioremap() causes the code to try the alternate address. I am not sure if that really adds value; devm_ioremap() fails because the system is out of virtual memory, and calling it again on a different address doesn't seem to add much value. I preferred the original code, which would only call devm_ioremap() after successfully reserving a memory region. >> + return -ENOMEM; >> + } > >> + dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", >> + mmio_addr); > > Unneeded noise. > >> + return ret; > > On top of above it's a NIH devm_ioremap_resource(). > Not sure what NIH means, but if you refer to the lack of devm_release_mem_region(), again, it isn't short lived. >> +} > > > ... > >> + int ret = 0; > > Redundant assignment. > > ... > >> + /* Check MMIO address conflict */ >> + ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name); > >> + >> + /* Check alternate MMIO address conflict */ > > Unify this with the previous comment. > Why ? It refers to the code below. If that is a single or two comments is really just POV. >> + if (ret) >> + ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr, >> + dev_name); > > ... > >> + if (alt_mmio_addr & ((SB800_ACPI_MMIO_DECODE_EN | >> + SB800_ACPI_MMIO_SEL) != >> + SB800_ACPI_MMIO_DECODE_EN)) { > > The split looks ugly. Consider to use temporary variables or somehow > rearrange the condition that it doesn't break in the middle of the one > logical token. Split at the &, maybe. > >> + alt_mmio_addr &= ~0xFFF; > > Why capital letters? > >> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET; >> + } > > ... > >> + if (!(alt_mmio_addr & (((SB800_ACPI_MMIO_DECODE_EN | >> + SB800_ACPI_MMIO_SEL)) != >> + SB800_ACPI_MMIO_DECODE_EN))) { > > Ditto. > >> + alt_mmio_addr &= ~0xFFF; > > Ditto. > >> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET; > > ... > > Okay, I see this is the original code like this... Perhaps it makes > sense to reshuffle them (indentation-wise) at the same time and > mention this in the changelog. > > ... > >> release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE); > > Is it still needed? I have no context to say if devm_iomap() and this > are not colliding, please double check the correctness. > Not sure I understand. This is the release of the io region reserved with request_muxed_region() at the beginning of this function. Why would it no longer be necessary to release that region ? Guenter
On 1/19/22 5:53 AM, Andy Shevchenko wrote: > On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <terry.bowman@amd.com> wrote: >> >> Combine MMIO base address and alternate base address detection. Combine >> based on layout type. This will simplify the function by eliminating >> a switch case. >> >> Move existing request/release code into functions. This currently only >> supports port I/O request/release. The move into a separate function >> will make it ready for adding MMIO region support. > > ... > >> To: Guenter Roeck <linux@roeck-us.net> >> To: linux-watchdog@vger.kernel.org >> To: Jean Delvare <jdelvare@suse.com> >> To: linux-i2c@vger.kernel.org >> To: Wolfram Sang <wsa@kernel.org> >> To: Andy Shevchenko <andy.shevchenko@gmail.com> >> To: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> Cc: linux-kernel@vger.kernel.org >> Cc: Wim Van Sebroeck <wim@linux-watchdog.org> >> Cc: Robert Richter <rrichter@amd.com> >> Cc: Thomas Lendacky <thomas.lendacky@amd.com> > > Same comment to all your patches. Ok. I'll reduce the patches' to/cc list to only contain maintainers owning the current patch. I prefer to leave the lengthy list in the cover letter if that is ok because it will not be added to the tree but will provide context this series has multiple systems and may need communication between maintainers. I'll use the -to & -cc commandline as you mentioned to send to the longer list of recipients without cluttering the patch. Let me know if you prefer otherwise. > > ... > >> +static int __sp5100_tco_prepare_base(struct sp5100_tco *tco, >> + u32 mmio_addr, >> + const char *dev_name) >> +{ >> + struct device *dev = tco->wdd.parent; > >> + int ret = 0; > > Not really used variable. Yes, I'll remove 'ret' and set hardcoded 0 return value below. > >> + if (!mmio_addr) >> + return -ENOMEM; >> + >> + if (!devm_request_mem_region(dev, mmio_addr, >> + SP5100_WDT_MEM_MAP_SIZE, >> + dev_name)) { >> + dev_dbg(dev, "MMIO address 0x%08x already in use\n", >> + mmio_addr); >> + return -EBUSY; >> + } >> + >> + tco->tcobase = devm_ioremap(dev, mmio_addr, >> + SP5100_WDT_MEM_MAP_SIZE); >> + if (!tco->tcobase) { >> + dev_dbg(dev, "MMIO address 0x%08x failed mapping.\n", >> + mmio_addr); > >> + devm_release_mem_region(dev, mmio_addr, >> + SP5100_WDT_MEM_MAP_SIZE); > > Why? If it's a short live mapping, do not use devm. This region isn't short lived. This is a region request for the WDT registers used through the lifetime of the driver. The short lived mapping you may be thinking of is in sp5100_tco_setupdevice_mmio() from patch 3. The first register in this region is FCH::PM::DECODEEN and is used to setup the mmio_addr and alt_mmio_addr MMIO (longterm) regions. > >> + return -ENOMEM; >> + } > >> + dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", >> + mmio_addr); > > Unneeded noise. This was present pre-series. The current driver dmesg output with default logging settings is: dmesg | grep -i sp5100 [ 8.508515] sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver [ 8.525172] sp5100-tco sp5100-tco: Using 0xfeb00000 for watchdog MMIO address [ 8.539912] sp5100-tco sp5100-tco: initialized. heartbeat=60 sec (nowayout=0) I'll remove the dev_info. > >> + return ret; > > On top of above it's a NIH devm_ioremap_resource(). I'm not familiar with NIH term. My friends google and grep weren't much help. > >> +} > > > ... > >> + int ret = 0; > > Redundant assignment. Thanks. I'll leave the variable but remove the 0 assignment in the definition. > > ... > >> + /* Check MMIO address conflict */ >> + ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name); > >> + >> + /* Check alternate MMIO address conflict */ > > Unify this with the previous comment. Ok > >> + if (ret) >> + ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr, >> + dev_name); > > ... > >> + if (alt_mmio_addr & ((SB800_ACPI_MMIO_DECODE_EN | >> + SB800_ACPI_MMIO_SEL) != >> + SB800_ACPI_MMIO_DECODE_EN)) { > > The split looks ugly. Consider to use temporary variables or somehow > rearrange the condition that it doesn't break in the middle of the one > logical token. I'll try splitting at the '&' as Guenter mentioned in other email. > >> + alt_mmio_addr &= ~0xFFF; > > Why capital letters? This was already present pre-series. I'll change to lowercase to make it consistent with the other constants in the file. > >> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET; >> + } > > ... > >> + if (!(alt_mmio_addr & (((SB800_ACPI_MMIO_DECODE_EN | >> + SB800_ACPI_MMIO_SEL)) != >> + SB800_ACPI_MMIO_DECODE_EN))) { > > Ditto. My understanding is #defines should be capitalized. No? > >> + alt_mmio_addr &= ~0xFFF; > > Ditto. Another uppercase constant I will make lowercase. > >> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET; > > ... > > Okay, I see this is the original code like this... Perhaps it makes > sense to reshuffle them (indentation-wise) at the same time and > mention this in the changelog. > > ... > >> release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE); > > Is it still needed? I have no context to say if devm_iomap() and this > are not colliding, please double check the correctness. > Yes, this is needed. The release/request in sp5100_tco_setupdevice() is for the non-efch mmio layout cases. It is using port I/O registers to detect mmio_addr, alt_mmio_addr, and configure the device. Regards, Terry Bowman
On 1/19/22 8:57 AM, Terry Bowman wrote: > > > On 1/19/22 5:53 AM, Andy Shevchenko wrote: >> On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <terry.bowman@amd.com> wrote: >>> >>> Combine MMIO base address and alternate base address detection. Combine >>> based on layout type. This will simplify the function by eliminating >>> a switch case. >>> >>> Move existing request/release code into functions. This currently only >>> supports port I/O request/release. The move into a separate function >>> will make it ready for adding MMIO region support. >> >> ... >> >>> To: Guenter Roeck <linux@roeck-us.net> >>> To: linux-watchdog@vger.kernel.org >>> To: Jean Delvare <jdelvare@suse.com> >>> To: linux-i2c@vger.kernel.org >>> To: Wolfram Sang <wsa@kernel.org> >>> To: Andy Shevchenko <andy.shevchenko@gmail.com> >>> To: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> Cc: linux-kernel@vger.kernel.org >>> Cc: Wim Van Sebroeck <wim@linux-watchdog.org> >>> Cc: Robert Richter <rrichter@amd.com> >>> Cc: Thomas Lendacky <thomas.lendacky@amd.com> >> >> Same comment to all your patches. > > Ok. I'll reduce the patches' to/cc list to only contain maintainers owning > the current patch. I prefer to leave the lengthy list in the cover letter > if that is ok because it will not be added to the tree but will provide > context this series has multiple systems and may need communication > between maintainers. I'll use the -to & -cc commandline as you mentioned to > send to the longer list of recipients without cluttering the patch. Let me > know if you prefer otherwise. >> >> ... >> >>> +static int __sp5100_tco_prepare_base(struct sp5100_tco *tco, >>> + u32 mmio_addr, >>> + const char *dev_name) >>> +{ >>> + struct device *dev = tco->wdd.parent; >> >>> + int ret = 0; >> >> Not really used variable. > > Yes, I'll remove 'ret' and set hardcoded 0 return value below. > >> >>> + if (!mmio_addr) >>> + return -ENOMEM; >>> + >>> + if (!devm_request_mem_region(dev, mmio_addr, >>> + SP5100_WDT_MEM_MAP_SIZE, >>> + dev_name)) { >>> + dev_dbg(dev, "MMIO address 0x%08x already in use\n", >>> + mmio_addr); >>> + return -EBUSY; >>> + } >>> + >>> + tco->tcobase = devm_ioremap(dev, mmio_addr, >>> + SP5100_WDT_MEM_MAP_SIZE); >>> + if (!tco->tcobase) { >>> + dev_dbg(dev, "MMIO address 0x%08x failed mapping.\n", >>> + mmio_addr); >> >>> + devm_release_mem_region(dev, mmio_addr, >>> + SP5100_WDT_MEM_MAP_SIZE); >> >> Why? If it's a short live mapping, do not use devm. > > This region isn't short lived. This is a region request for the > WDT registers used through the lifetime of the driver. > > The short lived mapping you may be thinking of is in > sp5100_tco_setupdevice_mmio() from patch 3. The first register in > this region is FCH::PM::DECODEEN and is used to setup the mmio_addr > and alt_mmio_addr MMIO (longterm) regions. > >> >>> + return -ENOMEM; >>> + } >> >>> + dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", >>> + mmio_addr); >> >> Unneeded noise. > > This was present pre-series. The current driver dmesg output with default > logging settings is: > > dmesg | grep -i sp5100 > [ 8.508515] sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver > [ 8.525172] sp5100-tco sp5100-tco: Using 0xfeb00000 for watchdog MMIO address > [ 8.539912] sp5100-tco sp5100-tco: initialized. heartbeat=60 sec (nowayout=0) > > I'll remove the dev_info. > >> >>> + return ret; >> >> On top of above it's a NIH devm_ioremap_resource(). > > I'm not familiar with NIH term. My friends google and grep weren't much help. > >> >>> +} >> >> >> ... >> >>> + int ret = 0; >> >> Redundant assignment. > > Thanks. I'll leave the variable but remove the 0 assignment in the definition. > >> >> ... >> >>> + /* Check MMIO address conflict */ >>> + ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name); >> >>> + >>> + /* Check alternate MMIO address conflict */ >> >> Unify this with the previous comment. > > Ok > >> >>> + if (ret) >>> + ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr, >>> + dev_name); >> >> ... >> >>> + if (alt_mmio_addr & ((SB800_ACPI_MMIO_DECODE_EN | >>> + SB800_ACPI_MMIO_SEL) != >>> + SB800_ACPI_MMIO_DECODE_EN)) { >> >> The split looks ugly. Consider to use temporary variables or somehow >> rearrange the condition that it doesn't break in the middle of the one >> logical token. > > I'll try splitting at the '&' as Guenter mentioned in other email. > >> >>> + alt_mmio_addr &= ~0xFFF; >> >> Why capital letters? > > This was already present pre-series. I'll change to lowercase to make it > consistent with the other constants in the file. > >> >>> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET; >>> + } >> >> ... >> >>> + if (!(alt_mmio_addr & (((SB800_ACPI_MMIO_DECODE_EN | >>> + SB800_ACPI_MMIO_SEL)) != >>> + SB800_ACPI_MMIO_DECODE_EN))) { >> >> Ditto. > > My understanding is #defines should be capitalized. No? > I think that Ditto referred to the line split comment. Guenter >> >>> + alt_mmio_addr &= ~0xFFF; >> >> Ditto. > > Another uppercase constant I will make lowercase. > >> >>> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET; >> >> ... >> >> Okay, I see this is the original code like this... Perhaps it makes >> sense to reshuffle them (indentation-wise) at the same time and >> mention this in the changelog. >> >> ... >> >>> release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE); >> >> Is it still needed? I have no context to say if devm_iomap() and this >> are not colliding, please double check the correctness. >> > > Yes, this is needed. The release/request in sp5100_tco_setupdevice() is > for the non-efch mmio layout cases. It is using port I/O registers to > detect mmio_addr, alt_mmio_addr, and configure the device. > > Regards, > Terry Bowman >
On Wed, Jan 19, 2022 at 6:57 PM Terry Bowman <Terry.Bowman@amd.com> wrote: > On 1/19/22 5:53 AM, Andy Shevchenko wrote: > > On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <terry.bowman@amd.com> wrote: > Ok. I'll reduce the patches' to/cc list to only contain maintainers owning > the current patch. I prefer to leave the lengthy list in the cover letter > if that is ok because it will not be added to the tree but will provide > context this series has multiple systems and may need communication > between maintainers. I'll use the -to & -cc commandline as you mentioned to > send to the longer list of recipients without cluttering the patch. Let me > know if you prefer otherwise. My point is that: supply the list implicitly. For the help of choosing the right people I have written a script [1] that shows a very good heuristics approach to me. [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh ... > >> + if (!devm_request_mem_region(dev, mmio_addr, > >> + SP5100_WDT_MEM_MAP_SIZE, > >> + dev_name)) { > >> + dev_dbg(dev, "MMIO address 0x%08x already in use\n", > >> + mmio_addr); > >> + return -EBUSY; > >> + } > >> + > >> + tco->tcobase = devm_ioremap(dev, mmio_addr, > >> + SP5100_WDT_MEM_MAP_SIZE); > >> + if (!tco->tcobase) { > >> + dev_dbg(dev, "MMIO address 0x%08x failed mapping.\n", > >> + mmio_addr); > > On top of above it's a NIH devm_ioremap_resource(). > > I'm not familiar with NIH term. My friends google and grep weren't much help. [2]: https://en.wikipedia.org/wiki/Not_invented_here Means that you could very well simplify the code by using existing functions. ... > > Okay, I see this is the original code like this... Perhaps it makes > > sense to reshuffle them (indentation-wise) at the same time and > > mention this in the changelog. Here is the explanation that I noticed that the code you move is original, and not written by you.
On Wed, Jan 19, 2022 at 5:46 PM Guenter Roeck <linux@roeck-us.net> wrote: > On 1/19/22 3:53 AM, Andy Shevchenko wrote: > > On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <terry.bowman@amd.com> wrote: ... > >> + devm_release_mem_region(dev, mmio_addr, > >> + SP5100_WDT_MEM_MAP_SIZE); > > > > Why? If it's a short live mapping, do not use devm. > > This is not short lived; it is needed by the driver. The release > is an artifact of calling this function twice and ignoring the error > from devm_ioremap() if the first call fails. devm_release_mem_region() > isn't strictly needed but that would result in keeping the memory > region reserved even though it isn't used by the driver. So, this seems like micro-optimization, but okay, at least it justifies it. Thanks for explaining. > There is a functional difference to the original code, though. > The failing devm_ioremap() causes the code to try the alternate > address. I am not sure if that really adds value; devm_ioremap() > fails because the system is out of virtual memory, and calling > it again on a different address doesn't seem to add much value. > I preferred the original code, which would only call devm_ioremap() > after successfully reserving a memory region. ... > > On top of above it's a NIH devm_ioremap_resource(). > > Not sure what NIH means Not Invented Here (syndrome) ... > >> + /* Check MMIO address conflict */ > >> + ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name); > > > >> + > >> + /* Check alternate MMIO address conflict */ > > > > Unify this with the previous comment. > > Why ? It refers to the code below. If that is a single or two comments > is really just POV. It depends on the angle from which you see it (i.o.w. like you said POV). I considered it from the code perspective and personally found the /* * Bla bla bla */ ret = foo(); if (ret) bar(); better than above. > >> + if (ret) > >> + ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr, > >> + dev_name); ... > >> release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE); > > > > Is it still needed? I have no context to say if devm_iomap() and this > > are not colliding, please double check the correctness. > > > Not sure I understand. This is the release of the io region reserved with > request_muxed_region() at the beginning of this function. Why would it no > longer be necessary to release that region ? Thank you for explaining, as I said I have no full context here, and I simply asked for double check.
Hi Terry, Sorry for the late review, I hope you did not send an updated series already. On Tue, 18 Jan 2022 14:22:32 -0600, Terry Bowman wrote: > Combine MMIO base address and alternate base address detection. Combine > based on layout type. This will simplify the function by eliminating > a switch case. > > Move existing request/release code into functions. This currently only > supports port I/O request/release. The move into a separate function > will make it ready for adding MMIO region support. > > (...) > +static int __sp5100_tco_prepare_base(struct sp5100_tco *tco, > + u32 mmio_addr, > + const char *dev_name) > +{ > + struct device *dev = tco->wdd.parent; > + int ret = 0; > + > + if (!mmio_addr) > + return -ENOMEM; Can this actually happen? If it does, is -ENOMEM really the best error value? And if it can happen, I think I would prefer if you would simply not call this function, knowing it can only fail. In other words, I'd go for something like the following in the function below: /* Check MMIO address conflict */ if (mmio_addr) ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name); The intention is clearer and execution is faster too. > + > + if (!devm_request_mem_region(dev, mmio_addr, > + SP5100_WDT_MEM_MAP_SIZE, > + dev_name)) { > + dev_dbg(dev, "MMIO address 0x%08x already in use\n", > + mmio_addr); > + return -EBUSY; > + } > + > + tco->tcobase = devm_ioremap(dev, mmio_addr, > + SP5100_WDT_MEM_MAP_SIZE); > + if (!tco->tcobase) { > + dev_dbg(dev, "MMIO address 0x%08x failed mapping.\n", > + mmio_addr); Remove trailing dot for consistency with the other messages. > + devm_release_mem_region(dev, mmio_addr, > + SP5100_WDT_MEM_MAP_SIZE); > + return -ENOMEM; > + } > + > + dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", > + mmio_addr); > + > + return ret; > +} > + > +static int sp5100_tco_prepare_base(struct sp5100_tco *tco, > + u32 mmio_addr, > + u32 alt_mmio_addr, > + const char *dev_name) > +{ > + struct device *dev = tco->wdd.parent; > + int ret = 0; > + > + dev_dbg(dev, "Got 0x%08x from SBResource_MMIO register\n", > + mmio_addr); > + > + /* Check MMIO address conflict */ > + ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name); > + > + /* Check alternate MMIO address conflict */ > + if (ret) > + ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr, > + dev_name); > + > + if (ret) > + dev_err(dev, "Failed to reserve-map MMIO (%X) and alternate MMIO (%X) regions. ret=%X", > + mmio_addr, alt_mmio_addr, ret); Format for the addresses is inconsistent with the other messages above, please use 0x%08x for consistency. As for the return value (which should be preceded by a comma rather than a dot), it should be printed as a decimal, not hexadecimal, value. (And nitpicking: I'd split after "dev," so as to not make the line longer than needed. > + > + return ret; > +} > + > static int sp5100_tco_timer_init(struct sp5100_tco *tco) > { > struct watchdog_device *wdd = &tco->wdd; > @@ -264,6 +324,7 @@ static int sp5100_tco_setupdevice(struct device *dev, > struct sp5100_tco *tco = watchdog_get_drvdata(wdd); > const char *dev_name; > u32 mmio_addr = 0, val; > + u32 alt_mmio_addr = 0; > int ret; > > /* Request the IO ports used by this driver */ > @@ -282,11 +343,35 @@ static int sp5100_tco_setupdevice(struct device *dev, > dev_name = SP5100_DEVNAME; > mmio_addr = sp5100_tco_read_pm_reg32(SP5100_PM_WATCHDOG_BASE) & > 0xfffffff8; > + > + /* > + * Secondly, Find the watchdog timer MMIO address > + * from SBResource_MMIO register. > + */ > + /* Read SBResource_MMIO from PCI config(PCI_Reg: 9Ch) */ > + pci_read_config_dword(sp5100_tco_pci, > + SP5100_SB_RESOURCE_MMIO_BASE, > + &alt_mmio_addr); > + if (alt_mmio_addr & ((SB800_ACPI_MMIO_DECODE_EN | > + SB800_ACPI_MMIO_SEL) != > + SB800_ACPI_MMIO_DECODE_EN)) { > + alt_mmio_addr &= ~0xFFF; > + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET; > + } > break; > case sb800: > dev_name = SB800_DEVNAME; > mmio_addr = sp5100_tco_read_pm_reg32(SB800_PM_WATCHDOG_BASE) & > 0xfffffff8; > + /* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */ > + alt_mmio_addr = > + sp5100_tco_read_pm_reg32(SB800_PM_ACPI_MMIO_EN); > + if (!(alt_mmio_addr & (((SB800_ACPI_MMIO_DECODE_EN | > + SB800_ACPI_MMIO_SEL)) != > + SB800_ACPI_MMIO_DECODE_EN))) { The condition is the opposite of the sp5100 case above, which looks quite suspicious. As far as I can see, that wasn't the case in the original code. Please double check. In any case, please avoid double negations, they are too easy to get wrong. > + alt_mmio_addr &= ~0xFFF; > + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET; > + } > break; > case efch: > dev_name = SB800_DEVNAME; > @@ -305,87 +390,24 @@ static int sp5100_tco_setupdevice(struct device *dev, > val = sp5100_tco_read_pm_reg8(EFCH_PM_DECODEEN); > if (val & EFCH_PM_DECODEEN_WDT_TMREN) > mmio_addr = EFCH_PM_WDT_ADDR; > + > + val = sp5100_tco_read_pm_reg8(EFCH_PM_ISACONTROL); > + if (val & EFCH_PM_ISACONTROL_MMIOEN) > + alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR + > + EFCH_PM_ACPI_MMIO_WDT_OFFSET; > break; > default: > return -ENODEV; > } > (...) Rest looks OK to me.
On 1/25/22 7:45 AM, Jean Delvare wrote: > Hi Terry, > > Sorry for the late review, I hope you did not send an updated series > already. > Hi Jean, No problem. I have not sent another revision yet. I'll add your requested changes in the next revision. > On Tue, 18 Jan 2022 14:22:32 -0600, Terry Bowman wrote: >> Combine MMIO base address and alternate base address detection. Combine >> based on layout type. This will simplify the function by eliminating >> a switch case. >> >> Move existing request/release code into functions. This currently only >> supports port I/O request/release. The move into a separate function >> will make it ready for adding MMIO region support. >> >> (...) >> +static int __sp5100_tco_prepare_base(struct sp5100_tco *tco, >> + u32 mmio_addr, >> + const char *dev_name) >> +{ >> + struct device *dev = tco->wdd.parent; >> + int ret = 0; >> + >> + if (!mmio_addr) >> + return -ENOMEM; > > Can this actually happen? If it does, is -ENOMEM really the best error > value? > This can happen if mmio_addr is not assigned in sp5100_tco_setupdevice_mmio() before calling sp5100_tco_prepare_base() and __sp5100_tco_prepare_base(). I can move the NULL check out of __sp5100_tco_prepare_base() and into sp5100_tco_prepare_base() before calling __sp5100_tco_prepare_base(). As you describe below. The ENOMEM return value should be interpreted as the mmio_addr is not available. EBUSY does not describe the failure correctly because EBUSY implies the resource is present and normally available but not available at this time. Do you have a return value preference ? > And if it can happen, I think I would prefer if you would simply not > call this function, knowing it can only fail. In other words, I'd go > for something like the following in the function below: > > /* Check MMIO address conflict */ > if (mmio_addr) > ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name); > > The intention is clearer and execution is faster too. > Ok >> + >> + if (!devm_request_mem_region(dev, mmio_addr, >> + SP5100_WDT_MEM_MAP_SIZE, >> + dev_name)) { >> + dev_dbg(dev, "MMIO address 0x%08x already in use\n", >> + mmio_addr); >> + return -EBUSY; >> + } >> + >> + tco->tcobase = devm_ioremap(dev, mmio_addr, >> + SP5100_WDT_MEM_MAP_SIZE); >> + if (!tco->tcobase) { >> + dev_dbg(dev, "MMIO address 0x%08x failed mapping.\n", >> + mmio_addr); > > Remove trailing dot for consistency with the other messages. > Ok. >> + devm_release_mem_region(dev, mmio_addr, >> + SP5100_WDT_MEM_MAP_SIZE); >> + return -ENOMEM; >> + } >> + >> + dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", >> + mmio_addr); >> + >> + return ret; >> +} >> + >> +static int sp5100_tco_prepare_base(struct sp5100_tco *tco, >> + u32 mmio_addr, >> + u32 alt_mmio_addr, >> + const char *dev_name) >> +{ >> + struct device *dev = tco->wdd.parent; >> + int ret = 0; >> + >> + dev_dbg(dev, "Got 0x%08x from SBResource_MMIO register\n", >> + mmio_addr); >> + >> + /* Check MMIO address conflict */ >> + ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name); >> + >> + /* Check alternate MMIO address conflict */ >> + if (ret) >> + ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr, >> + dev_name); >> + >> + if (ret) >> + dev_err(dev, "Failed to reserve-map MMIO (%X) and alternate MMIO (%X) regions. ret=%X", >> + mmio_addr, alt_mmio_addr, ret); > > Format for the addresses is inconsistent with the other messages above, > please use 0x%08x for consistency. As for the return value (which > should be preceded by a comma rather than a dot), it should be printed > as a decimal, not hexadecimal, value. > Ok, I'll correct the address format to use '0x', change the period to a comma, and display the the return code as decimal. > (And nitpicking: I'd split after "dev," so as to not make the line > longer than needed. > I'll move the line break at 'dev,'. >> + >> + return ret; >> +} >> + >> static int sp5100_tco_timer_init(struct sp5100_tco *tco) >> { >> struct watchdog_device *wdd = &tco->wdd; >> @@ -264,6 +324,7 @@ static int sp5100_tco_setupdevice(struct device *dev, >> struct sp5100_tco *tco = watchdog_get_drvdata(wdd); >> const char *dev_name; >> u32 mmio_addr = 0, val; >> + u32 alt_mmio_addr = 0; >> int ret; >> >> /* Request the IO ports used by this driver */ >> @@ -282,11 +343,35 @@ static int sp5100_tco_setupdevice(struct device *dev, >> dev_name = SP5100_DEVNAME; >> mmio_addr = sp5100_tco_read_pm_reg32(SP5100_PM_WATCHDOG_BASE) & >> 0xfffffff8; >> + >> + /* >> + * Secondly, Find the watchdog timer MMIO address >> + * from SBResource_MMIO register. >> + */ >> + /* Read SBResource_MMIO from PCI config(PCI_Reg: 9Ch) */ >> + pci_read_config_dword(sp5100_tco_pci, >> + SP5100_SB_RESOURCE_MMIO_BASE, >> + &alt_mmio_addr); >> + if (alt_mmio_addr & ((SB800_ACPI_MMIO_DECODE_EN | >> + SB800_ACPI_MMIO_SEL) != >> + SB800_ACPI_MMIO_DECODE_EN)) { >> + alt_mmio_addr &= ~0xFFF; >> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET; >> + } >> break; >> case sb800: >> dev_name = SB800_DEVNAME; >> mmio_addr = sp5100_tco_read_pm_reg32(SB800_PM_WATCHDOG_BASE) & >> 0xfffffff8; >> + /* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */ >> + alt_mmio_addr = >> + sp5100_tco_read_pm_reg32(SB800_PM_ACPI_MMIO_EN); >> + if (!(alt_mmio_addr & (((SB800_ACPI_MMIO_DECODE_EN | >> + SB800_ACPI_MMIO_SEL)) != >> + SB800_ACPI_MMIO_DECODE_EN))) { > > The condition is the opposite of the sp5100 case above, which looks > quite suspicious. As far as I can see, that wasn't the case in the > original code. Please double check. In any case, please avoid double > negations, they are too easy to get wrong. > Yes, I found this earlier. I have fix for this in the next revision. >> + alt_mmio_addr &= ~0xFFF; >> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET; >> + } >> break; >> case efch: >> dev_name = SB800_DEVNAME; >> @@ -305,87 +390,24 @@ static int sp5100_tco_setupdevice(struct device *dev, >> val = sp5100_tco_read_pm_reg8(EFCH_PM_DECODEEN); >> if (val & EFCH_PM_DECODEEN_WDT_TMREN) >> mmio_addr = EFCH_PM_WDT_ADDR; >> + >> + val = sp5100_tco_read_pm_reg8(EFCH_PM_ISACONTROL); >> + if (val & EFCH_PM_ISACONTROL_MMIOEN) >> + alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR + >> + EFCH_PM_ACPI_MMIO_WDT_OFFSET; >> break; >> default: >> return -ENODEV; >> } >> (...) > > Rest looks OK to me. >
On Tue, 25 Jan 2022 09:18:59 -0600, Terry Bowman wrote: > On 1/25/22 7:45 AM, Jean Delvare wrote: > > On Tue, 18 Jan 2022 14:22:32 -0600, Terry Bowman wrote: > >> +static int __sp5100_tco_prepare_base(struct sp5100_tco *tco, > >> + u32 mmio_addr, > >> + const char *dev_name) > >> +{ > >> + struct device *dev = tco->wdd.parent; > >> + int ret = 0; > >> + > >> + if (!mmio_addr) > >> + return -ENOMEM; > > > > Can this actually happen? If it does, is -ENOMEM really the best error > > value? > > This can happen if mmio_addr is not assigned in sp5100_tco_setupdevice_mmio() > before calling sp5100_tco_prepare_base() and __sp5100_tco_prepare_base(). Ah yes, I can see it now. > I can move the NULL check out of __sp5100_tco_prepare_base() and into > sp5100_tco_prepare_base() before calling __sp5100_tco_prepare_base(). > As you describe below. > > The ENOMEM return value should be interpreted as the mmio_addr is not > available. EBUSY does not describe the failure correctly because EBUSY > implies the resource is present and normally available but not available > at this time. Do you have a return value preference ? Well, if one mmio_addr isn't set, you shouldn't call __sp5100_tco_prepare_base() for it so there's no error to return. If neither mmio_addr is set then the hardware is simply not configured to be used, so that would be a -NODEV returned by sp5100_tco_prepare_base() I suppose? BTW... > >> (...) > >> + if (ret) > >> + dev_err(dev, "Failed to reserve-map MMIO (%X) and alternate MMIO (%X) regions. ret=%X", > >> + mmio_addr, alt_mmio_addr, ret); ... I think that should be a "or" rather than "and", and singular "region", in this error message? I mean, the plan was never to reserve-map both of them, if I understand correctly.
On 1/25/22 10:38 AM, Jean Delvare wrote: > On Tue, 25 Jan 2022 09:18:59 -0600, Terry Bowman wrote: >> On 1/25/22 7:45 AM, Jean Delvare wrote: >>> On Tue, 18 Jan 2022 14:22:32 -0600, Terry Bowman wrote: >>>> +static int __sp5100_tco_prepare_base(struct sp5100_tco *tco, >>>> + u32 mmio_addr, >>>> + const char *dev_name) >>>> +{ >>>> + struct device *dev = tco->wdd.parent; >>>> + int ret = 0; >>>> + >>>> + if (!mmio_addr) >>>> + return -ENOMEM; >>> >>> Can this actually happen? If it does, is -ENOMEM really the best error >>> value? >> >> This can happen if mmio_addr is not assigned in sp5100_tco_setupdevice_mmio() >> before calling sp5100_tco_prepare_base() and __sp5100_tco_prepare_base(). > > Ah yes, I can see it now. > >> I can move the NULL check out of __sp5100_tco_prepare_base() and into >> sp5100_tco_prepare_base() before calling __sp5100_tco_prepare_base(). >> As you describe below. >> >> The ENOMEM return value should be interpreted as the mmio_addr is not >> available. EBUSY does not describe the failure correctly because EBUSY >> implies the resource is present and normally available but not available >> at this time. Do you have a return value preference ? > > Well, if one mmio_addr isn't set, you shouldn't call > __sp5100_tco_prepare_base() for it so there's no error to return. If > neither mmio_addr is set then the hardware is simply not configured to > be used, so that would be a -NODEV returned by > sp5100_tco_prepare_base() I suppose? I agree, -NODEV communicates the error status better. > > BTW... > >>>> (...) >>>> + if (ret) >>>> + dev_err(dev, "Failed to reserve-map MMIO (%X) and alternate MMIO (%X) regions. ret=%X", >>>> + mmio_addr, alt_mmio_addr, ret); > > ... I think that should be a "or" rather than "and", and singular > "region", in this error message? I mean, the plan was never to > reserve-map both of them, if I understand correctly. > This dev_err() is executed when both mmio_addr and alt_mmio_addr addresses failed either devm_request_mem_region() or failed devm_ioremap(). I think the following would be most accurate: dev_err(dev, "Failed to reserve or map the MMIO (0x%X) and alternate MMIO (0x%X) regions, ret=%d", mmio_addr, alt_mmio_addr, ret); Above is my preference but I don't have a strong opinion. Providing the address and error code information in the message is most important. I will make the change as you requested. Regards, Terry
On Tue, 25 Jan 2022 12:02:45 -0600, Terry Bowman wrote: > On 1/25/22 10:38 AM, Jean Delvare wrote: > > On Tue, 25 Jan 2022 09:18:59 -0600, Terry Bowman wrote: > >> On 1/25/22 7:45 AM, Jean Delvare wrote: > >>> On Tue, 18 Jan 2022 14:22:32 -0600, Terry Bowman wrote: > >>>> (...) > >>>> + if (ret) > >>>> + dev_err(dev, "Failed to reserve-map MMIO (%X) and alternate MMIO (%X) regions. ret=%X", > >>>> + mmio_addr, alt_mmio_addr, ret); > > > > ... I think that should be a "or" rather than "and", and singular > > "region", in this error message? I mean, the plan was never to > > reserve-map both of them, if I understand correctly. > > > > This dev_err() is executed when both mmio_addr and alt_mmio_addr addresses failed either > devm_request_mem_region() or failed devm_ioremap(). I think the following would be most accurate: > > dev_err(dev, > "Failed to reserve or map the MMIO (0x%X) and alternate MMIO (0x%X) regions, ret=%d", > mmio_addr, alt_mmio_addr, ret); > > Above is my preference but I don't have a strong opinion. Providing the address and error code > information in the message is most important. I will make the change as you requested. I agree, fine with me, no worry.
diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c index ecc273b9b17f..64ecebd93403 100644 --- a/drivers/watchdog/sp5100_tco.c +++ b/drivers/watchdog/sp5100_tco.c @@ -223,6 +223,66 @@ static u32 sp5100_tco_read_pm_reg32(u8 index) return val; } +static int __sp5100_tco_prepare_base(struct sp5100_tco *tco, + u32 mmio_addr, + const char *dev_name) +{ + struct device *dev = tco->wdd.parent; + int ret = 0; + + if (!mmio_addr) + return -ENOMEM; + + if (!devm_request_mem_region(dev, mmio_addr, + SP5100_WDT_MEM_MAP_SIZE, + dev_name)) { + dev_dbg(dev, "MMIO address 0x%08x already in use\n", + mmio_addr); + return -EBUSY; + } + + tco->tcobase = devm_ioremap(dev, mmio_addr, + SP5100_WDT_MEM_MAP_SIZE); + if (!tco->tcobase) { + dev_dbg(dev, "MMIO address 0x%08x failed mapping.\n", + mmio_addr); + devm_release_mem_region(dev, mmio_addr, + SP5100_WDT_MEM_MAP_SIZE); + return -ENOMEM; + } + + dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", + mmio_addr); + + return ret; +} + +static int sp5100_tco_prepare_base(struct sp5100_tco *tco, + u32 mmio_addr, + u32 alt_mmio_addr, + const char *dev_name) +{ + struct device *dev = tco->wdd.parent; + int ret = 0; + + dev_dbg(dev, "Got 0x%08x from SBResource_MMIO register\n", + mmio_addr); + + /* Check MMIO address conflict */ + ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name); + + /* Check alternate MMIO address conflict */ + if (ret) + ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr, + dev_name); + + if (ret) + dev_err(dev, "Failed to reserve-map MMIO (%X) and alternate MMIO (%X) regions. ret=%X", + mmio_addr, alt_mmio_addr, ret); + + return ret; +} + static int sp5100_tco_timer_init(struct sp5100_tco *tco) { struct watchdog_device *wdd = &tco->wdd; @@ -264,6 +324,7 @@ static int sp5100_tco_setupdevice(struct device *dev, struct sp5100_tco *tco = watchdog_get_drvdata(wdd); const char *dev_name; u32 mmio_addr = 0, val; + u32 alt_mmio_addr = 0; int ret; /* Request the IO ports used by this driver */ @@ -282,11 +343,35 @@ static int sp5100_tco_setupdevice(struct device *dev, dev_name = SP5100_DEVNAME; mmio_addr = sp5100_tco_read_pm_reg32(SP5100_PM_WATCHDOG_BASE) & 0xfffffff8; + + /* + * Secondly, Find the watchdog timer MMIO address + * from SBResource_MMIO register. + */ + /* Read SBResource_MMIO from PCI config(PCI_Reg: 9Ch) */ + pci_read_config_dword(sp5100_tco_pci, + SP5100_SB_RESOURCE_MMIO_BASE, + &alt_mmio_addr); + if (alt_mmio_addr & ((SB800_ACPI_MMIO_DECODE_EN | + SB800_ACPI_MMIO_SEL) != + SB800_ACPI_MMIO_DECODE_EN)) { + alt_mmio_addr &= ~0xFFF; + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET; + } break; case sb800: dev_name = SB800_DEVNAME; mmio_addr = sp5100_tco_read_pm_reg32(SB800_PM_WATCHDOG_BASE) & 0xfffffff8; + /* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */ + alt_mmio_addr = + sp5100_tco_read_pm_reg32(SB800_PM_ACPI_MMIO_EN); + if (!(alt_mmio_addr & (((SB800_ACPI_MMIO_DECODE_EN | + SB800_ACPI_MMIO_SEL)) != + SB800_ACPI_MMIO_DECODE_EN))) { + alt_mmio_addr &= ~0xFFF; + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET; + } break; case efch: dev_name = SB800_DEVNAME; @@ -305,87 +390,24 @@ static int sp5100_tco_setupdevice(struct device *dev, val = sp5100_tco_read_pm_reg8(EFCH_PM_DECODEEN); if (val & EFCH_PM_DECODEEN_WDT_TMREN) mmio_addr = EFCH_PM_WDT_ADDR; + + val = sp5100_tco_read_pm_reg8(EFCH_PM_ISACONTROL); + if (val & EFCH_PM_ISACONTROL_MMIOEN) + alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR + + EFCH_PM_ACPI_MMIO_WDT_OFFSET; break; default: return -ENODEV; } - /* Check MMIO address conflict */ - if (!mmio_addr || - !devm_request_mem_region(dev, mmio_addr, SP5100_WDT_MEM_MAP_SIZE, - dev_name)) { - if (mmio_addr) - dev_dbg(dev, "MMIO address 0x%08x already in use\n", - mmio_addr); - switch (tco->tco_reg_layout) { - case sp5100: - /* - * Secondly, Find the watchdog timer MMIO address - * from SBResource_MMIO register. - */ - /* Read SBResource_MMIO from PCI config(PCI_Reg: 9Ch) */ - pci_read_config_dword(sp5100_tco_pci, - SP5100_SB_RESOURCE_MMIO_BASE, - &mmio_addr); - if ((mmio_addr & (SB800_ACPI_MMIO_DECODE_EN | - SB800_ACPI_MMIO_SEL)) != - SB800_ACPI_MMIO_DECODE_EN) { - ret = -ENODEV; - goto unreg_region; - } - mmio_addr &= ~0xFFF; - mmio_addr += SB800_PM_WDT_MMIO_OFFSET; - break; - case sb800: - /* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */ - mmio_addr = - sp5100_tco_read_pm_reg32(SB800_PM_ACPI_MMIO_EN); - if ((mmio_addr & (SB800_ACPI_MMIO_DECODE_EN | - SB800_ACPI_MMIO_SEL)) != - SB800_ACPI_MMIO_DECODE_EN) { - ret = -ENODEV; - goto unreg_region; - } - mmio_addr &= ~0xFFF; - mmio_addr += SB800_PM_WDT_MMIO_OFFSET; - break; - case efch: - val = sp5100_tco_read_pm_reg8(EFCH_PM_ISACONTROL); - if (!(val & EFCH_PM_ISACONTROL_MMIOEN)) { - ret = -ENODEV; - goto unreg_region; - } - mmio_addr = EFCH_PM_ACPI_MMIO_ADDR + - EFCH_PM_ACPI_MMIO_WDT_OFFSET; - break; - } - dev_dbg(dev, "Got 0x%08x from SBResource_MMIO register\n", - mmio_addr); - if (!devm_request_mem_region(dev, mmio_addr, - SP5100_WDT_MEM_MAP_SIZE, - dev_name)) { - dev_dbg(dev, "MMIO address 0x%08x already in use\n", - mmio_addr); - ret = -EBUSY; - goto unreg_region; - } - } + ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name); + if (!ret) { + /* Setup the watchdog timer */ + tco_timer_enable(tco); - tco->tcobase = devm_ioremap(dev, mmio_addr, SP5100_WDT_MEM_MAP_SIZE); - if (!tco->tcobase) { - dev_err(dev, "failed to get tcobase address\n"); - ret = -ENOMEM; - goto unreg_region; + ret = sp5100_tco_timer_init(tco); } - dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", mmio_addr); - - /* Setup the watchdog timer */ - tco_timer_enable(tco); - - ret = sp5100_tco_timer_init(tco); - -unreg_region: release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE); return ret; }