mbox series

[v1,0/6] PolarFire SoC Auto Update Support

Message ID 20230217164023.14255-1-conor@kernel.org (mailing list archive)
Headers show
Series PolarFire SoC Auto Update Support | expand

Message

Conor Dooley Feb. 17, 2023, 4:40 p.m. UTC
From: Conor Dooley <conor.dooley@microchip.com>

Hey all,

This patchset adds support for the "Auto Update" feature on PolarFire
SoC that allows for writing an FPGA bistream to the SPI flash connected
to the system controller.
On powercycle (or reboot depending on how the firmware implements the
openSBI SRST extension) "Auto Update" will take place, and program the
FPGA with the contents of the SPI flash - provided that that image is
valid and an actual upgrade from that already programmed!

Unfortunately, this series is not really testable yet - the Engineering
Sample silicon on most dev boards has a bug in the QSPI controller
connected to the system controller's flash and cannot access it.
Pre-production and later silicon has this bug fixed.

I previously posted an RFC about my approach in this driver, since as a
flash-based FPGA we are somewhat different to the existing
self-reprogramming drivers here. That RFC is here:
https://lore.kernel.org/linux-fpga/20221121225748.124900-1-conor@kernel.org/

This series depends on the following fixes:
https://patchwork.kernel.org/project/linux-riscv/list/?series=714160

The patch adding the driver depends on the soc patches earlier in the
series, so taking both through the same tree makes sense. Depending on
sequencing with the dependencies, me taking it through the soc tree
(with Acks etc of course) may make the most sense.

The other caveat here I guess is that this uses debugfs to trigger the
write, as we do not yet have a userspace for this yet!

Cheers,
Conor.

CC: Conor Dooley <conor.dooley@microchip.com>
CC: Daire McNamara <daire.mcnamara@microchip.com>
CC: Rob Herring <robh+dt@kernel.org>
CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
CC: Moritz Fischer <mdf@kernel.org>
CC: Wu Hao <hao.wu@intel.com>
CC: Xu Yilun <yilun.xu@intel.com>
CC: Tom Rix <trix@redhat.com>
CC: linux-riscv@lists.infradead.org
CC: devicetree@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-fpga@vger.kernel.org

Conor Dooley (6):
  soc: microchip: mpfs: add a prefix to rx_callback()
  dt-bindings: soc: microchip: add a property for system controller
    flash
  soc: microchip: mpfs: enable access to the system controller's flash
  soc: microchip: mpfs: add auto-update subdev to system controller
  fpga: add PolarFire SoC Auto Update support
  riscv: dts: microchip: add the mpfs' system controller qspi &
    associated flash

 .../microchip,mpfs-sys-controller.yaml        |  10 +
 .../boot/dts/microchip/mpfs-icicle-kit.dts    |  21 +
 arch/riscv/boot/dts/microchip/mpfs.dtsi       |  24 +-
 drivers/fpga/Kconfig                          |   9 +
 drivers/fpga/Makefile                         |   1 +
 drivers/fpga/microchip-auto-update.c          | 495 ++++++++++++++++++
 drivers/soc/microchip/Kconfig                 |   1 +
 drivers/soc/microchip/mpfs-sys-controller.c   |  33 +-
 include/soc/microchip/mpfs.h                  |   2 +
 9 files changed, 586 insertions(+), 10 deletions(-)
 create mode 100644 drivers/fpga/microchip-auto-update.c

Comments

Xu Yilun Feb. 24, 2023, 7:57 a.m. UTC | #1
On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> Hey all,
> 
> This patchset adds support for the "Auto Update" feature on PolarFire
> SoC that allows for writing an FPGA bistream to the SPI flash connected
> to the system controller.

I haven't fully checked the patches yet, just some quick comments:

Since this feature is just to R/W the flash, and would not affect the
runtime FPGA region, I don't think an FPGA manager is actually needed.
Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
MTD tool if I remember correctly.

Thanks,
Yilun

