Messages in this thread | | | Date | Tue, 19 Jul 2022 16:01:40 -0700 | Subject | Re: [PATCH 1/2] scsi: ufs: Add Multi-Circular Queue support | From | Bart Van Assche <> |
| |
On 7/19/22 00:01, Can Guo wrote: > +/** > + * @ucdl_base_addr: UFS Command Descriptor base address > + * @sqe_base_addr: submission queue entry base address > + * @sqe_shadow_addr: submission queue entry shadow address > + * @ucdl_dma_addr: UFS Command Descriptor DMA address > + * @sqe_dma_addr: submission queue dma address > + * @cqe_base_addr: completion queue base address > + * @cqe_dma_addr: completion queue dma address > + * @lrb: array of lrb for this hardware queue > + * @max_entries: max number of slots in this hardware queue > + * @sq_tp_slot: current slot to which SQ tail pointer is pointing > + * @sq_hp_slot: current slot to which SQ head pointer is pointing > + * @cq_tp_slot: current slot to which CQ tail pointer is pointing > + * @cq_hp_slot: current slot to which CQ head pointer is pointing > + */ > +struct ufs_hw_queue { > + struct utp_transfer_cmd_desc *ucdl_base_addr; > + void *sqe_base_addr; > + struct utp_transfer_req_desc *sqe_shadow_addr; > + dma_addr_t ucdl_dma_addr; > + dma_addr_t sqe_dma_addr; > + struct cq_entry *cqe_base_addr; > + dma_addr_t cqe_dma_addr; > + struct ufshcd_lrb *lrb; > + u32 max_entries; > + u32 id; > + > + void __iomem *mcq_sq_hp; > + void __iomem *mcq_sq_tp; > + void __iomem *mcq_cq_hp; > + void __iomem *mcq_cq_tp; > + > + spinlock_t sq_lock; > + u32 sq_tp_slot; > + u32 sq_hp_slot; > + spinlock_t cq_lock; > + u32 cq_tp_slot; > + u32 cq_hp_slot; > }; >
Please move all new data structures into a private header that can be moved into a private header. I think the above data structure can be moved from a public into a private header (a header that is not shared with the host drivers).
> +#define for_each_hw_queue(hba, i) \ > + for ((i) = 0; (i) < (hba)->nr_hw_queues; (i) ++)
A macro like the above reduces code readability. Please remove this macro definition.
> +static inline bool is_mcq_enabled(struct ufs_hba *hba) > +{ > + return hba->use_mcq; > +} > + > +static inline bool is_mcq_supported(struct ufs_hba *hba) > +{ > + return hba->mcq_sup; > +}
The names of the two above functions are longer than their implementation so it's probably better to remove these function definitions.
> -#define ufshcd_writel(hba, val, reg) \ > +#define ufshcd_writel(hba, val, reg) \ > writel((val), (hba)->mmio_base + (reg))
Is this a whitespace-only change? If so, should that change be in this patch?
Thanks,
Bart.
| |