[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 08/12] hw/nvram: NPCM7xx OTP device model
From: |
Havard Skinnemoen |
Subject: |
Re: [PATCH v4 08/12] hw/nvram: NPCM7xx OTP device model |
Date: |
Wed, 8 Jul 2020 10:04:22 -0700 |
On Wed, Jul 8, 2020 at 1:54 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 7/7/20 8:47 PM, Havard Skinnemoen wrote:
> > + value = tswap32(nc->disabled_modules);
> > + npcm7xx_otp_array_write(&s->fuse_array, &value, 64, sizeof(value));
>
> What is magic offset 64 for?
Good point. I'll add some definitions based on
https://github.com/Nuvoton-Israel/bootblock/blob/master/Src/fuse_wrapper/fuse_wrapper.h#L23
> > + /* Preserve read-only and write-one-to-clear bits */
> > + value =
> > + (value & ~FST_RO_MASK) | (s->regs[NPCM7XX_OTP_FST] &
> > FST_RO_MASK);
>
> Trivial to review as:
>
> value &= ~FST_RO_MASK;
> value |= s->regs[NPCM7XX_OTP_FST] & FST_RO_MASK;
You're right, will do.
> > +/* Each OTP module holds 8192 bits of one-time programmable storage */
> > +#define NPCM7XX_OTP_ARRAY_BITS (8192)
> > +#define NPCM7XX_OTP_ARRAY_BYTES (NPCM7XX_OTP_ARRAY_BITS / 8)
>
> You could replace 8 by BITS_PER_BYTE.
Will do.
> > +typedef struct NPCM7xxOTPClass {
> > + SysBusDeviceClass parent;
> > +
> > + const MemoryRegionOps *mmio_ops;
> > +} NPCM7xxOTPClass;
> > +
> > +#define NPCM7XX_OTP_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(NPCM7xxOTPClass, (klass), TYPE_NPCM7XX_OTP)
> > +#define NPCM7XX_OTP_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(NPCM7xxOTPClass, (obj), TYPE_NPCM7XX_OTP)
>
> If nothing outside of the model implementation requires accessing
> the class fields (which is certainly the case here, no code out of
> npcm7xx_otp.c should access mmio_ops directly), then I recommend
> to keep the class definitions local to the single source file where
> it is used. This also makes this header simpler to look at.
Good idea. Will do.
> Very high code quality, so I just made nitpicking comments.
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Thank you. I'll incorporate your feedback and send out a v5 series shortly.
Havard
- Re: [PATCH v4 05/12] hw/arm: Add NPCM730 and NPCM750 SoC models, (continued)
- Re: [PATCH v4 05/12] hw/arm: Add NPCM730 and NPCM750 SoC models, Philippe Mathieu-Daudé, 2020/07/09
- Re: [PATCH v4 05/12] hw/arm: Add NPCM730 and NPCM750 SoC models, Havard Skinnemoen, 2020/07/08
- Re: [PATCH v4 05/12] hw/arm: Add NPCM730 and NPCM750 SoC models, Havard Skinnemoen, 2020/07/08
- Re: [PATCH v4 05/12] hw/arm: Add NPCM730 and NPCM750 SoC models, Havard Skinnemoen, 2020/07/08
- Re: [PATCH v4 05/12] hw/arm: Add NPCM730 and NPCM750 SoC models, Philippe Mathieu-Daudé, 2020/07/09
- Re: [PATCH v4 05/12] hw/arm: Add NPCM730 and NPCM750 SoC models, Havard Skinnemoen, 2020/07/09
[PATCH v4 06/12] hw/arm: Add two NPCM7xx-based machines, Havard Skinnemoen, 2020/07/07
[PATCH v4 07/12] hw/arm: Load -bios image as a boot ROM for npcm7xx, Havard Skinnemoen, 2020/07/07
[PATCH v4 08/12] hw/nvram: NPCM7xx OTP device model, Havard Skinnemoen, 2020/07/07
[PATCH v4 09/12] hw/mem: Stubbed out NPCM7xx Memory Controller model, Havard Skinnemoen, 2020/07/07
[PATCH v4 10/12] hw/ssi: NPCM7xx Flash Interface Unit device model, Havard Skinnemoen, 2020/07/07
[PATCH v4 11/12] hw/arm: Wire up BMC boot flash for npcm750-evb and quanta-gsj, Havard Skinnemoen, 2020/07/07
[PATCH v4 12/12] docs/system: Add Nuvoton machine documentation, Havard Skinnemoen, 2020/07/07