> On powercycle (or reboot depending on how the firmware implements the
> openSBI SRST extension) "Auto Update" will take place, and program the
> FPGA with the contents of the SPI flash - provided that that image is
> valid and an actual upgrade from that already programmed!
> 
> Unfortunately, this series is not really testable yet - the Engineering
> Sample silicon on most dev boards has a bug in the QSPI controller
> connected to the system controller's flash and cannot access it.
> Pre-production and later silicon has this bug fixed.
> 
> I previously posted an RFC about my approach in this driver, since as a
> flash-based FPGA we are somewhat different to the existing
> self-reprogramming drivers here. That RFC is here:
> https://lore.kernel.org/linux-fpga/20221121225748.124900-1-conor@kernel.org/
> 
> This series depends on the following fixes:
> https://patchwork.kernel.org/project/linux-riscv/list/?series=714160
> 
> The patch adding the driver depends on the soc patches earlier in the
> series, so taking both through the same tree makes sense. Depending on
> sequencing with the dependencies, me taking it through the soc tree
> (with Acks etc of course) may make the most sense.
> 
> The other caveat here I guess is that this uses debugfs to trigger the
> write, as we do not yet have a userspace for this yet!
> 
> Cheers,
> Conor.
> 
> CC: Conor Dooley <conor.dooley@microchip.com>
> CC: Daire McNamara <daire.mcnamara@microchip.com>
> CC: Rob Herring <robh+dt@kernel.org>
> CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> CC: Moritz Fischer <mdf@kernel.org>
> CC: Wu Hao <hao.wu@intel.com>
> CC: Xu Yilun <yilun.xu@intel.com>
> CC: Tom Rix <trix@redhat.com>
> CC: linux-riscv@lists.infradead.org
> CC: devicetree@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> CC: linux-fpga@vger.kernel.org
> 
> Conor Dooley (6):
>   soc: microchip: mpfs: add a prefix to rx_callback()
>   dt-bindings: soc: microchip: add a property for system controller
>     flash
>   soc: microchip: mpfs: enable access to the system controller's flash
>   soc: microchip: mpfs: add auto-update subdev to system controller
>   fpga: add PolarFire SoC Auto Update support
>   riscv: dts: microchip: add the mpfs' system controller qspi &
>     associated flash
> 
>  .../microchip,mpfs-sys-controller.yaml        |  10 +
>  .../boot/dts/microchip/mpfs-icicle-kit.dts    |  21 +
>  arch/riscv/boot/dts/microchip/mpfs.dtsi       |  24 +-
>  drivers/fpga/Kconfig                          |   9 +
>  drivers/fpga/Makefile                         |   1 +
>  drivers/fpga/microchip-auto-update.c          | 495 ++++++++++++++++++
>  drivers/soc/microchip/Kconfig                 |   1 +
>  drivers/soc/microchip/mpfs-sys-controller.c   |  33 +-
>  include/soc/microchip/mpfs.h                  |   2 +
>  9 files changed, 586 insertions(+), 10 deletions(-)
>  create mode 100644 drivers/fpga/microchip-auto-update.c
> 
> -- 
> 2.39.1
>
Conor Dooley Feb. 24, 2023, 8:28 a.m. UTC | #2
On Fri, Feb 24, 2023 at 03:57:09PM +0800, Xu Yilun wrote:
> On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:

> > This patchset adds support for the "Auto Update" feature on PolarFire
> > SoC that allows for writing an FPGA bistream to the SPI flash connected
> > to the system controller.
> 
> I haven't fully checked the patches yet, just some quick comments:
> 
> Since this feature is just to R/W the flash, and would not affect the
> runtime FPGA region, I don't think an FPGA manager is actually needed.
> Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
> MTD tool if I remember correctly.

A lack of interest in opening up the system controller to userspace!
You're right in that the writing of the image can be done that way, and
while I was testing I used the userspace bits of mtd along the way - but
for validating that the image we are writing we rely on the system
controller.
I'm really not interested in exposing the system controller's
functionality, especially the bitstream manipulation parts, to userspace
due to the risk of input validation bugs, so at least that side of
things should remain in the kernel.
I suppose I could implement something custom in drivers/soc that does
the validation only, and push the rest out to userspace. Just seemed
fitting to do the whole lot in drivers/fpga.

