Re: [PATCH mtd] mtd:devices: Add Altera EPCQ Driver

[Brian Norris]

On Tue, Jan 27, 2015 at 02:53:59PM +0800, Viet Nga Dao wrote:
> On Tue, Jan 20, 2015 at 10:05 AM, Viet Nga Dao <vndao@xxxxxxxxxx> wrote:
> > On Thu, Jan 15, 2015 at 5:27 PM, Viet Nga Dao <vndao@xxxxxxxxxx> wrote:
> >> On Tue, Jan 13, 2015 at 11:33 AM, Brian Norris
> >> <computersforpeace@xxxxxxxxx> wrote:
> >>> On Thu, Dec 18, 2014 at 12:23:16AM -0800, vndao@xxxxxxxxxx wrote:
> >>>> From: Viet Nga Dao <vndao@xxxxxxxxxx>

> >> That is why if rewrite the drivers to follow spi-nor structure, it
> >> will require extra decoding works for the only few used opcodes.
> >>
> >I think you'd only have some very trivial work here.
> >
> >There would be some small work to reintroduce a properly-replaceable ID
> >table, and callbacks like ->lock() and ->unlock(), but those should be
> >implemented in spi-nor.c sooner or later anyway.
> >
> 
> I am trying to modify the code to follow your recommendation. However,
> for lock and unlock functions, i have to implement it in spi_nor.c ,
> am i right? If yes, currently we already have existing spi_nor_lock
> and spi_nor_unlock functions but they could not apply for my driver.
> Should i create a new functions named altera_epcq_lock and unlock and
> then map it to callback functions or i should the put  those code
> sunder existing spi_nor_lock/unlock?

We probably want a replaceable spi_nor callback that does the
flash-specific unlock. spi_nor_lock/unlock would then call the
nor->unlock() / nor->lock() functions for you, when present.
Some of the existing code should move into their own spi_nor_st_lock() /
spi_nor_st_unlock() functions.

> >>>> diff --git a/Documentation/devicetree/bindings/mtd/altera_epcq.txt b/Documentation/devicetree/bindings/mtd/altera_epcq.txt
> >>>> new file mode 100644
> >>>> index 0000000..d14f50e
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/mtd/altera_epcq.txt
> >>>> @@ -0,0 +1,45 @@
> >>>> +* MTD Altera EPCQ driver
> >>>> +
> >>>> +Required properties:
> >>>> +- compatible: Should be "altr,epcq-1.0"
> >>>> +- reg: Address and length of the register set  for the device. It contains
> >>>> +  the information of registers in the same order as described by reg-names
> >>>> +- reg-names: Should contain the reg names
> >>>> +  "csr_base": Should contain the register configuration base address
> >>>> +  "data_base": Should contain the data base address
> >>>> +- is-epcs: boolean type.
> >>>> +             If present, the device contains EPCS flashes.
> >>>> +             Otherwise, it contains EPCQ flashes.
> >>>> +- #address-cells: Must be <1>.
> >>>> +- #size-cells: Must be <0>.
> >>>> +- flash device tree subnode, there must be a node with the following fields:
> >>>
> >>> These subnodes definitely require a 'compatible' string. Perhaps they
> >>> should be used to differentiate EPCS vs. EPCQ. Does "is-epcs" really
> >>> need to be in the top-level controller node?
> >>>
> >>>> +     - reg: Should contain the flash id
> >>>
> >>> Should, or must? (This question is relevant, because you seem to make it
> >>> optional in your code.) And what does the "flash ID" mean? It seems like
> >>> you're using as a chip-select or bank index.
> >>>
> Yes, this is used for chip select. It is required, not optional. This
> to ID for each flash in the device

OK, so correct the language here and enforce this in your driver. Right
now, you don't fail gracefully if this is missing.

> >>>> +             if (sector_start < num_sectors-(num_sectors / 4))
> >>>> +                     sr_bp = __ilog2_u32(num_sectors);
> >>>> +             else if (sector_start < num_sectors-(num_sectors / 8))
> >>>> +                     sr_bp = __ilog2_u32(num_sectors) - 1;
> >>>> +             else if (sector_start < num_sectors-(num_sectors / 16))
> >>>> +                     sr_bp = __ilog2_u32(num_sectors) - 2;
> >>>> +             else if (sector_start < num_sectors-(num_sectors / 32))
> >>>> +                     sr_bp = __ilog2_u32(num_sectors) - 3;
> >>>> +             else if (sector_start < num_sectors-(num_sectors / 64))
> >>>> +                     sr_bp = __ilog2_u32(num_sectors) - 4;
> >>>> +             else if (sector_start < num_sectors-(num_sectors / 128))
> >>>> +                     sr_bp = __ilog2_u32(num_sectors) - 5;
> >>>> +             else if (sector_start < num_sectors-(num_sectors / 256))
> >>>> +                     sr_bp = __ilog2_u32(num_sectors) - 6;
> >>>> +             else if (sector_start < num_sectors-(num_sectors / 512))
> >>>> +                     sr_bp = __ilog2_u32(num_sectors) - 7;
> >>>> +             else if (sector_start < num_sectors-(num_sectors / 1024))
> >>>> +                     sr_bp = __ilog2_u32(num_sectors) - 8;
> >>>> +             else
> >>>> +                     sr_bp = 0;  /* non area protected */
> >>>
> >>> Yikes, that's ugly! And I'm not sure it matches the EPCQ doc I found.
> >>> I'm pretty sure you can rewrite this if/else-if/else block in about 1
> >>> line though.
> >>>
> Yes, i understand that it looks ugly. But it is the best i can do
> since this function has to satisfy for all the supported devices
> (http://www.altera.com.my/literature/hb/cfg/cfg_cf52012.pdf, start
> from page 19)

Did you even try? It was possible to simplify the other case, and I'm
pretty sure this case can be simplified too. How about this? I hacked
this together and it seems to match:

        if (sector_start <= num_sectors / 2)
		sr_bp = __ilog2_u32(num_sectors);
	else
		sr_bp = fls(num_sectors - 1 - sector_start) + 1;

> >>>> +
> >>>> +             if (sr_bp < 0) {
> >>>
> >>> sr_bp is unsigned, so this is never true.
> >>>
> Ok.  I will change to int type.

Are there ever negative values?

> >>>> +static int altera_epcq_probe_config_dt(struct platform_device *pdev,
> >>>> +                                    struct device_node *np,
> >>>> +                                    struct altera_epcq_plat_data *pdata)
> >>>> +{
> >>>> +     struct device_node *pp = NULL;
> >>>> +     struct resource *epcq_res;
> >>>> +     int i = 0;
> >>>> +     u32 id;
...
> >>>> +             pdata->np[i] = pp;
> >>>> +
> >>>> +             /* Read bank id from DT */
> >>>> +             if (of_get_property(pp, "reg", &id))

I just realized; you're not using this correctly. of_get_property()
returns the *length* in the third parameter, so you're not actually
saving the bank ID here. You probably want of_property_read_u32()
instead.

> >>>
> >>> Is this property optional? Your DT binding doc doesn't make it clear,
> >>> but it seems like a property which would be wise to require (i.e., not
> >>> optional).

^^^ so there should be a failure case, where you return failure if the
property is missing.

> >>>> +                     pdata->board_flash_info[i].bank = id;
> >>>> +             i++;
> >>>> +     }
> >>>> +     pdata->num_flashes = i;
> >>>> +     return 0;
> >>>> +}

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/