Re: [PATCH v18 05/20] PCI: dwc: Add outbound MSG TLPs support

[Serge Semin <fancer.lancer@gmail.com>]

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@renesas.com>
> > >
> > 
> > > 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@TYBPR01MB5341.jpnprd01.prod.outlook.com/
> 
> 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@linaro.org>
> > >
> > > - Mani
> > >
> > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> > > > ---
> > > >  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
> > > >
> > >
> > > --
> > > மணிவண்ணன் சதாசிவம்