| Message ID | 20190422150500.11082-4-pure.logic@nexus-software.ie (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add i.MX8MM support | expand |
On 4/22/2019 6:05 PM, Bryan O'Donoghue wrote: > The RELAX field of the OCOTP block quote "specifies the time to add to all > default timing parameters other than the Tpgm and Trd. It is given in > number of ipg_clk periods". > > On the i.MX8MM the calculation for the RELAX value is turning out to be > zero which is not a problem for programming OTP values but, does > subsequently mess up reloading the OTP shadow registers. > > This patch drops the -1 component of the RELAX field calculation. Since > RELAX specifies a number of cycles to do nothing, adding one extra cycle is > safe. The DEF_RELAX components of the timing calculation are unaffected. > > Fixes: 0642bac7da42 ("nvmem: imx-ocotp: add write support") > > diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c > @@ -185,7 +185,7 @@ static void imx_ocotp_set_imx6_timing(struct ocotp_priv *priv) > > - relax = clk_rate / (1000000000 / DEF_RELAX) - 1; > + relax = clk_rate / (1000000000 / DEF_RELAX); On a closer look the lines below also look odd: > strobe_prog = clk_rate / (1000000000 / 10000) + 2 * (DEF_RELAX + 1) - 1; > strobe_read = clk_rate / (1000000000 / 40) + 2 * (DEF_RELAX + 1) - 1; The DEF_RELAX constant is defined in ns but strobe_prog and strobe_read are in ipg cycles so this mixes units?! It might work if the result is a value "higher" than needed but it should be fixed. My suggestion would be to check the uboot code, maybe copy it and and rerun your tests: https://git.denx.de/?p=u-boot.git;a=blob;f=drivers/misc/mxc_ocotp.c;h=f84fe88db193583d8f17917b5b2dc2aec313e483;hb=HEAD#l280 Or just replace DEF_RELAX with relax. -- Regards, Leonard
On 23/04/2019 09:48, Leonard Crestez wrote: > On 4/22/2019 6:05 PM, Bryan O'Donoghue wrote: >> The RELAX field of the OCOTP block quote "specifies the time to add to all >> default timing parameters other than the Tpgm and Trd. It is given in >> number of ipg_clk periods". >> >> On the i.MX8MM the calculation for the RELAX value is turning out to be >> zero which is not a problem for programming OTP values but, does >> subsequently mess up reloading the OTP shadow registers. >> >> This patch drops the -1 component of the RELAX field calculation. Since >> RELAX specifies a number of cycles to do nothing, adding one extra cycle is >> safe. The DEF_RELAX components of the timing calculation are unaffected. >> >> Fixes: 0642bac7da42 ("nvmem: imx-ocotp: add write support") >> >> diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c >> @@ -185,7 +185,7 @@ static void imx_ocotp_set_imx6_timing(struct ocotp_priv *priv) >> >> - relax = clk_rate / (1000000000 / DEF_RELAX) - 1; >> + relax = clk_rate / (1000000000 / DEF_RELAX); > > On a closer look the lines below also look odd: > >> strobe_prog = clk_rate / (1000000000 / 10000) + 2 * (DEF_RELAX + 1) - 1; >> strobe_read = clk_rate / (1000000000 / 40) + 2 * (DEF_RELAX + 1) - 1; > > The DEF_RELAX constant is defined in ns but strobe_prog and strobe_read > are in ipg cycles so this mixes units?! It might work if the result is a > value "higher" than needed but it should be fixed. > > My suggestion would be to check the uboot code, maybe copy it and and > rerun your tests: > > https://git.denx.de/?p=u-boot.git;a=blob;f=drivers/misc/mxc_ocotp.c;h=f84fe88db193583d8f17917b5b2dc2aec313e483;hb=HEAD#l280 > > Or just replace DEF_RELAX with relax. The original sin here is specification of the RELAX, STROBE_READ and STROBE_PROG fields in terms of absolute time values. The u-boot code and therefore the Linux code specifies timings in terms of absolute nanoseconds but, the documentation specifies the values in terms of ipg clocks. The error we have here, and also in u-boot is specifying 17 nanoseconds for relax or 37 nanoseconds of strobe_read and then trying to bludgeon those values via an ipg_clk division into the HW_OCOTP_TIMING bit-fields. All fine and dandy if your ipg_clk is always the same on each SoC but, arseways if your ipg_clk changes between SoC versions.. as it invariably will. So.. if we are going to change the ratios, we should get rid of these silly nanosecond declarations and instead express the values clocks, like the documentation does. As an example: #define RELAX_IPG_CLOCKS 2 #define STROBE_PROG_CLOCKS 800 #define STROBE_READ_CLOCKS 40 17 nanoseconds is literally the defintion of a magic number which _ought_ to have been defined in cycles from the beginnign ie. @ 66 Mhz one cycle is aprox 15 nanoseconds So OK, I'll have a go a rewriting this ... --- bod
On 4/23/2019 5:07 PM, Bryan O'Donoghue wrote: > On 23/04/2019 09:48, Leonard Crestez wrote: >> On 4/22/2019 6:05 PM, Bryan O'Donoghue wrote: >>> The RELAX field of the OCOTP block quote "specifies the time to add to all >>> default timing parameters other than the Tpgm and Trd. It is given in >>> number of ipg_clk periods". >>> >>> On the i.MX8MM the calculation for the RELAX value is turning out to be >>> zero which is not a problem for programming OTP values but, does >>> subsequently mess up reloading the OTP shadow registers. >>> >>> This patch drops the -1 component of the RELAX field calculation. Since >>> RELAX specifies a number of cycles to do nothing, adding one extra cycle is >>> safe. The DEF_RELAX components of the timing calculation are unaffected. >>> >>> Fixes: 0642bac7da42 ("nvmem: imx-ocotp: add write support") >>> >>> diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c >>> @@ -185,7 +185,7 @@ static void imx_ocotp_set_imx6_timing(struct ocotp_priv *priv) >>> >>> - relax = clk_rate / (1000000000 / DEF_RELAX) - 1; >>> + relax = clk_rate / (1000000000 / DEF_RELAX); >> >> On a closer look the lines below also look odd: >> >>> strobe_prog = clk_rate / (1000000000 / 10000) + 2 * (DEF_RELAX + 1) - 1; >>> strobe_read = clk_rate / (1000000000 / 40) + 2 * (DEF_RELAX + 1) - 1; >> >> The DEF_RELAX constant is defined in ns but strobe_prog and strobe_read >> are in ipg cycles so this mixes units?! It might work if the result is a >> value "higher" than needed but it should be fixed. >> >> My suggestion would be to check the uboot code, maybe copy it and and >> rerun your tests: >> Or just replace DEF_RELAX with relax. > > The original sin here is specification of the RELAX, STROBE_READ and > STROBE_PROG fields in terms of absolute time values. > > The u-boot code and therefore the Linux code specifies timings in terms > of absolute nanoseconds but, the documentation specifies the values in > terms of ipg clocks. > > The error we have here, and also in u-boot is specifying 17 nanoseconds > for relax or 37 nanoseconds of strobe_read and then trying to bludgeon > those values via an ipg_clk division into the HW_OCOTP_TIMING bit-fields. > > All fine and dandy if your ipg_clk is always the same on each SoC but, > arseways if your ipg_clk changes between SoC versions.. as it invariably > will. > > So.. if we are going to change the ratios, we should get rid of these > silly nanosecond declarations and instead express the values clocks, > like the documentation does. Having nanosecond constants in the code and converting them to ipg clocks is the right approach. It's just that linux driver mixed the units. > 17 nanoseconds is literally the defintion of a magic number which > _ought_ to have been defined in cycles from the beginning It makes sense for physical hardware requirements really to be measured in seconds. It's just the OCOTP block can only measure time based on the bus clock input so it wants the driver to convert "real time" to "ipg clock periods". Supporting arbitrary ipg clock rates seems reasonable for a driver in a general purpose OS. -- Regards, Leonard
On 23/04/2019 15:42, Leonard Crestez wrote: >> 17 nanoseconds is literally the defintion of a magic number which >> _ought_ to have been defined in cycles from the beginning > > It makes sense for physical hardware requirements really to be measured > in seconds. It's just the OCOTP block can only measure time based on the > bus clock input so it wants the driver to convert "real time" to "ipg > clock periods". Exactly. The block only understands clocks, not absolute time values. If you increase your ipg_clock to 133 Mhz the initial 17 nanosecond value breaks again or if you reduce your ipg_clk to 6.6 Mhz, again that value breaks. But if you specify the right _ratio_ between RELAX and STROBE/READ cycles - you can push your clock in any direction you like. Because the block 'ticks over' so to speak, on clock edges.
diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c index 85a7d0da3abb..2fa45d1d17eb 100644 --- a/drivers/nvmem/imx-ocotp.c +++ b/drivers/nvmem/imx-ocotp.c @@ -185,7 +185,7 @@ static void imx_ocotp_set_imx6_timing(struct ocotp_priv *priv) */ clk_rate = clk_get_rate(priv->clk); - relax = clk_rate / (1000000000 / DEF_RELAX) - 1; + relax = clk_rate / (1000000000 / DEF_RELAX); strobe_prog = clk_rate / (1000000000 / 10000) + 2 * (DEF_RELAX + 1) - 1; strobe_read = clk_rate / (1000000000 / 40) + 2 * (DEF_RELAX + 1) - 1;
The RELAX field of the OCOTP block quote "specifies the time to add to all default timing parameters other than the Tpgm and Trd. It is given in number of ipg_clk periods". On the i.MX8MM the calculation for the RELAX value is turning out to be zero which is not a problem for programming OTP values but, does subsequently mess up reloading the OTP shadow registers. This patch drops the -1 component of the RELAX field calculation. Since RELAX specifies a number of cycles to do nothing, adding one extra cycle is safe. The DEF_RELAX components of the timing calculation are unaffected. Fixes: 0642bac7da42 ("nvmem: imx-ocotp: add write support") Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie> Suggested-by: Leonard Crestez <leonard.crestez@nxp.com> --- drivers/nvmem/imx-ocotp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)