Messages in this thread | | | Date | Wed, 29 Mar 2023 12:43:52 +0100 | From | Mark Brown <> | Subject | Re: [PATCH 1/2] spi: octeontx2: Add support for octeontx2 spi controller |
| |
On Mon, Mar 27, 2023 at 11:07:52AM -0700, Piyush Malgujar wrote:
> +static int tbi_clk_en = 1; > +module_param(tbi_clk_en, uint, 0644); > +MODULE_PARM_DESC(tbi_clk_en, > + "Use Fixed Time Base 100MHz Reference Clock (0=Disable, 1=Enable [default])"); > + > +static int cfg_mode_delay = 30; > +module_param(cfg_mode_delay, uint, 0644); > +MODULE_PARM_DESC(cfg_mode_delay, > + "Delay in micro-seconds for mode change in MPI CFG register (30 [default])");
These don't look like they should be configurable at runtime, either DT or (especially in the second case) just have the driver do the right thing.
> +static void octeontx2_spi_wait_ready(struct octeontx2_spi *p) > +{ > + union mpix_sts mpi_sts; > + unsigned int loops = 0; > + > + mpi_sts.u64 = 0; > + do { > + if (loops++) > + __delay(500); > + mpi_sts.u64 = readq(p->register_base + OCTEONTX2_SPI_STS(p)); > + } while (mpi_sts.s.busy);
Might want a cpu_relax() in there.
> + p->cs_enax |= (0xFull << 12); > + mpi_cfg.u64 |= p->cs_enax; > + > + if (mpi_cfg.u64 != p->last_cfg) { > + p->last_cfg = mpi_cfg.u64; > + writeq(mpi_cfg.u64, p->register_base + OCTEONTX2_SPI_CFG(p)); > + mpi_cfg.u64 = readq(p->register_base + OCTEONTX2_SPI_CFG(p)); > + udelay(cfg_mode_delay); /* allow CS change to settle */ > + }
It looks like the hardware has independent control of chip select so should just implement a set_cs() operation. That seems to be the only thing stopping the driver using transfer_one() instead of transfer_one_message() so it should do that too and save more code.
> + p->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(p->clk)) { > + p->clk = devm_clk_get(dev, "sclk"); > + p->sys_freq = 0; > + } else { > + ret = clk_prepare_enable(p->clk); > + if (!ret) > + p->sys_freq = clk_get_rate(p->clk); > + }
We don't need to enable sclk or care about errors? That seems buggy. The driver should just rely on the clock API here.
> + if (!p->sys_freq) > + p->sys_freq = SYS_FREQ_DEFAULT; > + if (tbi_clk_en) > + p->sys_freq = TBI_FREQ;
We never seem to do anything to actually configure this in the hardware. [unhandled content-type:application/pgp-signature] | |