Help needed: supporting new device with unique register bitfields

Help needed: supporting new device with unique register bitfields

[Feiyang Chen]

Hi, all,

We are hoping to add support for a new device which shares almost
identical logic with dwmac1000 (dwmac_lib.c, dwmac1000_core.c, and
dwmac1000_dma.c), but with significant differences in the register
bitfields (dwmac_dma.h and dwmac1000.h).

We are seeking guidance on the best approach to support this new
device. Any advice on how to proceed would be greatly appreciated.

Thank you for your time and expertise.

Thanks,
Feiyang

Re: Help needed: supporting new device with unique register bitfields

[Jakub Kicinski]

On Sun, 23 Apr 2023 16:19:11 +0800 Feiyang Chen wrote:
> We are hoping to add support for a new device which shares almost
> identical logic with dwmac1000 (dwmac_lib.c, dwmac1000_core.c, and
> dwmac1000_dma.c), but with significant differences in the register
> bitfields (dwmac_dma.h and dwmac1000.h).
> 
> We are seeking guidance on the best approach to support this new
> device. Any advice on how to proceed would be greatly appreciated.
> 
> Thank you for your time and expertise.

There's no recipe on how to support devices with different register
layout :(  You'll need to find the right balance of (1) indirect calls,
(2) if conditions and (3) static description data that's right for you.

Static description data (e.g. putting register addresses in a struct
and using the members of that struct rather than #defines) is probably
the best but the least flexible.

Adding the stmmac maintainers.

Re: Help needed: supporting new device with unique register bitfields

[Daniel.Machon]

Den Mon, Apr 24, 2023 at 12:53:57PM -0700 skrev Jakub Kicinski:
> On Sun, 23 Apr 2023 16:19:11 +0800 Feiyang Chen wrote:
> > We are hoping to add support for a new device which shares almost
> > identical logic with dwmac1000 (dwmac_lib.c, dwmac1000_core.c, and
> > dwmac1000_dma.c), but with significant differences in the register
> > bitfields (dwmac_dma.h and dwmac1000.h).
> > 
> > We are seeking guidance on the best approach to support this new
> > device. Any advice on how to proceed would be greatly appreciated.
> > 
> > Thank you for your time and expertise.
> 
> There's no recipe on how to support devices with different register
> layout :(  You'll need to find the right balance of (1) indirect calls,
> (2) if conditions and (3) static description data that's right for you.
> 
> Static description data (e.g. putting register addresses in a struct
> and using the members of that struct rather than #defines) is probably
> the best but the least flexible.
> 
> Adding the stmmac maintainers.

Hi,

I thought I would chip in, as we are facing a similar case as described
here.

I am working on adding support for a new chip, that shares most of the
same logic as Sparx5, yet with differences in register layout. Our
approach has basically been what Jakub describes.

- We use a macro-based approach for accessing registers (see
  sparx5_main_regs.h). We have added a layer of indirection, so that any
  _differences_ between the two chips (offset, count, width etc.) have
  been moved into respective structs, which is then consulted when
  accessing registers. This allows us to reuse most of the Sparx5 code.

- Any unique chip features or similar are ops'ed out. In a few cases
  handled by if's.

- Additionally, chip-specific constants like port count, buffer sizes
  etc.  are statically described for the driver.

I think this has worked out pretty well so far. But again, this is
balance, as Jakub said. If the differences are too great, it might not
be the best solution for you.

/Daniel

Re: Help needed: supporting new device with unique register bitfields

[Feiyang Chen]

On Tue, Apr 25, 2023 at 3:54 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sun, 23 Apr 2023 16:19:11 +0800 Feiyang Chen wrote:
> > We are hoping to add support for a new device which shares almost
> > identical logic with dwmac1000 (dwmac_lib.c, dwmac1000_core.c, and
> > dwmac1000_dma.c), but with significant differences in the register
> > bitfields (dwmac_dma.h and dwmac1000.h).
> >
> > We are seeking guidance on the best approach to support this new
> > device. Any advice on how to proceed would be greatly appreciated.
> >
> > Thank you for your time and expertise.
>
> There's no recipe on how to support devices with different register
> layout :(  You'll need to find the right balance of (1) indirect calls,
> (2) if conditions and (3) static description data that's right for you.
>
> Static description data (e.g. putting register addresses in a struct
> and using the members of that struct rather than #defines) is probably
> the best but the least flexible.
>
> Adding the stmmac maintainers.

Hi, Jakub,

Thank you very much!

Thanks,
Feiyang

Re: Help needed: supporting new device with unique register bitfields

[Feiyang Chen]

On Fri, Apr 28, 2023 at 4:47 PM <Daniel.Machon@microchip.com> wrote:
>
> Den Mon, Apr 24, 2023 at 12:53:57PM -0700 skrev Jakub Kicinski:
> > On Sun, 23 Apr 2023 16:19:11 +0800 Feiyang Chen wrote:
> > > We are hoping to add support for a new device which shares almost
> > > identical logic with dwmac1000 (dwmac_lib.c, dwmac1000_core.c, and
> > > dwmac1000_dma.c), but with significant differences in the register
> > > bitfields (dwmac_dma.h and dwmac1000.h).
> > >
> > > We are seeking guidance on the best approach to support this new
> > > device. Any advice on how to proceed would be greatly appreciated.
> > >
> > > Thank you for your time and expertise.
> >
> > There's no recipe on how to support devices with different register
> > layout :(  You'll need to find the right balance of (1) indirect calls,
> > (2) if conditions and (3) static description data that's right for you.
> >
> > Static description data (e.g. putting register addresses in a struct
> > and using the members of that struct rather than #defines) is probably
> > the best but the least flexible.
> >
> > Adding the stmmac maintainers.
>
> Hi,
>
> I thought I would chip in, as we are facing a similar case as described
> here.
>
> I am working on adding support for a new chip, that shares most of the
> same logic as Sparx5, yet with differences in register layout. Our
> approach has basically been what Jakub describes.
>
> - We use a macro-based approach for accessing registers (see
>   sparx5_main_regs.h). We have added a layer of indirection, so that any
>   _differences_ between the two chips (offset, count, width etc.) have
>   been moved into respective structs, which is then consulted when
>   accessing registers. This allows us to reuse most of the Sparx5 code.
>
> - Any unique chip features or similar are ops'ed out. In a few cases
>   handled by if's.
>
> - Additionally, chip-specific constants like port count, buffer sizes
>   etc.  are statically described for the driver.
>

Hi, Daniel,

That sounds like a great idea. I'll definitely give it a try.

Thanks,
Feiyang

> I think this has worked out pretty well so far. But again, this is
> balance, as Jakub said. If the differences are too great, it might not
> be the best solution for you.
>
> /Daniel