[GIT PULL] FPGA Manager additional changes for 5.10

[GIT PULL] FPGA Manager additional changes for 5.10

[Moritz Fischer]

The following changes since commit 9ba3a0aa09fe505540a3bdd11f0da3b8e9d73055:

  fpga: dfl: create a dfl bus type to support DFL devices (2020-09-09 20:28:16 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mdf/linux-fpga.git tags/fpga-extra-for-5.10

for you to fetch changes up to 41b9b36fe986e15eba0a4220c18d72fa5eb9f0dd:

  fpga: dfl: n3000-nios: Make m10_n3000_info static (2020-09-16 19:16:58 -0700)

----------------------------------------------------------------
FPGA Manager changes for 5.10-rc1

Here is the second set of FPGA changes for the 5.10 merge window.

Mircea's changes are part of a changeset to add support for FPGA based
clock drivers.

My change fixed a whitespace error in that patch that I missed when
initially applying it.

Xu's changes contain additional changes to support the new DFL bus,
including some clean ups, refactoring as well as its first user, the
NIOS 3000 driver.

YueHaibing's change addresses a sparse warning.

All patches have been reviewed on the mailing list, and have been in the
last few linux-next releases (as part of my for-next branch) without issues.

Signed-off-by: Moritz Fischer <mdf@kernel.org>

----------------------------------------------------------------
Mircea Caprioru (1):
      include: fpga: adi-axi-common.h: add definitions for supported FPGAs

Moritz Fischer (1):
      include: fpga: adi-axi-common: Fix leading whitespace in header

Xu Yilun (5):
      fpga: dfl: move dfl_device_id to mod_devicetable.h
      dfl: add dfl bus support to MODULE_DEVICE_TABLE()
      fpga: dfl: fix the comments of type & feature_id fields
      fpga: dfl: add support for N3000 Nios private feature
      fpga: dfl: move dfl bus related APIs to include/linux/fpga/dfl.h

YueHaibing (1):
      fpga: dfl: n3000-nios: Make m10_n3000_info static

 .../ABI/testing/sysfs-bus-dfl-devices-n3000-nios   |  49 ++
 Documentation/fpga/dfl-n3000-nios.rst              |  73 +++
 Documentation/fpga/index.rst                       |   1 +
 MAINTAINERS                                        |   3 +-
 drivers/fpga/Kconfig                               |  11 +
 drivers/fpga/Makefile                              |   2 +
 drivers/fpga/dfl-n3000-nios.c                      | 573 +++++++++++++++++++++
 drivers/fpga/dfl.c                                 |   1 +
 drivers/fpga/dfl.h                                 |  85 +--
 include/linux/fpga/adi-axi-common.h                | 103 ++++
 include/linux/fpga/dfl.h                           |  86 ++++
 include/linux/mod_devicetable.h                    |  12 +
 scripts/mod/devicetable-offsets.c                  |   4 +
 scripts/mod/file2alias.c                           |  17 +
 14 files changed, 935 insertions(+), 85 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
 create mode 100644 Documentation/fpga/dfl-n3000-nios.rst
 create mode 100644 drivers/fpga/dfl-n3000-nios.c
 create mode 100644 include/linux/fpga/dfl.h

Re: [GIT PULL] FPGA Manager additional changes for 5.10

[Greg KH]

On Sun, Sep 20, 2020 at 05:08:55PM -0700, Moritz Fischer wrote:
> The following changes since commit 9ba3a0aa09fe505540a3bdd11f0da3b8e9d73055:
> 
>   fpga: dfl: create a dfl bus type to support DFL devices (2020-09-09 20:28:16 -0700)
> 
> are available in the Git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/mdf/linux-fpga.git tags/fpga-extra-for-5.10
> 
> for you to fetch changes up to 41b9b36fe986e15eba0a4220c18d72fa5eb9f0dd:
> 
>   fpga: dfl: n3000-nios: Make m10_n3000_info static (2020-09-16 19:16:58 -0700)
> 
> ----------------------------------------------------------------
> FPGA Manager changes for 5.10-rc1
> 
> Here is the second set of FPGA changes for the 5.10 merge window.
> 
> Mircea's changes are part of a changeset to add support for FPGA based
> clock drivers.
> 
> My change fixed a whitespace error in that patch that I missed when
> initially applying it.
> 
> Xu's changes contain additional changes to support the new DFL bus,
> including some clean ups, refactoring as well as its first user, the
> NIOS 3000 driver.
> 
> YueHaibing's change addresses a sparse warning.
> 
> All patches have been reviewed on the mailing list, and have been in the
> last few linux-next releases (as part of my for-next branch) without issues.
> 
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> 
> ----------------------------------------------------------------
> Mircea Caprioru (1):
>       include: fpga: adi-axi-common.h: add definitions for supported FPGAs
> 
> Moritz Fischer (1):
>       include: fpga: adi-axi-common: Fix leading whitespace in header
> 
> Xu Yilun (5):
>       fpga: dfl: move dfl_device_id to mod_devicetable.h
>       dfl: add dfl bus support to MODULE_DEVICE_TABLE()
>       fpga: dfl: fix the comments of type & feature_id fields
>       fpga: dfl: add support for N3000 Nios private feature
>       fpga: dfl: move dfl bus related APIs to include/linux/fpga/dfl.h
> 
> YueHaibing (1):
>       fpga: dfl: n3000-nios: Make m10_n3000_info static
> 
>  .../ABI/testing/sysfs-bus-dfl-devices-n3000-nios   |  49 ++
>  Documentation/fpga/dfl-n3000-nios.rst              |  73 +++
>  Documentation/fpga/index.rst                       |   1 +
>  MAINTAINERS                                        |   3 +-
>  drivers/fpga/Kconfig                               |  11 +
>  drivers/fpga/Makefile                              |   2 +
>  drivers/fpga/dfl-n3000-nios.c                      | 573 +++++++++++++++++++++
>  drivers/fpga/dfl.c                                 |   1 +
>  drivers/fpga/dfl.h                                 |  85 +--
>  include/linux/fpga/adi-axi-common.h                | 103 ++++
>  include/linux/fpga/dfl.h                           |  86 ++++
>  include/linux/mod_devicetable.h                    |  12 +
>  scripts/mod/devicetable-offsets.c                  |   4 +
>  scripts/mod/file2alias.c                           |  17 +

It's hard to comment on patches on a pull request, but this series still
needs some work.

I'll try to list the patch names here and comments, but really, patches
would be best to make it easier to review:

	0001-include-fpga-adi-axi-common.h-add-definitions-for-su.patch
		- no users of any of these things you added, why is this
		  patch needed?

	0002-fpga-dfl-move-dfl_device_id-to-mod_devicetable.h.patch
		- dfl_device_id is not descriptive, it means nothing to
		  anyone outside of the fpga subsystem.
		- fpga_dfl_device_id perhaps instead?  That gives people
		  a chance to know where to look for this

	0003-dfl-add-dfl-bus-support-to-MODULE_DEVICE_TABLE.patch
		- same "prefix" issues as patch 0002, make it "fpga_dfl"
		  please.

	0004-fpga-dfl-fix-the-comments-of-type-feature_id-fields.patch
		- You talk about 4 bits here, and then point to an
		  enumerated type with no actual values set for them at
		  all.  How is that possible???

	0005-include-fpga-adi-axi-common-Fix-leading-whitespace-i.patch
		- wrong usage of "Fixes" in a signed-off-by area, please
		  see the submitting patches documentation for the
		  correct format.  My scripts, and linux-next's scripts
		  would have caught this, making this pull request not
		  be able to be accepted anyway...

	0006-fpga-dfl-add-support-for-N3000-Nios-private-feature.patch
		- module parameters are for drivers written in the
		  1990's.  Please just "do the right thing" and make the
		  code work properly without having to have custom
		  options.  Note this option does not really work if you
		  have multiple devices in the system at once, which is
		  one reason why we don't use module parameters anymore.

	0007-fpga-dfl-move-dfl-bus-related-APIs-to-include-linux-.patch
		- no one uses this header file move, so don't do it
		  until it is required.

	0008-fpga-dfl-n3000-nios-Make-m10_n3000_info-static.patch
		- as patch 0006 is going to be redone, this can be
		  merged into that when completed.  Why the original
		  developer didn't run sparse on the file to start with,
		  I don't know :(


Wow, 8 out of 8 patches rejected, I think that's a new record!  :)

thanks,

greg k-h

Re: [GIT PULL] FPGA Manager additional changes for 5.10

[Moritz Fischer]

Everyone,

On Tue, Sep 22, 2020 at 10:13:39AM +0200, Greg KH wrote:
> On Sun, Sep 20, 2020 at 05:08:55PM -0700, Moritz Fischer wrote:
> > The following changes since commit 9ba3a0aa09fe505540a3bdd11f0da3b8e9d73055:
> > 
> >   fpga: dfl: create a dfl bus type to support DFL devices (2020-09-09 20:28:16 -0700)
> > 
> > are available in the Git repository at:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/mdf/linux-fpga.git tags/fpga-extra-for-5.10
> > 
> > for you to fetch changes up to 41b9b36fe986e15eba0a4220c18d72fa5eb9f0dd:
> > 
> >   fpga: dfl: n3000-nios: Make m10_n3000_info static (2020-09-16 19:16:58 -0700)
> > 
> > ----------------------------------------------------------------
> > FPGA Manager changes for 5.10-rc1
> > 
> > Here is the second set of FPGA changes for the 5.10 merge window.
> > 
> > Mircea's changes are part of a changeset to add support for FPGA based
> > clock drivers.
> > 
> > My change fixed a whitespace error in that patch that I missed when
> > initially applying it.
> > 
> > Xu's changes contain additional changes to support the new DFL bus,
> > including some clean ups, refactoring as well as its first user, the
> > NIOS 3000 driver.
> > 
> > YueHaibing's change addresses a sparse warning.
> > 
> > All patches have been reviewed on the mailing list, and have been in the
> > last few linux-next releases (as part of my for-next branch) without issues.
> > 
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > 
> > ----------------------------------------------------------------
> > Mircea Caprioru (1):
> >       include: fpga: adi-axi-common.h: add definitions for supported FPGAs
> > 
> > Moritz Fischer (1):
> >       include: fpga: adi-axi-common: Fix leading whitespace in header
> > 
> > Xu Yilun (5):
> >       fpga: dfl: move dfl_device_id to mod_devicetable.h
> >       dfl: add dfl bus support to MODULE_DEVICE_TABLE()
> >       fpga: dfl: fix the comments of type & feature_id fields
> >       fpga: dfl: add support for N3000 Nios private feature
> >       fpga: dfl: move dfl bus related APIs to include/linux/fpga/dfl.h
> > 
> > YueHaibing (1):
> >       fpga: dfl: n3000-nios: Make m10_n3000_info static
> > 
> >  .../ABI/testing/sysfs-bus-dfl-devices-n3000-nios   |  49 ++
> >  Documentation/fpga/dfl-n3000-nios.rst              |  73 +++
> >  Documentation/fpga/index.rst                       |   1 +
> >  MAINTAINERS                                        |   3 +-
> >  drivers/fpga/Kconfig                               |  11 +
> >  drivers/fpga/Makefile                              |   2 +
> >  drivers/fpga/dfl-n3000-nios.c                      | 573 +++++++++++++++++++++
> >  drivers/fpga/dfl.c                                 |   1 +
> >  drivers/fpga/dfl.h                                 |  85 +--
> >  include/linux/fpga/adi-axi-common.h                | 103 ++++
> >  include/linux/fpga/dfl.h                           |  86 ++++
> >  include/linux/mod_devicetable.h                    |  12 +
> >  scripts/mod/devicetable-offsets.c                  |   4 +
> >  scripts/mod/file2alias.c                           |  17 +
> 
> It's hard to comment on patches on a pull request, but this series still
> needs some work.
> 
> I'll try to list the patch names here and comments, but really, patches
> would be best to make it easier to review:
> 
> 	0001-include-fpga-adi-axi-common.h-add-definitions-for-su.patch
> 		- no users of any of these things you added, why is this
> 		  patch needed?
> 
> 	0002-fpga-dfl-move-dfl_device_id-to-mod_devicetable.h.patch
> 		- dfl_device_id is not descriptive, it means nothing to
> 		  anyone outside of the fpga subsystem.
> 		- fpga_dfl_device_id perhaps instead?  That gives people
> 		  a chance to know where to look for this
> 
> 	0003-dfl-add-dfl-bus-support-to-MODULE_DEVICE_TABLE.patch
> 		- same "prefix" issues as patch 0002, make it "fpga_dfl"
> 		  please.
> 
> 	0004-fpga-dfl-fix-the-comments-of-type-feature_id-fields.patch
> 		- You talk about 4 bits here, and then point to an
> 		  enumerated type with no actual values set for them at
> 		  all.  How is that possible???
> 
> 	0005-include-fpga-adi-axi-common-Fix-leading-whitespace-i.patch
> 		- wrong usage of "Fixes" in a signed-off-by area, please
> 		  see the submitting patches documentation for the
> 		  correct format.  My scripts, and linux-next's scripts
> 		  would have caught this, making this pull request not
> 		  be able to be accepted anyway...
> 
> 	0006-fpga-dfl-add-support-for-N3000-Nios-private-feature.patch
> 		- module parameters are for drivers written in the
> 		  1990's.  Please just "do the right thing" and make the
> 		  code work properly without having to have custom
> 		  options.  Note this option does not really work if you
> 		  have multiple devices in the system at once, which is
> 		  one reason why we don't use module parameters anymore.
> 
> 	0007-fpga-dfl-move-dfl-bus-related-APIs-to-include-linux-.patch
> 		- no one uses this header file move, so don't do it
> 		  until it is required.

This patch will be the first user:
https://lore.kernel.org/lkml/1600666280-25651-1-git-send-email-yilun.xu@intel.com/

Krzysztof has since explained that this patch should have been on a separate
branch with a tag from me, so he can pull it in.

> 
> 	0008-fpga-dfl-n3000-nios-Make-m10_n3000_info-static.patch
> 		- as patch 0006 is going to be redone, this can be
> 		  merged into that when completed.  Why the original
> 		  developer didn't run sparse on the file to start with,
> 		  I don't know :(
> 
> 
> Wow, 8 out of 8 patches rejected, I think that's a new record!  :)
> 
> thanks,
> 
> greg k-h

Everyone: I've reverted the patches. You've seen the comments, so
if one of the patches above is yours please address the comments and
resend them.

Thanks and sorry for messing this up ...

- Moritz

Re: [GIT PULL] FPGA Manager additional changes for 5.10

[Greg KH]

On Wed, Sep 23, 2020 at 03:49:08AM +0800, Xu Yilun wrote:
> On Tue, Sep 22, 2020 at 10:13:39AM +0200, Greg KH wrote:
> > 	0002-fpga-dfl-move-dfl_device_id-to-mod_devicetable.h.patch
> > 		- dfl_device_id is not descriptive, it means nothing to
> > 		  anyone outside of the fpga subsystem.
> > 		- fpga_dfl_device_id perhaps instead?  That gives people
> > 		  a chance to know where to look for this
> 
> Yes, I could change to fpga_dfl_device_id.
> And do we also need to change the naming of other dfl bus objects & APIs?
> I tend to not change them because the names would become too long. The
> clue of fpga_dfl_device_id would be enough for people to know where they
> are.

You are now "playing in the namespace of the whole kernel", so yes, you
need to be as descriptive as possible so that everyone knows what is
happening as all subsystems touch these files.

No one knows what "dfl_" means.  Heck, I have no idea, I keep thinking
"Device Firmware Loader" or something like that.

So making it obvious, and unique, and "short enough" is the requirement.

Naming is hard :)

