Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Jim Quinlan]

On Thu, Apr 8, 2021 at 12:20 PM Rob Herring  wrote:
>
> On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
> > On Tue, Apr 6, 2021 at 1:32 PM Mark Brown  wrote:
> > >
> > > On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote:
> > > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown  wrote:
> > >
> > > > > No great problem with having these in the controller node (assming it
> > > > > accurately describes the hardware) but I do think we ought to also be
> > > > > able to describe these per slot.
>
> PCIe is effectively point to point, so there's only 1 slot unless
> there's a PCIe switch in the middle. If that's the case, then it's all
> more complicated.
>
> > > > Can you explain what you think that would look like in the DT?
> > >
> > > I *think* that's just some properties on the nodes for the endpoints,
> > > note that the driver could just ignore them for now.  Not sure where or
> > > if we document any extensions but child nodes are in section 4 of the
> > > v2.1 PCI bus binding.
> >
> > Hi Mark,
> >
> > I'm a little confused -- here is how I remember the chronology of the
> > "DT bindings" commit reviews, please correct me if I'm wrong:
> >
> > o JimQ submitted a pullreq for using voltage regulators in the same
> > style as the existing "rockport" PCIe driver.
> > o After some deliberation, RobH preferred that the voltage regulators
> > should go into the PCIe subnode device's DT node.
>
> IIRC, that's because you said there isn't a standard slot.
Admittedly, I'm not exactly sure what you mean by a "standard slot".
Our PCIIe HW does not support  hotplug or have a presence bit or
anything like that.  Our root complex has one port; it can only
directly connect to a single switch or endpoint. This connection shows
up as slot0.  The voltage regulator(s) involved depend on a GPIO that
turns the power  on/off for the connected device/chip.  The gpio pin
can vary from board to board but this is easily handled in our DT.
Some boards have regulators that are always on and not associated with
a GPIO pin -- these have no representation in our DT.

Regards,
Jim


>
> > o JimQ put the voltage regulators in the subnode device's DT node.
> > o MarkB didn't like the fact that the code did a global search for the
> > regulator since it could not provide the owning struct device* handle.
> > o RobH relented, and said that if it is just two specific and standard
> > voltage regulators, perhaps they can go in the parent DT node after
> > all.
> > o JimQ put the regulators back in the PCIe node.
> > o MarkB now wants the regulators to go back into the child node again?
> >
> > Folks, please advise.
> >
> > Regards,
> > Jim Quinlan
> > Broadcom STB

Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Jim Quinlan]

On Tue, Apr 6, 2021 at 1:32 PM Mark Brown  wrote:
>
> On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote:
> > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown  wrote:
>
> > > No great problem with having these in the controller node (assming it
> > > accurately describes the hardware) but I do think we ought to also be
> > > able to describe these per slot.
>
> > Can you explain what you think that would look like in the DT?
>
> I *think* that's just some properties on the nodes for the endpoints,
> note that the driver could just ignore them for now.  Not sure where or
> if we document any extensions but child nodes are in section 4 of the
> v2.1 PCI bus binding.

Hi Mark,

I'm a little confused -- here is how I remember the chronology of the
"DT bindings" commit reviews, please correct me if I'm wrong:

o JimQ submitted a pullreq for using voltage regulators in the same
style as the existing "rockport" PCIe driver.
o After some deliberation, RobH preferred that the voltage regulators
should go into the PCIe subnode device's DT node.
o JimQ put the voltage regulators in the subnode device's DT node.
o MarkB didn't like the fact that the code did a global search for the
regulator since it could not provide the owning struct device* handle.
o RobH relented, and said that if it is just two specific and standard
voltage regulators, perhaps they can go in the parent DT node after
all.
o JimQ put the regulators back in the PCIe node.
o MarkB now wants the regulators to go back into the child node again?

Folks, please advise.

Regards,
Jim Quinlan
Broadcom STB

Re: [PATCH v4 3/6] PCI: brcmstb: Add control of slot0 device voltage regulators

[Jim Quinlan]

On Tue, Apr 6, 2021 at 1:23 PM Mark Brown  wrote:
>
> On Tue, Apr 06, 2021 at 12:59:16PM -0400, Jim Quinlan wrote:
> > On Tue, Apr 6, 2021 at 12:34 PM Mark Brown  wrote:
> > > On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote:
>
> > > This is broken, the driver knows which supplies are expected, the device
> > > can't function without these supplies so the driver should just
> > > unconditionally request them like any other supply.
>
> > Some boards require the regulators, some do not.  So the driver is
>
> No, some boards have the supplies described in firmware and some do not.
True.
>
> > only  sure what the names may be if they are present.  If  I put these
> > names in my struct regulator_bulk_data array and do a
> > devm_regulator_bulk_get(), I will get the following for the boards
> > that do not need the regulators (e.g. the RPi SOC):
> >
> > [6.823820] brcm-pcie xxx.pcie: supply vpcie12v-supply not found,
> > using dummy regulator
> > [6.832265] brcm-pcie xxx.pcie: supply vpcie3v3-supply not found,
> > using dummy regulator
>
> Sure, those are just warnings.
>
> > IIRC you consider this a debug feature?  Be that as it may, these
> > lines will confuse our customers and I'd like that they not be printed
> > if possible.
>
> You can stop the warnings by updating your firmware to more completely
> describe the system - ideally all the supplies in the system would be
> described for future proofing.  Or if this is a custom software stack
> just delete whatever error checking and warnings you like.  The warnings
> are there in case we've not got something mapped properly (eg, if there
> were a typo in a property name) and things stop working, it's not great
> to just ignore errors.
A lot of this is really not under our control.
>
> > So I ask you to allow the code as is.  If you still insist, I will
> > change and resubmit.
>
> Remove it, conditional code like this is just as bad in this driver as
> it is in every other one.
I will remove this and resubmit.
Thanks,
Jim Quinlan
Broadcom STB

Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Jim Quinlan]

On Tue, Apr 6, 2021 at 12:47 PM Mark Brown  wrote:
>
> On Thu, Apr 01, 2021 at 05:21:42PM -0400, Jim Quinlan wrote:
> > Similar to the regulator bindings found in "rockchip-pcie-host.txt", this
> > allows optional regulators to be attached and controlled by the PCIe RC
> > driver.  That being said, this driver searches in the DT subnode (the EP
> > node, eg pci@0,0) for the regulator property.
>
> > The use of a regulator property in the pcie EP subnode such as
> > "vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml
> > file at
> >
> > https://github.com/devicetree-org/dt-schema/pull/54
> >
> > Signed-off-by: Jim Quinlan 
> > ---
> >  Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml 
> > b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index f90557f6deb8..f2caa5b3b281 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > @@ -64,6 +64,9 @@ properties:
> >
> >aspm-no-l0s: true
> >
> > +  vpcie12v-supply: true
> > +  vpcie3v3-supply: true
> > +
>
> No great problem with having these in the controller node (assming it
> accurately describes the hardware) but I do think we ought to also be
> able to describe these per slot.

Hi Mark,
Can you explain what you think that would look like in the DT?
Thanks,
Jim Quinlan
Broadcom STB

Re: [PATCH v4 3/6] PCI: brcmstb: Add control of slot0 device voltage regulators

[Jim Quinlan]

On Tue, Apr 6, 2021 at 12:34 PM Mark Brown  wrote:
>
> On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote:
>
> > + /* Look for specific pcie regulators in the RC DT node. */
> > + for_each_property_of_node(np, pp) {
> > + for (i = 0; i < ns; i++)
> > + if (strcmp(supplies[i], pp->name) == 0)
>
> This is broken, the driver knows which supplies are expected, the device
> can't function without these supplies so the driver should just
> unconditionally request them like any other supply.

Hi  Mark,
Some boards require the regulators, some do not.  So the driver is
only  sure what the names may be if they are present.  If  I put these
names in my struct regulator_bulk_data array and do a
devm_regulator_bulk_get(), I will get the following for the boards
that do not need the regulators (e.g. the RPi SOC):

[6.823820] brcm-pcie xxx.pcie: supply vpcie12v-supply not found,
using dummy regulator
[6.832265] brcm-pcie xxx.pcie: supply vpcie3v3-supply not found,
using dummy regulator

IIRC you consider this a debug feature?  Be that as it may, these
lines will confuse our customers and I'd like that they not be printed
if possible.

So I ask you to allow the code as is.  If you still insist, I will
change and resubmit.

Regards,
Jim Quinlan
Broadcom STB

[PATCH v4 3/6] PCI: brcmstb: Add control of slot0 device voltage regulators

[Jim Quinlan]

This Broadcom STB has one port and connects directly to one device, be it a
switch or an endpoint.  We want to be able to turn on/off any regulators
for that device.  Control of regulators is needed because of the
chicken-and-egg situation: although the regulator is "owned" by the device
and would be best handled by its driver, the device cannot be discovered
and probed unless its regulator is already turned on.

Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 83 +--
 1 file changed, 78 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index 4ce1f3a60574..1b0de0c7da60 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -169,6 +170,7 @@
 #define SSC_STATUS_SSC_MASK0x400
 #define SSC_STATUS_PLL_LOCK_MASK   0x800
 #define PCIE_BRCM_MAX_MEMC 3
+#define PCIE_BRCM_MAX_EP_REGULATORS4
 
 #define IDX_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_INDEX])
 #define DATA_ADDR(pcie)
(pcie->reg_offsets[EXT_CFG_DATA])
@@ -192,6 +194,11 @@ static inline void brcm_pcie_perst_set_4908(struct 
brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 
val);
 
+static const char * const supplies[] = {
+   "vpcie12v-supply",
+   "vpcie3v3-supply",
+};
+
 enum {
RGR1_SW_INIT_1,
EXT_CFG_INDEX,
@@ -295,8 +302,27 @@ struct brcm_pcie {
u32 hw_rev;
void(*perst_set)(struct brcm_pcie *pcie, u32 val);
void(*bridge_sw_init_set)(struct brcm_pcie *pcie, 
u32 val);
+   struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
+   unsigned intnum_supplies;
 };
 
+static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+{
+   struct device *dev = pcie->dev;
+   int ret;
+
+   if (!pcie->num_supplies)
+   return 0;
+   if (on)
+   ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
+   else
+   ret = regulator_bulk_disable(pcie->num_supplies, 
pcie->supplies);
+   if (ret)
+   dev_err(dev, "failed to %s EP regulators\n",
+   on ? "enable" : "disable");
+   return ret;
+}
+
 /*
  * This is to convert the size of the inbound "BAR" region to the
  * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
@@ -1112,9 +1138,10 @@ static inline int brcm_phy_start(struct brcm_pcie *pcie)
return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0;
 }
 
-static inline int brcm_phy_stop(struct brcm_pcie *pcie)
+static inline void brcm_phy_stop(struct brcm_pcie *pcie)
 {
-   return pcie->rescal ? brcm_phy_cntl(pcie, 0) : 0;
+   if (pcie->rescal)
+   brcm_phy_cntl(pcie, 0);
 }
 
 static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
@@ -1141,16 +1168,45 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
pcie->bridge_sw_init_set(pcie, 1);
 }
 
+static int brcm_pcie_get_regulators(struct brcm_pcie *pcie)
+{
+   struct device_node *np = pcie->np;
+   struct property *pp;
+   const unsigned int ns = ARRAY_SIZE(supplies);
+   unsigned int i;
+   int ret = 0;
+
+   /* Look for specific pcie regulators in the RC DT node. */
+   for_each_property_of_node(np, pp) {
+   for (i = 0; i < ns; i++)
+   if (strcmp(supplies[i], pp->name) == 0)
+   break;
+   if (i >= ns)
+   continue;
+
+   if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS)
+   pcie->supplies[pcie->num_supplies++].supply
+   = supplies[i];
+   else
+   dev_warn(pcie->dev, "No room for supply %s\n",
+supplies[i]);
+   }
+
+   if (pcie->num_supplies)
+   ret = devm_regulator_bulk_get(pcie->dev, pcie->num_supplies,
+ pcie->supplies);
+   return ret;
+}
+
 static int brcm_pcie_suspend(struct device *dev)
 {
struct brcm_pcie *pcie = dev_get_drvdata(dev);
-   int ret;
 
brcm_pcie_turn_off(pcie);
-   ret = brcm_phy_stop(pcie);
+   brcm_phy_stop(pcie);
clk_disable_unprepare(pcie->clk);
 
-   return ret;
+   return brcm_set_regulators(pcie, false);
 }
 
 static int brcm_pcie_resume(struct device *dev)
@@ -1165,6 +1221,10 @@ static int brcm_pcie_resume(struct device 

[PATCH v4 5/6] PCI: brcmstb: Give 7216 SOCs their own config type

[Jim Quinlan]

This distinction is required for an imminent commit.

Signed-off-by: Jim Quinlan 
Acked-by: Florian Fainelli 
---
 drivers/pci/controller/pcie-brcmstb.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index 4c79aff66de7..44128df33785 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -265,6 +265,13 @@ static const struct pcie_cfg_data bcm2711_cfg = {
.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 };
 
+static const struct pcie_cfg_data bcm7216_cfg = {
+   .offsets= pcie_offset_bcm7278,
+   .type   = BCM7278,
+   .perst_set  = brcm_pcie_perst_set_7278,
+   .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
+};
+
 struct brcm_msi {
struct device   *dev;
void __iomem*base;
@@ -1325,7 +1332,7 @@ static const struct of_device_id brcm_pcie_match[] = {
{ .compatible = "brcm,bcm4908-pcie", .data = _cfg },
{ .compatible = "brcm,bcm7211-pcie", .data = _cfg },
{ .compatible = "brcm,bcm7278-pcie", .data = _cfg },
-   { .compatible = "brcm,bcm7216-pcie", .data = _cfg },
+   { .compatible = "brcm,bcm7216-pcie", .data = _cfg },
{ .compatible = "brcm,bcm7445-pcie", .data = _cfg },
{},
 };
-- 
2.17.1

[PATCH v4 6/6] PCI: brcmstb: Add panic/die handler to RC driver

[Jim Quinlan]

Whereas most PCIe HW returns 0x on illegal accesses and the like,
by default Broadcom's STB PCIe controller effects an abort.  This simple
handler determines if the PCIe controller was the cause of the abort and if
so, prints out diagnostic info.

Example output:
  brcm-pcie 8b2.pcie: Error: Mem Acc: 32bit, Read, @0x3800
  brcm-pcie 8b2.pcie:  Type: TO=0 Abt=0 UnspReq=1 AccDsble=0 BadAddr=0

Signed-off-by: Jim Quinlan 
Acked-by: Florian Fainelli 
---
 drivers/pci/controller/pcie-brcmstb.c | 122 ++
 1 file changed, 122 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index 44128df33785..73a12c62b94e 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -12,11 +12,13 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -186,6 +188,39 @@
 #define  PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_MASK0x1
 #define  PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_SHIFT   0x0
 
+/* Error report regiseters */
+#define PCIE_OUTB_ERR_TREAT0x6000
+#define  PCIE_OUTB_ERR_TREAT_CONFIG_MASK   0x1
+#define  PCIE_OUTB_ERR_TREAT_MEM_MASK  0x2
+#define PCIE_OUTB_ERR_VALID0x6004
+#define PCIE_OUTB_ERR_CLEAR0x6008
+#define PCIE_OUTB_ERR_ACC_INFO 0x600c
+#define  PCIE_OUTB_ERR_ACC_INFO_CFG_ERR_MASK   0x01
+#define  PCIE_OUTB_ERR_ACC_INFO_MEM_ERR_MASK   0x02
+#define  PCIE_OUTB_ERR_ACC_INFO_TYPE_64_MASK   0x04
+#define  PCIE_OUTB_ERR_ACC_INFO_DIR_WRITE_MASK 0x10
+#define  PCIE_OUTB_ERR_ACC_INFO_BYTE_LANES_MASK0xff00
+#define PCIE_OUTB_ERR_ACC_ADDR 0x6010
+#define PCIE_OUTB_ERR_ACC_ADDR_BUS_MASK0xff0
+#define PCIE_OUTB_ERR_ACC_ADDR_DEV_MASK0xf8000
+#define PCIE_OUTB_ERR_ACC_ADDR_FUNC_MASK   0x7000
+#define PCIE_OUTB_ERR_ACC_ADDR_REG_MASK0xfff
+#define PCIE_OUTB_ERR_CFG_CAUSE0x6014
+#define  PCIE_OUTB_ERR_CFG_CAUSE_TIMEOUT_MASK  0x40
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ABORT_MASK0x20
+#define  PCIE_OUTB_ERR_CFG_CAUSE_UNSUPP_REQ_MASK   0x10
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_TIMEOUT_MASK  0x4
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_DISABLED_MASK 0x2
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_64BIT__MASK   0x1
+#define PCIE_OUTB_ERR_MEM_ADDR_LO  0x6018
+#define PCIE_OUTB_ERR_MEM_ADDR_HI  0x601c
+#define PCIE_OUTB_ERR_MEM_CAUSE0x6020
+#define  PCIE_OUTB_ERR_MEM_CAUSE_TIMEOUT_MASK  0x40
+#define  PCIE_OUTB_ERR_MEM_CAUSE_ABORT_MASK0x20
+#define  PCIE_OUTB_ERR_MEM_CAUSE_UNSUPP_REQ_MASK   0x10
+#define  PCIE_OUTB_ERR_MEM_CAUSE_ACC_DISABLED_MASK 0x2
+#define  PCIE_OUTB_ERR_MEM_CAUSE_BAD_ADDR_MASK 0x1
+
 /* Forward declarations */
 struct brcm_pcie;
 static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, 
u32 val);
@@ -223,6 +258,7 @@ struct pcie_cfg_data {
const enum pcie_type type;
void (*perst_set)(struct brcm_pcie *pcie, u32 val);
void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+   const bool has_err_report;
 };
 
 static const int pcie_offsets[] = {
@@ -270,6 +306,7 @@ static const struct pcie_cfg_data bcm7216_cfg = {
.type   = BCM7278,
.perst_set  = brcm_pcie_perst_set_7278,
.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
+   .has_err_report = true,
 };
 
 struct brcm_msi {
@@ -313,8 +350,87 @@ struct brcm_pcie {
struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
unsigned intnum_supplies;
boolep_wakeup_capable;
+   boolhas_err_report;
+   struct notifier_block   die_notifier;
 };
 
+/* Dump out PCIe errors on die or panic */
+static int dump_pcie_error(struct notifier_block *self, unsigned long v, void 
*p)
+{
+   const struct brcm_pcie *pcie = container_of(self, struct brcm_pcie, 
die_notifier);
+   void __iomem *base = pcie->base;
+   int i, is_cfg_err, is_mem_err, lanes;
+   char *width_str, *direction_str, lanes_str[9];
+   u32 info;
+
+   if (readl(base + PCIE_OUTB_ERR_VALID) == 0)
+   return NOTIFY_DONE;
+   info = readl(base + PCIE_OUTB_ERR_ACC_INFO);
+
+
+   is_cfg_err = !!(info & PCIE_OUTB_ERR_ACC_INFO_CFG_ERR_MASK);
+   is_mem_err = !!(info & PCIE_OUTB_ERR_ACC_INFO_MEM_ERR_MASK);
+   width_str = (info & PCIE_OUTB_ERR_ACC_INFO_TYPE_64_MASK) ? "64bit" : 
"32bit";
+   direction_str = (info & PCIE_OUTB_ERR_ACC_INFO_DIR_WRITE_M

[PATCH v4 4/6] PCI: brcmstb: Do not turn off regulators if EP can wake up

[Jim Quinlan]

If any downstream device may wake up during S2/S3 suspend, we do not want
to turn off its power when suspending.

Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 58 +++
 1 file changed, 51 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index 1b0de0c7da60..4c79aff66de7 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -193,6 +193,7 @@ static inline void 
brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie,
 static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 
val);
+static bool brcm_pcie_link_up(struct brcm_pcie *pcie);
 
 static const char * const supplies[] = {
"vpcie12v-supply",
@@ -304,22 +305,65 @@ struct brcm_pcie {
void(*bridge_sw_init_set)(struct brcm_pcie *pcie, 
u32 val);
struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
unsigned intnum_supplies;
+   boolep_wakeup_capable;
 };
 
-static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
 {
+   bool *ret = data;
+
+   if (device_may_wakeup(>dev)) {
+   *ret = true;
+   dev_dbg(>dev, "disable cancelled for wake-up device\n");
+   }
+   return (int) *ret;
+}
+
+enum {
+   TURN_OFF,   /* Turn regulators off, unless an EP is 
wakeup-capable */
+   TURN_OFF_ALWAYS,/* Turn regulators off, no exceptions */
+   TURN_ON,/* Turn regulators on, unless 
pcie->ep_wakeup_capable */
+};
+
+static int brcm_set_regulators(struct brcm_pcie *pcie, int how)
+{
+   struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
struct device *dev = pcie->dev;
int ret;
 
if (!pcie->num_supplies)
return 0;
-   if (on)
+   if (how == TURN_ON) {
+   if (pcie->ep_wakeup_capable) {
+   /*
+* We are resuming from a suspend.  In the
+* previous suspend we did not disable the power
+* supplies, so there is no need to enable them
+* (and falsely increase their usage count).
+*/
+   pcie->ep_wakeup_capable = false;
+   return 0;
+   }
+   } else if (how == TURN_OFF) {
+   /*
+* If at least one device on this bus is enabled as a
+* wake-up source, do not turn off regulators.
+*/
+   pcie->ep_wakeup_capable = false;
+   if (bridge->bus && brcm_pcie_link_up(pcie)) {
+   pci_walk_bus(bridge->bus, pci_dev_may_wakeup, 
>ep_wakeup_capable);
+   if (pcie->ep_wakeup_capable)
+   return 0;
+   }
+   }
+
+   if (how == TURN_ON)
ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
else
ret = regulator_bulk_disable(pcie->num_supplies, 
pcie->supplies);
if (ret)
dev_err(dev, "failed to %s EP regulators\n",
-   on ? "enable" : "disable");
+   how == TURN_ON ? "enable" : "disable");
return ret;
 }
 
@@ -1206,7 +1250,7 @@ static int brcm_pcie_suspend(struct device *dev)
brcm_phy_stop(pcie);
clk_disable_unprepare(pcie->clk);
 
-   return brcm_set_regulators(pcie, false);
+   return brcm_set_regulators(pcie, TURN_OFF);
 }
 
 static int brcm_pcie_resume(struct device *dev)
@@ -1221,7 +1265,7 @@ static int brcm_pcie_resume(struct device *dev)
if (ret)
return ret;
 
-   ret = brcm_set_regulators(pcie, true);
+   ret = brcm_set_regulators(pcie, TURN_ON);
if (ret)
return ret;
 
@@ -1261,7 +1305,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
brcm_phy_stop(pcie);
reset_control_assert(pcie->rescal);
clk_disable_unprepare(pcie->clk);
-   brcm_set_regulators(pcie, false);
+   brcm_set_regulators(pcie, TURN_OFF_ALWAYS);
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
@@ -1360,7 +1404,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
goto fail;
}
 
-   ret = brcm_set_regulators(pcie, true);
+   ret = brcm_set_regulators(pcie, TURN_ON);
if (ret)
goto fail;
 
-- 
2.17.1

[PATCH v4 0/6] PCI: brcmstb: add slot0 device regulators and panic handler

[Jim Quinlan]

v4 [NOTE: I'm not sure this fixes RobH and MarkB constraints but I'd
  like to use this pullreq as a basis for future discussion.]
   [Commit: Add bindings for ...]
 -- Fix syntax error in YAML bindings example (RobH)
 -- {vpcie12v,vpcie3v3}-supply props are back in root complex DT node
(I believe RobH said this was okay)
   [Commit: Add control of ..]
 -- Do not do global search for regulator; now we look specifically
for the property {vpcie12v,vpcie3v3}-supply in the root complex
DT node and then call devm_regulator_bulk_get() (MarkB)
 -- Use devm_regulator_bulk_get() (Bjorn)
 -- s/EP/slot0 device/ (Bjorn)
 -- Spelling, capitalization (Bjorn)
 -- Have brcm_phy_stop() return a void (Bjorn)
   [Commit: Do not turn off ...]
 -- Capitalization (Bjorn)
   [Commit: Check return value ...]
 -- Commit message content (Bjorn)
 -- Move 6/6 hunk to 2/6 where it belongs (Bjorn)
 -- Move the rest of 6/6 before all other commits (Bjorn)

v3 -- Driver now searches for EP DT subnode for any regulators to turn on.
  If present, these regulators have the property names
  "vpcie12v-supply" and "vpcie3v3-supply".  The existence of these
  regulators in the EP subnode are currently pending as a pullreq
  in pci-bus.yaml at
  https://github.com/devicetree-org/dt-schema/pull/54
  (MarkB, RobH).
   -- Check return of brcm_set_regulators() (Florian)
   -- Specify one regulator string per line for easier update (Florian)
   -- Author/Committer/Signoff email changed from that of V2 from
  'james.quin...@broadcom.com' to 'jim2101...@gmail.com'.

v2 -- Use regulator bulk API rather than multiple calls (MarkB).

v1 -- Bindings are added for fixed regulators that may power the EP device.
   -- The brcmstb RC driver is modified to control these regulators
  during probe, suspend, and resume.
   -- 7216 type SOCs have additional error reporting HW and a
  panic handler is added to dump its info.
   -- A missing return value check is added.



Jim Quinlan (6):
  PCI: brcmstb: Check return value of clk_prepare_enable()
  dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage
regulators
  PCI: brcmstb: Add control of slot0 device voltage regulators
  PCI: brcmstb: Do not turn off regulators if EP can wake up
  PCI: brcmstb: Give 7216 SOCs their own config type
  PCI: brcmstb: Add panic/die handler to RC driver

 .../bindings/pci/brcm,stb-pcie.yaml   |   4 +
 drivers/pci/controller/pcie-brcmstb.c | 262 +-
 2 files changed, 259 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Jim Quinlan]

Similar to the regulator bindings found in "rockchip-pcie-host.txt", this
allows optional regulators to be attached and controlled by the PCIe RC
driver.  That being said, this driver searches in the DT subnode (the EP
node, eg pci@0,0) for the regulator property.

The use of a regulator property in the pcie EP subnode such as
"vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml
file at

https://github.com/devicetree-org/dt-schema/pull/54

Signed-off-by: Jim Quinlan 
---
 Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 4 
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml 
b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index f90557f6deb8..f2caa5b3b281 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -64,6 +64,9 @@ properties:
 
   aspm-no-l0s: true
 
+  vpcie12v-supply: true
+  vpcie3v3-supply: true
+
   brcm,scb-sizes:
 description: u64 giving the 64bit PCIe memory
   viewport size of a memory controller.  There may be up to
@@ -156,5 +159,6 @@ examples:
  <0x4200 0x1 0x8000 0x3 0x 0x0 
0x8000>;
 brcm,enable-ssc;
 brcm,scb-sizes =  <0x8000 0x8000>;
+vpcie12v-supply = <>;
 };
 };
-- 
2.17.1

[PATCH v4 1/6] PCI: brcmstb: Check return value of clk_prepare_enable()

[Jim Quinlan]

Check for failure of clk_prepare_enable() on device resume.

Signed-off-by: Jim Quinlan 
Acked-by: Florian Fainelli 
Fixes: 8195b7417018 ("PCI: brcmstb: Add suspend and resume pm_ops")
---
 drivers/pci/controller/pcie-brcmstb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index e330e6811f0b..4ce1f3a60574 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1161,7 +1161,9 @@ static int brcm_pcie_resume(struct device *dev)
int ret;
 
base = pcie->base;
-   clk_prepare_enable(pcie->clk);
+   ret = clk_prepare_enable(pcie->clk);
+   if (ret)
+   return ret;
 
ret = brcm_phy_start(pcie);
if (ret)
-- 
2.17.1

Re: [PATCH v3 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Jim Quinlan]

