qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v1 3/7] hw/fsi: Introduce IBM's cfam,fsi-slave


From: Cédric Le Goater
Subject: Re: [PATCH v1 3/7] hw/fsi: Introduce IBM's cfam,fsi-slave
Date: Tue, 29 Aug 2023 15:43:43 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0

On 8/29/23 15:39, Ninad Palsule wrote:
Hello Thomas,

On 8/28/23 21:03, Thomas Huth wrote:
On 25/08/2023 22.30, Ninad Palsule wrote:
This is a part of patchset where IBM's Flexible Service Interface is
introduced.

The Common FRU Access Macro (CFAM), an address space containing
various "engines" that drive accesses on busses internal and external
to the POWER chip. Examples include the SBEFIFO and I2C masters. The
engines hang off of an internal Local Bus (LBUS) which is described
by the CFAM configuration block.

The FSI slave: The slave is the terminal point of the FSI bus for
FSI symbols addressed to it. Slaves can be cascaded off of one
another. The slave's configuration registers appear in address space
of the CFAM to which it is attached.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
...
diff --git a/hw/fsi/cfam.c b/hw/fsi/cfam.c
new file mode 100644
index 0000000000..19256050bd
--- /dev/null
+++ b/hw/fsi/cfam.c
@@ -0,0 +1,235 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2023 IBM Corp.
+ *
+ * IBM Common FRU Access Macro
+ */
+
+#include "qemu/osdep.h"
+
+#include "qapi/error.h"
+#include "qemu/log.h"
+
+#include "hw/fsi/bits.h"
+#include "hw/fsi/cfam.h"
+#include "hw/fsi/engine-scratchpad.h"
+
+#include "hw/qdev-properties.h"
+
+#define TO_REG(x)                          ((x) >> 2)
+
+#define CFAM_ENGINE_CONFIG                  TO_REG(0x04)
+
+#define CFAM_CONFIG_CHIP_ID                TO_REG(0x00)
+#define CFAM_CONFIG_CHIP_ID_P9             0xc0022d15
+#define   CFAM_CONFIG_CHIP_ID_BREAK        0xc0de0000
+
+static uint64_t cfam_config_read(void *opaque, hwaddr addr, unsigned size)
+{
+    CFAMConfig *config;
+    CFAMState *cfam;
+    LBusNode *node;
+    int i;
+
+    config = CFAM_CONFIG(opaque);
+    cfam = container_of(config, CFAMState, config);
+
+    qemu_log_mask(LOG_UNIMP, "%s: read @0x%" HWADDR_PRIx " size=%d\n",
+                  __func__, addr, size);
+
+    assert(size == 4);
+    assert(!(addr & 3));
+
+    switch (addr) {
+    case 0x00:
+        return CFAM_CONFIG_CHIP_ID_P9;
+    case 0x04:
+        return ENGINE_CONFIG_NEXT
+            | 0x00010000                    /* slots */
+            | 0x00001000                    /* version */
+            | ENGINE_CONFIG_TYPE_PEEK   /* type */
+            | 0x0000000c;                   /* crc */
+    case 0x08:
+        return ENGINE_CONFIG_NEXT
+            | 0x00010000                    /* slots */
+            | 0x00005000                    /* version */
+            | ENGINE_CONFIG_TYPE_FSI    /* type */
+            | 0x0000000a;                   /* crc */
+        break;
+    default:
+        /* FIXME: Improve this */
+        i = 0xc;
+        QLIST_FOREACH(node, &cfam->lbus.devices, next) {
+            if (i == addr) {
+                return LBUS_DEVICE_GET_CLASS(node->ldev)->config;
+            }
+            i += size;
+        }
+
+        if (i == addr) {
+            return 0;
+        }
+
+        return 0xc0de0000;

Can you explain the magic number at least with a comment?
Added comment for the magic number 0xc0de0000

Maybe it would also make sense to add a qemu_log_mask(LOG_GUEST_ERROR, ...) or 
qemu_log_mask(LOG_UNIMP, ...) statement here?
There is LOG_UNIMP most of the function. I added it in the reset function.

I took a quick look at the series and I think that all the
qemu_log_mask(LOG_UNIMP, ..) as the one above should be replaced
by trace events instead.

Thanks,

C.



 Thomas


Thank you for the review.

Ninad





reply via email to

[Prev in Thread] Current Thread [Next in Thread]