And there is no problem with having "long" names in your drivers, we
have no max numbers of characters that the compiler can accept, this
isn't the 1960's anymore...

> > 	0003-dfl-add-dfl-bus-support-to-MODULE_DEVICE_TABLE.patch
> > 		- same "prefix" issues as patch 0002, make it "fpga_dfl"
> > 		  please.
> > 
> > 	0004-fpga-dfl-fix-the-comments-of-type-feature_id-fields.patch
> > 		- You talk about 4 bits here, and then point to an
> > 		  enumerated type with no actual values set for them at
> > 		  all.  How is that possible???
> 
> I'm sorry I didn't fully understand the issue. Actually the
> enum dfl_id_type is introduced in previous patchset, it contains the
> implied values FME_ID == 0, PORT_ID == 1.

You say "implied", that's the problem.

If you really are going to guarantee that these values are only 4 bits
big, then explicitly set them to that value so we all know what is going
on.

But the bigger question is "why"?  Who cares it's only 4 bits, we have
room in the kernel to have it be a full 8 bits or 32 or whatever, right?

If this is a value shared by hardware or a spec, then you HAVE TO
explicitly set the values of an enumerated type.  Otherwise the compiler
is in its right to do whatever it wants with them (well, within
reason...)

> > 	0005-include-fpga-adi-axi-common-Fix-leading-whitespace-i.patch
> > 		- wrong usage of "Fixes" in a signed-off-by area, please
> > 		  see the submitting patches documentation for the
> > 		  correct format.  My scripts, and linux-next's scripts
> > 		  would have caught this, making this pull request not
> > 		  be able to be accepted anyway...
> > 
> > 	0006-fpga-dfl-add-support-for-N3000-Nios-private-feature.patch
> > 		- module parameters are for drivers written in the
> > 		  1990's.  Please just "do the right thing" and make the
> > 		  code work properly without having to have custom
> > 		  options.  Note this option does not really work if you
> > 		  have multiple devices in the system at once, which is
> > 		  one reason why we don't use module parameters anymore.
> 
> I'm wondering which is the better way to handle the case, but let me
> first describe it.
> 
> The n3000-nios driver will first require the embedded firmware to
> finish some netdev configuration (via the SPI controller) according to
> user's option, then the driver takes over the control of the SPI
> controller and continue to enumerate various devices behind the SPI bus.
> The firmware will never be active again after the first configuration on
> power cycle. So I tend to let the driver finish the configuration
> requirement at probe, then the following drivers for SPI devices could be
> autoloaded. This is why I choose the module parameter.

