Messages in this thread | | | Subject | Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs | From | Stephen Boyd <> | Date | Wed, 22 Jan 2020 10:15:23 -0800 |
| |
Quoting patrick.rudolph@9elements.com (2019-12-19 23:12:22) > On Mon, 2019-12-09 at 22:54 -0800, Julius Werner wrote: > > > +static int cbmem_probe(struct coreboot_device *cdev) > > > +{ > > > + struct device *dev = &cdev->dev; > > > + struct cb_priv *priv; > > > + int err; > > > + > > > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > > > + if (!priv) > > > + return -ENOMEM; > > > + > > > + memcpy(&priv->entry, &cdev->cbmem_entry, sizeof(priv- > > > >entry)); > > > + > > > + priv->remap = memremap(priv->entry.address, > > > + priv->entry.entry_size, > > > MEMREMAP_WB); > > > > We've just been discussing some problems with CBMEM areas and memory > > mapping types in Chrome OS. CBMEM is not guaranteed to be page- > > aligned > > (at least not the "small" entries), but the kernel can only assign > > memory attributes for a page at a time (and refuses to map the same > > area twice with two different memory types, for good reason). So if > > CBMEM entries sharing a page are mapped as writeback by one driver > > but > > uncached by the other, things break. > > > > There are some CBMEM entries that need to be mapped uncached (e.g. > > the > > ACPI UCSI table, which isn't even handled by anything using this > > CBMEM > > code) and others for which it would make more sense (e.g. the memory > > console, where firmware may add more lines at runtime), but I don't > > think there are any regions that really *need* to be writeback. None > > of the stuff accessing these areas should access them often enough > > that caching matters, and I think it's generally more common to map > > firmware memory areas as uncached anyway. So how about we standardize > > on mapping it all uncached to avoid any attribute clashes? (That > > would > > mean changing the existing VPD and memconsole drivers to use > > ioremap(), too.) > > I wasn't aware that CBMEM is used for DMA as there's no such concept in > coreboot yet. For me it looks like the UCSI is regular DRAM mapped as > WB accessed by the ACPI interpreter. > I'll prepare a new patch-set using ioremap in all drivers that access > CBMEM. >
We shouldn't use ioremap() here as this isn't I/O memory. It's just regular memory that wants be mapped with some particular set of attributes, hence the use of memremap().
| |