Hi Serge,
> From: Serge Semin, Sent: Tuesday, August 1, 2023 7:12 AM
>
> 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]
> >
<snip URL>
> >
> > 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.
Thank you for your reply. I understood that my suggestion is not good.
I'll keep the order as-is on v19.
Best regards,
Yoshihiro Shimoda
> -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
> > > > >
> > > >
> > > > --
> > > > மணிவண்ணன் சதாசிவம்
