[PATCH RFC v3 2/2] memory: pl353: Add driver for arm pl353 static memory controller

[punnaiah choudary kalluri]

Hi Arnd and Brian,

   Any comments on this smc driver ?

Regards,
Punnaiah

On Mon, Jul 7, 2014 at 10:15 PM, punnaiah choudary kalluri
<punnaia at xilinx.com> wrote:
> On Thu, Jul 3, 2014 at 4:09 PM, Arnd Bergmann <arnd at arndb.de> wrote:
>> On Friday 27 June 2014 08:32:42 Punnaiah Choudary Kalluri wrote:
>>
>>> +/**
>>> + * struct pl353_smc_data - Private smc driver structure
>>> + * @devclk:          Pointer to the peripheral clock
>>> + * @aperclk:         Pointer to the APER clock
>>> + */
>>> +struct pl353_smc_data {
>>> +     struct clk              *memclk;
>>> +     struct clk              *aclk;
>>> +};
>>> +
>>> +/* SMC virtual register base */
>>> +static void __iomem *pl353_smc_base;
>>
>> Drivers should not assume that there is only a single instance of the
>> hardware in the system. Please remove this global variable and
>> change the code to pass around a pointer to a structure containing
>> it.
>
> Since, the pl353 nand driver has dependency with this driver for configuring the
> bus width,operational parameters and ecc specific settings.
> So, i didnt find any mechanism for passing this instance from other driver other
> than exporting this instance.
> http://lists.infradead.org/pipermail/linux-mtd/2014-April/053432.html
>
> Please suggest the better option if you want it to be not global.
>
>>> +/**
>>> + * pl353_smc_set_buswidth - Set memory buswidth
>>> + * @bw:      Memory buswidth (8 | 16)
>>> + * Return: 0 on success or negative errno.
>>> + */
>>> +int pl353_smc_set_buswidth(unsigned int bw)
>>> +{
>>> +
>>> +     if (bw != PL353_SMC_MEM_WIDTH_8  && bw != PL353_SMC_MEM_WIDTH_16)
>>> +             return -EINVAL;
>>> +
>>> +     writel(bw, pl353_smc_base + PL353_SMC_SET_OPMODE_OFFS);
>>> +     writel(PL353_SMC_DC_UPT_NAND_REGS, pl353_smc_base +
>>> +            PL353_SMC_DIRECT_CMD_OFFS);
>>> +
>>> +     return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(pl353_smc_set_buswidth);
>>
>> Why is this an exported interface? Wouldn't that setting just be
>> determined from DT when the device is probed?
>
> Nand driver rely on auto bus width detection mechanism for identifying
> the bus width using onfi parameter page settings. So, once the driver
> finds the bus width, updates accordingly using this API
>
>>
>>> +/**
>>> + * pl353_smc_get_nand_int_status_raw - Get NAND interrupt status bit
>>> + * Return: the raw_int_status1 bit from the memc_status register
>>> + */
>>> +int pl353_smc_get_nand_int_status_raw(void)
>>> +{
>>> +     u32 reg;
>>> +
>>> +     reg = readl(pl353_smc_base + PL353_SMC_MEMC_STATUS_OFFS);
>>> +     reg >>= PL353_SMC_MEMC_STATUS_RAW_INT_1_SHIFT;
>>> +     reg &= 1;
>>> +
>>> +     return reg;
>>> +}
>>> +EXPORT_SYMBOL_GPL(pl353_smc_get_nand_int_status_raw);
>>> +
>>> +/**
>>> + * pl353_smc_clr_nand_int - Clear NAND interrupt
>>> + */
>>> +void pl353_smc_clr_nand_int(void)
>>> +{
>>> +     writel(PL353_SMC_CFG_CLR_INT_CLR_1,
>>> +             pl353_smc_base + PL353_SMC_CFG_CLR_OFFS);
>>> +}
>>> +EXPORT_SYMBOL_GPL(pl353_smc_clr_nand_int);
>>
>> Why aren't these just part of the NAND driver?
>
> As mentioned, SMC supports two interfaces and most of the register contains
> the control/status bits for both the interfaces. so, the smc driver
> provides relevant
> APIs for controlling the nand and nor/sram interfaces. The current
> version of the
> driver supports nand interface but the future versions include
> nor/sram interface.
>
>>
>>> +     pl353_smc_set_cycles(t_rc, t_wc, t_rea, t_wp, t_clr, t_ar, t_rr);
>>> +     writel(PL353_SMC_CFG_CLR_INT_CLR_1,
>>> +             pl353_smc_base + PL353_SMC_CFG_CLR_OFFS);
>>> +     writel(PL353_SMC_DC_UPT_NAND_REGS, pl353_smc_base +
>>> +            PL353_SMC_DIRECT_CMD_OFFS);
>>> +     /* Wait till the ECC operation is complete */
>>> +     do {
>>> +             if (pl353_smc_ecc_is_busy_noirq())
>>> +                     cpu_relax();
>>> +             else
>>> +                     break;
>>> +     } while (!time_after_eq(jiffies, timeout));
>>
>> This blocks the CPU for up to a second (the timeout value). Since you are
>> not in atomic context, you can instead sleep here, e.g. using msleep(1)
>> or some other appropriate timeout.
>>
>> What is the expected average duration of the loop?
>
> The expected duration of the loop depends on the given timing parameters.
> In the worst case it should not be more than 2-3 milli seconds and in the best
> case it should be 2-3 micro seconds. I agree your point to use msleep rather
> than the busy wait using cpu_relax. I will fix this in next version.
>
> Please provide your inputs for my above answers.
>
> Thanks for the review
> Punnaiah
>>
>>         Arnd