mbox series

[RFC,00/16] Introduce ICSSG Ethernet driver

Message ID 20231219101225.3443978-1-danishanwar@ti.com
Headers show
Series Introduce ICSSG Ethernet driver | expand

Message

MD Danish Anwar Dec. 19, 2023, 10:11 a.m. UTC
Introduce ICSSG PRUETH support in uboot. The ICSSG driver is used in TI
AM654 SR2.0.

The ICSSG PRU Sub-system runs on EMAC firmware. This series Introduces
support for ICSSG driver in uboot. This series also adds the driver's
dependencies.

The ICSSG2 node is added in device tree overlay so that it remains in
sync with linux kernel.

The series introduces device tree and config changes and AM65x
to enable ICSSG driver. The series also enables SPL_LOAD_FIT_APPLY_OVERLAY
for AM65x in order to load overlay over spl.

This series has been tested on AM65x SR2.0, and the ICSSG interface is
able to ping / dhcp and boot kernel using tftp in uboot.

To use ICSSG2 ethernet, the ICSSG firmware needs to be loaded to PRU RPROC
cores and RPROC cores need to be booted with the firmware. This step is
done inside driver in kernel as kernel supports APIs like
rproc_set_firmware() and rproc_fw_boot(). But as u-boot doesn't have these
APIs, the same needs to be done via u-boot cmds.

To make sure icssg-eth works we need to do below steps.

1. Initialize rproc cores i.e. rproc_init()
2. Load $firmware_file from partition '1:2' (root) on device (mmc in this
   example)
3. Load the firmware file to rproc cores passing. i.e. rproc_load()
   taking rproc_id, loadaddr and file size as arguments.
4. Start rproc cores. i.e. rproc_start() taking rproc_id as arguments

The above steps are done by running the below commands at u-boot prompt.

=> setenv start_icssg2 'rproc start 14; rproc start 15; rproc start 16; rproc start 17; rproc start 18; rproc start 19'
=> setenv stop_icssg2 'rproc stop 14; rproc stop 15; rproc stop 16; rproc stop 17; rproc stop 18; rproc stop 19'
=> setenv firmware_dir '/lib/firmware/ti-pruss'
=> setenv get_firmware_mmc 'load mmc ${bootpart} ${loadaddr} ${firmware_dir}/${firmware_file}'

=> setenv init_icssg2 'setenv ethact icssg2-eth; setenv autoload no; rproc init; setenv loadaddr 0x80000000; \
    setenv firmware_file am65x-sr2-pru0-prueth-fw.elf; run get_firmware_mmc;  rproc load 14 0x80000000 ${filesize}; \
    setenv loadaddr 0x89000000; setenv firmware_file am65x-sr2-rtu0-prueth-fw.elf; run get_firmware_mmc; rproc load 15 0x89000000 ${filesize}; \
    setenv loadaddr 0x90000000; setenv firmware_file am65x-sr2-txpru0-prueth-fw.elf; run get_firmware_mmc; rproc load 16 0x90000000 ${filesize}; \
    setenv loadaddr 0x80000000; setenv firmware_file am65x-sr2-pru1-prueth-fw.elf; run get_firmware_mmc; rproc load 17 0x80000000 ${filesize}; \
    setenv loadaddr 0x89000000; setenv firmware_file am65x-sr2-rtu1-prueth-fw.elf; run get_firmware_mmc; rproc load 18 0x89000000 ${filesize}; \
    setenv loadaddr 0x90000000; setenv firmware_file am65x-sr2-txpru1-prueth-fw.elf; run get_firmware_mmc; rproc load 19 0x90000000 ${filesize}; \
    run start_icssg2;'

=> run init_icssg2
=> dhcp
k3-navss-ringacc ringacc@3c000000: Ring Accelerator probed rings:818, gp-rings[304,100] sci-dev-id:187
k3-navss-ringacc ringacc@3c000000: dma-ring-reset-quirk: disabled
prueth icssg2-eth: K3 ICSSG: rflow_id_base: 8, chn_name = rx0
link up on port 0, speed 1000, full duplex
BOOTP broadcast 1
BOOTP broadcast 2
BOOTP broadcast 3
DHCP client bound to address 192.168.4.58 (1020 ms)

Thanks and Regards,
MD Danish Anwar

MD Danish Anwar (16):
  net: ti: icssg: Add Firmware Interface for ICSSG Ethernet driver.
  net: ti: icssg: Add Firmware config and classification APIs.
  net: ti: icssg: Add icssg queues APIs and macros
  net: ti: icssg: Add ICSSG ethernet driver
  net: ti: icssg: Add support sending FDB command to update rx_flow_id
  net: ti: icssg: Enforce pinctrl state on the MDIO child node
  arm: dts: k3-am65: Add additional regs for DMA components
  arm: dts: k3-am65: Add cfg reg region to ringacc node
  arm: dts: k3-am65-main: Add ICSSG IEP nodes
  arm: dts: k3-am654-base-board: Add ICSSG2 Ethernet support
  arm: dts: k3-am65x-binman: Add ICSSG2 overlay and configuration
  configs: am65x_evm_a53: Enable ICSSG Driver
  configs: am65x_evm_a53_defconfig: add SPL_LOAD_FIT_APPLY_OVERLAY
  tools/fdtgrep: Include __symbols__ table
  board: ti: am65x: Add check for k3-am654-icssg2 in
    board_fit_config_match()
  Revert "dm: core: Report bootph-pre-ram/sram node as pre-reloc after
    relocation"

 arch/arm/dts/Makefile             |   3 +-
 arch/arm/dts/k3-am65-main.dtsi    |  49 ++-
 arch/arm/dts/k3-am65-mcu.dtsi     |  13 +-
 arch/arm/dts/k3-am654-icssg2.dtso | 145 +++++++
 arch/arm/dts/k3-am65x-binman.dtsi |  85 ++++
 board/ti/am65x/evm.c              |  11 +-
 configs/am65x_evm_a53_defconfig   |   4 +
 drivers/core/ofnode.c             |   2 +-
 drivers/net/ti/Kconfig            |   9 +
 drivers/net/ti/Makefile           |   1 +
 drivers/net/ti/icss_mii_rt.h      | 192 +++++++++
 drivers/net/ti/icssg_classifier.c | 376 +++++++++++++++++
 drivers/net/ti/icssg_config.c     | 469 +++++++++++++++++++++
 drivers/net/ti/icssg_config.h     | 195 +++++++++
 drivers/net/ti/icssg_prueth.c     | 654 ++++++++++++++++++++++++++++++
 drivers/net/ti/icssg_prueth.h     |  89 ++++
 drivers/net/ti/icssg_queues.c     |  51 +++
 drivers/net/ti/icssg_switch_map.h | 209 ++++++++++
 include/dm/ofnode.h               |   8 +-
 tools/fdtgrep.c                   |   8 +
 20 files changed, 2555 insertions(+), 18 deletions(-)
 create mode 100644 arch/arm/dts/k3-am654-icssg2.dtso
 create mode 100644 drivers/net/ti/icss_mii_rt.h
 create mode 100644 drivers/net/ti/icssg_classifier.c
 create mode 100644 drivers/net/ti/icssg_config.c
 create mode 100644 drivers/net/ti/icssg_config.h
 create mode 100644 drivers/net/ti/icssg_prueth.c
 create mode 100644 drivers/net/ti/icssg_prueth.h
 create mode 100644 drivers/net/ti/icssg_queues.c
 create mode 100644 drivers/net/ti/icssg_switch_map.h

base:commit: a6f86132e30a407c7f96461df53c62fbe52e2b54

Comments

MD Danish Anwar Dec. 19, 2023, 10:30 a.m. UTC | #1
Hi All,

Please ignore this thread. Some mails seems to have been duplicated.
I will post another thread soon. Pls ignore this.
Sorry for the inconvenience.

On 19/12/23 3:41 pm, MD Danish Anwar wrote:
> Introduce ICSSG PRUETH support in uboot. The ICSSG driver is used in TI
> AM654 SR2.0.
> 
> The ICSSG PRU Sub-system runs on EMAC firmware. This series Introduces
> support for ICSSG driver in uboot. This series also adds the driver's
> dependencies.
> 
> The ICSSG2 node is added in device tree overlay so that it remains in
> sync with linux kernel.
> 
> The series introduces device tree and config changes and AM65x
> to enable ICSSG driver. The series also enables SPL_LOAD_FIT_APPLY_OVERLAY
> for AM65x in order to load overlay over spl.
> 
> This series has been tested on AM65x SR2.0, and the ICSSG interface is
> able to ping / dhcp and boot kernel using tftp in uboot.
> 
> To use ICSSG2 ethernet, the ICSSG firmware needs to be loaded to PRU RPROC
> cores and RPROC cores need to be booted with the firmware. This step is
> done inside driver in kernel as kernel supports APIs like
> rproc_set_firmware() and rproc_fw_boot(). But as u-boot doesn't have these
> APIs, the same needs to be done via u-boot cmds.
> 
> To make sure icssg-eth works we need to do below steps.
> 
> 1. Initialize rproc cores i.e. rproc_init()
> 2. Load $firmware_file from partition '1:2' (root) on device (mmc in this
>    example)
> 3. Load the firmware file to rproc cores passing. i.e. rproc_load()
>    taking rproc_id, loadaddr and file size as arguments.
> 4. Start rproc cores. i.e. rproc_start() taking rproc_id as arguments
> 
> The above steps are done by running the below commands at u-boot prompt.
> 
> => setenv start_icssg2 'rproc start 14; rproc start 15; rproc start 16; rproc start 17; rproc start 18; rproc start 19'
> => setenv stop_icssg2 'rproc stop 14; rproc stop 15; rproc stop 16; rproc stop 17; rproc stop 18; rproc stop 19'
> => setenv firmware_dir '/lib/firmware/ti-pruss'
> => setenv get_firmware_mmc 'load mmc ${bootpart} ${loadaddr} ${firmware_dir}/${firmware_file}'
> 
> => setenv init_icssg2 'setenv ethact icssg2-eth; setenv autoload no; rproc init; setenv loadaddr 0x80000000; \
>     setenv firmware_file am65x-sr2-pru0-prueth-fw.elf; run get_firmware_mmc;  rproc load 14 0x80000000 ${filesize}; \
>     setenv loadaddr 0x89000000; setenv firmware_file am65x-sr2-rtu0-prueth-fw.elf; run get_firmware_mmc; rproc load 15 0x89000000 ${filesize}; \
>     setenv loadaddr 0x90000000; setenv firmware_file am65x-sr2-txpru0-prueth-fw.elf; run get_firmware_mmc; rproc load 16 0x90000000 ${filesize}; \
>     setenv loadaddr 0x80000000; setenv firmware_file am65x-sr2-pru1-prueth-fw.elf; run get_firmware_mmc; rproc load 17 0x80000000 ${filesize}; \
>     setenv loadaddr 0x89000000; setenv firmware_file am65x-sr2-rtu1-prueth-fw.elf; run get_firmware_mmc; rproc load 18 0x89000000 ${filesize}; \
>     setenv loadaddr 0x90000000; setenv firmware_file am65x-sr2-txpru1-prueth-fw.elf; run get_firmware_mmc; rproc load 19 0x90000000 ${filesize}; \
>     run start_icssg2;'
> 
> => run init_icssg2
> => dhcp
> k3-navss-ringacc ringacc@3c000000: Ring Accelerator probed rings:818, gp-rings[304,100] sci-dev-id:187
> k3-navss-ringacc ringacc@3c000000: dma-ring-reset-quirk: disabled
> prueth icssg2-eth: K3 ICSSG: rflow_id_base: 8, chn_name = rx0
> link up on port 0, speed 1000, full duplex
> BOOTP broadcast 1
> BOOTP broadcast 2
> BOOTP broadcast 3
> DHCP client bound to address 192.168.4.58 (1020 ms)
> 
> Thanks and Regards,
> MD Danish Anwar
> 
> MD Danish Anwar (16):
>   net: ti: icssg: Add Firmware Interface for ICSSG Ethernet driver.
>   net: ti: icssg: Add Firmware config and classification APIs.
>   net: ti: icssg: Add icssg queues APIs and macros
>   net: ti: icssg: Add ICSSG ethernet driver
>   net: ti: icssg: Add support sending FDB command to update rx_flow_id
>   net: ti: icssg: Enforce pinctrl state on the MDIO child node
>   arm: dts: k3-am65: Add additional regs for DMA components
>   arm: dts: k3-am65: Add cfg reg region to ringacc node
>   arm: dts: k3-am65-main: Add ICSSG IEP nodes
>   arm: dts: k3-am654-base-board: Add ICSSG2 Ethernet support
>   arm: dts: k3-am65x-binman: Add ICSSG2 overlay and configuration
>   configs: am65x_evm_a53: Enable ICSSG Driver
>   configs: am65x_evm_a53_defconfig: add SPL_LOAD_FIT_APPLY_OVERLAY
>   tools/fdtgrep: Include __symbols__ table
>   board: ti: am65x: Add check for k3-am654-icssg2 in
>     board_fit_config_match()
>   Revert "dm: core: Report bootph-pre-ram/sram node as pre-reloc after
>     relocation"
> 
>  arch/arm/dts/Makefile             |   3 +-
>  arch/arm/dts/k3-am65-main.dtsi    |  49 ++-
>  arch/arm/dts/k3-am65-mcu.dtsi     |  13 +-
>  arch/arm/dts/k3-am654-icssg2.dtso | 145 +++++++
>  arch/arm/dts/k3-am65x-binman.dtsi |  85 ++++
>  board/ti/am65x/evm.c              |  11 +-
>  configs/am65x_evm_a53_defconfig   |   4 +
>  drivers/core/ofnode.c             |   2 +-
>  drivers/net/ti/Kconfig            |   9 +
>  drivers/net/ti/Makefile           |   1 +
>  drivers/net/ti/icss_mii_rt.h      | 192 +++++++++
>  drivers/net/ti/icssg_classifier.c | 376 +++++++++++++++++
>  drivers/net/ti/icssg_config.c     | 469 +++++++++++++++++++++
>  drivers/net/ti/icssg_config.h     | 195 +++++++++
>  drivers/net/ti/icssg_prueth.c     | 654 ++++++++++++++++++++++++++++++
>  drivers/net/ti/icssg_prueth.h     |  89 ++++
>  drivers/net/ti/icssg_queues.c     |  51 +++
>  drivers/net/ti/icssg_switch_map.h | 209 ++++++++++
>  include/dm/ofnode.h               |   8 +-
>  tools/fdtgrep.c                   |   8 +
>  20 files changed, 2555 insertions(+), 18 deletions(-)
>  create mode 100644 arch/arm/dts/k3-am654-icssg2.dtso
>  create mode 100644 drivers/net/ti/icss_mii_rt.h
>  create mode 100644 drivers/net/ti/icssg_classifier.c
>  create mode 100644 drivers/net/ti/icssg_config.c
>  create mode 100644 drivers/net/ti/icssg_config.h
>  create mode 100644 drivers/net/ti/icssg_prueth.c
>  create mode 100644 drivers/net/ti/icssg_prueth.h
>  create mode 100644 drivers/net/ti/icssg_queues.c
>  create mode 100644 drivers/net/ti/icssg_switch_map.h
> 
> base:commit: a6f86132e30a407c7f96461df53c62fbe52e2b54
Roger Quadros Dec. 20, 2023, 9:59 a.m. UTC | #2
Hi,

On 19/12/2023 12:11, MD Danish Anwar wrote:
> Introduce ICSSG PRUETH support in uboot. The ICSSG driver is used in TI
> AM654 SR2.0.
> 
> The ICSSG PRU Sub-system runs on EMAC firmware. This series Introduces
> support for ICSSG driver in uboot. This series also adds the driver's
> dependencies.
> 
> The ICSSG2 node is added in device tree overlay so that it remains in
> sync with linux kernel.
> 
> The series introduces device tree and config changes and AM65x
> to enable ICSSG driver. The series also enables SPL_LOAD_FIT_APPLY_OVERLAY
> for AM65x in order to load overlay over spl.
> 
> This series has been tested on AM65x SR2.0, and the ICSSG interface is
> able to ping / dhcp and boot kernel using tftp in uboot.
> 
> To use ICSSG2 ethernet, the ICSSG firmware needs to be loaded to PRU RPROC
> cores and RPROC cores need to be booted with the firmware. This step is
> done inside driver in kernel as kernel supports APIs like
> rproc_set_firmware() and rproc_fw_boot(). But as u-boot doesn't have these
> APIs, the same needs to be done via u-boot cmds.
> 
> To make sure icssg-eth works we need to do below steps.
> 
> 1. Initialize rproc cores i.e. rproc_init()
> 2. Load $firmware_file from partition '1:2' (root) on device (mmc in this
>    example)
> 3. Load the firmware file to rproc cores passing. i.e. rproc_load()
>    taking rproc_id, loadaddr and file size as arguments.
> 4. Start rproc cores. i.e. rproc_start() taking rproc_id as arguments
> 
> The above steps are done by running the below commands at u-boot prompt.
> 
> => setenv start_icssg2 'rproc start 14; rproc start 15; rproc start 16; rproc start 17; rproc start 18; rproc start 19'
> => setenv stop_icssg2 'rproc stop 14; rproc stop 15; rproc stop 16; rproc stop 17; rproc stop 18; rproc stop 19'
> => setenv firmware_dir '/lib/firmware/ti-pruss'
> => setenv get_firmware_mmc 'load mmc ${bootpart} ${loadaddr} ${firmware_dir}/${firmware_file}'
> 
> => setenv init_icssg2 'setenv ethact icssg2-eth; setenv autoload no; rproc init; setenv loadaddr 0x80000000; \
>     setenv firmware_file am65x-sr2-pru0-prueth-fw.elf; run get_firmware_mmc;  rproc load 14 0x80000000 ${filesize}; \
>     setenv loadaddr 0x89000000; setenv firmware_file am65x-sr2-rtu0-prueth-fw.elf; run get_firmware_mmc; rproc load 15 0x89000000 ${filesize}; \
>     setenv loadaddr 0x90000000; setenv firmware_file am65x-sr2-txpru0-prueth-fw.elf; run get_firmware_mmc; rproc load 16 0x90000000 ${filesize}; \
>     setenv loadaddr 0x80000000; setenv firmware_file am65x-sr2-pru1-prueth-fw.elf; run get_firmware_mmc; rproc load 17 0x80000000 ${filesize}; \
>     setenv loadaddr 0x89000000; setenv firmware_file am65x-sr2-rtu1-prueth-fw.elf; run get_firmware_mmc; rproc load 18 0x89000000 ${filesize}; \
>     setenv loadaddr 0x90000000; setenv firmware_file am65x-sr2-txpru1-prueth-fw.elf; run get_firmware_mmc; rproc load 19 0x90000000 ${filesize}; \
>     run start_icssg2;'

A whole bunch of commands are required to get ethernet functional.
This is not at all user friendly and will be a maintenance nightmare.
What worries me is tracking the 6 different rproc cores and the 6 different firmware files to start 1 ethernet device.
This will get even more interesting when we have to deal with different ICSSG instances on different boards.

What is preventing the driver from starting the rproc cores it needs so user doesn't need to care about it?
All the necessary information is in the Device tree. At least this is how it is done on Linux.

> 
> => run init_icssg2
> => dhcp
> k3-navss-ringacc ringacc@3c000000: Ring Accelerator probed rings:818, gp-rings[304,100] sci-dev-id:187
> k3-navss-ringacc ringacc@3c000000: dma-ring-reset-quirk: disabled
> prueth icssg2-eth: K3 ICSSG: rflow_id_base: 8, chn_name = rx0
> link up on port 0, speed 1000, full duplex
> BOOTP broadcast 1
> BOOTP broadcast 2
> BOOTP broadcast 3
> DHCP client bound to address 192.168.4.58 (1020 ms)
> 

What about shutting down the interface?
I believe this has to be done manually too?

> Thanks and Regards,
> MD Danish Anwar
> 
> MD Danish Anwar (16):
>   net: ti: icssg: Add Firmware Interface for ICSSG Ethernet driver.
>   net: ti: icssg: Add Firmware config and classification APIs.
>   net: ti: icssg: Add icssg queues APIs and macros
>   net: ti: icssg: Add ICSSG ethernet driver
>   net: ti: icssg: Add support sending FDB command to update rx_flow_id
>   net: ti: icssg: Enforce pinctrl state on the MDIO child node
>   arm: dts: k3-am65: Add additional regs for DMA components
>   arm: dts: k3-am65: Add cfg reg region to ringacc node
>   arm: dts: k3-am65-main: Add ICSSG IEP nodes
>   arm: dts: k3-am654-base-board: Add ICSSG2 Ethernet support
>   arm: dts: k3-am65x-binman: Add ICSSG2 overlay and configuration
>   configs: am65x_evm_a53: Enable ICSSG Driver
>   configs: am65x_evm_a53_defconfig: add SPL_LOAD_FIT_APPLY_OVERLAY
>   tools/fdtgrep: Include __symbols__ table
>   board: ti: am65x: Add check for k3-am654-icssg2 in
>     board_fit_config_match()
>   Revert "dm: core: Report bootph-pre-ram/sram node as pre-reloc after
>     relocation"
> 
>  arch/arm/dts/Makefile             |   3 +-
>  arch/arm/dts/k3-am65-main.dtsi    |  49 ++-
>  arch/arm/dts/k3-am65-mcu.dtsi     |  13 +-
>  arch/arm/dts/k3-am654-icssg2.dtso | 145 +++++++
>  arch/arm/dts/k3-am65x-binman.dtsi |  85 ++++
>  board/ti/am65x/evm.c              |  11 +-
>  configs/am65x_evm_a53_defconfig   |   4 +
>  drivers/core/ofnode.c             |   2 +-
>  drivers/net/ti/Kconfig            |   9 +
>  drivers/net/ti/Makefile           |   1 +
>  drivers/net/ti/icss_mii_rt.h      | 192 +++++++++
>  drivers/net/ti/icssg_classifier.c | 376 +++++++++++++++++
>  drivers/net/ti/icssg_config.c     | 469 +++++++++++++++++++++
>  drivers/net/ti/icssg_config.h     | 195 +++++++++
>  drivers/net/ti/icssg_prueth.c     | 654 ++++++++++++++++++++++++++++++
>  drivers/net/ti/icssg_prueth.h     |  89 ++++
>  drivers/net/ti/icssg_queues.c     |  51 +++
>  drivers/net/ti/icssg_switch_map.h | 209 ++++++++++
>  include/dm/ofnode.h               |   8 +-
>  tools/fdtgrep.c                   |   8 +
>  20 files changed, 2555 insertions(+), 18 deletions(-)
>  create mode 100644 arch/arm/dts/k3-am654-icssg2.dtso
>  create mode 100644 drivers/net/ti/icss_mii_rt.h
>  create mode 100644 drivers/net/ti/icssg_classifier.c
>  create mode 100644 drivers/net/ti/icssg_config.c
>  create mode 100644 drivers/net/ti/icssg_config.h
>  create mode 100644 drivers/net/ti/icssg_prueth.c
>  create mode 100644 drivers/net/ti/icssg_prueth.h
>  create mode 100644 drivers/net/ti/icssg_queues.c
>  create mode 100644 drivers/net/ti/icssg_switch_map.h
> 
> base:commit: a6f86132e30a407c7f96461df53c62fbe52e2b54
MD Danish Anwar Dec. 22, 2023, 10:26 a.m. UTC | #3
Hi Roger,

On 20/12/23 3:29 pm, Roger Quadros wrote:
> Hi,
> 
> On 19/12/2023 12:11, MD Danish Anwar wrote:
>> Introduce ICSSG PRUETH support in uboot. The ICSSG driver is used in TI
>> AM654 SR2.0.
>>
>> The ICSSG PRU Sub-system runs on EMAC firmware. This series Introduces
>> support for ICSSG driver in uboot. This series also adds the driver's
>> dependencies.
>>
>> The ICSSG2 node is added in device tree overlay so that it remains in
>> sync with linux kernel.
>>
>> The series introduces device tree and config changes and AM65x
>> to enable ICSSG driver. The series also enables SPL_LOAD_FIT_APPLY_OVERLAY
>> for AM65x in order to load overlay over spl.
>>
>> This series has been tested on AM65x SR2.0, and the ICSSG interface is
>> able to ping / dhcp and boot kernel using tftp in uboot.
>>
>> To use ICSSG2 ethernet, the ICSSG firmware needs to be loaded to PRU RPROC
>> cores and RPROC cores need to be booted with the firmware. This step is
>> done inside driver in kernel as kernel supports APIs like
>> rproc_set_firmware() and rproc_fw_boot(). But as u-boot doesn't have these
>> APIs, the same needs to be done via u-boot cmds.
>>
>> To make sure icssg-eth works we need to do below steps.
>>
>> 1. Initialize rproc cores i.e. rproc_init()
>> 2. Load $firmware_file from partition '1:2' (root) on device (mmc in this
>>    example)
>> 3. Load the firmware file to rproc cores passing. i.e. rproc_load()
>>    taking rproc_id, loadaddr and file size as arguments.
>> 4. Start rproc cores. i.e. rproc_start() taking rproc_id as arguments
>>
>> The above steps are done by running the below commands at u-boot prompt.
>>
>> => setenv start_icssg2 'rproc start 14; rproc start 15; rproc start 16; rproc start 17; rproc start 18; rproc start 19'
>> => setenv stop_icssg2 'rproc stop 14; rproc stop 15; rproc stop 16; rproc stop 17; rproc stop 18; rproc stop 19'
>> => setenv firmware_dir '/lib/firmware/ti-pruss'
>> => setenv get_firmware_mmc 'load mmc ${bootpart} ${loadaddr} ${firmware_dir}/${firmware_file}'
>>
>> => setenv init_icssg2 'setenv ethact icssg2-eth; setenv autoload no; rproc init; setenv loadaddr 0x80000000; \
>>     setenv firmware_file am65x-sr2-pru0-prueth-fw.elf; run get_firmware_mmc;  rproc load 14 0x80000000 ${filesize}; \
>>     setenv loadaddr 0x89000000; setenv firmware_file am65x-sr2-rtu0-prueth-fw.elf; run get_firmware_mmc; rproc load 15 0x89000000 ${filesize}; \
>>     setenv loadaddr 0x90000000; setenv firmware_file am65x-sr2-txpru0-prueth-fw.elf; run get_firmware_mmc; rproc load 16 0x90000000 ${filesize}; \
>>     setenv loadaddr 0x80000000; setenv firmware_file am65x-sr2-pru1-prueth-fw.elf; run get_firmware_mmc; rproc load 17 0x80000000 ${filesize}; \
>>     setenv loadaddr 0x89000000; setenv firmware_file am65x-sr2-rtu1-prueth-fw.elf; run get_firmware_mmc; rproc load 18 0x89000000 ${filesize}; \
>>     setenv loadaddr 0x90000000; setenv firmware_file am65x-sr2-txpru1-prueth-fw.elf; run get_firmware_mmc; rproc load 19 0x90000000 ${filesize}; \
>>     run start_icssg2;'
> 
> A whole bunch of commands are required to get ethernet functional.
> This is not at all user friendly and will be a maintenance nightmare.
> What worries me is tracking the 6 different rproc cores and the 6 different firmware files to start 1 ethernet device.
> This will get even more interesting when we have to deal with different ICSSG instances on different boards.
> 
> What is preventing the driver from starting the rproc cores it needs so user doesn't need to care about it?
> All the necessary information is in the Device tree. At least this is how it is done on Linux.
> 

I tried removing the need for these commands and implementing them
inside the driver only. I am able to load the firmware from driver using
the fs_loader API request_firmware_into_buf(). It requires changes to
dt. A DT node called fs-loader needs to be added also CONFIG_FS_LOADER
needs to enabled. In the DT node we need to specify the storage media
that we are using i.e. mmc, ospi, usb. It's upto user to modify the
storage media, the driver will take the media from DT and try to laod
firmware from their.

For loading firmwares to rproc cores, rproc_load() API is needed. Now
this API takes rproc_id, loadaddr and firmware_size as arguments.
loadaddr is fixed for all three pru cores. firmware_size is obtained
from request_firmware_into_buf() but I couldn't find a way to get the
rproc_id. For now based on the ICSSG instance and slice number I am
figuring out the rproc_id and passing it to rproc_load() and
rproc_start() APIs. Please let me know if you have any other / better
way of finding rproc_id.

Below is the entire diff to remove these commands and move their
functionality to driver. Please have a look and let me know if this is ok.


Save New Duplicate & Edit Just Text Twitter
diff --git a/arch/arm/dts/k3-am654-base-board.dts
b/arch/arm/dts/k3-am654-base-board.dts
index cfbcebfa37..c8da72e49c 100644
--- a/arch/arm/dts/k3-am654-base-board.dts
+++ b/arch/arm/dts/k3-am654-base-board.dts
@@ -16,6 +16,13 @@
 	chosen {
 		stdout-path = "serial2:115200n8";
 		bootargs = "earlycon=ns16550a,mmio32,0x02800000";
+		firmware-loader = <&fs_loader0>;
+	};
+
+	fs_loader0: fs-loader {
+		bootph-all;
+		compatible = "u-boot,fs-loader";
+		phandlepart = <&sdhci1 2>;
 	};

 	memory@80000000 {
diff --git a/configs/am65x_evm_a53_defconfig
b/configs/am65x_evm_a53_defconfig
index 2755d7082f..c53e938abb 100644
--- a/configs/am65x_evm_a53_defconfig
+++ b/configs/am65x_evm_a53_defconfig
@@ -112,6 +112,7 @@ CONFIG_DM_I2C_GPIO=y
 CONFIG_SYS_I2C_OMAP24XX=y
 CONFIG_DM_MAILBOX=y
 CONFIG_K3_SEC_PROXY=y
+CONFIG_FS_LOADER=y
 CONFIG_SUPPORT_EMMC_BOOT=y
 CONFIG_MMC_IO_VOLTAGE=y
 CONFIG_MMC_UHS_SUPPORT=y
diff --git a/configs/am65x_evm_r5_defconfig b/configs/am65x_evm_r5_defconfig
index b2f1e721b3..2d19935a41 100644
--- a/configs/am65x_evm_r5_defconfig
+++ b/configs/am65x_evm_r5_defconfig
@@ -98,6 +98,7 @@ CONFIG_I2C_SET_DEFAULT_BUS_NUM=y
 CONFIG_SYS_I2C_OMAP24XX=y
 CONFIG_DM_MAILBOX=y
 CONFIG_K3_SEC_PROXY=y
+CONFIG_FS_LOADER=y
 CONFIG_K3_AVS0=y
 CONFIG_SUPPORT_EMMC_BOOT=y
 CONFIG_MMC_HS200_SUPPORT=y
diff --git a/drivers/net/ti/icssg_prueth.c b/drivers/net/ti/icssg_prueth.c
index 40ad827e49..1c4edeb7b7 100644
--- a/drivers/net/ti/icssg_prueth.c
+++ b/drivers/net/ti/icssg_prueth.c
@@ -227,6 +227,10 @@ static int prueth_start(struct udevice *dev)
 	void *config;
 	int ret, i;

+	ret = icssg_start_pru_cores(dev);
+	if (ret)
+		return ret;
+
 	/* To differentiate channels for SLICE0 vs SLICE1 */
 	snprintf(chn_name, sizeof(chn_name), "tx%d-0", priv->slice);

@@ -355,9 +359,11 @@ static void prueth_stop(struct udevice *dev)
 	phy_shutdown(priv->phydev);

 	dma_disable(&priv->dma_tx);
-	dma_free(&priv->dma_tx);
-
 	dma_disable(&priv->dma_rx);
+
+	icssg_stop_pru_cores(dev);
+
+	dma_free(&priv->dma_tx);
 	dma_free(&priv->dma_rx);
 }

@@ -434,6 +440,181 @@ static const struct soc_attr k3_mdio_soc_data[] = {
 	{ /* sentinel */ },
 };

