| Message ID | 20230111092547.21425-1-kvalo@kernel.org |
|---|---|
| Headers | show |
| Series | ath11k: support firmware-2.bin | expand |
Jeffrey Hugo <quic_jhugo@quicinc.com> writes: > On 1/11/2023 2:25 AM, Kalle Valo wrote: >> From: Kalle Valo <quic_kvalo@quicinc.com> >> >> Currently MHI loads the firmware image from the path provided by client >> devices. ath11k needs to support firmware image embedded along with meta data >> (named as firmware-2.bin). So allow the client driver to request the firmware >> file from user space on it's own and provide the firmware image data and size >> to MHI via a pointer struct mhi_controller::fw_data. >> >> This is an optional feature, if fw_data is NULL MHI load the firmware using the >> name from struct mhi_controller::fw_image string as before. >> >> Tested with ath11k and WCN6855 hw2.0. >> >> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> [...] >> @@ -478,14 +489,14 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl) >> */ >> if (mhi_cntrl->fbc_download) { >> ret = mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->fbc_image, >> - firmware->size); >> + fw_sz); > > Minor nit, but it seems like this could be all on one line. Will fix in v2. >> --- a/include/linux/mhi.h >> +++ b/include/linux/mhi.h >> @@ -299,6 +299,10 @@ struct mhi_controller_config { >> * @iova_start: IOMMU starting address for data (required) >> * @iova_stop: IOMMU stop address for data (required) >> * @fw_image: Firmware image name for normal booting (optional) >> + * @fw_data: Firmware image data content for normal booting, used only >> + * if fw_image is NULL (optional) > > The implementation requires fbc_download to be set, which is not a > requirement for fw_image. That is not apparent here. Ah, I had missed that. Will mention that in v2. >> @@ -384,6 +388,8 @@ struct mhi_controller { >> dma_addr_t iova_start; >> dma_addr_t iova_stop; >> const char *fw_image; >> + const u8 *fw_data; >> + size_t fw_sz; > > Did you run pahole? I remember this struct being well packed, and I > think this will add a compiler hole but I have not actually verified. I actually haven't used pahole for ages. I will run it and check how this structure is packed.
Kalle Valo <kvalo@kernel.org> writes: > Jeffrey Hugo <quic_jhugo@quicinc.com> writes: > >> On 1/11/2023 2:25 AM, Kalle Valo wrote: >>> From: Kalle Valo <quic_kvalo@quicinc.com> >>> >>> Currently MHI loads the firmware image from the path provided by client >>> devices. ath11k needs to support firmware image embedded along with meta data >>> (named as firmware-2.bin). So allow the client driver to request the firmware >>> file from user space on it's own and provide the firmware image data and size >>> to MHI via a pointer struct mhi_controller::fw_data. >>> >>> This is an optional feature, if fw_data is NULL MHI load the firmware using the >>> name from struct mhi_controller::fw_image string as before. >>> >>> Tested with ath11k and WCN6855 hw2.0. >>> >>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> [...] >> Did you run pahole? I remember this struct being well packed, and I >> think this will add a compiler hole but I have not actually verified. > > I actually haven't used pahole for ages. I will run it and check how > this structure is packed. Below is what pahole shows with GCC 8.3 on x86_64. Look pretty well packed, right? struct mhi_controller { struct device * cntrl_dev; /* 0 8 */ struct mhi_device * mhi_dev; /* 8 8 */ struct dentry * debugfs_dentry; /* 16 8 */ void * regs; /* 24 8 */ void * bhi; /* 32 8 */ void * bhie; /* 40 8 */ void * wake_db; /* 48 8 */ dma_addr_t iova_start; /* 56 8 */ /* --- cacheline 1 boundary (64 bytes) --- */ dma_addr_t iova_stop; /* 64 8 */ const char * fw_image; /* 72 8 */ const u8 * fw_data; /* 80 8 */ size_t fw_sz; /* 88 8 */ const char * edl_image; /* 96 8 */ size_t rddm_size; /* 104 8 */ size_t sbl_size; /* 112 8 */ size_t seg_len; /* 120 8 */ /* --- cacheline 2 boundary (128 bytes) --- */ size_t reg_len; /* 128 8 */ struct image_info * fbc_image; /* 136 8 */ struct image_info * rddm_image; /* 144 8 */ struct mhi_chan * mhi_chan; /* 152 8 */ struct list_head lpm_chans; /* 160 16 */ int * irq; /* 176 8 */ u32 max_chan; /* 184 4 */ u32 total_ev_rings; /* 188 4 */ /* --- cacheline 3 boundary (192 bytes) --- */ u32 hw_ev_rings; /* 192 4 */ u32 sw_ev_rings; /* 196 4 */ u32 nr_irqs; /* 200 4 */ u32 family_number; /* 204 4 */ u32 device_number; /* 208 4 */ u32 major_version; /* 212 4 */ u32 minor_version; /* 216 4 */ u32 serial_number; /* 220 4 */ u32 oem_pk_hash[16]; /* 224 64 */ /* --- cacheline 4 boundary (256 bytes) was 32 bytes ago --- */ struct mhi_event * mhi_event; /* 288 8 */ struct mhi_cmd * mhi_cmd; /* 296 8 */ struct mhi_ctxt * mhi_ctxt; /* 304 8 */ struct mutex pm_mutex; /* 312 160 */ /* --- cacheline 7 boundary (448 bytes) was 24 bytes ago --- */ rwlock_t pm_lock; /* 472 72 */ /* --- cacheline 8 boundary (512 bytes) was 32 bytes ago --- */ u32 timeout_ms; /* 544 4 */ u32 pm_state; /* 548 4 */ u32 db_access; /* 552 4 */ enum mhi_ee_type ee; /* 556 4 */ enum mhi_state dev_state; /* 560 4 */ atomic_t dev_wake; /* 564 4 */ atomic_t pending_pkts; /* 568 4 */ u32 M0; /* 572 4 */ /* --- cacheline 9 boundary (576 bytes) --- */ u32 M2; /* 576 4 */ u32 M3; /* 580 4 */ struct list_head transition_list; /* 584 16 */ spinlock_t transition_lock; /* 600 72 */ /* --- cacheline 10 boundary (640 bytes) was 32 bytes ago --- */ spinlock_t wlock; /* 672 72 */ /* --- cacheline 11 boundary (704 bytes) was 40 bytes ago --- */ struct mhi_link_info mhi_link_info; /* 744 8 */ struct work_struct st_worker; /* 752 80 */ /* --- cacheline 13 boundary (832 bytes) --- */ struct workqueue_struct * hiprio_wq; /* 832 8 */ wait_queue_head_t state_event; /* 840 88 */ /* --- cacheline 14 boundary (896 bytes) was 32 bytes ago --- */ void (*status_cb)(struct mhi_controller *, enum mhi_callback); /* 928 8 */ void (*wake_get)(struct mhi_controller *, bool); /* 936 8 */ void (*wake_put)(struct mhi_controller *, bool); /* 944 8 */ void (*wake_toggle)(struct mhi_controller *); /* 952 8 */ /* --- cacheline 15 boundary (960 bytes) --- */ int (*runtime_get)(struct mhi_controller *); /* 960 8 */ void (*runtime_put)(struct mhi_controller *); /* 968 8 */ int (*map_single)(struct mhi_controller *, struct mhi_buf_info *); /* 976 8 */ void (*unmap_single)(struct mhi_controller *, struct mhi_buf_info *); /* 984 8 */ int (*read_reg)(struct mhi_controller *, void *, u32 *); /* 992 8 */ void (*write_reg)(struct mhi_controller *, void *, u32); /* 1000 8 */ void (*reset)(struct mhi_controller *); /* 1008 8 */ size_t buffer_len; /* 1016 8 */ /* --- cacheline 16 boundary (1024 bytes) --- */ int index; /* 1024 4 */ bool bounce_buf; /* 1028 1 */ bool fbc_download; /* 1029 1 */ bool wake_set; /* 1030 1 */ /* XXX 1 byte hole, try to pack */ long unsigned int irq_flags; /* 1032 8 */ u32 mru; /* 1040 4 */ /* size: 1048, cachelines: 17, members: 73 */ /* sum members: 1043, holes: 1, sum holes: 1 */ /* padding: 4 */ /* last cacheline: 24 bytes */ };
On 3/8/2023 6:20 AM, Kalle Valo wrote: > Kalle Valo <kvalo@kernel.org> writes: > >> Jeffrey Hugo <quic_jhugo@quicinc.com> writes: >> >>> On 1/11/2023 2:25 AM, Kalle Valo wrote: >>>> From: Kalle Valo <quic_kvalo@quicinc.com> >>>> >>>> Currently MHI loads the firmware image from the path provided by client >>>> devices. ath11k needs to support firmware image embedded along with meta data >>>> (named as firmware-2.bin). So allow the client driver to request the firmware >>>> file from user space on it's own and provide the firmware image data and size >>>> to MHI via a pointer struct mhi_controller::fw_data. >>>> >>>> This is an optional feature, if fw_data is NULL MHI load the firmware using the >>>> name from struct mhi_controller::fw_image string as before. >>>> >>>> Tested with ath11k and WCN6855 hw2.0. >>>> >>>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> > > [...] > >>> Did you run pahole? I remember this struct being well packed, and I >>> think this will add a compiler hole but I have not actually verified. >> >> I actually haven't used pahole for ages. I will run it and check how >> this structure is packed. > > Below is what pahole shows with GCC 8.3 on x86_64. Look pretty well > packed, right? Yes, looks almost perfect. > > struct mhi_controller { > struct device * cntrl_dev; /* 0 8 */ > struct mhi_device * mhi_dev; /* 8 8 */ > struct dentry * debugfs_dentry; /* 16 8 */ > void * regs; /* 24 8 */ > void * bhi; /* 32 8 */ > void * bhie; /* 40 8 */ > void * wake_db; /* 48 8 */ > dma_addr_t iova_start; /* 56 8 */ > /* --- cacheline 1 boundary (64 bytes) --- */ > dma_addr_t iova_stop; /* 64 8 */ > const char * fw_image; /* 72 8 */ > const u8 * fw_data; /* 80 8 */ > size_t fw_sz; /* 88 8 */ > const char * edl_image; /* 96 8 */ > size_t rddm_size; /* 104 8 */ > size_t sbl_size; /* 112 8 */ > size_t seg_len; /* 120 8 */ > /* --- cacheline 2 boundary (128 bytes) --- */ > size_t reg_len; /* 128 8 */ > struct image_info * fbc_image; /* 136 8 */ > struct image_info * rddm_image; /* 144 8 */ > struct mhi_chan * mhi_chan; /* 152 8 */ > struct list_head lpm_chans; /* 160 16 */ > int * irq; /* 176 8 */ > u32 max_chan; /* 184 4 */ > u32 total_ev_rings; /* 188 4 */ > /* --- cacheline 3 boundary (192 bytes) --- */ > u32 hw_ev_rings; /* 192 4 */ > u32 sw_ev_rings; /* 196 4 */ > u32 nr_irqs; /* 200 4 */ > u32 family_number; /* 204 4 */ > u32 device_number; /* 208 4 */ > u32 major_version; /* 212 4 */ > u32 minor_version; /* 216 4 */ > u32 serial_number; /* 220 4 */ > u32 oem_pk_hash[16]; /* 224 64 */ > /* --- cacheline 4 boundary (256 bytes) was 32 bytes ago --- */ > struct mhi_event * mhi_event; /* 288 8 */ > struct mhi_cmd * mhi_cmd; /* 296 8 */ > struct mhi_ctxt * mhi_ctxt; /* 304 8 */ > struct mutex pm_mutex; /* 312 160 */ > /* --- cacheline 7 boundary (448 bytes) was 24 bytes ago --- */ > rwlock_t pm_lock; /* 472 72 */ > /* --- cacheline 8 boundary (512 bytes) was 32 bytes ago --- */ > u32 timeout_ms; /* 544 4 */ > u32 pm_state; /* 548 4 */ > u32 db_access; /* 552 4 */ > enum mhi_ee_type ee; /* 556 4 */ > enum mhi_state dev_state; /* 560 4 */ > atomic_t dev_wake; /* 564 4 */ > atomic_t pending_pkts; /* 568 4 */ > u32 M0; /* 572 4 */ > /* --- cacheline 9 boundary (576 bytes) --- */ > u32 M2; /* 576 4 */ > u32 M3; /* 580 4 */ > struct list_head transition_list; /* 584 16 */ > spinlock_t transition_lock; /* 600 72 */ > /* --- cacheline 10 boundary (640 bytes) was 32 bytes ago --- */ > spinlock_t wlock; /* 672 72 */ > /* --- cacheline 11 boundary (704 bytes) was 40 bytes ago --- */ > struct mhi_link_info mhi_link_info; /* 744 8 */ > struct work_struct st_worker; /* 752 80 */ > /* --- cacheline 13 boundary (832 bytes) --- */ > struct workqueue_struct * hiprio_wq; /* 832 8 */ > wait_queue_head_t state_event; /* 840 88 */ > /* --- cacheline 14 boundary (896 bytes) was 32 bytes ago --- */ > void (*status_cb)(struct mhi_controller *, enum mhi_callback); /* 928 8 */ > void (*wake_get)(struct mhi_controller *, bool); /* 936 8 */ > void (*wake_put)(struct mhi_controller *, bool); /* 944 8 */ > void (*wake_toggle)(struct mhi_controller *); /* 952 8 */ > /* --- cacheline 15 boundary (960 bytes) --- */ > int (*runtime_get)(struct mhi_controller *); /* 960 8 */ > void (*runtime_put)(struct mhi_controller *); /* 968 8 */ > int (*map_single)(struct mhi_controller *, struct mhi_buf_info *); /* 976 8 */ > void (*unmap_single)(struct mhi_controller *, struct mhi_buf_info *); /* 984 8 */ > int (*read_reg)(struct mhi_controller *, void *, u32 *); /* 992 8 */ > void (*write_reg)(struct mhi_controller *, void *, u32); /* 1000 8 */ > void (*reset)(struct mhi_controller *); /* 1008 8 */ > size_t buffer_len; /* 1016 8 */ > /* --- cacheline 16 boundary (1024 bytes) --- */ > int index; /* 1024 4 */ > bool bounce_buf; /* 1028 1 */ > bool fbc_download; /* 1029 1 */ > bool wake_set; /* 1030 1 */ > > /* XXX 1 byte hole, try to pack */ > > long unsigned int irq_flags; /* 1032 8 */ > u32 mru; /* 1040 4 */ > > /* size: 1048, cachelines: 17, members: 73 */ > /* sum members: 1043, holes: 1, sum holes: 1 */ > /* padding: 4 */ > /* last cacheline: 24 bytes */ > }; >
From: Kalle Valo <quic_kvalo@quicinc.com> We need firmware-2.bin support in ath11k so that we can add ath11k specific meta data to firmware releases, for example feature flags so that ath11k can automatically detect what features the firmware release supports. Also makes it easier and more reliable to update the firmware for PCI devices as it's not possible to mix firmware files, everything will be in one file. Please review and comment. It would be easier if I could take the MHI patch (patch 1) to my ath.git tree along with the rest of the patches but let's discuss that separately. Anilkumar Kolli (1): ath11k: add firmware-2.bin support Kalle Valo (2): mhi: allow MHI client drivers to provide the firmware via a pointer ath11k: qmi: refactor ath11k_qmi_m3_load() drivers/bus/mhi/host/boot.c | 27 ++-- drivers/net/wireless/ath/ath11k/Makefile | 3 +- drivers/net/wireless/ath/ath11k/core.c | 8 ++ drivers/net/wireless/ath/ath11k/core.h | 15 +++ drivers/net/wireless/ath/ath11k/fw.c | 157 +++++++++++++++++++++++ drivers/net/wireless/ath/ath11k/fw.h | 27 ++++ drivers/net/wireless/ath/ath11k/mhi.c | 18 ++- drivers/net/wireless/ath/ath11k/qmi.c | 54 +++++--- include/linux/mhi.h | 6 + 9 files changed, 283 insertions(+), 32 deletions(-) create mode 100644 drivers/net/wireless/ath/ath11k/fw.c create mode 100644 drivers/net/wireless/ath/ath11k/fw.h base-commit: e51980bf88c65f6ad4916baf720ad1234de01791