lkml.org 
[lkml]   [2022]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v1] regmap: add generic indirect regmap support


On Thu, 9 Jun 2022, Mark Brown wrote:

> On Wed, Jun 08, 2022 at 11:54:26PM +0000, Zhang, Tianfei wrote:
>
>>>> Would a different name help?
>>>
>>> This wouldn't address the major problem which is...
>>>
>>>>>> + writel(0, ctx->base + INDIRECT_CMD_OFF);
>>>>>> + ret = readl_poll_timeout((ctx->base + INDIRECT_CMD_OFF), cmd,
>>>>>> + (!cmd), INDIRECT_INT_US,
>>> INDIRECT_TIMEOUT_US);
>>>>>> + if (ret)
>>>>>> + dev_err(ctx->dev, "%s timed out on clearing cmd 0x%xn",
>>>>>> +__func__, cmd);
>>>
>>>>> ...and this doesn't look particularly generic, it looks like it's
>>>>> for some particular controller/bridge?
>>>
>>> ...that this appears to be entirely specific to some particular device, it's got
>>> things like hard coded register addresses and timeouts which mean it can't be
>>> reused.
>>
>> Yet, this is a register access hardware controller/bridge widely used in FPGA IP blocks, like PMCI, HSSI.
>> How about we change the patch title like this:
>
>> regmap: add indirect register controller support
>
> No, please enage with my feedback above.
>

Hi Mark,

I think part of the confusion is that this patch should have been included
in a patch set that actually uses this regmap. This patch really should
be included with the following:

https://lore.kernel.org/all/20220607032833.3482-1-tianfei.zhang@intel.com

The hard coded register definitions are offsets to the passed in void
__iomem base address. This set of registers provides the semantics of
indirect register read/write to whatever the register set is connected
to on the back end. Conceptually this could be considered a specific type
indirect register access controller, but currently we have very different
backend implementations in RTL. Part of our intent is to have consistent
register interfaces for our FPGA IP so multiple drivers can reuse this
regmap.

I totally agree the hardcoded timeout values used for polling should be
parameterized.

We would like to submit a v2 patch set that combines this patch with the
first consumer of the regmap, PMCI. We would also parameterize the
timeout values, but most importantly the name must be better. It is a
long name, but how about something like "Intel FPGA Indirect Register
Interface"?

Matthew Gerlach

\
 
 \ /
  Last update: 2022-06-10 02:31    [W:0.051 / U:1.300 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site