SDK fixes for BL702 (EFUSE, USB, ADC, etc.) #145

mdednev · 2023-05-13T09:41:36Z

Hello,

We have got lot of issues with recent bouffalo_sdk for BL702 MCU especially with EFUSE support, clocking, USB, ADC and compilation warnings. This issues were fixed and extensively tested on custom BL702 board and with XT-ZB1 evaluation board.
Please, make review and pull changes to upstream SDK version (v2.0).

…is set

match original bl_mcu_sdk/bl_io_sdk MAC EFUSE data formats (byte order, parity, etc.)

…values for ADC data to match hardware properties.

…lity and interoperability with different USB hosts. Recent implementation had issues, leading to improper received data sizes due to the invalid USB DC flags handling. This behaviour sometimes break enumeration or control HID transactions.

…yte.

sakumisu · 2023-05-13T15:24:05Z

drivers/lhal/src/bflb_ef_ctrl.c

-#ifndef BOOTROM
-#define EF_CTRL_LOAD_BEFORE_READ_R0 bflb_ef_ctrl_load_efuse_r0(dev)
-#define EF_CTRL_LOAD_BEFORE_READ_R1 bflb_ef_ctrl_load_efuse_r1(dev)
+#ifndef BFLB_USE_ROM_DRIVER


efuse can not use romdriver.

Are you sure? As I could see, original bl_mcu_sdk (look at the V1.4.5 Makefile) uses romdriver by default:

option config to use SUPPORT_ROMAPI?=y ... skipped to be short ... cmake_definition+= -DCONFIG_ROMAPI=$(SUPPORT_ROMAPI)

and drivers/bl702_driver/CMakeLists.txt has following lines of code:

#add components denpend on this component if(CONFIG_ROMAPI) list(APPEND ADD_DEFINITIONS -DBFLB_USE_ROM_DRIVER) endif()

I've tested my implementation with romdriver and it works perfectly, no issues were detected. EFUSE values were read properly and read results are valid especially for BLE MAC address values, which are exactly the same as the values, which were read by applications build with V1.4.5 branch of bl_mcu_sdk.

Are you sure? As I could see, original bl_mcu_sdk (look at the V1.4.5 Makefile) uses romdriver by default:

option config to use SUPPORT_ROMAPI?=y ... skipped to be short ... cmake_definition+= -DCONFIG_ROMAPI=$(SUPPORT_ROMAPI)

and drivers/bl702_driver/CMakeLists.txt has following lines of code:

#add components denpend on this component if(CONFIG_ROMAPI) list(APPEND ADD_DEFINITIONS -DBFLB_USE_ROM_DRIVER) endif()

I've tested my implementation with romdriver and it works perfectly, no issues were detected. EFUSE values were read properly and read results are valid especially for BLE MAC address values, which are exactly the same as the values, which were read by applications build with V1.4.5 branch of bl_mcu_sdk.

sure, this is new sdk,in order to make driver common, give up efuse rom driver

Common driver is good reason, but ROM driver helps to save space in flash and I suppose it should be used as much as possible. At the same time there is no open documentation for EFUSE controller registers in provided BL702 reference manual and how could we could trust/verify/modify any EFUSE driver implementation? So using of the ROM driver seems to be more reliable decision in case of EFUSE. Provided patch allows user to choose what driver to use: ROM or SDK. And if in some future time we'll get some issues with ROM drivers we could safely turn to use SDK driver without any modifications of application code. This is common and generic way, already used in SDK for many driver API implementations. But what is your proposal regarding provided patch?

sakumisu · 2023-05-13T15:26:46Z

drivers/lhal/src/bflb_usb_v1.c

@@ -266,6 +315,10 @@ int usb_dc_init(void)

    regval = 0;
    regval |= USB_CR_USB_RESET_EN;
+    /* CMD_* interrupts added to enabled set. They are needed in advanced bflb_usb_ep_set_ready() logic */
+    regval |= USB_CR_EP0_SETUP_CMD_EN;


disable these irqs because too many irq enter and leave.