+struct icssg_firmware_load_address {
+	u32 pru;
+	u32 rtu;
+	u32 txpru;
+};
+
+struct icssg_firmwares {
+	char *pru;
+	char *rtu;
+	char *txpru;
+};
+
+static struct icssg_firmwares icssg_emac_firmwares[] = {
+	{
+		.pru = "/lib/firmware/ti-pruss/am65x-sr2-pru0-prueth-fw.elf",
+		.rtu = "/lib/firmware/ti-pruss/am65x-sr2-rtu0-prueth-fw.elf",
+		.txpru = "/lib/firmware/ti-pruss/am65x-sr2-txpru0-prueth-fw.elf",
+	},
+	{
+		.pru = "/lib/firmware/ti-pruss/am65x-sr2-pru1-prueth-fw.elf",
+		.rtu = "/lib/firmware/ti-pruss/am65x-sr2-rtu1-prueth-fw.elf",
+		.txpru = "/lib/firmware/ti-pruss/am65x-sr2-txpru1-prueth-fw.elf",
+	}
+};
+
+int load_firmware(char *name_fw, u32 *loadaddr)
+{
+	struct udevice *fsdev;
+	int size = 0;
+
+	if (!IS_ENABLED(CONFIG_FS_LOADER))
+		return -EINVAL;
+
+	if (!*loadaddr)
+		return -EINVAL;
+
+	if (!get_fs_loader(&fsdev))
+		size = request_firmware_into_buf(fsdev, name_fw, (void *)*loadaddr,
40524, 0);
+
+	return size;
+}
+
+static int icssg_get_instance(struct udevice *dev)
+{
+	if (!strcmp(dev->name, "icssg2-eth"))
+		return 2;
+	else if (!strcmp(dev->name, "icssg1-eth"))
+		return 1;
+	else if (!strcmp(dev->name, "icssg0-eth"))
+		return 0;
+
+	dev_err(dev, "Invalid icssg instance\n");
+	return -EINVAL;
+}
+
+static int icssg_get_pru_core_number(struct udevice *dev, int slice)
+{
+	int instance, num_r5_cores;
+
+	instance = icssg_get_instance(dev);
+	if (instance < 0)
+		return instance;
+
+	if (IS_ENABLED(CONFIG_REMOTEPROC_TI_K3_R5F))
+		num_r5_cores = 2;
+
+	return num_r5_cores +
+	       instance * PRU_TYPE_MAX * PRUETH_NUM_MACS +
+	       slice * PRU_TYPE_MAX;
+}
+
+int icssg_start_pru_cores(struct udevice *dev)
+{
+	struct prueth *prueth = dev_get_priv(dev);
+	struct icssg_firmwares *firmwares;
+	u32 pru_fw_loadaddr = 0x80000000;
+	u32 rtu_fw_loadaddr = 0x89000000;
+	u32 txpru_fw_loadaddr = 0x90000000;
+	int slice, ret, core_id;
+
+	firmwares = icssg_emac_firmwares;
+	slice = prueth->slice;
+
+	core_id = icssg_get_pru_core_number(dev, slice);
+	if (core_id < 0)
+		return core_id;
+
+	/* Initialize all rproc cores */
+	rproc_init();
+
+	/* Loading PRU firmware to PRU core */
+	ret = load_firmware(firmwares[slice].pru, &pru_fw_loadaddr);
+
+	if (ret < 0) {
+		dev_err(dev, "Unable to load_firmware %s at addr 0x%x err %d\n",
+			firmwares[slice].pru, pru_fw_loadaddr, ret);
+		return ret;
+	}
+
+	dev_info(dev, "Loaded FW %s successfully at addr 0x%x Size = %d Bytes\n",
+		firmwares[slice].pru, pru_fw_loadaddr, ret);
+	rproc_load(core_id + PRU_TYPE_PRU, pru_fw_loadaddr, ret);
+
+	/* Loading RTU firmware to RTU core */
+	ret = load_firmware(firmwares[slice].rtu, &rtu_fw_loadaddr);
+
+	if (ret < 0) {
+		dev_err(dev, "Unable to load_firmware %s at addr 0x%x err %d\n",
+			firmwares[slice].rtu, rtu_fw_loadaddr, ret);
+		return ret;
+	}
+
+	dev_info(dev, "Loaded FW %s successfully at addr 0x%x Size = %d Bytes\n",
+		firmwares[slice].rtu, rtu_fw_loadaddr, ret);
+	rproc_load(core_id + PRU_TYPE_RTU, rtu_fw_loadaddr, ret);
+
+	/* Loading TX_PRU firmware to TX_PRU core */
+	ret = load_firmware(firmwares[slice].txpru, &txpru_fw_loadaddr);
+
+	if (ret < 0) {
+		dev_err(dev, "Unable to load_firmware %s at addr 0x%x err %d\n",
+			firmwares[slice].txpru, txpru_fw_loadaddr, ret);
+		return ret;
+	}
+
+	dev_info(dev, "Loaded FW %s successfully at addr 0x%x Size = %d Bytes\n",
+		firmwares[slice].txpru, txpru_fw_loadaddr, ret);
+	rproc_load(core_id + PRU_TYPE_TX_PRU, txpru_fw_loadaddr, ret);
+
+	ret = rproc_start(core_id + PRU_TYPE_PRU);
+	if (ret) {
+		dev_err(dev, "failed to start PRU%d: %d\n", slice, ret);
+		return ret;
+	}
+
+	ret = rproc_start(core_id + PRU_TYPE_RTU);
+	if (ret) {
+		dev_err(dev, "failed to start RTU%d: %d\n", slice, ret);
+		goto halt_pru;
+	}
+
+	ret = rproc_start(core_id + PRU_TYPE_TX_PRU);
+	if (ret) {
+		dev_err(dev, "failed to start TX_PRU%d: %d\n", slice, ret);
+		goto halt_rtu;
+	}
+
+	return 0;
+
+halt_rtu:
+	rproc_stop(core_id + PRU_TYPE_RTU);
+
+halt_pru:
+	rproc_stop(PRU_TYPE_PRU);
+	return ret;
+}
+
+int icssg_stop_pru_cores(struct udevice *dev)
+{
+	struct prueth *prueth = dev_get_priv(dev);
+	int slice, core_id;
+
+	slice = prueth->slice;
+
+	core_id = icssg_get_pru_core_number(dev, slice);
+	if (core_id < 0)
+		return core_id;
+
+	rproc_stop(core_id + PRU_TYPE_PRU);
+	rproc_stop(core_id + PRU_TYPE_RTU);
+	rproc_stop(core_id + PRU_TYPE_TX_PRU);
+
+	return 0;
+}
+
 static int prueth_probe(struct udevice *dev)
 {
 	ofnode eth_ports_node, eth0_node, eth1_node, eth_node;
diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h
index 25272e850e..f17fe8bf58 100644
--- a/include/linux/pruss_driver.h
+++ b/include/linux/pruss_driver.h
@@ -114,6 +114,21 @@ enum pru_ctable_idx {
 	PRU_C31,
 };

+/**
+ * enum pru_type - PRU core type identifier
+ *
+ * @PRU_TYPE_PRU: Programmable Real-time Unit
+ * @PRU_TYPE_RTU: Auxiliary Programmable Real-Time Unit
+ * @PRU_TYPE_TX_PRU: Transmit Programmable Real-Time Unit
+ * @PRU_TYPE_MAX: just keep this one at the end
+ */
+enum pru_type {
+	PRU_TYPE_PRU,
+	PRU_TYPE_RTU,
+	PRU_TYPE_TX_PRU,
+	PRU_TYPE_MAX,
+};
+
 /**
  * enum pruss_mem - PRUSS memory range identifiers
  */

with this diff, user don't need to run any extra commands at u-boot.
Once u-boot prompt is reached, just running ping / dhcp will suffice.

>>
>> => run init_icssg2
>> => dhcp
>> k3-navss-ringacc ringacc@3c000000: Ring Accelerator probed rings:818, gp-rings[304,100] sci-dev-id:187
>> k3-navss-ringacc ringacc@3c000000: dma-ring-reset-quirk: disabled
>> prueth icssg2-eth: K3 ICSSG: rflow_id_base: 8, chn_name = rx0
>> link up on port 0, speed 1000, full duplex
>> BOOTP broadcast 1
>> BOOTP broadcast 2
>> BOOTP broadcast 3
>> DHCP client bound to address 192.168.4.58 (1020 ms)
>>
> 
> What about shutting down the interface?
> I believe this has to be done manually too?
> 

Yes that is also needed.

>> Thanks and Regards,
>> MD Danish Anwar
>>
>> MD Danish Anwar (16):
>>   net: ti: icssg: Add Firmware Interface for ICSSG Ethernet driver.
>>   net: ti: icssg: Add Firmware config and classification APIs.
>>   net: ti: icssg: Add icssg queues APIs and macros
>>   net: ti: icssg: Add ICSSG ethernet driver
>>   net: ti: icssg: Add support sending FDB command to update rx_flow_id
>>   net: ti: icssg: Enforce pinctrl state on the MDIO child node
>>   arm: dts: k3-am65: Add additional regs for DMA components
>>   arm: dts: k3-am65: Add cfg reg region to ringacc node
>>   arm: dts: k3-am65-main: Add ICSSG IEP nodes
>>   arm: dts: k3-am654-base-board: Add ICSSG2 Ethernet support
>>   arm: dts: k3-am65x-binman: Add ICSSG2 overlay and configuration
>>   configs: am65x_evm_a53: Enable ICSSG Driver
>>   configs: am65x_evm_a53_defconfig: add SPL_LOAD_FIT_APPLY_OVERLAY
>>   tools/fdtgrep: Include __symbols__ table
>>   board: ti: am65x: Add check for k3-am654-icssg2 in
>>     board_fit_config_match()
>>   Revert "dm: core: Report bootph-pre-ram/sram node as pre-reloc after
>>     relocation"
>>
>>  arch/arm/dts/Makefile             |   3 +-
>>  arch/arm/dts/k3-am65-main.dtsi    |  49 ++-
>>  arch/arm/dts/k3-am65-mcu.dtsi     |  13 +-
>>  arch/arm/dts/k3-am654-icssg2.dtso | 145 +++++++
>>  arch/arm/dts/k3-am65x-binman.dtsi |  85 ++++
>>  board/ti/am65x/evm.c              |  11 +-
>>  configs/am65x_evm_a53_defconfig   |   4 +
>>  drivers/core/ofnode.c             |   2 +-
>>  drivers/net/ti/Kconfig            |   9 +
>>  drivers/net/ti/Makefile           |   1 +
>>  drivers/net/ti/icss_mii_rt.h      | 192 +++++++++
>>  drivers/net/ti/icssg_classifier.c | 376 +++++++++++++++++
>>  drivers/net/ti/icssg_config.c     | 469 +++++++++++++++++++++
>>  drivers/net/ti/icssg_config.h     | 195 +++++++++
>>  drivers/net/ti/icssg_prueth.c     | 654 ++++++++++++++++++++++++++++++
>>  drivers/net/ti/icssg_prueth.h     |  89 ++++
>>  drivers/net/ti/icssg_queues.c     |  51 +++
>>  drivers/net/ti/icssg_switch_map.h | 209 ++++++++++
>>  include/dm/ofnode.h               |   8 +-
>>  tools/fdtgrep.c                   |   8 +
>>  20 files changed, 2555 insertions(+), 18 deletions(-)
>>  create mode 100644 arch/arm/dts/k3-am654-icssg2.dtso
>>  create mode 100644 drivers/net/ti/icss_mii_rt.h
>>  create mode 100644 drivers/net/ti/icssg_classifier.c
>>  create mode 100644 drivers/net/ti/icssg_config.c
>>  create mode 100644 drivers/net/ti/icssg_config.h
>>  create mode 100644 drivers/net/ti/icssg_prueth.c
>>  create mode 100644 drivers/net/ti/icssg_prueth.h
>>  create mode 100644 drivers/net/ti/icssg_queues.c
>>  create mode 100644 drivers/net/ti/icssg_switch_map.h
>>
>> base:commit: a6f86132e30a407c7f96461df53c62fbe52e2b54
>
Roger Quadros Dec. 22, 2023, 12:43 p.m. UTC | #4
On 22/12/2023 12:26, MD Danish Anwar wrote:
> Hi Roger,
> 
> On 20/12/23 3:29 pm, Roger Quadros wrote:
>> Hi,
>>
>> On 19/12/2023 12:11, MD Danish Anwar wrote:
>>> Introduce ICSSG PRUETH support in uboot. The ICSSG driver is used in TI
>>> AM654 SR2.0.
>>>
>>> The ICSSG PRU Sub-system runs on EMAC firmware. This series Introduces
>>> support for ICSSG driver in uboot. This series also adds the driver's
>>> dependencies.
>>>
>>> The ICSSG2 node is added in device tree overlay so that it remains in
>>> sync with linux kernel.
>>>
>>> The series introduces device tree and config changes and AM65x
>>> to enable ICSSG driver. The series also enables SPL_LOAD_FIT_APPLY_OVERLAY
>>> for AM65x in order to load overlay over spl.
>>>
>>> This series has been tested on AM65x SR2.0, and the ICSSG interface is
>>> able to ping / dhcp and boot kernel using tftp in uboot.
>>>
>>> To use ICSSG2 ethernet, the ICSSG firmware needs to be loaded to PRU RPROC
>>> cores and RPROC cores need to be booted with the firmware. This step is
>>> done inside driver in kernel as kernel supports APIs like
>>> rproc_set_firmware() and rproc_fw_boot(). But as u-boot doesn't have these
>>> APIs, the same needs to be done via u-boot cmds.
>>>
>>> To make sure icssg-eth works we need to do below steps.
>>>
>>> 1. Initialize rproc cores i.e. rproc_init()
>>> 2. Load $firmware_file from partition '1:2' (root) on device (mmc in this
>>>    example)
>>> 3. Load the firmware file to rproc cores passing. i.e. rproc_load()
>>>    taking rproc_id, loadaddr and file size as arguments.
>>> 4. Start rproc cores. i.e. rproc_start() taking rproc_id as arguments
>>>
>>> The above steps are done by running the below commands at u-boot prompt.
>>>
>>> => setenv start_icssg2 'rproc start 14; rproc start 15; rproc start 16; rproc start 17; rproc start 18; rproc start 19'
>>> => setenv stop_icssg2 'rproc stop 14; rproc stop 15; rproc stop 16; rproc stop 17; rproc stop 18; rproc stop 19'
>>> => setenv firmware_dir '/lib/firmware/ti-pruss'
>>> => setenv get_firmware_mmc 'load mmc ${bootpart} ${loadaddr} ${firmware_dir}/${firmware_file}'
>>>
>>> => setenv init_icssg2 'setenv ethact icssg2-eth; setenv autoload no; rproc init; setenv loadaddr 0x80000000; \
>>>     setenv firmware_file am65x-sr2-pru0-prueth-fw.elf; run get_firmware_mmc;  rproc load 14 0x80000000 ${filesize}; \
>>>     setenv loadaddr 0x89000000; setenv firmware_file am65x-sr2-rtu0-prueth-fw.elf; run get_firmware_mmc; rproc load 15 0x89000000 ${filesize}; \
>>>     setenv loadaddr 0x90000000; setenv firmware_file am65x-sr2-txpru0-prueth-fw.elf; run get_firmware_mmc; rproc load 16 0x90000000 ${filesize}; \
>>>     setenv loadaddr 0x80000000; setenv firmware_file am65x-sr2-pru1-prueth-fw.elf; run get_firmware_mmc; rproc load 17 0x80000000 ${filesize}; \
>>>     setenv loadaddr 0x89000000; setenv firmware_file am65x-sr2-rtu1-prueth-fw.elf; run get_firmware_mmc; rproc load 18 0x89000000 ${filesize}; \
>>>     setenv loadaddr 0x90000000; setenv firmware_file am65x-sr2-txpru1-prueth-fw.elf; run get_firmware_mmc; rproc load 19 0x90000000 ${filesize}; \
>>>     run start_icssg2;'
>>
>> A whole bunch of commands are required to get ethernet functional.
>> This is not at all user friendly and will be a maintenance nightmare.
>> What worries me is tracking the 6 different rproc cores and the 6 different firmware files to start 1 ethernet device.
>> This will get even more interesting when we have to deal with different ICSSG instances on different boards.
>>
>> What is preventing the driver from starting the rproc cores it needs so user doesn't need to care about it?
>> All the necessary information is in the Device tree. At least this is how it is done on Linux.
>>
> 
> I tried removing the need for these commands and implementing them
> inside the driver only. I am able to load the firmware from driver using
> the fs_loader API request_firmware_into_buf(). It requires changes to
> dt. A DT node called fs-loader needs to be added also CONFIG_FS_LOADER
> needs to enabled. In the DT node we need to specify the storage media
> that we are using i.e. mmc, ospi, usb. It's upto user to modify the
> storage media, the driver will take the media from DT and try to laod
> firmware from their.
> 
> For loading firmwares to rproc cores, rproc_load() API is needed. Now
> this API takes rproc_id, loadaddr and firmware_size as arguments.
> loadaddr is fixed for all three pru cores. firmware_size is obtained
> from request_firmware_into_buf() but I couldn't find a way to get the
> rproc_id. For now based on the ICSSG instance and slice number I am
> figuring out the rproc_id and passing it to rproc_load() and
> rproc_start() APIs. Please let me know if you have any other / better
> way of finding rproc_id.
> 
> Below is the entire diff to remove these commands and move their
> functionality to driver. Please have a look and let me know if this is ok.
> 

Good to see you got something working so quickly.
It has some rough edges but nothing that is blocking.

> 
> Save New Duplicate & Edit Just Text Twitter
> diff --git a/arch/arm/dts/k3-am654-base-board.dts
> b/arch/arm/dts/k3-am654-base-board.dts
> index cfbcebfa37..c8da72e49c 100644
> --- a/arch/arm/dts/k3-am654-base-board.dts
> +++ b/arch/arm/dts/k3-am654-base-board.dts
> @@ -16,6 +16,13 @@
>  	chosen {
>  		stdout-path = "serial2:115200n8";
>  		bootargs = "earlycon=ns16550a,mmio32,0x02800000";
> +		firmware-loader = <&fs_loader0>;
> +	};
> +
> +	fs_loader0: fs-loader {
> +		bootph-all;
> +		compatible = "u-boot,fs-loader";
> +		phandlepart = <&sdhci1 2>;
>  	};

This is has 2 issues
1) It will not be accepted in Kernel DT. Maybe it could be done in -u-boot.dtsi file.
2) You cannot assume boot medium is always sdhci1 2

> 
>  	memory@80000000 {
> diff --git a/configs/am65x_evm_a53_defconfig
> b/configs/am65x_evm_a53_defconfig
> index 2755d7082f..c53e938abb 100644
> --- a/configs/am65x_evm_a53_defconfig
> +++ b/configs/am65x_evm_a53_defconfig
> @@ -112,6 +112,7 @@ CONFIG_DM_I2C_GPIO=y
>  CONFIG_SYS_I2C_OMAP24XX=y
>  CONFIG_DM_MAILBOX=y
>  CONFIG_K3_SEC_PROXY=y
> +CONFIG_FS_LOADER=y
>  CONFIG_SUPPORT_EMMC_BOOT=y
>  CONFIG_MMC_IO_VOLTAGE=y
>  CONFIG_MMC_UHS_SUPPORT=y
> diff --git a/configs/am65x_evm_r5_defconfig b/configs/am65x_evm_r5_defconfig
> index b2f1e721b3..2d19935a41 100644
> --- a/configs/am65x_evm_r5_defconfig
> +++ b/configs/am65x_evm_r5_defconfig
> @@ -98,6 +98,7 @@ CONFIG_I2C_SET_DEFAULT_BUS_NUM=y
>  CONFIG_SYS_I2C_OMAP24XX=y
>  CONFIG_DM_MAILBOX=y
>  CONFIG_K3_SEC_PROXY=y
> +CONFIG_FS_LOADER=y
>  CONFIG_K3_AVS0=y
>  CONFIG_SUPPORT_EMMC_BOOT=y
>  CONFIG_MMC_HS200_SUPPORT=y
> diff --git a/drivers/net/ti/icssg_prueth.c b/drivers/net/ti/icssg_prueth.c
> index 40ad827e49..1c4edeb7b7 100644
> --- a/drivers/net/ti/icssg_prueth.c
> +++ b/drivers/net/ti/icssg_prueth.c
> @@ -227,6 +227,10 @@ static int prueth_start(struct udevice *dev)
>  	void *config;
>  	int ret, i;
> 
> +	ret = icssg_start_pru_cores(dev);
> +	if (ret)
> +		return ret;
> +
>  	/* To differentiate channels for SLICE0 vs SLICE1 */
>  	snprintf(chn_name, sizeof(chn_name), "tx%d-0", priv->slice);
> 
> @@ -355,9 +359,11 @@ static void prueth_stop(struct udevice *dev)
>  	phy_shutdown(priv->phydev);
> 
>  	dma_disable(&priv->dma_tx);
> -	dma_free(&priv->dma_tx);
> -
>  	dma_disable(&priv->dma_rx);
> +
> +	icssg_stop_pru_cores(dev);
> +
> +	dma_free(&priv->dma_tx);
>  	dma_free(&priv->dma_rx);
>  }
> 
> @@ -434,6 +440,181 @@ static const struct soc_attr k3_mdio_soc_data[] = {
>  	{ /* sentinel */ },
>  };
> 
> +struct icssg_firmware_load_address {
> +	u32 pru;
> +	u32 rtu;
> +	u32 txpru;
> +};
> +
> +struct icssg_firmwares {
> +	char *pru;
> +	char *rtu;
> +	char *txpru;
> +};
> +
> +static struct icssg_firmwares icssg_emac_firmwares[] = {
> +	{
> +		.pru = "/lib/firmware/ti-pruss/am65x-sr2-pru0-prueth-fw.elf",
> +		.rtu = "/lib/firmware/ti-pruss/am65x-sr2-rtu0-prueth-fw.elf",
> +		.txpru = "/lib/firmware/ti-pruss/am65x-sr2-txpru0-prueth-fw.elf",
> +	},
> +	{
> +		.pru = "/lib/firmware/ti-pruss/am65x-sr2-pru1-prueth-fw.elf",
> +		.rtu = "/lib/firmware/ti-pruss/am65x-sr2-rtu1-prueth-fw.elf",
> +		.txpru = "/lib/firmware/ti-pruss/am65x-sr2-txpru1-prueth-fw.elf",
> +	}
> +};

This information is contained in the DT.

		firmware-name = "ti-pruss/am65x-sr2-pru0-prueth-fw.elf",
				"ti-pruss/am65x-sr2-rtu0-prueth-fw.elf",
				"ti-pruss/am65x-sr2-txpru0-prueth-fw.elf",
				"ti-pruss/am65x-sr2-pru1-prueth-fw.elf",
				"ti-pruss/am65x-sr2-rtu1-prueth-fw.elf",
				"ti-pruss/am65x-sr2-txpru1-prueth-fw.elf";

You will need to introduce a rproc_set_firmware() API so clients can
set their own firmware names.

> +
> +int load_firmware(char *name_fw, u32 *loadaddr)
> +{
> +	struct udevice *fsdev;
> +	int size = 0;
> +
> +	if (!IS_ENABLED(CONFIG_FS_LOADER))
> +		return -EINVAL;
> +
> +	if (!*loadaddr)
> +		return -EINVAL;
> +
> +	if (!get_fs_loader(&fsdev))
> +		size = request_firmware_into_buf(fsdev, name_fw, (void *)*loadaddr,
> 40524, 0);
> +
> +	return size;
> +}

On Linux rproc_boot() does both loading the firmware and starting the rproc
as that is farely generic.
You should introduce rproc_boot() API so loading is taken care of at rproc driver.
All you need to do is call rproc_set_firmware() before rproc_boot().

> +
> +static int icssg_get_instance(struct udevice *dev)
> +{
> +	if (!strcmp(dev->name, "icssg2-eth"))
> +		return 2;
> +	else if (!strcmp(dev->name, "icssg1-eth"))
> +		return 1;
> +	else if (!strcmp(dev->name, "icssg0-eth"))
> +		return 0;
> +
> +	dev_err(dev, "Invalid icssg instance\n");
> +	return -EINVAL;
> +}
> +
> +static int icssg_get_pru_core_number(struct udevice *dev, int slice)
> +{
> +	int instance, num_r5_cores;
> +
> +	instance = icssg_get_instance(dev);
> +	if (instance < 0)
> +		return instance;
> +
> +	if (IS_ENABLED(CONFIG_REMOTEPROC_TI_K3_R5F))
> +		num_r5_cores = 2;
> +
> +	return num_r5_cores +
> +	       instance * PRU_TYPE_MAX * PRUETH_NUM_MACS +
> +	       slice * PRU_TYPE_MAX;

All this doesn't look right. What we need is the rproc device
that matches the PRU/RTU rprocs that we are interested in.

The DT already has this information

		ti,prus = <&pru2_0>, <&rtu2_0>, <&tx_pru2_0>,
			<&pru2_1>, <&rtu2_1>, <&tx_pru2_1>;


All the current rproc APIs use the below to get rproc device from ID
ret = uclass_get_device_by_seq(UCLASS_REMOTEPROC, id, &dev);

You just need to introduce APIs that takes rproc device directly as argument.

In your driver you can call uclass_get_device_by_phandle_id() to get the
rproc device from the rproc phandle?


> +}
> +
> +int icssg_start_pru_cores(struct udevice *dev)
> +{
> +	struct prueth *prueth = dev_get_priv(dev);
> +	struct icssg_firmwares *firmwares;
> +	u32 pru_fw_loadaddr = 0x80000000;
> +	u32 rtu_fw_loadaddr = 0x89000000;
> +	u32 txpru_fw_loadaddr = 0x90000000;

Please avoid hardcoding. You can use malloc to get a temporary buffer area?

Why do you need 3 different addresses?
Once you do a rproc_load isn't the buffer already copied to rproc's memory
so you can discard it or use it for the other rprocs?

> +	int slice, ret, core_id;
> +
> +	firmwares = icssg_emac_firmwares;
> +	slice = prueth->slice;
> +
> +	core_id = icssg_get_pru_core_number(dev, slice);
> +	if (core_id < 0)
> +		return core_id;
> +
> +	/* Initialize all rproc cores */
> +	rproc_init();
> +
> +	/* Loading PRU firmware to PRU core */
> +	ret = load_firmware(firmwares[slice].pru, &pru_fw_loadaddr);

On Linux, loading the firmware is the responsibility of the rproc driver.
Shouldn't it be the same in u-boot?

> +
> +	if (ret < 0) {
> +		dev_err(dev, "Unable to load_firmware %s at addr 0x%x err %d\n",
> +			firmwares[slice].pru, pru_fw_loadaddr, ret);
> +		return ret;
> +	}
> +
> +	dev_info(dev, "Loaded FW %s successfully at addr 0x%x Size = %d Bytes\n",
> +		firmwares[slice].pru, pru_fw_loadaddr, ret);

dev_dbg() here an at all dev_info().

> +	rproc_load(core_id + PRU_TYPE_PRU, pru_fw_loadaddr, ret);
> +
> +	/* Loading RTU firmware to RTU core */
> +	ret = load_firmware(firmwares[slice].rtu, &rtu_fw_loadaddr);
> +
> +	if (ret < 0) {
> +		dev_err(dev, "Unable to load_firmware %s at addr 0x%x err %d\n",
> +			firmwares[slice].rtu, rtu_fw_loadaddr, ret);
> +		return ret;
> +	}
> +
> +	dev_info(dev, "Loaded FW %s successfully at addr 0x%x Size = %d Bytes\n",
> +		firmwares[slice].rtu, rtu_fw_loadaddr, ret);
> +	rproc_load(core_id + PRU_TYPE_RTU, rtu_fw_loadaddr, ret);
> +
> +	/* Loading TX_PRU firmware to TX_PRU core */
> +	ret = load_firmware(firmwares[slice].txpru, &txpru_fw_loadaddr);
> +
> +	if (ret < 0) {
> +		dev_err(dev, "Unable to load_firmware %s at addr 0x%x err %d\n",
> +			firmwares[slice].txpru, txpru_fw_loadaddr, ret);
> +		return ret;
> +	}
> +
> +	dev_info(dev, "Loaded FW %s successfully at addr 0x%x Size = %d Bytes\n",
> +		firmwares[slice].txpru, txpru_fw_loadaddr, ret);
> +	rproc_load(core_id + PRU_TYPE_TX_PRU, txpru_fw_loadaddr, ret);
> +
> +	ret = rproc_start(core_id + PRU_TYPE_PRU);
> +	if (ret) {
> +		dev_err(dev, "failed to start PRU%d: %d\n", slice, ret);
> +		return ret;
> +	}
> +
> +	ret = rproc_start(core_id + PRU_TYPE_RTU);
> +	if (ret) {
> +		dev_err(dev, "failed to start RTU%d: %d\n", slice, ret);
> +		goto halt_pru;
> +	}
> +
> +	ret = rproc_start(core_id + PRU_TYPE_TX_PRU);
> +	if (ret) {
> +		dev_err(dev, "failed to start TX_PRU%d: %d\n", slice, ret);
> +		goto halt_rtu;
> +	}
> +
> +	return 0;
> +
> +halt_rtu:
> +	rproc_stop(core_id + PRU_TYPE_RTU);
> +
> +halt_pru:
> +	rproc_stop(PRU_TYPE_PRU);
> +	return ret;
> +}
> +
> +int icssg_stop_pru_cores(struct udevice *dev)
> +{
> +	struct prueth *prueth = dev_get_priv(dev);
> +	int slice, core_id;
> +
> +	slice = prueth->slice;
> +
> +	core_id = icssg_get_pru_core_number(dev, slice);
> +	if (core_id < 0)
> +		return core_id;
> +
> +	rproc_stop(core_id + PRU_TYPE_PRU);
> +	rproc_stop(core_id + PRU_TYPE_RTU);
> +	rproc_stop(core_id + PRU_TYPE_TX_PRU);
> +
> +	return 0;
> +}
> +
>  static int prueth_probe(struct udevice *dev)
>  {
>  	ofnode eth_ports_node, eth0_node, eth1_node, eth_node;
> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h
> index 25272e850e..f17fe8bf58 100644
> --- a/include/linux/pruss_driver.h
> +++ b/include/linux/pruss_driver.h
> @@ -114,6 +114,21 @@ enum pru_ctable_idx {
>  	PRU_C31,
>  };
> 
> +/**
> + * enum pru_type - PRU core type identifier
> + *
> + * @PRU_TYPE_PRU: Programmable Real-time Unit
> + * @PRU_TYPE_RTU: Auxiliary Programmable Real-Time Unit
> + * @PRU_TYPE_TX_PRU: Transmit Programmable Real-Time Unit
> + * @PRU_TYPE_MAX: just keep this one at the end
> + */
> +enum pru_type {
> +	PRU_TYPE_PRU,
> +	PRU_TYPE_RTU,
> +	PRU_TYPE_TX_PRU,
> +	PRU_TYPE_MAX,
> +};
> +
>  /**
>   * enum pruss_mem - PRUSS memory range identifiers
>   */
> 
> with this diff, user don't need to run any extra commands at u-boot.
> Once u-boot prompt is reached, just running ping / dhcp will suffice.
> 
Great!

<snip>
MD Danish Anwar Dec. 27, 2023, 6:56 a.m. UTC | #5
On 22/12/23 6:13 pm, Roger Quadros wrote:
> 
> On 22/12/2023 12:26, MD Danish Anwar wrote:
>> Hi Roger,
>>
>> On 20/12/23 3:29 pm, Roger Quadros wrote:
>>> Hi,
>>>
>>> On 19/12/2023 12:11, MD Danish Anwar wrote:
>>>> Introduce ICSSG PRUETH support in uboot. The ICSSG driver is used in TI
>>>> AM654 SR2.0.
>>>>
>>>> The ICSSG PRU Sub-system runs on EMAC firmware. This series Introduces
>>>> support for ICSSG driver in uboot. This series also adds the driver's
>>>> dependencies.
>>>>
>>>> The ICSSG2 node is added in device tree overlay so that it remains in
>>>> sync with linux kernel.
>>>>
>>>> The series introduces device tree and config changes and AM65x
>>>> to enable ICSSG driver. The series also enables SPL_LOAD_FIT_APPLY_OVERLAY
>>>> for AM65x in order to load overlay over spl.
>>>>
>>>> This series has been tested on AM65x SR2.0, and the ICSSG interface is
>>>> able to ping / dhcp and boot kernel using tftp in uboot.
>>>>
>>>> To use ICSSG2 ethernet, the ICSSG firmware needs to be loaded to PRU RPROC
>>>> cores and RPROC cores need to be booted with the firmware. This step is
>>>> done inside driver in kernel as kernel supports APIs like
>>>> rproc_set_firmware() and rproc_fw_boot(). But as u-boot doesn't have these
>>>> APIs, the same needs to be done via u-boot cmds.
>>>>
>>>> To make sure icssg-eth works we need to do below steps.
>>>>
>>>> 1. Initialize rproc cores i.e. rproc_init()
>>>> 2. Load $firmware_file from partition '1:2' (root) on device (mmc in this
>>>>    example)
>>>> 3. Load the firmware file to rproc cores passing. i.e. rproc_load()
>>>>    taking rproc_id, loadaddr and file size as arguments.
>>>> 4. Start rproc cores. i.e. rproc_start() taking rproc_id as arguments
>>>>
>>>> The above steps are done by running the below commands at u-boot prompt.
>>>>
>>>> => setenv start_icssg2 'rproc start 14; rproc start 15; rproc start 16; rproc start 17; rproc start 18; rproc start 19'
>>>> => setenv stop_icssg2 'rproc stop 14; rproc stop 15; rproc stop 16; rproc stop 17; rproc stop 18; rproc stop 19'
>>>> => setenv firmware_dir '/lib/firmware/ti-pruss'
>>>> => setenv get_firmware_mmc 'load mmc ${bootpart} ${loadaddr} ${firmware_dir}/${firmware_file}'
>>>>
>>>> => setenv init_icssg2 'setenv ethact icssg2-eth; setenv autoload no; rproc init; setenv loadaddr 0x80000000; \
>>>>     setenv firmware_file am65x-sr2-pru0-prueth-fw.elf; run get_firmware_mmc;  rproc load 14 0x80000000 ${filesize}; \
>>>>     setenv loadaddr 0x89000000; setenv firmware_file am65x-sr2-rtu0-prueth-fw.elf; run get_firmware_mmc; rproc load 15 0x89000000 ${filesize}; \
>>>>     setenv loadaddr 0x90000000; setenv firmware_file am65x-sr2-txpru0-prueth-fw.elf; run get_firmware_mmc; rproc load 16 0x90000000 ${filesize}; \
>>>>     setenv loadaddr 0x80000000; setenv firmware_file am65x-sr2-pru1-prueth-fw.elf; run get_firmware_mmc; rproc load 17 0x80000000 ${filesize}; \
>>>>     setenv loadaddr 0x89000000; setenv firmware_file am65x-sr2-rtu1-prueth-fw.elf; run get_firmware_mmc; rproc load 18 0x89000000 ${filesize}; \
>>>>     setenv loadaddr 0x90000000; setenv firmware_file am65x-sr2-txpru1-prueth-fw.elf; run get_firmware_mmc; rproc load 19 0x90000000 ${filesize}; \
>>>>     run start_icssg2;'
>>>
>>> A whole bunch of commands are required to get ethernet functional.
>>> This is not at all user friendly and will be a maintenance nightmare.
>>> What worries me is tracking the 6 different rproc cores and the 6 different firmware files to start 1 ethernet device.
>>> This will get even more interesting when we have to deal with different ICSSG instances on different boards.
>>>
>>> What is preventing the driver from starting the rproc cores it needs so user doesn't need to care about it?
>>> All the necessary information is in the Device tree. At least this is how it is done on Linux.
>>>
>>
>> I tried removing the need for these commands and implementing them
>> inside the driver only. I am able to load the firmware from driver using
>> the fs_loader API request_firmware_into_buf(). It requires changes to
>> dt. A DT node called fs-loader needs to be added also CONFIG_FS_LOADER
>> needs to enabled. In the DT node we need to specify the storage media
>> that we are using i.e. mmc, ospi, usb. It's upto user to modify the
>> storage media, the driver will take the media from DT and try to laod
>> firmware from their.
>>
>> For loading firmwares to rproc cores, rproc_load() API is needed. Now
>> this API takes rproc_id, loadaddr and firmware_size as arguments.
>> loadaddr is fixed for all three pru cores. firmware_size is obtained
>> from request_firmware_into_buf() but I couldn't find a way to get the
>> rproc_id. For now based on the ICSSG instance and slice number I am
>> figuring out the rproc_id and passing it to rproc_load() and
>> rproc_start() APIs. Please let me know if you have any other / better
>> way of finding rproc_id.
>>
>> Below is the entire diff to remove these commands and move their
>> functionality to driver. Please have a look and let me know if this is ok.
>>
> 
> Good to see you got something working so quickly.
> It has some rough edges but nothing that is blocking.
> 
>>
>> Save New Duplicate & Edit Just Text Twitter
>> diff --git a/arch/arm/dts/k3-am654-base-board.dts
>> b/arch/arm/dts/k3-am654-base-board.dts
>> index cfbcebfa37..c8da72e49c 100644
>> --- a/arch/arm/dts/k3-am654-base-board.dts
>> +++ b/arch/arm/dts/k3-am654-base-board.dts
>> @@ -16,6 +16,13 @@
>>  	chosen {
>>  		stdout-path = "serial2:115200n8";
>>  		bootargs = "earlycon=ns16550a,mmio32,0x02800000";
>> +		firmware-loader = <&fs_loader0>;
>> +	};
>> +
>> +	fs_loader0: fs-loader {
>> +		bootph-all;
>> +		compatible = "u-boot,fs-loader";
>> +		phandlepart = <&sdhci1 2>;
>>  	};
> 
> This is has 2 issues
> 1) It will not be accepted in Kernel DT. Maybe it could be done in -u-boot.dtsi file.

Sure. I'll move this to k3-am654-base-board-u-boot.dtsi

> 2) You cannot assume boot medium is always sdhci1 2
> 

Yes. It's upto user to provide the boot medium and boot partition for
loading firmware. By default the firmware is loacated in root partion of
shdci1 on am65x so I am adding this as default. But user can easily
modify this to any other medium / partition needed.

>>
>>  	memory@80000000 {
>> diff --git a/configs/am65x_evm_a53_defconfig
>> b/configs/am65x_evm_a53_defconfig
>> index 2755d7082f..c53e938abb 100644
>> --- a/configs/am65x_evm_a53_defconfig
>> +++ b/configs/am65x_evm_a53_defconfig
>> @@ -112,6 +112,7 @@ CONFIG_DM_I2C_GPIO=y
>>  CONFIG_SYS_I2C_OMAP24XX=y
>>  CONFIG_DM_MAILBOX=y
>>  CONFIG_K3_SEC_PROXY=y
>> +CONFIG_FS_LOADER=y
>>  CONFIG_SUPPORT_EMMC_BOOT=y
>>  CONFIG_MMC_IO_VOLTAGE=y
>>  CONFIG_MMC_UHS_SUPPORT=y
>> diff --git a/configs/am65x_evm_r5_defconfig b/configs/am65x_evm_r5_defconfig
>> index b2f1e721b3..2d19935a41 100644
>> --- a/configs/am65x_evm_r5_defconfig
>> +++ b/configs/am65x_evm_r5_defconfig
>> @@ -98,6 +98,7 @@ CONFIG_I2C_SET_DEFAULT_BUS_NUM=y
>>  CONFIG_SYS_I2C_OMAP24XX=y
>>  CONFIG_DM_MAILBOX=y
>>  CONFIG_K3_SEC_PROXY=y
>> +CONFIG_FS_LOADER=y
>>  CONFIG_K3_AVS0=y
>>  CONFIG_SUPPORT_EMMC_BOOT=y
>>  CONFIG_MMC_HS200_SUPPORT=y
>> diff --git a/drivers/net/ti/icssg_prueth.c b/drivers/net/ti/icssg_prueth.c
>> index 40ad827e49..1c4edeb7b7 100644
>> --- a/drivers/net/ti/icssg_prueth.c
>> +++ b/drivers/net/ti/icssg_prueth.c
>> @@ -227,6 +227,10 @@ static int prueth_start(struct udevice *dev)
>>  	void *config;
>>  	int ret, i;
>>
>> +	ret = icssg_start_pru_cores(dev);
>> +	if (ret)
>> +		return ret;
>> +
>>  	/* To differentiate channels for SLICE0 vs SLICE1 */
>>  	snprintf(chn_name, sizeof(chn_name), "tx%d-0", priv->slice);
>>
>> @@ -355,9 +359,11 @@ static void prueth_stop(struct udevice *dev)
>>  	phy_shutdown(priv->phydev);
>>
>>  	dma_disable(&priv->dma_tx);
>> -	dma_free(&priv->dma_tx);
>> -
>>  	dma_disable(&priv->dma_rx);
>> +
>> +	icssg_stop_pru_cores(dev);
>> +
>> +	dma_free(&priv->dma_tx);
>>  	dma_free(&priv->dma_rx);
>>  }
>>
>> @@ -434,6 +440,181 @@ static const struct soc_attr k3_mdio_soc_data[] = {
>>  	{ /* sentinel */ },
>>  };
>>
>> +struct icssg_firmware_load_address {
>> +	u32 pru;
>> +	u32 rtu;
>> +	u32 txpru;
>> +};
>> +
>> +struct icssg_firmwares {
>> +	char *pru;
>> +	char *rtu;
>> +	char *txpru;
>> +};
>> +
>> +static struct icssg_firmwares icssg_emac_firmwares[] = {
>> +	{
>> +		.pru = "/lib/firmware/ti-pruss/am65x-sr2-pru0-prueth-fw.elf",
>> +		.rtu = "/lib/firmware/ti-pruss/am65x-sr2-rtu0-prueth-fw.elf",
>> +		.txpru = "/lib/firmware/ti-pruss/am65x-sr2-txpru0-prueth-fw.elf",
>> +	},
>> +	{
>> +		.pru = "/lib/firmware/ti-pruss/am65x-sr2-pru1-prueth-fw.elf",
>> +		.rtu = "/lib/firmware/ti-pruss/am65x-sr2-rtu1-prueth-fw.elf",
>> +		.txpru = "/lib/firmware/ti-pruss/am65x-sr2-txpru1-prueth-fw.elf",
>> +	}
>> +};
> 
> This information is contained in the DT.
> 
> 		firmware-name = "ti-pruss/am65x-sr2-pru0-prueth-fw.elf",
> 				"ti-pruss/am65x-sr2-rtu0-prueth-fw.elf",
> 				"ti-pruss/am65x-sr2-txpru0-prueth-fw.elf",
> 				"ti-pruss/am65x-sr2-pru1-prueth-fw.elf",
> 				"ti-pruss/am65x-sr2-rtu1-prueth-fw.elf",
> 				"ti-pruss/am65x-sr2-txpru1-prueth-fw.elf";
> 