Remember, module parameters affect ALL devices the driver controls, you
just broke the ability for this driver to handle multiple devices per a
single driver, with no way of ever fixing that (module parameters are
userspace apis...)

> I was considering the sysfs interface option to trigger this one-time
> configuration. But this makes the SPI devices unavailable until user
> inputs something to this sysfs node. We may need to add some specific
> udev rules to make the whole functionalities of the FPGA card enabled
> automatically.

Why can't you figure out the state of the device without having to worry
about a module option?  No one will know how to set that thing, and if
they do, it will get baked into a bootloader somewhere and no one can
ever turn it off.

Be dynamic and make things "just work" so that the user does not have to
configure anything at all please.

> And our customers don't expect different configurations between
> multiple cards on one system, which also influences my decision.

They might not say that today, but you just forced them to never be able
to do that ever, in the next 20+ years :(

thanks,

greg k-h

Re: [GIT PULL] FPGA Manager additional changes for 5.10

[Xu Yilun]

> > > 	0006-fpga-dfl-add-support-for-N3000-Nios-private-feature.patch
> > > 		- module parameters are for drivers written in the
> > > 		  1990's.  Please just "do the right thing" and make the
> > > 		  code work properly without having to have custom
> > > 		  options.  Note this option does not really work if you
> > > 		  have multiple devices in the system at once, which is
> > > 		  one reason why we don't use module parameters anymore.
> > 
> > I'm wondering which is the better way to handle the case, but let me
> > first describe it.
> > 
> > The n3000-nios driver will first require the embedded firmware to
> > finish some netdev configuration (via the SPI controller) according to
> > user's option, then the driver takes over the control of the SPI
> > controller and continue to enumerate various devices behind the SPI bus.
> > The firmware will never be active again after the first configuration on
> > power cycle. So I tend to let the driver finish the configuration
> > requirement at probe, then the following drivers for SPI devices could be
> > autoloaded. This is why I choose the module parameter.
> 
> Remember, module parameters affect ALL devices the driver controls, you
> just broke the ability for this driver to handle multiple devices per a
> single driver, with no way of ever fixing that (module parameters are
> userspace apis...)
> 
> > I was considering the sysfs interface option to trigger this one-time
> > configuration. But this makes the SPI devices unavailable until user
> > inputs something to this sysfs node. We may need to add some specific
> > udev rules to make the whole functionalities of the FPGA card enabled
> > automatically.
> 
> Why can't you figure out the state of the device without having to worry
> about a module option?  No one will know how to set that thing, and if
> they do, it will get baked into a bootloader somewhere and no one can
> ever turn it off.

It's the discrete hot-plugable PCIe card which is not managed in host
bootloader. And because of the HW design, the onboard firmware doesn't
get a writable non-volatile memory for the options, so it needs the input
from user.

Anyway, I need to reconsider this solution. But I got learned that "don't
make the user have to configure something out of their scope".

Thanks very much,
Yilun

> 
> Be dynamic and make things "just work" so that the user does not have to
> configure anything at all please.
> 
> > And our customers don't expect different configurations between
> > multiple cards on one system, which also influences my decision.
> 
> They might not say that today, but you just forced them to never be able
> to do that ever, in the next 20+ years :(
> 
> thanks,
> 
> greg k-h