On Tue, Mar 30, 2021 at 11:30 AM Mark Brown  wrote:
>10.22.8.121
> On Tue, Mar 30, 2021 at 10:08:16AM -0500, Rob Herring wrote:
> > On Fri, Mar 26, 2021 at 03:18:59PM -0400, Jim Quinlan wrote:
>
> > > +pcie-ep@0,0 {
> > > +reg = <0x0 0x0 0x0 0x0 0x0>;
> > > +compatible = "pci14e4,1688";
> > > +vpcie12v-supply: <>;
>
> > For other cases, these properties are in the host bridge node. If these
> > are standard PCI rails, then I think that's where they belong unless 
> > we10.22.8.121
> > define slot nodes.
>
> For a soldered down part I'd expect we'd want both (if the host even
> cares) - for anything except a supply that I/O or something else shared
> is referenced off there's no great reason why it has to be physically
> the same supply going to every device on the bus so each device should
> be able to specify separately.
Our developer and reference boards frequently have Mini and half-mini
PCIe sockets (a few exceptions), whereas production boards are mostly
soldered down.

If I resubmit this pullreq  so that it  looks for "vpcie12v-supply"
and "vpcie3v3-supply" in the host node, will that be acceptable for
both of you?

Thanks,
Jim Quinlan
Broadcom STB

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Jim Quinlan]

/* Pmap_idx to avs pmap number */
const uint8_t pmap_idx_to_avs_id[20];

On Mon, Mar 29, 2021 at 1:16 PM Mark Brown  wrote:
>
> On Mon, Mar 29, 2021 at 12:39:50PM -0400, Jim Quinlan wrote:
> > On Mon, Mar 29, 2021 at 12:25 PM Mark Brown 
>
> > > Here you are figuring out a device local supply name...
>
> > > > + /*
> > > > +  * Get the regulators that the EP devianswerces require.  We 
> > > > cannot use
> > > > +  * pcie->dev as the device argument in regulator_bulk_get() since
> > > > +  * it will not find the regulators.  Instead, use NULL and the
> > > > +  * regulators are looked up by their name.
> > > > +  */
> > > > + return regulator_bulk_get(NULL, pcie->num_supplies, 
> > > > pcie->supplies);
>
> > > ...and here you are trying to look up that device local name in the
> > > global namespace.  That's not going to work well, the global names that
> > > supplies are labelled with may be completely different to what the chip
> > > designer called them and there could easily be naming collisions between
> > > different chips.
>
> > "devm_regulator_bulk_get(pcie->dev, ...)"; is your concern about the
> > NULL for the device and if so does this fix it?  If not, what do you
> > suggest that I do?
>
> If you use the struct device for the PCIe controller then that's going
> to first try the PCIe controller then the global namespace so you still
> have all the same problems.  You really need to use the struct device
> for the device that is being supplied not some random other struct
> device you happened to find in the system.
Hello Mark,
I'm not concerned about a namespace collision and I don't think you
should be concerned either.  First, this driver is for Broadcom STB
PCIe chips and boards, and we also deliver the DT to the customers.
We typically do not have any other regulators in the DT besides the
ones I am proposing.  For example, the 7216 SOC DT has 0 other
regulators -- no namespace collision possible.  Our DT-generating
scripts also flag namespace issues.  I admit that this driver is also
used by RPi chips, but I can easily exclude this feature from the RPI
if Nicolas has any objection.

Further, if you want, I can restrict the search to the two regulators
I am proposing to add to pci-bus.yaml:  "vpcie12v-supply" and
"vpcie3v3-supply".

Is the above enough to alleviate your concerns about global namespace collision?

>
> As I said in my earlier reply I think either the driver core or PCI
> needs something like Soundwire has which lets it create struct devices
> for things that have been enumerated via software but not enumerated by
> hardware and a callback or something which lets those devices take
> whatever steps are needed to trigger probe.

Can you please elaborate this further and in detail?  This sounds like
a decent-size undertaking, and the last time I followed a review
suggestion that was similar in spirit to this, the pullreq was
ironically NAKed by the person who suggested it.  Do you really think
it is necessary to construct such a subsystem just to avoid the  very
remote possibility of a namespace collision which is our (Broadcom
STB) responsibility and consequence to avoid?

Regards,
Jim Quinlan
Broadcom STB

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Jim Quinlan]

On Mon, Mar 29, 2021 at 12:25 PM Mark Brown 
w./lib/python3.6/site-packages/dtschema/schemasrote:
>
> On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote:
>
> > + /* Now look for regulator supply properties */
> > + for_each_property_of_node(child, pp) {
> > + int i, n = strnlen(pp->name, max_name_len);
> > +
> > + if (n <= 7 || strncmp("-supply", >name[n - 7], 7))
> > + continue;
>
> Here you are figuring out a device local supply name...
>
> > + /*
> > +  * Get the regulators that the EP devianswerces require.  We cannot 
> > use
> > +  * pcie->dev as the device argument in regulator_bulk_get() since
> > +  * it will not find the regulators.  Instead, use NULL and the
> > +  * regulators are looked up by their name.
> > +  */
> > + return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
>
> ...and here you are trying to look up that device local name in the
> global namespace.  That's not going to work well, the global names that
> supplies are labelled with may be completely different to what the chip
> designer called them and there could easily be naming collisions between
> different chips.

Hello Mark,
I am re-submitting this pullreq using
"devm_regulator_bulk_get(pcie->dev, ...)"; is your concern about the
NULL for the device and if so does this fix it?  If not, what do you
suggest that I do?
Thanks,
Jim

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Jim Quinlan]

On Fri, Mar 26, 2021 at 4:11 PM Bjorn Helgaas  wrote:
>
> On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote:
> > Control of EP regulators by the RC is needed because of the chicken-and-egg
>
> Can you expand "EP"?  Not sure if this refers to "endpoint" or
> something else.
Yes I meant "endpoint" -- I will expand it.
>
> If this refers to a device in a slot, I guess it isn't necessarily aWe only 
> support this feature for endpoint devices; it they hav
> PCIe *endpoint*; it could also be a switch upstream port.
True; to be precise I mean the device directly connected to the single RC port.
>
> > situation: although the regulator is "owned" by the EP and would be best
> > handled on its driver, the EP cannot be discovered and probed unless its
> > regulator is already turned on.
> >
> > Signed-off-by: Jim Quinlan 
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 90 ++-
> >  1 file changed, 87 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c 
> > b/drivers/pci/controller/pcie-brcmstb.c
> > index e330e6811f0b..b76ec7d9af32 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -24,6 +24,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -169,6 +170,7 @@
> >  #define SSC_STATUS_SSC_MASK  0x400
> >  #define SSC_STATUS_PLL_LOCK_MASK 0x800
> >  #define PCIE_BRCM_MAX_MEMC   3
> > +#define PCIE_BRCM_MAX_EP_REGULATORS  4
> >
> >  #define IDX_ADDR(pcie)   
> > (pcie->reg_offsets[EXT_CFG_INDEX])
> >  #define DATA_ADDR(pcie)  
> > (pcie->reg_offsets[EXT_CFG_DATA])
> > @@ -295,8 +297,27 @@ struct brcm_pcie {
> >   u32 hw_rev;
> >   void(*perst_set)(struct brcm_pcie *pcie, u32 val);
> >   void(*bridge_sw_init_set)(struct brcm_pcie *pcie, 
> > u32 val);
> > + struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
> > + unsigned intnum_supplies;
> >  };
> >
> > +static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
> > +{
> > + struct device *dev = pcie->dev;
> > + int ret;
> > +
> > + if (!pcie->num_supplies)
> > + return 0;
> > + if (on)
> > + ret = regulator_bulk_enable(pcie->num_supplies, 
> > pcie->supplies);
> > + else
> > + ret = regulator_bulk_disable(pcie->num_supplies, 
> > pcie->supplies);
> > + if (ret)
> > + dev_err(dev, "failed to %s EP regulators\n",
> > + on ? "enable" : "disable");
> > + return ret;
> > +}
> > +
> >  /*
> >   * This is to convert the size of the inbound "BAR" region to the
> >   * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
> > @@ -1141,16 +1162,63 @@ static void brcm_pcie_turn_off(struct brcm_pcie 
> > *pcie)
> >   pcie->bridge_sw_init_set(pcie, 1);
> >  }
> >
> > +static int brcm_pcie_get_regulators(struct brcm_pcie *pcie)
> > +{
> > + struct device_node *child, *parent = pcie->np;
> > + const unsigned int max_name_len = 64 + 4;
> > + struct property *pp;
> > +
> > + /* Look for regulator supply property in the EP device subnodes */
> > + for_each_available_child_of_node(parent, child) {
> > + /*
> > +  * Do a santiy test to ensure that this is an EP node
>
> s/santiy/sanity/
>
> > +  * (e.g. node name: "pci-ep@0,0").  The slot number
> > +  * should always be 0 as our controller only has a single
> > +  * port.
> > +  */
> > + const char *p = strstr(child->full_name, "@0");
> > +
> > + if (!p || (p[2] && p[2] != ','))
> > + continue;
> > +
> > + /* Now look for regulator supply properties */
> > + for_each_property_of_node(child, pp) {
> > + int i, n = strnlen(pp->name, max_name_len);
> > +
> > + if (n <= 7 || strncmp("-supply", >name[n - 7], 7))
> > + continue;
> > +
> > + /* Make sure this is not a dup

[PATCH v3 5/6] PCI: brcmstb: Add panic/die handler to RC driver

[Jim Quinlan]

Whereas most PCIe HW returns 0x on illegal accesses and the like,
by default Broadcom's STB PCIe controller effects an abort.  This simple
handler determines if the PCIe controller was the cause of the abort and if
so, prints out diagnostic info.

Example output:
  brcm-pcie 8b2.pcie: Error: Mem Acc: 32bit, Read, @0x3800
  brcm-pcie 8b2.pcie:  Type: TO=0 Abt=0 UnspReq=1 AccDsble=0 BadAddr=0

Signed-off-by: Jim Quinlan 
Acked-by: Florian Fainelli 
---
 drivers/pci/controller/pcie-brcmstb.c | 122 ++
 1 file changed, 122 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index 9c8b922a9c19..2d9288399014 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -12,11 +12,13 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -186,6 +188,39 @@
 #define  PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_MASK0x1
 #define  PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_SHIFT   0x0
 
+/* Error report regiseters */
+#define PCIE_OUTB_ERR_TREAT0x6000
+#define  PCIE_OUTB_ERR_TREAT_CONFIG_MASK   0x1
+#define  PCIE_OUTB_ERR_TREAT_MEM_MASK  0x2
+#define PCIE_OUTB_ERR_VALID0x6004
+#define PCIE_OUTB_ERR_CLEAR0x6008
+#define PCIE_OUTB_ERR_ACC_INFO 0x600c
+#define  PCIE_OUTB_ERR_ACC_INFO_CFG_ERR_MASK   0x01
+#define  PCIE_OUTB_ERR_ACC_INFO_MEM_ERR_MASK   0x02
+#define  PCIE_OUTB_ERR_ACC_INFO_TYPE_64_MASK   0x04
+#define  PCIE_OUTB_ERR_ACC_INFO_DIR_WRITE_MASK 0x10
+#define  PCIE_OUTB_ERR_ACC_INFO_BYTE_LANES_MASK0xff00
+#define PCIE_OUTB_ERR_ACC_ADDR 0x6010
+#define PCIE_OUTB_ERR_ACC_ADDR_BUS_MASK0xff0
+#define PCIE_OUTB_ERR_ACC_ADDR_DEV_MASK0xf8000
+#define PCIE_OUTB_ERR_ACC_ADDR_FUNC_MASK   0x7000
+#define PCIE_OUTB_ERR_ACC_ADDR_REG_MASK0xfff
+#define PCIE_OUTB_ERR_CFG_CAUSE0x6014
+#define  PCIE_OUTB_ERR_CFG_CAUSE_TIMEOUT_MASK  0x40
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ABORT_MASK0x20
+#define  PCIE_OUTB_ERR_CFG_CAUSE_UNSUPP_REQ_MASK   0x10
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_TIMEOUT_MASK  0x4
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_DISABLED_MASK 0x2
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_64BIT__MASK   0x1
+#define PCIE_OUTB_ERR_MEM_ADDR_LO  0x6018
+#define PCIE_OUTB_ERR_MEM_ADDR_HI  0x601c
+#define PCIE_OUTB_ERR_MEM_CAUSE0x6020
+#define  PCIE_OUTB_ERR_MEM_CAUSE_TIMEOUT_MASK  0x40
+#define  PCIE_OUTB_ERR_MEM_CAUSE_ABORT_MASK0x20
+#define  PCIE_OUTB_ERR_MEM_CAUSE_UNSUPP_REQ_MASK   0x10
+#define  PCIE_OUTB_ERR_MEM_CAUSE_ACC_DISABLED_MASK 0x2
+#define  PCIE_OUTB_ERR_MEM_CAUSE_BAD_ADDR_MASK 0x1
+
 /* Forward declarations */
 struct brcm_pcie;
 static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, 
u32 val);
@@ -218,6 +253,7 @@ struct pcie_cfg_data {
const enum pcie_type type;
void (*perst_set)(struct brcm_pcie *pcie, u32 val);
void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+   const bool has_err_report;
 };
 
 static const int pcie_offsets[] = {
@@ -265,6 +301,7 @@ static const struct pcie_cfg_data bcm7216_cfg = {
.type   = BCM7278,
.perst_set  = brcm_pcie_perst_set_7278,
.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
+   .has_err_report = true,
 };
 
 struct brcm_msi {
@@ -308,8 +345,87 @@ struct brcm_pcie {
struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
unsigned intnum_supplies;
boolep_wakeup_capable;
+   boolhas_err_report;
+   struct notifier_block   die_notifier;
 };
 
+/* Dump out PCIe errors on die or panic */
+static int dump_pcie_error(struct notifier_block *self, unsigned long v, void 
*p)
+{
+   const struct brcm_pcie *pcie = container_of(self, struct brcm_pcie, 
die_notifier);
+   void __iomem *base = pcie->base;
+   int i, is_cfg_err, is_mem_err, lanes;
+   char *width_str, *direction_str, lanes_str[9];
+   u32 info;
+
+   if (readl(base + PCIE_OUTB_ERR_VALID) == 0)
+   return NOTIFY_DONE;
+   info = readl(base + PCIE_OUTB_ERR_ACC_INFO);
+
+
+   is_cfg_err = !!(info & PCIE_OUTB_ERR_ACC_INFO_CFG_ERR_MASK);
+   is_mem_err = !!(info & PCIE_OUTB_ERR_ACC_INFO_MEM_ERR_MASK);
+   width_str = (info & PCIE_OUTB_ERR_ACC_INFO_TYPE_64_MASK) ? "64bit" : 
"32bit";
+   direction_str = (info & PCIE_OUTB_ERR_ACC_INFO_DIR_WRITE_M

[PATCH v3 3/6] PCI: brcmstb: Do not turn off regulators if EP can wake up

[Jim Quinlan]

If any downstream device may wake up during S2/S3 suspend, we do not want
to turn off its power when suspending.

Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 58 +++
 1 file changed, 51 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index b76ec7d9af32..89745bb6ada6 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -193,6 +193,7 @@ static inline void 
brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie,
 static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 
val);
+static bool brcm_pcie_link_up(struct brcm_pcie *pcie);
 
 enum {
RGR1_SW_INIT_1,
@@ -299,22 +300,65 @@ struct brcm_pcie {
void(*bridge_sw_init_set)(struct brcm_pcie *pcie, 
u32 val);
struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
unsigned intnum_supplies;
+   boolep_wakeup_capable;
 };
 
-static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
 {
+   bool *ret = data;
+
+   if (device_may_wakeup(>dev)) {
+   *ret = true;
+   dev_dbg(>dev, "disable cancelled for wake-up device\n");
+   }
+   return (int) *ret;
+}
+
+enum {
+   TURN_OFF,   /* Turn egulators off, unless an EP is 
wakeup-capable */
+   TURN_OFF_ALWAYS,/* Turn Regulators off, no exceptions */
+   TURN_ON,/* Turn regulators on, unless 
pcie->ep_wakeup_capable */
+};
+
+static int brcm_set_regulators(struct brcm_pcie *pcie, int how)
+{
+   struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
struct device *dev = pcie->dev;
int ret;
 
if (!pcie->num_supplies)
return 0;
-   if (on)
+   if (how == TURN_ON) {
+   if (pcie->ep_wakeup_capable) {
+   /*
+* We are resuming from a suspend.  In the
+* previous suspend we did not disable the power
+* supplies, so there is no need to enable them
+* (and falsely increase their usage count).
+*/
+   pcie->ep_wakeup_capable = false;
+   return 0;
+   }
+   } else if (how == TURN_OFF) {
+   /*
+* If at least one device on this bus is enabled as a
+* wake-up source, do not turn off regulators.
+*/
+   pcie->ep_wakeup_capable = false;
+   if (bridge->bus && brcm_pcie_link_up(pcie)) {
+   pci_walk_bus(bridge->bus, pci_dev_may_wakeup, 
>ep_wakeup_capable);
+   if (pcie->ep_wakeup_capable)
+   return 0;
+   }
+   }
+
+   if (how == TURN_ON)
ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
else
ret = regulator_bulk_disable(pcie->num_supplies, 
pcie->supplies);
if (ret)
dev_err(dev, "failed to %s EP regulators\n",
-   on ? "enable" : "disable");
+   how == TURN_ON ? "enable" : "disable");
return ret;
 }
 
@@ -1218,7 +1262,7 @@ static int brcm_pcie_suspend(struct device *dev)
brcm_phy_stop(pcie);
clk_disable_unprepare(pcie->clk);
 
-   return brcm_set_regulators(pcie, false);
+   return brcm_set_regulators(pcie, TURN_OFF);
 }
 
 static int brcm_pcie_resume(struct device *dev)
@@ -1231,7 +1275,7 @@ static int brcm_pcie_resume(struct device *dev)
base = pcie->base;
clk_prepare_enable(pcie->clk);
 
-   ret = brcm_set_regulators(pcie, true);
+   ret = brcm_set_regulators(pcie, TURN_ON);
if (ret)
return ret;
 
@@ -1271,7 +1315,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
brcm_phy_stop(pcie);
reset_control_assert(pcie->rescal);
clk_disable_unprepare(pcie->clk);
-   brcm_set_regulators(pcie, false);
+   brcm_set_regulators(pcie, TURN_OFF_ALWAYS);
regulator_bulk_free(pcie->num_supplies, pcie->supplies);
 }
 
@@ -1369,7 +1413,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
goto fail;
}
 
-   ret = brcm_set_regulators(pcie, true);
+   ret = brcm_set_regulators(pcie, TURN_ON);
if (ret)
goto fail;
 
-- 
2.17.1

[PATCH v3 4/6] PCI: brcmstb: Give 7216 SOCs their own config type

[Jim Quinlan]

This distinction is required for an imminent commit.

Signed-off-by: Jim Quinlan 
Acked-by: Florian Fainelli 
---
 drivers/pci/controller/pcie-brcmstb.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index 89745bb6ada6..9c8b922a9c19 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -260,6 +260,13 @@ static const struct pcie_cfg_data bcm2711_cfg = {
.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 };
 
+static const struct pcie_cfg_data bcm7216_cfg = {
+   .offsets= pcie_offset_bcm7278,
+   .type   = BCM7278,
+   .perst_set  = brcm_pcie_perst_set_7278,
+   .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
+};
+
 struct brcm_msi {
struct device   *dev;
void __iomem*base;
@@ -1336,7 +1343,7 @@ static const struct of_device_id brcm_pcie_match[] = {
{ .compatible = "brcm,bcm4908-pcie", .data = _cfg },
{ .compatible = "brcm,bcm7211-pcie", .data = _cfg },
{ .compatible = "brcm,bcm7278-pcie", .data = _cfg },
-   { .compatible = "brcm,bcm7216-pcie", .data = _cfg },
+   { .compatible = "brcm,bcm7216-pcie", .data = _cfg },
{ .compatible = "brcm,bcm7445-pcie", .data = _cfg },
{},
 };
-- 
2.17.1

[PATCH v3 6/6] PCI: brcmstb: Check return value of clk_prepare_enable()

[Jim Quinlan]

The check was missing on PCIe resume.

Signed-off-by: Jim Quinlan 
Acked-by: Florian Fainelli 
Fixes: 8195b7417018 ("PCI: brcmstb: Add suspend and resume pm_ops")
---
 drivers/pci/controller/pcie-brcmstb.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index 2d9288399014..f6d9d785b301 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1396,7 +1396,9 @@ static int brcm_pcie_resume(struct device *dev)
int ret;
 
base = pcie->base;
-   clk_prepare_enable(pcie->clk);
+   ret = clk_prepare_enable(pcie->clk);
+   if (ret)
+   return ret;
 
ret = brcm_set_regulators(pcie, TURN_ON);
if (ret)
@@ -1535,7 +1537,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 
ret = brcm_pcie_get_regulators(pcie);
if (ret) {
-   dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
+   pcie->num_supplies = 0;
+   if (ret != -EPROBE_DEFER)
+   dev_err(pcie->dev, "failed to get regulators 
(err=%d)\n", ret);
goto fail;
}
 
-- 
2.17.1

[PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Jim Quinlan]

Control of EP regulators by the RC is needed because of the chicken-and-egg
situation: although the regulator is "owned" by the EP and would be best
handled on its driver, the EP cannot be discovered and probed unless its
regulator is already turned on.

Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 90 ++-
 1 file changed, 87 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index e330e6811f0b..b76ec7d9af32 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -169,6 +170,7 @@
 #define SSC_STATUS_SSC_MASK0x400
 #define SSC_STATUS_PLL_LOCK_MASK   0x800
 #define PCIE_BRCM_MAX_MEMC 3
+#define PCIE_BRCM_MAX_EP_REGULATORS4
 
 #define IDX_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_INDEX])
 #define DATA_ADDR(pcie)
(pcie->reg_offsets[EXT_CFG_DATA])
@@ -295,8 +297,27 @@ struct brcm_pcie {
u32 hw_rev;
void(*perst_set)(struct brcm_pcie *pcie, u32 val);
void(*bridge_sw_init_set)(struct brcm_pcie *pcie, 
u32 val);
+   struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
+   unsigned intnum_supplies;
 };
 
+static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+{
+   struct device *dev = pcie->dev;
+   int ret;
+
+   if (!pcie->num_supplies)
+   return 0;
+   if (on)
+   ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
+   else
+   ret = regulator_bulk_disable(pcie->num_supplies, 
pcie->supplies);
+   if (ret)
+   dev_err(dev, "failed to %s EP regulators\n",
+   on ? "enable" : "disable");
+   return ret;
+}
+
 /*
  * This is to convert the size of the inbound "BAR" region to the
  * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
@@ -1141,16 +1162,63 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
pcie->bridge_sw_init_set(pcie, 1);
 }
 
+static int brcm_pcie_get_regulators(struct brcm_pcie *pcie)
+{
+   struct device_node *child, *parent = pcie->np;
+   const unsigned int max_name_len = 64 + 4;
+   struct property *pp;
+
+   /* Look for regulator supply property in the EP device subnodes */
+   for_each_available_child_of_node(parent, child) {
+   /*
+* Do a santiy test to ensure that this is an EP node
+* (e.g. node name: "pci-ep@0,0").  The slot number
+* should always be 0 as our controller only has a single
+* port.
+*/
+   const char *p = strstr(child->full_name, "@0");
+
+   if (!p || (p[2] && p[2] != ','))
+   continue;
+
+   /* Now look for regulator supply properties */
+   for_each_property_of_node(child, pp) {
+   int i, n = strnlen(pp->name, max_name_len);
+
+   if (n <= 7 || strncmp("-supply", >name[n - 7], 7))
+   continue;
+
+   /* Make sure this is not a duplicate */
+   for (i = 0; i < pcie->num_supplies; i++)
+   if (strncmp(pcie->supplies[i].supply,
+   pp->name, max_name_len) == 0)
+   continue;
+
+   if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS)
+   pcie->supplies[pcie->num_supplies++].supply = 
pp->name;
+   else
+   dev_warn(pcie->dev, "No room for EP supply 
%s\n",
+pp->name);
+   }
+   }
+   /*
+* Get the regulators that the EP devices require.  We cannot use
+* pcie->dev as the device argument in regulator_bulk_get() since
+* it will not find the regulators.  Instead, use NULL and the
+* regulators are looked up by their name.
+*/
+   return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
+}
+
 static int brcm_pcie_suspend(struct device *dev)
 {
struct brcm_pcie *pcie = dev_get_drvdata(dev);
-   int ret;
 
brcm_pcie_turn_off(pcie);
-   ret = brcm_phy_stop(pcie);
+   brcm_phy_stop(pcie);
clk_disable_unprepare(pcie->clk);
 
-   return ret;
+   return brcm_set_regulators(pcie, false);
 }
 
 static int brcm_pcie_resume(struct device *dev)
@@ -1163,6 +1231,10 @@ static int brcm_pcie_

[PATCH v3 0/6] PCI: brcmstb: add EP regulators and panic handler

[Jim Quinlan]

v3 -- Driver now searches for EP DT subnode for any regulators to turn on.
  If present, these regulators have the property names
  "vpcie12v-supply" and "vpcie3v3-supply".  The existence of these
  regulators in the EP subnode are currently pending as a pullreq
  in pci-bus.yaml at
  https://github.com/devicetree-org/dt-schema/pull/54
  (MarkB, RobH).
   -- Check return of brcm_set_regulators() (Florian)
   -- Specify one regulator string per line for easier update (Florian)
   -- Author/Committer/Signoff email changed from that of V2 from
  'james.quin...@broadcom.com' to 'jim2101...@gmail.com'.

v2 -- Use regulator bulk API rather than multiple calls (MarkB).

v1 -- Bindings are added for fixed regulators that may power the EP device.
   -- The brcmstb RC driver is modified to control these regulators
  during probe, suspend, and resume.
   -- 7216 type SOCs have additional error reporting HW and a
  panic handler is added to dump its info.
   -- A missing return value check is added.


Jim Quinlan (6):
  dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators
  PCI: brcmstb: Add control of EP voltage regulators
  PCI: brcmstb: Do not turn off regulators if EP can wake up
  PCI: brcmstb: Give 7216 SOCs their own config type
  PCI: brcmstb: Add panic/die handler to RC driver
  PCI: brcmstb: Check return value of clk_prepare_enable()

 .../bindings/pci/brcm,stb-pcie.yaml   |   6 +
 drivers/pci/controller/pcie-brcmstb.c | 271 +-
 2 files changed, 272 insertions(+), 5 deletions(-)

-- 
2.17.1

[PATCH v3 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Jim Quinlan]

Similar to the regulator bindings found in "rockchip-pcie-host.txt", this
allows optional regulators to be attached and controlled by the PCIe RC
driver.  That being said, this driver searches in the DT subnode (the EP
node, eg pci@0,0) for the regulator property.

The use of a regulator property in the pcie EP subnode such as
"vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml
file at

https://github.com/devicetree-org/dt-schema/pull/54

Signed-off-by: Jim Quinlan 
---
 Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml 
b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index f90557f6deb8..ea3e6f55e365 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -156,5 +156,11 @@ examples:
  <0x4200 0x1 0x8000 0x3 0x 0x0 
0x8000>;
 brcm,enable-ssc;
 brcm,scb-sizes =  <0x8000 0x8000>;
+
+pcie-ep@0,0 {
+reg = <0x0 0x0 0x0 0x0 0x0>;
+compatible = "pci14e4,1688";
+vpcie12v-supply: <>;
+};
 };
 };
-- 
2.17.1

[PATCH v5 2/2] PCI: brcmstb: Use reset/rearm instead of deassert/assert

[Jim Quinlan]

The Broadcom STB PCIe RC uses a reset control "rescal" for certain chips.
The "rescal" implements a "pulse reset" so using assert/deassert is wrong
for this device.  Instead, we use reset/rearm.  We need to use rearm so
that we can reset it after a suspend/resume cycle; w/o using "rearm", the
"rescal" device will only ever fire once.

Of course for suspend/resume to work we also need to put the reset/rearm
calls in the suspend and resume routines.

Fixes: 740d6c3708a9 ("PCI: brcmstb: Add control of rescal reset")
Signed-off-by: Jim Quinlan 
Acked-by: Florian Fainelli 
---
 drivers/pci/controller/pcie-brcmstb.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index e330e6811f0b..3b35d629035e 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1148,6 +1148,7 @@ static int brcm_pcie_suspend(struct device *dev)
 
brcm_pcie_turn_off(pcie);
ret = brcm_phy_stop(pcie);
+   reset_control_rearm(pcie->rescal);
clk_disable_unprepare(pcie->clk);
 
return ret;
@@ -1163,9 +1164,13 @@ static int brcm_pcie_resume(struct device *dev)
base = pcie->base;
clk_prepare_enable(pcie->clk);
 
