| Message ID | 20250103101231.1508151-1-b-padhi@ti.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Refactor TI K3 DSP and M4 Drivers | expand |
Missed few changelog. Adding below. On 03/01/25 15:42, Beleswar Padhi wrote: > This series refactors a lot of functions & callbacks from ti_k3_dsp_remoteproc.c > and ti_k3_m4_remoteproc.c drivers. This is the third and final series as part of > the refactoring of K3 remoteproc drivers. The patches for internal refactoring > and bug fixes of TI K3 R5 remoteproc driver has been already posted[0][1]. Since > the R5 driver has worked out separate data structures and reset logic than the > DSP/M4 drivers, I have excluded R5 from this refactoring. > > NOTE: > This series is _dependent_ upon the [PATCH 2/3] of below series: > https://lore.kernel.org/all/20241224091457.1050233-3-b-padhi@ti.com/ > > Testing Done: > 1. Tested boot of C66x DSPs, C71x DSPs across Jacinto J7* devices in Remoteproc > mode and IPC-Only mode. > 2. Tested boot of M4F core _only_ in _AM62xx SK_ board in Remoteproc mode and > IPC-Only mode. > 3. Tested Core stop and detach operations from sysfs for C66x DSPs, C71x DSPs > and M4F. > 4. Tested device removal paths by executing 'modprobe -r ti_k3_dsp_remoteproc' > and 'modprobe -r ti_k3_m4_remoteproc'. > 5. Tested usecases where firmware not available at device probe time, but later > in sysfs, able to load firmware into a remotecore and start it. [C66x, C71x, M4] > 6. Tested that each patch in this series generates no new warnings/errors. > > v8: Changelog: > 1. Broken down refactoring into patches, each patch dealing with one function > for ease in review. [Andrew] 2. Introduced checks to prevent mailbox IPC with detached M4 core. 3. Refined reset/release from reset logic for DSP cores that do not have a local reset in PATCH #6 and PATCH #7 of this series. 4. Refactored additional .start()/.stop()/.attach()/.detach()/mbox .rx_callback()/request_mbox() functions in this series. Thanks, Beleswar > > Links to older versions: > v7: https://lore.kernel.org/all/20240202175538.1705-1-hnagalla@ti.com/ > v6: https://lore.kernel.org/all/20230913111644.29889-1-hnagalla@ti.com/ > v5: https://lore.kernel.org/all/20230808044529.25925-1-hnagalla@ti.com/ > v4: https://lore.kernel.org/all/20230801141117.2559-1-hnagalla@ti.com/ > v3: https://lore.kernel.org/all/20230302171450.1598576-1-martyn.welch@collabora.com/ > v2: https://lore.kernel.org/all/20230301111323.1532479-4-martyn.welch@collabora.com/ > v1: https://lore.kernel.org/all/20220110040650.18186-1-hnagalla@ti.com/ > > Thanks, > Beleswar > > [0]: https://lore.kernel.org/all/20241219110545.1898883-1-b-padhi@ti.com/ > [1]: https://lore.kernel.org/all/20241224091457.1050233-1-b-padhi@ti.com/ > > Beleswar Padhi (20): > remoteproc: k3-m4: Prevent Mailbox level IPC with detached core > remoteproc: k3: Refactor shared data structures > remoteproc: k3: Refactor mailbox rx_callback functions into common > driver > remoteproc: k3: Refactor .kick rproc ops into common driver > remoteproc: k3-m4: Use k3_rproc_mem_data structure for memory info > remoteproc: k3: Refactor rproc_reset() implementation into common > driver > remoteproc: k3: Refactor rproc_release() implementation into common > driver > remoteproc: k3: Refactor rproc_request_mbox() implementations into > common driver > remoteproc: k3: Refactor .prepare rproc ops into common driver > remoteproc: k3: Refactor .unprepare rproc ops into common driver > remoteproc: k3: Refactor .start rproc ops into common driver > remoteproc: k3: Refactor .stop rproc ops into common driver > remoteproc: k3: Refactor .attach rproc ops into common driver > remoteproc: k3: Refactor .detach rproc ops into common driver > remoteproc: k3: Refactor .get_loaded_rsc_table ops into common driver > remoteproc: k3: Refactor .da_to_va rproc ops into common driver > remoteproc: k3: Refactor of_get_memories() functions into common > driver > remoteproc: k3: Refactor mem_release() functions into common driver > remoteproc: k3: Refactor reserved_mem_init() functions into common > driver > remoteproc: k3: Refactor release_tsp() functions into common driver > > drivers/remoteproc/Makefile | 4 +- > drivers/remoteproc/ti_k3_common.c | 586 ++++++++++++++++++++ > drivers/remoteproc/ti_k3_common.h | 113 ++++ > drivers/remoteproc/ti_k3_dsp_remoteproc.c | 643 +--------------------- > drivers/remoteproc/ti_k3_m4_remoteproc.c | 583 ++------------------ > 5 files changed, 765 insertions(+), 1164 deletions(-) > create mode 100644 drivers/remoteproc/ti_k3_common.c > create mode 100644 drivers/remoteproc/ti_k3_common.h >
On 1/3/25 4:12 AM, Beleswar Padhi wrote: > This series refactors a lot of functions & callbacks from ti_k3_dsp_remoteproc.c > and ti_k3_m4_remoteproc.c drivers. This is the third and final series as part of > the refactoring of K3 remoteproc drivers. The patches for internal refactoring > and bug fixes of TI K3 R5 remoteproc driver has been already posted[0][1]. Since > the R5 driver has worked out separate data structures and reset logic than the > DSP/M4 drivers, I have excluded R5 from this refactoring. > Diffstat looks great, 765 (+), 1164 (-), good to see all that duplicated code factored away. But R5 is the largest of the 3 drivers and really needs it the most. Looking at the data structure in R5 preventing this I see what should be the normal "struct k3_rproc" structure is really split into two, "struct k3_r5_rproc" and "struct k3_r5_core". The first containing a single instance of the latter. There is no reason for this split I can see, just combine the two structs. Next, there are some members of the struct that we don't need, such as atcm_enable and the others that are only used in probe (or functions called as part of probe). We only use these as a way to collect this info in one function, and use in a later one. Instead you could either fetch this info at the time of use. Or move these members into the cluster level "struct k3_r5_cluster". Speaking of "struct k3_r5_cluster", it is silly for cluster to keep a list of cores. There are two, and will only ever be two. No clue why a list was chosen as the data structure to hold two pointers, switch this two an array of size two, or even just two pointers. This also cleans up a bunch of the weird "list_for_each" logic and loops that have to then check if they have found with core0 or core1. Instead, just directly access core0 or core1. That gets rid of member "struct list_head" from the combined struct, and would you look at that, the struct now matches DSP/M4 :) I'd suggest doing the above fixups to R5 first, then you can do this series here after that and include R5. Thanks, Andrew > NOTE: > This series is _dependent_ upon the [PATCH 2/3] of below series: > https://lore.kernel.org/all/20241224091457.1050233-3-b-padhi@ti.com/ > > Testing Done: > 1. Tested boot of C66x DSPs, C71x DSPs across Jacinto J7* devices in Remoteproc > mode and IPC-Only mode. > 2. Tested boot of M4F core _only_ in _AM62xx SK_ board in Remoteproc mode and > IPC-Only mode. > 3. Tested Core stop and detach operations from sysfs for C66x DSPs, C71x DSPs > and M4F. > 4. Tested device removal paths by executing 'modprobe -r ti_k3_dsp_remoteproc' > and 'modprobe -r ti_k3_m4_remoteproc'. > 5. Tested usecases where firmware not available at device probe time, but later > in sysfs, able to load firmware into a remotecore and start it. [C66x, C71x, M4] > 6. Tested that each patch in this series generates no new warnings/errors. > > v8: Changelog: > 1. Broken down refactoring into patches, each patch dealing with one function > for ease in review. [Andrew] > > Links to older versions: > v7: https://lore.kernel.org/all/20240202175538.1705-1-hnagalla@ti.com/ > v6: https://lore.kernel.org/all/20230913111644.29889-1-hnagalla@ti.com/ > v5: https://lore.kernel.org/all/20230808044529.25925-1-hnagalla@ti.com/ > v4: https://lore.kernel.org/all/20230801141117.2559-1-hnagalla@ti.com/ > v3: https://lore.kernel.org/all/20230302171450.1598576-1-martyn.welch@collabora.com/ > v2: https://lore.kernel.org/all/20230301111323.1532479-4-martyn.welch@collabora.com/ > v1: https://lore.kernel.org/all/20220110040650.18186-1-hnagalla@ti.com/ > > Thanks, > Beleswar > > [0]: https://lore.kernel.org/all/20241219110545.1898883-1-b-padhi@ti.com/ > [1]: https://lore.kernel.org/all/20241224091457.1050233-1-b-padhi@ti.com/ > > Beleswar Padhi (20): > remoteproc: k3-m4: Prevent Mailbox level IPC with detached core > remoteproc: k3: Refactor shared data structures > remoteproc: k3: Refactor mailbox rx_callback functions into common > driver > remoteproc: k3: Refactor .kick rproc ops into common driver > remoteproc: k3-m4: Use k3_rproc_mem_data structure for memory info > remoteproc: k3: Refactor rproc_reset() implementation into common > driver > remoteproc: k3: Refactor rproc_release() implementation into common > driver > remoteproc: k3: Refactor rproc_request_mbox() implementations into > common driver > remoteproc: k3: Refactor .prepare rproc ops into common driver > remoteproc: k3: Refactor .unprepare rproc ops into common driver > remoteproc: k3: Refactor .start rproc ops into common driver > remoteproc: k3: Refactor .stop rproc ops into common driver > remoteproc: k3: Refactor .attach rproc ops into common driver > remoteproc: k3: Refactor .detach rproc ops into common driver > remoteproc: k3: Refactor .get_loaded_rsc_table ops into common driver > remoteproc: k3: Refactor .da_to_va rproc ops into common driver > remoteproc: k3: Refactor of_get_memories() functions into common > driver > remoteproc: k3: Refactor mem_release() functions into common driver > remoteproc: k3: Refactor reserved_mem_init() functions into common > driver > remoteproc: k3: Refactor release_tsp() functions into common driver > > drivers/remoteproc/Makefile | 4 +- > drivers/remoteproc/ti_k3_common.c | 586 ++++++++++++++++++++ > drivers/remoteproc/ti_k3_common.h | 113 ++++ > drivers/remoteproc/ti_k3_dsp_remoteproc.c | 643 +--------------------- > drivers/remoteproc/ti_k3_m4_remoteproc.c | 583 ++------------------ > 5 files changed, 765 insertions(+), 1164 deletions(-) > create mode 100644 drivers/remoteproc/ti_k3_common.c > create mode 100644 drivers/remoteproc/ti_k3_common.h >
Hi Andrew, On 08/01/25 20:33, Andrew Davis wrote: > On 1/3/25 4:12 AM, Beleswar Padhi wrote: >> This series refactors a lot of functions & callbacks from >> ti_k3_dsp_remoteproc.c >> and ti_k3_m4_remoteproc.c drivers. This is the third and final series >> as part of >> the refactoring of K3 remoteproc drivers. The patches for internal >> refactoring >> and bug fixes of TI K3 R5 remoteproc driver has been already >> posted[0][1]. Since >> the R5 driver has worked out separate data structures and reset logic >> than the >> DSP/M4 drivers, I have excluded R5 from this refactoring. >> > > Diffstat looks great, 765 (+), 1164 (-), good to see all that > duplicated code > factored away. But R5 is the largest of the 3 drivers and really needs > it the > most. > > Looking at the data structure in R5 preventing this I see what should be > the normal "struct k3_rproc" structure is really split into two, > "struct k3_r5_rproc" and "struct k3_r5_core". The first containing a > single > instance of the latter. There is no reason for this split I can see, just > combine the two structs. > > Next, there are some members of the struct that we don't need, such as > atcm_enable and the others that are only used in probe (or functions > called as part of probe). We only use these as a way to collect this > info in one function, and use in a later one. Instead you could either > fetch this info at the time of use. Or move these members into the > cluster level "struct k3_r5_cluster". > > Speaking of "struct k3_r5_cluster", it is silly for cluster to keep a > list > of cores. There are two, and will only ever be two. No clue why a list > was > chosen as the data structure to hold two pointers, switch this two an > array > of size two, or even just two pointers. This also cleans up a bunch of > the > weird "list_for_each" logic and loops that have to then check if they > have > found with core0 or core1. Instead, just directly access core0 or core1. > > That gets rid of member "struct list_head" from the combined struct, > and would you look at that, the struct now matches DSP/M4 :) > > I'd suggest doing the above fixups to R5 first, then you can do > this series here after that and include R5. Thanks a lot for suggesting this detailed plan! I agree with your assessment, and I will post a series addressing this. Thanks, Beleswar > > Thanks, > Andrew > >> NOTE: >> This series is _dependent_ upon the [PATCH 2/3] of below series: >> https://lore.kernel.org/all/20241224091457.1050233-3-b-padhi@ti.com/ >> >> Testing Done: >> 1. Tested boot of C66x DSPs, C71x DSPs across Jacinto J7* devices in >> Remoteproc >> mode and IPC-Only mode. >> 2. Tested boot of M4F core _only_ in _AM62xx SK_ board in Remoteproc >> mode and >> IPC-Only mode. >> 3. Tested Core stop and detach operations from sysfs for C66x DSPs, >> C71x DSPs >> and M4F. >> 4. Tested device removal paths by executing 'modprobe -r >> ti_k3_dsp_remoteproc' >> and 'modprobe -r ti_k3_m4_remoteproc'. >> 5. Tested usecases where firmware not available at device probe time, >> but later >> in sysfs, able to load firmware into a remotecore and start it. >> [C66x, C71x, M4] >> 6. Tested that each patch in this series generates no new >> warnings/errors. >> >> v8: Changelog: >> 1. Broken down refactoring into patches, each patch dealing with one >> function >> for ease in review. [Andrew] >> >> Links to older versions: >> v7: https://lore.kernel.org/all/20240202175538.1705-1-hnagalla@ti.com/ >> v6: https://lore.kernel.org/all/20230913111644.29889-1-hnagalla@ti.com/ >> v5: https://lore.kernel.org/all/20230808044529.25925-1-hnagalla@ti.com/ >> v4: https://lore.kernel.org/all/20230801141117.2559-1-hnagalla@ti.com/ >> v3: >> https://lore.kernel.org/all/20230302171450.1598576-1-martyn.welch@collabora.com/ >> v2: >> https://lore.kernel.org/all/20230301111323.1532479-4-martyn.welch@collabora.com/ >> v1: https://lore.kernel.org/all/20220110040650.18186-1-hnagalla@ti.com/ >> >> Thanks, >> Beleswar >> >> [0]: >> https://lore.kernel.org/all/20241219110545.1898883-1-b-padhi@ti.com/ >> [1]: >> https://lore.kernel.org/all/20241224091457.1050233-1-b-padhi@ti.com/ >> >> Beleswar Padhi (20): >> remoteproc: k3-m4: Prevent Mailbox level IPC with detached core >> remoteproc: k3: Refactor shared data structures >> remoteproc: k3: Refactor mailbox rx_callback functions into common >> driver >> remoteproc: k3: Refactor .kick rproc ops into common driver >> remoteproc: k3-m4: Use k3_rproc_mem_data structure for memory info >> remoteproc: k3: Refactor rproc_reset() implementation into common >> driver >> remoteproc: k3: Refactor rproc_release() implementation into common >> driver >> remoteproc: k3: Refactor rproc_request_mbox() implementations into >> common driver >> remoteproc: k3: Refactor .prepare rproc ops into common driver >> remoteproc: k3: Refactor .unprepare rproc ops into common driver >> remoteproc: k3: Refactor .start rproc ops into common driver >> remoteproc: k3: Refactor .stop rproc ops into common driver >> remoteproc: k3: Refactor .attach rproc ops into common driver >> remoteproc: k3: Refactor .detach rproc ops into common driver >> remoteproc: k3: Refactor .get_loaded_rsc_table ops into common driver >> remoteproc: k3: Refactor .da_to_va rproc ops into common driver >> remoteproc: k3: Refactor of_get_memories() functions into common >> driver >> remoteproc: k3: Refactor mem_release() functions into common driver >> remoteproc: k3: Refactor reserved_mem_init() functions into common >> driver >> remoteproc: k3: Refactor release_tsp() functions into common driver >> >> drivers/remoteproc/Makefile | 4 +- >> drivers/remoteproc/ti_k3_common.c | 586 ++++++++++++++++++++ >> drivers/remoteproc/ti_k3_common.h | 113 ++++ >> drivers/remoteproc/ti_k3_dsp_remoteproc.c | 643 +--------------------- >> drivers/remoteproc/ti_k3_m4_remoteproc.c | 583 ++------------------ >> 5 files changed, 765 insertions(+), 1164 deletions(-) >> create mode 100644 drivers/remoteproc/ti_k3_common.c >> create mode 100644 drivers/remoteproc/ti_k3_common.h >>
This series refactors a lot of functions & callbacks from ti_k3_dsp_remoteproc.c and ti_k3_m4_remoteproc.c drivers. This is the third and final series as part of the refactoring of K3 remoteproc drivers. The patches for internal refactoring and bug fixes of TI K3 R5 remoteproc driver has been already posted[0][1]. Since the R5 driver has worked out separate data structures and reset logic than the DSP/M4 drivers, I have excluded R5 from this refactoring. NOTE: This series is _dependent_ upon the [PATCH 2/3] of below series: https://lore.kernel.org/all/20241224091457.1050233-3-b-padhi@ti.com/ Testing Done: 1. Tested boot of C66x DSPs, C71x DSPs across Jacinto J7* devices in Remoteproc mode and IPC-Only mode. 2. Tested boot of M4F core _only_ in _AM62xx SK_ board in Remoteproc mode and IPC-Only mode. 3. Tested Core stop and detach operations from sysfs for C66x DSPs, C71x DSPs and M4F. 4. Tested device removal paths by executing 'modprobe -r ti_k3_dsp_remoteproc' and 'modprobe -r ti_k3_m4_remoteproc'. 5. Tested usecases where firmware not available at device probe time, but later in sysfs, able to load firmware into a remotecore and start it. [C66x, C71x, M4] 6. Tested that each patch in this series generates no new warnings/errors. v8: Changelog: 1. Broken down refactoring into patches, each patch dealing with one function for ease in review. [Andrew] Links to older versions: v7: https://lore.kernel.org/all/20240202175538.1705-1-hnagalla@ti.com/ v6: https://lore.kernel.org/all/20230913111644.29889-1-hnagalla@ti.com/ v5: https://lore.kernel.org/all/20230808044529.25925-1-hnagalla@ti.com/ v4: https://lore.kernel.org/all/20230801141117.2559-1-hnagalla@ti.com/ v3: https://lore.kernel.org/all/20230302171450.1598576-1-martyn.welch@collabora.com/ v2: https://lore.kernel.org/all/20230301111323.1532479-4-martyn.welch@collabora.com/ v1: https://lore.kernel.org/all/20220110040650.18186-1-hnagalla@ti.com/ Thanks, Beleswar [0]: https://lore.kernel.org/all/20241219110545.1898883-1-b-padhi@ti.com/ [1]: https://lore.kernel.org/all/20241224091457.1050233-1-b-padhi@ti.com/ Beleswar Padhi (20): remoteproc: k3-m4: Prevent Mailbox level IPC with detached core remoteproc: k3: Refactor shared data structures remoteproc: k3: Refactor mailbox rx_callback functions into common driver remoteproc: k3: Refactor .kick rproc ops into common driver remoteproc: k3-m4: Use k3_rproc_mem_data structure for memory info remoteproc: k3: Refactor rproc_reset() implementation into common driver remoteproc: k3: Refactor rproc_release() implementation into common driver remoteproc: k3: Refactor rproc_request_mbox() implementations into common driver remoteproc: k3: Refactor .prepare rproc ops into common driver remoteproc: k3: Refactor .unprepare rproc ops into common driver remoteproc: k3: Refactor .start rproc ops into common driver remoteproc: k3: Refactor .stop rproc ops into common driver remoteproc: k3: Refactor .attach rproc ops into common driver remoteproc: k3: Refactor .detach rproc ops into common driver remoteproc: k3: Refactor .get_loaded_rsc_table ops into common driver remoteproc: k3: Refactor .da_to_va rproc ops into common driver remoteproc: k3: Refactor of_get_memories() functions into common driver remoteproc: k3: Refactor mem_release() functions into common driver remoteproc: k3: Refactor reserved_mem_init() functions into common driver remoteproc: k3: Refactor release_tsp() functions into common driver drivers/remoteproc/Makefile | 4 +- drivers/remoteproc/ti_k3_common.c | 586 ++++++++++++++++++++ drivers/remoteproc/ti_k3_common.h | 113 ++++ drivers/remoteproc/ti_k3_dsp_remoteproc.c | 643 +--------------------- drivers/remoteproc/ti_k3_m4_remoteproc.c | 583 ++------------------ 5 files changed, 765 insertions(+), 1164 deletions(-) create mode 100644 drivers/remoteproc/ti_k3_common.c create mode 100644 drivers/remoteproc/ti_k3_common.h