Messages in this thread | | | Date | Mon, 18 Mar 2024 11:28:48 -0400 | Subject | Re: [PATCH 2/4] RAS: ATL: Expand helpers for adding and removing base and hole | From | Yazen Ghannam <> |
| |
On 3/14/24 12:35, John Allen wrote: > Data fabric 4.5 denormalization will need to frequently add and remove
More specifically, the non-power-of-2 cases will need this.
> the base and the legacy MMIO hole. Modify existing helpers to improve DF > 4.5 denormalization flow and add helper to remove the base and hole.
Please write the what/context, why/issue, and how/fix information as separate paragraphs even if they're just a single sentence each. I think this helps to find the details more easily.
> > Signed-off-by: John Allen <john.allen@amd.com> > --- > drivers/ras/amd/atl/core.c | 43 ++++++++++++++++++++++------------ > drivers/ras/amd/atl/internal.h | 3 +++ > 2 files changed, 31 insertions(+), 15 deletions(-) > > diff --git a/drivers/ras/amd/atl/core.c b/drivers/ras/amd/atl/core.c > index c1710d233adb..cafdfc57d929 100644 > --- a/drivers/ras/amd/atl/core.c > +++ b/drivers/ras/amd/atl/core.c > @@ -49,15 +49,26 @@ static bool legacy_hole_en(struct addr_ctx *ctx) > return FIELD_GET(DF_LEGACY_MMIO_HOLE_EN, reg); > } > > -static int add_legacy_hole(struct addr_ctx *ctx) > +static u64 add_legacy_hole(struct addr_ctx *ctx, u64 addr) > { > if (!legacy_hole_en(ctx)) > - return 0; > + return addr; > > - if (ctx->addr >= df_cfg.dram_hole_base) > - ctx->addr += (BIT_ULL(32) - df_cfg.dram_hole_base); > + if (addr >= df_cfg.dram_hole_base) > + addr += (BIT_ULL(32) - df_cfg.dram_hole_base); > > - return 0; > + return addr; > +} > + > +static u64 remove_legacy_hole(struct addr_ctx *ctx, u64 addr) > +{ > + if (!legacy_hole_en(ctx)) > + return addr; > + > + if (addr >= df_cfg.dram_hole_base) > + addr -= (BIT_ULL(32) - df_cfg.dram_hole_base); > + > + return addr; > } > > static u64 get_base_addr(struct addr_ctx *ctx) > @@ -72,14 +83,16 @@ static u64 get_base_addr(struct addr_ctx *ctx) > return base_addr << DF_DRAM_BASE_LIMIT_LSB; > } > > -static int add_base_and_hole(struct addr_ctx *ctx) > +u64 add_base_and_hole(struct addr_ctx *ctx, u64 addr) > { > - ctx->ret_addr += get_base_addr(ctx); > - > - if (add_legacy_hole(ctx)) > - return -EINVAL; > + addr += get_base_addr(ctx); > + return add_legacy_hole(ctx, addr); > +} > > - return 0; > +u64 remove_base_and_hole(struct addr_ctx *ctx, u64 addr) > +{ > + addr -= get_base_addr(ctx); > + return remove_legacy_hole(ctx, addr);
This should be the inverse of the "add" operation, I think. So remove the legacy hole first, then remove the base address.
> } > > static bool late_hole_remove(struct addr_ctx *ctx) > @@ -123,14 +136,14 @@ unsigned long norm_to_sys_addr(u8 socket_id, u8 die_id, u8 coh_st_inst_id, unsig > if (denormalize_address(&ctx)) > return -EINVAL; > > - if (!late_hole_remove(&ctx) && add_base_and_hole(&ctx)) > - return -EINVAL; > + if (!late_hole_remove(&ctx)) > + ctx.ret_addr = add_base_and_hole(&ctx, ctx.ret_addr); > > if (dehash_address(&ctx)) > return -EINVAL; > > - if (late_hole_remove(&ctx) && add_base_and_hole(&ctx)) > - return -EINVAL; > + if (late_hole_remove(&ctx)) > + ctx.ret_addr = add_base_and_hole(&ctx, ctx.ret_addr); > > if (addr_over_limit(&ctx)) > return -EINVAL; > diff --git a/drivers/ras/amd/atl/internal.h b/drivers/ras/amd/atl/internal.h > index 1413c8ddc6c5..05b870fcb24e 100644 > --- a/drivers/ras/amd/atl/internal.h > +++ b/drivers/ras/amd/atl/internal.h > @@ -236,6 +236,9 @@ int dehash_address(struct addr_ctx *ctx); > unsigned long norm_to_sys_addr(u8 socket_id, u8 die_id, u8 coh_st_inst_id, unsigned long addr); > unsigned long convert_umc_mca_addr_to_sys_addr(struct atl_err *err); > > +u64 add_base_and_hole(struct addr_ctx *ctx, u64 addr); > +u64 remove_base_and_hole(struct addr_ctx *ctx, u64 addr);
remove_base_and_hole() is only used in denormalize.c, correct? So why not define it there as static? Other than trying to keep the code together and symmetrical, I mean.
Thanks, Yazen
| |