BL702 USB device controller EP0 hardware interface requires this interrupt to allow proper USB control transaction processing, especially control IN. In this case host sends USB SETUP request to device (handled properly), BL702 device gets IN token and sends requested data (handled properly) and after this hosts send short OUT status packet to the device. Sometimes it is followed by next SETUP if we use fast USB Host or it is intensive USB HID data exchange. BL702 has only one common FIFO with single data size register and due to this restrictions software driver can't properly handle two consecutive data transfers (OUT closely followed by SETUP) from USB Host to the BL702 USB device. So in this case we some times will get SETUP xfer data length while trying to process short OUT packet in status stage of first USB transaction, closely followed by next control transfer. It's hardware race condition. Enabling EP0 CMD interrupts makes possible to detect such condition and handle it properly. At the same time I think that it is not problem and could not make too many IRQ enter/leave, cause USB control transfer is supposed to be slow and uses relatively rare (f.e. USB HID, etc.). We've implemented HID driver over EP0 (control transfer) and it couldn't perform properly without provided SDK modifications.

sakumisu · 2023-05-13T15:30:02Z

components/usb/cherryusb/class/hid/usbd_hid.c

@@ -17,8 +17,7 @@ static int hid_class_interface_request_handler(struct usb_setup_packet *setup, u
    switch (setup->bRequest) {
        case HID_REQUEST_GET_REPORT:
            /* report id ,report type */
-            (*data)[0] = usbh_hid_get_report(intf_num, LO_BYTE(setup->wValue), HI_BYTE(setup->wValue));
-            *len = 1;
+			usbh_hid_get_report(intf_num, LO_BYTE(setup->wValue), HI_BYTE(setup->wValue), data, len);


must use return value

We can't process long GET_REPORT requests while using return value. So the only way is to pass data and len pointers to the application usbh_hid_get_report() function. This issue present in original Cherry USB implementation, but we could change this in bouffalo_sdk to implement proper HID requests handling.

sakumisu · 2023-05-15T03:15:27Z

drivers/soc/bl702/std/src/bl702_ef_cfg.c

    HBN_Set_ROOT_CLK_Sel(HBN_ROOT_CLK_RC32M);
-    GLB_Set_System_CLK_Div(0, 0);


cpu clock need restore to 32M.

CPU clock is save from GLB function, but restored to HBN and this is different clocks. In my case this leads to using 32M RC clock after calling bflb_efuse_switch_cpu_clock_restore() and this breaks BLE RF interaction, cause it needs very stable clock from 32M external XTAL. Check this condition and you will see.

sakumisu · 2023-05-15T03:24:15Z

drivers/soc/bl702/std/src/bl702_ef_cfg.c

@@ -168,8 +179,8 @@ uint8_t bflb_efuse_is_mac_address_slot_empty(uint8_t slot, uint8_t reload)
        bflb_ef_ctrl_read_direct(NULL, EF_DATA_EF_DBG_PWD_HIGH_OFFSET, &tmp2, 1, reload);
    }

-    part1Empty = (bflb_ef_ctrl_is_all_bits_zero(tmp1, 0, 32));
-    part2Empty = (bflb_ef_ctrl_is_all_bits_zero(tmp2, 0, 22));
+    part1Empty = (bflb_ef_ctrl_is_all_bits_zero(tmp1, 16, 16));


following old mcu sdk(0,32) is right.

Ok. But not only parity bits are wrong in new SDK LHAL, but also EFUSE offsets (register addresses) are also should be corrected to match legacy bl_mcu_sdk and bl_io_sdk EFUSE address mappings.

sakumisu · 2023-05-15T03:26:54Z

drivers/lhal/include/bflb_adc.h

@@ -40,6 +40,7 @@
 /** @defgroup ADC_CLK_DIV adc clock divison definition
  * @{
  */
+#define ADC_CLK_DIV_1  0


give up div1

Please, explain. This is valid divider value for ADC clock and I'm using it in my project.

adc value is not inaccuracy in div1 in some cases.

Yes, of course. But ADC speed should match external signal driver impedance to get proper signal sampling. So, it is up to board and software developer to select proper divider value for their signal parameters to get accurate ADC sampling and reading. So I suppose, that we should not limit this parameter in SDK if it is allowed in datasheet and reference manual and there is no restrictions in errata.

For div1, we will remove in datasheet.

sakumisu · 2023-05-15T03:30:41Z

drivers/soc/bl702/std/src/bl702_glb.c

@@ -150,6 +150,7 @@ GLB_ROOT_CLK_Type ATTR_CLOCK_SECTION GLB_Get_Root_CLK_Sel(void)
 * @return SUCCESS or ERROR
 *
 *******************************************************************************/
+#ifndef BFLB_USE_ROM_DRIVER
 BL_Err_Type ATTR_CLOCK_SECTION GLB_Set_System_CLK_Div(uint8_t hclkDiv, uint8_t bclkDiv)


this api cannot use romdirver

But it works in presented implementation and in legacy SDK (v1.4.5). Is the bootrom API implementation is totally broken?
I can't find any errata regarding boot rom code. Do you have such document?

not provide.

and this api has disabled in romapi.c

I can't understand such canceling of RomAPI without any reasonable argument, f.e. errata or other similar documentation from manufacturer. Especially if it works properly on different boards and in different conditions. 🤦‍♂️

sakumisu · 2023-05-17T01:53:21Z

Thanks for you effort to point out sdk issues, i will spend sometime to check them and merge into our internal repo.If some commits can be cherry-picked, i will pick them, others will be modified by manual.

…ull-up/pull-down/smt and input enable modes.

sakumisu · 2023-05-18T15:07:57Z

drivers/lhal/src/bflb_gpio.c

@@ -86,6 +104,40 @@ void bflb_gpio_init(struct bflb_device_s *dev, uint8_t pin, uint32_t cfgset)
        cfg |= (1 << (is_odd * 16 + 15));
    }
 #endif
