On Tue, Nov 12, 2024 at 09:14:49PM +0100, Niklas Cassel wrote: > Hello Frank, > > On Tue, Nov 12, 2024 at 12:48:16PM -0500, Frank Li wrote: > > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data, > > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a > > doorbell address space. > > > > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell > > callback handler by writing doorbell_data to the mapped doorbell_bar's > > address space. > > > > Set doorbell_done in the doorbell callback to indicate completion. > > > > To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL > > and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL > > to map one bar's inbound address to MSI space. the command > > COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping. > > > > Host side new driver Host side old driver > > > > EP: new driver S F > > EP: old driver F F > > > > S: If EP side support MSI, 'pcitest -B' return success. > > If EP side doesn't support MSI, the same to 'F'. > > > > F: 'pcitest -B' return failure, other case as usual. > > > > Tested-by: Niklas Cassel <cassel@xxxxxxxxxx> > > Signed-off-by: Frank Li <Frank.Li@xxxxxxx> > > --- > > Change from v5 to v6 > > - rename doorbell_addr to doorbell_offset > > Is there a reason why you chose to not incorporate the helper function > that I suggested here: > https://lore.kernel.org/linux-pci/ZzMtKUFi30_o6SwL@ryzen/ > > I didn't see any reply from you to that message. Oh, you said at https://lore.kernel.org/imx/ZzIVzfkZe-hkAb4G@ryzen/T/#mc10e69e0e1e20cc8d8da9a8808177407d22bce06 I think you give up your idea about helper function, because it is one for doorbell_offset, the other is for the atu address. bar's struct is difference with reg. even it is similar, one if use "addr & (align - 1)" to get offset, the other is "addr & ~algin" to get base address. > > Personally I think that it is nice to have the alignment code in a single > function, rather than duplicating the code. The helper also looks quite > similar to how we do outbound address translation alignment: > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=endpoint&id=e73ea1c2d4d8f7ba5daaf7aa51171f63cf79bcd8 > so that people will recognize the pattern more easily. Do you means add help function, which wrap epc's .align_addr()? I know you make some improvement about EP's alignment, but I have not realized that related this thread at all. > > But I guess you didn't like my suggestion? > (Which is fine, but I would have expected some motivation.) May "I now see why you did this. One function is using the db offset, and the other is using the db base." mis-lead me. If I understand correct here, I can add wrap function for epc's .align_addr(). at next version. Frank > > > Kind regards, > Niklas