+   ret = reset_control_reset(pcie->rescal);
+   if (ret)
+   goto err_disable_clk;
+
ret = brcm_phy_start(pcie);
if (ret)
-   goto err;
+   goto err_reset;
 
/* Take bridge out of reset so we can access the SERDES reg */
pcie->bridge_sw_init_set(pcie, 0);
@@ -1180,14 +1185,16 @@ static int brcm_pcie_resume(struct device *dev)
 
ret = brcm_pcie_setup(pcie);
if (ret)
-   goto err;
+   goto err_reset;
 
if (pcie->msi)
brcm_msi_set_regs(pcie->msi);
 
return 0;
 
-err:
+err_reset:
+   reset_control_rearm(pcie->rescal);
+err_disable_clk:
clk_disable_unprepare(pcie->clk);
return ret;
 }
@@ -1197,7 +1204,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
brcm_msi_remove(pcie);
brcm_pcie_turn_off(pcie);
brcm_phy_stop(pcie);
-   reset_control_assert(pcie->rescal);
+   reset_control_rearm(pcie->rescal);
clk_disable_unprepare(pcie->clk);
 }
 
@@ -1278,13 +1285,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
return PTR_ERR(pcie->perst_reset);
}
 
-   ret = reset_control_deassert(pcie->rescal);
+   ret = reset_control_reset(pcie->rescal);
if (ret)
dev_err(>dev, "failed to deassert 'rescal'\n");
 
ret = brcm_phy_start(pcie);
if (ret) {
-   reset_control_assert(pcie->rescal);
+   reset_control_rearm(pcie->rescal);
clk_disable_unprepare(pcie->clk);
return ret;
}
-- 
2.17.1

[PATCH v5 1/2] ata: ahci_brcm: Fix use of BCM7216 reset controller

[Jim Quinlan]

This driver may use one of two resets controllers.  Keep them in separate
variables to keep things simple.  The reset controller "rescal" is shared
between the AHCI driver and the PCIe driver for the BrcmSTB 7216 chip.  Use
devm_reset_control_get_optional_shared() to handle this sharing.

Fixes: 272ecd60a636 ("ata: ahci_brcm: BCM7216 reset is self de-asserting")
Fixes: c345ec6a50e9 ("ata: ahci_brcm: Support BCM7216 reset controller name")
Signed-off-by: Jim Quinlan 
Acked-by: Florian Fainelli 
---
 drivers/ata/ahci_brcm.c | 46 -
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 5b32df5d33ad..6e9c5ade4c2e 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -86,7 +86,8 @@ struct brcm_ahci_priv {
u32 port_mask;
u32 quirks;
enum brcm_ahci_version version;
-   struct reset_control *rcdev;
+   struct reset_control *rcdev_rescal;
+   struct reset_control *rcdev_ahci;
 };
 
 static inline u32 brcm_sata_readreg(void __iomem *addr)
@@ -352,8 +353,8 @@ static int brcm_ahci_suspend(struct device *dev)
else
ret = 0;
 
-   if (priv->version != BRCM_SATA_BCM7216)
-   reset_control_assert(priv->rcdev);
+   reset_control_assert(priv->rcdev_ahci);
+   reset_control_rearm(priv->rcdev_rescal);
 
return ret;
 }
@@ -365,10 +366,10 @@ static int __maybe_unused brcm_ahci_resume(struct device 
*dev)
struct brcm_ahci_priv *priv = hpriv->plat_data;
int ret = 0;
 
-   if (priv->version == BRCM_SATA_BCM7216)
-   ret = reset_control_reset(priv->rcdev);
-   else
-   ret = reset_control_deassert(priv->rcdev);
+   ret = reset_control_deassert(priv->rcdev_ahci);
+   if (ret)
+   return ret;
+   ret = reset_control_reset(priv->rcdev_rescal);
if (ret)
return ret;
 
@@ -434,7 +435,6 @@ static int brcm_ahci_probe(struct platform_device *pdev)
 {
const struct of_device_id *of_id;
struct device *dev = >dev;
-   const char *reset_name = NULL;
struct brcm_ahci_priv *priv;
struct ahci_host_priv *hpriv;
struct resource *res;
@@ -456,15 +456,15 @@ static int brcm_ahci_probe(struct platform_device *pdev)
if (IS_ERR(priv->top_ctrl))
return PTR_ERR(priv->top_ctrl);
 
-   /* Reset is optional depending on platform and named differently */
-   if (priv->version == BRCM_SATA_BCM7216)
-   reset_name = "rescal";
-   else
-   reset_name = "ahci";
-
-   priv->rcdev = devm_reset_control_get_optional(>dev, reset_name);
-   if (IS_ERR(priv->rcdev))
-   return PTR_ERR(priv->rcdev);
+   if (priv->version == BRCM_SATA_BCM7216) {
+   priv->rcdev_rescal = devm_reset_control_get_optional_shared(
+   >dev, "rescal");
+   if (IS_ERR(priv->rcdev_rescal))
+   return PTR_ERR(priv->rcdev_rescal);
+   }
+   priv->rcdev_ahci = devm_reset_control_get_optional(>dev, "ahci");
+   if (IS_ERR(priv->rcdev_ahci))
+   return PTR_ERR(priv->rcdev_ahci);
 
hpriv = ahci_platform_get_resources(pdev, 0);
if (IS_ERR(hpriv))
@@ -485,10 +485,10 @@ static int brcm_ahci_probe(struct platform_device *pdev)
break;
}
 
-   if (priv->version == BRCM_SATA_BCM7216)
-   ret = reset_control_reset(priv->rcdev);
-   else
-   ret = reset_control_deassert(priv->rcdev);
+   ret = reset_control_reset(priv->rcdev_rescal);
+   if (ret)
+   return ret;
+   ret = reset_control_deassert(priv->rcdev_ahci);
if (ret)
return ret;
 
@@ -539,8 +539,8 @@ static int brcm_ahci_probe(struct platform_device *pdev)
 out_disable_clks:
ahci_platform_disable_clks(hpriv);
 out_reset:
-   if (priv->version != BRCM_SATA_BCM7216)
-   reset_control_assert(priv->rcdev);
+   reset_control_assert(priv->rcdev_ahci);
+   reset_control_rearm(priv->rcdev_rescal);
return ret;
 }
 
-- 
2.17.1

[PATCH v5 0/2] ata: ahci_brcm: Fix use of BCM7216 reset controller

[Jim Quinlan]

v5 -- Improved (I hope) commit description (Bjorn).
   -- Rnamed error labels (Krzyszt).
   -- Fixed typos.

v4 -- does not rely on a pending commit, unlike v3.

v3 -- discard commit from v2; instead rely on the new function
  reset_control_rearm provided in a recent commit [1] applied
  to reset/next.
   -- New commit to correct pcie-brcmstb.c usage of a reset controller
  to use reset/rearm verses deassert/assert.

v2 -- refactor rescal-reset driver to implement assert/deassert rather than
  reset because the reset call only fires once per lifetime and we need
  to reset after every resume from S2 or S3.
   -- Split the use of "ahci" and "rescal" controllers in separate fields
  to keep things simple.

v1 -- original

Jim Quinlan (2):
  ata: ahci_brcm: Fix use of BCM7216 reset controller
  PCI: brcmstb: Use reset/rearm instead of deassert/assert

 drivers/ata/ahci_brcm.c   | 46 +--
 drivers/pci/controller/pcie-brcmstb.c | 19 +++
 2 files changed, 36 insertions(+), 29 deletions(-)

-- 
2.17.1

[PATCH v4 2/2] nPCI: brcmstb: Use reset/rearm instead of deassert/assert

[Jim Quinlan]

The Brcmstb PCIe RC uses a reset control "rescal" for certain chips.  This
reset implements a "pulse reset" so it matches more the reset/rearm
calls instead of the deassert/assert calls.

Also, add reset_control calls in suspend/resume functions.

Fixes: 740d6c3708a9 ("PCI: brcmstb: Add control of rescal reset")
Signed-off-by: Jim Quinlan 
Acked-by: Florian Fainelli 
---
 drivers/pci/controller/pcie-brcmstb.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index e330e6811f0b..18f23cba7e3a 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1148,6 +1148,7 @@ static int brcm_pcie_suspend(struct device *dev)
 
brcm_pcie_turn_off(pcie);
ret = brcm_phy_stop(pcie);
+   reset_control_rearm(pcie->rescal);
clk_disable_unprepare(pcie->clk);
 
return ret;
@@ -1163,9 +1164,13 @@ static int brcm_pcie_resume(struct device *dev)
base = pcie->base;
clk_prepare_enable(pcie->clk);
 
+   ret = reset_control_reset(pcie->rescal);
+   if (ret)
+   goto err0;
+
ret = brcm_phy_start(pcie);
if (ret)
-   goto err;
+   goto err1;
 
/* Take bridge out of reset so we can access the SERDES reg */
pcie->bridge_sw_init_set(pcie, 0);
@@ -1180,14 +1185,16 @@ static int brcm_pcie_resume(struct device *dev)
 
ret = brcm_pcie_setup(pcie);
if (ret)
-   goto err;
+   goto err1;
 
if (pcie->msi)
brcm_msi_set_regs(pcie->msi);
 
return 0;
 
-err:
+err1:
+   reset_control_rearm(pcie->rescal);
+err0:
clk_disable_unprepare(pcie->clk);
return ret;
 }
@@ -1197,7 +1204,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
brcm_msi_remove(pcie);
brcm_pcie_turn_off(pcie);
brcm_phy_stop(pcie);
-   reset_control_assert(pcie->rescal);
+   reset_control_rearm(pcie->rescal);
clk_disable_unprepare(pcie->clk);
 }
 
@@ -1278,13 +1285,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
return PTR_ERR(pcie->perst_reset);
}
 
-   ret = reset_control_deassert(pcie->rescal);
+   ret = reset_control_reset(pcie->rescal);
if (ret)
dev_err(>dev, "failed to deassert 'rescal'\n");
 
ret = brcm_phy_start(pcie);
if (ret) {
-   reset_control_assert(pcie->rescal);
+   reset_control_rearm(pcie->rescal);
clk_disable_unprepare(pcie->clk);
return ret;
}
-- 
2.17.1

[PATCH v4 1/2] ata: ahci_brcm: Fix use of BCM7216 reset controller

[Jim Quinlan]

This driver may use one of two resets controllers.  Keep them in separate
variables to keep things simple.  The reset controller "rescal" is shared
between the AHCI driver and the PCIe driver for the BrcmSTB 7216 chip.  Use
devm_reset_control_get_optional_shared() to handle this sharing.

Fixes: 272ecd60a636 ("ata: ahci_brcm: BCM7216 reset is self de-asserting")
Fixes: c345ec6a50e9 ("ata: ahci_brcm: Support BCM7216 reset controller name")
Signed-off-by: Jim Quinlan 
Acked-by: Florian Fainelli 
---
 drivers/ata/ahci_brcm.c | 46 -
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 5b32df5d33ad..6e9c5ade4c2e 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -86,7 +86,8 @@ struct brcm_ahci_priv {
u32 port_mask;
u32 quirks;
enum brcm_ahci_version version;
-   struct reset_control *rcdev;
+   struct reset_control *rcdev_rescal;
+   struct reset_control *rcdev_ahci;
 };
 
 static inline u32 brcm_sata_readreg(void __iomem *addr)
@@ -352,8 +353,8 @@ static int brcm_ahci_suspend(struct device *dev)
else
ret = 0;
 
-   if (priv->version != BRCM_SATA_BCM7216)
-   reset_control_assert(priv->rcdev);
+   reset_control_assert(priv->rcdev_ahci);
+   reset_control_rearm(priv->rcdev_rescal);
 
return ret;
 }
@@ -365,10 +366,10 @@ static int __maybe_unused brcm_ahci_resume(struct device 
*dev)
struct brcm_ahci_priv *priv = hpriv->plat_data;
int ret = 0;
 
-   if (priv->version == BRCM_SATA_BCM7216)
-   ret = reset_control_reset(priv->rcdev);
-   else
-   ret = reset_control_deassert(priv->rcdev);
+   ret = reset_control_deassert(priv->rcdev_ahci);
+   if (ret)
+   return ret;
+   ret = reset_control_reset(priv->rcdev_rescal);
if (ret)
return ret;
 
@@ -434,7 +435,6 @@ static int brcm_ahci_probe(struct platform_device *pdev)
 {
const struct of_device_id *of_id;
struct device *dev = >dev;
-   const char *reset_name = NULL;
struct brcm_ahci_priv *priv;
struct ahci_host_priv *hpriv;
struct resource *res;
@@ -456,15 +456,15 @@ static int brcm_ahci_probe(struct platform_device *pdev)
if (IS_ERR(priv->top_ctrl))
return PTR_ERR(priv->top_ctrl);
 
-   /* Reset is optional depending on platform and named differently */
-   if (priv->version == BRCM_SATA_BCM7216)
-   reset_name = "rescal";
-   else
-   reset_name = "ahci";
-
-   priv->rcdev = devm_reset_control_get_optional(>dev, reset_name);
-   if (IS_ERR(priv->rcdev))
-   return PTR_ERR(priv->rcdev);
+   if (priv->version == BRCM_SATA_BCM7216) {
+   priv->rcdev_rescal = devm_reset_control_get_optional_shared(
+   >dev, "rescal");
+   if (IS_ERR(priv->rcdev_rescal))
+   return PTR_ERR(priv->rcdev_rescal);
+   }
+   priv->rcdev_ahci = devm_reset_control_get_optional(>dev, "ahci");
+   if (IS_ERR(priv->rcdev_ahci))
+   return PTR_ERR(priv->rcdev_ahci);
 
hpriv = ahci_platform_get_resources(pdev, 0);
if (IS_ERR(hpriv))
@@ -485,10 +485,10 @@ static int brcm_ahci_probe(struct platform_device *pdev)
break;
}
 
-   if (priv->version == BRCM_SATA_BCM7216)
-   ret = reset_control_reset(priv->rcdev);
-   else
-   ret = reset_control_deassert(priv->rcdev);
+   ret = reset_control_reset(priv->rcdev_rescal);
+   if (ret)
+   return ret;
+   ret = reset_control_deassert(priv->rcdev_ahci);
if (ret)
return ret;
 
@@ -539,8 +539,8 @@ static int brcm_ahci_probe(struct platform_device *pdev)
 out_disable_clks:
ahci_platform_disable_clks(hpriv);
 out_reset:
-   if (priv->version != BRCM_SATA_BCM7216)
-   reset_control_assert(priv->rcdev);
+   reset_control_assert(priv->rcdev_ahci);
+   reset_control_rearm(priv->rcdev_rescal);
return ret;
 }
 
-- 
2.17.1

[PATCH v4 0/2] ata: ahci_brcm: Fix use of BCM7216 reset controller

[Jim Quinlan]

v4 -- does not rely on a pending commit, unlike v3.

v3 -- discard commit from v2; instead rely on the new function
  reset_control_rearm provided in a recent commit [1] applied
  to reset/next.
   -- New commit to correct pcie-brcmstb.c usage of a reset controller
  to use reset/rearm verses deassert/assert.

v2 -- refactor rescal-reset driver to implement assert/deassert rather than
  reset because the reset call only fires once per lifetime and we need
  to reset after every resume from S2 or S3.
   -- Split the use of "ahci" and "rescal" controllers in separate fields
  to keep things simple.

v1 -- original

Jim Quinlan (2):
  ata: ahci_brcm: Fix use of BCM7216 reset controller
  nPCI: brcmstb: Use reset/rearm instead of deassert/assert

 drivers/ata/ahci_brcm.c   | 46 +--
 drivers/pci/controller/pcie-brcmstb.c | 19 +++
 2 files changed, 36 insertions(+), 29 deletions(-)

-- 
2.17.1

Re: [PATCH v2 5/6] PCI: brcmstb: Add panic/die handler to RC driver

[Jim Quinlan]

On Wed, Jan 6, 2021 at 2:42 PM Jim Quinlan  wrote:
>
> -- Forwarded message -
> From: Bjorn Helgaas 
> Date: Wed, Jan 6, 2021 at 2:19 PM
> Subject: Re: [PATCH v2 5/6] PCI: brcmstb: Add panic/die handler to RC driver
> To: Jim Quinlan 
> Cc: , Nicolas Saenz Julienne
> , ,
> , Lorenzo Pieralisi
> , Rob Herring , Bjorn
> Helgaas , Florian Fainelli
> , moderated list:BROADCOM BCM2711/BCM2835 ARM
> ARCHITECTURE , moderated
> list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE
> , open list
> 
>
>
> On Mon, Nov 30, 2020 at 04:11:42PM -0500, Jim Quinlan wrote:
> > Whereas most PCIe HW returns 0x on illegal accesses and the like,
> > by default Broadcom's STB PCIe controller effects an abort.  This simple
> > handler determines if the PCIe controller was the cause of the abort and if
> > so, prints out diagnostic info.
> >
> > Example output:
> >   brcm-pcie 8b2.pcie: Error: Mem Acc: 32bit, Read, @0x3800
> >   brcm-pcie 8b2.pcie:  Type: TO=0 Abt=0 UnspReq=1 AccDsble=0 BadAddr=0
>
> What does this mean for all the other PCI core code that expects
> 0x data returns?  Does it work?  Does it break differently on
> STB than on other platforms?
Hi Bjorn,

Our PCIe HW causes a CPU abort when this happens.  Occasionally a
customer will have a fault handler try to fix up the abort and
continue on, but we recommend solving the root problem.  This commit
just gives us a chance to glean info about the problem.  Our newer
SOCs have a mode that doesn't abort and instead returns 0x.

BTW, can you point me to example files where "PCI core code that
expects  0x data returns" [on bad accesses]?

Regards,
Jim Quinlan
Broadcom STB

>
> > +/*
> > + * Dump out pcie errors on die or panic.
>
> s/pcie/PCIe/
> This could be a single-line comment.
>
> > + */
>

Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

On Wed, Jan 6, 2021 at 4:30 AM Sudeep Holla  wrote:
>
> On Tue, Jan 05, 2021 at 01:32:49PM -0500, Jim Quinlan wrote:
>
> [...]
>
> >
> > I don't think that is the case;  the bottom routine,
> > do_wait_for_common(), decrements the x->done after a completion (which
> > does an increment).  Regardless, I think it is prudent to add the
> > reinit patch you've provided below.
> >
>
> Ah right, but I will add that.
>
> > BTW, one thing I could have done was incorporate the timeout value but
> > I thought that since this is the "fast" call we shouldn't be timing
> > out at all.
> >
>
> Agreed, we can add it later. However it is not related to fast call, it is
> more for protection against failure of delivery of interrupt from the platform
> or any firmware responsible IMO.
>
> > > This electronic communication and the information and any files
> > > transmitted with it, or attached to it, are confidential and are intended
> > > solely for the use of the individual or entity to whom it is addressed and
> > > may contain information that is confidential, legally privileged,
> > > protected by privacy laws, or otherwise restricted from disclosure to
> > > anyone else. If you are not the intended recipient or the person
> > > responsible for delivering the e-mail to the intended recipient, you are
> > > hereby notified that any use, copying, distributing, dissemination,
> > > forwarding, printing, or copying of this e-mail is strictly prohibited. If
> > > you received this e-mail in error, please return the e-mail to the sender,
> > > delete it from your computer, and destroy any printed copy of it.
>
> I am assuming this was unintentional and ignoring. If not disregard my
> response. Otherwise you may need to fix your mail server.
Hi Sudeep,
Yes please ignore the above legalize in my email -- our company is
attaching this text to all outgoing emails.  We are working on a
general solution.
Regards,
Jim Quinlan
Broadcom STB
>
> --
> Regards,
> Sudeep

Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

> From: Sudeep Holla 
> Date: Tue, Jan 5, 2021 at 12:35 PM
> Subject: Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow 
> optional interrupt
> To: Florian Fainelli 
> Cc: Jim Quinlan , Sudeep Holla , 
> , , open 
> list:SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE Mes... 
> , open list 
> 
>
>
> On Tue, Dec 22, 2020 at 07:37:22PM -0800, Florian Fainelli wrote:
> >
> >
> > On 12/22/2020 6:56 AM, Jim Quinlan wrote:
> > > The SMC/HVC SCMI transport is modified to allow the completion of an SCMI
> > > message to be indicated by an interrupt rather than the return of the smc
> > > call.  This accommodates the existing behavior of the BrcmSTB SCMI
> > > "platform" whose SW is already out in the field and cannot be changed.
> > >
> > > Signed-off-by: Jim Quinlan 
> >
> > This looks good to me, just one question below:
> >
> > [snip]
> >
> > > @@ -111,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info 
> > > *cinfo,
> > > shmem_tx_prepare(scmi_info->shmem, xfer);
> > >
> > > arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, );
> > > +   if (scmi_info->irq)
> > > +   wait_for_completion(_info->tx_complete);
> >
> > Do we need this to have a preceding call to reinit_completion()? It does
> > not look like this is going to make any practical difference but there
> > are drivers doing that for correctness.
>
> Why do you think that might not cause any issue ? After first message
> is completed and ISR is executed, the completion flag remains done for
> ever.
Hi Sudeep,

I don't think that is the case;  the bottom routine,
do_wait_for_common(), decrements the x->done after a completion (which
does an increment).  Regardless, I think it is prudent to add the
reinit patch you've provided below.

BTW, one thing I could have done was incorporate the timeout value but
I thought that since this is the "fast" call we shouldn't be timing
out at all.

Thanks much,
Jim Quinlan
Broadcom STB



So practically 2nd message onwards won't block in wait_for_completion
> which means return from smc/hvc is actually completion too which is clearly
> wrong or am I missing something ?
>
> Jim, please confirm either way. If you agree I can add the below snippet,
> no need to repost.
>
> Regards,
> Sudeep
>
> --
> diff --git i/drivers/firmware/arm_scmi/smc.c w/drivers/firmware/arm_scmi/smc.c
> index fd41d436e34b..86eac0831d3c 100644
> --- i/drivers/firmware/arm_scmi/smc.c
> +++ w/drivers/firmware/arm_scmi/smc.c
> @@ -144,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>
> shmem_tx_prepare(scmi_info->shmem, xfer);
>
> +   if (scmi_info->irq)
> +   reinit_completion(_info->tx_complete);
> arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, );
> if (scmi_info->irq)
> wait_for_completion(_info->tx_complete);
>
>
> This electronic communication and the information and any files transmitted 
> with it, or attached to it, are confidential and are intended solely for the 
> use of the individual or entity to whom it is addressed and may contain 
> information that is confidential, legally privileged, protected by privacy 
> laws, or otherwise restricted from disclosure to anyone else. If you are not 
> the intended recipient or the person responsible for delivering the e-mail to 
> the intended recipient, you are hereby notified that any use, copying, 
> distributing, dissemination, forwarding, printing, or copying of this e-mail 
> is strictly prohibited. If you received this e-mail in error, please return 
> the e-mail to the sender, delete it from your computer, and destroy any 
> printed copy of it.

Re: [PATCH v2 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Jim Quinlan]

On Tue, Jan 5, 2021 at 9:01 AM Mark Brown  wrote:
>
> On Mon, Jan 04, 2021 at 05:12:11PM -0500, Jim Quinlan wrote:
>
> > For us, the supplies are for the EP chip's power.  We have the PCIe
> > controller turning them "on" for power-on/resume and "off" for
> > power-off/suspend.  We need the "xxx-supply" property in the
> > controller's DT node because of the chicken-and-egg situation: if the
> > property was in the EP's DT node, the RC  will never discover the EP
> > to see that there is a regulator to turn on.   We would be happy with
> > a single supply name, something like "ep-power".  We would be ecstatic
> > to have two (ep0-power, ep1-power).
>
> Why can't the controller look at the nodes describing devices for
> standard properties?
Hi Mark,

It just feels wrong for the driver (RC) of one DT node to be acting on
a property of another driver's (EP) node, even though it is a subnode.
There is also the possibility of the EP driver acting upon the
property simultaneously; we don't really have control of what EP
device and drivers are paired with our SOCs.
In addition, this just pushes the binding name issue down a level --
what should these power supplies be called?  They are not slot power
supplies.  Can the  Broadcom STB PCIe RC driver's binding document
specify and define the properties of EP sub-nodes?

Regards,
Jim Quinlan
Broadcom STB

Re: [PATCH v2 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Jim Quinlan]

On Wed, Dec 9, 2020 at 10:07 AM Rob Herring  wrote:
>
> On Mon, Nov 30, 2020 at 04:11:38PM -0500, Jim Quinlan wrote:
> > Quite similar to the regulator bindings found in "rockchip-pcie-host.txt",
> > this allows optional regulators to be attached and controlled by the
> > PCIe RC driver.
> >
> > Signed-off-by: Jim Quinlan 
> > ---
> >  .../devicetree/bindings/pci/brcm,stb-pcie.yaml   | 12 
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml 
> > b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index 807694b4f41f..baacc3d7ec87 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > @@ -85,6 +85,18 @@ properties:
> >minItems: 1
> >maxItems: 3
> >
> > +  vpcie12v-supply:
> > +description: 12v regulator phandle for the endpoint device
> > +
> > +  vpcie3v3-supply:
> > +description: 3.3v regulator phandle for the endpoint device
>
> 12V and 3.3V are standard slot supplies, can you add them to
> pci-bus.yaml. Then some day maybe we can have common slot handling code.
>
> With that, here you just need:
>
> vpcie3v3-supply: true

Hi Rob,

Sorry for the delay in responding -- I just came back from vacation.

The problem we have is that these regulators are not "slot" supplies
-- our HW does not support PCI slots, so if and when general slot
power-handling code came along it would probably screw us up.   If you
don't think there is a problem then I will submit the two supply-names
you OKed, even though they may not match the voltages we are using for
the EPs.

For us, the supplies are for the EP chip's power.  We have the PCIe
controller turning them "on" for power-on/resume and "off" for
power-off/suspend.  We need the "xxx-supply" property in the
controller's DT node because of the chicken-and-egg situation: if the
property was in the EP's DT node, the RC  will never discover the EP
to see that there is a regulator to turn on.   We would be happy with
a single supply name, something like "ep-power".  We would be ecstatic
to have two (ep0-power, ep1-power).

I'm not sure if you remember but FlorianF talked to you about this
situation and concluded that something like the above was the way to
go forward.  For the latest pullreq I  just copied Rockchip's bindings
since you reviewed their bindings commit but it looks like you've
changed your mind.   Given the constraints I have described, what is
the best path forward?

Thanks,
Jim Quinlan
Broadcom STB
>
> > +
> > +  vpcie1v8-supply:
> > +description: 1.8v regulator phandle for the endpoint device
> > +
> > +  vpcie0v9-supply:
> > +description: 0.9v regulator phandle for the endpoint device
>
> These are not standard. They go to a soldered down device or
> non-standard connector? For the former, the device should really be
> described in DT and the supplies added there.
>
> Mini PCIe connector also has 1.5V supply.
>
> Rob

Re: [PATCH] drivers core: Free dma_range_map when driver probe failed

[Jim Quinlan]

