Message ID | 20240718061344.575653-2-quic_gokulsri@quicinc.com (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
Series | add improvements to mhi driver | expand |
$SUBJECT looks wrong On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote: > When using default memory for coherent memory allocation without > reservation, memory gets fragmented after several mhi > register/unregister cycles and no coherent reservation was possible. > > Client driver registering MHI shall reserve a dedicated region as > shared-dma-pool for mhi to help avoid this situation. On boards > which doesn't reserve this memory, it will continue to allocate > memory from default memory. > > DMA pool is reserved for coherent allocations of size SZ_512K > (mhi_cntrl->seg_len) to avoid fragmentation and always ensure > allocations of SZ_512K succeeds. Allocations of lower order from the > reserved memory would lead to fragmentation on multiple alloc/frees. > So use dma_alloc_coherent from mhi_cntrl->cntrl_dev for allocations > lower than mhi_cntrl->seg_len. If coherent pool is not reserved, all > reservations go through mhi_cntrl->cntrl_dev. > > Co-developed-by: Vignesh Viswanathan <quic_viswanat@quicinc.com> > Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com> > Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com> > --- > drivers/bus/mhi/host/boot.c | 19 ++++++------ > drivers/bus/mhi/host/init.c | 51 +++++++++++++++++++++++++++++++++ > drivers/bus/mhi/host/internal.h | 26 +++++++++++++++++ > include/linux/mhi.h | 5 ++++ > 4 files changed, 91 insertions(+), 10 deletions(-) > > diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c > index dedd29ca8db3..ca842facf820 100644 > --- a/drivers/bus/mhi/host/boot.c > +++ b/drivers/bus/mhi/host/boot.c > @@ -303,8 +303,8 @@ void mhi_free_bhie_table(struct mhi_controller *mhi_cntrl, > struct mhi_buf *mhi_buf = image_info->mhi_buf; > > for (i = 0; i < image_info->entries; i++, mhi_buf++) > - dma_free_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len, > - mhi_buf->buf, mhi_buf->dma_addr); > + mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len, > + mhi_buf->buf, mhi_buf->dma_addr); > > kfree(image_info->mhi_buf); > kfree(image_info); > @@ -340,9 +340,9 @@ int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl, > vec_size = sizeof(struct bhi_vec_entry) * i; > > mhi_buf->len = vec_size; > - mhi_buf->buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, > - vec_size, &mhi_buf->dma_addr, > - GFP_KERNEL); > + mhi_buf->buf = mhi_fw_alloc_coherent(mhi_cntrl, vec_size, > + &mhi_buf->dma_addr, > + GFP_KERNEL); > if (!mhi_buf->buf) > goto error_alloc_segment; > } > @@ -355,8 +355,8 @@ int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl, > > error_alloc_segment: > for (--i, --mhi_buf; i >= 0; i--, mhi_buf--) > - dma_free_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len, > - mhi_buf->buf, mhi_buf->dma_addr); > + mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len, > + mhi_buf->buf, mhi_buf->dma_addr); > > error_alloc_mhi_buf: > kfree(img_info); > @@ -452,8 +452,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl) > fw_sz = firmware->size; > > skip_req_fw: > - buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, size, &dma_addr, > - GFP_KERNEL); > + buf = mhi_fw_alloc_coherent(mhi_cntrl, size, &dma_addr, GFP_KERNEL); > if (!buf) { > release_firmware(firmware); > goto error_fw_load; > @@ -462,7 +461,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl) > /* Download image using BHI */ > memcpy(buf, fw_data, size); > ret = mhi_fw_load_bhi(mhi_cntrl, dma_addr, size); > - dma_free_coherent(mhi_cntrl->cntrl_dev, size, buf, dma_addr); > + mhi_fw_free_coherent(mhi_cntrl, size, buf, dma_addr); > > /* Error or in EDL mode, we're done */ > if (ret) { > diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c > index ce7d2e62c2f1..c1e1412c43e2 100644 > --- a/drivers/bus/mhi/host/init.c > +++ b/drivers/bus/mhi/host/init.c > @@ -8,9 +8,12 @@ > #include <linux/debugfs.h> > #include <linux/device.h> > #include <linux/dma-direction.h> > +#include <linux/dma-map-ops.h> > #include <linux/dma-mapping.h> > #include <linux/idr.h> > #include <linux/interrupt.h> > +#include <linux/of_address.h> > +#include <linux/of_reserved_mem.h> NACK. Not all platforms that use MHI have devicetree.
On 7/18/2024 9:47 PM, Jeffrey Hugo wrote: > $SUBJECT looks wrong > > On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote: >> When using default memory for coherent memory allocation without >> reservation, memory gets fragmented after several mhi >> register/unregister cycles and no coherent reservation was possible. >> >> Client driver registering MHI shall reserve a dedicated region as >> shared-dma-pool for mhi to help avoid this situation. On boards >> which doesn't reserve this memory, it will continue to allocate >> memory from default memory. >> >> DMA pool is reserved for coherent allocations of size SZ_512K >> (mhi_cntrl->seg_len) to avoid fragmentation and always ensure >> allocations of SZ_512K succeeds. Allocations of lower order from the >> reserved memory would lead to fragmentation on multiple alloc/frees. >> So use dma_alloc_coherent from mhi_cntrl->cntrl_dev for allocations >> lower than mhi_cntrl->seg_len. If coherent pool is not reserved, all >> reservations go through mhi_cntrl->cntrl_dev. >> >> Co-developed-by: Vignesh Viswanathan <quic_viswanat@quicinc.com> >> Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com> >> Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com> >> --- >> drivers/bus/mhi/host/boot.c | 19 ++++++------ >> drivers/bus/mhi/host/init.c | 51 +++++++++++++++++++++++++++++++++ >> drivers/bus/mhi/host/internal.h | 26 +++++++++++++++++ >> include/linux/mhi.h | 5 ++++ >> 4 files changed, 91 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c >> index dedd29ca8db3..ca842facf820 100644 >> --- a/drivers/bus/mhi/host/boot.c >> +++ b/drivers/bus/mhi/host/boot.c >> @@ -303,8 +303,8 @@ void mhi_free_bhie_table(struct mhi_controller >> *mhi_cntrl, >> struct mhi_buf *mhi_buf = image_info->mhi_buf; >> for (i = 0; i < image_info->entries; i++, mhi_buf++) >> - dma_free_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len, >> - mhi_buf->buf, mhi_buf->dma_addr); >> + mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len, >> + mhi_buf->buf, mhi_buf->dma_addr); >> kfree(image_info->mhi_buf); >> kfree(image_info); >> @@ -340,9 +340,9 @@ int mhi_alloc_bhie_table(struct mhi_controller >> *mhi_cntrl, >> vec_size = sizeof(struct bhi_vec_entry) * i; >> mhi_buf->len = vec_size; >> - mhi_buf->buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, >> - vec_size, &mhi_buf->dma_addr, >> - GFP_KERNEL); >> + mhi_buf->buf = mhi_fw_alloc_coherent(mhi_cntrl, vec_size, >> + &mhi_buf->dma_addr, >> + GFP_KERNEL); >> if (!mhi_buf->buf) >> goto error_alloc_segment; >> } >> @@ -355,8 +355,8 @@ int mhi_alloc_bhie_table(struct mhi_controller >> *mhi_cntrl, >> error_alloc_segment: >> for (--i, --mhi_buf; i >= 0; i--, mhi_buf--) >> - dma_free_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len, >> - mhi_buf->buf, mhi_buf->dma_addr); >> + mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len, >> + mhi_buf->buf, mhi_buf->dma_addr); >> error_alloc_mhi_buf: >> kfree(img_info); >> @@ -452,8 +452,7 @@ void mhi_fw_load_handler(struct mhi_controller >> *mhi_cntrl) >> fw_sz = firmware->size; >> skip_req_fw: >> - buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, size, &dma_addr, >> - GFP_KERNEL); >> + buf = mhi_fw_alloc_coherent(mhi_cntrl, size, &dma_addr, >> GFP_KERNEL); >> if (!buf) { >> release_firmware(firmware); >> goto error_fw_load; >> @@ -462,7 +461,7 @@ void mhi_fw_load_handler(struct mhi_controller >> *mhi_cntrl) >> /* Download image using BHI */ >> memcpy(buf, fw_data, size); >> ret = mhi_fw_load_bhi(mhi_cntrl, dma_addr, size); >> - dma_free_coherent(mhi_cntrl->cntrl_dev, size, buf, dma_addr); >> + mhi_fw_free_coherent(mhi_cntrl, size, buf, dma_addr); >> /* Error or in EDL mode, we're done */ >> if (ret) { >> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c >> index ce7d2e62c2f1..c1e1412c43e2 100644 >> --- a/drivers/bus/mhi/host/init.c >> +++ b/drivers/bus/mhi/host/init.c >> @@ -8,9 +8,12 @@ >> #include <linux/debugfs.h> >> #include <linux/device.h> >> #include <linux/dma-direction.h> >> +#include <linux/dma-map-ops.h> >> #include <linux/dma-mapping.h> >> #include <linux/idr.h> >> #include <linux/interrupt.h> >> +#include <linux/of_address.h> >> +#include <linux/of_reserved_mem.h> > > NACK. Not all platforms that use MHI have devicetree. > Got it. Removed this patch. This can be addressed from client driver that need this feature. Regards, Gokul
diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c index dedd29ca8db3..ca842facf820 100644 --- a/drivers/bus/mhi/host/boot.c +++ b/drivers/bus/mhi/host/boot.c @@ -303,8 +303,8 @@ void mhi_free_bhie_table(struct mhi_controller *mhi_cntrl, struct mhi_buf *mhi_buf = image_info->mhi_buf; for (i = 0; i < image_info->entries; i++, mhi_buf++) - dma_free_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len, - mhi_buf->buf, mhi_buf->dma_addr); + mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len, + mhi_buf->buf, mhi_buf->dma_addr); kfree(image_info->mhi_buf); kfree(image_info); @@ -340,9 +340,9 @@ int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl, vec_size = sizeof(struct bhi_vec_entry) * i; mhi_buf->len = vec_size; - mhi_buf->buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, - vec_size, &mhi_buf->dma_addr, - GFP_KERNEL); + mhi_buf->buf = mhi_fw_alloc_coherent(mhi_cntrl, vec_size, + &mhi_buf->dma_addr, + GFP_KERNEL); if (!mhi_buf->buf) goto error_alloc_segment; } @@ -355,8 +355,8 @@ int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl, error_alloc_segment: for (--i, --mhi_buf; i >= 0; i--, mhi_buf--) - dma_free_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len, - mhi_buf->buf, mhi_buf->dma_addr); + mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len, + mhi_buf->buf, mhi_buf->dma_addr); error_alloc_mhi_buf: kfree(img_info); @@ -452,8 +452,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl) fw_sz = firmware->size; skip_req_fw: - buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, size, &dma_addr, - GFP_KERNEL); + buf = mhi_fw_alloc_coherent(mhi_cntrl, size, &dma_addr, GFP_KERNEL); if (!buf) { release_firmware(firmware); goto error_fw_load; @@ -462,7 +461,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl) /* Download image using BHI */ memcpy(buf, fw_data, size); ret = mhi_fw_load_bhi(mhi_cntrl, dma_addr, size); - dma_free_coherent(mhi_cntrl->cntrl_dev, size, buf, dma_addr); + mhi_fw_free_coherent(mhi_cntrl, size, buf, dma_addr); /* Error or in EDL mode, we're done */ if (ret) { diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c index ce7d2e62c2f1..c1e1412c43e2 100644 --- a/drivers/bus/mhi/host/init.c +++ b/drivers/bus/mhi/host/init.c @@ -8,9 +8,12 @@ #include <linux/debugfs.h> #include <linux/device.h> #include <linux/dma-direction.h> +#include <linux/dma-map-ops.h> #include <linux/dma-mapping.h> #include <linux/idr.h> #include <linux/interrupt.h> +#include <linux/of_address.h> +#include <linux/of_reserved_mem.h> #include <linux/list.h> #include <linux/mhi.h> #include <linux/mod_devicetable.h> @@ -929,6 +932,51 @@ static int parse_config(struct mhi_controller *mhi_cntrl, return ret; } +static void mhi_init_fw_coherent_memory(struct mhi_controller *mhi_cntrl, + struct mhi_device *mhi_dev) +{ + struct reserved_mem *mhi_rmem = NULL; + struct device *dev = &mhi_dev->dev; + struct device_node *cma_node = mhi_cntrl->cma_node; + int ret; + + dev->coherent_dma_mask = mhi_cntrl->cntrl_dev->coherent_dma_mask; + + if (!cma_node) { + dev_err(mhi_cntrl->cntrl_dev, "mhi coherent pool is not reserved"); + return; + } + + mhi_rmem = of_reserved_mem_lookup(cma_node); + of_node_put(cma_node); + + if (!mhi_rmem) { + dev_err(mhi_cntrl->cntrl_dev, "Failed to get DMA reserved memory"); + return; + } + + mhi_cntrl->cma_base = mhi_rmem->base; + mhi_cntrl->cma_size = mhi_rmem->size; + + ret = dma_declare_coherent_memory(dev, mhi_cntrl->cma_base, + mhi_cntrl->cma_base, + mhi_cntrl->cma_size); + if (ret) + dev_err(mhi_cntrl->cntrl_dev, "Failed to declare dma coherent memory"); + else + dev_info(mhi_cntrl->cntrl_dev, "DMA Memory initialized at %pa, size %ld MiB", + &mhi_cntrl->cma_base, + (unsigned long)mhi_cntrl->cma_size / SZ_1M); +} + +static void mhi_deinit_fw_coherent_memory(struct mhi_controller *mhi_cntrl) +{ + struct mhi_device *mhi_dev = mhi_cntrl->mhi_dev; + + dma_release_coherent_memory(&mhi_dev->dev); + mhi_dev->dev.dma_mem = NULL; +} + int mhi_register_controller(struct mhi_controller *mhi_cntrl, const struct mhi_controller_config *config) { @@ -1028,6 +1076,7 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl, goto error_setup_irq; } + mhi_init_fw_coherent_memory(mhi_cntrl, mhi_dev); mhi_dev->dev_type = MHI_DEVICE_CONTROLLER; mhi_dev->mhi_cntrl = mhi_cntrl; dev_set_name(&mhi_dev->dev, "mhi%d", mhi_cntrl->index); @@ -1053,6 +1102,7 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl, return 0; err_release_dev: + mhi_deinit_fw_coherent_memory(mhi_cntrl); put_device(&mhi_dev->dev); error_setup_irq: mhi_deinit_free_irq(mhi_cntrl); @@ -1095,6 +1145,7 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl) } vfree(mhi_cntrl->mhi_chan); + mhi_deinit_fw_coherent_memory(mhi_cntrl); device_del(&mhi_dev->dev); put_device(&mhi_dev->dev); diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h index aaad40a07f69..41ce100d87d2 100644 --- a/drivers/bus/mhi/host/internal.h +++ b/drivers/bus/mhi/host/internal.h @@ -396,6 +396,32 @@ void mhi_deinit_chan_ctxt(struct mhi_controller *mhi_cntrl, void mhi_reset_chan(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan); +static inline void *mhi_fw_alloc_coherent(struct mhi_controller *mhi_cntrl, + size_t size, dma_addr_t *dma_handle, + gfp_t gfp) +{ + if (size < mhi_cntrl->seg_len || !mhi_cntrl->cma_base) { + return dma_alloc_coherent(mhi_cntrl->cntrl_dev, + size, dma_handle, gfp); + } else { + return dma_alloc_coherent(&mhi_cntrl->mhi_dev->dev, + size, dma_handle, gfp); + } +} + +static inline void mhi_fw_free_coherent(struct mhi_controller *mhi_cntrl, + size_t size, void *vaddr, + dma_addr_t dma_handle) +{ + if (size < mhi_cntrl->seg_len || !mhi_cntrl->cma_base) { + dma_free_coherent(mhi_cntrl->cntrl_dev, size, vaddr, + dma_handle); + } else { + dma_free_coherent(&mhi_cntrl->mhi_dev->dev, size, vaddr, + dma_handle); + } +} + /* Event processing methods */ void mhi_ctrl_ev_task(unsigned long data); void mhi_ev_task(unsigned long data); diff --git a/include/linux/mhi.h b/include/linux/mhi.h index 059dc94d20bb..c788c12039b5 100644 --- a/include/linux/mhi.h +++ b/include/linux/mhi.h @@ -362,6 +362,8 @@ struct mhi_controller_config { * @wake_set: Device wakeup set flag * @irq_flags: irq flags passed to request_irq (optional) * @mru: the default MRU for the MHI device + * @cma_base: Base address of the cohernet memory pool reserved + * @cma_size: Size of the cohernel memory pool reserved * * Fields marked as (required) need to be populated by the controller driver * before calling mhi_register_controller(). For the fields marked as (optional) @@ -447,6 +449,9 @@ struct mhi_controller { bool wake_set; unsigned long irq_flags; u32 mru; + struct device_node *cma_node; + phys_addr_t cma_base; + size_t cma_size; }; /**