Cheers,
Conor.
Russ Weight Feb. 27, 2023, 10:04 p.m. UTC | #3
Hi Conor,

On 2/24/23 00:28, Conor Dooley wrote:
> On Fri, Feb 24, 2023 at 03:57:09PM +0800, Xu Yilun wrote:
>> On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:
>>> This patchset adds support for the "Auto Update" feature on PolarFire
>>> SoC that allows for writing an FPGA bistream to the SPI flash connected
>>> to the system controller.
>> I haven't fully checked the patches yet, just some quick comments:
>>
>> Since this feature is just to R/W the flash, and would not affect the
>> runtime FPGA region, I don't think an FPGA manager is actually needed.
>> Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
>> MTD tool if I remember correctly.
> A lack of interest in opening up the system controller to userspace!
> You're right in that the writing of the image can be done that way, and
> while I was testing I used the userspace bits of mtd along the way - but
> for validating that the image we are writing we rely on the system
> controller.
> I'm really not interested in exposing the system controller's
> functionality, especially the bitstream manipulation parts, to userspace
> due to the risk of input validation bugs, so at least that side of
> things should remain in the kernel.
> I suppose I could implement something custom in drivers/soc that does
> the validation only, and push the rest out to userspace. Just seemed
> fitting to do the whole lot in drivers/fpga.
Conor,

In case you haven't already looked at the new firmware-upload
support in the firmware-loader, I'll give you some references
here to see if it fit you needs. This would only support the
write (not the read) but it would allow the controller to do
validation on the write.

The is the cover letter which shows a usage example:
https://lore.kernel.org/lkml/20220421212204.36052-1-russell.h.weight@intel.com/

And this is a pointer to the latest documentation for it:
https://docs.kernel.org/driver-api/firmware/fw_upload.html

The only current user is: drivers/fpga/intel-m10-bmc-sec-update.c

- Russ

>
> Cheers,
> Conor.
>
Conor Dooley Feb. 27, 2023, 10:16 p.m. UTC | #4
Hey Russ,

On Mon, Feb 27, 2023 at 02:04:36PM -0800, Russ Weight wrote:
> On 2/24/23 00:28, Conor Dooley wrote:
> > On Fri, Feb 24, 2023 at 03:57:09PM +0800, Xu Yilun wrote:
> >> On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:
> >>> This patchset adds support for the "Auto Update" feature on PolarFire
> >>> SoC that allows for writing an FPGA bistream to the SPI flash connected
> >>> to the system controller.
> >> I haven't fully checked the patches yet, just some quick comments:
> >>
> >> Since this feature is just to R/W the flash, and would not affect the
> >> runtime FPGA region, I don't think an FPGA manager is actually needed.
> >> Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
> >> MTD tool if I remember correctly.
> > A lack of interest in opening up the system controller to userspace!
> > You're right in that the writing of the image can be done that way, and
> > while I was testing I used the userspace bits of mtd along the way - but
> > for validating that the image we are writing we rely on the system
> > controller.
> > I'm really not interested in exposing the system controller's
> > functionality, especially the bitstream manipulation parts, to userspace
> > due to the risk of input validation bugs, so at least that side of
> > things should remain in the kernel.
> > I suppose I could implement something custom in drivers/soc that does
> > the validation only, and push the rest out to userspace. Just seemed
> > fitting to do the whole lot in drivers/fpga.

> In case you haven't already looked at the new firmware-upload
> support in the firmware-loader, I'll give you some references
> here to see if it fit you needs. This would only support the
> write (not the read) but it would allow the controller to do
> validation on the write.
> 
> The is the cover letter which shows a usage example:
> https://lore.kernel.org/lkml/20220421212204.36052-1-russell.h.weight@intel.com/
> 
> And this is a pointer to the latest documentation for it:
> https://docs.kernel.org/driver-api/firmware/fw_upload.html
> 
> The only current user is: drivers/fpga/intel-m10-bmc-sec-update.c

