| Message ID | 20240628205430.24775-7-james.quinlan@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: brcnstb: Enable STB 7712 SOC | expand |
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
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))
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(-)