Hi Naga,
> > > +/**
> > > + * pl353_nand_read_data_op - read chip data into buffer
> > > + * @chip: Pointer to the NAND chip info structure
> > > + * @in: Pointer to the buffer to store read data
> > > + * @len: Number of bytes to read
> > > + * Return: Always return zero
> > > + */
> > > +static int pl353_nand_read_data_op(struct nand_chip *chip,
> > > + u8 *in,
> > > + unsigned int len)
> > > +{
> > > + int i;
> > > + struct pl353_nand_info *xnfc =
> > > + container_of(chip, struct pl353_nand_info, chip);
> > > +
> > > + if (IS_ALIGNED((uint32_t)in, sizeof(uint32_t)) &&
> > > + IS_ALIGNED(len, sizeof(uint32_t))) {
> > > + u32 *ptr = (u32 *)in;
> > > +
> > > + len /= 4;
> > > + for (i = 0; i < len; i++)
> > > + ptr[i] = readl(xnfc->nandaddr);
> > > + } else {
> > > + for (i = 0; i < len; i++)
> > > + in[i] = readb(xnfc->nandaddr);
> > > + }
> >
> > What about reading 0-3 bytes with readb if the driver is not aligned, then
> > doing aligned
> > access with readl until readb must be used again for the last 0-3 bytes?
> The else case is handling that right?
No it's not. The else case reads byte per byte, always.
[...]
> > > +static int pl353_nand_calculate_hwecc(struct mtd_info *mtd,
> > > + const u8 *data, u8 *ecc)
> > > +{
> > > + u32 ecc_value;
> > > + u8 ecc_reg, ecc_byte, ecc_status;
> > > + unsigned long timeout = jiffies + PL353_NAND_ECC_BUSY_TIMEOUT;
> > > +
> > > + /* Wait till the ECC operation is complete or timeout */
> > > + do {
> > > + if (pl353_smc_ecc_is_busy())
> > > + cpu_relax();
> > > + else
> > > + break;
> > > + } while (!time_after_eq(jiffies, timeout));
> > > +
> > > + if (time_after_eq(jiffies, timeout)) {
> > > + pr_err("%s timed out\n", __func__);
> > > + return -ETIMEDOUT;
> > > + }
> >
> > All of this should be done by the function calling nand_calculate_hwecc(),
> > not here.
> Ok, I will update it.
> >
> > Plus, it should deserve a function on its own.
> Ok, will add new one.
> >
> > > +
> > > + for (ecc_reg = 0; ecc_reg < 4; ecc_reg++) {
> > > + /* Read ECC value for each block */
> >
> > So you assume here that there are 4, and only 4 "blocks" (I prefer "ECC
> > chunks" or
> > "subpages"). Is it always the case? Or is it just how it works in your
> > situation? I would rather
> > prefer a to define this value anyway.
> Sure, I will define a macro as ECC chunks to represent 4.
> And there are always 4 ECC registers as per the controller. And there is no
> assumption here.
What about smaller or larger NAND chips? Maybe there are some
limitations in what chips are actually supported, then they should be
rejected.
[...]
> > > +static void pl353_prepare_cmd(struct mtd_info *mtd, struct
nand_chip *chip,
> > > + int page, int column, int start_cmd, int end_cmd,
> > > + bool read)
> > > +{
> > > + unsigned long data_phase_addr;
> > > + u32 end_cmd_valid = 0;
> > > + unsigned long cmd_phase_addr = 0, cmd_data = 0;
> > > +
> > > + struct pl353_nand_info *xnfc =
> > > + container_of(chip, struct pl353_nand_info, chip);
> > > +
> > > + end_cmd_valid = read ? 1 : 0;
> > > +
> > > + cmd_phase_addr = (unsigned long __force)xnfc->nand_base +
> >
> > do you really need this cast?
> As I said above, during command and data phases, we have to update the AXI
> Read/Write addresses with cycles, start command, end command
> And some extra info. Hence I am converting it to a value and then adding the
> above values and then again converting back as an
> Address.
>
> >
> > > + ((xnfc->addr_cycles
> > > + << ADDR_CYCLES_SHIFT) |
> > > + (end_cmd_valid << END_CMD_VALID_SHIFT) |
> > > + (COMMAND_PHASE) |
> > > + (end_cmd << END_CMD_SHIFT) |
> > > + (start_cmd << START_CMD_SHIFT));
> >
> > So the number of address cycles changes the address where you should write
> > the address
> > cycles? that's weird, no?
> I agree, but the implementation of PL353 is like that.
> As I pointed the spec above, for every transfer we have to frame AXI Write
> address and Read address.
>
> >
> > > +
> > > + /* Get the data phase address */
> > > + data_phase_addr = (unsigned long __force)xnfc->nand_base +
> > > + ((0x0 << CLEAR_CS_SHIFT) |
> > > + (0 << END_CMD_VALID_SHIFT) |
> > > + (DATA_PHASE) |
> > > + (end_cmd << END_CMD_SHIFT) |
> > > + (0x0 << ECC_LAST_SHIFT));
> > > +
> > Same question here?
> >
> > > + xnfc->nandaddr = (void __iomem * __force)data_phase_addr;
> >
> > This should be done only once in the probe
> No, as explained above, once we frame the Axi Write address/Read address with
> valid data, then we
> Are converting back as memory address with the casting.
> Do you see any issues with that?
> Let me know, is there an alternative to achieve the same.
> All is about, constructing AXI Write address and Read address during command
> and data phases.
Ok, I'll ask Boris to also review your next version, once -rc1 is out.
Thanks,
Miquèl