Sounds interesting, I shall go and take a look. Just quickly, when you
say it wouldn't support the read, what exactly do you mean?
The only read that I am really interested in doing is reading the first
1K of flash as I need to RMW it, but I don't think that that's what you
mean.
Do you mean that I would not be able to dump the firmware using your
fw_upload interface? If so, that's an acceptable constraint IMO.

Your interface also circumvents us (Microchip) defining yet another
method for interacting with the FPGA, since from my nosing around,
everyone seems to do something different.

Cheers,
Conor.
Russ Weight Feb. 27, 2023, 10:42 p.m. UTC | #5
On 2/27/23 14:16, Conor Dooley wrote:
> Hey Russ,
>
> On Mon, Feb 27, 2023 at 02:04:36PM -0800, Russ Weight wrote:
>> On 2/24/23 00:28, Conor Dooley wrote:
>>> On Fri, Feb 24, 2023 at 03:57:09PM +0800, Xu Yilun wrote:
>>>> On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:
>>>>> This patchset adds support for the "Auto Update" feature on PolarFire
>>>>> SoC that allows for writing an FPGA bistream to the SPI flash connected
>>>>> to the system controller.
>>>> I haven't fully checked the patches yet, just some quick comments:
>>>>
>>>> Since this feature is just to R/W the flash, and would not affect the
>>>> runtime FPGA region, I don't think an FPGA manager is actually needed.
>>>> Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
>>>> MTD tool if I remember correctly.
>>> A lack of interest in opening up the system controller to userspace!
>>> You're right in that the writing of the image can be done that way, and
>>> while I was testing I used the userspace bits of mtd along the way - but
>>> for validating that the image we are writing we rely on the system
>>> controller.
>>> I'm really not interested in exposing the system controller's
>>> functionality, especially the bitstream manipulation parts, to userspace
>>> due to the risk of input validation bugs, so at least that side of
>>> things should remain in the kernel.
>>> I suppose I could implement something custom in drivers/soc that does
>>> the validation only, and push the rest out to userspace. Just seemed
>>> fitting to do the whole lot in drivers/fpga.
>> In case you haven't already looked at the new firmware-upload
>> support in the firmware-loader, I'll give you some references
>> here to see if it fit you needs. This would only support the
>> write (not the read) but it would allow the controller to do
>> validation on the write.
>>
>> The is the cover letter which shows a usage example:
>> https://lore.kernel.org/lkml/20220421212204.36052-1-russell.h.weight@intel.com/
>>
>> And this is a pointer to the latest documentation for it:
>> https://docs.kernel.org/driver-api/firmware/fw_upload.html
>>
>> The only current user is: drivers/fpga/intel-m10-bmc-sec-update.c
> Sounds interesting, I shall go and take a look. Just quickly, when you
> say it wouldn't support the read, what exactly do you mean?
> The only read that I am really interested in doing is reading the first
> 1K of flash as I need to RMW it, but I don't think that that's what you
> mean.
> Do you mean that I would not be able to dump the firmware using your
> fw_upload interface? If so, that's an acceptable constraint IMO.

Yes - I mean that you couldn't dump the firmware image from userspace
using the fw_upload interface. The sysfs interface allows you to read
and write a temporary buffer, but once you "echo 0 > loading", there
is no sysfs interface associated with the firmware-loader that would
allow you to read the image from flash. Your controller actually does
the writes, so RMW in that context is fine.