+    /* always on pads IE control (in HBN) */


lhal gpio is for common driver, so these give up, please use soc api GLB_GPIO_Init for these gpio.

No, it is not common, because it has many many ifdefs in it for various MCU models and kinds. So I'm sure, that bflb_gpio_init() should match GLB_GPIO_Init() in case of GLB_GPIO_USE_PSRAM__IO handling.

iamdavidcz · 2023-05-25T21:33:27Z

@mdednev
With your patch the basic blink example finally started to work for me on Sipeed M0Sense (BL702), so thank you very much :)
I hope that it will find a way to master one day...

@sakumisu
Is there any specific reason why you won't provide us with ROM API documentation?

gamelaster · 2023-05-26T05:11:36Z

@iamdavidcz as far as I remember, all functions in ROM are also implemented in SDK.

mdednev · 2023-05-26T05:27:26Z

@iamdavidcz as far as I remember, all functions in ROM are also implemented in SDK.

Yes, they are supposed to be implemented in SDK, but there is two questions:

Is their SDK implementation the same as ROM one? And is it so tested and functions as ROM one?
Should we spend user flash on SDK function implementation while this implementation is already in MCU ROM?

I think the answers are 'no' and 'no'. And at this time I've chosen ROM implementation.

mdednev · 2023-05-26T05:30:57Z

With your patch the basic blink example finally started to work for me on Sipeed M0Sense (BL702), so thank you very much :)
I hope that it will find a way to master one day...

I hope so, because I've got next patch set for BLE stack port integration from bl_io_sdk to new SDK 2.0, but it can't perform successfully without patches, provided in this PR. I've tested this port and it works seamlessly.

sakumisu · 2023-05-26T05:55:06Z

1, All the rom api has corresponding source code, we just disable some romapi and patch them in source code.
2, Some romapi has bugs, so if you use romapi, the result you should consider.

sakumisu · 2023-05-26T06:01:08Z

drivers/soc/bl702/std/startup/system_bl702.c


    for (i = 0; i < (IRQn_LAST + 3) / 4; i++) {
        p[i] = 0;
    }

-    p = (uint32_t *)(CLIC_HART0_BASE + CLIC_INTIP_OFFSET);
+    p = (uint8_t *)(CLIC_HART0_BASE + CLIC_INTIP_OFFSET);


uint8_t is incorrect because with for (i = 0; i < (IRQn_LAST + 3) / 4; i++)

From e24 RISC-V core description:

registers are 1B width and address is computed as CLIC Hart Base + 1 × Interrupt ID, but as I see now the /4 devision should be removed. So cycle should be for (i = 0; i < IRQn_LAST; i++). I've missed this while making a patch.

Use uint32_t to make it quickly.

uint32_t usage is incorrect, cause it will produce 4B aligned addresses while accessing pointer as array. CLIC registers are byte aligned, not word aligned and so their pointer type should be uint8_t *. Please look at provided figures.

@mdednev CLIC_INTIE_OFFSET and CLIC_INTIP_OFFSET are both 4 byte aligned. the number of registers is evenly divisible by 4. they're all being set to 0 anyway.
As long as the cpu supports writing 32bit values here the original code should do the correct thing, right?
Is there something else here that we're not seeing?

