On Thu, Mar 10, 2022 at 02:26:32PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Mar 10, 2022 at 11:41:12AM +0300, Serge Semin wrote:
> > On Thu, Mar 10, 2022 at 11:52:42AM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Mar 09, 2022 at 10:01:23PM +0300, Serge Semin wrote:
> > >
> > > [...]
> > >
> > > > > I'm afraid that this will not work for all cases (unless I miss something). As
> > > > > Zhi Li pointed out, there are places where only chip pointer will be passed and
> > > > > we'd need to extract the private data (dw_edma) from it.
> > > > >
> > > > > Tbh I also considered your idea but because of the above mentioned issue and
> > > > > also referring to other implementations like gpiochip, I settled with Frank's
> > > > > idea of copying the fields.
> > > >
> > > > What places are these? I see the only obstacle is the dw_edma_remove()
> > > > method. But it's easily fixable.
> > >
> > > Yeah, right. I overlooked that part.
> > >
> > > > Except that, everything else is more
> > > > or less straightforward (just a few methods need to have prototypes
> > > > converted to accepting dw_edma instead dw_edma_chip).
> > > >
> > > > In order to make the code design more coherent, we need to split up
> > > > private data and device/platform info. As I see it dw_edma_chip is
> > > > nothing but a chip info data. The eDMA driver is supposed to mainly
> > > > use and pass it's private data, not the platform info. It will greatly
> > > > improve the code readability and maintainability. Such approach will
> > > > also prevent a temptation of adding new private data fields into the
> > > > dw_edma_chip structure since reaching the pointer to dw_edma will be
> > > > much easier that getting the dw_edma_chip data. In this case
> > > > dw_edma_chip will be something like i2c_board_info in i2c.
> > > >
> > > > Ideally dw_edma_chip could be a temporarily defined device info, which
> > > > memory after the dw_edma_probe() method invocation could be freed. But
> > > > in order to implement that we'd need a bit more modifications
> > > > introduced.
> > > >
> > >
> >
> > > While at it, we should also consider adding an ops structure for passing the
> > > callbacks from controller drivers. Currently the eDMA driver has the callbacks
> > > defined in v0-core.c but it is used directly instead of as a callback.
> >
> > Are you saying about DBI/Native IOs? If so seems reasonable. Though in
> > my case it isn't required.) The only problem was a dword-aligned access,
> > which has been created in the DW eDMA driver by default.
> >
>
> It is not causing any problem but it doesn't look correct to me.
>
> Btw, do you have a patch for DWORD access? If so, please share. We are also
> facing the problem and like to see how to are handling it.
There are two types of accessors DW PCIe Host/EP driver defines:
CFG-space and DBI interface. Both of them can be implemented either
with default IO methods, or redefined by the particular platform.
In addition to that the CFG-space IOs are split up into two types:
host/EP own CFG-space and others (peripheral devices) CFG-spaces. In
my case the dword-aligned access is supposed to be performed only for
the DBI-interface and host CFG-space (which basically turns to be
the same registers space). The driver has a bug in this matter, which
I fixed with a patch attached to this email. After that patch is
applied the only thing you'll need is to redefine the
dw_pcie_ops.{read_dbi,write_dbi,write_dbi2} callbacks with the
platform specific ones, which would make sure IOs are dword aligned.
Like this:
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 PCIBIOS_BAD_REGISTER_NUMBER;
*val = readl(addr - ofs) >> ofs * BITS_PER_BYTE;
if (size == 4) {
return PCIBIOS_SUCCESSFUL;
} else if (size == 2) {
*val &= 0xffff;
return PCIBIOS_SUCCESSFUL;
} else if (size == 1) {
*val &= 0xff;
return PCIBIOS_SUCCESSFUL;
}
return PCIBIOS_BAD_REGISTER_NUMBER;
}
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 PCIBIOS_BAD_REGISTER_NUMBER;
if (size == 4) {
writel(val, addr);
return PCIBIOS_SUCCESSFUL;
} 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 PCIBIOS_SUCCESSFUL;
}
return PCIBIOS_BAD_REGISTER_NUMBER;
}
Regarding the peripheral devices CFG-space IOs. I didn't fix it there
since in my case there were no problems with iATU outbound windows.
DBI slave and iATU slave interfaces works differently. The latter in
my case permits non-dword-aligned IOs.
If you are experiencing problems with all the PCIe memory accesses
(not only DBI-space/host CFG-space, but the whole iATU outbound IOs),
then there won't be easy solution for that other than somehow fixing the
read{b,w,l(})/write{b,w,l}() platform-specific internals. In a way so they
would differentiate PCIe and normal MMIO IOs and make sure all of them
are properly aligned. Alternatively you can fix each particular
peripheral device driver. It isn't that good option though.
Regarding DW eDMA driver. It doesn't have the problem in this matter,
since it is designed to call readl/writel methods only for the
controller setups. They are dword-aligned.
Regarding attached patch and my work. I'll send it out after Frank
finishes his eDMA patchset review, since other than the attached patch
I've got some more useful fixes, which need to be rebased on top of the
Frank' work.
-Sergey
>
> Thanks,
> Mani
>
> > -Sergey
> >
> > >
> > > This should anyway needs to be fixed when another version of the IP get's added.
> > >
> > > Thanks,
> > > Mani