- Russ
>
> Your interface also circumvents us (Microchip) defining yet another
> method for interacting with the FPGA, since from my nosing around,
> everyone seems to do something different.
>
> Cheers,
> Conor.
>
Conor Dooley Feb. 27, 2023, 10:56 p.m. UTC | #6
On Mon, Feb 27, 2023 at 02:42:30PM -0800, Russ Weight wrote:
> 
> 
> On 2/27/23 14:16, Conor Dooley wrote:
> > Hey Russ,
> >
> > On Mon, Feb 27, 2023 at 02:04:36PM -0800, Russ Weight wrote:
> >> On 2/24/23 00:28, Conor Dooley wrote:
> >>> On Fri, Feb 24, 2023 at 03:57:09PM +0800, Xu Yilun wrote:
> >>>> On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:
> >>>>> This patchset adds support for the "Auto Update" feature on PolarFire
> >>>>> SoC that allows for writing an FPGA bistream to the SPI flash connected
> >>>>> to the system controller.
> >>>> I haven't fully checked the patches yet, just some quick comments:
> >>>>
> >>>> Since this feature is just to R/W the flash, and would not affect the
> >>>> runtime FPGA region, I don't think an FPGA manager is actually needed.
> >>>> Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
> >>>> MTD tool if I remember correctly.
> >>> A lack of interest in opening up the system controller to userspace!
> >>> You're right in that the writing of the image can be done that way, and
> >>> while I was testing I used the userspace bits of mtd along the way - but
> >>> for validating that the image we are writing we rely on the system
> >>> controller.
> >>> I'm really not interested in exposing the system controller's
> >>> functionality, especially the bitstream manipulation parts, to userspace
> >>> due to the risk of input validation bugs, so at least that side of
> >>> things should remain in the kernel.
> >>> I suppose I could implement something custom in drivers/soc that does
> >>> the validation only, and push the rest out to userspace. Just seemed
> >>> fitting to do the whole lot in drivers/fpga.
> >> In case you haven't already looked at the new firmware-upload
> >> support in the firmware-loader, I'll give you some references
> >> here to see if it fit you needs. This would only support the
> >> write (not the read) but it would allow the controller to do
> >> validation on the write.
> >>
> >> The is the cover letter which shows a usage example:
> >> https://lore.kernel.org/lkml/20220421212204.36052-1-russell.h.weight@intel.com/
> >>
> >> And this is a pointer to the latest documentation for it:
> >> https://docs.kernel.org/driver-api/firmware/fw_upload.html
> >>
> >> The only current user is: drivers/fpga/intel-m10-bmc-sec-update.c
> > Sounds interesting, I shall go and take a look. Just quickly, when you
> > say it wouldn't support the read, what exactly do you mean?
> > The only read that I am really interested in doing is reading the first
> > 1K of flash as I need to RMW it, but I don't think that that's what you
> > mean.
> > Do you mean that I would not be able to dump the firmware using your
> > fw_upload interface? If so, that's an acceptable constraint IMO.
> 
> Yes - I mean that you couldn't dump the firmware image from userspace
> using the fw_upload interface. The sysfs interface allows you to read
> and write a temporary buffer, but once you "echo 0 > loading", there
> is no sysfs interface associated with the firmware-loader that would
> allow you to read the image from flash. Your controller actually does
> the writes, so RMW in that context is fine.

Ahh cool. I don't really care about dumping the firmware via such a
mechanism, so that sounds good to me.
I'll check out your approach, the immediate thought is that it sounds
suitable to my use case, so thanks!
Conor Dooley March 22, 2023, 6:51 p.m. UTC | #7
Hey Russ,

