[reggen] Changing multireg structure

[eunchan]

To support nested multireg (eventually to the parameterized reggen), I want to suggest revised multireg data structure.

currently, it is:

{ multireg: {
    cname: "IP",
    name: "REG",
    count: "ParamA",
    fields: [
    ]
  }
}

This doesn't allow nested multireg with same format. So it could be easier if we change the structure as below

{ multireg: {
    count: "ParamA",
    register: {
      name: "REG",
      fields: [
      ]
    }
}

if nested is used (eventually)

{ multireg: {
    count: "ParamA",
    multireg: {
      count: "ParamB",
      register: {
        name: "REG",
      }
    }
  }
}

CC: @mdhayter

This is related to #30 #47 #266

[msfschaffner]

Sounds like a reasonable change to me.

[sjgitty]

how big of an effort is it to implement this?

[eunchan]

First stage would take less than a week I think. Nested multireg would take another a or two weeks if counted on the document, c header, and dv template also.

On Thu, Sep 26, 2019, 5:43 AM Scott Johnson notifications@github.com wrote:

how big of an effort is it to implement this?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAJDG3RCXLPIENN2VJCRBXLQLSU55A5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7VNRWQ#issuecomment-535484634, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJDG3VSS3VIE4II2ETFQDTQLSU55ANCNFSM4I2SZW2A .

[tjaychen]

just to clarify Eunchan, i don't think this is something we need to have by 0.5 right..? (i could be wrong)

On Thu, Sep 26, 2019 at 7:36 AM Eunchan Kim notifications@github.com wrote:

First stage would take less than a week I think. Nested multireg would take another a or two weeks if counted on the document, c header, and dv template also.

On Thu, Sep 26, 2019, 5:43 AM Scott Johnson notifications@github.com wrote:

how big of an effort is it to implement this?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub < https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAJDG3RCXLPIENN2VJCRBXLQLSU55A5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7VNRWQ#issuecomment-535484634 , or mute the thread < https://github.com/notifications/unsubscribe-auth/AAJDG3VSS3VIE4II2ETFQDTQLSU55ANCNFSM4I2SZW2A

.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAH2RSXTJE335ZHFERFMQFLQLTCHBA5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7VZM7I#issuecomment-535533181, or mute the thread https://github.com/notifications/unsubscribe-auth/AAH2RSRKJ3YX2YETNJUHWILQLTCHBANCNFSM4I2SZW2A .

[eunchan]

On Thu, Sep 26, 2019 at 12:03:35PM -0700, tjaychen wrote:

just to clarify Eunchan, i don't think this is something we need to have by 0.5 right..? (i could be wrong)

At least, I want to have the first step.

On Thu, Sep 26, 2019 at 7:36 AM Eunchan Kim notifications@github.com wrote:

First stage would take less than a week I think. Nested multireg would take another a or two weeks if counted on the document, c header, and dv template also.

On Thu, Sep 26, 2019, 5:43 AM Scott Johnson notifications@github.com wrote:

how big of an effort is it to implement this?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub < https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAJDG3RCXLPIENN2VJCRBXLQLSU55A5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7VNRWQ#issuecomment-535484634 , or mute the thread < https://github.com/notifications/unsubscribe-auth/AAJDG3VSS3VIE4II2ETFQDTQLSU55ANCNFSM4I2SZW2A

.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAH2RSXTJE335ZHFERFMQFLQLTCHBA5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7VZM7I#issuecomment-535533181, or mute the thread https://github.com/notifications/unsubscribe-auth/AAH2RSRKJ3YX2YETNJUHWILQLTCHBANCNFSM4I2SZW2A .

-- You are receiving this because you were assigned. Reply to this email directly or view it on GitHub: https://github.com/lowRISC/opentitan/issues/270#issuecomment-535642887

[tjaychen]

ah that sounds good.

On Thu, Sep 26, 2019 at 12:05 PM Eunchan Kim notifications@github.com wrote:

On Thu, Sep 26, 2019 at 12:03:35PM -0700, tjaychen wrote:

just to clarify Eunchan, i don't think this is something we need to have by 0.5 right..? (i could be wrong)

At least, I want to have the first step.

On Thu, Sep 26, 2019 at 7:36 AM Eunchan Kim notifications@github.com wrote:

First stage would take less than a week I think. Nested multireg would take another a or two weeks if counted on the document, c header, and dv template also.

On Thu, Sep 26, 2019, 5:43 AM Scott Johnson notifications@github.com wrote:

how big of an effort is it to implement this?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <

https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAJDG3RCXLPIENN2VJCRBXLQLSU55A5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7VNRWQ#issuecomment-535484634

, or mute the thread <

https://github.com/notifications/unsubscribe-auth/AAJDG3VSS3VIE4II2ETFQDTQLSU55ANCNFSM4I2SZW2A

.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub < https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAH2RSXTJE335ZHFERFMQFLQLTCHBA5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7VZM7I#issuecomment-535533181 , or mute the thread < https://github.com/notifications/unsubscribe-auth/AAH2RSRKJ3YX2YETNJUHWILQLTCHBANCNFSM4I2SZW2A

.

-- You are receiving this because you were assigned. Reply to this email directly or view it on GitHub: https://github.com/lowRISC/opentitan/issues/270#issuecomment-535642887

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAH2RSXVQGQ254JEI3ANQ5DQLUBW5A5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7WULQQ#issuecomment-535643586, or mute the thread https://github.com/notifications/unsubscribe-auth/AAH2RSQSRME7FWLWIV5GPOLQLUBW5ANCNFSM4I2SZW2A .

[eunchan]

When Tim, Michael, and I discussed about this issue (related to #272 ), looks like reggen should support multiple fields in a multireg. And multireg will have compact field to indicate whether the register fields need to be packed into a register width. For instance,

{ multireg: {
   count: "ParamA",
   compact: "true",
   register: {
     name: "REG"
     field: [
      { bit: "0", name: "field_a" }
     ]
   }
  }
}

If ParamA is 32, then the multireg is paced into a register. Though HW access doesn't change. reg2hw.mreg_reg[N].q.

If compact is false (default value), it creates 32 registers occupying 4B register space for each register. Still access to each register is reg2hw.mreg_reg[N].q.

compact is allowed to be true only when the entity of the register fields is only one. If multiple fields are defined, the tool always unrolls ( count number of registers ).

I will modify the TP document

The goal before November is to achieve the first step (not nested), supporting compact is TBD.

[tjaychen]

@eunchan I also wanted to check with you on a specific case. With the new compacting control, what in your opinion should we do with the regwen_incr type functions? I'm pretty sure we will still have cases (even if 1 field only) where we want to separately lock.

My current thinking is that we could make compact=true and regwen_incr type functions incompatible. Ie, if you compact, you cannot also expect to be able to increment regwen, since it is after all reg and not field.

What do you think? You might have a better solution already, but wanted to discuss it.

I think the same solution could apply if we wanted multireg regwen / registers that exceed 32 in count.

[eunchan]

I personally prefer deals with it in reggen validate function and connects the link between the original register and enable register (or between fields) not from the template.

It needs introducing a few member methods in field or register classes but regardless of regwen incr, the functions are still needed anyway.

On Mon, Sep 30, 2019, 12:07 PM tjaychen notifications@github.com wrote:

@eunchan https://github.com/eunchan I also wanted to check with you on a specific case. With the new compacting control, what in your opinion should we do with the regwen_incr type functions? I'm pretty sure we will still have cases (even if 1 field only) where we want to separately lock them.

My current thinking is that we could make compact=true and regwen_incr type functions incompatible. Ie, if you compact, you cannot also expect to be able to increment regwen, since it is after all reg and not field.

What do you think? You might have a better solution already, but wanted to discuss it.

I think the same solution could apply if we wanted multireg regwen / registers that exceed 32 in count.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAJDG3T3QAGAC5PFIHHSIH3QMJE7BA5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD76X5II#issuecomment-536706721, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJDG3X225G6IMXTKZSFRFLQMJE7BANCNFSM4I2SZW2A .

[tjaychen]

that sounds good, but do you think we should we should restrict it's usage at all based on how others bits are structured? Or you think it can be made flexible enough that there's no need to do so?

On Mon, Sep 30, 2019 at 1:12 PM Eunchan Kim notifications@github.com wrote:

I personally prefer deals with it in reggen validate function and connects the link between the original register and enable register (or between fields) not from the template.

It needs introducing a few member methods in field or register classes but regardless of regwen incr, the functions are still needed anyway.

On Mon, Sep 30, 2019, 12:07 PM tjaychen notifications@github.com wrote:

@eunchan https://github.com/eunchan I also wanted to check with you on a specific case. With the new compacting control, what in your opinion should we do with the regwen_incr type functions? I'm pretty sure we will still have cases (even if 1 field only) where we want to separately lock them.

My current thinking is that we could make compact=true and regwen_incr type functions incompatible. Ie, if you compact, you cannot also expect to be able to increment regwen, since it is after all reg and not field.

What do you think? You might have a better solution already, but wanted to discuss it.

I think the same solution could apply if we wanted multireg regwen / registers that exceed 32 in count.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAJDG3T3QAGAC5PFIHHSIH3QMJE7BA5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD76X5II#issuecomment-536706721 , or mute the thread < https://github.com/notifications/unsubscribe-auth/AAJDG3X225G6IMXTKZSFRFLQMJE7BANCNFSM4I2SZW2A

.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAH2RSWYC7IERBYVXJ63S7TQMJMRXA5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7655OY#issuecomment-536731323, or mute the thread https://github.com/notifications/unsubscribe-auth/AAH2RSUSTJGLDYEQHSFXFQLQMJMRXANCNFSM4I2SZW2A .

[eunchan]

On Mon, Sep 30, 2019 at 01:17:34PM -0700, tjaychen wrote:

that sounds good, but do you think we should we should restrict it's usage at all based on how others bits are structured? Or you think it can be made flexible enough that there's no need to do so?

I think we can make it flexible. For instance,

If a register uses regwen field, then the tool can search the name of regwen in the register field, the one before unroll the multi register. And the original register can have a connection (pointer) to the wen register. The register has a method to get the actual field of the register. If the WEN register isn't multireg, then, the original register can assume that the WEN is connected to a single ENABLE field. If not and if the original register also is multireg, then we can assume regwen_inr is true and matching the field of REGWEN to original register.

The Register, or Field class could have a method to get the actual signal name. So the template only will need to call the method like below:

assign ${r.print_wen()} = ${r.wen.print_sig()};

I think this isn't hard to implement. WDTY?

On Mon, Sep 30, 2019 at 1:12 PM Eunchan Kim notifications@github.com wrote:

I personally prefer deals with it in reggen validate function and connects the link between the original register and enable register (or between fields) not from the template.

It needs introducing a few member methods in field or register classes but regardless of regwen incr, the functions are still needed anyway.

On Mon, Sep 30, 2019, 12:07 PM tjaychen notifications@github.com wrote:

@eunchan https://github.com/eunchan I also wanted to check with you on a specific case. With the new compacting control, what in your opinion should we do with the regwen_incr type functions? I'm pretty sure we will still have cases (even if 1 field only) where we want to separately lock them.

My current thinking is that we could make compact=true and regwen_incr type functions incompatible. Ie, if you compact, you cannot also expect to be able to increment regwen, since it is after all reg and not field.

What do you think? You might have a better solution already, but wanted to discuss it.

I think the same solution could apply if we wanted multireg regwen / registers that exceed 32 in count.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAJDG3T3QAGAC5PFIHHSIH3QMJE7BA5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD76X5II#issuecomment-536706721 , or mute the thread < https://github.com/notifications/unsubscribe-auth/AAJDG3X225G6IMXTKZSFRFLQMJE7BANCNFSM4I2SZW2A

.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAH2RSWYC7IERBYVXJ63S7TQMJMRXA5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7655OY#issuecomment-536731323, or mute the thread https://github.com/notifications/unsubscribe-auth/AAH2RSUSTJGLDYEQHSFXFQLQMJMRXANCNFSM4I2SZW2A .

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/lowRISC/opentitan/issues/270#issuecomment-536733364

[tjaychen]

that sounds like a pretty good plan. So I guess in this scenario, since you're only referring to the multireg register as the pointer, you can also work out how far it increments later on?

On Mon, Sep 30, 2019 at 1:34 PM Eunchan Kim notifications@github.com wrote:

On Mon, Sep 30, 2019 at 01:17:34PM -0700, tjaychen wrote:

that sounds good, but do you think we should we should restrict it's usage at all based on how others bits are structured? Or you think it can be made flexible enough that there's no need to do so?

I think we can make it flexible. For instance,

If a register uses regwen field, then the tool can search the name of regwen in the register field, the one before unroll the multi register. And the original register can have a connection (pointer) to the wen register. The register has a method to get the actual field of the register. If the WEN register isn't multireg, then, the original register can assume that the WEN is connected to a single ENABLE field. If not and if the original register also is multireg, then we can assume regwen_inr is true and matching the field of REGWEN to original register.

The Register, or Field class could have a method to get the actual signal name. So the template only will need to call the method like below:

assign ${r.print_wen()} = ${r.wen.print_sig()};

I think this isn't hard to implement. WDTY?

On Mon, Sep 30, 2019 at 1:12 PM Eunchan Kim notifications@github.com wrote:

I personally prefer deals with it in reggen validate function and connects the link between the original register and enable register (or between fields) not from the template.

It needs introducing a few member methods in field or register classes but regardless of regwen incr, the functions are still needed anyway.

On Mon, Sep 30, 2019, 12:07 PM tjaychen notifications@github.com wrote:

@eunchan https://github.com/eunchan I also wanted to check with you on a specific case. With the new compacting control, what in your opinion should we do with the regwen_incr type functions? I'm pretty sure we will still have cases (even if 1 field only) where we want to separately lock them.

My current thinking is that we could make compact=true and regwen_incr type functions incompatible. Ie, if you compact, you cannot also expect to be able to increment regwen, since it is after all reg and not field.

What do you think? You might have a better solution already, but wanted to discuss it.

I think the same solution could apply if we wanted multireg regwen / registers that exceed 32 in count.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <

https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAJDG3T3QAGAC5PFIHHSIH3QMJE7BA5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD76X5II#issuecomment-536706721

, or mute the thread <

https://github.com/notifications/unsubscribe-auth/AAJDG3X225G6IMXTKZSFRFLQMJE7BANCNFSM4I2SZW2A

.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub < https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAH2RSWYC7IERBYVXJ63S7TQMJMRXA5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7655OY#issuecomment-536731323 , or mute the thread < https://github.com/notifications/unsubscribe-auth/AAH2RSUSTJGLDYEQHSFXFQLQMJMRXANCNFSM4I2SZW2A

.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/lowRISC/opentitan/issues/270#issuecomment-536733364

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAH2RSRRP224RMQFFOE54NDQMJPFPA5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7677WQ#issuecomment-536739802, or mute the thread https://github.com/notifications/unsubscribe-auth/AAH2RSXUGMDGXMMS3WTW3A3QMJPFPANCNFSM4I2SZW2A .

[eunchan]

On Mon, Sep 30, 2019 at 01:55:24PM -0700, tjaychen wrote:

that sounds like a pretty good plan. So I guess in this scenario, since you're only referring to the multireg register as the pointer, you can also work out how far it increments later on?

Right. If multireg uses the parameter as count value, then we can check if they expands to the same number of registers( or fields if compact) and raises an error if that's not the case.

On Mon, Sep 30, 2019 at 1:34 PM Eunchan Kim notifications@github.com wrote:

On Mon, Sep 30, 2019 at 01:17:34PM -0700, tjaychen wrote:

that sounds good, but do you think we should we should restrict it's usage at all based on how others bits are structured? Or you think it can be made flexible enough that there's no need to do so?

I think we can make it flexible. For instance,

If a register uses regwen field, then the tool can search the name of regwen in the register field, the one before unroll the multi register. And the original register can have a connection (pointer) to the wen register. The register has a method to get the actual field of the register. If the WEN register isn't multireg, then, the original register can assume that the WEN is connected to a single ENABLE field. If not and if the original register also is multireg, then we can assume regwen_inr is true and matching the field of REGWEN to original register.

The Register, or Field class could have a method to get the actual signal name. So the template only will need to call the method like below:

assign ${r.print_wen()} = ${r.wen.print_sig()};

I think this isn't hard to implement. WDTY?

On Mon, Sep 30, 2019 at 1:12 PM Eunchan Kim notifications@github.com wrote:

I personally prefer deals with it in reggen validate function and connects the link between the original register and enable register (or between fields) not from the template.

It needs introducing a few member methods in field or register classes but regardless of regwen incr, the functions are still needed anyway.

On Mon, Sep 30, 2019, 12:07 PM tjaychen notifications@github.com wrote:

@eunchan https://github.com/eunchan I also wanted to check with you on a specific case. With the new compacting control, what in your opinion should we do with the regwen_incr type functions? I'm pretty sure we will still have cases (even if 1 field only) where we want to separately lock them.

My current thinking is that we could make compact=true and regwen_incr type functions incompatible. Ie, if you compact, you cannot also expect to be able to increment regwen, since it is after all reg and not field.

What do you think? You might have a better solution already, but wanted to discuss it.

I think the same solution could apply if we wanted multireg regwen / registers that exceed 32 in count.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <

https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAJDG3T3QAGAC5PFIHHSIH3QMJE7BA5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD76X5II#issuecomment-536706721

, or mute the thread <

https://github.com/notifications/unsubscribe-auth/AAJDG3X225G6IMXTKZSFRFLQMJE7BANCNFSM4I2SZW2A

.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub < https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAH2RSWYC7IERBYVXJ63S7TQMJMRXA5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7655OY#issuecomment-536731323 , or mute the thread < https://github.com/notifications/unsubscribe-auth/AAH2RSUSTJGLDYEQHSFXFQLQMJMRXANCNFSM4I2SZW2A

.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/lowRISC/opentitan/issues/270#issuecomment-536733364

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAH2RSRRP224RMQFFOE54NDQMJPFPA5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7677WQ#issuecomment-536739802, or mute the thread https://github.com/notifications/unsubscribe-auth/AAH2RSXUGMDGXMMS3WTW3A3QMJPFPANCNFSM4I2SZW2A .

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/lowRISC/opentitan/issues/270#issuecomment-536748406

[msfschaffner]

Yes that sounds like a good plan, and having this pointer also allows us to do some more rigorous crosschecking than we do right now.

[eunchan]

@asb can anyone weigh-in the opinion from lowRISC?

[eunchan]

One thing I can slightly revise is to combine nested multireg into single multireg and have count to accept list of params. Then if the members of count indicates the dimension.

{   name: "REG_A",
    count: ["ParamA", "ParamB"],
    swaccess, hwaccess, etc,
    fields: [ {field1} {field2} ]
  }
}

Then we don't have to have multireg field but count indicates multiregister. Or we can have type field indicates multi.

If we need to have a loop for multiple registers such as :

{ multireg: {
    count: "ParamA",
    registers: [
      { name: "REG_A",
      },
      { name: "REG_B",
      }
    ]
  }
}

Then, we need multireg field, but count still can be a list.

[tjaychen]

just to make sure i understand, in the first example, paramA would basically be the count of the outer loop, and paramB the inner loop? so its effectively equivalent to multireg count: ParamA multireg count: ParamB Fields?

I actually like that approach since it means the structure underneath registers is all consistent without an extra multireg layer. And for the second case, couldn't we actually require they just split it up into two instances of the first? It's a little bit more typing, but IMO worth it.

On Tue, Oct 8, 2019 at 11:57 AM Eunchan Kim notifications@github.com wrote:

One thing I can slightly revise is to combine nested multireg into single multireg and have count to accept list of params. Then if the members of count indicates the dimension.

{ name: "REG_A", count: ["ParamA", "ParamB"], swaccess, hwaccess, etc, fields: [ {field1} {field2} ] } }

