| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d06ebb55-590a-4e30-a7ab-62d00e1dc19e@bootlin.com>
Date: Tue, 12 Nov 2024 13:53:51 +0100
From: Bastien Curutchet <bastien.curutchet@...tlin.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>, bastien.curutchet@...tlin.com
Cc: Santosh Shilimkar <ssantosh@...nel.org>,
Krzysztof Kozlowski <krzk@...nel.org>, Richard Weinberger <richard@....at>,
Vignesh Raghavendra <vigneshr@...com>, linux-kernel@...r.kernel.org,
linux-mtd@...ts.infradead.org,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
Herve Codina <herve.codina@...tlin.com>,
Christopher Cordahi <christophercordahi@...ometrics.ca>
Subject: Re: [PATCH v2 6/6] mtd: rawnand: davinci: Implement setup_interface()
operation
Hi Miquèl,
On 11/11/24 8:32 PM, Miquel Raynal wrote:
> Hi Bastien,
>
> On 06/11/2024 at 09:55:07 +01, Bastien Curutchet <bastien.curutchet@...tlin.com> wrote:
>
>> The setup_interface() operation isn't implemented. It forces the driver
>> to use the ONFI mode 0, though it could use more optimal modes.
>>
>> Implement the setup_interface() operation. It uses the
>> aemif_set_cs_timings() function from the AEMIF driver to update the
>> chip select timings. The calculation of the register's contents is
>> directly extracted from §20.3.2.3 of the DaVinci TRM [1]
>>
>> These timings are previously set by the AEMIF driver itself from
>> device-tree properties. Therefore, IMHO, failing to update them in the
>> setup_interface() isn't critical, which is why 0 is returned even when
>> timings aren't updated.
>
> Did you experience failures? Because I'd be more conservative and error
> out loudly when something is wrong. In general it is a safest approach
> on the long term. Here maybe you have good reasons to do differently, in
> this case I am all ears.
>
The DaVinci's configuration isn't very wide so if its reference clock
rate is too high, mode 0 timings can be too slow to fit while higher
modes will fit. So returning -EINVAL here will cause the probe to fail
with 'Failed to configure data interface to SDR timing mode 0' while
higher modes would have worked.
>> MAX_TH_PS and MAX_TSU_PS are the worst case timings based on the
>> Keystone2 and DaVinci datasheets.
>>
>> [1] : https://www.ti.com/lit/ug/spruh77c/spruh77c.pdf
>>
>> Signed-off-by: Bastien Curutchet <bastien.curutchet@...tlin.com>
>> ---
>> drivers/mtd/nand/raw/davinci_nand.c | 78 +++++++++++++++++++++++++++++
>> 1 file changed, 78 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/raw/davinci_nand.c b/drivers/mtd/nand/raw/davinci_nand.c
>> index 563045c7ce08..2d0c564c8d17 100644
>> --- a/drivers/mtd/nand/raw/davinci_nand.c
>> +++ b/drivers/mtd/nand/raw/davinci_nand.c
>> @@ -14,6 +14,7 @@
>> #include <linux/err.h>
>> #include <linux/iopoll.h>
>> #include <linux/kernel.h>
>> +#include <linux/memory/ti-aemif.h>
>> #include <linux/module.h>
>> #include <linux/mtd/rawnand.h>
>> #include <linux/mtd/partitions.h>
>> @@ -44,6 +45,9 @@
>> #define MASK_ALE 0x08
>> #define MASK_CLE 0x10
>>
>> +#define MAX_TSU_PS 3000 /* Input setup time in ps */
>> +#define MAX_TH_PS 1600 /* Input hold time in ps */
>> +
>> struct davinci_nand_pdata {
>> uint32_t mask_ale;
>> uint32_t mask_cle;
>> @@ -120,6 +124,7 @@ struct davinci_nand_info {
>> uint32_t core_chipsel;
>>
>> struct clk *clk;
>> + struct aemif_device *aemif;
>> };
>>
>> static DEFINE_SPINLOCK(davinci_nand_lock);
>> @@ -767,9 +772,81 @@ static int davinci_nand_exec_op(struct nand_chip *chip,
>> return 0;
>> }
>>
>> +#define TO_CYCLES(ps, period_ns) (DIV_ROUND_UP((ps) / 1000, (period_ns)))
>> +
>> +static int davinci_nand_setup_interface(struct nand_chip *chip, int chipnr,
>> + const struct nand_interface_config *conf)
>> +{
>> + struct davinci_nand_info *info = to_davinci_nand(nand_to_mtd(chip));
>> + const struct nand_sdr_timings *sdr;
>> + struct aemif_cs_timings timings;
>> + s32 cfg, min, cyc_ns;
>> +
>> + cyc_ns = 1000000000 / clk_get_rate(info->clk);
>> +
>> + sdr = nand_get_sdr_timings(conf);
>> + if (IS_ERR(sdr))
>> + return PTR_ERR(sdr);
>> +
>> + cfg = TO_CYCLES(sdr->tCLR_min, cyc_ns) - 1;
>> + timings.rsetup = cfg > 0 ? cfg : 0;
>> +
>> + cfg = max_t(s32, TO_CYCLES(sdr->tREA_max + MAX_TSU_PS, cyc_ns),
>> + TO_CYCLES(sdr->tRP_min, cyc_ns)) - 1;
>> + timings.rstrobe = cfg > 0 ? cfg : 0;
>> +
>> + min = TO_CYCLES(sdr->tCEA_max + MAX_TSU_PS, cyc_ns) - 2;
>> + while ((s32)(timings.rsetup + timings.rstrobe) < min)
>> + timings.rstrobe++;
>
> I see a lot of while loops which just stop if you reach a min/max, I
> believe a slightly more robust approach should be attempted, and
> returning errors when these limits are crossed would be probably
> beneficial on the long term?
>
This comes from the DaVinci NAND controller's documentation (cf p908 of
https://www.ti.com/lit/ug/spruh77c/spruh77c.pdf). A first formula, gives
the RSETUP timing, a second one the RSTROBE timing and then a third
formula gives a constraint that has to be met by the sum of RSETUP and
RSTROBE.
Then the validity of the timings themselves is checked by the
aemif_set_cs_timings() function.
It works the same way for WSETUP, WSTROBE and WHOLD with the other
while() loops.
>> +
>> + cfg = TO_CYCLES((s32)(MAX_TH_PS - sdr->tCHZ_max), cyc_ns) - 1;
>> + timings.rhold = cfg > 0 ? cfg : 0;
>> +
>> + min = TO_CYCLES(sdr->tRC_min, cyc_ns) - 3;
>> + while ((s32)(timings.rsetup + timings.rstrobe + timings.rhold) < min)
>> + timings.rhold++;
>> +
>> + cfg = TO_CYCLES((s32)(sdr->tRHZ_max - (timings.rhold + 1) * cyc_ns * 1000), cyc_ns);
>> + cfg = max_t(s32, cfg, TO_CYCLES(sdr->tCHZ_max, cyc_ns)) - 1;
>> + timings.ta = cfg > 0 ? cfg : 0;
>> +
>> + cfg = TO_CYCLES(sdr->tWP_min, cyc_ns) - 1;
>> + timings.wstrobe = cfg > 0 ? cfg : 0;
>> +
>> + cfg = max_t(s32, TO_CYCLES(sdr->tCLS_min, cyc_ns), TO_CYCLES(sdr->tALS_min, cyc_ns));
>> + cfg = max_t(s32, cfg, TO_CYCLES(sdr->tCS_min, cyc_ns)) - 1;
>> + timings.wsetup = cfg > 0 ? cfg : 0;
>> +
>> + min = TO_CYCLES(sdr->tDS_min, cyc_ns) - 2;
>> + while ((s32)(timings.wsetup + timings.wstrobe) < min)
>> + timings.wstrobe++;
>> +
>> + cfg = max_t(s32, TO_CYCLES(sdr->tCLH_min, cyc_ns), TO_CYCLES(sdr->tALH_min, cyc_ns));
>> + cfg = max_t(s32, cfg, TO_CYCLES(sdr->tCH_min, cyc_ns));
>> + cfg = max_t(s32, cfg, TO_CYCLES(sdr->tDH_min, cyc_ns)) - 1;
>> + timings.whold = cfg > 0 ? cfg : 0;
>> +
>> + min = TO_CYCLES(sdr->tWC_min, cyc_ns) - 2;
>> + while ((s32)(timings.wsetup + timings.wstrobe + timings.whold) < min)
>> + timings.whold++;
>> +
>> + dev_dbg(&info->pdev->dev, "RSETUP %x RSTROBE %x RHOLD %x\n",
>> + timings.rsetup, timings.rstrobe, timings.rhold);
>> + dev_dbg(&info->pdev->dev, "TA %x\n", timings.ta);
>> + dev_dbg(&info->pdev->dev, "WSETUP %x WSTROBE %x WHOLD %x\n",
>> + timings.wsetup, timings.wstrobe, timings.whold);
>> +
>
> Here you probably want to exit in the NAND_DATA_IFACE_CHECK_ONLY case.
>
I had missed this NAND_DATA_IFACE_CHECK_ONLY feature. In fact,
additionnal checks are done in the aemif_set_cs_timings() below. I'll
add an 'aemif_check_cs_timings()' function in AEMIF's driver in next
iteration.
>> + if (aemif_set_cs_timings(info->aemif, info->core_chipsel, &timings) < 0)
>> + dev_info(&info->pdev->dev,
>> + "Failed to dynamically update the CS timings, keep them unchanged");
>> +
>> + return 0;
>> +}
>> +
>
> Thanks,
> Miquèl
Powered by blists - more mailing lists