We've been assuming that if an SOC has a "rescal" reset controller that we
should automatically invoke brcm_phy_cntl(...). This will not be true in
future SOCs, so we create a bool "has_phy" and adjust the cfg_data
appropriately (we need to give 7216 its own cfg_data structure instead of
sharing one).
Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
drivers/pci/controller/pcie-brcmstb.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 4e0848e1311f..e740e2966a5c 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -227,6 +227,7 @@ enum pcie_type {
struct pcie_cfg_data {
const int *offsets;
const enum pcie_type type;
+ const bool has_phy;
void (*perst_set)(struct brcm_pcie *pcie, u32 val);
void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
};
@@ -277,6 +278,7 @@ struct brcm_pcie {
void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
struct subdev_regulators *sr;
bool ep_wakeup_capable;
+ bool has_phy;
};
static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -1316,12 +1318,12 @@ static int brcm_phy_cntl(struct brcm_pcie *pcie, const int start)
static inline int brcm_phy_start(struct brcm_pcie *pcie)
{
- return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0;
+ return pcie->has_phy ? brcm_phy_cntl(pcie, 1) : 0;
}
static inline int brcm_phy_stop(struct brcm_pcie *pcie)
{
- return pcie->rescal ? brcm_phy_cntl(pcie, 0) : 0;
+ return pcie->has_phy ? brcm_phy_cntl(pcie, 0) : 0;
}
static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
@@ -1564,12 +1566,20 @@ 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,
+ .has_phy = true,
+};
+
static const struct of_device_id brcm_pcie_match[] = {
{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
{ .compatible = "brcm,bcm7211-pcie", .data = &generic_cfg },
{ .compatible = "brcm,bcm7278-pcie", .data = &bcm7278_cfg },
- { .compatible = "brcm,bcm7216-pcie", .data = &bcm7278_cfg },
+ { .compatible = "brcm,bcm7216-pcie", .data = &bcm7216_cfg },
{ .compatible = "brcm,bcm7445-pcie", .data = &generic_cfg },
{ .compatible = "brcm,bcm7435-pcie", .data = &bcm7435_cfg },
{ .compatible = "brcm,bcm7425-pcie", .data = &bcm7425_cfg },
@@ -1617,6 +1627,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
pcie->type = data->type;
pcie->perst_set = data->perst_set;
pcie->bridge_sw_init_set = data->bridge_sw_init_set;
+ pcie->has_phy = data->has_phy;
pcie->base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(pcie->base))
--
2.17.1
On 6/28/24 23:54, Jim Quinlan wrote:
> We've been assuming that if an SOC has a "rescal" reset controller that we
> should automatically invoke brcm_phy_cntl(...). This will not be true in
> future SOCs, so we create a bool "has_phy" and adjust the cfg_data
> appropriately (we need to give 7216 its own cfg_data structure instead of
> sharing one).
>
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
> drivers/pci/controller/pcie-brcmstb.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 4e0848e1311f..e740e2966a5c 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -227,6 +227,7 @@ enum pcie_type {
> struct pcie_cfg_data {
> const int *offsets;
> const enum pcie_type type;
> + const bool has_phy;
> void (*perst_set)(struct brcm_pcie *pcie, u32 val);
> void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> };
> @@ -277,6 +278,7 @@ struct brcm_pcie {
> void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> struct subdev_regulators *sr;
> bool ep_wakeup_capable;
> + bool has_phy;
> };
>
> static inline bool is_bmips(const struct brcm_pcie *pcie)
> @@ -1316,12 +1318,12 @@ static int brcm_phy_cntl(struct brcm_pcie *pcie, const int start)
>
> static inline int brcm_phy_start(struct brcm_pcie *pcie)
> {
> - return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0;
> + return pcie->has_phy ? brcm_phy_cntl(pcie, 1) : 0;
> }
>
> static inline int brcm_phy_stop(struct brcm_pcie *pcie)
> {
> - return pcie->rescal ? brcm_phy_cntl(pcie, 0) : 0;
> + return pcie->has_phy ? brcm_phy_cntl(pcie, 0) : 0;
> }
>
> static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
> @@ -1564,12 +1566,20 @@ 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,
This "type" field is confusing, maybe it would be good to rename it to
"family"? For example BCM72XX family.
> + .perst_set = brcm_pcie_perst_set_7278,
> + .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
> + .has_phy = true,
> +};
> +
> static const struct of_device_id brcm_pcie_match[] = {
> { .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
> { .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
> { .compatible = "brcm,bcm7211-pcie", .data = &generic_cfg },
> { .compatible = "brcm,bcm7278-pcie", .data = &bcm7278_cfg },
> - { .compatible = "brcm,bcm7216-pcie", .data = &bcm7278_cfg },
> + { .compatible = "brcm,bcm7216-pcie", .data = &bcm7216_cfg },
> { .compatible = "brcm,bcm7445-pcie", .data = &generic_cfg },
> { .compatible = "brcm,bcm7435-pcie", .data = &bcm7435_cfg },
> { .compatible = "brcm,bcm7425-pcie", .data = &bcm7425_cfg },
> @@ -1617,6 +1627,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> pcie->type = data->type;
> pcie->perst_set = data->perst_set;
> pcie->bridge_sw_init_set = data->bridge_sw_init_set;
> + pcie->has_phy = data->has_phy;
>
> pcie->base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(pcie->base))
~Stan
On Tue, Jul 2, 2024 at 9:10 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
>
>
>
> On 6/28/24 23:54, Jim Quinlan wrote:
> > We've been assuming that if an SOC has a "rescal" reset controller that we
> > should automatically invoke brcm_phy_cntl(...). This will not be true in
> > future SOCs, so we create a bool "has_phy" and adjust the cfg_data
> > appropriately (we need to give 7216 its own cfg_data structure instead of
> > sharing one).
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> > drivers/pci/controller/pcie-brcmstb.c | 17 ++++++++++++++---
> > 1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 4e0848e1311f..e740e2966a5c 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -227,6 +227,7 @@ enum pcie_type {
> > struct pcie_cfg_data {
> > const int *offsets;
> > const enum pcie_type type;
> > + const bool has_phy;
> > void (*perst_set)(struct brcm_pcie *pcie, u32 val);
> > void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> > };
> > @@ -277,6 +278,7 @@ struct brcm_pcie {
> > void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> > struct subdev_regulators *sr;
> > bool ep_wakeup_capable;
> > + bool has_phy;
> > };
> >
> > static inline bool is_bmips(const struct brcm_pcie *pcie)
> > @@ -1316,12 +1318,12 @@ static int brcm_phy_cntl(struct brcm_pcie *pcie, const int start)
> >
> > static inline int brcm_phy_start(struct brcm_pcie *pcie)
> > {
> > - return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0;
> > + return pcie->has_phy ? brcm_phy_cntl(pcie, 1) : 0;
> > }
> >
> > static inline int brcm_phy_stop(struct brcm_pcie *pcie)
> > {
> > - return pcie->rescal ? brcm_phy_cntl(pcie, 0) : 0;
> > + return pcie->has_phy ? brcm_phy_cntl(pcie, 0) : 0;
> > }
> >
> > static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
> > @@ -1564,12 +1566,20 @@ 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,
>
> This "type" field is confusing, maybe it would be good to rename it to
> "family"? For example BCM72XX family.
Hi Stanimir,
I'm open for another name but "family" would present problems with Broadcom STB.
For example, we call 7216b0 a "family" as there are a number of
derivative products based off
of this general design. Second, having something like "BCM72XX" won't work;
we have 7211 which is something altogether different from the 7216.
Note that we only
introduce a new "type" when we need to; if the behavior is the same as
a previously declared
"type" we do not introduce new ones.
But if you wanted to change "type" to "model" then I have no problem with that.
Regards,
Jim Quinlan
Broadcom STB/CM
>
> > + .perst_set = brcm_pcie_perst_set_7278,
> > + .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
> > + .has_phy = true,
> > +};
> > +
> > static const struct of_device_id brcm_pcie_match[] = {
> > { .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
> > { .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
> > { .compatible = "brcm,bcm7211-pcie", .data = &generic_cfg },
> > { .compatible = "brcm,bcm7278-pcie", .data = &bcm7278_cfg },
> > - { .compatible = "brcm,bcm7216-pcie", .data = &bcm7278_cfg },
> > + { .compatible = "brcm,bcm7216-pcie", .data = &bcm7216_cfg },
> > { .compatible = "brcm,bcm7445-pcie", .data = &generic_cfg },
> > { .compatible = "brcm,bcm7435-pcie", .data = &bcm7435_cfg },
> > { .compatible = "brcm,bcm7425-pcie", .data = &bcm7425_cfg },
> > @@ -1617,6 +1627,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > pcie->type = data->type;
> > pcie->perst_set = data->perst_set;
> > pcie->bridge_sw_init_set = data->bridge_sw_init_set;
> > + pcie->has_phy = data->has_phy;
> >
> > pcie->base = devm_platform_ioremap_resource(pdev, 0);
> > if (IS_ERR(pcie->base))
>
> ~Stan
On 7/2/24 20:59, Jim Quinlan wrote:
> On Tue, Jul 2, 2024 at 9:10 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
>>
>>
>>
>> On 6/28/24 23:54, Jim Quinlan wrote:
>>> We've been assuming that if an SOC has a "rescal" reset controller that we
>>> should automatically invoke brcm_phy_cntl(...). This will not be true in
>>> future SOCs, so we create a bool "has_phy" and adjust the cfg_data
>>> appropriately (we need to give 7216 its own cfg_data structure instead of
>>> sharing one).
>>>
>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>> ---
>>> drivers/pci/controller/pcie-brcmstb.c | 17 ++++++++++++++---
>>> 1 file changed, 14 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
<cut>
>>>
>>> +static const struct pcie_cfg_data bcm7216_cfg = {
>>> + .offsets = pcie_offset_bcm7278,
>>> + .type = BCM7278,
>>
>> This "type" field is confusing, maybe it would be good to rename it to
>> "family"? For example BCM72XX family.
>
> Hi Stanimir,
>
> I'm open for another name but "family" would present problems with Broadcom STB.
> For example, we call 7216b0 a "family" as there are a number of
> derivative products based off
OK, sorry I'm not familiar with STB families. Then, it makes sense.
> of this general design. Second, having something like "BCM72XX" won't work;
> we have 7211 which is something altogether different from the 7216.
> Note that we only
> introduce a new "type" when we need to; if the behavior is the same as
> a previously declared
> "type" we do not introduce new ones.
>
> But if you wanted to change "type" to "model" then I have no problem with that.
>
"model" sounds good to me. We might need to document this in kernel doc
style comment in struct pcie_cfg_data as a separate patch.
> Regards,
> Jim Quinlan
thanks
~Stan
© 2016 - 2025 Red Hat, Inc.