RE: [PATCH 02/12] drm/ast: Handle configuration without P2A bridge
Tested-by: Y.C. Chen -Original Message- From: Benjamin Herrenschmidt [mailto:b...@kernel.crashing.org] Sent: Friday, February 24, 2017 6:54 AM To: dri-de...@lists.freedesktop.org Cc: YC Chen ; airl...@redhat.com; e...@suse.come; linuxppc-...@ozlabs.org Subject: [PATCH 02/12] drm/ast: Handle configuration without P2A bridge From: Russell Currey The ast driver configures a window to enable access into BMC memory space in order to read some configuration registers. If this window is disabled, which it can be from the BMC side, the ast driver can't function. Closing this window is a necessity for security if a machine's host side and BMC side are controlled by different parties; i.e. a cloud provider offering machines "bare metal". A recent patch went in to try to check if that window is open but it does so by trying to access the registers in question and testing if the result is 0x. This method will trigger a PCIe error when the window is closed which on some systems will be fatal (it will trigger an EEH for example on POWER which will take out the device). This patch improves this in two ways: - First, if the firmware has put properties in the device-tree containing the relevant configuration information, we use these. - Otherwise, a bit in one of the SCU scratch registers (which are readable via the VGA register space and writeable by the BMC) will indicate if the BMC has closed the window. This bit has been defined by Y.C Chen from Aspeed. If the window is closed and the configuration isn't available from the device-tree, some sane defaults are used. Those defaults are hopefully sufficient for standard video modes used on a server. Signed-off-by: Russell Currey Signed-off-by: Benjamin Herrenschmidt
Re: [PATCH 02/12] drm/ast: Handle configuration without P2A bridge
On Fri, 2017-02-24 at 12:51 +1030, Joel Stanley wrote:
>
> Are these properties supposed to repeat the prefix "ast,ast"?
>
> We've chosen aspeed as the vendor prefix for Aspeed stuff.
Sent my reply too early ... so yes, I can change that, our FW hasn't
merge the FW side yet. I'll respin now.
> > + if (mcr_scu_strap & 0x2000)
>
> This bit confused me. Bit 13 of the strap (SCU70) is the SPI mode.
The register is actually "MCR170: AST2000 Backward Compatible SCU
Hardware Strapping Value"
> > + ref_pll = 14318;
> > + else
> > + ref_pll = 12000;
> > +
> > + denum = mcr_scu_mpll & 0x1f;
> > + num = (mcr_scu_mpll & 0x3fe0) >> 5;
> > + dsel = (mcr_scu_mpll & 0xc000) >> 14;
>
> These calculations don't make sense for the ast2400 or ast2500.
They do if you look at this:
MCR120: AST2000 Backward Compatible SCU MPLL Parameter
It's not the SCU version of the register it's the MCU "copy" of it
that maintains some kind of legacy layout. Hence "mcr_scu" prefix
not "scu".
> > + switch (dsel) {
> > + case 3:
> > + div = 0x4;
> > + break;
> > + case 2:
> > + case 1:
> > + div = 0x2;
> > + break;
> > + default:
> > + div = 0x1;
> > + break;
> > }
> > + ast->mclk = ref_pll * (num + 2) / (denum + 2) * (div *
> > 1000);
> > return 0;
> > }
Re: [PATCH 02/12] drm/ast: Handle configuration without P2A bridge
On Fri, 2017-02-24 at 12:51 +1030, Joel Stanley wrote: > Are these properties supposed to repeat the prefix "ast,ast"? > > We've chosen aspeed as the vendor prefix for Aspeed stuff. Argh no, that's a typo... must have worked in my tests bcs the defaults are fine. I'll update. Cheers, Ben.
Re: [PATCH 02/12] drm/ast: Handle configuration without P2A bridge
On Fri, Feb 24, 2017 at 9:23 AM, Benjamin Herrenschmidt
wrote:
> static int ast_get_dram_info(struct drm_device *dev)
> {
> + struct device_node *np = dev->pdev->dev.of_node;
> struct ast_private *ast = dev->dev_private;
> - uint32_t data, data2;
> - uint32_t denum, num, div, ref_pll;
> + uint32_t mcr_cfg, mcr_scu_mpll, mcr_scu_strap;
> + uint32_t denum, num, div, ref_pll, dsel;
>
> - if (ast->DisableP2A)
> - {
> + switch (ast->config_mode) {
> + case ast_use_dt:
> + /*
> +* If some properties are missing, use reasonable
> +* defaults for AST2400
> +*/
> + if (of_property_read_u32(np, "ast,mcr-configuration",
> &mcr_cfg))
> + mcr_cfg = 0x0577;
> + if (of_property_read_u32(np, "ast,ast,mcr-scu-mpll",
> +&mcr_scu_mpll))
> + mcr_scu_mpll = 0x50C0;
> + if (of_property_read_u32(np, "ast,ast,mcr-scu-strap",
Are these properties supposed to repeat the prefix "ast,ast"?
We've chosen aspeed as the vendor prefix for Aspeed stuff.
> +&mcr_scu_strap))
> + mcr_scu_strap = 0;
> + break;
> + case ast_use_p2a:
> + ast_write32(ast, 0xf004, 0x1e6e);
> + ast_write32(ast, 0xf000, 0x1);
> + mcr_cfg = ast_read32(ast, 0x10004);
> + mcr_scu_mpll = ast_read32(ast, 0x10120);
> + mcr_scu_strap = ast_read32(ast, 0x10170);
> + break;
> + case ast_use_defaults:
> + default:
> ast->dram_bus_width = 16;
> ast->dram_type = AST_DRAM_1Gx16;
> ast->mclk = 396;
> + return 0;
> }
> - else
> - {
> - ast_write32(ast, 0xf004, 0x1e6e);
> - ast_write32(ast, 0xf000, 0x1);
> - data = ast_read32(ast, 0x10004);
>
> - if (data & 0x40)
> - ast->dram_bus_width = 16;
> - else
> - ast->dram_bus_width = 32;
> -
> - if (ast->chip == AST2300 || ast->chip == AST2400) {
> - switch (data & 0x03) {
> - case 0:
> - ast->dram_type = AST_DRAM_512Mx16;
> - break;
> - default:
> - case 1:
> - ast->dram_type = AST_DRAM_1Gx16;
> - break;
> - case 2:
> - ast->dram_type = AST_DRAM_2Gx16;
> - break;
> - case 3:
> - ast->dram_type = AST_DRAM_4Gx16;
> - break;
> - }
> - } else {
> - switch (data & 0x0c) {
> - case 0:
> - case 4:
> - ast->dram_type = AST_DRAM_512Mx16;
> - break;
> - case 8:
> - if (data & 0x40)
> - ast->dram_type = AST_DRAM_1Gx16;
> - else
> - ast->dram_type = AST_DRAM_512Mx32;
> - break;
> - case 0xc:
> - ast->dram_type = AST_DRAM_1Gx32;
> - break;
> - }
> - }
> + if (mcr_cfg & 0x40)
> + ast->dram_bus_width = 16;
> + else
> + ast->dram_bus_width = 32;
>
> - data = ast_read32(ast, 0x10120);
> - data2 = ast_read32(ast, 0x10170);
> - if (data2 & 0x2000)
> - ref_pll = 14318;
> - else
> - ref_pll = 12000;
> -
> - denum = data & 0x1f;
> - num = (data & 0x3fe0) >> 5;
> - data = (data & 0xc000) >> 14;
> - switch (data) {
> - case 3:
> - div = 0x4;
> + if (ast->chip == AST2300 || ast->chip == AST2400) {
> + switch (mcr_cfg & 0x03) {
> + case 0:
> + ast->dram_type = AST_DRAM_512Mx16;
> break;
> - case 2:
> + default:
> case 1:
> - div = 0x2;
> + ast->dram_type = AST_DRAM_1Gx16;
> break;
> - default:
> - div = 0x1;
> + case 2:
> + ast->dram_type = AST_DRAM_2Gx16;
> + break;
> +
