Write mask

[lorenzschmid]

This might be a bit a controversial PR. So please feel free to share your opinion about it.

Currently, multiple of the bus interface support some form of a write mask. This means that upon a write request, the request sender can specify, which parts of the written value should be updated and, which should be ignored. Hence, it can mask part of its write request. The following interfaces support this (corresponding signal name in parentheses):

• APB (pstrb)
• Avalon (byteenable)
• AXI4 Lite (wstrb)
• Wishbone (sel)

As I understand the current implementation in cheby, the write mask is only supported for

• memory writes (i.e. no write mask is applied when writing to registers)
• with a granularity of Bytes (i.e. one can only mask group of 8 bits at a time)

This PR changes these two points by:

• Changing the write mask granularity of the internal bus (ibus) to bits instead of Bytes
• Translating the write mask of every interface to the write mask of the internal bus and back (translating the Byte mask to a bit mask and back)
• Applying the write mask to registers too

The main idea behind this PR is to make the write mask more consistent by applying it to register writes too and to allow for bit manipulations on register writes. We need such a feature for our registers, where we want to manipulate individual bits, e.g. just change the enable bit without modifying any other setting in the register. Our idea was to implement an additional bus interface with a write mask granularity of 1 bit. We don't intend to push that bus interface to this repo, but we thought that the feature itself is nevertheless useful or the overall project, hence this PR.

Of course, as it stands now, none of the existing bus interfaces need such a low granularity. Hence, there is quite some translation from the interface to the internal bus and back for nothing. We assumed this trade-off to be acceptable as the current EDA tools should be able to recognize what is happening and optimize it away.

We tried to add a few settings to disable the new behavior but ultimately failed:

• Adding a write mask granularity setting makes the entire code much more complicated than simply always translating
• Adding a setting to disable write masks for register writes (as it was the case before) proved to complicated as there is no easy way to access the x-hdl options (defined per interface) from the root element (inside genreg.py)

You might have some better idea on how to implement such settings.

[tgingold-cern]

I am not against this idea, but:

I think allowing partial write in registers should be made optional as it slightly increase the number of gates. Default could be on or off (I would slightly prefer to have off as a default, as it is not usual - and backward compatibility).

I haven't looked at your patch now (sorry, I am a little bit overloaded currently).

It is a little bit weird that you need bit granularity, as no standard processors or buses support this. One usual convention for supporting bit access is to have multiple views of the same register. One of the view can be used to clear bits, the second one to set bits and the third one to set the whole register. I also understand you could want to do something more fancy and you need bit granularity.

If bit granularity is supported on registers, you also need to make them available when a register is of type wire (ie, no internal register). Probably in that case user expect only byte enables, but you might need bit enables.

Tell me how urgent is this PR. And do not hesitate to ping me.

[lorenzschmid]

Thanks for the quick reply and having a brief look at it. We would like to use this soonish but there is no urgency right now.

I think allowing partial write in registers should be made optional as it slightly increase the number of gates. Default could be on or off (I would slightly prefer to have off as a default, as it is not usual - and backward compatibility).

I fully agree with adding a setting (and a default to off). As mentioned in my initial comment, I could not find an easy way to propagate an x-hdl setting to genreg.py. (it seems that there are two ways how settings propagate in cheby: via constants in root as well as through opts/BusOption, which depends on the interface (i.e. not the top level memory map)). I would appreciate if you could give me a hint here how to best implement this.

It is a little bit weird that you need bit granularity, as no standard processors or buses support this.

I agree with you. The reason behind is as usual a legacy system. It allows us to do single cycle register writes in a time critical system. Of course, we could use multiple views to but as you concluded correctly for our large register map this would be quite an overhead.

If bit granularity is supported on registers, you also need to make them available when a register is of type wire (ie, no internal register).

I tried to implement the mask for all register types inside genreg.py where it seemed to make sense. Let me know if there is any register (or any other type for that matter) which I overlook.

Thanks again for your feedback. Please let me know how you would like to proceed and of course, no hurry.

[tgingold-cern]

I think the attribute (partial-write ? partial-access ? I am not sure) shouldn't belong to x-hdl but it should be a standard attribute. Simply because it affects the user (it's not an implementation choice, it also indicates if the user can do partial writes).

I think this attribute could be set at any level and then propagated or overridden by the user. Currently the propagation is done manually, and there is in general a c_​ variable that contained the "computed" value for an attribute.

I still have to check your patch about the wire type!

Message ID: @.***>

[lorenzschmid]

Thank you for your message. So one idea of mine was to add it as following:

diff --git a/proto/cheby/layout.py b/proto/cheby/layout.py
index a29d5db..3d384df 100644
--- a/proto/cheby/layout.py
+++ b/proto/cheby/layout.py
@@ -649,12 +649,14 @@ def layout_bus(root, name):
        * c_align_reg
        * c_bussplit
        * c_buserr
