Re: [PATCH v2 00/11] intel-m10-bmc: Split BMC to core and SPI parts & add PMCI+N6000 support

From: Xu Yilun <yilun.xu@intel.com>
To: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: linux-fpga@vger.kernel.org, Wu Hao <hao.wu@intel.com>,
	Tom Rix <trix@redhat.com>, Moritz Fischer <mdf@kernel.org>,
	Lee Jones <lee@kernel.org>,
	Matthew Gerlach <matthew.gerlach@linux.intel.com>,
	Russ Weight <russell.h.weight@intel.com>,
	Tianfei zhang <tianfei.zhang@intel.com>,
	Mark Brown <broonie@kernel.org>,
	Greg KH <gregkh@linuxfoundation.org>,
	Marco Pagani <marpagan@redhat.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 00/11] intel-m10-bmc: Split BMC to core and SPI parts & add PMCI+N6000 support
Date: Tue, 22 Nov 2022 10:43:32 +0800	[thread overview]
Message-ID: <Y3w3VLvjth4peSPd@yilunxu-OptiPlex-7050> (raw)
In-Reply-To: <20221117120515.37807-1-ilpo.jarvinen@linux.intel.com>

On 2022-11-17 at 14:05:04 +0200, Ilpo Järvinen wrote:
> Hi all,
> 
> Here are the patches for MAX 10 BMC core/SPI interface split and
> addition of the PMCI interface. There are a few supporting rearrangement
> patches prior to the actual split. This series also introduced regmap for
> indirect register access generic to Intel FPGA IPs.
> 
> The current downside of the split is that there's not that much code
> remaining in the core part when all the type specific definitions are
> moved to the file with the relevant interface.
> 
> I'm not entirely sure if I did properly address Yilun's comment on
> placement of the flash_ops related code. To me the original way which
> keeps them in mfd/intel-m10-bmc-{spi,pmci}.c still seems to the best
> way. If that is still not acceptable, I propose that I'll move just the
> flash ops functions into intel-m10-bmc-sec-update-{spi,pmci}.c and

I don't think the split of intel-m10-bmc-sec-update-{spi, pmci}.c is
needed. The mfd subdev is a platform device actually, it doesn't have to
know the bus type.

> create sec related platform info struct to select the correct flash
> ops. This would effectively be the "double split" approach I suggested
> earlier (both mfd and sec have their own per interface splits, to me
> the double split looks overkill but it would meet Yilun's goal of
> keeping sec related code within the sec driver).

Yes, this is still my preference.

My idea is, the secure update driver could be told by mfd core whether
there is a bypass channel for flash bulk read/write. If yes, use it. If
no, just direct access to flash staging areas as it previously does.

This is like:

struct intel_m10bmc {
        struct device *dev;
        struct regmap *regmap;
	...
+       const struct intel_m10bmc_flash_bulk_ops *flash_bulk_ops;
}

In intel-m10-bmc-pmci.c,

  +static const struct intel_m10bmc_flash_bulk_ops m10bmc_pmci_flash_bulk_ops = {
  +       .read = m10bmc_pmci_flash_bulk_read,
  +       .write = m10bmc_pmci_flash_bulk_write,
  };

In intel-m10-bmc-spi.c, no need to have flash_bulk_ops. 

In intel-m10-bmc-sec-update.c,

  Check if flash_bulk_ops is available, if yes,

    set_flash_host_mux(aquire)   /* I assume flash mux is dedicated for sec-update dev */
    sec->m10bmc->flash_bulk_ops->write()
    set_flash_host_mux(release)

  if no:
    follow previous direct access.

Thanks,
Yilun

> 
> The patch set is based on top of commit dfd10332596e ("fpga:
> m10bmc-sec: Fix kconfig dependencies") to avoid triggering a Kconfig
> conflict.
> 
> v2:
> - Drop type from mfd side, the platform info takes care of differentation
> - Explain introducing ->info to struct m10bmc in commit message (belongs logically there)
> - Intro PMCI better
> - Improve naming
>         - Rename M10BMC_PMCI_FLASH_CTRL to M10BMC_PMCI_FLASH_MUX_CTRL
>         - Use m10bmc_pmci/M10BMC_PMCI prefix consistently
>         - Use M10BMC_SPI prefix for SPI related defines
>         - Newly added stuff gets m10bmc_spi prefix
> - Removed dev from struct m10bmc_pmci_device
> - Rename "n_offset" variable to "offset" in PMCI flash ops
> - Always issue idle command in regmap indirect to clear RD/WR/ACK bits
> - Handle stride misaligned sizes in flash read/write ops
> 
> Ilpo Järvinen (11):
>   mfd: intel-m10-bmc: Create m10bmc_platform_info for type specific info
>   mfd: intel-m10-bmc: Rename the local variables
>   mfd: intel-m10-bmc: Split into core and spi specific parts
>   mfd: intel-m10-bmc: Support multiple CSR register layouts
>   fpga: intel-m10-bmc: Add flash ops for sec update
>   mfd: intel-m10-bmc: Downscope SPI defines & prefix with M10BMC_SPI
>   regmap: indirect: Add indirect regmap support
>   intel-m10-bmc: Add regmap_indirect_cfg for Intel FPGA IPs
>   mfd: intel-m10-bmc: Add PMCI driver
>   fpga: m10bmc-sec: Add support for N6000
>   mfd: intel-m10-bmc: Change MODULE_LICENSE() to GPL
> 
>  .../ABI/testing/sysfs-driver-intel-m10-bmc    |   8 +-
>  MAINTAINERS                                   |   2 +-
>  drivers/base/regmap/Kconfig                   |   3 +
>  drivers/base/regmap/Makefile                  |   1 +
>  drivers/base/regmap/regmap-indirect.c         | 128 +++++++
>  drivers/fpga/Kconfig                          |   2 +-
>  drivers/fpga/intel-m10-bmc-sec-update.c       | 149 ++++----
>  drivers/hwmon/Kconfig                         |   2 +-
>  drivers/mfd/Kconfig                           |  34 +-
>  drivers/mfd/Makefile                          |   6 +-
>  drivers/mfd/intel-m10-bmc-core.c              | 133 +++++++
>  drivers/mfd/intel-m10-bmc-pmci.c              | 361 ++++++++++++++++++
>  drivers/mfd/intel-m10-bmc-spi.c               | 270 +++++++++++++
>  drivers/mfd/intel-m10-bmc.c                   | 238 ------------
>  include/linux/mfd/intel-m10-bmc.h             | 122 +++---
>  include/linux/regmap.h                        |  55 +++
>  16 files changed, 1131 insertions(+), 383 deletions(-)
>  create mode 100644 drivers/base/regmap/regmap-indirect.c
>  create mode 100644 drivers/mfd/intel-m10-bmc-core.c
>  create mode 100644 drivers/mfd/intel-m10-bmc-pmci.c
>  create mode 100644 drivers/mfd/intel-m10-bmc-spi.c
>  delete mode 100644 drivers/mfd/intel-m10-bmc.c
> 
> -- 
> 2.30.2
> 