[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/9] hw/nvram: Introduce Xilinx eFuse QOM
From: |
Peter Maydell |
Subject: |
Re: [PATCH v2 1/9] hw/nvram: Introduce Xilinx eFuse QOM |
Date: |
Tue, 7 Sep 2021 15:44:02 +0100 |
On Mon, 23 Aug 2021 at 18:49, Tong Ho <tong.ho@xilinx.com> wrote:
>
> This introduces the QOM for Xilinx eFuse, an one-time
> field-programmable storage bit array.
>
> The actual mmio interface to the array varies by device
> families and will be provided in different change-sets.
>
> Co-authored-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Co-authored-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> Signed-off-by: Tong Ho <tong.ho@xilinx.com>
--- /dev/null
+++ b/hw/nvram/xlnx-efuse-crc.c
@@ -0,0 +1,118 @@
+/*
+ * Xilinx eFuse/bbram CRC calculator
+ */
+#include "hw/nvram/xlnx-efuse.h"
.c files must always include "qemu/osdep.h" as their first #include.
> +#ifndef XLNX_EFUSE_ERR_DEBUG
> +#define XLNX_EFUSE_ERR_DEBUG 0
> +#endif
This define doesn't seem to be used; you could just drop it.
> +#define XLNX_EFUSE(obj) \
> + OBJECT_CHECK(XLNXEFuse, (obj), TYPE_XLNX_EFUSE)
This is a bit of an old-style way to write this. These days we
recommend using the OBJECT_DECLARE_TYPE macro in your .h file
(which will provide the cast macro/function and also some typedefs
that you're currently manually providing).
> +static void efuse_sync_bdrv(XLNXEFuse *s, unsigned int bit)
> +{
> + const int bswap_adj = (const_le32(0x1234) != 0x1234 ? 3 : 0);
Don't do ad-hoc figuring out of the host endianness like this.
I would suggest using cpu_to_le32() on the relevant word
in fuse32[] and then writing that to the backing store.
> + unsigned int efuse_byte;
> +
> + if (!s->blk || s->blk_ro) {
> + return; /* Silient on read-only backend to avoid message flood */
"silent"
> + }
> +
> + efuse_byte = bit / 8;
> +
> + if (blk_pwrite(s->blk, efuse_byte,
> + ((uint8_t *) s->fuse32) + (efuse_byte ^ bswap_adj),
> + 1, 0) < 0) {
> + error_report("%s: write error in byte %" PRIu32 ".",
> + __func__, efuse_byte);
> + }
> +}
> +static void efuse_realize(DeviceState *dev, Error **errp)
> +{
> + XLNXEFuse *s = XLNX_EFUSE(dev);
> + BlockBackend *blk;
> + DriveInfo *dinfo;
> + unsigned int nr_bytes;
> + const char *prefix = object_get_canonical_path(OBJECT(dev));
> +
> + if (s->drv_index < 0) {
> + /* Set legacy compatibility */
> + s->drv_index = s->efuse_size <= 2048 ? 3 : 1;
> + }
> +
> + dinfo = drive_get_by_index(IF_PFLASH, s->drv_index);
> + blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
Don't get block backends like this in device models, please.
Instead, the device should have a property "drive" (use
DEFINE_PROP_DRIVE() to declare this), and the board should find
the drive and attach it to the device. In fact looking lower down
in the file I see you have a 'drive' property, so you should
just be using it (and getting rid of the drive-index property).
> +
> + nr_bytes = ROUND_UP((s->efuse_nr * s->efuse_size) / 8, 4);
> + s->fuse32 = g_malloc0(nr_bytes);
> + if (blk) {
> + qdev_prop_set_drive(dev, "drive", blk);
> +
> + s->blk_ro = !blk_supports_write_perm(s->blk);
> + if (s->blk_ro) {
> + warn_report("%s: update not saved: backstore is read-only",
> + object_get_canonical_path(OBJECT(s)));
> + }
> + blk_set_perm(s->blk,
> + (BLK_PERM_CONSISTENT_READ
> + | (s->blk_ro ? 0 : BLK_PERM_WRITE)), BLK_PERM_ALL,
> + &error_abort);
&error_abort isn't really appropriate in a device model, unless
you know the function call really can't fail. Better to pass
the error back up to the caller. (Watch out that you need to free
the s->fuse32 you just allocated if you return early.)
> +
> + if (blk_pread(s->blk, 0, (void *) s->fuse32, nr_bytes) < 0) {
> + error_setg(&error_abort, "%s: Unable to read-out contents."
> + "backing file too small? Expecting %" PRIu32"
> bytes",
> + prefix,
> + (unsigned int) (nr_bytes));
You should pass this to the caller, not use error_abort.
> + }
> + if (const_le32(0x1234) != 0x1234) {
Again, no ad-hoc endianness testing, please.
> + /* Convert from little-endian backstore for each 32-bit row */
> + unsigned int nr_u32;
> +
> + for (nr_u32 = 0; nr_u32 < (nr_bytes / 4); nr_u32++) {
> + s->fuse32[nr_u32] = le32_to_cpu(s->fuse32[nr_u32]);
> + }
> + }
> + }
> +
> + /* Sort readonly-list for bsearch lookup */
> + efuse_ro_bits_sort(s);
> +}
> +static const VMStateDescription vmstate_efuse = {
> + .name = TYPE_XLNX_EFUSE,
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .minimum_version_id_old = 1,
You don't need to specify the minimum_version_id_old here.
> + .fields = (VMStateField[]) {
> + VMSTATE_END_OF_LIST(),
If the device genuinely has no internal state (and it looks
like in this case that is true), you don't need to write out
an empty vmstate. Instead put a comment in the class init function
like:
/*
* This device has no internal state (it is all kept in the
* block device) so it does not need a vmstate.
*/
> +#ifndef XLNX_EFUSE_H
> +#define XLNX_EFUSE_H
> +
> +#include "qemu/osdep.h"
Headers should never include osdep.h.
> +#include "sysemu/block-backend.h"
> +#include "hw/qdev-core.h"
> +
> +#define TYPE_XLNX_EFUSE "xlnx,efuse"
> +
> +typedef struct XLNXEFuseLkSpec {
> + uint16_t row;
> + uint16_t lk_bit;
> +} XLNXEFuseLkSpec;
What's this struct for? Nothing in this patch or in these files uses it.
> +typedef struct XLNXEFuse {
> + DeviceState parent_obj;
> + BlockBackend *blk;
> + bool blk_ro;
> + uint32_t *fuse32;
> +
> + DeviceState *dev;
> +
> + bool init_tbits;
> + int drv_index;
> +
> + uint8_t efuse_nr;
> + uint32_t efuse_size;
> +
> + uint32_t *ro_bits;
> + uint32_t ro_bits_cnt;
> +} XLNXEFuse;
> +
> +uint32_t xlnx_efuse_calc_crc(const uint32_t *data, unsigned u32_cnt,
> + unsigned zpads);
Where you're providing function prototypes in a header to be used
by other parts of QEMU, can you provide some brief doc-comment format
comments that describe what the API of those functions is, please ?
> +
> +bool xlnx_efuse_get_bit(XLNXEFuse *s, unsigned int bit);
> +bool xlnx_efuse_set_bit(XLNXEFuse *s, unsigned int bit);
> +bool xlnx_efuse_k256_check(XLNXEFuse *s, uint32_t crc, unsigned start);
> +uint32_t xlnx_efuse_tbits_check(XLNXEFuse *s);
> +
> +/* Return whole row containing the given bit address */
> +static inline uint32_t xlnx_efuse_get_row(XLNXEFuse *s, unsigned int bit)
> +{
> + if (!(s->fuse32)) {
> + return 0;
> + } else {
> + unsigned int row_idx = bit / 32;
> +
> + assert(row_idx < (s->efuse_size * s->efuse_nr / 32));
> + return s->fuse32[row_idx];
> + }
> +}
> +
> +#endif
thanks
-- PMM
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH v2 1/9] hw/nvram: Introduce Xilinx eFuse QOM,
Peter Maydell <=