Yes it is. But the linux driver uses the firmware strcuture inside
driver only. The firmware-name property is not being used. The firmware
names for all applicable SoCs (AM65x and AM64x) is same so instead of
reading it from DT, the driver hardcodes the firmware names in linux. I
think the thought here was to use firmware-name property from DT when
different firmwares are needed for different SoCs. I am just following
the same here in uboot as well.

> You will need to introduce a rproc_set_firmware() API so clients can
> set their own firmware names.
> 

Sure. I will add an element 'fw_name' in struct dm_rproc_uclass_pdata
and set this fw_name to the firmware name requested by client using
rproc_set_firmware().

The client drivers can call rproc_set_firmware(dev, firmware_name) to
set the firmware name.

>> +
>> +int load_firmware(char *name_fw, u32 *loadaddr)
>> +{
>> +	struct udevice *fsdev;
>> +	int size = 0;
>> +
>> +	if (!IS_ENABLED(CONFIG_FS_LOADER))
>> +		return -EINVAL;
>> +
>> +	if (!*loadaddr)
>> +		return -EINVAL;
>> +
>> +	if (!get_fs_loader(&fsdev))
>> +		size = request_firmware_into_buf(fsdev, name_fw, (void *)*loadaddr,
>> 40524, 0);
>> +
>> +	return size;
>> +}
> 
> On Linux rproc_boot() does both loading the firmware and starting the rproc
> as that is farely generic.
> You should introduce rproc_boot() API so loading is taken care of at rproc driver.
> All you need to do is call rproc_set_firmware() before rproc_boot().
> 

Sure. I'll introduce rproc_boot() which will load the firmware to a
buffer and then load the firmware / buffer to rproc core usinig rproc_load.

For allocating memory for reading firmware we need to provide the size
of memory required as well. So rproc_boot will take two arguments.
1. dev (rproc device)
2. size (Maximum memory required to read firmware)

>> +
>> +static int icssg_get_instance(struct udevice *dev)
>> +{
>> +	if (!strcmp(dev->name, "icssg2-eth"))
>> +		return 2;
>> +	else if (!strcmp(dev->name, "icssg1-eth"))
>> +		return 1;
>> +	else if (!strcmp(dev->name, "icssg0-eth"))
>> +		return 0;
>> +
>> +	dev_err(dev, "Invalid icssg instance\n");
>> +	return -EINVAL;
>> +}
>> +
>> +static int icssg_get_pru_core_number(struct udevice *dev, int slice)
>> +{
>> +	int instance, num_r5_cores;
>> +
>> +	instance = icssg_get_instance(dev);
>> +	if (instance < 0)
>> +		return instance;
>> +
>> +	if (IS_ENABLED(CONFIG_REMOTEPROC_TI_K3_R5F))
>> +		num_r5_cores = 2;
>> +
>> +	return num_r5_cores +
>> +	       instance * PRU_TYPE_MAX * PRUETH_NUM_MACS +
>> +	       slice * PRU_TYPE_MAX;
> 
> All this doesn't look right. What we need is the rproc device
> that matches the PRU/RTU rprocs that we are interested in.
> 
> The DT already has this information
> 
> 		ti,prus = <&pru2_0>, <&rtu2_0>, <&tx_pru2_0>,
> 			<&pru2_1>, <&rtu2_1>, <&tx_pru2_1>;
> 
> 
> All the current rproc APIs use the below to get rproc device from ID
> ret = uclass_get_device_by_seq(UCLASS_REMOTEPROC, id, &dev);
> 
> You just need to introduce APIs that takes rproc device directly as argument.
> 
> In your driver you can call uclass_get_device_by_phandle_id() to get the
> rproc device from the rproc phandle?

Sure. I'll do that. Based on the icssg slice first I will get the
phandle for prus.

Index will be 0,1,2 for slice 0 and 3,4,5 for slice 1.

ofnode_read_u32_index(dev_ofnode(dev), "ti,prus", index, &phandle);

Then for each phandle I can call uclass_get_device_by_phandle_id() and
get the rproc device.

> 
> 
>> +}
>> +
>> +int icssg_start_pru_cores(struct udevice *dev)
>> +{
>> +	struct prueth *prueth = dev_get_priv(dev);
>> +	struct icssg_firmwares *firmwares;
>> +	u32 pru_fw_loadaddr = 0x80000000;
>> +	u32 rtu_fw_loadaddr = 0x89000000;
>> +	u32 txpru_fw_loadaddr = 0x90000000;
> 
> Please avoid hardcoding. You can use malloc to get a temporary buffer area?
> 
> Why do you need 3 different addresses?
> Once you do a rproc_load isn't the buffer already copied to rproc's memory
> so you can discard it or use it for the other rprocs?
> 

Sure I'll use malloc to get temporary buffer. Only thing is that to get
temporary buffer I will need to provide size as well.

void *addr = malloc(fw_size);

User needs to provide this fw_size to rproc_boot(). In my case the max
size the firmware can have is 64KB so I will be passing 64K as size to
rproc driver.

>> +	int slice, ret, core_id;
>> +
>> +	firmwares = icssg_emac_firmwares;
>> +	slice = prueth->slice;
>> +
>> +	core_id = icssg_get_pru_core_number(dev, slice);
>> +	if (core_id < 0)
>> +		return core_id;
>> +
>> +	/* Initialize all rproc cores */
>> +	rproc_init();
>> +
>> +	/* Loading PRU firmware to PRU core */
>> +	ret = load_firmware(firmwares[slice].pru, &pru_fw_loadaddr);
> 
> On Linux, loading the firmware is the responsibility of the rproc driver.
> Shouldn't it be the same in u-boot?
> 

Sure I'll move this to rproc driver.

>> +
>> +	if (ret < 0) {
>> +		dev_err(dev, "Unable to load_firmware %s at addr 0x%x err %d\n",
>> +			firmwares[slice].pru, pru_fw_loadaddr, ret);
>> +		return ret;
>> +	}
>> +
>> +	dev_info(dev, "Loaded FW %s successfully at addr 0x%x Size = %d Bytes\n",
>> +		firmwares[slice].pru, pru_fw_loadaddr, ret);
> 
> dev_dbg() here an at all dev_info().
> 
>> +	rproc_load(core_id + PRU_TYPE_PRU, pru_fw_loadaddr, ret);
>> +
>> +	/* Loading RTU firmware to RTU core */
>> +	ret = load_firmware(firmwares[slice].rtu, &rtu_fw_loadaddr);
>> +
>> +	if (ret < 0) {
>> +		dev_err(dev, "Unable to load_firmware %s at addr 0x%x err %d\n",
>> +			firmwares[slice].rtu, rtu_fw_loadaddr, ret);
>> +		return ret;
>> +	}
>> +
>> +	dev_info(dev, "Loaded FW %s successfully at addr 0x%x Size = %d Bytes\n",
>> +		firmwares[slice].rtu, rtu_fw_loadaddr, ret);
>> +	rproc_load(core_id + PRU_TYPE_RTU, rtu_fw_loadaddr, ret);
>> +
>> +	/* Loading TX_PRU firmware to TX_PRU core */
>> +	ret = load_firmware(firmwares[slice].txpru, &txpru_fw_loadaddr);
>> +
>> +	if (ret < 0) {
>> +		dev_err(dev, "Unable to load_firmware %s at addr 0x%x err %d\n",
>> +			firmwares[slice].txpru, txpru_fw_loadaddr, ret);
>> +		return ret;
>> +	}
>> +
>> +	dev_info(dev, "Loaded FW %s successfully at addr 0x%x Size = %d Bytes\n",
>> +		firmwares[slice].txpru, txpru_fw_loadaddr, ret);
>> +	rproc_load(core_id + PRU_TYPE_TX_PRU, txpru_fw_loadaddr, ret);
>> +
>> +	ret = rproc_start(core_id + PRU_TYPE_PRU);
>> +	if (ret) {
>> +		dev_err(dev, "failed to start PRU%d: %d\n", slice, ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = rproc_start(core_id + PRU_TYPE_RTU);
>> +	if (ret) {
>> +		dev_err(dev, "failed to start RTU%d: %d\n", slice, ret);
>> +		goto halt_pru;
>> +	}
>> +
>> +	ret = rproc_start(core_id + PRU_TYPE_TX_PRU);
>> +	if (ret) {
>> +		dev_err(dev, "failed to start TX_PRU%d: %d\n", slice, ret);
>> +		goto halt_rtu;
>> +	}
>> +
>> +	return 0;
>> +
>> +halt_rtu:
>> +	rproc_stop(core_id + PRU_TYPE_RTU);
>> +
>> +halt_pru:
>> +	rproc_stop(PRU_TYPE_PRU);
>> +	return ret;
>> +}
>> +
>> +int icssg_stop_pru_cores(struct udevice *dev)
>> +{
>> +	struct prueth *prueth = dev_get_priv(dev);
>> +	int slice, core_id;
>> +
>> +	slice = prueth->slice;
>> +
>> +	core_id = icssg_get_pru_core_number(dev, slice);
>> +	if (core_id < 0)
>> +		return core_id;
>> +
>> +	rproc_stop(core_id + PRU_TYPE_PRU);
>> +	rproc_stop(core_id + PRU_TYPE_RTU);
>> +	rproc_stop(core_id + PRU_TYPE_TX_PRU);
>> +
>> +	return 0;
>> +}
>> +
>>  static int prueth_probe(struct udevice *dev)
>>  {
>>  	ofnode eth_ports_node, eth0_node, eth1_node, eth_node;
>> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h
>> index 25272e850e..f17fe8bf58 100644
>> --- a/include/linux/pruss_driver.h
>> +++ b/include/linux/pruss_driver.h
>> @@ -114,6 +114,21 @@ enum pru_ctable_idx {
>>  	PRU_C31,
>>  };
>>
>> +/**
>> + * enum pru_type - PRU core type identifier
>> + *
>> + * @PRU_TYPE_PRU: Programmable Real-time Unit
>> + * @PRU_TYPE_RTU: Auxiliary Programmable Real-Time Unit
>> + * @PRU_TYPE_TX_PRU: Transmit Programmable Real-Time Unit
>> + * @PRU_TYPE_MAX: just keep this one at the end
>> + */
>> +enum pru_type {
>> +	PRU_TYPE_PRU,
>> +	PRU_TYPE_RTU,
>> +	PRU_TYPE_TX_PRU,
>> +	PRU_TYPE_MAX,
>> +};
>> +
>>  /**
>>   * enum pruss_mem - PRUSS memory range identifiers
>>   */
>>
>> with this diff, user don't need to run any extra commands at u-boot.
>> Once u-boot prompt is reached, just running ping / dhcp will suffice.
>>
> Great!
> 
> <snip>
> 

I have addressed all the above comments and made the changes needed.
below is the diff. Please have a look at it and let me know if it looks OK.

diff --git a/arch/arm/dts/k3-am654-base-board-u-boot.dtsi
b/arch/arm/dts/k3-am654-base-board-u-boot.dtsi
index 11d83927ac..0b2b72b2c5 100644
--- a/arch/arm/dts/k3-am654-base-board-u-boot.dtsi
+++ b/arch/arm/dts/k3-am654-base-board-u-boot.dtsi
@@ -6,6 +6,18 @@
 #include "k3-am654-r5-base-board-u-boot.dtsi"
 #include "k3-am65x-binman.dtsi"

+/ {
+	chosen {
+		firmware-loader = <&fs_loader0>;
+	};
+
+	fs_loader0: fs-loader {
+		bootph-all;
+		compatible = "u-boot,fs-loader";
+		phandlepart = <&sdhci1 2>;
+	};
+};
+
 &pru0_0 {
 	remoteproc-name = "pru0_0";
 };
diff --git a/configs/am65x_evm_a53_defconfig
b/configs/am65x_evm_a53_defconfig
index 2755d7082f..c53e938abb 100644
--- a/configs/am65x_evm_a53_defconfig
+++ b/configs/am65x_evm_a53_defconfig
@@ -112,6 +112,7 @@ CONFIG_DM_I2C_GPIO=y
 CONFIG_SYS_I2C_OMAP24XX=y
 CONFIG_DM_MAILBOX=y
 CONFIG_K3_SEC_PROXY=y
+CONFIG_FS_LOADER=y
 CONFIG_SUPPORT_EMMC_BOOT=y
 CONFIG_MMC_IO_VOLTAGE=y
 CONFIG_MMC_UHS_SUPPORT=y
diff --git a/configs/am65x_evm_r5_defconfig b/configs/am65x_evm_r5_defconfig
index b2f1e721b3..2d19935a41 100644
--- a/configs/am65x_evm_r5_defconfig
+++ b/configs/am65x_evm_r5_defconfig
@@ -98,6 +98,7 @@ CONFIG_I2C_SET_DEFAULT_BUS_NUM=y
 CONFIG_SYS_I2C_OMAP24XX=y
 CONFIG_DM_MAILBOX=y
 CONFIG_K3_SEC_PROXY=y
+CONFIG_FS_LOADER=y
 CONFIG_K3_AVS0=y
 CONFIG_SUPPORT_EMMC_BOOT=y
 CONFIG_MMC_HS200_SUPPORT=y
diff --git a/drivers/net/ti/icssg_prueth.c b/drivers/net/ti/icssg_prueth.c
index 40ad827e49..a2e3595956 100644
--- a/drivers/net/ti/icssg_prueth.c
+++ b/drivers/net/ti/icssg_prueth.c
@@ -62,6 +62,9 @@
 /* Management packet type */
 #define PRUETH_PKT_TYPE_CMD		0x10

+/* Number of PRU Cores per Slice */
+#define ICSSG_NUM_PRU_CORES		3
+
 static int icssg_phy_init(struct udevice *dev)
 {
 	struct prueth *priv = dev_get_priv(dev);
@@ -218,6 +221,116 @@ static int icssg_update_link(struct prueth *priv)
 	return phy->link;
 }

+struct icssg_firmwares {
+	char *pru;
+	char *rtu;
+	char *txpru;
+};
+
+static struct icssg_firmwares icssg_emac_firmwares[] = {
+	{
+		.pru = "/lib/firmware/ti-pruss/am65x-sr2-pru0-prueth-fw.elf",
+		.rtu = "/lib/firmware/ti-pruss/am65x-sr2-rtu0-prueth-fw.elf",
+		.txpru = "/lib/firmware/ti-pruss/am65x-sr2-txpru0-prueth-fw.elf",
+	},
+	{
+		.pru = "/lib/firmware/ti-pruss/am65x-sr2-pru1-prueth-fw.elf",
+		.rtu = "/lib/firmware/ti-pruss/am65x-sr2-rtu1-prueth-fw.elf",
+		.txpru = "/lib/firmware/ti-pruss/am65x-sr2-txpru1-prueth-fw.elf",
+	}
+};
+
+static int icssg_start_pru_cores(struct udevice *dev)
+{
+	struct prueth *prueth = dev_get_priv(dev);
+	struct icssg_firmwares *firmwares;
+	struct udevice *rproc_dev = NULL;
+	int ret, slice;
+	u32 phandle;
+	u8 index;
+
+	slice = prueth->slice;
+	index = slice * ICSSG_NUM_PRU_CORES;
+	firmwares = icssg_emac_firmwares;
+
+	ofnode_read_u32_index(dev_ofnode(dev), "ti,prus", index, &phandle);
+	ret = uclass_get_device_by_phandle_id(UCLASS_REMOTEPROC, phandle,
&rproc_dev);
+	if (ret) {
+		dev_err(dev, "Unknown remote processor with phandle '0x%x'
requested(%d)\n",
+		       phandle, ret);
+		return ret;
+	}
+
+	prueth->pru_core_id = dev_seq(rproc_dev);
+	ret = rproc_set_firmware(rproc_dev, firmwares[slice].pru);
+	if (ret)
+		return ret;
+
+	ret = rproc_boot(rproc_dev, SZ_64K);
+	if (ret) {
+		dev_err(dev, "failed to boot PRU%d: %d\n", slice, ret);
+		return -EINVAL;
+	}
+
+	ofnode_read_u32_index(dev_ofnode(dev), "ti,prus", index + 1, &phandle);
+	ret = uclass_get_device_by_phandle_id(UCLASS_REMOTEPROC, phandle,
&rproc_dev);
+	if (ret) {
+		dev_err(dev, "Unknown remote processor with phandle '0x%x'
requested(%d)\n",
+		       phandle, ret);
+		goto halt_pru;
+	}
+
+	prueth->rtu_core_id = dev_seq(rproc_dev);
+	ret = rproc_set_firmware(rproc_dev, firmwares[slice].rtu);
+	if (ret)
+		goto halt_pru;
+
+	ret = rproc_boot(rproc_dev, SZ_64K);
+	if (ret) {
+		dev_err(dev, "failed to boot RTU%d: %d\n", slice, ret);
+		goto halt_pru;
+	}
+
+	ofnode_read_u32_index(dev_ofnode(dev), "ti,prus", index + 2, &phandle);
+	ret = uclass_get_device_by_phandle_id(UCLASS_REMOTEPROC, phandle,
&rproc_dev);
+	if (ret) {
+		dev_err(dev, "Unknown remote processor with phandle '0x%x'
requested(%d)\n",
+		       phandle, ret);
+		goto halt_rtu;
+	}
+
+	prueth->txpru_core_id = dev_seq(rproc_dev);
+	ret = rproc_set_firmware(rproc_dev, firmwares[slice].txpru);
+	if (ret)
+		goto halt_rtu;
+
+	ret = rproc_boot(rproc_dev, SZ_64K);
+	if (ret) {
+		dev_err(dev, "failed to boot TXPRU%d: %d\n", slice, ret);
+		goto halt_rtu;
+	}
+
+	return 0;
+
+halt_rtu:
+	rproc_stop(prueth->rtu_core_id);
+
+halt_pru:
+	rproc_stop(prueth->pru_core_id);
+	return ret;
+}
+
+static int icssg_stop_pru_cores(struct udevice *dev)
+{
+	struct prueth *prueth = dev_get_priv(dev);
+
+	rproc_stop(prueth->pru_core_id);
+	rproc_stop(prueth->rtu_core_id);
+	rproc_stop(prueth->txpru_core_id);
+
+	return 0;
+}
+
 static int prueth_start(struct udevice *dev)
 {
 	struct ti_udma_drv_chan_cfg_data *dma_rx_cfg_data;
@@ -227,6 +340,10 @@ static int prueth_start(struct udevice *dev)
 	void *config;
 	int ret, i;

+	ret = icssg_start_pru_cores(dev);
+	if (ret)
+		return ret;
+
 	/* To differentiate channels for SLICE0 vs SLICE1 */
 	snprintf(chn_name, sizeof(chn_name), "tx%d-0", priv->slice);

@@ -355,9 +472,11 @@ static void prueth_stop(struct udevice *dev)
 	phy_shutdown(priv->phydev);

 	dma_disable(&priv->dma_tx);
-	dma_free(&priv->dma_tx);
-
 	dma_disable(&priv->dma_rx);
+
+	icssg_stop_pru_cores(dev);
+
+	dma_free(&priv->dma_tx);
 	dma_free(&priv->dma_rx);
 }

diff --git a/drivers/net/ti/icssg_prueth.h b/drivers/net/ti/icssg_prueth.h
index e41ed16a05..c92660401b 100644
--- a/drivers/net/ti/icssg_prueth.h
+++ b/drivers/net/ti/icssg_prueth.h
@@ -69,6 +69,9 @@ struct prueth {
 	int			speed;
 	int			duplex;
 	u8			icssg_hwcmdseq;
+	u8 			pru_core_id;
+	u8 			rtu_core_id;
+	u8 			txpru_core_id;
 };

 /* config helpers */
diff --git a/drivers/remoteproc/rproc-uclass.c
b/drivers/remoteproc/rproc-uclass.c
index 28b362c887..a085c44eaa 100644
--- a/drivers/remoteproc/rproc-uclass.c
+++ b/drivers/remoteproc/rproc-uclass.c
@@ -13,6 +13,7 @@
 #include <log.h>
 #include <malloc.h>
 #include <virtio_ring.h>
+#include <fs_loader.h>
 #include <remoteproc.h>
 #include <asm/io.h>
 #include <dm/device-internal.h>
@@ -961,3 +962,90 @@ unsigned long rproc_parse_resource_table(struct
udevice *dev, struct rproc *cfg)

 	return 1;
 }
+
+int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name)
+{
+	struct dm_rproc_uclass_pdata *uc_pdata;
+	int len;
+	char *p;
+
+	if (!rproc_dev || !fw_name)
+		return -EINVAL;
+
+	uc_pdata = dev_get_uclass_plat(rproc_dev);
+
+	len = strcspn(fw_name, "\n");
+	if (!len) {
+		debug("can't provide empty string for firmware name\n");
+		return -EINVAL;
+	}
+
+	p = strndup(fw_name, len);
+	if (!p)
+		return -ENOMEM;
+
+	uc_pdata->fw_name = p;
+
+	return 0;
+}
+
+int rproc_boot(struct udevice *rproc_dev, size_t fw_size)
+{
+	struct dm_rproc_uclass_pdata *uc_pdata;
+	struct udevice *fs_loader;
+	void *addr = malloc(fw_size);
+	int core_id, ret = 0;
+	char *firmware;
+	ulong rproc_addr;
+
+	if (!rproc_dev)
+		return -EINVAL;
+
+	if (!addr)
+		return -ENOMEM;
+
+	uc_pdata = dev_get_uclass_plat(rproc_dev);
+	core_id = dev_seq(rproc_dev);
+	firmware = uc_pdata->fw_name;
+
+	if (!firmware) {
+		debug("No firmware set for rproc core %d\n", core_id);
+		return -EINVAL;
+	}
+
+	/* Initialize all rproc cores */
+	rproc_init();
+
+	/* Loading firmware to a given address */
+	ret = get_fs_loader(&fs_loader);
+	if (ret) {
+		debug("could not get fs loader: %d\n", ret);
+		return ret;
+	}
+
+	ret = request_firmware_into_buf(fs_loader, firmware, addr, fw_size, 0);
+	if (ret < 0) {
+		debug("could not request %s: %d\n", firmware, ret);
+		return ret;
+	}
+
+	rproc_addr = (ulong)addr;
+
+	debug("Loaded %s to 0x%08lX size = %d Bytes\n",
+		uc_pdata->fw_name, rproc_addr, ret);
+
+	ret = rproc_load(core_id, rproc_addr, ret);
+	if (ret) {
+		debug("failed to load %s to rproc core %d from addr 0x%08lX err %d\n",
+		       uc_pdata->fw_name, core_id, rproc_addr, ret);
+		return ret;
+	}
+
+	ret = rproc_start(core_id);
+	if (ret) {
+		debug("failed to start rproc core %d\n", core_id);
+		return ret;
+	}
+
+	return ret;
+}
diff --git a/include/remoteproc.h b/include/remoteproc.h
index a11dc8a9b6..65b0ff7477 100644
--- a/include/remoteproc.h
+++ b/include/remoteproc.h
@@ -402,6 +402,7 @@ enum rproc_mem_type {
  * @name: Platform-specific way of naming the Remote proc
  * @mem_type: one of 'enum rproc_mem_type'
  * @driver_plat_data: driver specific platform data that may be needed.
+ * @fw_name: firmware name
  *
  * This can be accessed with dev_get_uclass_plat() for any
UCLASS_REMOTEPROC
  * device.
@@ -411,6 +412,7 @@ struct dm_rproc_uclass_pdata {
 	const char *name;
 	enum rproc_mem_type mem_type;
 	void *driver_plat_data;
+	char *fw_name;
 };

 /**
@@ -704,6 +706,35 @@ unsigned long rproc_parse_resource_table(struct
udevice *dev,
 struct resource_table *rproc_find_resource_table(struct udevice *dev,
 						 unsigned int addr,
 						 int *tablesz);
+/**
+ * rproc_set_firmware() - assign a new firmware
+ * @rproc_dev: device for wich new firmware is being assigned
+ * @fw_name: new firmware name to be assigned
+ *
+ * This function allows remoteproc drivers or clients to configure a custom
+ * firmware name. The function does not trigger a remote processor boot,
+ * only sets the firmware name used for a subsequent boot.
+ *
+ * This function sets the fw_name field in uclass pdata of the Remote proc
+ *
+ * Return: 0 on success or a negative value upon failure
+ */
+int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name);
+
+/**
+ * rproc_boot() - boot a remote processor
+ * @rproc_dev: rproc device to boot
+ * @fw_size: Size of the memory to allocate for firmeware
+ *
+ * Boot a remote processor (i.e. load its firmware, power it on, ...).
+ *
+ * This function first loads the firmware set in the uclass pdata of Remote
+ * processor to a buffer and then loads firmware to the remote processor
+ * using rproc_load().
+ *
+ * Return: 0 on success, and an appropriate error value otherwise
+ */
+int rproc_boot(struct udevice *rproc_dev, size_t fw_size);
 #else
 static inline int rproc_init(void) { return -ENOSYS; }
 static inline int rproc_dev_init(int id) { return -ENOSYS; }
@@ -743,6 +774,10 @@ static inline int rproc_elf_load_rsc_table(struct
udevice *dev, ulong fw_addr,
 					   ulong fw_size, ulong *rsc_addr,
 					   ulong *rsc_size)
 { return -ENOSYS; }
+static inline int rproc_set_firmware(struct udevice *rproc_dev, const
char *fw_name)
+{ return -ENOSYS; }
+static inline int rproc_boot(struct udevice *rproc_dev, size_t fw_size)
+{ return -ENOSYS; }
 #endif

 #endif	/* _RPROC_H_ */
Andrew Davis Jan. 2, 2024, 1:56 p.m. UTC | #6
On 12/27/23 12:56 AM, MD Danish Anwar wrote:
> 
> 
> On 22/12/23 6:13 pm, Roger Quadros wrote:
>>
>> On 22/12/2023 12:26, MD Danish Anwar wrote:
>>> Hi Roger,
>>>
>>> On 20/12/23 3:29 pm, Roger Quadros wrote:
>>>> Hi,
>>>>
>>>> On 19/12/2023 12:11, MD Danish Anwar wrote:
>>>>> Introduce ICSSG PRUETH support in uboot. The ICSSG driver is used in TI
>>>>> AM654 SR2.0.
>>>>>
>>>>> The ICSSG PRU Sub-system runs on EMAC firmware. This series Introduces
>>>>> support for ICSSG driver in uboot. This series also adds the driver's
>>>>> dependencies.
>>>>>
>>>>> The ICSSG2 node is added in device tree overlay so that it remains in
>>>>> sync with linux kernel.
>>>>>
>>>>> The series introduces device tree and config changes and AM65x
>>>>> to enable ICSSG driver. The series also enables SPL_LOAD_FIT_APPLY_OVERLAY
>>>>> for AM65x in order to load overlay over spl.
>>>>>
>>>>> This series has been tested on AM65x SR2.0, and the ICSSG interface is
>>>>> able to ping / dhcp and boot kernel using tftp in uboot.
>>>>>
>>>>> To use ICSSG2 ethernet, the ICSSG firmware needs to be loaded to PRU RPROC
>>>>> cores and RPROC cores need to be booted with the firmware. This step is
>>>>> done inside driver in kernel as kernel supports APIs like
>>>>> rproc_set_firmware() and rproc_fw_boot(). But as u-boot doesn't have these
>>>>> APIs, the same needs to be done via u-boot cmds.
>>>>>
>>>>> To make sure icssg-eth works we need to do below steps.
>>>>>
>>>>> 1. Initialize rproc cores i.e. rproc_init()
>>>>> 2. Load $firmware_file from partition '1:2' (root) on device (mmc in this
>>>>>     example)
>>>>> 3. Load the firmware file to rproc cores passing. i.e. rproc_load()
>>>>>     taking rproc_id, loadaddr and file size as arguments.
>>>>> 4. Start rproc cores. i.e. rproc_start() taking rproc_id as arguments
>>>>>
>>>>> The above steps are done by running the below commands at u-boot prompt.
>>>>>
>>>>> => setenv start_icssg2 'rproc start 14; rproc start 15; rproc start 16; rproc start 17; rproc start 18; rproc start 19'
>>>>> => setenv stop_icssg2 'rproc stop 14; rproc stop 15; rproc stop 16; rproc stop 17; rproc stop 18; rproc stop 19'
>>>>> => setenv firmware_dir '/lib/firmware/ti-pruss'
>>>>> => setenv get_firmware_mmc 'load mmc ${bootpart} ${loadaddr} ${firmware_dir}/${firmware_file}'
>>>>>
>>>>> => setenv init_icssg2 'setenv ethact icssg2-eth; setenv autoload no; rproc init; setenv loadaddr 0x80000000; \
>>>>>      setenv firmware_file am65x-sr2-pru0-prueth-fw.elf; run get_firmware_mmc;  rproc load 14 0x80000000 ${filesize}; \
>>>>>      setenv loadaddr 0x89000000; setenv firmware_file am65x-sr2-rtu0-prueth-fw.elf; run get_firmware_mmc; rproc load 15 0x89000000 ${filesize}; \
>>>>>      setenv loadaddr 0x90000000; setenv firmware_file am65x-sr2-txpru0-prueth-fw.elf; run get_firmware_mmc; rproc load 16 0x90000000 ${filesize}; \
>>>>>      setenv loadaddr 0x80000000; setenv firmware_file am65x-sr2-pru1-prueth-fw.elf; run get_firmware_mmc; rproc load 17 0x80000000 ${filesize}; \
>>>>>      setenv loadaddr 0x89000000; setenv firmware_file am65x-sr2-rtu1-prueth-fw.elf; run get_firmware_mmc; rproc load 18 0x89000000 ${filesize}; \
>>>>>      setenv loadaddr 0x90000000; setenv firmware_file am65x-sr2-txpru1-prueth-fw.elf; run get_firmware_mmc; rproc load 19 0x90000000 ${filesize}; \
>>>>>      run start_icssg2;'
>>>>
>>>> A whole bunch of commands are required to get ethernet functional.
>>>> This is not at all user friendly and will be a maintenance nightmare.
>>>> What worries me is tracking the 6 different rproc cores and the 6 different firmware files to start 1 ethernet device.
>>>> This will get even more interesting when we have to deal with different ICSSG instances on different boards.
>>>>
>>>> What is preventing the driver from starting the rproc cores it needs so user doesn't need to care about it?
>>>> All the necessary information is in the Device tree. At least this is how it is done on Linux.
>>>>
>>>
>>> I tried removing the need for these commands and implementing them
>>> inside the driver only. I am able to load the firmware from driver using
>>> the fs_loader API request_firmware_into_buf(). It requires changes to
>>> dt. A DT node called fs-loader needs to be added also CONFIG_FS_LOADER
>>> needs to enabled. In the DT node we need to specify the storage media
>>> that we are using i.e. mmc, ospi, usb. It's upto user to modify the
>>> storage media, the driver will take the media from DT and try to laod
>>> firmware from their.
>>>
>>> For loading firmwares to rproc cores, rproc_load() API is needed. Now
>>> this API takes rproc_id, loadaddr and firmware_size as arguments.
>>> loadaddr is fixed for all three pru cores. firmware_size is obtained
>>> from request_firmware_into_buf() but I couldn't find a way to get the
>>> rproc_id. For now based on the ICSSG instance and slice number I am
>>> figuring out the rproc_id and passing it to rproc_load() and
>>> rproc_start() APIs. Please let me know if you have any other / better
>>> way of finding rproc_id.
>>>
>>> Below is the entire diff to remove these commands and move their
>>> functionality to driver. Please have a look and let me know if this is ok.
>>>
>>
>> Good to see you got something working so quickly.
>> It has some rough edges but nothing that is blocking.
>>
>>>
>>> Save New Duplicate & Edit Just Text Twitter
>>> diff --git a/arch/arm/dts/k3-am654-base-board.dts
>>> b/arch/arm/dts/k3-am654-base-board.dts
>>> index cfbcebfa37..c8da72e49c 100644
>>> --- a/arch/arm/dts/k3-am654-base-board.dts
>>> +++ b/arch/arm/dts/k3-am654-base-board.dts
>>> @@ -16,6 +16,13 @@
>>>   	chosen {
>>>   		stdout-path = "serial2:115200n8";
>>>   		bootargs = "earlycon=ns16550a,mmio32,0x02800000";
>>> +		firmware-loader = <&fs_loader0>;
>>> +	};
>>> +
>>> +	fs_loader0: fs-loader {
>>> +		bootph-all;
>>> +		compatible = "u-boot,fs-loader";
>>> +		phandlepart = <&sdhci1 2>;
>>>   	};
>>
>> This is has 2 issues
>> 1) It will not be accepted in Kernel DT. Maybe it could be done in -u-boot.dtsi file.
> 
> Sure. I'll move this to k3-am654-base-board-u-boot.dtsi
> 
>> 2) You cannot assume boot medium is always sdhci1 2
>>
> 
> Yes. It's upto user to provide the boot medium and boot partition for
> loading firmware. By default the firmware is loacated in root partion of
> shdci1 on am65x so I am adding this as default. But user can easily
> modify this to any other medium / partition needed.
> 

Users should not have to modify DT to pick a boot medium/partition.
What would you do for complex cases or non block devices like eth/uart
booting? This is one reason kernel moved firmware loading to
userspace. The equivalant in U-Boot is the shell and scripts. Your
original command based loading was the correct solution IMHO.

If we want to try to hide some of that then we need a way to
run a user provided script from the environement to handle
the general cases for firmware loading.

Andrew

