Re: [PATCH v4 02/11] soc: imx: add i.MX8MP HSIO blk-ctrl

[Lucas Stach <l.stach@pengutronix.de>]

Am Montag, dem 11.04.2022 um 00:11 +0200 schrieb Marek Vasut:
> On 4/7/22 11:12, Lucas Stach wrote:
> > Am Donnerstag, dem 07.04.2022 um 01:43 +0200 schrieb Marek Vasut:
> > > On 4/6/22 17:33, Lucas Stach wrote:
> > > 
> > > [...]
> > > 
> > > > +static const struct imx8mp_blk_ctrl_data imx8mp_hsio_blk_ctl_dev_data = {
> > > > +	.max_reg = 0x24,
> > > 
> > > Doesn't the HSIO_BLK_CTL go up to 0x10c ?
> > 
> > Technically yes, but there is already a driver for the USB glue
> > (fsl,imx8mp-dwc3) that occupies the USB registers at and above 0x100.
> 
> Shouldn't that imx8mp-dwc3 glue code be switched to syscon and access 
> the registers through this driver then ?

I don't see why this would be needed. Registers up to 0x24 are a mixed
bag of blk-ctrls, but the registers above 0x100 are only USB wakeup
related, I don't see the blk-ctrl ever wanting to touch them.

I think the current split is fine. Just because the RM lumps those
register regions together into the HSIO blk-ctrl description doesn't
mean we need to cover them all in a single syscon.

Regards,
Lucas

> 
> > > > +	.power_on = imx8mp_hsio_blk_ctrl_power_on,
> > > > +	.power_off = imx8mp_hsio_blk_ctrl_power_off,
> > > > +	.power_notifier_fn = imx8mp_hsio_power_notifier,
> > > > +	.domains = imx8mp_hsio_domain_data,
> > > > +	.num_domains = ARRAY_SIZE(imx8mp_hsio_domain_data),
> > > > +};
> > > > +
> > > > +static int imx8mp_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > > > +{
> > > > +	struct imx8mp_blk_ctrl_domain *domain = to_imx8mp_blk_ctrl_domain(genpd);
> > > > +	const struct imx8mp_blk_ctrl_domain_data *data = domain->data;
> > > > +	struct imx8mp_blk_ctrl *bc = domain->bc;
> > > > +	int ret;
> > > > +
> > > > +	/* make sure bus domain is awake */
> > > > +	ret = pm_runtime_resume_and_get(bc->bus_power_dev);
> > > > +	if (ret < 0) {
> > > > +		dev_err(bc->dev, "failed to power up bus domain\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	/* enable upstream clocks */
> > > > +	ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);;
> > > > +	if (ret) {
> > > > +		dev_err(bc->dev, "failed to enable clocks\n");
> > > > +		goto bus_put;
> > > > +	}
> > > > +
> > > > +	/* domain specific blk-ctrl manipulation */
> > > > +	bc->power_on(bc, domain);
> > > 
> > > Would it make sense to add error checking ?
> > > 
> > I don't expect those functions to do any more than a bit of blk-ctrl
> > MMIO register poking. If that fails you are in much more trouble than
> > what you can reasonably fix with some error checking.
> 
> All right.