> Subject: [PATCH] drivers core: Free dma_range_map when driver probe failed
>
> There will be memory leak if driver probe failed. Trace as below:
>   backtrace:
> [<2415258f>] kmemleak_alloc+0x3c/0x50
> [<f447ebe4>] __kmalloc+0x208/0x530
> [<48bc7b3a>] of_dma_get_range+0xe4/0x1b0
> [<41e39065>] of_dma_configure_id+0x58/0x27c
> [<6356866a>] platform_dma_configure+0x2c/0x40
> ..
> [<0afcf9b5>] ret_from_fork+0x10/0x3c
>
> This issue is introduced by commit e0d072782c73("dma-mapping:
> introduce DMA range map, supplanting dma_pfn_offset "). It doesn't
> free dma_range_map when driver probe failed and cause above
> memory leak. So, add code to free it in error path.
>
> Fixes: e0d072782c73("dma-mapping: introduce DMA range map, supplanting
> dma_pfn_offset ")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Meng Li 
> ---
>  drivers/base/dd.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 148e81969e04..8e4871aa34bc 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -612,6 +612,7 @@ static int really_probe(struct device *dev, struct
> device_driver *drv)
> else if (drv->remove)
> drv->remove(dev);
>  probe_failed:
> +   kfree(dev->dma_range_map);
> if (dev->bus)
> blocking_notifier_call_chain(>bus->p->bus_notifier,
>  BUS_NOTIFY_DRIVER_NOT_BOUND, 
> dev);
> --
> 2.17.1
>
> --

Hi Meng,

Sorry for the delay -- I was on vacation.  I agree with you -- thanks.
However, note that in function  device_release()  in
drivers/base/core.c  there is this line:

kfree(dev->dma_range_map);

Won't this also be called  if all of the appropriate drivers' probes
fail for this device, effecting a double kfree?  Perhaps your patch
could also set "dev->dma_range_map" to NULL after calling kfree()?

Thanks much,
Jim Quinlan
Broadcom STB

Re: [PATCH v4 0/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

Hi Sudeep,

Since RobH has reviewed  patch 1/.2 and Florian has acked it, can you
please accept patches 1 & 2?

Thanks,
Jim Quinlan
Broadcom STB

On Tue, Dec 22, 2020 at 9:56 AM Jim Quinlan  wrote:
>
> v4 -- s/message-serviced/a2p/ in the bindings commit message.
>-- Changed author/s-o-b/committer email address as my company is now
>   appending boilerplate text to all outgoing emails.
>
> v3 -- Changed interrupt name from "message-serviced" to "a2p" (RobH)
>
> v2 -- Correct commit message, s/msg/message/, and remove extra WS on
>   "dt-bindings" commit (Sudeep)
>-- Change interrupt name to "message-serviced", move irq assignent to end
>   of function. (Sudeep)
>
> v1 -- original.
>
> Jim Quinlan (2):
>   dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport
>   firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt
>
>  .../devicetree/bindings/arm/arm,scmi.txt  |  8 
>  drivers/firmware/arm_scmi/smc.c   | 38 ++-
>  2 files changed, 45 insertions(+), 1 deletion(-)
>
> --
> 2.17.1
>

[PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

The SMC/HVC SCMI transport is modified to allow the completion of an SCMI
message to be indicated by an interrupt rather than the return of the smc
call.  This accommodates the existing behavior of the BrcmSTB SCMI
"platform" whose SW is already out in the field and cannot be changed.

Signed-off-by: Jim Quinlan 
---
 drivers/firmware/arm_scmi/smc.c | 38 -
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 82a82a5dc86a..fd41d436e34b 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -9,9 +9,11 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "common.h"
@@ -23,6 +25,8 @@
  * @shmem: Transmit/Receive shared memory area
  * @shmem_lock: Lock to protect access to Tx/Rx shared memory area
  * @func_id: smc/hvc call function id
+ * @irq: Optional; employed when platforms indicates msg completion by intr.
+ * @tx_complete: Optional, employed only when irq is valid.
  */
 
 struct scmi_smc {
@@ -30,8 +34,19 @@ struct scmi_smc {
struct scmi_shared_mem __iomem *shmem;
struct mutex shmem_lock;
u32 func_id;
+   int irq;
+   struct completion tx_complete;
 };
 
+static irqreturn_t smc_msg_done_isr(int irq, void *data)
+{
+   struct scmi_smc *scmi_info = data;
+
+   complete(_info->tx_complete);
+
+   return IRQ_HANDLED;
+}
+
 static bool smc_chan_available(struct device *dev, int idx)
 {
struct device_node *np = of_parse_phandle(dev->of_node, "shmem", 0);
@@ -51,7 +66,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, 
struct device *dev,
struct resource res;
struct device_node *np;
u32 func_id;
-   int ret;
+   int ret, irq = 0;
 
if (!tx)
return -ENODEV;
@@ -79,10 +94,29 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, 
struct device *dev,
if (ret < 0)
return ret;
 
+   /*
+* If there is an interrupt named "a2p", then the
+* service and completion of a message is signaled by an interrupt
+* rather than by the return of the SMC call.
+*/
+   ret = of_irq_get_byname(cdev->of_node, "a2p");
+   if (ret > 0) {
+   irq = ret;
+   ret = devm_request_irq(dev, irq, smc_msg_done_isr,
+  IRQF_NO_SUSPEND,
+  dev_name(dev),
+  scmi_info);
+   if (ret) {
+   dev_err(dev, "failed to setup SCMI smc irq\n");
+   return ret;
+   }
+   init_completion(_info->tx_complete);
+   }
scmi_info->func_id = func_id;
scmi_info->cinfo = cinfo;
mutex_init(_info->shmem_lock);
cinfo->transport_info = scmi_info;
+   scmi_info->irq = irq;
 
return 0;
 }
@@ -111,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
shmem_tx_prepare(scmi_info->shmem, xfer);
 
arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, );
+   if (scmi_info->irq)
+   wait_for_completion(_info->tx_complete);
scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem));
 
mutex_unlock(_info->shmem_lock);
-- 
2.17.1

[PATCH v4 1/2] dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport

[Jim Quinlan]

In normal use of smc/hvc transport in SCMI the message completion is
indicated by the return of the SMC call.  This commit provides for an
optional interrupt named "a2p" which is used instead to
indicate the completion of a message.

Signed-off-by: Jim Quinlan 
---
 Documentation/devicetree/bindings/arm/arm,scmi.txt | 8 
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt 
b/Documentation/devicetree/bindings/arm/arm,scmi.txt
index b5ce5b39bb9c..667d58e0a659 100644
--- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
@@ -31,6 +31,14 @@ Optional properties:
 
 - mbox-names: shall be "tx" or "rx" depending on mboxes entries.
 
+- interrupts : when using smc or hvc transports, this optional
+property indicates that msg completion by the platform is indicated
+by an interrupt rather than by the return of the smc call. This
+should not be used except when the platform requires such behavior.
+
+- interrupt-names : if "interrupts" is present, interrupt-names must also
+be present and have the value "a2p".
+
 See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details
 about the generic mailbox controller and client driver bindings.
 
-- 
2.17.1

[PATCH v4 0/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

v4 -- s/message-serviced/a2p/ in the bindings commit message.
   -- Changed author/s-o-b/committer email address as my company is now
  appending boilerplate text to all outgoing emails.

v3 -- Changed interrupt name from "message-serviced" to "a2p" (RobH)

v2 -- Correct commit message, s/msg/message/, and remove extra WS on
  "dt-bindings" commit (Sudeep)
   -- Change interrupt name to "message-serviced", move irq assignent to end
  of function. (Sudeep)

v1 -- original.

Jim Quinlan (2):
  dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport
  firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

 .../devicetree/bindings/arm/arm,scmi.txt  |  8 
 drivers/firmware/arm_scmi/smc.c   | 38 ++-
 2 files changed, 45 insertions(+), 1 deletion(-)

-- 
2.17.1

[RESEND PATCH v3 2/2] PCI: brcmstb: use reset/rearm instead of deassert/assert

[Jim Quinlan]

The Brcmstb PCIe RC uses a reset control "rescal" for certain chips.  This
reset implements a "pulse reset" so it matches more the reset/rearm
calls instead of the deassert/assert calls.

Also, add reset_control calls in suspend/resume functions.

Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index d41257f43a8f..9f190db4cfcf 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1127,6 +1127,7 @@ static int brcm_pcie_suspend(struct device *dev)
 
brcm_pcie_turn_off(pcie);
ret = brcm_phy_stop(pcie);
+   reset_control_rearm(pcie->rescal);
clk_disable_unprepare(pcie->clk);
 
return ret;
@@ -1142,9 +1143,13 @@ static int brcm_pcie_resume(struct device *dev)
base = pcie->base;
clk_prepare_enable(pcie->clk);
 
+   ret = reset_control_reset(pcie->rescal);
+   if (ret)
+   goto err0;
+
ret = brcm_phy_start(pcie);
if (ret)
-   goto err;
+   goto err1;
 
/* Take bridge out of reset so we can access the SERDES reg */
pcie->bridge_sw_init_set(pcie, 0);
@@ -1159,14 +1164,16 @@ static int brcm_pcie_resume(struct device *dev)
 
ret = brcm_pcie_setup(pcie);
if (ret)
-   goto err;
+   goto err1;
 
if (pcie->msi)
brcm_msi_set_regs(pcie->msi);
 
return 0;
 
-err:
+err1:
+   reset_control_rearm(pcie->rescal);
+err0:
clk_disable_unprepare(pcie->clk);
return ret;
 }
@@ -1176,7 +1183,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
brcm_msi_remove(pcie);
brcm_pcie_turn_off(pcie);
brcm_phy_stop(pcie);
-   reset_control_assert(pcie->rescal);
+   reset_control_rearm(pcie->rescal);
clk_disable_unprepare(pcie->clk);
 }
 
@@ -1251,13 +1258,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
return PTR_ERR(pcie->rescal);
}
 
-   ret = reset_control_deassert(pcie->rescal);
+   ret = reset_control_reset(pcie->rescal);
if (ret)
dev_err(>dev, "failed to deassert 'rescal'\n");
 
ret = brcm_phy_start(pcie);
if (ret) {
-   reset_control_assert(pcie->rescal);
+   reset_control_rearm(pcie->rescal);
clk_disable_unprepare(pcie->clk);
return ret;
}
-- 
2.17.1

[RESEND PATCH v3 1/2] ata: ahci_brcm: Fix use of BCM7216 reset controller

[Jim Quinlan]

From: Jim Quinlan 

This driver may use one of two resets controllers.  Keep them in separate
variables to keep things simple.  The reset controller "rescal" is shared
between the AHCI driver and the PCIe driver for the BrcmSTB 7216 chip.  Use
devm_reset_control_get_optional_shared() to handle this sharing.

Fixes: 272ecd60a636 ("ata: ahci_brcm: BCM7216 reset is self de-asserting")
Fixes: c345ec6a50e9 ("ata: ahci_brcm: Support BCM7216 reset controller name")
Signed-off-by: Jim Quinlan 
---
 drivers/ata/ahci_brcm.c | 46 -
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 49f7acbfcf01..bad455fb6ab8 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -86,7 +86,8 @@ struct brcm_ahci_priv {
u32 port_mask;
u32 quirks;
enum brcm_ahci_version version;
-   struct reset_control *rcdev;
+   struct reset_control *rcdev_rescal;
+   struct reset_control *rcdev_ahci;
 };
 
 static inline u32 brcm_sata_readreg(void __iomem *addr)
@@ -352,8 +353,8 @@ static int brcm_ahci_suspend(struct device *dev)
else
ret = 0;
 
-   if (priv->version != BRCM_SATA_BCM7216)
-   reset_control_assert(priv->rcdev);
+   reset_control_assert(priv->rcdev_ahci);
+   reset_control_rearm(priv->rcdev_rescal);
 
return ret;
 }
@@ -365,10 +366,10 @@ static int __maybe_unused brcm_ahci_resume(struct device 
*dev)
struct brcm_ahci_priv *priv = hpriv->plat_data;
int ret = 0;
 
-   if (priv->version == BRCM_SATA_BCM7216)
-   ret = reset_control_reset(priv->rcdev);
-   else
-   ret = reset_control_deassert(priv->rcdev);
+   ret = reset_control_deassert(priv->rcdev_ahci);
+   if (ret)
+   return ret;
+   ret = reset_control_reset(priv->rcdev_rescal);
if (ret)
return ret;
 
@@ -428,7 +429,6 @@ static int brcm_ahci_probe(struct platform_device *pdev)
 {
const struct of_device_id *of_id;
struct device *dev = >dev;
-   const char *reset_name = NULL;
struct brcm_ahci_priv *priv;
struct ahci_host_priv *hpriv;
struct resource *res;
@@ -450,15 +450,15 @@ static int brcm_ahci_probe(struct platform_device *pdev)
if (IS_ERR(priv->top_ctrl))
return PTR_ERR(priv->top_ctrl);
 
-   /* Reset is optional depending on platform and named differently */
-   if (priv->version == BRCM_SATA_BCM7216)
-   reset_name = "rescal";
-   else
-   reset_name = "ahci";
-
-   priv->rcdev = devm_reset_control_get_optional(>dev, reset_name);
-   if (IS_ERR(priv->rcdev))
-   return PTR_ERR(priv->rcdev);
+   if (priv->version == BRCM_SATA_BCM7216) {
+   priv->rcdev_rescal = devm_reset_control_get_optional_shared(
+   >dev, "rescal");
+   if (IS_ERR(priv->rcdev_rescal))
+   return PTR_ERR(priv->rcdev_rescal);
+   }
+   priv->rcdev_ahci = devm_reset_control_get_optional(>dev, "ahci");
+   if (IS_ERR(priv->rcdev_ahci))
+   return PTR_ERR(priv->rcdev_ahci);
 
hpriv = ahci_platform_get_resources(pdev, 0);
if (IS_ERR(hpriv))
@@ -479,10 +479,10 @@ static int brcm_ahci_probe(struct platform_device *pdev)
break;
}
 
-   if (priv->version == BRCM_SATA_BCM7216)
-   ret = reset_control_reset(priv->rcdev);
-   else
-   ret = reset_control_deassert(priv->rcdev);
+   ret = reset_control_reset(priv->rcdev_rescal);
+   if (ret)
+   return ret;
+   ret = reset_control_deassert(priv->rcdev_ahci);
if (ret)
return ret;
 
@@ -527,8 +527,8 @@ static int brcm_ahci_probe(struct platform_device *pdev)
 out_disable_clks:
ahci_platform_disable_clks(hpriv);
 out_reset:
-   if (priv->version != BRCM_SATA_BCM7216)
-   reset_control_assert(priv->rcdev);
+   reset_control_assert(priv->rcdev_ahci);
+   reset_control_rearm(priv->rcdev_rescal);
return ret;
 }
 
-- 
2.17.1

[RESEND PATCH v3 0/2] ata: ahci_brcm: Fix use of BCM7216 reset controller

[Jim Quinlan]

v3 -- discard commit from v2; instead rely on the new function
  reset_control_rearm provided in a recent commit [1] applied
  to reset/next.
   -- New commit to correct pcie-brcmstb.c usage of a reset controller
  to use reset/rearm verses deassert/assert.

v2 -- refactor rescal-reset driver to implement assert/deassert rather than
  reset because the reset call only fires once per lifetime and we need
  to reset after every resume from S2 or S3.
   -- Split the use of "ahci" and "rescal" controllers in separate fields
  to keep things simple.

v1 -- original


[1] Applied commit "reset: make shared pulsed reset controls re-triggerable"
found at git://git.pengutronix.de/git/pza/linux.git
branch reset/shared-retrigger

Jim Quinlan (2):
  ata: ahci_brcm: Fix use of BCM7216 reset controller
  PCI: brcmstb: use reset/rearm instead of deassert/assert

 drivers/ata/ahci_brcm.c   | 46 +--
 drivers/pci/controller/pcie-brcmstb.c | 19 +++
 2 files changed, 36 insertions(+), 29 deletions(-)

-- 
2.17.1

[PATCH v3 1/2] dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport

[Jim Quinlan]

In normal use of smc/hvc transport in SCMI the message completion is
indicated by the return of the SMC call.  This commit provides for an
optional interrupt named "message-serviced" which is used instead to
indicate the completion of a message.

Signed-off-by: Jim Quinlan 
---
 Documentation/devicetree/bindings/arm/arm,scmi.txt | 8 
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt 
b/Documentation/devicetree/bindings/arm/arm,scmi.txt
index b5ce5b39bb9c..667d58e0a659 100644
--- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
@@ -31,6 +31,14 @@ Optional properties:
 
 - mbox-names: shall be "tx" or "rx" depending on mboxes entries.
 
+- interrupts : when using smc or hvc transports, this optional
+property indicates that msg completion by the platform is indicated
+by an interrupt rather than by the return of the smc call. This
+should not be used except when the platform requires such behavior.
+
+- interrupt-names : if "interrupts" is present, interrupt-names must also
+be present and have the value "a2p".
+
 See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details
 about the generic mailbox controller and client driver bindings.
 
-- 
2.17.1

[PATCH v3 0/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

v3 -- Changed interrupt name from "message-serviced" to "a2p" (RobH)

v2 -- Correct commit message, s/msg/message/, and remove extra WS on
  "dt-bindings" commit (Sudeep)
   -- Change interrupt name to "message-serviced", move irq assignent to end
  of function. (Sudeep)

v1 -- original.


Jim Quinlan (2):
  dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport
  firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

 .../devicetree/bindings/arm/arm,scmi.txt  |  8 
 drivers/firmware/arm_scmi/smc.c   | 38 ++-
 2 files changed, 45 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

The SMC/HVC SCMI transport is modified to allow the completion of an SCMI
message to be indicated by an interrupt rather than the return of the smc
call.  This accommodates the existing behavior of the BrcmSTB SCMI
"platform" whose SW is already out in the field and cannot be changed.

Signed-off-by: Jim Quinlan 
---
 drivers/firmware/arm_scmi/smc.c | 38 -
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 82a82a5dc86a..fd41d436e34b 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -9,9 +9,11 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "common.h"
@@ -23,6 +25,8 @@
  * @shmem: Transmit/Receive shared memory area
  * @shmem_lock: Lock to protect access to Tx/Rx shared memory area
  * @func_id: smc/hvc call function id
+ * @irq: Optional; employed when platforms indicates msg completion by intr.
+ * @tx_complete: Optional, employed only when irq is valid.
  */
 
 struct scmi_smc {
@@ -30,8 +34,19 @@ struct scmi_smc {
struct scmi_shared_mem __iomem *shmem;
struct mutex shmem_lock;
u32 func_id;
+   int irq;
+   struct completion tx_complete;
 };
 
+static irqreturn_t smc_msg_done_isr(int irq, void *data)
+{
+   struct scmi_smc *scmi_info = data;
+
+   complete(_info->tx_complete);
+
+   return IRQ_HANDLED;
+}
+
 static bool smc_chan_available(struct device *dev, int idx)
 {
struct device_node *np = of_parse_phandle(dev->of_node, "shmem", 0);
@@ -51,7 +66,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, 
struct device *dev,
struct resource res;
struct device_node *np;
u32 func_id;
-   int ret;
+   int ret, irq = 0;
 
if (!tx)
return -ENODEV;
@@ -79,10 +94,29 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, 
struct device *dev,
if (ret < 0)
return ret;
 
+   /*
+* If there is an interrupt named "a2p", then the
+* service and completion of a message is signaled by an interrupt
+* rather than by the return of the SMC call.
+*/
+   ret = of_irq_get_byname(cdev->of_node, "a2p");
+   if (ret > 0) {
+   irq = ret;
+   ret = devm_request_irq(dev, irq, smc_msg_done_isr,
+  IRQF_NO_SUSPEND,
+  dev_name(dev),
+  scmi_info);
+   if (ret) {
+   dev_err(dev, "failed to setup SCMI smc irq\n");
+   return ret;
+   }
+   init_completion(_info->tx_complete);
+   }
scmi_info->func_id = func_id;
scmi_info->cinfo = cinfo;
mutex_init(_info->shmem_lock);
cinfo->transport_info = scmi_info;
+   scmi_info->irq = irq;
 
return 0;
 }
@@ -111,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
shmem_tx_prepare(scmi_info->shmem, xfer);
 
arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, );
+   if (scmi_info->irq)
+   wait_for_completion(_info->tx_complete);
scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem));
 
mutex_unlock(_info->shmem_lock);
-- 
2.17.1

Re: [PATCH v2 1/2] dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport

[Jim Quinlan]

On Mon, Dec 7, 2020 at 2:01 PM Rob Herring  wrote:
>
> On Thu, Nov 12, 2020 at 12:56:26PM -0500, Jim Quinlan wrote:
> > In normal use of smc/hvc transport in SCMI the message completion is
> > indicated by the return of the SMC call.  This commit provides for an
> > optional interrupt named "message-serviced" which is used instead to
> > indicate the completion of a message.
> >
> > Signed-off-by: Jim Quinlan 
> > ---
> >  Documentation/devicetree/bindings/arm/arm,scmi.txt | 8 
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt 
> > b/Documentation/devicetree/bindings/arm/arm,scmi.txt
> > index 55deb68230eb..7cdad11f40b1 100644
> > --- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
> > +++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
> > @@ -31,6 +31,14 @@ Optional properties:
> >
> >  - mbox-names: shall be "tx" or "rx" depending on mboxes entries.
> >
> > +- interrupts : when using smc or hvc transports, this optional
> > +  property indicates that msg completion by the platform is indicated
> > +  by an interrupt rather than by the return of the smc call. This
> > +  should not be used except when the platform requires such behavior.
> > +
> > +- interrupt-names : if "interrupts" is present, interrupt-names must also
> > +  be present and have the value "message-serviced".
>
> Don't really need names when only one, but wouldn't 'a2p' be more
> concise and based on SCMI spec (just guessing...).
I gave it a name because (a) its presence is atypical/not common and
(b) future changes may add more interrupts and I don't want this
interrupt at index 0.

As far as "message-serviced", this was Sudeep's suggestion, IIRC,
although I had a similarly long name.  Sudeep, are you okay with
"a2p".

Thanks,
Jim


>
> > +
> >  See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details
> >  about the generic mailbox controller and client driver bindings.
> >
> > --
> > 2.17.1
> >
>
>

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v2 5/6] PCI: brcmstb: Add panic/die handler to RC driver

[Jim Quinlan]

On Tue, Dec 1, 2020 at 1:05 PM Bjorn Helgaas  wrote:
>
> On Mon, Nov 30, 2020 at 04:11:42PM -0500, Jim Quinlan wrote:
> > Whereas most PCIe HW returns 0x on illegal accesses and the like,
> > by default Broadcom's STB PCIe controller effects an abort.  This simple
> > handler determines if the PCIe controller was the cause of the abort and if
> > so, prints out diagnostic info.
>
> What happens during enumeration?  pci_bus_generic_read_dev_vendor_id()
> assumes a read of Vendor ID returns 0x if the device doesn't
> exist.
>
> I assume this case doesn't cause the abort you're referring to here,
> or nothing would work.  I think this enumeration case results in PCIe
> Unsupported Request errors (PCIe r5.0, sec 2.3.2 implementation note).
Hi Bjorn,

Yes, our controller makes a special case  to allow for config-space
accesses to the dev_id and vendor_id registers. even if the device is
missing.  That being said, it will abort on any access if the link is
down.

However, the 7216-type SOCs bring PCIe error-reporting HW but also
have a mode where 0x  is returned on improper accesses, just
like many other controllers.  We are debating whether we should turn
this on by default.

Regards,
Jim Quinlan
Broadcom STB
>
> Bjorn


smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v2 6/6] PCI: brcmstb: check return value of clk_prepare_enable()

[Jim Quinlan]

The check was missing on PCIe resume.

Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index 3983d6c80769..64cf534e44d0 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1344,7 +1344,9 @@ static int brcm_pcie_resume(struct device *dev)
 
base = pcie->base;
brcm_set_regulators(pcie, TURN_ON);
-   clk_prepare_enable(pcie->clk);
+   ret = clk_prepare_enable(pcie->clk);
+   if (ret)
+   return ret;
 
ret = brcm_phy_start(pcie);
if (ret)
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v2 2/6] PCI: brcmstb: Add control of EP voltage regulator(s)

[Jim Quinlan]

Control of EP regulators by the RC is needed because of the chicken-and-egg
situation: although the regulator is "owned" by the EP and would be best
handled on its driver, the EP cannot be discovered and probed unless its
regulator is already turned on.

Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 38 ++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index bea86899bd5d..9d4ac42b3bee 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -210,6 +211,10 @@ enum pcie_type {
BCM2711,
 };
 
