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

[Punnaiah Choudary Kalluri]

Hi Arnd

>-----Original Message-----
>From: Arnd Bergmann [mailto:arnd at arndb.de]
>Sent: Thursday, July 24, 2014 1:18 AM
>To: Punnaiah Choudary Kalluri
>Cc: Punnaiah Choudary Kalluri; robh+dt at kernel.org; pawel.moll at arm.com;
>mark.rutland at arm.com; ijc+devicetree at hellion.org.uk; Kumar Gala; Rob
>Landley; Michal Simek; Grant Likely; gregkh at linuxfoundation.org;
>jason at lakedaemon.net; ezequiel.garcia at free-electrons.com; David
>Woodhouse; Brian Norris; artem.bityutskiy at linux.intel.com; Gupta, Pekon;
>jussi.kivilinna at iki.fi; acourbot at nvidia.com; Khoronzhuk, Ivan;
>joern at logfs.org; devicetree at vger.kernel.org; linux-doc at vger.kernel.org;
>linux-kernel at vger.kernel.org; linux-mtd at lists.infradead.org; Punnaiah
>Choudary; Anirudha Sarangi; Srikanth Vemula
>Subject: Re: [PATCH RFC v3 2/2] memory: pl353: Add driver for arm pl353 static
>memory controller
>
>On Monday 21 July 2014, punnaiah choudary kalluri wrote:
>> 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.
>
>I would expect the nand device to be a child of the smc device. In this case,
>the nand driver can just pass a pointer to its own 'struct device', and the
>smc driver can find its data using dev_get_drvdata(dev->parent).

Ok.
>
>> >>> +/**
>> >>> + * 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
>
>This should at least pass the device pointer then.

Ok. Thanks. I will update the driver and send you the updated version.

Regards,
Punnaiah
>
>
>	Arnd