>>>
>>>   	memory@80000000 {
>>> diff --git a/configs/am65x_evm_a53_defconfig
>>> b/configs/am65x_evm_a53_defconfig
>>> index 2755d7082f..c53e938abb 100644
>>> --- a/configs/am65x_evm_a53_defconfig
>>> +++ b/configs/am65x_evm_a53_defconfig
>>> @@ -112,6 +112,7 @@ CONFIG_DM_I2C_GPIO=y
>>>   CONFIG_SYS_I2C_OMAP24XX=y
>>>   CONFIG_DM_MAILBOX=y
>>>   CONFIG_K3_SEC_PROXY=y
>>> +CONFIG_FS_LOADER=y
>>>   CONFIG_SUPPORT_EMMC_BOOT=y
>>>   CONFIG_MMC_IO_VOLTAGE=y
>>>   CONFIG_MMC_UHS_SUPPORT=y
>>> diff --git a/configs/am65x_evm_r5_defconfig b/configs/am65x_evm_r5_defconfig
>>> index b2f1e721b3..2d19935a41 100644
>>> --- a/configs/am65x_evm_r5_defconfig
>>> +++ b/configs/am65x_evm_r5_defconfig
>>> @@ -98,6 +98,7 @@ CONFIG_I2C_SET_DEFAULT_BUS_NUM=y
>>>   CONFIG_SYS_I2C_OMAP24XX=y
>>>   CONFIG_DM_MAILBOX=y
>>>   CONFIG_K3_SEC_PROXY=y
>>> +CONFIG_FS_LOADER=y
>>>   CONFIG_K3_AVS0=y
>>>   CONFIG_SUPPORT_EMMC_BOOT=y
>>>   CONFIG_MMC_HS200_SUPPORT=y
>>> diff --git a/drivers/net/ti/icssg_prueth.c b/drivers/net/ti/icssg_prueth.c
>>> index 40ad827e49..1c4edeb7b7 100644
>>> --- a/drivers/net/ti/icssg_prueth.c
>>> +++ b/drivers/net/ti/icssg_prueth.c
>>> @@ -227,6 +227,10 @@ static int prueth_start(struct udevice *dev)
>>>   	void *config;
>>>   	int ret, i;
>>>
>>> +	ret = icssg_start_pru_cores(dev);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>   	/* To differentiate channels for SLICE0 vs SLICE1 */
>>>   	snprintf(chn_name, sizeof(chn_name), "tx%d-0", priv->slice);
>>>
>>> @@ -355,9 +359,11 @@ static void prueth_stop(struct udevice *dev)
>>>   	phy_shutdown(priv->phydev);
>>>
>>>   	dma_disable(&priv->dma_tx);
>>> -	dma_free(&priv->dma_tx);
>>> -
>>>   	dma_disable(&priv->dma_rx);
>>> +
>>> +	icssg_stop_pru_cores(dev);
>>> +
>>> +	dma_free(&priv->dma_tx);
>>>   	dma_free(&priv->dma_rx);
>>>   }
>>>
>>> @@ -434,6 +440,181 @@ static const struct soc_attr k3_mdio_soc_data[] = {
>>>   	{ /* sentinel */ },
>>>   };
>>>
>>> +struct icssg_firmware_load_address {
>>> +	u32 pru;
>>> +	u32 rtu;
>>> +	u32 txpru;
>>> +};
>>> +
>>> +struct icssg_firmwares {
>>> +	char *pru;
>>> +	char *rtu;
>>> +	char *txpru;
>>> +};
>>> +
>>> +static struct icssg_firmwares icssg_emac_firmwares[] = {
>>> +	{
>>> +		.pru = "/lib/firmware/ti-pruss/am65x-sr2-pru0-prueth-fw.elf",
>>> +		.rtu = "/lib/firmware/ti-pruss/am65x-sr2-rtu0-prueth-fw.elf",
>>> +		.txpru = "/lib/firmware/ti-pruss/am65x-sr2-txpru0-prueth-fw.elf",
>>> +	},
>>> +	{
>>> +		.pru = "/lib/firmware/ti-pruss/am65x-sr2-pru1-prueth-fw.elf",
>>> +		.rtu = "/lib/firmware/ti-pruss/am65x-sr2-rtu1-prueth-fw.elf",
>>> +		.txpru = "/lib/firmware/ti-pruss/am65x-sr2-txpru1-prueth-fw.elf",
>>> +	}
>>> +};
>>
>> This information is contained in the DT.
>>
>> 		firmware-name = "ti-pruss/am65x-sr2-pru0-prueth-fw.elf",
>> 				"ti-pruss/am65x-sr2-rtu0-prueth-fw.elf",
>> 				"ti-pruss/am65x-sr2-txpru0-prueth-fw.elf",
>> 				"ti-pruss/am65x-sr2-pru1-prueth-fw.elf",
>> 				"ti-pruss/am65x-sr2-rtu1-prueth-fw.elf",
>> 				"ti-pruss/am65x-sr2-txpru1-prueth-fw.elf";
>>
> 
> Yes it is. But the linux driver uses the firmware strcuture inside
> driver only. The firmware-name property is not being used. The firmware
> names for all applicable SoCs (AM65x and AM64x) is same so instead of
> reading it from DT, the driver hardcodes the firmware names in linux. I
> think the thought here was to use firmware-name property from DT when
> different firmwares are needed for different SoCs. I am just following
> the same here in uboot as well.
> 
>> You will need to introduce a rproc_set_firmware() API so clients can
>> set their own firmware names.
>>
> 
> Sure. I will add an element 'fw_name' in struct dm_rproc_uclass_pdata
> and set this fw_name to the firmware name requested by client using
> rproc_set_firmware().
> 
> The client drivers can call rproc_set_firmware(dev, firmware_name) to
> set the firmware name.
> 
>>> +
>>> +int load_firmware(char *name_fw, u32 *loadaddr)
>>> +{
>>> +	struct udevice *fsdev;
>>> +	int size = 0;
>>> +
>>> +	if (!IS_ENABLED(CONFIG_FS_LOADER))
>>> +		return -EINVAL;
>>> +
>>> +	if (!*loadaddr)
>>> +		return -EINVAL;
>>> +
>>> +	if (!get_fs_loader(&fsdev))
>>> +		size = request_firmware_into_buf(fsdev, name_fw, (void *)*loadaddr,
>>> 40524, 0);
>>> +
>>> +	return size;
>>> +}
>>
>> On Linux rproc_boot() does both loading the firmware and starting the rproc
>> as that is farely generic.
>> You should introduce rproc_boot() API so loading is taken care of at rproc driver.
>> All you need to do is call rproc_set_firmware() before rproc_boot().
>>
> 
> Sure. I'll introduce rproc_boot() which will load the firmware to a
> buffer and then load the firmware / buffer to rproc core usinig rproc_load.
> 
> For allocating memory for reading firmware we need to provide the size
> of memory required as well. So rproc_boot will take two arguments.
> 1. dev (rproc device)
> 2. size (Maximum memory required to read firmware)
> 
>>> +
>>> +static int icssg_get_instance(struct udevice *dev)
>>> +{
>>> +	if (!strcmp(dev->name, "icssg2-eth"))
>>> +		return 2;
>>> +	else if (!strcmp(dev->name, "icssg1-eth"))
>>> +		return 1;
>>> +	else if (!strcmp(dev->name, "icssg0-eth"))
>>> +		return 0;
>>> +
>>> +	dev_err(dev, "Invalid icssg instance\n");
>>> +	return -EINVAL;
>>> +}
>>> +
>>> +static int icssg_get_pru_core_number(struct udevice *dev, int slice)
>>> +{
>>> +	int instance, num_r5_cores;
>>> +
>>> +	instance = icssg_get_instance(dev);
>>> +	if (instance < 0)
>>> +		return instance;
>>> +
>>> +	if (IS_ENABLED(CONFIG_REMOTEPROC_TI_K3_R5F))
>>> +		num_r5_cores = 2;
>>> +
>>> +	return num_r5_cores +
>>> +	       instance * PRU_TYPE_MAX * PRUETH_NUM_MACS +
>>> +	       slice * PRU_TYPE_MAX;
>>
>> All this doesn't look right. What we need is the rproc device
>> that matches the PRU/RTU rprocs that we are interested in.
>>
>> The DT already has this information
>>
>> 		ti,prus = <&pru2_0>, <&rtu2_0>, <&tx_pru2_0>,
>> 			<&pru2_1>, <&rtu2_1>, <&tx_pru2_1>;
>>
>>
>> All the current rproc APIs use the below to get rproc device from ID
>> ret = uclass_get_device_by_seq(UCLASS_REMOTEPROC, id, &dev);
>>
>> You just need to introduce APIs that takes rproc device directly as argument.
>>
>> In your driver you can call uclass_get_device_by_phandle_id() to get the
>> rproc device from the rproc phandle?
> 
> Sure. I'll do that. Based on the icssg slice first I will get the
> phandle for prus.
> 
> Index will be 0,1,2 for slice 0 and 3,4,5 for slice 1.
> 
> ofnode_read_u32_index(dev_ofnode(dev), "ti,prus", index, &phandle);
> 
> Then for each phandle I can call uclass_get_device_by_phandle_id() and
> get the rproc device.
> 
>>
>>
>>> +}
>>> +
>>> +int icssg_start_pru_cores(struct udevice *dev)
>>> +{
>>> +	struct prueth *prueth = dev_get_priv(dev);
>>> +	struct icssg_firmwares *firmwares;
>>> +	u32 pru_fw_loadaddr = 0x80000000;
>>> +	u32 rtu_fw_loadaddr = 0x89000000;
>>> +	u32 txpru_fw_loadaddr = 0x90000000;
>>
>> Please avoid hardcoding. You can use malloc to get a temporary buffer area?
>>
>> Why do you need 3 different addresses?
>> Once you do a rproc_load isn't the buffer already copied to rproc's memory
>> so you can discard it or use it for the other rprocs?
>>
> 
> Sure I'll use malloc to get temporary buffer. Only thing is that to get
> temporary buffer I will need to provide size as well.
> 
> void *addr = malloc(fw_size);
> 
> User needs to provide this fw_size to rproc_boot(). In my case the max
> size the firmware can have is 64KB so I will be passing 64K as size to
> rproc driver.
> 
>>> +	int slice, ret, core_id;
>>> +
>>> +	firmwares = icssg_emac_firmwares;
>>> +	slice = prueth->slice;
>>> +
>>> +	core_id = icssg_get_pru_core_number(dev, slice);
>>> +	if (core_id < 0)
>>> +		return core_id;
>>> +
>>> +	/* Initialize all rproc cores */
>>> +	rproc_init();
>>> +
>>> +	/* Loading PRU firmware to PRU core */
>>> +	ret = load_firmware(firmwares[slice].pru, &pru_fw_loadaddr);
>>
>> On Linux, loading the firmware is the responsibility of the rproc driver.
>> Shouldn't it be the same in u-boot?
>>
> 
> Sure I'll move this to rproc driver.
> 
>>> +
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "Unable to load_firmware %s at addr 0x%x err %d\n",
>>> +			firmwares[slice].pru, pru_fw_loadaddr, ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	dev_info(dev, "Loaded FW %s successfully at addr 0x%x Size = %d Bytes\n",
>>> +		firmwares[slice].pru, pru_fw_loadaddr, ret);
>>
>> dev_dbg() here an at all dev_info().
>>
>>> +	rproc_load(core_id + PRU_TYPE_PRU, pru_fw_loadaddr, ret);
>>> +
>>> +	/* Loading RTU firmware to RTU core */
>>> +	ret = load_firmware(firmwares[slice].rtu, &rtu_fw_loadaddr);
>>> +
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "Unable to load_firmware %s at addr 0x%x err %d\n",
>>> +			firmwares[slice].rtu, rtu_fw_loadaddr, ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	dev_info(dev, "Loaded FW %s successfully at addr 0x%x Size = %d Bytes\n",
>>> +		firmwares[slice].rtu, rtu_fw_loadaddr, ret);
>>> +	rproc_load(core_id + PRU_TYPE_RTU, rtu_fw_loadaddr, ret);
>>> +
>>> +	/* Loading TX_PRU firmware to TX_PRU core */
>>> +	ret = load_firmware(firmwares[slice].txpru, &txpru_fw_loadaddr);
>>> +
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "Unable to load_firmware %s at addr 0x%x err %d\n",
>>> +			firmwares[slice].txpru, txpru_fw_loadaddr, ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	dev_info(dev, "Loaded FW %s successfully at addr 0x%x Size = %d Bytes\n",
>>> +		firmwares[slice].txpru, txpru_fw_loadaddr, ret);
>>> +	rproc_load(core_id + PRU_TYPE_TX_PRU, txpru_fw_loadaddr, ret);
>>> +
>>> +	ret = rproc_start(core_id + PRU_TYPE_PRU);
>>> +	if (ret) {
>>> +		dev_err(dev, "failed to start PRU%d: %d\n", slice, ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = rproc_start(core_id + PRU_TYPE_RTU);
>>> +	if (ret) {
>>> +		dev_err(dev, "failed to start RTU%d: %d\n", slice, ret);
>>> +		goto halt_pru;
>>> +	}
>>> +
>>> +	ret = rproc_start(core_id + PRU_TYPE_TX_PRU);
>>> +	if (ret) {
>>> +		dev_err(dev, "failed to start TX_PRU%d: %d\n", slice, ret);
>>> +		goto halt_rtu;
>>> +	}
>>> +
>>> +	return 0;
>>> +
>>> +halt_rtu:
>>> +	rproc_stop(core_id + PRU_TYPE_RTU);
>>> +
>>> +halt_pru:
>>> +	rproc_stop(PRU_TYPE_PRU);
>>> +	return ret;
>>> +}
>>> +
>>> +int icssg_stop_pru_cores(struct udevice *dev)
>>> +{
>>> +	struct prueth *prueth = dev_get_priv(dev);
>>> +	int slice, core_id;
>>> +
>>> +	slice = prueth->slice;
>>> +
>>> +	core_id = icssg_get_pru_core_number(dev, slice);
>>> +	if (core_id < 0)
>>> +		return core_id;
>>> +
>>> +	rproc_stop(core_id + PRU_TYPE_PRU);
>>> +	rproc_stop(core_id + PRU_TYPE_RTU);
>>> +	rproc_stop(core_id + PRU_TYPE_TX_PRU);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   static int prueth_probe(struct udevice *dev)
>>>   {
>>>   	ofnode eth_ports_node, eth0_node, eth1_node, eth_node;
>>> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h
>>> index 25272e850e..f17fe8bf58 100644
>>> --- a/include/linux/pruss_driver.h
>>> +++ b/include/linux/pruss_driver.h
>>> @@ -114,6 +114,21 @@ enum pru_ctable_idx {
>>>   	PRU_C31,
>>>   };
>>>
>>> +/**
>>> + * enum pru_type - PRU core type identifier
>>> + *
>>> + * @PRU_TYPE_PRU: Programmable Real-time Unit
>>> + * @PRU_TYPE_RTU: Auxiliary Programmable Real-Time Unit
>>> + * @PRU_TYPE_TX_PRU: Transmit Programmable Real-Time Unit
>>> + * @PRU_TYPE_MAX: just keep this one at the end
>>> + */
>>> +enum pru_type {
>>> +	PRU_TYPE_PRU,
>>> +	PRU_TYPE_RTU,
>>> +	PRU_TYPE_TX_PRU,
>>> +	PRU_TYPE_MAX,
>>> +};
>>> +
>>>   /**
>>>    * enum pruss_mem - PRUSS memory range identifiers
>>>    */
>>>
>>> with this diff, user don't need to run any extra commands at u-boot.
>>> Once u-boot prompt is reached, just running ping / dhcp will suffice.
>>>
>> Great!
>>
>> <snip>
>>
> 
> I have addressed all the above comments and made the changes needed.
> below is the diff. Please have a look at it and let me know if it looks OK.
> 
> diff --git a/arch/arm/dts/k3-am654-base-board-u-boot.dtsi
> b/arch/arm/dts/k3-am654-base-board-u-boot.dtsi
> index 11d83927ac..0b2b72b2c5 100644
> --- a/arch/arm/dts/k3-am654-base-board-u-boot.dtsi
> +++ b/arch/arm/dts/k3-am654-base-board-u-boot.dtsi
> @@ -6,6 +6,18 @@
>   #include "k3-am654-r5-base-board-u-boot.dtsi"
>   #include "k3-am65x-binman.dtsi"
> 
> +/ {
> +	chosen {
> +		firmware-loader = <&fs_loader0>;
> +	};
> +
> +	fs_loader0: fs-loader {
> +		bootph-all;
> +		compatible = "u-boot,fs-loader";
> +		phandlepart = <&sdhci1 2>;
> +	};
> +};
> +
>   &pru0_0 {
>   	remoteproc-name = "pru0_0";
>   };
> diff --git a/configs/am65x_evm_a53_defconfig
> b/configs/am65x_evm_a53_defconfig
> index 2755d7082f..c53e938abb 100644
> --- a/configs/am65x_evm_a53_defconfig
> +++ b/configs/am65x_evm_a53_defconfig
> @@ -112,6 +112,7 @@ CONFIG_DM_I2C_GPIO=y
>   CONFIG_SYS_I2C_OMAP24XX=y
>   CONFIG_DM_MAILBOX=y
>   CONFIG_K3_SEC_PROXY=y
> +CONFIG_FS_LOADER=y
>   CONFIG_SUPPORT_EMMC_BOOT=y
>   CONFIG_MMC_IO_VOLTAGE=y
>   CONFIG_MMC_UHS_SUPPORT=y
> diff --git a/configs/am65x_evm_r5_defconfig b/configs/am65x_evm_r5_defconfig
> index b2f1e721b3..2d19935a41 100644
> --- a/configs/am65x_evm_r5_defconfig
> +++ b/configs/am65x_evm_r5_defconfig
> @@ -98,6 +98,7 @@ CONFIG_I2C_SET_DEFAULT_BUS_NUM=y
>   CONFIG_SYS_I2C_OMAP24XX=y
>   CONFIG_DM_MAILBOX=y
>   CONFIG_K3_SEC_PROXY=y
> +CONFIG_FS_LOADER=y
>   CONFIG_K3_AVS0=y
>   CONFIG_SUPPORT_EMMC_BOOT=y
>   CONFIG_MMC_HS200_SUPPORT=y
> diff --git a/drivers/net/ti/icssg_prueth.c b/drivers/net/ti/icssg_prueth.c
> index 40ad827e49..a2e3595956 100644
> --- a/drivers/net/ti/icssg_prueth.c
> +++ b/drivers/net/ti/icssg_prueth.c
> @@ -62,6 +62,9 @@
>   /* Management packet type */
>   #define PRUETH_PKT_TYPE_CMD		0x10
> 
> +/* Number of PRU Cores per Slice */
> +#define ICSSG_NUM_PRU_CORES		3
> +
>   static int icssg_phy_init(struct udevice *dev)
>   {
>   	struct prueth *priv = dev_get_priv(dev);
> @@ -218,6 +221,116 @@ static int icssg_update_link(struct prueth *priv)
>   	return phy->link;
>   }
> 
> +struct icssg_firmwares {
> +	char *pru;
> +	char *rtu;
> +	char *txpru;
> +};
> +
> +static struct icssg_firmwares icssg_emac_firmwares[] = {
> +	{
> +		.pru = "/lib/firmware/ti-pruss/am65x-sr2-pru0-prueth-fw.elf",
> +		.rtu = "/lib/firmware/ti-pruss/am65x-sr2-rtu0-prueth-fw.elf",
> +		.txpru = "/lib/firmware/ti-pruss/am65x-sr2-txpru0-prueth-fw.elf",
> +	},
> +	{
> +		.pru = "/lib/firmware/ti-pruss/am65x-sr2-pru1-prueth-fw.elf",
> +		.rtu = "/lib/firmware/ti-pruss/am65x-sr2-rtu1-prueth-fw.elf",
> +		.txpru = "/lib/firmware/ti-pruss/am65x-sr2-txpru1-prueth-fw.elf",
> +	}
> +};
> +
> +static int icssg_start_pru_cores(struct udevice *dev)
> +{
> +	struct prueth *prueth = dev_get_priv(dev);
> +	struct icssg_firmwares *firmwares;
> +	struct udevice *rproc_dev = NULL;
> +	int ret, slice;
> +	u32 phandle;
> +	u8 index;
> +
> +	slice = prueth->slice;
> +	index = slice * ICSSG_NUM_PRU_CORES;
> +	firmwares = icssg_emac_firmwares;
> +
> +	ofnode_read_u32_index(dev_ofnode(dev), "ti,prus", index, &phandle);
> +	ret = uclass_get_device_by_phandle_id(UCLASS_REMOTEPROC, phandle,
> &rproc_dev);
> +	if (ret) {
> +		dev_err(dev, "Unknown remote processor with phandle '0x%x'
> requested(%d)\n",
> +		       phandle, ret);
> +		return ret;
> +	}
> +
> +	prueth->pru_core_id = dev_seq(rproc_dev);
> +	ret = rproc_set_firmware(rproc_dev, firmwares[slice].pru);
> +	if (ret)
> +		return ret;
> +
> +	ret = rproc_boot(rproc_dev, SZ_64K);
> +	if (ret) {
> +		dev_err(dev, "failed to boot PRU%d: %d\n", slice, ret);
> +		return -EINVAL;
> +	}
> +
> +	ofnode_read_u32_index(dev_ofnode(dev), "ti,prus", index + 1, &phandle);
> +	ret = uclass_get_device_by_phandle_id(UCLASS_REMOTEPROC, phandle,
> &rproc_dev);
> +	if (ret) {
> +		dev_err(dev, "Unknown remote processor with phandle '0x%x'
> requested(%d)\n",
> +		       phandle, ret);
> +		goto halt_pru;
> +	}
> +
> +	prueth->rtu_core_id = dev_seq(rproc_dev);
> +	ret = rproc_set_firmware(rproc_dev, firmwares[slice].rtu);
> +	if (ret)
> +		goto halt_pru;
> +
> +	ret = rproc_boot(rproc_dev, SZ_64K);
> +	if (ret) {
> +		dev_err(dev, "failed to boot RTU%d: %d\n", slice, ret);
> +		goto halt_pru;
> +	}
> +
> +	ofnode_read_u32_index(dev_ofnode(dev), "ti,prus", index + 2, &phandle);
> +	ret = uclass_get_device_by_phandle_id(UCLASS_REMOTEPROC, phandle,
> &rproc_dev);
> +	if (ret) {
> +		dev_err(dev, "Unknown remote processor with phandle '0x%x'
> requested(%d)\n",
> +		       phandle, ret);
> +		goto halt_rtu;
> +	}
> +
> +	prueth->txpru_core_id = dev_seq(rproc_dev);
> +	ret = rproc_set_firmware(rproc_dev, firmwares[slice].txpru);
> +	if (ret)
> +		goto halt_rtu;
> +
> +	ret = rproc_boot(rproc_dev, SZ_64K);
> +	if (ret) {
> +		dev_err(dev, "failed to boot TXPRU%d: %d\n", slice, ret);
> +		goto halt_rtu;
> +	}
> +
> +	return 0;
> +
> +halt_rtu:
> +	rproc_stop(prueth->rtu_core_id);
> +
> +halt_pru:
> +	rproc_stop(prueth->pru_core_id);
> +	return ret;
> +}
> +
> +static int icssg_stop_pru_cores(struct udevice *dev)
> +{
> +	struct prueth *prueth = dev_get_priv(dev);
> +
> +	rproc_stop(prueth->pru_core_id);
> +	rproc_stop(prueth->rtu_core_id);
> +	rproc_stop(prueth->txpru_core_id);
> +
> +	return 0;
> +}
> +
>   static int prueth_start(struct udevice *dev)
>   {
>   	struct ti_udma_drv_chan_cfg_data *dma_rx_cfg_data;
> @@ -227,6 +340,10 @@ static int prueth_start(struct udevice *dev)
>   	void *config;
>   	int ret, i;
> 
> +	ret = icssg_start_pru_cores(dev);
> +	if (ret)
> +		return ret;
> +
>   	/* To differentiate channels for SLICE0 vs SLICE1 */
>   	snprintf(chn_name, sizeof(chn_name), "tx%d-0", priv->slice);
> 
> @@ -355,9 +472,11 @@ static void prueth_stop(struct udevice *dev)
>   	phy_shutdown(priv->phydev);
> 
>   	dma_disable(&priv->dma_tx);
> -	dma_free(&priv->dma_tx);
> -
>   	dma_disable(&priv->dma_rx);
> +
> +	icssg_stop_pru_cores(dev);
> +
> +	dma_free(&priv->dma_tx);
>   	dma_free(&priv->dma_rx);
>   }
> 
> diff --git a/drivers/net/ti/icssg_prueth.h b/drivers/net/ti/icssg_prueth.h
> index e41ed16a05..c92660401b 100644
> --- a/drivers/net/ti/icssg_prueth.h
> +++ b/drivers/net/ti/icssg_prueth.h
> @@ -69,6 +69,9 @@ struct prueth {
>   	int			speed;
>   	int			duplex;
>   	u8			icssg_hwcmdseq;
> +	u8 			pru_core_id;
> +	u8 			rtu_core_id;
> +	u8 			txpru_core_id;
>   };
> 
>   /* config helpers */
> diff --git a/drivers/remoteproc/rproc-uclass.c
> b/drivers/remoteproc/rproc-uclass.c
> index 28b362c887..a085c44eaa 100644
> --- a/drivers/remoteproc/rproc-uclass.c
> +++ b/drivers/remoteproc/rproc-uclass.c
> @@ -13,6 +13,7 @@
>   #include <log.h>
>   #include <malloc.h>
>   #include <virtio_ring.h>
> +#include <fs_loader.h>
>   #include <remoteproc.h>
>   #include <asm/io.h>
>   #include <dm/device-internal.h>
> @@ -961,3 +962,90 @@ unsigned long rproc_parse_resource_table(struct
> udevice *dev, struct rproc *cfg)
> 
>   	return 1;
>   }
> +
> +int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name)
> +{
> +	struct dm_rproc_uclass_pdata *uc_pdata;
> +	int len;
> +	char *p;
> +
> +	if (!rproc_dev || !fw_name)
> +		return -EINVAL;
> +
> +	uc_pdata = dev_get_uclass_plat(rproc_dev);
> +
> +	len = strcspn(fw_name, "\n");
> +	if (!len) {
> +		debug("can't provide empty string for firmware name\n");
> +		return -EINVAL;
> +	}
> +
> +	p = strndup(fw_name, len);
> +	if (!p)
> +		return -ENOMEM;
> +
> +	uc_pdata->fw_name = p;
> +
> +	return 0;
> +}
> +
> +int rproc_boot(struct udevice *rproc_dev, size_t fw_size)
> +{
> +	struct dm_rproc_uclass_pdata *uc_pdata;
> +	struct udevice *fs_loader;
> +	void *addr = malloc(fw_size);
> +	int core_id, ret = 0;
> +	char *firmware;
> +	ulong rproc_addr;
> +
> +	if (!rproc_dev)
> +		return -EINVAL;
> +
> +	if (!addr)
> +		return -ENOMEM;
> +
> +	uc_pdata = dev_get_uclass_plat(rproc_dev);
> +	core_id = dev_seq(rproc_dev);
> +	firmware = uc_pdata->fw_name;
> +
> +	if (!firmware) {
> +		debug("No firmware set for rproc core %d\n", core_id);
> +		return -EINVAL;
> +	}
> +
> +	/* Initialize all rproc cores */
> +	rproc_init();
> +
> +	/* Loading firmware to a given address */
> +	ret = get_fs_loader(&fs_loader);
> +	if (ret) {
> +		debug("could not get fs loader: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = request_firmware_into_buf(fs_loader, firmware, addr, fw_size, 0);
> +	if (ret < 0) {
> +		debug("could not request %s: %d\n", firmware, ret);
> +		return ret;
> +	}
> +
> +	rproc_addr = (ulong)addr;
> +
> +	debug("Loaded %s to 0x%08lX size = %d Bytes\n",
> +		uc_pdata->fw_name, rproc_addr, ret);
> +
> +	ret = rproc_load(core_id, rproc_addr, ret);
> +	if (ret) {
> +		debug("failed to load %s to rproc core %d from addr 0x%08lX err %d\n",
> +		       uc_pdata->fw_name, core_id, rproc_addr, ret);
> +		return ret;
> +	}
> +
> +	ret = rproc_start(core_id);
> +	if (ret) {
> +		debug("failed to start rproc core %d\n", core_id);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> diff --git a/include/remoteproc.h b/include/remoteproc.h
> index a11dc8a9b6..65b0ff7477 100644
> --- a/include/remoteproc.h
> +++ b/include/remoteproc.h
> @@ -402,6 +402,7 @@ enum rproc_mem_type {
>    * @name: Platform-specific way of naming the Remote proc
>    * @mem_type: one of 'enum rproc_mem_type'
>    * @driver_plat_data: driver specific platform data that may be needed.
> + * @fw_name: firmware name
>    *
>    * This can be accessed with dev_get_uclass_plat() for any
> UCLASS_REMOTEPROC
>    * device.
> @@ -411,6 +412,7 @@ struct dm_rproc_uclass_pdata {
>   	const char *name;
>   	enum rproc_mem_type mem_type;
>   	void *driver_plat_data;
> +	char *fw_name;
>   };
> 
>   /**
> @@ -704,6 +706,35 @@ unsigned long rproc_parse_resource_table(struct
> udevice *dev,
>   struct resource_table *rproc_find_resource_table(struct udevice *dev,
>   						 unsigned int addr,
>   						 int *tablesz);
> +/**
> + * rproc_set_firmware() - assign a new firmware
> + * @rproc_dev: device for wich new firmware is being assigned
> + * @fw_name: new firmware name to be assigned
> + *
> + * This function allows remoteproc drivers or clients to configure a custom
> + * firmware name. The function does not trigger a remote processor boot,
> + * only sets the firmware name used for a subsequent boot.
> + *
> + * This function sets the fw_name field in uclass pdata of the Remote proc
> + *
> + * Return: 0 on success or a negative value upon failure
> + */
> +int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name);
> +
> +/**
> + * rproc_boot() - boot a remote processor
> + * @rproc_dev: rproc device to boot
> + * @fw_size: Size of the memory to allocate for firmeware
> + *
> + * Boot a remote processor (i.e. load its firmware, power it on, ...).
> + *
> + * This function first loads the firmware set in the uclass pdata of Remote
> + * processor to a buffer and then loads firmware to the remote processor
> + * using rproc_load().
> + *
> + * Return: 0 on success, and an appropriate error value otherwise
> + */
> +int rproc_boot(struct udevice *rproc_dev, size_t fw_size);
>   #else
>   static inline int rproc_init(void) { return -ENOSYS; }
>   static inline int rproc_dev_init(int id) { return -ENOSYS; }
> @@ -743,6 +774,10 @@ static inline int rproc_elf_load_rsc_table(struct
> udevice *dev, ulong fw_addr,
>   					   ulong fw_size, ulong *rsc_addr,
>   					   ulong *rsc_size)
>   { return -ENOSYS; }
> +static inline int rproc_set_firmware(struct udevice *rproc_dev, const
> char *fw_name)
> +{ return -ENOSYS; }
> +static inline int rproc_boot(struct udevice *rproc_dev, size_t fw_size)
> +{ return -ENOSYS; }
>   #endif
> 
>   #endif	/* _RPROC_H_ */
> 
>
MD Danish Anwar Jan. 3, 2024, 10:27 a.m. UTC | #7
On 02/01/24 7:26 pm, Andrew Davis wrote:
> On 12/27/23 12:56 AM, MD Danish Anwar wrote:
>>
>>
>> On 22/12/23 6:13 pm, Roger Quadros wrote:
>>>
>>> On 22/12/2023 12:26, MD Danish Anwar wrote:
>>>> Hi Roger,
>>>>
>>>> On 20/12/23 3:29 pm, Roger Quadros wrote:
>>>>> Hi,
>>>>>
>>>>> On 19/12/2023 12:11, MD Danish Anwar wrote:
>>>>>> Introduce ICSSG PRUETH support in uboot. The ICSSG driver is used
>>>>>> in TI
>>>>>> AM654 SR2.0.
>>>>>>
>>>>>> The ICSSG PRU Sub-system runs on EMAC firmware. This series
>>>>>> Introduces
>>>>>> support for ICSSG driver in uboot. This series also adds the driver's
>>>>>> dependencies.
>>>>>>
>>>>>> The ICSSG2 node is added in device tree overlay so that it remains in
>>>>>> sync with linux kernel.
>>>>>>
>>>>>> The series introduces device tree and config changes and AM65x
>>>>>> to enable ICSSG driver. The series also enables
>>>>>> SPL_LOAD_FIT_APPLY_OVERLAY
>>>>>> for AM65x in order to load overlay over spl.
>>>>>>
>>>>>> This series has been tested on AM65x SR2.0, and the ICSSG
>>>>>> interface is
>>>>>> able to ping / dhcp and boot kernel using tftp in uboot.
>>>>>>
>>>>>> To use ICSSG2 ethernet, the ICSSG firmware needs to be loaded to
>>>>>> PRU RPROC
>>>>>> cores and RPROC cores need to be booted with the firmware. This
>>>>>> step is
>>>>>> done inside driver in kernel as kernel supports APIs like
>>>>>> rproc_set_firmware() and rproc_fw_boot(). But as u-boot doesn't
>>>>>> have these
>>>>>> APIs, the same needs to be done via u-boot cmds.
>>>>>>
>>>>>> To make sure icssg-eth works we need to do below steps.
>>>>>>
>>>>>> 1. Initialize rproc cores i.e. rproc_init()
>>>>>> 2. Load $firmware_file from partition '1:2' (root) on device (mmc
>>>>>> in this
>>>>>>     example)
>>>>>> 3. Load the firmware file to rproc cores passing. i.e. rproc_load()
>>>>>>     taking rproc_id, loadaddr and file size as arguments.
>>>>>> 4. Start rproc cores. i.e. rproc_start() taking rproc_id as arguments
>>>>>>
>>>>>> The above steps are done by running the below commands at u-boot
>>>>>> prompt.
>>>>>>
>>>>>> => setenv start_icssg2 'rproc start 14; rproc start 15; rproc
>>>>>> start 16; rproc start 17; rproc start 18; rproc start 19'
>>>>>> => setenv stop_icssg2 'rproc stop 14; rproc stop 15; rproc stop
>>>>>> 16; rproc stop 17; rproc stop 18; rproc stop 19'
>>>>>> => setenv firmware_dir '/lib/firmware/ti-pruss'
>>>>>> => setenv get_firmware_mmc 'load mmc ${bootpart} ${loadaddr}
>>>>>> ${firmware_dir}/${firmware_file}'
>>>>>>
>>>>>> => setenv init_icssg2 'setenv ethact icssg2-eth; setenv autoload
>>>>>> no; rproc init; setenv loadaddr 0x80000000; \
>>>>>>      setenv firmware_file am65x-sr2-pru0-prueth-fw.elf; run
>>>>>> get_firmware_mmc;  rproc load 14 0x80000000 ${filesize}; \
>>>>>>      setenv loadaddr 0x89000000; setenv firmware_file
>>>>>> am65x-sr2-rtu0-prueth-fw.elf; run get_firmware_mmc; rproc load 15
>>>>>> 0x89000000 ${filesize}; \
>>>>>>      setenv loadaddr 0x90000000; setenv firmware_file
>>>>>> am65x-sr2-txpru0-prueth-fw.elf; run get_firmware_mmc; rproc load
>>>>>> 16 0x90000000 ${filesize}; \
>>>>>>      setenv loadaddr 0x80000000; setenv firmware_file
>>>>>> am65x-sr2-pru1-prueth-fw.elf; run get_firmware_mmc; rproc load 17
>>>>>> 0x80000000 ${filesize}; \
>>>>>>      setenv loadaddr 0x89000000; setenv firmware_file
>>>>>> am65x-sr2-rtu1-prueth-fw.elf; run get_firmware_mmc; rproc load 18
>>>>>> 0x89000000 ${filesize}; \
>>>>>>      setenv loadaddr 0x90000000; setenv firmware_file
>>>>>> am65x-sr2-txpru1-prueth-fw.elf; run get_firmware_mmc; rproc load
>>>>>> 19 0x90000000 ${filesize}; \
>>>>>>      run start_icssg2;'
>>>>>
>>>>> A whole bunch of commands are required to get ethernet functional.
>>>>> This is not at all user friendly and will be a maintenance nightmare.
>>>>> What worries me is tracking the 6 different rproc cores and the 6
>>>>> different firmware files to start 1 ethernet device.
>>>>> This will get even more interesting when we have to deal with
>>>>> different ICSSG instances on different boards.
>>>>>
>>>>> What is preventing the driver from starting the rproc cores it
>>>>> needs so user doesn't need to care about it?
>>>>> All the necessary information is in the Device tree. At least this
>>>>> is how it is done on Linux.
>>>>>
>>>>
>>>> I tried removing the need for these commands and implementing them
>>>> inside the driver only. I am able to load the firmware from driver
>>>> using
>>>> the fs_loader API request_firmware_into_buf(). It requires changes to
>>>> dt. A DT node called fs-loader needs to be added also CONFIG_FS_LOADER
>>>> needs to enabled. In the DT node we need to specify the storage media
>>>> that we are using i.e. mmc, ospi, usb. It's upto user to modify the
>>>> storage media, the driver will take the media from DT and try to laod
>>>> firmware from their.
>>>>
>>>> For loading firmwares to rproc cores, rproc_load() API is needed. Now
>>>> this API takes rproc_id, loadaddr and firmware_size as arguments.
>>>> loadaddr is fixed for all three pru cores. firmware_size is obtained
>>>> from request_firmware_into_buf() but I couldn't find a way to get the
>>>> rproc_id. For now based on the ICSSG instance and slice number I am
>>>> figuring out the rproc_id and passing it to rproc_load() and
>>>> rproc_start() APIs. Please let me know if you have any other / better
>>>> way of finding rproc_id.
>>>>
>>>> Below is the entire diff to remove these commands and move their
>>>> functionality to driver. Please have a look and let me know if this
>>>> is ok.
>>>>
>>>
>>> Good to see you got something working so quickly.
>>> It has some rough edges but nothing that is blocking.
>>>
>>>>
>>>> Save New Duplicate & Edit Just Text Twitter
>>>> diff --git a/arch/arm/dts/k3-am654-base-board.dts
>>>> b/arch/arm/dts/k3-am654-base-board.dts
>>>> index cfbcebfa37..c8da72e49c 100644
>>>> --- a/arch/arm/dts/k3-am654-base-board.dts
>>>> +++ b/arch/arm/dts/k3-am654-base-board.dts
>>>> @@ -16,6 +16,13 @@
>>>>       chosen {
>>>>           stdout-path = "serial2:115200n8";
>>>>           bootargs = "earlycon=ns16550a,mmio32,0x02800000";
>>>> +        firmware-loader = <&fs_loader0>;
>>>> +    };
>>>> +
>>>> +    fs_loader0: fs-loader {
>>>> +        bootph-all;
>>>> +        compatible = "u-boot,fs-loader";
>>>> +        phandlepart = <&sdhci1 2>;
>>>>       };
>>>
>>> This is has 2 issues
>>> 1) It will not be accepted in Kernel DT. Maybe it could be done in
>>> -u-boot.dtsi file.
>>
>> Sure. I'll move this to k3-am654-base-board-u-boot.dtsi
>>
>>> 2) You cannot assume boot medium is always sdhci1 2
>>>
>>
>> Yes. It's upto user to provide the boot medium and boot partition for
>> loading firmware. By default the firmware is loacated in root partion of
>> shdci1 on am65x so I am adding this as default. But user can easily
>> modify this to any other medium / partition needed.
>>
> 
> Users should not have to modify DT to pick a boot medium/partition.
> What would you do for complex cases or non block devices like eth/uart

In order to load firmware files from driver, we need to add the node for
fs-loader. The fs-loader has a phandlepart property which takes the boot
medium and the partition as input. Based on the medium and partition it
looks for the requested file and loads it to a given address. I am not
sure if there is any other way to load firmware from driver without
using the fs-loader.


> booting? This is one reason kernel moved firmware loading to
> userspace. The equivalant in U-Boot is the shell and scripts. Your
> original command based loading was the correct solution IMHO.

I intended to go ahead with command base approach only but as Roger
pointed out the command based approach is not very user friendly.

We need to align on what should be the correct approach for loading
firmware.

Roger, can you please chime in here.

> 
> If we want to try to hide some of that then we need a way to
> run a user provided script from the environement to handle
> the general cases for firmware loading.
> 
> Andrew
> 
>>>>
>>>>       memory@80000000 {
>>>> diff --git a/configs/am65x_evm_a53_defconfig
>>>> b/configs/am65x_evm_a53_defconfig
>>>> index 2755d7082f..c53e938abb 100644
>>>> --- a/configs/am65x_evm_a53_defconfig
>>>> +++ b/configs/am65x_evm_a53_defconfig
>>>> @@ -112,6 +112,7 @@ CONFIG_DM_I2C_GPIO=y
>>>>   CONFIG_SYS_I2C_OMAP24XX=y
>>>>   CONFIG_DM_MAILBOX=y
>>>>   CONFIG_K3_SEC_PROXY=y
>>>> +CONFIG_FS_LOADER=y
>>>>   CONFIG_SUPPORT_EMMC_BOOT=y
>>>>   CONFIG_MMC_IO_VOLTAGE=y
>>>>   CONFIG_MMC_UHS_SUPPORT=y
>>>> diff --git a/configs/am65x_evm_r5_defconfig
>>>> b/configs/am65x_evm_r5_defconfig
>>>> index b2f1e721b3..2d19935a41 100644
>>>> --- a/configs/am65x_evm_r5_defconfig
>>>> +++ b/configs/am65x_evm_r5_defconfig
>>>> @@ -98,6 +98,7 @@ CONFIG_I2C_SET_DEFAULT_BUS_NUM=y
>>>>   CONFIG_SYS_I2C_OMAP24XX=y
>>>>   CONFIG_DM_MAILBOX=y
>>>>   CONFIG_K3_SEC_PROXY=y
>>>> +CONFIG_FS_LOADER=y
>>>>   CONFIG_K3_AVS0=y
>>>>   CONFIG_SUPPORT_EMMC_BOOT=y
>>>>   CONFIG_MMC_HS200_SUPPORT=y
>>>> diff --git a/drivers/net/ti/icssg_prueth.c
>>>> b/drivers/net/ti/icssg_prueth.c
>>>> index 40ad827e49..1c4edeb7b7 100644
>>>> --- a/drivers/net/ti/icssg_prueth.c
>>>> +++ b/drivers/net/ti/icssg_prueth.c
>>>> @@ -227,6 +227,10 @@ static int prueth_start(struct udevice *dev)
>>>>       void *config;
>>>>       int ret, i;
>>>>
>>>> +    ret = icssg_start_pru_cores(dev);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>>       /* To differentiate channels for SLICE0 vs SLICE1 */
>>>>       snprintf(chn_name, sizeof(chn_name), "tx%d-0", priv->slice);
>>>>
>>>> @@ -355,9 +359,11 @@ static void prueth_stop(struct udevice *dev)
>>>>       phy_shutdown(priv->phydev);
>>>>
>>>>       dma_disable(&priv->dma_tx);
>>>> -    dma_free(&priv->dma_tx);
>>>> -
>>>>       dma_disable(&priv->dma_rx);
>>>> +
>>>> +    icssg_stop_pru_cores(dev);
>>>> +
>>>> +    dma_free(&priv->dma_tx);
>>>>       dma_free(&priv->dma_rx);
>>>>   }
>>>>
>>>> @@ -434,6 +440,181 @@ static const struct soc_attr
>>>> k3_mdio_soc_data[] = {
>>>>       { /* sentinel */ },
>>>>   };
>>>>
>>>> +struct icssg_firmware_load_address {
>>>> +    u32 pru;
>>>> +    u32 rtu;
>>>> +    u32 txpru;
>>>> +};
>>>> +
>>>> +struct icssg_firmwares {
>>>> +    char *pru;
>>>> +    char *rtu;
>>>> +    char *txpru;
>>>> +};
>>>> +
>>>> +static struct icssg_firmwares icssg_emac_firmwares[] = {
>>>> +    {
>>>> +        .pru = "/lib/firmware/ti-pruss/am65x-sr2-pru0-prueth-fw.elf",
>>>> +        .rtu = "/lib/firmware/ti-pruss/am65x-sr2-rtu0-prueth-fw.elf",
>>>> +        .txpru =
>>>> "/lib/firmware/ti-pruss/am65x-sr2-txpru0-prueth-fw.elf",
>>>> +    },
>>>> +    {
>>>> +        .pru = "/lib/firmware/ti-pruss/am65x-sr2-pru1-prueth-fw.elf",
>>>> +        .rtu = "/lib/firmware/ti-pruss/am65x-sr2-rtu1-prueth-fw.elf",
>>>> +        .txpru =
>>>> "/lib/firmware/ti-pruss/am65x-sr2-txpru1-prueth-fw.elf",
>>>> +    }
>>>> +};
>>>
>>> This information is contained in the DT.
>>>
>>>         firmware-name = "ti-pruss/am65x-sr2-pru0-prueth-fw.elf",
>>>                 "ti-pruss/am65x-sr2-rtu0-prueth-fw.elf",
>>>                 "ti-pruss/am65x-sr2-txpru0-prueth-fw.elf",
>>>                 "ti-pruss/am65x-sr2-pru1-prueth-fw.elf",
>>>                 "ti-pruss/am65x-sr2-rtu1-prueth-fw.elf",
>>>                 "ti-pruss/am65x-sr2-txpru1-prueth-fw.elf";
>>>
>>
>> Yes it is. But the linux driver uses the firmware strcuture inside
>> driver only. The firmware-name property is not being used. The firmware
>> names for all applicable SoCs (AM65x and AM64x) is same so instead of
>> reading it from DT, the driver hardcodes the firmware names in linux. I
>> think the thought here was to use firmware-name property from DT when
>> different firmwares are needed for different SoCs. I am just following
>> the same here in uboot as well.
>>
>>> You will need to introduce a rproc_set_firmware() API so clients can
>>> set their own firmware names.
>>>
>>
>> Sure. I will add an element 'fw_name' in struct dm_rproc_uclass_pdata
>> and set this fw_name to the firmware name requested by client using
>> rproc_set_firmware().
>>
>> The client drivers can call rproc_set_firmware(dev, firmware_name) to
>> set the firmware name.
>>
>>>> +
>>>> +int load_firmware(char *name_fw, u32 *loadaddr)
>>>> +{
>>>> +    struct udevice *fsdev;
>>>> +    int size = 0;
>>>> +
>>>> +    if (!IS_ENABLED(CONFIG_FS_LOADER))
>>>> +        return -EINVAL;
>>>> +
>>>> +    if (!*loadaddr)
>>>> +        return -EINVAL;
>>>> +
>>>> +    if (!get_fs_loader(&fsdev))
>>>> +        size = request_firmware_into_buf(fsdev, name_fw, (void
>>>> *)*loadaddr,
>>>> 40524, 0);
>>>> +
>>>> +    return size;
>>>> +}
>>>
>>> On Linux rproc_boot() does both loading the firmware and starting the
>>> rproc
>>> as that is farely generic.
>>> You should introduce rproc_boot() API so loading is taken care of at
>>> rproc driver.
>>> All you need to do is call rproc_set_firmware() before rproc_boot().
>>>
>>
>> Sure. I'll introduce rproc_boot() which will load the firmware to a
>> buffer and then load the firmware / buffer to rproc core usinig
>> rproc_load.
>>
>> For allocating memory for reading firmware we need to provide the size
>> of memory required as well. So rproc_boot will take two arguments.
>> 1. dev (rproc device)
>> 2. size (Maximum memory required to read firmware)
>>
>>>> +
>>>> +static int icssg_get_instance(struct udevice *dev)
>>>> +{
>>>> +    if (!strcmp(dev->name, "icssg2-eth"))
>>>> +        return 2;
>>>> +    else if (!strcmp(dev->name, "icssg1-eth"))
>>>> +        return 1;
>>>> +    else if (!strcmp(dev->name, "icssg0-eth"))
>>>> +        return 0;
>>>> +
>>>> +    dev_err(dev, "Invalid icssg instance\n");
>>>> +    return -EINVAL;
>>>> +}
>>>> +
>>>> +static int icssg_get_pru_core_number(struct udevice *dev, int slice)
>>>> +{
>>>> +    int instance, num_r5_cores;
>>>> +
>>>> +    instance = icssg_get_instance(dev);
>>>> +    if (instance < 0)
>>>> +        return instance;
>>>> +
>>>> +    if (IS_ENABLED(CONFIG_REMOTEPROC_TI_K3_R5F))
>>>> +        num_r5_cores = 2;
>>>> +
>>>> +    return num_r5_cores +
>>>> +           instance * PRU_TYPE_MAX * PRUETH_NUM_MACS +
>>>> +           slice * PRU_TYPE_MAX;
>>>
>>> All this doesn't look right. What we need is the rproc device
>>> that matches the PRU/RTU rprocs that we are interested in.
>>>
>>> The DT already has this information
>>>
>>>         ti,prus = <&pru2_0>, <&rtu2_0>, <&tx_pru2_0>,
>>>             <&pru2_1>, <&rtu2_1>, <&tx_pru2_1>;
>>>
>>>
>>> All the current rproc APIs use the below to get rproc device from ID
>>> ret = uclass_get_device_by_seq(UCLASS_REMOTEPROC, id, &dev);
>>>
>>> You just need to introduce APIs that takes rproc device directly as
>>> argument.
>>>
>>> In your driver you can call uclass_get_device_by_phandle_id() to get the
>>> rproc device from the rproc phandle?
>>
>> Sure. I'll do that. Based on the icssg slice first I will get the
>> phandle for prus.
>>
>> Index will be 0,1,2 for slice 0 and 3,4,5 for slice 1.
>>
>> ofnode_read_u32_index(dev_ofnode(dev), "ti,prus", index, &phandle);
>>
>> Then for each phandle I can call uclass_get_device_by_phandle_id() and
>> get the rproc device.
>>
>>>
>>>
>>>> +}
>>>> +
>>>> +int icssg_start_pru_cores(struct udevice *dev)
>>>> +{
>>>> +    struct prueth *prueth = dev_get_priv(dev);
>>>> +    struct icssg_firmwares *firmwares;
>>>> +    u32 pru_fw_loadaddr = 0x80000000;
>>>> +    u32 rtu_fw_loadaddr = 0x89000000;
>>>> +    u32 txpru_fw_loadaddr = 0x90000000;
>>>
>>> Please avoid hardcoding. You can use malloc to get a temporary buffer
>>> area?
>>>
>>> Why do you need 3 different addresses?
>>> Once you do a rproc_load isn't the buffer already copied to rproc's
>>> memory
>>> so you can discard it or use it for the other rprocs?
>>>
>>
>> Sure I'll use malloc to get temporary buffer. Only thing is that to get
>> temporary buffer I will need to provide size as well.
>>
>> void *addr = malloc(fw_size);
>>
>> User needs to provide this fw_size to rproc_boot(). In my case the max
>> size the firmware can have is 64KB so I will be passing 64K as size to
>> rproc driver.
>>
>>>> +    int slice, ret, core_id;
>>>> +
>>>> +    firmwares = icssg_emac_firmwares;
>>>> +    slice = prueth->slice;
>>>> +
>>>> +    core_id = icssg_get_pru_core_number(dev, slice);
>>>> +    if (core_id < 0)
>>>> +        return core_id;
>>>> +
>>>> +    /* Initialize all rproc cores */
>>>> +    rproc_init();
>>>> +
>>>> +    /* Loading PRU firmware to PRU core */
>>>> +    ret = load_firmware(firmwares[slice].pru, &pru_fw_loadaddr);
>>>
>>> On Linux, loading the firmware is the responsibility of the rproc
>>> driver.
>>> Shouldn't it be the same in u-boot?
>>>
>>
>> Sure I'll move this to rproc driver.
>>
>>>> +
>>>> +    if (ret < 0) {
>>>> +        dev_err(dev, "Unable to load_firmware %s at addr 0x%x err
>>>> %d\n",
>>>> +            firmwares[slice].pru, pru_fw_loadaddr, ret);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    dev_info(dev, "Loaded FW %s successfully at addr 0x%x Size = %d
>>>> Bytes\n",
>>>> +        firmwares[slice].pru, pru_fw_loadaddr, ret);
>>>
>>> dev_dbg() here an at all dev_info().
>>>
>>>> +    rproc_load(core_id + PRU_TYPE_PRU, pru_fw_loadaddr, ret);
>>>> +
>>>> +    /* Loading RTU firmware to RTU core */
>>>> +    ret = load_firmware(firmwares[slice].rtu, &rtu_fw_loadaddr);
>>>> +
>>>> +    if (ret < 0) {
>>>> +        dev_err(dev, "Unable to load_firmware %s at addr 0x%x err
>>>> %d\n",
>>>> +            firmwares[slice].rtu, rtu_fw_loadaddr, ret);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    dev_info(dev, "Loaded FW %s successfully at addr 0x%x Size = %d
>>>> Bytes\n",
>>>> +        firmwares[slice].rtu, rtu_fw_loadaddr, ret);
>>>> +    rproc_load(core_id + PRU_TYPE_RTU, rtu_fw_loadaddr, ret);
>>>> +
>>>> +    /* Loading TX_PRU firmware to TX_PRU core */
>>>> +    ret = load_firmware(firmwares[slice].txpru, &txpru_fw_loadaddr);
>>>> +
>>>> +    if (ret < 0) {
>>>> +        dev_err(dev, "Unable to load_firmware %s at addr 0x%x err
>>>> %d\n",
>>>> +            firmwares[slice].txpru, txpru_fw_loadaddr, ret);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    dev_info(dev, "Loaded FW %s successfully at addr 0x%x Size = %d
>>>> Bytes\n",
>>>> +        firmwares[slice].txpru, txpru_fw_loadaddr, ret);
>>>> +    rproc_load(core_id + PRU_TYPE_TX_PRU, txpru_fw_loadaddr, ret);
>>>> +
>>>> +    ret = rproc_start(core_id + PRU_TYPE_PRU);
>>>> +    if (ret) {
>>>> +        dev_err(dev, "failed to start PRU%d: %d\n", slice, ret);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    ret = rproc_start(core_id + PRU_TYPE_RTU);
>>>> +    if (ret) {
>>>> +        dev_err(dev, "failed to start RTU%d: %d\n", slice, ret);
>>>> +        goto halt_pru;
>>>> +    }
>>>> +
>>>> +    ret = rproc_start(core_id + PRU_TYPE_TX_PRU);
>>>> +    if (ret) {
>>>> +        dev_err(dev, "failed to start TX_PRU%d: %d\n", slice, ret);
>>>> +        goto halt_rtu;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +
>>>> +halt_rtu:
>>>> +    rproc_stop(core_id + PRU_TYPE_RTU);
>>>> +
>>>> +halt_pru:
>>>> +    rproc_stop(PRU_TYPE_PRU);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +int icssg_stop_pru_cores(struct udevice *dev)
>>>> +{
>>>> +    struct prueth *prueth = dev_get_priv(dev);
>>>> +    int slice, core_id;
>>>> +
>>>> +    slice = prueth->slice;
>>>> +
>>>> +    core_id = icssg_get_pru_core_number(dev, slice);
>>>> +    if (core_id < 0)
>>>> +        return core_id;
>>>> +
>>>> +    rproc_stop(core_id + PRU_TYPE_PRU);
>>>> +    rproc_stop(core_id + PRU_TYPE_RTU);
>>>> +    rproc_stop(core_id + PRU_TYPE_TX_PRU);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   static int prueth_probe(struct udevice *dev)
>>>>   {
>>>>       ofnode eth_ports_node, eth0_node, eth1_node, eth_node;
>>>> diff --git a/include/linux/pruss_driver.h
>>>> b/include/linux/pruss_driver.h
>>>> index 25272e850e..f17fe8bf58 100644
>>>> --- a/include/linux/pruss_driver.h
>>>> +++ b/include/linux/pruss_driver.h
>>>> @@ -114,6 +114,21 @@ enum pru_ctable_idx {
>>>>       PRU_C31,
>>>>   };
>>>>
>>>> +/**
>>>> + * enum pru_type - PRU core type identifier
>>>> + *
>>>> + * @PRU_TYPE_PRU: Programmable Real-time Unit
>>>> + * @PRU_TYPE_RTU: Auxiliary Programmable Real-Time Unit
>>>> + * @PRU_TYPE_TX_PRU: Transmit Programmable Real-Time Unit
>>>> + * @PRU_TYPE_MAX: just keep this one at the end
>>>> + */
>>>> +enum pru_type {
>>>> +    PRU_TYPE_PRU,
>>>> +    PRU_TYPE_RTU,
>>>> +    PRU_TYPE_TX_PRU,
>>>> +    PRU_TYPE_MAX,
>>>> +};
>>>> +
>>>>   /**
>>>>    * enum pruss_mem - PRUSS memory range identifiers
>>>>    */
>>>>
>>>> with this diff, user don't need to run any extra commands at u-boot.
>>>> Once u-boot prompt is reached, just running ping / dhcp will suffice.
>>>>
>>> Great!
>>>
>>> <snip>
>>>
>>
>> I have addressed all the above comments and made the changes needed.
>> below is the diff. Please have a look at it and let me know if it
>> looks OK.
>>
>> diff --git a/arch/arm/dts/k3-am654-base-board-u-boot.dtsi
>> b/arch/arm/dts/k3-am654-base-board-u-boot.dtsi
>> index 11d83927ac..0b2b72b2c5 100644
>> --- a/arch/arm/dts/k3-am654-base-board-u-boot.dtsi
>> +++ b/arch/arm/dts/k3-am654-base-board-u-boot.dtsi
>> @@ -6,6 +6,18 @@
>>   #include "k3-am654-r5-base-board-u-boot.dtsi"
>>   #include "k3-am65x-binman.dtsi"
>>
>> +/ {
>> +    chosen {
>> +        firmware-loader = <&fs_loader0>;
>> +    };
>> +
>> +    fs_loader0: fs-loader {
>> +        bootph-all;
>> +        compatible = "u-boot,fs-loader";
>> +        phandlepart = <&sdhci1 2>;
>> +    };
>> +};
>> +
>>   &pru0_0 {
>>       remoteproc-name = "pru0_0";
>>   };
>> diff --git a/configs/am65x_evm_a53_defconfig
>> b/configs/am65x_evm_a53_defconfig
>> index 2755d7082f..c53e938abb 100644
>> --- a/configs/am65x_evm_a53_defconfig
>> +++ b/configs/am65x_evm_a53_defconfig
>> @@ -112,6 +112,7 @@ CONFIG_DM_I2C_GPIO=y
>>   CONFIG_SYS_I2C_OMAP24XX=y
>>   CONFIG_DM_MAILBOX=y
>>   CONFIG_K3_SEC_PROXY=y
>> +CONFIG_FS_LOADER=y
>>   CONFIG_SUPPORT_EMMC_BOOT=y
>>   CONFIG_MMC_IO_VOLTAGE=y
>>   CONFIG_MMC_UHS_SUPPORT=y
>> diff --git a/configs/am65x_evm_r5_defconfig
>> b/configs/am65x_evm_r5_defconfig
>> index b2f1e721b3..2d19935a41 100644
>> --- a/configs/am65x_evm_r5_defconfig
>> +++ b/configs/am65x_evm_r5_defconfig
>> @@ -98,6 +98,7 @@ CONFIG_I2C_SET_DEFAULT_BUS_NUM=y
>>   CONFIG_SYS_I2C_OMAP24XX=y
>>   CONFIG_DM_MAILBOX=y
>>   CONFIG_K3_SEC_PROXY=y
>> +CONFIG_FS_LOADER=y
>>   CONFIG_K3_AVS0=y
>>   CONFIG_SUPPORT_EMMC_BOOT=y
>>   CONFIG_MMC_HS200_SUPPORT=y
>> diff --git a/drivers/net/ti/icssg_prueth.c
>> b/drivers/net/ti/icssg_prueth.c
>> index 40ad827e49..a2e3595956 100644
>> --- a/drivers/net/ti/icssg_prueth.c
>> +++ b/drivers/net/ti/icssg_prueth.c
>> @@ -62,6 +62,9 @@
>>   /* Management packet type */
>>   #define PRUETH_PKT_TYPE_CMD        0x10
>>
>> +/* Number of PRU Cores per Slice */
>> +#define ICSSG_NUM_PRU_CORES        3
>> +
>>   static int icssg_phy_init(struct udevice *dev)
>>   {
>>       struct prueth *priv = dev_get_priv(dev);
>> @@ -218,6 +221,116 @@ static int icssg_update_link(struct prueth *priv)
>>       return phy->link;
>>   }
>>
>> +struct icssg_firmwares {
>> +    char *pru;
>> +    char *rtu;
>> +    char *txpru;
>> +};
>> +
>> +static struct icssg_firmwares icssg_emac_firmwares[] = {
>> +    {
>> +        .pru = "/lib/firmware/ti-pruss/am65x-sr2-pru0-prueth-fw.elf",
>> +        .rtu = "/lib/firmware/ti-pruss/am65x-sr2-rtu0-prueth-fw.elf",
>> +        .txpru =
>> "/lib/firmware/ti-pruss/am65x-sr2-txpru0-prueth-fw.elf",
>> +    },
>> +    {
>> +        .pru = "/lib/firmware/ti-pruss/am65x-sr2-pru1-prueth-fw.elf",
>> +        .rtu = "/lib/firmware/ti-pruss/am65x-sr2-rtu1-prueth-fw.elf",
>> +        .txpru =
>> "/lib/firmware/ti-pruss/am65x-sr2-txpru1-prueth-fw.elf",
>> +    }
>> +};
>> +
>> +static int icssg_start_pru_cores(struct udevice *dev)
>> +{
>> +    struct prueth *prueth = dev_get_priv(dev);
>> +    struct icssg_firmwares *firmwares;
>> +    struct udevice *rproc_dev = NULL;
>> +    int ret, slice;
>> +    u32 phandle;
>> +    u8 index;
>> +
>> +    slice = prueth->slice;
>> +    index = slice * ICSSG_NUM_PRU_CORES;
>> +    firmwares = icssg_emac_firmwares;
>> +
>> +    ofnode_read_u32_index(dev_ofnode(dev), "ti,prus", index, &phandle);
>> +    ret = uclass_get_device_by_phandle_id(UCLASS_REMOTEPROC, phandle,
>> &rproc_dev);
>> +    if (ret) {
>> +        dev_err(dev, "Unknown remote processor with phandle '0x%x'
>> requested(%d)\n",
>> +               phandle, ret);
>> +        return ret;
>> +    }
>> +
>> +    prueth->pru_core_id = dev_seq(rproc_dev);
>> +    ret = rproc_set_firmware(rproc_dev, firmwares[slice].pru);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = rproc_boot(rproc_dev, SZ_64K);
>> +    if (ret) {
>> +        dev_err(dev, "failed to boot PRU%d: %d\n", slice, ret);
>> +        return -EINVAL;
>> +    }
>> +
>> +    ofnode_read_u32_index(dev_ofnode(dev), "ti,prus", index + 1,
>> &phandle);
>> +    ret = uclass_get_device_by_phandle_id(UCLASS_REMOTEPROC, phandle,
>> &rproc_dev);
>> +    if (ret) {
>> +        dev_err(dev, "Unknown remote processor with phandle '0x%x'
>> requested(%d)\n",
>> +               phandle, ret);
>> +        goto halt_pru;
>> +    }
>> +
>> +    prueth->rtu_core_id = dev_seq(rproc_dev);
>> +    ret = rproc_set_firmware(rproc_dev, firmwares[slice].rtu);
>> +    if (ret)
>> +        goto halt_pru;
>> +
>> +    ret = rproc_boot(rproc_dev, SZ_64K);
>> +    if (ret) {
>> +        dev_err(dev, "failed to boot RTU%d: %d\n", slice, ret);
>> +        goto halt_pru;
>> +    }
>> +
>> +    ofnode_read_u32_index(dev_ofnode(dev), "ti,prus", index + 2,
>> &phandle);
>> +    ret = uclass_get_device_by_phandle_id(UCLASS_REMOTEPROC, phandle,
>> &rproc_dev);
>> +    if (ret) {
>> +        dev_err(dev, "Unknown remote processor with phandle '0x%x'
>> requested(%d)\n",
>> +               phandle, ret);
>> +        goto halt_rtu;
>> +    }
>> +
>> +    prueth->txpru_core_id = dev_seq(rproc_dev);
>> +    ret = rproc_set_firmware(rproc_dev, firmwares[slice].txpru);
>> +    if (ret)
>> +        goto halt_rtu;
>> +
>> +    ret = rproc_boot(rproc_dev, SZ_64K);
>> +    if (ret) {
>> +        dev_err(dev, "failed to boot TXPRU%d: %d\n", slice, ret);
>> +        goto halt_rtu;
>> +    }
>> +
>> +    return 0;
>> +
>> +halt_rtu:
>> +    rproc_stop(prueth->rtu_core_id);
>> +
>> +halt_pru:
>> +    rproc_stop(prueth->pru_core_id);
>> +    return ret;
>> +}
>> +
>> +static int icssg_stop_pru_cores(struct udevice *dev)
>> +{
>> +    struct prueth *prueth = dev_get_priv(dev);
>> +
>> +    rproc_stop(prueth->pru_core_id);
>> +    rproc_stop(prueth->rtu_core_id);
>> +    rproc_stop(prueth->txpru_core_id);
>> +
>> +    return 0;
>> +}
>> +
>>   static int prueth_start(struct udevice *dev)
>>   {
>>       struct ti_udma_drv_chan_cfg_data *dma_rx_cfg_data;
>> @@ -227,6 +340,10 @@ static int prueth_start(struct udevice *dev)
>>       void *config;
>>       int ret, i;
>>
>> +    ret = icssg_start_pru_cores(dev);
>> +    if (ret)
>> +        return ret;
>> +
>>       /* To differentiate channels for SLICE0 vs SLICE1 */
>>       snprintf(chn_name, sizeof(chn_name), "tx%d-0", priv->slice);
>>
>> @@ -355,9 +472,11 @@ static void prueth_stop(struct udevice *dev)
>>       phy_shutdown(priv->phydev);
>>
>>       dma_disable(&priv->dma_tx);
>> -    dma_free(&priv->dma_tx);
>> -
>>       dma_disable(&priv->dma_rx);
>> +
>> +    icssg_stop_pru_cores(dev);
>> +
>> +    dma_free(&priv->dma_tx);
>>       dma_free(&priv->dma_rx);
>>   }
>>
>> diff --git a/drivers/net/ti/icssg_prueth.h
>> b/drivers/net/ti/icssg_prueth.h
>> index e41ed16a05..c92660401b 100644
>> --- a/drivers/net/ti/icssg_prueth.h
>> +++ b/drivers/net/ti/icssg_prueth.h
>> @@ -69,6 +69,9 @@ struct prueth {
>>       int            speed;
>>       int            duplex;
>>       u8            icssg_hwcmdseq;
>> +    u8             pru_core_id;
>> +    u8             rtu_core_id;
>> +    u8             txpru_core_id;
>>   };
>>
>>   /* config helpers */
>> diff --git a/drivers/remoteproc/rproc-uclass.c
>> b/drivers/remoteproc/rproc-uclass.c
>> index 28b362c887..a085c44eaa 100644
>> --- a/drivers/remoteproc/rproc-uclass.c
>> +++ b/drivers/remoteproc/rproc-uclass.c
>> @@ -13,6 +13,7 @@
>>   #include <log.h>
>>   #include <malloc.h>
>>   #include <virtio_ring.h>
>> +#include <fs_loader.h>
>>   #include <remoteproc.h>
>>   #include <asm/io.h>
>>   #include <dm/device-internal.h>
>> @@ -961,3 +962,90 @@ unsigned long rproc_parse_resource_table(struct
>> udevice *dev, struct rproc *cfg)
>>
>>       return 1;
>>   }
>> +
>> +int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name)
>> +{
>> +    struct dm_rproc_uclass_pdata *uc_pdata;
>> +    int len;
>> +    char *p;
>> +
>> +    if (!rproc_dev || !fw_name)
>> +        return -EINVAL;
>> +
>> +    uc_pdata = dev_get_uclass_plat(rproc_dev);
>> +
>> +    len = strcspn(fw_name, "\n");
>> +    if (!len) {
>> +        debug("can't provide empty string for firmware name\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    p = strndup(fw_name, len);
>> +    if (!p)
>> +        return -ENOMEM;
>> +
>> +    uc_pdata->fw_name = p;
>> +
>> +    return 0;
>> +}
>> +
>> +int rproc_boot(struct udevice *rproc_dev, size_t fw_size)
>> +{
>> +    struct dm_rproc_uclass_pdata *uc_pdata;
>> +    struct udevice *fs_loader;
>> +    void *addr = malloc(fw_size);
>> +    int core_id, ret = 0;
>> +    char *firmware;
>> +    ulong rproc_addr;
>> +
>> +    if (!rproc_dev)
>> +        return -EINVAL;
>> +
>> +    if (!addr)
>> +        return -ENOMEM;
>> +
>> +    uc_pdata = dev_get_uclass_plat(rproc_dev);
>> +    core_id = dev_seq(rproc_dev);
>> +    firmware = uc_pdata->fw_name;
>> +
>> +    if (!firmware) {
>> +        debug("No firmware set for rproc core %d\n", core_id);
>> +        return -EINVAL;
>> +    }
>> +
>> +    /* Initialize all rproc cores */
>> +    rproc_init();
>> +
>> +    /* Loading firmware to a given address */
>> +    ret = get_fs_loader(&fs_loader);
>> +    if (ret) {
>> +        debug("could not get fs loader: %d\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    ret = request_firmware_into_buf(fs_loader, firmware, addr,
>> fw_size, 0);
>> +    if (ret < 0) {
>> +        debug("could not request %s: %d\n", firmware, ret);
>> +        return ret;
>> +    }
>> +
>> +    rproc_addr = (ulong)addr;
>> +
>> +    debug("Loaded %s to 0x%08lX size = %d Bytes\n",
>> +        uc_pdata->fw_name, rproc_addr, ret);
>> +
>> +    ret = rproc_load(core_id, rproc_addr, ret);
>> +    if (ret) {
>> +        debug("failed to load %s to rproc core %d from addr 0x%08lX
>> err %d\n",
>> +               uc_pdata->fw_name, core_id, rproc_addr, ret);
>> +        return ret;
>> +    }
>> +
>> +    ret = rproc_start(core_id);
>> +    if (ret) {
>> +        debug("failed to start rproc core %d\n", core_id);
>> +        return ret;
>> +    }
>> +
>> +    return ret;
>> +}
>> diff --git a/include/remoteproc.h b/include/remoteproc.h
>> index a11dc8a9b6..65b0ff7477 100644
>> --- a/include/remoteproc.h
>> +++ b/include/remoteproc.h
>> @@ -402,6 +402,7 @@ enum rproc_mem_type {
>>    * @name: Platform-specific way of naming the Remote proc
>>    * @mem_type: one of 'enum rproc_mem_type'
>>    * @driver_plat_data: driver specific platform data that may be needed.
>> + * @fw_name: firmware name
>>    *
>>    * This can be accessed with dev_get_uclass_plat() for any
>> UCLASS_REMOTEPROC
>>    * device.
>> @@ -411,6 +412,7 @@ struct dm_rproc_uclass_pdata {
>>       const char *name;
>>       enum rproc_mem_type mem_type;
>>       void *driver_plat_data;
>> +    char *fw_name;
>>   };
>>
>>   /**
>> @@ -704,6 +706,35 @@ unsigned long rproc_parse_resource_table(struct
>> udevice *dev,
>>   struct resource_table *rproc_find_resource_table(struct udevice *dev,
>>                            unsigned int addr,
>>                            int *tablesz);
>> +/**
>> + * rproc_set_firmware() - assign a new firmware
>> + * @rproc_dev: device for wich new firmware is being assigned
>> + * @fw_name: new firmware name to be assigned
>> + *
>> + * This function allows remoteproc drivers or clients to configure a
>> custom
>> + * firmware name. The function does not trigger a remote processor boot,
>> + * only sets the firmware name used for a subsequent boot.
>> + *
>> + * This function sets the fw_name field in uclass pdata of the Remote
>> proc
>> + *
>> + * Return: 0 on success or a negative value upon failure
>> + */
>> +int rproc_set_firmware(struct udevice *rproc_dev, const char *fw_name);
>> +
>> +/**
>> + * rproc_boot() - boot a remote processor
>> + * @rproc_dev: rproc device to boot
>> + * @fw_size: Size of the memory to allocate for firmeware
>> + *
>> + * Boot a remote processor (i.e. load its firmware, power it on, ...).
>> + *
>> + * This function first loads the firmware set in the uclass pdata of
>> Remote
>> + * processor to a buffer and then loads firmware to the remote processor
>> + * using rproc_load().
>> + *
>> + * Return: 0 on success, and an appropriate error value otherwise
>> + */
>> +int rproc_boot(struct udevice *rproc_dev, size_t fw_size);
>>   #else
>>   static inline int rproc_init(void) { return -ENOSYS; }
>>   static inline int rproc_dev_init(int id) { return -ENOSYS; }
>> @@ -743,6 +774,10 @@ static inline int rproc_elf_load_rsc_table(struct
>> udevice *dev, ulong fw_addr,
>>                          ulong fw_size, ulong *rsc_addr,
>>                          ulong *rsc_size)
>>   { return -ENOSYS; }
>> +static inline int rproc_set_firmware(struct udevice *rproc_dev, const
>> char *fw_name)
>> +{ return -ENOSYS; }
>> +static inline int rproc_boot(struct udevice *rproc_dev, size_t fw_size)
>> +{ return -ENOSYS; }
>>   #endif
>>
>>   #endif    /* _RPROC_H_ */
>>
>>
Roger Quadros Jan. 5, 2024, 8:19 a.m. UTC | #8
On 03/01/2024 12:27, MD Danish Anwar wrote:
> 
> 
> On 02/01/24 7:26 pm, Andrew Davis wrote:
>> On 12/27/23 12:56 AM, MD Danish Anwar wrote:
>>>
>>>
>>> On 22/12/23 6:13 pm, Roger Quadros wrote:
>>>>
>>>> On 22/12/2023 12:26, MD Danish Anwar wrote:
>>>>> Hi Roger,
>>>>>
>>>>> On 20/12/23 3:29 pm, Roger Quadros wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 19/12/2023 12:11, MD Danish Anwar wrote:
>>>>>>> Introduce ICSSG PRUETH support in uboot. The ICSSG driver is used
>>>>>>> in TI
>>>>>>> AM654 SR2.0.
>>>>>>>
>>>>>>> The ICSSG PRU Sub-system runs on EMAC firmware. This series
>>>>>>> Introduces
>>>>>>> support for ICSSG driver in uboot. This series also adds the driver's
>>>>>>> dependencies.
>>>>>>>
>>>>>>> The ICSSG2 node is added in device tree overlay so that it remains in
>>>>>>> sync with linux kernel.
>>>>>>>
>>>>>>> The series introduces device tree and config changes and AM65x
>>>>>>> to enable ICSSG driver. The series also enables
>>>>>>> SPL_LOAD_FIT_APPLY_OVERLAY
>>>>>>> for AM65x in order to load overlay over spl.
>>>>>>>
>>>>>>> This series has been tested on AM65x SR2.0, and the ICSSG
>>>>>>> interface is
>>>>>>> able to ping / dhcp and boot kernel using tftp in uboot.
>>>>>>>
>>>>>>> To use ICSSG2 ethernet, the ICSSG firmware needs to be loaded to
>>>>>>> PRU RPROC
>>>>>>> cores and RPROC cores need to be booted with the firmware. This
>>>>>>> step is
>>>>>>> done inside driver in kernel as kernel supports APIs like
>>>>>>> rproc_set_firmware() and rproc_fw_boot(). But as u-boot doesn't
>>>>>>> have these
>>>>>>> APIs, the same needs to be done via u-boot cmds.
>>>>>>>
>>>>>>> To make sure icssg-eth works we need to do below steps.
>>>>>>>
>>>>>>> 1. Initialize rproc cores i.e. rproc_init()
>>>>>>> 2. Load $firmware_file from partition '1:2' (root) on device (mmc
>>>>>>> in this
>>>>>>>     example)
>>>>>>> 3. Load the firmware file to rproc cores passing. i.e. rproc_load()
>>>>>>>     taking rproc_id, loadaddr and file size as arguments.
>>>>>>> 4. Start rproc cores. i.e. rproc_start() taking rproc_id as arguments
>>>>>>>
>>>>>>> The above steps are done by running the below commands at u-boot
>>>>>>> prompt.
>>>>>>>
>>>>>>> => setenv start_icssg2 'rproc start 14; rproc start 15; rproc
>>>>>>> start 16; rproc start 17; rproc start 18; rproc start 19'
>>>>>>> => setenv stop_icssg2 'rproc stop 14; rproc stop 15; rproc stop
>>>>>>> 16; rproc stop 17; rproc stop 18; rproc stop 19'
>>>>>>> => setenv firmware_dir '/lib/firmware/ti-pruss'
>>>>>>> => setenv get_firmware_mmc 'load mmc ${bootpart} ${loadaddr}
>>>>>>> ${firmware_dir}/${firmware_file}'
>>>>>>>
>>>>>>> => setenv init_icssg2 'setenv ethact icssg2-eth; setenv autoload
>>>>>>> no; rproc init; setenv loadaddr 0x80000000; \
>>>>>>>      setenv firmware_file am65x-sr2-pru0-prueth-fw.elf; run
>>>>>>> get_firmware_mmc;  rproc load 14 0x80000000 ${filesize}; \
>>>>>>>      setenv loadaddr 0x89000000; setenv firmware_file
>>>>>>> am65x-sr2-rtu0-prueth-fw.elf; run get_firmware_mmc; rproc load 15
>>>>>>> 0x89000000 ${filesize}; \
>>>>>>>      setenv loadaddr 0x90000000; setenv firmware_file
>>>>>>> am65x-sr2-txpru0-prueth-fw.elf; run get_firmware_mmc; rproc load
>>>>>>> 16 0x90000000 ${filesize}; \
>>>>>>>      setenv loadaddr 0x80000000; setenv firmware_file
>>>>>>> am65x-sr2-pru1-prueth-fw.elf; run get_firmware_mmc; rproc load 17
>>>>>>> 0x80000000 ${filesize}; \
>>>>>>>      setenv loadaddr 0x89000000; setenv firmware_file
>>>>>>> am65x-sr2-rtu1-prueth-fw.elf; run get_firmware_mmc; rproc load 18
>>>>>>> 0x89000000 ${filesize}; \
>>>>>>>      setenv loadaddr 0x90000000; setenv firmware_file
>>>>>>> am65x-sr2-txpru1-prueth-fw.elf; run get_firmware_mmc; rproc load
>>>>>>> 19 0x90000000 ${filesize}; \
>>>>>>>      run start_icssg2;'
>>>>>>
>>>>>> A whole bunch of commands are required to get ethernet functional.
>>>>>> This is not at all user friendly and will be a maintenance nightmare.
>>>>>> What worries me is tracking the 6 different rproc cores and the 6
>>>>>> different firmware files to start 1 ethernet device.
>>>>>> This will get even more interesting when we have to deal with
>>>>>> different ICSSG instances on different boards.
>>>>>>
>>>>>> What is preventing the driver from starting the rproc cores it
>>>>>> needs so user doesn't need to care about it?
>>>>>> All the necessary information is in the Device tree. At least this
>>>>>> is how it is done on Linux.
>>>>>>
>>>>>
>>>>> I tried removing the need for these commands and implementing them
>>>>> inside the driver only. I am able to load the firmware from driver
>>>>> using
>>>>> the fs_loader API request_firmware_into_buf(). It requires changes to
>>>>> dt. A DT node called fs-loader needs to be added also CONFIG_FS_LOADER
>>>>> needs to enabled. In the DT node we need to specify the storage media
>>>>> that we are using i.e. mmc, ospi, usb. It's upto user to modify the
>>>>> storage media, the driver will take the media from DT and try to laod
>>>>> firmware from their.
>>>>>
>>>>> For loading firmwares to rproc cores, rproc_load() API is needed. Now
>>>>> this API takes rproc_id, loadaddr and firmware_size as arguments.
>>>>> loadaddr is fixed for all three pru cores. firmware_size is obtained
>>>>> from request_firmware_into_buf() but I couldn't find a way to get the
>>>>> rproc_id. For now based on the ICSSG instance and slice number I am
>>>>> figuring out the rproc_id and passing it to rproc_load() and
>>>>> rproc_start() APIs. Please let me know if you have any other / better
>>>>> way of finding rproc_id.
>>>>>
>>>>> Below is the entire diff to remove these commands and move their
>>>>> functionality to driver. Please have a look and let me know if this
>>>>> is ok.
>>>>>
>>>>
>>>> Good to see you got something working so quickly.
>>>> It has some rough edges but nothing that is blocking.
>>>>
>>>>>
>>>>> Save New Duplicate & Edit Just Text Twitter
>>>>> diff --git a/arch/arm/dts/k3-am654-base-board.dts
>>>>> b/arch/arm/dts/k3-am654-base-board.dts
>>>>> index cfbcebfa37..c8da72e49c 100644
>>>>> --- a/arch/arm/dts/k3-am654-base-board.dts
>>>>> +++ b/arch/arm/dts/k3-am654-base-board.dts
>>>>> @@ -16,6 +16,13 @@
>>>>>       chosen {
>>>>>           stdout-path = "serial2:115200n8";
>>>>>           bootargs = "earlycon=ns16550a,mmio32,0x02800000";
>>>>> +        firmware-loader = <&fs_loader0>;
>>>>> +    };
>>>>> +
>>>>> +    fs_loader0: fs-loader {
>>>>> +        bootph-all;
>>>>> +        compatible = "u-boot,fs-loader";
>>>>> +        phandlepart = <&sdhci1 2>;
>>>>>       };
>>>>
>>>> This is has 2 issues
>>>> 1) It will not be accepted in Kernel DT. Maybe it could be done in
>>>> -u-boot.dtsi file.
>>>
>>> Sure. I'll move this to k3-am654-base-board-u-boot.dtsi
>>>
>>>> 2) You cannot assume boot medium is always sdhci1 2
>>>>
>>>
>>> Yes. It's upto user to provide the boot medium and boot partition for
>>> loading firmware. By default the firmware is loacated in root partion of
>>> shdci1 on am65x so I am adding this as default. But user can easily
>>> modify this to any other medium / partition needed.
>>>
>>
>> Users should not have to modify DT to pick a boot medium/partition.
>> What would you do for complex cases or non block devices like eth/uart
> 
I agree with Andrew here.

> In order to load firmware files from driver, we need to add the node for
> fs-loader. The fs-loader has a phandlepart property which takes the boot
> medium and the partition as input. Based on the medium and partition it
> looks for the requested file and loads it to a given address. I am not
> sure if there is any other way to load firmware from driver without
> using the fs-loader.
> 
> 
>> booting? This is one reason kernel moved firmware loading to
>> userspace. The equivalant in U-Boot is the shell and scripts. Your
>> original command based loading was the correct solution IMHO.
> 
> I intended to go ahead with command base approach only but as Roger
> pointed out the command based approach is not very user friendly.
> 
> We need to align on what should be the correct approach for loading
> firmware.
> 
> Roger, can you please chime in here.

My intention was to make it user friendly so he does not have to
deal with 6 different Rproc IDs that can change between
platforms. The solution also has to be seamless between different
boot mediums. I think we can assume that the firmware will come from the
same medium that the platform was booted.

> 
>>
>> If we want to try to hide some of that then we need a way to
>> run a user provided script from the environement to handle
>> the general cases for firmware loading.

Assuming we go with the "script provided in environment: route,
how do we deal with the Rproc IDs? I'm not sure if they are constant
and can probably change based on which Rproc is registered first.
So there needs to be some way to make sure user can reference
the correct Rproc.

>>
>> Andrew

<snip>
Anwar, Md Danish Jan. 5, 2024, 10:15 a.m. UTC | #9
On 1/5/2024 1:49 PM, Roger Quadros wrote:
> 
> 
> On 03/01/2024 12:27, MD Danish Anwar wrote:
>>
>>
>> On 02/01/24 7:26 pm, Andrew Davis wrote:
>>> On 12/27/23 12:56 AM, MD Danish Anwar wrote:
>>>>
>>>>
>>>> On 22/12/23 6:13 pm, Roger Quadros wrote:
>>>>>
>>>>> On 22/12/2023 12:26, MD Danish Anwar wrote:
>>>>>> Hi Roger,
>>>>>>
>>>>>> On 20/12/23 3:29 pm, Roger Quadros wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 19/12/2023 12:11, MD Danish Anwar wrote:
>>>>>>>> Introduce ICSSG PRUETH support in uboot. The ICSSG driver is used
>>>>>>>> in TI
>>>>>>>> AM654 SR2.0.
>>>>>>>>
>>>>>>>> The ICSSG PRU Sub-system runs on EMAC firmware. This series
>>>>>>>> Introduces
>>>>>>>> support for ICSSG driver in uboot. This series also adds the driver's
>>>>>>>> dependencies.
>>>>>>>>
>>>>>>>> The ICSSG2 node is added in device tree overlay so that it remains in
>>>>>>>> sync with linux kernel.
>>>>>>>>
>>>>>>>> The series introduces device tree and config changes and AM65x
>>>>>>>> to enable ICSSG driver. The series also enables
>>>>>>>> SPL_LOAD_FIT_APPLY_OVERLAY
>>>>>>>> for AM65x in order to load overlay over spl.
>>>>>>>>
>>>>>>>> This series has been tested on AM65x SR2.0, and the ICSSG
>>>>>>>> interface is
>>>>>>>> able to ping / dhcp and boot kernel using tftp in uboot.
>>>>>>>>
>>>>>>>> To use ICSSG2 ethernet, the ICSSG firmware needs to be loaded to
>>>>>>>> PRU RPROC
>>>>>>>> cores and RPROC cores need to be booted with the firmware. This
>>>>>>>> step is
>>>>>>>> done inside driver in kernel as kernel supports APIs like
>>>>>>>> rproc_set_firmware() and rproc_fw_boot(). But as u-boot doesn't
>>>>>>>> have these
>>>>>>>> APIs, the same needs to be done via u-boot cmds.
>>>>>>>>
>>>>>>>> To make sure icssg-eth works we need to do below steps.
>>>>>>>>
>>>>>>>> 1. Initialize rproc cores i.e. rproc_init()
>>>>>>>> 2. Load $firmware_file from partition '1:2' (root) on device (mmc
>>>>>>>> in this
>>>>>>>>     example)
>>>>>>>> 3. Load the firmware file to rproc cores passing. i.e. rproc_load()
>>>>>>>>     taking rproc_id, loadaddr and file size as arguments.
>>>>>>>> 4. Start rproc cores. i.e. rproc_start() taking rproc_id as arguments
>>>>>>>>
>>>>>>>> The above steps are done by running the below commands at u-boot
>>>>>>>> prompt.
>>>>>>>>
>>>>>>>> => setenv start_icssg2 'rproc start 14; rproc start 15; rproc
>>>>>>>> start 16; rproc start 17; rproc start 18; rproc start 19'
>>>>>>>> => setenv stop_icssg2 'rproc stop 14; rproc stop 15; rproc stop
>>>>>>>> 16; rproc stop 17; rproc stop 18; rproc stop 19'
>>>>>>>> => setenv firmware_dir '/lib/firmware/ti-pruss'
>>>>>>>> => setenv get_firmware_mmc 'load mmc ${bootpart} ${loadaddr}
>>>>>>>> ${firmware_dir}/${firmware_file}'
>>>>>>>>
>>>>>>>> => setenv init_icssg2 'setenv ethact icssg2-eth; setenv autoload
>>>>>>>> no; rproc init; setenv loadaddr 0x80000000; \
>>>>>>>>      setenv firmware_file am65x-sr2-pru0-prueth-fw.elf; run
>>>>>>>> get_firmware_mmc;  rproc load 14 0x80000000 ${filesize}; \
>>>>>>>>      setenv loadaddr 0x89000000; setenv firmware_file
>>>>>>>> am65x-sr2-rtu0-prueth-fw.elf; run get_firmware_mmc; rproc load 15
>>>>>>>> 0x89000000 ${filesize}; \
>>>>>>>>      setenv loadaddr 0x90000000; setenv firmware_file
>>>>>>>> am65x-sr2-txpru0-prueth-fw.elf; run get_firmware_mmc; rproc load
>>>>>>>> 16 0x90000000 ${filesize}; \
>>>>>>>>      setenv loadaddr 0x80000000; setenv firmware_file
>>>>>>>> am65x-sr2-pru1-prueth-fw.elf; run get_firmware_mmc; rproc load 17
>>>>>>>> 0x80000000 ${filesize}; \
>>>>>>>>      setenv loadaddr 0x89000000; setenv firmware_file
>>>>>>>> am65x-sr2-rtu1-prueth-fw.elf; run get_firmware_mmc; rproc load 18
>>>>>>>> 0x89000000 ${filesize}; \
>>>>>>>>      setenv loadaddr 0x90000000; setenv firmware_file
>>>>>>>> am65x-sr2-txpru1-prueth-fw.elf; run get_firmware_mmc; rproc load
>>>>>>>> 19 0x90000000 ${filesize}; \
>>>>>>>>      run start_icssg2;'
>>>>>>>
>>>>>>> A whole bunch of commands are required to get ethernet functional.
>>>>>>> This is not at all user friendly and will be a maintenance nightmare.
>>>>>>> What worries me is tracking the 6 different rproc cores and the 6
>>>>>>> different firmware files to start 1 ethernet device.
>>>>>>> This will get even more interesting when we have to deal with
>>>>>>> different ICSSG instances on different boards.
>>>>>>>
>>>>>>> What is preventing the driver from starting the rproc cores it
>>>>>>> needs so user doesn't need to care about it?
>>>>>>> All the necessary information is in the Device tree. At least this
>>>>>>> is how it is done on Linux.
>>>>>>>
>>>>>>
>>>>>> I tried removing the need for these commands and implementing them
>>>>>> inside the driver only. I am able to load the firmware from driver
>>>>>> using
>>>>>> the fs_loader API request_firmware_into_buf(). It requires changes to
>>>>>> dt. A DT node called fs-loader needs to be added also CONFIG_FS_LOADER
>>>>>> needs to enabled. In the DT node we need to specify the storage media
>>>>>> that we are using i.e. mmc, ospi, usb. It's upto user to modify the
>>>>>> storage media, the driver will take the media from DT and try to laod
>>>>>> firmware from their.
>>>>>>
>>>>>> For loading firmwares to rproc cores, rproc_load() API is needed. Now
>>>>>> this API takes rproc_id, loadaddr and firmware_size as arguments.
>>>>>> loadaddr is fixed for all three pru cores. firmware_size is obtained
>>>>>> from request_firmware_into_buf() but I couldn't find a way to get the
>>>>>> rproc_id. For now based on the ICSSG instance and slice number I am
>>>>>> figuring out the rproc_id and passing it to rproc_load() and
>>>>>> rproc_start() APIs. Please let me know if you have any other / better
>>>>>> way of finding rproc_id.
>>>>>>
>>>>>> Below is the entire diff to remove these commands and move their
>>>>>> functionality to driver. Please have a look and let me know if this
>>>>>> is ok.
>>>>>>
>>>>>
>>>>> Good to see you got something working so quickly.
>>>>> It has some rough edges but nothing that is blocking.
>>>>>
>>>>>>
>>>>>> Save New Duplicate & Edit Just Text Twitter
>>>>>> diff --git a/arch/arm/dts/k3-am654-base-board.dts
>>>>>> b/arch/arm/dts/k3-am654-base-board.dts
>>>>>> index cfbcebfa37..c8da72e49c 100644
>>>>>> --- a/arch/arm/dts/k3-am654-base-board.dts
>>>>>> +++ b/arch/arm/dts/k3-am654-base-board.dts
>>>>>> @@ -16,6 +16,13 @@
>>>>>>       chosen {
>>>>>>           stdout-path = "serial2:115200n8";
>>>>>>           bootargs = "earlycon=ns16550a,mmio32,0x02800000";
>>>>>> +        firmware-loader = <&fs_loader0>;
>>>>>> +    };
>>>>>> +
>>>>>> +    fs_loader0: fs-loader {
>>>>>> +        bootph-all;
>>>>>> +        compatible = "u-boot,fs-loader";
>>>>>> +        phandlepart = <&sdhci1 2>;
>>>>>>       };
>>>>>
>>>>> This is has 2 issues
>>>>> 1) It will not be accepted in Kernel DT. Maybe it could be done in
>>>>> -u-boot.dtsi file.
>>>>
>>>> Sure. I'll move this to k3-am654-base-board-u-boot.dtsi
>>>>
>>>>> 2) You cannot assume boot medium is always sdhci1 2
>>>>>
>>>>
>>>> Yes. It's upto user to provide the boot medium and boot partition for
>>>> loading firmware. By default the firmware is loacated in root partion of
>>>> shdci1 on am65x so I am adding this as default. But user can easily
>>>> modify this to any other medium / partition needed.
>>>>
>>>
>>> Users should not have to modify DT to pick a boot medium/partition.
>>> What would you do for complex cases or non block devices like eth/uart
>>
> I agree with Andrew here.
> 
>> In order to load firmware files from driver, we need to add the node for
>> fs-loader. The fs-loader has a phandlepart property which takes the boot
>> medium and the partition as input. Based on the medium and partition it
>> looks for the requested file and loads it to a given address. I am not
>> sure if there is any other way to load firmware from driver without
>> using the fs-loader.
>>
>>
>>> booting? This is one reason kernel moved firmware loading to
>>> userspace. The equivalant in U-Boot is the shell and scripts. Your
>>> original command based loading was the correct solution IMHO.
>>
>> I intended to go ahead with command base approach only but as Roger
>> pointed out the command based approach is not very user friendly.
>>
>> We need to align on what should be the correct approach for loading
>> firmware.
>>
>> Roger, can you please chime in here.
> 
> My intention was to make it user friendly so he does not have to
> deal with 6 different Rproc IDs that can change between
> platforms. The solution also has to be seamless between different
> boot mediums. I think we can assume that the firmware will come from the
> same medium that the platform was booted.
> 

Yes and the with changes done by me using ICSSG port seems much easier
and user friendly. Just doing `dhcp` at u-boot prompt is enough to use
ICSSG port so the driver changes effectively makes the usage for user
friendly. But only way I have found to load the firmware files is to use
fs-loader that requires the boot medium and boot partition. I am not
sure how can I re-use the the boot medium used for booting u-boot images.

Also in SD card, u-boot images are located in /boot partition where as
firmware is located inside /root partition. So we'll need to specify the
partition.

Currently I don't have any way to load the firmware files from driver
without using below DT change

fs_loader0: fs-loader {
        bootph-all;
        compatible = "u-boot,fs-loader";
        phandlepart = <&sdhci1 2>;
};

>>
>>>
>>> If we want to try to hide some of that then we need a way to
>>> run a user provided script from the environement to handle
>>> the general cases for firmware loading.
> 
> Assuming we go with the "script provided in environment: route,
> how do we deal with the Rproc IDs? I'm not sure if they are constant
> and can probably change based on which Rproc is registered first.
> So there needs to be some way to make sure user can reference
> the correct Rproc.
> 

The Rproc IDs aren't constant. I think the IDs depend on the sequence of
init. The only way to know the IDs of different Rprocs is to run `rproc
init` and then `rproc list`. Mostly on AM65x I have seen Rporc ID 0 and
1 to be of r5 rprocs and from 2 to 19 of differnet PRUs (Total 18, 6 per
ICSSG instance). If r5 is not enabled, the sequence changes. Similarly
if some other rproc is enabled sequence will change.

The script should be able to read the list and determine which rproc
needs to be loaded based on the port we want to use. I am not sure how
to do this during u-boot.

>>>
>>> Andrew
> 
> <snip>
>
Roger Quadros Jan. 8, 2024, 9:30 a.m. UTC | #10
On 05/01/2024 12:15, Anwar, Md Danish wrote:
> 
> 
> On 1/5/2024 1:49 PM, Roger Quadros wrote:
>>
>>
>> On 03/01/2024 12:27, MD Danish Anwar wrote:
>>>
>>>
>>> On 02/01/24 7:26 pm, Andrew Davis wrote:
>>>> On 12/27/23 12:56 AM, MD Danish Anwar wrote:
>>>>>
>>>>>
>>>>> On 22/12/23 6:13 pm, Roger Quadros wrote:
>>>>>>
>>>>>> On 22/12/2023 12:26, MD Danish Anwar wrote:
>>>>>>> Hi Roger,
>>>>>>>
>>>>>>> On 20/12/23 3:29 pm, Roger Quadros wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 19/12/2023 12:11, MD Danish Anwar wrote:
>>>>>>>>> Introduce ICSSG PRUETH support in uboot. The ICSSG driver is used
>>>>>>>>> in TI
>>>>>>>>> AM654 SR2.0.
>>>>>>>>>
>>>>>>>>> The ICSSG PRU Sub-system runs on EMAC firmware. This series
>>>>>>>>> Introduces
>>>>>>>>> support for ICSSG driver in uboot. This series also adds the driver's
>>>>>>>>> dependencies.
>>>>>>>>>
>>>>>>>>> The ICSSG2 node is added in device tree overlay so that it remains in
>>>>>>>>> sync with linux kernel.
>>>>>>>>>
>>>>>>>>> The series introduces device tree and config changes and AM65x
>>>>>>>>> to enable ICSSG driver. The series also enables
>>>>>>>>> SPL_LOAD_FIT_APPLY_OVERLAY
>>>>>>>>> for AM65x in order to load overlay over spl.
>>>>>>>>>
>>>>>>>>> This series has been tested on AM65x SR2.0, and the ICSSG
>>>>>>>>> interface is
>>>>>>>>> able to ping / dhcp and boot kernel using tftp in uboot.
>>>>>>>>>
>>>>>>>>> To use ICSSG2 ethernet, the ICSSG firmware needs to be loaded to
>>>>>>>>> PRU RPROC
>>>>>>>>> cores and RPROC cores need to be booted with the firmware. This
>>>>>>>>> step is
>>>>>>>>> done inside driver in kernel as kernel supports APIs like
>>>>>>>>> rproc_set_firmware() and rproc_fw_boot(). But as u-boot doesn't
>>>>>>>>> have these
>>>>>>>>> APIs, the same needs to be done via u-boot cmds.
>>>>>>>>>
>>>>>>>>> To make sure icssg-eth works we need to do below steps.
>>>>>>>>>
>>>>>>>>> 1. Initialize rproc cores i.e. rproc_init()
>>>>>>>>> 2. Load $firmware_file from partition '1:2' (root) on device (mmc
>>>>>>>>> in this
>>>>>>>>>     example)
>>>>>>>>> 3. Load the firmware file to rproc cores passing. i.e. rproc_load()
>>>>>>>>>     taking rproc_id, loadaddr and file size as arguments.
>>>>>>>>> 4. Start rproc cores. i.e. rproc_start() taking rproc_id as arguments
>>>>>>>>>
>>>>>>>>> The above steps are done by running the below commands at u-boot
>>>>>>>>> prompt.
>>>>>>>>>
>>>>>>>>> => setenv start_icssg2 'rproc start 14; rproc start 15; rproc
>>>>>>>>> start 16; rproc start 17; rproc start 18; rproc start 19'
>>>>>>>>> => setenv stop_icssg2 'rproc stop 14; rproc stop 15; rproc stop
>>>>>>>>> 16; rproc stop 17; rproc stop 18; rproc stop 19'
>>>>>>>>> => setenv firmware_dir '/lib/firmware/ti-pruss'
>>>>>>>>> => setenv get_firmware_mmc 'load mmc ${bootpart} ${loadaddr}
>>>>>>>>> ${firmware_dir}/${firmware_file}'
>>>>>>>>>
>>>>>>>>> => setenv init_icssg2 'setenv ethact icssg2-eth; setenv autoload
>>>>>>>>> no; rproc init; setenv loadaddr 0x80000000; \
>>>>>>>>>      setenv firmware_file am65x-sr2-pru0-prueth-fw.elf; run
>>>>>>>>> get_firmware_mmc;  rproc load 14 0x80000000 ${filesize}; \
>>>>>>>>>      setenv loadaddr 0x89000000; setenv firmware_file
>>>>>>>>> am65x-sr2-rtu0-prueth-fw.elf; run get_firmware_mmc; rproc load 15
>>>>>>>>> 0x89000000 ${filesize}; \
>>>>>>>>>      setenv loadaddr 0x90000000; setenv firmware_file
>>>>>>>>> am65x-sr2-txpru0-prueth-fw.elf; run get_firmware_mmc; rproc load
>>>>>>>>> 16 0x90000000 ${filesize}; \
>>>>>>>>>      setenv loadaddr 0x80000000; setenv firmware_file
>>>>>>>>> am65x-sr2-pru1-prueth-fw.elf; run get_firmware_mmc; rproc load 17
>>>>>>>>> 0x80000000 ${filesize}; \
>>>>>>>>>      setenv loadaddr 0x89000000; setenv firmware_file
>>>>>>>>> am65x-sr2-rtu1-prueth-fw.elf; run get_firmware_mmc; rproc load 18
>>>>>>>>> 0x89000000 ${filesize}; \
>>>>>>>>>      setenv loadaddr 0x90000000; setenv firmware_file
>>>>>>>>> am65x-sr2-txpru1-prueth-fw.elf; run get_firmware_mmc; rproc load
>>>>>>>>> 19 0x90000000 ${filesize}; \
>>>>>>>>>      run start_icssg2;'
>>>>>>>>
>>>>>>>> A whole bunch of commands are required to get ethernet functional.
>>>>>>>> This is not at all user friendly and will be a maintenance nightmare.
>>>>>>>> What worries me is tracking the 6 different rproc cores and the 6
>>>>>>>> different firmware files to start 1 ethernet device.
>>>>>>>> This will get even more interesting when we have to deal with
>>>>>>>> different ICSSG instances on different boards.
>>>>>>>>
>>>>>>>> What is preventing the driver from starting the rproc cores it
>>>>>>>> needs so user doesn't need to care about it?
>>>>>>>> All the necessary information is in the Device tree. At least this
>>>>>>>> is how it is done on Linux.
>>>>>>>>
>>>>>>>
>>>>>>> I tried removing the need for these commands and implementing them
>>>>>>> inside the driver only. I am able to load the firmware from driver
>>>>>>> using
>>>>>>> the fs_loader API request_firmware_into_buf(). It requires changes to
>>>>>>> dt. A DT node called fs-loader needs to be added also CONFIG_FS_LOADER
>>>>>>> needs to enabled. In the DT node we need to specify the storage media
>>>>>>> that we are using i.e. mmc, ospi, usb. It's upto user to modify the
>>>>>>> storage media, the driver will take the media from DT and try to laod
>>>>>>> firmware from their.
>>>>>>>
>>>>>>> For loading firmwares to rproc cores, rproc_load() API is needed. Now
>>>>>>> this API takes rproc_id, loadaddr and firmware_size as arguments.
>>>>>>> loadaddr is fixed for all three pru cores. firmware_size is obtained
>>>>>>> from request_firmware_into_buf() but I couldn't find a way to get the
>>>>>>> rproc_id. For now based on the ICSSG instance and slice number I am
>>>>>>> figuring out the rproc_id and passing it to rproc_load() and
>>>>>>> rproc_start() APIs. Please let me know if you have any other / better
>>>>>>> way of finding rproc_id.
>>>>>>>
>>>>>>> Below is the entire diff to remove these commands and move their
>>>>>>> functionality to driver. Please have a look and let me know if this
>>>>>>> is ok.
>>>>>>>
>>>>>>
>>>>>> Good to see you got something working so quickly.
>>>>>> It has some rough edges but nothing that is blocking.
>>>>>>
>>>>>>>
>>>>>>> Save New Duplicate & Edit Just Text Twitter
>>>>>>> diff --git a/arch/arm/dts/k3-am654-base-board.dts
>>>>>>> b/arch/arm/dts/k3-am654-base-board.dts
>>>>>>> index cfbcebfa37..c8da72e49c 100644
>>>>>>> --- a/arch/arm/dts/k3-am654-base-board.dts
>>>>>>> +++ b/arch/arm/dts/k3-am654-base-board.dts
>>>>>>> @@ -16,6 +16,13 @@
>>>>>>>       chosen {
>>>>>>>           stdout-path = "serial2:115200n8";
>>>>>>>           bootargs = "earlycon=ns16550a,mmio32,0x02800000";
>>>>>>> +        firmware-loader = <&fs_loader0>;
>>>>>>> +    };
>>>>>>> +
>>>>>>> +    fs_loader0: fs-loader {
>>>>>>> +        bootph-all;
>>>>>>> +        compatible = "u-boot,fs-loader";
>>>>>>> +        phandlepart = <&sdhci1 2>;
>>>>>>>       };
>>>>>>
>>>>>> This is has 2 issues
>>>>>> 1) It will not be accepted in Kernel DT. Maybe it could be done in
>>>>>> -u-boot.dtsi file.
>>>>>
>>>>> Sure. I'll move this to k3-am654-base-board-u-boot.dtsi
>>>>>
>>>>>> 2) You cannot assume boot medium is always sdhci1 2
>>>>>>
>>>>>
>>>>> Yes. It's upto user to provide the boot medium and boot partition for
>>>>> loading firmware. By default the firmware is loacated in root partion of
>>>>> shdci1 on am65x so I am adding this as default. But user can easily
>>>>> modify this to any other medium / partition needed.
>>>>>
>>>>
>>>> Users should not have to modify DT to pick a boot medium/partition.
>>>> What would you do for complex cases or non block devices like eth/uart
>>>
>> I agree with Andrew here.
>>
>>> In order to load firmware files from driver, we need to add the node for
>>> fs-loader. The fs-loader has a phandlepart property which takes the boot
>>> medium and the partition as input. Based on the medium and partition it
>>> looks for the requested file and loads it to a given address. I am not
>>> sure if there is any other way to load firmware from driver without
>>> using the fs-loader.
>>>
>>>
>>>> booting? This is one reason kernel moved firmware loading to
>>>> userspace. The equivalant in U-Boot is the shell and scripts. Your
>>>> original command based loading was the correct solution IMHO.
>>>
>>> I intended to go ahead with command base approach only but as Roger
>>> pointed out the command based approach is not very user friendly.
>>>
>>> We need to align on what should be the correct approach for loading
>>> firmware.
>>>
>>> Roger, can you please chime in here.
>>
>> My intention was to make it user friendly so he does not have to
>> deal with 6 different Rproc IDs that can change between
>> platforms. The solution also has to be seamless between different
>> boot mediums. I think we can assume that the firmware will come from the
>> same medium that the platform was booted.
>>
> 
> Yes and the with changes done by me using ICSSG port seems much easier
> and user friendly. Just doing `dhcp` at u-boot prompt is enough to use
> ICSSG port so the driver changes effectively makes the usage for user
> friendly. But only way I have found to load the firmware files is to use
> fs-loader that requires the boot medium and boot partition. I am not
> sure how can I re-use the the boot medium used for booting u-boot images.
> 
> Also in SD card, u-boot images are located in /boot partition where as
> firmware is located inside /root partition. So we'll need to specify the
> partition.
> 
> Currently I don't have any way to load the firmware files from driver
> without using below DT change
> 
> fs_loader0: fs-loader {
>         bootph-all;
>         compatible = "u-boot,fs-loader";
>         phandlepart = <&sdhci1 2>;
> };
> 

Fromdoc/develop/driver-model/fs_firmware_loader.rst

"Firmware loader driver is also designed to support U-Boot environment
variables, so all these data from FDT can be overwritten
through the U-Boot environment variable during run time.

For examples:

storage_interface:
  Storage interface, it can be "mmc", "usb", "sata" or "ubi".
fw_dev_part:
  Block device number and its partition, it can be "0:1".
fw_ubi_mtdpart:
  UBI device mtd partition, it can be "UBI".
fw_ubi_volume:
  UBI volume, it can be "ubi0".

When above environment variables are set, environment values would be
used instead of data from FDT.
The benefit of this design allows user to change storage attribute data
at run time through U-Boot console and saving the setting as default
environment values in the storage for the next power cycle, so no
compilation is required for both driver and FDT."

So looks like we should provide this in environment variables instead of DT?

>>>
>>>>
>>>> If we want to try to hide some of that then we need a way to
>>>> run a user provided script from the environement to handle
>>>> the general cases for firmware loading.
>>
>> Assuming we go with the "script provided in environment: route,
>> how do we deal with the Rproc IDs? I'm not sure if they are constant
>> and can probably change based on which Rproc is registered first.
>> So there needs to be some way to make sure user can reference
>> the correct Rproc.
>>
> 
> The Rproc IDs aren't constant. I think the IDs depend on the sequence of
> init. The only way to know the IDs of different Rprocs is to run `rproc
> init` and then `rproc list`. Mostly on AM65x I have seen Rporc ID 0 and
> 1 to be of r5 rprocs and from 2 to 19 of differnet PRUs (Total 18, 6 per
> ICSSG instance). If r5 is not enabled, the sequence changes. Similarly
> if some other rproc is enabled sequence will change.
> 
> The script should be able to read the list and determine which rproc
> needs to be loaded based on the port we want to use. I am not sure how
> to do this during u-boot.
> 

rproc list gives an output in the following format.

=> rproc list
0 - Name:'r5f@41000000' type:'internal memory mapped' supports: load start stop reset
1 - Name:'r5f@41400000' type:'internal memory mapped' supports: load start stop reset
2 - Name:'r5f@5c00000' type:'internal memory mapped' supports: load start stop reset
3 - Name:'r5f@5d00000' type:'internal memory mapped' supports: load start stop reset
4 - Name:'r5f@5e00000' type:'internal memory mapped' supports: load start stop reset
5 - Name:'r5f@5f00000' type:'internal memory mapped' supports: load start stop reset
6 - Name:'dsp@4d80800000' type:'internal memory mapped' supports: load start stop reset
7 - Name:'dsp@4d81800000' type:'internal memory mapped' supports: load start stop reset
8 - Name:'dsp@64800000' type:'internal memory mapped' supports: load start stop reset

How can the script know which rproc to start for a specific Ethernet interface?
MD Danish Anwar Jan. 8, 2024, 10:25 a.m. UTC | #11
On 08/01/24 3:00 pm, Roger Quadros wrote:
> 
> 
> On 05/01/2024 12:15, Anwar, Md Danish wrote:
>>
>>
>> On 1/5/2024 1:49 PM, Roger Quadros wrote:
>>>
>>>
>>> On 03/01/2024 12:27, MD Danish Anwar wrote:
>>>>
>>>>
>>>> On 02/01/24 7:26 pm, Andrew Davis wrote:
>>>>> On 12/27/23 12:56 AM, MD Danish Anwar wrote:
>>>>>>
>>>>>>
>>>>>> On 22/12/23 6:13 pm, Roger Quadros wrote:
>>>>>>>
>>>>>>> On 22/12/2023 12:26, MD Danish Anwar wrote:
>>>>>>>> Hi Roger,
>>>>>>>>
>>>>>>>> On 20/12/23 3:29 pm, Roger Quadros wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 19/12/2023 12:11, MD Danish Anwar wrote:
>>>>>>>>>> Introduce ICSSG PRUETH support in uboot. The ICSSG driver is used
>>>>>>>>>> in TI
>>>>>>>>>> AM654 SR2.0.
>>>>>>>>>>
>>>>>>>>>> The ICSSG PRU Sub-system runs on EMAC firmware. This series
>>>>>>>>>> Introduces
>>>>>>>>>> support for ICSSG driver in uboot. This series also adds the driver's
>>>>>>>>>> dependencies.
>>>>>>>>>>
>>>>>>>>>> The ICSSG2 node is added in device tree overlay so that it remains in
>>>>>>>>>> sync with linux kernel.
>>>>>>>>>>
>>>>>>>>>> The series introduces device tree and config changes and AM65x
>>>>>>>>>> to enable ICSSG driver. The series also enables
>>>>>>>>>> SPL_LOAD_FIT_APPLY_OVERLAY
>>>>>>>>>> for AM65x in order to load overlay over spl.
>>>>>>>>>>
>>>>>>>>>> This series has been tested on AM65x SR2.0, and the ICSSG
>>>>>>>>>> interface is
>>>>>>>>>> able to ping / dhcp and boot kernel using tftp in uboot.
>>>>>>>>>>
>>>>>>>>>> To use ICSSG2 ethernet, the ICSSG firmware needs to be loaded to
>>>>>>>>>> PRU RPROC
>>>>>>>>>> cores and RPROC cores need to be booted with the firmware. This
>>>>>>>>>> step is
>>>>>>>>>> done inside driver in kernel as kernel supports APIs like
>>>>>>>>>> rproc_set_firmware() and rproc_fw_boot(). But as u-boot doesn't
>>>>>>>>>> have these
>>>>>>>>>> APIs, the same needs to be done via u-boot cmds.
>>>>>>>>>>
>>>>>>>>>> To make sure icssg-eth works we need to do below steps.
>>>>>>>>>>
>>>>>>>>>> 1. Initialize rproc cores i.e. rproc_init()
>>>>>>>>>> 2. Load $firmware_file from partition '1:2' (root) on device (mmc
>>>>>>>>>> in this
>>>>>>>>>>     example)
>>>>>>>>>> 3. Load the firmware file to rproc cores passing. i.e. rproc_load()
>>>>>>>>>>     taking rproc_id, loadaddr and file size as arguments.
>>>>>>>>>> 4. Start rproc cores. i.e. rproc_start() taking rproc_id as arguments
>>>>>>>>>>
>>>>>>>>>> The above steps are done by running the below commands at u-boot
>>>>>>>>>> prompt.
>>>>>>>>>>
>>>>>>>>>> => setenv start_icssg2 'rproc start 14; rproc start 15; rproc
>>>>>>>>>> start 16; rproc start 17; rproc start 18; rproc start 19'
>>>>>>>>>> => setenv stop_icssg2 'rproc stop 14; rproc stop 15; rproc stop
>>>>>>>>>> 16; rproc stop 17; rproc stop 18; rproc stop 19'
>>>>>>>>>> => setenv firmware_dir '/lib/firmware/ti-pruss'
>>>>>>>>>> => setenv get_firmware_mmc 'load mmc ${bootpart} ${loadaddr}
>>>>>>>>>> ${firmware_dir}/${firmware_file}'
>>>>>>>>>>
>>>>>>>>>> => setenv init_icssg2 'setenv ethact icssg2-eth; setenv autoload
>>>>>>>>>> no; rproc init; setenv loadaddr 0x80000000; \
>>>>>>>>>>      setenv firmware_file am65x-sr2-pru0-prueth-fw.elf; run
>>>>>>>>>> get_firmware_mmc;  rproc load 14 0x80000000 ${filesize}; \
>>>>>>>>>>      setenv loadaddr 0x89000000; setenv firmware_file
>>>>>>>>>> am65x-sr2-rtu0-prueth-fw.elf; run get_firmware_mmc; rproc load 15
>>>>>>>>>> 0x89000000 ${filesize}; \
>>>>>>>>>>      setenv loadaddr 0x90000000; setenv firmware_file
>>>>>>>>>> am65x-sr2-txpru0-prueth-fw.elf; run get_firmware_mmc; rproc load
>>>>>>>>>> 16 0x90000000 ${filesize}; \
>>>>>>>>>>      setenv loadaddr 0x80000000; setenv firmware_file
>>>>>>>>>> am65x-sr2-pru1-prueth-fw.elf; run get_firmware_mmc; rproc load 17
>>>>>>>>>> 0x80000000 ${filesize}; \
>>>>>>>>>>      setenv loadaddr 0x89000000; setenv firmware_file
>>>>>>>>>> am65x-sr2-rtu1-prueth-fw.elf; run get_firmware_mmc; rproc load 18
>>>>>>>>>> 0x89000000 ${filesize}; \
>>>>>>>>>>      setenv loadaddr 0x90000000; setenv firmware_file
>>>>>>>>>> am65x-sr2-txpru1-prueth-fw.elf; run get_firmware_mmc; rproc load
>>>>>>>>>> 19 0x90000000 ${filesize}; \
>>>>>>>>>>      run start_icssg2;'
>>>>>>>>>
>>>>>>>>> A whole bunch of commands are required to get ethernet functional.
>>>>>>>>> This is not at all user friendly and will be a maintenance nightmare.
>>>>>>>>> What worries me is tracking the 6 different rproc cores and the 6
>>>>>>>>> different firmware files to start 1 ethernet device.
>>>>>>>>> This will get even more interesting when we have to deal with
>>>>>>>>> different ICSSG instances on different boards.
>>>>>>>>>
>>>>>>>>> What is preventing the driver from starting the rproc cores it
>>>>>>>>> needs so user doesn't need to care about it?
>>>>>>>>> All the necessary information is in the Device tree. At least this
>>>>>>>>> is how it is done on Linux.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I tried removing the need for these commands and implementing them
>>>>>>>> inside the driver only. I am able to load the firmware from driver
>>>>>>>> using
>>>>>>>> the fs_loader API request_firmware_into_buf(). It requires changes to
>>>>>>>> dt. A DT node called fs-loader needs to be added also CONFIG_FS_LOADER
>>>>>>>> needs to enabled. In the DT node we need to specify the storage media
>>>>>>>> that we are using i.e. mmc, ospi, usb. It's upto user to modify the
>>>>>>>> storage media, the driver will take the media from DT and try to laod
>>>>>>>> firmware from their.
>>>>>>>>
>>>>>>>> For loading firmwares to rproc cores, rproc_load() API is needed. Now
>>>>>>>> this API takes rproc_id, loadaddr and firmware_size as arguments.
>>>>>>>> loadaddr is fixed for all three pru cores. firmware_size is obtained
>>>>>>>> from request_firmware_into_buf() but I couldn't find a way to get the
>>>>>>>> rproc_id. For now based on the ICSSG instance and slice number I am
>>>>>>>> figuring out the rproc_id and passing it to rproc_load() and
>>>>>>>> rproc_start() APIs. Please let me know if you have any other / better
>>>>>>>> way of finding rproc_id.
>>>>>>>>
>>>>>>>> Below is the entire diff to remove these commands and move their
>>>>>>>> functionality to driver. Please have a look and let me know if this
>>>>>>>> is ok.
>>>>>>>>
>>>>>>>
>>>>>>> Good to see you got something working so quickly.
>>>>>>> It has some rough edges but nothing that is blocking.
>>>>>>>
>>>>>>>>
>>>>>>>> Save New Duplicate & Edit Just Text Twitter
>>>>>>>> diff --git a/arch/arm/dts/k3-am654-base-board.dts
>>>>>>>> b/arch/arm/dts/k3-am654-base-board.dts
>>>>>>>> index cfbcebfa37..c8da72e49c 100644
>>>>>>>> --- a/arch/arm/dts/k3-am654-base-board.dts
>>>>>>>> +++ b/arch/arm/dts/k3-am654-base-board.dts
>>>>>>>> @@ -16,6 +16,13 @@
>>>>>>>>       chosen {
>>>>>>>>           stdout-path = "serial2:115200n8";
>>>>>>>>           bootargs = "earlycon=ns16550a,mmio32,0x02800000";
>>>>>>>> +        firmware-loader = <&fs_loader0>;
>>>>>>>> +    };
>>>>>>>> +
>>>>>>>> +    fs_loader0: fs-loader {
>>>>>>>> +        bootph-all;
>>>>>>>> +        compatible = "u-boot,fs-loader";
>>>>>>>> +        phandlepart = <&sdhci1 2>;
>>>>>>>>       };
>>>>>>>
>>>>>>> This is has 2 issues
>>>>>>> 1) It will not be accepted in Kernel DT. Maybe it could be done in
>>>>>>> -u-boot.dtsi file.
>>>>>>
>>>>>> Sure. I'll move this to k3-am654-base-board-u-boot.dtsi
>>>>>>
>>>>>>> 2) You cannot assume boot medium is always sdhci1 2
>>>>>>>
>>>>>>
>>>>>> Yes. It's upto user to provide the boot medium and boot partition for
>>>>>> loading firmware. By default the firmware is loacated in root partion of
>>>>>> shdci1 on am65x so I am adding this as default. But user can easily
>>>>>> modify this to any other medium / partition needed.
>>>>>>
>>>>>
>>>>> Users should not have to modify DT to pick a boot medium/partition.
>>>>> What would you do for complex cases or non block devices like eth/uart
>>>>
>>> I agree with Andrew here.
>>>
>>>> In order to load firmware files from driver, we need to add the node for
>>>> fs-loader. The fs-loader has a phandlepart property which takes the boot
>>>> medium and the partition as input. Based on the medium and partition it
>>>> looks for the requested file and loads it to a given address. I am not
>>>> sure if there is any other way to load firmware from driver without
>>>> using the fs-loader.
>>>>
>>>>
>>>>> booting? This is one reason kernel moved firmware loading to
>>>>> userspace. The equivalant in U-Boot is the shell and scripts. Your
>>>>> original command based loading was the correct solution IMHO.
>>>>
>>>> I intended to go ahead with command base approach only but as Roger
>>>> pointed out the command based approach is not very user friendly.
>>>>
>>>> We need to align on what should be the correct approach for loading
>>>> firmware.
>>>>
>>>> Roger, can you please chime in here.
>>>
>>> My intention was to make it user friendly so he does not have to
>>> deal with 6 different Rproc IDs that can change between
>>> platforms. The solution also has to be seamless between different
>>> boot mediums. I think we can assume that the firmware will come from the
>>> same medium that the platform was booted.
>>>
>>
>> Yes and the with changes done by me using ICSSG port seems much easier
>> and user friendly. Just doing `dhcp` at u-boot prompt is enough to use
>> ICSSG port so the driver changes effectively makes the usage for user
>> friendly. But only way I have found to load the firmware files is to use
>> fs-loader that requires the boot medium and boot partition. I am not
>> sure how can I re-use the the boot medium used for booting u-boot images.
>>
>> Also in SD card, u-boot images are located in /boot partition where as
>> firmware is located inside /root partition. So we'll need to specify the
>> partition.
>>
>> Currently I don't have any way to load the firmware files from driver
>> without using below DT change
>>
>> fs_loader0: fs-loader {
>>         bootph-all;
>>         compatible = "u-boot,fs-loader";
>>         phandlepart = <&sdhci1 2>;
>> };
>>
> 
> Fromdoc/develop/driver-model/fs_firmware_loader.rst
> 
> "Firmware loader driver is also designed to support U-Boot environment
> variables, so all these data from FDT can be overwritten
> through the U-Boot environment variable during run time.
> 
> For examples:
> 
> storage_interface:
>   Storage interface, it can be "mmc", "usb", "sata" or "ubi".
> fw_dev_part:
>   Block device number and its partition, it can be "0:1".
> fw_ubi_mtdpart:
>   UBI device mtd partition, it can be "UBI".
> fw_ubi_volume:
>   UBI volume, it can be "ubi0".
> 
> When above environment variables are set, environment values would be
> used instead of data from FDT.
> The benefit of this design allows user to change storage attribute data
> at run time through U-Boot console and saving the setting as default
> environment values in the storage for the next power cycle, so no
> compilation is required for both driver and FDT."
> 
> So looks like we should provide this in environment variables instead of DT?
> 

Thanks Roger for digging this up. I tried setting the below environment
values and it works.

	storage_interface=mmc
	fw_dev_part=1:2

No need to add the fs-loader node in DT. We can directly set the env
values and fs_loader will use them to load file. I will drop the DT
change for fs-loader. I think this env approach is same as running the
load command at u-boot (the initial approach). I think any medium that
could be used using load command, can be used here by setting
appropriate env values.

Roger, Should I add the below code to include/env/ti/ti_common.env or
board/ti/am65x/am65x.env so that we don't need to set these variables
manually everytime we try to use ICSSG Ethernet and by default these
variables will be set to mmc and 1:2. User can definately modify these
at u-boot prompt and set appropriate values before running `dhcp`.

diff --git a/include/env/ti/ti_common.env b/include/env/ti/ti_common.env
index f0f89a2287..0861d3be34 100644
--- a/include/env/ti/ti_common.env
+++ b/include/env/ti/ti_common.env
@@ -35,3 +35,8 @@ bootcmd_ti_mmc=
        else;
                run get_kern_${boot}; run get_fdt_${boot}; run
get_overlay_${boot}; run run_kern;
        fi;
+
+#if CONFIG_TI_ICSSG_PRUETH
+storage_interface=mmc
+fw_dev_part=1:2
+#endif


>>>>
>>>>>
>>>>> If we want to try to hide some of that then we need a way to
>>>>> run a user provided script from the environement to handle
>>>>> the general cases for firmware loading.
>>>
>>> Assuming we go with the "script provided in environment: route,
>>> how do we deal with the Rproc IDs? I'm not sure if they are constant
>>> and can probably change based on which Rproc is registered first.
>>> So there needs to be some way to make sure user can reference
>>> the correct Rproc.
>>>
>>
>> The Rproc IDs aren't constant. I think the IDs depend on the sequence of
>> init. The only way to know the IDs of different Rprocs is to run `rproc
>> init` and then `rproc list`. Mostly on AM65x I have seen Rporc ID 0 and
>> 1 to be of r5 rprocs and from 2 to 19 of differnet PRUs (Total 18, 6 per
>> ICSSG instance). If r5 is not enabled, the sequence changes. Similarly
>> if some other rproc is enabled sequence will change.
>>
>> The script should be able to read the list and determine which rproc
>> needs to be loaded based on the port we want to use. I am not sure how
>> to do this during u-boot.
>>
> 
> rproc list gives an output in the following format.
> 
> => rproc list
> 0 - Name:'r5f@41000000' type:'internal memory mapped' supports: load start stop reset
> 1 - Name:'r5f@41400000' type:'internal memory mapped' supports: load start stop reset
> 2 - Name:'r5f@5c00000' type:'internal memory mapped' supports: load start stop reset
> 3 - Name:'r5f@5d00000' type:'internal memory mapped' supports: load start stop reset
> 4 - Name:'r5f@5e00000' type:'internal memory mapped' supports: load start stop reset
> 5 - Name:'r5f@5f00000' type:'internal memory mapped' supports: load start stop reset
> 6 - Name:'dsp@4d80800000' type:'internal memory mapped' supports: load start stop reset
> 7 - Name:'dsp@4d81800000' type:'internal memory mapped' supports: load start stop reset
> 8 - Name:'dsp@64800000' type:'internal memory mapped' supports: load start stop reset
> 
> How can the script know which rproc to start for a specific Ethernet interface?
> 

I am not sure. For ICSSGx port-y, we need to start pru_x_y, rtu_x_y,
tx_pru_x_y. The script could search for these core names and start the
core number corresponding to these names in the result of `rproc list`.

I think it will not be needed now as we can modify the envs to select
different mediums, instead of hardcoding DT.
Roger Quadros Jan. 9, 2024, 9:26 a.m. UTC | #12
On 08/01/2024 12:25, MD Danish Anwar wrote:
> On 08/01/24 3:00 pm, Roger Quadros wrote:
>>
>>
>> On 05/01/2024 12:15, Anwar, Md Danish wrote:
>>>
>>>
>>> On 1/5/2024 1:49 PM, Roger Quadros wrote:
>>>>
>>>>
>>>> On 03/01/2024 12:27, MD Danish Anwar wrote:
>>>>>
>>>>>
>>>>> On 02/01/24 7:26 pm, Andrew Davis wrote:
>>>>>> On 12/27/23 12:56 AM, MD Danish Anwar wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 22/12/23 6:13 pm, Roger Quadros wrote:
>>>>>>>>
>>>>>>>> On 22/12/2023 12:26, MD Danish Anwar wrote:
>>>>>>>>> Hi Roger,
>>>>>>>>>
>>>>>>>>> On 20/12/23 3:29 pm, Roger Quadros wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 19/12/2023 12:11, MD Danish Anwar wrote:
>>>>>>>>>>> Introduce ICSSG PRUETH support in uboot. The ICSSG driver is used
>>>>>>>>>>> in TI
>>>>>>>>>>> AM654 SR2.0.
>>>>>>>>>>>
>>>>>>>>>>> The ICSSG PRU Sub-system runs on EMAC firmware. This series
>>>>>>>>>>> Introduces
>>>>>>>>>>> support for ICSSG driver in uboot. This series also adds the driver's
>>>>>>>>>>> dependencies.
>>>>>>>>>>>
>>>>>>>>>>> The ICSSG2 node is added in device tree overlay so that it remains in
>>>>>>>>>>> sync with linux kernel.
>>>>>>>>>>>
>>>>>>>>>>> The series introduces device tree and config changes and AM65x
>>>>>>>>>>> to enable ICSSG driver. The series also enables
>>>>>>>>>>> SPL_LOAD_FIT_APPLY_OVERLAY
>>>>>>>>>>> for AM65x in order to load overlay over spl.
>>>>>>>>>>>
>>>>>>>>>>> This series has been tested on AM65x SR2.0, and the ICSSG
>>>>>>>>>>> interface is
>>>>>>>>>>> able to ping / dhcp and boot kernel using tftp in uboot.
>>>>>>>>>>>
>>>>>>>>>>> To use ICSSG2 ethernet, the ICSSG firmware needs to be loaded to
>>>>>>>>>>> PRU RPROC
>>>>>>>>>>> cores and RPROC cores need to be booted with the firmware. This
>>>>>>>>>>> step is
>>>>>>>>>>> done inside driver in kernel as kernel supports APIs like
>>>>>>>>>>> rproc_set_firmware() and rproc_fw_boot(). But as u-boot doesn't
>>>>>>>>>>> have these
>>>>>>>>>>> APIs, the same needs to be done via u-boot cmds.
>>>>>>>>>>>
>>>>>>>>>>> To make sure icssg-eth works we need to do below steps.
>>>>>>>>>>>
>>>>>>>>>>> 1. Initialize rproc cores i.e. rproc_init()
>>>>>>>>>>> 2. Load $firmware_file from partition '1:2' (root) on device (mmc
>>>>>>>>>>> in this
>>>>>>>>>>>     example)
>>>>>>>>>>> 3. Load the firmware file to rproc cores passing. i.e. rproc_load()
>>>>>>>>>>>     taking rproc_id, loadaddr and file size as arguments.
>>>>>>>>>>> 4. Start rproc cores. i.e. rproc_start() taking rproc_id as arguments
>>>>>>>>>>>
>>>>>>>>>>> The above steps are done by running the below commands at u-boot
>>>>>>>>>>> prompt.
>>>>>>>>>>>
>>>>>>>>>>> => setenv start_icssg2 'rproc start 14; rproc start 15; rproc
>>>>>>>>>>> start 16; rproc start 17; rproc start 18; rproc start 19'
>>>>>>>>>>> => setenv stop_icssg2 'rproc stop 14; rproc stop 15; rproc stop
>>>>>>>>>>> 16; rproc stop 17; rproc stop 18; rproc stop 19'
>>>>>>>>>>> => setenv firmware_dir '/lib/firmware/ti-pruss'
>>>>>>>>>>> => setenv get_firmware_mmc 'load mmc ${bootpart} ${loadaddr}
>>>>>>>>>>> ${firmware_dir}/${firmware_file}'
>>>>>>>>>>>
>>>>>>>>>>> => setenv init_icssg2 'setenv ethact icssg2-eth; setenv autoload
>>>>>>>>>>> no; rproc init; setenv loadaddr 0x80000000; \
>>>>>>>>>>>      setenv firmware_file am65x-sr2-pru0-prueth-fw.elf; run
>>>>>>>>>>> get_firmware_mmc;  rproc load 14 0x80000000 ${filesize}; \
>>>>>>>>>>>      setenv loadaddr 0x89000000; setenv firmware_file
>>>>>>>>>>> am65x-sr2-rtu0-prueth-fw.elf; run get_firmware_mmc; rproc load 15
>>>>>>>>>>> 0x89000000 ${filesize}; \
>>>>>>>>>>>      setenv loadaddr 0x90000000; setenv firmware_file
>>>>>>>>>>> am65x-sr2-txpru0-prueth-fw.elf; run get_firmware_mmc; rproc load
>>>>>>>>>>> 16 0x90000000 ${filesize}; \
>>>>>>>>>>>      setenv loadaddr 0x80000000; setenv firmware_file
>>>>>>>>>>> am65x-sr2-pru1-prueth-fw.elf; run get_firmware_mmc; rproc load 17
>>>>>>>>>>> 0x80000000 ${filesize}; \
>>>>>>>>>>>      setenv loadaddr 0x89000000; setenv firmware_file
>>>>>>>>>>> am65x-sr2-rtu1-prueth-fw.elf; run get_firmware_mmc; rproc load 18
>>>>>>>>>>> 0x89000000 ${filesize}; \
>>>>>>>>>>>      setenv loadaddr 0x90000000; setenv firmware_file
>>>>>>>>>>> am65x-sr2-txpru1-prueth-fw.elf; run get_firmware_mmc; rproc load
>>>>>>>>>>> 19 0x90000000 ${filesize}; \
>>>>>>>>>>>      run start_icssg2;'
>>>>>>>>>>
>>>>>>>>>> A whole bunch of commands are required to get ethernet functional.
>>>>>>>>>> This is not at all user friendly and will be a maintenance nightmare.
>>>>>>>>>> What worries me is tracking the 6 different rproc cores and the 6
>>>>>>>>>> different firmware files to start 1 ethernet device.
>>>>>>>>>> This will get even more interesting when we have to deal with
>>>>>>>>>> different ICSSG instances on different boards.
>>>>>>>>>>
>>>>>>>>>> What is preventing the driver from starting the rproc cores it
>>>>>>>>>> needs so user doesn't need to care about it?
>>>>>>>>>> All the necessary information is in the Device tree. At least this
>>>>>>>>>> is how it is done on Linux.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I tried removing the need for these commands and implementing them
>>>>>>>>> inside the driver only. I am able to load the firmware from driver
>>>>>>>>> using
>>>>>>>>> the fs_loader API request_firmware_into_buf(). It requires changes to
>>>>>>>>> dt. A DT node called fs-loader needs to be added also CONFIG_FS_LOADER
>>>>>>>>> needs to enabled. In the DT node we need to specify the storage media
>>>>>>>>> that we are using i.e. mmc, ospi, usb. It's upto user to modify the
>>>>>>>>> storage media, the driver will take the media from DT and try to laod
>>>>>>>>> firmware from their.
>>>>>>>>>
>>>>>>>>> For loading firmwares to rproc cores, rproc_load() API is needed. Now
>>>>>>>>> this API takes rproc_id, loadaddr and firmware_size as arguments.
>>>>>>>>> loadaddr is fixed for all three pru cores. firmware_size is obtained
>>>>>>>>> from request_firmware_into_buf() but I couldn't find a way to get the
>>>>>>>>> rproc_id. For now based on the ICSSG instance and slice number I am
>>>>>>>>> figuring out the rproc_id and passing it to rproc_load() and
>>>>>>>>> rproc_start() APIs. Please let me know if you have any other / better
>>>>>>>>> way of finding rproc_id.
>>>>>>>>>
>>>>>>>>> Below is the entire diff to remove these commands and move their
>>>>>>>>> functionality to driver. Please have a look and let me know if this
>>>>>>>>> is ok.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Good to see you got something working so quickly.
>>>>>>>> It has some rough edges but nothing that is blocking.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Save New Duplicate & Edit Just Text Twitter
>>>>>>>>> diff --git a/arch/arm/dts/k3-am654-base-board.dts
>>>>>>>>> b/arch/arm/dts/k3-am654-base-board.dts
>>>>>>>>> index cfbcebfa37..c8da72e49c 100644
>>>>>>>>> --- a/arch/arm/dts/k3-am654-base-board.dts
>>>>>>>>> +++ b/arch/arm/dts/k3-am654-base-board.dts
>>>>>>>>> @@ -16,6 +16,13 @@
>>>>>>>>>       chosen {
>>>>>>>>>           stdout-path = "serial2:115200n8";
>>>>>>>>>           bootargs = "earlycon=ns16550a,mmio32,0x02800000";
>>>>>>>>> +        firmware-loader = <&fs_loader0>;
>>>>>>>>> +    };
>>>>>>>>> +
>>>>>>>>> +    fs_loader0: fs-loader {
>>>>>>>>> +        bootph-all;
>>>>>>>>> +        compatible = "u-boot,fs-loader";
>>>>>>>>> +        phandlepart = <&sdhci1 2>;
>>>>>>>>>       };
>>>>>>>>
>>>>>>>> This is has 2 issues
>>>>>>>> 1) It will not be accepted in Kernel DT. Maybe it could be done in
>>>>>>>> -u-boot.dtsi file.
>>>>>>>
>>>>>>> Sure. I'll move this to k3-am654-base-board-u-boot.dtsi
>>>>>>>
>>>>>>>> 2) You cannot assume boot medium is always sdhci1 2
>>>>>>>>
>>>>>>>
>>>>>>> Yes. It's upto user to provide the boot medium and boot partition for
>>>>>>> loading firmware. By default the firmware is loacated in root partion of
>>>>>>> shdci1 on am65x so I am adding this as default. But user can easily
>>>>>>> modify this to any other medium / partition needed.
>>>>>>>
>>>>>>
>>>>>> Users should not have to modify DT to pick a boot medium/partition.
>>>>>> What would you do for complex cases or non block devices like eth/uart
>>>>>
>>>> I agree with Andrew here.
>>>>
>>>>> In order to load firmware files from driver, we need to add the node for
>>>>> fs-loader. The fs-loader has a phandlepart property which takes the boot
>>>>> medium and the partition as input. Based on the medium and partition it
>>>>> looks for the requested file and loads it to a given address. I am not
>>>>> sure if there is any other way to load firmware from driver without
>>>>> using the fs-loader.
>>>>>
>>>>>
>>>>>> booting? This is one reason kernel moved firmware loading to
>>>>>> userspace. The equivalant in U-Boot is the shell and scripts. Your
>>>>>> original command based loading was the correct solution IMHO.
>>>>>
>>>>> I intended to go ahead with command base approach only but as Roger
>>>>> pointed out the command based approach is not very user friendly.
>>>>>
>>>>> We need to align on what should be the correct approach for loading
>>>>> firmware.
>>>>>
>>>>> Roger, can you please chime in here.
>>>>
>>>> My intention was to make it user friendly so he does not have to
>>>> deal with 6 different Rproc IDs that can change between
>>>> platforms. The solution also has to be seamless between different
>>>> boot mediums. I think we can assume that the firmware will come from the
>>>> same medium that the platform was booted.
>>>>
>>>
>>> Yes and the with changes done by me using ICSSG port seems much easier
>>> and user friendly. Just doing `dhcp` at u-boot prompt is enough to use
>>> ICSSG port so the driver changes effectively makes the usage for user
>>> friendly. But only way I have found to load the firmware files is to use
>>> fs-loader that requires the boot medium and boot partition. I am not
>>> sure how can I re-use the the boot medium used for booting u-boot images.
>>>
>>> Also in SD card, u-boot images are located in /boot partition where as
>>> firmware is located inside /root partition. So we'll need to specify the
>>> partition.
>>>
>>> Currently I don't have any way to load the firmware files from driver
>>> without using below DT change
>>>
>>> fs_loader0: fs-loader {
>>>         bootph-all;
>>>         compatible = "u-boot,fs-loader";
>>>         phandlepart = <&sdhci1 2>;
>>> };
>>>
>>
>> Fromdoc/develop/driver-model/fs_firmware_loader.rst
>>
>> "Firmware loader driver is also designed to support U-Boot environment
>> variables, so all these data from FDT can be overwritten
>> through the U-Boot environment variable during run time.
>>
>> For examples:
>>
>> storage_interface:
>>   Storage interface, it can be "mmc", "usb", "sata" or "ubi".
>> fw_dev_part:
>>   Block device number and its partition, it can be "0:1".
>> fw_ubi_mtdpart:
>>   UBI device mtd partition, it can be "UBI".
>> fw_ubi_volume:
>>   UBI volume, it can be "ubi0".
>>
>> When above environment variables are set, environment values would be
>> used instead of data from FDT.
>> The benefit of this design allows user to change storage attribute data
>> at run time through U-Boot console and saving the setting as default
>> environment values in the storage for the next power cycle, so no
>> compilation is required for both driver and FDT."
>>
>> So looks like we should provide this in environment variables instead of DT?
>>
> 
> Thanks Roger for digging this up. I tried setting the below environment
> values and it works.
> 
> 	storage_interface=mmc
> 	fw_dev_part=1:2
> 
> No need to add the fs-loader node in DT. We can directly set the env
> values and fs_loader will use them to load file. I will drop the DT
> change for fs-loader. I think this env approach is same as running the
> load command at u-boot (the initial approach). I think any medium that
> could be used using load command, can be used here by setting
> appropriate env values.
> 
> Roger, Should I add the below code to include/env/ti/ti_common.env or
> board/ti/am65x/am65x.env so that we don't need to set these variables
> manually everytime we try to use ICSSG Ethernet and by default these
> variables will be set to mmc and 1:2. User can definately modify these
> at u-boot prompt and set appropriate values before running `dhcp`.
> 
> diff --git a/include/env/ti/ti_common.env b/include/env/ti/ti_common.env
> index f0f89a2287..0861d3be34 100644
> --- a/include/env/ti/ti_common.env
> +++ b/include/env/ti/ti_common.env
> @@ -35,3 +35,8 @@ bootcmd_ti_mmc=
>         else;
>                 run get_kern_${boot}; run get_fdt_${boot}; run
> get_overlay_${boot}; run run_kern;
>         fi;
> +
> +#if CONFIG_TI_ICSSG_PRUETH
> +storage_interface=mmc

"storage_interface" is vague. It should be updated to fw_storage_interface in fs-loader driver
and environment. It could fallback to storage_interface to not break existing implementations.

> +fw_dev_part=1:2
> +#endif
> 

I think it should go in the respective <board>.env file as not all TI platforms
have the same boot/firmware partition.
so I would put it in board/ti/am65x/am65x.env

> 
>>>>>
>>>>>>
>>>>>> If we want to try to hide some of that then we need a way to
>>>>>> run a user provided script from the environement to handle
>>>>>> the general cases for firmware loading.
>>>>
>>>> Assuming we go with the "script provided in environment: route,
>>>> how do we deal with the Rproc IDs? I'm not sure if they are constant
>>>> and can probably change based on which Rproc is registered first.
>>>> So there needs to be some way to make sure user can reference
>>>> the correct Rproc.
>>>>
>>>
>>> The Rproc IDs aren't constant. I think the IDs depend on the sequence of
>>> init. The only way to know the IDs of different Rprocs is to run `rproc
>>> init` and then `rproc list`. Mostly on AM65x I have seen Rporc ID 0 and
>>> 1 to be of r5 rprocs and from 2 to 19 of differnet PRUs (Total 18, 6 per
>>> ICSSG instance). If r5 is not enabled, the sequence changes. Similarly
>>> if some other rproc is enabled sequence will change.
>>>
>>> The script should be able to read the list and determine which rproc
>>> needs to be loaded based on the port we want to use. I am not sure how
>>> to do this during u-boot.
>>>
>>
>> rproc list gives an output in the following format.
>>
>> => rproc list
>> 0 - Name:'r5f@41000000' type:'internal memory mapped' supports: load start stop reset
>> 1 - Name:'r5f@41400000' type:'internal memory mapped' supports: load start stop reset
>> 2 - Name:'r5f@5c00000' type:'internal memory mapped' supports: load start stop reset
>> 3 - Name:'r5f@5d00000' type:'internal memory mapped' supports: load start stop reset
>> 4 - Name:'r5f@5e00000' type:'internal memory mapped' supports: load start stop reset
>> 5 - Name:'r5f@5f00000' type:'internal memory mapped' supports: load start stop reset
>> 6 - Name:'dsp@4d80800000' type:'internal memory mapped' supports: load start stop reset
>> 7 - Name:'dsp@4d81800000' type:'internal memory mapped' supports: load start stop reset
>> 8 - Name:'dsp@64800000' type:'internal memory mapped' supports: load start stop reset
>>
>> How can the script know which rproc to start for a specific Ethernet interface?
>>
> 
> I am not sure. For ICSSGx port-y, we need to start pru_x_y, rtu_x_y,
> tx_pru_x_y. The script could search for these core names and start the
> core number corresponding to these names in the result of `rproc list`.
> 
> I think it will not be needed now as we can modify the envs to select
> different mediums, instead of hardcoding DT.
> 

OK.
MD Danish Anwar Jan. 9, 2024, 9:49 a.m. UTC | #13
On 09/01/24 2:56 pm, Roger Quadros wrote:
> 
> 
> On 08/01/2024 12:25, MD Danish Anwar wrote:
>> On 08/01/24 3:00 pm, Roger Quadros wrote:
>>>
>>>
>>> On 05/01/2024 12:15, Anwar, Md Danish wrote:
>>>>
>>>>
>>>> On 1/5/2024 1:49 PM, Roger Quadros wrote:
>>>>>
>>>>>
>>>>> On 03/01/2024 12:27, MD Danish Anwar wrote:
>>>>>>
>>>>>>
>>>>>> On 02/01/24 7:26 pm, Andrew Davis wrote:
>>>>>>> On 12/27/23 12:56 AM, MD Danish Anwar wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 22/12/23 6:13 pm, Roger Quadros wrote:
>>>>>>>>>
>>>>>>>>> On 22/12/2023 12:26, MD Danish Anwar wrote:
>>>>>>>>>> Hi Roger,
>>>>>>>>>>
>>>>>>>>>> On 20/12/23 3:29 pm, Roger Quadros wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On 19/12/2023 12:11, MD Danish Anwar wrote:
>>>>>>>>>>>> Introduce ICSSG PRUETH support in uboot. The ICSSG driver is used
>>>>>>>>>>>> in TI
>>>>>>>>>>>> AM654 SR2.0.
>>>>>>>>>>>>
>>>>>>>>>>>> The ICSSG PRU Sub-system runs on EMAC firmware. This series
>>>>>>>>>>>> Introduces
>>>>>>>>>>>> support for ICSSG driver in uboot. This series also adds the driver's
>>>>>>>>>>>> dependencies.
>>>>>>>>>>>>
>>>>>>>>>>>> The ICSSG2 node is added in device tree overlay so that it remains in
>>>>>>>>>>>> sync with linux kernel.
>>>>>>>>>>>>
>>>>>>>>>>>> The series introduces device tree and config changes and AM65x
>>>>>>>>>>>> to enable ICSSG driver. The series also enables
>>>>>>>>>>>> SPL_LOAD_FIT_APPLY_OVERLAY
>>>>>>>>>>>> for AM65x in order to load overlay over spl.
>>>>>>>>>>>>
>>>>>>>>>>>> This series has been tested on AM65x SR2.0, and the ICSSG
>>>>>>>>>>>> interface is
>>>>>>>>>>>> able to ping / dhcp and boot kernel using tftp in uboot.
>>>>>>>>>>>>
>>>>>>>>>>>> To use ICSSG2 ethernet, the ICSSG firmware needs to be loaded to
>>>>>>>>>>>> PRU RPROC
>>>>>>>>>>>> cores and RPROC cores need to be booted with the firmware. This
>>>>>>>>>>>> step is
>>>>>>>>>>>> done inside driver in kernel as kernel supports APIs like
>>>>>>>>>>>> rproc_set_firmware() and rproc_fw_boot(). But as u-boot doesn't
>>>>>>>>>>>> have these
>>>>>>>>>>>> APIs, the same needs to be done via u-boot cmds.
>>>>>>>>>>>>
>>>>>>>>>>>> To make sure icssg-eth works we need to do below steps.
>>>>>>>>>>>>
>>>>>>>>>>>> 1. Initialize rproc cores i.e. rproc_init()
>>>>>>>>>>>> 2. Load $firmware_file from partition '1:2' (root) on device (mmc
>>>>>>>>>>>> in this
>>>>>>>>>>>>     example)
>>>>>>>>>>>> 3. Load the firmware file to rproc cores passing. i.e. rproc_load()
>>>>>>>>>>>>     taking rproc_id, loadaddr and file size as arguments.
>>>>>>>>>>>> 4. Start rproc cores. i.e. rproc_start() taking rproc_id as arguments
>>>>>>>>>>>>
>>>>>>>>>>>> The above steps are done by running the below commands at u-boot
>>>>>>>>>>>> prompt.
>>>>>>>>>>>>
>>>>>>>>>>>> => setenv start_icssg2 'rproc start 14; rproc start 15; rproc
>>>>>>>>>>>> start 16; rproc start 17; rproc start 18; rproc start 19'
>>>>>>>>>>>> => setenv stop_icssg2 'rproc stop 14; rproc stop 15; rproc stop
>>>>>>>>>>>> 16; rproc stop 17; rproc stop 18; rproc stop 19'
>>>>>>>>>>>> => setenv firmware_dir '/lib/firmware/ti-pruss'
>>>>>>>>>>>> => setenv get_firmware_mmc 'load mmc ${bootpart} ${loadaddr}
>>>>>>>>>>>> ${firmware_dir}/${firmware_file}'
>>>>>>>>>>>>
>>>>>>>>>>>> => setenv init_icssg2 'setenv ethact icssg2-eth; setenv autoload
>>>>>>>>>>>> no; rproc init; setenv loadaddr 0x80000000; \
>>>>>>>>>>>>      setenv firmware_file am65x-sr2-pru0-prueth-fw.elf; run
>>>>>>>>>>>> get_firmware_mmc;  rproc load 14 0x80000000 ${filesize}; \
>>>>>>>>>>>>      setenv loadaddr 0x89000000; setenv firmware_file
>>>>>>>>>>>> am65x-sr2-rtu0-prueth-fw.elf; run get_firmware_mmc; rproc load 15
>>>>>>>>>>>> 0x89000000 ${filesize}; \
>>>>>>>>>>>>      setenv loadaddr 0x90000000; setenv firmware_file
>>>>>>>>>>>> am65x-sr2-txpru0-prueth-fw.elf; run get_firmware_mmc; rproc load
>>>>>>>>>>>> 16 0x90000000 ${filesize}; \
>>>>>>>>>>>>      setenv loadaddr 0x80000000; setenv firmware_file
>>>>>>>>>>>> am65x-sr2-pru1-prueth-fw.elf; run get_firmware_mmc; rproc load 17
>>>>>>>>>>>> 0x80000000 ${filesize}; \
>>>>>>>>>>>>      setenv loadaddr 0x89000000; setenv firmware_file
>>>>>>>>>>>> am65x-sr2-rtu1-prueth-fw.elf; run get_firmware_mmc; rproc load 18
>>>>>>>>>>>> 0x89000000 ${filesize}; \
>>>>>>>>>>>>      setenv loadaddr 0x90000000; setenv firmware_file
>>>>>>>>>>>> am65x-sr2-txpru1-prueth-fw.elf; run get_firmware_mmc; rproc load
>>>>>>>>>>>> 19 0x90000000 ${filesize}; \
>>>>>>>>>>>>      run start_icssg2;'
>>>>>>>>>>>
>>>>>>>>>>> A whole bunch of commands are required to get ethernet functional.
>>>>>>>>>>> This is not at all user friendly and will be a maintenance nightmare.
>>>>>>>>>>> What worries me is tracking the 6 different rproc cores and the 6
>>>>>>>>>>> different firmware files to start 1 ethernet device.
>>>>>>>>>>> This will get even more interesting when we have to deal with
>>>>>>>>>>> different ICSSG instances on different boards.
>>>>>>>>>>>
>>>>>>>>>>> What is preventing the driver from starting the rproc cores it
>>>>>>>>>>> needs so user doesn't need to care about it?
>>>>>>>>>>> All the necessary information is in the Device tree. At least this
>>>>>>>>>>> is how it is done on Linux.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I tried removing the need for these commands and implementing them
>>>>>>>>>> inside the driver only. I am able to load the firmware from driver
>>>>>>>>>> using
>>>>>>>>>> the fs_loader API request_firmware_into_buf(). It requires changes to
>>>>>>>>>> dt. A DT node called fs-loader needs to be added also CONFIG_FS_LOADER
>>>>>>>>>> needs to enabled. In the DT node we need to specify the storage media
>>>>>>>>>> that we are using i.e. mmc, ospi, usb. It's upto user to modify the
>>>>>>>>>> storage media, the driver will take the media from DT and try to laod
>>>>>>>>>> firmware from their.
>>>>>>>>>>
>>>>>>>>>> For loading firmwares to rproc cores, rproc_load() API is needed. Now
>>>>>>>>>> this API takes rproc_id, loadaddr and firmware_size as arguments.
>>>>>>>>>> loadaddr is fixed for all three pru cores. firmware_size is obtained
>>>>>>>>>> from request_firmware_into_buf() but I couldn't find a way to get the
>>>>>>>>>> rproc_id. For now based on the ICSSG instance and slice number I am
>>>>>>>>>> figuring out the rproc_id and passing it to rproc_load() and
>>>>>>>>>> rproc_start() APIs. Please let me know if you have any other / better
>>>>>>>>>> way of finding rproc_id.
>>>>>>>>>>
>>>>>>>>>> Below is the entire diff to remove these commands and move their
>>>>>>>>>> functionality to driver. Please have a look and let me know if this
>>>>>>>>>> is ok.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Good to see you got something working so quickly.
>>>>>>>>> It has some rough edges but nothing that is blocking.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Save New Duplicate & Edit Just Text Twitter
>>>>>>>>>> diff --git a/arch/arm/dts/k3-am654-base-board.dts
>>>>>>>>>> b/arch/arm/dts/k3-am654-base-board.dts
>>>>>>>>>> index cfbcebfa37..c8da72e49c 100644
>>>>>>>>>> --- a/arch/arm/dts/k3-am654-base-board.dts
>>>>>>>>>> +++ b/arch/arm/dts/k3-am654-base-board.dts
>>>>>>>>>> @@ -16,6 +16,13 @@
>>>>>>>>>>       chosen {
>>>>>>>>>>           stdout-path = "serial2:115200n8";
>>>>>>>>>>           bootargs = "earlycon=ns16550a,mmio32,0x02800000";
>>>>>>>>>> +        firmware-loader = <&fs_loader0>;
>>>>>>>>>> +    };
>>>>>>>>>> +
>>>>>>>>>> +    fs_loader0: fs-loader {
>>>>>>>>>> +        bootph-all;
>>>>>>>>>> +        compatible = "u-boot,fs-loader";
>>>>>>>>>> +        phandlepart = <&sdhci1 2>;
>>>>>>>>>>       };
>>>>>>>>>
>>>>>>>>> This is has 2 issues
>>>>>>>>> 1) It will not be accepted in Kernel DT. Maybe it could be done in
>>>>>>>>> -u-boot.dtsi file.
>>>>>>>>
>>>>>>>> Sure. I'll move this to k3-am654-base-board-u-boot.dtsi
>>>>>>>>
>>>>>>>>> 2) You cannot assume boot medium is always sdhci1 2
>>>>>>>>>
>>>>>>>>
>>>>>>>> Yes. It's upto user to provide the boot medium and boot partition for
>>>>>>>> loading firmware. By default the firmware is loacated in root partion of
>>>>>>>> shdci1 on am65x so I am adding this as default. But user can easily
>>>>>>>> modify this to any other medium / partition needed.
>>>>>>>>
>>>>>>>
>>>>>>> Users should not have to modify DT to pick a boot medium/partition.
>>>>>>> What would you do for complex cases or non block devices like eth/uart
>>>>>>
>>>>> I agree with Andrew here.
>>>>>
>>>>>> In order to load firmware files from driver, we need to add the node for
>>>>>> fs-loader. The fs-loader has a phandlepart property which takes the boot
>>>>>> medium and the partition as input. Based on the medium and partition it
>>>>>> looks for the requested file and loads it to a given address. I am not
>>>>>> sure if there is any other way to load firmware from driver without
>>>>>> using the fs-loader.
>>>>>>
>>>>>>
>>>>>>> booting? This is one reason kernel moved firmware loading to
>>>>>>> userspace. The equivalant in U-Boot is the shell and scripts. Your
>>>>>>> original command based loading was the correct solution IMHO.
>>>>>>
>>>>>> I intended to go ahead with command base approach only but as Roger
>>>>>> pointed out the command based approach is not very user friendly.
>>>>>>
>>>>>> We need to align on what should be the correct approach for loading
>>>>>> firmware.
>>>>>>
>>>>>> Roger, can you please chime in here.
>>>>>
>>>>> My intention was to make it user friendly so he does not have to
>>>>> deal with 6 different Rproc IDs that can change between
>>>>> platforms. The solution also has to be seamless between different
>>>>> boot mediums. I think we can assume that the firmware will come from the
>>>>> same medium that the platform was booted.
>>>>>
>>>>
>>>> Yes and the with changes done by me using ICSSG port seems much easier
>>>> and user friendly. Just doing `dhcp` at u-boot prompt is enough to use
>>>> ICSSG port so the driver changes effectively makes the usage for user
>>>> friendly. But only way I have found to load the firmware files is to use
>>>> fs-loader that requires the boot medium and boot partition. I am not
>>>> sure how can I re-use the the boot medium used for booting u-boot images.
>>>>
>>>> Also in SD card, u-boot images are located in /boot partition where as
>>>> firmware is located inside /root partition. So we'll need to specify the
>>>> partition.
>>>>
>>>> Currently I don't have any way to load the firmware files from driver
>>>> without using below DT change
>>>>
>>>> fs_loader0: fs-loader {
>>>>         bootph-all;
>>>>         compatible = "u-boot,fs-loader";
>>>>         phandlepart = <&sdhci1 2>;
>>>> };
>>>>
>>>
>>> Fromdoc/develop/driver-model/fs_firmware_loader.rst
>>>
>>> "Firmware loader driver is also designed to support U-Boot environment
>>> variables, so all these data from FDT can be overwritten
>>> through the U-Boot environment variable during run time.
>>>
>>> For examples:
>>>
>>> storage_interface:
>>>   Storage interface, it can be "mmc", "usb", "sata" or "ubi".
>>> fw_dev_part:
>>>   Block device number and its partition, it can be "0:1".
>>> fw_ubi_mtdpart:
>>>   UBI device mtd partition, it can be "UBI".
>>> fw_ubi_volume:
>>>   UBI volume, it can be "ubi0".
>>>
>>> When above environment variables are set, environment values would be
>>> used instead of data from FDT.
>>> The benefit of this design allows user to change storage attribute data
>>> at run time through U-Boot console and saving the setting as default
>>> environment values in the storage for the next power cycle, so no
>>> compilation is required for both driver and FDT."
>>>
>>> So looks like we should provide this in environment variables instead of DT?
>>>
>>
>> Thanks Roger for digging this up. I tried setting the below environment
>> values and it works.
>>
>> 	storage_interface=mmc
>> 	fw_dev_part=1:2
>>
>> No need to add the fs-loader node in DT. We can directly set the env
>> values and fs_loader will use them to load file. I will drop the DT
>> change for fs-loader. I think this env approach is same as running the
>> load command at u-boot (the initial approach). I think any medium that
>> could be used using load command, can be used here by setting
>> appropriate env values.
>>
>> Roger, Should I add the below code to include/env/ti/ti_common.env or
>> board/ti/am65x/am65x.env so that we don't need to set these variables
>> manually everytime we try to use ICSSG Ethernet and by default these
>> variables will be set to mmc and 1:2. User can definately modify these
>> at u-boot prompt and set appropriate values before running `dhcp`.
>>
>> diff --git a/include/env/ti/ti_common.env b/include/env/ti/ti_common.env
>> index f0f89a2287..0861d3be34 100644
>> --- a/include/env/ti/ti_common.env
>> +++ b/include/env/ti/ti_common.env
>> @@ -35,3 +35,8 @@ bootcmd_ti_mmc=
>>         else;
>>                 run get_kern_${boot}; run get_fdt_${boot}; run
>> get_overlay_${boot}; run run_kern;
>>         fi;
>> +
>> +#if CONFIG_TI_ICSSG_PRUETH
>> +storage_interface=mmc
> 
> "storage_interface" is vague. It should be updated to fw_storage_interface in fs-loader driver
> and environment. It could fallback to storage_interface to not break existing implementations.
> 

Sure. But changing "storage_interface" to "fw_storage_interface" in  the
fs-loader driver will also require changing the variable name in all the
users drivers (e.g. arch/arm/mach-k3/common.c)

>> +fw_dev_part=1:2
>> +#endif
>>
> 
> I think it should go in the respective <board>.env file as not all TI platforms
> have the same boot/firmware partition.
> so I would put it in board/ti/am65x/am65x.env
> 

Sure, I'll keep it in board/ti/am65x/am65x.env.

Andrew, Can you please confirm if you are okay with this approach. I
will post v2 soon, once you confirm.

>>
>>>>>>
>>>>>>>
>>>>>>> If we want to try to hide some of that then we need a way to
>>>>>>> run a user provided script from the environement to handle
>>>>>>> the general cases for firmware loading.
>>>>>
>>>>> Assuming we go with the "script provided in environment: route,
>>>>> how do we deal with the Rproc IDs? I'm not sure if they are constant
>>>>> and can probably change based on which Rproc is registered first.
>>>>> So there needs to be some way to make sure user can reference
>>>>> the correct Rproc.
>>>>>
>>>>
>>>> The Rproc IDs aren't constant. I think the IDs depend on the sequence of
>>>> init. The only way to know the IDs of different Rprocs is to run `rproc
>>>> init` and then `rproc list`. Mostly on AM65x I have seen Rporc ID 0 and
>>>> 1 to be of r5 rprocs and from 2 to 19 of differnet PRUs (Total 18, 6 per
>>>> ICSSG instance). If r5 is not enabled, the sequence changes. Similarly
>>>> if some other rproc is enabled sequence will change.
>>>>
>>>> The script should be able to read the list and determine which rproc
>>>> needs to be loaded based on the port we want to use. I am not sure how
>>>> to do this during u-boot.
>>>>
>>>
>>> rproc list gives an output in the following format.
>>>
>>> => rproc list
>>> 0 - Name:'r5f@41000000' type:'internal memory mapped' supports: load start stop reset
>>> 1 - Name:'r5f@41400000' type:'internal memory mapped' supports: load start stop reset
>>> 2 - Name:'r5f@5c00000' type:'internal memory mapped' supports: load start stop reset
>>> 3 - Name:'r5f@5d00000' type:'internal memory mapped' supports: load start stop reset
>>> 4 - Name:'r5f@5e00000' type:'internal memory mapped' supports: load start stop reset
>>> 5 - Name:'r5f@5f00000' type:'internal memory mapped' supports: load start stop reset
>>> 6 - Name:'dsp@4d80800000' type:'internal memory mapped' supports: load start stop reset
>>> 7 - Name:'dsp@4d81800000' type:'internal memory mapped' supports: load start stop reset
>>> 8 - Name:'dsp@64800000' type:'internal memory mapped' supports: load start stop reset
>>>
>>> How can the script know which rproc to start for a specific Ethernet interface?
>>>
>>
>> I am not sure. For ICSSGx port-y, we need to start pru_x_y, rtu_x_y,
>> tx_pru_x_y. The script could search for these core names and start the
>> core number corresponding to these names in the result of `rproc list`.
>>
>> I think it will not be needed now as we can modify the envs to select
>> different mediums, instead of hardcoding DT.
>>
> 
> OK.
>
Roger Quadros Jan. 9, 2024, 12:34 p.m. UTC | #14
On 09/01/2024 11:49, MD Danish Anwar wrote:
> 
> 
> On 09/01/24 2:56 pm, Roger Quadros wrote:
>>
>>
>> On 08/01/2024 12:25, MD Danish Anwar wrote:
>>> On 08/01/24 3:00 pm, Roger Quadros wrote:
>>>>
>>>>
>>>> On 05/01/2024 12:15, Anwar, Md Danish wrote:
>>>>>
>>>>>
>>>>> On 1/5/2024 1:49 PM, Roger Quadros wrote:
>>>>>>
>>>>>>
>>>>>> On 03/01/2024 12:27, MD Danish Anwar wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 02/01/24 7:26 pm, Andrew Davis wrote:
>>>>>>>> On 12/27/23 12:56 AM, MD Danish Anwar wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 22/12/23 6:13 pm, Roger Quadros wrote:
>>>>>>>>>>
>>>>>>>>>> On 22/12/2023 12:26, MD Danish Anwar wrote:
>>>>>>>>>>> Hi Roger,
>>>>>>>>>>>
>>>>>>>>>>> On 20/12/23 3:29 pm, Roger Quadros wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> On 19/12/2023 12:11, MD Danish Anwar wrote:
>>>>>>>>>>>>> Introduce ICSSG PRUETH support in uboot. The ICSSG driver is used
>>>>>>>>>>>>> in TI
>>>>>>>>>>>>> AM654 SR2.0.
>>>>>>>>>>>>>
>>>>>>>>>>>>> The ICSSG PRU Sub-system runs on EMAC firmware. This series
>>>>>>>>>>>>> Introduces
>>>>>>>>>>>>> support for ICSSG driver in uboot. This series also adds the driver's
>>>>>>>>>>>>> dependencies.
>>>>>>>>>>>>>
>>>>>>>>>>>>> The ICSSG2 node is added in device tree overlay so that it remains in
>>>>>>>>>>>>> sync with linux kernel.
>>>>>>>>>>>>>
>>>>>>>>>>>>> The series introduces device tree and config changes and AM65x
>>>>>>>>>>>>> to enable ICSSG driver. The series also enables
>>>>>>>>>>>>> SPL_LOAD_FIT_APPLY_OVERLAY
>>>>>>>>>>>>> for AM65x in order to load overlay over spl.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This series has been tested on AM65x SR2.0, and the ICSSG
>>>>>>>>>>>>> interface is
>>>>>>>>>>>>> able to ping / dhcp and boot kernel using tftp in uboot.
>>>>>>>>>>>>>
>>>>>>>>>>>>> To use ICSSG2 ethernet, the ICSSG firmware needs to be loaded to
>>>>>>>>>>>>> PRU RPROC
>>>>>>>>>>>>> cores and RPROC cores need to be booted with the firmware. This
>>>>>>>>>>>>> step is
>>>>>>>>>>>>> done inside driver in kernel as kernel supports APIs like
>>>>>>>>>>>>> rproc_set_firmware() and rproc_fw_boot(). But as u-boot doesn't
>>>>>>>>>>>>> have these
>>>>>>>>>>>>> APIs, the same needs to be done via u-boot cmds.
>>>>>>>>>>>>>
>>>>>>>>>>>>> To make sure icssg-eth works we need to do below steps.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1. Initialize rproc cores i.e. rproc_init()
>>>>>>>>>>>>> 2. Load $firmware_file from partition '1:2' (root) on device (mmc
>>>>>>>>>>>>> in this
>>>>>>>>>>>>>     example)
>>>>>>>>>>>>> 3. Load the firmware file to rproc cores passing. i.e. rproc_load()
>>>>>>>>>>>>>     taking rproc_id, loadaddr and file size as arguments.
>>>>>>>>>>>>> 4. Start rproc cores. i.e. rproc_start() taking rproc_id as arguments
>>>>>>>>>>>>>
>>>>>>>>>>>>> The above steps are done by running the below commands at u-boot
>>>>>>>>>>>>> prompt.
>>>>>>>>>>>>>
>>>>>>>>>>>>> => setenv start_icssg2 'rproc start 14; rproc start 15; rproc
>>>>>>>>>>>>> start 16; rproc start 17; rproc start 18; rproc start 19'
>>>>>>>>>>>>> => setenv stop_icssg2 'rproc stop 14; rproc stop 15; rproc stop
>>>>>>>>>>>>> 16; rproc stop 17; rproc stop 18; rproc stop 19'
>>>>>>>>>>>>> => setenv firmware_dir '/lib/firmware/ti-pruss'
>>>>>>>>>>>>> => setenv get_firmware_mmc 'load mmc ${bootpart} ${loadaddr}
>>>>>>>>>>>>> ${firmware_dir}/${firmware_file}'
>>>>>>>>>>>>>
>>>>>>>>>>>>> => setenv init_icssg2 'setenv ethact icssg2-eth; setenv autoload
>>>>>>>>>>>>> no; rproc init; setenv loadaddr 0x80000000; \
>>>>>>>>>>>>>      setenv firmware_file am65x-sr2-pru0-prueth-fw.elf; run
>>>>>>>>>>>>> get_firmware_mmc;  rproc load 14 0x80000000 ${filesize}; \
>>>>>>>>>>>>>      setenv loadaddr 0x89000000; setenv firmware_file
>>>>>>>>>>>>> am65x-sr2-rtu0-prueth-fw.elf; run get_firmware_mmc; rproc load 15
>>>>>>>>>>>>> 0x89000000 ${filesize}; \
>>>>>>>>>>>>>      setenv loadaddr 0x90000000; setenv firmware_file
>>>>>>>>>>>>> am65x-sr2-txpru0-prueth-fw.elf; run get_firmware_mmc; rproc load
>>>>>>>>>>>>> 16 0x90000000 ${filesize}; \
>>>>>>>>>>>>>      setenv loadaddr 0x80000000; setenv firmware_file
>>>>>>>>>>>>> am65x-sr2-pru1-prueth-fw.elf; run get_firmware_mmc; rproc load 17
>>>>>>>>>>>>> 0x80000000 ${filesize}; \
>>>>>>>>>>>>>      setenv loadaddr 0x89000000; setenv firmware_file
>>>>>>>>>>>>> am65x-sr2-rtu1-prueth-fw.elf; run get_firmware_mmc; rproc load 18
>>>>>>>>>>>>> 0x89000000 ${filesize}; \
>>>>>>>>>>>>>      setenv loadaddr 0x90000000; setenv firmware_file
>>>>>>>>>>>>> am65x-sr2-txpru1-prueth-fw.elf; run get_firmware_mmc; rproc load
>>>>>>>>>>>>> 19 0x90000000 ${filesize}; \
>>>>>>>>>>>>>      run start_icssg2;'
>>>>>>>>>>>>
>>>>>>>>>>>> A whole bunch of commands are required to get ethernet functional.
>>>>>>>>>>>> This is not at all user friendly and will be a maintenance nightmare.
>>>>>>>>>>>> What worries me is tracking the 6 different rproc cores and the 6
>>>>>>>>>>>> different firmware files to start 1 ethernet device.
>>>>>>>>>>>> This will get even more interesting when we have to deal with
>>>>>>>>>>>> different ICSSG instances on different boards.
>>>>>>>>>>>>
>>>>>>>>>>>> What is preventing the driver from starting the rproc cores it
>>>>>>>>>>>> needs so user doesn't need to care about it?
>>>>>>>>>>>> All the necessary information is in the Device tree. At least this
>>>>>>>>>>>> is how it is done on Linux.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I tried removing the need for these commands and implementing them
>>>>>>>>>>> inside the driver only. I am able to load the firmware from driver
>>>>>>>>>>> using
>>>>>>>>>>> the fs_loader API request_firmware_into_buf(). It requires changes to
>>>>>>>>>>> dt. A DT node called fs-loader needs to be added also CONFIG_FS_LOADER
>>>>>>>>>>> needs to enabled. In the DT node we need to specify the storage media
>>>>>>>>>>> that we are using i.e. mmc, ospi, usb. It's upto user to modify the
>>>>>>>>>>> storage media, the driver will take the media from DT and try to laod
>>>>>>>>>>> firmware from their.
>>>>>>>>>>>
>>>>>>>>>>> For loading firmwares to rproc cores, rproc_load() API is needed. Now
>>>>>>>>>>> this API takes rproc_id, loadaddr and firmware_size as arguments.
>>>>>>>>>>> loadaddr is fixed for all three pru cores. firmware_size is obtained
>>>>>>>>>>> from request_firmware_into_buf() but I couldn't find a way to get the
>>>>>>>>>>> rproc_id. For now based on the ICSSG instance and slice number I am
>>>>>>>>>>> figuring out the rproc_id and passing it to rproc_load() and
>>>>>>>>>>> rproc_start() APIs. Please let me know if you have any other / better
>>>>>>>>>>> way of finding rproc_id.
>>>>>>>>>>>
>>>>>>>>>>> Below is the entire diff to remove these commands and move their
>>>>>>>>>>> functionality to driver. Please have a look and let me know if this
>>>>>>>>>>> is ok.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Good to see you got something working so quickly.
>>>>>>>>>> It has some rough edges but nothing that is blocking.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Save New Duplicate & Edit Just Text Twitter
>>>>>>>>>>> diff --git a/arch/arm/dts/k3-am654-base-board.dts
>>>>>>>>>>> b/arch/arm/dts/k3-am654-base-board.dts
>>>>>>>>>>> index cfbcebfa37..c8da72e49c 100644
>>>>>>>>>>> --- a/arch/arm/dts/k3-am654-base-board.dts
>>>>>>>>>>> +++ b/arch/arm/dts/k3-am654-base-board.dts
>>>>>>>>>>> @@ -16,6 +16,13 @@
>>>>>>>>>>>       chosen {
>>>>>>>>>>>           stdout-path = "serial2:115200n8";
>>>>>>>>>>>           bootargs = "earlycon=ns16550a,mmio32,0x02800000";
>>>>>>>>>>> +        firmware-loader = <&fs_loader0>;
>>>>>>>>>>> +    };
>>>>>>>>>>> +
>>>>>>>>>>> +    fs_loader0: fs-loader {
>>>>>>>>>>> +        bootph-all;
>>>>>>>>>>> +        compatible = "u-boot,fs-loader";
>>>>>>>>>>> +        phandlepart = <&sdhci1 2>;
>>>>>>>>>>>       };
>>>>>>>>>>
>>>>>>>>>> This is has 2 issues
>>>>>>>>>> 1) It will not be accepted in Kernel DT. Maybe it could be done in
>>>>>>>>>> -u-boot.dtsi file.
>>>>>>>>>
>>>>>>>>> Sure. I'll move this to k3-am654-base-board-u-boot.dtsi
>>>>>>>>>
>>>>>>>>>> 2) You cannot assume boot medium is always sdhci1 2
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Yes. It's upto user to provide the boot medium and boot partition for
>>>>>>>>> loading firmware. By default the firmware is loacated in root partion of
>>>>>>>>> shdci1 on am65x so I am adding this as default. But user can easily
>>>>>>>>> modify this to any other medium / partition needed.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Users should not have to modify DT to pick a boot medium/partition.
>>>>>>>> What would you do for complex cases or non block devices like eth/uart
>>>>>>>
>>>>>> I agree with Andrew here.
>>>>>>
>>>>>>> In order to load firmware files from driver, we need to add the node for
>>>>>>> fs-loader. The fs-loader has a phandlepart property which takes the boot
>>>>>>> medium and the partition as input. Based on the medium and partition it
>>>>>>> looks for the requested file and loads it to a given address. I am not
>>>>>>> sure if there is any other way to load firmware from driver without
>>>>>>> using the fs-loader.
>>>>>>>
>>>>>>>
>>>>>>>> booting? This is one reason kernel moved firmware loading to
>>>>>>>> userspace. The equivalant in U-Boot is the shell and scripts. Your
>>>>>>>> original command based loading was the correct solution IMHO.
>>>>>>>
>>>>>>> I intended to go ahead with command base approach only but as Roger
>>>>>>> pointed out the command based approach is not very user friendly.
>>>>>>>
>>>>>>> We need to align on what should be the correct approach for loading
>>>>>>> firmware.
>>>>>>>
>>>>>>> Roger, can you please chime in here.
>>>>>>
>>>>>> My intention was to make it user friendly so he does not have to
>>>>>> deal with 6 different Rproc IDs that can change between
>>>>>> platforms. The solution also has to be seamless between different
>>>>>> boot mediums. I think we can assume that the firmware will come from the
>>>>>> same medium that the platform was booted.
>>>>>>
>>>>>
>>>>> Yes and the with changes done by me using ICSSG port seems much easier
>>>>> and user friendly. Just doing `dhcp` at u-boot prompt is enough to use
>>>>> ICSSG port so the driver changes effectively makes the usage for user
>>>>> friendly. But only way I have found to load the firmware files is to use
>>>>> fs-loader that requires the boot medium and boot partition. I am not
>>>>> sure how can I re-use the the boot medium used for booting u-boot images.
>>>>>
>>>>> Also in SD card, u-boot images are located in /boot partition where as
>>>>> firmware is located inside /root partition. So we'll need to specify the
>>>>> partition.
>>>>>
>>>>> Currently I don't have any way to load the firmware files from driver
>>>>> without using below DT change
>>>>>
>>>>> fs_loader0: fs-loader {
>>>>>         bootph-all;
>>>>>         compatible = "u-boot,fs-loader";
>>>>>         phandlepart = <&sdhci1 2>;
>>>>> };
>>>>>
>>>>
>>>> Fromdoc/develop/driver-model/fs_firmware_loader.rst
>>>>
>>>> "Firmware loader driver is also designed to support U-Boot environment
>>>> variables, so all these data from FDT can be overwritten
>>>> through the U-Boot environment variable during run time.
>>>>
>>>> For examples:
>>>>
>>>> storage_interface:
>>>>   Storage interface, it can be "mmc", "usb", "sata" or "ubi".
>>>> fw_dev_part:
>>>>   Block device number and its partition, it can be "0:1".
>>>> fw_ubi_mtdpart:
>>>>   UBI device mtd partition, it can be "UBI".
>>>> fw_ubi_volume:
>>>>   UBI volume, it can be "ubi0".
>>>>
>>>> When above environment variables are set, environment values would be
>>>> used instead of data from FDT.
>>>> The benefit of this design allows user to change storage attribute data
>>>> at run time through U-Boot console and saving the setting as default
>>>> environment values in the storage for the next power cycle, so no
>>>> compilation is required for both driver and FDT."
>>>>
>>>> So looks like we should provide this in environment variables instead of DT?
>>>>
>>>
>>> Thanks Roger for digging this up. I tried setting the below environment
>>> values and it works.
>>>
>>> 	storage_interface=mmc
>>> 	fw_dev_part=1:2
>>>
>>> No need to add the fs-loader node in DT. We can directly set the env
>>> values and fs_loader will use them to load file. I will drop the DT
>>> change for fs-loader. I think this env approach is same as running the
>>> load command at u-boot (the initial approach). I think any medium that
>>> could be used using load command, can be used here by setting
>>> appropriate env values.
>>>
>>> Roger, Should I add the below code to include/env/ti/ti_common.env or
>>> board/ti/am65x/am65x.env so that we don't need to set these variables
>>> manually everytime we try to use ICSSG Ethernet and by default these
>>> variables will be set to mmc and 1:2. User can definately modify these
>>> at u-boot prompt and set appropriate values before running `dhcp`.
>>>
>>> diff --git a/include/env/ti/ti_common.env b/include/env/ti/ti_common.env
>>> index f0f89a2287..0861d3be34 100644
>>> --- a/include/env/ti/ti_common.env
>>> +++ b/include/env/ti/ti_common.env
>>> @@ -35,3 +35,8 @@ bootcmd_ti_mmc=
>>>         else;
>>>                 run get_kern_${boot}; run get_fdt_${boot}; run
>>> get_overlay_${boot}; run run_kern;
>>>         fi;
>>> +
>>> +#if CONFIG_TI_ICSSG_PRUETH
>>> +storage_interface=mmc
>>
>> "storage_interface" is vague. It should be updated to fw_storage_interface in fs-loader driver
>> and environment. It could fallback to storage_interface to not break existing implementations.
>>
> 
> Sure. But changing "storage_interface" to "fw_storage_interface" in  the
> fs-loader driver will also require changing the variable name in all the
> users drivers (e.g. arch/arm/mach-k3/common.c)
> 

