| Message ID | 20190419171927.24269-4-pure.logic@nexus-software.ie (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add i.MX8MM support | expand |
On 4/19/2019 8:19 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 ensures the RELAX field is at least one ipg_clk cycle, which > seems like a pretty obvious floor to place on a value such as this. > > Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie> > --- > drivers/nvmem/imx-ocotp.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c > index 85a7d0da3abb..5b625d61e433 100644 > --- a/drivers/nvmem/imx-ocotp.c > +++ b/drivers/nvmem/imx-ocotp.c > @@ -186,6 +186,8 @@ 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; > + if (!relax) > + relax = 1; Math here hurts my head. Why is there a -1 above? How about "relax = DIV_ROUND_UP(clk_rate, 1000000000 / DEF_RELAX);"? Rounding up seems safe and it should guarantee that the result is >= 1. As far as I understand a value which is slightly larger here shouldn't hurt. Looking at uboot there DEF_RELAX is moved to the other side but that risks overflow at very high ipg rates. Other chips have similar 66Mhz ipg rates; my guess is that this bug affects other chips as well. -- Regards, Leonard
On 22/04/2019 11:28, Leonard Crestez wrote: > On 4/19/2019 8:19 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 ensures the RELAX field is at least one ipg_clk cycle, which >> seems like a pretty obvious floor to place on a value such as this. >> >> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie> >> --- >> drivers/nvmem/imx-ocotp.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c >> index 85a7d0da3abb..5b625d61e433 100644 >> --- a/drivers/nvmem/imx-ocotp.c >> +++ b/drivers/nvmem/imx-ocotp.c >> @@ -186,6 +186,8 @@ 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; >> + if (!relax) >> + relax = 1; > > Math here hurts my head. Why is there a -1 above? I thought of that, in theory it probably works, in practice I'm not so sure how it will affect the imx6, where the above code 'just works' right now. > How about "relax = DIV_ROUND_UP(clk_rate, 1000000000 / DEF_RELAX);"? > Rounding up seems safe and it should guarantee that the result is >= 1. > As far as I understand a value which is slightly larger here shouldn't hurt. > > Looking at uboot there DEF_RELAX is moved to the other side but that > risks overflow at very high ipg rates. > > Other chips have similar 66Mhz ipg rates; my guess is that this bug > affects other chips as well. Could be. I thought of converting this calculation to the u-boot version but... I don't want to break existing code for other users. I have one i.MX6 board. It seems safer to me to catch the corner-case for imx8mm. But, I'm open to further convincing
On 22/04/2019 11:28, Leonard Crestez wrote: > How about "relax = DIV_ROUND_UP(clk_rate, 1000000000 / DEF_RELAX);"? > Rounding up seems safe and it should guarantee that the result is >= 1. > As far as I understand a value which is slightly larger here shouldn't hurt. Actually re-reading your comment I think I'd be on for that. Dropping the -1 for the RELAX field should be fine for all users - because DEF_RELAX != RELAX. RELAX specifies a number of cycles to do nothing. DEF_RELAX is a field in a calculation, they are not in fact the same thing. So it is completely valid to vary RELAX w/r to the DEF_RELAX value we have right now and I'm probably being too conservative with the fix-up as-is. --- bod
On 4/22/2019 4:37 PM, Bryan O'Donoghue wrote: > On 22/04/2019 11:28, Leonard Crestez wrote: >> How about "relax = DIV_ROUND_UP(clk_rate, 1000000000 / DEF_RELAX);"? >> Rounding up seems safe and it should guarantee that the result is >= 1. >> As far as I understand a value which is slightly larger here shouldn't hurt. > > Actually re-reading your comment I think I'd be on for that. > > Dropping the -1 for the RELAX field should be fine for all users - > because DEF_RELAX != RELAX. > > RELAX specifies a number of cycles to do nothing. > DEF_RELAX is a field in a calculation, they are not in fact the same thing. > > So it is completely valid to vary RELAX w/r to the DEF_RELAX value we > have right now and I'm probably being too conservative with the fix-up > as-is. It seems that all imx6 have an ipg clk of 66Mhz and writes currently look like this: clk_rate=66000000 relax=0 strobe_read=43 strobe_prog=701 timing=002b02bd As far as I can tell the issue you reported for 8m should affect all imx6. -- Regards, Leonard
On 22/04/2019 15:35, Leonard Crestez wrote: > clk_rate=66000000 relax=0 strobe_read=43 strobe_prog=701 timing=002b02bd > > As far as I can tell the issue you reported for 8m should affect all imx6. OK, I'll add a Fixes in that case
diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c index 85a7d0da3abb..5b625d61e433 100644 --- a/drivers/nvmem/imx-ocotp.c +++ b/drivers/nvmem/imx-ocotp.c @@ -186,6 +186,8 @@ 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; + if (!relax) + relax = 1; 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 ensures the RELAX field is at least one ipg_clk cycle, which seems like a pretty obvious floor to place on a value such as this. Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie> --- drivers/nvmem/imx-ocotp.c | 2 ++ 1 file changed, 2 insertions(+)