+static const char * const ep_regulator_names[] = {
+   "vpcie12v", "vpcie3v3", "vpcie1v8", "vpcie0v9",
+};
+
 struct pcie_cfg_data {
const int *offsets;
const enum pcie_type type;
@@ -287,8 +292,25 @@ struct brcm_pcie {
u32 hw_rev;
void(*perst_set)(struct brcm_pcie *pcie, u32 val);
void(*bridge_sw_init_set)(struct brcm_pcie *pcie, 
u32 val);
+   struct regulator_bulk_data supplies[ARRAY_SIZE(ep_regulator_names)];
 };
 
+static void brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+{
+   struct device *dev = pcie->dev;
+   int ret;
+
+   if (on)
+   ret = regulator_bulk_enable(ARRAY_SIZE(ep_regulator_names),
+   pcie->supplies);
+   else
+   ret = regulator_bulk_disable(ARRAY_SIZE(ep_regulator_names),
+pcie->supplies);
+   if (ret)
+   dev_err(dev, "failed to %s EP regulators\n",
+   on ? "enable" : "disable");
+}
+
 /*
  * This is to convert the size of the inbound "BAR" region to the
  * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
@@ -1139,6 +1161,7 @@ static int brcm_pcie_suspend(struct device *dev)
brcm_pcie_turn_off(pcie);
ret = brcm_phy_stop(pcie);
clk_disable_unprepare(pcie->clk);
+   brcm_set_regulators(pcie, false);
 
return ret;
 }
@@ -1151,6 +1174,7 @@ static int brcm_pcie_resume(struct device *dev)
int ret;
 
base = pcie->base;
+   brcm_set_regulators(pcie, true);
clk_prepare_enable(pcie->clk);
 
ret = brcm_phy_start(pcie);
@@ -1189,6 +1213,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
brcm_phy_stop(pcie);
reset_control_assert(pcie->rescal);
clk_disable_unprepare(pcie->clk);
+   brcm_set_regulators(pcie, false);
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
@@ -1218,7 +1243,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
struct pci_host_bridge *bridge;
const struct pcie_cfg_data *data;
struct brcm_pcie *pcie;
-   int ret;
+   int ret, i;
 
bridge = devm_pci_alloc_host_bridge(>dev, sizeof(*pcie));
if (!bridge)
@@ -1246,6 +1271,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
if (IS_ERR(pcie->clk))
return PTR_ERR(pcie->clk);
 
+   for (i = 0; i < ARRAY_SIZE(ep_regulator_names); i++)
+   pcie->supplies[i].supply = ep_regulator_names[i];
+
+   ret = devm_regulator_bulk_get(pcie->dev, ARRAY_SIZE(ep_regulator_names),
+ pcie->supplies);
+   if (ret) {
+   dev_err(pcie->dev, "failed to get regulators\n");
+   return ret;
+   }
+
ret = of_pci_get_max_link_speed(np);
pcie->gen = (ret < 0) ? 0 : ret;
 
@@ -1273,6 +1308,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
return ret;
}
 
+   brcm_set_regulators(pcie, true);
ret = brcm_pcie_setup(pcie);
if (ret)
goto fail;
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v2 4/6] PCI: brcmstb: Give 7216 SOCs their own config type

[Jim Quinlan]

This distinction is required for an imminent commit.

Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index cbdb315d4b2f..989e4231d136 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -256,6 +256,13 @@ static const struct pcie_cfg_data bcm2711_cfg = {
.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 };
 
+static const struct pcie_cfg_data bcm7216_cfg = {
+   .offsets= pcie_offset_bcm7278,
+   .type   = BCM7278,
+   .perst_set  = brcm_pcie_perst_set_7278,
+   .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
+};
+
 struct brcm_msi {
struct device   *dev;
void __iomem*base;
@@ -1276,7 +1283,7 @@ static const struct of_device_id brcm_pcie_match[] = {
{ .compatible = "brcm,bcm2711-pcie", .data = _cfg },
{ .compatible = "brcm,bcm7211-pcie", .data = _cfg },
{ .compatible = "brcm,bcm7278-pcie", .data = _cfg },
-   { .compatible = "brcm,bcm7216-pcie", .data = _cfg },
+   { .compatible = "brcm,bcm7216-pcie", .data = _cfg },
{ .compatible = "brcm,bcm7445-pcie", .data = _cfg },
{},
 };
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v2 5/6] PCI: brcmstb: Add panic/die handler to RC driver

[Jim Quinlan]

Whereas most PCIe HW returns 0x on illegal accesses and the like,
by default Broadcom's STB PCIe controller effects an abort.  This simple
handler determines if the PCIe controller was the cause of the abort and if
so, prints out diagnostic info.

Example output:
  brcm-pcie 8b2.pcie: Error: Mem Acc: 32bit, Read, @0x3800
  brcm-pcie 8b2.pcie:  Type: TO=0 Abt=0 UnspReq=1 AccDsble=0 BadAddr=0

Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 124 ++
 1 file changed, 124 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index 989e4231d136..3983d6c80769 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -12,11 +12,13 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -187,6 +189,39 @@
 #define  PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_MASK0x1
 #define  PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_SHIFT   0x0
 
+/* Error report regiseters */
+#define PCIE_OUTB_ERR_TREAT0x6000
+#define  PCIE_OUTB_ERR_TREAT_CONFIG_MASK   0x1
+#define  PCIE_OUTB_ERR_TREAT_MEM_MASK  0x2
+#define PCIE_OUTB_ERR_VALID0x6004
+#define PCIE_OUTB_ERR_CLEAR0x6008
+#define PCIE_OUTB_ERR_ACC_INFO 0x600c
+#define  PCIE_OUTB_ERR_ACC_INFO_CFG_ERR_MASK   0x01
+#define  PCIE_OUTB_ERR_ACC_INFO_MEM_ERR_MASK   0x02
+#define  PCIE_OUTB_ERR_ACC_INFO_TYPE_64_MASK   0x04
+#define  PCIE_OUTB_ERR_ACC_INFO_DIR_WRITE_MASK 0x10
+#define  PCIE_OUTB_ERR_ACC_INFO_BYTE_LANES_MASK0xff00
+#define PCIE_OUTB_ERR_ACC_ADDR 0x6010
+#define PCIE_OUTB_ERR_ACC_ADDR_BUS_MASK0xff0
+#define PCIE_OUTB_ERR_ACC_ADDR_DEV_MASK0xf8000
+#define PCIE_OUTB_ERR_ACC_ADDR_FUNC_MASK   0x7000
+#define PCIE_OUTB_ERR_ACC_ADDR_REG_MASK0xfff
+#define PCIE_OUTB_ERR_CFG_CAUSE0x6014
+#define  PCIE_OUTB_ERR_CFG_CAUSE_TIMEOUT_MASK  0x40
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ABORT_MASK0x20
+#define  PCIE_OUTB_ERR_CFG_CAUSE_UNSUPP_REQ_MASK   0x10
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_TIMEOUT_MASK  0x4
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_DISABLED_MASK 0x2
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_64BIT__MASK   0x1
+#define PCIE_OUTB_ERR_MEM_ADDR_LO  0x6018
+#define PCIE_OUTB_ERR_MEM_ADDR_HI  0x601c
+#define PCIE_OUTB_ERR_MEM_CAUSE0x6020
+#define  PCIE_OUTB_ERR_MEM_CAUSE_TIMEOUT_MASK  0x40
+#define  PCIE_OUTB_ERR_MEM_CAUSE_ABORT_MASK0x20
+#define  PCIE_OUTB_ERR_MEM_CAUSE_UNSUPP_REQ_MASK   0x10
+#define  PCIE_OUTB_ERR_MEM_CAUSE_ACC_DISABLED_MASK 0x2
+#define  PCIE_OUTB_ERR_MEM_CAUSE_BAD_ADDR_MASK 0x1
+
 /* Forward declarations */
 struct brcm_pcie;
 static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, 
u32 val);
@@ -221,6 +256,7 @@ struct pcie_cfg_data {
const enum pcie_type type;
void (*perst_set)(struct brcm_pcie *pcie, u32 val);
void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+   const bool has_err_report;
 };
 
 static const int pcie_offsets[] = {
@@ -261,6 +297,7 @@ static const struct pcie_cfg_data bcm7216_cfg = {
.type   = BCM7278,
.perst_set  = brcm_pcie_perst_set_7278,
.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
+   .has_err_report = true,
 };
 
 struct brcm_msi {
@@ -302,8 +339,89 @@ struct brcm_pcie {
void(*bridge_sw_init_set)(struct brcm_pcie *pcie, 
u32 val);
struct regulator_bulk_data supplies[ARRAY_SIZE(ep_regulator_names)];
boolep_wakeup_capable;
+   boolhas_err_report;
+   struct notifier_block   die_notifier;
 };
 
+/*
+ * Dump out pcie errors on die or panic.
+ */
+static int dump_pcie_error(struct notifier_block *self, unsigned long v, void 
*p)
+{
+   const struct brcm_pcie *pcie = container_of(self, struct brcm_pcie, 
die_notifier);
+   void __iomem *base = pcie->base;
+   int i, is_cfg_err, is_mem_err, lanes;
+   char *width_str, *direction_str, lanes_str[9];
+   u32 info;
+
+   if (readl(base + PCIE_OUTB_ERR_VALID) == 0)
+   return NOTIFY_DONE;
+   info = readl(base + PCIE_OUTB_ERR_ACC_INFO);
+
+
+   is_cfg_err = !!(info & PCIE_OUTB_ERR_ACC_INFO_CFG_ERR_MASK);
+   is_mem_err = !!(info & PCIE_OUTB_ERR_ACC_INFO_MEM_ERR_MASK);
+   width_str = (info & PCIE_OUTB_ERR_ACC_INFO_TYPE_64_MASK) ? "64bit" : 
"32bit";
+   direction_str = (info & PCIE_OUTB

[PATCH v2 0/6] brcmstb: add EP regulators and panic handler

[Jim Quinlan]

v2 -- Use regulator bulk API rather than multiple calls (MarkB).

v1 -- Bindings are added for fixed regulators that may power the EP device.
   -- The brcmstb RC driver is modified to control these regulators
  during probe, suspend, and resume.
   -- 7216 type SOCs have additional error reporting HW and a
  panic handler is added to dump its info.
   -- A missing return value check is added.


Jim Quinlan (6):
  dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators
  PCI: brcmstb: Add control of EP voltage regulator(s)
  PCI: brcmstb: Do not turn off regulators if EP can wake up
  PCI: brcmstb: Give 7216 SOCs their own config type
  PCI: brcmstb: Add panic/die handler to RC driver
  PCI: brcmstb: check return value of clk_prepare_enable()

 .../bindings/pci/brcm,stb-pcie.yaml   |  12 +
 drivers/pci/controller/pcie-brcmstb.c | 219 +-
 2 files changed, 228 insertions(+), 3 deletions(-)

-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v2 3/6] PCI: brcmstb: Do not turn off regulators if EP can wake up

[Jim Quinlan]

If any downstream device may wake up during S2/S3 suspend, we do not want
to turn off its power when suspending.

Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 58 +++
 1 file changed, 51 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index 9d4ac42b3bee..cbdb315d4b2f 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -193,6 +193,7 @@ static inline void brcm_pcie_bridge_sw_init_set_7278(struct 
brcm_pcie *pcie, u32
 static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie 
*pcie, u32 val);
 static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 
val);
+static bool brcm_pcie_link_up(struct brcm_pcie *pcie);
 
 enum {
RGR1_SW_INIT_1,
@@ -293,14 +294,57 @@ struct brcm_pcie {
void(*perst_set)(struct brcm_pcie *pcie, u32 val);
void(*bridge_sw_init_set)(struct brcm_pcie *pcie, 
u32 val);
struct regulator_bulk_data supplies[ARRAY_SIZE(ep_regulator_names)];
+   boolep_wakeup_capable;
 };
 
-static void brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
 {
+   bool *ret = data;
+
+   if (device_may_wakeup(>dev)) {
+   *ret = true;
+   dev_dbg(>dev, "disable cancelled for wake-up device\n");
+   }
+   return (int) *ret;
+}
+
+enum {
+   TURN_OFF,   /* Turn egulators off, unless an EP is 
wakeup-capable */
+   TURN_OFF_ALWAYS,/* Turn Regulators off, no exceptions */
+   TURN_ON,/* Turn regulators on, unless 
pcie->ep_wakeup_capable */
+};
+
+static void brcm_set_regulators(struct brcm_pcie *pcie, int how)
+{
+   struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
struct device *dev = pcie->dev;
int ret;
 
-   if (on)
+   if (how == TURN_ON) {
+   if (pcie->ep_wakeup_capable) {
+   /*
+* We are resuming from a suspend.  In the
+* previous suspend we did not disable the power
+* supplies, so there is no need to enable them
+* (and falsely increase their usage count).
+*/
+   pcie->ep_wakeup_capable = false;
+   return;
+   }
+   } else if (how == TURN_OFF) {
+   /*
+* If at least one device on this bus is enabled as a
+* wake-up source, do not turn off regulators.
+*/
+   pcie->ep_wakeup_capable = false;
+   if (bridge->bus && brcm_pcie_link_up(pcie)) {
+   pci_walk_bus(bridge->bus, pci_dev_may_wakeup, 
>ep_wakeup_capable);
+   if (pcie->ep_wakeup_capable)
+   return;
+   }
+   }
+
+   if (how == TURN_ON)
ret = regulator_bulk_enable(ARRAY_SIZE(ep_regulator_names),
pcie->supplies);
else
@@ -308,7 +352,7 @@ static void brcm_set_regulators(struct brcm_pcie *pcie, 
bool on)
 pcie->supplies);
if (ret)
dev_err(dev, "failed to %s EP regulators\n",
-   on ? "enable" : "disable");
+   how == TURN_ON ? "enable" : "disable");
 }
 
 /*
@@ -1161,7 +1205,7 @@ static int brcm_pcie_suspend(struct device *dev)
brcm_pcie_turn_off(pcie);
ret = brcm_phy_stop(pcie);
clk_disable_unprepare(pcie->clk);
-   brcm_set_regulators(pcie, false);
+   brcm_set_regulators(pcie, TURN_OFF);
 
return ret;
 }
@@ -1174,7 +1218,7 @@ static int brcm_pcie_resume(struct device *dev)
int ret;
 
base = pcie->base;
-   brcm_set_regulators(pcie, true);
+   brcm_set_regulators(pcie, TURN_ON);
clk_prepare_enable(pcie->clk);
 
ret = brcm_phy_start(pcie);
@@ -1213,7 +1257,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
brcm_phy_stop(pcie);
reset_control_assert(pcie->rescal);
clk_disable_unprepare(pcie->clk);
-   brcm_set_regulators(pcie, false);
+   brcm_set_regulators(pcie, TURN_OFF_ALWAYS);
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
@@ -1308,7 +1352,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
return ret;
}
 
-   brcm_set_regulators(pcie, true);
+   brcm_set_regulators(pcie, TURN_ON);
ret = brcm_pcie_setup(pcie);
if (ret)
goto fail;
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v2 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Jim Quinlan]

Quite similar to the regulator bindings found in "rockchip-pcie-host.txt",
this allows optional regulators to be attached and controlled by the
PCIe RC driver.

Signed-off-by: Jim Quinlan 
---
 .../devicetree/bindings/pci/brcm,stb-pcie.yaml   | 12 
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml 
b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 807694b4f41f..baacc3d7ec87 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -85,6 +85,18 @@ properties:
   minItems: 1
   maxItems: 3
 
+  vpcie12v-supply:
+description: 12v regulator phandle for the endpoint device
+
+  vpcie3v3-supply:
+description: 3.3v regulator phandle for the endpoint device
+
+  vpcie1v8-supply:
+description: 1.8v regulator phandle for the endpoint device
+
+  vpcie0v9-supply:
+description: 0.9v regulator phandle for the endpoint device
+
 required:
   - reg
   - ranges
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v1 2/6] PCI: brcmstb: Add control of EP voltage regulator(s)

[Jim Quinlan]

On Thu, Nov 26, 2020 at 6:49 AM Mark Brown  wrote:
>
> On Wed, Nov 25, 2020 at 02:24:19PM -0500, Jim Quinlan wrote:
>
> > + for (i = 0; i < PCIE_REGULATORS_MAX; i++) {
> > + ep_reg = devm_regulator_get_optional(dev, 
> > ep_regulator_names[i]);
> > + if (IS_ERR(ep_reg)) {
>
> Does PCI allow supplies to be physically absent?  If not then the driver
> shouldn't be using regulator_get_optional() and much of the code here
> can be deleted.
Hi Mark,

First, as an aside, I'm  a little confused about the purpose of
devm_regulator_get_optional(...);  the other  xxx_get_optional() calls
I am familiar with (eg clock, reset, gpio) return NULL if the desired
item does not exist, and then NULL can be used as a valid pointer for
the rest of the API.  Not so here.

At any rate, our SOCs are placed in a variety of boards which
implement the PCIe RC-EP connection as they wish.  From the PCIe
driver's point of view, the type of power supply that needs to be
turned on is specified in the DT and they cannot be hard coded by the
driver.  I've listed all of the four possibilities; typically one,
maybe two will  be specified, but never all of them.  In addition,
sometimes a regulator is hard wired on and not even controllable by
the PCIe controller.

>
> > +static void brcm_set_regulators(struct brcm_pcie *pcie, bool on)
> > +{
>
> This is open coding the regulator bulk APIs.
Except that a bulk regulator "get"  requires that all supplies are
present.  I would have to first scan the node's properties for the
"-supply" properties and fill in the bulk regulator structure.  I'm
fine with doing that.

However, a previous incarnation of this  commit was reviewed by RobH,
and if I understood him correctly he wanted the actual names of the
possible regulators to be used and specified in the bindings doc.   I
just followed the example of "pcie-rockchip-host.c" whose bindings doc
was reviewed by RobH.

Regards,
Jim Quinlan
Broadcom STB


smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v1 2/6] PCI: brcmstb: Add control of EP voltage regulator(s)

[Jim Quinlan]

Control of EP regulators by the RC is needed because of the chicken-and-egg
situation: although the regulator is "owned" by the EP and would be best
handled on its driver, the EP cannot be discovered and probed unless its
regulator is already turned on.

Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 66 +++
 1 file changed, 66 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index bea86899bd5d..34d6bad07b66 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -210,6 +211,18 @@ enum pcie_type {
BCM2711,
 };
 
+enum pcie_regulators {
+   VPCIE12V,
+   VPCIE3V3,
+   VPCIE1V8,
+   VPCIE0V9,
+   PCIE_REGULATORS_MAX,
+};
+
+static const char *ep_regulator_names[PCIE_REGULATORS_MAX] = {
+   "vpcie12v", "vpcie3v3", "vpcie1v8", "vpcie0v9",
+};
+
 struct pcie_cfg_data {
const int *offsets;
const enum pcie_type type;
@@ -287,8 +300,53 @@ struct brcm_pcie {
u32 hw_rev;
void(*perst_set)(struct brcm_pcie *pcie, u32 val);
void(*bridge_sw_init_set)(struct brcm_pcie *pcie, 
u32 val);
+   struct regulator*regulators[PCIE_REGULATORS_MAX];
+   int num_regulators;
 };
 
+static int brcm_parse_regulators(struct brcm_pcie *pcie)
+{
+   struct device *dev = pcie->dev;
+   struct regulator *ep_reg;
+   int i;
+
+   for (i = 0; i < PCIE_REGULATORS_MAX; i++) {
+   ep_reg = devm_regulator_get_optional(dev, 
ep_regulator_names[i]);
+   if (IS_ERR(ep_reg)) {
+   if (PTR_ERR(ep_reg) == -ENODEV)
+   continue;
+   dev_err(dev, "failed to get regulator %s\n", 
ep_regulator_names[i]);
+   return PTR_ERR(ep_reg);
+   }
+   pcie->regulators[i] = ep_reg;
+   pcie->num_regulators++;
+   }
+   return 0;
+}
+
+static void brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+{
+   struct device *dev = pcie->dev;
+   int ret, i;
+
+   if (pcie->num_regulators == 0)
+   return;
+
+   for (i = 0; i < PCIE_REGULATORS_MAX; i++) {
+   if (!pcie->regulators[i])
+   continue;
+   if (on) {
+   ret = regulator_enable(pcie->regulators[i]);
+   dev_dbg(dev, "enable regulator %s (%s)\n",
+   ep_regulator_names[i], ret ? "fail" : "pass");
+   } else {
+   ret = regulator_disable(pcie->regulators[i]);
+   dev_dbg(dev, "disable regulator %s (%s)\n",
+   ep_regulator_names[i], ret ? "fail" : "pass");
+   }
+   }
+}
+
 /*
  * This is to convert the size of the inbound "BAR" region to the
  * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
@@ -1139,6 +1197,7 @@ static int brcm_pcie_suspend(struct device *dev)
brcm_pcie_turn_off(pcie);
ret = brcm_phy_stop(pcie);
clk_disable_unprepare(pcie->clk);
+   brcm_set_regulators(pcie, false);
 
return ret;
 }
@@ -1151,6 +1210,7 @@ static int brcm_pcie_resume(struct device *dev)
int ret;
 
base = pcie->base;
+   brcm_set_regulators(pcie, true);
clk_prepare_enable(pcie->clk);
 
ret = brcm_phy_start(pcie);
@@ -1189,6 +1249,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
brcm_phy_stop(pcie);
reset_control_assert(pcie->rescal);
clk_disable_unprepare(pcie->clk);
+   brcm_set_regulators(pcie, false);
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
@@ -1238,6 +1299,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
pcie->perst_set = data->perst_set;
pcie->bridge_sw_init_set = data->bridge_sw_init_set;
 
+   ret = brcm_parse_regulators(pcie);
+   if (ret)
+   return ret;
+
pcie->base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(pcie->base))
return PTR_ERR(pcie->base);
@@ -1273,6 +1338,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
return ret;
}
 
+   brcm_set_regulators(pcie, true);
ret = brcm_pcie_setup(pcie);
if (ret)
goto fail;
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v1 3/6] PCI: brcmstb: Do not turn off regulators if EP can wake up

[Jim Quinlan]

If any downstream device may wake up during S2/S3 suspend, we do not want
to turn off its power when suspending.

Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 56 ---
 1 file changed, 50 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index 34d6bad07b66..9b46f0bc 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -193,6 +193,7 @@ static inline void brcm_pcie_bridge_sw_init_set_7278(struct 
brcm_pcie *pcie, u32
 static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie 
*pcie, u32 val);
 static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 
val);
+static bool brcm_pcie_link_up(struct brcm_pcie *pcie);
 
 enum {
RGR1_SW_INIT_1,
@@ -302,6 +303,7 @@ struct brcm_pcie {
void(*bridge_sw_init_set)(struct brcm_pcie *pcie, 
u32 val);
struct regulator*regulators[PCIE_REGULATORS_MAX];
int num_regulators;
+   boolep_wakeup_capable;
 };
 
 static int brcm_parse_regulators(struct brcm_pcie *pcie)
@@ -324,18 +326,60 @@ static int brcm_parse_regulators(struct brcm_pcie *pcie)
return 0;
 }
 
-static void brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
 {
+   bool *ret = data;
+
+   if (device_may_wakeup(>dev)) {
+   *ret = true;
+   dev_dbg(>dev, "disable cancelled for wake-up device\n");
+   }
+   return (int) *ret;
+}
+
+enum {
+   TURN_OFF,   /* Turn egulators off, unless an EP is 
wakeup-capable */
+   TURN_OFF_ALWAYS,/* Turn Regulators off, no exceptions */
+   TURN_ON,/* Turn regulators on, unless 
pcie->ep_wakeup_capable */
+};
+
+static void brcm_set_regulators(struct brcm_pcie *pcie, int how)
+{
+   struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
struct device *dev = pcie->dev;
int ret, i;
 
if (pcie->num_regulators == 0)
return;
 
+   if (how == TURN_ON) {
+   if (pcie->ep_wakeup_capable) {
+   /*
+* We are resuming from a suspend.  In the
+* previous suspend we did not disable the power
+* supplies, so there is no need to enable them
+* (and falsely increase their usage count).
+*/
+   pcie->ep_wakeup_capable = false;
+   return;
+   }
+   } else if (how == TURN_OFF) {
+   /*
+* If at least one device on this bus is enabled as a
+* wake-up source, do not turn off regulators.
+*/
+   pcie->ep_wakeup_capable = false;
+   if (bridge->bus && brcm_pcie_link_up(pcie)) {
+   pci_walk_bus(bridge->bus, pci_dev_may_wakeup, 
>ep_wakeup_capable);
+   if (pcie->ep_wakeup_capable)
+   return;
+   }
+   }
+
for (i = 0; i < PCIE_REGULATORS_MAX; i++) {
if (!pcie->regulators[i])
continue;
-   if (on) {
+   if (how == TURN_ON) {
ret = regulator_enable(pcie->regulators[i]);
dev_dbg(dev, "enable regulator %s (%s)\n",
ep_regulator_names[i], ret ? "fail" : "pass");
@@ -1197,7 +1241,7 @@ static int brcm_pcie_suspend(struct device *dev)
brcm_pcie_turn_off(pcie);
ret = brcm_phy_stop(pcie);
clk_disable_unprepare(pcie->clk);
-   brcm_set_regulators(pcie, false);
+   brcm_set_regulators(pcie, TURN_OFF);
 
return ret;
 }
@@ -1210,7 +1254,7 @@ static int brcm_pcie_resume(struct device *dev)
int ret;
 
base = pcie->base;
-   brcm_set_regulators(pcie, true);
+   brcm_set_regulators(pcie, TURN_ON);
clk_prepare_enable(pcie->clk);
 
ret = brcm_phy_start(pcie);
@@ -1249,7 +1293,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
brcm_phy_stop(pcie);
reset_control_assert(pcie->rescal);
clk_disable_unprepare(pcie->clk);
-   brcm_set_regulators(pcie, false);
+   brcm_set_regulators(pcie, TURN_OFF_ALWAYS);
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
@@ -1338,7 +1382,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
return ret;
}
 
-   brcm_set_regulators(pcie, true);
+   brcm_set_regulators(pcie, TURN_ON);
ret = br

[PATCH v1 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Jim Quinlan]

Quite similar to the regulator bindings found in "rockchip-pcie-host.txt",
this allows optional regulators to be attached and controlled by the
PCIe RC driver.

Signed-off-by: Jim Quinlan 
---
 .../devicetree/bindings/pci/brcm,stb-pcie.yaml   | 12 
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml 
b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 807694b4f41f..baacc3d7ec87 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -85,6 +85,18 @@ properties:
   minItems: 1
   maxItems: 3
 
+  vpcie12v-supply:
+description: 12v regulator phandle for the endpoint device
+
+  vpcie3v3-supply:
+description: 3.3v regulator phandle for the endpoint device
+
+  vpcie1v8-supply:
+description: 1.8v regulator phandle for the endpoint device
+
+  vpcie0v9-supply:
+description: 0.9v regulator phandle for the endpoint device
+
 required:
   - reg
   - ranges
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v1 0/6] PCI: brcmstb: add EP regulators and panic handler

[Jim Quinlan]

v1 -- Bindings are added for fixed regulators that may power the EP device.
   -- The brcmstb RC driver is modified to control these regulators
  during probe, suspend, and resume.
   -- 7216 type SOCs have additional error reporting HW and a
  panic handler is added to dump its info.
   -- A missing return value check is added.

Jim Quinlan (6):
  dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators
  PCI: brcmstb: Add control of EP voltage regulator(s)
  PCI: brcmstb: Do not turn off regulators if EP can wake up
  PCI: brcmstb: Give 7216 SOCs their own config type
  PCI: brcmstb: Add panic/die handler to RC driver
  PCI: brcmstb: check return value of clk_prepare_enable()

 .../bindings/pci/brcm,stb-pcie.yaml   |  12 +
 drivers/pci/controller/pcie-brcmstb.c | 247 +-
 2 files changed, 257 insertions(+), 2 deletions(-)

-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v1 5/6] PCI: brcmstb: Add panic/die handler to RC driver

[Jim Quinlan]

Whereas most PCIe HW returns 0x on illegal accesses and the like,
by default Broadcom's STB PCIe controller effects an abort.  This simple
handler determines if the PCIe controller was the cause of the abort and if
so, prints out diagnostic info.

Example output:
  brcm-pcie 8b2.pcie: Error: Mem Acc: 32bit, Read, @0x3800
  brcm-pcie 8b2.pcie:  Type: TO=0 Abt=0 UnspReq=1 AccDsble=0 BadAddr=0

Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 124 ++
 1 file changed, 124 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index e39bd93790d0..469bbb0ebdd9 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -12,11 +12,13 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -187,6 +189,39 @@
 #define  PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_MASK0x1
 #define  PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_SHIFT   0x0
 
+/* Error report regiseters */
+#define PCIE_OUTB_ERR_TREAT0x6000
+#define  PCIE_OUTB_ERR_TREAT_CONFIG_MASK   0x1
+#define  PCIE_OUTB_ERR_TREAT_MEM_MASK  0x2
+#define PCIE_OUTB_ERR_VALID0x6004
+#define PCIE_OUTB_ERR_CLEAR0x6008
+#define PCIE_OUTB_ERR_ACC_INFO 0x600c
+#define  PCIE_OUTB_ERR_ACC_INFO_CFG_ERR_MASK   0x01
+#define  PCIE_OUTB_ERR_ACC_INFO_MEM_ERR_MASK   0x02
+#define  PCIE_OUTB_ERR_ACC_INFO_TYPE_64_MASK   0x04
+#define  PCIE_OUTB_ERR_ACC_INFO_DIR_WRITE_MASK 0x10
+#define  PCIE_OUTB_ERR_ACC_INFO_BYTE_LANES_MASK0xff00
+#define PCIE_OUTB_ERR_ACC_ADDR 0x6010
+#define PCIE_OUTB_ERR_ACC_ADDR_BUS_MASK0xff0
+#define PCIE_OUTB_ERR_ACC_ADDR_DEV_MASK0xf8000
+#define PCIE_OUTB_ERR_ACC_ADDR_FUNC_MASK   0x7000
+#define PCIE_OUTB_ERR_ACC_ADDR_REG_MASK0xfff
+#define PCIE_OUTB_ERR_CFG_CAUSE0x6014
+#define  PCIE_OUTB_ERR_CFG_CAUSE_TIMEOUT_MASK  0x40
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ABORT_MASK0x20
+#define  PCIE_OUTB_ERR_CFG_CAUSE_UNSUPP_REQ_MASK   0x10
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_TIMEOUT_MASK  0x4
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_DISABLED_MASK 0x2
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_64BIT__MASK   0x1
+#define PCIE_OUTB_ERR_MEM_ADDR_LO  0x6018
+#define PCIE_OUTB_ERR_MEM_ADDR_HI  0x601c
+#define PCIE_OUTB_ERR_MEM_CAUSE0x6020
+#define  PCIE_OUTB_ERR_MEM_CAUSE_TIMEOUT_MASK  0x40
+#define  PCIE_OUTB_ERR_MEM_CAUSE_ABORT_MASK0x20
+#define  PCIE_OUTB_ERR_MEM_CAUSE_UNSUPP_REQ_MASK   0x10
+#define  PCIE_OUTB_ERR_MEM_CAUSE_ACC_DISABLED_MASK 0x2
+#define  PCIE_OUTB_ERR_MEM_CAUSE_BAD_ADDR_MASK 0x1
+
 /* Forward declarations */
 struct brcm_pcie;
 static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, 
