On Mon, Jun 06, 2022 at 09:37:55PM -0400, Tianfei Zhang wrote:
> From: Matthew Gerlach <[email protected]>
>
> This patch adds support for regmap APIs that are intended to be used by
> the drivers of some devices which support generic indirect register access,
> for example PMCI (Platform Management Control Interface) device, HSSI
> (High Speed Serial Interface) device in FPGA.
What is "generic indirect register access"? I'm not clear what this is
intended to support...
> +static int indirect_bus_clr_cmd(struct indirect_ctx *ctx)
> +{
> + unsigned int cmd;
> + int ret;
> +
> + 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?
On Tue, 7 Jun 2022, Mark Brown wrote:
> On Mon, Jun 06, 2022 at 09:37:55PM -0400, Tianfei Zhang wrote:
>> From: Matthew Gerlach <[email protected]>
>>
>> This patch adds support for regmap APIs that are intended to be used by
>> the drivers of some devices which support generic indirect register access,
>> for example PMCI (Platform Management Control Interface) device, HSSI
>> (High Speed Serial Interface) device in FPGA.
>
> What is "generic indirect register access"? I'm not clear what this is
> intended to support...
"indirect register access" is a RTL design pattern we use in FPGAs
frequently. The design pattern involves a small number of registers plus
a little handshake code to access various register spaces inside the FPGA
fabric. The design pattern is "generic" in the sense that the same small
number of registers and handshake can be used with many different IP
components in the FPGA. Historically, the bit definitions and handshaking
was slightly different for each IP component. This is an attempt at a
consistent usage across IP components.
Would a different name help?
>
>> +static int indirect_bus_clr_cmd(struct indirect_ctx *ctx)
>> +{
>> + unsigned int cmd;
>> + int ret;
>> +
>> + 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?
>
On Tue, Jun 07, 2022 at 05:27:38PM -0700, [email protected] wrote:
> On Tue, 7 Jun 2022, Mark Brown wrote:
> > On Mon, Jun 06, 2022 at 09:37:55PM -0400, Tianfei Zhang wrote:
> > > From: Matthew Gerlach <[email protected]>
> > > This patch adds support for regmap APIs that are intended to be used by
> > > the drivers of some devices which support generic indirect register access,
> > > for example PMCI (Platform Management Control Interface) device, HSSI
> > > (High Speed Serial Interface) device in FPGA.
> > What is "generic indirect register access"? I'm not clear what this is
> > intended to support...
> "indirect register access" is a RTL design pattern we use in FPGAs
> frequently. The design pattern involves a small number of registers plus a
> little handshake code to access various register spaces inside the FPGA
> fabric. The design pattern is "generic" in the sense that the same small
> number of registers and handshake can be used with many different IP
> components in the FPGA. Historically, the bit definitions and handshaking
> was slightly different for each IP component. This is an attempt at a
> consistent usage across IP components.
> 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.
> -----Original Message-----
> From: Mark Brown <[email protected]>
> Sent: Wednesday, June 8, 2022 6:43 PM
> To: [email protected]
> Cc: Zhang, Tianfei <[email protected]>; [email protected];
> [email protected]; [email protected]; Wu, Hao
> <[email protected]>; [email protected]; Xu, Yilun <[email protected]>;
> Weight, Russell H <[email protected]>
> Subject: Re: [PATCH v1] regmap: add generic indirect regmap support
>
> On Tue, Jun 07, 2022 at 05:27:38PM -0700, [email protected]
> wrote:
> > On Tue, 7 Jun 2022, Mark Brown wrote:
> > > On Mon, Jun 06, 2022 at 09:37:55PM -0400, Tianfei Zhang wrote:
> > > > From: Matthew Gerlach <[email protected]>
>
> > > > This patch adds support for regmap APIs that are intended to be
> > > > used by the drivers of some devices which support generic indirect
> > > > register access, for example PMCI (Platform Management Control
> > > > Interface) device, HSSI (High Speed Serial Interface) device in FPGA.
>
> > > What is "generic indirect register access"? I'm not clear what this
> > > is intended to support...
>
> > "indirect register access" is a RTL design pattern we use in FPGAs
> > frequently. The design pattern involves a small number of registers
> > plus a little handshake code to access various register spaces inside
> > the FPGA fabric. The design pattern is "generic" in the sense that
> > the same small number of registers and handshake can be used with many
> > different IP components in the FPGA. Historically, the bit
> > definitions and handshaking was slightly different for each IP
> > component. This is an attempt at a consistent usage across IP components.
>
> > 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
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.
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/[email protected]
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
On Thu, Jun 09, 2022 at 05:30:43PM -0700, [email protected] wrote:
> On Thu, 9 Jun 2022, Mark Brown wrote:
> > On Wed, Jun 08, 2022 at 11:54:26PM +0000, Zhang, Tianfei wrote:
> > > > ...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.
> 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/[email protected]
> 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.
...which is all specific to your particular implmentation of this and
not a generic thing that will work for anyone else who implements the
concept of indirected register access and happens to use a different
control mechanism. If you have to use the same register interfaces to
reuse this code then the code isn't generic.