AST2700 fmc/spi controller's address decoding unit is 64KB
and only bits [31:16] are used for decoding. Introduce seg_to_reg
and reg_to_seg handlers for ast2700 fmc/spi controller.
In addition, adds ast2700 fmc, spi0, spi1, and spi2 class init handler.
Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
---
hw/ssi/aspeed_smc.c | 222 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 220 insertions(+), 2 deletions(-)
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index df0c63469c..b4006c8339 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -185,7 +185,7 @@
* 0: 4 bytes
* 0x1FFFFFC: 32M bytes
*
- * DMA length is from 1 byte to 32MB (AST2600, AST10x0)
+ * DMA length is from 1 byte to 32MB (AST2600, AST10x0 and AST2700)
* 0: 1 byte
* 0x1FFFFFF: 32M bytes
*/
@@ -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,
},
};
@@ -1926,6 +1926,220 @@ static const TypeInfo aspeed_1030_spi2_info = {
.class_init = aspeed_1030_spi2_class_init,
};
+/*
+ * The FMC Segment Registers of the AST2700 have a 64KB unit.
+ * Only bits [31:16] are used for decoding.
+ */
+#define AST2700_SEG_ADDR_MASK 0xffff0000
+
+static uint32_t aspeed_2700_smc_segment_to_reg(const AspeedSMCState *s,
+ const AspeedSegments *seg)
+{
+ uint32_t reg = 0;
+
+ /* Disabled segments have a nil register */
+ if (!seg->size) {
+ return 0;
+ }
+
+ reg |= (seg->addr & AST2700_SEG_ADDR_MASK) >> 16; /* start offset */
+ reg |= (seg->addr + seg->size - 1) & AST2700_SEG_ADDR_MASK; /* end offset */
+ return reg;
+}
+
+static void aspeed_2700_smc_reg_to_segment(const AspeedSMCState *s,
+ uint32_t reg, AspeedSegments *seg)
+{
+ uint32_t start_offset = (reg << 16) & AST2700_SEG_ADDR_MASK;
+ uint32_t end_offset = reg & AST2700_SEG_ADDR_MASK;
+ AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
+
+ if (reg) {
+ seg->addr = asc->flash_window_base + start_offset;
+ seg->size = end_offset + (64 * KiB) - start_offset;
+ } else {
+ seg->addr = asc->flash_window_base;
+ seg->size = 0;
+ }
+}
+
+static const uint32_t aspeed_2700_fmc_resets[ASPEED_SMC_R_MAX] = {
+ [R_CONF] = (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0 |
+ CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE1),
+ [R_CE_CTRL] = 0x0000aa00,
+ [R_CTRL0] = 0x406b0641,
+ [R_CTRL1] = 0x00000400,
+ [R_CTRL2] = 0x00000400,
+ [R_CTRL3] = 0x00000400,
+ [R_SEG_ADDR0] = 0x08000000,
+ [R_SEG_ADDR1] = 0x10000800,
+ [R_SEG_ADDR2] = 0x00000000,
+ [R_SEG_ADDR3] = 0x00000000,
+ [R_DUMMY_DATA] = 0x00010000,
+ [R_DMA_DRAM_ADDR_HIGH] = 0x00000000,
+ [R_TIMINGS] = 0x007b0000,
+};
+
+static const AspeedSegments aspeed_2700_fmc_segments[] = {
+ { 0x0, 128 * MiB }, /* start address is readonly */
+ { 128 * MiB, 128 * MiB }, /* default is disabled but needed for -kernel */
+ { 256 * MiB, 128 * MiB }, /* default is disabled but needed for -kernel */
+ { 0x0, 0 }, /* disabled */
+};
+
+static void aspeed_2700_fmc_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ AspeedSMCClass *asc = ASPEED_SMC_CLASS(klass);
+
+ dc->desc = "Aspeed 2700 FMC Controller";
+ asc->r_conf = R_CONF;
+ asc->r_ce_ctrl = R_CE_CTRL;
+ asc->r_ctrl0 = R_CTRL0;
+ asc->r_timings = R_TIMINGS;
+ asc->nregs_timings = 3;
+ asc->conf_enable_w0 = CONF_ENABLE_W0;
+ asc->cs_num_max = 3;
+ asc->segments = aspeed_2700_fmc_segments;
+ asc->segment_addr_mask = 0xffffffff;
+ asc->resets = aspeed_2700_fmc_resets;
+ asc->flash_window_base = 0x100000000;
+ asc->flash_window_size = 1 * GiB;
+ asc->features = ASPEED_SMC_FEATURE_DMA |
+ ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH;
+ asc->dma_flash_mask = 0x2FFFFFFC;
+ asc->dma_dram_mask = 0xFFFFFFFC;
+ asc->dma_start_length = 1;
+ asc->nregs = ASPEED_SMC_R_MAX;
+ asc->segment_to_reg = aspeed_2700_smc_segment_to_reg;
+ asc->reg_to_segment = aspeed_2700_smc_reg_to_segment;
+ asc->dma_ctrl = aspeed_2600_smc_dma_ctrl;
+}
+
+static const TypeInfo aspeed_2700_fmc_info = {
+ .name = "aspeed.fmc-ast2700",
+ .parent = TYPE_ASPEED_SMC,
+ .class_init = aspeed_2700_fmc_class_init,
+};
+
+static const AspeedSegments aspeed_2700_spi0_segments[] = {
+ { 0x0, 128 * MiB }, /* start address is readonly */
+ { 128 * MiB, 128 * MiB }, /* start address is readonly */
+ { 0x0, 0 }, /* disabled */
+};
+
+static void aspeed_2700_spi0_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ AspeedSMCClass *asc = ASPEED_SMC_CLASS(klass);
+
+ dc->desc = "Aspeed 2700 SPI0 Controller";
+ asc->r_conf = R_CONF;
+ asc->r_ce_ctrl = R_CE_CTRL;
+ asc->r_ctrl0 = R_CTRL0;
+ asc->r_timings = R_TIMINGS;
+ asc->nregs_timings = 2;
+ asc->conf_enable_w0 = CONF_ENABLE_W0;
+ asc->cs_num_max = 2;
+ asc->segments = aspeed_2700_spi0_segments;
+ asc->segment_addr_mask = 0xffffffff;
+ asc->flash_window_base = 0x180000000;
+ asc->flash_window_size = 1 * GiB;
+ asc->features = ASPEED_SMC_FEATURE_DMA |
+ ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH;
+ asc->dma_flash_mask = 0x2FFFFFFC;
+ asc->dma_dram_mask = 0xFFFFFFFC;
+ asc->dma_start_length = 1;
+ asc->nregs = ASPEED_SMC_R_MAX;
+ asc->segment_to_reg = aspeed_2700_smc_segment_to_reg;
+ asc->reg_to_segment = aspeed_2700_smc_reg_to_segment;
+ asc->dma_ctrl = aspeed_2600_smc_dma_ctrl;
+}
+
+static const TypeInfo aspeed_2700_spi0_info = {
+ .name = "aspeed.spi0-ast2700",
+ .parent = TYPE_ASPEED_SMC,
+ .class_init = aspeed_2700_spi0_class_init,
+};
+
+static const AspeedSegments aspeed_2700_spi1_segments[] = {
+ { 0x0, 128 * MiB }, /* start address is readonly */
+ { 0x0, 0 }, /* disabled */
+};
+
+static void aspeed_2700_spi1_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ AspeedSMCClass *asc = ASPEED_SMC_CLASS(klass);
+
+ dc->desc = "Aspeed 2700 SPI1 Controller";
+ asc->r_conf = R_CONF;
+ asc->r_ce_ctrl = R_CE_CTRL;
+ asc->r_ctrl0 = R_CTRL0;
+ asc->r_timings = R_TIMINGS;
+ asc->nregs_timings = 2;
+ asc->conf_enable_w0 = CONF_ENABLE_W0;
+ asc->cs_num_max = 2;
+ asc->segments = aspeed_2700_spi1_segments;
+ asc->segment_addr_mask = 0xffffffff;
+ asc->flash_window_base = 0x200000000;
+ asc->flash_window_size = 1 * GiB;
+ asc->features = ASPEED_SMC_FEATURE_DMA |
+ ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH;
+ asc->dma_flash_mask = 0x2FFFFFFC;
+ asc->dma_dram_mask = 0xFFFFFFFC;
+ asc->dma_start_length = 1;
+ asc->nregs = ASPEED_SMC_R_MAX;
+ asc->segment_to_reg = aspeed_2700_smc_segment_to_reg;
+ asc->reg_to_segment = aspeed_2700_smc_reg_to_segment;
+ asc->dma_ctrl = aspeed_2600_smc_dma_ctrl;
+}
+
+static const TypeInfo aspeed_2700_spi1_info = {
+ .name = "aspeed.spi1-ast2700",
+ .parent = TYPE_ASPEED_SMC,
+ .class_init = aspeed_2700_spi1_class_init,
+};
+
+static const AspeedSegments aspeed_2700_spi2_segments[] = {
+ { 0x0, 128 * MiB }, /* start address is readonly */
+ { 0x0, 0 }, /* disabled */
+};
+
+static void aspeed_2700_spi2_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ AspeedSMCClass *asc = ASPEED_SMC_CLASS(klass);
+
+ dc->desc = "Aspeed 2700 SPI2 Controller";
+ asc->r_conf = R_CONF;
+ asc->r_ce_ctrl = R_CE_CTRL;
+ asc->r_ctrl0 = R_CTRL0;
+ asc->r_timings = R_TIMINGS;
+ asc->nregs_timings = 2;
+ asc->conf_enable_w0 = CONF_ENABLE_W0;
+ asc->cs_num_max = 2;
+ asc->segments = aspeed_2700_spi2_segments;
+ asc->segment_addr_mask = 0xffffffff;
+ asc->flash_window_base = 0x280000000;
+ asc->flash_window_size = 1 * GiB;
+ asc->features = ASPEED_SMC_FEATURE_DMA |
+ ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH;
+ asc->dma_flash_mask = 0x0FFFFFFC;
+ asc->dma_dram_mask = 0xFFFFFFFC;
+ asc->dma_start_length = 1;
+ asc->nregs = ASPEED_SMC_R_MAX;
+ asc->segment_to_reg = aspeed_2700_smc_segment_to_reg;
+ asc->reg_to_segment = aspeed_2700_smc_reg_to_segment;
+ asc->dma_ctrl = aspeed_2600_smc_dma_ctrl;
+}
+
+static const TypeInfo aspeed_2700_spi2_info = {
+ .name = "aspeed.spi2-ast2700",
+ .parent = TYPE_ASPEED_SMC,
+ .class_init = aspeed_2700_spi2_class_init,
+};
+
static void aspeed_smc_register_types(void)
{
type_register_static(&aspeed_smc_flash_info);
@@ -1942,6 +2156,10 @@ static void aspeed_smc_register_types(void)
type_register_static(&aspeed_1030_fmc_info);
type_register_static(&aspeed_1030_spi1_info);
type_register_static(&aspeed_1030_spi2_info);
+ type_register_static(&aspeed_2700_fmc_info);
+ type_register_static(&aspeed_2700_spi0_info);
+ type_register_static(&aspeed_2700_spi1_info);
+ type_register_static(&aspeed_2700_spi2_info);
}
type_init(aspeed_smc_register_types)
--
2.25.1
Hi,
On 27/5/24 10:02, Jamin Lin wrote:
> AST2700 fmc/spi controller's address decoding unit is 64KB
> and only bits [31:16] are used for decoding. Introduce seg_to_reg
> and reg_to_seg handlers for ast2700 fmc/spi controller.
> In addition, adds ast2700 fmc, spi0, spi1, and spi2 class init handler.
>
> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/ssi/aspeed_smc.c | 222 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 220 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index df0c63469c..b4006c8339 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -185,7 +185,7 @@
> * 0: 4 bytes
> * 0x1FFFFFC: 32M bytes
> *
> - * DMA length is from 1 byte to 32MB (AST2600, AST10x0)
> + * DMA length is from 1 byte to 32MB (AST2600, AST10x0 and AST2700)
> * 0: 1 byte
> * 0x1FFFFFF: 32M bytes
> */
> @@ -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.
Thanks,
Phil.
> },
> };
On 5/27/24 17:58, Philippe Mathieu-Daudé wrote:
> Hi,
>
> On 27/5/24 10:02, Jamin Lin wrote:
>> AST2700 fmc/spi controller's address decoding unit is 64KB
>> and only bits [31:16] are used for decoding. Introduce seg_to_reg
>> and reg_to_seg handlers for ast2700 fmc/spi controller.
>> In addition, adds ast2700 fmc, spi0, spi1, and spi2 class init handler.
>>
>> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> hw/ssi/aspeed_smc.c | 222 +++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 220 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>> index df0c63469c..b4006c8339 100644
>> --- a/hw/ssi/aspeed_smc.c
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -185,7 +185,7 @@
>> * 0: 4 bytes
>> * 0x1FFFFFC: 32M bytes
>> *
>> - * DMA length is from 1 byte to 32MB (AST2600, AST10x0)
>> + * DMA length is from 1 byte to 32MB (AST2600, AST10x0 and AST2700)
>> * 0: 1 byte
>> * 0x1FFFFFF: 32M bytes
>> */
>> @@ -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.
Ah I missed that. I wonder how we could set different access width
tough on the model ?
Should we allocate a MemoryRegionOps in the realize() handler and set
the width depending on the SoC ?
Thanks,
C.
Hi Cedric, Philippe
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Tuesday, May 28, 2024 3:03 PM
> To: Philippe Mathieu-Daudé <philmd@linaro.org>; Jamin Lin
> <jamin_lin@aspeedtech.com>; Peter Maydell <peter.maydell@linaro.org>;
> Andrew Jeffery <andrew@codeconstruct.com.au>; Joel Stanley
> <joel@jms.id.au>; Alistair Francis <alistair@alistair23.me>; Cleber Rosa
> <crosa@redhat.com>; Wainer dos Santos Moschetta <wainersm@redhat.com>;
> Beraldo Leal <bleal@redhat.com>; open list:ASPEED BMCs
> <qemu-arm@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_lee@aspeedtech.com>; Yunlin Tang
> <yunlin.tang@aspeedtech.com>
> Subject: Re: [SPAM] Re: [PATCH v4 09/16] aspeed/smc: Add AST2700 support
>
> On 5/27/24 17:58, Philippe Mathieu-Daudé wrote:
> > Hi,
> >
> > On 27/5/24 10:02, Jamin Lin wrote:
> >> AST2700 fmc/spi controller's address decoding unit is 64KB and only
> >> bits [31:16] are used for decoding. Introduce seg_to_reg and
> >> reg_to_seg handlers for ast2700 fmc/spi controller.
> >> In addition, adds ast2700 fmc, spi0, spi1, and spi2 class init handler.
> >>
> >> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> >> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> >> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >> hw/ssi/aspeed_smc.c | 222
> >> +++++++++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 220 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index
> >> df0c63469c..b4006c8339 100644
> >> --- a/hw/ssi/aspeed_smc.c
> >> +++ b/hw/ssi/aspeed_smc.c
> >> @@ -185,7 +185,7 @@
> >> * 0: 4 bytes
> >> * 0x1FFFFFC: 32M bytes
> >> *
> >> - * DMA length is from 1 byte to 32MB (AST2600, AST10x0)
> >> + * DMA length is from 1 byte to 32MB (AST2600, AST10x0 and AST2700)
> >> * 0: 1 byte
> >> * 0x1FFFFFF: 32M bytes
> >> */
> >> @@ -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/1062a07420f9aed4ed7dc9deb3429b8e7828f5cf/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/arm64/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/arm/kernel/io.c#L45
If you have any suggestion about this patch modification, please let me know.
I am going to re-send v5 patch for AST2700 support.
Thanks-Jamin
> Ah I missed that. I wonder how we could set different access width tough on
> the model ?
>
> Should we allocate a MemoryRegionOps in the realize() handler and set the
> width depending on the SoC ?
>
>
> Thanks,
>
> C.
>
>
>
>>>> @@ -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/1062a07420f9aed4ed7dc9deb3429b8e7828f5cf/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/arm64/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/arm/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.
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
/*
* Use the default segment value to size the memory region. This
* can be changed by FW at runtime.
*/
memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_smc_flash_ops,
s, name, s->asc->segments[s->cs].size);
------
}
Thanks-Jamin
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.
Hi Cedric,
> From: Cédric Le Goater <clg@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 <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/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.
>
> 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);
> }
LGTM,
Thanks,
C.
© 2016 - 2025 Red Hat, Inc.