| Message ID | 1488300926-3517-1-git-send-email-kdasu.kdev@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Boris Brezillon |
| Headers | show |
On 02/28/2017 05:55 PM, Kamal Dasu wrote: > Changing brcmnand_set_wp() prototype in prepration for refactoring the nand > write protect logic to add flash status byte check for the protection bit. > > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com> > --- > drivers/mtd/nand/brcmnand/brcmnand.c | 27 +++++++++++++++++---------- > 1 file changed, 17 insertions(+), 10 deletions(-) > > diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c > index 42ebd73..c7c4efe 100644 > --- a/drivers/mtd/nand/brcmnand/brcmnand.c > +++ b/drivers/mtd/nand/brcmnand/brcmnand.c > @@ -765,11 +765,14 @@ enum { > CS_SELECT_AUTO_DEVICE_ID_CFG = BIT(30), > }; > > -static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en) > +static int brcmnand_set_wp(struct brcmnand_host *host, int en) > { > + struct brcmnand_controller *ctrl = host->ctrl; > u32 val = en ? CS_SELECT_NAND_WP : 0; > > brcmnand_rmw_reg(ctrl, BRCMNAND_CS_SELECT, CS_SELECT_NAND_WP, 0, val); > + > + return 0; > } > > /*********************************************************************** > @@ -1029,7 +1032,7 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp) > dev_dbg(ctrl->dev, "WP %s\n", wp ? "on" : "off"); > old_wp = wp; > } > - brcmnand_set_wp(ctrl, wp); > + brcmnand_set_wp(host, wp); > } > } > > @@ -2462,14 +2465,6 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) > /* Disable XOR addressing */ > brcmnand_rmw_reg(ctrl, BRCMNAND_CS_XOR, 0xff, 0, 0); > > - if (ctrl->features & BRCMNAND_HAS_WP) { > - /* Permanently disable write protection */ > - if (wp_on == 2) > - brcmnand_set_wp(ctrl, false); > - } else { > - wp_on = 0; > - } > - > /* IRQ */ > ctrl->irq = platform_get_irq(pdev, 0); > if ((int)ctrl->irq < 0) { > @@ -2522,6 +2517,18 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) > } > > list_add_tail(&host->node, &ctrl->host_list); > + > + if (ctrl->features & BRCMNAND_HAS_WP) { > + /* Permanently disable write protection */ > + if (wp_on == 2) { > + ret = brcmnand_set_wp(host, false); > + if (ret) > + goto err; > + } > + } else { > + wp_on = 0; > + } > + Would it be worth it to move this into separate function ? Or maybe make brcmnand_set_wp() check for BRCMNAND_HAS_WP and only set the WP if the controller is actually capable of that ? I think that might make the code readability a bit better ... > } > } > >
Marek, On Tue, Feb 28, 2017 at 1:25 PM, Marek Vasut <marek.vasut@gmail.com> wrote: > On 02/28/2017 05:55 PM, Kamal Dasu wrote: >> Changing brcmnand_set_wp() prototype in prepration for refactoring the nand >> write protect logic to add flash status byte check for the protection bit. >> >> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com> >> --- >> drivers/mtd/nand/brcmnand/brcmnand.c | 27 +++++++++++++++++---------- >> 1 file changed, 17 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c >> index 42ebd73..c7c4efe 100644 >> --- a/drivers/mtd/nand/brcmnand/brcmnand.c >> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c >> @@ -765,11 +765,14 @@ enum { >> CS_SELECT_AUTO_DEVICE_ID_CFG = BIT(30), >> }; >> >> -static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en) >> +static int brcmnand_set_wp(struct brcmnand_host *host, int en) >> { >> + struct brcmnand_controller *ctrl = host->ctrl; >> u32 val = en ? CS_SELECT_NAND_WP : 0; >> >> brcmnand_rmw_reg(ctrl, BRCMNAND_CS_SELECT, CS_SELECT_NAND_WP, 0, val); >> + >> + return 0; >> } >> >> /*********************************************************************** >> @@ -1029,7 +1032,7 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp) >> dev_dbg(ctrl->dev, "WP %s\n", wp ? "on" : "off"); >> old_wp = wp; >> } >> - brcmnand_set_wp(ctrl, wp); >> + brcmnand_set_wp(host, wp); >> } >> } >> >> @@ -2462,14 +2465,6 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) >> /* Disable XOR addressing */ >> brcmnand_rmw_reg(ctrl, BRCMNAND_CS_XOR, 0xff, 0, 0); >> >> - if (ctrl->features & BRCMNAND_HAS_WP) { >> - /* Permanently disable write protection */ >> - if (wp_on == 2) >> - brcmnand_set_wp(ctrl, false); >> - } else { >> - wp_on = 0; >> - } >> - >> /* IRQ */ >> ctrl->irq = platform_get_irq(pdev, 0); >> if ((int)ctrl->irq < 0) { >> @@ -2522,6 +2517,18 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) >> } >> >> list_add_tail(&host->node, &ctrl->host_list); >> + >> + if (ctrl->features & BRCMNAND_HAS_WP) { >> + /* Permanently disable write protection */ >> + if (wp_on == 2) { >> + ret = brcmnand_set_wp(host, false); >> + if (ret) >> + goto err; >> + } >> + } else { >> + wp_on = 0; >> + } >> + > > Would it be worth it to move this into separate function ? Or maybe make > brcmnand_set_wp() check for BRCMNAND_HAS_WP and only set the WP > if the controller is actually capable of that ? I think that might make > the code readability a bit better ... > >> } >> } >> >> The above code is called during the probe and only when the bootline wants to permanently set the wp off( wp == 2) hence structured that way. I would prefer to leave it that way. In all other cases there is already brcmand_wp(mtd_info *mtd, int wp) checks and maintains the wp state. > > -- > Best regards, > Marek Vasut Thanks Kamal
On Tue, 28 Feb 2017 11:55:25 -0500 Kamal Dasu <kdasu.kdev@gmail.com> wrote: > Changing brcmnand_set_wp() prototype in prepration for refactoring the nand > write protect logic to add flash status byte check for the protection bit. > > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com> > --- > drivers/mtd/nand/brcmnand/brcmnand.c | 27 +++++++++++++++++---------- > 1 file changed, 17 insertions(+), 10 deletions(-) > > diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c > index 42ebd73..c7c4efe 100644 > --- a/drivers/mtd/nand/brcmnand/brcmnand.c > +++ b/drivers/mtd/nand/brcmnand/brcmnand.c > @@ -765,11 +765,14 @@ enum { > CS_SELECT_AUTO_DEVICE_ID_CFG = BIT(30), > }; > > -static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en) > +static int brcmnand_set_wp(struct brcmnand_host *host, int en) > { > + struct brcmnand_controller *ctrl = host->ctrl; > u32 val = en ? CS_SELECT_NAND_WP : 0; > > brcmnand_rmw_reg(ctrl, BRCMNAND_CS_SELECT, CS_SELECT_NAND_WP, 0, val); > + > + return 0; > } > > /*********************************************************************** > @@ -1029,7 +1032,7 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp) > dev_dbg(ctrl->dev, "WP %s\n", wp ? "on" : "off"); > old_wp = wp; > } > - brcmnand_set_wp(ctrl, wp); > + brcmnand_set_wp(host, wp); > } > } > > @@ -2462,14 +2465,6 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) > /* Disable XOR addressing */ > brcmnand_rmw_reg(ctrl, BRCMNAND_CS_XOR, 0xff, 0, 0); > > - if (ctrl->features & BRCMNAND_HAS_WP) { > - /* Permanently disable write protection */ > - if (wp_on == 2) > - brcmnand_set_wp(ctrl, false); > - } else { > - wp_on = 0; > - } > - if (!(ctrl->features & BRCMNAND_HAS_WP)) wp_on = 0; > /* IRQ */ > ctrl->irq = platform_get_irq(pdev, 0); > if ((int)ctrl->irq < 0) { > @@ -2522,6 +2517,18 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) > } > > list_add_tail(&host->node, &ctrl->host_list); > + > + if (ctrl->features & BRCMNAND_HAS_WP) { > + /* Permanently disable write protection */ > + if (wp_on == 2) { > + ret = brcmnand_set_wp(host, false); > + if (ret) > + goto err; > + } > + } else { > + wp_on = 0; > + } > + /* Permanently disable write protection */ if (wp_on == 2) { ret = brcmnand_set_wp(host, false); if (ret) goto err; } > } > } >
Boris, On Tue, Feb 28, 2017 at 5:05 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Tue, 28 Feb 2017 11:55:25 -0500 > Kamal Dasu <kdasu.kdev@gmail.com> wrote: > >> Changing brcmnand_set_wp() prototype in prepration for refactoring the nand >> write protect logic to add flash status byte check for the protection bit. >> >> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com> >> --- >> drivers/mtd/nand/brcmnand/brcmnand.c | 27 +++++++++++++++++---------- >> 1 file changed, 17 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c >> index 42ebd73..c7c4efe 100644 >> --- a/drivers/mtd/nand/brcmnand/brcmnand.c >> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c >> @@ -765,11 +765,14 @@ enum { >> CS_SELECT_AUTO_DEVICE_ID_CFG = BIT(30), >> }; >> >> -static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en) >> +static int brcmnand_set_wp(struct brcmnand_host *host, int en) >> { >> + struct brcmnand_controller *ctrl = host->ctrl; >> u32 val = en ? CS_SELECT_NAND_WP : 0; >> >> brcmnand_rmw_reg(ctrl, BRCMNAND_CS_SELECT, CS_SELECT_NAND_WP, 0, val); >> + >> + return 0; >> } >> >> /*********************************************************************** >> @@ -1029,7 +1032,7 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp) >> dev_dbg(ctrl->dev, "WP %s\n", wp ? "on" : "off"); >> old_wp = wp; >> } >> - brcmnand_set_wp(ctrl, wp); >> + brcmnand_set_wp(host, wp); >> } >> } >> >> @@ -2462,14 +2465,6 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) >> /* Disable XOR addressing */ >> brcmnand_rmw_reg(ctrl, BRCMNAND_CS_XOR, 0xff, 0, 0); >> >> - if (ctrl->features & BRCMNAND_HAS_WP) { >> - /* Permanently disable write protection */ >> - if (wp_on == 2) >> - brcmnand_set_wp(ctrl, false); >> - } else { >> - wp_on = 0; >> - } >> - > > if (!(ctrl->features & BRCMNAND_HAS_WP)) > wp_on = 0; > >> /* IRQ */ >> ctrl->irq = platform_get_irq(pdev, 0); >> if ((int)ctrl->irq < 0) { >> @@ -2522,6 +2517,18 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) >> } >> >> list_add_tail(&host->node, &ctrl->host_list); >> + >> + if (ctrl->features & BRCMNAND_HAS_WP) { >> + /* Permanently disable write protection */ >> + if (wp_on == 2) { >> + ret = brcmnand_set_wp(host, false); >> + if (ret) >> + goto err; >> + } >> + } else { >> + wp_on = 0; >> + } >> + > > /* Permanently disable write protection */ > if (wp_on == 2) { > ret = brcmnand_set_wp(host, false); > if (ret) > goto err; > } > > >> } >> } >> > Let me rework this and send a new patch. Actually I just need to set the controller wp bit and the wp value since I do not need to worry about the status since we are not going to try and change the #WP at all in this case when we send the erase/program command. brcmnand_set_wp_reg() calling this should be sufficient during probe.
diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c index 42ebd73..c7c4efe 100644 --- a/drivers/mtd/nand/brcmnand/brcmnand.c +++ b/drivers/mtd/nand/brcmnand/brcmnand.c @@ -765,11 +765,14 @@ enum { CS_SELECT_AUTO_DEVICE_ID_CFG = BIT(30), }; -static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en) +static int brcmnand_set_wp(struct brcmnand_host *host, int en) { + struct brcmnand_controller *ctrl = host->ctrl; u32 val = en ? CS_SELECT_NAND_WP : 0; brcmnand_rmw_reg(ctrl, BRCMNAND_CS_SELECT, CS_SELECT_NAND_WP, 0, val); + + return 0; } /*********************************************************************** @@ -1029,7 +1032,7 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp) dev_dbg(ctrl->dev, "WP %s\n", wp ? "on" : "off"); old_wp = wp; } - brcmnand_set_wp(ctrl, wp); + brcmnand_set_wp(host, wp); } } @@ -2462,14 +2465,6 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) /* Disable XOR addressing */ brcmnand_rmw_reg(ctrl, BRCMNAND_CS_XOR, 0xff, 0, 0); - if (ctrl->features & BRCMNAND_HAS_WP) { - /* Permanently disable write protection */ - if (wp_on == 2) - brcmnand_set_wp(ctrl, false); - } else { - wp_on = 0; - } - /* IRQ */ ctrl->irq = platform_get_irq(pdev, 0); if ((int)ctrl->irq < 0) { @@ -2522,6 +2517,18 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) } list_add_tail(&host->node, &ctrl->host_list); + + if (ctrl->features & BRCMNAND_HAS_WP) { + /* Permanently disable write protection */ + if (wp_on == 2) { + ret = brcmnand_set_wp(host, false); + if (ret) + goto err; + } + } else { + wp_on = 0; + } + } }
Changing brcmnand_set_wp() prototype in prepration for refactoring the nand write protect logic to add flash status byte check for the protection bit. Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com> --- drivers/mtd/nand/brcmnand/brcmnand.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)