> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> Sent: Tuesday, 4 March 2025 15:26
> To: Vadim Pasternak <vadimp@xxxxxxxxxx>
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>; Michael Shych
> <michaelsh@xxxxxxxxxx>; Ciju Rajan K <crajank@xxxxxxxxxx>; Felix
> Radensky <fradensky@xxxxxxxxxx>; Oleksandr Shamray
> <oleksandrs@xxxxxxxxxx>; platform-driver-x86@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v6 2/9] platform/mellanox mlxreg-hotplug: Add support
> for new flavor of capability registers
>
> On Tue, 11 Feb 2025, Vadim Pasternak wrote:
>
> Hi Vadim,
>
> > Hotplug platform data is common across the various systems, while
> > hotplug driver should be able to configure only the instances relevant
> > to specific system.
> >
> > For example, platform hoptplug data might contain descriptions for
> > fan1,
>
> hotplug
>
> > fan2, ..., fan{n}, while some systems equipped with all 'n' fans,
> > others with less. Same for power units, power controllers, ASICs and
> > so on.
> >
> > New 'capability_mask' is introduced to allow sharing of same hoptplug
> > structure between different systems, equipped with different number of
> > hotplug devices. It contains superset mask for all systems sharing the
> > same configuration.
> >
> > The purpose is to reduce unnecessary duplication of hoptplug
> > structures
>
> hotplug
>
> > between different systems - same structure is to be used for example
> > for system equipped fir for 4, 6 or 8 fans.
> >
> > For detection of the real number of equipped devices capability
> > registers are used. These registers used to indicate presence of
> > hotplug devices. On some systems presence is porvided through the
>
> provided
>
> > bitmap. For some new big modular systems, these registers will provide
> > presence by counters. Use slot parameter to determine whether
> > capability register contains bitmask or counter.
> >
> > Reviewed-by: Felix Radensky <fradensky@xxxxxxxxxx>
> > Signed-off-by: Vadim Pasternak <vadimp@xxxxxxxxxx>
> > ---
> > v5->v6
> > Revised after comments pointed out by Ilpo:
> > - Drop 'capability_bit' from structure 'mlxreg_core_data', since it is
> > not used.
> > ---
> > drivers/platform/mellanox/mlxreg-hotplug.c | 25
> > ++++++++++++++++++++--
> > 1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/platform/mellanox/mlxreg-hotplug.c
> > b/drivers/platform/mellanox/mlxreg-hotplug.c
> > index 0ce9fff1f7d4..93bdd20fd71a 100644
> > --- a/drivers/platform/mellanox/mlxreg-hotplug.c
> > +++ b/drivers/platform/mellanox/mlxreg-hotplug.c
> > @@ -274,6 +274,16 @@ static int mlxreg_hotplug_attr_init(struct
> mlxreg_hotplug_priv_data *priv)
> > if (ret)
> > return ret;
> >
> > + if (!regval)
> > + continue;
> > +
> > + /*
> > + * Remove non-relevant bits: 'regval' contains
> bitmask of attributes or
> > + * number of attributtes, which should be handled.
> While 'capability mask'
>
> attributes
>
> > + * is superset mask.
> > + */
> > + if (item->capability_mask)
> > + regval = (regval & item->capability_mask);
> > item->mask = GENMASK((regval & item->mask) - 1,
> 0);
>
> I still don't understand how this can be correct.
>
> As a last step, the code here is taking GENMASK(<num_of_bits>-1, 0) I
> assume. That would mean regval has a field that is indicated by item->mask
> that the number of bits (or "number of attributes" as mentioned in the
> comment). Is that correct?
>
> Now, in your comment and the commit message, you also say 'regval'
> might contain "bitmask of attributes' and you mask part of the bits away
> from what I assume is a bitmask in the newly introduced code. A bitmask,
> however, is not something that seems directly compatible with GENMASK()
> that inputs bit _indexes_, so how can that be passed directly into GENMASK
> without anything to convert it into number of bits/bit index first???
Hi Ilpo!
Apologize for the delay.
Sorry, I provided misleading description and comment.
It is true, that capability registers can contain bitmask or count.
But for 'hotplug' driver, when 'capability' register is specified in configuration - it
is always the number of elements.
For example, 'capability'register is 2 and item mask is 1111 1111.
Then
item->mask = GENMASK((regval & item->mask) - 1, 0); will be GENMASK(1, 0)
If 'capability' register is 6 and item mask is 1111 1111.
item->mask = GENMASK((regval & item->mask) - 1, 0); will be GENMASK(5, 0)
All registers are of 8 bits size.
Value in capability register can be f.e. 12 for 12 elements (fans, power controllers or anything else).
In such case the first 8 elements will be provided through register "A", 4 others through the register "B".
For A item->mask should be set 1111 1111, for B 0000 1111.
Along with it, intention to use same code for example there are 14 elements (A 1111 1111, for B 0011 1111)
or 16 elements (A 1111 1111, for B 1111 1111)
So, item->mask will be configured 0xff for register A and for register B and then will be cut off
according to the value of 'capability' register.
Patch adds 'capability_mask' and treated it as:
if (item->capability_mask)
regval = (regval & item->capability_mask);
It seems that we need here just to change
item->mask = GENMASK((regval & item->mask) - 1, 0);
to
item->mask = GENMASK(((regval % 8) & item->mask) - 1, 0);
And drop item->capability_mask.
>
> --
> i.
>
> > }
> >
> > @@ -294,7 +304,18 @@ static int mlxreg_hotplug_attr_init(struct
> mlxreg_hotplug_priv_data *priv)
> > if (ret)
> > return ret;
> >
> > - if (!(regval & data->bit)) {
> > + if (data->capability_mask)
> > + regval = (regval & data-
> >capability_mask);
> > +
> > + /*
> > + * In case slot field is provided, capability
> register contains
> > + * counter, otherwise bitmask. Skip non-
> relevant entries if slot
> > + * is set and exceeds counter. Othewise
> validate entry by matching
> > + * bitmask.
> > + */
> > + if (data->slot > regval)
> > + break;
> > + if (!(regval & data->bit) && !data->slot) {
> > data++;
> > continue;
> > }
> > @@ -611,7 +632,7 @@ static int mlxreg_hotplug_set_irq(struct
> mlxreg_hotplug_priv_data *priv)
> > if (ret)
> > goto out;
> >
> > - if (!(regval & data->bit))
> > + if (!(regval & data->bit) && !data->slot)
> > item->mask &= ~BIT(j);
> > }
> > }
> >
