Messages in this thread | | | Subject | Re: [PATCH 2/3] EDAC, amd64_edac: Extend scrub rate programmability feature for F15hM60h | From | Aravind Gopalakrishnan <> | Date | Thu, 24 Sep 2015 11:15:08 -0500 |
| |
On 9/24/2015 4:18 AM, Borislav Petkov wrote: > On Wed, Sep 16, 2015 at 03:53:30PM -0500, Aravind Gopalakrishnan wrote: >> -static int __set_scrub_rate(struct pci_dev *ctl, u32 new_bw, u32 min_rate) >> +static u32 find_scrub_rate(u32 new_bw, u32 min_rate, u32 *scrub_bw) >> { >> u32 scrubval; >> int i; >> @@ -200,28 +200,52 @@ static int __set_scrub_rate(struct pci_dev *ctl, u32 new_bw, u32 min_rate) >> } >> >> scrubval = scrubrates[i].scrubval; >> + *scrub_bw = scrubval ? scrubrates[i].bandwidth : 0; >> >> - pci_write_bits32(ctl, SCRCTRL, scrubval, 0x001F); >> + return scrubval; >> +} >> >> - if (scrubval) >> - return scrubrates[i].bandwidth; >> +static inline void __set_scrub_rate(struct pci_dev *ctl, int offset, >> + u32 scrubval) >> +{ >> + pci_write_bits32(ctl, offset, scrubval, SCRMASK); >> >> - return 0; >> } >> > What is all that churn good for? > > What's wrong with simply adding the model 0x60 check to > __set_scrub_rate() and doing the proper write there?
I was thinking it's a little better from readability POV to separate out the for loop which does the job of finding the scrub value to program; And __set_scrub_rate() writes the value to the appropriate register.
Maybe I am making it too modular?
>> >> - amd64_read_pci_cfg(pvt->F3, SCRCTRL, &scrubval); >> + if (pvt->fam == 0x15 && pvt->model == 0x60) { >> + /* Since we mirror the same scrubrate value across >> + * both DCTs, it is enough to read the value off one of >> + * the DCT registers. >> + */ >> + f15h_select_dct(pvt, 0); > If it is enough, why do you select DCT 0? Just read the currently > selected one, whichever it is...
Yeah, that's a good point! Will fix this.
>> static int f16_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct, >> - unsigned cs_mode, int cs_mask_nr) >> + unsigned cs_mode, int cs_mask_nr) >> { >> WARN_ON(cs_mode > 12); > Why is that hunk here? > >> @@ -1666,7 +1699,7 @@ static int f1x_match_to_this_node(struct amd64_pvt *pvt, unsigned range, >> } >> >> static int f15_m30h_match_to_this_node(struct amd64_pvt *pvt, unsigned range, >> - u64 sys_addr, int *chan_sel) >> + u64 sys_addr, int *chan_sel) >> { >> int cs_found = -EINVAL; >> int num_dcts_intlv = 0; > That one too? >
I realize it's unrelated to the patch and it's not doing something useful; But I was thinking I'll fix the above indentation issues to keep indent rules consistent and since I was touching the file anyway. Can remove those in a V2..
Thanks, -Aravind.
| |