Messages in this thread | | | Date | Tue, 25 Jun 2019 14:02:33 +0100 | From | Piotr Sroka <> | Subject | Re: [v3 1/2] mtd: nand: Add Cadence NAND controller driver |
| |
Hi Dmitry
The 06/16/2019 16:42, Dmitry Osipenko wrote: >EXTERNAL MAIL > > >14.06.2019 18:09, Piotr Sroka пишет: > >Commit description is mandatory. > >> Signed-off-by: Piotr Sroka <piotrs@cadence.com> >> --- > >[snip] > >> + >> +/* Cadnence NAND flash controller capabilities get from driver data. */ >> +struct cadence_nand_dt_devdata { >> + /* Skew value of the output signals of the NAND Flash interface. */ >> + u32 if_skew; >> + /* It informs if aging feature in the DLL PHY supported. */ >> + u8 phy_dll_aging; >> + /* >> + * It informs if per bit deskew for read and write path in >> + * the PHY is supported. >> + */ >> + u8 phy_per_bit_deskew; >> + /* It informs if slave DMA interface is connected to DMA engine. */ >> + u8 has_dma; > >There is no needed to dedicate 8 bits to a variable if you only care about a single >bit. You may write this as: > >bool has_dma : 1; I modified it locally but it looks that checkpatch does not like such notation "WARNING: Avoid using bool as bitfield. Prefer bool bitfields as unsigned int or u<8|16|32>" So maybe I will leave it as is.
>[snip] > >> +static struct >> +cdns_nand_chip *to_cdns_nand_chip(struct nand_chip *chip) >> +{ >> + return container_of(chip, struct cdns_nand_chip, chip); >> +} >> + >> +static struct >> +cdns_nand_ctrl *to_cdns_nand_ctrl(struct nand_controller *controller) >> +{ >> + return container_of(controller, struct cdns_nand_ctrl, controller); >> +} > >It's better to inline explicitly such cases because they won't get inlined with some >kernel configurations, like enabled ftracing for example. > >> +static bool >> +cadence_nand_dma_buf_ok(struct cdns_nand_ctrl *cdns_ctrl, const void *buf, >> + u32 buf_len) >> +{ >> + u8 data_dma_width = cdns_ctrl->caps2.data_dma_width; >> + >> + return buf && virt_addr_valid(buf) && >> + likely(IS_ALIGNED((uintptr_t)buf, data_dma_width)) && >> + likely(IS_ALIGNED(buf_len, data_dma_width)); >> +} >> + >> +static int cadence_nand_wait_for_value(struct cdns_nand_ctrl *cdns_ctrl, >> + u32 reg_offset, u32 timeout_us, >> + u32 mask, bool is_clear) >> +{ >> + u32 val; >> + int ret = 0; >> + >> + ret = readl_poll_timeout(cdns_ctrl->reg + reg_offset, >> + val, !(val & mask) == is_clear, >> + 10, timeout_us); > >Apparently you don't care about having memory barrier here, hence >readl_relaxed_poll_timeout(). ok I will update it. > >> + if (ret < 0) { >> + dev_err(cdns_ctrl->dev, >> + "Timeout while waiting for reg %x with mask %x is clear %d\n", >> + reg_offset, mask, is_clear); >> + } >> + >> + return ret; >> +} >> + >> +static int cadence_nand_set_ecc_enable(struct cdns_nand_ctrl *cdns_ctrl, >> + bool enable) >> +{ >> + u32 reg; >> + >> + if (cadence_nand_wait_for_value(cdns_ctrl, CTRL_STATUS, >> + 1000000, >> + CTRL_STATUS_CTRL_BUSY, true)) >> + return -ETIMEDOUT; >> + >> + reg = readl(cdns_ctrl->reg + ECC_CONFIG_0); >> + >> + if (enable) >> + reg |= ECC_CONFIG_0_ECC_EN; >> + else >> + reg &= ~ECC_CONFIG_0_ECC_EN; >> + >> + writel(reg, cdns_ctrl->reg + ECC_CONFIG_0); > >And here.. looks like there is no need for the memory barries, hence use the relaxed >versions of readl/writel. Same for the rest of the patch. > ok >> + return 0; >> +} >> + >> +static void cadence_nand_set_ecc_strength(struct cdns_nand_ctrl *cdns_ctrl, >> + u8 corr_str_idx) >> +{ >> + u32 reg; >> + >> + if (cdns_ctrl->curr_corr_str_idx == corr_str_idx) >> + return; >> + >> + reg = readl(cdns_ctrl->reg + ECC_CONFIG_0); >> + reg &= ~ECC_CONFIG_0_CORR_STR; >> + reg |= FIELD_PREP(ECC_CONFIG_0_CORR_STR, corr_str_idx); >> + writel(reg, cdns_ctrl->reg + ECC_CONFIG_0); >> + >> + cdns_ctrl->curr_corr_str_idx = corr_str_idx; >> +} >> + >> +static u8 cadence_nand_get_ecc_strength_idx(struct cdns_nand_ctrl *cdns_ctrl, >> + u8 strength) >> +{ >> + u8 i, corr_str_idx = 0; >> + >> + for (i = 0; i < BCH_MAX_NUM_CORR_CAPS; i++) { >> + if (cdns_ctrl->ecc_strengths[i] == strength) { >> + corr_str_idx = i; >> + break; >> + } >> + } > >Is it a error case when i == BCH_MAX_NUM_CORR_CAPS? Yes it is an error but it could appear only if ECC capability registers have wrong values. I will handle that error anyway.
>> + return corr_str_idx; >> +} >> + >> +static int cadence_nand_set_skip_marker_val(struct cdns_nand_ctrl *cdns_ctrl, >> + u16 marker_value) >> +{ >> + u32 reg = 0; >> + >> + if (cadence_nand_wait_for_value(cdns_ctrl, CTRL_STATUS, >> + 1000000, >> + CTRL_STATUS_CTRL_BUSY, true)) >> + return -ETIMEDOUT; >> + >> + reg = readl(cdns_ctrl->reg + SKIP_BYTES_CONF); >> + reg &= ~SKIP_BYTES_MARKER_VALUE; >> + reg |= FIELD_PREP(SKIP_BYTES_MARKER_VALUE, >> + marker_value); >> + >> + writel(reg, cdns_ctrl->reg + SKIP_BYTES_CONF); >> + >> + return 0; >> +} >> + >> +static int cadence_nand_set_skip_bytes_conf(struct cdns_nand_ctrl *cdns_ctrl, >> + u8 num_of_bytes, >> + u32 offset_value, >> + int enable) >> +{ >> + u32 reg = 0; >> + u32 skip_bytes_offset = 0; > >Please don't initialize variables if not necessary. You could also write this in a >single line. > > u32 skip_bytes_offset, reg; > >Same for the rest of the patch. > Ok to I will change it. >> + if (cadence_nand_wait_for_value(cdns_ctrl, CTRL_STATUS, >> + 1000000, >> + CTRL_STATUS_CTRL_BUSY, true)) >> + return -ETIMEDOUT; >> + >> + if (!enable) { >> + num_of_bytes = 0; >> + offset_value = 0; >> + } >> + >> + reg = readl(cdns_ctrl->reg + SKIP_BYTES_CONF); >> + reg &= ~SKIP_BYTES_NUM_OF_BYTES; >> + reg |= FIELD_PREP(SKIP_BYTES_NUM_OF_BYTES, >> + num_of_bytes); >> + skip_bytes_offset = FIELD_PREP(SKIP_BYTES_OFFSET_VALUE, >> + offset_value); >> + >> + writel(reg, cdns_ctrl->reg + SKIP_BYTES_CONF); >> + writel(skip_bytes_offset, cdns_ctrl->reg + SKIP_BYTES_OFFSET); >> + >> + return 0; >> +} >> + >> +/* Fucntions enables/disables hardware detection of erased data */ > >s/Fucntions/Function/, please use spellchecker. I'd also recommend to start all >single-line comments with a lower case (and without a dot in the end) because it is a >more common style in the kernel and is a bit easier for the eyes. Ok I will do it. > >[snip] Thanks Piotr Sroka
| |