lkml.org 
[lkml]   [2021]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectPossible issue at the kirin-pcie driver
Hi,

I was asked by Rob Herring to convert the kiring-pcie driver on two parts,
splitting the PHY logic from it, in order to be able to add PHY support
for Hikey 970 at drivers/pci/controller/dwc/pcie-kirin.c.

While doing so, I noticed something weird issue at the driver, with regards
to a certain register (PCIE_APB_PHY_STATUS0), as shown below:

...

#define PCIE_APB_PHY_STATUS0 0x400
...
static inline u32 kirin_apb_ctrl_readl(struct kirin_pcie *kirin_pcie, u32 reg)
{
return readl(kirin_pcie->apb_base + reg);
}
...
static inline u32 kirin_apb_phy_readl(struct kirin_pcie *kirin_pcie, u32 reg)
{
return readl(kirin_pcie->phy_base + reg);
}
...
static int kirin_pcie_phy_init(struct kirin_pcie *kirin_pcie)
{
...
reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);
if (reg_val & PIPE_CLK_STABLE) {
dev_err(dev, "PIPE clk is not stable\n");
return -EINVAL;
}
}
...
static int kirin_pcie_link_up(struct dw_pcie *pci)
{
struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
u32 val = kirin_apb_ctrl_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);

if ((val & PCIE_LINKUP_ENABLE) == PCIE_LINKUP_ENABLE)
return 1;

return 0;

u32 val = kirin_apb_ctrl_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);

if ((val & PCIE_LINKUP_ENABLE) == PCIE_LINKUP_ENABLE)
return 1;

Basically, the code at kirin_pcie_phy_init() use this register as if it is
part of the PHY memory region (0xf3f20000 + 0x400), while the code at
kirin_pcie_link_up() considers is as belonging to the APB memory
region (0xff3fe000 + 0x400).

It sounds to me that there's a mistake somewhere. I mean, either:

1. there is a cut-and-paste error, caused it to access the wrong memory
region, e.g. at kirin_pcie_link_up() the logic should be:

u32 val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);

instead of:

u32 val = kirin_apb_ctrl_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);

(or the reverse)

2. Both memory regions have a register at address 0x400 with similar
names that ended being merged into the same macro;

3. the register for APB PHY status0 is duplicated on both regions and,
on both, they are at region_base + 0x400.

I suspect that it is (1), but, as I don't have any datasheets or
register map, I can't tell for sure.

Could someone with access to the datahseets shed the light?

Thanks,
Mauro

\
 
 \ /
  Last update: 2021-07-06 11:36    [W:0.030 / U:0.548 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site