Then we don't have to have multireg field but count indicates multiregister. Or we can have type field indicates multi.

If we need to have a loop for multiple registers such as :

{ multireg: { count: "ParamA", registers: [ { name: "REG_A", }, { name: "REG_B", } ] } }

Then, we need multireg field, but count still can be a list.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAH2RSTVG6AYE2BJOSBX4K3QNTJXXA5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAVHMPY#issuecomment-539653695, or mute the thread https://github.com/notifications/unsubscribe-auth/AAH2RSSXUZPZCFYPICZMYO3QNTJXXANCNFSM4I2SZW2A .

[msfschaffner]

Concerning the second case, @tjaychen, do you suggest to split these into two separate multireg instances? I thought you need non-homogeneous multiregs somewhere in flash_ctrl, or am I wrong?

[tjaychen]

yes that is true. But I thought non-homogenous support was going to be added via the Eunchan's compact function, and not necessarily through the nesting. Am i wrong?

[msfschaffner]

You would still have to specify the fields within the same multireg {} block such that the compacting can be done. Or maybe I did not entirely get what you meant by "splitting it up into two instances" :)...

[tjaychen]

sorry let me clarify. I was mainly asking if the following two are equivalent. If they are, I just meant that I preferred the second since then the hjson would have a uniform structure between non-multireg and multireg. Am I understanding correctly?

