[Pdbg] [RFC] Detect if we probed the target correctly
Balbir Singh
bsingharora at gmail.com
Tue May 22 18:41:43 AEST 2018
The patch adds detection of probe being called prior to calling
read and write functions on the target. The idea being that
we should not invoke these functions on unprobed targets.
By default the status is set ot PDBG_TARGET_UNKNOWN
and if the status of the target is UNKNOWN at the time of probe
except for when it's being probed (Alistair pointed out that
hmfsi probe routine for example calls its read and write routines
in the operation of probing), a new status
PDBG_TARGET_BEING_PROBED is added for detection of such
recursion.
All of this is done using macro's, the side-effect is that
the namespace has been cleaned up to match the target structure.
New DECLARE_HW_* and HW_UNIT_FN_NAME macros have been added
TODOs
1. Allow for auto-probe in the future
2. Extend this infrastructure to non-read/write functions
of targets (stop/reset/etc)
3. Cleanup to reduce the number of macros and clean up
for macro size.
Signed-off-by: Balbir Singh <bsingharora at gmail.com>
---
libpdbg/bmcfsi.c | 8 +++++--
libpdbg/cfam.c | 70 ++++++++++++++++++++++++++++++++++---------------------
libpdbg/i2c.c | 16 ++++++++-----
libpdbg/kernel.c | 19 ++++++++-------
libpdbg/libpdbg.h | 2 +-
libpdbg/target.c | 7 ++++++
libpdbg/target.h | 24 +++++++++++++++++++
7 files changed, 103 insertions(+), 43 deletions(-)
diff --git a/libpdbg/bmcfsi.c b/libpdbg/bmcfsi.c
index b57e029..74c247d 100644
--- a/libpdbg/bmcfsi.c
+++ b/libpdbg/bmcfsi.c
@@ -492,15 +492,19 @@ int bmcfsi_probe(struct pdbg_target *target)
return 0;
}
+DECLARE_HW_UNIT_READ_FN(fsi, getcfam, uint32_t);
+DECLARE_HW_UNIT_WRITE_FN(fsi, putcfam, uint32_t);
+
static struct fsi bmcfsi = {
.target = {
.name = "BMC GPIO bit-banging FSI master",
.compatible = "ibm,bmcfsi",
.class = "fsi",
.probe = bmcfsi_probe,
+ .status = PDBG_TARGET_UNKNOWN,
},
- .read = fsi_getcfam,
- .write = fsi_putcfam,
+ .read = HW_UNIT_FN_NAME(fsi, getcfam),
+ .write = HW_UNIT_FN_NAME(fsi, putcfam),
};
DECLARE_HW_UNIT(bmcfsi);
diff --git a/libpdbg/cfam.c b/libpdbg/cfam.c
index ec79d75..27c5a9d 100644
--- a/libpdbg/cfam.c
+++ b/libpdbg/cfam.c
@@ -78,7 +78,7 @@
/* We try up to 1.2ms for an OPB access */
#define MFSI_OPB_MAX_TRIES 1200
-static int fsi2pib_getscom(struct pib *pib, uint64_t addr, uint64_t *value)
+static int pib_fsi2pib_getscom(struct pib *pib, uint64_t addr, uint64_t *value)
{
uint32_t result;
@@ -95,7 +95,7 @@ static int fsi2pib_getscom(struct pib *pib, uint64_t addr, uint64_t *value)
return 0;
}
-static int fsi2pib_putscom(struct pib *pib, uint64_t addr, uint64_t value)
+static int pib_fsi2pib_putscom(struct pib *pib, uint64_t addr, uint64_t value)
{
usleep(FSI2PIB_RELAX);
@@ -106,7 +106,7 @@ static int fsi2pib_putscom(struct pib *pib, uint64_t addr, uint64_t value)
return 0;
}
-static int fsi2pib_reset(struct pdbg_target *target)
+static int pib_fsi2pib_reset(struct pdbg_target *target)
{
/* Reset the PIB master interface. We used to reset the entire FSI2PIB
* engine but that had the unfortunate side effect of clearing existing
@@ -115,15 +115,19 @@ static int fsi2pib_reset(struct pdbg_target *target)
return 0;
}
+DECLARE_HW_UNIT_READ_FN(pib, fsi2pib_getscom, uint64_t);
+DECLARE_HW_UNIT_WRITE_FN(pib, fsi2pib_putscom, uint64_t);
+
static struct pib fsi_pib = {
.target = {
.name = "POWER FSI2PIB",
.compatible = "ibm,fsi-pib",
.class = "pib",
- .probe = fsi2pib_reset,
+ .probe = pib_fsi2pib_reset,
+ .status = PDBG_TARGET_UNKNOWN,
},
- .read = fsi2pib_getscom,
- .write = fsi2pib_putscom,
+ .read = HW_UNIT_FN_NAME(pib, fsi2pib_getscom),
+ .write = HW_UNIT_FN_NAME(pib, fsi2pib_putscom),
};
DECLARE_HW_UNIT(fsi_pib);
@@ -180,7 +184,7 @@ static uint64_t opb_poll(struct opb *opb, uint32_t *read_data)
return rc;
}
-static int p8_opb_read(struct opb *opb, uint32_t addr, uint32_t *data)
+static int opb_p8_read(struct opb *opb, uint32_t addr, uint32_t *data)
{
uint64_t opb_cmd = OPB_CMD_READ | OPB_CMD_32BIT;
int64_t rc;
@@ -203,7 +207,7 @@ static int p8_opb_read(struct opb *opb, uint32_t addr, uint32_t *data)
return opb_poll(opb, data);
}
-static int p8_opb_write(struct opb *opb, uint32_t addr, uint32_t data)
+static int opb_p8_write(struct opb *opb, uint32_t addr, uint32_t data)
{
uint64_t opb_cmd = OPB_CMD_WRITE | OPB_CMD_32BIT;
int64_t rc;
@@ -226,16 +230,20 @@ static int p8_opb_write(struct opb *opb, uint32_t addr, uint32_t data)
return opb_poll(opb, NULL);
}
-static struct opb p8_opb = {
+DECLARE_HW_UNIT_READ_FN(opb, p8_read, uint32_t);
+DECLARE_HW_UNIT_WRITE_FN(opb, p8_write, uint32_t);
+
+static struct opb opb_p8 = {
.target = {
.name = "POWER8 OPB",
.compatible = "ibm,power8-opb",
.class = "opb",
+ .status = PDBG_TARGET_UNKNOWN,
},
- .read = p8_opb_read,
- .write = p8_opb_write,
+ .read = HW_UNIT_FN_NAME(opb, p8_read),
+ .write = HW_UNIT_FN_NAME(opb, p8_write),
};
-DECLARE_HW_UNIT(p8_opb);
+DECLARE_HW_UNIT(opb_p8);
enum chip_type get_chip_type(uint64_t chip_id)
{
@@ -251,17 +259,17 @@ enum chip_type get_chip_type(uint64_t chip_id)
}
}
-static int p8_hmfsi_read(struct fsi *fsi, uint32_t addr, uint32_t *data)
+static int fsi_p8_hmfsi_read(struct fsi *fsi, uint32_t addr, uint32_t *data)
{
return opb_read(&fsi->target, addr, data);
}
-static int p8_hmfsi_write(struct fsi *fsi, uint32_t addr, uint32_t data)
+static int fsi_p8_hmfsi_write(struct fsi *fsi, uint32_t addr, uint32_t data)
{
return opb_write(&fsi->target, addr, data);
}
-static int p8_hmfsi_probe(struct pdbg_target *target)
+static int fsi_p8_hmfsi_probe(struct pdbg_target *target)
{
struct fsi *fsi = target_to_fsi(target);
uint32_t value;
@@ -279,19 +287,23 @@ static int p8_hmfsi_probe(struct pdbg_target *target)
return 0;
}
+DECLARE_HW_UNIT_READ_FN(fsi, p8_hmfsi_read, uint32_t);
+DECLARE_HW_UNIT_WRITE_FN(fsi, p8_hmfsi_write, uint32_t);
+
static struct fsi p8_opb_hmfsi = {
.target = {
.name = "POWER8 OPB attached hMFSI",
.compatible = "ibm,power8-opb-hmfsi",
.class = "fsi",
- .probe = p8_hmfsi_probe,
+ .probe = fsi_p8_hmfsi_probe,
+ .status = PDBG_TARGET_UNKNOWN,
},
- .read = p8_hmfsi_read,
- .write = p8_hmfsi_write,
+ .read = HW_UNIT_FN_NAME(fsi, p8_hmfsi_read),
+ .write = HW_UNIT_FN_NAME(fsi, p8_hmfsi_write),
};
DECLARE_HW_UNIT(p8_opb_hmfsi);
-static int cfam_hmfsi_read(struct fsi *fsi, uint32_t addr, uint32_t *data)
+static int fsi_cfam_hmfsi_read(struct fsi *fsi, uint32_t addr, uint32_t *data)
{
struct pdbg_target *parent_fsi = fsi->target.parent;
@@ -300,7 +312,7 @@ static int cfam_hmfsi_read(struct fsi *fsi, uint32_t addr, uint32_t *data)
return fsi_read(parent_fsi, addr, data);
}
-static int cfam_hmfsi_write(struct fsi *fsi, uint32_t addr, uint32_t data)
+static int fsi_cfam_hmfsi_write(struct fsi *fsi, uint32_t addr, uint32_t data)
{
struct pdbg_target *parent_fsi = fsi->target.parent;
@@ -309,7 +321,7 @@ static int cfam_hmfsi_write(struct fsi *fsi, uint32_t addr, uint32_t data)
return fsi_write(parent_fsi, addr, data);
}
-static int cfam_hmfsi_probe(struct pdbg_target *target)
+static int fsi_cfam_hmfsi_probe(struct pdbg_target *target)
{
struct fsi *fsi = target_to_fsi(target);
struct pdbg_target *fsi_parent = target->parent;
@@ -321,12 +333,14 @@ static int cfam_hmfsi_probe(struct pdbg_target *target)
fsi_read(fsi_parent, 0x3404, &value);
value |= 1 << (31 - port);
if ((rc = fsi_write(fsi_parent, 0x3404, value))) {
- PR_ERROR("Unale to enable HMFSI port %d\n", port);
+ PR_ERROR("Unable to enable HMFSI port %d\n", port);
return rc;
}
- if ((rc = fsi_read(&fsi->target, 0xc09, &value)))
+ if ((rc = fsi_read(&fsi->target, 0xc09, &value))) {
+ PR_ERROR("Unable to enable HMFSI port %d\n", port);
return rc;
+ }
fsi->chip_type = get_chip_type(value);
@@ -337,14 +351,18 @@ static int cfam_hmfsi_probe(struct pdbg_target *target)
return 0;
}
+DECLARE_HW_UNIT_READ_FN(fsi, cfam_hmfsi_read, uint32_t);
+DECLARE_HW_UNIT_WRITE_FN(fsi, cfam_hmfsi_write, uint32_t);
+
static struct fsi cfam_hmfsi = {
.target = {
.name = "CFAM hMFSI Port",
.compatible = "ibm,fsi-hmfsi",
.class = "fsi",
- .probe = cfam_hmfsi_probe,
+ .probe = fsi_cfam_hmfsi_probe,
+ .status = PDBG_TARGET_UNKNOWN,
},
- .read = cfam_hmfsi_read,
- .write = cfam_hmfsi_write,
+ .read = HW_UNIT_FN_NAME(fsi, cfam_hmfsi_read),
+ .write = HW_UNIT_FN_NAME(fsi, cfam_hmfsi_write),
};
DECLARE_HW_UNIT(cfam_hmfsi);
diff --git a/libpdbg/i2c.c b/libpdbg/i2c.c
index b1580e1..9c94dd0 100644
--- a/libpdbg/i2c.c
+++ b/libpdbg/i2c.c
@@ -59,7 +59,7 @@ static int i2c_set_scom_addr(struct i2c_data *i2c_data, uint32_t addr)
return 0;
}
-static int i2c_getscom(struct pib *pib, uint64_t addr, uint64_t *value)
+static int pib_i2c_getscom(struct pib *pib, uint64_t addr, uint64_t *value)
{
struct i2c_data *i2c_data = pib->priv;
uint64_t data;
@@ -76,7 +76,7 @@ static int i2c_getscom(struct pib *pib, uint64_t addr, uint64_t *value)
return 0;
}
-static int i2c_putscom(struct pib *pib, uint64_t addr, uint64_t value)
+static int pib_i2c_putscom(struct pib *pib, uint64_t addr, uint64_t value)
{
struct i2c_data *i2c_data = pib->priv;
uint8_t data[12];
@@ -123,7 +123,7 @@ static void i2c_destroy(struct pib *pib)
/*
* Initialise a i2c backend on the given bus at the given bus address.
*/
-int i2c_target_probe(struct pdbg_target *target)
+int pib_i2c_target_probe(struct pdbg_target *target)
{
struct pib *pib = target_to_pib(target);
struct i2c_data *i2c_data;
@@ -153,14 +153,18 @@ int i2c_target_probe(struct pdbg_target *target)
return 0;
}
+DECLARE_HW_UNIT_READ_FN(pib, i2c_getscom, uint64_t);
+DECLARE_HW_UNIT_WRITE_FN(pib, i2c_putscom, uint64_t);
+
static struct pib p8_i2c_pib = {
.target = {
.name = "POWER8 I2C Slave",
.compatible = "ibm,power8-i2c-slave",
.class = "pib",
- .probe = i2c_target_probe,
+ .probe = pib_i2c_target_probe,
+ .status = PDBG_TARGET_UNKNOWN,
},
- .read = i2c_getscom,
- .write = i2c_putscom,
+ .read = HW_UNIT_FN_NAME(pib, i2c_getscom),
+ .write = HW_UNIT_FN_NAME(pib, i2c_putscom),
};
DECLARE_HW_UNIT(p8_i2c_pib);
diff --git a/libpdbg/kernel.c b/libpdbg/kernel.c
index 8b597ef..cac9736 100644
--- a/libpdbg/kernel.c
+++ b/libpdbg/kernel.c
@@ -35,7 +35,7 @@
int fsi_fd;
-static int kernel_fsi_getcfam(struct fsi *fsi, uint32_t addr64, uint32_t *value)
+static int fsi_kernel_getcfam(struct fsi *fsi, uint32_t addr64, uint32_t *value)
{
int rc;
uint32_t tmp, addr = (addr64 & 0x7ffc00) | ((addr64 & 0x3ff) << 2);
@@ -60,7 +60,7 @@ static int kernel_fsi_getcfam(struct fsi *fsi, uint32_t addr64, uint32_t *value)
return 0;
}
-static int kernel_fsi_putcfam(struct fsi *fsi, uint32_t addr64, uint32_t data)
+static int fsi_kernel_putcfam(struct fsi *fsi, uint32_t addr64, uint32_t data)
{
int rc;
uint32_t tmp, addr = (addr64 & 0x7ffc00) | ((addr64 & 0x3ff) << 2);
@@ -91,7 +91,7 @@ static void kernel_fsi_destroy(struct pdbg_target *target)
}
#endif
-static void kernel_fsi_scan_devices(void)
+static void fsi_kernel_scan_devices(void)
{
const char one = '1';
int rc, fd;
@@ -107,7 +107,7 @@ static void kernel_fsi_scan_devices(void)
close(fd);
}
-int kernel_fsi_probe(struct pdbg_target *target)
+int fsi_kernel_probe(struct pdbg_target *target)
{
if (!fsi_fd) {
int tries = 5;
@@ -120,7 +120,7 @@ int kernel_fsi_probe(struct pdbg_target *target)
tries--;
/* Scan */
- kernel_fsi_scan_devices();
+ fsi_kernel_scan_devices();
sleep(1);
}
if (fsi_fd < 0) {
@@ -133,14 +133,17 @@ int kernel_fsi_probe(struct pdbg_target *target)
return -1;
}
+DECLARE_HW_UNIT_READ_FN(fsi, kernel_getcfam, uint32_t);
+DECLARE_HW_UNIT_WRITE_FN(fsi, kernel_putcfam, uint32_t);
+
static struct fsi kernel_fsi = {
.target = {
.name = "Kernel based FSI master",
.compatible = "ibm,kernel-fsi",
.class = "fsi",
- .probe = kernel_fsi_probe,
+ .probe = fsi_kernel_probe,
},
- .read = kernel_fsi_getcfam,
- .write = kernel_fsi_putcfam,
+ .read = HW_UNIT_FN_NAME(fsi, kernel_getcfam),
+ .write = HW_UNIT_FN_NAME(fsi, kernel_putcfam),
};
DECLARE_HW_UNIT(kernel_fsi);
diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h
index 51e1523..8fd36cd 100644
--- a/libpdbg/libpdbg.h
+++ b/libpdbg/libpdbg.h
@@ -49,7 +49,7 @@ struct pdbg_target *__pdbg_next_child_target(struct pdbg_target *parent, struct
enum pdbg_target_status {PDBG_TARGET_UNKNOWN = 0, PDBG_TARGET_ENABLED,
PDBG_TARGET_DISABLED, PDBG_TARGET_MUSTEXIST,
PDBG_TARGET_NONEXISTENT, PDBG_TARGET_RELEASED,
- PDBG_TARGET_PENDING_RELEASE };
+ PDBG_TARGET_PENDING_RELEASE, PDBG_TARGET_BEING_PROBED };
#define pdbg_for_each_target(class, parent, target) \
for (target = __pdbg_next_target(class, parent, NULL); \
diff --git a/libpdbg/target.c b/libpdbg/target.c
index bcf4bb9..0076185 100644
--- a/libpdbg/target.c
+++ b/libpdbg/target.c
@@ -292,6 +292,8 @@ enum pdbg_target_status pdbg_target_probe(struct pdbg_target *target)
* it's status won't have changed */
return status;
+ pdbg_log(PDBG_DEBUG, "probing %s\n", target->name);
+ target->status = PDBG_TARGET_BEING_PROBED;
parent = target->parent;
if (parent) {
/* Recurse up the tree to probe and set parent target status */
@@ -322,6 +324,10 @@ enum pdbg_target_status pdbg_target_probe(struct pdbg_target *target)
case PDBG_TARGET_ENABLED:
break;
+ case PDBG_TARGET_BEING_PROBED:
+ /* concurrent probing? */
+ assert(0);
+ break;
}
}
@@ -332,6 +338,7 @@ enum pdbg_target_status pdbg_target_probe(struct pdbg_target *target)
target->status = PDBG_TARGET_NONEXISTENT;
return PDBG_TARGET_NONEXISTENT;
}
+ pdbg_log(PDBG_DEBUG, "probed %s\n", target->name);
target->status = PDBG_TARGET_ENABLED;
return PDBG_TARGET_ENABLED;
diff --git a/libpdbg/target.h b/libpdbg/target.h
index 5541729..9bf1f77 100644
--- a/libpdbg/target.h
+++ b/libpdbg/target.h
@@ -89,6 +89,30 @@ struct hw_unit_info *find_compatible_target(const char *compat);
{ .hw_unit = &name, .size = sizeof(name) }; \
const struct hw_unit_info __used __section("hw_units") *name ##_hw_unit_p = &name ##_hw_unit
+#define DECLARE_HW_UNIT_READ_FN(unit, name, type) \
+ static inline int __##unit##_##name(struct unit *unit, type addr, \
+ type *value) { \
+ if (unit->target.status == PDBG_TARGET_UNKNOWN) { \
+ pdbg_log(PDBG_ERROR, "target %s not probed\n", #name); \
+ return -1; \
+ } else { \
+ return unit##_##name(unit, addr, value); \
+ } \
+ }
+
+#define DECLARE_HW_UNIT_WRITE_FN(unit, name, type) \
+ static inline int __##unit##_##name(struct unit *unit, type addr, \
+ type value) { \
+ if (unit->target.status == PDBG_TARGET_UNKNOWN) { \
+ pdbg_log(PDBG_ERROR, "target %s not probed\n", #name); \
+ return -1; \
+ } else { \
+ return unit##_##name(unit, addr, value); \
+ } \
+ }
+
+#define HW_UNIT_FN_NAME(unit, name) (__##unit##_##name)
+
struct htm {
struct pdbg_target target;
int (*start)(struct htm *);
--
2.13.6
More information about the Pdbg
mailing list