bug in bsim4.va

[felix-salfelder]

There is a bug ... in bsim4.va, lines 12356 and 12357.

These statements effectively disconnect di,d and si,s if BSIM4rdsMod is not set. Otherwise they seem correct, and they should be made conditional on BSIM4rdsMod.

Greetings felix

[dwarning]

Thank you for pointing to this issue. I will check it against the C-code model and fix it if required.

[dwarning]

The model is a 1:1 compilation of the C-code v.4.8 from Cogenda and is surely not a masterpiece of Verilog-A modelling. After investigation I see no problem with disconnection of di from d (and si from s). The contribution for this branches is made in lines 12218 ff. And dependent from BSIM4rdsMod (default 0 if not given) the drain/source resistances are inserted or the nodes are collapsed. The mentioned contribution at line 12356 ff are the thermal contributions for noise analysis. They are conditional from BSIM4rdsMod in the code before in calculation gdpr and gspr. The modified code in branch adms-xyce seems to work with ADMS. Can you show the problem in a test case. Thanks, Dietmar

[felix-salfelder]

On Fri, Jul 21, 2023 at 10:26:25AM -0700, dwarning wrote:

Can you show the problem in a test case.

See LRM, section 5.6.1.3 "Value retention" rules. Example 2 illustrates the desired behaviour.

Specifically, the statements

V(a,b) <+ v; I(a,b) <+ i;

will turn the (a, b) branch into a current source. Please check your code generator for compliance with the standard, there might be a bug.

I expect ADMS to also generate the desired code from

if (BSIM4rdsMod) begin I(di,d) <+ white_noise ... ; I(si,s) <+ white_noise ... ; end.

Cf. BSIM6.1.1.va, around line 4486: a similar thing implemented correctly.

thanks felix

[dwarning]

Thank you for spotting this. It is fixed now for both branches. I think for body network is OK but for gate resistance the situation is same wrong. Voltage contribution changed into current contribution. Your opinion?

openvaf still not support noise modeling - so I assume he will not check on value retention. But vampyre.py also gave no complain!

[felix-salfelder]

On Sat, Jul 22, 2023 at 12:20:36AM -0700, dwarning wrote:

Thank you for spotting this. It is fixed now for both branches.

Great. It seems to work for me now.

Your opinion?

Very little. I think we should adhere to the standard where possible. Thanks for your cooperation.

Best wishes felix

[dwarning]

I mean this @ line 12235:

case (BSIM4rgateMod) 0: begin // No gate resistance V(g,gm) <+ 0.0; V(gm,gi) <+ 0.0; end 1: begin // Constant resistance V(g,gm) <+ I(g,gm) / BSIM4grgeltd; V(gm,gi) <+ 0.0; end 2: begin // Variable resistance V(g,gm) <+ 0.0; V(gm,gi) <+ I(gm,gi) / (BSIM4gcrg + BSIM4grgeltd); end 3: begin // 2 internal gate nodes, TODO check gcgmgmb needs? if (verbose == 1) begin $strobe("BSIM4grgeltd=%g\n",BSIM4grgeltd); $strobe("BSIM4gcrg=%g\n",BSIM4gcrg); end V(g,gm) <+ I(g,gm) / BSIM4grgeltd; V(gm,gi) <+ I(gm,gi) / BSIM4gcrg; end endcase // case(BSIM4rgateMod)

and this @ line 12365:

if ((BSIM4rgateMod == 1) || (BSIM4rgateMod == 2)) I(gi, g) <+ white_noise(4 * P_K * T * BSIM4grgeltd, "Rg"); else if (BSIM4rgateMod == 3) I(gm, g) <+ white_noise(4 * P_K * T * BSIM4grgeltd, "Rg");

Correct belong LRM?

[felix-salfelder]

On Sat, Jul 22, 2023 at 03:17:41AM -0700, dwarning wrote:

I mean this @ line 12235: ` case (BSIM4rgateMod) [..] 3: begin // 2 internal gate nodes, TODO check gcgmgmb needs? if (verbose == 1) begin $strobe("BSIM4grgeltd=%g\n",BSIM4grgeltd); $strobe("BSIM4gcrg=%g\n",BSIM4gcrg); end V(g,gm) <+ I(g,gm) / BSIM4grgeltd; V(gm,gi) <+ I(gm,gi) / BSIM4gcrg; end endcase // case(BSIM4rgateMod)

and this @ line 12365:

` if ((BSIM4rgateMod == 1) || (BSIM4rgateMod == 2)) I(gi, g) <+ white_noise(4 P_K T BSIM4grgeltd, "Rg"); else if (BSIM4rgateMod == 3) I(gm, g) <+ white_noise(4 P_K T BSIM4grgeltd, "Rg");

Oh, now I see what you mean. The BSIM4rgateMod == 3 case looks adventurous indeed.

Seemingly, the intent is

I(gm,g) <+ V(gm,g) * BSIM4grgeltd + white_noise(..);

However, according to the LRM, the effect is just

I(g,gm) <+ white_noise(..);

For reasons similar to the earlier ones.

I have not yet tried anything more involved, including BSIM4rgateMod==3. Thorough inspection and testing seems a little more work...

Bsim4.va appears to be a contributed non-expert port, and that would explain the inconsistencies. It's a useful test case still, and possibly worth debugging. I would hope for fewer bugs in the native Verilog-A models.

[dwarning]

Felix, coming back to rdsmode: I think now we are inline with LRM but the model is not correct anymore compared to the C model. Looking into b4set.c we see that only for rdsmode=0 and tnoimode=0 we collapsing inner and outer nodes. There are more cases for inner node generation which are omitted in the translated model.

b4noi.c shows an noise calculation for this branch in any case (this is like the initial Cogenda model comes from). It looks spice3 uses specific code for omitting matrix entry in case nodes are collapsed.

I made a modification of the va-code but tend more to remove the model from repository.

[felix-salfelder]

Hi Dietmar.

On Sun, Jul 23, 2023 at 02:21:01AM -0700, dwarning wrote:

I made a modification of the va-code [..]

• I(di, d) <+ white_noise(4 `P_K T * gdpr, "Rd");
• I(si, s) <+ white_noise(4 `P_K T * gspr, "Rs");

Perhaps you misunderstand the "white_noise" call. The white_noise function returns zero in dc/tr mode see "4.6.4 Noise" in the LRM. The issue at hand has nothing to do with how the spice model implements noise, but how contribution statements are implemented.

The dc/tr case. Now the statements above are equivalent to

• I(di, d) <+ 0.;
• I(si, s) <+ 0.;

Read as (x2):

1) forget the voltage set previously (in this case: 0.) 2) turh branch into current source 3) set current value to zero.

A zero amp current branch is the opposite of a short circuit. If you do this unconditionally, there will be no connection between di,d or si,s.

[..] but tend more to remove the model from repository.

Up to you. I'd rather try and grow the collection, use it to uncover more bugs. I'm not attached to bsim4 by any means.

To debug your model generator, single out

electrical di; // internal node analog begin V(di, d) <+ 0. I(di, d) <+ 0. end

and see what happens. The LRM requires that di will be floating.

Best wishes felix

[dwarning]

According recommendation of Geoffrey Coram (Thanks!) the problem is solved by using separate branches for potential and flow contributions for RD/RS branches.

Citiation G.C: _The two branches br_v_d_di and br_i_ddi are separate branches, so we don't have the value retention rules coming into play. When RDSMOD!=0, the current contributions are active, and the "v" branches have no contribution, so they are "treated as a flow source with a value of 0" per 5.6.1.3. When RDSMOD=0, the "v" branch shorts the two nodes, and adding the noise current sources is just like connecting a current source across the terminals of a voltage source.

For mentioned Rgate noise the problem is simple solved by swapping to voltage contribution.

The translated VA model still not reflect same behaviour as the original C code model.

[felix-salfelder]

On Mon, Jul 24, 2023 at 08:25:12AM -0700, dwarning wrote:

According recommendation of Geoffrey Coram (Thanks!) the problem is solved by using separate branches for potential and flow contributions for RD/RS branches.

Thanks for the follow-up!

(Thanks for throwing in ideas, Geoffrey.)

Citiation G.C: _The two branches br_v_d_di and br_i_d_di are separate branches,

This is clearly confusing. br_v_d_di is not a branch, nor is br_i_d_di. So they can't be separate branches. Is this a misquotation or did he actually write that?