{ multireg: {
    count: "ParamA",
    registers: [
      { name: "REG_A",
      },
      { name: "REG_B",
      }
    ]
  }
}

and

{   name: "REG_A",
    count: ["ParamA"],
    swaccess, hwaccess, etc,
    fields: [ {field1} {field2} ]
  }
}

{   name: "REG_B",
    count: ["ParamA"],
    swaccess, hwaccess, etc,
    fields: [ {field1} {field2} ]
  }
}

[eunchan]

On Tue, Oct 08, 2019 at 02:29:52PM -0700, tjaychen wrote:

sorry let me clarify. I was mainly asking if the following two are equivalent. If they are, I just meant that I preferred the second since then the hjson would have a uniform structure between non-multireg and multireg. Am I understanding correctly?

{ multireg: {
   count: "ParamA",
   registers: [
     { name: "REG_A",
     },
     { name: "REG_B",
     }
   ]
 }
}

and

{   name: "REG_A",
   count: ["ParamA"],
   swaccess, hwaccess, etc,
   fields: [ {field1} {field2} ]
 }
}

{   name: "REG_B",
   count: ["ParamA"],
   swaccess, hwaccess, etc,
   fields: [ {field1} {field2} ]
 }
}

Intentions is to the first description has replicate two registers at a loop. So, the address could be REG_A_OFFSET + 'h4 => REG_B_OFFSET and incremented by 'h8 for each ParamA.