u32 val);
@@ -229,6 +264,7 @@ struct pcie_cfg_data {
const enum pcie_type type;
void (*perst_set)(struct brcm_pcie *pcie, u32 val);
void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+   const bool has_err_report;
 };
 
 static const int pcie_offsets[] = {
@@ -269,6 +305,7 @@ static const struct pcie_cfg_data bcm7216_cfg = {
.type   = BCM7278,
.perst_set  = brcm_pcie_perst_set_7278,
.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
+   .has_err_report = true,
 };
 
 struct brcm_msi {
@@ -311,8 +348,89 @@ struct brcm_pcie {
struct regulator*regulators[PCIE_REGULATORS_MAX];
int num_regulators;
boolep_wakeup_capable;
+   boolhas_err_report;
+   struct notifier_block   die_notifier;
 };
 
+/*
+ * Dump out pcie errors on die or panic.
+ */
+static int dump_pcie_error(struct notifier_block *self, unsigned long v, void 
*p)
+{
+   const struct brcm_pcie *pcie = container_of(self, struct brcm_pcie, 
die_notifier);
+   void __iomem *base = pcie->base;
+   int i, is_cfg_err, is_mem_err, lanes;
+   char *width_str, *direction_str, lanes_str[9];
+   u32 info;
+
+   if (readl(base + PCIE_OUTB_ERR_VALID) == 0)
+   return NOTIFY_DONE;
+   info = readl(base + PCIE_OUTB_ERR_ACC_INFO);
+
+
+   is_cfg_err = !!(info & PCIE_OUTB_ERR_ACC_INFO_CFG_ERR_MASK);
+   is_mem_err = !!(info & PCIE_OUTB_ERR_ACC_INFO_MEM_ERR_MASK);
+   width_str = (info & PCIE_OUTB_ERR_ACC_INFO_TYPE_64_MASK) ? "64bit" : 
"32bit";
+   direction_str = (info & PCIE_OUTB_ERR_ACC_INFO_DIR_WRITE_MASK) ? 
&quo

[PATCH v1 6/6] PCI: brcmstb: check return value of clk_prepare_enable()

[Jim Quinlan]

The check was missing on PCIe resume.

Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index 469bbb0ebdd9..56c88d2b4f87 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1380,7 +1380,9 @@ static int brcm_pcie_resume(struct device *dev)
 
base = pcie->base;
brcm_set_regulators(pcie, TURN_ON);
-   clk_prepare_enable(pcie->clk);
+   ret = clk_prepare_enable(pcie->clk);
+   if (ret)
+   return ret;
 
ret = brcm_phy_start(pcie);
if (ret)
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v1 4/6] PCI: brcmstb: Give 7216 SOCs their own config type

[Jim Quinlan]

This distinction is required for an imminent commit.

Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index 9b46f0bc..e39bd93790d0 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -264,6 +264,13 @@ static const struct pcie_cfg_data bcm2711_cfg = {
.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 };
 
+static const struct pcie_cfg_data bcm7216_cfg = {
+   .offsets= pcie_offset_bcm7278,
+   .type   = BCM7278,
+   .perst_set  = brcm_pcie_perst_set_7278,
+   .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
+};
+
 struct brcm_msi {
struct device   *dev;
void __iomem*base;
@@ -1312,7 +1319,7 @@ static const struct of_device_id brcm_pcie_match[] = {
{ .compatible = "brcm,bcm2711-pcie", .data = _cfg },
{ .compatible = "brcm,bcm7211-pcie", .data = _cfg },
{ .compatible = "brcm,bcm7278-pcie", .data = _cfg },
-   { .compatible = "brcm,bcm7216-pcie", .data = _cfg },
+   { .compatible = "brcm,bcm7216-pcie", .data = _cfg },
{ .compatible = "brcm,bcm7445-pcie", .data = _cfg },
{},
 };
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH 1/3] serial: 8250: of: Check for CONFIG_SERIAL_8250_BCM7271

[Jim Quinlan]

On Mon, Nov 23, 2020 at 10:58 AM Jim Quinlan  wrote:
>
> On Fri, Nov 20, 2020 at 2:45 PM Al Cooper  wrote:
> >
> > From: Jim Quinlan 
> >
> > This commit has of_platform_serial_probe() check specifically for the
> > "brcm,bcm7271-uart" and whether its companion driver is enabled. If it
> > is the case, and the clock provider is not ready, we want to make sure
> > that when the 8250_bcm7271.c driver returns EPROBE_DEFER, we are not
> > getting the UART registered via 8250_of.c.
> >
> > Signed-off-by: Jim Quinlan 
> > ---
> >  drivers/tty/serial/8250/8250_of.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_of.c 
> > b/drivers/tty/serial/8250/8250_of.c
> > index 65e9045dafe6..aa458f3c6644 100644
> > --- a/drivers/tty/serial/8250/8250_of.c
> > +++ b/drivers/tty/serial/8250/8250_of.c
> > @@ -192,6 +192,10 @@ static int of_platform_serial_probe(struct 
> > platform_device *ofdev)
> > u32 tx_threshold;
> > int ret;
> >
> > +   if (IS_ENABLED(CONFIG_SERIAL_8250_BCM7271) &&
> > +   of_device_is_compatible(ofdev->dev.of_node, 
> > "brcm,bcm7271-uart"))
> > +   return -ENODEV;
> > +
> NOTE: this commit is a "strawman" commit, and I will not be surprised
> if it gets quickly NAKed. We have a new idea on how to solve this
> issue, and if that not is not viable, will ask for a dialog on this
> problem either in this thread or through a separate RFC.


This commit is no longer needed  as part of this patchset; we have
addressed the problem elsewhere.  Sorry about the noise.
>
>
> Regards,
> Jim Quinlan
> Broadcom STB
>
>
>
> > port_type = (unsigned long)of_device_get_match_data(>dev);
> > if (port_type == PORT_UNKNOWN)
> > return -EINVAL;
> > --
> > 2.17.1
> >


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH 1/3] serial: 8250: of: Check for CONFIG_SERIAL_8250_BCM7271

[Jim Quinlan]

On Fri, Nov 20, 2020 at 2:45 PM Al Cooper  wrote:
>
> From: Jim Quinlan 
>
> This commit has of_platform_serial_probe() check specifically for the
> "brcm,bcm7271-uart" and whether its companion driver is enabled. If it
> is the case, and the clock provider is not ready, we want to make sure
> that when the 8250_bcm7271.c driver returns EPROBE_DEFER, we are not
> getting the UART registered via 8250_of.c.
>
> Signed-off-by: Jim Quinlan 
> ---
>  drivers/tty/serial/8250/8250_of.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_of.c 
> b/drivers/tty/serial/8250/8250_of.c
> index 65e9045dafe6..aa458f3c6644 100644
> --- a/drivers/tty/serial/8250/8250_of.c
> +++ b/drivers/tty/serial/8250/8250_of.c
> @@ -192,6 +192,10 @@ static int of_platform_serial_probe(struct 
> platform_device *ofdev)
> u32 tx_threshold;
> int ret;
>
> +   if (IS_ENABLED(CONFIG_SERIAL_8250_BCM7271) &&
> +   of_device_is_compatible(ofdev->dev.of_node, "brcm,bcm7271-uart"))
> +   return -ENODEV;
> +
NOTE: this commit is a "strawman" commit, and I will not be surprised
if it gets quickly NAKed. We have a new idea on how to solve this
issue, and if that not is not viable, will ask for a dialog on this
problem either in this thread or through a separate RFC.

Regards,
Jim Quinlan
Broadcom STB



> port_type = (unsigned long)of_device_get_match_data(>dev);
> if (port_type == PORT_UNKNOWN)
> return -EINVAL;
> --
> 2.17.1
>


smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v3 1/2] ata: ahci_brcm: Fix use of BCM7216 reset controller

[Jim Quinlan]

From: Jim Quinlan 

This driver may use one of two resets controllers.  Keep them in separate
variables to keep things simple.  The reset controller "rescal" is shared
between the AHCI driver and the PCIe driver for the BrcmSTB 7216 chip.  Use
devm_reset_control_get_optional_shared() to handle this sharing.

Fixes: 272ecd60a636 ("ata: ahci_brcm: BCM7216 reset is self de-asserting")
Fixes: c345ec6a50e9 ("ata: ahci_brcm: Support BCM7216 reset controller name")
Signed-off-by: Jim Quinlan 
---
 drivers/ata/ahci_brcm.c | 46 -
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 49f7acbfcf01..bad455fb6ab8 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -86,7 +86,8 @@ struct brcm_ahci_priv {
u32 port_mask;
u32 quirks;
enum brcm_ahci_version version;
-   struct reset_control *rcdev;
+   struct reset_control *rcdev_rescal;
+   struct reset_control *rcdev_ahci;
 };
 
 static inline u32 brcm_sata_readreg(void __iomem *addr)
@@ -352,8 +353,8 @@ static int brcm_ahci_suspend(struct device *dev)
else
ret = 0;
 
-   if (priv->version != BRCM_SATA_BCM7216)
-   reset_control_assert(priv->rcdev);
+   reset_control_assert(priv->rcdev_ahci);
+   reset_control_rearm(priv->rcdev_rescal);
 
return ret;
 }
@@ -365,10 +366,10 @@ static int __maybe_unused brcm_ahci_resume(struct device 
*dev)
struct brcm_ahci_priv *priv = hpriv->plat_data;
int ret = 0;
 
-   if (priv->version == BRCM_SATA_BCM7216)
-   ret = reset_control_reset(priv->rcdev);
-   else
-   ret = reset_control_deassert(priv->rcdev);
+   ret = reset_control_deassert(priv->rcdev_ahci);
+   if (ret)
+   return ret;
+   ret = reset_control_reset(priv->rcdev_rescal);
if (ret)
return ret;
 
@@ -428,7 +429,6 @@ static int brcm_ahci_probe(struct platform_device *pdev)
 {
const struct of_device_id *of_id;
struct device *dev = >dev;
-   const char *reset_name = NULL;
struct brcm_ahci_priv *priv;
struct ahci_host_priv *hpriv;
struct resource *res;
@@ -450,15 +450,15 @@ static int brcm_ahci_probe(struct platform_device *pdev)
if (IS_ERR(priv->top_ctrl))
return PTR_ERR(priv->top_ctrl);
 
-   /* Reset is optional depending on platform and named differently */
-   if (priv->version == BRCM_SATA_BCM7216)
-   reset_name = "rescal";
-   else
-   reset_name = "ahci";
-
-   priv->rcdev = devm_reset_control_get_optional(>dev, reset_name);
-   if (IS_ERR(priv->rcdev))
-   return PTR_ERR(priv->rcdev);
+   if (priv->version == BRCM_SATA_BCM7216) {
+   priv->rcdev_rescal = devm_reset_control_get_optional_shared(
+   >dev, "rescal");
+   if (IS_ERR(priv->rcdev_rescal))
+   return PTR_ERR(priv->rcdev_rescal);
+   }
+   priv->rcdev_ahci = devm_reset_control_get_optional(>dev, "ahci");
+   if (IS_ERR(priv->rcdev_ahci))
+   return PTR_ERR(priv->rcdev_ahci);
 
hpriv = ahci_platform_get_resources(pdev, 0);
if (IS_ERR(hpriv))
@@ -479,10 +479,10 @@ static int brcm_ahci_probe(struct platform_device *pdev)
break;
}
 
-   if (priv->version == BRCM_SATA_BCM7216)
-   ret = reset_control_reset(priv->rcdev);
-   else
-   ret = reset_control_deassert(priv->rcdev);
+   ret = reset_control_reset(priv->rcdev_rescal);
+   if (ret)
+   return ret;
+   ret = reset_control_deassert(priv->rcdev_ahci);
if (ret)
return ret;
 
@@ -527,8 +527,8 @@ static int brcm_ahci_probe(struct platform_device *pdev)
 out_disable_clks:
ahci_platform_disable_clks(hpriv);
 out_reset:
-   if (priv->version != BRCM_SATA_BCM7216)
-   reset_control_assert(priv->rcdev);
+   reset_control_assert(priv->rcdev_ahci);
+   reset_control_rearm(priv->rcdev_rescal);
return ret;
 }
 
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v3 0/2] ata: ahci_brcm: Fix use of BCM7216 reset controller

[Jim Quinlan]

v3 -- discard commit from v2; instead relay on the new function
  reset_control_rearm provided in a recent commit [1] applied
  to reset/next.
   -- New commit to correct pcie-brcmstb.c usage of a reset controller
  to use reset/rearm verses deassert/assert.

v2 -- refactor rescal-reset driver to implement assert/deassert rather than
  reset because the reset call only fires once per lifetime and we need
  to reset after every resume from S2 or S3.
   -- Split the use of "ahci" and "rescal" controllers in separate fields
  to keep things simple.

v1 -- original


[1] Applied commit "reset: make shared pulsed reset controls re-triggerable"
found at git://git.pengutronix.de/git/pza/linux.git
branch reset/shared-retrigger

Jim Quinlan (2):
  ata: ahci_brcm: Fix use of BCM7216 reset controller
  PCI: brcmstb: use reset/rearm instead of deassert/assert

 drivers/ata/ahci_brcm.c   | 46 +--
 drivers/pci/controller/pcie-brcmstb.c | 19 +++
 2 files changed, 36 insertions(+), 29 deletions(-)

-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v3 2/2] PCI: brcmstb: use reset/rearm instead of deassert/assert

[Jim Quinlan]

The Brcmstb PCIe RC uses a reset control "rescal" for certain chips.  This
reset implements a "pulse reset" so it matches more the reset/rearm
calls instead of the deassert/assert calls.

Also, add reset_control calls in suspend/resume functions.

Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index bea86899bd5d..b9745ac2ee0b 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1138,6 +1138,7 @@ static int brcm_pcie_suspend(struct device *dev)
 
brcm_pcie_turn_off(pcie);
ret = brcm_phy_stop(pcie);
+   reset_control_rearm(pcie->rescal);
clk_disable_unprepare(pcie->clk);
 
return ret;
@@ -1153,9 +1154,13 @@ static int brcm_pcie_resume(struct device *dev)
base = pcie->base;
clk_prepare_enable(pcie->clk);
 
+   ret = reset_control_reset(pcie->rescal);
+   if (ret)
+   goto err0;
+
ret = brcm_phy_start(pcie);
if (ret)
-   goto err;
+   goto err1;
 
/* Take bridge out of reset so we can access the SERDES reg */
pcie->bridge_sw_init_set(pcie, 0);
@@ -1170,14 +1175,16 @@ static int brcm_pcie_resume(struct device *dev)
 
ret = brcm_pcie_setup(pcie);
if (ret)
-   goto err;
+   goto err1;
 
if (pcie->msi)
brcm_msi_set_regs(pcie->msi);
 
return 0;
 
-err:
+err1:
+   reset_control_rearm(pcie->rescal);
+err0:
clk_disable_unprepare(pcie->clk);
return ret;
 }
@@ -1187,7 +1194,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
brcm_msi_remove(pcie);
brcm_pcie_turn_off(pcie);
brcm_phy_stop(pcie);
-   reset_control_assert(pcie->rescal);
+   reset_control_rearm(pcie->rescal);
clk_disable_unprepare(pcie->clk);
 }
 
@@ -1262,13 +1269,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
return PTR_ERR(pcie->rescal);
}
 
-   ret = reset_control_deassert(pcie->rescal);
+   ret = reset_control_reset(pcie->rescal);
if (ret)
dev_err(>dev, "failed to deassert 'rescal'\n");
 
ret = brcm_phy_start(pcie);
if (ret) {
-   reset_control_assert(pcie->rescal);
+   reset_control_rearm(pcie->rescal);
clk_disable_unprepare(pcie->clk);
return ret;
}
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v2 1/2] dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport

[Jim Quinlan]

On Thu, Nov 12, 2020 at 12:56 PM Jim Quinlan  wrote:
>
> In normal use of smc/hvc transport in SCMI the message completion is
> indicated by the return of the SMC call.  This commit provides for an
> optional interrupt named "message-serviced" which is used instead to
> indicate the completion of a message.
>
> Signed-off-by: Jim Quinlan 
> ---
>  Documentation/devicetree/bindings/arm/arm,scmi.txt | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt 
> b/Documentation/devicetree/bindings/arm/arm,scmi.txt
> index 55deb68230eb..7cdad11f40b1 100644
> --- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
> +++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
> @@ -31,6 +31,14 @@ Optional properties:
>
>  - mbox-names: shall be "tx" or "rx" depending on mboxes entries.
>
> +- interrupts : when using smc or hvc transports, this optional
> +property indicates that msg completion by the platform is indicated
> +by an interrupt rather than by the return of the smc call. This
> +should not be used except when the platform requires such behavior.
> +
> +- interrupt-names : if "interrupts" is present, interrupt-names must also
> +be present and have the value "message-serviced".
> +
>  See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details
>  about the generic mailbox controller and client driver bindings.
Hi Rob,

Are you okay with this commit?

Regards,
Jim Quinlan
Broadcom STB
>
> --
> 2.17.1
>


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v2 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

On Fri, Nov 20, 2020 at 6:14 AM Sudeep Holla  wrote:
>
> On Thu, Nov 19, 2020 at 01:34:18PM -0500, Jim Quinlan wrote:
> > On Fri, Nov 13, 2020 at 10:12 AM Jim Quinlan  
> > wrote:
> > >
> > > On Fri, Nov 13, 2020 at 9:36 AM Sudeep Holla  wrote:
> > > >
> > > > On Fri, Nov 13, 2020 at 09:26:43AM -0500, Jim Quinlan wrote:
> > > > > Hi, these are fast calls.  Regards, Jim
> > > > >
> > > > >
> > > > > On Fri, Nov 13, 2020 at 4:47 AM Sudeep Holla  
> > > > > wrote:
> > > > > >
> > > > > > On Thu, Nov 12, 2020 at 12:56:27PM -0500, Jim Quinlan wrote:
> > > > > > > The SMC/HVC SCMI transport is modified to allow the completion of 
> > > > > > > an SCMI
> > > > > > > message to be indicated by an interrupt rather than the return of 
> > > > > > > the smc
> > > > > > > call.  This accommodates the existing behavior of the BrcmSTB SCMI
> > > > > > > "platform" whose SW is already out in the field and cannot be 
> > > > > > > changed.
> > > > > > >
> > > > > >
> > > > > > Sorry for missing to check with you earlier. Are these not fast smc 
> > > > > > calls ?
> > > > > > Can we check the SMC Function IDs for the same and expect IRQ to be 
> > > > > > present
> > > > > > if they are not fast calls ?
> > > > > Hi, if I understand you correctly you want to do something like this:
> > > > >
> > > > >  if (! ARM_SMCCC_IS_FAST_CALL(func_id)) {
> > > > > /* look for irq and request it */
> > > > > }
> > > > >
> > > >
> > > > Yes.
> > > >
> > > > > But we  do use fast calls.
> > > >
> > > > What was the rationale for retaining fast SMC calls but use IRQ for Tx
> > > > completion ?
> > > >
> > > > Is it because you offload it to some other microprocessor and don't
> > > > continue execution on secure side in whcih case you can afford fast 
> > > > call ?
> > Hi Sudeep,
> >
>
> Thanks for the details. Unfortunately more questions:
>
> > Here is my understanding:  Some SMC calls may take a few longer to
> > complete than others. The longer ones tie up the CPU core that is
> > handling the SMC call, and so nothing can be scheduled on that
> > specific core.
>
> So far good.
>
> > Unfortunately, we have a real-time OS that runs
> > sporadically on one specific core and if that happens to be the same
> > core that is handling the SMC, the RTOS will miss its deadline.  So we
> > need to have the SMC return immediately and use an SGI for task
> > completion.
> >
>
> So it sounds more like it can't be fast call then.
Hi Sudeep,

To be honest,  I'm not sure what the big difference between fast and
slow SMC calls are other than the latter is "yielding" and
interruptible.  We cannot tolerate them being interruptible.

>
> Does that me, it will always return early and send SGI when the request
> is complete ?
Most calls send the SGI and return immediately.  The ones that take
longer return from the SMC and send the SGI when the operation is
completed.
>
> 1. If yes, what happens if there are multiple requests in parallel and
>second one completes before the first. Can we handle that with this
>patch set. Of will the second request fails until the first one is
>complete ? It extends to number of cpus in the system worst case.
With SCMI we only have one message pending  at  a time;  perhaps I do
not understand your question.  Having the SMC return is mostly a no-op
as far as the SCMI  driver is concerned.

Our SCMI messages cannot fail.  When we do have timeouts it indicates
that something is wrong and needs to be fixed.

Regards,
Jim Quinlan
Broadcom STB
>
> 2. If no, will this not cause issues if we unconditional wait for interrupt
>every single time ?
>
> --
> Regards,
> Sudeep


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v2 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

On Fri, Nov 13, 2020 at 10:12 AM Jim Quinlan  wrote:
>
> On Fri, Nov 13, 2020 at 9:36 AM Sudeep Holla  wrote:
> >
> > On Fri, Nov 13, 2020 at 09:26:43AM -0500, Jim Quinlan wrote:
> > > Hi, these are fast calls.  Regards, Jim
> > >
> > >
> > > On Fri, Nov 13, 2020 at 4:47 AM Sudeep Holla  wrote:
> > > >
> > > > On Thu, Nov 12, 2020 at 12:56:27PM -0500, Jim Quinlan wrote:
> > > > > The SMC/HVC SCMI transport is modified to allow the completion of an 
> > > > > SCMI
> > > > > message to be indicated by an interrupt rather than the return of the 
> > > > > smc
> > > > > call.  This accommodates the existing behavior of the BrcmSTB SCMI
> > > > > "platform" whose SW is already out in the field and cannot be changed.
> > > > >
> > > >
> > > > Sorry for missing to check with you earlier. Are these not fast smc 
> > > > calls ?
> > > > Can we check the SMC Function IDs for the same and expect IRQ to be 
> > > > present
> > > > if they are not fast calls ?
> > > Hi, if I understand you correctly you want to do something like this:
> > >
> > >  if (! ARM_SMCCC_IS_FAST_CALL(func_id)) {
> > > /* look for irq and request it */
> > > }
> > >
> >
> > Yes.
> >
> > > But we  do use fast calls.
> >
> > What was the rationale for retaining fast SMC calls but use IRQ for Tx
> > completion ?
> >
> > Is it because you offload it to some other microprocessor and don't
> > continue execution on secure side in whcih case you can afford fast call ?
Hi Sudeep,
Here is my understanding:  Some SMC calls may take a few longer to
complete than others.  The longer ones tie up the CPU core that is
handling the SMC call, and so nothing can be scheduled on that
specific core.  Unfortunately, we have a real-time OS that runs
sporadically on one specific core and if that happens to be the same
core that is handling the SMC, the RTOS will miss its deadline.  So we
need to have the SMC return immediately and use an SGI for task
completion.

Regards,
Jim Quinlan
Broadcom STB


>
> Hi Sudeep,
> I have an answer for this but allow me time to contact the platform FW
> engineer to make sure I have the full picture -- this may take a day
> or two.  Regardless, our implementation has already "shipped" to
> customers for some time so we may not be able to change it.
> Regards, Jim
> >
> >
> > --
> > Regards,
> > Sudeep


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v2 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

On Fri, Nov 13, 2020 at 9:36 AM Sudeep Holla  wrote:
>
> On Fri, Nov 13, 2020 at 09:26:43AM -0500, Jim Quinlan wrote:
> > Hi, these are fast calls.  Regards, Jim
> >
> >
> > On Fri, Nov 13, 2020 at 4:47 AM Sudeep Holla  wrote:
> > >
> > > On Thu, Nov 12, 2020 at 12:56:27PM -0500, Jim Quinlan wrote:
> > > > The SMC/HVC SCMI transport is modified to allow the completion of an 
> > > > SCMI
> > > > message to be indicated by an interrupt rather than the return of the 
> > > > smc
> > > > call.  This accommodates the existing behavior of the BrcmSTB SCMI
> > > > "platform" whose SW is already out in the field and cannot be changed.
> > > >
> > >
> > > Sorry for missing to check with you earlier. Are these not fast smc calls 
> > > ?
> > > Can we check the SMC Function IDs for the same and expect IRQ to be 
> > > present
> > > if they are not fast calls ?
> > Hi, if I understand you correctly you want to do something like this:
> >
> >  if (! ARM_SMCCC_IS_FAST_CALL(func_id)) {
> > /* look for irq and request it */
> > }
> >
>
> Yes.
>
> > But we  do use fast calls.
>
> What was the rationale for retaining fast SMC calls but use IRQ for Tx
> completion ?
>
> Is it because you offload it to some other microprocessor and don't
> continue execution on secure side in whcih case you can afford fast call ?

Hi Sudeep,
I have an answer for this but allow me time to contact the platform FW
engineer to make sure I have the full picture -- this may take a day
or two.  Regardless, our implementation has already "shipped" to
customers for some time so we may not be able to change it.
Regards, Jim
>
>
> --
> Regards,
> Sudeep


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v2 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

Hi, these are fast calls.  Regards, Jim


On Fri, Nov 13, 2020 at 4:47 AM Sudeep Holla  wrote:
>
> On Thu, Nov 12, 2020 at 12:56:27PM -0500, Jim Quinlan wrote:
> > The SMC/HVC SCMI transport is modified to allow the completion of an SCMI
> > message to be indicated by an interrupt rather than the return of the smc
> > call.  This accommodates the existing behavior of the BrcmSTB SCMI
> > "platform" whose SW is already out in the field and cannot be changed.
> >
>
> Sorry for missing to check with you earlier. Are these not fast smc calls ?
> Can we check the SMC Function IDs for the same and expect IRQ to be present
> if they are not fast calls ?
Hi, if I understand you correctly you want to do something like this:

 if (! ARM_SMCCC_IS_FAST_CALL(func_id)) {
/* look for irq and request it */
}

But we  do use fast calls.
Regards,
Jim

>
> --
> Regards,
> Sudeep


smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v2 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

The SMC/HVC SCMI transport is modified to allow the completion of an SCMI
message to be indicated by an interrupt rather than the return of the smc
call.  This accommodates the existing behavior of the BrcmSTB SCMI
"platform" whose SW is already out in the field and cannot be changed.

Signed-off-by: Jim Quinlan 
---
 drivers/firmware/arm_scmi/smc.c | 38 -
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 82a82a5dc86a..68ee6eb5fc27 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -9,9 +9,11 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "common.h"
@@ -23,6 +25,8 @@
  * @shmem: Transmit/Receive shared memory area
  * @shmem_lock: Lock to protect access to Tx/Rx shared memory area
  * @func_id: smc/hvc call function id
+ * @irq: Optional; employed when platforms indicates msg completion by intr.
+ * @tx_complete: Optional, employed only when irq is valid.
  */
 
 struct scmi_smc {
@@ -30,8 +34,19 @@ struct scmi_smc {
struct scmi_shared_mem __iomem *shmem;
struct mutex shmem_lock;
u32 func_id;
+   int irq;
+   struct completion tx_complete;
 };
 
+static irqreturn_t smc_msg_done_isr(int irq, void *data)
+{
+   struct scmi_smc *scmi_info = data;
+
+   complete(_info->tx_complete);
+
+   return IRQ_HANDLED;
+}
+
 static bool smc_chan_available(struct device *dev, int idx)
 {
struct device_node *np = of_parse_phandle(dev->of_node, "shmem", 0);
@@ -51,7 +66,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, 
struct device *dev,
struct resource res;
struct device_node *np;
u32 func_id;
-   int ret;
+   int ret, irq = 0;
 
if (!tx)
return -ENODEV;
@@ -79,10 +94,29 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, 
struct device *dev,
if (ret < 0)
return ret;
 
+   /*
+* If there is an interrupt named "message-serviced", then the
+* service and completion of a message is signaled by an interrupt
+* rather than by the return of the SMC call.
+*/
+   ret = of_irq_get_byname(cdev->of_node, "message-serviced");
+   if (ret > 0) {
+   irq = ret;
+   ret = devm_request_irq(dev, irq, smc_msg_done_isr,
+  IRQF_NO_SUSPEND,
+  dev_name(dev),
+  scmi_info);
+   if (ret) {
+   dev_err(dev, "failed to setup SCMI smc irq\n");
+   return ret;
+   }
+   init_completion(_info->tx_complete);
+   }
scmi_info->func_id = func_id;
scmi_info->cinfo = cinfo;
mutex_init(_info->shmem_lock);
cinfo->transport_info = scmi_info;
+   scmi_info->irq = irq;
 
return 0;
 }
@@ -111,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
shmem_tx_prepare(scmi_info->shmem, xfer);
 
arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, );
+   if (scmi_info->irq)
+   wait_for_completion(_info->tx_complete);
scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem));
 
mutex_unlock(_info->shmem_lock);
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v2 1/2] dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport

[Jim Quinlan]

In normal use of smc/hvc transport in SCMI the message completion is
indicated by the return of the SMC call.  This commit provides for an
optional interrupt named "message-serviced" which is used instead to
indicate the completion of a message.

Signed-off-by: Jim Quinlan 
---
 Documentation/devicetree/bindings/arm/arm,scmi.txt | 8 
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt 
b/Documentation/devicetree/bindings/arm/arm,scmi.txt
index 55deb68230eb..7cdad11f40b1 100644
--- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
@@ -31,6 +31,14 @@ Optional properties:
 
 - mbox-names: shall be "tx" or "rx" depending on mboxes entries.
 
+- interrupts : when using smc or hvc transports, this optional
+property indicates that msg completion by the platform is indicated
+by an interrupt rather than by the return of the smc call. This
+should not be used except when the platform requires such behavior.
+
+- interrupt-names : if "interrupts" is present, interrupt-names must also
+be present and have the value "message-serviced".
+
 See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details
 about the generic mailbox controller and client driver bindings.
 
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v2 0/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

v2 -- Correct commit message, s/msg/message/, and remove extra WS on
  "dt-bindings" commit (Sudeep)
   -- Change interrupt name to "message-serviced", move irq assignent to end
  of function. (Sudeep)

v1 -- original.

Jim Quinlan (2):
  dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport
  firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

 .../devicetree/bindings/arm/arm,scmi.txt  |  8 
 drivers/firmware/arm_scmi/smc.c   | 38 ++-
 2 files changed, 45 insertions(+), 1 deletion(-)

-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v1 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