That's why I said to fallback to "storage_interface" i.e. not drop it entirely.
FS-loader Documentation will also need to be updated.

>>> +fw_dev_part=1:2
>>> +#endif
>>>
>>
>> I think it should go in the respective <board>.env file as not all TI platforms
>> have the same boot/firmware partition.
>> so I would put it in board/ti/am65x/am65x.env
>>
> 
> Sure, I'll keep it in board/ti/am65x/am65x.env.
> 
> Andrew, Can you please confirm if you are okay with this approach. I
> will post v2 soon, once you confirm.
> 
>>>
>>>>>>>
>>>>>>>>
>>>>>>>> If we want to try to hide some of that then we need a way to
>>>>>>>> run a user provided script from the environement to handle
>>>>>>>> the general cases for firmware loading.
>>>>>>
>>>>>> Assuming we go with the "script provided in environment: route,
>>>>>> how do we deal with the Rproc IDs? I'm not sure if they are constant
>>>>>> and can probably change based on which Rproc is registered first.
>>>>>> So there needs to be some way to make sure user can reference
>>>>>> the correct Rproc.
>>>>>>
>>>>>
>>>>> The Rproc IDs aren't constant. I think the IDs depend on the sequence of
>>>>> init. The only way to know the IDs of different Rprocs is to run `rproc
>>>>> init` and then `rproc list`. Mostly on AM65x I have seen Rporc ID 0 and
>>>>> 1 to be of r5 rprocs and from 2 to 19 of differnet PRUs (Total 18, 6 per
>>>>> ICSSG instance). If r5 is not enabled, the sequence changes. Similarly
>>>>> if some other rproc is enabled sequence will change.
>>>>>
>>>>> The script should be able to read the list and determine which rproc
>>>>> needs to be loaded based on the port we want to use. I am not sure how
>>>>> to do this during u-boot.
>>>>>
>>>>
>>>> rproc list gives an output in the following format.
>>>>
>>>> => rproc list
>>>> 0 - Name:'r5f@41000000' type:'internal memory mapped' supports: load start stop reset
>>>> 1 - Name:'r5f@41400000' type:'internal memory mapped' supports: load start stop reset
>>>> 2 - Name:'r5f@5c00000' type:'internal memory mapped' supports: load start stop reset
>>>> 3 - Name:'r5f@5d00000' type:'internal memory mapped' supports: load start stop reset
>>>> 4 - Name:'r5f@5e00000' type:'internal memory mapped' supports: load start stop reset
>>>> 5 - Name:'r5f@5f00000' type:'internal memory mapped' supports: load start stop reset
>>>> 6 - Name:'dsp@4d80800000' type:'internal memory mapped' supports: load start stop reset
>>>> 7 - Name:'dsp@4d81800000' type:'internal memory mapped' supports: load start stop reset
>>>> 8 - Name:'dsp@64800000' type:'internal memory mapped' supports: load start stop reset
>>>>
>>>> How can the script know which rproc to start for a specific Ethernet interface?
>>>>
>>>
>>> I am not sure. For ICSSGx port-y, we need to start pru_x_y, rtu_x_y,
>>> tx_pru_x_y. The script could search for these core names and start the
>>> core number corresponding to these names in the result of `rproc list`.
>>>
>>> I think it will not be needed now as we can modify the envs to select
>>> different mediums, instead of hardcoding DT.
>>>
>>
>> OK.
>>
>