RE: [PATCH 2/2] PCI: rcar: Fix missing MACCTLR register setting in initialize sequence

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Eugeniu-san,

> From: Eugeniu Rosca, Sent: Thursday, October 31, 2019 1:35 AM
<snip>
> > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> > index 40d8c54..d470ab8 100644
> > --- a/drivers/pci/controller/pcie-rcar.c
> > +++ b/drivers/pci/controller/pcie-rcar.c
> > @@ -91,6 +91,7 @@
> >  #define  LINK_SPEED_2_5GTS	(1 << 16)
> >  #define  LINK_SPEED_5_0GTS	(2 << 16)
> >  #define MACCTLR			0x011058
> > +#define  MACCTLR_INIT_VAL	0x80ff0000
> 
> Actually, I do believe there is an inconsistency in the manual,
> since the following statements pretty much contradict one another:
> 
> 1. (as quoted by Shimoda-san from "Initial Setting of PCI Express")
>    > Be sure to write the initial value (= H'80FF 0000) to MACCTLR
>    > before enabling PCIETCTLR.CFINIT.
> 2. Description of SPCHG bit in "54.2.98 MAC Control Register (MACCTLR)"
>    > Only writing 1 is valid and writing 0 is invalid.
> 
> The last "invalid" sounds like "bad things can happen" aka "expect
> undefined behaviors" when SPCHG is written "0".

I asked the hardware team about the "invalid" in the SPCHG bit and then
this "invalid" means "ignored", not "prohibited". So, even if we write
the SPCHG to 0, no bad things/undefined behaviors happen.

> While leaving the decision on the patch inclusion to the maintainers, I
> hope, in the long run, Renesas can resolve the documentation conflict
> with the HW team and the tech writers.

So, I don't think the documentation conflict exists about the MACCTLR register.
But, what do you think?

Best regards,
Yoshihiro Shimoda

> >  #define  SPEED_CHANGE		BIT(24)
> >  #define  SCRAMBLE_DISABLE	BIT(27)
> >  #define PMSR			0x01105c
> > @@ -613,6 +614,8 @@ static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
> >  	if (IS_ENABLED(CONFIG_PCI_MSI))
> >  		rcar_pci_write_reg(pcie, 0x801f0000, PCIEMSITXR);
> >
> > +	rcar_pci_write_reg(pcie, MACCTLR_INIT_VAL, MACCTLR);
> > +
> >  	/* Finish initialization - establish a PCI Express link */
> >  	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
> >
> > @@ -1235,6 +1238,7 @@ static int rcar_pcie_resume_noirq(struct device *dev)
> >  		return 0;
> >
> >  	/* Re-establish the PCIe link */
> > +	rcar_pci_write_reg(pcie, MACCTLR_INIT_VAL, MACCTLR);
> >  	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
> >  	return rcar_pcie_wait_for_dl(pcie);
> >  }
> 
> --
> Best Regards,
> Eugeniu




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux