Hello Serge,
> From: Serge Semin, Sent: Monday, June 5, 2023 9:16 AM
>
> On Wed, May 10, 2023 at 03:22:19PM +0900, Yoshihiro Shimoda wrote:
> > Add "code" and "routing" into struct dw_pcie_outbound_atu for
>
> structure name has been changed.
Oops. I'll fix it.
> > sending MSG by iATU in the PCIe endpoint mode in near the future.
> > PCIE_ATU_INHIBIT_PAYLOAD is set to issue TLP type of Msg instead of
> > MsgD.
>
> So your implementation implies the data-less messages only. This note
> should have been added at least to the commit log. Ideally it would be
> useful to have an in-situ comment above the code setting these flags.
I got it. I'll add such explanation at both the commit log and code.
> > PCIE_ATU_HEADER_SUB_ENABLE is set to issue the translated
> > TLP header by using PCIE_ATU_{LOW,UPP}ER_TARGET registers' values.
>
> Why is that needed? Please elaborate in the patch log.
Thank you for your comment. I confirmed that this was not needed. So,
I'll drop this comment and related code.
> Other than that the change looks good.
Thank you for your review!
Best regards,
Yoshihiro Shimoda
> * I'll get back to the series review tomorrow.
>
> -Serge(y)
>
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > ---
> > drivers/pci/controller/dwc/pcie-designware.c | 7 +++++--
> > drivers/pci/controller/dwc/pcie-designware.h | 5 +++++
> > 2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index bd85a73d354b..a7c724ba7385 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,10 @@ 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)
> > + val |= PCIE_ATU_INHIBIT_PAYLOAD | PCIE_ATU_HEADER_SUB_ENABLE | 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 b8fa099bbed3..c83d1d176e62 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -150,11 +150,14 @@
> > #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_HEADER_SUB_ENABLE BIT(21)
> > #define PCIE_ATU_FUNC_NUM_MATCH_EN BIT(19)
> > #define PCIE_ATU_LOWER_BASE 0x008
> > #define PCIE_ATU_UPPER_BASE 0x00C
> > @@ -295,6 +298,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
> >
