by Nicolas Saenz Julienne

[permalink] [raw]

Subject: Re: [PATCH 09/15] device core: Add ability to handle multiple dma offsets

Hi Jim,
thanks for having a go at this! My two cents.

On Tue, 2020-05-19 at 16:34 -0400, Jim Quinlan wrote:
> The device variable 'dma_pfn_offset' is used to do a single
> linear map between cpu addrs and dma addrs. The variable
> 'dma_map' is added to struct device to point to an array
> of multiple offsets which is required for some devices.
>
> Signed-off-by: Jim Quinlan <[email protected]>
> ---

[...]

> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -493,6 +493,8 @@ struct dev_links_info {
> * @bus_dma_limit: Limit of an upstream bridge or bus which imposes a smaller
> * DMA limit than the device itself supports.
> * @dma_pfn_offset: offset of DMA memory range relatively of RAM
> + * @dma_map: Like dma_pfn_offset but used when there are multiple
> + * pfn offsets for multiple dma-ranges.
> * @dma_parms: A low level driver may set these to teach IOMMU code
> about
> * segment limitations.
> * @dma_pools: Dma pools (if dma'ble device).
> @@ -578,7 +580,12 @@ struct device {
> allocations such descriptors. */
> u64 bus_dma_limit; /* upstream dma constraint */
> unsigned long dma_pfn_offset;
> -
> +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> + const void *dma_offset_map; /* Like dma_pfn_offset, but for
> + * the unlikely case of multiple
> + * offsets. If non-null, dma_pfn_offset
> + * will be 0. */

I get a bad feeling about separating the DMA offset handling into two distinct
variables. Albeit generally frowned upon, there is a fair amount of trickery
around dev->dma_pfn_offset all over the kernel. usb_alloc_dev() comes to mind
for example. And this obviously doesn't play well with it. I feel a potential
solution to multiple DMA ranges should completely integrate with the current
device DMA handling code, without special cases, on top of that, be transparent
to the user.

In more concrete terms, I'd repackage dev->bus_dma_limit and
dev->dma_pfn_offset into a list/array of DMA range structures and adapt/create
the relevant getter/setter functions so as for DMA users not to have to worry
about the specifics of a device's DMA constraints. For example, instead of
editing dev->dma_pfn_offset, you'd be passing a DMA range structure to the
device core, and let it take the relevant decisions on how to handle it
internally (overwrite, add a new entry, merge them, etc...).

Easier said than done. :)

Regards,
Nicolas

Attachments:

signature.asc (499.00 B)
This is a digitally signed message part

2020-05-20 11:55:15

by Nicolas Saenz Julienne

[permalink] [raw]

Subject: Re: [PATCH 04/15] PCI: brcmstb: Add compatibily of other chips

Hi Jim,

On Tue, 2020-05-19 at 16:34 -0400, Jim Quinlan wrote:
> From: Jim Quinlan <[email protected]>
>
> Add in compatibility strings and code for three Broadcom STB chips.
> Some of the register locations, shifts, and masks are different
> for certain chips, requiring the use of different constants based
> on of_id.
>
> We would like to add the following at this time to the match list
> but we need to wait until the end of this patchset so that
> everything works.
>
> { .compatible = "brcm,bcm7211-pcie", .data = &generic_cfg },
> { .compatible = "brcm,bcm7278-pcie", .data = &bcm7278_cfg },
> { .compatible = "brcm,bcm7216-pcie", .data = &bcm7278_cfg },
> { .compatible = "brcm,bcm7445-pcie", .data = &generic_cfg },
>
> Signed-off-by: Jim Quinlan <[email protected]>
> ---
> drivers/pci/controller/pcie-brcmstb.c | 103 +++++++++++++++++++++++---
> 1 file changed, 91 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c
> b/drivers/pci/controller/pcie-brcmstb.c
> index 73020b4ff090..c1cf4ea7d3d9 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -120,9 +120,8 @@
> #define PCIE_EXT_SLOT_SHIFT 15
> #define PCIE_EXT_FUNC_SHIFT 12
>
> -#define PCIE_RGR1_SW_INIT_1 0x9210
> #define PCIE_RGR1_SW_INIT_1_PERST_MASK 0x1
> -#define PCIE_RGR1_SW_INIT_1_INIT_MASK 0x2
> +#define PCIE_RGR1_SW_INIT_1_PERST_SHIFT 0x0
>
> /* PCIe parameters */
> #define BRCM_NUM_PCIE_OUT_WINS 0x4
> @@ -152,6 +151,69 @@
> #define SSC_STATUS_SSC_MASK 0x400
> #define SSC_STATUS_PLL_LOCK_MASK 0x800
>
> +#define IDX_ADDR(pcie) \
> + (pcie->reg_offsets[EXT_CFG_INDEX])
> +#define DATA_ADDR(pcie) \
> + (pcie->reg_offsets[EXT_CFG_DATA])
> +#define PCIE_RGR1_SW_INIT_1(pcie) \
> + (pcie->reg_offsets[RGR1_SW_INIT_1])
> +
> +enum {
> + RGR1_SW_INIT_1,
> + EXT_CFG_INDEX,
> + EXT_CFG_DATA,
> +};
> +
> +enum {
> + RGR1_SW_INIT_1_INIT_MASK,
> + RGR1_SW_INIT_1_INIT_SHIFT,
> +};
> +
> +enum pcie_type {
> + GENERIC,
> + BCM7278,
> +};
> +
> +struct pcie_cfg_data {
> + const int *reg_field_info;
> + const int *offsets;
> + const enum pcie_type type;
> +};
> +
> +static const int pcie_reg_field_info[] = {
> + [RGR1_SW_INIT_1_INIT_MASK] = 0x2,
> + [RGR1_SW_INIT_1_INIT_SHIFT] = 0x1,
> +};
> +
> +static const int pcie_reg_field_info_bcm7278[] = {
> + [RGR1_SW_INIT_1_INIT_MASK] = 0x1,
> + [RGR1_SW_INIT_1_INIT_SHIFT] = 0x0,
> +};
> +
> +static const int pcie_offsets[] = {
> + [RGR1_SW_INIT_1] = 0x9210,
> + [EXT_CFG_INDEX] = 0x9000,
> + [EXT_CFG_DATA] = 0x9004,
> +};
> +
> +static const struct pcie_cfg_data generic_cfg = {
> + .reg_field_info = pcie_reg_field_info,
> + .offsets = pcie_offsets,
> + .type = GENERIC,
> +};
> +
> +static const int pcie_offset_bcm7278[] = {
> + [RGR1_SW_INIT_1] = 0xc010,
> + [EXT_CFG_INDEX] = 0x9000,
> + [EXT_CFG_DATA] = 0x9004,
> +};
> +
> +static const struct pcie_cfg_data bcm7278_cfg = {
> + .reg_field_info = pcie_reg_field_info_bcm7278,
> + .offsets = pcie_offset_bcm7278,
> + .type = BCM7278,
> +};

It's not essential, but if v2 is due I'd suggest factoring out the bcm2728
specific structures above, and moving them to patch #15. This will keep a
clearer division between the patch introducing the infrastructure and the one
adding the support for a new device.

> +
> struct brcm_msi {
> struct device *dev;
> void __iomem *base;
> @@ -176,6 +238,9 @@ struct brcm_pcie {
> int gen;
> u64 msi_target_addr;
> struct brcm_msi *msi;
> + const int *reg_offsets;
> + const int *reg_field_info;
> + enum pcie_type type;
> };
>
> /*
> @@ -602,20 +667,21 @@ static struct pci_ops brcm_pcie_ops = {
>
> static inline void brcm_pcie_bridge_sw_init_set(struct brcm_pcie *pcie, u32
> val)
> {
> - u32 tmp;
> + u32 tmp, mask = pcie->reg_field_info[RGR1_SW_INIT_1_INIT_MASK];
> + u32 shift = pcie->reg_field_info[RGR1_SW_INIT_1_INIT_SHIFT];

I don't think you need shift here, IIUC u32p_replace_bits() will take care of
all the masking and shifting internally, moreover, you'd be able to drop the
shift entry from reg_field_info.

> - tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1);
> - u32p_replace_bits(&tmp, val, PCIE_RGR1_SW_INIT_1_INIT_MASK);
> - writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1);
> + tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> + tmp = (tmp & ~mask) | ((val << shift) & mask);
> + writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> }

Regards,
Nicolas

Attachments:

signature.asc (499.00 B)
This is a digitally signed message part

2020-05-20 13:46:30

On 5/21/2020 2:48 PM, Jim Quinlan wrote:
> On Wed, May 20, 2020 at 3:27 AM Philipp Zabel <[email protected]> wrote:
>>
>> Hi Jim,
>>
>> On Tue, May 19, 2020 at 04:34:05PM -0400, Jim Quinlan wrote:
>>> From: Jim Quinlan <[email protected]>
>>>
>>> Some STB chips have a special purpose reset controller named
>>> RESCAL (reset calibration). This commit adds the control
>>> of RESCAL as well as the ability to start and stop its
>>> operation for PCIe HW.
>>>
>>> Signed-off-by: Jim Quinlan <[email protected]>
>>> ---
>>> drivers/pci/controller/pcie-brcmstb.c | 81 ++++++++++++++++++++++++++-
>>> 1 file changed, 80 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
>>> index 2c470104ba38..0787e8f6f7e5 100644
>>> --- a/drivers/pci/controller/pcie-brcmstb.c
>>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>> [...]
>>> @@ -1100,6 +1164,21 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>> dev_err(&pdev->dev, "could not enable clock\n");
>>> return ret;
>>> }
>>> + pcie->rescal = devm_reset_control_get_shared(&pdev->dev, "rescal");
>>> + if (IS_ERR(pcie->rescal)) {
>>> + if (PTR_ERR(pcie->rescal) == -EPROBE_DEFER)
>>> + return -EPROBE_DEFER;
>>> + pcie->rescal = NULL;
>>
>> This is effectively an optional reset control, so it is better to use:
>> ↵
>> pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev,
>> "rescal");↵
>> if (IS_ERR(pcie->rescal))
>> return PTR_ERR(pcie->rescal);
>>
>>> + } else {
>>> + ret = reset_control_deassert(pcie->rescal);
>>> + if (ret)
>>> + dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
>>> + }
>>
>> reset_control_* can handle rstc == NULL parameters for optional reset
>> controls, so this can be done unconditionally:
>>
>> ret = reset_control_deassert(pcie->rescal);↵
>> if (ret)↵
>> dev_err(&pdev->dev, "failed to deassert 'rescal'\n");↵
>>
>> Is rescal handled by the reset-brcmstb-rescal driver? Since that only
>> implements the .reset op, I would expect reset_control_reset() here.
> Where exactly? "reset.h" says that "Calling reset_control_rese()t is
> not allowed on a shared reset control." so I'm not sure why you would
> want me to invoke it.

Yes this is handled by drivers/reset/reset-brcmstb-rescal.c which only
implements a .reset() callback, what would be the appropriate API usage
here given that this is a shared reset between AHCI and PCIe, should
drivers/reset/reset-brcmstb-rescal.c not implement a .reset() callback
and .assert() callback instead?

>> Otherwise this looks like it'd be missing a reset_control_assert in
>> remove.
> I can add this.
> Thanks,
> Jim
>>
>> regards
>> Philipp

--
Florian