From 17d3d3202334157b2b4fb43c2bebed8f00a6d8a1 Mon Sep 17 00:00:00 2001 From: Javier Mora Date: Fri, 12 Jul 2024 16:59:47 +0200 Subject: [PATCH 1/2] peripheral_subsystem.sv.tpl: fix bug #548; use keys The use of `in` for string comparison results in `"name" in "name2"` giving a false positive; the right thing is to use `==`. In peripheral_subsystem.sv.tpl, this may result in the peripheral being added twice, or being added when it shouldn't. This has been fixed in this commit. In addition, the way in which dict keys are handled has been changed from iterating through all the keys to just finding the relevant key. --- .../peripheral_subsystem.sv.tpl | 48 ++++--------------- 1 file changed, 8 insertions(+), 40 deletions(-) diff --git a/hw/core-v-mini-mcu/peripheral_subsystem.sv.tpl b/hw/core-v-mini-mcu/peripheral_subsystem.sv.tpl index 12fc624fd..68d08acc5 100644 --- a/hw/core-v-mini-mcu/peripheral_subsystem.sv.tpl +++ b/hw/core-v-mini-mcu/peripheral_subsystem.sv.tpl @@ -292,9 +292,7 @@ module peripheral_subsystem .reg_rsp_o(peripheral_slv_rsp[core_v_mini_mcu_pkg::RV_PLIC_IDX]) ); -% for peripheral in peripherals.items(): -% if peripheral[0] in ("rv_plic"): -% if peripheral[1]['is_included'] in ("yes"): +% if 'rv_plic' in peripherals and peripherals['rv_plic']['is_included'] == 'yes': rv_plic rv_plic_i ( .clk_i(clk_cg), .rst_ni, @@ -315,12 +313,8 @@ module peripheral_subsystem assign irq_plic_o = '0; assign plic_tl_d2h = '0; % endif -% endif -% endfor -% for peripheral in peripherals.items(): -% if peripheral[0] in ("spi_host"): -% if peripheral[1]['is_included'] in ("yes"): +% if 'spi_host' in peripherals and peripherals['spi_host']['is_included'] == 'yes': spi_host #( .reg_req_t(reg_pkg::reg_req_t), .reg_rsp_t(reg_pkg::reg_rsp_t) @@ -357,14 +351,10 @@ module peripheral_subsystem assign spi_rx_valid_o = '0; assign spi_tx_ready_o = '0; % endif -% endif -% endfor -% for peripheral in peripherals.items(): -% if peripheral[0] in ("gpio"): -% if peripheral[1]['is_included'] in ("yes"): +% if 'gpio' in peripherals and peripherals['gpio']['is_included'] == 'yes': gpio #( .reg_req_t(reg_pkg::reg_req_t), .reg_rsp_t(reg_pkg::reg_rsp_t) @@ -386,8 +376,6 @@ module peripheral_subsystem assign gpio_intr = '0; assign peripheral_slv_rsp[core_v_mini_mcu_pkg::GPIO_IDX] = '0; % endif -% endif -% endfor reg_to_tlul #( .req_t(reg_pkg::reg_req_t), @@ -406,9 +394,7 @@ module peripheral_subsystem .reg_rsp_o(peripheral_slv_rsp[core_v_mini_mcu_pkg::I2C_IDX]) ); -% for peripheral in peripherals.items(): -% if peripheral[0] in ("i2c"): -% if peripheral[1]['is_included'] in ("yes"): +% if 'i2c' in peripherals and peripherals['i2c']['is_included'] == 'yes': i2c i2c_i ( .clk_i(clk_cg), .rst_ni, @@ -460,8 +446,6 @@ module peripheral_subsystem assign i2c_intr_ack_stop = '0; assign i2c_intr_host_timeout = '0; % endif -% endif -% endfor reg_to_tlul #( .req_t(reg_pkg::reg_req_t), @@ -480,9 +464,7 @@ module peripheral_subsystem .reg_rsp_o(peripheral_slv_rsp[core_v_mini_mcu_pkg::RV_TIMER_IDX]) ); -% for peripheral in peripherals.items(): -% if peripheral[0] in ("rv_timer"): -% if peripheral[1]['is_included'] in ("yes"): +% if 'rv_timer' in peripherals and peripherals['rv_timer']['is_included'] == 'yes': rv_timer rv_timer_2_3_i ( .clk_i(clk_cg), .rst_ni, @@ -496,12 +478,8 @@ module peripheral_subsystem assign rv_timer_2_intr_o = '0; assign rv_timer_3_intr_o = '0; % endif -% endif -% endfor -% for peripheral in peripherals.items(): -% if peripheral[0] in ("spi2"): -% if peripheral[1]['is_included'] in ("yes"): +% if 'spi2' in peripherals and peripherals['spi2']['is_included'] == 'yes': spi_host #( .reg_req_t(reg_pkg::reg_req_t), .reg_rsp_t(reg_pkg::reg_rsp_t) @@ -536,12 +514,8 @@ module peripheral_subsystem assign spi2_sd_en_o = '0; assign spi2_intr_event = '0; % endif -% endif -% endfor -% for peripheral in peripherals.items(): -% if peripheral[0] in ("pdm2pcm"): -% if peripheral[1]['is_included'] in ("yes"): +% if 'pdm2pcm' in peripherals and peripherals['pdm2pcm']['is_included'] == 'yes': pdm2pcm #( .reg_req_t(reg_pkg::reg_req_t), .reg_rsp_t(reg_pkg::reg_rsp_t) @@ -557,14 +531,10 @@ module peripheral_subsystem assign peripheral_slv_rsp[core_v_mini_mcu_pkg::PDM2PCM_IDX] = '0; assign pdm2pcm_clk_o = '0; % endif -% endif -% endfor assign pdm2pcm_clk_en_o = 1; -% for peripheral in peripherals.items(): -% if peripheral[0] in ("i2s"): -% if peripheral[1]['is_included'] in ("yes"): +% if 'i2s' in peripherals and peripherals['i2s']['is_included'] == 'yes': i2s #( .reg_req_t(reg_pkg::reg_req_t), .reg_rsp_t(reg_pkg::reg_rsp_t) @@ -598,7 +568,5 @@ module peripheral_subsystem assign i2s_intr_event = 1'b0; assign i2s_rx_valid_o = 1'b0; % endif -% endif -% endfor endmodule : peripheral_subsystem From 9c70f9439cd63d5d6b42906bb8247fce0bb6bb61 Mon Sep 17 00:00:00 2001 From: Javier Mora Date: Fri, 12 Jul 2024 17:31:03 +0200 Subject: [PATCH 2/2] mcu_gen.py: replace `in` with `==` for string cmp This is the same issue that was found at bug #548, although in this case it has not shown any adverse effects *for now*. Nevertheless, it is never a bad idea to fix it as well just in case. Also fixed file `pad_cfg.hjson` which incorrectly adds a comma after quoteless string `active: low,` which isn't valid Hjson. This worked before because `"low" in "low,"` is technically true, but this was just one bug hiding another bug. This issue affects several other files and will be reported separately. --- pad_cfg.hjson | 4 ++-- util/mcu_gen.py | 30 ++++++++++++++---------------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/pad_cfg.hjson b/pad_cfg.hjson index 1cfa8c798..0bfa7ae88 100644 --- a/pad_cfg.hjson +++ b/pad_cfg.hjson @@ -33,7 +33,7 @@ }, rst: { num: 1, - active: low, + active: low driven_manually: True type: input }, @@ -55,7 +55,7 @@ }, jtag_trst: { num: 1, - active: low, + active: low type: input }, jtag_tdi: { diff --git a/util/mcu_gen.py b/util/mcu_gen.py index c661d780d..433feb5bf 100755 --- a/util/mcu_gen.py +++ b/util/mcu_gen.py @@ -219,7 +219,7 @@ def __init__(self, name, cell_name, pad_type, pad_mapping, index, pad_active, pa self.pad_mapping = pad_mapping self.pad_mux_list = pad_mux_list - if('low' in pad_active): + if pad_active == 'low': name_active = 'n' else: name_active = '' @@ -474,7 +474,7 @@ def extract_peripherals(peripherals): if isinstance(info, dict): new_info = {} for k, v in info.items(): - if k not in ("is_included"): + if k != "is_included": new_info[k] = string2int(v) else: new_info[k] = v @@ -487,7 +487,7 @@ def discard_path(peripherals): new = {} for k,v in peripherals.items(): if isinstance(v, dict): - new[k] = {key:val for key,val in v.items() if key not in ("path")} + new[k] = {key:val for key,val in v.items() if key != "path"} else: new[k] = v return new @@ -496,10 +496,8 @@ def len_extracted_peripherals(peripherals): len_ep = 0 for name, info in peripherals.items(): if isinstance(info, dict): - for k, v in info.items(): - if k in ("is_included"): - if v in ("yes"): - len_ep += 1 + if info['is_included'] == "yes": + len_ep += 1 return len_ep ao_peripherals = extract_peripherals(discard_path(obj['ao_peripherals'])) @@ -598,7 +596,7 @@ def len_extracted_peripherals(peripherals): pad_mux_list_hjson = [] try: - if ('True' in pads[key]['driven_manually']): + if pads[key]['driven_manually'] == 'True': pad_driven_manually = True else: pad_driven_manually = False @@ -606,7 +604,7 @@ def len_extracted_peripherals(peripherals): pad_driven_manually = False try: - if ('True' in pads[key]['skip_declaration']): + if pads[key]['skip_declaration'] == 'True': pad_skip_declaration = True else: pad_skip_declaration = False @@ -614,7 +612,7 @@ def len_extracted_peripherals(peripherals): pad_skip_declaration = False try: - if ('True' in pads[key]['keep_internal']): + if pads[key]['keep_internal'] == 'True': pad_keep_internal = True else: pad_keep_internal = False @@ -631,7 +629,7 @@ def len_extracted_peripherals(peripherals): pad_active_mux = 'high' try: - if ('True' in pads[key]['mux'][pad_mux]['driven_manually']): + if pads[key]['mux'][pad_mux]['driven_manually'] == 'True': pad_driven_manually_mux = True else: pad_driven_manually_mux = False @@ -639,7 +637,7 @@ def len_extracted_peripherals(peripherals): pad_driven_manually_mux = False try: - if ('True' in pads[key]['mux'][pad_mux]['skip_declaration']): + if pads[key]['mux'][pad_mux]['skip_declaration'] == 'True': pad_skip_declaration_mux = True else: pad_skip_declaration_mux = False @@ -717,7 +715,7 @@ def len_extracted_peripherals(peripherals): pad_mux_list_hjson = [] try: - if ('True' in external_pads[key]['driven_manually']): + if external_pads[key]['driven_manually'] == 'True': pad_driven_manually = True else: pad_driven_manually = False @@ -725,7 +723,7 @@ def len_extracted_peripherals(peripherals): pad_driven_manually = False try: - if ('True' in external_pads[key]['skip_declaration']): + if external_pads[key]['skip_declaration'] == 'True': pad_skip_declaration = True else: pad_skip_declaration = False @@ -742,7 +740,7 @@ def len_extracted_peripherals(peripherals): pad_active_mux = 'high' try: - if ('True' in external_pads[key]['mux'][pad_mux]['driven_manually']): + if external_pads[key]['mux'][pad_mux]['driven_manually'] == 'True': pad_driven_manually_mux = True else: pad_driven_manually_mux = False @@ -750,7 +748,7 @@ def len_extracted_peripherals(peripherals): pad_driven_manually_mux = False try: - if ('True' in external_pads[key]['mux'][pad_mux]['skip_declaration']): + if external_pads[key]['mux'][pad_mux]['skip_declaration'] == 'True': pad_skip_declaration_mux = True else: pad_skip_declaration_mux = False