On Sun, 17 Jan 2016, Rafał Miłecki wrote:
> Using KSEG0ADDR makes code highly MIPS dependent and not portable.
> Ideally we should use ioremap_cache which is generic but right now MIPS
> doesn't define it which results in kernel using ioremap instead.
> Once MIPS gets ioremap_cache we will switch to it, it's going to be
> trivial with this patch applied first.
Agreed, I think your proposal is a move in the right direction and does
not regress current code as we're using KSEG0ADDR anyway, which is
MIPS-specific.
> KSEG0ADDR was translating 0x1c000000 into 0x9c000000. With
> ioremap_cachable we use MIPS's __ioremap (and remap_area_pages). This
> results in different address (e.g. 0xc0080000) but it still should be
> cached as expected and it was successfully tested with BCM47186B0.
This is due to this piece:
/*
* Map uncached objects in the low 512mb of address space using KSEG1,
* otherwise map using page tables.
*/
if (IS_LOW512(phys_addr) && IS_LOW512(last_addr) &&
flags == _CACHE_UNCACHED)
return (void __iomem *) CKSEG1ADDR(phys_addr);
special-casing uncached mapping only (replicated in 2 places). I think
there will really be no harm from returning a KSEG0 mapping for calls
requesting a caching mode equal to `_page_cachable_default', which --
depending on the cache architecture -- will have been either hardwired or
prearranged via Config.K0. I think there's really no need to put pressure
on the TLB, which may be small, in cases where a fixed mapping will do.
We also seem to pessimise the `cpu_has_64bit_addresses' case and always
return a fixed uncached mapping, regardless of the mode requested, hmm...
> Moreover drivers/bcma/driver_chipcommon_sflash.c nicely sets us up a
> struct resource for access window, but we just aren't using it. Use it
> now and drop duplicated info.
Agreed, again, except from one nit, as noted below.
> diff --git a/drivers/mtd/devices/bcm47xxsflash.c
> b/drivers/mtd/devices/bcm47xxsflash.c
> index 347bb83..84f8fae 100644
> --- a/drivers/mtd/devices/bcm47xxsflash.c
> +++ b/drivers/mtd/devices/bcm47xxsflash.c
> @@ -275,15 +275,33 @@ static void bcm47xxsflash_bcma_cc_write(struct
> bcm47xxsflash *b47s, u16 offset,
>
> static int bcm47xxsflash_bcma_probe(struct platform_device *pdev)
> {
> - struct bcma_sflash *sflash = dev_get_platdata(&pdev->dev);
> + struct device *dev = &pdev->dev;
> + struct bcma_sflash *sflash = dev_get_platdata(dev);
> struct bcm47xxsflash *b47s;
> + struct resource *res;
> int err;
>
> - b47s = devm_kzalloc(&pdev->dev, sizeof(*b47s), GFP_KERNEL);
> + b47s = devm_kzalloc(dev, sizeof(*b47s), GFP_KERNEL);
> if (!b47s)
> return -ENOMEM;
> sflash->priv = b47s;
>
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(dev, "invalid resource\n");
> + return -EINVAL;
> + }
> + if (!devm_request_mem_region(dev, res->start, resource_size(res),
> + res->name)) {
> + dev_err(dev, "can't request region for resource %pR\n", res);
> + return -EBUSY;
> + }
> + b47s->window = ioremap_cachable(res->start, resource_size(res));
> + if (!b47s->window) {
> + dev_err(dev, "ioremap failed for resource %pR\n", res);
You need to call `devm_release_mem_region' in this case.
> + return -ENOMEM;
> + }
> +
> b47s->bcma_cc = container_of(sflash, struct bcma_drv_cc, sflash);
> b47s->cc_read = bcm47xxsflash_bcma_cc_read;
> b47s->cc_write = bcm47xxsflash_bcma_cc_write;
LGTM otherwise.
Maciej
