lkml.org 
[lkml]   [2022]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 2/4] Watchdog: sp5100_tco: Refactor MMIO base address initialization
From
Date
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
>

\
 
 \ /
  Last update: 2022-01-19 18:08    [W:0.086 / U:0.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site