google / skywater-pdk

Open source process design kit for usage with SkyWater Technology Foundry's 130nm node.
https://skywater-pdk.rtfd.io
Apache License 2.0
2.95k stars 383 forks source link

Incorrect use of "temp" instead of "temper" in sky130_fd_pr__res_iso_pw.model.spice #171

Closed RTimothyEdwards closed 3 years ago

RTimothyEdwards commented 3 years ago

The file "sky130_fd_pr__res_iso_pw.model.spice" has a resistance equation that depends on temperature. In ngspice (and also spectre), temperature that appears in a parameter equation should use the name "temper", not "temp". This is a SkyWater error.

Interestingly, ngspice didn't flag this as an error. Possibly ngspice considers "temp" and "temper" as equivalent?

This could be considered "no action necessary", depending on what tools might flag this as an error.

RTimothyEdwards commented 3 years ago

Apparently the answer is that I put ".param TEMP=" into the testbenches to solve the issue of "temp" not being recognized as a parameter. But this parameter should not be needed, and in fact is probably misleading because ".option TEMP" would be the correct way to set the simulation temperature. The correct solution is to replace "temp" with "temper" in all the device model equations. This includes the poly resistor as well as the pwell resistor.

dwarning commented 3 years ago

Yes - that is correct. You can access circuit temperature with "temper" in equations but you can't set by e.g. .param temper=value. .param temp=temper should work I believe, but it is better to use temper direct. BTW this syntax is inline with hspice too.

RTimothyEdwards commented 3 years ago

@mithro : Squeaky wheel, squeaky wheel.

mithro commented 3 years ago

@RTimothyEdwards - I'm still unclear what needs to be done here. I don't know which files cover the "device model equations". Can you give me a list of which files need to be updated and a diff for a few of the files showing how they are changed?

Even better would be a patch file which changes all the files as needed.

dwarning commented 3 years ago

Sorry?

Am 06.11.2020 um 21:40 schrieb R. Timothy Edwards:

@mithro https://github.com/mithro : Squeaky wheel, squeaky wheel.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/google/skywater-pdk/issues/171#issuecomment-723289897, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEMWNTCEQ2LLFS7J7BCMAQLSORNMJANCNFSM4SGLWF6Q.

diadatp commented 3 years ago

This patch fixes temperature modeling in high_po, xhigh_po, iso_pw and minor issues in all other resistor models. Also addresses #169. I have signed the Google CLA.

How: temp -> temper in various equations. tref -> tnom in simple resistor models. Explicit non param tc1 and tc2 specified for behavioral resistors. tnom not supported.

RTimothyEdwards commented 3 years ago

@mithro : This is what I like best about open source; I am about to come here to say that I don't have the time to investigate and come up with a patch file, and I find that @diadatp has already written one!

I looked over it; I can't vouch for every single equation, but it looks okay to me and follows the discussion we've been having on the #analog-design channel on Slack. SkyWater's incorrect use of "temp" and "tref" is now pretty well settled. Once this patch is applied, my ".param temp=" statements in the test SPICE netlists in sky130_fd_pr should be removed, as I put them in to get around this problem, not knowing that the real underlying issue was that the original files from SkyWater were using the wrong names for temperature variables.

RTimothyEdwards commented 3 years ago

@mithro : The point of the above statement is that you can go ahead and apply the patch from @diadatp and close this and #169 . . . although maybe leave this one open with the point of making sure that the test netlists have the ".param temp" removed from them.

RTimothyEdwards commented 3 years ago

@mithro : This patch supercedes the previous one and also corrects issue #228 (I would have put them in separate patch files, but I got it from Stefan Schippers and just uploaded it here):

sky130_fd_pr.zip

mithro commented 3 years ago

@RTimothyEdwards The sky130_fd_pr.patch file in your zip is a HTML file...

mithro commented 3 years ago

I think you wanted me to use https://raw.githubusercontent.com/StefanSchippers/xschem_sky130/main/sky130_fd_pr.patch ?

RTimothyEdwards commented 3 years ago

@mithro : Yes.

mithro commented 3 years ago

It looks like the cells/rf_pfet_01v8_mvt/sky130_fd_pr__rf_pfet_01v8_mvt.pm3.spice file might have been missed?

mithro commented 3 years ago
cells/rf_pfet_01v8_mvt/sky130_fd_pr__rf_pfet_01v8_mvt.pm3.spice:rg 2  g r = {(rg_sub_tnom*(1+(temp-tref)*tc1rcgp+(temp-tref)*(temp-tref)*tc2rcgp))+(rg_dist_tnom*(1+(temp-tref)*tc1rsgpu+(temp-tref)*(temp-tref)*tc2rsgpu))}
cells/rf_pfet_01v8_mvt/sky130_fd_pr__rf_pfet_01v8_mvt.pm3.spice:rg 2  g r = {(rg_stub_tnom*(1+(temp-tref)*tc1rcgp+(temp-tref)*(temp-tref)*tc2rcgp))+(rg_dist_tnom*(1+(temp-tref)*tc1rsgpu+(temp-tref)*(temp-tref)*tc2rsgpu))}
cells/rf_pfet_01v8_mvt/sky130_fd_pr__rf_pfet_01v8_mvt.pm3.spice:rg 2  g r = {(rg_stub_tnom*(1+(temp-tref)*tc1rcgp+(temp-tref)*(temp-tref)*tc2rcgp))+(rg_dist_tnom*(1+(temp-tref)*tc1rsgpu+(temp-tref)*(temp-tref)*tc2rsgpu))}
mithro commented 3 years ago

Once this patch is applied, my ".param temp=" statements in the test SPICE netlists in sky130_fd_pr should be removed

I don't see anything like .param temp= statements in the current tests? I do see .param TEMP=27 and similar?