Re: [SPAM] Re: [PATCH v4 09/16] aspeed/smc: Add AST2700 support

[Cédric Le Goater]

On 6/3/24 11:49, Jamin Lin wrote:

Hi Cedric,

From: Cédric Le Goater <c...@kaod.org>
Subject: Re: [SPAM] Re: [PATCH v4 09/16] aspeed/smc: Add AST2700 support

@@ -670,7 +670,7 @@ static const MemoryRegionOps

aspeed_smc_flash_ops

= {
        .endianness = DEVICE_LITTLE_ENDIAN,
        .valid = {
            .min_access_size = 1,
-        .max_access_size = 4,
+        .max_access_size = 8,

Is this a bugfix? If so, please use a separate patch. Otherwise
please mention why it is OK to widen access for AST2600 & AST10x0.

According the design of SPI drivers, it uses this "memcpy_fromio" KERNEL API

for SPI calibration.

https://github.com/AspeedTech-BMC/linux/blob/1062a07420f9aed4ed7dc9deb

3429b8e7828f5cf/drivers/spi/spi-aspeed-smc.c#L1832
AST2700 is a 64 bits quad core cpus(Cortex-a35), so kernel API use 64 bits for

data access.

https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/a
rm64/kernel/io.c#L25 I simply set the max_access_size to 8 for AST2700
support.
AST2500, AST2600 and AST10x0 are all 32bits CPUS, that was why this

max_access_size 8 did not impact these models.

https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/a
rm/kernel/io.c#L45

Yes. I think we are safe on that side.

If you have any suggestion about this patch modification, please let me know.
I am going to re-send v5 patch for AST2700 support.

Please move this change in its own commit explaining the reason and add a
TODO comment in the code.

The aspeed_smc_flash_ops MemoryRegionOps should be copied in _realize()
to set a different width for the AST2700 SoC. You could do that too.

Thanks,

C.

I will do the following changes. Could you give me any suggestion?

1. add asc->max_access_size = 8 in aspeed_2700_fmc_class_init, 
aspeed_2700_spi0_class_init, aspeed_2700_spi1_class_init and 
aspeed_2700_spi2_class_init
2. Update aspeed_smc_flash_realize as below
static void aspeed_smc_flash_realize(DeviceState *dev, Error **errp)
{
    ------
    s->asc = ASPEED_SMC_GET_CLASS(s->controller)
    if (s->asc->max_access_size ==8) --> check max_access_size
       aspeed_smc_flash_ops.valid.max_access_size = s->asc->max_access --> 
update max_access_size

You can not because aspeed_smc_flash_ops is a static const shared
by all models

Best option is to introduce a new 'const MemoryRegionOps*' attribute
in AspeedSMCClass and use it in aspeed_smc_flash_realize().


Thanks,

C.