On the other hand, the second has REG_A_OFFSET + ParamA => REG_B_OFFSET and incremented by 'h4 for each register.

First one is quite hard to implement, but looks good in the document and nicely fit into the memory map.

REG_A[0]    -- 'h00
REG_B[0]    -- 'h04
----
REG_A[1]    -- 'h8
REG_B[1]    -- 'hC
----
..

If we allow skipto inside it, then we could make it 'h00 , 'h100 etc, which can replace current rv_plic case. I understand it is difficult to consider the case, and benefit is only to be nicely visible. Even we choose the first, the register list inside multireg isn't going to be implemented soon.

[tjaychen]

ah i see. I personally don't have a strong preference for how registers are allocated, ie, I don't care a ton whether REG_B is immediately arranged after REG_A, or whether there is a large block of REG_A, followed by a large block of REG_B. I have a feeling the latter could in some situations be easier for software to manipulate? But the differences would be very minor.

Maybe this is something we should solicit more feedback. My entire preference for separate (and thus no multireg key, but simply the count like Eunchan originally suggested) was for a uniform hjson.

[msfschaffner]

I like the first option better, as it gives you more control over the memory map, and hence you could specify the full PLIC regmap without having to use another level of hjson templating. However, the RTL backend currently assumes everything within a multireg to be uniform (i.e., REG_A and REG_B must have the same field structure and names), and hence this requires quite some refactoring of the multireg code...

