[PATCH 1/1] PCI: iproc: Invalidate PAXB address mapping before programming it

[PATCH 1/1] PCI: iproc: Invalidate PAXB address mapping before programming it

[Abhishek Shah]

Invalidate PAXB inbound/outbound address mapping each time before
programming it. This is helpful for the cases where we need to
reprogram inbound/outbound address mapping without resetting PAXB.
kexec kernel is one such example.

Signed-off-by: Abhishek Shah <abhishek.shah@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Vikram Mysore Prakash <vikram.prakash@broadcom.com>
---
 drivers/pci/controller/pcie-iproc.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
index e3ca46497470..99a9521ba7ab 100644
--- a/drivers/pci/controller/pcie-iproc.c
+++ b/drivers/pci/controller/pcie-iproc.c
@@ -1245,6 +1245,32 @@ static int iproc_pcie_map_dma_ranges(struct iproc_pcie *pcie)
 	return ret;
 }
 
+static void iproc_pcie_invalidate_mapping(struct iproc_pcie *pcie)
+{
+	struct iproc_pcie_ib *ib = &pcie->ib;
+	struct iproc_pcie_ob *ob = &pcie->ob;
+	int idx;
+
+	if (pcie->ep_is_internal)
+		return;
+
+	if (pcie->need_ob_cfg) {
+		/* iterate through all OARR mapping regions */
+		for (idx = ob->nr_windows - 1; idx >= 0; idx--) {
+			iproc_pcie_write_reg(pcie,
+					     MAP_REG(IPROC_PCIE_OARR0, idx), 0);
+		}
+	}
+
+	if (pcie->need_ib_cfg) {
+		/* iterate through all IARR mapping regions */
+		for (idx = 0; idx < ib->nr_regions; idx++) {
+			iproc_pcie_write_reg(pcie,
+					     MAP_REG(IPROC_PCIE_IARR0, idx), 0);
+		}
+	}
+}
+
 static int iproce_pcie_get_msi(struct iproc_pcie *pcie,
 			       struct device_node *msi_node,
 			       u64 *msi_addr)
@@ -1517,6 +1543,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 	iproc_pcie_perst_ctrl(pcie, true);
 	iproc_pcie_perst_ctrl(pcie, false);
 
