On Mon, Jul 31, 2023 at 01:18:30AM +0000, Yoshihiro Shimoda wrote: > Hi Serge, > > > From: Serge Semin, Sent: Saturday, July 29, 2023 10:41 AM > > > > On Mon, Jul 24, 2023 at 01:42:50PM +0530, Manivannan Sadhasivam wrote: > > > On Fri, Jul 21, 2023 at 04:44:37PM +0900, Yoshihiro Shimoda wrote: > > > > Add "code" and "routing" into struct dw_pcie_ob_atu_cfg for sending > > > > MSG by iATU in the PCIe endpoint mode in near the future. > > > > > > It's better to specify the exact requirement here "triggering INTx IRQs" > > > instead of implying. > > > > > > > PCIE_ATU_INHIBIT_PAYLOAD is set to issue TLP type of Msg instead of > > > > MsgD. So, this implementation supports the data-less messages only > > > > for now. > > > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx> > > > > > > > > Same comment for patch 4/20 applies here also. With that fixed, > > > > Yoshihiro, as we greed with Mani in the PATCH 4/20 discussion please > > ignore this request. > > By the way, do you have any comment about my suggestion? [1] > > [1] > https://lore.kernel.org/linux-pci/TYBPR01MB5341407DC508F0B390B84090D801A@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ > > If you don't agree my suggestion, I'll ignore this request. Your suggested is not good for several reasons: 1. You suggest to add the function caller context-wise comments to the structure. It will cause the maintainers to keep the comments and the callers semantics in sync which is almost always gets to be diverged at some point. 2. dw_pcie_prog_outbound_atu() doesn't know whether it is called for an End-point or a Root Port controller. It just maps the CPU->PCIe spaces by means of the outbound iATU engine with the specified mapping parameters. This makes the comments you suggest misleading. Moreover depending on the application the low-level drivers or even the DW PCIe core driver may decided to map them in any way. In that case the respective change will need to update the comments too, otherwise they'll get to be wrong which gets us to the reason 1. 3. The current arguments/fields order more-or-less preserves the natural settings setup: first you specifies the entity index, then you specify the mapping settings, then you specified the mapping itself (addresses and size). Ideally the "func_no" field should be moved to the head of the structure since it also represents the mapping entity index but it will cause having the pads (so called "holes") if we didn't change it type. Anyway inverting the order so the mapping itself goes first will break that, the structure will look as if, for instance, the device-managed function taking the device pointer somewhere in the middle or at the tail of the arguments lists. The most important settings which are normally initialized first will be defined at some random place in the structure. So to speak, it's better to keep the structure fields as is for now. -Serge(y) > > Best regards, > Yoshihiro Shimoda > > > -Serge(y) > > > > > > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> > > > > > > - Mani > > > > > > > Reviewed-by: Serge Semin <fancer.lancer@xxxxxxxxx> > > > > --- > > > > drivers/pci/controller/dwc/pcie-designware.c | 9 +++++++-- > > > > drivers/pci/controller/dwc/pcie-designware.h | 4 ++++ > > > > 2 files changed, 11 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > > index 49b785509576..2d0f816fa0ab 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > > @@ -498,7 +498,7 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, > > > > dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_TARGET, > > > > upper_32_bits(atu->pci_addr)); > > > > > > > > - val = atu->type | PCIE_ATU_FUNC_NUM(atu->func_no); > > > > + val = atu->type | atu->routing | PCIE_ATU_FUNC_NUM(atu->func_no); > > > > if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) && > > > > dw_pcie_ver_is_ge(pci, 460A)) > > > > val |= PCIE_ATU_INCREASE_REGION_SIZE; > > > > @@ -506,7 +506,12 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, > > > > val = dw_pcie_enable_ecrc(val); > > > > dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL1, val); > > > > > > > > - dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE); > > > > + val = PCIE_ATU_ENABLE; > > > > + if (atu->type == PCIE_ATU_TYPE_MSG) { > > > > + /* The data-less messages only for now */ > > > > + val |= PCIE_ATU_INHIBIT_PAYLOAD | atu->code; > > > > + } > > > > + dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2, val); > > > > > > > > /* > > > > * Make sure ATU enable takes effect before any subsequent config > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > > > > index 85de0d8346fa..c626d21243b0 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-designware.h > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h > > > > @@ -147,11 +147,13 @@ > > > > #define PCIE_ATU_TYPE_IO 0x2 > > > > #define PCIE_ATU_TYPE_CFG0 0x4 > > > > #define PCIE_ATU_TYPE_CFG1 0x5 > > > > +#define PCIE_ATU_TYPE_MSG 0x10 > > > > #define PCIE_ATU_TD BIT(8) > > > > #define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20) > > > > #define PCIE_ATU_REGION_CTRL2 0x004 > > > > #define PCIE_ATU_ENABLE BIT(31) > > > > #define PCIE_ATU_BAR_MODE_ENABLE BIT(30) > > > > +#define PCIE_ATU_INHIBIT_PAYLOAD BIT(22) > > > > #define PCIE_ATU_FUNC_NUM_MATCH_EN BIT(19) > > > > #define PCIE_ATU_LOWER_BASE 0x008 > > > > #define PCIE_ATU_UPPER_BASE 0x00C > > > > @@ -292,6 +294,8 @@ struct dw_pcie_ob_atu_cfg { > > > > int index; > > > > int type; > > > > u8 func_no; > > > > + u8 code; > > > > + u8 routing; > > > > u64 cpu_addr; > > > > u64 pci_addr; > > > > u64 size; > > > > -- > > > > 2.25.1 > > > > > > > > > > -- > > > மணிவண்ணன் சதாசிவம்
