[WIP]wishbone.Connector class #21

Fatsie · 2020-07-01T16:06:18Z

Currently pull request as WIP:

First code for review
Further discussion in Wishbone access from initiator bus with data_width smaller than the one of the subordinate bus. #18 on feature set of class
Connecting initiator bus with data width greater than data width of subordinate bus is not implemented. Would PR be acceptable without this implementation ?

codecov · 2020-07-01T16:07:12Z

Codecov Report

Merging #21 (ec94f31) into master (c754caf) will not change coverage.
The diff coverage is 100.00%.

❗ Current head ec94f31 differs from pull request most recent head 3db2a83. Consider uploading reports for the commit 3db2a83 to get more accurate results

@@            Coverage Diff            @@
##            master       #21   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         6    -1     
  Lines          693       725   +32     
  Branches       145       160   +15     
=========================================
+ Hits           693       725   +32

Impacted Files	Coverage Δ
nmigen_soc/wishbone/bus.py	`100.00% <100.00%> (ø)`
nmigen_soc/periph.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecfad4d...3db2a83. Read the comment docs.

Fatsie · 2020-07-01T16:09:54Z

* Connecting initiator bus with data width greater than data width of subordinate bus is not implemented. Would PR be acceptable without this implementation ?

Reason I ask is that I don't need this feature for my use case and implementation of it may take some time.

whitequark · 2020-07-01T19:26:10Z

Reason I ask is that I don't need this feature for my use case and implementation of it may take some time.

In general my preference is to have a complete solution even if it takes a bit longer, but let's get the review started before you put more time into that.

jfng

We should also add support for memory maps. If the subordinate bus has a memory map, it should be exposed to the initiator bus. Something like this:

try:
    sub_map  = sub_bus.memory_map
    intr_map = MemoryMap(...)
    intr_map.add_window(sub_map, addr=0, sparse=False)
    self.intr_bus.memory_map = intr_map
except NotImplementedError:
    # Subordinate bus does not have a memory map.
    pass

nmigen_soc/wishbone/bus.py

jfng · 2020-07-06T17:05:36Z

nmigen_soc/wishbone/bus.py

@@ -166,6 +167,147 @@ def memory_map(self, memory_map):
        self._map = memory_map


+class Connector(Elaboratable):


Connector seems too generic for a width up-converter. How about Upconverter ?

As discussed in #18 I did use Connector because the functionality is broader then upconverting, it's functionality is to connect a single initiator to a single subordinate. Upconverting will be done when necessary.
You will need to agree with @whitequark what to go for.

Could the Connector class be composed internally of two separate up- and down-converter classes ?

I can see during implementation of the feature if it makes sense to do that but current feeling is that it will cause unnecessary code duplication with negligible improvement in code clarity or maintainability.

nmigen_soc/wishbone/bus.py

jfng · 2020-07-06T17:07:18Z

nmigen_soc/wishbone/bus.py

+            raise NotImplementedError(
+                "Support for multi-cycle bus operation when initiator data_width is"
+                "bigger than the subordinate one is not implemented."
+            )


This should be a ValueError.

Plan is to later also implement downsizing in this class therefor NotImlementedError. This is thus alsot related to discussion in #18.

nmigen_soc/wishbone/bus.py

jfng · 2020-07-06T17:22:25Z

nmigen_soc/wishbone/bus.py

+
+        m = Module()
+
+        common_addr_width = min(intr_bus.addr_width, sub_bus.addr_width)


Could this be simplified to sub_bus.addr_width ? intr_bus.addr_width >= sub_bus.addr_width is asserted later is the code.

I removed the assert now, it was planned to be removed if multi-cycle bus was implemented.
Kept the common_addr_width definition as it is already future proof.

Fatsie · 2020-07-07T11:37:37Z

We should also add support for memory maps. If the subordinate bus has a memory map, it should be exposed to the initiator bus. Something like this:

try:
    sub_map  = sub_bus.memory_map
    intr_map = MemoryMap(...)
    intr_map.add_window(sub_map, addr=0, sparse=False)
    self.intr_bus.memory_map = intr_map
except NotImplementedError:
    # Subordinate bus does not have a memory map.
    pass

Actually in the first version I even enforced a memory map on subordinate bus like the Decoder does. But I removed all memory map code after I got problems with the unit test for different granularity. I based the data_width of the memory maps on the granularity of each bus. Then you get an error when adding the subordinate memorymap to the initiator one due to different data_width.

Implementation of a module that allows to connect one initiator to one subordinate.bus. Currently connecting initiator bus with data width greater than the subordinate bus is not implemented. Implementation of that feature will need to instantiate multiple subordinate bus cycles for one initiator bus cycle.

codecov-commenter · 2022-02-21T13:19:32Z

Codecov Report

❗ No coverage uploaded for pull request base (main@217d4ea). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head ec94f31 differs from pull request most recent head 1c7e14f. Consider uploading reports for the commit 1c7e14f to get more accurate results

@@           Coverage Diff            @@
##             main       #21   +/-   ##
========================================
  Coverage        ?   100.00%           
========================================
  Files           ?         6           
  Lines           ?       725           
  Branches        ?       160           
========================================
  Hits            ?       725           
  Misses          ?         0           
  Partials        ?         0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 217d4ea...1c7e14f. Read the comment docs.

Fatsie mentioned this pull request Jul 1, 2020

Wishbone access from initiator bus with data_width smaller than the one of the subordinate bus. #18

Open

whitequark requested a review from jfng July 1, 2020 19:25

jeanthom mentioned this pull request Jul 3, 2020

Use Up-Down converters from nmigen-soc jeanthom/gram#21

Closed

jfng requested changes Jul 6, 2020

View reviewed changes

Fatsie force-pushed the wishbone_connector branch from 280890c to ec94f31 Compare July 7, 2020 11:56

Fatsie force-pushed the wishbone_connector branch from ec94f31 to 3db2a83 Compare April 20, 2021 08:02

FatsieFS force-pushed the wishbone_connector branch from 3db2a83 to 1c7e14f Compare February 21, 2022 13:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP]wishbone.Connector class #21

[WIP]wishbone.Connector class #21

Fatsie commented Jul 1, 2020

codecov bot commented Jul 1, 2020 •

edited

Loading

Fatsie commented Jul 1, 2020

whitequark commented Jul 1, 2020

jfng left a comment

jfng Jul 6, 2020

Fatsie Jul 7, 2020

jfng Jul 8, 2020

Fatsie Jul 8, 2020

jfng Jul 6, 2020

Fatsie Jul 7, 2020

jfng Jul 6, 2020

Fatsie Jul 7, 2020

Fatsie commented Jul 7, 2020 •

edited

Loading

codecov-commenter commented Feb 21, 2022

		@@ -166,6 +167,147 @@ def memory_map(self, memory_map):
		self._map = memory_map


		class Connector(Elaboratable):


		m = Module()

		common_addr_width = min(intr_bus.addr_width, sub_bus.addr_width)

[WIP]wishbone.Connector class #21

Are you sure you want to change the base?

[WIP]wishbone.Connector class #21

Conversation

Fatsie commented Jul 1, 2020

codecov bot commented Jul 1, 2020 • edited Loading

Codecov Report

Fatsie commented Jul 1, 2020

whitequark commented Jul 1, 2020

jfng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fatsie commented Jul 7, 2020 • edited Loading

codecov-commenter commented Feb 21, 2022

Codecov Report

codecov bot commented Jul 1, 2020 •

edited

Loading

Fatsie commented Jul 7, 2020 •

edited

Loading