mbox series

[00/10] spi: spi-mem: Add driver for Aspeed SMC controllers

Message ID 20220214094231.3753686-1-clg@kaod.org
Headers show
Series spi: spi-mem: Add driver for Aspeed SMC controllers | expand

Message

Cédric Le Goater Feb. 14, 2022, 9:42 a.m. UTC
Hi,

This series adds a new SPI driver using the spi-mem interface for the
Aspeed static memory controllers of the AST2600, AST2500 and AST2400
SoCs.

 * AST2600 Firmware SPI Memory Controller (FMC)
 * AST2600 SPI Flash Controller (SPI1 and SPI2)
 * AST2500 Firmware SPI Memory Controller (FMC)
 * AST2500 SPI Flash Controller (SPI1 and SPI2)
 * AST2400 New Static Memory Controller (also referred as FMC)
 * AST2400 SPI Flash Controller (SPI)

It is based on the current OpenBMC kernel driver [1], using directly
the MTD SPI-NOR interface and on a patchset [2] previously proposed
adding support for the AST2600 only. This driver takes a slightly
different approach to cover all 6 controllers.

It does not make use of the controller register disabling Address and
Data byte lanes because is not available on the AST2400 SoC. We could
introduce a specific handler for new features available on recent SoCs
if needed. As there is not much difference on performance, the driver
chooses the common denominator: "User mode" which has been heavily
tested in [1]. "User mode" is also used as a fall back method when
flash device mapping window is too small.

Problems to address with spi-mem were the configuration of the mapping
windows and the calibration of the read timings. The driver handles
them in the direct mapping handler when some knowledge on the size of
the flash device is know. It is not perfect but not incorrect either.
The algorithm is one from [1] because it doesn't require the DMA
registers which are not available on all controllers.

Direct mapping for writes is not supported (yet). I have seen some
corruption with writes and I preferred to use the safer and proven
method of the initial driver [1]. We can improve that later.

The driver supports Quad SPI RX transfers on the AST2600 SoC but it
didn't have the expected results. Therefore it is not activated yet.
This needs more tests.

The series does not remove the current Aspeed SMC driver but prepares
ground for its removal by changing its CONFIG option. This last step
can be addressed as a followup when the new driver using the spi-mem
interface has been sufficiently exposed. 

Tested on:
 
 * OpenPOWER Palmetto (AST2400)
 * Evaluation board (AST2500) 
 * OpenPOWER Witherspoon (AST2500)
 * Evaluation board (AST2600 A0)
 * Rainier board (AST2600)
 
[1] https://github.com/openbmc/linux/blob/dev-5.15/drivers/mtd/spi-nor/controllers/aspeed-smc.c
[2] https://patchwork.ozlabs.org/project/linux-aspeed/list/?series=212394

Thanks,

C. 

Cédric Le Goater (10):
  mtd: spi-nor: aspeed: Rename Kconfig option
  dt-bindings: spi: Add Aspeed SMC controllers device tree binding
  spi: spi-mem: Add driver for Aspeed SMC controllers
  spi: aspeed: Add support for direct mapping
  spi: aspeed: Adjust direct mapping to device size
  spi: aspeed: Workaround AST2500 limitations
  spi: aspeed: Add support for the AST2400 SPI controller
  spi: aspeed: Calibrate read timings
  ARM: dts: aspeed: Enable Dual SPI RX transfers
  spi: aspeed: Activate new spi-mem driver

 drivers/spi/spi-aspeed-smc.c                  | 1241 +++++++++++++++++
 .../bindings/spi/aspeed,ast2600-fmc.yaml      |   92 ++
 arch/arm/boot/dts/aspeed-g4.dtsi              |    6 +
 arch/arm/boot/dts/aspeed-g5.dtsi              |    7 +
 arch/arm/boot/dts/aspeed-g6.dtsi              |    8 +
 drivers/mtd/spi-nor/controllers/Kconfig       |    4 +-
 drivers/mtd/spi-nor/controllers/Makefile      |    2 +-
 drivers/spi/Kconfig                           |   11 +
 drivers/spi/Makefile                          |    1 +
 9 files changed, 1369 insertions(+), 3 deletions(-)
 create mode 100644 drivers/spi/spi-aspeed-smc.c
 create mode 100644 Documentation/devicetree/bindings/spi/aspeed,ast2600-fmc.yaml

Comments