+	iproc_pcie_invalidate_mapping(pcie);
+
 	if (pcie->need_ob_cfg) {
 		ret = iproc_pcie_map_ranges(pcie, res);
 		if (ret) {
-- 
2.17.1

Re: [PATCH 1/1] PCI: iproc: Invalidate PAXB address mapping before programming it

[Andrew Murray]

On Fri, Sep 06, 2019 at 09:28:13AM +0530, Abhishek Shah wrote:
> Invalidate PAXB inbound/outbound address mapping each time before
> programming it. This is helpful for the cases where we need to
> reprogram inbound/outbound address mapping without resetting PAXB.
> kexec kernel is one such example.

Why is this approach better than resetting the PAXB (I assume that's
the PCI controller IP)? Wouldn't resetting the PAXB address this issue,
and ensure that no other configuration is left behind?

Or is this related to earlier boot stages loading firmware for the emulated
downstream endpoints (ep_is_internal)?

Finally, in the case where ep_is_internal do you need to disable anything
prior to invalidating the mappings?

> 
> Signed-off-by: Abhishek Shah <abhishek.shah@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Vikram Mysore Prakash <vikram.prakash@broadcom.com>
> ---
>  drivers/pci/controller/pcie-iproc.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> index e3ca46497470..99a9521ba7ab 100644
> --- a/drivers/pci/controller/pcie-iproc.c
> +++ b/drivers/pci/controller/pcie-iproc.c
> @@ -1245,6 +1245,32 @@ static int iproc_pcie_map_dma_ranges(struct iproc_pcie *pcie)
>  	return ret;
>  }
>  
> +static void iproc_pcie_invalidate_mapping(struct iproc_pcie *pcie)
> +{
> +	struct iproc_pcie_ib *ib = &pcie->ib;
> +	struct iproc_pcie_ob *ob = &pcie->ob;
> +	int idx;
> +
> +	if (pcie->ep_is_internal)
> +		return;
> +
> +	if (pcie->need_ob_cfg) {
> +		/* iterate through all OARR mapping regions */
> +		for (idx = ob->nr_windows - 1; idx >= 0; idx--) {
> +			iproc_pcie_write_reg(pcie,
> +					     MAP_REG(IPROC_PCIE_OARR0, idx), 0);
> +		}
> +	}
> +
> +	if (pcie->need_ib_cfg) {
> +		/* iterate through all IARR mapping regions */
> +		for (idx = 0; idx < ib->nr_regions; idx++) {
> +			iproc_pcie_write_reg(pcie,
> +					     MAP_REG(IPROC_PCIE_IARR0, idx), 0);
> +		}
> +	}
> +}
> +
>  static int iproce_pcie_get_msi(struct iproc_pcie *pcie,
>  			       struct device_node *msi_node,
>  			       u64 *msi_addr)
> @@ -1517,6 +1543,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  	iproc_pcie_perst_ctrl(pcie, true);
>  	iproc_pcie_perst_ctrl(pcie, false);
>  
> +	iproc_pcie_invalidate_mapping(pcie);
> +
>  	if (pcie->need_ob_cfg) {
>  		ret = iproc_pcie_map_ranges(pcie, res);
>  		if (ret) {

The code changes look good to me.

Thanks,

Andrew Murray

> -- 
> 2.17.1
> 

Re: [PATCH 1/1] PCI: iproc: Invalidate PAXB address mapping before programming it

[Abhishek Shah]

Hi Andrew,

Thanks for the review. Please see my response inline:

On Fri, Sep 6, 2019 at 2:08 PM Andrew Murray <andrew.murray@arm.com> wrote:
>
> On Fri, Sep 06, 2019 at 09:28:13AM +0530, Abhishek Shah wrote:
> > Invalidate PAXB inbound/outbound address mapping each time before
> > programming it. This is helpful for the cases where we need to
> > reprogram inbound/outbound address mapping without resetting PAXB.
> > kexec kernel is one such example.
>
> Why is this approach better than resetting the PAXB (I assume that's
> the PCI controller IP)? Wouldn't resetting the PAXB address this issue,
> and ensure that no other configuration is left behind?
>
We normally reset PAXB in the firmware(ATF). But for cases like kexec
kernel boot,
we do not execute any firmware code and directly boot into kernel.

We could have done PAXB reset in the driver itself as you have suggested here.
But note that this detail could vary for each SoC, because these
registers are not part
of PAXB register space itself, rather exists in a register space responsible for
controlling power to various wrappers in PCIe IP. Normally, this kind
of SoC specific
details are handled in firmware itself, we don't bring them to driver level.

> Or is this related to earlier boot stages loading firmware for the emulated
> downstream endpoints (ep_is_internal)?
>
> Finally, in the case where ep_is_internal do you need to disable anything
> prior to invalidating the mappings?
>
No, ep_is_internal  is indicator for PAXC IP. It does not have
mappings as in PAXB.


Regards,
Abhishek
> >
> > Signed-off-by: Abhishek Shah <abhishek.shah@broadcom.com>
> > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > Reviewed-by: Vikram Mysore Prakash <vikram.prakash@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-iproc.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> > index e3ca46497470..99a9521ba7ab 100644
> > --- a/drivers/pci/controller/pcie-iproc.c
> > +++ b/drivers/pci/controller/pcie-iproc.c
> > @@ -1245,6 +1245,32 @@ static int iproc_pcie_map_dma_ranges(struct iproc_pcie *pcie)
> >       return ret;
> >  }
> >
> > +static void iproc_pcie_invalidate_mapping(struct iproc_pcie *pcie)
> > +{
> > +     struct iproc_pcie_ib *ib = &pcie->ib;
> > +     struct iproc_pcie_ob *ob = &pcie->ob;
> > +     int idx;
> > +
> > +     if (pcie->ep_is_internal)
> > +             return;
> > +
> > +     if (pcie->need_ob_cfg) {
> > +             /* iterate through all OARR mapping regions */
> > +             for (idx = ob->nr_windows - 1; idx >= 0; idx--) {
> > +                     iproc_pcie_write_reg(pcie,
> > +                                          MAP_REG(IPROC_PCIE_OARR0, idx), 0);
> > +             }
> > +     }
> > +
> > +     if (pcie->need_ib_cfg) {
> > +             /* iterate through all IARR mapping regions */
> > +             for (idx = 0; idx < ib->nr_regions; idx++) {
> > +                     iproc_pcie_write_reg(pcie,
> > +                                          MAP_REG(IPROC_PCIE_IARR0, idx), 0);
> > +             }
> > +     }
> > +}
> > +
> >  static int iproce_pcie_get_msi(struct iproc_pcie *pcie,
> >                              struct device_node *msi_node,
> >                              u64 *msi_addr)
> > @@ -1517,6 +1543,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
> >       iproc_pcie_perst_ctrl(pcie, true);
> >       iproc_pcie_perst_ctrl(pcie, false);
> >
> > +     iproc_pcie_invalidate_mapping(pcie);
> > +
> >       if (pcie->need_ob_cfg) {
> >               ret = iproc_pcie_map_ranges(pcie, res);
> >               if (ret) {
>
> The code changes look good to me.
>
> Thanks,
>
> Andrew Murray
>
> > --
> > 2.17.1
> >

Re: [PATCH 1/1] PCI: iproc: Invalidate PAXB address mapping before programming it

[Andrew Murray]

On Fri, Sep 06, 2019 at 02:55:19PM +0530, Abhishek Shah wrote:
> Hi Andrew,
> 
> Thanks for the review. Please see my response inline:
> 
> On Fri, Sep 6, 2019 at 2:08 PM Andrew Murray <andrew.murray@arm.com> wrote:
> >
> > On Fri, Sep 06, 2019 at 09:28:13AM +0530, Abhishek Shah wrote:
> > > Invalidate PAXB inbound/outbound address mapping each time before
> > > programming it. This is helpful for the cases where we need to
> > > reprogram inbound/outbound address mapping without resetting PAXB.
> > > kexec kernel is one such example.
> >
> > Why is this approach better than resetting the PAXB (I assume that's
> > the PCI controller IP)? Wouldn't resetting the PAXB address this issue,
> > and ensure that no other configuration is left behind?
> >
> We normally reset PAXB in the firmware(ATF). But for cases like kexec
> kernel boot,
> we do not execute any firmware code and directly boot into kernel.
> 
> We could have done PAXB reset in the driver itself as you have suggested here.
> But note that this detail could vary for each SoC, because these
> registers are not part
> of PAXB register space itself, rather exists in a register space responsible for
> controlling power to various wrappers in PCIe IP. Normally, this kind
> of SoC specific
> details are handled in firmware itself, we don't bring them to driver level.

OK understood.

> 
> > Or is this related to earlier boot stages loading firmware for the emulated
> > downstream endpoints (ep_is_internal)?
> >
> > Finally, in the case where ep_is_internal do you need to disable anything
> > prior to invalidating the mappings?
> >
> No, ep_is_internal  is indicator for PAXC IP. It does not have
> mappings as in PAXB.

I think I meant !ep_is_internal. I.e. is there possibility of inbound traffic
prior to invalidating the mappings. I'd assume not, but that's an assumption.

Either way:

Reviewed-by: Andrew Murray <andrew.murray@arm.com>

> 
> 
> Regards,
> Abhishek
> > >
> > > Signed-off-by: Abhishek Shah <abhishek.shah@broadcom.com>
> > > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > > Reviewed-by: Vikram Mysore Prakash <vikram.prakash@broadcom.com>
> > > ---
> > >  drivers/pci/controller/pcie-iproc.c | 28 ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> > > index e3ca46497470..99a9521ba7ab 100644
> > > --- a/drivers/pci/controller/pcie-iproc.c
> > > +++ b/drivers/pci/controller/pcie-iproc.c
> > > @@ -1245,6 +1245,32 @@ static int iproc_pcie_map_dma_ranges(struct iproc_pcie *pcie)
> > >       return ret;
> > >  }
> > >
> > > +static void iproc_pcie_invalidate_mapping(struct iproc_pcie *pcie)
> > > +{
> > > +     struct iproc_pcie_ib *ib = &pcie->ib;
> > > +     struct iproc_pcie_ob *ob = &pcie->ob;
> > > +     int idx;
> > > +
> > > +     if (pcie->ep_is_internal)
> > > +             return;
> > > +
> > > +     if (pcie->need_ob_cfg) {
> > > +             /* iterate through all OARR mapping regions */
> > > +             for (idx = ob->nr_windows - 1; idx >= 0; idx--) {
> > > +                     iproc_pcie_write_reg(pcie,
> > > +                                          MAP_REG(IPROC_PCIE_OARR0, idx), 0);
> > > +             }
> > > +     }
> > > +
> > > +     if (pcie->need_ib_cfg) {
> > > +             /* iterate through all IARR mapping regions */
> > > +             for (idx = 0; idx < ib->nr_regions; idx++) {
> > > +                     iproc_pcie_write_reg(pcie,
> > > +                                          MAP_REG(IPROC_PCIE_IARR0, idx), 0);
> > > +             }
> > > +     }
> > > +}
> > > +
> > >  static int iproce_pcie_get_msi(struct iproc_pcie *pcie,
> > >                              struct device_node *msi_node,
> > >                              u64 *msi_addr)
> > > @@ -1517,6 +1543,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
> > >       iproc_pcie_perst_ctrl(pcie, true);
> > >       iproc_pcie_perst_ctrl(pcie, false);
> > >
> > > +     iproc_pcie_invalidate_mapping(pcie);
> > > +
> > >       if (pcie->need_ob_cfg) {
> > >               ret = iproc_pcie_map_ranges(pcie, res);
> > >               if (ret) {
> >
> > The code changes look good to me.
> >
> > Thanks,
> >
> > Andrew Murray
> >
> > > --
> > > 2.17.1
> > >

Re: [PATCH 1/1] PCI: iproc: Invalidate PAXB address mapping before programming it

[Abhishek Shah]

Hi Andrew,


On Fri, Sep 6, 2019 at 3:31 PM Andrew Murray <andrew.murray@arm.com> wrote:
>
> On Fri, Sep 06, 2019 at 02:55:19PM +0530, Abhishek Shah wrote:
> > Hi Andrew,
> >
> > Thanks for the review. Please see my response inline:
> >
> > On Fri, Sep 6, 2019 at 2:08 PM Andrew Murray <andrew.murray@arm.com> wrote:
> > >
> > > On Fri, Sep 06, 2019 at 09:28:13AM +0530, Abhishek Shah wrote:
> > > > Invalidate PAXB inbound/outbound address mapping each time before
> > > > programming it. This is helpful for the cases where we need to
> > > > reprogram inbound/outbound address mapping without resetting PAXB.
> > > > kexec kernel is one such example.
> > >
> > > Why is this approach better than resetting the PAXB (I assume that's
> > > the PCI controller IP)? Wouldn't resetting the PAXB address this issue,
> > > and ensure that no other configuration is left behind?
> > >
> > We normally reset PAXB in the firmware(ATF). But for cases like kexec
> > kernel boot,
> > we do not execute any firmware code and directly boot into kernel.
> >
> > We could have done PAXB reset in the driver itself as you have suggested here.
> > But note that this detail could vary for each SoC, because these
> > registers are not part
> > of PAXB register space itself, rather exists in a register space responsible for
> > controlling power to various wrappers in PCIe IP. Normally, this kind
> > of SoC specific
> > details are handled in firmware itself, we don't bring them to driver level.
>
> OK understood.
>
> >
> > > Or is this related to earlier boot stages loading firmware for the emulated
> > > downstream endpoints (ep_is_internal)?
> > >
> > > Finally, in the case where ep_is_internal do you need to disable anything
> > > prior to invalidating the mappings?
> > >
> > No, ep_is_internal  is indicator for PAXC IP. It does not have
> > mappings as in PAXB.
>
> I think I meant !ep_is_internal. I.e. is there possibility of inbound traffic
> prior to invalidating the mappings. I'd assume not, but that's an assumption.
>
No, EP devices are not even enumerated yet.

> Either way:
>
> Reviewed-by: Andrew Murray <andrew.murray@arm.com>
>
> >
> >
> > Regards,
> > Abhishek
> > > >
> > > > Signed-off-by: Abhishek Shah <abhishek.shah@broadcom.com>
> > > > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > > > Reviewed-by: Vikram Mysore Prakash <vikram.prakash@broadcom.com>
> > > > ---
> > > >  drivers/pci/controller/pcie-iproc.c | 28 ++++++++++++++++++++++++++++
> > > >  1 file changed, 28 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> > > > index e3ca46497470..99a9521ba7ab 100644
> > > > --- a/drivers/pci/controller/pcie-iproc.c
> > > > +++ b/drivers/pci/controller/pcie-iproc.c
> > > > @@ -1245,6 +1245,32 @@ static int iproc_pcie_map_dma_ranges(struct iproc_pcie *pcie)
> > > >       return ret;
> > > >  }
> > > >
> > > > +static void iproc_pcie_invalidate_mapping(struct iproc_pcie *pcie)
> > > > +{
> > > > +     struct iproc_pcie_ib *ib = &pcie->ib;
> > > > +     struct iproc_pcie_ob *ob = &pcie->ob;
> > > > +     int idx;
> > > > +
> > > > +     if (pcie->ep_is_internal)
> > > > +             return;
> > > > +
> > > > +     if (pcie->need_ob_cfg) {
> > > > +             /* iterate through all OARR mapping regions */
> > > > +             for (idx = ob->nr_windows - 1; idx >= 0; idx--) {
> > > > +                     iproc_pcie_write_reg(pcie,
> > > > +                                          MAP_REG(IPROC_PCIE_OARR0, idx), 0);
> > > > +             }
> > > > +     }
> > > > +
> > > > +     if (pcie->need_ib_cfg) {
> > > > +             /* iterate through all IARR mapping regions */
> > > > +             for (idx = 0; idx < ib->nr_regions; idx++) {
> > > > +                     iproc_pcie_write_reg(pcie,
> > > > +                                          MAP_REG(IPROC_PCIE_IARR0, idx), 0);
> > > > +             }
> > > > +     }
> > > > +}
> > > > +
> > > >  static int iproce_pcie_get_msi(struct iproc_pcie *pcie,
> > > >                              struct device_node *msi_node,
> > > >                              u64 *msi_addr)
> > > > @@ -1517,6 +1543,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
> > > >       iproc_pcie_perst_ctrl(pcie, true);
> > > >       iproc_pcie_perst_ctrl(pcie, false);
> > > >
> > > > +     iproc_pcie_invalidate_mapping(pcie);
> > > > +
> > > >       if (pcie->need_ob_cfg) {
> > > >               ret = iproc_pcie_map_ranges(pcie, res);
> > > >               if (ret) {
> > >
> > > The code changes look good to me.
> > >
> > > Thanks,
> > >
> > > Andrew Murray
> > >
> > > > --
> > > > 2.17.1
> > > >

Re: [PATCH 1/1] PCI: iproc: Invalidate PAXB address mapping before programming it

[Abhishek Shah]

Hi Bjorn/Lorenzo,

Can you please help review this patch?


Regards,
Abhishek


On Fri, Sep 6, 2019 at 7:41 PM Abhishek Shah <abhishek.shah@broadcom.com> wrote:
>
> Hi Andrew,
>
>
> On Fri, Sep 6, 2019 at 3:31 PM Andrew Murray <andrew.murray@arm.com> wrote:
> >
> > On Fri, Sep 06, 2019 at 02:55:19PM +0530, Abhishek Shah wrote:
> > > Hi Andrew,
> > >
> > > Thanks for the review. Please see my response inline:
> > >
> > > On Fri, Sep 6, 2019 at 2:08 PM Andrew Murray <andrew.murray@arm.com> wrote:
> > > >
> > > > On Fri, Sep 06, 2019 at 09:28:13AM +0530, Abhishek Shah wrote:
> > > > > Invalidate PAXB inbound/outbound address mapping each time before
> > > > > programming it. This is helpful for the cases where we need to
> > > > > reprogram inbound/outbound address mapping without resetting PAXB.
> > > > > kexec kernel is one such example.
> > > >
> > > > Why is this approach better than resetting the PAXB (I assume that's
> > > > the PCI controller IP)? Wouldn't resetting the PAXB address this issue,
> > > > and ensure that no other configuration is left behind?
> > > >
> > > We normally reset PAXB in the firmware(ATF). But for cases like kexec
> > > kernel boot,
> > > we do not execute any firmware code and directly boot into kernel.
> > >
> > > We could have done PAXB reset in the driver itself as you have suggested here.
> > > But note that this detail could vary for each SoC, because these
> > > registers are not part
> > > of PAXB register space itself, rather exists in a register space responsible for
> > > controlling power to various wrappers in PCIe IP. Normally, this kind
> > > of SoC specific
> > > details are handled in firmware itself, we don't bring them to driver level.
> >
> > OK understood.
> >
> > >
> > > > Or is this related to earlier boot stages loading firmware for the emulated
> > > > downstream endpoints (ep_is_internal)?
> > > >
> > > > Finally, in the case where ep_is_internal do you need to disable anything
> > > > prior to invalidating the mappings?
> > > >
> > > No, ep_is_internal  is indicator for PAXC IP. It does not have
> > > mappings as in PAXB.
> >
> > I think I meant !ep_is_internal. I.e. is there possibility of inbound traffic
> > prior to invalidating the mappings. I'd assume not, but that's an assumption.
> >
> No, EP devices are not even enumerated yet.
>
> > Either way:
> >
> > Reviewed-by: Andrew Murray <andrew.murray@arm.com>
> >
> > >
> > >
> > > Regards,
> > > Abhishek
> > > > >
> > > > > Signed-off-by: Abhishek Shah <abhishek.shah@broadcom.com>
> > > > > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > > > > Reviewed-by: Vikram Mysore Prakash <vikram.prakash@broadcom.com>
> > > > > ---
> > > > >  drivers/pci/controller/pcie-iproc.c | 28 ++++++++++++++++++++++++++++
> > > > >  1 file changed, 28 insertions(+)
> > > > >
> > > > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> > > > > index e3ca46497470..99a9521ba7ab 100644
> > > > > --- a/drivers/pci/controller/pcie-iproc.c
> > > > > +++ b/drivers/pci/controller/pcie-iproc.c
> > > > > @@ -1245,6 +1245,32 @@ static int iproc_pcie_map_dma_ranges(struct iproc_pcie *pcie)
> > > > >       return ret;
> > > > >  }
> > > > >
> > > > > +static void iproc_pcie_invalidate_mapping(struct iproc_pcie *pcie)
> > > > > +{
> > > > > +     struct iproc_pcie_ib *ib = &pcie->ib;
> > > > > +     struct iproc_pcie_ob *ob = &pcie->ob;
> > > > > +     int idx;
> > > > > +
> > > > > +     if (pcie->ep_is_internal)
> > > > > +             return;
> > > > > +
> > > > > +     if (pcie->need_ob_cfg) {
> > > > > +             /* iterate through all OARR mapping regions */
> > > > > +             for (idx = ob->nr_windows - 1; idx >= 0; idx--) {
> > > > > +                     iproc_pcie_write_reg(pcie,
> > > > > +                                          MAP_REG(IPROC_PCIE_OARR0, idx), 0);
> > > > > +             }
> > > > > +     }
> > > > > +
> > > > > +     if (pcie->need_ib_cfg) {
> > > > > +             /* iterate through all IARR mapping regions */
> > > > > +             for (idx = 0; idx < ib->nr_regions; idx++) {
> > > > > +                     iproc_pcie_write_reg(pcie,
> > > > > +                                          MAP_REG(IPROC_PCIE_IARR0, idx), 0);
> > > > > +             }
> > > > > +     }
> > > > > +}
> > > > > +
> > > > >  static int iproce_pcie_get_msi(struct iproc_pcie *pcie,
> > > > >                              struct device_node *msi_node,
> > > > >                              u64 *msi_addr)
> > > > > @@ -1517,6 +1543,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
> > > > >       iproc_pcie_perst_ctrl(pcie, true);
> > > > >       iproc_pcie_perst_ctrl(pcie, false);
> > > > >
> > > > > +     iproc_pcie_invalidate_mapping(pcie);
> > > > > +
> > > > >       if (pcie->need_ob_cfg) {
> > > > >               ret = iproc_pcie_map_ranges(pcie, res);
> > > > >               if (ret) {
> > > >
> > > > The code changes look good to me.
> > > >
> > > > Thanks,
> > > >
> > > > Andrew Murray
> > > >
> > > > > --
> > > > > 2.17.1
> > > > >

Re: [PATCH 1/1] PCI: iproc: Invalidate PAXB address mapping before programming it

[Lorenzo Pieralisi]

On Fri, Sep 06, 2019 at 09:28:13AM +0530, Abhishek Shah wrote:
> Invalidate PAXB inbound/outbound address mapping each time before
> programming it. This is helpful for the cases where we need to
> reprogram inbound/outbound address mapping without resetting PAXB.
> kexec kernel is one such example.

This looks like a hack, explain to us please what it actually solves and
why a full reset is not necessary.

> Signed-off-by: Abhishek Shah <abhishek.shah@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Vikram Mysore Prakash <vikram.prakash@broadcom.com>

Patches are reviewed on public mailing lists, remove tags given
on internal reviews - they are not relevant.

> ---
>  drivers/pci/controller/pcie-iproc.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> index e3ca46497470..99a9521ba7ab 100644
> --- a/drivers/pci/controller/pcie-iproc.c
> +++ b/drivers/pci/controller/pcie-iproc.c
> @@ -1245,6 +1245,32 @@ static int iproc_pcie_map_dma_ranges(struct iproc_pcie *pcie)
>  	return ret;
>  }
>  
> +static void iproc_pcie_invalidate_mapping(struct iproc_pcie *pcie)
> +{
> +	struct iproc_pcie_ib *ib = &pcie->ib;
> +	struct iproc_pcie_ob *ob = &pcie->ob;
> +	int idx;
> +
> +	if (pcie->ep_is_internal)

What's this check for and why leaving mappings in place is safe for
this category of IPs ?

> +		return;
> +
> +	if (pcie->need_ob_cfg) {
> +		/* iterate through all OARR mapping regions */
> +		for (idx = ob->nr_windows - 1; idx >= 0; idx--) {
> +			iproc_pcie_write_reg(pcie,
> +					     MAP_REG(IPROC_PCIE_OARR0, idx), 0);
> +		}
> +	}
> +
> +	if (pcie->need_ib_cfg) {
> +		/* iterate through all IARR mapping regions */
> +		for (idx = 0; idx < ib->nr_regions; idx++) {
> +			iproc_pcie_write_reg(pcie,
> +					     MAP_REG(IPROC_PCIE_IARR0, idx), 0);
> +		}
> +	}
> +}
> +
>  static int iproce_pcie_get_msi(struct iproc_pcie *pcie,
>  			       struct device_node *msi_node,
>  			       u64 *msi_addr)
> @@ -1517,6 +1543,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  	iproc_pcie_perst_ctrl(pcie, true);
>  	iproc_pcie_perst_ctrl(pcie, false);
>  
> +	iproc_pcie_invalidate_mapping(pcie);

It makes more sense to call this in the .shutdown() method if I
understand what it does.

Lorenzo

>  	if (pcie->need_ob_cfg) {
>  		ret = iproc_pcie_map_ranges(pcie, res);
>  		if (ret) {
> -- 
> 2.17.1
> 

Re: [PATCH 1/1] PCI: iproc: Invalidate PAXB address mapping before programming it

[Abhishek Shah]

Hi Lorenzo,

Please see my comments inline:

On Tue, Oct 15, 2019 at 10:13 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Fri, Sep 06, 2019 at 09:28:13AM +0530, Abhishek Shah wrote:
> > Invalidate PAXB inbound/outbound address mapping each time before
> > programming it. This is helpful for the cases where we need to
> > reprogram inbound/outbound address mapping without resetting PAXB.
> > kexec kernel is one such example.
>
> This looks like a hack, explain to us please what it actually solves and
> why a full reset is not necessary.
>
The PAXB IP performs address translation(PCI<->AXI address) for both inbound and
outbound addresses (amongst other things) based on version of IP being used.
It does so using the IMAP/IARR/OMAP/OARR registers.

These registers get programmed as per mappings specified in device tree during
PCI driver probe for each RC and do not get reset when kexec/kdump kernel boots.
This results in driver assuming valid mappings in place for some mapping windows
during kexec/kdump kernel boot, consequently it skips those windows and
we run out of available mapping windows, leading to mapping failure.

Normally, we take care of resetting PAXB block in firmware, but in
primary kernel
to kexec/kdump kernel handover, no firmware is executed in between.
So, we just, by default, invalidate the mapping registers each time before
programming them to solve the issue described above..
We do not need full reset for handling this.

> > Signed-off-by: Abhishek Shah <abhishek.shah@broadcom.com>
> > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > Reviewed-by: Vikram Mysore Prakash <vikram.prakash@broadcom.com>
>
> Patches are reviewed on public mailing lists, remove tags given
> on internal reviews - they are not relevant.
>
Ok, will remove.

> > ---
> >  drivers/pci/controller/pcie-iproc.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> > index e3ca46497470..99a9521ba7ab 100644
> > --- a/drivers/pci/controller/pcie-iproc.c
> > +++ b/drivers/pci/controller/pcie-iproc.c
> > @@ -1245,6 +1245,32 @@ static int iproc_pcie_map_dma_ranges(struct iproc_pcie *pcie)
> >       return ret;
> >  }
> >
> > +static void iproc_pcie_invalidate_mapping(struct iproc_pcie *pcie)
> > +{
> > +     struct iproc_pcie_ib *ib = &pcie->ib;
> > +     struct iproc_pcie_ob *ob = &pcie->ob;
> > +     int idx;
> > +
> > +     if (pcie->ep_is_internal)
>
> What's this check for and why leaving mappings in place is safe for
> this category of IPs ?
For this category of IP(PAXC), no mappings need to be programmed in
the first place.

>
> > +             return;
> > +
> > +     if (pcie->need_ob_cfg) {
> > +             /* iterate through all OARR mapping regions */
> > +             for (idx = ob->nr_windows - 1; idx >= 0; idx--) {
> > +                     iproc_pcie_write_reg(pcie,
> > +                                          MAP_REG(IPROC_PCIE_OARR0, idx), 0);
> > +             }
> > +     }
> > +
> > +     if (pcie->need_ib_cfg) {
> > +             /* iterate through all IARR mapping regions */
> > +             for (idx = 0; idx < ib->nr_regions; idx++) {
> > +                     iproc_pcie_write_reg(pcie,
> > +                                          MAP_REG(IPROC_PCIE_IARR0, idx), 0);
> > +             }
> > +     }
> > +}
> > +
> >  static int iproce_pcie_get_msi(struct iproc_pcie *pcie,
> >                              struct device_node *msi_node,
> >                              u64 *msi_addr)
> > @@ -1517,6 +1543,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
> >       iproc_pcie_perst_ctrl(pcie, true);
> >       iproc_pcie_perst_ctrl(pcie, false);
> >
> > +     iproc_pcie_invalidate_mapping(pcie);
>
> It makes more sense to call this in the .shutdown() method if I
> understand what it does.
>
It would work for kexec kernel, but not for kdump kernel as only for
kexec'ed kernel,
"device_shutdown" callback is present. We are here taking care of both the cases
with this patch.


Regards,
Abhishek

> Lorenzo
>
> >       if (pcie->need_ob_cfg) {
> >               ret = iproc_pcie_map_ranges(pcie, res);
> >               if (ret) {
> > --
> > 2.17.1
> >

Re: [PATCH 1/1] PCI: iproc: Invalidate PAXB address mapping before programming it

[Lorenzo Pieralisi]

On Thu, Oct 17, 2019 at 07:57:56PM +0530, Abhishek Shah wrote:
> Hi Lorenzo,
> 
> Please see my comments inline:
> 
> On Tue, Oct 15, 2019 at 10:13 PM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Fri, Sep 06, 2019 at 09:28:13AM +0530, Abhishek Shah wrote:
> > > Invalidate PAXB inbound/outbound address mapping each time before
> > > programming it. This is helpful for the cases where we need to
> > > reprogram inbound/outbound address mapping without resetting PAXB.
> > > kexec kernel is one such example.
> >
> > This looks like a hack, explain to us please what it actually solves and
> > why a full reset is not necessary.
> >
> The PAXB IP performs address translation(PCI<->AXI address) for both inbound and
> outbound addresses (amongst other things) based on version of IP being used.
> It does so using the IMAP/IARR/OMAP/OARR registers.
> 
> These registers get programmed as per mappings specified in device tree during
> PCI driver probe for each RC and do not get reset when kexec/kdump kernel boots.
> This results in driver assuming valid mappings in place for some mapping windows
> during kexec/kdump kernel boot, consequently it skips those windows and
> we run out of available mapping windows, leading to mapping failure.
> 
> Normally, we take care of resetting PAXB block in firmware, but in
> primary kernel to kexec/kdump kernel handover, no firmware is executed
> in between.  So, we just, by default, invalidate the mapping registers
> each time before
> programming them to solve the issue described above..
> We do not need full reset for handling this.

I see. A simple bitmap to detect which windows are *actually*
programmed by the current kernel (that can be used by

iproc_pcie_ob_is_valid()

to carry out a valid check) would do as well instead of having to
invalidate all the OB registers.

It is up to you, let me know and I will merge code accordingly.

Lorenzo

> > > Signed-off-by: Abhishek Shah <abhishek.shah@broadcom.com>
> > > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > > Reviewed-by: Vikram Mysore Prakash <vikram.prakash@broadcom.com>
> >
> > Patches are reviewed on public mailing lists, remove tags given
> > on internal reviews - they are not relevant.
> >
> Ok, will remove.
> 
> > > ---
> > >  drivers/pci/controller/pcie-iproc.c | 28 ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> > > index e3ca46497470..99a9521ba7ab 100644
> > > --- a/drivers/pci/controller/pcie-iproc.c
> > > +++ b/drivers/pci/controller/pcie-iproc.c
> > > @@ -1245,6 +1245,32 @@ static int iproc_pcie_map_dma_ranges(struct iproc_pcie *pcie)
> > >       return ret;
> > >  }
> > >
> > > +static void iproc_pcie_invalidate_mapping(struct iproc_pcie *pcie)
> > > +{
> > > +     struct iproc_pcie_ib *ib = &pcie->ib;
> > > +     struct iproc_pcie_ob *ob = &pcie->ob;
> > > +     int idx;
> > > +
> > > +     if (pcie->ep_is_internal)
> >
> > What's this check for and why leaving mappings in place is safe for
> > this category of IPs ?
> For this category of IP(PAXC), no mappings need to be programmed in
> the first place.
> 
> >
> > > +             return;
> > > +
> > > +     if (pcie->need_ob_cfg) {
> > > +             /* iterate through all OARR mapping regions */
> > > +             for (idx = ob->nr_windows - 1; idx >= 0; idx--) {
> > > +                     iproc_pcie_write_reg(pcie,
> > > +                                          MAP_REG(IPROC_PCIE_OARR0, idx), 0);
> > > +             }
> > > +     }
> > > +
> > > +     if (pcie->need_ib_cfg) {
> > > +             /* iterate through all IARR mapping regions */
> > > +             for (idx = 0; idx < ib->nr_regions; idx++) {
> > > +                     iproc_pcie_write_reg(pcie,
> > > +                                          MAP_REG(IPROC_PCIE_IARR0, idx), 0);
> > > +             }
> > > +     }
> > > +}
> > > +
> > >  static int iproce_pcie_get_msi(struct iproc_pcie *pcie,
> > >                              struct device_node *msi_node,
> > >                              u64 *msi_addr)
> > > @@ -1517,6 +1543,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
> > >       iproc_pcie_perst_ctrl(pcie, true);
> > >       iproc_pcie_perst_ctrl(pcie, false);
> > >
> > > +     iproc_pcie_invalidate_mapping(pcie);
> >
> > It makes more sense to call this in the .shutdown() method if I
> > understand what it does.
> >
> It would work for kexec kernel, but not for kdump kernel as only for
> kexec'ed kernel,
> "device_shutdown" callback is present. We are here taking care of both the cases
> with this patch.
> 
> 
> Regards,
> Abhishek
> 
> > Lorenzo
> >
> > >       if (pcie->need_ob_cfg) {
> > >               ret = iproc_pcie_map_ranges(pcie, res);
> > >               if (ret) {
> > > --
> > > 2.17.1
> > >

Re: [PATCH 1/1] PCI: iproc: Invalidate PAXB address mapping before programming it

[Abhishek Shah]

On Mon, Oct 21, 2019 at 4:08 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Thu, Oct 17, 2019 at 07:57:56PM +0530, Abhishek Shah wrote:
> > Hi Lorenzo,
> >
> > Please see my comments inline:
> >
> > On Tue, Oct 15, 2019 at 10:13 PM Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> wrote:
> > >
> > > On Fri, Sep 06, 2019 at 09:28:13AM +0530, Abhishek Shah wrote:
> > > > Invalidate PAXB inbound/outbound address mapping each time before
> > > > programming it. This is helpful for the cases where we need to
> > > > reprogram inbound/outbound address mapping without resetting PAXB.
> > > > kexec kernel is one such example.
> > >
> > > This looks like a hack, explain to us please what it actually solves and
> > > why a full reset is not necessary.
> > >
> > The PAXB IP performs address translation(PCI<->AXI address) for both inbound and
> > outbound addresses (amongst other things) based on version of IP being used.
> > It does so using the IMAP/IARR/OMAP/OARR registers.
> >
> > These registers get programmed as per mappings specified in device tree during
> > PCI driver probe for each RC and do not get reset when kexec/kdump kernel boots.
> > This results in driver assuming valid mappings in place for some mapping windows
> > during kexec/kdump kernel boot, consequently it skips those windows and
> > we run out of available mapping windows, leading to mapping failure.
> >
> > Normally, we take care of resetting PAXB block in firmware, but in
> > primary kernel to kexec/kdump kernel handover, no firmware is executed
> > in between.  So, we just, by default, invalidate the mapping registers
> > each time before
> > programming them to solve the issue described above..
> > We do not need full reset for handling this.
>
> I see. A simple bitmap to detect which windows are *actually*
> programmed by the current kernel (that can be used by
>
> iproc_pcie_ob_is_valid()
>
> to carry out a valid check) would do as well instead of having to
> invalidate all the OB registers.
>
Okay, so you are suggesting to use variable/bitmap to hold status of
ib/ob windows (mapped/unmapped)
instead of using registers to check it. Please note that we would
still be programming corresponding
window register to mark it valid (HW requirement).

@Ray, could you please provide feedback on this? I think existing way
is proper for given driver design.

Also, as internal review tags are irrelevant as suggested by Lorenzo earlier,
could you please put sign again once reviewed?


Regards,
Abhishek

> It is up to you, let me know and I will merge code accordingly.
>
> Lorenzo
>
> > > > Signed-off-by: Abhishek Shah <abhishek.shah@broadcom.com>
> > > > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > > > Reviewed-by: Vikram Mysore Prakash <vikram.prakash@broadcom.com>
> > >
> > > Patches are reviewed on public mailing lists, remove tags given
> > > on internal reviews - they are not relevant.
> > >
> > Ok, will remove.
> >
> > > > ---
> > > >  drivers/pci/controller/pcie-iproc.c | 28 ++++++++++++++++++++++++++++
> > > >  1 file changed, 28 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> > > > index e3ca46497470..99a9521ba7ab 100644
> > > > --- a/drivers/pci/controller/pcie-iproc.c
> > > > +++ b/drivers/pci/controller/pcie-iproc.c
> > > > @@ -1245,6 +1245,32 @@ static int iproc_pcie_map_dma_ranges(struct iproc_pcie *pcie)
> > > >       return ret;
> > > >  }
> > > >
> > > > +static void iproc_pcie_invalidate_mapping(struct iproc_pcie *pcie)
> > > > +{
> > > > +     struct iproc_pcie_ib *ib = &pcie->ib;
> > > > +     struct iproc_pcie_ob *ob = &pcie->ob;
> > > > +     int idx;
> > > > +
> > > > +     if (pcie->ep_is_internal)
> > >
> > > What's this check for and why leaving mappings in place is safe for
> > > this category of IPs ?
> > For this category of IP(PAXC), no mappings need to be programmed in
> > the first place.
> >
> > >
> > > > +             return;
> > > > +
> > > > +     if (pcie->need_ob_cfg) {
> > > > +             /* iterate through all OARR mapping regions */
> > > > +             for (idx = ob->nr_windows - 1; idx >= 0; idx--) {
> > > > +                     iproc_pcie_write_reg(pcie,
> > > > +                                          MAP_REG(IPROC_PCIE_OARR0, idx), 0);
> > > > +             }
> > > > +     }
> > > > +
> > > > +     if (pcie->need_ib_cfg) {
> > > > +             /* iterate through all IARR mapping regions */
> > > > +             for (idx = 0; idx < ib->nr_regions; idx++) {
> > > > +                     iproc_pcie_write_reg(pcie,
> > > > +                                          MAP_REG(IPROC_PCIE_IARR0, idx), 0);
> > > > +             }
> > > > +     }
> > > > +}
> > > > +
> > > >  static int iproce_pcie_get_msi(struct iproc_pcie *pcie,
> > > >                              struct device_node *msi_node,
> > > >                              u64 *msi_addr)
> > > > @@ -1517,6 +1543,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
> > > >       iproc_pcie_perst_ctrl(pcie, true);
> > > >       iproc_pcie_perst_ctrl(pcie, false);
> > > >
> > > > +     iproc_pcie_invalidate_mapping(pcie);
> > >
> > > It makes more sense to call this in the .shutdown() method if I
> > > understand what it does.
> > >
> > It would work for kexec kernel, but not for kdump kernel as only for
> > kexec'ed kernel,
> > "device_shutdown" callback is present. We are here taking care of both the cases
> > with this patch.
> >
> >
> > Regards,
> > Abhishek
> >
> > > Lorenzo
> > >
> > > >       if (pcie->need_ob_cfg) {
> > > >               ret = iproc_pcie_map_ranges(pcie, res);
> > > >               if (ret) {
> > > > --
> > > > 2.17.1
> > > >

Re: [PATCH 1/1] PCI: iproc: Invalidate PAXB address mapping before programming it

[Lorenzo Pieralisi]

On Thu, Oct 24, 2019 at 08:52:41AM +0530, Abhishek Shah wrote:
> On Mon, Oct 21, 2019 at 4:08 PM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Thu, Oct 17, 2019 at 07:57:56PM +0530, Abhishek Shah wrote:
> > > Hi Lorenzo,
> > >
> > > Please see my comments inline:
> > >
> > > On Tue, Oct 15, 2019 at 10:13 PM Lorenzo Pieralisi
> > > <lorenzo.pieralisi@arm.com> wrote:
> > > >
> > > > On Fri, Sep 06, 2019 at 09:28:13AM +0530, Abhishek Shah wrote:
> > > > > Invalidate PAXB inbound/outbound address mapping each time before
> > > > > programming it. This is helpful for the cases where we need to
> > > > > reprogram inbound/outbound address mapping without resetting PAXB.
> > > > > kexec kernel is one such example.
> > > >
> > > > This looks like a hack, explain to us please what it actually solves and
> > > > why a full reset is not necessary.
> > > >
> > > The PAXB IP performs address translation(PCI<->AXI address) for both inbound and
> > > outbound addresses (amongst other things) based on version of IP being used.
> > > It does so using the IMAP/IARR/OMAP/OARR registers.
> > >
> > > These registers get programmed as per mappings specified in device tree during
> > > PCI driver probe for each RC and do not get reset when kexec/kdump kernel boots.
> > > This results in driver assuming valid mappings in place for some mapping windows
> > > during kexec/kdump kernel boot, consequently it skips those windows and
> > > we run out of available mapping windows, leading to mapping failure.
> > >
> > > Normally, we take care of resetting PAXB block in firmware, but in
> > > primary kernel to kexec/kdump kernel handover, no firmware is executed
> > > in between.  So, we just, by default, invalidate the mapping registers
> > > each time before
> > > programming them to solve the issue described above..
> > > We do not need full reset for handling this.
> >
> > I see. A simple bitmap to detect which windows are *actually*
> > programmed by the current kernel (that can be used by
> >
> > iproc_pcie_ob_is_valid()
> >
> > to carry out a valid check) would do as well instead of having to
> > invalidate all the OB registers.
> >
> Okay, so you are suggesting to use variable/bitmap to hold status of
> ib/ob windows (mapped/unmapped) instead of using registers to check
> it. Please note that we would still be programming corresponding
> window register to mark it valid (HW requirement).

It is your choice.

> @Ray, could you please provide feedback on this? I think existing way
> is proper for given driver design.
> 
> Also, as internal review tags are irrelevant as suggested by Lorenzo
> earlier, could you please put sign again once reviewed?

Yes please, repost (with the solution you prefer) and Ray please
tag accordingly on ML.

Thanks,
Lorenzo

> Regards,
> Abhishek
> 
> > It is up to you, let me know and I will merge code accordingly.
> >
> > Lorenzo
> >
> > > > > Signed-off-by: Abhishek Shah <abhishek.shah@broadcom.com>
> > > > > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > > > > Reviewed-by: Vikram Mysore Prakash <vikram.prakash@broadcom.com>
> > > >
> > > > Patches are reviewed on public mailing lists, remove tags given
> > > > on internal reviews - they are not relevant.
> > > >
> > > Ok, will remove.
> > >
> > > > > ---
> > > > >  drivers/pci/controller/pcie-iproc.c | 28 ++++++++++++++++++++++++++++
> > > > >  1 file changed, 28 insertions(+)
> > > > >
> > > > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> > > > > index e3ca46497470..99a9521ba7ab 100644
> > > > > --- a/drivers/pci/controller/pcie-iproc.c
> > > > > +++ b/drivers/pci/controller/pcie-iproc.c
> > > > > @@ -1245,6 +1245,32 @@ static int iproc_pcie_map_dma_ranges(struct iproc_pcie *pcie)
> > > > >       return ret;
> > > > >  }
> > > > >
> > > > > +static void iproc_pcie_invalidate_mapping(struct iproc_pcie *pcie)
> > > > > +{
> > > > > +     struct iproc_pcie_ib *ib = &pcie->ib;
> > > > > +     struct iproc_pcie_ob *ob = &pcie->ob;
> > > > > +     int idx;
> > > > > +
> > > > > +     if (pcie->ep_is_internal)
> > > >
> > > > What's this check for and why leaving mappings in place is safe for
> > > > this category of IPs ?
> > > For this category of IP(PAXC), no mappings need to be programmed in
> > > the first place.
> > >
> > > >
> > > > > +             return;
> > > > > +
> > > > > +     if (pcie->need_ob_cfg) {
> > > > > +             /* iterate through all OARR mapping regions */
> > > > > +             for (idx = ob->nr_windows - 1; idx >= 0; idx--) {
> > > > > +                     iproc_pcie_write_reg(pcie,
> > > > > +                                          MAP_REG(IPROC_PCIE_OARR0, idx), 0);
> > > > > +             }
> > > > > +     }
> > > > > +
> > > > > +     if (pcie->need_ib_cfg) {
> > > > > +             /* iterate through all IARR mapping regions */
> > > > > +             for (idx = 0; idx < ib->nr_regions; idx++) {
> > > > > +                     iproc_pcie_write_reg(pcie,
> > > > > +                                          MAP_REG(IPROC_PCIE_IARR0, idx), 0);
> > > > > +             }
> > > > > +     }
> > > > > +}
> > > > > +
> > > > >  static int iproce_pcie_get_msi(struct iproc_pcie *pcie,
> > > > >                              struct device_node *msi_node,
> > > > >                              u64 *msi_addr)
> > > > > @@ -1517,6 +1543,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
> > > > >       iproc_pcie_perst_ctrl(pcie, true);
> > > > >       iproc_pcie_perst_ctrl(pcie, false);
> > > > >
> > > > > +     iproc_pcie_invalidate_mapping(pcie);
> > > >
> > > > It makes more sense to call this in the .shutdown() method if I
> > > > understand what it does.
> > > >
> > > It would work for kexec kernel, but not for kdump kernel as only for
> > > kexec'ed kernel,
> > > "device_shutdown" callback is present. We are here taking care of both the cases
> > > with this patch.
> > >
> > >
> > > Regards,
> > > Abhishek
> > >
> > > > Lorenzo
> > > >
> > > > >       if (pcie->need_ob_cfg) {
> > > > >               ret = iproc_pcie_map_ranges(pcie, res);
> > > > >               if (ret) {
> > > > > --
> > > > > 2.17.1
> > > > >

Re: [PATCH 1/1] PCI: iproc: Invalidate PAXB address mapping before programming it

[Lorenzo Pieralisi]

On Fri, Sep 06, 2019 at 09:28:13AM +0530, Abhishek Shah wrote:
> Invalidate PAXB inbound/outbound address mapping each time before
> programming it. This is helpful for the cases where we need to
> reprogram inbound/outbound address mapping without resetting PAXB.
> kexec kernel is one such example.
> 
> Signed-off-by: Abhishek Shah <abhishek.shah@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Vikram Mysore Prakash <vikram.prakash@broadcom.com>
> ---
>  drivers/pci/controller/pcie-iproc.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)

Merged in pci/iproc (rewrote commit log), thanks.

Lorenzo

> diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> index e3ca46497470..99a9521ba7ab 100644
> --- a/drivers/pci/controller/pcie-iproc.c
> +++ b/drivers/pci/controller/pcie-iproc.c
> @@ -1245,6 +1245,32 @@ static int iproc_pcie_map_dma_ranges(struct iproc_pcie *pcie)
>  	return ret;
>  }
>  
> +static void iproc_pcie_invalidate_mapping(struct iproc_pcie *pcie)
> +{
> +	struct iproc_pcie_ib *ib = &pcie->ib;
> +	struct iproc_pcie_ob *ob = &pcie->ob;
> +	int idx;
> +
> +	if (pcie->ep_is_internal)
> +		return;
> +
> +	if (pcie->need_ob_cfg) {
> +		/* iterate through all OARR mapping regions */
> +		for (idx = ob->nr_windows - 1; idx >= 0; idx--) {
> +			iproc_pcie_write_reg(pcie,
> +					     MAP_REG(IPROC_PCIE_OARR0, idx), 0);
> +		}
> +	}
> +
> +	if (pcie->need_ib_cfg) {
> +		/* iterate through all IARR mapping regions */
> +		for (idx = 0; idx < ib->nr_regions; idx++) {
> +			iproc_pcie_write_reg(pcie,
> +					     MAP_REG(IPROC_PCIE_IARR0, idx), 0);
> +		}
> +	}
> +}
> +
>  static int iproce_pcie_get_msi(struct iproc_pcie *pcie,
>  			       struct device_node *msi_node,
>  			       u64 *msi_addr)
> @@ -1517,6 +1543,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>  	iproc_pcie_perst_ctrl(pcie, true);
>  	iproc_pcie_perst_ctrl(pcie, false);
>  
> +	iproc_pcie_invalidate_mapping(pcie);
> +
>  	if (pcie->need_ob_cfg) {
>  		ret = iproc_pcie_map_ranges(pcie, res);
>  		if (ret) {
> -- 
> 2.17.1
>