On Wed, Nov 11, 2020 at 5:42 AM Sudeep Holla  wrote:
>
> On Tue, Nov 10, 2020 at 01:38:19PM -0500, Jim Quinlan wrote:
> > The SMC/HVC SCMI transport is modified to allow the completion of an SCMI
> > message to be indicated by an interrupt rather than the return of the smc
> > call.  This accommodates the existing behavior of the BrcmSTB SCMI
> > "platform" whose SW is already out in the field and cannot be changed.
> >
> > Signed-off-by: Jim Quinlan 
> > ---
> >  drivers/firmware/arm_scmi/smc.c | 31 +++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/smc.c 
> > b/drivers/firmware/arm_scmi/smc.c
> > index 82a82a5dc86a..3bf935dbd00e 100644
> > --- a/drivers/firmware/arm_scmi/smc.c
> > +++ b/drivers/firmware/arm_scmi/smc.c
> > @@ -9,9 +9,11 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >
> >  #include "common.h"
> > @@ -23,6 +25,8 @@
> >   * @shmem: Transmit/Receive shared memory area
> >   * @shmem_lock: Lock to protect access to Tx/Rx shared memory area
> >   * @func_id: smc/hvc call function id
> > + * @irq: Optional; employed when platforms indicates msg completion by 
> > intr.
> > + * @tx_complete: Optional, employed only when irq is valid.
> >   */
> >
> >  struct scmi_smc {
> > @@ -30,8 +34,19 @@ struct scmi_smc {
> >   struct scmi_shared_mem __iomem *shmem;
> >   struct mutex shmem_lock;
> >   u32 func_id;
> > + int irq;
> > + struct completion tx_complete;
> >  };
> >
> > +static irqreturn_t smc_msg_done_isr(int irq, void *data)
> > +{
> > + struct scmi_smc *scmi_info = data;
> > +
> > + complete(_info->tx_complete);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> >  static bool smc_chan_available(struct device *dev, int idx)
> >  {
> >   struct device_node *np = of_parse_phandle(dev->of_node, "shmem", 0);
> > @@ -79,6 +94,20 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, 
> > struct device *dev,
> >   if (ret < 0)
> >   return ret;
> >
> > + /* Optional feature -- signal message completion using an interrupt */
> > + ret = of_irq_get_byname(cdev->of_node, "msg-serviced");
>
> So, looks like it is mandatory if "interrupts" is used. Please update the
> binding or if that is not the practice followed elsewhere, drop search by
> name.

Well, I can certainly change the comment. I do not want it to be
"mandatory" if just interrupts are used.
 The reason I prefer using "interrupt-names" is that this allows
unforeseen use of future additional interrupts w/o caring about order
in the interrupts DT node.  If you are absolutely positive that there
will never be other interrupts used  in the future for the SCMI node
then I will drop it.

>
> Also I prefer full name "message-serviced" if possible, not strong
> opinion.

Will do.

>
>
> > + if (ret > 0) {
> > + scmi_info->irq = ret;
>
> May be set this only if we succeed setting up handler ? I mean move this
> down.

Will do.

Regards,
Jim Quinlan
Broadcom STB


>
>
> Other than these, the changes look fine.
>
> --
> Regards,
> Sudeep


smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v1 1/2] dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport

[Jim Quinlan]

This change allows the SCMI "platform" to use the smc/hvc transport and to
optionally indicate the completion of an SCMI message with an interrupt.
This is in contrast to the nominal case where the return of the SMC call
indicates message completion.

Signed-off-by: Jim Quinlan 
---
 Documentation/devicetree/bindings/arm/arm,scmi.txt | 8 
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt 
b/Documentation/devicetree/bindings/arm/arm,scmi.txt
index 55deb68230eb..d3b0c9f387fe 100644
--- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
@@ -31,6 +31,14 @@ Optional properties:
 
 - mbox-names: shall be "tx" or "rx" depending on mboxes entries.
 
+- interrupts : when using smc or hvc transports, this optional
+property indicates that msg completion by the platform is indicated
+by an interrupt rather than by the return of the smc call. This
+should not be used except when the platform requires such behavior.
+
+- interrupt-names : the name must be "msg-serviced".
+
+
 See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details
 about the generic mailbox controller and client driver bindings.
 
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v1 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

The SMC/HVC SCMI transport is modified to allow the completion of an SCMI
message to be indicated by an interrupt rather than the return of the smc
call.  This accommodates the existing behavior of the BrcmSTB SCMI
"platform" whose SW is already out in the field and cannot be changed.

Signed-off-by: Jim Quinlan 
---
 drivers/firmware/arm_scmi/smc.c | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 82a82a5dc86a..3bf935dbd00e 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -9,9 +9,11 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "common.h"
@@ -23,6 +25,8 @@
  * @shmem: Transmit/Receive shared memory area
  * @shmem_lock: Lock to protect access to Tx/Rx shared memory area
  * @func_id: smc/hvc call function id
+ * @irq: Optional; employed when platforms indicates msg completion by intr.
+ * @tx_complete: Optional, employed only when irq is valid.
  */
 
 struct scmi_smc {
@@ -30,8 +34,19 @@ struct scmi_smc {
struct scmi_shared_mem __iomem *shmem;
struct mutex shmem_lock;
u32 func_id;
+   int irq;
+   struct completion tx_complete;
 };
 
+static irqreturn_t smc_msg_done_isr(int irq, void *data)
+{
+   struct scmi_smc *scmi_info = data;
+
+   complete(_info->tx_complete);
+
+   return IRQ_HANDLED;
+}
+
 static bool smc_chan_available(struct device *dev, int idx)
 {
struct device_node *np = of_parse_phandle(dev->of_node, "shmem", 0);
@@ -79,6 +94,20 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, 
struct device *dev,
if (ret < 0)
return ret;
 
+   /* Optional feature -- signal message completion using an interrupt */
+   ret = of_irq_get_byname(cdev->of_node, "msg-serviced");
+   if (ret > 0) {
+   scmi_info->irq = ret;
+   ret = devm_request_irq(dev, ret, smc_msg_done_isr,
+  IRQF_NO_SUSPEND,
+  dev_name(dev),
+  scmi_info);
+   if (ret) {
+   dev_err(dev, "failed to setup SCMI smc irq\n");
+   return ret;
+   }
+   init_completion(_info->tx_complete);
+   }
scmi_info->func_id = func_id;
scmi_info->cinfo = cinfo;
mutex_init(_info->shmem_lock);
@@ -111,6 +140,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
shmem_tx_prepare(scmi_info->shmem, xfer);
 
arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, );
+   if (scmi_info->irq)
+   wait_for_completion(_info->tx_complete);
scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem));
 
mutex_unlock(_info->shmem_lock);
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v2 1/2] reset: brcmstb rescal: implement {de}assert() instead of reset()

[Jim Quinlan]

On Mon, Nov 9, 2020 at 5:05 AM Philipp Zabel  wrote:
>
> Hi Jim,
>
> On Fri, 2020-11-06 at 14:17 -0500, Jim Quinlan wrote:
> > Before, only control_reset() was implemented.  However, the reset core only
> > invokes control_reset() once in its lifetime.  Because we need it to invoke
> > control_reset() again after resume out of S2 or S3, we have switched to
> > putting the reset functionality into the control_deassert() method and
> > having an empty control_assert() method.
> >
> > Signed-off-by: Jim Quinlan 
>
> You are switching to the wrong abstraction to work around a deficiency
> of the reset controller framework. Instead, it would be better to allow
> to "reactivate" shared pulsed resets so they can be triggered again.


True.
>
>
> Could you please have a look at [1], which tries to implement this with
> a new API call, and check if this can fix your problem? If so, it would
> be great if you could coordinate with Amjad to see this fixed in the
> core.
>
> [1] 
> https://lore.kernel.org/lkml/20201001132758.12280-1-aouledam...@baylibre.com/


Yes, this would work for our usage.  Amjad please let me know if I can
help here.  The only "nit" I have is that I favor the name 'unreset'
over 'resettable' but truly I don't care one way or the other.

Thanks and kind regards,
Jim Quinaln
Broadcom STB
>
>
>
> regards
> Philipp
>
> > ---
> >  drivers/reset/reset-brcmstb-rescal.c | 13 ++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/reset/reset-brcmstb-rescal.c 
> > b/drivers/reset/reset-brcmstb-rescal.c
> > index b6f074d6a65f..1f54ae4f91fe 100644
> > --- a/drivers/reset/reset-brcmstb-rescal.c
> > +++ b/drivers/reset/reset-brcmstb-rescal.c
> > @@ -20,8 +20,8 @@ struct brcm_rescal_reset {
> >   struct reset_controller_dev rcdev;
> >  };
> >
> > -static int brcm_rescal_reset_set(struct reset_controller_dev *rcdev,
> > -  unsigned long id)
> > +static int brcm_rescal_reset_deassert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> >  {
> >   struct brcm_rescal_reset *data =
> >   container_of(rcdev, struct brcm_rescal_reset, rcdev);
> > @@ -52,6 +52,12 @@ static int brcm_rescal_reset_set(struct 
> > reset_controller_dev *rcdev,
> >   return 0;
> >  }
> >
> > +static int brcm_rescal_reset_assert(struct reset_controller_dev *rcdev,
> > +   unsigned long id)
> > +{
> > + return 0;
> > +}
> > +
> >  static int brcm_rescal_reset_xlate(struct reset_controller_dev *rcdev,
> >  const struct of_phandle_args *reset_spec)
> >  {
> > @@ -60,7 +66,8 @@ static int brcm_rescal_reset_xlate(struct 
> > reset_controller_dev *rcdev,
> >  }
> >
> >  static const struct reset_control_ops brcm_rescal_reset_ops = {
> > - .reset = brcm_rescal_reset_set,
> > + .deassert = brcm_rescal_reset_deassert,
> > + .assert = brcm_rescal_reset_assert,
> >  };
> >
> >  static int brcm_rescal_reset_probe(struct platform_device *pdev)


smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v2 2/2] ata: ahci_brcm: Fix use of BCM7216 reset controller

[Jim Quinlan]

From: Jim Quinlan 

This driver may use one of two resets controllers.  Keep them in separate
variables to keep things simple.  The reset controller "rescal" is shared
between the AHCI driver and the PCIe driver for the BrcmSTB 7216 chip.  Use
devm_reset_control_get_optional_shared() to handle this sharing.

Fixes: 272ecd60a636 ("ata: ahci_brcm: BCM7216 reset is self de-asserting")
Fixes: c345ec6a50e9 ("ata: ahci_brcm: Support BCM7216 reset controller name")
Signed-off-by: Jim Quinlan 
---
 drivers/ata/ahci_brcm.c | 46 -
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 49f7acbfcf01..58eae075d790 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -86,7 +86,8 @@ struct brcm_ahci_priv {
u32 port_mask;
u32 quirks;
enum brcm_ahci_version version;
-   struct reset_control *rcdev;
+   struct reset_control *rcdev_rescal;
+   struct reset_control *rcdev_ahci;
 };
 
 static inline u32 brcm_sata_readreg(void __iomem *addr)
@@ -352,8 +353,8 @@ static int brcm_ahci_suspend(struct device *dev)
else
ret = 0;
 
-   if (priv->version != BRCM_SATA_BCM7216)
-   reset_control_assert(priv->rcdev);
+   reset_control_assert(priv->rcdev_ahci);
+   reset_control_assert(priv->rcdev_rescal);
 
return ret;
 }
@@ -365,10 +366,10 @@ static int __maybe_unused brcm_ahci_resume(struct device 
*dev)
struct brcm_ahci_priv *priv = hpriv->plat_data;
int ret = 0;
 
-   if (priv->version == BRCM_SATA_BCM7216)
-   ret = reset_control_reset(priv->rcdev);
-   else
-   ret = reset_control_deassert(priv->rcdev);
+   ret = reset_control_deassert(priv->rcdev_ahci);
+   if (ret)
+   return ret;
+   ret = reset_control_deassert(priv->rcdev_rescal);
if (ret)
return ret;
 
@@ -428,7 +429,6 @@ static int brcm_ahci_probe(struct platform_device *pdev)
 {
const struct of_device_id *of_id;
struct device *dev = >dev;
-   const char *reset_name = NULL;
struct brcm_ahci_priv *priv;
struct ahci_host_priv *hpriv;
struct resource *res;
@@ -450,15 +450,15 @@ static int brcm_ahci_probe(struct platform_device *pdev)
if (IS_ERR(priv->top_ctrl))
return PTR_ERR(priv->top_ctrl);
 
-   /* Reset is optional depending on platform and named differently */
-   if (priv->version == BRCM_SATA_BCM7216)
-   reset_name = "rescal";
-   else
-   reset_name = "ahci";
-
-   priv->rcdev = devm_reset_control_get_optional(>dev, reset_name);
-   if (IS_ERR(priv->rcdev))
-   return PTR_ERR(priv->rcdev);
+   if (priv->version == BRCM_SATA_BCM7216) {
+   priv->rcdev_rescal = devm_reset_control_get_optional_shared(
+   >dev, "rescal");
+   if (IS_ERR(priv->rcdev_rescal))
+   return PTR_ERR(priv->rcdev_rescal);
+   }
+   priv->rcdev_ahci = devm_reset_control_get_optional(>dev, "ahci");
+   if (IS_ERR(priv->rcdev_ahci))
+   return PTR_ERR(priv->rcdev_ahci);
 
hpriv = ahci_platform_get_resources(pdev, 0);
if (IS_ERR(hpriv))
@@ -479,10 +479,10 @@ static int brcm_ahci_probe(struct platform_device *pdev)
break;
}
 
-   if (priv->version == BRCM_SATA_BCM7216)
-   ret = reset_control_reset(priv->rcdev);
-   else
-   ret = reset_control_deassert(priv->rcdev);
+   ret = reset_control_deassert(priv->rcdev_rescal);
+   if (ret)
+   return ret;
+   ret = reset_control_deassert(priv->rcdev_ahci);
if (ret)
return ret;
 
@@ -527,8 +527,8 @@ static int brcm_ahci_probe(struct platform_device *pdev)
 out_disable_clks:
ahci_platform_disable_clks(hpriv);
 out_reset:
-   if (priv->version != BRCM_SATA_BCM7216)
-   reset_control_assert(priv->rcdev);
+   reset_control_assert(priv->rcdev_ahci);
+   reset_control_assert(priv->rcdev_rescal);
return ret;
 }
 
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v2 1/2] reset: brcmstb rescal: implement {de}assert() instead of reset()

[Jim Quinlan]

Before, only control_reset() was implemented.  However, the reset core only
invokes control_reset() once in its lifetime.  Because we need it to invoke
control_reset() again after resume out of S2 or S3, we have switched to
putting the reset functionality into the control_deassert() method and
having an empty control_assert() method.

Signed-off-by: Jim Quinlan 
---
 drivers/reset/reset-brcmstb-rescal.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/reset/reset-brcmstb-rescal.c 
b/drivers/reset/reset-brcmstb-rescal.c
index b6f074d6a65f..1f54ae4f91fe 100644
--- a/drivers/reset/reset-brcmstb-rescal.c
+++ b/drivers/reset/reset-brcmstb-rescal.c
@@ -20,8 +20,8 @@ struct brcm_rescal_reset {
struct reset_controller_dev rcdev;
 };
 
-static int brcm_rescal_reset_set(struct reset_controller_dev *rcdev,
-unsigned long id)
+static int brcm_rescal_reset_deassert(struct reset_controller_dev *rcdev,
+   unsigned long id)
 {
struct brcm_rescal_reset *data =
container_of(rcdev, struct brcm_rescal_reset, rcdev);
@@ -52,6 +52,12 @@ static int brcm_rescal_reset_set(struct reset_controller_dev 
*rcdev,
return 0;
 }
 
+static int brcm_rescal_reset_assert(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+   return 0;
+}
+
 static int brcm_rescal_reset_xlate(struct reset_controller_dev *rcdev,
   const struct of_phandle_args *reset_spec)
 {
@@ -60,7 +66,8 @@ static int brcm_rescal_reset_xlate(struct 
reset_controller_dev *rcdev,
 }
 
 static const struct reset_control_ops brcm_rescal_reset_ops = {
-   .reset = brcm_rescal_reset_set,
+   .deassert = brcm_rescal_reset_deassert,
+   .assert = brcm_rescal_reset_assert,
 };
 
 static int brcm_rescal_reset_probe(struct platform_device *pdev)
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v2 0/2] ata: ahci_brcm: Fix use of BCM7216 reset controller

[Jim Quinlan]

v2 -- refactor rescal-reset driver to implement assert/deassert rather than
  reset because the reset call only fires once per lifetime and we need
  to reset after every resume from S2 or S3.
   -- Split the use of "ahci" and "rescal" controllers in separate fields
  to keep things simple.

v1 -- original

Jim Quinlan (2):
  reset: brcmstb rescal: implement {de}assert() instead of reset()
  ata: ahci_brcm: Fix use of BCM7216 reset controller

 drivers/ata/ahci_brcm.c  | 46 ++--
 drivers/reset/reset-brcmstb-rescal.c | 13 ++--
 2 files changed, 33 insertions(+), 26 deletions(-)

-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v4 1/2] dt-bindings: Add bindings for BrcmSTB SCMI mailbox driver

[Jim Quinlan]

On Thu, Nov 5, 2020 at 1:27 PM Sudeep Holla  wrote:
>
> On Thu, Nov 05, 2020 at 10:28:25AM -0500, Jim Quinlan wrote:
> > On Thu, Nov 5, 2020 at 10:13 AM Rob Herring  wrote:
> > >
> > > On Wed, Nov 4, 2020 at 4:04 PM Jim Quinlan  
> > > wrote:
> > > >
> > > > On Wed, Nov 4, 2020 at 4:50 PM Rob Herring  wrote:
> > > > >
> > > > > On Thu, Oct 29, 2020 at 03:59:06PM -0400, Jim Quinlan wrote:
> > > > > > Bindings are added.  Only one interrupt is needed because
> > > > > > we do not yet employ the SCMI p2a channel.
> > > > >
> > > > > I still don't understand what this is. To repeat from v1: I thought 
> > > > > SCMI
> > > > > was a mailbox consumer, not provider?
> > > >
> > > > Hi Rob,
> > > >
> > > > I'm not sure where I am implying that SCMI is a mailbox provider?
> > > > Should I not mention "SCMI" in the subject line?
> > > >
> > > > This is just a mailbox driver, "consumed" by SCMI.Our SCMI DT node
> > > > looks like this:
> > > >
> > > > brcm_scmi_mailbox: brcm_scmi_mailbox@0 {
> > > > #mbox-cells = <1>;
> > > > compatible = "brcm,brcmstb-mbox";
> > > > };
> > > >
> > > > brcm_scmi@0 {
> > > > compatible = "arm,scmi";
> > > > mboxes = <_scmi_mailbox 0>;;
> > > > mbox-names = "tx";
> > > > shmem = <>;
> > > > /* ... */
> > > > };
> > >
> > > Okay, that makes more sense. Though it seems like this is just adding
> > > a pointless level of indirection to turn an interrupt into a mailbox.
> > > There's nothing more to 'the mailbox' is there?
> >
> > Correct.  Although you can see that it uses both interrupts and SMC
> > calls to get the job done.
> >
>
> I was against having 2 separate solutions and would have raised my concern
> again. As I mentioned earlier, either extend what we have or move the
> existing SMC solution into this mailbox driver. Having 2 different solution
> for this just because you have extra interrupt to deal with is definite
> NACK from me as I had previously mentioned.
>
> > > So why not either
> > > allow SCMI to have an interrupt directly
> > Not sure here -- perhaps the SCMI folks have an answer?
> >
>
> I did ask why can't you extend the existing SCMI/SMC binding to add this
> as optional feature ?
Hi Sudeep,

Looking at the email you said, "In that case any reason why you can't
reuse the existing smc transport for SCMI." ,  and I replied with the
reason.  I did not interpret your statement  above as what you are
clearly saying now: "either extend what we have or move the existing
SMC solution into this mailbox driver. "

Fair enough, I  will look into this.

Regards,
Jim


>
> > > or have a generic irq mailbox driver?
>
> Fine with this too.
>
> > The SCMI implementation doesn't offer a generic irq mailbox driver
> > AFAICT.  The SCMI folks recently provided  an "smc transport" driver
> > in "drivers/firmware/arm_scmi/smc.c" -- it is close to what we need
> > but is missing interrupts.
>
> IIRC, you were using SGIs and it can't be represented and use today as
> is ? Am  I missing something or anything has changed ?
>
> --
> Regards,
> Sudeep


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v4 1/2] dt-bindings: Add bindings for BrcmSTB SCMI mailbox driver

[Jim Quinlan]

On Thu, Nov 5, 2020 at 10:13 AM Rob Herring  wrote:
>
> On Wed, Nov 4, 2020 at 4:04 PM Jim Quinlan  wrote:
> >
> > On Wed, Nov 4, 2020 at 4:50 PM Rob Herring  wrote:
> > >
> > > On Thu, Oct 29, 2020 at 03:59:06PM -0400, Jim Quinlan wrote:
> > > > Bindings are added.  Only one interrupt is needed because
> > > > we do not yet employ the SCMI p2a channel.
> > >
> > > I still don't understand what this is. To repeat from v1: I thought SCMI
> > > was a mailbox consumer, not provider?
> >
> > Hi Rob,
> >
> > I'm not sure where I am implying that SCMI is a mailbox provider?
> > Should I not mention "SCMI" in the subject line?
> >
> > This is just a mailbox driver, "consumed" by SCMI.Our SCMI DT node
> > looks like this:
> >
> > brcm_scmi_mailbox: brcm_scmi_mailbox@0 {
> > #mbox-cells = <1>;
> > compatible = "brcm,brcmstb-mbox";
> > };
> >
> > brcm_scmi@0 {
> > compatible = "arm,scmi";
> > mboxes = <_scmi_mailbox 0>;;
> > mbox-names = "tx";
> > shmem = <>;
> > /* ... */
> > };
>
> Okay, that makes more sense. Though it seems like this is just adding
> a pointless level of indirection to turn an interrupt into a mailbox.
> There's nothing more to 'the mailbox' is there?
Correct.  Although you can see that it uses both interrupts and SMC
calls to get the job done.

> So why not either
> allow SCMI to have an interrupt directly
Not sure here -- perhaps the SCMI folks have an answer?

 > or have a generic irq mailbox
> driver?
The SCMI implementation doesn't offer a generic irq mailbox driver
AFAICT.  The SCMI folks recently provided  an "smc transport" driver
in "drivers/firmware/arm_scmi/smc.c" -- it is close to what we need
but is missing interrupts.

Regards,
Jim Quinlan
Broadcom STB

>
> Rob


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v4 1/2] dt-bindings: Add bindings for BrcmSTB SCMI mailbox driver

[Jim Quinlan]

On Wed, Nov 4, 2020 at 4:50 PM Rob Herring  wrote:
>
> On Thu, Oct 29, 2020 at 03:59:06PM -0400, Jim Quinlan wrote:
> > Bindings are added.  Only one interrupt is needed because
> > we do not yet employ the SCMI p2a channel.
>
> I still don't understand what this is. To repeat from v1: I thought SCMI
> was a mailbox consumer, not provider?

Hi Rob,

I'm not sure where I am implying that SCMI is a mailbox provider?
Should I not mention "SCMI" in the subject line?

This is just a mailbox driver, "consumed" by SCMI.Our SCMI DT node
looks like this:

brcm_scmi_mailbox: brcm_scmi_mailbox@0 {
#mbox-cells = <1>;
compatible = "brcm,brcmstb-mbox";
};

brcm_scmi@0 {
compatible = "arm,scmi";
mboxes = <_scmi_mailbox 0>;;
    mbox-names = "tx";
shmem = <>;
/* ... */
};

Please advise,
Jim Quinlan
Broadcom STB

>
> >
> > Signed-off-by: Jim Quinlan 
> > ---
> >  .../bindings/mailbox/brcm,brcmstb-mbox.yaml   | 39 +++
> >  1 file changed, 39 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml
> >
> > diff --git 
> > a/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml 
> > b/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml
> > new file mode 100644
> > index ..797c0cc609a3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml
> > @@ -0,0 +1,39 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#;
> > +$id: http://devicetree.org/schemas/mailbox/brcm,brcmstb-mbox.yaml#
> > +
> > +title: Broadcom STB mailbox driver bindings
> > +
> > +maintainers:
> > +  - Jim Quinlan 
> > +
> > +properties:
> > +  compatible:
> > +enum:
> > +  - brcm,brcmstb-mbox
> > +
> > +  interrupts:
> > +items:
> > +  - description: a2p return interrupt, indicates SCMI msg completion.
> > +
> > +  "#mbox-cells":
> > +const: 1
> > +
> > +required:
> > +  - compatible
> > +  - interrupts
> > +  - "#mbox-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +#include 
> > +mailbox {
> > +  compatible = "brcm,brcmstb-mailbox";
> > +  #mbox-cells = <1>;
> > +  interrupts = ;
> > +};
> > +...
> > --
> > 2.17.1
> >
>
>


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v1] PCI: brcmstb: variable is missing proper initialization

[Jim Quinlan]

On Tue, Nov 3, 2020 at 2:38 PM Bjorn Helgaas  wrote:
>
> On Mon, Nov 02, 2020 at 03:57:12PM -0500, Jim Quinlan wrote:
> > The variable 'tmp' is used multiple times in the brcm_pcie_setup()
> > function.  One such usage did not initialize 'tmp' to the current value of
> > the target register.  By luck the mistake does not currently affect
> > behavior;  regardless 'tmp' is now initialized properly.
>
> This is so trivial that there's probably no reason to post this again,
> but if you post a v2 for some reason, please update the subject to
> match the convention ("PCI: brcmstb: Verb ..."), e.g.,
>
>   PCI: brcmstb: Initialize "tmp" before use
>
> The commit log does not say what the patch does, leaving it to the
> reader to infer it.

Got it.
Thanks,
Jim Quinlan
Broadcom STB
>
>
>
>
> Lorenzo will likely fix this up when he applies it.
>
> Incidental curiosity: where should I look to see what
> u32p_replace_bits() does?  "git grep u32p_replace_bits" shows several
> calls, but no definitions.
>
> > Fixes: c0452137034b ("PCI: brcmstb: Add Broadcom STB PCIe host controller 
> > driver")
> > Suggested-by: Rafał Miłecki 
> > Signed-off-by: Jim Quinlan 
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c 
> > b/drivers/pci/controller/pcie-brcmstb.c
> > index bea86899bd5d..9c3d2982248d 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -893,6 +893,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> >   burst = 0x2; /* 512 bytes */
> >
> >   /* Set SCB_MAX_BURST_SIZE, CFG_READ_UR_MODE, SCB_ACCESS_EN */
> > + tmp = readl(base + PCIE_MISC_MISC_CTRL);
> >   u32p_replace_bits(, 1, PCIE_MISC_MISC_CTRL_SCB_ACCESS_EN_MASK);
> >   u32p_replace_bits(, 1, PCIE_MISC_MISC_CTRL_CFG_READ_UR_MODE_MASK);
> >   u32p_replace_bits(, burst, 
> > PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_MASK);
> > --
> > 2.17.1
> >
>
>


smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v1] PCI: brcmstb: variable is missing proper initialization

[Jim Quinlan]

The variable 'tmp' is used multiple times in the brcm_pcie_setup()
function.  One such usage did not initialize 'tmp' to the current value of
the target register.  By luck the mistake does not currently affect
behavior;  regardless 'tmp' is now initialized properly.

Fixes: c0452137034b ("PCI: brcmstb: Add Broadcom STB PCIe host controller 
driver")
Suggested-by: Rafał Miłecki 
Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index bea86899bd5d..9c3d2982248d 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -893,6 +893,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
burst = 0x2; /* 512 bytes */
 
/* Set SCB_MAX_BURST_SIZE, CFG_READ_UR_MODE, SCB_ACCESS_EN */
+   tmp = readl(base + PCIE_MISC_MISC_CTRL);
u32p_replace_bits(, 1, PCIE_MISC_MISC_CTRL_SCB_ACCESS_EN_MASK);
u32p_replace_bits(, 1, PCIE_MISC_MISC_CTRL_CFG_READ_UR_MODE_MASK);
u32p_replace_bits(, burst, PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_MASK);
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v4 2/2] mailbox: Add Broadcom STB mailbox driver

[Jim Quinlan]

This is a simple mailbox driver to be used by the SCMI protocol stack.  It
only implements the agent-to-platform channel; we may implement the
platform-to-agent channel in the future.  An unusual aspect of this driver
is how the completion of an SCMI message is indicated.  An SCMI message is
initiated with an ARM SMC call, but the return of this call does not
indicate the execution or completion of the message.  Rather, the message's
completion is signaled by an interrupt.

