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

[Viet Nga Dao]

On Thu, Feb 5, 2015 at 4:37 AM, Brian Norris
<computersforpeace@xxxxxxxxx> wrote:
> 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.
>

When this will be implemented? As for time being, what should i do for
these 2 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/