@mdednev CLIC_INTIE_OFFSET and CLIC_INTIP_OFFSET are both 4 byte aligned.

Yes, this is right.

the number of registers is evenly divisible by 4.

It depends on implementation and it isn't true in general. In other words this is error-prone way. So I've suggested more general and error-proof initialization code. Yes, it is little-bit slower, but it is more correct and matches e24 core registers description (1B registers should be accessed as 1B and not as 4B). For example SDK IRQ driver uses 1B accesses to this registers (drivers/lhal/src/bflb_irq.c):

void bflb_irq_disable(int irq) { #if defined(BL702) || defined(BL602) || defined(BL702L) putreg8(0, CLIC_HART0_BASE + CLIC_INTIE_OFFSET + irq); #else csi_vic_disable_irq(irq); #endif }

they're all being set to 0 anyway. As long as the cpu supports writing 32bit values here the original code should do the correct thing, right? Is there something else here that we're not seeing?

Yes, this code works, but if it will be reused for future MCUs it could lead to errors in initialization.

mdednev · 2023-05-26T06:01:43Z

1, All the rom api has corresponding source code, we just disable some romapi and patch them in source code.

Do you have original ROM API source code to patch it and reimplement in SDK?

2, Some romapi has bugs, so if you use romapi, the result you should consider.

Could you provide some examples of such bugs? I want to try it with my firmware, based on ROM API.

sakumisu · 2023-05-26T06:06:01Z

drivers/lhal/src/bflb_gpio.c

@@ -26,16 +29,31 @@ void bflb_gpio_init(struct bflb_device_s *dev, uint8_t pin, uint32_t cfgset)
 #endif

 #if defined(BL702) || defined(BL602) || defined(BL702L)


Only for bl702

BL602 and BL702L have no such register.

There is no documentation for this register in BL702 RM also. ;-)
I'll correct this and add ifdef to skip this register checking for BL602/BL702L.

sakumisu · 2023-05-26T06:11:01Z

Some changes i will pick and others need be modified correctly( these i will modify in sdk without picking your commits).

sakumisu · 2023-05-26T06:15:15Z

1, All the rom api has corresponding source code, we just disable some romapi and patch them in source code.

Do you have original ROM API source code to patch it and reimplement in SDK?

2, Some romapi has bugs, so if you use romapi, the result you should consider.

Could you provide some examples of such bugs? I want to try it with my firmware, based on ROM API.

1, You can search the same api in romapi.c and other file like bl702_glb.c, bl702_hbn.c and so on.
2, Other chips we provide romapi_patch.c with source code and disable them in romapi.c, for bl702 and bl602, you can see source code in other c files, follow 1.

sakumisu · 2023-05-26T06:18:04Z

These romapi patches have synced in iot sdk and bouffalo_sdk, so cannot enable some of them. This will cause different romapi versions.

…02 build

sakumisu · 2023-06-28T02:34:02Z

i2c cannot work in our all chips by your change, so i revert this commit.

gamelaster · 2023-06-28T05:24:40Z

@sakumisu can't be then this change applied only for BL70X?

sakumisu · 2023-06-28T06:12:21Z

@sakumisu can't be then this change applied only for BL70X?

Not work on bl702 also.

mdednev · 2023-06-28T06:22:04Z

@sakumisu can't be then this change applied only for BL70X?

Not work on bl702 also.

I've checked this code and it works perfectly with unresponsive devices. In contrast original SDK driver hangs without timeout error.

NAK condition is one of the valid cases in I2C bus interaction, regarding to the I2C bus specification. So it should be explicitly handled in properly written driver.

sakumisu · 2023-06-28T06:24:52Z

@sakumisu can't be then this change applied only for BL70X?

Not work on bl702 also.

I've checked this code and it works perfectly with unresponsive devices. In contrast original SDK driver hangs without timeout error.

NAK condition is one of the valid cases in I2C bus interaction, regarding to the I2C bus specification. So it should be explicitly handled in properly written driver.

But we use three image sensor, all of them fail.

sakumisu · 2023-06-28T06:26:03Z

If you think it is ok, pick into your own repo.

mdednev · 2023-06-28T06:37:47Z

But we use three image sensor, all of them fail.

How it fails? Could you provide more information on this failures?
Did you try to check, which added NAK condition check causes failure?

If you think it is ok, pick into your own repo.

