On Tue, Apr 16, 2013 at 12:15:47PM -0500, Aravind wrote: > >This one case in point, please redo it against tip/master. > > I had based off bp.git's master... and it misses an additional > 'PCI_DEVICE' line (Hence the conflict) > I shall redo it against Linus's tree.. No, against tip/master, please. git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git, the master branch. > >>@@ -133,6 +134,15 @@ static int f15_read_dct_pci_cfg(struct amd64_pvt *pvt, int addr, u32 *val, > >> return __amd64_read_pci_cfg_dword(pvt->F2, addr, val, func); > >> } > >>+static int f16_read_dct_pci_cfg(struct amd64_pvt *pvt, int addr, u32 *val, > >>+ const char *func) > >>+{ > >>+ if (addr >= 0x100) > >>+ return -EINVAL; > >I'm very sceptical F16h doesn't have F2 extended PCI config addresses. > >Please check the BKDG. > > > >If it does have, use f10_read_dct_pci_cfg, if it doesn't, use > >k8_read_dct_pci_cfg without introducing a new accessor while the other > >ones can be used. > > > >Whichever one you take, please add a comment somewhere explaining why it > >is ok to use it on F16h. > > Here, What I really wanted to do was to restrict the access to > only 1 DCT (as fam16 does not have a DCT1 and hence not allow any > addr > =0x100). What are you talking about? I'm sure it has, say, D18F2x110 DRAM Controller Select Low, for example. And this address is > 0x100. So for F16h you can simply take the F10h methods and ignore DctCfgSel because it always will be 0. > Yes, for this I can modify the code to just use f10_read_dct_pci_cfg > or k8_read_dct_pci_cfg. Yes, please do that. > >>+ u64 base_bits_low, base_bits_high; > >>+ u64 mask_bits_low, mask_bits_high; > >>+ u8 addr_shift_low, addr_shift_high; > >>+ > >>+ csbase = pvt->csels[dct].csbases[csrow]; > >>+ csmask = pvt->csels[dct].csmasks[csrow >> 1]; > >>+ base_bits_low = mask_bits_low = GENMASK(5 , 15); > >>+ base_bits_high = mask_bits_high = GENMASK(19 , 30); > >>+ addr_shift_low = 6; > >>+ addr_shift_high = 8; > >Hold on, are you saying "D18F2x[5C:40]_dct[1:0] DRAM CS Base Address" > >register definitions in the F16h BKDG has this: > > > >30:19 -> BaseAddr[38:27]: normalized physical base address bits [38:27] > > > >and > > > >15:5 -> BaseAddr[21:11]: normalized physical base address bits [21:11] > > > >? > > > >Please verify with BKDG authors whether those numbers are correct > >because the diff of 8 address bits has always been this up until now. > > That is correct. (I have verified it internally too..) Ok, then do the following: Read the low bits, shift them by 2 so that they're at the right position to be shifted by 8 like the high bits: *base = (csbase & GENMASK(5, 15)) << 2; *mask = (csmask & GENMASK(5, 15)) << 2; *base |= (csbase & GENMASK(19, 30)) << 8; *mask |= (csmask & GENMASK(19, 30)) << 8; return; AFAICT, this looks much simpler. Also, add a small comment why the special handling for F16h. Thanks. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html