RE: [PATCH v4 01/39] ARM: OMAP2+: gpmc: driver conversion
Hi Jon, On Tue, May 08, 2012 at 20:38:24, Hunter, Jon wrote: Different ways of doing things, this looks cleaner to me as already it is checked, and time of execution in both cases would not differ much. Sure. However, I think that this could be simplified so that it is easier to read. At a minimum you may wish to add some comments to explain the purposes of the multi-level if-statements as it is not intuitive for the reader. Ok What happens if a device uses more than one wait-pin? In other words a device with two chip-selects that uses two wait-pins? Please re-read http://www.mail-archive.com/linux-omap@vger.kernel.org/msg67702.html and your reply Ok thats fine. Sorry for my repeated questions, but I think that this just highlights that this code is not clear in it purpose. So again may be some comments would make this all clearer. Ok Regards Afzal -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 01/39] ARM: OMAP2+: gpmc: driver conversion
Hi Jon,
On Mon, May 07, 2012 at 21:32:58, Hunter, Jon wrote:
+ /* no waitpin */
+ case 0:
+ break;
+ default:
+ dev_err(gpmc-dev, multiple waitpins selected on CS:%u\n, cs);
+ return -EINVAL;
+ break;
+ }
Why not combined case 0 and default? Both are invalid configurations so
just report invalid selection.
Case 0 is not invalid, a case where waitpin is not used, default refers
to when a user selects multiple waitpins wrongly.
Ok. Then for case 0, just return here. If the polarity is set, you could
print an error here.
Different ways of doing things, this looks cleaner to me as already it is
checked, and time of execution in both cases would not differ much.
+ if (gd-have_waitpin) {
+ if (gd-waitpin != idx ||
+ gd-waitpin_polarity != polarity) {
+ dev_err(gpmc-dev, error: conflict: waitpin %u
with polarity %d on device %s.%d\n,
+ gd-waitpin, gd-waitpin_polarity,
+ gd-name, gd-id);
+ return -EBUSY;
+ }
+ } else {
Don't need the else as you are going to return in the above.
Not always, only in case of error
Ok, but seems that it can be simplified a little.
What happens if a device uses more than one wait-pin? In other words a
device with two chip-selects that uses two wait-pins?
Please re-read
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg67702.html
and your reply
Regards
Afzal
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 01/39] ARM: OMAP2+: gpmc: driver conversion
Hi Jon, On Mon, May 07, 2012 at 21:53:34, Hunter, Jon wrote: Write protect is a pin and there is only one. Like the waitpins and CS signals this needs to be associated with a device. It would make sense that this is associated with the cs data. As far as platform are concerned, it is associated with cs, it is only that while configuring CS, it is propagated such that it is done once. Hmmm ... but here it looks like if write-protect is used you are going to turn it on. A feature like this seems that it does need to be handled by the device's driver. Enabling and disabling write-protect are functions that I would expect are exported to the device's driver and not directly enabled in the gpmc driver during probe. However, maybe is could be valid for the write protect to be enabled by default which could make sense. However, I don't see anywhere else in the driver to control it. Shouldn't we warn on multiple devices trying to use write-protect when parsing their configuration? Even if a single device sets write protect, kernel will log it. That does not seem sufficient. It should be flagged as an error. Write protect is a resource like the CS and waitpins and so I would have thought that it should be reserved in the same way. Isn't it valid for multiple devices to make use of write protect pin ? Regards Afzal -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/39] ARM: OMAP2+: gpmc: driver conversion
Hi Afzal,
On 05/08/2012 01:18 AM, Mohammed, Afzal wrote:
Hi Jon,
On Mon, May 07, 2012 at 21:32:58, Hunter, Jon wrote:
+ /* no waitpin */
+ case 0:
+ break;
+ default:
+ dev_err(gpmc-dev, multiple waitpins selected on CS:%u\n, cs);
+ return -EINVAL;
+ break;
+ }
Why not combined case 0 and default? Both are invalid configurations so
just report invalid selection.
Case 0 is not invalid, a case where waitpin is not used, default refers
to when a user selects multiple waitpins wrongly.
Ok. Then for case 0, just return here. If the polarity is set, you could
print an error here.
Different ways of doing things, this looks cleaner to me as already it is
checked, and time of execution in both cases would not differ much.
Sure. However, I think that this could be simplified so that it is
easier to read. At a minimum you may wish to add some comments to
explain the purposes of the multi-level if-statements as it is not
intuitive for the reader.
+ if (gd-have_waitpin) {
+ if (gd-waitpin != idx ||
+ gd-waitpin_polarity != polarity) {
+ dev_err(gpmc-dev, error: conflict: waitpin %u
with polarity %d on device %s.%d\n,
+ gd-waitpin, gd-waitpin_polarity,
+ gd-name, gd-id);
+ return -EBUSY;
+ }
+ } else {
Don't need the else as you are going to return in the above.
Not always, only in case of error
Ok, but seems that it can be simplified a little.
What happens if a device uses more than one wait-pin? In other words a
device with two chip-selects that uses two wait-pins?
Please re-read
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg67702.html
and your reply
Ok thats fine. Sorry for my repeated questions, but I think that this
just highlights that this code is not clear in it purpose. So again may
be some comments would make this all clearer.
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 01/39] ARM: OMAP2+: gpmc: driver conversion
Hi Jon, On Fri, May 04, 2012 at 21:50:16, Hunter, Jon wrote: As mentioned in the cover letter, Additional features that currently boards in mainline does not make use of like, waitpin interrupt handling, changes to leverage revision 6 IP differences has not been incorporated. Priority in this series is to convert into a driver, get all boards working on mainline. Once all boards are working with gpmc driver, these features which are not required for currently supported boards can be added later. Yes, but I meant why 2 and not say 5? Anyway, I think that this should be marked with a comment like a TODO so it is clear that this needs to be re-visited. Ok, it will be marked with TODO I think that it make more sense to have the wait-pin information part of the gpmc_cs_data structure for the following reasons ... Waitpin information is indeed a part of cs as far as boards are concerned, it is only that it gets propogated to device Why does the device's driver care? From my point of view, the wait-pin is just part of the interface setup/configuration. The external device may require this and assumes that this has been setup along with the CS and interface timing, but the device's driver should not care. Remember this is just a ready signal used to stall the access. Once configured, software should be unaware of it. By device, it is referred to gpmc device which only gpmc driver is aware, the peripheral device's driver is not at all aware. Also, any reason why waitpin_polarity is an int? I see you define LOW as -1, but I not sure why LOW cannot be 0 as 0 is programmed into the register for an active low wait signal. Only intention is not to alter default waitpin polarity value, i.e. if any board does make use of it w/o knowledge of Kernel, I don't want to break it, there may be an argument saying that the board code is buggy, while if some board does so, it is, but don't want to break working one. Here unless user explicitly set the flag, it does nothing on polarity Ok. Do such scenario's exist today? Please note that board code will be removed in the future and device-tree will replace. So if there are no cases today, I would not be concerned. Unless this could be something that has already been configured by the bootloader. However, in that case would be even call this function? Let me check this, here only intent was to play safe, as I have only two boards to test. +int gpmc_cs_reconfigure(char *name, int id, struct gpmc_cs_data *cs) What scenario is this function used in? May be worth adding a comment about the function. Ok, it was required for OneNAND, as it needs to reconfigure Ok, but why is that? Unless this is something generic about one-nand I don't comprehend. I have a high-level understanding of one-nand, but I am not familiar with the specifics that require it to be reconfigured. Not too familiar with OneNAND, from the code it has been understood that to set into sync mode, first it may have to set to async mode, read frequency from OneNAND, then setup sync mode for that frequency. + gpmc_setup_writeprotect(gpmc); Write protect is a pin and there is only one. Like the waitpins and CS signals this needs to be associated with a device. It would make sense that this is associated with the cs data. As far as platform are concerned, it is associated with cs, it is only that while configuring CS, it is propagated such that it is done once. Hmmm ... but here it looks like if write-protect is used you are going to turn it on. A feature like this seems that it does need to be handled by the device's driver. Enabling and disabling write-protect are functions that I would expect are exported to the device's driver and not directly enabled in the gpmc driver during probe. However, maybe is could be valid for the write protect to be enabled by default which could make sense. However, I don't see anywhere else in the driver to control it. Shouldn't we warn on multiple devices trying to use write-protect when parsing their configuration? Even if a single device sets write protect, kernel will log it. Regards Afzal -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 01/39] ARM: OMAP2+: gpmc: driver conversion
Hi Jon,
On Fri, May 04, 2012 at 21:57:10, Hunter, Jon wrote:
- gpmc_write_reg(GPMC_SYSCONFIG, l);
- gpmc_mem_init();
+ switch (conf GPMC_WAITPIN_MASK) {
+ case GPMC_WAITPIN_0:
+ idx = GPMC_WAITPIN_IDX0;
+ break;
+ case GPMC_WAITPIN_1:
+ idx = GPMC_WAITPIN_IDX1;
+ break;
+ case GPMC_WAITPIN_2:
+ idx = GPMC_WAITPIN_IDX2;
+ break;
+ case GPMC_WAITPIN_3:
+ idx = GPMC_WAITPIN_IDX3;
+ break;
+ /* no waitpin */
+ case 0:
+ break;
+ default:
+ dev_err(gpmc-dev, multiple waitpins selected on CS:%u\n, cs);
+ return -EINVAL;
+ break;
+ }
Why not combined case 0 and default? Both are invalid configurations so
just report invalid selection.
Case 0 is not invalid, a case where waitpin is not used, default refers
to when a user selects multiple waitpins wrongly.
- /* initalize the irq_chained */
- irq = OMAP_GPMC_IRQ_BASE;
- for (cs = 0; cs GPMC_CS_NUM; cs++) {
- irq_set_chip_and_handler(irq, dummy_irq_chip,
- handle_simple_irq);
- set_irq_flags(irq, IRQF_VALID);
- irq++;
+ switch (conf GPMC_WAITPIN_POLARITY_MASK) {
+ case GPMC_WAITPIN_ACTIVE_LOW:
+ polarity = LOW;
+ break;
+ case GPMC_WAITPIN_ACTIVE_HIGH:
+ polarity = HIGH;
+ break;
+ /* no waitpin */
+ case 0:
+ break;
+ default:
+ dev_err(gpmc-dev, waitpin polarity set to low high\n);
+ return -EINVAL;
+ break;
}
Again, combine case 0 and default as these are invalid.
Similar to above
+ if (gd-have_waitpin) {
+ if (gd-waitpin != idx ||
+ gd-waitpin_polarity != polarity) {
+ dev_err(gpmc-dev, error: conflict: waitpin %u
with polarity %d on device %s.%d\n,
+ gd-waitpin, gd-waitpin_polarity,
+ gd-name, gd-id);
+ return -EBUSY;
+ }
+ } else {
Don't need the else as you are going to return in the above.
Not always, only in case of error
+ gd-have_waitpin = true;
+ gd-waitpin = idx;
+ gd-waitpin_polarity = polarity;
+ }
+
+ l = ~GPMC_CONFIG1_WAIT_PIN_SEL_MASK;
+ l |= GPMC_CONFIG1_WAIT_PIN_SEL(idx);
+ gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
+ } else if (polarity) {
+ dev_err(gpmc-dev, error: waitpin polarity specified with out
wait pin number on device %s.%d\n,
+ gd-name, gd-id);
+ return -EINVAL;
Drop this else-if. The above switch statements will report the bad
configuration. This seems a bit redundant.
This is required as switch statements will not report error if polarity
is specified, w/o waitpin to be used.
Regards
Afzal
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/39] ARM: OMAP2+: gpmc: driver conversion
Hi Afzal,
On 05/07/2012 06:01 AM, Mohammed, Afzal wrote:
Hi Jon,
On Fri, May 04, 2012 at 21:57:10, Hunter, Jon wrote:
- gpmc_write_reg(GPMC_SYSCONFIG, l);
- gpmc_mem_init();
+ switch (conf GPMC_WAITPIN_MASK) {
+ case GPMC_WAITPIN_0:
+ idx = GPMC_WAITPIN_IDX0;
+ break;
+ case GPMC_WAITPIN_1:
+ idx = GPMC_WAITPIN_IDX1;
+ break;
+ case GPMC_WAITPIN_2:
+ idx = GPMC_WAITPIN_IDX2;
+ break;
+ case GPMC_WAITPIN_3:
+ idx = GPMC_WAITPIN_IDX3;
+ break;
+ /* no waitpin */
+ case 0:
+ break;
+ default:
+ dev_err(gpmc-dev, multiple waitpins selected on CS:%u\n, cs);
+ return -EINVAL;
+ break;
+ }
Why not combined case 0 and default? Both are invalid configurations so
just report invalid selection.
Case 0 is not invalid, a case where waitpin is not used, default refers
to when a user selects multiple waitpins wrongly.
Ok. Then for case 0, just return here. If the polarity is set, you could
print an error here.
- /* initalize the irq_chained */
- irq = OMAP_GPMC_IRQ_BASE;
- for (cs = 0; cs GPMC_CS_NUM; cs++) {
- irq_set_chip_and_handler(irq, dummy_irq_chip,
- handle_simple_irq);
- set_irq_flags(irq, IRQF_VALID);
- irq++;
+ switch (conf GPMC_WAITPIN_POLARITY_MASK) {
+ case GPMC_WAITPIN_ACTIVE_LOW:
+ polarity = LOW;
+ break;
+ case GPMC_WAITPIN_ACTIVE_HIGH:
+ polarity = HIGH;
+ break;
+ /* no waitpin */
+ case 0:
+ break;
+ default:
+ dev_err(gpmc-dev, waitpin polarity set to low high\n);
+ return -EINVAL;
+ break;
}
Again, combine case 0 and default as these are invalid.
Similar to above
If you return above, then case 0 is not needed.
+ if (gd-have_waitpin) {
+ if (gd-waitpin != idx ||
+ gd-waitpin_polarity != polarity) {
+ dev_err(gpmc-dev, error: conflict: waitpin %u
with polarity %d on device %s.%d\n,
+ gd-waitpin, gd-waitpin_polarity,
+ gd-name, gd-id);
+ return -EBUSY;
+ }
+ } else {
Don't need the else as you are going to return in the above.
Not always, only in case of error
Ok, but seems that it can be simplified a little.
What happens if a device uses more than one wait-pin? In other words a
device with two chip-selects that uses two wait-pins?
+ gd-have_waitpin = true;
+ gd-waitpin = idx;
+ gd-waitpin_polarity = polarity;
+ }
+
+ l = ~GPMC_CONFIG1_WAIT_PIN_SEL_MASK;
+ l |= GPMC_CONFIG1_WAIT_PIN_SEL(idx);
+ gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
+ } else if (polarity) {
+ dev_err(gpmc-dev, error: waitpin polarity specified with out
wait pin number on device %s.%d\n,
+ gd-name, gd-id);
+ return -EINVAL;
Drop this else-if. The above switch statements will report the bad
configuration. This seems a bit redundant.
This is required as switch statements will not report error if polarity
is specified, w/o waitpin to be used.
Ok, may be you can print that above when you detect that there are no
wait-pins selected.
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/39] ARM: OMAP2+: gpmc: driver conversion
Hi Afzal, On 05/07/2012 05:57 AM, Mohammed, Afzal wrote: Hi Jon, On Fri, May 04, 2012 at 21:50:16, Hunter, Jon wrote: As mentioned in the cover letter, Additional features that currently boards in mainline does not make use of like, waitpin interrupt handling, changes to leverage revision 6 IP differences has not been incorporated. Priority in this series is to convert into a driver, get all boards working on mainline. Once all boards are working with gpmc driver, these features which are not required for currently supported boards can be added later. Yes, but I meant why 2 and not say 5? Anyway, I think that this should be marked with a comment like a TODO so it is clear that this needs to be re-visited. Ok, it will be marked with TODO I think that it make more sense to have the wait-pin information part of the gpmc_cs_data structure for the following reasons ... Waitpin information is indeed a part of cs as far as boards are concerned, it is only that it gets propogated to device Why does the device's driver care? From my point of view, the wait-pin is just part of the interface setup/configuration. The external device may require this and assumes that this has been setup along with the CS and interface timing, but the device's driver should not care. Remember this is just a ready signal used to stall the access. Once configured, software should be unaware of it. By device, it is referred to gpmc device which only gpmc driver is aware, the peripheral device's driver is not at all aware. Also, any reason why waitpin_polarity is an int? I see you define LOW as -1, but I not sure why LOW cannot be 0 as 0 is programmed into the register for an active low wait signal. Only intention is not to alter default waitpin polarity value, i.e. if any board does make use of it w/o knowledge of Kernel, I don't want to break it, there may be an argument saying that the board code is buggy, while if some board does so, it is, but don't want to break working one. Here unless user explicitly set the flag, it does nothing on polarity Ok. Do such scenario's exist today? Please note that board code will be removed in the future and device-tree will replace. So if there are no cases today, I would not be concerned. Unless this could be something that has already been configured by the bootloader. However, in that case would be even call this function? Let me check this, here only intent was to play safe, as I have only two boards to test. +int gpmc_cs_reconfigure(char *name, int id, struct gpmc_cs_data *cs) What scenario is this function used in? May be worth adding a comment about the function. Ok, it was required for OneNAND, as it needs to reconfigure Ok, but why is that? Unless this is something generic about one-nand I don't comprehend. I have a high-level understanding of one-nand, but I am not familiar with the specifics that require it to be reconfigured. Not too familiar with OneNAND, from the code it has been understood that to set into sync mode, first it may have to set to async mode, read frequency from OneNAND, then setup sync mode for that frequency. + gpmc_setup_writeprotect(gpmc); Write protect is a pin and there is only one. Like the waitpins and CS signals this needs to be associated with a device. It would make sense that this is associated with the cs data. As far as platform are concerned, it is associated with cs, it is only that while configuring CS, it is propagated such that it is done once. Hmmm ... but here it looks like if write-protect is used you are going to turn it on. A feature like this seems that it does need to be handled by the device's driver. Enabling and disabling write-protect are functions that I would expect are exported to the device's driver and not directly enabled in the gpmc driver during probe. However, maybe is could be valid for the write protect to be enabled by default which could make sense. However, I don't see anywhere else in the driver to control it. Shouldn't we warn on multiple devices trying to use write-protect when parsing their configuration? Even if a single device sets write protect, kernel will log it. That does not seem sufficient. It should be flagged as an error. Write protect is a resource like the CS and waitpins and so I would have thought that it should be reserved in the same way. Jon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/39] ARM: OMAP2+: gpmc: driver conversion
Hi Afzal,
On 05/03/2012 03:23 AM, Mohammed, Afzal wrote:
Hi Jon,
On Tue, May 01, 2012 at 23:23:00, Hunter, Jon wrote:
Hi Afzal,
Looks good! Some minor comments ...
Thanks
+#defineGPMC_WAITPIN_IDX0 0x0
+#defineGPMC_WAITPIN_IDX1 0x1
+#defineGPMC_WAITPIN_IDX2 0x2
+#defineGPMC_WAITPIN_IDX3 0x3
+#defineGPMC_NR_WAITPIN 0x4
How about an enum here? Also OMAP4 only have 3 wait pins and so the
number is device dependent.
Ok
#define GPMC_MEM_START 0x
#define GPMC_MEM_END 0x3FFF
#define BOOT_ROM_SPACE 0x10/* 1MB */
@@ -64,6 +106,55 @@
#define ENABLE_PREFETCH(0x1 7)
#define DMA_MPU_MODE 2
+#defineDRIVER_NAME omap-gpmc
+
+#defineGPMC_NR_IRQ 2
Why has this been changed to 2? It was 6 in the previous version. As we
discussed before the number of IRQs should be detected based upon GPMC
IP version.
As mentioned in the cover letter,
Additional features that currently boards in mainline does not make
use of like, waitpin interrupt handling, changes to leverage revision
6 IP differences has not been incorporated.
Priority in this series is to convert into a driver, get all boards working
on mainline. Once all boards are working with gpmc driver, these features
which are not required for currently supported boards can be added later.
Yes, but I meant why 2 and not say 5? Anyway, I think that this should
be marked with a comment like a TODO so it is clear that this needs to
be re-visited.
+struct gpmc_device {
+ char*name;
+ int id;
+ void*pdata;
+ unsignedpdata_size;
+ struct resource *per_res;
+ unsignedper_res_cnt;
+ struct resource *gpmc_res;
+ unsignedgpmc_res_cnt;
+ boolhave_waitpin;
+ unsignedwaitpin;
+ int waitpin_polarity;
I think that it make more sense to have the wait-pin information part of
the gpmc_cs_data structure for the following reasons ...
Waitpin information is indeed a part of cs as far as boards are concerned,
it is only that it gets propogated to device
Why does the device's driver care? From my point of view, the wait-pin
is just part of the interface setup/configuration. The external device
may require this and assumes that this has been setup along with the CS
and interface timing, but the device's driver should not care. Remember
this is just a ready signal used to stall the access. Once configured,
software should be unaware of it.
1. If a device can use multiple CS, then it could use multiple wait signals.
2. Eventually, all board information needs to move to device tree. By
having it in the pdata it will be easier to migrate to device tree.
It may be nice to have have_waitpin be an integer too, that indicates
if wait is used for both read and write or just one or the other.
Also, any reason why waitpin_polarity is an int? I see you define LOW as
-1, but I not sure why LOW cannot be 0 as 0 is programmed into the
register for an active low wait signal.
Only intention is not to alter default waitpin polarity value, i.e. if
any board does make use of it w/o knowledge of Kernel, I don't want to
break it, there may be an argument saying that the board code is buggy,
while if some board does so, it is, but don't want to break working one.
Here unless user explicitly set the flag, it does nothing on polarity
Ok. Do such scenario's exist today? Please note that board code will be
removed in the future and device-tree will replace. So if there are no
cases today, I would not be concerned. Unless this could be something
that has already been configured by the bootloader. However, in that
case would be even call this function?
+ if (idx != ~0x0) {
+ if (gd-have_waitpin) {
+ if (gd-waitpin != idx ||
+ gd-waitpin_polarity != polarity) {
+ dev_err(gpmc-dev, error: conflict: waitpin %u
with polarity %d on device %s.%d\n,
+ gd-waitpin, gd-waitpin_polarity,
+ gd-name, gd-id);
+ return -EBUSY;
+ }
+ } else {
+ gd-have_waitpin = true;
+ gd-waitpin = idx;
+ gd-waitpin_polarity = polarity;
+ }
I am not sure about the above code. What happens if a device has
multiple CS signals and uses multiple wait signals?
I am also not sure why gpmc_setup_cs_waitpin and gpmc_setup_waitpin
cannot be combined. I think that it would be a fair assumption to make
that anyone using the waitpin does so
Re: [PATCH v4 01/39] ARM: OMAP2+: gpmc: driver conversion
Hi Afzal,
On 05/01/2012 07:19 AM, Afzal Mohammed wrote:
[...]
+static int gpmc_setup_cs_waitpin(struct gpmc *gpmc, struct gpmc_device *gd,
+ unsigned cs, unsigned conf)
+{
+ u32 l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
+ unsigned idx = ~0x0;
+ int polarity = 0;
- l = gpmc_read_reg(GPMC_REVISION);
- printk(KERN_INFO GPMC revision %d.%d\n, (l 4) 0x0f, l 0x0f);
- /* Set smart idle mode and automatic L3 clock gating */
- l = gpmc_read_reg(GPMC_SYSCONFIG);
- l = 0x03 3;
- l |= (0x02 3) | (1 0);
- gpmc_write_reg(GPMC_SYSCONFIG, l);
- gpmc_mem_init();
+ switch (conf GPMC_WAITPIN_MASK) {
+ case GPMC_WAITPIN_0:
+ idx = GPMC_WAITPIN_IDX0;
+ break;
+ case GPMC_WAITPIN_1:
+ idx = GPMC_WAITPIN_IDX1;
+ break;
+ case GPMC_WAITPIN_2:
+ idx = GPMC_WAITPIN_IDX2;
+ break;
+ case GPMC_WAITPIN_3:
+ idx = GPMC_WAITPIN_IDX3;
+ break;
+ /* no waitpin */
+ case 0:
+ break;
+ default:
+ dev_err(gpmc-dev, multiple waitpins selected on CS:%u\n, cs);
+ return -EINVAL;
+ break;
+ }
Why not combined case 0 and default? Both are invalid configurations so
just report invalid selection.
- /* initalize the irq_chained */
- irq = OMAP_GPMC_IRQ_BASE;
- for (cs = 0; cs GPMC_CS_NUM; cs++) {
- irq_set_chip_and_handler(irq, dummy_irq_chip,
- handle_simple_irq);
- set_irq_flags(irq, IRQF_VALID);
- irq++;
+ switch (conf GPMC_WAITPIN_POLARITY_MASK) {
+ case GPMC_WAITPIN_ACTIVE_LOW:
+ polarity = LOW;
+ break;
+ case GPMC_WAITPIN_ACTIVE_HIGH:
+ polarity = HIGH;
+ break;
+ /* no waitpin */
+ case 0:
+ break;
+ default:
+ dev_err(gpmc-dev, waitpin polarity set to low high\n);
+ return -EINVAL;
+ break;
}
Again, combine case 0 and default as these are invalid.
- ret = request_irq(gpmc_irq, gpmc_handle_irq, IRQF_SHARED, gpmc, NULL);
- if (ret)
- pr_err(gpmc: irq-%d could not claim: err %d\n,
- gpmc_irq, ret);
- return ret;
+ if (idx != ~0x0) {
If you combine the above cases, then you can drop the idx test here.
+ if (gd-have_waitpin) {
+ if (gd-waitpin != idx ||
+ gd-waitpin_polarity != polarity) {
+ dev_err(gpmc-dev, error: conflict: waitpin %u
with polarity %d on device %s.%d\n,
+ gd-waitpin, gd-waitpin_polarity,
+ gd-name, gd-id);
+ return -EBUSY;
+ }
+ } else {
Don't need the else as you are going to return in the above.
+ gd-have_waitpin = true;
+ gd-waitpin = idx;
+ gd-waitpin_polarity = polarity;
+ }
+
+ l = ~GPMC_CONFIG1_WAIT_PIN_SEL_MASK;
+ l |= GPMC_CONFIG1_WAIT_PIN_SEL(idx);
+ gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
+ } else if (polarity) {
+ dev_err(gpmc-dev, error: waitpin polarity specified with out
wait pin number on device %s.%d\n,
+ gd-name, gd-id);
+ return -EINVAL;
Drop this else-if. The above switch statements will report the bad
configuration. This seems a bit redundant.
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 01/39] ARM: OMAP2+: gpmc: driver conversion
Hi Jon,
On Tue, May 01, 2012 at 23:23:00, Hunter, Jon wrote:
Hi Afzal,
Looks good! Some minor comments ...
Thanks
+#defineGPMC_WAITPIN_IDX0 0x0
+#defineGPMC_WAITPIN_IDX1 0x1
+#defineGPMC_WAITPIN_IDX2 0x2
+#defineGPMC_WAITPIN_IDX3 0x3
+#defineGPMC_NR_WAITPIN 0x4
How about an enum here? Also OMAP4 only have 3 wait pins and so the
number is device dependent.
Ok
#define GPMC_MEM_START 0x
#define GPMC_MEM_END 0x3FFF
#define BOOT_ROM_SPACE 0x10/* 1MB */
@@ -64,6 +106,55 @@
#define ENABLE_PREFETCH(0x1 7)
#define DMA_MPU_MODE 2
+#defineDRIVER_NAME omap-gpmc
+
+#defineGPMC_NR_IRQ 2
Why has this been changed to 2? It was 6 in the previous version. As we
discussed before the number of IRQs should be detected based upon GPMC
IP version.
As mentioned in the cover letter,
Additional features that currently boards in mainline does not make
use of like, waitpin interrupt handling, changes to leverage revision
6 IP differences has not been incorporated.
Priority in this series is to convert into a driver, get all boards working
on mainline. Once all boards are working with gpmc driver, these features
which are not required for currently supported boards can be added later.
+struct gpmc_device {
+ char*name;
+ int id;
+ void*pdata;
+ unsignedpdata_size;
+ struct resource *per_res;
+ unsignedper_res_cnt;
+ struct resource *gpmc_res;
+ unsignedgpmc_res_cnt;
+ boolhave_waitpin;
+ unsignedwaitpin;
+ int waitpin_polarity;
I think that it make more sense to have the wait-pin information part of
the gpmc_cs_data structure for the following reasons ...
Waitpin information is indeed a part of cs as far as boards are concerned,
it is only that it gets propogated to device
1. If a device can use multiple CS, then it could use multiple wait signals.
2. Eventually, all board information needs to move to device tree. By
having it in the pdata it will be easier to migrate to device tree.
It may be nice to have have_waitpin be an integer too, that indicates
if wait is used for both read and write or just one or the other.
Also, any reason why waitpin_polarity is an int? I see you define LOW as
-1, but I not sure why LOW cannot be 0 as 0 is programmed into the
register for an active low wait signal.
Only intention is not to alter default waitpin polarity value, i.e. if
any board does make use of it w/o knowledge of Kernel, I don't want to
break it, there may be an argument saying that the board code is buggy,
while if some board does so, it is, but don't want to break working one.
Here unless user explicitly set the flag, it does nothing on polarity
+ if (idx != ~0x0) {
+ if (gd-have_waitpin) {
+ if (gd-waitpin != idx ||
+ gd-waitpin_polarity != polarity) {
+ dev_err(gpmc-dev, error: conflict: waitpin %u
with polarity %d on device %s.%d\n,
+ gd-waitpin, gd-waitpin_polarity,
+ gd-name, gd-id);
+ return -EBUSY;
+ }
+ } else {
+ gd-have_waitpin = true;
+ gd-waitpin = idx;
+ gd-waitpin_polarity = polarity;
+ }
I am not sure about the above code. What happens if a device has
multiple CS signals and uses multiple wait signals?
I am also not sure why gpmc_setup_cs_waitpin and gpmc_setup_waitpin
cannot be combined. I think that it would be a fair assumption to make
that anyone using the waitpin does so inconjunction with the CS (I think
that they would have too).
However, if there is a reason for splitting it up this way please
explain why.
Initially it was done per CS, the problem happened while trying to
convert tusb6010. It had multiple CS, but using same waitpin. Problem
with incorporating this onto CS is we have to sacrifice on wait pin
conflict prevention capability. The code now handles case of wait pin
conflict per device, the code can be extended to have multiple
wait pin per device also. But I prefer to defer it to later, not as
part of this series, as this feature what you asking for is not required
for any of the existing boards. I can add it in this series if there is
a strong opinion in the community for the same in this series.
Policy that is being followed here is first sit, then straighten legs, ;)
+static int gpmc_setup_cs_nonres(struct gpmc *gpmc,
+
Re: [PATCH v4 01/39] ARM: OMAP2+: gpmc: driver conversion
Hi Afzal, Looks good! Some minor comments ... On 05/01/2012 07:19 AM, Afzal Mohammed wrote: Convert GPMC code to driver. Boards using GPMC should provide driver with type of configuration, timing, CS. Platform devices would then be created for each connected peripheral (details also to be passed by board so that it reaches respective driver). And GPMC driver would populate memory resource details for the connected peripheral driver. Boards should inform gpmc driver with platform data destined for peripheral driver. gpmc driver will provide the same information to peripheral driver. A peripheral connected to GPMC can have multiple address spaces using different chip select. Hence GPMC driver has been provided capability to create platform device for peripheral using mutiple CS. The peripheral that made it necessary was tusb6010. Interrupts of GPMC are presented to drivers of connected peripherals as resource. A fictitious interrupt controller chip handles these interrupts at GPMC hardware level. Clients can use normal interrupt APIs. Platform information of peripheral passed to GPMC driver should indicate interrupts to be used via flags. Driver is capable of configuring waitpin, waitpin details has to be provided per CS. Wait pin has been considered as exclusive resource as multiple peripherals should not using the same pin, at the same it is valid for mutiple CS to use same waitpin provided they are a part of single peripheral (eg. tusb6010) An exported symbol for reconfiguring GPMC settings has been provided. OneNAND is the one that neccessitated this. Acquiring CS# for NAND is done on a few boards. It means, depending on bootloader to embed this information. Probably CS# being used can be set in the Kernel, and acquiring it can be removed. If ever this capbility is needed, GPMC driver has to be made aware of handling it. Modifications has been made keeping in mind that the driver would have to move to driver folder. This explains requirement of clk_prd field; even though clk_prd variable is not necessary as gpmc_get_fclk_period is present in the same file as of now, this will help in moving the driver easily to drivers folder. Code related to GPMC clock may have to continue live in platform folders as input clock is beyond the control of GPMC and calculating timing for the peripheral may need other helpers. This explains presence of 'gpmc_cs_calc_divider' along with 'gpmc_calc_divider', both doing same work, latter meant to go with driver, former for calculation in platform code. Thanks to Vaibhav Hiremath Jonathan Hunter on their various good suggestions which resulted in improving the code. Cc: Vaibhav Hiremath hvaib...@ti.com Cc: Jon Hunter jon-hun...@ti.com Signed-off-by: Afzal Mohammed af...@ti.com --- arch/arm/mach-omap2/gpmc.c | 877 arch/arm/plat-omap/include/plat/gpmc.h | 93 +++- 2 files changed, 872 insertions(+), 98 deletions(-) diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c index 580e684..12916f3 100644 --- a/arch/arm/mach-omap2/gpmc.c +++ b/arch/arm/mach-omap2/gpmc.c @@ -14,8 +14,11 @@ */ #undef DEBUG +#include linux/platform_device.h + #include linux/irq.h #include linux/kernel.h +#include linux/slab.h #include linux/init.h #include linux/err.h #include linux/clk.h @@ -53,6 +56,45 @@ #define GPMC_CS0_OFFSET 0x60 #define GPMC_CS_SIZE 0x30 +/* GPMC register bits */ +#define GPMC_CONFIG1_TIMEPARAGRANULARITYBIT(4) +#define GPMC_CONFIG1_DEVICETYPE_NAND GPMC_CONFIG1_DEVICETYPE(0x2) +#define GPMC_CONFIG1_WAIT_PIN_SEL_MASK GPMC_CONFIG1_WAIT_PIN_SEL(0x3) +#define GPMC_CONFIG1_WAIT_MON_TIME(val) ((val 0x3) 18) +#define GPMC_CONFIG1_WRITEMULTIPLE BIT(28) +#define GPMC_CONFIG1_READMULTIPLE BIT(30) +#define GPMC_CONFIG1_WRAPBURST BIT(31) +#define GPMC_CONFIG_WAITPIN_POLARITY_SHIFT 0x8 +#define GPMC_CONFIG1_WAITPIN_MONITOR_TIME(val) ((val 0x3) 18) +#define GPMC_CONFIG1_WAITPIN_MONITOR_TIME_1 \ + GPMC_CONFIG1_WAITPIN_MONITOR_TIME(0x1) +#define GPMC_CONFIG1_WAITPIN_MONITOR_TIME_2 \ + GPMC_CONFIG1_WAITPIN_MONITOR_TIME(0x2) +#define GPMC_CONFIG1_CLOCKACTIVATION_TIME(val) ((val 0x3) 25) +#define GPMC_CONFIG1_CLOCKACTIVATION_TIME_1 \ + GPMC_CONFIG1_CLOCKACTIVATION_TIME(0x1) +#define GPMC_CONFIG1_CLOCKACTIVATION_TIME_2 \ + GPMC_CONFIG1_CLOCKACTIVATION_TIME(0x2) + +#define GPMC_CONFIG2_CSEXTRADELAY BIT(7) + +#define GPMC_CONFIG3_ADVEXTRADELAY BIT(7) + +#define GPMC_CONFIG4_OEEXTRADELAY BIT(7) +#define GPMC_CONFIG4_WEEXTRADELAY
