[Skiboot] [PATCH v2 2/6] VAS: Initialize the basic VAS internal registers

[Sukadev Bhattiprolu]

Oliver O'Halloran [oohall at gmail.com] wrote:
> > +
> > +       for (reg = 0; reg < nregs; reg++) {
> > +               if (!((valid_wcm_mask >> reg) & 0x1))
> > +                       continue;
> > +
> > +               *(window_start + reg) = 0x0ULL;
> 
> Is it safe to access these MMIO registers with cached loads/stores?

I am not sure. Please let me know of a safer/better way to do this.

> 
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int init_windows(struct proc_chip *chip)
> > +{
> > +       int i, rc;
> > +
> > +       for (i = 0; i < max_win_per_chip; i++) {
> > +               rc = init_one_window(chip, i);
> > +               if (rc)
> > +                       return rc;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int init_one_chip(struct proc_chip *chip)
> > +{
> > +       if (init_wcbs(chip) || init_wcm(chip) || init_uwcm(chip))
> > +               return -1;
> > +
> > +       reset_fir(chip);
> > +
> > +       if (init_north_ctl(chip))
> > +               return -1;
> > +
> > +       if (init_rma(chip) || init_windows(chip))
> > +               goto out;
> > +
> > +       printf("VAS: Initialized chip %d\n", chip->id);
> 
> In general we prefer using prlog() with an appropriate log level over
> raw printf(),

Ok.

> 
> > +       return 0;
> > +
> > +out:
> > +       reset_north_ctl(chip);
> > +       return -1;
> > +}
> > +
> > +void init_vas()
> > +{
> > +       int ct;
> > +       struct proc_chip *chip;
> > +
> > +       chip = next_chip(NULL);
> > +       ct = chip->type;
> > +
> > +       if (ct != PROC_CHIP_P9_CUMULUS && ct != PROC_CHIP_P9_NIMBUS)
> > +               return;
> 
> Can this be replaced with a proc_gen >= proc_gen_p9 check?

Yes, will do that.

> 
> > +
> > +       for_each_chip(chip) {
> > +               if (init_one_chip(chip))
> > +                       goto cleanup;
> 
> Can you report an error if we fail to initialise VAS on a given chip.

Yes.

> Also, if we succeed in initialising some chips how what happens to the
> memory that's allocated with local_alloc(). It looks like it will be
> leaked here.

It will leave the memory allocated but unused but its not a continuous
leak. I saw lot of assert() calls when memory allocation fails in skiboot
and assumed it was less likely. I will add a field to proc_chip and use
it it free up the memory.

Thanks for the review.

Sukadev