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

["Cédric Le Goater" <clg@kaod.org>]

On 6/3/24 11:49, Jamin Lin wrote:
> Hi Cedric,
> 
>> From: Cédric Le Goater <clg@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.