On Mon, Sep 12, 2022 at 03:02:11AM +0300, Serge Semin wrote:
[...]
> > > +/*
> > > + * Baikal-T1 MMIO space must be read/written by the dword-aligned
> > > + * instructions. Note the methods are optimized to have the dword operations
> > > + * performed with minimum overhead as the most frequently used ones.
> > > + */
> > > +static int bt1_pcie_read_mmio(void __iomem *addr, int size, u32 *val)
> > > +{
> > > + unsigned int ofs = (uintptr_t)addr & 0x3;
> > > +
> > > + if (!IS_ALIGNED((uintptr_t)addr, size))
> > > + return -EINVAL;
> > > +
> > > + *val = readl(addr - ofs) >> ofs * BITS_PER_BYTE;
> >
>
> > Is it always safe to read more than requested ?
>
> This question is kind of contradicting. No matter whether it's safe or
> not we just can't perform the IOs with size other than of the dword
> size. Doing otherwise will cause the bus access error.
It is not contradicting. You are reading more than the requested
size, which can have side effects.
I understand there is no other way around it - still it would be good
to understand whether that can compromise the driver functionality.
> > > + if (size == 4) {
> > > + return 0;
> > > + } else if (size == 2) {
> > > + *val &= 0xffff;
> > > + return 0;
> > > + } else if (size == 1) {
> > > + *val &= 0xff;
> > > + return 0;
> > > + }
> > > +
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static int bt1_pcie_write_mmio(void __iomem *addr, int size, u32 val)
> > > +{
> > > + unsigned int ofs = (uintptr_t)addr & 0x3;
> > > + u32 tmp, mask;
> > > +
> > > + if (!IS_ALIGNED((uintptr_t)addr, size))
> > > + return -EINVAL;
> > > +
> > > + if (size == 4) {
> > > + writel(val, addr);
> > > + return 0;
> > > + } else if (size == 2 || size == 1) {
> > > + mask = GENMASK(size * BITS_PER_BYTE - 1, 0);
> > > + tmp = readl(addr - ofs) & ~(mask << ofs * BITS_PER_BYTE);
> > > + tmp |= (val & mask) << ofs * BITS_PER_BYTE;
> > > + writel(tmp, addr - ofs);
> > > + return 0;
> > > + }
> >
>
> > Same question read/modify/write, is it always safe to do it
> > regardless of size ?
>
> ditto
See above.
> >
> > > +
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static u32 bt1_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base, u32 reg,
> > > + size_t size)
> > > +{
> > > + int ret;
> > > + u32 val;
> > > +
> > > + ret = bt1_pcie_read_mmio(base + reg, size, &val);
> > > + if (ret) {
> > > + dev_err(pci->dev, "Read DBI address failed\n");
> > > + return ~0U;
> >
>
> > Is this a special magic value the DWC core is expecting ?
> >
> > Does it clash with a _valid_ return value ?
>
> It's a normal return value if the PCIe IO wasn't successful.
I don't understand what you mean sorry. I understand you want to log
the error - what I don't get is why you change val to ~0U - why ~0U
and to what use, the function reading dbi can't use that value to
detect an error anyway, it would read whatever value is returned by
this function - regardless of the error condition.
> In this particular case there is no actual PCIe-bus IO though, but
> there are conditions which can cause the errors. So the error status
> is still sanity checked. This part was already commented by Rob here:
> https://lore.kernel.org/linux-pci/20220615171045.GD1413880-robh@xxxxxxxxxx/
> my response was:
> https://lore.kernel.org/linux-pci/20220619203904.h7q2eb7e4ctsujsk@mobilestation/
>
> >
> > > + }
> > > +
> > > + return val;
> > > +}
> > > +
> > > +static void bt1_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base, u32 reg,
> > > + size_t size, u32 val)
> > > +{
> > > + int ret;
> > > +
> > > + ret = bt1_pcie_write_mmio(base + reg, size, val);
> > > + if (ret)
> > > + dev_err(pci->dev, "Write DBI address failed\n");
> > > +}
> > > +
> > > +static void bt1_pcie_write_dbi2(struct dw_pcie *pci, void __iomem *base, u32 reg,
> > > + size_t size, u32 val)
> > > +{
> > > + struct bt1_pcie *btpci = to_bt1_pcie(pci);
> > > + int ret;
> > > +
> > > + regmap_update_bits(btpci->sys_regs, BT1_CCU_PCIE_GENC,
> > > + BT1_CCU_PCIE_DBI2_MODE, BT1_CCU_PCIE_DBI2_MODE);
> > > +
> > > + ret = bt1_pcie_write_mmio(base + reg, size, val);
> > > + if (ret)
> > > + dev_err(pci->dev, "Write DBI2 address failed\n");
> > > +
> > > + regmap_update_bits(btpci->sys_regs, BT1_CCU_PCIE_GENC,
> > > + BT1_CCU_PCIE_DBI2_MODE, 0);
> >
>
> > IIUC the regmap_update_bits() set up decoding for DBI2.
>
> Right and then switches it back off.
>
> > Hopefully the
> > DBI/DBI2 writes are sequentialized, this is a question valid also
> > for other DWC controllers.
>
> In general you are right, but not in particular case of the DW PCIe
> Root Ports. So the concurrent access to DBI and DBI2 won't cause any
> problem.
>
> >
> > What I want to say is, the regmap update in this function sets the
> > DWC HW in a way that can decode DBI2 (please correct me if I am wrong),
>
> Right.
>
> > between the two _update_bits() no DBI access should happen because
> > it just would not work.
>
> No. Because in case of the DW PCIe Root Ports, DBI and DBI2 are almost
> identical. The difference is only in two CSR fields which turn to be
> R/W in DBI2 instead of being RO in DBI. Other than that the DBI and
> DBI2 spaces are identical. That's why we don't need any software-based
> synchronization between the DBI/DBI2 accesses.
>
> Moreover we won't need to worry about the synchronisation at all if
> DBI2 is mapped via a separate reg-space (see dw_pcie.dbi_base2 field)
> because any concurrency is resolved behind the scene by means of the
> DBI bus HW implementation.
>
> >
> > It is a question.
>
> The situation gets to be more complex in case of DW PCIe End-points
> because some of the DBI CSRs change semantics in DBI2. At the very
> least it concerns the TYPE0_HDR.{BAR0-BAR5} registers, which determine
> the corresponding BARx size and whether it is enabled in DBI2 (see the
> reset_bar() and set_bar() methods implementation in
> drivers/pci/controller/dwc/pcie-designware-ep.c). But my controller is
> the Root Port controller, so the denoted peculiarity doesn't concern
> it.
>
> >
> > > +static int bt1_pcie_host_init(struct dw_pcie_rp *pp)
> > > +{
> > > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > + struct bt1_pcie *btpci = to_bt1_pcie(pci);
> > > + int ret;
> > > +
> > > + ret = bt1_pcie_get_resources(btpci);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + bt1_pcie_full_stop_bus(btpci, true);
> > > +
> > > + return bt1_pcie_cold_start_bus(btpci);
> > > +}
> > > +
> > > +static void bt1_pcie_host_deinit(struct dw_pcie_rp *pp)
> > > +{
> > > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > + struct bt1_pcie *btpci = to_bt1_pcie(pci);
> > > +
> > > + bt1_pcie_full_stop_bus(btpci, false);
> > > +}
> > > +
> > > +static const struct dw_pcie_host_ops bt1_pcie_host_ops = {
> > > + .host_init = bt1_pcie_host_init,
> > > + .host_deinit = bt1_pcie_host_deinit,
> > > +};
> > > +
> > > +static struct bt1_pcie *bt1_pcie_create_data(struct platform_device *pdev)
> > > +{
> > > + struct bt1_pcie *btpci;
> > > +
> > > + btpci = devm_kzalloc(&pdev->dev, sizeof(*btpci), GFP_KERNEL);
> > > + if (!btpci)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + btpci->pdev = pdev;
> > > +
> > > + platform_set_drvdata(pdev, btpci);
> > > +
> > > + return btpci;
> > > +}
> > > +
> > > +static int bt1_pcie_add_port(struct bt1_pcie *btpci)
> > > +{
> > > + struct device *dev = &btpci->pdev->dev;
> > > + int ret;
> > > +
> > > + /*
> > > + * DW PCIe Root Port controller is equipped with eDMA capable of
> > > + * working with the 64-bit memory addresses.
> > > + */
> > > + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> > > + if (ret)
> > > + return ret;
> >
>
> > Is this the right place to set the DMA mask for the host controller
> > embedded DMA controller (actually, the dev pointer is the _host_
> > controller device) ?
>
> Yes it's. The DMA controller is embedded into the PCIe Root Port
> controller. It CSRs are accessed via either the same CSR space or via
> a separate space but synchronously clocked by the same clock source
> (it's called unrolled iATU/eDMA mode). The memory range the
> controller is capable to reach is platform specific. So the glue
> driver is the best place to set the device DMA-mask. (For instance the
> DW PCIe master AXI-bus width is selected by means of the
> MASTER_BUS_ADDR_WIDTH parameter of the DW PCIe IP-core.)
I need to defer this question to Robin - I think the DMA mask for the
DMA controller device should be set in the respective device driver
(which isn't the host controller driver).
> > How this is going to play when combined with:
> >
> > https://lore.kernel.org/linux-pci/1e63a581-14ae-b4b5-a5bf-ca8f09c33af6@xxxxxxx
> >
> > It is getting a bit confusing. I believe the code in the link
> > above sets the mask so that through the DMA API we are capable
> > of getting an MSI doorbell virtual address whose physical address
> > can be addressed by the endpoint; this through the DMA API.
>
> I don't really understand why the code in the link above tries to
> analyze the MSI capability of the DW PCIe Root Port in the framework
> of the dw_pcie_msi_host_init() method. The method utilizes the iMSI-RX
> engine which is specific to the DW PCIe AXI-bus controller
> implementation. It has nothing to do with the PCIe MSI capability
> normally available over the standard PCIe config space.
>
> As Rob correctly noted here
> https://lore.kernel.org/all/CAL_JsqJh=d-B51b6yPBRq0tOwbChN=AFPr-a19U1QdQZAE7c1A@xxxxxxxxxxxxxx
> MSI TLPs never reaches the system memory. (But I would add that this
> only concerns the iMSI-RX engine.) So no matter which memory
> allocated and where, the only thing that matters is the PCIe-bus
> address specified to the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI CSRs,
> which are the DW PCIe-specific and both are always available thus
> supporting the 64-bit messages in any case. So if we had a way to just
> reserve a PCIe-bus address range which at the same time wouldn't have
> a system memory behind, we could have used the reserved range to
> initialize the iMSI-RX MSI-address without need to allocate any
> DMA-able memory at all. That's why the commit 07940c369a6b ("PCI: dwc:
> Fix MSI page leakage in suspend/resume") was absolutely correct.
Again - I would appreciate if Will/Robin can comment on this given
that it is down to DWC controller internals and their relation
with the DMA core layer.
Thanks,
Lorenzo
> > This patch is setting the DMA mask for a different reason, namely
> > setting the host controller embedded DMA controller addressing
> > capabilities.
>
> AFAIU what is done in that patch is incorrect.
>
> >
> > AFAICS - both approaches set the mask for the same device - now
> > the question is about which one is legitimate and how to handle
> > the other.
>
> That's simple. Mine is legitimate for sure. Another one isn't.
>
> >
> > > +
> > > + btpci->dw.version = DW_PCIE_VER_460A;
> > > + btpci->dw.dev = dev;
> > > + btpci->dw.ops = &bt1_pcie_ops;
> > > +
> > > + btpci->dw.pp.num_vectors = MAX_MSI_IRQS;
> > > + btpci->dw.pp.ops = &bt1_pcie_host_ops;
> > > +
> > > + dw_pcie_cap_set(&btpci->dw, REQ_RES);
> > > +
> > > + ret = dw_pcie_host_init(&btpci->dw.pp);
> > > + if (ret)
> > > + dev_err_probe(dev, ret, "Failed to initialize DWC PCIe host\n");
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static void bt1_pcie_del_port(struct bt1_pcie *btpci)
> > > +{
> > > + dw_pcie_host_deinit(&btpci->dw.pp);
> > > +}
> > > +
> > > +static int bt1_pcie_probe(struct platform_device *pdev)
> > > +{
> > > + struct bt1_pcie *btpci;
> > > +
> > > + btpci = bt1_pcie_create_data(pdev);
> >
>
> > Do we really need a function for that ? I am not too
> > bothered but I think it is overkill.
>
> I prefer splitting the probe method up into a set of small and
> coherent methods. It IMO improves the code readability for just no
> price since the compiler will embed the single-time used static
> methods anyway.
>
> -Sergey
>
> >
> > Thanks,
> > Lorenzo
> >
> > > + if (IS_ERR(btpci))
> > > + return PTR_ERR(btpci);
> > > +
> > > + return bt1_pcie_add_port(btpci);
> > > +}
> > > +
> > > +static int bt1_pcie_remove(struct platform_device *pdev)
> > > +{
> > > + struct bt1_pcie *btpci = platform_get_drvdata(pdev);
> > > +
> > > + bt1_pcie_del_port(btpci);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct of_device_id bt1_pcie_of_match[] = {
> > > + { .compatible = "baikal,bt1-pcie" },
> > > + {},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, bt1_pcie_of_match);
> > > +
> > > +static struct platform_driver bt1_pcie_driver = {
> > > + .probe = bt1_pcie_probe,
> > > + .remove = bt1_pcie_remove,
> > > + .driver = {
> > > + .name = "bt1-pcie",
> > > + .of_match_table = bt1_pcie_of_match,
> > > + },
> > > +};
> > > +module_platform_driver(bt1_pcie_driver);
> > > +
> > > +MODULE_AUTHOR("Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>");
> > > +MODULE_DESCRIPTION("Baikal-T1 PCIe driver");
> > > +MODULE_LICENSE("GPL");
> > > --
> > > 2.35.1
> > >
