Hi Cedric, > From: Cédric Le Goater <c...@kaod.org> > Subject: Re: [SPAM] Re: [PATCH v4 09/16] aspeed/smc: Add AST2700 support > > 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/1062a07420f9aed4ed7dc9de > >> b > >>> 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 for your suggestion. How about these changes?
1. aspeed_smc.h struct AspeedSMCClass { const MemoryRegionOps *reg_ops; } 2. aspeed_smc.c a. create new memory region opts for ast2700 static const MemoryRegionOps aspeed_2700_smc_flash_ops = { .read = aspeed_smc_flash_read, .write = aspeed_smc_flash_write, .endianness = DEVICE_LITTLE_ENDIAN, .valid = { .min_access_size = 1, .max_access_size = 8, }, }; b. set memory region opts in all model class init static void aspeed_2400_smc_class_init(ObjectClass *klass, void *data){ asc->reg_ops = &aspeed_smc_flash_ops; } static void aspeed_2400_fmc_class_init (ObjectClass *klass, void *data){ asc->reg_ops = &aspeed_smc_flash_ops; } static void aspeed_2400_spi1_class_init (ObjectClass *klass, void *data){ asc->reg_ops = &aspeed_smc_flash_ops; } static void aspeed_2500_fmc_class_init (ObjectClass *klass, void *data){ asc->reg_ops = &aspeed_smc_flash_ops; } static void aspeed_2500_spi1_class_init (ObjectClass *klass, void *data){ asc->reg_ops = &aspeed_smc_flash_ops; } static void aspeed_2500_spi2_class_init (ObjectClass *klass, void *data){ asc->reg_ops = &aspeed_smc_flash_ops; } static void aspeed_2600_fmc_class_init (ObjectClass *klass, void *data){ asc->reg_ops = &aspeed_smc_flash_ops; } static void aspeed_2600_spi1_class_init (ObjectClass *klass, void *data){ asc->reg_ops = &aspeed_smc_flash_ops; } static void aspeed_2600_spi2_class_init (ObjectClass *klass, void *data){ asc->reg_ops = &aspeed_smc_flash_ops; } static void aspeed_1030_fmc_class_init (ObjectClass *klass, void *data){ asc->reg_ops = &aspeed_smc_flash_ops; } static void aspeed_1030_spi1_class_init (ObjectClass *klass, void *data){ asc->reg_ops = &aspeed_smc_flash_ops; } static void aspeed_1030_spi2_class_init (ObjectClass *klass, void *data){ asc->reg_ops = &aspeed_smc_flash_ops; } static void aspeed_2700_fmc_class_init(ObjectClass *klass, void *data) { asc->reg_ops = &aspeed_2700_smc_flash_ops; } static void aspeed_2700_spi0_class_init (ObjectClass *klass, void *data) { asc->reg_ops = &aspeed_2700_smc_flash_ops; } static void aspeed_2700_spi1_class_init (ObjectClass *klass, void *data) { asc->reg_ops = &aspeed_2700_smc_flash_ops; } static void aspeed_2700_spi2_class_init (ObjectClass *klass, void *data) { asc->reg_ops = &aspeed_2700_smc_flash_ops; } c. update realize to use memory region opts from class reg_opts static void aspeed_smc_flash_realize(DeviceState *dev, Error **errp) { memory_region_init_io(&s->mmio, OBJECT(s), s->asc->reg_ops, s, name, s->asc->segments[s->cs].size); } Thanks-Jamin > > Thanks, > > C. >