Lukas Wunner Feb. 15, 2022, 6:27 a.m. UTC | #1
On Mon, Feb 14, 2022 at 10:42:24AM +0100, Cédric Le Goater wrote:
> +static int aspeed_spi_probe(struct platform_device *pdev)
> +{
[...]
> +	ctlr = spi_alloc_master(dev, sizeof(*aspi));
> +	if (!ctlr)
> +		return -ENOMEM;

Use devm_spi_alloc_master() and remove the "put_controller" error path
for simplicity.


> +	ret = devm_spi_register_controller(dev, ctlr);
[...]
> +static int aspeed_spi_remove(struct platform_device *pdev)
> +{
> +	struct spi_controller *ctlr = platform_get_drvdata(pdev);
> +	struct aspeed_spi *aspi = spi_controller_get_devdata(ctlr);
> +
> +	aspeed_spi_enable(aspi, false);
> +	spi_unregister_controller(ctlr);
> +	clk_disable_unprepare(aspi->clk);
> +	return 0;
> +}

You need to move the call to spi_unregister_controller() *before*
the call to aspeed_spi_enable().  The controller must be fully
operational until spi_unregister_controller() returns.

You also need to replace the call to devm_spi_register_controller()
in aspeed_spi_probe() with spi_register_controller().
Otherwise you'll unregister the controller twice because you're
calling spi_unregister_controller() in aspeed_spi_remove().

Thanks,

Lukas
Cédric Le Goater Feb. 15, 2022, 9:07 a.m. UTC | #2
Hello Lukas,

On 2/15/22 07:27, Lukas Wunner wrote:
> On Mon, Feb 14, 2022 at 10:42:24AM +0100, Cédric Le Goater wrote:
>> +static int aspeed_spi_probe(struct platform_device *pdev)
>> +{
> [...]
>> +	ctlr = spi_alloc_master(dev, sizeof(*aspi));
>> +	if (!ctlr)
>> +		return -ENOMEM;
> 
> Use devm_spi_alloc_master() and remove the "put_controller" error path
> for simplicity.
  
yes.

>> +	ret = devm_spi_register_controller(dev, ctlr);
> [...]
>> +static int aspeed_spi_remove(struct platform_device *pdev)
>> +{
>> +	struct spi_controller *ctlr = platform_get_drvdata(pdev);
>> +	struct aspeed_spi *aspi = spi_controller_get_devdata(ctlr);
>> +
>> +	aspeed_spi_enable(aspi, false);
>> +	spi_unregister_controller(ctlr);
>> +	clk_disable_unprepare(aspi->clk);
>> +	return 0;
>> +}
> 
> You need to move the call to spi_unregister_controller() *before*
> the call to aspeed_spi_enable().  The controller must be fully
> operational until spi_unregister_controller() returns.
> 
> You also need to replace the call to devm_spi_register_controller()
> in aspeed_spi_probe() with spi_register_controller().
> Otherwise you'll unregister the controller twice because you're
> calling spi_unregister_controller() in aspeed_spi_remove().

ok. Understood. Done in v2.

Thanks,

C.
Joel Stanley Feb. 16, 2022, 7:02 a.m. UTC | #3
On Mon, 14 Feb 2022 at 09:43, Cédric Le Goater <clg@kaod.org> wrote:
>
> The previous driver using the MTD SPI NOR interface is kept in case we
> find some issues but we should remove it quickly once the new driver
> using the spi-mem interface has been sufficiently exposed.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

I suggest we drop the defconfig changes from both this patch and the
first. This way we'll always have the new driver being built, with
less churn.

If you strongly prefer the way you've done it then that's fine too.

> ---
>  arch/arm/configs/aspeed_g4_defconfig | 2 +-
>  arch/arm/configs/aspeed_g5_defconfig | 2 +-
>  arch/arm/configs/multi_v5_defconfig  | 2 +-
>  arch/arm/configs/multi_v7_defconfig  | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/configs/aspeed_g4_defconfig b/arch/arm/configs/aspeed_g4_defconfig
> index 964536444cd7..b4a1b2ed1a17 100644
> --- a/arch/arm/configs/aspeed_g4_defconfig
> +++ b/arch/arm/configs/aspeed_g4_defconfig
> @@ -64,7 +64,7 @@ CONFIG_MTD_BLOCK=y
>  CONFIG_MTD_PARTITIONED_MASTER=y
>  CONFIG_MTD_SPI_NOR=y
>  # CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is not set
> -CONFIG_SPI_ASPEED_SMC_MTD_SPI_NOR=y
> +CONFIG_SPI_ASPEED_SMC=y
>  CONFIG_MTD_UBI=y
>  CONFIG_MTD_UBI_FASTMAP=y
>  CONFIG_MTD_UBI_BLOCK=y
> diff --git a/arch/arm/configs/aspeed_g5_defconfig b/arch/arm/configs/aspeed_g5_defconfig
> index e809236ca88b..ccc4240ee4b5 100644
> --- a/arch/arm/configs/aspeed_g5_defconfig
> +++ b/arch/arm/configs/aspeed_g5_defconfig
> @@ -103,7 +103,7 @@ CONFIG_MTD_BLOCK=y
>  CONFIG_MTD_PARTITIONED_MASTER=y
>  CONFIG_MTD_SPI_NOR=y
>  # CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is not set
> -CONFIG_SPI_ASPEED_SMC_MTD_SPI_NOR=y
> +CONFIG_SPI_ASPEED_SMC=y
>  CONFIG_MTD_UBI=y
>  CONFIG_MTD_UBI_FASTMAP=y
>  CONFIG_MTD_UBI_BLOCK=y
> diff --git a/arch/arm/configs/multi_v5_defconfig b/arch/arm/configs/multi_v5_defconfig
> index 49083ef05fb0..80a3ae02d759 100644
> --- a/arch/arm/configs/multi_v5_defconfig
> +++ b/arch/arm/configs/multi_v5_defconfig
> @@ -103,7 +103,7 @@ CONFIG_MTD_RAW_NAND=y
>  CONFIG_MTD_NAND_ATMEL=y
>  CONFIG_MTD_NAND_ORION=y
>  CONFIG_MTD_SPI_NOR=y
> -CONFIG_SPI_ASPEED_SMC_MTD_SPI_NOR=y
> +CONFIG_SPI_ASPEED_SMC=y
>  CONFIG_MTD_UBI=y
>  CONFIG_BLK_DEV_LOOP=y
>  CONFIG_ATMEL_SSC=m
> diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
> index fc1b69256b64..33572998dbbe 100644
> --- a/arch/arm/configs/multi_v7_defconfig
> +++ b/arch/arm/configs/multi_v7_defconfig
> @@ -217,7 +217,7 @@ CONFIG_MTD_NAND_DAVINCI=y
>  CONFIG_MTD_NAND_STM32_FMC2=y
>  CONFIG_MTD_NAND_PL35X=y
>  CONFIG_MTD_SPI_NOR=y
> -CONFIG_SPI_ASPEED_SMC_MTD_SPI_NOR=m
> +CONFIG_SPI_ASPEED_SMC=m
>  CONFIG_MTD_UBI=y
>  CONFIG_BLK_DEV_LOOP=y
>  CONFIG_BLK_DEV_RAM=y
> --
> 2.34.1
>
Joel Stanley Feb. 16, 2022, 7:21 a.m. UTC | #4
On Mon, 14 Feb 2022 at 09:42, Cédric Le Goater <clg@kaod.org> wrote:
>
> Hi,
>
> This series adds a new SPI driver using the spi-mem interface for the
> Aspeed static memory controllers of the AST2600, AST2500 and AST2400
> SoCs.
>
>  * AST2600 Firmware SPI Memory Controller (FMC)
>  * AST2600 SPI Flash Controller (SPI1 and SPI2)
>  * AST2500 Firmware SPI Memory Controller (FMC)
>  * AST2500 SPI Flash Controller (SPI1 and SPI2)
>  * AST2400 New Static Memory Controller (also referred as FMC)
>  * AST2400 SPI Flash Controller (SPI)
>
> It is based on the current OpenBMC kernel driver [1], using directly
> the MTD SPI-NOR interface and on a patchset [2] previously proposed
> adding support for the AST2600 only. This driver takes a slightly
> different approach to cover all 6 controllers.
>
> It does not make use of the controller register disabling Address and
> Data byte lanes because is not available on the AST2400 SoC. We could
> introduce a specific handler for new features available on recent SoCs
> if needed. As there is not much difference on performance, the driver
> chooses the common denominator: "User mode" which has been heavily
> tested in [1]. "User mode" is also used as a fall back method when
> flash device mapping window is too small.
>
> Problems to address with spi-mem were the configuration of the mapping
> windows and the calibration of the read timings. The driver handles
> them in the direct mapping handler when some knowledge on the size of
> the flash device is know. It is not perfect but not incorrect either.
> The algorithm is one from [1] because it doesn't require the DMA
> registers which are not available on all controllers.
>
> Direct mapping for writes is not supported (yet). I have seen some
> corruption with writes and I preferred to use the safer and proven
> method of the initial driver [1]. We can improve that later.
>
> The driver supports Quad SPI RX transfers on the AST2600 SoC but it
> didn't have the expected results. Therefore it is not activated yet.
> This needs more tests.
>
> The series does not remove the current Aspeed SMC driver but prepares
> ground for its removal by changing its CONFIG option. This last step
> can be addressed as a followup when the new driver using the spi-mem
> interface has been sufficiently exposed.
>
> Tested on:
>
>  * OpenPOWER Palmetto (AST2400)
>  * Evaluation board (AST2500)
>  * OpenPOWER Witherspoon (AST2500)
>  * Evaluation board (AST2600 A0)
>  * Rainier board (AST2600)

Looks great! Thanks for doing this work Cédric.

I reviewed all of the patches. The device tree and defconfig ones,
which we will send via my aspeed tree, are good to go.

The others look good too, to the best of my knowledge.

I'll do some more testing of your v2 when you send it out.

Cheers,

Joel

>
> [1] https://github.com/openbmc/linux/blob/dev-5.15/drivers/mtd/spi-nor/controllers/aspeed-smc.c
> [2] https://patchwork.ozlabs.org/project/linux-aspeed/list/?series=212394
>
> Thanks,
>
> C.
>
> Cédric Le Goater (10):
>   mtd: spi-nor: aspeed: Rename Kconfig option
>   dt-bindings: spi: Add Aspeed SMC controllers device tree binding
>   spi: spi-mem: Add driver for Aspeed SMC controllers
>   spi: aspeed: Add support for direct mapping
>   spi: aspeed: Adjust direct mapping to device size
>   spi: aspeed: Workaround AST2500 limitations
>   spi: aspeed: Add support for the AST2400 SPI controller
>   spi: aspeed: Calibrate read timings
>   ARM: dts: aspeed: Enable Dual SPI RX transfers
>   spi: aspeed: Activate new spi-mem driver
>
>  drivers/spi/spi-aspeed-smc.c                  | 1241 +++++++++++++++++
>  .../bindings/spi/aspeed,ast2600-fmc.yaml      |   92 ++
>  arch/arm/boot/dts/aspeed-g4.dtsi              |    6 +
>  arch/arm/boot/dts/aspeed-g5.dtsi              |    7 +
>  arch/arm/boot/dts/aspeed-g6.dtsi              |    8 +
>  drivers/mtd/spi-nor/controllers/Kconfig       |    4 +-
>  drivers/mtd/spi-nor/controllers/Makefile      |    2 +-
>  drivers/spi/Kconfig                           |   11 +
>  drivers/spi/Makefile                          |    1 +
>  9 files changed, 1369 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/spi/spi-aspeed-smc.c
>  create mode 100644 Documentation/devicetree/bindings/spi/aspeed,ast2600-fmc.yaml
>
> --
> 2.34.1
>
Joel Stanley Feb. 16, 2022, 7:21 a.m. UTC | #5
On Mon, 14 Feb 2022 at 09:43, Cédric Le Goater <clg@kaod.org> wrote:
>
> All these controllers support at least Dual SPI. Update the DTs.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  arch/arm/boot/dts/aspeed-g4.dtsi | 6 ++++++
>  arch/arm/boot/dts/aspeed-g5.dtsi | 7 +++++++
>  arch/arm/boot/dts/aspeed-g6.dtsi | 8 ++++++++
>  3 files changed, 21 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
> index f14dace34c5a..da211fbd8658 100644
> --- a/arch/arm/boot/dts/aspeed-g4.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g4.dtsi
> @@ -65,27 +65,32 @@ fmc: spi@1e620000 {
>                         flash@0 {
>                                 reg = < 0 >;
>                                 compatible = "jedec,spi-nor";
> +                               spi-rx-bus-width = <2>;
>                                 spi-max-frequency = <50000000>;
>                                 status = "disabled";
>                         };
>                         flash@1 {
>                                 reg = < 1 >;
>                                 compatible = "jedec,spi-nor";
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                         flash@2 {
>                                 reg = < 2 >;
>                                 compatible = "jedec,spi-nor";
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                         flash@3 {
>                                 reg = < 3 >;
>                                 compatible = "jedec,spi-nor";
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                         flash@4 {
>                                 reg = < 4 >;
>                                 compatible = "jedec,spi-nor";
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                 };
> @@ -102,6 +107,7 @@ flash@0 {
>                                 reg = < 0 >;
>                                 compatible = "jedec,spi-nor";
>                                 spi-max-frequency = <50000000>;
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                 };
> diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
> index 7495f93c5069..804b66d32127 100644
> --- a/arch/arm/boot/dts/aspeed-g5.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g5.dtsi
> @@ -67,18 +67,21 @@ flash@0 {
>                                 reg = < 0 >;
>                                 compatible = "jedec,spi-nor";
>                                 spi-max-frequency = <50000000>;
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                         flash@1 {
>                                 reg = < 1 >;
>                                 compatible = "jedec,spi-nor";
>                                 spi-max-frequency = <50000000>;
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                         flash@2 {
>                                 reg = < 2 >;
>                                 compatible = "jedec,spi-nor";
>                                 spi-max-frequency = <50000000>;
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                 };
> @@ -95,12 +98,14 @@ flash@0 {
>                                 reg = < 0 >;
>                                 compatible = "jedec,spi-nor";
>                                 spi-max-frequency = <50000000>;
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                         flash@1 {
>                                 reg = < 1 >;
>                                 compatible = "jedec,spi-nor";
>                                 spi-max-frequency = <50000000>;
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                 };
> @@ -117,12 +122,14 @@ flash@0 {
>                                 reg = < 0 >;
>                                 compatible = "jedec,spi-nor";
>                                 spi-max-frequency = <50000000>;
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                         flash@1 {
>                                 reg = < 1 >;
>                                 compatible = "jedec,spi-nor";
>                                 spi-max-frequency = <50000000>;
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                 };
> diff --git a/arch/arm/boot/dts/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed-g6.dtsi
> index c32e87fad4dc..542714c61a85 100644
> --- a/arch/arm/boot/dts/aspeed-g6.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g6.dtsi
> @@ -107,18 +107,21 @@ flash@0 {
>                                 reg = < 0 >;
>                                 compatible = "jedec,spi-nor";
>                                 spi-max-frequency = <50000000>;
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                         flash@1 {
>                                 reg = < 1 >;
>                                 compatible = "jedec,spi-nor";
>                                 spi-max-frequency = <50000000>;
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                         flash@2 {
>                                 reg = < 2 >;
>                                 compatible = "jedec,spi-nor";
>                                 spi-max-frequency = <50000000>;
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                 };
> @@ -135,12 +138,14 @@ flash@0 {
>                                 reg = < 0 >;
>                                 compatible = "jedec,spi-nor";
>                                 spi-max-frequency = <50000000>;
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                         flash@1 {
>                                 reg = < 1 >;
>                                 compatible = "jedec,spi-nor";
>                                 spi-max-frequency = <50000000>;
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                 };
> @@ -157,18 +162,21 @@ flash@0 {
>                                 reg = < 0 >;
>                                 compatible = "jedec,spi-nor";
>                                 spi-max-frequency = <50000000>;
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                         flash@1 {
>                                 reg = < 1 >;
>                                 compatible = "jedec,spi-nor";
>                                 spi-max-frequency = <50000000>;
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                         flash@2 {
>                                 reg = < 2 >;
>                                 compatible = "jedec,spi-nor";
>                                 spi-max-frequency = <50000000>;
> +                               spi-rx-bus-width = <2>;
>                                 status = "disabled";
>                         };
>                 };
> --
> 2.34.1
>
Cédric Le Goater Feb. 16, 2022, 8:12 a.m. UTC | #6
On 2/16/22 08:02, Joel Stanley wrote:
> On Mon, 14 Feb 2022 at 09:43, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> The previous driver using the MTD SPI NOR interface is kept in case we
>> find some issues but we should remove it quickly once the new driver
>> using the spi-mem interface has been sufficiently exposed.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> I suggest we drop the defconfig changes from both this patch and the
> first. This way we'll always have the new driver being built, with
> less churn.
> 
> If you strongly prefer the way you've done it then that's fine too.

I am fine with that, but, with only patch 1, the defconfig files would
be referencing an non-existing CONFIG. Is that ok ?

Thanks,

C.



> 
>> ---
>>   arch/arm/configs/aspeed_g4_defconfig | 2 +-
>>   arch/arm/configs/aspeed_g5_defconfig | 2 +-
>>   arch/arm/configs/multi_v5_defconfig  | 2 +-
>>   arch/arm/configs/multi_v7_defconfig  | 2 +-
>>   4 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/configs/aspeed_g4_defconfig b/arch/arm/configs/aspeed_g4_defconfig
>> index 964536444cd7..b4a1b2ed1a17 100644
>> --- a/arch/arm/configs/aspeed_g4_defconfig
>> +++ b/arch/arm/configs/aspeed_g4_defconfig
>> @@ -64,7 +64,7 @@ CONFIG_MTD_BLOCK=y
>>   CONFIG_MTD_PARTITIONED_MASTER=y
>>   CONFIG_MTD_SPI_NOR=y
>>   # CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is not set
>> -CONFIG_SPI_ASPEED_SMC_MTD_SPI_NOR=y
>> +CONFIG_SPI_ASPEED_SMC=y
>>   CONFIG_MTD_UBI=y
>>   CONFIG_MTD_UBI_FASTMAP=y
>>   CONFIG_MTD_UBI_BLOCK=y
>> diff --git a/arch/arm/configs/aspeed_g5_defconfig b/arch/arm/configs/aspeed_g5_defconfig
>> index e809236ca88b..ccc4240ee4b5 100644
>> --- a/arch/arm/configs/aspeed_g5_defconfig
>> +++ b/arch/arm/configs/aspeed_g5_defconfig
>> @@ -103,7 +103,7 @@ CONFIG_MTD_BLOCK=y
>>   CONFIG_MTD_PARTITIONED_MASTER=y
>>   CONFIG_MTD_SPI_NOR=y
>>   # CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is not set
>> -CONFIG_SPI_ASPEED_SMC_MTD_SPI_NOR=y
>> +CONFIG_SPI_ASPEED_SMC=y
>>   CONFIG_MTD_UBI=y
>>   CONFIG_MTD_UBI_FASTMAP=y
>>   CONFIG_MTD_UBI_BLOCK=y
>> diff --git a/arch/arm/configs/multi_v5_defconfig b/arch/arm/configs/multi_v5_defconfig
>> index 49083ef05fb0..80a3ae02d759 100644
>> --- a/arch/arm/configs/multi_v5_defconfig
>> +++ b/arch/arm/configs/multi_v5_defconfig
>> @@ -103,7 +103,7 @@ CONFIG_MTD_RAW_NAND=y
>>   CONFIG_MTD_NAND_ATMEL=y
>>   CONFIG_MTD_NAND_ORION=y
>>   CONFIG_MTD_SPI_NOR=y
>> -CONFIG_SPI_ASPEED_SMC_MTD_SPI_NOR=y
>> +CONFIG_SPI_ASPEED_SMC=y
>>   CONFIG_MTD_UBI=y
>>   CONFIG_BLK_DEV_LOOP=y
>>   CONFIG_ATMEL_SSC=m
>> diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
>> index fc1b69256b64..33572998dbbe 100644
>> --- a/arch/arm/configs/multi_v7_defconfig
>> +++ b/arch/arm/configs/multi_v7_defconfig
>> @@ -217,7 +217,7 @@ CONFIG_MTD_NAND_DAVINCI=y
>>   CONFIG_MTD_NAND_STM32_FMC2=y
>>   CONFIG_MTD_NAND_PL35X=y
>>   CONFIG_MTD_SPI_NOR=y
>> -CONFIG_SPI_ASPEED_SMC_MTD_SPI_NOR=m
>> +CONFIG_SPI_ASPEED_SMC=m
>>   CONFIG_MTD_UBI=y
>>   CONFIG_BLK_DEV_LOOP=y
>>   CONFIG_BLK_DEV_RAM=y
>> --
>> 2.34.1
>>
Pratyush Yadav Feb. 25, 2022, 7:31 a.m. UTC | #7
On 14/02/22 10:42AM, Cédric Le Goater wrote:
> To prepare transition to the new Aspeed SMC SPI controller driver using
> the spi-mem interface, change the kernel CONFIG option of the current
> driver to reflect that the implementation uses the MTD SPI-NOR interface.
> Once the new driver is sufficiently exposed, we should remove the old one.

I don't quite understand the reasoning behind this. Why keep the old 
driver around? Why not directly replace it with the new one? Does the 
new one have any limitations that this one doesn't?
Pratyush Yadav Feb. 25, 2022, 7:50 a.m. UTC | #8
On 14/02/22 10:42AM, Cédric Le Goater wrote:
> This SPI driver adds support for the Aspeed static memory controllers
> of the AST2600, AST2500 and AST2400 SoCs using the spi-mem interface.
> 
>  * AST2600 Firmware SPI Memory Controller (FMC)
>    . BMC firmware
>    . 3 chip select pins (CE0 ~ CE2)
>    . Only supports SPI type flash memory
>    . different segment register interface
>    . single, dual and quad mode.
> 
>  * AST2600 SPI Flash Controller (SPI1 and SPI2)
>    . host firmware
>    . 2 chip select pins (CE0 ~ CE1)
>    . different segment register interface
>    . single, dual and quad mode.
> 
>  * AST2500 Firmware SPI Memory Controller (FMC)
>    . BMC firmware
>    . 3 chip select pins (CE0 ~ CE2)
>    . supports SPI type flash memory (CE0-CE1)
>    . CE2 can be of NOR type flash but this is not supported by the driver
>    . single, dual mode.
> 
>  * AST2500 SPI Flash Controller (SPI1 and SPI2)
>    . host firmware
>    . 2 chip select pins (CE0 ~ CE1)
>    . single, dual mode.
> 
>  * AST2400 New Static Memory Controller (also referred as FMC)
>    . BMC firmware
>    . New register set
>    . 5 chip select pins (CE0 ∼ CE4)
>    . supports NOR flash, NAND flash and SPI flash memory.
>    . single, dual and quad mode.
> 
> Each controller has a memory range on which flash devices contents are
> mapped. Each device is assigned a window that can be changed at bootime
> with the Segment Address Registers.
> 
> Each SPI flash device can then be accessed in two modes: Command and
> User. When in User mode, SPI transfers are initiated with accesses to
> the memory segment of a device. When in Command mode, memory
> operations on the memory segment of a device generate SPI commands
> automatically using a Control Register for the settings.
> 
> This initial patch adds support for User mode. Command mode needs a little
> more work to check that the memory window on the AHB bus fits the device
> size. It will come later when support for direct mapping is added.
> 
> Single and dual mode RX transfers are supported. Other types than SPI
> are not supported.
> 
> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  drivers/spi/spi-aspeed-smc.c            | 766 ++++++++++++++++++++++++
>  drivers/mtd/spi-nor/controllers/Kconfig |   2 +-
>  drivers/spi/Kconfig                     |  11 +
>  drivers/spi/Makefile                    |   1 +
>  4 files changed, 779 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/spi/spi-aspeed-smc.c
> 
[...]
> +
> +/* support for 1-1-1, 1-1-2 or 1-1-4 */
> +static bool aspeed_spi_supports_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> +	if (op->cmd.buswidth > 1)
> +		return false;
> +
> +	if (op->addr.nbytes != 0) {
> +		if (op->addr.buswidth > 1 || op->addr.nbytes > 4)
> +			return false;
> +	}
> +
> +	if (op->dummy.nbytes != 0) {
> +		if (op->dummy.buswidth > 1 || op->dummy.nbytes > 7)
> +			return false;
> +	}
> +
> +	if (op->data.nbytes != 0 && op->data.buswidth > 4)
> +		return false;
> +
> +	if (!spi_mem_default_supports_op(mem, op))
> +		return false;
> +
> +	return true;

Nitpick: You can just do return spi_mem_default_supports_op(mem, op);

> +}
> +
[...]
> +
> +static int aspeed_spi_init_devices(struct platform_device *pdev, struct aspeed_spi *aspi)
> +{
> +	struct device_node *np;
> +	unsigned int cs;
> +	int ret;
> +
> +	for_each_available_child_of_node(aspi->dev->of_node, np) {
> +		struct aspeed_spi_chip *chip;
> +
> +		if (!of_device_is_compatible(np, "jedec,spi-nor"))
> +			continue;
> +
> +		ret = of_property_read_u32(np, "reg", &cs);
> +		if (ret) {
> +			dev_err(aspi->dev, "Couldn't not read chip select.\n");
> +			of_node_put(np);
> +			return ret;
> +		}
> +
> +		if (cs > aspi->data->max_cs) {
> +			dev_err(aspi->dev, "Chip select %d out of range.\n", cs);
> +			of_node_put(np);
> +			return -ERANGE;
> +		}
> +
> +		chip = &aspi->chips[cs];
> +		chip->aspi = aspi;
> +		chip->cs = cs;
> +
> +		ret = aspeed_spi_chip_init(chip);
> +		if (ret) {
> +			of_node_put(np);
> +			return ret;
> +		}
> +
> +		if (of_property_read_u32(np, "spi-max-frequency", &chip->clk_freq))
> +			chip->clk_freq = ASPEED_SPI_DEFAULT_FREQ;
> +
> +		aspi->num_cs++;
> +	}

SPI MEM already gives you all this information. Get it from there, don't 
parse it yourself.

You can get Chip Select via spi_mem->spi->chip_select.
You can get clock frequency via spi_mem->spi->max_speed_hz.

With these comments fixed,

Acked-by: Pratyush Yadav <p.yadav@ti.com>

> +
> +	return 0;
> +}
> +
[...]
Pratyush Yadav Feb. 25, 2022, 9:12 a.m. UTC | #9
On 14/02/22 10:42AM, Cédric Le Goater wrote:
> Use direct mapping to read the flash device contents. This operation
> mode is called "Command mode" on Aspeed SoC SMC controllers. It uses a
> Control Register for the settings to apply when a memory operation is
> performed on the flash device mapping window.
> 
> If the window is not big enough, fall back to the "User mode" to
> perform the read.
> 
> Direct mapping for writes will come later when validated.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  drivers/spi/spi-aspeed-smc.c | 67 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 65 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
> index 0aeff6f468af..8d33fcb7736a 100644
> --- a/drivers/spi/spi-aspeed-smc.c
> +++ b/drivers/spi/spi-aspeed-smc.c
> @@ -345,8 +345,8 @@ static int do_aspeed_spi_exec_op(struct spi_mem *mem, const struct spi_mem_op *o
>  		if (!op->addr.nbytes)
>  			ret = aspeed_spi_read_reg(chip, op);
>  		else
> -			ret = aspeed_spi_read_user(chip, op, op->addr.val,
> -						   op->data.nbytes, op->data.buf.in);
> +			memcpy_fromio(op->data.buf.in, chip->ahb_base + op->addr.val,
> +				      op->data.nbytes);

Why change this? exec_op should be independent from dirmap APIs. And you 
don't even do the ahb_window_size checks here.

>  	} else {
>  		if (!op->addr.nbytes)
>  			ret = aspeed_spi_write_reg(chip, op);
> @@ -426,10 +426,73 @@ static int aspeed_spi_chip_set_default_window(struct aspeed_spi_chip *chip)
>  	return chip->ahb_window_size ? 0 : -1;
>  }
>  
> +static int aspeed_spi_dirmap_create(struct spi_mem_dirmap_desc *desc)
> +{
> +	struct aspeed_spi *aspi = spi_controller_get_devdata(desc->mem->spi->master);
> +	struct aspeed_spi_chip *chip = &aspi->chips[desc->mem->spi->chip_select];
> +	struct spi_mem_op *op = &desc->info.op_tmpl;
> +	u32 ctl_val;
> +	int ret = 0;
> +
> +	chip->clk_freq = desc->mem->spi->max_speed_hz;
> +
> +	/* Only for reads */
> +	if (op->data.dir != SPI_MEM_DATA_IN)
> +		return -EOPNOTSUPP;
> +
> +	if (desc->info.length > chip->ahb_window_size)
> +		dev_warn(aspi->dev, "CE%d window (%dMB) too small for mapping",
> +			 chip->cs, chip->ahb_window_size >> 20);
> +
> +	/* Define the default IO read settings */
> +	ctl_val = readl(chip->ctl) & ~CTRL_IO_CMD_MASK;
> +	ctl_val |= aspeed_spi_get_io_mode(op) |
> +		op->cmd.opcode << CTRL_COMMAND_SHIFT |
> +		CTRL_IO_DUMMY_SET(op->dummy.nbytes / op->dummy.buswidth) |
> +		CTRL_IO_MODE_READ;
> +
> +	/* Tune 4BYTE address mode */
> +	if (op->addr.nbytes) {
> +		u32 addr_mode = readl(aspi->regs + CE_CTRL_REG);
> +
> +		if (op->addr.nbytes == 4)
> +			addr_mode |= (0x11 << chip->cs);
> +		else
> +			addr_mode &= ~(0x11 << chip->cs);
> +		writel(addr_mode, aspi->regs + CE_CTRL_REG);
> +	}
> +
> +	/* READ mode is the controller default setting */
> +	chip->ctl_val[ASPEED_SPI_READ] = ctl_val;
> +	writel(chip->ctl_val[ASPEED_SPI_READ], chip->ctl);
> +
> +	dev_info(aspi->dev, "CE%d read buswidth:%d [0x%08x]\n",
> +		 chip->cs, op->data.buswidth, chip->ctl_val[ASPEED_SPI_READ]);
> +
> +	return ret;
> +}
> +
> +static int aspeed_spi_dirmap_read(struct spi_mem_dirmap_desc *desc,
> +				  u64 offset, size_t len, void *buf)
> +{
> +	struct aspeed_spi *aspi = spi_controller_get_devdata(desc->mem->spi->master);
> +	struct aspeed_spi_chip *chip = &aspi->chips[desc->mem->spi->chip_select];
> +
> +	/* Switch to USER command mode if mapping window is too small */
> +	if (chip->ahb_window_size < offset + len)
> +		aspeed_spi_read_user(chip, &desc->info.op_tmpl, offset, len, buf);
> +	else
> +		memcpy_fromio(buf, chip->ahb_base + offset, len);
> +
> +	return len;
> +}
> +
>  static const struct spi_controller_mem_ops aspeed_spi_mem_ops = {
>  	.supports_op = aspeed_spi_supports_op,
>  	.exec_op = aspeed_spi_exec_op,
>  	.get_name = aspeed_spi_get_name,
> +	.dirmap_create = aspeed_spi_dirmap_create,
> +	.dirmap_read = aspeed_spi_dirmap_read,
>  };
>  
>  static void aspeed_spi_chip_set_type(struct aspeed_spi_chip *chip, int type)
> -- 
> 2.34.1
>
Pratyush Yadav Feb. 25, 2022, 9:18 a.m. UTC | #10
On 14/02/22 10:42AM, Cédric Le Goater wrote:
> To accommodate the different response time of SPI transfers on different
> boards and different SPI NOR devices, the Aspeed controllers provide a
> set of Read Timing Compensation registers to tune the timing delays
> depending on the frequency being used. The AST2600 SoC has one of
> these registers per device. On the AST2500 and AST2400 SoCs, the
> timing register is shared by all devices which is a bit problematic to
> get good results other than for one device.
> 
> The algorithm first reads a golden buffer at low speed and then performs
> reads with different clocks and delay cycle settings to find a breaking
> point. This selects a default good frequency for the CEx control register.
> The current settings are bit optimistic as we pick the first delay giving
> good results. A safer approach would be to determine an interval and
> choose the middle value.
> 
> Due to the lack of API, calibration is performed when the direct mapping
> for reads is created.

The dirmap_create mapping says nothing about _when_ it should be called. 
So there is no guarantee that it will only be called after the flash is 
fully initialized. I suggest you either make this a requirement of the 
API, or create a new API that guarantees it will only be called after 
the flash is initialized, like [0].

[0] https://patchwork.ozlabs.org/project/linux-mtd/patch/20210311191216.7363-2-p.yadav@ti.com/

> 
> Cc: Pratyush Yadav <p.yadav@ti.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
Cédric Le Goater Feb. 27, 2022, 6:46 p.m. UTC | #11
On 2/25/22 08:50, Pratyush Yadav wrote:
> On 14/02/22 10:42AM, Cédric Le Goater wrote:
>> This SPI driver adds support for the Aspeed static memory controllers
>> of the AST2600, AST2500 and AST2400 SoCs using the spi-mem interface.
>>
>>   * AST2600 Firmware SPI Memory Controller (FMC)
>>     . BMC firmware
>>     . 3 chip select pins (CE0 ~ CE2)
>>     . Only supports SPI type flash memory
>>     . different segment register interface
>>     . single, dual and quad mode.
>>
>>   * AST2600 SPI Flash Controller (SPI1 and SPI2)
>>     . host firmware
>>     . 2 chip select pins (CE0 ~ CE1)
>>     . different segment register interface
>>     . single, dual and quad mode.
>>
>>   * AST2500 Firmware SPI Memory Controller (FMC)
>>     . BMC firmware
>>     . 3 chip select pins (CE0 ~ CE2)
>>     . supports SPI type flash memory (CE0-CE1)
>>     . CE2 can be of NOR type flash but this is not supported by the driver
>>     . single, dual mode.
>>
>>   * AST2500 SPI Flash Controller (SPI1 and SPI2)
>>     . host firmware
>>     . 2 chip select pins (CE0 ~ CE1)
>>     . single, dual mode.
>>
>>   * AST2400 New Static Memory Controller (also referred as FMC)
>>     . BMC firmware
>>     . New register set
>>     . 5 chip select pins (CE0 ∼ CE4)
>>     . supports NOR flash, NAND flash and SPI flash memory.
>>     . single, dual and quad mode.
>>
>> Each controller has a memory range on which flash devices contents are
>> mapped. Each device is assigned a window that can be changed at bootime
>> with the Segment Address Registers.
>>
>> Each SPI flash device can then be accessed in two modes: Command and
>> User. When in User mode, SPI transfers are initiated with accesses to
>> the memory segment of a device. When in Command mode, memory
>> operations on the memory segment of a device generate SPI commands
>> automatically using a Control Register for the settings.
>>
>> This initial patch adds support for User mode. Command mode needs a little
>> more work to check that the memory window on the AHB bus fits the device
>> size. It will come later when support for direct mapping is added.
>>
>> Single and dual mode RX transfers are supported. Other types than SPI
>> are not supported.
>>
>> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   drivers/spi/spi-aspeed-smc.c            | 766 ++++++++++++++++++++++++
>>   drivers/mtd/spi-nor/controllers/Kconfig |   2 +-
>>   drivers/spi/Kconfig                     |  11 +
>>   drivers/spi/Makefile                    |   1 +
>>   4 files changed, 779 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/spi/spi-aspeed-smc.c
>>
> [...]
>> +
>> +/* support for 1-1-1, 1-1-2 or 1-1-4 */
>> +static bool aspeed_spi_supports_op(struct spi_mem *mem, const struct spi_mem_op *op)
>> +{
>> +	if (op->cmd.buswidth > 1)
>> +		return false;
>> +
>> +	if (op->addr.nbytes != 0) {
>> +		if (op->addr.buswidth > 1 || op->addr.nbytes > 4)
>> +			return false;
>> +	}
>> +
>> +	if (op->dummy.nbytes != 0) {
>> +		if (op->dummy.buswidth > 1 || op->dummy.nbytes > 7)
>> +			return false;
>> +	}
>> +
>> +	if (op->data.nbytes != 0 && op->data.buswidth > 4)
>> +		return false;
>> +
>> +	if (!spi_mem_default_supports_op(mem, op))
>> +		return false;
>> +
>> +	return true;
> 
> Nitpick: You can just do return spi_mem_default_supports_op(mem, op);
> 
>> +}
>> +
> [...]
>> +
>> +static int aspeed_spi_init_devices(struct platform_device *pdev, struct aspeed_spi *aspi)
>> +{
>> +	struct device_node *np;
>> +	unsigned int cs;
>> +	int ret;
>> +
>> +	for_each_available_child_of_node(aspi->dev->of_node, np) {
>> +		struct aspeed_spi_chip *chip;
>> +
>> +		if (!of_device_is_compatible(np, "jedec,spi-nor"))
>> +			continue;
>> +
>> +		ret = of_property_read_u32(np, "reg", &cs);
>> +		if (ret) {
>> +			dev_err(aspi->dev, "Couldn't not read chip select.\n");
>> +			of_node_put(np);
>> +			return ret;
>> +		}
>> +
>> +		if (cs > aspi->data->max_cs) {
>> +			dev_err(aspi->dev, "Chip select %d out of range.\n", cs);
>> +			of_node_put(np);
>> +			return -ERANGE;
>> +		}
>> +
>> +		chip = &aspi->chips[cs];
>> +		chip->aspi = aspi;
>> +		chip->cs = cs;
>> +
>> +		ret = aspeed_spi_chip_init(chip);
>> +		if (ret) {
>> +			of_node_put(np);
>> +			return ret;
>> +		}
>> +
>> +		if (of_property_read_u32(np, "spi-max-frequency", &chip->clk_freq))
>> +			chip->clk_freq = ASPEED_SPI_DEFAULT_FREQ;
>> +
>> +		aspi->num_cs++;
>> +	}
> 
> SPI MEM already gives you all this information. Get it from there, don't
> parse it yourself.

I agree for spi-max-frequency". It's even redundant with the setting
done in :

   [PATCH 04/10] spi: aspeed: Add support for direct mapping

> You can get Chip Select via spi_mem->spi->chip_select.

yes but we are still in the probing sequence and some initial settings
need to be done for each device before accessing them. See routine
aspeed_spi_chip_init().

I think a spi setup hook could do that. I will change in v2.

> You can get clock frequency via spi_mem->spi->max_speed_hz.
>
> With these comments fixed,
> 
> Acked-by: Pratyush Yadav <p.yadav@ti.com>
Please recheck v2.

Thanks,

C.


> 
>> +
>> +	return 0;
>> +}
>> +
> [...]
>
Cédric Le Goater Feb. 27, 2022, 6:50 p.m. UTC | #12
On 2/25/22 08:31, Pratyush Yadav wrote:
> On 14/02/22 10:42AM, Cédric Le Goater wrote:
>> To prepare transition to the new Aspeed SMC SPI controller driver using
>> the spi-mem interface, change the kernel CONFIG option of the current
>> driver to reflect that the implementation uses the MTD SPI-NOR interface.
>> Once the new driver is sufficiently exposed, we should remove the old one.
> 
> I don't quite understand the reasoning behind this. Why keep the old
> driver around? Why not directly replace it with the new one? Does the
> new one have any limitations that this one doesn't?

No. The old one has more limitations than the new one. The old one in
mainline is half baked since we could never merge the necessary bits
for training. We have been keeping a full version on the OpenBMC tree.

Joel, could we simply drop the old driver in mainline and keep the old
one in the OpenBMC tree until we feel comfortable ? I guess we need
more testing.

Thanks,

C.

>
Cédric Le Goater Feb. 27, 2022, 9:06 p.m. UTC | #13
On 2/25/22 10:12, Pratyush Yadav wrote:
> On 14/02/22 10:42AM, Cédric Le Goater wrote:
>> Use direct mapping to read the flash device contents. This operation
>> mode is called "Command mode" on Aspeed SoC SMC controllers. It uses a
>> Control Register for the settings to apply when a memory operation is
>> performed on the flash device mapping window.
>>
>> If the window is not big enough, fall back to the "User mode" to
>> perform the read.
>>
>> Direct mapping for writes will come later when validated.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   drivers/spi/spi-aspeed-smc.c | 67 ++++++++++++++++++++++++++++++++++--
>>   1 file changed, 65 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
>> index 0aeff6f468af..8d33fcb7736a 100644
>> --- a/drivers/spi/spi-aspeed-smc.c
>> +++ b/drivers/spi/spi-aspeed-smc.c
>> @@ -345,8 +345,8 @@ static int do_aspeed_spi_exec_op(struct spi_mem *mem, const struct spi_mem_op *o
>>   		if (!op->addr.nbytes)
>>   			ret = aspeed_spi_read_reg(chip, op);
>>   		else
>> -			ret = aspeed_spi_read_user(chip, op, op->addr.val,
>> -						   op->data.nbytes, op->data.buf.in);
>> +			memcpy_fromio(op->data.buf.in, chip->ahb_base + op->addr.val,
>> +				      op->data.nbytes);
> 
> Why change this? exec_op should be independent from dirmap APIs. And you
> don't even do the ahb_window_size checks here.

no indeed. Now that direct map is configured, all reads of flash contents
should go through the direct map op. This is mostly for the RDSFDP command
which has a different address space and uses 3B.

Theoretically, we should be able to use memcpy_fromio() and memcpy_toio()
for all commands but not all controllers (6 of them) support this mode.

Thanks,

C.


> 
>>   	} else {
>>   		if (!op->addr.nbytes)
>>   			ret = aspeed_spi_write_reg(chip, op);
>> @@ -426,10 +426,73 @@ static int aspeed_spi_chip_set_default_window(struct aspeed_spi_chip *chip)
>>   	return chip->ahb_window_size ? 0 : -1;
>>   }
>>   
>> +static int aspeed_spi_dirmap_create(struct spi_mem_dirmap_desc *desc)
>> +{
>> +	struct aspeed_spi *aspi = spi_controller_get_devdata(desc->mem->spi->master);
>> +	struct aspeed_spi_chip *chip = &aspi->chips[desc->mem->spi->chip_select];
>> +	struct spi_mem_op *op = &desc->info.op_tmpl;
>> +	u32 ctl_val;
>> +	int ret = 0;
>> +
>> +	chip->clk_freq = desc->mem->spi->max_speed_hz;
>> +
>> +	/* Only for reads */
>> +	if (op->data.dir != SPI_MEM_DATA_IN)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (desc->info.length > chip->ahb_window_size)
>> +		dev_warn(aspi->dev, "CE%d window (%dMB) too small for mapping",
>> +			 chip->cs, chip->ahb_window_size >> 20);
>> +
>> +	/* Define the default IO read settings */
>> +	ctl_val = readl(chip->ctl) & ~CTRL_IO_CMD_MASK;
>> +	ctl_val |= aspeed_spi_get_io_mode(op) |
>> +		op->cmd.opcode << CTRL_COMMAND_SHIFT |
>> +		CTRL_IO_DUMMY_SET(op->dummy.nbytes / op->dummy.buswidth) |
>> +		CTRL_IO_MODE_READ;
>> +
>> +	/* Tune 4BYTE address mode */
>> +	if (op->addr.nbytes) {
>> +		u32 addr_mode = readl(aspi->regs + CE_CTRL_REG);
>> +
>> +		if (op->addr.nbytes == 4)
>> +			addr_mode |= (0x11 << chip->cs);
>> +		else
>> +			addr_mode &= ~(0x11 << chip->cs);
>> +		writel(addr_mode, aspi->regs + CE_CTRL_REG);
>> +	}
>> +
>> +	/* READ mode is the controller default setting */
>> +	chip->ctl_val[ASPEED_SPI_READ] = ctl_val;
>> +	writel(chip->ctl_val[ASPEED_SPI_READ], chip->ctl);
>> +
>> +	dev_info(aspi->dev, "CE%d read buswidth:%d [0x%08x]\n",
>> +		 chip->cs, op->data.buswidth, chip->ctl_val[ASPEED_SPI_READ]);
>> +
>> +	return ret;
>> +}
>> +
>> +static int aspeed_spi_dirmap_read(struct spi_mem_dirmap_desc *desc,
>> +				  u64 offset, size_t len, void *buf)
>> +{
>> +	struct aspeed_spi *aspi = spi_controller_get_devdata(desc->mem->spi->master);
>> +	struct aspeed_spi_chip *chip = &aspi->chips[desc->mem->spi->chip_select];
>> +
>> +	/* Switch to USER command mode if mapping window is too small */
>> +	if (chip->ahb_window_size < offset + len)
>> +		aspeed_spi_read_user(chip, &desc->info.op_tmpl, offset, len, buf);
>> +	else
>> +		memcpy_fromio(buf, chip->ahb_base + offset, len);
>> +
>> +	return len;
>> +}
>> +
>>   static const struct spi_controller_mem_ops aspeed_spi_mem_ops = {
>>   	.supports_op = aspeed_spi_supports_op,
>>   	.exec_op = aspeed_spi_exec_op,
>>   	.get_name = aspeed_spi_get_name,
>> +	.dirmap_create = aspeed_spi_dirmap_create,
>> +	.dirmap_read = aspeed_spi_dirmap_read,
>>   };
>>   
>>   static void aspeed_spi_chip_set_type(struct aspeed_spi_chip *chip, int type)
>> -- 
>> 2.34.1
>>
>
Cédric Le Goater Feb. 27, 2022, 9:27 p.m. UTC | #14
On 2/25/22 10:18, Pratyush Yadav wrote:
> On 14/02/22 10:42AM, Cédric Le Goater wrote:
>> To accommodate the different response time of SPI transfers on different
>> boards and different SPI NOR devices, the Aspeed controllers provide a
>> set of Read Timing Compensation registers to tune the timing delays
>> depending on the frequency being used. The AST2600 SoC has one of
>> these registers per device. On the AST2500 and AST2400 SoCs, the
>> timing register is shared by all devices which is a bit problematic to
>> get good results other than for one device.
>>
>> The algorithm first reads a golden buffer at low speed and then performs
>> reads with different clocks and delay cycle settings to find a breaking
>> point. This selects a default good frequency for the CEx control register.
>> The current settings are bit optimistic as we pick the first delay giving
>> good results. A safer approach would be to determine an interval and
>> choose the middle value.
>>
>> Due to the lack of API, calibration is performed when the direct mapping
>> for reads is created.
> 
> The dirmap_create mapping says nothing about _when_ it should be called.
> So there is no guarantee that it will only be called after the flash is
> fully initialized. 

spi_nor_create_read_dirmap() is called after spi_nor_scan() in spi_nor_probe().
Since a spi_mem_dirmap_info descriptor is created using the nor fields :

	struct spi_mem_dirmap_info info = {
		.op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 0),
				      SPI_MEM_OP_ADDR(nor->addr_width, 0, 0),
				      SPI_MEM_OP_DUMMY(nor->read_dummy, 0),
				      SPI_MEM_OP_DATA_IN(0, NULL, 0)),
		.offset = 0,
		.length = nor->params->size,
	};
	struct spi_mem_op *op = &info.op_tmpl;

the spi-mem framework makes the assumption that the nor object is initialized.

> I suggest you either make this a requirement of the API,

how ?

Thanks,

C.


> or create a new API that guarantees it will only be called after
> the flash is initialized, like [0].
> 
> [0] https://patchwork.ozlabs.org/project/linux-mtd/patch/20210311191216.7363-2-p.yadav@ti.com/
> 
>>
>> Cc: Pratyush Yadav <p.yadav@ti.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>
Joel Stanley Feb. 28, 2022, 6:07 a.m. UTC | #15
On Sun, 27 Feb 2022 at 18:50, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 2/25/22 08:31, Pratyush Yadav wrote:
> > On 14/02/22 10:42AM, Cédric Le Goater wrote:
> >> To prepare transition to the new Aspeed SMC SPI controller driver using
> >> the spi-mem interface, change the kernel CONFIG option of the current
> >> driver to reflect that the implementation uses the MTD SPI-NOR interface.
> >> Once the new driver is sufficiently exposed, we should remove the old one.
> >
> > I don't quite understand the reasoning behind this. Why keep the old
> > driver around? Why not directly replace it with the new one? Does the
> > new one have any limitations that this one doesn't?
>
> No. The old one has more limitations than the new one. The old one in
> mainline is half baked since we could never merge the necessary bits
> for training. We have been keeping a full version on the OpenBMC tree.
>
> Joel, could we simply drop the old driver in mainline and keep the old
> one in the OpenBMC tree until we feel comfortable ? I guess we need
> more testing.

I would answer Pratyush's question with: the old one is well tested,
and the new one is not. We would intend to keep the old one around for
a release cycle or two, and once we're confident the new one is stable
we would remove the old.
Cédric Le Goater Feb. 28, 2022, 2:37 p.m. UTC | #16
On 2/28/22 07:07, Joel Stanley wrote:
> On Sun, 27 Feb 2022 at 18:50, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 2/25/22 08:31, Pratyush Yadav wrote:
>>> On 14/02/22 10:42AM, Cédric Le Goater wrote:
>>>> To prepare transition to the new Aspeed SMC SPI controller driver using
>>>> the spi-mem interface, change the kernel CONFIG option of the current
>>>> driver to reflect that the implementation uses the MTD SPI-NOR interface.
>>>> Once the new driver is sufficiently exposed, we should remove the old one.
>>>
>>> I don't quite understand the reasoning behind this. Why keep the old
>>> driver around? Why not directly replace it with the new one? Does the
>>> new one have any limitations that this one doesn't?
>>
>> No. The old one has more limitations than the new one. The old one in
>> mainline is half baked since we could never merge the necessary bits
>> for training. We have been keeping a full version in the OpenBMC tree.
>>
>> Joel, could we simply drop the old driver in mainline and keep the old
>> one in the OpenBMC tree until we feel comfortable ? I guess we need
>> more testing.
> 
> I would answer Pratyush's question with: the old one is well tested,
> and the new one is not. We would intend to keep the old one around for
> a release cycle or two, and once we're confident the new one is stable
> we would remove the old.

yes but we could handle the transition in the OpenBMC tree without putting
the burden on mainline.
  
mainline would only have the newer spi-mem based driver, the OpenBMC tree
would have it also, along with the older SPI-NOR based driver.

So this patch renaming the Kconfig option would only apply to the OpenBMC
tree.

C.