Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.
On 30 July 2015 at 13:24, Marek Vasut wrote: > On Monday, July 27, 2015 at 10:43:05 PM, Michal Suchanek wrote: >> On 27 July 2015 at 19:43, Marek Vasut wrote: >> > On Monday, July 27, 2015 at 11:46:25 AM, Michal Suchanek wrote: >> >> On 24 July 2015 at 10:34, Marek Vasut wrote: >> >> > On Thursday, July 23, 2015 at 07:03:47 PM, Michal Suchanek wrote: >> >> Ok, so here is some summary. >> >> >> >> I have a NOR flash attached to a s3c64xx SPI controller with 64byte >> >> fifo and a pl330 dma controller. >> >> >> >> Normally DMA controller is used for transfers that do not fit into the >> >> FIFO. >> >> >> >> I tried adding the flash memory ID to the spi-nor driver table and >> >> adding a DT node for it. >> >> >> >> The flash is rated at 120MHz so I used that speed but the ID was >> >> bit-shifted and identification failed. There is DT property >> >> samsung,spi-feedback-delay for addressing this and at 120MHz it must >> >> be 2 or 3 on this board. 40MHz works with default value 0. >> >> >> >> The next step after identification worked was to try reading the flash >> >> content. For this the DMA controller is used because data is >> >> transferred in blocks larger than 64 bytes. When reading the whole 4MB >> >> flash the transfer failed silently. I got a 4MB file of all ones or >> >> all zeroes. >> >> >> >> It turns out that >> >> >> >> - the pl330 locks up when transfering large amount of data. >> >> >> >> Specifically, the maximum power of 2 sized transfer at 120MHz is 128 >> >> bytes and 64k at 40MHz. Transferring more than this results in the >> >> pl330 locking up and never signalling completion of the transfer. >> > >> > Hypothesis: >> > I think this might just be that the controller didn't catch all the >> > inbound clock ticks and thus counted less inbound data than it was >> > set up to receive. The controller thus waits for more data. >> >> The flash chip can transfer data as long as you keep the clock going, >> especially when you transfer 64k off a 4M flash. >> >> The read command is bound just by stopping the clock. There is always >> more data to be had. > > Sure, if you keep clocking the chip, the data will keep going. Is the > chip being clocked ? There is always data. Like in whenever you want to read SPI data you sample the input pin and watever you sample is data so far as SPI bus is concerned. > > Doesn't the PL330 have some kind of a counter register where you can check > how much data were received so far ? That should be sufficient to verify > this hypothesis of mine. Could check that, yes. Maybe I could read the counter in a function which dmaengine client uses to tear down the dma transfer. On a related note I enabled more prints on the spidev test program. I have code that tests maximum transfer size up to the 4k spidev limit and it can transfer full 4k buffer of NOPs at 80MHz. The recieved data is all ones except a 00 at the start. So it looks like read-only transfers fail but full-duplex sort of work. And it reproduces the 00 prefix that was seen in the dma-only setup in otherwise working setup :-S An attempt to read a page of data using the fast-read command fails with timeout. > >> >> Data >> >> is left in FIFO which causes subsequent commands to fail since garbage >> >> is returned instead of command reply. Also subsequent read may cause >> >> I/O error or or return an empty page depending on the garbage around. >> > >> > So the driver for the DMA controller might need to be augmented to handle >> > this case -- add a timeout and in case DMA times out, drain the FIFO to >> > make sure subsequent transfers do not fail. >> >> There is no way to add timeout in the DMA driver since it does not >> know the SPI clock. >> >> The timeout is handled by the SPI driver and it should be possible to >> augment it to drain the FIFO when DMA fails so long as FIFO level is >> readable somewhere. > > If the DMA doesn't complete in certain amount of time, it should time out > I'd say. Don't you think ? No. The DMA driver has no idea if the transfer is at 133MHz, 40MHz, 1MHz, 1kHz, whatever. On the other hand, adding a flush_fifo in the SPI driver after DMA failure allows probing the chip reliably by realoding the driver even just after a failed transfer. > >> >> - the I/O errors are not checked in spi-nor at all which
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On 29 July 2015 at 20:40, Mark Brown wrote: > On Wed, Jul 29, 2015 at 08:21:34PM +0200, Michal Suchanek wrote: >> On 29 July 2015 at 19:16, Mark Brown wrote: > >> >> It will not break anything. It will just spam dmesg. > >> > I'm confused - if all this change does is to spam dmesg then what's the >> > point? > >> Presumably when your SPI NOR flash fails to probe this message will be >> just above and you will look into the binding doc and add the >> compatible. > > If you're looking to add a warning message when the flash device fails > to probe then add that in the flash driver when it detects an error, > this will cause needless noise for everyone else using this controller > purely to work around the broken binding. Technically nobody needs to see the warning with in-tree boards since the dts can be amended with the compatible. There is no error in flash device driver. There is only error parsing partition scheme. In my opinion this should never cause an error. With disk drives failure to parse partition table results in unpartitioned disk. As there are number of partitioning schemes failure to parse one still does not prevent other in succeding. > > And like I say compatible really seems like it isn't an appropriate > property here. So to sum up the discussion adding compatible to s3c64xx controller-data is not desirable and making ofpart more robust is desirable. I think the suggestion to use a subnode for ofpart gives the most robust solution. Even adding compatibles to the partition subnodes ofpart still monopolizes the address and cells property of the mtd node which does not pass the 'if another driver did the same would it work together?' test. So my suggestion is to 1) search of ofpart subnode in mtd node. If present read address and reg from it and search partitions as subnodes of the ofpart node. In this case unknown nodes can cause error. 2) failing that issue a warning and try to parse ofpart partitions from the mtd node itself. In this case unknown nodes cannot cause error for compatibility with other drivers including the already exisitn s3c64xx controller-data node. The parser code can be the same for both cases and only operate on different node with a flag to reject unknown subnodes or not. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On 29 July 2015 at 19:16, Mark Brown wrote: > On Wed, Jul 29, 2015 at 06:19:24PM +0200, Michal Suchanek wrote: >> On 29 July 2015 at 16:00, Mark Brown wrote: > >> > I can't tell from this commit message what the issue you're trying to >> > fix is, sorry. Nodes without compatible strings are entirely normal and >> > don't need compatible strings. It sounds like a bug in whatever other >> > driver is becoming confused. > >> The driver that gets confused is ofpart. > >> The two-line patch to allow it to just ignore controller-data has been >> rejected on the basis that s3c64xx should use a compatible string >> because ofpart monopolizes all nodes without compatible which are >> children of a mtd device. Devicetrees containing such nodes that are >> not partitions are presumably invalid and should be rejected when >> ofpart is compiled into the kernel. > > That seems like an extremely limited binding, the normal thing here > would be to create a specifically named node to contain the collection > of subnodes like many PMICs do for their regulators. As a fix I'd > suggest just silently ignoring nodes it can't understand, or printing a > warning if that's a serious issue. > >> >> + if (!of_get_property(data_np, "compatible", NULL) || >> >> + strcmp(of_get_property(data_np, "compatible", NULL), >> >> +"samsung,s3c-controller-data")) >> >> + dev_err(&spi->dev, "child node 'controller-data' does not >> >> have correct compatible\n"); > >> > This will break all existing users which is not acceptable for >> > mainline, we need to preserve compatibility with existing device trees. > >> It will not break anything. It will just spam dmesg. > > I'm confused - if all this change does is to spam dmesg then what's the > point? Presumably when your SPI NOR flash fails to probe this message will be just above and you will look into the binding doc and add the compatible. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
On 29 July 2015 at 16:00, Mark Brown wrote: > On Wed, Jul 29, 2015 at 12:19:57PM +0200, Michal Suchanek wrote: > > Please use subject lines matching the style for the subsytsem so people > can spot that the patch is in some way relevant. > >> The controller-data subnode has no compatible. This can lead to other >> drivers getting confused by it. Add a compatible to make devicetreee >> unambiguous. > > I can't tell from this commit message what the issue you're trying to > fix is, sorry. Nodes without compatible strings are entirely normal and > don't need compatible strings. It sounds like a bug in whatever other > driver is becoming confused. The driver that gets confused is ofpart. The two-line patch to allow it to just ignore controller-data has been rejected on the basis that s3c64xx should use a compatible string because ofpart monopolizes all nodes without compatible which are children of a mtd device. Devicetrees containing such nodes that are not partitions are presumably invalid and should be rejected when ofpart is compiled into the kernel. > >> + if (!of_get_property(data_np, "compatible", NULL) || >> + strcmp(of_get_property(data_np, "compatible", NULL), >> +"samsung,s3c-controller-data")) >> + dev_err(&spi->dev, "child node 'controller-data' does not have >> correct compatible\n"); > > This will break all existing users which is not acceptable for > mainline, we need to preserve compatibility with existing device trees. It will not break anything. It will just spam dmesg. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.
On 27 July 2015 at 19:43, Marek Vasut wrote: > On Monday, July 27, 2015 at 11:46:25 AM, Michal Suchanek wrote: >> On 24 July 2015 at 10:34, Marek Vasut wrote: >> > On Thursday, July 23, 2015 at 07:03:47 PM, Michal Suchanek wrote: >> > >> Ok, so here is some summary. >> >> I have a NOR flash attached to a s3c64xx SPI controller with 64byte >> fifo and a pl330 dma controller. >> >> Normally DMA controller is used for transfers that do not fit into the >> FIFO. >> >> I tried adding the flash memory ID to the spi-nor driver table and >> adding a DT node for it. >> >> The flash is rated at 120MHz so I used that speed but the ID was >> bit-shifted and identification failed. There is DT property >> samsung,spi-feedback-delay for addressing this and at 120MHz it must >> be 2 or 3 on this board. 40MHz works with default value 0. >> >> The next step after identification worked was to try reading the flash >> content. For this the DMA controller is used because data is >> transferred in blocks larger than 64 bytes. When reading the whole 4MB >> flash the transfer failed silently. I got a 4MB file of all ones or >> all zeroes. >> >> It turns out that >> >> - the pl330 locks up when transfering large amount of data. >> Specifically, the maximum power of 2 sized transfer at 120MHz is 128 >> bytes and 64k at 40MHz. Transferring more than this results in the >> pl330 locking up and never signalling completion of the transfer. > > Hypothesis: > I think this might just be that the controller didn't catch all the > inbound clock ticks and thus counted less inbound data than it was > set up to receive. The controller thus waits for more data. The flash chip can transfer data as long as you keep the clock going, especially when you transfer 64k off a 4M flash. The read command is bound just by stopping the clock. There is always more data to be had. > >> Data >> is left in FIFO which causes subsequent commands to fail since garbage >> is returned instead of command reply. Also subsequent read may cause >> I/O error or or return an empty page depending on the garbage around. > > So the driver for the DMA controller might need to be augmented to handle > this case -- add a timeout and in case DMA times out, drain the FIFO to > make sure subsequent transfers do not fail. There is no way to add timeout in the DMA driver since it does not know the SPI clock. The timeout is handled by the SPI driver and it should be possible to augment it to drain the FIFO when DMA fails so long as FIFO level is readable somewhere. > >> - the I/O errors are not checked in spi-nor at all which leads to >> silent data corruption. >> >> On a suggestion that this may improve reliability I changed the >> s3c64xx driver to use DMA for all transfers. This caused >> identification to fail in spin-nor because the ID was prefixed with >> extra 00 byte. Testing with spidev confirmed that everything is >> prefixed with extra 00. > > The determinism of this is in fact excellent news. > >> Also when the DMA controller locked up no >> transfers were possible anymore. When DMA was not used for sending >> commands the controller would recover on next command. I made the >> option to always use DMA configurable and it turns out that the >> returned data is prefixed with 00 only when no transfer without DMA >> was ever made. Loading the spi-nor driver with the dma-only option off >> and then with dma-only option on results in correct operation. Only >> loading the dma-only driver first causes the 00 prefix. > > Can we conclude that the PIO codepath somehow programs a register somewhere > which gets rid of this 0x00 prefix ? If so, this should then also be part > of the DMA codepath, or even better, this should be set in probe() somewhere. Yes, it looks like it. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.
On 24 July 2015 at 10:34, Marek Vasut wrote: > On Thursday, July 23, 2015 at 07:03:47 PM, Michal Suchanek wrote: > > Hi! > > [...] > >> >>> It's probably slower to set up DMA for 2-byte commands but it might >> >>> work nonetheless. >> >> >> >> It is, the overhead will be considerable. It might help the stability >> >> though. I'm really looking forward to the results! >> > >> > Hello, >> > >> > this does not quite work. >> > >> > My test with spidev: >> > >> > # ./spinor /dev/spidev1.0 >> > Sending 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> > Received 00 ff c8 60 16 c8 60 16 c8 60 16 c8 60 16 c8 60 >> > Sending 90 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> > Received 00 ff ff ff ff c8 15 c8 15 c8 15 c8 15 c8 15 c8 >> > Sending 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> > Received 00 ff c8 60 16 c8 60 16 c8 60 16 c8 60 16 c8 60 >> > >> > I receive correct ID but spi-nor complains it does not know ID 00 c8 60. >> > IIRC garbage should be sent only at the time command is transferred so >> > only one byte of garbage should be received. Also the garbage tends to >> > be the last state of the data output - all 0 or all 1. >> > So it seems using DMA for all transfers including 1-byte commands >> > results in (some?) received data getting an extra 00 prefix. >> > >> > >> > I also managed to lock up the controller completely since there is >> > some error passing the SPI speed somewhere :( >> > >> > [ 1352.977530] spidev spi1.0: setup mode 0, 8 bits/w, 8000 Hz max --> >> > 0 [ 1352.977540] spidev spi1.0: spi mode 0 >> > [ 1352.977576] spidev spi1.0: setup mode 0, 8 bits/w, 8000 Hz max --> >> > 0 [ 1352.977582] spidev spi1.0: msb first >> > [ 1352.977614] spidev spi1.0: setup mode 0, 8 bits/w, 8000 Hz max --> >> > 0 [ 1352.977620] spidev spi1.0: 0 bits per word >> > [ 1352.977652] spidev spi1.0: setup mode 0, 8 bits/w, 2690588672 Hz max >> > --> 0 [ 1352.977726] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1 >> > src_clk sclk_spi1 mode bpw 8 >> > [ 1352.977753] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: xfer >> > bpw 8 speed -1604378624 >> > [ 1352.977760] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1 >> > src_clk sclk_spi1 mode bpw 8 >> > [ 1352.977781] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: using >> > dma [ 1352.977797] dma-pl330 121b.pdma: setting up request on thread >> > 1 >> >> Hmm, on a second thought it probably works as expected more or less. >> >> The nonsensical value was passed from application and there is no >> guard against that. >> >> Since I don't do PIO the controller remains locked up indefinitely. > > I have to admit, I don't quite understand the above. I also don't quite know > what your spidev test does. Can you possibly just bind a regular SPI NOR > driver > and run mtdtests to see if it is stable ? Ok, so here is some summary. I have a NOR flash attached to a s3c64xx SPI controller with 64byte fifo and a pl330 dma controller. Normally DMA controller is used for transfers that do not fit into the FIFO. I tried adding the flash memory ID to the spi-nor driver table and adding a DT node for it. The flash is rated at 120MHz so I used that speed but the ID was bit-shifted and identification failed. There is DT property samsung,spi-feedback-delay for addressing this and at 120MHz it must be 2 or 3 on this board. 40MHz works with default value 0. The next step after identification worked was to try reading the flash content. For this the DMA controller is used because data is transferred in blocks larger than 64 bytes. When reading the whole 4MB flash the transfer failed silently. I got a 4MB file of all ones or all zeroes. It turns out that - the pl330 locks up when transfering large amount of data. Specifically, the maximum power of 2 sized transfer at 120MHz is 128 bytes and 64k at 40MHz. Transferring more than this results in the pl330 locking up and never signalling completion of the transfer. Data is left in FIFO which causes subsequent commands to fail since garbage is returned instead of command reply. Also subsequent read may cause I/O error or or return an empty page depending on the garbage around. - the I/O errors are not checked in spi-nor at all which leads to silent data corruption. On a suggestion that this may improve reliability I changed the s3c64xx driver to use DMA for all transfers. This caused identification to fail in spin-nor because the ID was pref
Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.
On 24 July 2015 at 10:34, Marek Vasut wrote: > On Thursday, July 23, 2015 at 07:03:47 PM, Michal Suchanek wrote: > > Hi! > > [...] > >> >>> It's probably slower to set up DMA for 2-byte commands but it might >> >>> work nonetheless. >> >> >> >> It is, the overhead will be considerable. It might help the stability >> >> though. I'm really looking forward to the results! >> > >> > Hello, >> > >> > this does not quite work. >> > >> > My test with spidev: >> > >> > # ./spinor /dev/spidev1.0 >> > Sending 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> > Received 00 ff c8 60 16 c8 60 16 c8 60 16 c8 60 16 c8 60 >> > Sending 90 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> > Received 00 ff ff ff ff c8 15 c8 15 c8 15 c8 15 c8 15 c8 >> > Sending 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> > Received 00 ff c8 60 16 c8 60 16 c8 60 16 c8 60 16 c8 60 >> > >> > I receive correct ID but spi-nor complains it does not know ID 00 c8 60. >> > IIRC garbage should be sent only at the time command is transferred so >> > only one byte of garbage should be received. Also the garbage tends to >> > be the last state of the data output - all 0 or all 1. >> > So it seems using DMA for all transfers including 1-byte commands >> > results in (some?) received data getting an extra 00 prefix. >> > >> > >> > I also managed to lock up the controller completely since there is >> > some error passing the SPI speed somewhere :( >> > >> > [ 1352.977530] spidev spi1.0: setup mode 0, 8 bits/w, 8000 Hz max --> >> > 0 [ 1352.977540] spidev spi1.0: spi mode 0 >> > [ 1352.977576] spidev spi1.0: setup mode 0, 8 bits/w, 8000 Hz max --> >> > 0 [ 1352.977582] spidev spi1.0: msb first >> > [ 1352.977614] spidev spi1.0: setup mode 0, 8 bits/w, 8000 Hz max --> >> > 0 [ 1352.977620] spidev spi1.0: 0 bits per word >> > [ 1352.977652] spidev spi1.0: setup mode 0, 8 bits/w, 2690588672 Hz max >> > --> 0 [ 1352.977726] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1 >> > src_clk sclk_spi1 mode bpw 8 >> > [ 1352.977753] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: xfer >> > bpw 8 speed -1604378624 >> > [ 1352.977760] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1 >> > src_clk sclk_spi1 mode bpw 8 >> > [ 1352.977781] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: using >> > dma [ 1352.977797] dma-pl330 121b.pdma: setting up request on thread >> > 1 >> >> Hmm, on a second thought it probably works as expected more or less. >> >> The nonsensical value was passed from application and there is no >> guard against that. >> >> Since I don't do PIO the controller remains locked up indefinitely. > > I have to admit, I don't quite understand the above. I also don't quite know > what your spidev test does. It does a full duplex transfer sending what is printed and printing what is received. > Can you possibly just bind a regular SPI NOR driver > and run mtdtests to see if it is stable ? I can if I use PIO for short transfers. Using DMA for all transfers results in the received data prefixed with 00 so the NOR flash identification fails. Admittedly I have no idea what the flash memory actually contains so if all DMA reads were always prefixed with 00 I could not tell. I vaguely recall reading the whole content and parsing the I can probably make the minimum length for DMA configurable so I can fall back to PIO when the controler locks up. It seems setting up a PIO transfer makes it work again. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.
On 23 July 2015 at 18:46, Michal Suchanek wrote: > On 22 July 2015 at 11:01, Marek Vasut wrote: >> On Wednesday, July 22, 2015 at 10:38:14 AM, Michal Suchanek wrote: >>> On 22 July 2015 at 10:24, Marek Vasut wrote: >>> > On Wednesday, July 22, 2015 at 10:18:04 AM, Michal Suchanek wrote: >>> >> On 22 July 2015 at 09:58, Marek Vasut wrote: >>> >> > On Wednesday, July 22, 2015 at 09:45:27 AM, Michal Suchanek wrote: >>> >> >> On 22 July 2015 at 09:33, Marek Vasut wrote: >>> >> >> > On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote: >>> >> >> >> On 22 July 2015 at 06:49, Vinod Koul wrote: >>> >> >> >> > On Tue, Jul 21, 2015 at 10:14:11AM +0200, Michal Suchanek wrote: >>> >> >> >> >> > Or alternatively we could publish the limitations of the >>> >> >> >> >> > channel using capabilities so SPI knows I have a dmaengine >>> >> >> >> >> > channel and it can transfer max N length transfers so would >>> >> >> >> >> > be able to break rather than guessing it or coding in DT. >>> >> >> >> >> > Yes it may come from DT but that should be dmaengine driver >>> >> >> >> >> > rather than client driver :) >>> >> >> >> >> > >>> >> >> >> >> > This can be done by dma_get_slave_caps(chan, &caps) >>> >> >> >> >> > >>> >> >> >> >> > And we add max_length as one more parameter to existing set >>> >> >> >> >> > >>> >> >> >> >> > Also all this could be handled in generic SPI-dmaengine layer >>> >> >> >> >> > so that individual drivers don't have to code it in >>> >> >> >> >> > >>> >> >> >> >> > Let me know if this idea is okay, I can push the dmaengine >>> >> >> >> >> > bits... >>> >> >> >> >> >>> >> >> >> >> It would be ok if there was a fixed limit. However, the limit >>> >> >> >> >> depends on SPI slave settings. Presumably for other buses using >>> >> >> >> >> the dmaengine the limit would depend on the bus or slave >>> >> >> >> >> settings as well. I do not see a sane way of passing this all >>> >> >> >> >> the way to the dmaengine driver. >>> >> >> >> > >>> >> >> >> > I don't see why this should be client (SPI) dependent. The max >>> >> >> >> > length supported is a dmaengine constraint, typically flowing >>> >> >> >> > from max blocks/length it can transfer. Know this limit can >>> >> >> >> > allow clients to split transfers. >>> >> >> >> >>> >> >> >> In practice on the board I have the maximum transfer length before >>> >> >> >> it fails depends on SPI bus speed which is set up per slave. I >>> >> >> >> did not try searching the space of possible settings thorougly >>> >> >> >> and settled for a setting that gives reasonable speed and >>> >> >> >> transfer length. >>> >> >> > >>> >> >> > This looks more like a signal integrity issue though. >>> >> >> >>> >> >> It certainly does on the surface. However, when wrong data is >>> >> >> delivered over the SPI bus (such as when I use wrong phase setting) >>> >> >> the SPI controller happily delivers wrong data over PIO. >>> >> >> >>> >> >> The failure I am seeing is that the pl330 DMA program which >>> >> >> repeatedly waits for data from the SPI controller never finishes the >>> >> >> read loop and does not signal the interrupt. It seems it also leaves >>> >> >> some data in a FIFO somewhere so next command on the flash returns >>> >> >> garbage and fails. >>> >> > >>> >> > I observed something similar on MXS (mx28) SPI block. Do you use mixed >>> >> > PIO/DMA mode perhaps ? >>> >> >>> >&
Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.
On 22 July 2015 at 11:01, Marek Vasut wrote: > On Wednesday, July 22, 2015 at 10:38:14 AM, Michal Suchanek wrote: >> On 22 July 2015 at 10:24, Marek Vasut wrote: >> > On Wednesday, July 22, 2015 at 10:18:04 AM, Michal Suchanek wrote: >> >> On 22 July 2015 at 09:58, Marek Vasut wrote: >> >> > On Wednesday, July 22, 2015 at 09:45:27 AM, Michal Suchanek wrote: >> >> >> On 22 July 2015 at 09:33, Marek Vasut wrote: >> >> >> > On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote: >> >> >> >> On 22 July 2015 at 06:49, Vinod Koul wrote: >> >> >> >> > On Tue, Jul 21, 2015 at 10:14:11AM +0200, Michal Suchanek wrote: >> >> >> >> >> > Or alternatively we could publish the limitations of the >> >> >> >> >> > channel using capabilities so SPI knows I have a dmaengine >> >> >> >> >> > channel and it can transfer max N length transfers so would >> >> >> >> >> > be able to break rather than guessing it or coding in DT. >> >> >> >> >> > Yes it may come from DT but that should be dmaengine driver >> >> >> >> >> > rather than client driver :) >> >> >> >> >> > >> >> >> >> >> > This can be done by dma_get_slave_caps(chan, &caps) >> >> >> >> >> > >> >> >> >> >> > And we add max_length as one more parameter to existing set >> >> >> >> >> > >> >> >> >> >> > Also all this could be handled in generic SPI-dmaengine layer >> >> >> >> >> > so that individual drivers don't have to code it in >> >> >> >> >> > >> >> >> >> >> > Let me know if this idea is okay, I can push the dmaengine >> >> >> >> >> > bits... >> >> >> >> >> >> >> >> >> >> It would be ok if there was a fixed limit. However, the limit >> >> >> >> >> depends on SPI slave settings. Presumably for other buses using >> >> >> >> >> the dmaengine the limit would depend on the bus or slave >> >> >> >> >> settings as well. I do not see a sane way of passing this all >> >> >> >> >> the way to the dmaengine driver. >> >> >> >> > >> >> >> >> > I don't see why this should be client (SPI) dependent. The max >> >> >> >> > length supported is a dmaengine constraint, typically flowing >> >> >> >> > from max blocks/length it can transfer. Know this limit can >> >> >> >> > allow clients to split transfers. >> >> >> >> >> >> >> >> In practice on the board I have the maximum transfer length before >> >> >> >> it fails depends on SPI bus speed which is set up per slave. I >> >> >> >> did not try searching the space of possible settings thorougly >> >> >> >> and settled for a setting that gives reasonable speed and >> >> >> >> transfer length. >> >> >> > >> >> >> > This looks more like a signal integrity issue though. >> >> >> >> >> >> It certainly does on the surface. However, when wrong data is >> >> >> delivered over the SPI bus (such as when I use wrong phase setting) >> >> >> the SPI controller happily delivers wrong data over PIO. >> >> >> >> >> >> The failure I am seeing is that the pl330 DMA program which >> >> >> repeatedly waits for data from the SPI controller never finishes the >> >> >> read loop and does not signal the interrupt. It seems it also leaves >> >> >> some data in a FIFO somewhere so next command on the flash returns >> >> >> garbage and fails. >> >> > >> >> > I observed something similar on MXS (mx28) SPI block. Do you use mixed >> >> > PIO/DMA mode perhaps ? >> >> >> >> The SPI driver uses PIO for short transfers and DMA for transfers >> >> longer than the controller FIFO. This seems to be the standard way to >> >> do things.It works flawlessly so long as submitting overly long DMA >> >> programs is avoi
Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.
On 22 July 2015 at 10:24, Marek Vasut wrote: > On Wednesday, July 22, 2015 at 10:18:04 AM, Michal Suchanek wrote: >> On 22 July 2015 at 09:58, Marek Vasut wrote: >> > On Wednesday, July 22, 2015 at 09:45:27 AM, Michal Suchanek wrote: >> >> On 22 July 2015 at 09:33, Marek Vasut wrote: >> >> > On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote: >> >> >> On 22 July 2015 at 06:49, Vinod Koul wrote: >> >> >> > On Tue, Jul 21, 2015 at 10:14:11AM +0200, Michal Suchanek wrote: >> >> >> >> > Or alternatively we could publish the limitations of the channel >> >> >> >> > using capabilities so SPI knows I have a dmaengine channel and >> >> >> >> > it can transfer max N length transfers so would be able to >> >> >> >> > break rather than guessing it or coding in DT. Yes it may come >> >> >> >> > from DT but that should be dmaengine driver rather than client >> >> >> >> > driver :) >> >> >> >> > >> >> >> >> > This can be done by dma_get_slave_caps(chan, &caps) >> >> >> >> > >> >> >> >> > And we add max_length as one more parameter to existing set >> >> >> >> > >> >> >> >> > Also all this could be handled in generic SPI-dmaengine layer so >> >> >> >> > that individual drivers don't have to code it in >> >> >> >> > >> >> >> >> > Let me know if this idea is okay, I can push the dmaengine >> >> >> >> > bits... >> >> >> >> >> >> >> >> It would be ok if there was a fixed limit. However, the limit >> >> >> >> depends on SPI slave settings. Presumably for other buses using >> >> >> >> the dmaengine the limit would depend on the bus or slave settings >> >> >> >> as well. I do not see a sane way of passing this all the way to >> >> >> >> the dmaengine driver. >> >> >> > >> >> >> > I don't see why this should be client (SPI) dependent. The max >> >> >> > length supported is a dmaengine constraint, typically flowing from >> >> >> > max blocks/length it can transfer. Know this limit can allow >> >> >> > clients to split transfers. >> >> >> >> >> >> In practice on the board I have the maximum transfer length before it >> >> >> fails depends on SPI bus speed which is set up per slave. I did not >> >> >> try searching the space of possible settings thorougly and settled >> >> >> for a setting that gives reasonable speed and transfer length. >> >> > >> >> > This looks more like a signal integrity issue though. >> >> >> >> It certainly does on the surface. However, when wrong data is >> >> delivered over the SPI bus (such as when I use wrong phase setting) >> >> the SPI controller happily delivers wrong data over PIO. >> >> >> >> The failure I am seeing is that the pl330 DMA program which repeatedly >> >> waits for data from the SPI controller never finishes the read loop >> >> and does not signal the interrupt. It seems it also leaves some data >> >> in a FIFO somewhere so next command on the flash returns garbage and >> >> fails. >> > >> > I observed something similar on MXS (mx28) SPI block. Do you use mixed >> > PIO/DMA mode perhaps ? >> >> The SPI driver uses PIO for short transfers and DMA for transfers >> longer than the controller FIFO. This seems to be the standard way to >> do things.It works flawlessly so long as submitting overly long DMA >> programs is avoided. > > Can you try doing JUST DMA, no PIO ? I remember seeing some bus > synchronisation > issues when I did mixed PIO/DMA on the MXS and it was nasty to track down. > Just > give pure DMA a go to see if the thing stabilizes somehow. It's probably slower to set up DMA for 2-byte commands but it might work nonetheless. I will give it a go. > >> > Do you have the option to connect a bus analyzer? >> > I can probably offer you some tools, I'm in Prague ... >> >> The flash chip is accessible when removing the bottom cover. It is >> VSOP8 package slightly lower than SOP8 so attaching clips to it might >> be a bit proble
Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.
On 22 July 2015 at 09:58, Marek Vasut wrote: > On Wednesday, July 22, 2015 at 09:45:27 AM, Michal Suchanek wrote: >> On 22 July 2015 at 09:33, Marek Vasut wrote: >> > On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote: >> >> On 22 July 2015 at 06:49, Vinod Koul wrote: >> >> > On Tue, Jul 21, 2015 at 10:14:11AM +0200, Michal Suchanek wrote: >> >> >> > Or alternatively we could publish the limitations of the channel >> >> >> > using capabilities so SPI knows I have a dmaengine channel and it >> >> >> > can transfer max N length transfers so would be able to break >> >> >> > rather than guessing it or coding in DT. Yes it may come from DT >> >> >> > but that should be dmaengine driver rather than client driver :) >> >> >> > >> >> >> > This can be done by dma_get_slave_caps(chan, &caps) >> >> >> > >> >> >> > And we add max_length as one more parameter to existing set >> >> >> > >> >> >> > Also all this could be handled in generic SPI-dmaengine layer so >> >> >> > that individual drivers don't have to code it in >> >> >> > >> >> >> > Let me know if this idea is okay, I can push the dmaengine bits... >> >> >> >> >> >> It would be ok if there was a fixed limit. However, the limit depends >> >> >> on SPI slave settings. Presumably for other buses using the dmaengine >> >> >> the limit would depend on the bus or slave settings as well. I do not >> >> >> see a sane way of passing this all the way to the dmaengine driver. >> >> > >> >> > I don't see why this should be client (SPI) dependent. The max length >> >> > supported is a dmaengine constraint, typically flowing from max >> >> > blocks/length it can transfer. Know this limit can allow clients to >> >> > split transfers. >> >> >> >> In practice on the board I have the maximum transfer length before it >> >> fails depends on SPI bus speed which is set up per slave. I did not >> >> try searching the space of possible settings thorougly and settled for >> >> a setting that gives reasonable speed and transfer length. >> > >> > This looks more like a signal integrity issue though. >> >> It certainly does on the surface. However, when wrong data is >> delivered over the SPI bus (such as when I use wrong phase setting) >> the SPI controller happily delivers wrong data over PIO. >> >> The failure I am seeing is that the pl330 DMA program which repeatedly >> waits for data from the SPI controller never finishes the read loop >> and does not signal the interrupt. It seems it also leaves some data >> in a FIFO somewhere so next command on the flash returns garbage and >> fails. > > I observed something similar on MXS (mx28) SPI block. Do you use mixed > PIO/DMA mode perhaps ? The SPI driver uses PIO for short transfers and DMA for transfers longer than the controller FIFO. This seems to be the standard way to do things.It works flawlessly so long as submitting overly long DMA programs is avoided. > Do you have the option to connect a bus analyzer? > I can probably offer you some tools, I'm in Prague ... The flash chip is accessible when removing the bottom cover. It is VSOP8 package slightly lower than SOP8 so attaching clips to it might be a bit problematic. That's the only accessible part. Everything other than SPI is inside the SoC. Since SPI has no verification whatsoever the chip might be completely dead and you can still read fine all zeroes or all ones when attempting a read from it. I observed this behaviour when I used a flash chip in a socket and it was not firmly seated. It was with a different SPI controller, though. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.
On 22 July 2015 at 09:33, Marek Vasut wrote: > On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote: >> On 22 July 2015 at 06:49, Vinod Koul wrote: >> > On Tue, Jul 21, 2015 at 10:14:11AM +0200, Michal Suchanek wrote: >> >> > Or alternatively we could publish the limitations of the channel using >> >> > capabilities so SPI knows I have a dmaengine channel and it can >> >> > transfer max N length transfers so would be able to break rather than >> >> > guessing it or coding in DT. Yes it may come from DT but that should >> >> > be dmaengine driver rather than client driver :) >> >> > >> >> > This can be done by dma_get_slave_caps(chan, &caps) >> >> > >> >> > And we add max_length as one more parameter to existing set >> >> > >> >> > Also all this could be handled in generic SPI-dmaengine layer so that >> >> > individual drivers don't have to code it in >> >> > >> >> > Let me know if this idea is okay, I can push the dmaengine bits... >> >> >> >> It would be ok if there was a fixed limit. However, the limit depends >> >> on SPI slave settings. Presumably for other buses using the dmaengine >> >> the limit would depend on the bus or slave settings as well. I do not >> >> see a sane way of passing this all the way to the dmaengine driver. >> > >> > I don't see why this should be client (SPI) dependent. The max length >> > supported is a dmaengine constraint, typically flowing from max >> > blocks/length it can transfer. Know this limit can allow clients to split >> > transfers. >> >> In practice on the board I have the maximum transfer length before it >> fails depends on SPI bus speed which is set up per slave. I did not >> try searching the space of possible settings thorougly and settled for >> a setting that gives reasonable speed and transfer length. > > This looks more like a signal integrity issue though. > It certainly does on the surface. However, when wrong data is delivered over the SPI bus (such as when I use wrong phase setting) the SPI controller happily delivers wrong data over PIO. The failure I am seeing is that the pl330 DMA program which repeatedly waits for data from the SPI controller never finishes the read loop and does not signal the interrupt. It seems it also leaves some data in a FIFO somewhere so next command on the flash returns garbage and fails. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.
On 22 July 2015 at 06:49, Vinod Koul wrote: > On Tue, Jul 21, 2015 at 10:14:11AM +0200, Michal Suchanek wrote: >> > Or alternatively we could publish the limitations of the channel using >> > capabilities so SPI knows I have a dmaengine channel and it can transfer >> > max N >> > length transfers so would be able to break rather than guessing it or >> > coding >> > in DT. Yes it may come from DT but that should be dmaengine driver rather >> > than client driver :) >> > >> > This can be done by dma_get_slave_caps(chan, &caps) >> > >> > And we add max_length as one more parameter to existing set >> > >> > Also all this could be handled in generic SPI-dmaengine layer so that >> > individual drivers don't have to code it in >> > >> > Let me know if this idea is okay, I can push the dmaengine bits... >> >> It would be ok if there was a fixed limit. However, the limit depends >> on SPI slave settings. Presumably for other buses using the dmaengine >> the limit would depend on the bus or slave settings as well. I do not >> see a sane way of passing this all the way to the dmaengine driver. > I don't see why this should be client (SPI) dependent. The max length > supported is a dmaengine constraint, typically flowing from max > blocks/length it can transfer. Know this limit can allow clients to split > transfers. In practice on the board I have the maximum transfer length before it fails depends on SPI bus speed which is set up per slave. I did not try searching the space of possible settings thorougly and settled for a setting that gives reasonable speed and transfer length. However, if this was not tied to the particular slave setting picked in the current DT a formula would be needed that translates arbitrary client settings to transfer size limit and there would be need to somehow get the client settings to the formula in the dmaengine driver. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.
Hello, On 21 July 2015 at 06:29, Vinod Koul wrote: > On Sun, Jul 19, 2015 at 09:01:34PM +0200, Michal Suchanek wrote: >> Hello, >> >> On 15 July 2015 at 17:59, Brian Norris wrote: >> > Hi Michal, >> > >> > On Wed, Jul 15, 2015 at 01:52:27PM +0200, Marek Vasut wrote: >> >> The problem is, if you add a new DT binding, you'd have to support it >> >> forever, no matter how bad idea that binding turned out to be. >> > >> > Agreed, and a solid NAK to this patch. I could have sworn I gave such a >> > response when this was originally being discussed a month ago. >> > >> > AFAICT, you have one of two general approaches available to you: >> > >> > 1. Fix up the SPI driver so that it knows how to break large SPI >> > transfers up into smaller segments that its constituent hardware (DMA >> > controllers, fast clocks, etc.) can handle. >> > >> > 2. Utilize/create a parameter within the SPI subsystem to communicate >> > that the SPI master has a limited max transfer size (notably: NOT a >> > per-device DT property, but a SPI API property), and modify SPI device >> > drivers (like m25p80) to honor it. Mark Brown seemed open to this, and I >> > thought he suggested it somewhere. >> >> It is not known what exactly is limited here. >> >> It seems that the pl330 fails but it is not possible to transfer that >> much data over the spi bus in one go without the help of the pl330. >> >> With either approach the limit depends on the SPI transfer settings >> which are known the the SPI driver. The pl330 driver is oblivious to >> these because it just transfers data from one port to another port and >> has no idea that the port is wired to SPI in the SoC. >> >> On the other hand, AFAICT the SPI driver only allocates a DMA channel >> which it receives through DT binding and is oblivious to the fact the >> DMA channel lives on a pl330. It could probably determine that the >> channel is indeed driven by a pl330. I don't think it's a great idea >> to add device-specific handling to a generic dmaengine driver or a >> dmaengine-spiecific handling to a SPI driver. >> >> It's technically possible to pass SPI transfer parameters to the >> dmaengine driver prior to transfer and the dmaengine could impose some >> limitation based on those parameters. However, generalising this to >> drivers other than SPI might be problematic. Should this interface >> also handle i2c parameters, VE parameters, audio parameters, ethernet >> parameters, etc? > Or alternatively we could publish the limitations of the channel using > capabilities so SPI knows I have a dmaengine channel and it can transfer max N > length transfers so would be able to break rather than guessing it or coding > in DT. Yes it may come from DT but that should be dmaengine driver rather > than client driver :) > > This can be done by dma_get_slave_caps(chan, &caps) > > And we add max_length as one more parameter to existing set > > Also all this could be handled in generic SPI-dmaengine layer so that > individual drivers don't have to code it in > > Let me know if this idea is okay, I can push the dmaengine bits... > It would be ok if there was a fixed limit. However, the limit depends on SPI slave settings. Presumably for other buses using the dmaengine the limit would depend on the bus or slave settings as well. I do not see a sane way of passing this all the way to the dmaengine driver. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.
Hello, On 15 July 2015 at 17:59, Brian Norris wrote: > Hi Michal, > > On Wed, Jul 15, 2015 at 01:52:27PM +0200, Marek Vasut wrote: >> The problem is, if you add a new DT binding, you'd have to support it >> forever, no matter how bad idea that binding turned out to be. > > Agreed, and a solid NAK to this patch. I could have sworn I gave such a > response when this was originally being discussed a month ago. > > AFAICT, you have one of two general approaches available to you: > > 1. Fix up the SPI driver so that it knows how to break large SPI > transfers up into smaller segments that its constituent hardware (DMA > controllers, fast clocks, etc.) can handle. > > 2. Utilize/create a parameter within the SPI subsystem to communicate > that the SPI master has a limited max transfer size (notably: NOT a > per-device DT property, but a SPI API property), and modify SPI device > drivers (like m25p80) to honor it. Mark Brown seemed open to this, and I > thought he suggested it somewhere. It is not known what exactly is limited here. It seems that the pl330 fails but it is not possible to transfer that much data over the spi bus in one go without the help of the pl330. With either approach the limit depends on the SPI transfer settings which are known the the SPI driver. The pl330 driver is oblivious to these because it just transfers data from one port to another port and has no idea that the port is wired to SPI in the SoC. On the other hand, AFAICT the SPI driver only allocates a DMA channel which it receives through DT binding and is oblivious to the fact the DMA channel lives on a pl330. It could probably determine that the channel is indeed driven by a pl330. I don't think it's a great idea to add device-specific handling to a generic dmaengine driver or a dmaengine-spiecific handling to a SPI driver. It's technically possible to pass SPI transfer parameters to the dmaengine driver prior to transfer and the dmaengine could impose some limitation based on those parameters. However, generalising this to drivers other than SPI might be problematic. Should this interface also handle i2c parameters, VE parameters, audio parameters, ethernet parameters, etc? In the DT you have the information that this particular device is connected to a Samsung SPI controller connected to a pl330 dma engine. > > It is most definitely wrong to put this information solely in the slave > device DT (i.e., for the flash), when it is not a property of the flash > device at all. It's a property of the SPI master and/or its clocks and > DMA controllers. However, the clocks are set by the parameters of the device, not the parameters of the master. So the limitation applies for the master with the settings of the particular slave device. Different slave settings do impose different master limitations. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.
On 4 June 2015 at 19:15, Richard Cochran wrote: > On Thu, Jun 04, 2015 at 10:31:45AM +0200, Michal Suchanek wrote: >> You might want to try to run the bus at 60MHz or 80MHz and then the >> values would probably again be different. >> >> The first two values are set in DT so the logical place for setting >> the third is also in DT. >> >> Otherwise you would need some in-kernel table of these settings. > > Or a formula. > This formula probably needs to take into account - the unknown reason for the pl330 to fail transfer - the device transfer speed and transfer phase as set in DT - possibly device-specific latency and board-specific trace design and assembly tolerances Seriously, until I have at least a vague idea why the transfer fails I am not comfortable pulling some formula out of thin air and pretending I have a working patch. On the other hand, a parameter you can set in the DT and which comes with a suggested value which can be tuned depending on the system seems more viable. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/11] mtd: ofpart: do not fail probe when no partitions exist
On 4 June 2015 at 17:28, Marek Vasut wrote:
> On Thursday, June 04, 2015 at 06:54:00 AM, Michal Suchanek wrote:
>> On 4 June 2015 at 00:58, Marek Vasut wrote:
>> > On Wednesday, June 03, 2015 at 11:26:40 PM, Michal Suchanek wrote:
>> >> On Exynos it is necessary to set SPI controller parameters that apply to
>> >> a SPI slave in a DT subnode of the slave device. The ofpart code returns
>> >> an error when there are subnodes of the SPI flash but no partitions are
>> >> found. Change this condition to a warning so that flash without
>> >> partitions can be accessed on Exynos.
>> >
>> > I have to admit the rationale for this patch is not very clear to me,
>> > sorry. Can you please explain this a bit more ?
>>
>> This is how the DT entry for SPI slave looks with s3c64xx:
>> flash: m25p80@0 {
>> #address-cells = <1>;
>> #size-cells = <1>;
>> compatible = "jedec,spi-nor";
>> reg = <0>;
>> spi-max-frequency = <4000>;
>> linux,max_tx_len = <65536>;
>
> SIDENOTE: I thought this was actually added by your patch #8 in this
> series. The underscores in the name of the property are not really
> consistent with the rest of the names.
>
>> m25p,fast-read;
>> controller-data {
>> samsung,spi-feedback-delay = <0>;
>> };
>> };
>>
>> this is example of flash partitions:
>> flash@0 {
>> #address-cells = <1>;
>> #size-cells = <1>;
>>
>> partition@0 {
>> label = "u-boot";
>> reg = <0x000 0x10>;
>> read-only;
>> };
>>
>> uimage@10 {
>> reg = <0x010 0x20>;
>> };
>> };
>>
>> The parser ignores any flash without subnodes and returns 0 (no
>> partititon). When there is a subnode it assumes the flash is
>> partitioned and tries to parse the subnodes as partitions. When there
>> are subnodes and none parses as partition an error is returned. As
>> shown above it is valid to have subnodes on unpartitioned flash.
>>
>> When an error is returned from a partition parser the mtdpart code
>> passes on this error to the flash probe function and the proble of the
>> flash fails.
>
> What does /proc/mtd tell you when you have no partitions defined
> in the DT ? It should provide you with the entire MTD device and
> the code shouldn't even try to parse any OF partitions, since you
> don't have any.
mtdinfo shows I have no mtd devices and the log shows that probe
failed unless I patch the kernel.
When there is *support* for of partitions the ofparser is run on mtd
probe. If it fails because it considers
>> controller-data {
>> samsung,spi-feedback-delay = <0>;
>> };
an invalid partition and does not find any valid partition the probe fails.
There are two problems here
1) the above is not invalid. It just is not a partition. The parser
should not fail seeing this
2) the mtd probe should not fail when a partition parser fails and
should present the unpartitioned device
Both are addressed in separate patches.
Thanks
Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
On 4 June 2015 at 12:26, Mark Brown wrote: > On Thu, Jun 04, 2015 at 11:33:37AM +0200, Michal Suchanek wrote: >> On 4 June 2015 at 11:16, Mark Brown wrote: > >> > Also for this patch (which just adds some trace) there isn't any clear >> > reason for it to be sent as part of the series at all, it doesn't help >> > deliver the functionality and doesn't depend on the rest of the series. > >> I used this patch to add missing information to dmesg output so I >> could diagnose the SPI failures so I think it is relevant. > > The fact that you used this to debug problems is not relevant to any fix > you might have developed for those problems; the fix can be explained > without needing to know how you got there. Similarly the debugging is > hopefully useful in general without needing to know which particular > problem prompted it's creation. Yes, this patch should be meaningful on its own. The ultimate purpose of this patch series is to make the SPI NOR flash chip usable on boards of which I have 1 sample. If the results on boards other than mine happen to be different the debug prints will be useful for users of this flash chip. There is no code interdependency with the other patches so if it's preferred I can send patches like this separately. > >> To print a clock name you AFAICT need this header. I think this is an >> abstraction problem in the clock framework. Fixing the abstraction >> problem with clock framework would only grow the list of recipients >> which is already so long you complain about it. > > This is a bit of a warning sign that the series isn't very focused. > It's also just not good practice, sending patches with obvious problems > (especially obvious problems that aren't clearly flagged up as something > temporary) will frustrate reviewers and can often lead to other issues > being obscured. As has been pointed out public interface already exists for getting the clock name so the issue here is cosmetic. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/11] spi: add more debug prints in s3c64xx
Hello,
On 4 June 2015 at 11:16, Mark Brown wrote:
> On Wed, Jun 03, 2015 at 09:26:42PM -0000, Michal Suchanek wrote:
>> The SPI NOR transfers mysteriously fail so add more debug prints about
>> SPI transactions.
>
> Please try to only send patches to relevant people - the list of
> recipients for this is so large that it only barely fits on a single
> screen in my mail client.
>
> Also for this patch (which just adds some trace) there isn't any clear
> reason for it to be sent as part of the series at all, it doesn't help
> deliver the functionality and doesn't depend on the rest of the series.
I used this patch to add missing information to dmesg output so I
could diagnose the SPI failures so I think it is relevant.
The failure is reported by the s3c64xx but the root cause is pl330 not
finishing a DMA transfer.
It is non-obvious that this is the issue. The information that a call
to pl330 failed is only available in the s3c64xx driver and not SPI
core so I think there is no reasonable way to debug this issue other
than adding prints in s3c64xx. This issue is not completely resolved
or even well tested (I have only single board to test with) so these
prints remain relevant in my view.
>
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -18,6 +18,7 @@
>> #include
>> #include
>> #include
>> +#include
>
> Whatever you're doing here this indicates that there's a very big
> abstraction problem going on.
I wanted to print a clock name in case the information was relevant
for the issue at hand.
To print a clock name you AFAICT need this header. I think this is an
abstraction problem in the clock framework. Fixing the abstraction
problem with clock framework would only grow the list of recipients
which is already so long you complain about it.
It turns out that in this case the clock was not at fault so I did not
need the clock name in the end.
>
>> + pr_debug("%s %s %s waiting for %ims transferring %zubytes@%iHz",
>> + __func__, sdd->pdev ? dev_name(&sdd->pdev->dev) : NULL,
>> + dev_name(&sdd->master->dev),
>> + ms, xfer->len, sdd->cur_speed);
>
> I'd say dev_dbg() but more generally this is just tracing things that
> seem to be already covered by the trace points already present in the
> core, the same goes for most of the rest of it. If there's things
> missing from the existing trace it seems better to add to it.
There is master and slave device involved but it's certainly possible
to use dev_dbg on one of them and pass the other as string.
Thanks
Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.
On 4 June 2015 at 08:42, Geert Uytterhoeven wrote: > On Wed, Jun 3, 2015 at 11:26 PM, Michal Suchanek wrote: >> On sunxi the SPI controller currently does not have DMA support and fails >> any transfer larger than 63 bytes. > > This is a driver limitation, not a hardware limitation. > >> On Exynos the pl330 DMA controller fails any transfer larger than 64kb >> when using slower speed like 40MHz and any transfer larger than 128bytes >> when running at 133MHz. > > This may be a driver bug. > >> The best thing is that in both cases the controller can just lock up and >> never finish potentially leaving the hardware in unusable state. >> >> So it is required that the m25p80 driver actively prevents doing >> transfers that are too large for the current driver state on a >> particular piece of hardware. > > OK. > > DT describes the hardware, not buggy drivers. > > So IMHO this doesn't belong in DT, but it can be a field in struct spi_master. > Unfortunately, this cannot be a field in struct spi_master. The value can and does vary with device configuration and device configuration is only available in DT. For example, on the Snow board one working configuration is 40MHz spi bus speed, feedback delay 0 and maximum transfer size 64k. Another working configuration is 133MHz bus speed, feedback delay 3, and maximum transfer size 128 bytes. You might want to try to run the bus at 60MHz or 80MHz and then the values would probably again be different. The first two values are set in DT so the logical place for setting the third is also in DT. Otherwise you would need some in-kernel table of these settings. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/11] mtd: ofpart: do not fail probe when no partitions exist
On 4 June 2015 at 00:58, Marek Vasut wrote:
> On Wednesday, June 03, 2015 at 11:26:40 PM, Michal Suchanek wrote:
>> On Exynos it is necessary to set SPI controller parameters that apply to
>> a SPI slave in a DT subnode of the slave device. The ofpart code returns
>> an error when there are subnodes of the SPI flash but no partitions are
>> found. Change this condition to a warning so that flash without
>> partitions can be accessed on Exynos.
>
> I have to admit the rationale for this patch is not very clear to me, sorry.
> Can you please explain this a bit more ?
This is how the DT entry for SPI slave looks with s3c64xx:
flash: m25p80@0 {
#address-cells = <1>;
#size-cells = <1>;
compatible = "jedec,spi-nor";
reg = <0>;
spi-max-frequency = <4000>;
linux,max_tx_len = <65536>;
m25p,fast-read;
controller-data {
samsung,spi-feedback-delay = <0>;
};
};
this is example of flash partitions:
flash@0 {
#address-cells = <1>;
#size-cells = <1>;
partition@0 {
label = "u-boot";
reg = <0x000 0x10>;
read-only;
};
uimage@10 {
reg = <0x010 0x20>;
};
};
The parser ignores any flash without subnodes and returns 0 (no
partititon). When there is a subnode it assumes the flash is
partitioned and tries to parse the subnodes as partitions. When there
are subnodes and none parses as partition an error is returned. As
shown above it is valid to have subnodes on unpartitioned flash.
When an error is returned from a partition parser the mtdpart code
passes on this error to the flash probe function and the proble of the
flash fails.
Thanks
Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.
On 4 June 2015 at 01:03, Marek Vasut wrote: > On Wednesday, June 03, 2015 at 11:26:41 PM, Michal Suchanek wrote: >> On sunxi the SPI controller currently does not have DMA support and fails >> any transfer larger than 63 bytes. >> >> On Exynos the pl330 DMA controller fails any transfer larger than 64kb >> when using slower speed like 40MHz and any transfer larger than 128bytes >> when running at 133MHz. > > This smells more like some corruption of the data on the bus or something > even more obscure. If the data was corrupted you would get corrupted data and not transfer failure. AFAIK there is no checksum or anything. I actually do get corrupted data when I use wrong feedback delay setting. > >> The best thing is that in both cases the controller can just lock up and >> never finish potentially leaving the hardware in unusable state. >> >> So it is required that the m25p80 driver actively prevents doing >> transfers that are too large for the current driver state on a >> particular piece of hardware. >> >> Signed-off-by: Michal Suchanek >> >> -- >> >> Update commit message and documentation text. >> --- >> .../devicetree/bindings/mtd/jedec,spi-nor.txt | 6 ++ >> drivers/mtd/devices/m25p80.c | 24 >> -- 2 files changed, 28 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt >> b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt index >> 2bee681..4e63ae8 100644 >> --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt >> +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt >> @@ -19,6 +19,12 @@ Optional properties: >> all chips and support for it can not be detected at >> runtime. Refer to your chips' datasheet to check if this is supported by >> your chip. >> +- linux,max_tx_len : With some SPI controller drivers possible transfer >> size is + limited. This may be hardware or driver bug. >> + Transfer data in chunks no larger than this value. + >> Using this option may severely degrade performance and >> + possibly flash memory life when max_tx_len is >> smaller than + flash page size (typically 256 bytes) > > Will we need similar patch for all other SPI slave drivers, like SPI NAND ? Probably. Some SPI slave drivers already do have similar option. In general it cannot be expected that you can reliably transfer arbitrarily large data over SPI it seems. However, if the nand driver transfers data a page at a time it should be fine. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/11] Enable access to SPI NOR flash on Samsung Snow board
On 4 June 2015 at 00:53, Marek Vasut wrote: > On Wednesday, June 03, 2015 at 11:26:39 PM, Michal Suchanek wrote: >> Hello, > > Hi, > >> this patch series makes it possible to access the SPI NOR flash in the >> Samsung XE303 'Snow' Chromebook. >> >> Unfortunately not all issues are resolved. To work around an issue with the >> pl330 dma engine I respun the patch for limiting transfer size in m25p80 >> driver. > > Looks like fixing a bug at the wrong place to me ... > >> As the flash does not contain any sane filesystem and is only likely to be >> accessed with mtd_debug or similar tool the limit of the dma engine is >> easily reached. Filesystems using shorter data transfers are less likely >> to be affected. > > Sounds like the DMA engine driver should be fixed. I looked at the pl330 datasheet and don't see anything obviusly wrong with the pl330 driver in Linux. Ideally it would be fixed but I have no idea what's wrong with it. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: exynos_defconfig: add options to make wifi usable
On 11 May 2015 at 13:25, Javier Martinez Canillas wrote: > Hello Michal, > > On 05/11/2015 12:22 PM, Michal Suchanek wrote: >> The Exynos defconfig includes mwifiex sdio support which is present on >> some of the Exynos boards. >> >> For the WiFi to be usable two extra options are needed. Usermode > > Your subject line and the commit message are somehow misleading since > these options are needed to make the WiFi usable with your current setup. > >> firmware helper to load out-of-kernel firmware and wireless extensions > > For example, this is only needed if the in-kernel fw loader is not > able to find the firmware but isn't needed if the fw is for example > in an initial ramdisk and the kernel is able to load it, built in the > kernel or if the mwifiex driver is built as a module. I am not sure how is including the firmware in a ramdisk going to improve things over including it in my root filesystem. As far as I am aware it does not make any difference for the kernel. The firmware is not included in the kernel tree nor configured as extra firmware option in the defconfig, either. Otherwise the firmware loader would supposedly find the firmware and we would not have this discussion. The mwifiex driver is configured as built-in in the defconfig so what happens when the driver is built as a module is not relevant for this defconfig. > >> so the interface can be configured with wireless-tools. >> > > And wireless extensions is deprecated AFAIK and is only needed for old > user-space since most tools should had been converted to use the netlink > based CONFIG_CFG80211 interface instead. > > I'm booting a debian jessie and have WiFi working without CFG80211_WEXT > for example. I'm booting Debian Jessie as well and for me WiFi is not working without CFG80211_WEXT for another example. So it might be that some tools have migrated to another interface but at first glance I have no idea what those tools might be in Debian so for me the WiFi is unusable without wireless extensions. > > That doesn't mean that I'm against your patch (I'm always happy to enable > more config options if that makes the defconfig more useful) but the commit > message should be accurate about why a change has to be done. Do you have some specific suggestions about improvements to the commit message? Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ARM: exynos_defconfig: add options to make wifi usable
The Exynos defconfig includes mwifiex sdio support which is present on some of the Exynos boards. For the WiFi to be usable two extra options are needed. Usermode firmware helper to load out-of-kernel firmware and wireless extensions so the interface can be configured with wireless-tools. Signed-off-by: Michal Suchanek --- arch/arm/configs/exynos_defconfig | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/arm/configs/exynos_defconfig b/arch/arm/configs/exynos_defconfig index d034c96..5d4ee83f 100644 --- a/arch/arm/configs/exynos_defconfig +++ b/arch/arm/configs/exynos_defconfig @@ -42,6 +42,7 @@ CONFIG_IP_PNP_BOOTP=y CONFIG_IP_PNP_RARP=y CONFIG_WIRELESS=y CONFIG_CFG80211=y +CONFIG_CFG80211_WEXT=y CONFIG_MWIFIEX=y CONFIG_MWIFIEX_SDIO=y CONFIG_RFKILL_REGULATOR=y @@ -49,6 +50,7 @@ CONFIG_UEVENT_HELPER_PATH="/sbin/hotplug" CONFIG_DEVTMPFS=y CONFIG_DEVTMPFS_MOUNT=y CONFIG_PROC_DEVICETREE=y +CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y CONFIG_DMA_CMA=y CONFIG_CMA_SIZE_MBYTES=64 CONFIG_BLK_DEV_LOOP=y -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
