`sberror` is not signaled if `sbaccess` is invalid

[bluewww]

We are supposed to signal sberror when the requested sbaccess is not supported (which currently is every type except the "native" bit width)

See also https://github.com/pulp-platform/riscv-dbg/issues/57

[Silabs-ArjanB]

Hi @bluewww

Do you agree that because of the following code in dm_csrs.sv:

    sbcs_d.sbaccess128          = 1'b0;
    sbcs_d.sbaccess64           = logic'(BusWidth == 32'd64);
    sbcs_d.sbaccess32           = logic'(BusWidth == 32'd32);
    sbcs_d.sbaccess16           = 1'b0;
    sbcs_d.sbaccess8            = 1'b0;
    sbcs_d.sbaccess             = (BusWidth == 32'd64) ? 3'd3 : 3'd2;

, that the following code from dm_sba.sv contains 'dead code':

    unique case (sbaccess_i)
      3'b000: begin
        be_mask[be_idx] = '1;
      end
      3'b001: begin
        be_mask[int'({be_idx[$high(be_idx):1], 1'b0}) +: 2] = '1;
      end
      3'b010: begin
        if (BusWidth == 32'd64) be_mask[int'({be_idx[$high(be_idx)], 2'b0}) +: 4] = '1;
        else                    be_mask = '1;
      end
      3'b011: be_mask = '1;
      default: ;
    endcase

, and can be simplified to:

    unique case (sbaccess_i)
      3'b010: be_mask = '1;
      3'b011: be_mask = '1;
      default: ;
    endcase

I ask this question in relationship to https://github.com/pulp-platform/riscv-dbg/issues/58. I intend to fix that issue and at the same time do above code cleanup if you agree that above simplification is okay.

[Silabs-ArjanB]

Alternatively the debug module could support all access sizes up to its bus width in which case byte enables can be used to implement narrow accesses on a wider bus. In that case the initial fix in dm_csrs.sv would be to use something like the following:

    sbcs_d.sbaccess128          = 1'b0;
    sbcs_d.sbaccess64           = logic'(BusWidth == 32'd64);
    sbcs_d.sbaccess32           = 1'b1;
    sbcs_d.sbaccess16           = 1'b1;
    sbcs_d.sbaccess8            = 1'b1;

I.e. 8-bit, 16-bit, and 32-bit accesses are ALWAYS allowed, which seems to be the intention of dm_sba.sv Which approach should be implemented?

A) Only allow either 32-bit or 64-bit accesses
B) Allow all access widths of at most the BusWidth (so always allow 8,16,32-bit accesses and allow 64-bit accesses as well if BusWidth==64)

[jm4rtin]

My preference would be to support the other access sizes. How far off is the RTL from working with these other access sizes?

[Silabs-ArjanB]

@jm4rtin I stuck to the originally supported access sizes. Adding the others is not too much work either, but my main concern was to just get this bug fixed.

[bluewww]

@Silabs-ArjanB I think the best way is we enable byte and halfword accesses like you suggested:

    sbcs_d.sbaccess16           = 1'b1;
    sbcs_d.sbaccess8            = 1'b1;

instead of disallowing for all accesses but the native bitwidth.

[Silabs-ArjanB]

@Silabs-ArjanB I think the best way is we enable byte and halfword accesses like you suggested:

    sbcs_d.sbaccess16           = 1'b1;
    sbcs_d.sbaccess8            = 1'b1;

instead of disallowing for all accesses but the native bitwidth.

Fully agreed with that.

[noytzach]

see #106,

setting sbaccess8 and sbaccess16 is only half of the solution. had to shift the data to align with the byte-enable.

[Silabs-ArjanB]

Hi @noytzach How about reads, did you implement the shift for that as well (or is that already in the RTL)?

[noytzach]

Thanks @Silabs-ArjanB, https://github.com/pulp-platform/riscv-dbg/pull/106/commits/8814cb2a958f77790f0db92aa2f0de8b781839c4 fixes for the read

note that according to the spec:

If the width of the read access is less than the width of sbdata, the contents of the remaining high bits may take on any value.

so higher bits are not masked

[Silabs-ArjanB]

Thanks @noytzach