On Tue, Oct 8, 2019 at 3:10 PM tjaychen notifications@github.com wrote:

ah i see. I personally don't have a strong preference for how registers are allocated, ie, I don't care a ton whether REG_B is immediately arranged after REG_A, or whether there is a large block of REG_A, followed by a large block of REG_B. I have a feeling the latter could in some situations be easier for software to manipulate? But the differences would be very minor.

Maybe this is something we should solicit more feedback. My entire preference for separate (and thus no multireg key, but simply the count like Eunchan originally suggested) was for a uniform hjson.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AJ3RJJKUVEO2KRRC2URTMK3QNUAOXA5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAVY3GY#issuecomment-539725211, or mute the thread https://github.com/notifications/unsubscribe-auth/AJ3RJJJOHYILX3PQVAHA2Y3QNUAOXANCNFSM4I2SZW2A .

[eunchan]

So.. looks like the discussion is settled here. Eventually first option (multiple registers in the count loop) is needed. But that case is somewhat special (not need it except rv_plic or rv_timer with multiple Harts). So, let me implement the single register option first and adding the multiple registers in a loop later. Please comment if any other good idea you have.

[tjaychen]

sgtm, thanks Eunchan.

On Mon, Oct 14, 2019 at 10:44 AM Eunchan Kim notifications@github.com wrote:

