pulp-platform / pulp_soc

pulp_soc is the core building component of PULP based SoCs
Other
78 stars 81 forks source link

Pad and GPIO configuration parameters not used completely in the full design #21

Closed dronox closed 4 years ago

dronox commented 4 years ago

Hi guys,

in the latest commit of pulp_soc the parameters mentioned in the issue title (NGPIO, NBIT_PADCFG and NBIT_PADMUX) are not used completely, i.e. line 964 to 991 looks as follows:


    //********************************************************
    //*** PAD AND GPIO CONFIGURATION SIGNALS PACK ************
    //********************************************************

    generate
        for (i=0; i<32; i++) begin
            for (j=0; j<6; j++) begin
                assign gpio_cfg_o[j+6*i] = s_gpio_cfg[i][j];
            end
        end
    endgenerate

    generate
        for (i=0; i<64; i++) begin
            for (j=0; j<2; j++) begin
                assign pad_mux_o[j+2*i] = s_pad_mux[i][j];
            end
        end
    endgenerate

    generate
        for (i=0; i<64; i++) begin
            for (j=0; j<6; j++) begin
                assign pad_cfg_o[j+6*i] = s_pad_cfg[i][j];
            end
        end
    endgenerate

This is basically correct for the configuration of these signals in line 304 to 306:


    logic [31:0][5:0]      s_gpio_cfg;
    logic [63:0][1:0]      s_pad_mux;
    logic [63:0][5:0]      s_pad_cfg;

Unfortunately, the corresponding connections to soc_peripherals look as follows:


    output logic [NGPIO-1:0][NBIT_PADCFG-1:0] gpio_padcfg,

    output logic [NPAD-1:0][NBIT_PADMUX-1:0] pad_mux_o,
    output logic [NPAD-1:0][NBIT_PADCFG-1:0] pad_cfg_o,

This leads to port width mismatch issues and - in case your parameterized pad configuration is bigger than the statically implemented there are also functional issues.

So, I assume that the signals in pulp_soc should be defined like this:

    logic [NGPIO-1:0][NBIT_PADCFG-1:0]     s_gpio_cfg;
    logic [NPAD-1:0][NBIT_PADMUX-1:0]      s_pad_mux;
    logic [NPAD-1:0][NBIT_PADCFG-1:0]      s_pad_cfg;

And then the configuration assignments (line 964 to 991) then need to be changed as follows:

    //********************************************************
    //*** PAD AND GPIO CONFIGURATION SIGNALS PACK ************
    //********************************************************

    generate

        for (i=0; i<NGPIO; i++) begin
            for (j=0; j<NBIT_PADCFG; j++) begin
                assign gpio_cfg_o[j+6*i] = s_gpio_cfg[i][j];
            end
        end
    endgenerate

    generate
        for (i=0; i<NPAD; i++) begin
            for (j=0; j<NBIT_PADMUX; j++) begin
                assign pad_mux_o[j+2*i] = s_pad_mux[i][j];
            end
        end
    endgenerate

    generate
        for (i=0; i<NPAD; i++) begin
            for (j=0; j<NBIT_PADCFG;j++) begin
                assign pad_cfg_o[j+6*i] = s_pad_cfg[i][j];
            end
        end
    endgenerate

Furthermore, you then need to add the pad parameters to apb_soc_ctrl.sv:

module apb_soc_ctrl
  #(
    parameter APB_ADDR_WIDTH = 12,  // APB slaves are 4KB by default
    parameter NB_CLUSTERS    = 0,   // N_CLUSTERS
    parameter NB_CORES       = 4,   // N_CORES
    parameter NPAD           = 64,
    parameter NBIT_PADCFG    = 4,
    parameter NBIT_PADMUX    = 2,
    parameter JTAG_REG_SIZE  = 8
    )

And the corresponding pad_mux and pad_cfg ports need also to be parameterized:

    output logic         [NPAD-1:0][NBIT_PADCFG-1:0] pad_cfg,
    output logic         [NPAD-1:0][NBIT_PADMUX-1:0] pad_mux,

Then all port width mismatch issues and also functional issues if you change the pad or gpio configuration width are gone.

Furthermore: Is it possible that there is a typo in the NGPIO setting? You set it to 43 in pulp_soc.sv (line 33) but you leave all gpio pin widths at 32 so I assume 32 is the right choice?

Thanks & Best Regards, Dustin

bluewww commented 4 years ago

Thanks this definitely looks like an issue. @meggiman do you have already fixes regarding this from your last tapeout?

bluewww commented 4 years ago

Fixed in #28