[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 2/5] ast2400: add SMC controllers (FMC and SP
From: |
Cédric Le Goater |
Subject: |
Re: [Qemu-devel] [PATCH v3 2/5] ast2400: add SMC controllers (FMC and SPI) |
Date: |
Fri, 24 Jun 2016 15:30:08 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.1.0 |
On 06/23/2016 08:43 PM, Peter Maydell wrote:
> On 23 June 2016 at 17:33, Cédric Le Goater <address@hidden> wrote:
>> The Aspeed AST2400 soc includes a static memory controller for the BMC
>> which supports NOR, NAND and SPI flash memory modules. This controller
>> has two modes : the SMC for the legacy interface which supports only
>> one module and the FMC for the new interface which supports up to five
>> modules. The AST2400 also includes a SPI only controller used for the
>> host firmware, commonly called BIOS on Intel. It can be used in three
>> mode : a SPI master, SPI slave and SPI pass-through
>>
>> Below is the initial framework for the SMC controller (FMC mode only)
>> and the SPI controller: the sysbus object, MMIO for registers
>> configuration and controls. Each controller has a SPI bus and a
>> configurable number of CS lines for SPI flash slaves.
>>
>> The differences between the controllers are small, so they are
>> abstracted using indirections on the register numbers.
>>
>> Only SPI flash modules are supported.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>
>> Changes since v2:
>>
>> - switched to a realize ops to handle errors.
>>
>> hw/arm/ast2400.c | 31 +++++
>> hw/ssi/Makefile.objs | 1 +
>> hw/ssi/aspeed_smc.c | 298
>> ++++++++++++++++++++++++++++++++++++++++++++
>> include/hw/arm/ast2400.h | 3 +
>> include/hw/ssi/aspeed_smc.h | 79 ++++++++++++
>> 5 files changed, 412 insertions(+)
>> create mode 100644 hw/ssi/aspeed_smc.c
>> create mode 100644 include/hw/ssi/aspeed_smc.h
>>
>> diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
>> index 1a26d74e695c..c2665c0c6ead 100644
>> --- a/hw/arm/ast2400.c
>> +++ b/hw/arm/ast2400.c
>> @@ -23,6 +23,9 @@
>> #define AST2400_UART_5_BASE 0x00184000
>> #define AST2400_IOMEM_SIZE 0x00200000
>> #define AST2400_IOMEM_BASE 0x1E600000
>> +#define AST2400_SMC_BASE AST2400_IOMEM_BASE /* Legacy SMC */
>> +#define AST2400_FMC_BASE 0X1E620000
>> +#define AST2400_SPI_BASE 0X1E630000
>> #define AST2400_VIC_BASE 0x1E6C0000
>> #define AST2400_SCU_BASE 0x1E6E2000
>> #define AST2400_TIMER_BASE 0x1E782000
>> @@ -81,6 +84,14 @@ static void ast2400_init(Object *obj)
>> "hw-strap1", &error_abort);
>> object_property_add_alias(obj, "hw-strap2", OBJECT(&s->scu),
>> "hw-strap2", &error_abort);
>> +
>> + object_initialize(&s->smc, sizeof(s->smc), "aspeed.smc.fmc");
>> + object_property_add_child(obj, "smc", OBJECT(&s->smc), NULL);
>> + qdev_set_parent_bus(DEVICE(&s->smc), sysbus_get_default());
>> +
>> + object_initialize(&s->spi, sizeof(s->spi), "aspeed.smc.spi");
>> + object_property_add_child(obj, "spi", OBJECT(&s->spi), NULL);
>> + qdev_set_parent_bus(DEVICE(&s->spi), sysbus_get_default());
>> }
>>
>> static void ast2400_realize(DeviceState *dev, Error **errp)
>> @@ -143,6 +154,26 @@ static void ast2400_realize(DeviceState *dev, Error
>> **errp)
>> sysbus_mmio_map(SYS_BUS_DEVICE(&s->i2c), 0, AST2400_I2C_BASE);
>> sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c), 0,
>> qdev_get_gpio_in(DEVICE(&s->vic), 12));
>> +
>> + /* SMC */
>> + object_property_set_int(OBJECT(&s->smc), 1, "num-cs", &err);
>> + object_property_set_bool(OBJECT(&s->smc), true, "realized", &err);
>> + if (err) {
>> + error_propagate(errp, err);
>> + return;
>> + }
>
> You can't pass the same err to two functions in a row
> without checking it like this. See the comments on usage in
> include/qapi/error.h.
ah yes. this a copy paste without thinking ...
>> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->smc), 0, AST2400_FMC_BASE);
>> + sysbus_connect_irq(SYS_BUS_DEVICE(&s->smc), 0,
>> + qdev_get_gpio_in(DEVICE(&s->vic), 19));
>> +
>> + /* SPI */
>> + object_property_set_int(OBJECT(&s->spi), 1, "num-cs", &err);
>> + object_property_set_bool(OBJECT(&s->spi), true, "realized", &err);
>> + if (err) {
>> + error_propagate(errp, err);
>> + return;
>> + }
>> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 0, AST2400_SPI_BASE);
>> }
>>
>> static void ast2400_class_init(ObjectClass *oc, void *data)
>> diff --git a/hw/ssi/Makefile.objs b/hw/ssi/Makefile.objs
>> index fcbb79ef0185..c79a8dcd86a9 100644
>> --- a/hw/ssi/Makefile.objs
>> +++ b/hw/ssi/Makefile.objs
>> @@ -2,6 +2,7 @@ common-obj-$(CONFIG_PL022) += pl022.o
>> common-obj-$(CONFIG_SSI) += ssi.o
>> common-obj-$(CONFIG_XILINX_SPI) += xilinx_spi.o
>> common-obj-$(CONFIG_XILINX_SPIPS) += xilinx_spips.o
>> +common-obj-$(CONFIG_ASPEED_SOC) += aspeed_smc.o
>>
>> obj-$(CONFIG_OMAP) += omap_spi.o
>> obj-$(CONFIG_IMX) += imx_spi.o
>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>> new file mode 100644
>> index 000000000000..6b52123a67bb
>> --- /dev/null
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -0,0 +1,298 @@
>> +/*
>> + * ASPEED AST2400 SMC Controller (SPI Flash Only)
>> + *
>> + * Copyright (C) 2016 IBM Corp.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> copy
>> + * of this software and associated documentation files (the "Software"), to
>> deal
>> + * in the Software without restriction, including without limitation the
>> rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/sysbus.h"
>> +#include "sysemu/sysemu.h"
>> +#include "qemu/log.h"
>> +#include "include/qemu/error-report.h"
>> +#include "exec/address-spaces.h"
>> +
>> +#include "hw/ssi/aspeed_smc.h"
>> +
>> +/* CE Type Setting Register */
>> +#define R_CONF (0x00 / 4)
>> +#define CONF_LEGACY_DISABLE (1 << 31)
>> +#define CONF_ENABLE_W4 20
>> +#define CONF_ENABLE_W3 19
>> +#define CONF_ENABLE_W2 18
>> +#define CONF_ENABLE_W1 17
>> +#define CONF_ENABLE_W0 16
>> +#define CONF_FLASH_TYPE4 9
>> +#define CONF_FLASH_TYPE3 7
>> +#define CONF_FLASH_TYPE2 5
>> +#define CONF_FLASH_TYPE1 3
>> +#define CONF_FLASH_TYPE0 1
>> +
>> +/* CE Control Register */
>> +#define R_CE_CTRL (0x04 / 4)
>> +#define CRTL_EXTENDED4 4 /* 32 bit addressing for SPI */
>> +#define CRTL_EXTENDED3 3 /* 32 bit addressing for SPI */
>> +#define CRTL_EXTENDED2 2 /* 32 bit addressing for SPI */
>> +#define CRTL_EXTENDED1 1 /* 32 bit addressing for SPI */
>> +#define CRTL_EXTENDED0 0 /* 32 bit addressing for SPI */
>
> Presumably these should be CTRL, not CRTL.
CTRL.
>> +
>> +/* Interrupt Control and Status Register */
>> +#define R_INTR_CTRL (0x08 / 4)
>> +
>> +/* CEx Control Register */
>> +#define R_CTRL0 (0x10 / 4)
>> +#define CTRL_CMD_SHIFT 16
>> +#define CTRL_CMD_MASK 0xff
>> +#define CTRL_CE_STOP_ACTIVE (1 << 2)
>> +#define CTRL_CMD_MODE_MASK 0x3
>> +#define CTRL_READMODE 0x0
>> +#define CTRL_FREADMODE 0x1
>> +#define CTRL_WRITEMODE 0x2
>> +#define CTRL_USERMODE 0x3
>> +#define R_CTRL1 (0x14 / 4)
>> +#define R_CTRL2 (0x18 / 4)
>> +#define R_CTRL3 (0x1C / 4)
>> +#define R_CTRL4 (0x20 / 4)
>> +
>> +/* CEx Segment Address Register */
>> +#define R_SEG_ADDR0 (0x30 / 4)
>> +#define SEG_SIZE_SHIFT 24 /* 8MB units */
>> +#define SEG_SIZE_MASK 0x7f
>> +#define SEG_START_SHIFT 16 /* address bit [A29-A23] */
>> +#define SEG_START_MASK 0x7f
>> +#define R_SEG_ADDR1 (0x34 / 4)
>> +#define R_SEG_ADDR2 (0x38 / 4)
>> +#define R_SEG_ADDR3 (0x3C / 4)
>> +#define R_SEG_ADDR4 (0x40 / 4)
>> +
>> +/* Misc Control Register #1 */
>> +#define R_MISC_CRTL1 (0x50 / 4)
>
> Is this really spelt CRTL and not CTRL ?
This is really a CTRL :) I will fix that.
>> +
>> +/* Misc Control Register #2 */
>> +#define R_MISC_CRTL2 (0x54 / 4)
>> +
>> +/* Misc Control Register #2 */
>> +#define R_TIMINGS (0x94 / 4)
>> +
>> +/* SPI controller registers and bits */
>> +#define R_SPI_CONF (0x00 / 4)
>> +#define SPI_CONF_ENABLE_W0 0
>> +#define R_SPI_CTRL0 (0x4 / 4)
>> +#define R_SPI_MISC_CRTL (0x10 / 4)
>> +#define R_SPI_TIMINGS (0x14 / 4)
>> +
>> +static const AspeedSMCController controllers[] = {
>> + { "aspeed.smc.smc", R_CONF, R_CE_CTRL, R_CTRL0, R_TIMINGS,
>> + CONF_ENABLE_W0, 5 },
>> + { "aspeed.smc.fmc", R_CONF, R_CE_CTRL, R_CTRL0, R_TIMINGS,
>> + CONF_ENABLE_W0, 5 },
>> + { "aspeed.smc.spi", R_SPI_CONF, 0xff, R_SPI_CTRL0, R_SPI_TIMINGS,
>> + SPI_CONF_ENABLE_W0, 1 },
>> +};
>> +
>> +static bool aspeed_smc_is_ce_stop_active(AspeedSMCState *s, int cs)
>> +{
>> + return s->regs[s->r_ctrl0 + cs] & CTRL_CE_STOP_ACTIVE;
>> +}
>> +
>> +static void aspeed_smc_update_cs(AspeedSMCState *s)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < s->num_cs; ++i) {
>> + qemu_set_irq(s->cs_lines[i], aspeed_smc_is_ce_stop_active(s, i));
>> + }
>> +}
>> +
>> +static void aspeed_smc_reset(DeviceState *d)
>> +{
>> + AspeedSMCState *s = ASPEED_SMC(d);
>> + int i;
>> +
>> + memset(s->regs, 0, sizeof s->regs);
>> +
>> + /* Unselect all slaves */
>> + for (i = 0; i < s->num_cs; ++i) {
>> + s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE;
>> + }
>> +
>> + aspeed_smc_update_cs(s);
>> +}
>> +
>> +static bool aspeed_smc_is_implemented(AspeedSMCState *s, hwaddr addr)
>> +{
>> + return (addr == s->r_conf || addr == s->r_timings || addr ==
>> s->r_ce_ctrl ||
>> + (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs));
>> +}
>
> This is one of those borderline cases which is "this is an
> odd way to do this but not so odd that I'm going to ask you
> to change it" :-)
yes ... I am not that happy with it either :)
I don't think I will stand adding anything else to it without
finding another solution. As U-Boot is using some extra features,
it might not survive very long in that state.
>> +
>> +static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int
>> size)
>> +{
>> + AspeedSMCState *s = ASPEED_SMC(opaque);
>> +
>> + addr >>= 2;
>> +
>> + if (addr >= ARRAY_SIZE(s->regs)) {
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: Out-of-bounds read at 0x%" HWADDR_PRIx "\n",
>> + __func__, addr);
>> + return 0;
>> + }
>> +
>> + if (!aspeed_smc_is_implemented(s, addr)) {
>> + qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx
>> "\n",
>> + __func__, addr);
>> + return 0;
>> + }
>> +
>> + return s->regs[addr];
>> +}
>> +
>> +static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
>> + unsigned int size)
>> +{
>> + AspeedSMCState *s = ASPEED_SMC(opaque);
>> + uint32_t value = data;
>> +
>> + addr >>= 2;
>> +
>> + if (addr >= ARRAY_SIZE(s->regs)) {
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: Out-of-bounds write at 0x%" HWADDR_PRIx "\n",
>> + __func__, addr);
>> + return;
>> + }
>> +
>> + if (!aspeed_smc_is_implemented(s, addr)) {
>> + qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx
>> "\n",
>> + __func__, addr);
>> + return;
>> + }
>> +
>> + /*
>> + * Not much to do apart from storing the value and set the cs
>> + * lines if the register is a controlling one.
>> + */
>> + s->regs[addr] = value;
>> + if (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs) {
>> + aspeed_smc_update_cs(s);
>> + }
>> +}
>> +
>> +static const MemoryRegionOps aspeed_smc_ops = {
>> + .read = aspeed_smc_read,
>> + .write = aspeed_smc_write,
>> + .endianness = DEVICE_LITTLE_ENDIAN,
>> + .valid.unaligned = true,
>> +};
>> +
>> +static void aspeed_smc_realize(DeviceState *dev, Error **errp)
>> +{
>> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> + AspeedSMCState *s = ASPEED_SMC(dev);
>> + AspeedSMCClass *mc = ASPEED_SMC_GET_CLASS(s);
>> + int i;
>> +
>> + s->ctrl = mc->ctrl;
>> +
>> + /* keep a copy under AspeedSMCState to speed up accesses */
>> + s->r_conf = s->ctrl->r_conf;
>> + s->r_ce_ctrl = s->ctrl->r_ce_ctrl;
>> + s->r_ctrl0 = s->ctrl->r_ctrl0;
>> + s->r_timings = s->ctrl->r_timings;
>> + s->conf_enable_w0 = s->ctrl->conf_enable_w0;
>> +
>> + /* Enforce some real HW limits */
>> + if (s->num_cs > s->ctrl->max_slaves) {
>> + s->num_cs = s->ctrl->max_slaves;
>
> Should this be a "fail the realize" instead? (I don't know the
> hardware, so genuine question.)
The FMC controller supports up to 5, the SPI only one, but there
should be more on the ast2500 for the SPI and I would like to reuse
that code.
I added the 'num_cs' property to not create uselessly slaves and fit
how a platform is used in "real". For example, one flash module for
the BMC, one for the host or two flash modules for the BMC, the extra
being a gold image and one for the host.
I don't think it should fail but I could log some message if we are
going beyond the limits.
Thanks,
C.
>> + }
>> +
>> + s->spi = ssi_create_bus(dev, "spi");
>> +
>> + /* Setup cs_lines for slaves */
>> + sysbus_init_irq(sbd, &s->irq);
>> + s->cs_lines = g_new0(qemu_irq, s->num_cs);
>> + ssi_auto_connect_slaves(dev, s->cs_lines, s->spi);
>> +
>> + for (i = 0; i < s->num_cs; ++i) {
>> + sysbus_init_irq(sbd, &s->cs_lines[i]);
>> + }
>> +
>> + aspeed_smc_reset(dev);
>> +
>> + memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_smc_ops, s,
>> + s->ctrl->name, ASPEED_SMC_R_MAX * 4);
>> + sysbus_init_mmio(sbd, &s->mmio);
>> +}
>
> thanks
> -- PMM
>
- [Qemu-devel] [PATCH v3 0/5] ast2400: SMC controllers, Cédric Le Goater, 2016/06/23
- [Qemu-devel] [PATCH v3 1/5] m25p80: qdev-ify drive property, Cédric Le Goater, 2016/06/23
- [Qemu-devel] [PATCH v3 2/5] ast2400: add SMC controllers (FMC and SPI), Cédric Le Goater, 2016/06/23
- [Qemu-devel] [PATCH v3 3/5] ast2400: add SPI flash slave object, Cédric Le Goater, 2016/06/23
- [Qemu-devel] [PATCH v3 4/5] ast2400: create SPI flash slaves, Cédric Le Goater, 2016/06/23
- [Qemu-devel] [PATCH v3 5/5] tests: add a m25p80 test, Cédric Le Goater, 2016/06/23