So.. looks like the discussion is settled here. Eventually first option (multiple registers in the count loop) is needed. But that case is somewhat special (not need it except rv_plic or rv_timer with multiple Harts). So, let me implement the single register option first and adding the multiple registers in a loop later. Please comment if any other good idea you have.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/270?email_source=notifications&email_token=AAH2RSRUXGITJTHQJQHYTK3QOSVWLA5CNFSM4I2SZW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBFYBMI#issuecomment-541819057, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH2RST3NKGCK4ZGEXY56NLQOSVWLANCNFSM4I2SZW2A .

[msfschaffner]

Sounds good to me as well.

[vogelpi]

Sorry for joining the party late. I think the discussed options make sense, but it is hard judge which one is the best. I am not sure which proposal you are planing to go for now @eunchan . Would you mind to quickly repeat/summarize the example or the staged versions you would like implement?

Two more points I wanted to comment on:

• The "compact" feature discussed above where multiple bit-fields can be mapped into a single register sounds very nice. However, I am not sure how useful it really is. Because once you change the number of fields or bits per element software interfacing those registers will have to change completely, even if all the elements can still be mapped onto the same register, but especially if this compacting into a single register no longer works. This is not just about handling address offsets but also about shifts, masks etc. which the register tool is not able to handle (I am not sure if it would not be complete overkill to also let the tool doing this, i.e., generate the C-macros to access the fields in such a register). But maybe it is sufficient to just only allow a single field in registers using this "compact" feature.

