Skip to content

Various hardware improvements and fixed - #2

Open
phsauter wants to merge 18 commits into
Freakness109:mainfrom
phsauter:fix/reset-clear-sram-buffer
Open

Various hardware improvements and fixed#2
phsauter wants to merge 18 commits into
Freakness109:mainfrom
phsauter:fix/reset-clear-sram-buffer

Conversation

@phsauter

Copy link
Copy Markdown

I clankered the list below, might be a bit off:

  • Replaced soft-reset clock gating with synchronous clear signals across the controller blocks.
  • Guarded SRAM buffer accesses during transfers to avoid unsafe reads/writes.
  • Parameterized DAT buffer depth and added tb_dat_buffer_sizes coverage for multiple buffer sizes.
  • Fixed finite DAT write buffering and block-count-limited transfer behavior, including multiblock transfers with block_count_enable.
  • Removed unimplemented register fields from writable behavior so they act read-only.
  • Removed redundant command/argument/response-type latches from cmd_logic and centralized accepted command payload latching in autocmd_wrap.
  • Cleaned up SD clock divider handling:
    • replaced ClkPreDivLog with integer ClkPreDiv
    • preserved SDHCI power-of-two divider encoding
    • enforced even/minimum divide-by-2 physical output
    • kept sd_clk_o as a registered clock-as-data output
  • Added sdhci_top_synth as a concrete synthesis wrapper for frontend/synthesis elaboration.

@Freakness109 Freakness109 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the PR! The cleanup of the clocking and the clears is especially appreciated.
There are a few clears that you missed, would you mind adding those as well? The register file clear can wait for another day.
As for the chunking idea, although I quite like it, I'm not sure that the spec allows it.
I'd skip that part of the PR for now, if we come up with an idea on how to implement it in a non-violating way, I'd be happy to merge that.

Comment thread hw/sd_clk_generator.sv
Comment thread hw/reg/sdhci_reg_logic.sv Outdated
Comment on lines 180 to 181

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also fall into the general domain

Comment thread hw/reg/sdhci_reg_logic.sv
!reg2hw_i.present_state.command_inhibit_cmd.q && reg2hw_i.transfer_mode.auto_cmd12_enable .qe, '0)
`FFL (transfer_mode_reg_o.block_count_enable .d, reg2hw_i.transfer_mode.block_count_enable .q,
!reg2hw_i.present_state.command_inhibit_cmd.q && reg2hw_i.transfer_mode.block_count_enable .qe, '0)
`FFL (transfer_mode_reg_o.dma_enable .d, reg2hw_i.transfer_mode.dma_enable .q,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should probably also be cleared, falls into the general domain

Comment thread hw/reg/sdhci_reg_top.sv

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The register file should also be cleared on software reset, but I guess we can leave this for another time.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't 100% sure if this is the case reading the spec but I can add it.

Comment thread hw/dat_buffer.sv Outdated
Comment on lines +86 to +115
buffer_read_enable_o.d = has_block;
buffer_read_enable_o.d = has_chunk && !write_valid_i;
buffer_data_port_d_o = reg_pop_data;
reg_pop = reg2hw_i.buffer_data_port.re;
reg_pop = reg2hw_i.buffer_data_port.re && !reg_empty && !write_valid_i;
end else if (write_operation_i) begin
reg_pop = read_ready_i;
reg_pop = read_ready_i && !reg_empty;
read_data_o = reg_pop_data;
read_valid_o = has_block;
read_valid_o = !reg_empty;

buffer_write_enable_o.d = has_space;
buffer_write_enable_o.d = accepts_write_chunk && has_chunk_space;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I like the solution to apply backpressure to software + optimisations to allow for smaller buffers, the spec disallows it: once buffer write enable or buffer read enable is set, the software is allowed to read/write an entire block without checking the bits (page 7, §1.7(8) in the spec).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have another look but I would really really like an option to also have a smaller peripheral that can be used in Croc sized systems.
Maybe there is a better solution though...

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about it for a bit, I think I have a way forward that would work:

  • Leave the chunking as solution but add a "warranty void" parameter (e.g. AllowNoncompliantBufferSizes) that disables an assertion, so that users of the HW are aware and know why COTS drivers may break
  • Put the size of the chunks in the vendor specific area so that the drivers know how many bytes can be written before checking flags
  • Once we have DMA, most drivers will prefer it. As the HW then controls data movement, smaller buffers should be no problem even for off-the-shelf drivers

If you could add a warranty-void flag, I'd be happy to merge the chunk solution.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already changed it to that but with a slightly different name, I am adjusting it before pushing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants