From: "SkyLake.Huang" <skylake.huang@mediatek.com>
This patch removes parens around TRIGGER_NETDEV_RX/TRIGGER_NETDEV_TX in
mtk_phy_led_hw_ctrl_set(), which improves readability.
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: SkyLake.Huang <skylake.huang@mediatek.com>
---
drivers/net/phy/mediatek/mtk-phy-lib.c | 44 ++++++++++++++------------
1 file changed, 24 insertions(+), 20 deletions(-)
diff --git a/drivers/net/phy/mediatek/mtk-phy-lib.c b/drivers/net/phy/mediatek/mtk-phy-lib.c
index 34b0957..8d795bc 100644
--- a/drivers/net/phy/mediatek/mtk-phy-lib.c
+++ b/drivers/net/phy/mediatek/mtk-phy-lib.c
@@ -129,29 +129,33 @@ int mtk_phy_led_hw_ctrl_set(struct phy_device *phydev, u8 index,
on |= MTK_PHY_LED_ON_LINK2500;
if (rules & BIT(TRIGGER_NETDEV_RX)) {
- blink |= (on & on_set) ?
- (((on & MTK_PHY_LED_ON_LINK10) ?
- MTK_PHY_LED_BLINK_10RX : 0) |
- ((on & MTK_PHY_LED_ON_LINK100) ?
- MTK_PHY_LED_BLINK_100RX : 0) |
- ((on & MTK_PHY_LED_ON_LINK1000) ?
- MTK_PHY_LED_BLINK_1000RX : 0) |
- ((on & MTK_PHY_LED_ON_LINK2500) ?
- MTK_PHY_LED_BLINK_2500RX : 0)) :
- rx_blink_set;
+ if (on & on_set) {
+ if (on & MTK_PHY_LED_ON_LINK10)
+ blink |= MTK_PHY_LED_BLINK_10RX;
+ if (on & MTK_PHY_LED_ON_LINK100)
+ blink |= MTK_PHY_LED_BLINK_100RX;
+ if (on & MTK_PHY_LED_ON_LINK1000)
+ blink |= MTK_PHY_LED_BLINK_1000RX;
+ if (on & MTK_PHY_LED_ON_LINK2500)
+ blink |= MTK_PHY_LED_BLINK_2500RX;
+ } else {
+ blink |= rx_blink_set;
+ }
}
if (rules & BIT(TRIGGER_NETDEV_TX)) {
- blink |= (on & on_set) ?
- (((on & MTK_PHY_LED_ON_LINK10) ?
- MTK_PHY_LED_BLINK_10TX : 0) |
- ((on & MTK_PHY_LED_ON_LINK100) ?
- MTK_PHY_LED_BLINK_100TX : 0) |
- ((on & MTK_PHY_LED_ON_LINK1000) ?
- MTK_PHY_LED_BLINK_1000TX : 0) |
- ((on & MTK_PHY_LED_ON_LINK2500) ?
- MTK_PHY_LED_BLINK_2500TX : 0)) :
- tx_blink_set;
+ if (on & on_set) {
+ if (on & MTK_PHY_LED_ON_LINK10)
+ blink |= MTK_PHY_LED_BLINK_10TX;
+ if (on & MTK_PHY_LED_ON_LINK100)
+ blink |= MTK_PHY_LED_BLINK_100TX;
+ if (on & MTK_PHY_LED_ON_LINK1000)
+ blink |= MTK_PHY_LED_BLINK_1000TX;
+ if (on & MTK_PHY_LED_ON_LINK2500)
+ blink |= MTK_PHY_LED_BLINK_2500TX;
+ } else {
+ blink |= tx_blink_set;
+ }
}
if (blink || on)
--
2.45.2Hi Miquel/Martin, About this driver, including F35SQA001G/F35SQA002G parts, I'm concerned that the driver will always use 32H for update_cache operations, which means it's not compitable with those SPI controller who can't transmit 2048 bytes (most small-density SPI-NAND's page size nowadays) at one time. The following controller's driver seems that they can't transmit 2048 bytes in one transmission: - spi-amd.c: 64 bytes (AMD_SPI_MAX_DATA) - spi-amlogic-spifc-a1.c: 512 bytes (SPIFC_A1_BUFFER_SIZE) - spi-fsl-qspi.c: 1KB - spi-hisi-sfc-v3xx.c: 64*6 bytes - spi-intel.c: 64 bytes (INTEL_SPI_FIFO_SZ) - spi-microchip-core-qspi.c: 256 bytesc (MAX_DATA_CMD_LEN) - spi-nxp-fspi.c: TX:1KB, RX: 512B in FIFO mode - spi-wpcm-fiu.c: 4B I guess we need to add some check to make sure that F35SQA series work only with those SPI controllers who can transmit more than 2048 bytes(NAND page size) at one time? BRs, Sky
Hi Sky, On 12/11/2024 at 10:08:31 GMT, SkyLake Huang (黃啟澤) <SkyLake.Huang@mediatek.com> wrote: > Hi Miquel/Martin, > About this driver, including F35SQA001G/F35SQA002G parts, I'm concerned > that the driver will always use 32H for update_cache operations, which > means it's not compitable with those SPI controller who can't transmit > 2048 bytes (most small-density SPI-NAND's page size nowadays) at one > time. > > The following controller's driver seems that they can't transmit 2048 > bytes in one transmission: > - spi-amd.c: 64 bytes (AMD_SPI_MAX_DATA) > - spi-amlogic-spifc-a1.c: 512 bytes (SPIFC_A1_BUFFER_SIZE) > - spi-fsl-qspi.c: 1KB > - spi-hisi-sfc-v3xx.c: 64*6 bytes > - spi-intel.c: 64 bytes (INTEL_SPI_FIFO_SZ) > - spi-microchip-core-qspi.c: 256 bytesc (MAX_DATA_CMD_LEN) > - spi-nxp-fspi.c: TX:1KB, RX: 512B in FIFO mode > - spi-wpcm-fiu.c: 4B I believe most of these drivers are still able to send one page of data without toggling the CS (which is what actually matters, I believe). If they were broken, they would be broken with all spi memory devices, not only Foresee's. > I guess we need to add some check to make sure that F35SQA series work > only with those SPI controllers who can transmit more than 2048 > bytes(NAND page size) at one time? There is already a supports_op() hook for that, I believe we are fine. If however you experience errors, please report them and we'll look for a solution. Thanks, Miquèl
On Tue, 2024-11-12 at 11:48 +0100, Miquel Raynal wrote: > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > Hi Sky, > > On 12/11/2024 at 10:08:31 GMT, SkyLake Huang (黃啟澤) < > SkyLake.Huang@mediatek.com> wrote: > > > Hi Miquel/Martin, > > About this driver, including F35SQA001G/F35SQA002G parts, I'm > > concerned > > that the driver will always use 32H for update_cache operations, > > which > > means it's not compitable with those SPI controller who can't > > transmit > > 2048 bytes (most small-density SPI-NAND's page size nowadays) at > > one > > time. > > > > The following controller's driver seems that they can't transmit > > 2048 > > bytes in one transmission: > > - spi-amd.c: 64 bytes (AMD_SPI_MAX_DATA) > > - spi-amlogic-spifc-a1.c: 512 bytes (SPIFC_A1_BUFFER_SIZE) > > - spi-fsl-qspi.c: 1KB > > - spi-hisi-sfc-v3xx.c: 64*6 bytes > > - spi-intel.c: 64 bytes (INTEL_SPI_FIFO_SZ) > > - spi-microchip-core-qspi.c: 256 bytesc (MAX_DATA_CMD_LEN) > > - spi-nxp-fspi.c: TX:1KB, RX: 512B in FIFO mode > > - spi-wpcm-fiu.c: 4B > > I believe most of these drivers are still able to send one page of > data > without toggling the CS (which is what actually matters, I believe). > If > they were broken, they would be broken with all spi memory devices, > not > only Foresee's. > Hi Miquel, I think it's not about toggling the CS? If a SPI controller tries to execute write page and it's capable to send only 1KB in one transmission, it should transmit data in two steps: 1st 34H (random program load x4) and 2nd 34H. However, when F35SQA002G executes 2nd 34H command, it needs to execute 32H first, and it will clear data transmitted by 1st 34H in NAND flash's cache. This will cause data corruption. Other SPI-NANDs doesn't need to execute 32H before 34H. > > I guess we need to add some check to make sure that F35SQA series > > work > > only with those SPI controllers who can transmit more than 2048 > > bytes(NAND page size) at one time? > > There is already a supports_op() hook for that, I believe we are > fine. If however you experience errors, please report them and we'll > look for a solution. > > Thanks, > Miquèl We can block 32H update_cache opcode in supports_op() hook for those light SPI controllers(transmittion cap. < 2048 Bytes)? Not sure if there's a better solution. Indeed, most SPI controllers support DMA transmission(>2048 Bytes) now, including our MTK filogic platform. If this doesn't bother other FIFO- only SPI controllers, I'll take it. BRs, Sky
On 12/11/2024 at 11:25:25 GMT, SkyLake Huang (黃啟澤) <SkyLake.Huang@mediatek.com> wrote: > On Tue, 2024-11-12 at 11:48 +0100, Miquel Raynal wrote: >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> >> >> Hi Sky, >> >> On 12/11/2024 at 10:08:31 GMT, SkyLake Huang (黃啟澤) < >> SkyLake.Huang@mediatek.com> wrote: >> >> > Hi Miquel/Martin, >> > About this driver, including F35SQA001G/F35SQA002G parts, I'm >> > concerned >> > that the driver will always use 32H for update_cache operations, >> > which >> > means it's not compitable with those SPI controller who can't >> > transmit >> > 2048 bytes (most small-density SPI-NAND's page size nowadays) at >> > one >> > time. >> > >> > The following controller's driver seems that they can't transmit >> > 2048 >> > bytes in one transmission: >> > - spi-amd.c: 64 bytes (AMD_SPI_MAX_DATA) >> > - spi-amlogic-spifc-a1.c: 512 bytes (SPIFC_A1_BUFFER_SIZE) >> > - spi-fsl-qspi.c: 1KB >> > - spi-hisi-sfc-v3xx.c: 64*6 bytes >> > - spi-intel.c: 64 bytes (INTEL_SPI_FIFO_SZ) >> > - spi-microchip-core-qspi.c: 256 bytesc (MAX_DATA_CMD_LEN) >> > - spi-nxp-fspi.c: TX:1KB, RX: 512B in FIFO mode >> > - spi-wpcm-fiu.c: 4B >> >> I believe most of these drivers are still able to send one page of >> data >> without toggling the CS (which is what actually matters, I believe). >> If >> they were broken, they would be broken with all spi memory devices, >> not >> only Foresee's. >> > Hi Miquel, > I think it's not about toggling the CS? > > If a SPI controller tries to execute write page and it's capable to > send only 1KB in one transmission, it should transmit data in two > steps: 1st 34H (random program load x4) and 2nd 34H. However, when > F35SQA002G executes 2nd 34H command, it needs to execute 32H first, and > it will clear data transmitted by 1st 34H in NAND flash's cache. This > will cause data corruption. Other SPI-NANDs doesn't need to execute 32H > before 34H. Is it really what happens? I'd instead expect the spi controller to send: - 34h - 1k data - 1k data ... Why should we repeat the command while we are in the I/O phase? Thanks, Miquèl
Hello all! On Wed, Nov 13, 2024 at 5:05 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > On 12/11/2024 at 11:25:25 GMT, SkyLake Huang (黃啟澤) <SkyLake.Huang@mediatek.com> wrote: > > > On Tue, 2024-11-12 at 11:48 +0100, Miquel Raynal wrote: > >> External email : Please do not click links or open attachments until > >> you have verified the sender or the content. > >> > >> > >> Hi Sky, > >> > >> On 12/11/2024 at 10:08:31 GMT, SkyLake Huang (黃啟澤) < > >> SkyLake.Huang@mediatek.com> wrote: > >> > >> > Hi Miquel/Martin, > >> > About this driver, including F35SQA001G/F35SQA002G parts, I'm > >> > concerned > >> > that the driver will always use 32H for update_cache operations, > >> > which > >> > means it's not compitable with those SPI controller who can't > >> > transmit > >> > 2048 bytes (most small-density SPI-NAND's page size nowadays) at > >> > one > >> > time. > >> > > >> > The following controller's driver seems that they can't transmit > >> > 2048 > >> > bytes in one transmission: > >> > - spi-amd.c: 64 bytes (AMD_SPI_MAX_DATA) > >> > - spi-amlogic-spifc-a1.c: 512 bytes (SPIFC_A1_BUFFER_SIZE) > >> > - spi-fsl-qspi.c: 1KB > >> > - spi-hisi-sfc-v3xx.c: 64*6 bytes > >> > - spi-intel.c: 64 bytes (INTEL_SPI_FIFO_SZ) > >> > - spi-microchip-core-qspi.c: 256 bytesc (MAX_DATA_CMD_LEN) > >> > - spi-nxp-fspi.c: TX:1KB, RX: 512B in FIFO mode > >> > - spi-wpcm-fiu.c: 4B > >> > >> I believe most of these drivers are still able to send one page of > >> data > >> without toggling the CS (which is what actually matters, I believe). > >> If > >> they were broken, they would be broken with all spi memory devices, > >> not > >> only Foresee's. > >> > > Hi Miquel, > > I think it's not about toggling the CS? > > > > If a SPI controller tries to execute write page and it's capable to > > send only 1KB in one transmission, it should transmit data in two > > steps: 1st 34H (random program load x4) and 2nd 34H. However, when > > F35SQA002G executes 2nd 34H command, it needs to execute 32H first, and I don't think that's what the datasheet means. I think it needs 02h/32h as the first command to write page cache, and the subsequent page cache writing can be done using 84h/34h before the final page program happens. Otherwise the 84h/34h command is just completely broken and useless. If it actually is broken, we do need additional guards in spinand_write_cache_op to abort when spi_mem_dirmap_write doesn't return exactly nbytes as requested. > > it will clear data transmitted by 1st 34H in NAND flash's cache. This > > will cause data corruption. Other SPI-NANDs doesn't need to execute 32H > > before 34H. > > Is it really what happens? I'd instead expect the spi controller to > send: > - 34h > - 1k data > - 1k data > ... > > Why should we repeat the command while we are in the I/O phase? Several SPI-MEM controller don't allow software controlled CS, or for some other reasons are unable to execute a longer spi-mem op. spi_mem_dirmap_write returns the actual request size it's able to make, and spinand_write_to_cache_op use a while loop to split one update_cache request into multiple ones. This only works using the Random Program Load instruction (84h/34h) which preserves the previous written data in the flash data cache. All current supported flashes are exclusively using 84h/34h as the command to write the page cache. Load Program Data(02h/32h) will clear the entire page cache. As a result, when a request is split as described above, the previous written data will be cleared, breaking the page cache writing procedure. We can change write_to_cache_op to use Load Program Data on the first request. If that doesn't finish writing, use Random Program Load on subsequent data loading. -- Regards, Chuanhong Guo
On Wed, 2024-11-13 at 18:10 +0800, Chuanhong Guo wrote: > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > Hello all! > > On Wed, Nov 13, 2024 at 5:05 PM Miquel Raynal < > miquel.raynal@bootlin.com> wrote: > > > > On 12/11/2024 at 11:25:25 GMT, SkyLake Huang (黃啟澤) < > > SkyLake.Huang@mediatek.com> wrote: > > > > > On Tue, 2024-11-12 at 11:48 +0100, Miquel Raynal wrote: > > > > External email : Please do not click links or open attachments > > > > until > > > > you have verified the sender or the content. > > > > > > > > > > > > Hi Sky, > > > > > > > > On 12/11/2024 at 10:08:31 GMT, SkyLake Huang (黃啟澤) < > > > > SkyLake.Huang@mediatek.com> wrote: > > > > > > > > > Hi Miquel/Martin, > > > > > About this driver, including F35SQA001G/F35SQA002G parts, I'm > > > > > concerned > > > > > that the driver will always use 32H for update_cache > > > > > operations, > > > > > which > > > > > means it's not compitable with those SPI controller who can't > > > > > transmit > > > > > 2048 bytes (most small-density SPI-NAND's page size nowadays) > > > > > at > > > > > one > > > > > time. > > > > > > > > > > The following controller's driver seems that they can't > > > > > transmit > > > > > 2048 > > > > > bytes in one transmission: > > > > > - spi-amd.c: 64 bytes (AMD_SPI_MAX_DATA) > > > > > - spi-amlogic-spifc-a1.c: 512 bytes (SPIFC_A1_BUFFER_SIZE) > > > > > - spi-fsl-qspi.c: 1KB > > > > > - spi-hisi-sfc-v3xx.c: 64*6 bytes > > > > > - spi-intel.c: 64 bytes (INTEL_SPI_FIFO_SZ) > > > > > - spi-microchip-core-qspi.c: 256 bytesc (MAX_DATA_CMD_LEN) > > > > > - spi-nxp-fspi.c: TX:1KB, RX: 512B in FIFO mode > > > > > - spi-wpcm-fiu.c: 4B > > > > > > > > I believe most of these drivers are still able to send one page > > > > of > > > > data > > > > without toggling the CS (which is what actually matters, I > > > > believe). > > > > If > > > > they were broken, they would be broken with all spi memory > > > > devices, > > > > not > > > > only Foresee's. > > > > > > > > > > Hi Miquel, > > > I think it's not about toggling the CS? > > > > > > If a SPI controller tries to execute write page and it's capable > > > to > > > send only 1KB in one transmission, it should transmit data in two > > > steps: 1st 34H (random program load x4) and 2nd 34H. However, > > > when > > > F35SQA002G executes 2nd 34H command, it needs to execute 32H > > > first, and > > I don't think that's what the datasheet means. I think it needs > 02h/32h as the first > command to write page cache, and the subsequent page cache writing > can > be done using 84h/34h before the final page program happens. > Otherwise the > 84h/34h command is just completely broken and useless. > If it actually is broken, we do need additional guards in > spinand_write_cache_op > to abort when spi_mem_dirmap_write doesn't return exactly nbytes as > requested. > Confirm with Foresee and yes you're right XD > > > it will clear data transmitted by 1st 34H in NAND flash's cache. > > > This > > > will cause data corruption. Other SPI-NANDs doesn't need to > > > execute 32H > > > before 34H. > > > > Is it really what happens? I'd instead expect the spi controller to > > send: > > - 34h > > - 1k data > > - 1k data > > ... > > > > Why should we repeat the command while we are in the I/O phase? > > Several SPI-MEM controller don't allow software controlled CS, or for > some > other reasons are unable to execute a longer spi-mem op. > spi_mem_dirmap_write returns the actual request size it's able to > make, > and spinand_write_to_cache_op use a while loop to split one > update_cache > request into multiple ones. > > This only works using the Random Program Load instruction (84h/34h) > which preserves the previous written data in the flash data cache. > All current supported flashes are exclusively using 84h/34h as the > command > to write the page cache. > > Load Program Data(02h/32h) will clear the entire page cache. As a > result, when a request is split as described above, the previous > written > data will be cleared, breaking the page cache writing procedure. > > We can change write_to_cache_op to use Load Program Data on the > first request. If that doesn't finish writing, use Random Program > Load > on subsequent data loading. > -- > Regards, > Chuanhong Guo I observe that exec_op() in drivers/spi/spi-mt65xx.c on our platform will issue the following commands once I limit its capability in 1024Bytes: (try to write 1 page) - 34H - addr - 1020KB (1024KB - 1 byte opcode - 3 bytes addr) - 34H - addr - 1020KB (1024KB - 1 byte opcode - 3 bytes addr) - 34H - addr - 72KB Is it possible to send 34H, 1K, 1K with current nbytes loop in spinand_write_to_cache_op()? With which SPI controller? I submit a patch according to this discussion, anyway. https://lore.kernel.org/linux-mtd/20241119093949.3014-1-SkyLake.Huang@mediatek.com/ BRs, Sky
Hello, > Several SPI-MEM controller don't allow software controlled CS, or for some > other reasons are unable to execute a longer spi-mem op. > spi_mem_dirmap_write returns the actual request size it's able to make, > and spinand_write_to_cache_op use a while loop to split one update_cache > request into multiple ones. > > This only works using the Random Program Load instruction (84h/34h) > which preserves the previous written data in the flash data cache. > All current supported flashes are exclusively using 84h/34h as the command > to write the page cache. > > Load Program Data(02h/32h) will clear the entire page cache. As a > result, when a request is split as described above, the previous written > data will be cleared, breaking the page cache writing procedure. > > We can change write_to_cache_op to use Load Program Data on the > first request. If that doesn't finish writing, use Random Program Load > on subsequent data loading. Sounds reasonable indeed, feel free to send a patch. Thanks, Miquèl
© 2016 - 2024 Red Hat, Inc.