| Message ID | 20170829064840.3808-1-wak@google.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | flash: Support adding the no-erase property to flash | expand |
Hi, On Mon, 2017-08-28 at 23:48 -0700, William A. Kennington III wrote: > Currently, flash devices like mbox-flash ignore erase commands. This > means that issuing an erase from userspace, through the mtd device, > and > back returns a successful operation that does nothing. Unfortunately > this makes userspace tools unhappy. Linux MTD devices support the > MTD_NO_ERASE flag which conveys that writes do not require erases on > the > underlying flash devices. We should set this property on all of our > devices which do not require erases to be performed. > > NOTE: This still requires a linux kernel component to set the > MTD_NO_ERASE flag from the device tree property. Can I just check, does the MTD_NO_ERASE flag mean that the mtd device doesn't have an erase function, or that an erase isn't necessary before a write? Because the V2 mbox has an erase method which mbox-flash will call when an erase is performed. So if it means there isn't an erase function then this is incorrect. If it means that an erase isn't required before a write however then I support this as the mbox protocol explicitly states this to be the case. Thanks, Suraj > > Signed-off-by: William A. Kennington III <wak@google.com> > --- > core/flash.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/core/flash.c b/core/flash.c > index 53e6eba08..9e230174f 100644 > --- a/core/flash.c > +++ b/core/flash.c > @@ -31,6 +31,7 @@ > struct flash { > struct list_node list; > bool busy; > + bool no_erase; > struct blocklevel_device *bl; > uint64_t size; > uint32_t block_size; > @@ -199,6 +200,8 @@ static struct dt_node *flash_add_dt_node(struct > flash *flash, int id) > dt_add_property_u64(flash_node, "reg", flash->size); > dt_add_property_cells(flash_node, "ibm,flash-block-size", > flash->block_size); > + if (flash->no_erase) > + dt_add_property(flash_node, "no-erase", NULL, 0); > > /* we fix to 32-bits */ > dt_add_property_cells(flash_node, "#address-cells", 1); > @@ -281,6 +284,7 @@ int flash_register(struct blocklevel_device *bl) > > flash->busy = false; > flash->bl = bl; > + flash->no_erase = !(bl->flags & WRITE_NEED_ERASE); > flash->size = size; > flash->block_size = block_size; > flash->id = num_flashes();
https://github.com/torvalds/linux/blob/master/Documentation/ABI/testing/sysfs-class-mtd#L65 "No erase necessary" On Fri, Sep 1, 2017 at 12:16 AM Suraj Jitindar Singh < sjitindarsingh@gmail.com> wrote: > Hi, > > On Mon, 2017-08-28 at 23:48 -0700, William A. Kennington III wrote: > > Currently, flash devices like mbox-flash ignore erase commands. This > > means that issuing an erase from userspace, through the mtd device, > > and > > back returns a successful operation that does nothing. Unfortunately > > this makes userspace tools unhappy. Linux MTD devices support the > > MTD_NO_ERASE flag which conveys that writes do not require erases on > > the > > underlying flash devices. We should set this property on all of our > > devices which do not require erases to be performed. > > > > NOTE: This still requires a linux kernel component to set the > > MTD_NO_ERASE flag from the device tree property. > > Can I just check, does the MTD_NO_ERASE flag mean that the mtd device > doesn't have an erase function, or that an erase isn't necessary before > a write? > > Because the V2 mbox has an erase method which mbox-flash will call when > an erase is performed. So if it means there isn't an erase function > then this is incorrect. > > If it means that an erase isn't required before a write however then I > support this as the mbox protocol explicitly states this to be the > case. > > Thanks, > Suraj > > > > > Signed-off-by: William A. Kennington III <wak@google.com> > > --- > > core/flash.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/core/flash.c b/core/flash.c > > index 53e6eba08..9e230174f 100644 > > --- a/core/flash.c > > +++ b/core/flash.c > > @@ -31,6 +31,7 @@ > > struct flash { > > struct list_node list; > > bool busy; > > + bool no_erase; > > struct blocklevel_device *bl; > > uint64_t size; > > uint32_t block_size; > > @@ -199,6 +200,8 @@ static struct dt_node *flash_add_dt_node(struct > > flash *flash, int id) > > dt_add_property_u64(flash_node, "reg", flash->size); > > dt_add_property_cells(flash_node, "ibm,flash-block-size", > > flash->block_size); > > + if (flash->no_erase) > > + dt_add_property(flash_node, "no-erase", NULL, 0); > > > > /* we fix to 32-bits */ > > dt_add_property_cells(flash_node, "#address-cells", 1); > > @@ -281,6 +284,7 @@ int flash_register(struct blocklevel_device *bl) > > > > flash->busy = false; > > flash->bl = bl; > > + flash->no_erase = !(bl->flags & WRITE_NEED_ERASE); > > flash->size = size; > > flash->block_size = block_size; > > flash->id = num_flashes(); > <div dir="ltr"><a href="https://github.com/torvalds/linux/blob/master/Documentation/ABI/testing/sysfs-class-mtd#L65">https://github.com/torvalds/linux/blob/master/Documentation/ABI/testing/sysfs-class-mtd#L65</a><br><div>"No erase necessary"</div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Sep 1, 2017 at 12:16 AM Suraj Jitindar Singh <<a href="mailto:sjitindarsingh@gmail.com">sjitindarsingh@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br> <br> On Mon, 2017-08-28 at 23:48 -0700, William A. Kennington III wrote:<br> > Currently, flash devices like mbox-flash ignore erase commands. This<br> > means that issuing an erase from userspace, through the mtd device,<br> > and<br> > back returns a successful operation that does nothing. Unfortunately<br> > this makes userspace tools unhappy. Linux MTD devices support the<br> > MTD_NO_ERASE flag which conveys that writes do not require erases on<br> > the<br> > underlying flash devices. We should set this property on all of our<br> > devices which do not require erases to be performed.<br> ><br> > NOTE: This still requires a linux kernel component to set the<br> > MTD_NO_ERASE flag from the device tree property.<br> <br> Can I just check, does the MTD_NO_ERASE flag mean that the mtd device<br> doesn't have an erase function, or that an erase isn't necessary before<br> a write?<br> <br> Because the V2 mbox has an erase method which mbox-flash will call when<br> an erase is performed. So if it means there isn't an erase function<br> then this is incorrect.<br> <br> If it means that an erase isn't required before a write however then I<br> support this as the mbox protocol explicitly states this to be the<br> case.<br> <br> Thanks,<br> Suraj<br> <br> ><br> > Signed-off-by: William A. Kennington III <<a href="mailto:wak@google.com" target="_blank">wak@google.com</a>><br> > ---<br> > core/flash.c | 4 ++++<br> > 1 file changed, 4 insertions(+)<br> ><br> > diff --git a/core/flash.c b/core/flash.c<br> > index 53e6eba08..9e230174f 100644<br> > --- a/core/flash.c<br> > +++ b/core/flash.c<br> > @@ -31,6 +31,7 @@<br> > struct flash {<br> > struct list_node list;<br> > bool busy;<br> > + bool no_erase;<br> > struct blocklevel_device *bl;<br> > uint64_t size;<br> > uint32_t block_size;<br> > @@ -199,6 +200,8 @@ static struct dt_node *flash_add_dt_node(struct<br> > flash *flash, int id)<br> > dt_add_property_u64(flash_node, "reg", flash->size);<br> > dt_add_property_cells(flash_node, "ibm,flash-block-size",<br> > flash->block_size);<br> > + if (flash->no_erase)<br> > + dt_add_property(flash_node, "no-erase", NULL, 0);<br> > <br> > /* we fix to 32-bits */<br> > dt_add_property_cells(flash_node, "#address-cells", 1);<br> > @@ -281,6 +284,7 @@ int flash_register(struct blocklevel_device *bl)<br> > <br> > flash->busy = false;<br> > flash->bl = bl;<br> > + flash->no_erase = !(bl->flags & WRITE_NEED_ERASE);<br> > flash->size = size;<br> > flash->block_size = block_size;<br> > flash->id = num_flashes();<br> </blockquote></div>
On Fri, 2017-09-01 at 17:01 +0000, William Kennington wrote: > https://github.com/torvalds/linux/blob/master/Documentation/ABI/testi > ng/sysfs-class-mtd#L65 > "No erase necessary" Ok, thanks. In that case I agree with this patch but think the commit message needs slight rewording as below. :) > > On Fri, Sep 1, 2017 at 12:16 AM Suraj Jitindar Singh <sjitindarsingh@ > gmail.com> wrote: > > Hi, > > > > On Mon, 2017-08-28 at 23:48 -0700, William A. Kennington III wrote: > > > Currently, flash devices like mbox-flash ignore erase commands. mbox-flash now has erase capability (for V2 implementations any way) so I think this sentence is misleading. You might be better off referring to the fact that the mbox protocol explicitly states that an erase is not required before a write, which is exactly what this flag is designed to convey. > > This > > > means that issuing an erase from userspace, through the mtd > > device, > > > and > > > back returns a successful operation that does nothing. > > Unfortunately > > > this makes userspace tools unhappy. Linux MTD devices support the > > > MTD_NO_ERASE flag which conveys that writes do not require erases > > on > > > the > > > underlying flash devices. We should set this property on all of > > our > > > devices which do not require erases to be performed. > > > > > > NOTE: This still requires a linux kernel component to set the > > > MTD_NO_ERASE flag from the device tree property. > > > > Can I just check, does the MTD_NO_ERASE flag mean that the mtd > > device > > doesn't have an erase function, or that an erase isn't necessary > > before > > a write? > > > > Because the V2 mbox has an erase method which mbox-flash will call > > when > > an erase is performed. So if it means there isn't an erase function > > then this is incorrect. > > > > If it means that an erase isn't required before a write however > > then I > > support this as the mbox protocol explicitly states this to be the > > case. > > > > Thanks, > > Suraj > > > > > > > > Signed-off-by: William A. Kennington III <wak@google.com> With a slight commit message rewording, Reviewed-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> > > > --- > > > core/flash.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/core/flash.c b/core/flash.c > > > index 53e6eba08..9e230174f 100644 > > > --- a/core/flash.c > > > +++ b/core/flash.c > > > @@ -31,6 +31,7 @@ > > > struct flash { > > > struct list_node list; > > > bool busy; > > > + bool no_erase; > > > struct blocklevel_device *bl; > > > uint64_t size; > > > uint32_t block_size; > > > @@ -199,6 +200,8 @@ static struct dt_node > > *flash_add_dt_node(struct > > > flash *flash, int id) > > > dt_add_property_u64(flash_node, "reg", flash->size); > > > dt_add_property_cells(flash_node, "ibm,flash-block-size", > > > flash->block_size); > > > + if (flash->no_erase) > > > + dt_add_property(flash_node, "no-erase", NULL, 0); > > > > > > /* we fix to 32-bits */ > > > dt_add_property_cells(flash_node, "#address-cells", 1); > > > @@ -281,6 +284,7 @@ int flash_register(struct blocklevel_device > > *bl) > > > > > > flash->busy = false; > > > flash->bl = bl; > > > + flash->no_erase = !(bl->flags & WRITE_NEED_ERASE); > > > flash->size = size; > > > flash->block_size = block_size; > > > flash->id = num_flashes(); > >
"William A. Kennington III" <wak@google.com> writes: > Currently, flash devices like mbox-flash ignore erase commands. This > means that issuing an erase from userspace, through the mtd device, and > back returns a successful operation that does nothing. Unfortunately > this makes userspace tools unhappy. Linux MTD devices support the > MTD_NO_ERASE flag which conveys that writes do not require erases on the > underlying flash devices. We should set this property on all of our > devices which do not require erases to be performed. > > NOTE: This still requires a linux kernel component to set the > MTD_NO_ERASE flag from the device tree property. > > Signed-off-by: William A. Kennington III <wak@google.com> Thanks! Merged to master (with slight reword based on Suraj's suggestion) as of ba99af9b149d02438347b055e6e7d6bd15e33551
diff --git a/core/flash.c b/core/flash.c index 53e6eba08..9e230174f 100644 --- a/core/flash.c +++ b/core/flash.c @@ -31,6 +31,7 @@ struct flash { struct list_node list; bool busy; + bool no_erase; struct blocklevel_device *bl; uint64_t size; uint32_t block_size; @@ -199,6 +200,8 @@ static struct dt_node *flash_add_dt_node(struct flash *flash, int id) dt_add_property_u64(flash_node, "reg", flash->size); dt_add_property_cells(flash_node, "ibm,flash-block-size", flash->block_size); + if (flash->no_erase) + dt_add_property(flash_node, "no-erase", NULL, 0); /* we fix to 32-bits */ dt_add_property_cells(flash_node, "#address-cells", 1); @@ -281,6 +284,7 @@ int flash_register(struct blocklevel_device *bl) flash->busy = false; flash->bl = bl; + flash->no_erase = !(bl->flags & WRITE_NEED_ERASE); flash->size = size; flash->block_size = block_size; flash->id = num_flashes();
Currently, flash devices like mbox-flash ignore erase commands. This means that issuing an erase from userspace, through the mtd device, and back returns a successful operation that does nothing. Unfortunately this makes userspace tools unhappy. Linux MTD devices support the MTD_NO_ERASE flag which conveys that writes do not require erases on the underlying flash devices. We should set this property on all of our devices which do not require erases to be performed. NOTE: This still requires a linux kernel component to set the MTD_NO_ERASE flag from the device tree property. Signed-off-by: William A. Kennington III <wak@google.com> --- core/flash.c | 4 ++++ 1 file changed, 4 insertions(+)