Of course I can, but I want to get reliable I2C driver for all of us. :-)

Max A. Dednev added 14 commits May 13, 2023 12:10

Fix CLIC register sizes

b6199b2

Add callback for low-level initialization before SystemInit

ae8f6e5

Add required functions to get current clock settings

53c0ef8

Fix compilation warning

fe0e015

Use GLB_Set_System_CLK_Div ROM implementation if BFLB_USE_ROM_DRIVER …

b0fd101

…is set

Fix compilation warnings

4d8363c

Fix EFUSE API functions to use proper EFUSE value addresses and

46f5981

match original bl_mcu_sdk/bl_io_sdk MAC EFUSE data formats (byte order, parity, etc.)

Fix invalid double inclusion definition macro

e559bfd

Implement proper I2C bus NAK condition handling for unresponsive devices

95e5934

Add missing ADC clock divider definition

1dfb247

Fix bflb_adc_channel_config() function prototype and correct maximum …

725478b

…values for ADC data to match hardware properties.

Fix compilation warnings

153ba83

Add support for GET_REPORT HID requests with lengths greater than 1 b…

597077a

…yte.

sakumisu reviewed May 13, 2023

View reviewed changes

sakumisu reviewed May 15, 2023

View reviewed changes

Fix for GPIO9...GPI13 and GPIO23...GPI28 parameters initialization: p…

fc313fe

…ull-up/pull-down/smt and input enable modes.

sakumisu reviewed May 18, 2023

View reviewed changes

sakumisu reviewed May 26, 2023

View reviewed changes

sakumisu approved these changes May 26, 2023

View reviewed changes

sakumisu reviewed May 26, 2023

View reviewed changes

sakumisu approved these changes May 26, 2023

View reviewed changes

Max A. Dednev added 3 commits May 26, 2023 09:20

Corrected cycle ranges to get all CLIC registers initialized.

d6c2771

Restricted GLB_GPIO_USE_PSRAM__IO_OFFSET_ADDR processing only for BL7…

638ded6

…02 build

Fix using register value instead of it's address.

ba2efc7

sakumisue force-pushed the master branch 2 times, most recently from 60e4156 to de40c1e Compare August 18, 2023 02:40

sakumisue force-pushed the master branch 2 times, most recently from c037b71 to be623ce Compare February 6, 2024 09:23

		HBN_Set_ROOT_CLK_Sel(HBN_ROOT_CLK_RC32M);
		GLB_Set_System_CLK_Div(0, 0);

		@@ -26,16 +29,31 @@ void bflb_gpio_init(struct bflb_device_s *dev, uint8_t pin, uint32_t cfgset)
		#endif

		#if defined(BL702) \|\| defined(BL602) \|\| defined(BL702L)

SDK fixes for BL702 (EFUSE, USB, ADC, etc.) #145

Are you sure you want to change the base?

SDK fixes for BL702 (EFUSE, USB, ADC, etc.) #145

Conversation

mdednev commented May 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdednev May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakumisu commented May 17, 2023

Choose a reason for hiding this comment

mdednev May 18, 2023 • edited Loading

Choose a reason for hiding this comment

iamdavidcz commented May 25, 2023

gamelaster commented May 26, 2023

mdednev commented May 26, 2023

mdednev commented May 26, 2023 • edited Loading

sakumisu commented May 26, 2023

Choose a reason for hiding this comment

mdednev May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdednev commented May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakumisu commented May 26, 2023 • edited Loading

sakumisu commented May 26, 2023

sakumisu commented May 26, 2023 • edited Loading

sakumisu commented Jun 28, 2023

gamelaster commented Jun 28, 2023

sakumisu commented Jun 28, 2023

mdednev commented Jun 28, 2023

sakumisu commented Jun 28, 2023

sakumisu commented Jun 28, 2023

mdednev commented Jun 28, 2023 • edited Loading

mdednev commented May 13, 2023 •

edited

Loading

mdednev May 16, 2023 •

edited

Loading

mdednev May 18, 2023 •

edited

Loading

mdednev commented May 26, 2023 •

edited

Loading

mdednev May 26, 2023 •

edited

Loading

mdednev commented May 26, 2023 •

edited

Loading

sakumisu commented May 26, 2023 •

edited

Loading

sakumisu commented May 26, 2023 •

edited

Loading

mdednev commented Jun 28, 2023 •

edited

Loading