Re: [PATCH 1/2] hw: m25p80: Add Block Protect and Top Bottom bits for write protect

[Francisco Iglesias <frasse.iglesias@gmail.com>]

On [2022 Jul 01] Fri 14:23:17, Cédric Le Goater wrote:
> On 7/1/22 13:40, Francisco Iglesias wrote:
> > Hi Iris,
> > 
> > Looks good, a couple of minor comments below!
> > 
> > On [2022 Jun 27] Mon 11:52:33, Iris Chen wrote:
> > > Signed-off-by: Iris Chen <irischenlj@fb.com>
> > > ---
> > >   hw/block/m25p80.c | 74 +++++++++++++++++++++++++++++++++++++++--------
> > >   1 file changed, 62 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > index 50b523e5b1..0156a70f5e 100644
> > > --- a/hw/block/m25p80.c
> > > +++ b/hw/block/m25p80.c
> > > @@ -38,21 +38,19 @@
> > >   #include "trace.h"
> > >   #include "qom/object.h"
> > > -/* Fields for FlashPartInfo->flags */
> > > -
> > > -/* erase capabilities */
> > > -#define ER_4K 1
> > > -#define ER_32K 2
> > > -/* set to allow the page program command to write 0s back to 1. Useful for
> > > - * modelling EEPROM with SPI flash command set
> > > - */
> > > -#define EEPROM 0x100
> > > -
> > >   /* 16 MiB max in 3 byte address mode */
> > >   #define MAX_3BYTES_SIZE 0x1000000
> > > -
> > >   #define SPI_NOR_MAX_ID_LEN 6
> > > +/* Fields for FlashPartInfo->flags */
> > > +enum spi_nor_option_flags {
> > 
> > (A suggestion is to s/nor/flash/ above (and s/SNOR_F_//  below) since there
> > looks to be nand flashes as W25N01GV using the protocol to).
> > 
> > > +    ER_4K                  = BIT(0),
> > > +    ER_32K                 = BIT(1),
> > > +    EEPROM                 = BIT(2),
> > > +    SNOR_F_HAS_SR_TB       = BIT(3),
> > > +    SNOR_F_HAS_SR_BP3_BIT6 = BIT(4),
> > > +};
> > > +
> > >   typedef struct FlashPartInfo {
> > >       const char *part_name;
> > >       /*
> > > @@ -253,7 +251,8 @@ static const FlashPartInfo known_devices[] = {
> > >       { INFO("n25q512a11",  0x20bb20,      0,  64 << 10, 1024, ER_4K) },
> > >       { INFO("n25q512a13",  0x20ba20,      0,  64 << 10, 1024, ER_4K) },
> > >       { INFO("n25q128",     0x20ba18,      0,  64 << 10, 256, 0) },
> > > -    { INFO("n25q256a",    0x20ba19,      0,  64 << 10, 512, ER_4K) },
> > > +    { INFO("n25q256a",    0x20ba19,      0,  64 << 10, 512,
> > > +           ER_4K | SNOR_F_HAS_SR_BP3_BIT6 | SNOR_F_HAS_SR_TB) },
> > >       { INFO("n25q512a",    0x20ba20,      0,  64 << 10, 1024, ER_4K) },
> > >       { INFO("n25q512ax3",  0x20ba20,  0x1000,  64 << 10, 1024, ER_4K) },
> > >       { INFO("mt25ql512ab", 0x20ba20, 0x1044, 64 << 10, 1024, ER_4K | ER_32K) },
> > > @@ -480,6 +479,11 @@ struct Flash {
> > >       bool reset_enable;
> > >       bool quad_enable;
> > >       bool aai_enable;
> > > +    bool block_protect0;
> > > +    bool block_protect1;
> > > +    bool block_protect2;
> > > +    bool block_protect3;
> > > +    bool top_bottom_bit;
> > >       bool status_register_write_disabled;
> > >       uint8_t ear;
> > > @@ -630,6 +634,29 @@ void flash_write8(Flash *s, uint32_t addr, uint8_t data)
> > >           qemu_log_mask(LOG_GUEST_ERROR, "M25P80: write with write protect!\n");
> > >           return;
> > >       }
> > > +    uint32_t block_protect_value = (s->block_protect3 << 3) |
> > > +                                   (s->block_protect2 << 2) |
> > > +                                   (s->block_protect1 << 1) |
> > > +                                   (s->block_protect0 << 0);
> > > +
> > > +     uint32_t num_protected_sectors = 1 << (block_protect_value - 1);
> > > +     uint32_t sector = addr / s->pi->sector_size;
> > > +
> > > +     /* top_bottom_bit == 0 means TOP */
> > 
> > Indentation needs minor fixing on above lines, also the declarations should
> > be at the top of the function.
> 

Hi Cédric,

> I agree in that case it would be better to have at the top
> but checkpatch does not complain. What's the rule ?
> 

Below is taken from docs/devel/style.rst:

"
Declarations
============

Mixed declarations (interleaving statements and declarations within
blocks) are generally not allowed; declarations should be at the beginning
of blocks.

Every now and then, an exception is made for declarations inside a
#ifdef or #ifndef block: if the code looks nicer, such declarations can
be placed at the top of the block even if there are statements above.
On the other hand, however, it's often best to move that #ifdef/#ifndef
block to a separate function altogether
"

I understand above as this is generally not allowed but in case you would
prefer to keep the declarations as they are placed one can see this as 'code
looks nicer' (to me the loop case is often cleaner in the way you are
mentioning). I should though have been more clear that 'moving to the top' was
a suggestion for not breaking this 'general rule' (also in case I've
interpreted this wrongly), sorry for that!

Best regards,
Francisco Iglesias

> For loop indexes, I do prefer to declare in the block
> statement.
> 
> > 
> > > +    if (!s->top_bottom_bit) {
> > > +        if (block_protect_value > 0 &&
> > > +            s->pi->n_sectors <= sector + num_protected_sectors) {
> > > +            qemu_log_mask(LOG_GUEST_ERROR,
> > > +                          "M25P80: write with write protect!\n");
> > > +            return;
> > > +        }
> > > +    } else {
> > > +        if (block_protect_value > 0 && sector < num_protected_sectors) {
> > > +            qemu_log_mask(LOG_GUEST_ERROR,
> > > +                          "M25P80: write with write protect!\n");
> > > +            return;
> > > +        }
> > > +    }
> > >       if ((prev ^ data) & data) {
> > >           trace_m25p80_programming_zero_to_one(s, addr, prev, data);
> > > @@ -728,6 +755,15 @@ static void complete_collecting_data(Flash *s)
> > >           break;
> > >       case WRSR:
> > >           s->status_register_write_disabled = extract32(s->data[0], 7, 1);
> > > +        s->block_protect0 = extract32(s->data[0], 2, 1);
> > > +        s->block_protect1 = extract32(s->data[0], 3, 1);
> > > +        s->block_protect2 = extract32(s->data[0], 4, 1);
> > > +        if (s->pi->flags & SNOR_F_HAS_SR_TB) {
> > > +            s->top_bottom_bit = extract32(s->data[0], 5, 1);
> > > +        }
> > > +        if (s->pi->flags & SNOR_F_HAS_SR_BP3_BIT6) {
> > > +            s->block_protect3 = extract32(s->data[0], 6, 1);
> > > +        }
> > >           switch (get_man(s)) {
> > >           case MAN_SPANSION:
> > > @@ -1213,6 +1249,15 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> > >       case RDSR:
> > >           s->data[0] = (!!s->write_enable) << 1;
> > >           s->data[0] |= (!!s->status_register_write_disabled) << 7;
> > > +        s->data[0] |= (!!s->block_protect0) << 2;
> > > +        s->data[0] |= (!!s->block_protect1) << 3;
> > > +        s->data[0] |= (!!s->block_protect2) << 4;
> > > +        if (s->pi->flags & SNOR_F_HAS_SR_TB) {
> > > +            s->data[0] |= (!!s->top_bottom_bit) << 5;
> > > +        }
> > > +        if (s->pi->flags & SNOR_F_HAS_SR_BP3_BIT6) {
> > > +            s->data[0] |= (!!s->block_protect3) << 6;
> > > +        }
> > >           if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
> > >               s->data[0] |= (!!s->quad_enable) << 6;
> > > @@ -1553,6 +1598,11 @@ static void m25p80_reset(DeviceState *d)
> > >       s->wp_level = true;
> > >       s->status_register_write_disabled = false;
> > > +    s->block_protect0 = false;
> > > +    s->block_protect1 = false;
> > > +    s->block_protect2 = false;
> > > +    s->block_protect3 = false;
> > > +    s->top_bottom_bit = false;
> > 
> > We need to place above ones in a subsection in the vmstate (similar to the your
> > previous patch).
> 
> Ah yes. I keep forgetting these ...
> 
> Thanks for the second look Francisco.
> 
> C.
> 
> 
> > 
> > Looks good to me otherwise!
> > 
> > Thanks!
> > Best regards,
> > Francisco Iglesias
> > 
> > >       reset_memory(s);
> > >   }
> > > -- 
> > > 2.30.2
> > > 
> > > 
>