[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/2] ppc/pnv: Add an I2C controller model
|
From: |
Cédric Le Goater |
|
Subject: |
Re: [PATCH v2 1/2] ppc/pnv: Add an I2C controller model |
|
Date: |
Tue, 17 Oct 2023 08:57:18 +0200 |
|
User-agent: |
Mozilla Thunderbird |
On 10/16/23 22:55, Miles Glenn wrote:
On Fri, 2023-10-13 at 10:58 +0200, Philippe Mathieu-Daudé wrote:
Hi Glenn, Cédric,
On 12/10/23 22:08, Glenn Miles wrote:
From: Cédric Le Goater <clg@kaod.org>
The more recent IBM power processors have an embedded I2C
controller that is accessible by software via the XSCOM
address space.
Each instance of the I2C controller is capable of controlling
multiple I2C buses (one at a time). Prior to beginning a
transaction on an I2C bus, the bus must be selected by writing
the port number associated with the bus into the PORT_NUM
field of the MODE register. Once an I2C bus is selected,
the status of the bus can be determined by reading the
Status and Extended Status registers.
I2C bus transactions can be started by writing a command to
the Command register and reading/writing data from/to the
FIFO register.
Not supported :
. 10 bit I2C addresses
. Multimaster
. Slave
Signed-off-by: Cédric Le Goater <clg@kaod.org>
[milesg: Split wiring to powernv9 into its own commit]
[milesg: Added more detail to commit message]
[milesg: Added SPDX Licensed Identifier to new files]
[milesg: updated copyright dates]
[milesg: Added use of g_autofree]
Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
---
Changes in v2:
- Updated copyright dates
- Removed copyright paragraph (replaced by SPDX-License-
Identifier)
- Added use of g_autofree
hw/ppc/meson.build | 1 +
hw/ppc/pnv_i2c.c | 673
+++++++++++++++++++++++++++++++++++++
include/hw/ppc/pnv_i2c.h | 38 +++
include/hw/ppc/pnv_xscom.h | 3 +
4 files changed, 715 insertions(+)
create mode 100644 hw/ppc/pnv_i2c.c
create mode 100644 include/hw/ppc/pnv_i2c.h
+/* I2C mode register */
+#define I2C_MODE_REG 0x6
+#define I2C_MODE_BIT_RATE_DIV PPC_BITMASK(0, 15)
+#define I2C_MODE_PORT_NUM PPC_BITMASK(16, 21)
+#define I2C_MODE_ENHANCED PPC_BIT(28)
+#define I2C_MODE_DIAGNOSTIC PPC_BIT(29)
+#define I2C_MODE_PACING_ALLOW PPC_BIT(30)
+#define I2C_MODE_WRAP PPC_BIT(31)
+static I2CBus *pnv_i2c_get_bus(PnvI2C *i2c)
+{
+ uint8_t port = GETFIELD(I2C_MODE_PORT_NUM, i2c-
regs[I2C_MODE_REG]);
+
+ if (port >= i2c->num_busses) {
Can we sanitize in pnv_i2c_xscom_write() instead ...?
Hi Phil,
Thanks for taking a look. You have a good question! I tried reading
through the spec that I have in order to see how the hardware reacts to
an invalid port number being written to the mode register and didn't
find anything obvious.
If we did what you suggest, how do we prevent attempts from accessing
an invalid bus? The options I see are:
1) Do an assert on the port value being valid so invalid port values
result in the process dieing. Uses the big hammer, but simple to
implement and exposes problems early on.
2) Fail the xscom write. There doesn't seem to be an easy way to
notify software of the xscom write failure, and xscom write failures
probably shouldn't happen as long as we are writing to a correct xscom
address.
HW would probably set FIR bits in the various impacted logics
to track/report the error and raise an HMI.
This is badly implemented in QEMU :
static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
{
/*
* TODO: When the read/write comes from the monitor, NULL is
* passed for the cpu, and no CPU completion is generated.
*/
if (cs) {
PowerPCCPU *cpu = POWERPC_CPU(cs);
CPUPPCState *env = &cpu->env;
/*
* TODO: Need a CPU helper to set HMER, also handle generation
* of HMIs
*/
cpu_synchronize_state(cs);
env->spr[SPR_HMER] |= hmer_bits;
}
}
3) Allow the xscom write, but prevent writing an invalid port value.
Simple, but hides the failure and will probably lead to less obvious
failures down the road.
Do you have other suggestions? Honestly, I don't like any of these
options, but if I had to pick one, I'd probably choose option 1. Even
though it's the sledge hammer approach, it prevents access of an
invalid bus, and exposes the error early on.
Cedric's approach allows for the invalid port values to be written to
the register which is, IMHO, most likely what the hardware would do.
However, it does look like there are some areas where we do not handle
getting a NULL pointer from pnv_i2c_get_bus() correctly. For example,
i2c_bus_busy() doesn't check for NULL and yet we are passing the return
value of pnv_i2c_get_bus into that function without checking it.
So, after looking into this a bit, I think I prefer Cedric's approach
with the addition of auditing all of the places where pnv_i2c_get_bus()
is called and handling the NULL case better.
Of course, I may have missed something and look forward to other
suggestions as well.
It is difficult to be that precise in the models. Reporting in QEMU
is good enough unless you are interested in modeling failures.
The skiboot FW being close to HW it would be interesting. You would
need mechanisms to inject errors. Nick proposed an mce interface
some years ago on that topic.
Thanks,
C.
Thanks,
Glenn
+ qemu_log_mask(LOG_GUEST_ERROR, "I2C: invalid bus number
%d/%d\n", port,
+ i2c->num_busses);
+ return NULL;
+ }
+ return i2c->busses[port];
+}
+static void pnv_i2c_xscom_write(void *opaque, hwaddr addr,
+ uint64_t val, unsigned size)
+{
+ PnvI2C *i2c = PNV_I2C(opaque);
+ uint32_t offset = addr >> 3;
+
+ switch (offset) {
+ case I2C_MODE_REG:
... here?
+ i2c->regs[offset] = val;
+ if (i2c_bus_busy(pnv_i2c_get_bus(i2c))) {
+ i2c->regs[I2C_STAT_REG] |= I2C_STAT_INVALID_CMD;
+ qemu_log_mask(LOG_GUEST_ERROR, "I2C: command in
progress\n");
+ }
+ break;
[...]
Regards,
Phil.