+       * c_wmask_reg
        And deduce:
        * c_addr_word_bits
        * c_word_bits"""
     root.c_align_reg = True
     root.c_buserr = False
     root.c_bussplit = False
+    root.c_wmask_reg = False
     if name is None:    # default
         root.c_word_size = 4
         root.c_word_endian = 'big'
diff --git a/proto/cheby/hdl/genreg.py b/proto/cheby/hdl/genreg.py
index 92ac816..2eeebdf 100644
--- a/proto/cheby/hdl/genreg.py
+++ b/proto/cheby/hdl/genreg.py
@@ -129,7 +129,7 @@ class GenFieldReg(GenFieldBase):
         reg, dat, mask = self.extract_reg_dat(
             off, self.field.h_reg, ibus.wr_dat, ibus.wr_sel
         )
-        if mask is not None:
+        if self.root.c_wmask_reg and mask is not None:
             then_stmts.append(
                 HDLAssign(
                     reg,
@@ -157,7 +157,7 @@ class GenFieldWire(GenFieldBase):
             reg, dat, mask = self.extract_reg_dat(
                 off, self.field.h_oport, ibus.wr_dat, ibus.wr_sel
             )
-            if mask is not None:
+            if self.root.c_wmask_reg and mask is not None:
                 stmts.append(
                     HDLAssign(
                         reg,
@@ -193,7 +193,7 @@ class GenFieldAutoclear(GenFieldBase):
         reg, dat, mask = self.extract_reg_dat(
             off, self.field.h_reg, ibus.wr_dat, ibus.wr_sel
         )
-        if mask is not None:
+        if self.root.c_wmask_reg and mask is not None:
             then_stmts.append(
                 HDLAssign(
                     reg,

The problem being that root of GenFieldBase does not always point to the "same" root. Hence, the code above leads to AttributeErrors. It seems that I do not fully understand cheby's configuration and propagation thereof.

[tgingold-cern]

Which tests are failing if you refer to root ?

Depending on whether the register comes from the same memmap or not, the root may differ. But you should always have a root.

[lorenzschmid]

So I added the changes proposed in https://github.com/tgingold-cern/cheby/pull/16#issuecomment-1706032159 with 2a95a322c470620d9589aa0262932c0e736d9589.

The error I receive is upon running python ./cheby/proto/tests.py:

Traceback (most recent call last):
  File "./cheby/proto/tests.py", line 864, in <module>
    main()
  File "./cheby/proto/tests.py", line 841, in main
    test_hdl()
  File "./cheby/proto/tests.py", line 289, in test_hdl
    h = gen_hdl.generate_hdl(sp)
        ^^^^^^^^^^^^^^^^^^^^^^^^
  File "./cheby/proto/cheby/gen_hdl.py", line 330, in generate_hdl
    root.h_gen.gen_processes(ibus)
  File "./cheby/proto/cheby/hdl/genblock.py", line 67, in gen_processes
    n.h_gen.gen_processes(ibus)
  File "./cheby/proto/cheby/hdl/genreg.py", line 466, in gen_processes
    f.h_gen.assign_reg(wr_if.then_stmts, wr_if.else_stmts, off, ibus)
  File "./cheby/proto/cheby/hdl/genreg.py", line 132, in assign_reg
    if self.root.c_wmask_reg and mask is not None:
       ^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'AddressSpace' object has no attribute 'c_wmask_reg'

The error seems to originate from the test_hdl() function and the only two files affected are the following:

• issue129/acdipole_ip
• issue129/acdipole_ip-orig

Probably since they contain address-space:

memory-map:
  ...
  children:
    - address-space:

For all the other tests, there does not seem to be any problem.

[lorenzschmid]

With 2a95a322c470620d9589aa0262932c0e736d9589, the global setting is by default set to be off (previous behavior). Hence, there are only a few changed golden files tested with ./cheby/proto/tests.py.

For the write mask tests newly added to ./cheby/testfiles/tb/run.sh, I wrote a few bash lines temporarily changing the default value before switching it back again.

[tgingold-cern]

In layout.py, you should also update copy_bus to fix the issue with address-spaces.

[lorenzschmid]

Thanks a lot for the quick reply. I just added it to the copy_bus bus function and there is no more error.

I rebased the branch to the current master, i.e. I updated it in regard to the changes in #17. Unfortunately, it seems that I have not added all golden files in #17 (as they were masked by the .gitignore file). So I allowed myself, to include them in this PR instead. Sorry about that.

[lorenzschmid]

I rebased the branch to the current master, i.e. I updated it in regard to the changes in #17. Unfortunately, it seems that I have not added all golden files in #17 (as they were masked by the .gitignore file). So I allowed myself, to include them in this PR instead. Sorry about that.

I added the missing golden files now to a separate PR in #18 as this is the cleaner approached. As soon as it is merged, I will rebase this MR on top of it (which should remove most of the edits in this PR).

[lorenzschmid]

Any updates from your side on this, @tgingold-cern? Do you need anything else from my side for a merge?

[tgingold-cern]

Now merged. Thanks!