On 6/18/24 12:00, Leon Romanovsky wrote: > On Tue, Jun 18, 2024 at 07:58:55AM +0000, Omer Shpigelman wrote: >> On 6/18/24 10:08, Leon Romanovsky wrote: >>> On Tue, Jun 18, 2024 at 05:50:15AM +0000, Omer Shpigelman wrote: >>>> On 6/17/24 16:18, Leon Romanovsky wrote: >>>>> [Some people who received this message don't often get email from leon@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] >>>>> >>>>> On Thu, Jun 13, 2024 at 11:21:57AM +0300, Omer Shpigelman wrote: >>>>>> Add a common QP state machine which handles the moving for a QP from one >>>>>> state to another including performing necessary checks, draining >>>>>> in-flight transactions, invalidating caches and error reporting. >>>>>> >>>>>> Signed-off-by: Omer Shpigelman <oshpigelman@xxxxxxxxx> >>>>>> Co-developed-by: Abhilash K V <kvabhilash@xxxxxxxxx> >>>>>> Signed-off-by: Abhilash K V <kvabhilash@xxxxxxxxx> >>>>>> Co-developed-by: Andrey Agranovich <aagranovich@xxxxxxxxx> >>>>>> Signed-off-by: Andrey Agranovich <aagranovich@xxxxxxxxx> >>>>>> Co-developed-by: Bharat Jauhari <bjauhari@xxxxxxxxx> >>>>>> Signed-off-by: Bharat Jauhari <bjauhari@xxxxxxxxx> >>>>>> Co-developed-by: David Meriin <dmeriin@xxxxxxxxx> >>>>>> Signed-off-by: David Meriin <dmeriin@xxxxxxxxx> >>>>>> Co-developed-by: Sagiv Ozeri <sozeri@xxxxxxxxx> >>>>>> Signed-off-by: Sagiv Ozeri <sozeri@xxxxxxxxx> >>>>>> Co-developed-by: Zvika Yehudai <zyehudai@xxxxxxxxx> >>>>>> Signed-off-by: Zvika Yehudai <zyehudai@xxxxxxxxx> >>>>>> --- >>>>>> .../ethernet/intel/hbl_cn/common/hbl_cn_qp.c | 480 +++++++++++++++++- >>>>>> 1 file changed, 479 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/net/ethernet/intel/hbl_cn/common/hbl_cn_qp.c b/drivers/net/ethernet/intel/hbl_cn/common/hbl_cn_qp.c >>>>>> index 9ddc23bf8194..26ebdf448193 100644 >>>>>> --- a/drivers/net/ethernet/intel/hbl_cn/common/hbl_cn_qp.c >>>>>> +++ b/drivers/net/ethernet/intel/hbl_cn/common/hbl_cn_qp.c >>>>>> @@ -6,8 +6,486 @@ >>>>> >>>>> <...> >>>>> >>>>>> +/* The following table represents the (valid) operations that can be performed on >>>>>> + * a QP in order to move it from one state to another >>>>>> + * For example: a QP in RTR state can be moved to RTS state using the CN_QP_OP_RTR_2RTS >>>>>> + * operation. >>>>>> + */ >>>>>> +static const enum hbl_cn_qp_state_op qp_valid_state_op[CN_QP_NUM_STATE][CN_QP_NUM_STATE] = { >>>>>> + [CN_QP_STATE_RESET] = { >>>>>> + [CN_QP_STATE_RESET] = CN_QP_OP_2RESET, >>>>>> + [CN_QP_STATE_INIT] = CN_QP_OP_RST_2INIT, >>>>>> + [CN_QP_STATE_SQD] = CN_QP_OP_NOP, >>>>>> + [CN_QP_STATE_QPD] = CN_QP_OP_NOP, >>>>>> + }, >>>>>> + [CN_QP_STATE_INIT] = { >>>>>> + [CN_QP_STATE_RESET] = CN_QP_OP_2RESET, >>>>>> + [CN_QP_STATE_ERR] = CN_QP_OP_2ERR, >>>>>> + [CN_QP_STATE_INIT] = CN_QP_OP_NOP, >>>>>> + [CN_QP_STATE_RTR] = CN_QP_OP_INIT_2RTR, >>>>>> + [CN_QP_STATE_SQD] = CN_QP_OP_NOP, >>>>>> + [CN_QP_STATE_QPD] = CN_QP_OP_NOP, >>>>>> + }, >>>>>> + [CN_QP_STATE_RTR] = { >>>>>> + [CN_QP_STATE_RESET] = CN_QP_OP_2RESET, >>>>>> + [CN_QP_STATE_ERR] = CN_QP_OP_2ERR, >>>>>> + [CN_QP_STATE_RTR] = CN_QP_OP_RTR_2RTR, >>>>>> + [CN_QP_STATE_RTS] = CN_QP_OP_RTR_2RTS, >>>>>> + [CN_QP_STATE_SQD] = CN_QP_OP_NOP, >>>>>> + [CN_QP_STATE_QPD] = CN_QP_OP_RTR_2QPD, >>>>>> + }, >>>>>> + [CN_QP_STATE_RTS] = { >>>>>> + [CN_QP_STATE_RESET] = CN_QP_OP_2RESET, >>>>>> + [CN_QP_STATE_ERR] = CN_QP_OP_2ERR, >>>>>> + [CN_QP_STATE_RTS] = CN_QP_OP_RTS_2RTS, >>>>>> + [CN_QP_STATE_SQD] = CN_QP_OP_RTS_2SQD, >>>>>> + [CN_QP_STATE_QPD] = CN_QP_OP_RTS_2QPD, >>>>>> + [CN_QP_STATE_SQERR] = CN_QP_OP_RTS_2SQERR, >>>>>> + }, >>>>>> + [CN_QP_STATE_SQD] = { >>>>>> + [CN_QP_STATE_RESET] = CN_QP_OP_2RESET, >>>>>> + [CN_QP_STATE_ERR] = CN_QP_OP_2ERR, >>>>>> + [CN_QP_STATE_SQD] = CN_QP_OP_SQD_2SQD, >>>>>> + [CN_QP_STATE_RTS] = CN_QP_OP_SQD_2RTS, >>>>>> + [CN_QP_STATE_QPD] = CN_QP_OP_SQD_2QPD, >>>>>> + [CN_QP_STATE_SQERR] = CN_QP_OP_SQD_2SQ_ERR, >>>>>> + }, >>>>>> + [CN_QP_STATE_QPD] = { >>>>>> + [CN_QP_STATE_RESET] = CN_QP_OP_2RESET, >>>>>> + [CN_QP_STATE_ERR] = CN_QP_OP_2ERR, >>>>>> + [CN_QP_STATE_SQD] = CN_QP_OP_NOP, >>>>>> + [CN_QP_STATE_QPD] = CN_QP_OP_NOP, >>>>>> + [CN_QP_STATE_RTR] = CN_QP_OP_QPD_2RTR, >>>>>> + }, >>>>>> + [CN_QP_STATE_SQERR] = { >>>>>> + [CN_QP_STATE_RESET] = CN_QP_OP_2RESET, >>>>>> + [CN_QP_STATE_ERR] = CN_QP_OP_2ERR, >>>>>> + [CN_QP_STATE_SQD] = CN_QP_OP_SQ_ERR_2SQD, >>>>>> + [CN_QP_STATE_SQERR] = CN_QP_OP_NOP, >>>>>> + }, >>>>>> + [CN_QP_STATE_ERR] = { >>>>>> + [CN_QP_STATE_RESET] = CN_QP_OP_2RESET, >>>>>> + [CN_QP_STATE_ERR] = CN_QP_OP_2ERR, >>>>>> + } >>>>>> +}; >>>>> >>>>> I don't understand why IBTA QP state machine is declared in ETH driver >>>>> and not in IB driver. >>>>> >>>> >>>> Implementing the actual transitions between the states requires full >>>> knowledge of the HW e.g. when to flush, cache invalidation, timeouts. >>>> Our IB driver is agnostic to the ASIC type by design. Note that more ASIC >>>> generations are planned to be added and the IB driver should not be aware >>>> of these additional HWs. >>>> Hence we implemeted the QP state machine in the CN driver which is aware >>>> of the actual HW. >>> >>> Somehow ALL other IB drivers are able to implement this logic in the IB, >>> while supporting multiple ASICs. I don't see a reason why you can't do >>> the same. >>> >> >> If we are referring to this actual table, then I can move it to the IB >> driver and the CN driver will fetch the needed opcode via a function >> pointer. >> Is that ok? > > This table spotted my attention, but right separation shouldn't be limited > to only this table. The outcome of this conversation should be: > "IB specific logic should be in IB driver, and CN driver should be able to > handle only low-level operations". > > Thanks Ok, I'll check how we can move the IB specific logic to the IB driver.
