NXP / i3c-slave-design

MIPI I3C Basic v1.0 communication Slave source code in Verilog with BSD license to support use in sensors and other devices.
Other
104 stars 33 forks source link

MAP_DA_AUTO[12:8] = 0 causes compile error #25

Closed rosholm closed 4 years ago

rosholm commented 4 years ago

When we set MAP_DA_AUTO[12:8] to 0 as specified on page 15 in the uArch spec when PID is not replaced, it causes these errors because PID is set to MAP_DA_AUTO[12:8]:

############### FATAL MESSAGES ############### 
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
ID       Rule          Alias         Severity    File                                                                                                                          Line    Wt      Message
======================================================================================
[336]    STX_VE_800    STX_VE_800    Syntax      /i3c_regs.v             171     1000    Array bound exceeds integer range in expression [((MAP_CNT * PID_CNT) - 1):0] 
[337]    STX_VE_800    STX_VE_800    Syntax      /i3c_daa_slave.v        159     1000    Array bound exceeds integer range in expression [((MAP_CNT * PID_CNT) - 1):0] 
[338]    STX_VE_800    STX_VE_800    Syntax      /i3c_slave_wrapper.v    256     1000    Array bound exceeds integer range in expression [((MAP_CNT * PID_CNT) - 1):0] 
[339]    STX_VE_800    STX_VE_800    Syntax      /i3c_full_wrapper.v     193     1000    Array bound exceeds integer range in expression [((MAP_CNT * PID_CNT) - 1):0] 

We have made a (temporary) fix by using these settings:

ENA_MAPPED = 5'd1; // Originally 0
MAP_CNT = 4'd1; // Originally 0
MAP_DA_AUTO = 13'h104; // Originally 0

Are we missing something or do you see a better solution?

pkimelman-nxp commented 4 years ago

I recommend MAP_CNT=1 and ENA_MAPPED=0. That is permitted and the default. It avoids the array bounds issue and does not have any impact since ENA_MAPPED=0.

