[Pdbg] [RFC 02/12] sbefifo: Rework the memory interfaces
Amitay Isaacs
amitay at ozlabs.org
Tue Aug 20 13:42:02 AEST 2019
On Tue, 2019-08-06 at 11:37 +1000, Alistair Popple wrote:
> The SBEFIFO unit introduced it's own implementation specific
> interface
> for accessing memory. Instead it should conform to the existing
> memory
> access interfaces. This patch splits the SBEFIFO memory access into
> it's own hardware unit so that it can conform with the existing
> interfaces.
>
> Signed-off-by: Alistair Popple <alistair at popple.id.au>
> ---
> libpdbg/hwunit.h | 2 --
> libpdbg/sbefifo.c | 38 ++++++++++++++++++++++++++++++--------
> libpdbg/target.c | 38 ++++++++------------------------------
> p9-kernel.dts.m4 | 4 ++++
> p9-pib.dts.m4 | 5 -----
> src/mem.c | 46 ++++++---------------------------------------
> -
> 6 files changed, 48 insertions(+), 85 deletions(-)
>
> diff --git a/libpdbg/hwunit.h b/libpdbg/hwunit.h
> index 2618941..a359af0 100644
> --- a/libpdbg/hwunit.h
> +++ b/libpdbg/hwunit.h
> @@ -68,8 +68,6 @@ struct mem {
> struct sbefifo {
> struct pdbg_target target;
> int (*istep)(struct sbefifo *, uint32_t major, uint32_t minor);
> - int (*mem_read)(struct sbefifo *, uint64_t, uint8_t *,
> uint64_t, bool);
> - int (*mem_write)(struct sbefifo *, uint64_t, uint8_t *,
> uint64_t, bool);
> int (*thread_start)(struct sbefifo *, uint32_t core_id,
> uint32_t thread_id);
> int (*thread_stop)(struct sbefifo *, uint32_t core_id, uint32_t
> thread_id);
> int (*thread_step)(struct sbefifo *, uint32_t core_id, uint32_t
> thread_id);
> diff --git a/libpdbg/sbefifo.c b/libpdbg/sbefifo.c
> index a2442cd..b7f52a1 100644
> --- a/libpdbg/sbefifo.c
> +++ b/libpdbg/sbefifo.c
> @@ -244,9 +244,9 @@ static int sbefifo_op_istep(struct sbefifo
> *sbefifo,
> return 0;
> }
>
> -static int sbefifo_op_getmem(struct sbefifo *sbefifo,
> +static int sbefifo_op_getmem(struct mem *sbefifo,
> uint64_t addr, uint8_t *data, uint64_t
> size,
> - bool ci)
> + uint8_t block_size, bool ci)
> {
> uint8_t *out;
> uint64_t start_addr, end_addr;
> @@ -257,6 +257,11 @@ static int sbefifo_op_getmem(struct sbefifo
> *sbefifo,
>
> align = ci ? 8 : 128;
>
> + if (block_size && block_size != 8) {
> + PR_ERROR("sbefifo: Only 8 byte block sizes are
> supported\n");
> + return -1;
> + };
> +
> start_addr = addr & (~(uint64_t)(align-1));
> end_addr = (addr + size + (align-1)) & (~(uint64_t)(align-1));
>
> @@ -285,7 +290,8 @@ static int sbefifo_op_getmem(struct sbefifo
> *sbefifo,
> msg[5] = htobe32(len);
>
> out_len = len + 4;
> - rc = sbefifo_op(sbefifo, msg, sizeof(msg), cmd, &out, &out_len,
> &status);
> + rc = sbefifo_op(target_to_sbefifo(sbefifo->target.parent), msg,
> sizeof(msg), cmd,
> + &out, &out_len, &status);
> if (rc)
> return rc;
>
> @@ -304,9 +310,9 @@ static int sbefifo_op_getmem(struct sbefifo
> *sbefifo,
> return 0;
> }
>
> -static int sbefifo_op_putmem(struct sbefifo *sbefifo,
> +static int sbefifo_op_putmem(struct mem *sbefifo,
> uint64_t addr, uint8_t *data, uint64_t
> size,
> - bool ci)
> + uint8_t block_size, bool ci)
> {
> uint8_t *out;
> uint32_t *msg;
> @@ -316,6 +322,11 @@ static int sbefifo_op_putmem(struct sbefifo
> *sbefifo,
>
> align = ci ? 8 : 128;
>
> + if (block_size && block_size != 8) {
> + PR_ERROR("sbefifo: Only 8 byte block sizes are
> supported\n");
> + return -1;
> + };
> +
> if (addr & (align-1)) {
> PR_ERROR("sbefifo: Address must be aligned to %d
> bytes\n", align);
> return -1;
> @@ -353,7 +364,8 @@ static int sbefifo_op_putmem(struct sbefifo
> *sbefifo,
> memcpy(&msg[6], data, len);
>
> out_len = 4;
> - rc = sbefifo_op(sbefifo, msg, msg_len, cmd, &out, &out_len,
> &status);
> + rc = sbefifo_op(target_to_sbefifo(sbefifo->target.parent), msg,
> msg_len, cmd,
> + &out, &out_len, &status);
> if (rc)
> return rc;
>
> @@ -479,6 +491,17 @@ static int sbefifo_probe(struct pdbg_target
> *target)
> return 0;
> }
>
> +struct mem sbefifo_mem = {
> + .target = {
> + .name = "SBE FIFO Chip-op based memory access",
> + .compatible = "ibm,sbefifo-mem",
> + .class = "mem",
> + },
> + .read = sbefifo_op_getmem,
> + .write = sbefifo_op_putmem,
> +};
> +DECLARE_HW_UNIT(sbefifo_mem);
> +
> struct sbefifo kernel_sbefifo = {
> .target = {
> .name = "Kernel based FSI SBE FIFO",
> @@ -487,8 +510,6 @@ struct sbefifo kernel_sbefifo = {
> .probe = sbefifo_probe,
> },
> .istep = sbefifo_op_istep,
> - .mem_read = sbefifo_op_getmem,
> - .mem_write = sbefifo_op_putmem,
> .thread_start = sbefifo_op_thread_start,
> .thread_stop = sbefifo_op_thread_stop,
> .thread_step = sbefifo_op_thread_step,
> @@ -503,4 +524,5 @@ __attribute__((constructor))
> static void register_sbefifo(void)
> {
> pdbg_hwunit_register(&kernel_sbefifo_hw_unit);
> + pdbg_hwunit_register(&sbefifo_mem_hw_unit);
> }
> diff --git a/libpdbg/target.c b/libpdbg/target.c
> index 8880bf3..61353bc 100644
> --- a/libpdbg/target.c
> +++ b/libpdbg/target.c
> @@ -219,48 +219,26 @@ int fsi_write(struct pdbg_target *fsi_dt,
> uint32_t addr, uint32_t data)
>
> int mem_read(struct pdbg_target *target, uint64_t addr, uint8_t
> *output, uint64_t size, uint8_t block_size, bool ci)
> {
> + struct mem *mem;
> int rc = -1;
>
> - assert(pdbg_target_is_class(target, "sbefifo") ||
> - pdbg_target_is_class(target, "mem"));
> + assert(pdbg_target_is_class(target, "mem"));
>
> - if (pdbg_target_is_class(target, "sbefifo")) {
> - struct sbefifo *sbefifo;
> -
> - sbefifo = target_to_sbefifo(target);
> - rc = sbefifo->mem_read(sbefifo, addr, output, size,
> ci);
> - }
> -
> - if (pdbg_target_is_class(target, "mem")) {
> - struct mem *mem;
> -
> - mem = target_to_mem(target);
> - rc = mem->read(mem, addr, output, size, block_size,
> ci);
> - }
> + mem = target_to_mem(target);
> + rc = mem->read(mem, addr, output, size, block_size, ci);
>
> return rc;
> }
>
> int mem_write(struct pdbg_target *target, uint64_t addr, uint8_t
> *input, uint64_t size, uint8_t block_size, bool ci)
> {
> + struct mem *mem;
> int rc = -1;
>
> - assert(pdbg_target_is_class(target, "sbefifo") ||
> - pdbg_target_is_class(target, "mem"));
> + assert(pdbg_target_is_class(target, "mem"));
>
> - if (pdbg_target_is_class(target, "sbefifo")) {
> - struct sbefifo *sbefifo;
> -
> - sbefifo = target_to_sbefifo(target);
> - rc = sbefifo->mem_write(sbefifo, addr, input, size,
> ci);
> - }
> -
> - if (pdbg_target_is_class(target, "mem")) {
> - struct mem *mem;
> -
> - mem = target_to_mem(target);
> - rc = mem->write(mem, addr, input, size, block_size,
> ci);
> - }
> + mem = target_to_mem(target);
> + rc = mem->write(mem, addr, input, size, block_size, ci);
>
> return rc;
> }
> diff --git a/p9-kernel.dts.m4 b/p9-kernel.dts.m4
> index 9ab46b8..30cde95 100644
> --- a/p9-kernel.dts.m4
> +++ b/p9-kernel.dts.m4
> @@ -28,6 +28,10 @@
> index = <0x0>;
> compatible = "ibm,kernel-sbefifo";
> device-path = "/dev/sbefifo1";
> +
> + sbefifo-mem at 0 {
> + compatible = "ibm,sbefifo-mem";
> + };
> };
>
> hmfsi at 100000 {
Shouldn't we add sbefifo-mem at 1 (for sbefifo at 1) for completeness?
> diff --git a/p9-pib.dts.m4 b/p9-pib.dts.m4
> index 3e312e5..3a99157 100644
> --- a/p9-pib.dts.m4
> +++ b/p9-pib.dts.m4
> @@ -36,11 +36,6 @@ reg = <0x0 HEX(CHIPLET_BASE($1)) 0xfffff>;
> }')dnl
>
>
> -adu at 90000 {
> - compatible = "ibm,power9-adu";
> - reg = <0x0 0x90000 0x5>;
> -};
> -
> htm at 5012880 {
> compatible = "ibm,power9-nhtm";
> reg = <0x0 0x5012880 0x40>;
> diff --git a/src/mem.c b/src/mem.c
> index cacd394..53d33b8 100644
> --- a/src/mem.c
> +++ b/src/mem.c
> @@ -95,23 +95,6 @@ static int _getmem(uint64_t addr, uint64_t size,
> uint8_t block_size, bool ci, bo
> buf = malloc(size);
> assert(buf);
>
> - pdbg_for_each_class_target("sbefifo", target) {
> - if (pdbg_target_probe(target) != PDBG_TARGET_ENABLED)
> - continue;
> -
> - pdbg_set_progress_tick(progress_tick);
> - progress_init();
> - rc = mem_read(target, addr, buf, size, block_size, ci);
> - progress_end();
> - if (rc) {
> - PR_ERROR("Unable to read memory using
> sbefifo\n");
> - continue;
> - }
> -
> - count++;
> - goto done;
> - }
> -
> pdbg_for_each_class_target("mem", target) {
> if (pdbg_target_probe(target) != PDBG_TARGET_ENABLED)
> continue;
> @@ -121,15 +104,15 @@ static int _getmem(uint64_t addr, uint64_t
> size, uint8_t block_size, bool ci, bo
> rc = mem_read(target, addr, buf, size, block_size, ci);
> progress_end();
> if (rc) {
> - PR_ERROR("Unable to read memory using adu\n");
> + PR_ERROR("Unable to read memory from %s\n",
> + pdbg_target_path(target));
> continue;
> }
>
> count++;
> - goto done;
> + break;
> }
>
> -done:
> if (count > 0) {
> uint64_t i;
> bool printable = true;
> @@ -188,23 +171,6 @@ static int _putmem(uint64_t addr, uint8_t
> block_size, bool ci)
> buf = read_stdin(&buflen);
> assert(buf);
>
> - pdbg_for_each_class_target("sbefifo", target) {
> - if (pdbg_target_probe(target) != PDBG_TARGET_ENABLED)
> - continue;
> -
> - pdbg_set_progress_tick(progress_tick);
> - progress_init();
> - rc = mem_write(target, addr, buf, buflen, block_size,
> ci);
> - progress_end();
> - if (rc) {
> - printf("Unable to write memory using
> sbefifo\n");
> - continue;
> - }
> -
> - count++;
> - goto done;
> - }
> -
> pdbg_for_each_class_target("mem", target) {
> if (pdbg_target_probe(target) != PDBG_TARGET_ENABLED)
> continue;
> @@ -214,15 +180,15 @@ static int _putmem(uint64_t addr, uint8_t
> block_size, bool ci)
> rc = mem_write(target, addr, buf, buflen, block_size,
> ci);
> progress_end();
> if (rc) {
> - printf("Unable to write memory using adu\n");
> + printf("Unable to write memory using %s\n",
> + pdbg_target_path(target));
> continue;
> }
>
> count++;
> - goto done;
> + break;
> }
>
> -done:
> if (count > 0)
> printf("Wrote %zu bytes starting at 0x%016" PRIx64
> "\n", buflen, addr);
>
> --
> 2.20.1
>
Amitay.
--
Nothing is so contagious as enthusiasm; it moves stones, it charms brutes.
Enthusiasm is the genius of sincerity, and truth accomplishes no victories
without it. - Edward Bulwer-Lytton
More information about the Pdbg
mailing list