| Message ID | 20220118202234.410555-3-terry.bowman@amd.com |
|---|---|
| State | New |
| 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 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; }