Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spi flash fix #5948

Merged
merged 4 commits into from
Feb 2, 2022
Merged

Spi flash fix #5948

merged 4 commits into from
Feb 2, 2022

Conversation

EmergReanimator
Copy link

@EmergReanimator EmergReanimator commented Jan 30, 2022

Tested with STM32 F4VE board

https://stm32-base.org/boards/STM32F407VET6-STM32-F4VE-V2.0.html

Re-configuration the SPI on STM32F407 causes SCK signal to toggle.
This leads to Fast ReaD (0x0B) command failure in check_fs: 0x0B is treated as 0x85
image

  /* Find an FAT partition on the drive. Supports only generic partitioning rules, FDISK (MBR) and SFD (w/o partition). */
    bsect = 0;
    fmt = check_fs(fs, bsect);          /* Load sector 0 and check if it is an FAT-VBR as SFD */
    if (fmt == 2 || (fmt < 2 && LD2PT(fs) != 0)) { /* Not an FAT-VBR or forced partition number */
        for (i = 0; i < 4; i++) {       /* Get partition offset */
            pt = fs->win + (MBR_Table + i * SZ_PTE);
            br[i] = pt[PTE_System] ? ld_dword(pt + PTE_StLba) : 0;
        }
        i = LD2PT(fs);                  /* Partition number: 0:auto, 1-4:forced */
        if (i != 0) i--;
        do {                            /* Find an FAT volume */
            bsect = br[i];
            fmt = bsect ? check_fs(fs, bsect) : 3;  /* Check the partition */
        } while (LD2PT(fs) == 0 && fmt >= 2 && ++i < 4);
    }
    if (fmt == 4) return FR_DISK_ERR;       /* An error occured in the disk I/O layer */
    if (fmt >= 2) return FR_NO_FILESYSTEM;  /* No FAT volume is found */

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you want to do this. Removing this configures will break cases where the flash is on a shared SPI bus with a device that has different settings. This is rare but possible.

Instead, the port should guard against repeated calls and exit early. https://github.com/adafruit/circuitpython/blob/main/ports/atmel-samd/common-hal/busio/SPI.c#L216-L222 This protects the SPI from similar issues when used by SPIDevice which also calls configure before each transaction.

…/write_data

Removing this configures will break cases where the flash is on a shared SPI bus with a device that has different settings.
This is rare but possible.
@EmergReanimator
Copy link
Author

EmergReanimator commented Feb 1, 2022

Removing this configures will break cases where the flash is on a shared SPI bus with a device that has different settings. This is rare but possible.

I see. I also was not sure about this change. But I was not aware that shared SPI bus use cases are possible with circuitpython.

Instead, the port should guard against repeated calls and exit early. https://github.com/adafruit/circuitpython/blob/main/ports/atmel-samd/common-hal/busio/SPI.c#L216-L222 This protects the SPI from similar issues when used by SPIDevice which also calls configure before each transaction.

Actually STM port does check the SPI configuration change before altering SPI state.
https://github.com/adafruit/circuitpython/blob/main/ports/stm/common-hal/busio/SPI.c#L279
The thing is that common_hal_busio_spi_construct sets default SPI frequency (baudrate) that differs from the SPI flash settings. And the first call of spi_flash_read_data made out from check_fs causes the issue.

I have added common_hal_busio_spi_configure back to spi_flash_read_data and spi_flash_write_data. But they are called before chip select signal is activated now. Moreover I have added common_hal_busio_spi_configure to spi_flash_init_device.

@tannewt
Copy link
Member

tannewt commented Feb 2, 2022

The thing is that common_hal_busio_spi_construct sets default SPI frequency (baudrate) that differs from the SPI flash settings.

We're careful with the starting baudrate due to picky SD cards. That's where the 250k (IIRC) comes from.

I have added common_hal_busio_spi_configure back to spi_flash_read_data and spi_flash_write_data. But they are called before chip select signal is activated now. Moreover I have added common_hal_busio_spi_configure to spi_flash_init_device.

That sounds perfect!

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thank you!

@tannewt tannewt merged commit f437518 into adafruit:main Feb 2, 2022
@EmergReanimator EmergReanimator deleted the spi_flash_fix branch February 3, 2022 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants