Re: [PATCH v5 6/7] ARM: zynq: Add OCM controller driver

[Arnd Bergmann <arnd@arndb.de>]

On Monday 01 December 2014 14:24:57 Michal Simek wrote:
> The driver provide memory allocator which can
> be used by others drivers to allocate memory inside OCM.
> All locations for 64kB blocks are supported
> and driver is trying to allocate the largest continuous
> block of memory.
> 
> Checking mpcore addressing filterring is not done here
> but could be added in future.
> 
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>

The driver doesn't actually have any interface as far as I can tell,
so I wonder how you expect it to be used.

Do you have a list of drivers that would be using it?

How does it relate to the generic "mmio-sram" binding? Is this
meant as a specialization?

> +/**
> + * zynq_ocmc_irq_handler - Interrupt service routine of the OCM controller
> + * @irq:	IRQ number
> + * @data:	Pointer to the zynq_ocmc_dev structure
> + *
> + * Return:     IRQ_HANDLED always
> + */
> +static irqreturn_t zynq_ocmc_irq_handler(int irq, void *data)
> +{
> +	u32 sts;
> +	u32 err_addr;
> +	struct zynq_ocmc_dev *zynq_ocmc = data;
> +
> +	/* check status */
> +	sts = readl(zynq_ocmc->base + ZYNQ_OCMC_IRQ_STS);
> +	if (sts & ZYNQ_OCMC_IRQ_STS_ERR_MASK) {
> +		/* check error address */
> +		err_addr = readl(zynq_ocmc->base + ZYNQ_OCMC_PARITY_ERRADDRESS);
> +		pr_err("%s: OCM err intr generated at 0x%04x (stat: 0x%08x).",
> +		       __func__, err_addr, sts & ZYNQ_OCMC_IRQ_STS_ERR_MASK);
> +	}
> +	pr_warn("%s: Interrupt generated by OCM, but no error is found.",
> +		__func__);
> +
> +	return IRQ_HANDLED;
> +}

I'm also puzzled by this: you don't really do anything here but print
a message. What is the purpose of this interrupt? As this looks unrelated
to the rest of the driver, maybe this should be part of drivers/edac
instead?

> +static int zynq_ocmc_probe(struct platform_device *pdev)
> +{
> +       int ret;
> +       struct zynq_ocmc_dev *zynq_ocmc;
> +       u32 i, ocm_config, curr;
> +       struct resource *res;
> +
> +       ocm_config = zynq_slcr_get_ocm_config();
> +
> +       zynq_ocmc = devm_kzalloc(&pdev->dev, sizeof(*zynq_ocmc),
> GFP_KERNEL);
> +       if (!zynq_ocmc)
> +               return -ENOMEM;
> +
> +       zynq_ocmc->pool = devm_gen_pool_create(&pdev->dev,
> +                                              ilog2(ZYNQ_OCMC_GRANULARITY),
> +                                              -1);
> +       if (!zynq_ocmc->pool)
> +               return -ENOMEM;
> +
> +       curr = 0; /* For storing current struct resource for OCM */
> +       for (i = 0; i < ZYNQ_OCMC_BLOCKS; i++) {
> +               u32 base, start, end;
> +
> +               /* Setup base address for 64kB OCM block */
> +               if (ocm_config & BIT(i))
> +                       base = ZYNQ_OCMC_HIGHADDR;
> +               else
> +                       base = ZYNQ_OCMC_LOWADDR;

You write in the introductory mail that you want to support detecting the
address from the slcr, and possibly even allow changing it at runtime,
but I don't understand what that would be good for.

It's not uncommon to describe in DT settings that one can also find
out from hardware but that are set up by the boot loader in a particular
way. Why not this one? For all I can tell, that would let you simply
use one driver for the EDAC and the generic mmio-sram from the memory,
without the need to do anything further.

	Arnd