On Mon, Feb 27, 2023 at 10:56:07PM +0000, Conor Dooley wrote:
> On Mon, Feb 27, 2023 at 02:42:30PM -0800, Russ Weight wrote:
> > On 2/27/23 14:16, Conor Dooley wrote:
> > > On Mon, Feb 27, 2023 at 02:04:36PM -0800, Russ Weight wrote:
> > >> On 2/24/23 00:28, Conor Dooley wrote:
> > >>> On Fri, Feb 24, 2023 at 03:57:09PM +0800, Xu Yilun wrote:
> > >>>> On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:
> > >>>>> This patchset adds support for the "Auto Update" feature on PolarFire
> > >>>>> SoC that allows for writing an FPGA bistream to the SPI flash connected
> > >>>>> to the system controller.
> > >>>> I haven't fully checked the patches yet, just some quick comments:
> > >>>>
> > >>>> Since this feature is just to R/W the flash, and would not affect the
> > >>>> runtime FPGA region, I don't think an FPGA manager is actually needed.
> > >>>> Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
> > >>>> MTD tool if I remember correctly.
> > >>> A lack of interest in opening up the system controller to userspace!
> > >>> You're right in that the writing of the image can be done that way, and
> > >>> while I was testing I used the userspace bits of mtd along the way - but
> > >>> for validating that the image we are writing we rely on the system
> > >>> controller.
> > >>> I'm really not interested in exposing the system controller's
> > >>> functionality, especially the bitstream manipulation parts, to userspace
> > >>> due to the risk of input validation bugs, so at least that side of
> > >>> things should remain in the kernel.
> > >>> I suppose I could implement something custom in drivers/soc that does
> > >>> the validation only, and push the rest out to userspace. Just seemed
> > >>> fitting to do the whole lot in drivers/fpga.
> > >> In case you haven't already looked at the new firmware-upload
> > >> support in the firmware-loader, I'll give you some references
> > >> here to see if it fit you needs. This would only support the
> > >> write (not the read) but it would allow the controller to do
> > >> validation on the write.
> > >>
> > >> The is the cover letter which shows a usage example:
> > >> https://lore.kernel.org/lkml/20220421212204.36052-1-russell.h.weight@intel.com/
> > >>
> > >> And this is a pointer to the latest documentation for it:
> > >> https://docs.kernel.org/driver-api/firmware/fw_upload.html
> > >>
> > >> The only current user is: drivers/fpga/intel-m10-bmc-sec-update.c
> > > Sounds interesting, I shall go and take a look. Just quickly, when you
> > > say it wouldn't support the read, what exactly do you mean?
> > > The only read that I am really interested in doing is reading the first
> > > 1K of flash as I need to RMW it, but I don't think that that's what you
> > > mean.
> > > Do you mean that I would not be able to dump the firmware using your
> > > fw_upload interface? If so, that's an acceptable constraint IMO.
> > 
> > Yes - I mean that you couldn't dump the firmware image from userspace
> > using the fw_upload interface. The sysfs interface allows you to read
> > and write a temporary buffer, but once you "echo 0 > loading", there
> > is no sysfs interface associated with the firmware-loader that would
> > allow you to read the image from flash. Your controller actually does
> > the writes, so RMW in that context is fine.
> 
> Ahh cool. I don't really care about dumping the firmware via such a
> mechanism, so that sounds good to me.
> I'll check out your approach, the immediate thought is that it sounds
> suitable to my use case, so thanks!

Taken me a while to get around to it, but thanks for your suggestion.
Looks like it is suitable for what I am trying to do, so in the middle
of working on another version of this using fw_upload.
The enum return codes from write are a little clumsy, but oh well, could
be worse I suppose.

Just one thing I noted (although I rarely pay much attention to/rely on
the driver-api docs when recent drivers exist as usable examples) is
that the docs for this stuff is a wee bit out of date due to some API
changes.
In the code example in this document:
https://docs.kernel.org/driver-api/firmware/fw_upload.html
firmware_upload_register() has fewer arguments than it does when you
look at the signature of the function in the documentation right below
it.

Cheers,
Conor.
Russ Weight March 30, 2023, 12:11 a.m. UTC | #8
Hi Conor,

Sorry for the slow reply - I was out of the office for a week...