• The IP cores I worked with in the past and which offered similar or higher configurability as our timer or PLIC (these were mainly ARM IPs such as the system-level MMU) were mostly not interleaving registers belonging to the same context. More precisely, they did not have

  // Case 1
  REG_A[0]    -- 'h00
  REG_B[0]    -- 'h04
  ----
  REG_A[1]    -- 'h8
  REG_B[1]    -- 'hC
  ----
  ..

  but rather

  // Case 2
  REG_A[0]    -- 'h00
  REG_A[1]    -- 'h04
  ----
  REG_B[0]    -- 'h800
  REG_B[1]    -- 'h804
  ----
  ..

  Such that all REG_A where grouped together and all REG_B grouped separately with a sufficiently large gap in address space in between to accommodate a configurable number of REG_A and REG_B. Of course this gap can be handled by our register tool. But what I am wondering is whether firmware people are not anyway more used to "Case 2" and there is little benefit in going for "Case 1" as our tool is able to handle the main problem with the second option already (the potentially large gap). But I might be wrong here.

[eunchan]

Thanks for the comment. I was also aware of the case below you mentioned, as PLIC needs multiple registers to be looped together to be compatible with SiFive's PLIC driver. So the eventual goal will be to support loop in those multiple registers, including skipto inside multiple registers to set the start address of each loop.