Signed-off-by: Jim Quinlan 
Signed-off-by: Florian Fainelli 
---
 drivers/mailbox/Kconfig   |  12 +++
 drivers/mailbox/Makefile  |   2 +
 drivers/mailbox/brcmstb-mailbox.c | 167 ++
 3 files changed, 181 insertions(+)
 create mode 100644 drivers/mailbox/brcmstb-mailbox.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 05b1009e2820..b1e697e8d5f9 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -254,4 +254,16 @@ config QCOM_IPCC
  acts as an interrupt controller for receiving interrupts from clients.
  Say Y here if you want to build this driver.
 
+config BRCMSTB_MBOX
+   tristate "Broadcom STB Mailbox"
+   depends on ARM64 || ARM
+   depends on ARM_SCMI_PROTOCOL && ARCH_BRCMSTB
+   default ARM_SCMI_PROTOCOL && ARCH_BRCMSTB
+   help
+ Mailbox implementation of the Broadcom STB for the sole purposes
+ of SCMI communication.  This is used by the SCMI drivers to
+ communicate with FW that runs in EL3.  This mailbox only implements
+ the agent-to-platform channgel of SCMI but may be augmented in
+ the future to add the platform-to-agent channel.
+
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 2e06e02b2e03..95fbcd121f2e 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -54,3 +54,5 @@ obj-$(CONFIG_SUN6I_MSGBOX)+= sun6i-msgbox.o
 obj-$(CONFIG_SPRD_MBOX)+= sprd-mailbox.o
 
 obj-$(CONFIG_QCOM_IPCC)+= qcom-ipcc.o
+
+obj-$(CONFIG_BRCMSTB_MBOX) += brcmstb-mailbox.o
diff --git a/drivers/mailbox/brcmstb-mailbox.c 
b/drivers/mailbox/brcmstb-mailbox.c
new file mode 100644
index ..d9e953995417
--- /dev/null
+++ b/drivers/mailbox/brcmstb-mailbox.c
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2017-2020, Broadcom */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define BRCM_SCMI_SMC_OEM_FUNC 0x400
+#define BRCM_SCMI_MBOX_NUM 0
+#define BRCM_FID(ch) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+   IS_ENABLED(CONFIG_ARM64), \
+   ARM_SMCCC_OWNER_OEM, \
+   BRCM_SCMI_SMC_OEM_FUNC + (ch))
+enum {
+   A2P_CHAN = 0,
+   NUM_CHAN
+};
+
+struct chan_priv {
+   unsigned int mbox_num;
+   unsigned int ch;
+};
+
+struct brcm_mbox {
+   struct mbox_controller controller;
+   int irqs[NUM_CHAN];
+};
+
+static struct mbox_chan *brcm_mbox_of_xlate(struct mbox_controller *controller,
+   const struct of_phandle_args *sp)
+{
+   unsigned int ch = sp->args[0];
+   struct brcm_mbox *mbox
+   = container_of(controller, struct brcm_mbox, controller);
+
+   if (!mbox || ch >= NUM_CHAN)
+   return ERR_PTR(-ENOENT);
+
+   return >controller.chans[ch];
+}
+
+static int announce_msg(unsigned int mbox_num, unsigned int ch)
+{
+   struct arm_smccc_res res;
+
+   if (ch >= NUM_CHAN)
+   return -EIO;
+   arm_smccc_smc(BRCM_FID(ch), mbox_num, 0, 0, 0, 0, 0, 0, );
+   if (res.a0)
+   return -EIO;
+   return 0;
+}
+
+static int brcm_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+   struct chan_priv *priv = chan->con_priv;
+
+   return announce_msg(priv->mbox_num, priv->ch);
+}
+
+static int brcm_mbox_startup(struct mbox_chan *chan)
+{
+   return 0;
+}
+
+static const struct mbox_chan_ops brcm_mbox_ops = {
+   .send_data = brcm_mbox_send_data,
+   .startup = brcm_mbox_startup,
+};
+
+static irqreturn_t brcm_a2p_isr(int irq, void *data)
+{
+   struct mbox_chan *chan = data;
+
+   mbox_chan_received_data(chan, NULL);
+   return IRQ_HANDLED;
+}
+
+static int brcm_mbox_probe(struct platform_device *pdev)
+{
+   struct brcm_mbox *mbox;
+   struct device *dev = >dev;
+   struct device_node *np = dev->of_node;
+   struct chan_priv *chan_priv;
+   int ret;
+
+   if (!np)
+   return -EINVAL;
+
+   mbox = devm_kzalloc(>dev, sizeof(*mbox), GFP_KERNEL);
+   if (!mbox)
+   return -ENOMEM;
+
+   /* Allocate channels */
+   mbox->controller.chans = devm_kzalloc(
+   >dev, NUM_CHAN * sizeof(struct mbox_chan), GFP_KERNEL);
+

[PATCH v4 0/2] mailbox: Add Broadcom STB mailbox driver for SCMI

[Jim Quinlan]

Patchset Summary:
  Adds a simple mailbox driver for the use of the ARM SCMI drivers.

v4:
  Commit "mailbox: Add Broadcom STB mailbox driver"
  -- Fixed indentation on Kconfig file (again, RandyD).
  -- Removed superfluous #ifdefs for ARM/ARM64 (RandyD).
  -- Fixed Copyright year on source file (RandyD).

v3:
  Commit "mailbox: Add Broadcom STB mailbox driver"
  -- Fixed indentation on Kconfig file (RandyD).

v2:
  Commit "mailbox: Add Broadcom STB mailbox driver"
  -- Remove the Kconfig dependency on SMP (Florian)
  Commit "mailbox: Add Broadcom STB mailbox driver"
  -- Drop label,unit address; changed title,description (RobH)

v1:
  -- Original submission.

Jim Quinlan (2):
  dt-bindings: Add bindings for BrcmSTB SCMI mailbox driver
  mailbox: Add Broadcom STB mailbox driver

 .../bindings/mailbox/brcm,brcmstb-mbox.yaml   |  39 
 drivers/mailbox/Kconfig   |  12 ++
 drivers/mailbox/Makefile  |   2 +
 drivers/mailbox/brcmstb-mailbox.c | 167 ++
 4 files changed, 220 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml
 create mode 100644 drivers/mailbox/brcmstb-mailbox.c

-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v4 1/2] dt-bindings: Add bindings for BrcmSTB SCMI mailbox driver

[Jim Quinlan]

Bindings are added.  Only one interrupt is needed because
we do not yet employ the SCMI p2a channel.

Signed-off-by: Jim Quinlan 
---
 .../bindings/mailbox/brcm,brcmstb-mbox.yaml   | 39 +++
 1 file changed, 39 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml

diff --git a/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml 
b/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml
new file mode 100644
index ..797c0cc609a3
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$schema: "http://devicetree.org/meta-schemas/core.yaml#;
+$id: http://devicetree.org/schemas/mailbox/brcm,brcmstb-mbox.yaml#
+
+title: Broadcom STB mailbox driver bindings
+
+maintainers:
+  - Jim Quinlan 
+
+properties:
+  compatible:
+enum:
+  - brcm,brcmstb-mbox
+
+  interrupts:
+items:
+  - description: a2p return interrupt, indicates SCMI msg completion.
+
+  "#mbox-cells":
+const: 1
+
+required:
+  - compatible
+  - interrupts
+  - "#mbox-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+#include 
+mailbox {
+  compatible = "brcm,brcmstb-mailbox";
+  #mbox-cells = <1>;
+  interrupts = ;
+};
+...
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v3 2/2] mailbox: Add Broadcom STB mailbox driver

[Jim Quinlan]

On Fri, Oct 9, 2020 at 12:38 PM Sudeep Holla  wrote:
>
> On Sat, Sep 19, 2020 at 03:22:30PM -0400, Jim Quinlan wrote:
> > only implements the agent-to-platform channel;
>
> In that case any reason why you can't reuse the existing smc transport
> for SCMI. It was added recently in case you haven't checked the latest
> kernel version(v5.8 or above). Check out for drivers/firmware/arm_scmi/smc.c
> IIUC rather vaguely Florian was cc-ed on those patches.

Hi Sudeep,

Sorry for the delay.  As Florian mentioned, we tried to use what
you've submitted but could not because in our system a return does not
indicate the completion of the SCMI operation.  It is indicated by an
interrupt.  There are a number of reasons for this and some are out of
our control.
>
>
> > we may implement the platform-to-agent channel in the future.
>
> This is not yet support with that transport, it is hard to generalise
> as different vendors have their own solutions there.
>
> > An unusual aspect of this driver is how the completion of an SCMI message
> > is indicated.  An SCMI message is initiated with an ARM SMC call, but the
> > return of this call does not indicate the execution or completion of the
> > message.  Rather, the message's completion is signaled by an interrupt.
> >
>
> So are these not fast SMC/HVC calls then ? If so we may need some changes
> to that driver. I just rejected multiple message support as we had assumed
> fast smc/hvc.

Yes, we are using fast SMC calls.  We don't have multiple message
support either.  The disconnect we have with the smc/hvc transport
commit si this:

smc_send_message(...)
{
/* ... */
+ arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, );
+ scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem));

In our code the second line is not here but in the interrupt handler.
I don't see any way you can easily change/augment the smc/hvc
transport to accommodate us.

Regards,
Jim





>
>
> --
> Regards,
> Sudeep


smime.p7s
Description: S/MIME Cryptographic Signature

Re: linux-next: manual merge of the usb tree with the pci tree

[Jim Quinlan]

On Thu, Oct 15, 2020 at 5:48 PM Stephen Rothwell  wrote:
>
> Hi all,
>
> On Mon, 21 Sep 2020 15:18:07 +1000 Stephen Rothwell  
> wrote:
> >
> > Today's linux-next merge of the usb tree got a conflict in:
> >
> >   drivers/pci/controller/pcie-brcmstb.c
> >
> > between commit:
> >
> >   1cf1b0a6dd95 ("PCI: brcmstb: Add bcm7278 register info")
> >
> > from the pci tree and commit:
> >
> >   f48cc509c935 ("Revert "PCI: brcmstb: Wait for Raspberry Pi's firmware 
> > when present"")
> >
> > from the usb tree.
> >
> > I fixed it up (see below) and can carry the fix as necessary. This
> > is now fixed as far as linux-next is concerned, but any non trivial
> > conflicts should be mentioned to your upstream maintainer when your tree
> > is submitted for merging.  You may also want to consider cooperating
> > with the maintainer of the conflicting tree to minimise any particularly
> > complex conflicts.
> >
> > diff --cc drivers/pci/controller/pcie-brcmstb.c
> > index 6e7aa82a54a3,bac63d04297f..
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@@ -1213,8 -929,6 +1211,7 @@@ static int brcm_pcie_probe(struct platf
> >   {
> >   struct device_node *np = pdev->dev.of_node, *msi_np;
> >   struct pci_host_bridge *bridge;
> > - struct device_node *fw_np;
> >  +const struct pcie_cfg_data *data;
> >   struct brcm_pcie *pcie;
> >   int ret;
> >
>
> This is now a conflict between the pci tree and Linus' tree.
Hello,
Sorry, I did not foresee a conflict.  Please keep both lines below.

 struct device_node *fw_np;
 const struct pcie_cfg_data *data;

Thank you,
Jim Quinlan
Broadcom STB


>
> --
> Cheers,
> Stephen Rothwell


smime.p7s
Description: S/MIME Cryptographic Signature

Re: arm-smmu 5000000.iommu: Cannot accommodate DMA offset for IOMMU page tables

[Jim Quinlan]

On Sat, Oct 10, 2020 at 2:53 PM Stephen Rothwell  wrote:
>
> Hi Naresh,
>
> Just adding Christoph and Jim to cc]
>
> On Fri, 9 Oct 2020 19:26:24 +0530 Naresh Kamboju  
> wrote:
> >
> > On Fri, 9 Oct 2020 at 19:24, Naresh Kamboju  
> > wrote:
> > >
> > >
> > >
> > > On Thu, 24 Sep 2020 at 15:26, Joerg Roedel  wrote:
> > > >
> > > > On Thu, Sep 24, 2020 at 10:36:47AM +0100, Robin Murphy wrote:
> > > > > Yes, the issue was introduced by one of the changes in "dma-mapping:
> > > > > introduce DMA range map, supplanting dma_pfn_offset", so it only 
> > > > > existed in
> > > > > the dma-mapping/for-next branch anyway.
> > >
> >
> > FYI,
> > The reported problem still exists on 5.9.0-rc8-next-20201009.
> >
> > [1.843814] Driver must set ecc.strength when using hardware ECC
> > [1.849847] WARNING: CPU: 4 PID: 1 at
> > drivers/mtd/nand/raw/nand_base.c:5687 nand_scan_with_ids+0x1450/0x1470
> > [1.859676] Modules linked in:
> > [1.862730] CPU: 4 PID: 1 Comm: swapper/0 Not tainted
> > 5.9.0-rc8-next-20201009 #1
> > [1.870125] Hardware name: Freescale Layerscape 2088A RDB Board (DT)
> > [1.876478] pstate: 4005 (nZcv daif -PAN -UAO -TCO BTYPE=--)
> > [1.882483] pc : nand_scan_with_ids+0x1450/0x1470
> > [1.887183] lr : nand_scan_with_ids+0x1450/0x1470

Hi,

I'm having a hard time coming up with a theory regarding  how a commit
concerning DMA offsets can affect the operation of a NAND driver that
appears not to use DMA or the dma-ranges property.  Does anyone else
have some ideas, or is there perhaps someone familiar with this test
configuration that I can correspond with to get to the bottom of the
warning?

Thanks,
Jim Quinlan
Broadcom STB

> >
> > full test log,
> > https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20201009/testrun/3284876/suite/linux-log-parser/test/check-kernel-warning-92014/log
> >
> > > >
> > > > Okay, alright then.
> > > >
> >
> > - Naresh
>
> --
> Cheers,
> Stephen Rothwell


smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v1] PCI: brcmstb: fix error return paths in probe()

[Jim Quinlan]

Fixes two cases where we were returning without calling
clk_disable_unprepare().  Although there is a common place to go on probe()
errors (the 'fail' label), one can only jump there after executing
brcm_pcie_setup(), so we have to add clk_disable_unprepare() calls to the
two error paths.

Fixes: b98f52bc6495 ("PCI: brcmstb: Add control of rescal reset")
Reported-by: kernel test robot 
Reported-by: Dan Carpenter 
Signed-off-by: Jim Quinlan 
---
 drivers/pci/controller/pcie-brcmstb.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c 
b/drivers/pci/controller/pcie-brcmstb.c
index 6e7aa82a54a3..da2fefe80d47 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1269,8 +1269,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
return ret;
}
pcie->rescal = devm_reset_control_get_optional_shared(>dev, 
"rescal");
-   if (IS_ERR(pcie->rescal))
+   if (IS_ERR(pcie->rescal)) {
+   clk_disable_unprepare(pcie->clk);
return PTR_ERR(pcie->rescal);
+   }
 
ret = reset_control_deassert(pcie->rescal);
if (ret)
@@ -1279,6 +1281,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
ret = brcm_phy_start(pcie);
if (ret) {
reset_control_assert(pcie->rescal);
+   clk_disable_unprepare(pcie->clk);
return ret;
}
 
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v2 RESEND 1/1] PCI: pcie_bus_config can be set at build time

[Jim Quinlan]

On Tue, Sep 29, 2020 at 5:31 PM Bjorn Helgaas  wrote:
>
> On Mon, Sep 28, 2020 at 03:46:51PM -0400, Jim Quinlan wrote:
> > The Kconfig is modified so that the pcie_bus_config setting can be done at
> > build time in the same manner as the CONFIG_PCIEASPM_ choice.  The
> > pci_bus_config setting may still be overridden by the bootline param.
> >
> > Signed-off-by: Jim Quinlan 
>
> Applied to pci/enumeration for v5.10, thanks!
>
> I tweaked the help texts and made the Kconfig stuff depend on
> CONFIG_EXPERT.  Will that still be enough for what you need?  I'm
> worried that users will get themselves in trouble if they fiddle with
> things without really understanding what's going on.

Hi Bjorn,

I didn't even know about CONFIG_EXPERT -- makes sense and looks good to me!

Thanks much,
Jim Quinlan
Broadcom STB

>
> Here's what I applied:
>
>
> commit 2a87f534d198 ("PCI: Add Kconfig options for pcie_bus_config")
> Author: Jim Quinlan 
> Date:   Mon Sep 28 15:46:51 2020 -0400
>
> PCI: Add Kconfig options for pcie_bus_config
>
> Add Kconfig options for changing the default pcie_bus_config in the same
> manner as the CONFIG_PCIEASPM_ choice.  The pci_bus_config setting may
> still be overridden by kernel command-line parameters, e.g.,
> "pci=pcie_bus_tune_off".
>
> [bhelgaas: depend on EXPERT, tweak help texts]
> Link: 
> https://lore.kernel.org/r/20200928194651.5393-2-james.quin...@broadcom.com
> Signed-off-by: Jim Quinlan 
> Signed-off-by: Bjorn Helgaas 
>
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 4bef5c2bae9f..d323b25ae27e 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -187,6 +187,68 @@ config PCI_HYPERV
>   The PCI device frontend driver allows the kernel to import arbitrary
>   PCI devices from a PCI backend to support PCI driver domains.
>
> +choice
> +   prompt "PCI Express hierarchy optimization setting"
> +   default PCIE_BUS_DEFAULT
> +   depends on PCI && EXPERT
> +   help
> + MPS (Max Payload Size) and MRRS (Max Read Request Size) are PCIe
> + device parameters that affect performance and the ability to
> + support hotplug and peer-to-peer DMA.
> +
> + The following choices set the MPS and MRRS optimization strategy
> + at compile-time.  The choices are the same as those offered for
> + the kernel command-line parameter 'pci', i.e.,
> + 'pci=pcie_bus_tune_off', 'pci=pcie_bus_safe',
> + 'pci=pcie_bus_perf', and 'pci=pcie_bus_peer2peer'.
> +
> + This is a compile-time setting and can be overridden by the above
> + command-line parameters.  If unsure, choose PCIE_BUS_DEFAULT.
> +
> +config PCIE_BUS_TUNE_OFF
> +   bool "Tune Off"
> +   depends on PCI
> +   help
> + Use the BIOS defaults; don't touch MPS at all.  This is the same
> + as booting with 'pci=pcie_bus_tune_off'.
> +
> +config PCIE_BUS_DEFAULT
> +   bool "Default"
> +   depends on PCI
> +   help
> + Default choice; ensure that the MPS matches upstream bridge.
> +
> +config PCIE_BUS_SAFE
> +   bool "Safe"
> +   depends on PCI
> +   help
> + Use largest MPS that boot-time devices support.  If you have a
> + closed system with no possibility of adding new devices, this
> + will use the largest MPS that's supported by all devices.  This
> + is the same as booting with 'pci=pcie_bus_safe'.
> +
> +config PCIE_BUS_PERFORMANCE
> +   bool "Performance"
> +   depends on PCI
> +   help
> + Use MPS and MRRS for best performance.  Ensure that a given
> + device's MPS is no larger than its parent MPS, which allows us to
> + keep all switches/bridges to the max MPS supported by their
> + parent.  This is the same as booting with 'pci=pcie_bus_perf'.
> +
> +config PCIE_BUS_PEER2PEER
> +   bool "Peer2peer"
> +   depends on PCI
> +   help
> + Set MPS = 128 for all devices.  MPS configuration effected by the
> + other options could cause the MPS on one root port to be
> + different than that of the MPS on another, which may cause
> + hot-added devices or peer-to-peer DMA to fail.  Set MPS to the
> + smallest possible value (128B) system-wide to avoid these issues.
> + This is the same as booting with 'pci=pcie_bus_peer2peer'.
> +
> +endchoice
> +
>  source "drivers/pci/hotplug/Kconfig"
>  source "drivers/pci/controller/Kconfig"
>  source 

[PATCH v2 RESEND 0/1] PCI: pcie_bus_config can be set at build time

[Jim Quinlan]

v2: Add more description text in the new Kconfig settings (Bjorn).

v1: Original

Jim Quinlan (1):
  PCI: pcie_bus_config can be set at build time

 drivers/pci/Kconfig | 56 +
 drivers/pci/pci.c   | 12 ++
 2 files changed, 68 insertions(+)

-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v2 RESEND 1/1] PCI: pcie_bus_config can be set at build time

[Jim Quinlan]

The Kconfig is modified so that the pcie_bus_config setting can be done at
build time in the same manner as the CONFIG_PCIEASPM_ choice.  The
pci_bus_config setting may still be overridden by the bootline param.

Signed-off-by: Jim Quinlan 
---
 drivers/pci/Kconfig | 56 +
 drivers/pci/pci.c   | 12 ++
 2 files changed, 68 insertions(+)

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 4bef5c2bae9f..15ce948858fb 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -187,6 +187,62 @@ config PCI_HYPERV
  The PCI device frontend driver allows the kernel to import arbitrary
  PCI devices from a PCI backend to support PCI driver domains.
 
+choice
+   prompt "PCIE default bus config setting"
+   default PCIE_BUS_DEFAULT
+   depends on PCI
+   help
+ One of the following choices will set the pci_bus_config at
+ compile time.  The choices offered are the same as those offered
+ for the bootline parameter 'pci'; i.e. 'pci=pcie_bus_tune_off',
+ 'pci=pcie_bus_safe', 'pci=pcie_bus_perf', and 
'pci=pcie_bus_peer2peer'.
+ This is a compile-time setting and is still be overridden by the
+ above bootline parameters, if present.  If unsure, chose 
PCIE_BUS_DEFAULT.
+
+config PCIE_BUS_TUNE_OFF
+   bool "Tune Off"
+   depends on PCI
+   help
+ Use the BIOS defaults; doesn't touch MPS at all.  This is the same
+ as booting with 'pci=pcie_bus_tune_off'.
+
+config PCIE_BUS_DEFAULT
+   bool "Default"
+   depends on PCI
+   help
+ Default choice; ensures that the MPS matches upstream bridge.
+
+config PCIE_BUS_SAFE
+   bool "Safe"
+   depends on PCI
+   help
+ Use largest MPS that boot-time devices support.  If you have a
+ closed system with no possibility of adding new devices,
+ this will use the largest MPS that's supported by all devices.
+ This is the same as booting with 'pci=pcie_bus_safe'.
+
+config PCIE_BUS_PERFORMANCE
+   bool "Performance"
+   depends on PCI
+   help
+ Use MPS and MRRS for best performance.  This setting ensures
+ that a given device's MPS is no larger than its parent MPS,
+ which allows us to keep all switches/bridges to the max MPS supported
+ by their parent and eventually the PHB.  This is the same as
+ booting with 'pci=pcie_bus_perf'.
+
+config PCIE_BUS_PEER2PEER
+   bool "Peer2peer"
+   depends on PCI
+   help
+ Set MPS = 128 for all devices.  MPS configuration effected by
+ the other options could cause the MPS on one root port to be
+ different than that of the MPS on another.  Simply making the system
+ wide MPS be set to the smallest possible value (128B) solves
+ this issue.  This is the same as booting with 
'pci=pcie_bus_peer2peer'.
+
+endchoice
+
 source "drivers/pci/hotplug/Kconfig"
 source "drivers/pci/controller/Kconfig"
 source "drivers/pci/endpoint/Kconfig"
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e39c5499770f..dfb52ed4a931 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -101,7 +101,19 @@ unsigned long pci_hotplug_mmio_pref_size = 
DEFAULT_HOTPLUG_MMIO_PREF_SIZE;
 #define DEFAULT_HOTPLUG_BUS_SIZE   1
 unsigned long pci_hotplug_bus_size = DEFAULT_HOTPLUG_BUS_SIZE;
 
+
+/* PCIE bus config, can be overridden by bootline param */
+#ifdef CONFIG_PCIE_BUS_TUNE_OFF
+enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_TUNE_OFF;
+#elif defined CONFIG_PCIE_BUS_SAFE
+enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_SAFE;
+#elif defined CONFIG_PCIE_BUS_PERFORMANCE
+enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_PERFORMANCE;
+#elif defined CONFIG_PCIE_BUS_PEER2PEER
+enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_PEER2PEER;
+#else
 enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_DEFAULT;
+#endif
 
 /*
  * The default CLS is used if arch didn't set CLS explicitly and not
-- 
2.17.1



smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v2] PCI: brcmstb: fix a missing if statement on a return error check

[Jim Quinlan]

On Tue, Sep 22, 2020 at 7:49 AM Markus Elfring  wrote:
>
> > The error return ret is not being check with an if statement and
>
> Wording alternative:
> The return value from a call of the function “brcm_phy_start” was not checked 
> and
>
>
> > V2: disable clock as noted by Florian Fainelli and suggested by
> > Jim Quinlan.
>
> Alex Dewar contributed another update suggestion.
>
> [PATCH v2] PCI: brcmstb: Add missing if statement and error path
> https://lore.kernel.org/linux-arm-kernel/20200921211623.33908-1-alex.dewa...@gmail.com/
> https://lore.kernel.org/patchwork/patch/1309860/
>
> The exception handling needs further development considerations
> for this function implementation.
Hello,

I agree with Alex's patch.  I should have suggested this at the
beginning but as our upstream STB suspend/resume is not yet functional
and the one-line change would have worked until we fixed
suspend/resume..  But this is the proper modification.

Thanks,
Jim
> Regards,
> Markus


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH] PCI: brcmstb: Add missing if statement

[Jim Quinlan]

On Mon, Sep 21, 2020 at 4:45 PM Alex Dewar  wrote:
>
> brcm_pcie_resume() contains a return statement that was presumably
> intended to have an "if (ret)" in front of it, otherwise the function
> returns prematurely. Fix this.
>
> I don't know if this code was tested or not, but I assume that this bug
> means that this driver will not resume properly.
>
> Fixes: ad3d29c77e1e ("PCI: brcmstb: Add control of rescal reset")
> Addresses-Coverity: CID 1497099: Control flow issues (UNREACHABLE)
> Signed-off-by: Alex Dewar 
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c 
> b/drivers/pci/controller/pcie-brcmstb.c
> index 7a3ff4632e7c..cb0c11b7308e 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1154,6 +1154,7 @@ static int brcm_pcie_resume(struct device *dev)
> clk_prepare_enable(pcie->clk);
>
> ret = brcm_phy_start(pcie);
> +   if (ret)

Hello,
Florian suggested adding braces to the if clause and inserting a
"clk_disable_unprepare(-cie->clk);" before the return.  I am fine with
either what you have or implementing Florian's additional suggestion.

Thank you,
Jim Quinlan
Broadcom  STB
> return ret;
>
> /* Take bridge out of reset so we can access the SERDES reg */
> --
> 2.28.0
>


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH][next] PCI: brcmstb: fix a missing if statement on a return error check

[Jim Quinlan]

Hello,
I am fine with Colin's suggested change or Florians as well:

 ret = brcm_phy_start(pcie);
+if (ret) {
+clk_disable_unprepare(pcie->clk);
 return ret;
+}

Currently, our STB upstream suspend/resume is not functional yet and
this is how this omission slipped by testing.

Thanks,
Jim Quinlan
Broadcom STB

On Mon, Sep 21, 2020 at 3:43 PM Florian Fainelli  wrote:
>
> On 9/21/20 7:40 AM, Colin King wrote:
> > From: Colin Ian King 
> >
> > The error return ret is not being check with an if statement and
> > currently the code always returns leaving the following code as
> > dead code. Fix this by adding in the missing if statement.
> >
> > Addresses-Coverity: ("Structurally dead code")
> > Fixes: ad3d29c77e1e ("PCI: brcmstb: Add control of rescal reset")
> > Signed-off-by: Colin Ian King 
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c 
> > b/drivers/pci/controller/pcie-brcmstb.c
> > index 7a3ff4632e7c..cb0c11b7308e 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -1154,6 +1154,7 @@ static int brcm_pcie_resume(struct device *dev)
> >   clk_prepare_enable(pcie->clk);
> >
> >   ret = brcm_phy_start(pcie);
> > + if (ret)
> >   return ret;
>
> Maybe this should also disable the clock if we failed to start the PHY
> somehow.

Hi Florian,

I'm fine with Colin's change as


>
> --
> Florian


smime.p7s
Description: S/MIME Cryptographic Signature