On 3/22/23 11:51, Conor Dooley wrote:
> Hey Russ,
>
> On Mon, Feb 27, 2023 at 10:56:07PM +0000, Conor Dooley wrote:
>> On Mon, Feb 27, 2023 at 02:42:30PM -0800, Russ Weight wrote:
>>> On 2/27/23 14:16, Conor Dooley wrote:
>>>> On Mon, Feb 27, 2023 at 02:04:36PM -0800, Russ Weight wrote:
>>>>> On 2/24/23 00:28, Conor Dooley wrote:
>>>>>> On Fri, Feb 24, 2023 at 03:57:09PM +0800, Xu Yilun wrote:
>>>>>>> On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:
>>>>>>>> This patchset adds support for the "Auto Update" feature on PolarFire
>>>>>>>> SoC that allows for writing an FPGA bistream to the SPI flash connected
>>>>>>>> to the system controller.
>>>>>>> I haven't fully checked the patches yet, just some quick comments:
>>>>>>>
>>>>>>> Since this feature is just to R/W the flash, and would not affect the
>>>>>>> runtime FPGA region, I don't think an FPGA manager is actually needed.
>>>>>>> Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
>>>>>>> MTD tool if I remember correctly.
>>>>>> A lack of interest in opening up the system controller to userspace!
>>>>>> You're right in that the writing of the image can be done that way, and
>>>>>> while I was testing I used the userspace bits of mtd along the way - but
>>>>>> for validating that the image we are writing we rely on the system
>>>>>> controller.
>>>>>> I'm really not interested in exposing the system controller's
>>>>>> functionality, especially the bitstream manipulation parts, to userspace
>>>>>> due to the risk of input validation bugs, so at least that side of
>>>>>> things should remain in the kernel.
>>>>>> I suppose I could implement something custom in drivers/soc that does
>>>>>> the validation only, and push the rest out to userspace. Just seemed
>>>>>> fitting to do the whole lot in drivers/fpga.
>>>>> In case you haven't already looked at the new firmware-upload
>>>>> support in the firmware-loader, I'll give you some references
>>>>> here to see if it fit you needs. This would only support the
>>>>> write (not the read) but it would allow the controller to do
>>>>> validation on the write.
>>>>>
>>>>> The is the cover letter which shows a usage example:
>>>>> https://lore.kernel.org/lkml/20220421212204.36052-1-russell.h.weight@intel.com/
>>>>>
>>>>> And this is a pointer to the latest documentation for it:
>>>>> https://docs.kernel.org/driver-api/firmware/fw_upload.html
>>>>>
>>>>> The only current user is: drivers/fpga/intel-m10-bmc-sec-update.c
>>>> Sounds interesting, I shall go and take a look. Just quickly, when you
>>>> say it wouldn't support the read, what exactly do you mean?
>>>> The only read that I am really interested in doing is reading the first
>>>> 1K of flash as I need to RMW it, but I don't think that that's what you
>>>> mean.
>>>> Do you mean that I would not be able to dump the firmware using your
>>>> fw_upload interface? If so, that's an acceptable constraint IMO.
>>> Yes - I mean that you couldn't dump the firmware image from userspace
>>> using the fw_upload interface. The sysfs interface allows you to read
>>> and write a temporary buffer, but once you "echo 0 > loading", there
>>> is no sysfs interface associated with the firmware-loader that would
>>> allow you to read the image from flash. Your controller actually does
>>> the writes, so RMW in that context is fine.
>> Ahh cool. I don't really care about dumping the firmware via such a
>> mechanism, so that sounds good to me.
>> I'll check out your approach, the immediate thought is that it sounds
>> suitable to my use case, so thanks!
> Taken me a while to get around to it, but thanks for your suggestion.
> Looks like it is suitable for what I am trying to do, so in the middle
> of working on another version of this using fw_upload.
> The enum return codes from write are a little clumsy, but oh well, could
> be worse I suppose.
>
> Just one thing I noted (although I rarely pay much attention to/rely on
> the driver-api docs when recent drivers exist as usable examples) is
> that the docs for this stuff is a wee bit out of date due to some API
> changes.
> In the code example in this document:
> https://docs.kernel.org/driver-api/firmware/fw_upload.html
> firmware_upload_register() has fewer arguments than it does when you
> look at the signature of the function in the documentation right below
> it.

I saw your patch to fix this. Thanks!

- Russ
>
> Cheers,
> Conor.