Re: [v3 1/2] mtd: nand: Add Cadence NAND controller driver
On Tue, Jun 25, 2019 at 4:45 PM Dmitry Osipenko wrote: > 25.06.2019 16:02, Piotr Sroka пишет: > > The 06/16/2019 16:42, Dmitry Osipenko wrote: > >> 14.06.2019 18:09, Piotr Sroka пишет: > >>> +/* 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. > > You may also use the "u8 : 1" form then, to satisfy the checkpatch. Probably > "unsigned int : 1" will be the best in this case, it's up to you. Exactly. The compiler will allocate the sufficient amount of space to store the bitfield. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [v3 1/2] mtd: nand: Add Cadence NAND controller driver
25.06.2019 16:02, Piotr Sroka пишет: > 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 >>> --- >> >> [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. You may also use the "u8 : 1" form then, to satisfy the checkpatch. Probably "unsigned int : 1" will be the best in this case, it's up to you.
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 --- [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, + 100, + 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, + 100, + CTRL_STATUS_CTRL_BUSY, true)) + return -ETIMEDOUT; + +
Re: [v3 1/2] mtd: nand: Add Cadence NAND controller driver
14.06.2019 18:09, Piotr Sroka пишет: Commit description is mandatory. > Signed-off-by: Piotr Sroka > --- [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; [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(). > + 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, > + 100, > + 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. > + 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? > + 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, > + 100, > + 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