I want to take a simple step first, the case of single register (register field with count) is most frequently used case in OpenTitan. And it is relatively easier to support than multiple registers case. So, this can be my first thing to implement. We can make it with the format of multiple registers by just allowing a register in a multireg field. I don't have strong opinion on one side. Both are fine and the former, register with count, personally looks a little bit better.

compact is there to support (or be compatible to) current multireg feature. By default, current reggen compacts the field, which is quite useful in some case. I didn't think about the C-header or the software access to the register in detail yet but roughly, if the software knows which register is compacted multi-register, then then don't have to RMW but can split into 32bit access by itself.

On Tue, Oct 15, 2019 at 03:04:05AM -0700, Pirmin Vogel wrote:

Sorry for joining the party late. I think the discussed options make sense, but it is hard judge which one is the best. I am not sure which proposal you are planing to go for now @eunchan . Would you mind to quickly repeat/summarize the example or the staged versions you would like implement?

Two more points I wanted to comment on:

• The "compact" feature discussed above where multiple bit-fields can be mapped into a single register sounds very nice. However, I am not sure how useful it really is. Because once you change the number of fields or bits per element software interfacing those registers will have to change completely, even if all the elements can still be mapped onto the same register, but especially if this compacting into a single register no longer works. This is not just about handling address offsets but also about shifts, masks etc. which the register tool is not able to handle (I am not sure if it would not be complete overkill to also let the tool doing this, i.e., generate the C-macros to access the fields in such a register). But maybe it is sufficient to just only allow a single field in registers using this "compact" feature.

• The IP cores I worked with in the past and which offered similar or higher configurability as our timer or PLIC (these were mainly ARM IPs such as the system-level MMU) were mostly not interleaving registers belonging to the same context. More precisely, they did not have

  // Case 1
  REG_A[0]    -- 'h00
  REG_B[0]    -- 'h04
  ----
  REG_A[1]    -- 'h8
  REG_B[1]    -- 'hC
  ----
  ..

  but rather

  // Case 2
  REG_A[0]    -- 'h00
  REG_A[1]    -- 'h04
  ----
  REG_B[0]    -- 'h800
  REG_B[1]    -- 'h804
  ----
  ..

  Such that all REG_A where grouped together and all REG_B grouped separately with a sufficiently large gap in address space in between to accommodate a configurable number of REG_A and REG_B. Of course this gap can be handled by our register tool. But what I am wondering is whether firmware people are not anyway more used to "Case 2" and there is little benefit in going for "Case 1" as our tool is able to handle the main problem with the second option already (the potentially large gap). But I might be wrong here.

[rasmus-madsen]

I was just made aware of this discussion by @vogelpi I think if I understand discussion correctly this also means going from multireg access like ```

REG_A0 REG_A1

to `
REG_A[0]
REG_A[1]`

this will be a huge improvement for DV side as that would allow us to loop through the multiregs.

[msfschaffner]

Indeed, that is one of the goals. Part of these upcoming changes are already partially there, but only on the RTL side, and only for 1D arrays. Here for exaple: https://github.com/lowRISC/opentitan/blob/38cd899fdc1250ca3adb77af9b80ef8277372fb5/hw/ip/hmac/rtl/hmac.sv#L102-L109

[lenary]

I think this was closed by #1149?