It does appear that PID_CNT gets forced to 0 on MAP_DA_AUTO[12:8]=0 and I guess it should be 1. I will look at this. For one, I should have clipped out the logic in regs since you do not have access to the MAP_AUTO feature in the free version. So, the correct setting for the param, which I will change as the default is:

    parameter         MAP_DA_AUTO={5'd1,1'b0,1'b0,3'd0,1'b0,1'b0,1'b0},

The 5’d0 becomes 5’d1.

Thanks for letting me know.

Regards, Paul

On 12/11/19, 9:24 AM, "rosholm" notifications@github.com wrote:

Caution: EXT Email

When we set MAP_DA_AUTO[12:8] to 0 as specified on page 15 in the uArch spec when PID is not replaced, it causes these errors because PID is set to MAP_DA_AUTO[12:8]: ############### FATAL MESSAGES ############### +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ ID       Rule          Alias         Severity    File                                                                                                                          Line    Wt      Message

[336]    STX_VE_800    STX_VE_800    Syntax      /i3c_regs.v             171     1000    Array bound exceeds integer range in expression [((MAP_CNT PID_CNT) - 1):0] [337]    STX_VE_800    STX_VE_800    Syntax      /i3c_daa_slave.v        159     1000    Array bound exceeds integer range in expression [((MAP_CNT PID_CNT) - 1):0] [338]    STX_VE_800    STX_VE_800    Syntax      /i3c_slave_wrapper.v    256     1000    Array bound exceeds integer range in expression [((MAP_CNT PID_CNT) - 1):0] [339]    STX_VE_800    STX_VE_800    Syntax      /i3c_full_wrapper.v     193     1000    Array bound exceeds integer range in expression [((MAP_CNT PID_CNT) - 1):0] We have made a (temporary) fix by using these settings: ENA_MAPPED = 5'd1; // Originally 0 MAP_CNT = 4'd1; // Originally 0 MAP_DA_AUTO = 13'h104; // Originally 0 Are we missing something or do you see a better solution?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

rosholm commented 4 years ago

Thank you for the fast reply.

However, your suggestion will make bad_map trigger an error due to (~ENA_MAPPED[0]&|MAP_DA_AUTO) resolving to 1 (line 1275 in i3c_full_wrapper.v).

That's why we came up with the solution in my original post:

  1. To prevent the out of bound error, MAP_CNT and PID_CNT both have to be non-zero.
  2. PID_CNT is set to MAP_DA_AUTO[12:8] which thus has to be non-zero.
  3. MAP_DA_AUTO being non-zero forces ENA_MAPPED[0] to be 1 according to the line above.
  4. MAP_DA_AUTO[12:8] being non-zero forces MAP_DA_AUTO[2] to be 1 according to line 1278 in i3c_full_wrapper.v: (~MAP_DA_AUTO[2]&|MAP_DA_AUTO[12:6])

BR /Anders

pkimelman-nxp commented 4 years ago

I will be doing an update in the next or two. This will also include an assertion block for the free version which catches params being used that should not be. It also removes the register logic that cannot be used (just for neatness). Also, some redundant drivers (duplicated assign to 0) are removed. That will also include the micro-arch spec changed to an integration guide along with checklist.

--

Paul Kimelman – Automotive Platform Architecture 408.906.9676

On 12/12/19, 1:35 AM, "rosholm" notifications@github.com wrote:

Caution: EXT Email

Thank you for the fast reply.

However, your suggestion will make bad_map trigger an error due to (~ENA_MAPPED[0]&|MAP_DA_AUTO) resolving to 1 (line 1275 in i3c_full_wrapper.v).

That's why we came up with the solution in my original post:

  1. To prevent the out of bound error, MAP_CNT and PID_CNT both have to be non-zero.

  2. PID_CNT is set to MAP_DA_AUTO[12:8] which thus has to be non-zero.

  3. MAP_DA_AUTO being non-zero forces ENA_MAPPED[0] to be 1 according to the line above.

  4. MAP_DA_AUTO[12:8] being non-zero forces MAP_DA_AUTO[2] to be 1 according to line 1278 in i3c_full_wrapper.v: (~MAP_DA_AUTO[2]&|MAP_DA_AUTO[12:6])

BR /Anders

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

pkimelman-nxp commented 4 years ago

I have updated to branch-tag 1.1.11.a.1.0 which has just minor corrections - no change in logic. This includes fixing the defaults for unused MAP_DA_AUTO to avoid the warnings from tools. It also has the free version cut lines inside the if() of generate so no warnings. The micro-arch spec is now the integration guide with a checklist for back-end. If you have any problems, let me know. Since very minor changes, this was not run through the test system. One final note: because every file shows its version tag, GitHub will say I changed every file.

rosholm commented 4 years ago

The new code has syntax error in i3c_full_wrapper.v line 1294: (|ENA_HDR) || (|ENA_TIMEC)); there is one end parenthesis too many.

Also, there still is an issue with the suggested/default configuration of ENA_MAPPED and MAP_DA_AUTO. The two expressions below (line 1278 and 1280) both evaluated to 1:

                   (~ENA_MAPPED[0]&|MAP_DA_AUTO) ||

and (~MAP_DA_AUTO[2]&|MAP_DA_AUTO[12:7]) ||

BR /Anders

pkimelman-nxp commented 4 years ago

Ouch. OK, my apologies. I did a fast merge and go, so did not expect any issues – I did not run any tests since was not a change in logic.  I will fix this.

The new code has syntax error in i3c_full_wrapper.v line 1294: (|ENA_HDR) || (|ENA_TIMEC)); there is one end parenthesis too many.

I will fix and update.

Also, there still is an issue with the suggested/default configuration of ENA_MAPPED and MAP_DA_AUTO. The two expressions below (line 1278 and 1280) both evaluated to 1:                    (~ENA_MAPPED[0]&|MAP_DA_AUTO) ||

OK, this needs the same [12:9] test.

and (~MAP_DA_AUTO[2]&|MAP_DA_AUTO[12:7]) ||

This should be 12:9. I will fix.

BR /Anders

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

pkimelman-nxp commented 4 years ago

Update checked in. Note that previous one had tag number backwards. Should be 1.1.11.a.0.1 and not 1.0. So, fixed that as well (matches source control, so can be pulled easily). The change applied only was the 3 assertions. The 12:9 vector allows "1" in the bottom bit of the PID width.