See 3.12 Branches: "A branch is a path between two nets". A branch can be (d,di) or (di,d), (but clearly not br_v_d_di!).

Now, what you might want to say is that (d,di) and (di,d) are two separate branches. This would make some of the logic in the attempted solution work (and also relate to bsim4.va). But, no cigar: see 3.12 Branches: "There shall be at most one unnamed branch between any two nets". Using both (d,di) and (di,d) is an error, and should have been caught by the model generator.

First of all: Please reopen the issue.

Towards solving this issue: I believe Geoffreys approach will work with named branches. If you do

branch (d,di) drainnoise;

and then do

V(d,di) <+ 0; I(drainnoise) <+ white_noise(..); // (*)

We should end up with (d,di) shortened, and not disconnected by the noise statement (*), If I read the standard correctly. Yet, I believe the conditional noise current contribution is more straightforward, and less confusing (See BSIM6 model).

The discrepancies between the C model and the va model are curious, I can see some, too. Need to investigate.

Best wishes felix

[dwarning]

You saw around line 482:

branch (d,di) br_v_d_di, br_i_d_di;
branch (s,si) br_v_s_si, br_i_s_si;

[felix-salfelder]

On Mon, Jul 24, 2023 at 09:51:20AM -0700, dwarning wrote:

You saw around line 482:

branch (d,di) br_v_d_di, br_i_d_di;
branch (s,si) br_v_s_si, br_i_s_si;

!!

I missed the last few commits. Thanks, all of this makes complete sense.

Little remark, and less urgent: there are still potential bugs left, related to unnamed branches. For example:

line 12215: I(si,bi) <+ BSIM4type * Igisl;

line 12295: I(bi,si) <+ BSIM4type ddt(qbulk); ()

There can be only one branch acc to 3.12. I suspect for full compliance, we need to change the latter to

I(si,di) <+ - BSIM4type * ddt(qbulk); (**)

Unless for some other reason, (si,di) and (di,si) are somehow "the same unnamed branch": Turning (*) into (**) looks safe to me, perhaps the model generator should warn in that case.

Sorry for the noise.

[dwarning]

I(si,di) <+ - BSIM4type * ddt(qbulk); (**) ???

You meant: I(si,bi) <+ - BSIM4type * ddt(qbulk);

[gjcoram]

But, no cigar: see 3.12 Branches: "There shall be at most one unnamed branch between any two nets". Using both (d,di) and (di,d) is an error, and should have been caught by the model generator.

I believe it is legal to have both (d,di) and (di,d) in the module, but they both refer to the same unnamed branch. So, if you contribute a voltage to (d,di), then any flows contributed to (di,d) would be discarded along with flows contributed to (d,di).

Your suggestion: branch (d,di) drainnoise; makes it clear what that branch is for, so it is better than two-branch solution that was attributed to me (I wasn't necessarily suggesting it, just pointing out that it was a possibility).

I have struggled with whether it is better to keep all the contributions to (d,di) together, or keep all the noise contributions together. But I have seen some compilers struggle when the switch branch is split (one if/else for the deterministic contribution and a second "if" for the noise (with no else since the noise is 0 when the resistor is eliminated)). And the model author can cause trouble by updating one condition and neglecting to update the other!

[felix-salfelder]

On Mon, Jul 24, 2023 at 11:43:51AM -0700, dwarning wrote:

I(si,di) <+ - BSIM4type * ddt(qbulk); (**) ???

You meant: I(si,bi) <+ - BSIM4type * ddt(qbulk);

Yes, apologies. Just the ports swapped, and the sign flip.

[felix-salfelder]

On Mon, Jul 24, 2023 at 02:45:46PM -0700, gjcoram wrote:

I believe it is legal to have both (d,di) and (di,d) in the module, but they both refer to the same unnamed branch. So, if you contribute a voltage to (d,di), then any flows contributed to (di,d) would be discarded along with flows contributed to (d,di).

Thanks Geoffrey for spelling it out. I favour this interpretation, as the logic is more natural. I wish the standard was more explicit about it...

[felix-salfelder]

The current bsim4.va seems close enough to the c implementation for now. I notified upstream, and there's not much else to do about this one.

Thanks for resolving this.