Hi Cedric,
> From: Cédric Le Goater <[email protected]>
> 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 <[email protected]>
> >> 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.
>
